[PATCH v2 0/5] MR551: win32u: Complete support for display modes in nulldrv.

[Rémi Bernon]

With this series it's now possible to run and pass `user32:monitor` and `user32:sysparams` tests with nulldrv, and so most `user32` tests (except for a few desktop cursor position tests). This still requires some prefix configuration to enable the nulldrv driver, or a change like https://gitlab.winehq.org/rbernon/wine/-/commit/753368ad0ec52f03f8d6e78ca794... to enable it when `DISPLAY` environment variable is unset.

This then shows that some of the user32 tests are failing with winex11 but passing with nulldrv, as in https://gitlab.winehq.org/rbernon/wine/-/commit/6d5f4109a514a0dc266899fcacfa....

--
  v2: win32u: Add a default EnumDisplaySettingsEx driver implementation.
      win32u: Add a default ChangeDisplaySettingsEx driver implementation.
      win32u: Move enumeration of available modes out of graphics drivers.
      win32u: Validate NtUserChangeDisplaySettings mode against available modes.
      win32u: Ignore DM_POSITION mode fields for available modes.
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/551

[PATCH v2 1/5] win32u: Ignore DM_POSITION mode fields for available modes.

[Rémi Bernon]

From: R��mi Bernon <rbernon(a)codeweavers.com>

---
 dlls/win32u/sysparams.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/dlls/win32u/sysparams.c b/dlls/win32u/sysparams.c
index 3c1051b2d80..78bf77400b1 100644
--- a/dlls/win32u/sysparams.c
+++ b/dlls/win32u/sysparams.c
@@ -448,8 +448,11 @@ static BOOL write_adapter_mode( HKEY adapter_key, DWORD index, const DEVMODEW *m
     set_mode_field( flagsW, dmDisplayFlags, DM_DISPLAYFLAGS );
     set_mode_field( orientationW, dmDisplayOrientation, DM_DISPLAYORIENTATION );
     set_mode_field( fixed_outputW, dmDisplayFixedOutput, DM_DISPLAYFIXEDOUTPUT );
-    set_mode_field( x_panningW, dmPosition.x, DM_POSITION );
-    set_mode_field( y_panningW, dmPosition.y, DM_POSITION );
+    if (index == ENUM_CURRENT_SETTINGS || index == ENUM_REGISTRY_SETTINGS)
+    {
+        set_mode_field( x_panningW, dmPosition.x, DM_POSITION );
+        set_mode_field( y_panningW, dmPosition.y, DM_POSITION );
+    }
     ret = set_reg_value( hkey, driver_extraW, REG_BINARY, mode + 1, mode->dmDriverExtra );
 
 #undef set_mode_field
@@ -487,8 +490,11 @@ static BOOL read_adapter_mode( HKEY adapter_key, DWORD index, DEVMODEW *mode )
     query_mode_field( y_resolutionW, dmPelsHeight, DM_PELSHEIGHT );
     query_mode_field( v_refreshW, dmDisplayFrequency, DM_DISPLAYFREQUENCY );
     query_mode_field( flagsW, dmDisplayFlags, DM_DISPLAYFLAGS );
-    query_mode_field( x_panningW, dmPosition.x, DM_POSITION );
-    query_mode_field( y_panningW, dmPosition.y, DM_POSITION );
+    if (index == ENUM_CURRENT_SETTINGS || index == ENUM_REGISTRY_SETTINGS)
+    {
+        query_mode_field( x_panningW, dmPosition.x, DM_POSITION );
+        query_mode_field( y_panningW, dmPosition.y, DM_POSITION );
+    }
     query_mode_field( orientationW, dmDisplayOrientation, DM_DISPLAYORIENTATION );
     query_mode_field( fixed_outputW, dmDisplayFixedOutput, 0 );
 
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/551

[PATCH v2 2/5] win32u: Validate NtUserChangeDisplaySettings mode against available modes.

[Rémi Bernon]

From: R��mi Bernon <rbernon(a)codeweavers.com>

---
 dlls/win32u/sysparams.c    | 39 +++++++++++++++++++++++++++++++++-----
 dlls/winemac.drv/display.c |  3 +--
 dlls/winex11.drv/display.c | 29 +---------------------------
 3 files changed, 36 insertions(+), 35 deletions(-)

diff --git a/dlls/win32u/sysparams.c b/dlls/win32u/sysparams.c
index 78bf77400b1..846119872b5 100644
--- a/dlls/win32u/sysparams.c
+++ b/dlls/win32u/sysparams.c
@@ -2037,8 +2037,11 @@ static BOOL is_detached_mode( const DEVMODEW *mode )
            mode->dmPelsHeight == 0;
 }
 
-static DEVMODEW *validate_display_settings( DEVMODEW *default_mode, DEVMODEW *current_mode, DEVMODEW *devmode )
+static DEVMODEW *validate_display_settings( DEVMODEW *modes, DEVMODEW *default_mode,
+                                            DEVMODEW *current_mode, DEVMODEW *devmode )
 {
+    DEVMODEW *mode;
+
     if (devmode)
     {
         trace_devmode( devmode );
@@ -2072,7 +2075,28 @@ static DEVMODEW *validate_display_settings( DEVMODEW *default_mode, DEVMODEW *cu
         if (!devmode->dmPelsHeight) devmode->dmPelsHeight = current_mode->dmPelsHeight;
     }
 
-    return devmode;
+    if (is_detached_mode( devmode )) return devmode;
+
+    for (mode = modes; mode && mode->dmSize; mode = NEXT_DEVMODEW(mode))
+    {
+        if ((devmode->dmFields & DM_BITSPERPEL) && devmode->dmBitsPerPel && devmode->dmBitsPerPel != mode->dmBitsPerPel)
+            continue;
+        if ((devmode->dmFields & DM_PELSWIDTH) && devmode->dmPelsWidth != mode->dmPelsWidth)
+            continue;
+        if ((devmode->dmFields & DM_PELSHEIGHT) && devmode->dmPelsHeight != mode->dmPelsHeight)
+            continue;
+        if ((devmode->dmFields & DM_DISPLAYFREQUENCY) && devmode->dmDisplayFrequency && mode->dmDisplayFrequency &&
+            devmode->dmDisplayFrequency != 1 && devmode->dmDisplayFrequency != mode->dmDisplayFrequency)
+            continue;
+        if ((devmode->dmFields & DM_DISPLAYORIENTATION) && devmode->dmDisplayOrientation != mode->dmDisplayOrientation)
+            continue;
+
+        mode->dmFields |= DM_POSITION;
+        mode->dmPosition = devmode->dmPosition;
+        return mode;
+    }
+
+    return NULL;
 }
 
 /***********************************************************************
@@ -2081,10 +2105,11 @@ static DEVMODEW *validate_display_settings( DEVMODEW *default_mode, DEVMODEW *cu
 LONG WINAPI NtUserChangeDisplaySettings( UNICODE_STRING *devname, DEVMODEW *devmode, HWND hwnd,
                                          DWORD flags, void *lparam )
 {
-    DEVMODEW default_mode = {.dmSize = sizeof(DEVMODEW)}, current_mode = {.dmSize = sizeof(DEVMODEW)};
+    DEVMODEW *modes, default_mode = {.dmSize = sizeof(DEVMODEW)}, current_mode = {.dmSize = sizeof(DEVMODEW)};
     WCHAR device_name[CCHDEVICENAME], adapter_path[MAX_PATH];
     LONG ret = DISP_CHANGE_SUCCESSFUL;
     struct adapter *adapter;
+    SIZE_T size;
 
     TRACE( "%s %p %p %#x %p\n", debugstr_us(devname), devmode, hwnd, flags, lparam );
     TRACE( "flags=%s\n", _CDS_flags(flags) );
@@ -2102,6 +2127,10 @@ LONG WINAPI NtUserChangeDisplaySettings( UNICODE_STRING *devname, DEVMODEW *devm
     {
         lstrcpyW( device_name, adapter->dev.device_name );
         lstrcpyW( adapter_path, adapter->config_key );
+        size = sizeof(DEVMODEW) + adapter->modes[0].dmDriverExtra;
+        /* allocate an extra mode to make iteration easier */
+        modes = calloc( adapter->mode_count + 1, size );
+        if (modes) memcpy( modes, adapter->modes, adapter->mode_count * size );
     }
     unlock_display_devices();
     if (!adapter)
@@ -2113,11 +2142,11 @@ LONG WINAPI NtUserChangeDisplaySettings( UNICODE_STRING *devname, DEVMODEW *devm
     if (!read_registry_settings( adapter_path, &default_mode )) default_mode.dmSize = 0;
     if (!NtUserEnumDisplaySettings( devname, ENUM_CURRENT_SETTINGS, ¤t_mode, 0 )) current_mode.dmSize = 0;
 
-    if (!(devmode = validate_display_settings( &default_mode, ¤t_mode, devmode ))) ret = DISP_CHANGE_BADMODE;
-    else if (user_driver->pChangeDisplaySettingsEx( device_name, devmode, hwnd, flags | CDS_TEST, lparam )) ret = DISP_CHANGE_BADMODE;
+    if (!(devmode = validate_display_settings( modes, &default_mode, ¤t_mode, devmode ))) ret = DISP_CHANGE_BADMODE;
     else if ((flags & CDS_UPDATEREGISTRY) && !write_registry_settings( adapter_path, devmode )) ret = DISP_CHANGE_NOTUPDATED;
     else if (flags & (CDS_TEST | CDS_NORESET)) ret = DISP_CHANGE_SUCCESSFUL;
     else ret = user_driver->pChangeDisplaySettingsEx( device_name, devmode, hwnd, flags, lparam );
+    free( modes );
 
     if (ret) ERR( "Changing %s display settings returned %d.\n", debugstr_us(devname), ret );
     return ret;
diff --git a/dlls/winemac.drv/display.c b/dlls/winemac.drv/display.c
index 1350ffcede9..cfff9ee0e74 100644
--- a/dlls/winemac.drv/display.c
+++ b/dlls/winemac.drv/display.c
@@ -874,8 +874,7 @@ better:
         /* we have a valid mode */
         TRACE("Requested display settings match mode %ld\n", best);
 
-        if (flags & (CDS_TEST | CDS_NORESET)) ret = DISP_CHANGE_SUCCESSFUL;
-        else if (wcsicmp(primary_adapter, devname))
+        if (wcsicmp(primary_adapter, devname))
         {
             FIXME("Changing non-primary adapter settings is currently unsupported.\n");
             ret = DISP_CHANGE_SUCCESSFUL;
diff --git a/dlls/winex11.drv/display.c b/dlls/winex11.drv/display.c
index 174411c7915..d296c0ab932 100644
--- a/dlls/winex11.drv/display.c
+++ b/dlls/winex11.drv/display.c
@@ -779,40 +779,13 @@ LONG X11DRV_ChangeDisplaySettingsEx( LPCWSTR devname, LPDEVMODEW devmode,
                                      HWND hwnd, DWORD flags, LPVOID lpvoid )
 {
     struct x11drv_display_setting *displays;
-    INT display_idx, display_count;
-    DEVMODEW *full_mode;
+    INT display_count;
     LONG ret;
 
     ret = get_display_settings(&displays, &display_count, devname, devmode);
     if (ret != DISP_CHANGE_SUCCESSFUL)
         return ret;
 
-    if (flags & CDS_UPDATEREGISTRY && devname && devmode)
-    {
-        for (display_idx = 0; display_idx < display_count; ++display_idx)
-        {
-            if (!wcsicmp(displays[display_idx].desired_mode.dmDeviceName, devname))
-            {
-                full_mode = get_full_mode(displays[display_idx].id, &displays[display_idx].desired_mode);
-                if (!full_mode)
-                {
-                    free(displays);
-                    return DISP_CHANGE_BADMODE;
-                }
-
-                memcpy( &devmode->dmFields, &full_mode->dmFields, devmode->dmSize - offsetof(DEVMODEW, dmFields) );
-                free_full_mode(full_mode);
-                break;
-            }
-        }
-    }
-
-    if (flags & (CDS_TEST | CDS_NORESET))
-    {
-        free(displays);
-        return DISP_CHANGE_SUCCESSFUL;
-    }
-
     if (all_detached_settings(displays, display_count))
     {
         WARN("Detaching all displays is not permitted.\n");
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/551

[PATCH v2 3/5] win32u: Move enumeration of available modes out of graphics drivers.

[Rémi Bernon]

From: R��mi Bernon <rbernon(a)codeweavers.com>

---
 dlls/win32u/sysparams.c     |  26 +++++++-
 dlls/wineandroid.drv/init.c |  25 +++-----
 dlls/winemac.drv/display.c  | 114 +-----------------------------------
 dlls/winex11.drv/display.c  |  52 ++--------------
 4 files changed, 41 insertions(+), 176 deletions(-)

diff --git a/dlls/win32u/sysparams.c b/dlls/win32u/sysparams.c
index 846119872b5..80f18244049 100644
--- a/dlls/win32u/sysparams.c
+++ b/dlls/win32u/sysparams.c
@@ -2160,6 +2160,8 @@ BOOL WINAPI NtUserEnumDisplaySettings( UNICODE_STRING *device, DWORD index, DEVM
     static const WCHAR wine_display_driverW[] = {'W','i','n','e',' ','D','i','s','p','l','a','y',' ','D','r','i','v','e','r',0};
     WCHAR device_name[CCHDEVICENAME], adapter_path[MAX_PATH];
     struct adapter *adapter;
+    DEVMODEW *mode, *modes;
+    SIZE_T size;
     BOOL ret;
 
     TRACE( "device %s, index %#x, devmode %p, flags %#x\n", debugstr_us(device), index, devmode, flags );
@@ -2169,6 +2171,10 @@ BOOL WINAPI NtUserEnumDisplaySettings( UNICODE_STRING *device, DWORD index, DEVM
     {
         lstrcpyW( device_name, adapter->dev.device_name );
         lstrcpyW( adapter_path, adapter->config_key );
+        size = sizeof(DEVMODEW) + adapter->modes[0].dmDriverExtra;
+        /* allocate an extra mode for easier iteration */
+        modes = calloc( adapter->mode_count + 1, size );
+        if (modes) memcpy( modes, adapter->modes, adapter->mode_count * size );
     }
     unlock_display_devices();
     if (!adapter)
@@ -2184,7 +2190,25 @@ BOOL WINAPI NtUserEnumDisplaySettings( UNICODE_STRING *device, DWORD index, DEVM
     memset( &devmode->dmDriverExtra, 0, devmode->dmSize - offsetof(DEVMODEW, dmDriverExtra) );
 
     if (index == ENUM_REGISTRY_SETTINGS) ret = read_registry_settings( adapter_path, devmode );
-    else ret = user_driver->pEnumDisplaySettingsEx( device_name, index, devmode, flags );
+    else ret = user_driver->pEnumDisplaySettingsEx( device_name, ENUM_CURRENT_SETTINGS, devmode, flags );
+
+    if (index != ENUM_CURRENT_SETTINGS && index != ENUM_REGISTRY_SETTINGS && ret)
+    {
+        for (ret = FALSE, mode = modes; mode && mode->dmSize; mode = NEXT_DEVMODEW(mode))
+        {
+            if (!(flags & EDS_ROTATEDMODE))
+            {
+                if ((mode->dmFields & DM_DISPLAYORIENTATION) != (devmode->dmFields & DM_DISPLAYORIENTATION)) continue;
+                if (mode->dmDisplayOrientation != devmode->dmDisplayOrientation) continue;
+            }
+            if ((ret = !index--))
+            {
+                memcpy( &devmode->dmFields, &mode->dmFields, devmode->dmSize - FIELD_OFFSET(DEVMODEW, dmFields) );
+                break;
+            }
+        }
+    }
+    free( modes );
 
     if (!ret) WARN( "Failed to query %s display settings.\n", debugstr_w(device_name) );
     else TRACE( "position %dx%d, resolution %ux%u, frequency %u, depth %u, orientation %#x.\n",
diff --git a/dlls/wineandroid.drv/init.c b/dlls/wineandroid.drv/init.c
index 04328346852..f2330987333 100644
--- a/dlls/wineandroid.drv/init.c
+++ b/dlls/wineandroid.drv/init.c
@@ -316,22 +316,15 @@ BOOL ANDROID_EnumDisplaySettingsEx( LPCWSTR name, DWORD n, LPDEVMODEW devmode, D
     devmode->u1.s2.dmDisplayOrientation = 0;
     devmode->u1.s2.dmDisplayFixedOutput = 0;
 
-    if (n == ENUM_CURRENT_SETTINGS) n = 0;
-    if (n == 0)
-    {
-        devmode->dmPelsWidth = screen_width;
-        devmode->dmPelsHeight = screen_height;
-        devmode->dmBitsPerPel = screen_bpp;
-        devmode->dmDisplayFrequency = 60;
-        devmode->dmFields = DM_PELSWIDTH | DM_PELSHEIGHT | DM_BITSPERPEL | DM_DISPLAYFLAGS | DM_DISPLAYFREQUENCY;
-        TRACE( "mode %d -- %dx%d %d bpp @%d Hz\n", n,
-               devmode->dmPelsWidth, devmode->dmPelsHeight,
-               devmode->dmBitsPerPel, devmode->dmDisplayFrequency );
-        return TRUE;
-    }
-    TRACE( "mode %d -- not present\n", n );
-    SetLastError( ERROR_NO_MORE_FILES );
-    return FALSE;
+    devmode->dmPelsWidth = screen_width;
+    devmode->dmPelsHeight = screen_height;
+    devmode->dmBitsPerPel = screen_bpp;
+    devmode->dmDisplayFrequency = 60;
+    devmode->dmFields = DM_PELSWIDTH | DM_PELSHEIGHT | DM_BITSPERPEL | DM_DISPLAYFLAGS | DM_DISPLAYFREQUENCY;
+    TRACE( "mode %d -- %dx%d %d bpp @%d Hz\n", n,
+           devmode->dmPelsWidth, devmode->dmPelsHeight,
+           devmode->dmBitsPerPel, devmode->dmDisplayFrequency );
+    return TRUE;
 }
 
 
diff --git a/dlls/winemac.drv/display.c b/dlls/winemac.drv/display.c
index cfff9ee0e74..4bd83ffffa3 100644
--- a/dlls/winemac.drv/display.c
+++ b/dlls/winemac.drv/display.c
@@ -53,8 +53,6 @@ static const WCHAR initial_mode_keyW[] = {'I','n','i','t','i','a','l',' ','D','i
     ' ','M','o','d','e'};
 static const WCHAR pixelencodingW[] = {'P','i','x','e','l','E','n','c','o','d','i','n','g',0};
 
-static CFArrayRef modes;
-static BOOL modes_has_8bpp, modes_has_16bpp;
 static int default_mode_bpp;
 static pthread_mutex_t modes_mutex = PTHREAD_MUTEX_INITIALIZER;
 
@@ -993,121 +991,23 @@ BOOL macdrv_EnumDisplaySettingsEx(LPCWSTR devname, DWORD mode, DEVMODEW *devmode
     struct macdrv_display *displays = NULL;
     int num_displays;
     CGDisplayModeRef display_mode;
-    int display_mode_bpp;
-    BOOL synthesized = FALSE;
 
     TRACE("%s, %u, %p + %hu, %08x\n", debugstr_w(devname), mode, devmode, devmode->dmSize, flags);
 
     init_original_display_mode();
 
     if (macdrv_get_displays(&displays, &num_displays))
-        goto failed;
-
-    if (mode == ENUM_CURRENT_SETTINGS)
-    {
-        TRACE("mode %d (current) -- getting current mode\n", mode);
-        display_mode = CGDisplayCopyDisplayMode(displays[0].displayID);
-        display_mode_bpp = display_mode_bits_per_pixel(display_mode);
-    }
-    else
-    {
-        DWORD count, i;
-
-        pthread_mutex_lock(&modes_mutex);
-
-        if (mode == 0 || !modes)
-        {
-            if (modes) CFRelease(modes);
-            modes = copy_display_modes(displays[0].displayID, (flags & EDS_RAWMODE) != 0);
-            modes_has_8bpp = modes_has_16bpp = FALSE;
-
-            if (modes)
-            {
-                count = CFArrayGetCount(modes);
-                for (i = 0; i < count && !(modes_has_8bpp && modes_has_16bpp); i++)
-                {
-                    CGDisplayModeRef mode = (CGDisplayModeRef)CFArrayGetValueAtIndex(modes, i);
-                    int bpp = display_mode_bits_per_pixel(mode);
-                    if (bpp == 8)
-                        modes_has_8bpp = TRUE;
-                    else if (bpp == 16)
-                        modes_has_16bpp = TRUE;
-                }
-            }
-        }
-
-        display_mode = NULL;
-        if (modes)
-        {
-            int default_bpp;
-            DWORD seen_modes = 0;
-
-            count = CFArrayGetCount(modes);
-            for (i = 0; i < count; i++)
-            {
-                CGDisplayModeRef candidate = (CGDisplayModeRef)CFArrayGetValueAtIndex(modes, i);
-
-                seen_modes++;
-                if (seen_modes > mode)
-                {
-                    display_mode = (CGDisplayModeRef)CFRetain(candidate);
-                    display_mode_bpp = display_mode_bits_per_pixel(display_mode);
-                    break;
-                }
-            }
-
-            default_bpp = get_default_bpp();
-
-            /* If all the real modes are exhausted, synthesize lower bpp modes. */
-            if (!display_mode && (!modes_has_16bpp || !modes_has_8bpp))
-            {
-                /* We want to synthesize higher depths first. */
-                int synth_bpps[] = { modes_has_16bpp ? 0 : 16, modes_has_8bpp ? 0 : 8 };
-                size_t synth_bpp_idx;
-                for (synth_bpp_idx = 0; synth_bpp_idx < 2; synth_bpp_idx++)
-                {
-                    int synth_bpp = synth_bpps[synth_bpp_idx];
-                    if (synth_bpp == 0)
-                        continue;
-
-                    for (i = 0; i < count; i++)
-                    {
-                        CGDisplayModeRef candidate = (CGDisplayModeRef)CFArrayGetValueAtIndex(modes, i);
-                        /* We only synthesize modes from those having the default bpp. */
-                        if (display_mode_bits_per_pixel(candidate) != default_bpp)
-                            continue;
-
-                        seen_modes++;
-                        if (seen_modes > mode)
-                        {
-                            display_mode = (CGDisplayModeRef)CFRetain(candidate);
-                            display_mode_bpp = synth_bpp;
-                            synthesized = TRUE;
-                            break;
-                        }
-                    }
-
-                    if (display_mode)
-                        break;
-                }
-            }
-        }
-
-        pthread_mutex_unlock(&modes_mutex);
-    }
+        return FALSE;
 
-    if (!display_mode)
-        goto failed;
+    TRACE("mode %d (current) -- getting current mode\n", mode);
 
     /* We currently only report modes for the primary display, so it's at (0, 0). */
     devmode->dmPosition.x = 0;
     devmode->dmPosition.y = 0;
     devmode->dmFields |= DM_POSITION;
 
+    display_mode = CGDisplayCopyDisplayMode(displays[0].displayID);
     display_mode_to_devmode(displays[0].displayID, display_mode, devmode);
-    devmode->dmBitsPerPel = display_mode_bpp;
-    if (devmode->dmBitsPerPel)
-        devmode->dmFields |= DM_BITSPERPEL;
     if (retina_enabled)
     {
         struct display_mode_descriptor* desc = create_original_display_mode_descriptor(displays[0].displayID);
@@ -1131,17 +1031,9 @@ BOOL macdrv_EnumDisplaySettingsEx(LPCWSTR devname, DWORD mode, DEVMODEW *devmode
         TRACE(" stretched");
     if (devmode->dmDisplayFlags & DM_INTERLACED)
         TRACE(" interlaced");
-    if (synthesized)
-        TRACE(" (synthesized)");
     TRACE("\n");
 
     return TRUE;
-
-failed:
-    TRACE("mode %d -- not present\n", mode);
-    if (displays) macdrv_free_displays(displays);
-    SetLastError(ERROR_NO_MORE_FILES);
-    return FALSE;
 }
 
 
diff --git a/dlls/winex11.drv/display.c b/dlls/winex11.drv/display.c
index d296c0ab932..dcc055c3a11 100644
--- a/dlls/winex11.drv/display.c
+++ b/dlls/winex11.drv/display.c
@@ -58,12 +58,6 @@ static const unsigned int depths_24[]  = {8, 16, 24};
 static const unsigned int depths_32[]  = {8, 16, 32};
 const unsigned int *depths;
 
-/* Cached display modes for a device, protected by modes_section */
-static WCHAR cached_device_name[CCHDEVICENAME];
-static DWORD cached_flags;
-static DEVMODEW *cached_modes;
-static UINT cached_mode_count;
-
 static pthread_mutex_t settings_mutex = PTHREAD_MUTEX_INITIALIZER;
 
 void X11DRV_Settings_SetHandler(const struct x11drv_settings_handler *new_handler)
@@ -329,55 +323,17 @@ static DWORD get_display_depth(ULONG_PTR display_id)
  */
 BOOL X11DRV_EnumDisplaySettingsEx( LPCWSTR name, DWORD n, LPDEVMODEW devmode, DWORD flags)
 {
-    DEVMODEW *modes, mode;
-    UINT mode_count;
+    DEVMODEW mode;
     ULONG_PTR id;
 
-    if (n == ENUM_CURRENT_SETTINGS)
-    {
-        if (!settings_handler.get_id( name, &id ) || !settings_handler.get_current_mode( id, &mode ))
-        {
-            ERR("Failed to get %s current display settings.\n", wine_dbgstr_w(name));
-            return FALSE;
-        }
-
-        memcpy( &devmode->dmFields, &mode.dmFields, devmode->dmSize - offsetof(DEVMODEW, dmFields) );
-        if (!is_detached_mode( devmode )) devmode->dmBitsPerPel = get_display_depth( id );
-        return TRUE;
-    }
-
-    pthread_mutex_lock( &settings_mutex );
-    if (n == 0 || wcsicmp(cached_device_name, name) || cached_flags != flags)
-    {
-        if (!settings_handler.get_id(name, &id) || !settings_handler.get_modes(id, flags, &modes, &mode_count))
-        {
-            ERR("Failed to get %s supported display modes.\n", wine_dbgstr_w(name));
-            pthread_mutex_unlock( &settings_mutex );
-            return FALSE;
-        }
-
-        qsort(modes, mode_count, sizeof(*modes) + modes[0].dmDriverExtra, mode_compare);
-
-        if (cached_modes)
-            settings_handler.free_modes(cached_modes);
-        lstrcpyW(cached_device_name, name);
-        cached_flags = flags;
-        cached_modes = modes;
-        cached_mode_count = mode_count;
-    }
-
-    if (n >= cached_mode_count)
+    if (!settings_handler.get_id(name, &id) || !settings_handler.get_current_mode(id, &mode))
     {
-        pthread_mutex_unlock( &settings_mutex );
-        WARN("handler:%s device:%s mode index:%#x not found.\n", settings_handler.name, wine_dbgstr_w(name), n);
-        SetLastError(ERROR_NO_MORE_FILES);
+        ERR("Failed to get %s current display settings.\n", wine_dbgstr_w(name));
         return FALSE;
     }
 
-    mode = *(DEVMODEW *)((BYTE *)cached_modes + (sizeof(*cached_modes) + cached_modes[0].dmDriverExtra) * n);
-    pthread_mutex_unlock( &settings_mutex );
-
     memcpy( &devmode->dmFields, &mode.dmFields, devmode->dmSize - offsetof(DEVMODEW, dmFields) );
+    if (!is_detached_mode(devmode)) devmode->dmBitsPerPel = get_display_depth(id);
     return TRUE;
 }
 
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/551

[PATCH v2 4/5] win32u: Add a default ChangeDisplaySettingsEx driver implementation.

[Rémi Bernon]

From: R��mi Bernon <rbernon(a)codeweavers.com>

---
 dlls/win32u/driver.c    |  2 +-
 dlls/win32u/sysparams.c | 73 +++++++++++++++++++++++++++++++----------
 2 files changed, 56 insertions(+), 19 deletions(-)

diff --git a/dlls/win32u/driver.c b/dlls/win32u/driver.c
index 3d29453c2bf..d7bc5ebc17d 100644
--- a/dlls/win32u/driver.c
+++ b/dlls/win32u/driver.c
@@ -756,7 +756,7 @@ static void nulldrv_UpdateClipboard(void)
 static LONG nulldrv_ChangeDisplaySettingsEx( LPCWSTR name, LPDEVMODEW mode, HWND hwnd,
                                              DWORD flags, LPVOID lparam )
 {
-    return DISP_CHANGE_FAILED;
+    return E_NOTIMPL;
 }
 
 static BOOL nulldrv_EnumDisplaySettingsEx( LPCWSTR name, DWORD num, LPDEVMODEW mode, DWORD flags )
diff --git a/dlls/win32u/sysparams.c b/dlls/win32u/sysparams.c
index 80f18244049..4eb0f23be50 100644
--- a/dlls/win32u/sysparams.c
+++ b/dlls/win32u/sysparams.c
@@ -530,7 +530,7 @@ static BOOL read_registry_settings( const WCHAR *adapter_path, DEVMODEW *mode )
     return ret;
 }
 
-static BOOL write_registry_settings( const WCHAR *adapter_path, const DEVMODEW *mode )
+static BOOL write_registry_settings( const WCHAR *adapter_path, DWORD index, const DEVMODEW *mode )
 {
     HANDLE mutex;
     HKEY hkey;
@@ -542,7 +542,7 @@ static BOOL write_registry_settings( const WCHAR *adapter_path, const DEVMODEW *
     if (!(hkey = reg_open_key( config_key, adapter_path, lstrlenW( adapter_path ) * sizeof(WCHAR) ))) ret = FALSE;
     else
     {
-        ret = write_adapter_mode( hkey, ENUM_REGISTRY_SETTINGS, mode );
+        ret = write_adapter_mode( hkey, index, mode );
         NtClose( hkey );
     }
 
@@ -1478,7 +1478,7 @@ static BOOL update_display_cache_from_registry(void)
     return ret;
 }
 
-static BOOL update_display_cache(void)
+static BOOL update_display_cache( BOOL force )
 {
     HWINSTA winstation = NtUserGetProcessWindowStation();
     struct device_manager_ctx ctx = {0};
@@ -1495,14 +1495,17 @@ static BOOL update_display_cache(void)
         return TRUE;
     }
 
-    user_driver->pUpdateDisplayDevices( &device_manager, FALSE, &ctx );
-    release_display_manager_ctx( &ctx );
-
-    if (update_display_cache_from_registry()) return TRUE;
-    if (ctx.gpu_count)
+    if (!force)
     {
-        ERR( "driver reported devices, but we failed to read them\n" );
-        return FALSE;
+        user_driver->pUpdateDisplayDevices( &device_manager, FALSE, &ctx );
+        release_display_manager_ctx( &ctx );
+
+        if (update_display_cache_from_registry()) return TRUE;
+        if (ctx.gpu_count)
+        {
+            ERR( "driver reported devices, but we failed to read them\n" );
+            return FALSE;
+        }
     }
 
     if (!user_driver->pUpdateDisplayDevices( &device_manager, TRUE, &ctx ))
@@ -1530,13 +1533,20 @@ static BOOL update_display_cache(void)
         struct gdi_monitor monitor =
         {
             .state_flags = DISPLAY_DEVICE_ACTIVE | DISPLAY_DEVICE_ATTACHED,
-            .rc_monitor = {.right = modes[2].dmPelsWidth, .bottom = modes[2].dmPelsHeight},
-            .rc_work = {.right = modes[2].dmPelsWidth, .bottom = modes[2].dmPelsHeight},
         };
+        DEVMODEW mode = {{0}};
         UINT i;
 
         add_gpu( &gpu, &ctx );
         add_adapter( &adapter, &ctx );
+
+        if (!read_adapter_mode( ctx.adapter_key, ENUM_CURRENT_SETTINGS, &mode )) mode = modes[2];
+        write_adapter_mode( ctx.adapter_key, ENUM_CURRENT_SETTINGS, &mode );
+        monitor.rc_monitor.right = mode.dmPelsWidth;
+        monitor.rc_monitor.bottom = mode.dmPelsHeight;
+        monitor.rc_work.right = mode.dmPelsWidth;
+        monitor.rc_work.bottom = mode.dmPelsHeight;
+
         add_monitor( &monitor, &ctx );
         for (i = 0; i < ARRAY_SIZE(modes); ++i) add_mode( modes + i, &ctx );
     }
@@ -1552,7 +1562,7 @@ static BOOL update_display_cache(void)
 
 static BOOL lock_display_devices(void)
 {
-    if (!update_display_cache()) return FALSE;
+    if (!update_display_cache( FALSE )) return FALSE;
     pthread_mutex_lock( &display_lock );
     return TRUE;
 }
@@ -2107,9 +2117,10 @@ LONG WINAPI NtUserChangeDisplaySettings( UNICODE_STRING *devname, DEVMODEW *devm
 {
     DEVMODEW *modes, default_mode = {.dmSize = sizeof(DEVMODEW)}, current_mode = {.dmSize = sizeof(DEVMODEW)};
     WCHAR device_name[CCHDEVICENAME], adapter_path[MAX_PATH];
+    DISPLAY_DEVICEW info = {.cb = sizeof(DISPLAY_DEVICEW)};
     LONG ret = DISP_CHANGE_SUCCESSFUL;
     struct adapter *adapter;
-    SIZE_T size;
+    SIZE_T i, size;
 
     TRACE( "%s %p %p %#x %p\n", debugstr_us(devname), devmode, hwnd, flags, lparam );
     TRACE( "flags=%s\n", _CDS_flags(flags) );
@@ -2117,8 +2128,19 @@ LONG WINAPI NtUserChangeDisplaySettings( UNICODE_STRING *devname, DEVMODEW *devm
     if ((!devname || !devname->Length) && !devmode)
     {
         ret = user_driver->pChangeDisplaySettingsEx( NULL, NULL, hwnd, flags, lparam );
-        if (ret != DISP_CHANGE_SUCCESSFUL)
-            ERR( "Restoring all displays to their registry settings returned %d.\n", ret );
+        if (ret != E_NOTIMPL)
+        {
+            if (ret) ERR( "Restoring all displays to their registry settings returned %d.\n", ret );
+            return ret;
+        }
+
+        for (i = 0; !NtUserEnumDisplayDevices( NULL, i, &info, 0 ); ++i)
+        {
+            UNICODE_STRING str;
+            RtlInitUnicodeString( &str, info.DeviceName );
+            if ((ret = NtUserChangeDisplaySettings( &str, NULL, hwnd, flags, lparam ))) break;
+        }
+
         return ret;
     }
 
@@ -2143,9 +2165,24 @@ LONG WINAPI NtUserChangeDisplaySettings( UNICODE_STRING *devname, DEVMODEW *devm
     if (!NtUserEnumDisplaySettings( devname, ENUM_CURRENT_SETTINGS, ¤t_mode, 0 )) current_mode.dmSize = 0;
 
     if (!(devmode = validate_display_settings( modes, &default_mode, ¤t_mode, devmode ))) ret = DISP_CHANGE_BADMODE;
-    else if ((flags & CDS_UPDATEREGISTRY) && !write_registry_settings( adapter_path, devmode )) ret = DISP_CHANGE_NOTUPDATED;
+    else if ((flags & CDS_UPDATEREGISTRY) && !write_registry_settings( adapter_path, ENUM_REGISTRY_SETTINGS, devmode )) ret = DISP_CHANGE_NOTUPDATED;
     else if (flags & (CDS_TEST | CDS_NORESET)) ret = DISP_CHANGE_SUCCESSFUL;
-    else ret = user_driver->pChangeDisplaySettingsEx( device_name, devmode, hwnd, flags, lparam );
+    else if ((ret = user_driver->pChangeDisplaySettingsEx( device_name, devmode, hwnd, flags, lparam )) == E_NOTIMPL)
+    {
+        if (!write_registry_settings( adapter_path, ENUM_CURRENT_SETTINGS, devmode )) ret = DISP_CHANGE_FAILED;
+        else if (!update_display_cache( TRUE )) ret = DISP_CHANGE_FAILED;
+        else
+        {
+            NtUserSetWindowPos( NtUserGetDesktopWindow(), 0, devmode->dmPosition.x, devmode->dmPosition.y,
+                                devmode->dmPelsWidth, devmode->dmPelsHeight,
+                                SWP_NOZORDER | SWP_NOACTIVATE | SWP_DEFERERASE );
+            NtUserClipCursor( NULL );
+            send_message_timeout( HWND_BROADCAST, WM_DISPLAYCHANGE, devmode->dmBitsPerPel,
+                                  MAKELPARAM( devmode->dmPelsWidth, devmode->dmPelsHeight ),
+                                  SMTO_ABORTIFHUNG, 2000, NULL, FALSE );
+            ret = DISP_CHANGE_SUCCESSFUL;
+        }
+    }
     free( modes );
 
     if (ret) ERR( "Changing %s display settings returned %d.\n", debugstr_us(devname), ret );
-- 
GitLab


https://gitlab.winehq.org/wine/wine/-/merge_requests/551

[PATCH v2 5/5] win32u: Add a default EnumDisplaySettingsEx driver implementation.

[Rémi Bernon]

From: R��mi Bernon <rbernon(a)codeweavers.com>

---
 dlls/win32u/driver.c        |  6 +++---
 dlls/win32u/sysparams.c     | 14 +++++++++-----
 dlls/wineandroid.drv/init.c |  4 ++--
 dlls/winemac.drv/display.c  |  8 ++++----
 dlls/winemac.drv/macdrv.h   |  4 ++--
 dlls/winex11.drv/display.c  |  6 +++---
 dlls/winex11.drv/x11drv.h   |  4 ++--
 7 files changed, 25 insertions(+), 21 deletions(-)

diff --git a/dlls/win32u/driver.c b/dlls/win32u/driver.c
index d7bc5ebc17d..aed6a7cf308 100644
--- a/dlls/win32u/driver.c
+++ b/dlls/win32u/driver.c
@@ -759,9 +759,9 @@ static LONG nulldrv_ChangeDisplaySettingsEx( LPCWSTR name, LPDEVMODEW mode, HWND
     return E_NOTIMPL;
 }
 
-static BOOL nulldrv_EnumDisplaySettingsEx( LPCWSTR name, DWORD num, LPDEVMODEW mode, DWORD flags )
+static INT nulldrv_EnumDisplaySettingsEx( LPCWSTR name, DWORD num, LPDEVMODEW mode, DWORD flags )
 {
-    return FALSE;
+    return -1;  /* use default implementation */
 }
 
 static BOOL nulldrv_UpdateDisplayDevices( const struct gdi_device_manager *manager, BOOL force, void *param )
@@ -1072,7 +1072,7 @@ static LONG loaderdrv_ChangeDisplaySettingsEx( LPCWSTR name, LPDEVMODEW mode, HW
     return load_driver()->pChangeDisplaySettingsEx( name, mode, hwnd, flags, lparam );
 }
 
-static BOOL loaderdrv_EnumDisplaySettingsEx( LPCWSTR name, DWORD num, LPDEVMODEW mode, DWORD flags )
+static INT loaderdrv_EnumDisplaySettingsEx( LPCWSTR name, DWORD num, LPDEVMODEW mode, DWORD flags )
 {
     return load_driver()->pEnumDisplaySettingsEx( name, num, mode, flags );
 }
diff --git a/dlls/win32u/sysparams.c b/dlls/win32u/sysparams.c
index 4eb0f23be50..d0f2446d058 100644
--- a/dlls/win32u/sysparams.c
+++ b/dlls/win32u/sysparams.c
@@ -510,7 +510,7 @@ done:
     return TRUE;
 }
 
-static BOOL read_registry_settings( const WCHAR *adapter_path, DEVMODEW *mode )
+static BOOL read_registry_settings( const WCHAR *adapter_path, DWORD index, DEVMODEW *mode )
 {
     BOOL ret = FALSE;
     HANDLE mutex;
@@ -522,7 +522,7 @@ static BOOL read_registry_settings( const WCHAR *adapter_path, DEVMODEW *mode )
     else if (!(hkey = reg_open_key( config_key, adapter_path, lstrlenW( adapter_path ) * sizeof(WCHAR) ))) ret = FALSE;
     else
     {
-        ret = read_adapter_mode( hkey, ENUM_REGISTRY_SETTINGS, mode );
+        ret = read_adapter_mode( hkey, index, mode );
         NtClose( hkey );
     }
 
@@ -2161,7 +2161,7 @@ LONG WINAPI NtUserChangeDisplaySettings( UNICODE_STRING *devname, DEVMODEW *devm
         return DISP_CHANGE_BADPARAM;
     }
 
-    if (!read_registry_settings( adapter_path, &default_mode )) default_mode.dmSize = 0;
+    if (!read_registry_settings( adapter_path, ENUM_REGISTRY_SETTINGS, &default_mode )) default_mode.dmSize = 0;
     if (!NtUserEnumDisplaySettings( devname, ENUM_CURRENT_SETTINGS, ¤t_mode, 0 )) current_mode.dmSize = 0;
 
     if (!(devmode = validate_display_settings( modes, &default_mode, ¤t_mode, devmode ))) ret = DISP_CHANGE_BADMODE;
@@ -2226,8 +2226,12 @@ BOOL WINAPI NtUserEnumDisplaySettings( UNICODE_STRING *device, DWORD index, DEVM
     devmode->dmSize = offsetof(DEVMODEW, dmICMMethod);
     memset( &devmode->dmDriverExtra, 0, devmode->dmSize - offsetof(DEVMODEW, dmDriverExtra) );
 
-    if (index == ENUM_REGISTRY_SETTINGS) ret = read_registry_settings( adapter_path, devmode );
-    else ret = user_driver->pEnumDisplaySettingsEx( device_name, ENUM_CURRENT_SETTINGS, devmode, flags );
+    if (index == ENUM_REGISTRY_SETTINGS) ret = read_registry_settings( adapter_path, ENUM_REGISTRY_SETTINGS, devmode );
+    else
+    {
+        ret = user_driver->pEnumDisplaySettingsEx( device_name, ENUM_CURRENT_SETTINGS, devmode, flags );
+        if (ret < 0) ret = read_registry_settings( adapter_path, ENUM_CURRENT_SETTINGS, devmode );
+    }
 
     if (index != ENUM_CURRENT_SETTINGS && index != ENUM_REGISTRY_SETTINGS && ret)
     {
diff --git a/dlls/wineandroid.drv/init.c b/dlls/wineandroid.drv/init.c
index f2330987333..6cfe736d2ec 100644
--- a/dlls/wineandroid.drv/init.c
+++ b/dlls/wineandroid.drv/init.c
@@ -307,7 +307,7 @@ BOOL ANDROID_UpdateDisplayDevices( const struct gdi_device_manager *device_manag
 /***********************************************************************
  *           ANDROID_EnumDisplaySettingsEx
  */
-BOOL ANDROID_EnumDisplaySettingsEx( LPCWSTR name, DWORD n, LPDEVMODEW devmode, DWORD flags )
+INT ANDROID_EnumDisplaySettingsEx( LPCWSTR name, DWORD n, LPDEVMODEW devmode, DWORD flags )
 {
     devmode->u2.dmDisplayFlags = 0;
     devmode->dmDisplayFrequency = 0;
@@ -324,7 +324,7 @@ BOOL ANDROID_EnumDisplaySettingsEx( LPCWSTR name, DWORD n, LPDEVMODEW devmode, D
     TRACE( "mode %d -- %dx%d %d bpp @%d Hz\n", n,
            devmode->dmPelsWidth, devmode->dmPelsHeight,
            devmode->dmBitsPerPel, devmode->dmDisplayFrequency );
-    return TRUE;
+    return 1;
 }
 
 
diff --git a/dlls/winemac.drv/display.c b/dlls/winemac.drv/display.c
index 4bd83ffffa3..ce2c95f77d1 100644
--- a/dlls/winemac.drv/display.c
+++ b/dlls/winemac.drv/display.c
@@ -47,7 +47,7 @@ struct display_mode_descriptor
 };
 
 
-BOOL macdrv_EnumDisplaySettingsEx(LPCWSTR devname, DWORD mode, LPDEVMODEW devmode, DWORD flags);
+INT macdrv_EnumDisplaySettingsEx(LPCWSTR devname, DWORD mode, LPDEVMODEW devmode, DWORD flags);
 
 static const WCHAR initial_mode_keyW[] = {'I','n','i','t','i','a','l',' ','D','i','s','p','l','a','y',
     ' ','M','o','d','e'};
@@ -986,7 +986,7 @@ static DEVMODEW *display_get_modes(CGDirectDisplayID display_id, int *modes_coun
  *              EnumDisplaySettingsEx  (MACDRV.@)
  *
  */
-BOOL macdrv_EnumDisplaySettingsEx(LPCWSTR devname, DWORD mode, DEVMODEW *devmode, DWORD flags)
+INT macdrv_EnumDisplaySettingsEx(LPCWSTR devname, DWORD mode, DEVMODEW *devmode, DWORD flags)
 {
     struct macdrv_display *displays = NULL;
     int num_displays;
@@ -997,7 +997,7 @@ BOOL macdrv_EnumDisplaySettingsEx(LPCWSTR devname, DWORD mode, DEVMODEW *devmode
     init_original_display_mode();
 
     if (macdrv_get_displays(&displays, &num_displays))
-        return FALSE;
+        return 0;
 
     TRACE("mode %d (current) -- getting current mode\n", mode);
 
@@ -1033,7 +1033,7 @@ BOOL macdrv_EnumDisplaySettingsEx(LPCWSTR devname, DWORD mode, DEVMODEW *devmode
         TRACE(" interlaced");
     TRACE("\n");
 
-    return TRUE;
+    return 1;
 }
 
 
diff --git a/dlls/winemac.drv/macdrv.h b/dlls/winemac.drv/macdrv.h
index 7cb1665a8fc..d5c68f871eb 100644
--- a/dlls/winemac.drv/macdrv.h
+++ b/dlls/winemac.drv/macdrv.h
@@ -125,8 +125,8 @@ extern BOOL macdrv_ActivateKeyboardLayout(HKL hkl, UINT flags) DECLSPEC_HIDDEN;
 extern void macdrv_Beep(void) DECLSPEC_HIDDEN;
 extern LONG macdrv_ChangeDisplaySettingsEx(LPCWSTR devname, LPDEVMODEW devmode,
                                            HWND hwnd, DWORD flags, LPVOID lpvoid) DECLSPEC_HIDDEN;
-extern BOOL macdrv_EnumDisplaySettingsEx(LPCWSTR devname, DWORD mode,
-                                         LPDEVMODEW devmode, DWORD flags) DECLSPEC_HIDDEN;
+extern INT macdrv_EnumDisplaySettingsEx(LPCWSTR devname, DWORD mode,
+                                        LPDEVMODEW devmode, DWORD flags) DECLSPEC_HIDDEN;
 extern LRESULT macdrv_ClipboardWindowProc(HWND hwnd, UINT msg, WPARAM wp, LPARAM lp) DECLSPEC_HIDDEN;
 extern BOOL macdrv_UpdateDisplayDevices( const struct gdi_device_manager *device_manager,
                                          BOOL force, void *param ) DECLSPEC_HIDDEN;
diff --git a/dlls/winex11.drv/display.c b/dlls/winex11.drv/display.c
index dcc055c3a11..9fa051ed17d 100644
--- a/dlls/winex11.drv/display.c
+++ b/dlls/winex11.drv/display.c
@@ -321,7 +321,7 @@ static DWORD get_display_depth(ULONG_PTR display_id)
  *      EnumDisplaySettingsEx  (X11DRV.@)
  *
  */
-BOOL X11DRV_EnumDisplaySettingsEx( LPCWSTR name, DWORD n, LPDEVMODEW devmode, DWORD flags)
+INT X11DRV_EnumDisplaySettingsEx( LPCWSTR name, DWORD n, LPDEVMODEW devmode, DWORD flags)
 {
     DEVMODEW mode;
     ULONG_PTR id;
@@ -329,12 +329,12 @@ BOOL X11DRV_EnumDisplaySettingsEx( LPCWSTR name, DWORD n, LPDEVMODEW devmode, DW
     if (!settings_handler.get_id(name, &id) || !settings_handler.get_current_mode(id, &mode))
     {
         ERR("Failed to get %s current display settings.\n", wine_dbgstr_w(name));
-        return FALSE;
+        return 0;
     }
 
     memcpy( &devmode->dmFields, &mode.dmFields, devmode->dmSize - offsetof(DEVMODEW, dmFields) );
     if (!is_detached_mode(devmode)) devmode->dmBitsPerPel = get_display_depth(id);
-    return TRUE;
+    return 1;
 }
 
 BOOL is_detached_mode(const DEVMODEW *mode)
diff --git a/dlls/winex11.drv/x11drv.h b/dlls/winex11.drv/x11drv.h
index 0fe29d8e322..e124ca57bad 100644
--- a/dlls/winex11.drv/x11drv.h
+++ b/dlls/winex11.drv/x11drv.h
@@ -215,8 +215,8 @@ extern BOOL X11DRV_GetCursorPos( LPPOINT pos ) DECLSPEC_HIDDEN;
 extern BOOL X11DRV_ClipCursor( LPCRECT clip ) DECLSPEC_HIDDEN;
 extern LONG X11DRV_ChangeDisplaySettingsEx( LPCWSTR devname, LPDEVMODEW devmode,
                                             HWND hwnd, DWORD flags, LPVOID lpvoid ) DECLSPEC_HIDDEN;
-extern BOOL X11DRV_EnumDisplaySettingsEx( LPCWSTR name, DWORD n, LPDEVMODEW devmode,
-                                          DWORD flags ) DECLSPEC_HIDDEN;
+extern INT X11DRV_EnumDisplaySettingsEx( LPCWSTR name, DWORD n, LPDEVMODEW devmode,
+                                         DWORD flags ) DECLSPEC_HIDDEN;
 extern BOOL X11DRV_UpdateDisplayDevices( const struct gdi_device_manager *device_manager,
                                          BOOL force, void *param ) DECLSPEC_HIDDEN;
 extern BOOL X11DRV_CreateDesktopWindow( HWND hwnd ) DECLSPEC_HIDDEN;
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/551

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/win32u/sysparams.c:
> +    for (mode = modes; mode && mode->dmSize; mode = NEXT_DEVMODEW(mode))
> +    {
> +        if ((devmode->dmFields & DM_BITSPERPEL) && devmode->dmBitsPerPel && devmode->dmBitsPerPel != mode->dmBitsPerPel)
> +            continue;
> +        if ((devmode->dmFields & DM_PELSWIDTH) && devmode->dmPelsWidth != mode->dmPelsWidth)
> +            continue;
> +        if ((devmode->dmFields & DM_PELSHEIGHT) && devmode->dmPelsHeight != mode->dmPelsHeight)
> +            continue;
> +        if ((devmode->dmFields & DM_DISPLAYFREQUENCY) && devmode->dmDisplayFrequency && mode->dmDisplayFrequency &&
> +            devmode->dmDisplayFrequency != 1 && devmode->dmDisplayFrequency != mode->dmDisplayFrequency)
> +            continue;
> +        if ((devmode->dmFields & DM_DISPLAYORIENTATION) && devmode->dmDisplayOrientation != mode->dmDisplayOrientation)
> +            continue;
> +
> +        mode->dmFields |= DM_POSITION;
> +        mode->dmPosition = devmode->dmPosition;
> I don't think you should write to the user-provided structure. Please make a copy.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/551#note_5237

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/win32u/sysparams.c:
> {
>          lstrcpyW( device_name, adapter->dev.device_name );
>          lstrcpyW( adapter_path, adapter->config_key );
> +        size = sizeof(DEVMODEW) + adapter->modes[0].dmDriverExtra;
> +        /* allocate an extra mode to make iteration easier */
> +        modes = calloc( adapter->mode_count + 1, size );
> +        if (modes) memcpy( modes, adapter->modes, adapter->mode_count * size );

If you pass an adapter pointer to validate_display_settings(), you can avoid copying these modes and have access to adapter->mode_count.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/551#note_5238

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/win32u/sysparams.c:
> {
>          lstrcpyW( device_name, adapter->dev.device_name );
>          lstrcpyW( adapter_path, adapter->config_key );
> +        size = sizeof(DEVMODEW) + adapter->modes[0].dmDriverExtra;
> +        /* allocate an extra mode for easier iteration */
> +        modes = calloc( adapter->mode_count + 1, size );
> +        if (modes) memcpy( modes, adapter->modes, adapter->mode_count * size );

I think you can avoid the memory copying here as well.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/551#note_5239

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/win32u/sysparams.c:
> memset( &devmode->dmDriverExtra, 0, devmode->dmSize - offsetof(DEVMODEW, dmDriverExtra) );
>
> if (index == ENUM_REGISTRY_SETTINGS) ret = read_registry_settings( adapter_path, devmode );
> -    else ret = user_driver->pEnumDisplaySettingsEx( device_name, index, devmode, flags );
> +    else ret = user_driver->pEnumDisplaySettingsEx( device_name, ENUM_CURRENT_SETTINGS, devmode, flags );

If you're enumerating available modes and EDS_ROTATEDMODE is not specified. You don't need to query the current display mode, which is quite expensive and should be avoided.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/551#note_5240

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/win32u/sysparams.c:
> memset( &devmode->dmDriverExtra, 0, devmode->dmSize - offsetof(DEVMODEW, dmDriverExtra) );
>
> if (index == ENUM_REGISTRY_SETTINGS) ret = read_registry_settings( adapter_path, devmode );
> -    else ret = user_driver->pEnumDisplaySettingsEx( device_name, index, devmode, flags );
> +    else ret = user_driver->pEnumDisplaySettingsEx( device_name, ENUM_CURRENT_SETTINGS, devmode, flags );
> +
> +    if (index != ENUM_CURRENT_SETTINGS && index != ENUM_REGISTRY_SETTINGS && ret)
> +    {
> +        for (ret = FALSE, mode = modes; mode && mode->dmSize; mode = NEXT_DEVMODEW(mode))
> +        {
> +            if (!(flags & EDS_ROTATEDMODE))
> +            {
> +                if ((mode->dmFields & DM_DISPLAYORIENTATION) != (devmode->dmFields & DM_DISPLAYORIENTATION)) continue;

All available display modes and the current display modes must have DM_DISPLAYORIENTATION. So you can simply this condition. Don't forget to set DM_DISPLAYORIENTATION for the current mode from other drivers, which can be a separate patch.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/551#note_5241

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/wineandroid.drv/init.c:
> -        devmode->dmDisplayFrequency = 60;
> -        devmode->dmFields = DM_PELSWIDTH | DM_PELSHEIGHT | DM_BITSPERPEL | DM_DISPLAYFLAGS | DM_DISPLAYFREQUENCY;
> -        TRACE( "mode %d -- %dx%d %d bpp @%d Hz\n", n,
> -               devmode->dmPelsWidth, devmode->dmPelsHeight,
> -               devmode->dmBitsPerPel, devmode->dmDisplayFrequency );
> -        return TRUE;
> -    }
> -    TRACE( "mode %d -- not present\n", n );
> -    SetLastError( ERROR_NO_MORE_FILES );
> -    return FALSE;
> +    devmode->dmPelsWidth = screen_width;
> +    devmode->dmPelsHeight = screen_height;
> +    devmode->dmBitsPerPel = screen_bpp;
> +    devmode->dmDisplayFrequency = 60;
> +    devmode->dmFields = DM_PELSWIDTH | DM_PELSHEIGHT | DM_BITSPERPEL | DM_DISPLAYFLAGS | DM_DISPLAYFREQUENCY;
> +    TRACE( "mode %d -- %dx%d %d bpp @%d Hz\n", n,
> n is always ENUM_CURRENT_SETTINGS now. Change the comment a bit. And add DM_DISPLAYORIENTATION.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/551#note_5242

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/winemac.drv/display.c:
> -        }
> -
> -        pthread_mutex_unlock(&modes_mutex);
> -    }
> +        return FALSE;
>
> -    if (!display_mode)
> -        goto failed;
> +    TRACE("mode %d (current) -- getting current mode\n", mode);
>
> /* We currently only report modes for the primary display, so it's at (0, 0). */
>      devmode->dmPosition.x = 0;
>      devmode->dmPosition.y = 0;
>      devmode->dmFields |= DM_POSITION;
>
> +    display_mode = CGDisplayCopyDisplayMode(displays[0].displayID);
> display_mode should be released with CGDisplayModeRelease when you're done using it.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/551#note_5243

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/winemac.drv/display.c:
> TRACE(" stretched");
>      if (devmode->dmDisplayFlags & DM_INTERLACED)
>          TRACE(" interlaced");
> -    if (synthesized)
> -        TRACE(" (synthesized)");
>      TRACE("\n");
>
> return TRUE;

Leaking displays.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/551#note_5244

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/winex11.drv/display.c:
> */
>  BOOL X11DRV_EnumDisplaySettingsEx( LPCWSTR name, DWORD n, LPDEVMODEW devmode, DWORD flags)
> Well, if n is always ENUM_CURRENT_SETTINGS now, might as well remove the n parameter and maybe change the function name to something like EnumCurrentDisplaySettings

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/551#note_5245

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/win32u/sysparams.c:
> if ((!devname || !devname->Length) && !devmode)
>      {
>          ret = user_driver->pChangeDisplaySettingsEx( NULL, NULL, hwnd, flags, lparam );
> -        if (ret != DISP_CHANGE_SUCCESSFUL)
> -            ERR( "Restoring all displays to their registry settings returned %d.\n", ret );
> +        if (ret != E_NOTIMPL)
> +        {
> +            if (ret) ERR( "Restoring all displays to their registry settings returned %d.\n", ret );
> +            return ret;
> +        }
> +
> +        for (i = 0; !NtUserEnumDisplayDevices( NULL, i, &info, 0 ); ++i)
> +        {
> +            UNICODE_STRING str;
> +            RtlInitUnicodeString( &str, info.DeviceName );
> +            if ((ret = NtUserChangeDisplaySettings( &str, NULL, hwnd, flags, lparam ))) break;

I don't like the way the nulldrv display settings handler code is mixing with the normal code path. You should place nulldrv's ChangeDisplaySettings implementation in nulldrv_ChangeDisplaySettingsEx(), even at the expense of code duplication. Same for the force parameter in update_display_cache(). There should be a nulldrv_UpdateDisplayDevices that always succeeds. Then you don't need to check E_NOTIMPL everywhere. Same for below.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/551#note_5246

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/win32u/driver.c:
> return E_NOTIMPL;
>  }
>
> -static BOOL nulldrv_EnumDisplaySettingsEx( LPCWSTR name, DWORD num, LPDEVMODEW mode, DWORD flags )
> +static INT nulldrv_EnumDisplaySettingsEx( LPCWSTR name, DWORD num, LPDEVMODEW mode, DWORD flags )
>  {
> -    return FALSE;
> +    return -1;  /* use default implementation */

As I said, keep what's in nulldrv in nulldrv_* functions. This is making things messy.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/551#note_5247

[Rémi Bernon]

On Thu Jul 28 10:30:34 2022 +0000, Zhiyi Zhang wrote:
> I don't think you should write to the user-provided structure. Please
> make a copy.
> I think it's not? `mode` is a full display mode, iterating in the adapter mode array copy, and it includes the driver extra. `devmode` is the user provided one.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/551#note_5249

[Rémi Bernon]

On Thu Jul 28 10:30:53 2022 +0000, Zhiyi Zhang wrote:
> If you pass an adapter pointer to validate_display_settings(), you can
> avoid copying these modes and have access to adapter->mode_count.
> Yeah but adapter pointer is only valid while holding the critical section.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/551#note_5250

[Rémi Bernon]

On Thu Jul 28 10:31:26 2022 +0000, Zhiyi Zhang wrote:
> I think you can avoid the memory copying here as well.
> As above, only if we extend the critical section to include most of the calls. I think it's more efficient to copy.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/551#note_5251

[Zhiyi Zhang (＠zhiyi)]

On Thu Jul 28 12:13:04 2022 +0000, R��mi Bernon wrote:
> I think it's not? `mode` is a full display mode, iterating in the
> adapter mode array copy, and it includes the driver extra. `devmode` is
> the user provided one.
> I see. You're right.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/551#note_5253

[Rémi Bernon]

On Thu Jul 28 10:39:14 2022 +0000, Zhiyi Zhang wrote:
> I don't like the way the nulldrv display settings handler code is mixing
> with the normal code path. You should place nulldrv's
> ChangeDisplaySettings implementation in
> nulldrv_ChangeDisplaySettingsEx(), even at the expense of code
> duplication. Same for the force parameter in update_display_cache().
> There should be a nulldrv_UpdateDisplayDevices that always succeeds.
> Then you don't need to check E_NOTIMPL everywhere. Same for below.
> There's been a similar discussion before when implementing better keyboard layout support, and @julliard explicitly asked to have code in the generic part of `user32` rather than in nulldrv.

The `nulldrv` isn't supposed to implement anything, and the generic part of `user32` / `win32u` should implement the correct behavior unless the driver indicates that it took over.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/551#note_5254

[Zhiyi Zhang (＠zhiyi)]

On Thu Jul 28 12:23:57 2022 +0000, R��mi Bernon wrote:
> There's been a similar discussion before when implementing better
> keyboard layout support, and @julliard explicitly asked to have code in
> the generic part of `user32` rather than in nulldrv.
> The `nulldrv` isn't supposed to implement anything, and the generic part
> of `user32` / `win32u` should implement the correct behavior unless the
> driver indicates that it took over.
> I still don't like it personally but if we have to do it this way then some comments separating the nulldrv/fallback code path would be nice.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/551#note_5312

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/win32u/driver.c:
> static LONG nulldrv_ChangeDisplaySettingsEx( LPCWSTR name, LPDEVMODEW mode, HWND hwnd,
>                                               DWORD flags, LPVOID lparam )
>  {
> -    return DISP_CHANGE_FAILED;
> +    return E_NOTIMPL;
> E_NOTIMPL is for HRESULT. ERROR_CALL_NOT_IMPLEMENTED seems better.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/551#note_5313

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/win32u/sysparams.c:
> {
>          ret = user_driver->pChangeDisplaySettingsEx( NULL, NULL, hwnd, flags, lparam );
> -        if (ret != DISP_CHANGE_SUCCESSFUL)
> -            ERR( "Restoring all displays to their registry settings returned %d.\n", ret );
> +        if (ret != E_NOTIMPL)
> +        {
> +            if (ret) ERR( "Restoring all displays to their registry settings returned %d.\n", ret );
> +            return ret;
> +        }
> +
> +        for (i = 0; !NtUserEnumDisplayDevices( NULL, i, &info, 0 ); ++i)
> +        {
> +            UNICODE_STRING str;
> +            RtlInitUnicodeString( &str, info.DeviceName );
> +            if ((ret = NtUserChangeDisplaySettings( &str, NULL, hwnd, flags, lparam ))) break;
> +        }

You can simply pass \\.\DISPLAY1 to NtUserChangeDisplaySettings(). I don't think nulldrv is going to support multiple monitors, is it? If you do, this loop to reset adapter settings is wrong. ChangeDisplaySettingsEx(NULL, NULL, ...) to reset all adapters to their registry settings should read all registry settings at once, then resolve any position conflicts, then finally apply adjusted modes to each adapter. Doing it in a loop calling NtUserChangeDisplaySettings separately for each monitor might end up with the wrong position because you're adjusting one monitor at a time. For details, see how place_all_displays() is used in X11DRV_ChangeDisplaySettingsEx().

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/551#note_5314

[Rémi Bernon]

On Fri Jul 29 02:14:06 2022 +0000, Zhiyi Zhang wrote:
> You can simply pass \\.\DISPLAY1 to NtUserChangeDisplaySettings(). I
> don't think nulldrv is going to support multiple monitors, is it? If you
> do, this loop to reset adapter settings is wrong.
> ChangeDisplaySettingsEx(NULL, NULL, ...) to reset all adapters to their
> registry settings should read all registry settings at once, then
> resolve any position conflicts, then finally apply adjusted modes to
> each adapter. Doing it in a loop calling NtUserChangeDisplaySettings
> separately for each monitor might end up with the wrong position because
> you're adjusting one monitor at a time. For details, see how
> place_all_displays() is used in X11DRV_ChangeDisplaySettingsEx().
> The idea is that the generic code should do the right thing generally, not just what's needed for nulldrv, so I guess I'll have to move the monitor placing logic there.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/551#note_5318