[PATCH v2 0/2] MR5579: win32u: Write / read available display modes as a single binary blob.
Reduces the time needed to write them from 20ms to 2ms and to read them from 10ms to 1ms, as well as reducing the number of wineserver requests by a lot. -- v2: win32u: Read / write source modes as a single registry blob. winex11: Only request display modes driver data when needed. https://gitlab.winehq.org/wine/wine/-/merge_requests/5579
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/winex11.drv/display.c | 7 +++---- dlls/winex11.drv/x11drv.h | 4 +++- dlls/winex11.drv/xrandr.c | 32 ++++++++++++++++---------------- dlls/winex11.drv/xvidmode.c | 14 +++++++------- 4 files changed, 29 insertions(+), 28 deletions(-) diff --git a/dlls/winex11.drv/display.c b/dlls/winex11.drv/display.c index b4a2e4b9487..3ec20743f66 100644 --- a/dlls/winex11.drv/display.c +++ b/dlls/winex11.drv/display.c @@ -71,7 +71,7 @@ static BOOL nores_get_id(const WCHAR *device_name, BOOL is_primary, x11drv_setti return TRUE; } -static BOOL nores_get_modes(x11drv_settings_id id, DWORD flags, DEVMODEW **new_modes, UINT *mode_count) +static BOOL nores_get_modes( x11drv_settings_id id, DWORD flags, DEVMODEW **new_modes, UINT *mode_count, BOOL full ) { RECT primary = get_host_primary_monitor_rect(); DEVMODEW *modes; @@ -261,8 +261,7 @@ static DEVMODEW *get_full_mode(x11drv_settings_id id, DEVMODEW *dev_mode) if (is_detached_mode(dev_mode)) return dev_mode; - if (!settings_handler.get_modes(id, EDS_ROTATEDMODE, &modes, &mode_count)) - return NULL; + if (!settings_handler.get_modes( id, EDS_ROTATEDMODE, &modes, &mode_count, TRUE )) return NULL; for (mode_idx = 0; mode_idx < mode_count; ++mode_idx) { @@ -547,7 +546,7 @@ BOOL X11DRV_UpdateDisplayDevices( const struct gdi_device_manager *device_manage if (!settings_handler.get_id( devname, is_primary, &settings_id )) break; settings_handler.get_current_mode( settings_id, ¤t_mode ); - if (settings_handler.get_modes( settings_id, EDS_ROTATEDMODE, &modes, &mode_count )) + if (settings_handler.get_modes( settings_id, EDS_ROTATEDMODE, &modes, &mode_count, FALSE )) { device_manager->add_modes( ¤t_mode, mode_count, modes, param ); settings_handler.free_modes( modes ); diff --git a/dlls/winex11.drv/x11drv.h b/dlls/winex11.drv/x11drv.h index 0ccffecec49..384074becad 100644 --- a/dlls/winex11.drv/x11drv.h +++ b/dlls/winex11.drv/x11drv.h @@ -726,7 +726,7 @@ struct x11drv_settings_handler * dmDisplayFlags and dmDisplayFrequency * * Return FALSE on failure with parameters unchanged and error code set. Return TRUE on success */ - BOOL (*get_modes)(x11drv_settings_id id, DWORD flags, DEVMODEW **modes, UINT *mode_count); + BOOL (*get_modes)(x11drv_settings_id id, DWORD flags, DEVMODEW **modes, UINT *mode_count, BOOL full); /* free_modes() will be called to free the mode list returned from get_modes() */ void (*free_modes)(DEVMODEW *modes); @@ -747,6 +747,8 @@ struct x11drv_settings_handler LONG (*set_current_mode)(x11drv_settings_id id, const DEVMODEW *mode); }; +#define NEXT_DEVMODEW(mode) ((DEVMODEW *)((char *)((mode) + 1) + (mode)->dmDriverExtra)) + extern void X11DRV_Settings_SetHandler(const struct x11drv_settings_handler *handler); extern void X11DRV_init_desktop( Window win, unsigned int width, unsigned int height ); diff --git a/dlls/winex11.drv/xrandr.c b/dlls/winex11.drv/xrandr.c index c161b119cc9..89ce2a6a263 100644 --- a/dlls/winex11.drv/xrandr.c +++ b/dlls/winex11.drv/xrandr.c @@ -152,10 +152,10 @@ static BOOL xrandr10_get_id( const WCHAR *device_name, BOOL is_primary, x11drv_s } static void add_xrandr10_mode( DEVMODEW *mode, DWORD depth, DWORD width, DWORD height, - DWORD frequency, SizeID size_id ) + DWORD frequency, SizeID size_id, BOOL full ) { mode->dmSize = sizeof(*mode); - mode->dmDriverExtra = sizeof(SizeID); + mode->dmDriverExtra = full ? sizeof(SizeID) : 0; mode->dmFields = DM_DISPLAYORIENTATION | DM_BITSPERPEL | DM_PELSWIDTH | DM_PELSHEIGHT | DM_DISPLAYFLAGS; if (frequency) @@ -168,10 +168,10 @@ static void add_xrandr10_mode( DEVMODEW *mode, DWORD depth, DWORD width, DWORD h mode->dmPelsWidth = width; mode->dmPelsHeight = height; mode->dmDisplayFlags = 0; - memcpy( (BYTE *)mode + sizeof(*mode), &size_id, sizeof(size_id) ); + if (full) memcpy( mode + 1, &size_id, sizeof(size_id) ); } -static BOOL xrandr10_get_modes( x11drv_settings_id id, DWORD flags, DEVMODEW **new_modes, UINT *new_mode_count ) +static BOOL xrandr10_get_modes( x11drv_settings_id id, DWORD flags, DEVMODEW **new_modes, UINT *new_mode_count, BOOL full ) { INT size_idx, depth_idx, rate_idx, mode_idx = 0; INT size_count, rate_count, mode_count = 0; @@ -201,24 +201,24 @@ static BOOL xrandr10_get_modes( x11drv_settings_id id, DWORD flags, DEVMODEW **n return FALSE; } - for (size_idx = 0; size_idx < size_count; ++size_idx) + for (size_idx = 0, mode = modes; size_idx < size_count; ++size_idx) { for (depth_idx = 0; depth_idx < DEPTH_COUNT; ++depth_idx) { rates = pXRRRates( gdi_display, DefaultScreen( gdi_display ), size_idx, &rate_count ); if (!rate_count) { - mode = (DEVMODEW *)((BYTE *)modes + (sizeof(*mode) + sizeof(SizeID)) * mode_idx++); add_xrandr10_mode( mode, depths[depth_idx], sizes[size_idx].width, - sizes[size_idx].height, 0, size_idx ); + sizes[size_idx].height, 0, size_idx, full ); + mode = NEXT_DEVMODEW( mode ); continue; } for (rate_idx = 0; rate_idx < rate_count; ++rate_idx) { - mode = (DEVMODEW *)((BYTE *)modes + (sizeof(*mode) + sizeof(SizeID)) * mode_idx++); add_xrandr10_mode( mode, depths[depth_idx], sizes[size_idx].width, - sizes[size_idx].height, rates[rate_idx], size_idx ); + sizes[size_idx].height, rates[rate_idx], size_idx, full ); + mode = NEXT_DEVMODEW( mode ); } } } @@ -1316,10 +1316,10 @@ static BOOL xrandr14_get_id( const WCHAR *device_name, BOOL is_primary, x11drv_s } static void add_xrandr14_mode( DEVMODEW *mode, XRRModeInfo *info, DWORD depth, DWORD frequency, - DWORD orientation ) + DWORD orientation, BOOL full ) { mode->dmSize = sizeof(*mode); - mode->dmDriverExtra = sizeof(RRMode); + mode->dmDriverExtra = full ? sizeof(RRMode) : 0; mode->dmFields = DM_DISPLAYORIENTATION | DM_BITSPERPEL | DM_PELSWIDTH | DM_PELSHEIGHT | DM_DISPLAYFLAGS; if (frequency) @@ -1340,10 +1340,10 @@ static void add_xrandr14_mode( DEVMODEW *mode, XRRModeInfo *info, DWORD depth, D mode->dmDisplayOrientation = orientation; mode->dmBitsPerPel = depth; mode->dmDisplayFlags = 0; - memcpy( (BYTE *)mode + sizeof(*mode), &info->id, sizeof(info->id) ); + if (full) memcpy( mode + 1, &info->id, sizeof(info->id) ); } -static BOOL xrandr14_get_modes( x11drv_settings_id id, DWORD flags, DEVMODEW **new_modes, UINT *mode_count ) +static BOOL xrandr14_get_modes( x11drv_settings_id id, DWORD flags, DEVMODEW **new_modes, UINT *mode_count, BOOL full ) { DWORD frequency, orientation, orientation_count; XRRScreenResources *screen_resources; @@ -1415,7 +1415,7 @@ static BOOL xrandr14_get_modes( x11drv_settings_id id, DWORD flags, DEVMODEW **n if (!modes) goto done; - for (i = 0; i < output_info->nmode; ++i) + for (i = 0, mode = modes; i < output_info->nmode; ++i) { for (j = 0; j < screen_resources->nmode; ++j) { @@ -1432,8 +1432,8 @@ static BOOL xrandr14_get_modes( x11drv_settings_id id, DWORD flags, DEVMODEW **n if (!((1 << orientation) & rotations)) continue; - mode = (DEVMODEW *)((BYTE *)modes + (sizeof(*modes) + sizeof(RRMode)) * mode_idx); - add_xrandr14_mode( mode, mode_info, depths[depth_idx], frequency, orientation ); + add_xrandr14_mode( mode, mode_info, depths[depth_idx], frequency, orientation, full ); + mode = NEXT_DEVMODEW( mode ); ++mode_idx; } } diff --git a/dlls/winex11.drv/xvidmode.c b/dlls/winex11.drv/xvidmode.c index fd9b1e7e8b1..f5e49409080 100644 --- a/dlls/winex11.drv/xvidmode.c +++ b/dlls/winex11.drv/xvidmode.c @@ -91,10 +91,10 @@ static BOOL xf86vm_get_id(const WCHAR *device_name, BOOL is_primary, x11drv_sett return TRUE; } -static void add_xf86vm_mode(DEVMODEW *mode, DWORD depth, const XF86VidModeModeInfo *mode_info) +static void add_xf86vm_mode( DEVMODEW *mode, DWORD depth, const XF86VidModeModeInfo *mode_info, BOOL full ) { mode->dmSize = sizeof(*mode); - mode->dmDriverExtra = sizeof(mode_info); + mode->dmDriverExtra = full ? sizeof(mode_info) : 0; mode->dmFields = DM_DISPLAYORIENTATION | DM_BITSPERPEL | DM_PELSWIDTH | DM_PELSHEIGHT | DM_DISPLAYFLAGS; if (mode_info->htotal && mode_info->vtotal) { @@ -106,10 +106,10 @@ static void add_xf86vm_mode(DEVMODEW *mode, DWORD depth, const XF86VidModeModeIn mode->dmPelsWidth = mode_info->hdisplay; mode->dmPelsHeight = mode_info->vdisplay; mode->dmDisplayFlags = 0; - memcpy((BYTE *)mode + sizeof(*mode), &mode_info, sizeof(mode_info)); + if (full) memcpy( mode + 1, &mode_info, sizeof(mode_info) ); } -static BOOL xf86vm_get_modes(x11drv_settings_id id, DWORD flags, DEVMODEW **new_modes, UINT *mode_count) +static BOOL xf86vm_get_modes( x11drv_settings_id id, DWORD flags, DEVMODEW **new_modes, UINT *mode_count, BOOL full ) { INT xf86vm_mode_idx, xf86vm_mode_count; XF86VidModeModeInfo **xf86vm_modes; @@ -139,12 +139,12 @@ static BOOL xf86vm_get_modes(x11drv_settings_id id, DWORD flags, DEVMODEW **new_ memcpy(ptr, &xf86vm_modes, sizeof(xf86vm_modes)); modes = (DEVMODEW *)(ptr + sizeof(xf86vm_modes)); - for (depth_idx = 0; depth_idx < DEPTH_COUNT; ++depth_idx) + for (depth_idx = 0, mode = modes; depth_idx < DEPTH_COUNT; ++depth_idx) { for (xf86vm_mode_idx = 0; xf86vm_mode_idx < xf86vm_mode_count; ++xf86vm_mode_idx) { - mode = (DEVMODEW *)((BYTE *)modes + (sizeof(DEVMODEW) + sizeof(XF86VidModeModeInfo *)) * mode_idx++); - add_xf86vm_mode(mode, depths[depth_idx], xf86vm_modes[xf86vm_mode_idx]); + add_xf86vm_mode( mode, depths[depth_idx], xf86vm_modes[xf86vm_mode_idx], full ); + mode = NEXT_DEVMODEW( mode ); } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5579
From: Rémi Bernon <rbernon(a)codeweavers.com> --- dlls/win32u/sysparams.c | 53 ++++++++++++++++++++++------------------- 1 file changed, 28 insertions(+), 25 deletions(-) diff --git a/dlls/win32u/sysparams.c b/dlls/win32u/sysparams.c index e66ad488486..3211d4e7d8b 100644 --- a/dlls/win32u/sysparams.c +++ b/dlls/win32u/sysparams.c @@ -67,6 +67,7 @@ static const WCHAR device_descW[] = {'D','e','v','i','c','e','D','e','s','c',0}; static const WCHAR driver_descW[] = {'D','r','i','v','e','r','D','e','s','c',0}; static const WCHAR yesW[] = {'Y','e','s',0}; static const WCHAR noW[] = {'N','o',0}; +static const WCHAR modesW[] = {'M','o','d','e','s',0}; static const WCHAR mode_countW[] = {'M','o','d','e','C','o','u','n','t',0}; static const char guid_devclass_displayA[] = "{4D36E968-E325-11CE-BFC1-08002BE10318}"; @@ -431,10 +432,13 @@ static const char *debugstr_devmodew( const DEVMODEW *devmode ) static BOOL write_source_mode( HKEY hkey, UINT index, const DEVMODEW *mode ) { WCHAR bufferW[MAX_PATH] = {0}; - char buffer[MAX_PATH]; - snprintf( buffer, sizeof(buffer), "Modes\\%08X", index ); - asciiz_to_unicode( bufferW, buffer ); + assert( index == ENUM_CURRENT_SETTINGS || index == ENUM_REGISTRY_SETTINGS ); + + if (index == ENUM_CURRENT_SETTINGS) asciiz_to_unicode( bufferW, "Current" ); + else if (index == ENUM_REGISTRY_SETTINGS) asciiz_to_unicode( bufferW, "Registry" ); + else return FALSE; + return set_reg_value( hkey, bufferW, REG_BINARY, &mode->dmFields, sizeof(*mode) - offsetof(DEVMODEW, dmFields) ); } @@ -442,11 +446,15 @@ static BOOL read_source_mode( HKEY hkey, UINT index, DEVMODEW *mode ) { char value_buf[offsetof(KEY_VALUE_PARTIAL_INFORMATION, Data[sizeof(*mode)])]; KEY_VALUE_PARTIAL_INFORMATION *value = (void *)value_buf; - char buffer[MAX_PATH]; + const char *key; - snprintf( buffer, sizeof(buffer), "Modes\\%08X", index ); - if (!query_reg_ascii_value( hkey, buffer, value, sizeof(value_buf) )) return FALSE; + assert( index == ENUM_CURRENT_SETTINGS || index == ENUM_REGISTRY_SETTINGS ); + if (index == ENUM_CURRENT_SETTINGS) key = "Current"; + else if (index == ENUM_REGISTRY_SETTINGS) key = "Registry"; + else return FALSE; + + if (!query_reg_ascii_value( hkey, key, value, sizeof(value_buf) )) return FALSE; memcpy( &mode->dmFields, value->Data, sizeof(*mode) - offsetof(DEVMODEW, dmFields) ); return TRUE; } @@ -620,7 +628,6 @@ static BOOL reade_source_from_registry( unsigned int index, struct source *sourc char buffer[4096]; KEY_VALUE_PARTIAL_INFORMATION *value = (void *)buffer; WCHAR *value_str = (WCHAR *)value->Data; - DEVMODEW *mode; DWORD i, size; HKEY hkey; @@ -646,19 +653,18 @@ static BOOL reade_source_from_registry( unsigned int index, struct source *sourc if (query_reg_ascii_value( hkey, "ModeCount", value, sizeof(buffer) ) && value->Type == REG_DWORD) source->mode_count = *(const DWORD *)value->Data; - /* Modes, allocate an extra mode for easier iteration */ - if ((source->modes = calloc( source->mode_count + 1, sizeof(DEVMODEW) ))) + /* Modes */ + size = offsetof( KEY_VALUE_PARTIAL_INFORMATION, Data[(source->mode_count + 1) * sizeof(*source->modes)] ); + if (!(value = malloc( size )) || !query_reg_ascii_value( hkey, "Modes", value, size )) free( value ); + else { - for (i = 0, mode = source->modes; i < source->mode_count; i++) - { - mode->dmSize = offsetof(DEVMODEW, dmICMMethod); - if (!read_source_mode( hkey, i, mode )) break; - mode = NEXT_DEVMODEW(mode); - } - source->mode_count = i; - - qsort(source->modes, source->mode_count, sizeof(*source->modes) + source->modes->dmDriverExtra, mode_compare); + source->modes = (DEVMODEW *)value; + source->mode_count = value->DataLength / sizeof(*source->modes); + memmove( source->modes, value->Data, value->DataLength ); + memset( source->modes + source->mode_count, 0, sizeof(*source->modes) ); /* extra empty mode for easier iteration */ + qsort( source->modes, source->mode_count, sizeof(*source->modes), mode_compare ); } + value = (void *)buffer; /* DeviceID */ size = query_reg_ascii_value( hkey, "GPUID", value, sizeof(buffer) ); @@ -1451,7 +1457,6 @@ static void add_monitor( const struct gdi_monitor *gdi_monitor, void *param ) static void add_modes( const DEVMODEW *current, UINT modes_count, const DEVMODEW *modes, void *param ) { struct device_manager_ctx *ctx = param; - const DEVMODEW *mode; DEVMODEW dummy, detached = *current; TRACE( "current %s, modes_count %u, modes %p, param %p\n", debugstr_devmodew( current ), modes_count, modes, param ); @@ -1466,12 +1471,10 @@ static void add_modes( const DEVMODEW *current, UINT modes_count, const DEVMODEW write_source_mode( ctx->source_key, ENUM_REGISTRY_SETTINGS, current ); write_source_mode( ctx->source_key, ENUM_CURRENT_SETTINGS, current ); - for (mode = modes; modes_count; mode = NEXT_DEVMODEW(mode), modes_count--) - { - TRACE( "mode: %s\n", debugstr_devmodew( mode ) ); - if (write_source_mode( ctx->source_key, ctx->source.mode_count, mode )) ctx->source.mode_count++; - } - set_reg_value( ctx->source_key, mode_countW, REG_DWORD, &ctx->source.mode_count, sizeof(ctx->source.mode_count) ); + assert( !modes_count || modes->dmDriverExtra == 0 ); + set_reg_value( ctx->source_key, modesW, REG_BINARY, modes, modes_count * sizeof(*modes) ); + set_reg_value( ctx->source_key, mode_countW, REG_DWORD, &modes_count, sizeof(modes_count) ); + ctx->source.mode_count = modes_count; } static const struct gdi_device_manager device_manager = -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/5579
On Mon May 6 14:01:10 2024 +0000, Zhiyi Zhang wrote:
it’s unlikely but value leaks when the reg query fails. Fixed.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5579#note_69787
On Tue May 7 08:37:34 2024 +0000, Rémi Bernon wrote:
I don't understand? This is used to iterate in an array of DEVMODEW, the allocated array might be larger than what is actually used but that doesn't matter? NEXT_DEVMODEW will be used to move to the next devmode in the array and fill it.
static void add_xrandr14_mode( DEVMODEW *mode, XRRModeInfo *info, DWORD depth, DWORD frequency,
DWORD orientation )
DWORD orientation, BOOL full )
{
mode->dmSize = sizeof(*mode);
mode->dmDriverExtra = sizeof(RRMode);
mode->dmDriverExtra = full ? sizeof(RRMode) : 0;
``` /* Allocate space for display modes in different color depths and orientations. * Store a RRMode at the end of each DEVMODEW as private driver data */ modes = calloc( output_info->nmode * DEPTH_COUNT * orientation_count, sizeof(*modes) + sizeof(RRMode) ); ``` The problem is that dmDriverExtra is 0 when full is FALSE, yet the memory for dmDriverExtra is still allocated, which means you can't use NEXT_DEVMODEW to iterate such a DEVMODEW array. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5579#note_69791
On Tue May 7 08:37:34 2024 +0000, Zhiyi Zhang wrote:
``` static void add_xrandr14_mode( DEVMODEW *mode, XRRModeInfo *info, DWORD depth, DWORD frequency, DWORD orientation ) DWORD orientation, BOOL full ) { mode->dmSize = sizeof(*mode); mode->dmDriverExtra = sizeof(RRMode); mode->dmDriverExtra = full ? sizeof(RRMode) : 0; ``` ``` /* Allocate space for display modes in different color depths and orientations. * Store a RRMode at the end of each DEVMODEW as private driver data */ modes = calloc( output_info->nmode * DEPTH_COUNT * orientation_count, sizeof(*modes) + sizeof(RRMode) ); ``` The problem is that dmDriverExtra is 0 when full is FALSE, yet the memory for dmDriverExtra is still allocated, which means you can't use NEXT_DEVMODEW to iterate such a DEVMODEW array.
It's just a contiguous array you can very well write the elements the way you like as long as there's enough memory? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5579#note_69797
On Tue May 7 08:37:34 2024 +0000, Rémi Bernon wrote:
It's just a contiguous array you can very well write the elements the way you like as long as there's enough memory? Okay, I see, it works because you also use NEXT_DEVMODEW to write the array.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/5579#note_69798
This merge request was approved by Zhiyi Zhang. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/5579
participants (2)
-
Rémi Bernon -
Zhiyi Zhang (＠zhiyi)