[PATCH 0/5] MR665: win32u: Tweak reading / writing of DEVMODEW into the registry.
Split from https://gitlab.winehq.org/wine/wine/-/merge_requests/576, for which I'll then add modes sorting, and move support for stretched / interlaced modes from winemac to win32u. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/665
From: R��mi Bernon <rbernon(a)codeweavers.com> It's not always serializable and cannot be shared across processes. --- dlls/win32u/sysparams.c | 19 ++++--------------- 1 file changed, 4 insertions(+), 15 deletions(-) diff --git a/dlls/win32u/sysparams.c b/dlls/win32u/sysparams.c index 52665f7a641..85ffd2f69e9 100644 --- a/dlls/win32u/sysparams.c +++ b/dlls/win32u/sysparams.c @@ -178,7 +178,6 @@ static const WCHAR x_panningW[] = {'X','P','a','n','n','i','n','g',0}; static const WCHAR y_panningW[] = {'Y','P','a','n','n','i','n','g',0}; static const WCHAR orientationW[] = {'O','r','i','e','n','t','a','t','i','o','n',0}; static const WCHAR fixed_outputW[] = {'F','i','x','e','d','O','u','t','p','u','t',0}; -static const WCHAR driver_extraW[] = {'D','r','i','v','e','r','E','x','t','r','a',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}"; @@ -453,7 +452,6 @@ static BOOL write_adapter_mode( HKEY adapter_key, DWORD index, const DEVMODEW *m 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 @@ -500,11 +498,6 @@ static BOOL read_adapter_mode( HKEY adapter_key, DWORD index, DEVMODEW *mode ) #undef query_mode_field - ret = query_reg_value( hkey, driver_extraW, value, sizeof(value_buf) ) && - value->Type == REG_BINARY; - if (ret && value->DataLength <= mode->dmDriverExtra) - memcpy( mode + 1, value->Data, mode->dmDriverExtra ); - done: NtClose( hkey ); return TRUE; @@ -555,8 +548,8 @@ static BOOL read_display_adapter_settings( unsigned int index, struct adapter *i char buffer[4096]; KEY_VALUE_PARTIAL_INFORMATION *value = (void *)buffer; WCHAR *value_str = (WCHAR *)value->Data; - DWORD i, driver_extra = 0, size; DEVMODEW *mode; + DWORD i, size; HKEY hkey; if (!enum_key && !(enum_key = reg_open_key( NULL, enum_keyW, sizeof(enum_keyW) ))) @@ -596,19 +589,16 @@ static BOOL read_display_adapter_settings( unsigned int index, struct adapter *i /* Interface name */ info->dev.interface_name[0] = 0; - /* ModeCount / DriverExtra */ + /* ModeCount */ if (query_reg_value( hkey, mode_countW, value, sizeof(buffer) ) && value->Type == REG_DWORD) info->mode_count = *(const DWORD *)value->Data; - if (query_reg_value( hkey, driver_extraW, value, sizeof(buffer) ) && value->Type == REG_DWORD) - driver_extra = *(const DWORD *)value->Data; /* Modes */ - if ((info->modes = calloc( info->mode_count, sizeof(DEVMODEW) + driver_extra ))) + if ((info->modes = calloc( info->mode_count, sizeof(DEVMODEW) ))) { for (i = 0, mode = info->modes; i < info->mode_count; i++) { mode->dmSize = offsetof(DEVMODEW, dmICMMethod); - mode->dmDriverExtra = driver_extra; if (!read_adapter_mode( hkey, i, mode )) break; mode = NEXT_DEVMODEW(mode); } @@ -1349,8 +1339,7 @@ static void add_mode( const DEVMODEW *mode, void *param ) if (write_adapter_mode( ctx->adapter_key, ctx->mode_count, mode )) { - if (!ctx->mode_count++) set_reg_value( ctx->adapter_key, driver_extraW, REG_DWORD, - &mode->dmDriverExtra, sizeof(mode->dmDriverExtra) ); + ctx->mode_count++; set_reg_value( ctx->adapter_key, mode_countW, REG_DWORD, &ctx->mode_count, sizeof(ctx->mode_count) ); } } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/665
From: R��mi Bernon <rbernon(a)codeweavers.com> --- dlls/win32u/sysparams.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/dlls/win32u/sysparams.c b/dlls/win32u/sysparams.c index 85ffd2f69e9..514585a0988 100644 --- a/dlls/win32u/sysparams.c +++ b/dlls/win32u/sysparams.c @@ -2026,7 +2026,7 @@ 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( const WCHAR *adapter_path, const WCHAR *device_name, DEVMODEW *devmode, DEVMODEW *temp_mode ) { if (devmode) { @@ -2043,9 +2043,9 @@ static DEVMODEW *validate_display_settings( DEVMODEW *default_mode, DEVMODEW *cu if (!devmode) { - if (!default_mode->dmSize) return NULL; + if (!read_registry_settings( adapter_path, temp_mode )) return NULL; TRACE( "Return to original display mode\n" ); - devmode = default_mode; + devmode = temp_mode; } if ((devmode->dmFields & (DM_PELSWIDTH | DM_PELSHEIGHT)) != (DM_PELSWIDTH | DM_PELSHEIGHT)) @@ -2056,9 +2056,10 @@ static DEVMODEW *validate_display_settings( DEVMODEW *default_mode, DEVMODEW *cu if (!is_detached_mode( devmode ) && (!devmode->dmPelsWidth || !devmode->dmPelsHeight)) { - if (!current_mode->dmSize) return NULL; - if (!devmode->dmPelsWidth) devmode->dmPelsWidth = current_mode->dmPelsWidth; - if (!devmode->dmPelsHeight) devmode->dmPelsHeight = current_mode->dmPelsHeight; + DEVMODEW current_mode = {.dmSize = sizeof(DEVMODEW)}; + if (!user_driver->pGetCurrentDisplaySettings( device_name, ¤t_mode )) return NULL; + if (!devmode->dmPelsWidth) devmode->dmPelsWidth = current_mode.dmPelsWidth; + if (!devmode->dmPelsHeight) devmode->dmPelsHeight = current_mode.dmPelsHeight; } return devmode; @@ -2070,8 +2071,8 @@ 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)}; WCHAR device_name[CCHDEVICENAME], adapter_path[MAX_PATH]; + DEVMODEW temp_mode = {.dmSize = sizeof(DEVMODEW)}; LONG ret = DISP_CHANGE_SUCCESSFUL; struct adapter *adapter; @@ -2099,10 +2100,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 (!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; + if (!(devmode = validate_display_settings( adapter_path, device_name, devmode, &temp_mode ))) ret = DISP_CHANGE_BADMODE; else if (user_driver->pChangeDisplaySettingsEx( device_name, devmode, hwnd, flags | CDS_TEST, lparam )) 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; -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/665
From: R��mi Bernon <rbernon(a)codeweavers.com> So that updating dmPelsWidth / dmPelsHeight doesn't modify user devmode. --- dlls/win32u/sysparams.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/dlls/win32u/sysparams.c b/dlls/win32u/sysparams.c index 514585a0988..fb5b90af414 100644 --- a/dlls/win32u/sysparams.c +++ b/dlls/win32u/sysparams.c @@ -2041,12 +2041,13 @@ static DEVMODEW *validate_display_settings( const WCHAR *adapter_path, const WCH devmode = NULL; } - if (!devmode) + if (devmode) memcpy( temp_mode, devmode, devmode->dmSize ); + else { if (!read_registry_settings( adapter_path, temp_mode )) return NULL; TRACE( "Return to original display mode\n" ); - devmode = temp_mode; } + devmode = temp_mode; if ((devmode->dmFields & (DM_PELSWIDTH | DM_PELSHEIGHT)) != (DM_PELSWIDTH | DM_PELSHEIGHT)) { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/665
From: R��mi Bernon <rbernon(a)codeweavers.com> --- dlls/win32u/sysparams.c | 7 ++++++- dlls/winex11.drv/display.c | 10 ---------- 2 files changed, 6 insertions(+), 11 deletions(-) diff --git a/dlls/win32u/sysparams.c b/dlls/win32u/sysparams.c index fb5b90af414..edf094cd1e7 100644 --- a/dlls/win32u/sysparams.c +++ b/dlls/win32u/sysparams.c @@ -2055,12 +2055,17 @@ static DEVMODEW *validate_display_settings( const WCHAR *adapter_path, const WCH return NULL; } - if (!is_detached_mode( devmode ) && (!devmode->dmPelsWidth || !devmode->dmPelsHeight)) + if (!is_detached_mode( devmode ) && (!devmode->dmPelsWidth || !devmode->dmPelsHeight || !(devmode->dmFields & DM_POSITION))) { DEVMODEW current_mode = {.dmSize = sizeof(DEVMODEW)}; if (!user_driver->pGetCurrentDisplaySettings( device_name, ¤t_mode )) return NULL; if (!devmode->dmPelsWidth) devmode->dmPelsWidth = current_mode.dmPelsWidth; if (!devmode->dmPelsHeight) devmode->dmPelsHeight = current_mode.dmPelsHeight; + if (!(devmode->dmFields & DM_POSITION)) + { + devmode->dmPosition = current_mode.dmPosition; + devmode->dmFields |= DM_POSITION; + } } return devmode; diff --git a/dlls/winex11.drv/display.c b/dlls/winex11.drv/display.c index 32ed1bf3032..c9f2da7e163 100644 --- a/dlls/winex11.drv/display.c +++ b/dlls/winex11.drv/display.c @@ -498,16 +498,6 @@ static LONG get_display_settings(DEVMODEW **new_displays, const WCHAR *dev_name, else if (!wcsicmp(dev_name, display_device.DeviceName)) { *mode = *dev_mode; - if (!(dev_mode->dmFields & DM_POSITION)) - { - memset(¤t_mode, 0, sizeof(current_mode)); - current_mode.dmSize = sizeof(current_mode); - if (!NtUserEnumDisplaySettings( &device_name, ENUM_CURRENT_SETTINGS, ¤t_mode, 0 )) - goto done; - - mode->dmFields |= DM_POSITION; - mode->dmPosition = current_mode.dmPosition; - } } else { -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/665
From: R��mi Bernon <rbernon(a)codeweavers.com> And make dmDisplayFixedOutput / DM_DISPLAYFIXEDOUTPUT optional. --- dlls/win32u/sysparams.c | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/dlls/win32u/sysparams.c b/dlls/win32u/sysparams.c index edf094cd1e7..94c827304c3 100644 --- a/dlls/win32u/sysparams.c +++ b/dlls/win32u/sysparams.c @@ -433,12 +433,20 @@ static BOOL write_adapter_mode( HKEY adapter_key, DWORD index, const DEVMODEW *m BOOL ret; sprintf( buffer, "Modes\\%08X", index ); + reg_delete_tree( adapter_key, bufferW, asciiz_to_unicode( bufferW, buffer ) - sizeof(WCHAR) ); if (!(hkey = reg_create_key( adapter_key, bufferW, asciiz_to_unicode( bufferW, buffer ) - sizeof(WCHAR), REG_OPTION_VOLATILE, NULL ))) return FALSE; -#define set_mode_field( name, field, flag ) \ - if (!(ret = set_reg_value( hkey, (name), REG_DWORD, &mode->field, sizeof(mode->field) ))) goto done; + +#define set_mode_field( name, field, flag ) \ + do \ + { \ + if (!(mode->dmFields & (flag))) ret = (index == ENUM_REGISTRY_SETTINGS); \ + else ret = set_reg_value( hkey, (name), REG_DWORD, &mode->field, \ + sizeof(mode->field) ); \ + if (!ret) goto done; \ + } while (0) set_mode_field( bits_per_pelW, dmBitsPerPel, DM_BITSPERPEL ); set_mode_field( x_resolutionW, dmPelsWidth, DM_PELSWIDTH ); @@ -446,7 +454,7 @@ static BOOL write_adapter_mode( HKEY adapter_key, DWORD index, const DEVMODEW *m set_mode_field( v_refreshW, dmDisplayFrequency, DM_DISPLAYFREQUENCY ); set_mode_field( flagsW, dmDisplayFlags, DM_DISPLAYFLAGS ); set_mode_field( orientationW, dmDisplayOrientation, DM_DISPLAYORIENTATION ); - set_mode_field( fixed_outputW, dmDisplayFixedOutput, DM_DISPLAYFIXEDOUTPUT ); + if (mode->dmFields & DM_DISPLAYFIXEDOUTPUT) set_mode_field( fixed_outputW, dmDisplayFixedOutput, DM_DISPLAYFIXEDOUTPUT ); if (index == ENUM_CURRENT_SETTINGS || index == ENUM_REGISTRY_SETTINGS) { set_mode_field( x_panningW, dmPosition.x, DM_POSITION ); @@ -467,7 +475,6 @@ static BOOL read_adapter_mode( HKEY adapter_key, DWORD index, DEVMODEW *mode ) WCHAR bufferW[MAX_PATH]; char buffer[MAX_PATH]; HKEY hkey; - BOOL ret; sprintf( buffer, "Modes\\%08X", index ); if (!(hkey = reg_open_key( adapter_key, bufferW, asciiz_to_unicode( bufferW, buffer ) - sizeof(WCHAR) ))) @@ -476,11 +483,12 @@ static BOOL read_adapter_mode( HKEY adapter_key, DWORD index, DEVMODEW *mode ) #define query_mode_field( name, field, flag ) \ do \ { \ - ret = query_reg_value( hkey, (name), value, sizeof(value_buf) ) && \ - value->Type == REG_DWORD; \ - if (!ret) goto done; \ - mode->field = *(const DWORD *)value->Data; \ - mode->dmFields |= (flag); \ + if (query_reg_value( hkey, (name), value, sizeof(value_buf) ) && \ + value->Type == REG_DWORD) \ + { \ + mode->field = *(const DWORD *)value->Data; \ + mode->dmFields |= (flag); \ + } \ } while (0) query_mode_field( bits_per_pelW, dmBitsPerPel, DM_BITSPERPEL ); @@ -488,17 +496,16 @@ 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( orientationW, dmDisplayOrientation, DM_DISPLAYORIENTATION ); + query_mode_field( fixed_outputW, dmDisplayFixedOutput, DM_DISPLAYFIXEDOUTPUT ); 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 ); #undef query_mode_field -done: NtClose( hkey ); return TRUE; } -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/665
Zhiyi Zhang (@zhiyi) commented about dlls/win32u/sysparams.c:
BOOL ret;
sprintf( buffer, "Modes\\%08X", index ); + reg_delete_tree( adapter_key, bufferW, asciiz_to_unicode( bufferW, buffer ) - sizeof(WCHAR) ); if (!(hkey = reg_create_key( adapter_key, bufferW, asciiz_to_unicode( bufferW, buffer ) - sizeof(WCHAR), REG_OPTION_VOLATILE, NULL ))) return FALSE;
-#define set_mode_field( name, field, flag ) \ - if (!(ret = set_reg_value( hkey, (name), REG_DWORD, &mode->field, sizeof(mode->field) ))) goto done; + +#define set_mode_field( name, field, flag ) \ + do \ + { \ + if (!(mode->dmFields & (flag))) ret = (index == ENUM_REGISTRY_SETTINGS); \
Why is ENUM_REGISTRY_SETTINGS getting special treatment? The display mode written into the registry should be a full display mode. Or, are you doing a check for the required flags? If that's case, I don't think it's the responsibility of the macro. What I had in mind for the macro is something like the following and it should be enough. ` if (mode->dmFields & flag) if (!(ret = set_reg_value( hkey, (name), REG_DWORD, &mode->field, sizeof(mode->field) ))) goto done; ` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/665#note_6447
Zhiyi Zhang (@zhiyi) commented about dlls/win32u/sysparams.c:
set_mode_field( v_refreshW, dmDisplayFrequency, DM_DISPLAYFREQUENCY ); set_mode_field( flagsW, dmDisplayFlags, DM_DISPLAYFLAGS ); set_mode_field( orientationW, dmDisplayOrientation, DM_DISPLAYORIENTATION ); - set_mode_field( fixed_outputW, dmDisplayFixedOutput, DM_DISPLAYFIXEDOUTPUT ); + if (mode->dmFields & DM_DISPLAYFIXEDOUTPUT) set_mode_field( fixed_outputW, dmDisplayFixedOutput, DM_DISPLAYFIXEDOUTPUT );
You can move the flag check inside the macro. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/665#note_6448
Zhiyi Zhang (@zhiyi) commented about dlls/win32u/sysparams.c:
set_mode_field( v_refreshW, dmDisplayFrequency, DM_DISPLAYFREQUENCY ); set_mode_field( flagsW, dmDisplayFlags, DM_DISPLAYFLAGS ); set_mode_field( orientationW, dmDisplayOrientation, DM_DISPLAYORIENTATION ); - set_mode_field( fixed_outputW, dmDisplayFixedOutput, DM_DISPLAYFIXEDOUTPUT ); + if (mode->dmFields & DM_DISPLAYFIXEDOUTPUT) set_mode_field( fixed_outputW, dmDisplayFixedOutput, DM_DISPLAYFIXEDOUTPUT ); if (index == ENUM_CURRENT_SETTINGS || index == ENUM_REGISTRY_SETTINGS)
Then you can also remove this check because available modes don't have DM_POSITION set. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/665#note_6449
On Thu Aug 18 03:55:07 2022 +0000, Zhiyi Zhang wrote:
Why is ENUM_REGISTRY_SETTINGS getting special treatment? The display mode written into the registry should be a full display mode. Or, are you doing a check for the required flags? If that's case, I don't think it's the responsibility of the macro. What I had in mind for the macro is something like the following and it should be enough. ` if (mode->dmFields & flag) if (!(ret = set_reg_value( hkey, (name), REG_DWORD, &mode->field, sizeof(mode->field) ))) goto done; ` I was seeing some failures with missing DM_BITSPERPEL field, I'll have a better look.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/665#note_6463
On Thu Aug 18 07:32:48 2022 +0000, R��mi Bernon wrote:
I was seeing some failures with missing DM_BITSPERPEL field, I'll have a better look. It happens when writing a detached mode to the registry index, it should be fine I guess.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/665#note_6466
participants (2)
-
Rémi Bernon -
Zhiyi Zhang (＠zhiyi)