[PATCH v3 0/2] MR2593: Draft: winex11.drv: UpdateDisplayDevice regression fixes

[Alexandros Frantzis (＠afrantzis)]

This MR contains two somewhat related commits/fixes for regressions that were made apparent in https://bugs.winehq.org/show_bug.cgi?id=54781.

1. The adapter ids used by the host and settings handlers are not guaranteed to be compatible, so ensure we are using the proper id with each
handler.

Note that although the possibly incompatible id was already used in `settings_handler.get_modes()` before, the settings handler was ignoring it anyway for `get_modes()` in the case it was really incompatible (e.g., using host handler xrandr 1.4, settings handler xrandr 1.0), . The problem manifests now since we are additionally calling `settings_handler.get_current_mode()` which does check the id value (e.g., in xrandr 1.0 to differentiate primary vs non-primary).

2. The virtual desktop get_current_mode() implementation recurses into win32u (through NtUserGetPrimaryScreenRect) causing a deadlock if we call it from within UpdateDisplayDevices. There is a way to avoid the deadlock if we get the current_modes before calling add_gpu(), but we don't need to get and update the win32u current/registry in the virtual desktop case anyway, so skip the call completely instead.

--
  v3: winex11.drv: Do not call desktop get_current_mode() from UpdateDisplayDevices.
      winex11.drv: Use the proper id with the settings handler.

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

[PATCH v3 1/2] winex11.drv: Use the proper id with the settings handler.

[Alexandros Frantzis]

From: Alexandros Frantzis

The adapter ids used by the host and settings handlers are not guaranteed
to be compatible, so ensure we are using the proper id with each
handler.

Signed-off-by: Alexandros Frantzis 
---
 dlls/winex11.drv/display.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/dlls/winex11.drv/display.c b/dlls/winex11.drv/display.c
index cd36c3c559c..75b585ff201 100644
--- a/dlls/winex11.drv/display.c
+++ b/dlls/winex11.drv/display.c
@@ -564,6 +564,10 @@ BOOL X11DRV_UpdateDisplayDevices( const struct gdi_device_manager *device_manage
         for (adapter = 0; adapter < adapter_count; adapter++)
         {
             DEVMODEW current_mode = {.dmSize = sizeof(current_mode)};
+            WCHAR devname[32];
+            char buffer[32];
+            ULONG_PTR settings_id;
+            BOOL is_primary = adapters[adapter].state_flags & DISPLAY_DEVICE_PRIMARY_DEVICE;

device_manager->add_adapter( &adapters[adapter], param );

@@ -576,8 +580,13 @@ BOOL X11DRV_UpdateDisplayDevices( const struct gdi_device_manager *device_manage

handler->free_monitors(monitors, monitor_count);

-            settings_handler.get_current_mode( adapters[adapter].id, &current_mode );
-            if (!settings_handler.get_modes( adapters[adapter].id, EDS_ROTATEDMODE, &modes, &mode_count ))
+            /* Get the settings handler id for the adapter */
+            snprintf( buffer, sizeof(buffer), "\\.\DISPLAY%d", adapter + 1 );
+            asciiz_to_unicode( devname, buffer );
+            if (!settings_handler.get_id( devname, is_primary, &settings_id )) break;
+
+            settings_handler.get_current_mode( settings_id, &current_mode );
+            if (!settings_handler.get_modes( settings_id, EDS_ROTATEDMODE, &modes, &mode_count ))
                 continue;

for (mode = modes; mode_count; mode_count--)

[PATCH v3 2/2] winex11.drv: Do not call desktop get_current_mode() from UpdateDisplayDevices.

[Alexandros Frantzis]

From: Alexandros Frantzis

We don't need to set the win32u current mode when we are in virtual
desktop mode, and, additionally, skipping this avoids a deadlock, since
the virtual desktop get_current_mode() implementation recurses into
win32u.

Signed-off-by: Alexandros Frantzis 
Fixes: 4232312dffe1cd115aa6bfd755f5b7c6a403e3fc
Wine-Bug: https://bugs.winehq.org/show_bug.cgi?id=54781
---
 dlls/winex11.drv/display.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/dlls/winex11.drv/display.c b/dlls/winex11.drv/display.c
index 75b585ff201..c60ffd1bc5b 100644
--- a/dlls/winex11.drv/display.c
+++ b/dlls/winex11.drv/display.c
@@ -585,7 +585,12 @@ BOOL X11DRV_UpdateDisplayDevices( const struct gdi_device_manager *device_manage
             asciiz_to_unicode( devname, buffer );
             if (!settings_handler.get_id( devname, is_primary, &settings_id )) break;

-            settings_handler.get_current_mode( settings_id, &current_mode );
+            /* We don't need to set the win32u current mode when we are in
+             * virtual desktop mode, and, additionally, skipping this avoids a
+             * deadlock, since the desktop get_current_mode() implementation
+             * recurses into win32u. */
+            if (handler != &desktop_handler)
+                settings_handler.get_current_mode( settings_id, &current_mode );
             if (!settings_handler.get_modes( settings_id, EDS_ROTATEDMODE, &modes, &mode_count ))
                 continue;

[Rémi Bernon (＠rbernon)]

Rémi Bernon (@rbernon) commented about dlls/winex11.drv/display.c:
>          asciiz_to_unicode( devname, buffer );
>          if (!settings_handler.get_id( devname, is_primary, &settings_id )) break;
>
> -
>        settings_handler.get_current_mode( settings_id, &current_mode );
> -
>        /* Get the current mode, but only if we are not not using the desktop
> -
>         * handler, since the desktop handler get_current_mode() recurses into
> -
>         * win32u. */
> -
>        if (handler != &desktop_handler)

It's probably better to use `is_virtual_desktop()` for consistency, though I don't know if that's enough.

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/winex11.drv/display.c:
>          asciiz_to_unicode( devname, buffer );
>          if (!settings_handler.get_id( devname, is_primary, &settings_id )) break;
>
> -
>        settings_handler.get_current_mode( settings_id, &current_mode );
> -
>        /* We don't need to set the win32u current mode when we are in
> -
>         * virtual desktop mode, and, additionally, skipping this avoids a
> -
>         * deadlock, since the desktop get_current_mode() implementation
> -
>         * recurses into win32u. */
> -
>        if (handler != &desktop_handler)

You can save the return value of the previous is_virtual_desktop() call and use it throughout the function.

[Zhiyi Zhang (＠zhiyi)]

Zhiyi Zhang (@zhiyi) commented about dlls/winex11.drv/display.c:
>          handler->free_monitors(monitors, monitor_count);
>
> -
>        settings_handler.get_current_mode( adapters[adapter].id, &current_mode );
> -
>        if (!settings_handler.get_modes( adapters[adapter].id, EDS_ROTATEDMODE, &modes, &mode_count ))
> -
>        /* Get the settings handler id for the adapter */
> -
>        snprintf( buffer, sizeof(buffer), "\\\\.\\DISPLAY%d", adapter + 1 );
> -
>        asciiz_to_unicode( devname, buffer );
> -
>        if (!settings_handler.get_id( devname, is_primary, &settings_id )) break;

Yes, this is overlooked. When using both the XRandR 1.4 display device handler and settings handler, their IDs are compatible but not in other cases. To avoid future confusion, maybe it's better to introduce types for adapter device ID and adapter setting ID. Or maybe adding comments in struct x11drv_settings_handler and struct x11drv_display_device_handler about not to mix IDs is enough.