[PATCH] winex11.drv: Derive the monitor device name from the EDID where available.
Signed-off-by: Eduard Permyakov <epermyakov(a)codeweavers.com> --- dlls/winex11.drv/xrandr.c | 43 +++++++++++++++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 4 deletions(-) diff --git a/dlls/winex11.drv/xrandr.c b/dlls/winex11.drv/xrandr.c index eb2f7021ba6..3ae1e845ed3 100644 --- a/dlls/winex11.drv/xrandr.c +++ b/dlls/winex11.drv/xrandr.c @@ -499,6 +499,43 @@ static void get_edid(RROutput output, unsigned char **prop, unsigned long *len) } } +static BOOL edid_parse_name(const unsigned char *edid, unsigned long edid_len, WCHAR name[14]) +{ + BOOL found = FALSE; + enum { DESC_OFFSET = 0x48, DESC_SIZE = 18, NDESCS = 3, MAXLEN = 13 }; + static unsigned char disp_product_name_hdr[5] = {0x00, 0x00, 0x00, 0xFC, 0x00}; + const unsigned char *cursor = edid + DESC_OFFSET; + int i, name_idx = 0; + + if (edid_len < 128) + return FALSE; + + for (i = 0; i < NDESCS; i++, cursor += DESC_SIZE) + { + if (memcmp( cursor, disp_product_name_hdr, sizeof(disp_product_name_hdr) ) != 0) + continue; + + cursor += sizeof(disp_product_name_hdr); + while (*cursor != 0x0A && name_idx < MAXLEN) + name[name_idx++] = *cursor++; + name[name_idx++] = '\0'; + + found = TRUE; + break; + } + return found; +} + +static void monitor_set_name(struct x11drv_monitor *monitor, int index, const WCHAR *default_name) +{ + WCHAR name[14]; + if (monitor->edid && edid_parse_name(monitor->edid, monitor->edid_len, name)) + lstrcpyW( monitor->name, name ); + else + lstrcpyW( monitor->name, default_name ); + TRACE("set name of monitor %d: %s\n", index, debugstr_w(monitor->name)); +} + static void set_screen_size( int width, int height ) { int screen = default_visual.screen; @@ -1067,9 +1104,9 @@ static BOOL xrandr14_get_monitors( ULONG_PTR adapter_id, struct x11drv_monitor * /* Inactive but attached monitor, no need to check for mirrored/replica monitors */ if (!output_info->crtc || !crtc_info->mode) { - lstrcpyW( monitors[monitor_count].name, generic_nonpnp_monitorW ); monitors[monitor_count].state_flags = DISPLAY_DEVICE_ATTACHED; get_edid( adapter_id, &monitors[monitor_count].edid, &monitors[monitor_count].edid_len ); + monitor_set_name( monitors + monitor_count, monitor_count, generic_nonpnp_monitorW ); monitor_count = 1; } /* Active monitors, need to find other monitors with the same coordinates as mirrored */ @@ -1112,9 +1149,6 @@ static BOOL xrandr14_get_monitors( ULONG_PTR adapter_id, struct x11drv_monitor * enum_crtc_info->width == crtc_info->width && enum_crtc_info->height == crtc_info->height) { - /* FIXME: Read output EDID property and parse the data to get the correct name */ - lstrcpyW( monitors[monitor_count].name, generic_nonpnp_monitorW ); - SetRect( &monitors[monitor_count].rc_monitor, crtc_info->x, crtc_info->y, crtc_info->x + crtc_info->width, crtc_info->y + crtc_info->height ); monitors[monitor_count].rc_work = get_work_area( &monitors[monitor_count].rc_monitor ); @@ -1128,6 +1162,7 @@ static BOOL xrandr14_get_monitors( ULONG_PTR adapter_id, struct x11drv_monitor * get_edid( screen_resources->outputs[i], &monitors[monitor_count].edid, &monitors[monitor_count].edid_len ); + monitor_set_name( monitors + monitor_count, monitor_count, generic_nonpnp_monitorW ); monitor_count++; } -- 2.25.1
Hi, While running your changed tests, I think I found new failures. Being a bot and all I'm not very good at pattern recognition, so I might be wrong, but could you please double-check? Full results can be found at: https://testbot.winehq.org/JobDetails.pl?Key=97312 Your paranoid android. === debiant2 (build log) === error: patch failed: dlls/winex11.drv/xrandr.c:1067 Task: Patch failed to apply === debiant2 (build log) === error: patch failed: dlls/winex11.drv/xrandr.c:1067 Task: Patch failed to apply
You need to put this patch in the same series for it to apply. On 9/3/21 10:32 PM, Eduard Permyakov wrote:
Signed-off-by: Eduard Permyakov <epermyakov(a)codeweavers.com> --- dlls/winex11.drv/xrandr.c | 43 +++++++++++++++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 4 deletions(-)
diff --git a/dlls/winex11.drv/xrandr.c b/dlls/winex11.drv/xrandr.c index eb2f7021ba6..3ae1e845ed3 100644 --- a/dlls/winex11.drv/xrandr.c +++ b/dlls/winex11.drv/xrandr.c @@ -499,6 +499,43 @@ static void get_edid(RROutput output, unsigned char **prop, unsigned long *len) } }
+static BOOL edid_parse_name(const unsigned char *edid, unsigned long edid_len, WCHAR name[14]) +{ + BOOL found = FALSE; + enum { DESC_OFFSET = 0x48, DESC_SIZE = 18, NDESCS = 3, MAXLEN = 13 }; You can just use normal variable names here instead of an enum;
+ static unsigned char disp_product_name_hdr[5] = {0x00, 0x00, 0x00, 0xFC, 0x00};
static const
+ const unsigned char *cursor = edid + DESC_OFFSET; + int i, name_idx = 0; + + if (edid_len < 128) + return FALSE; + + for (i = 0; i < NDESCS; i++, cursor += DESC_SIZE) + { + if (memcmp( cursor, disp_product_name_hdr, sizeof(disp_product_name_hdr) ) != 0) + continue; + + cursor += sizeof(disp_product_name_hdr); + while (*cursor != 0x0A && name_idx < MAXLEN) + name[name_idx++] = *cursor++; + name[name_idx++] = '\0'; + + found = TRUE; + break; You can return directly.
+ } + return found; +} + +static void monitor_set_name(struct x11drv_monitor *monitor, int index, const WCHAR *default_name) +{ You don't need default_name. generic_nonpnp_monitorW is the default name, you can move generic_nonpnp_monitorW to this function and remove default_name. get_monitor_name() seems better.
+ WCHAR name[14]; You can pass monitor->name directly to edid_parse_name to avoid copying.
+ if (monitor->edid && edid_parse_name(monitor->edid, monitor->edid_len, name)) + lstrcpyW( monitor->name, name ); + else + lstrcpyW( monitor->name, default_name ); + TRACE("set name of monitor %d: %s\n", index, debugstr_w(monitor->name)); +} + static void set_screen_size( int width, int height ) { int screen = default_visual.screen; @@ -1067,9 +1104,9 @@ static BOOL xrandr14_get_monitors( ULONG_PTR adapter_id, struct x11drv_monitor * /* Inactive but attached monitor, no need to check for mirrored/replica monitors */ if (!output_info->crtc || !crtc_info->mode) { - lstrcpyW( monitors[monitor_count].name, generic_nonpnp_monitorW ); monitors[monitor_count].state_flags = DISPLAY_DEVICE_ATTACHED; get_edid( adapter_id, &monitors[monitor_count].edid, &monitors[monitor_count].edid_len ); + monitor_set_name( monitors + monitor_count, monitor_count, generic_nonpnp_monitorW ); monitor_count = 1; } /* Active monitors, need to find other monitors with the same coordinates as mirrored */ @@ -1112,9 +1149,6 @@ static BOOL xrandr14_get_monitors( ULONG_PTR adapter_id, struct x11drv_monitor * enum_crtc_info->width == crtc_info->width && enum_crtc_info->height == crtc_info->height) { - /* FIXME: Read output EDID property and parse the data to get the correct name */ - lstrcpyW( monitors[monitor_count].name, generic_nonpnp_monitorW ); - SetRect( &monitors[monitor_count].rc_monitor, crtc_info->x, crtc_info->y, crtc_info->x + crtc_info->width, crtc_info->y + crtc_info->height ); monitors[monitor_count].rc_work = get_work_area( &monitors[monitor_count].rc_monitor ); @@ -1128,6 +1162,7 @@ static BOOL xrandr14_get_monitors( ULONG_PTR adapter_id, struct x11drv_monitor *
get_edid( screen_resources->outputs[i], &monitors[monitor_count].edid, &monitors[monitor_count].edid_len ); + monitor_set_name( monitors + monitor_count, monitor_count, generic_nonpnp_monitorW ); monitor_count++; }
I just checked on Windows and there seems to be some complications here. EnumDisplayDevice() still report a "Generic Non-PnP Monitor" for me; On my machine, the monitor EDID name "LG Ultra HD" is used for DISPLAYCONFIG_DEVICE_INFO_GET_TARGET_NAME. So either this patch needs more work to see whether DisplayConfigGetDeviceInfo() parses EDID directly, or get stored the monitor EDID name from somewhere, or we can leave this patch for now.
On 2021-09-06 1:20 p.m., Zhiyi Zhang wrote:
You need to put this patch in the same series for it to apply.
On 9/3/21 10:32 PM, Eduard Permyakov wrote:
Signed-off-by: Eduard Permyakov <epermyakov(a)codeweavers.com> --- dlls/winex11.drv/xrandr.c | 43 +++++++++++++++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 4 deletions(-)
diff --git a/dlls/winex11.drv/xrandr.c b/dlls/winex11.drv/xrandr.c index eb2f7021ba6..3ae1e845ed3 100644 --- a/dlls/winex11.drv/xrandr.c +++ b/dlls/winex11.drv/xrandr.c @@ -499,6 +499,43 @@ static void get_edid(RROutput output, unsigned char **prop, unsigned long *len) } }
+static BOOL edid_parse_name(const unsigned char *edid, unsigned long edid_len, WCHAR name[14]) +{ + BOOL found = FALSE; + enum { DESC_OFFSET = 0x48, DESC_SIZE = 18, NDESCS = 3, MAXLEN = 13 }; You can just use normal variable names here instead of an enum;
+ static unsigned char disp_product_name_hdr[5] = {0x00, 0x00, 0x00, 0xFC, 0x00}; static const
+ const unsigned char *cursor = edid + DESC_OFFSET; + int i, name_idx = 0; + + if (edid_len < 128) + return FALSE; + + for (i = 0; i < NDESCS; i++, cursor += DESC_SIZE) + { + if (memcmp( cursor, disp_product_name_hdr, sizeof(disp_product_name_hdr) ) != 0) + continue; + + cursor += sizeof(disp_product_name_hdr); + while (*cursor != 0x0A && name_idx < MAXLEN) + name[name_idx++] = *cursor++; + name[name_idx++] = '\0'; + + found = TRUE; + break; You can return directly.
+ } + return found; +} + +static void monitor_set_name(struct x11drv_monitor *monitor, int index, const WCHAR *default_name) +{ You don't need default_name. generic_nonpnp_monitorW is the default name, you can move generic_nonpnp_monitorW to this function and remove default_name. get_monitor_name() seems better.
+ WCHAR name[14]; You can pass monitor->name directly to edid_parse_name to avoid copying.
+ if (monitor->edid && edid_parse_name(monitor->edid, monitor->edid_len, name)) + lstrcpyW( monitor->name, name ); + else + lstrcpyW( monitor->name, default_name ); + TRACE("set name of monitor %d: %s\n", index, debugstr_w(monitor->name)); +} + static void set_screen_size( int width, int height ) { int screen = default_visual.screen; @@ -1067,9 +1104,9 @@ static BOOL xrandr14_get_monitors( ULONG_PTR adapter_id, struct x11drv_monitor * /* Inactive but attached monitor, no need to check for mirrored/replica monitors */ if (!output_info->crtc || !crtc_info->mode) { - lstrcpyW( monitors[monitor_count].name, generic_nonpnp_monitorW ); monitors[monitor_count].state_flags = DISPLAY_DEVICE_ATTACHED; get_edid( adapter_id, &monitors[monitor_count].edid, &monitors[monitor_count].edid_len ); + monitor_set_name( monitors + monitor_count, monitor_count, generic_nonpnp_monitorW ); monitor_count = 1; } /* Active monitors, need to find other monitors with the same coordinates as mirrored */ @@ -1112,9 +1149,6 @@ static BOOL xrandr14_get_monitors( ULONG_PTR adapter_id, struct x11drv_monitor * enum_crtc_info->width == crtc_info->width && enum_crtc_info->height == crtc_info->height) { - /* FIXME: Read output EDID property and parse the data to get the correct name */ - lstrcpyW( monitors[monitor_count].name, generic_nonpnp_monitorW ); - SetRect( &monitors[monitor_count].rc_monitor, crtc_info->x, crtc_info->y, crtc_info->x + crtc_info->width, crtc_info->y + crtc_info->height ); monitors[monitor_count].rc_work = get_work_area( &monitors[monitor_count].rc_monitor ); @@ -1128,6 +1162,7 @@ static BOOL xrandr14_get_monitors( ULONG_PTR adapter_id, struct x11drv_monitor *
get_edid( screen_resources->outputs[i], &monitors[monitor_count].edid, &monitors[monitor_count].edid_len ); + monitor_set_name( monitors + monitor_count, monitor_count, generic_nonpnp_monitorW ); monitor_count++; }
I just checked on Windows and there seems to be some complications here. EnumDisplayDevice() still report a "Generic Non-PnP Monitor" for me; On my machine, the monitor EDID name "LG Ultra HD" is used for DISPLAYCONFIG_DEVICE_INFO_GET_TARGET_NAME. So either this patch needs more work to see whether DisplayConfigGetDeviceInfo() parses EDID directly, or get stored the monitor EDID name from somewhere, or we can leave this patch for now.
Hi, Thanks for the feedback on my patches. The parsing logic here is correct. It reports: 007c:trace:xrandr:monitor_set_name set name of monitor 0: L"Generic Non-PnP Monitor" 007c:trace:xrandr:monitor_set_name set name of monitor 1: L"E2250" This is consistent with my monitors' EDIDs (The first monitor does not have a ModelName property, hence the default name is used). However, this patch does not expose the data via DisplayConfigGetDeviceInfo(). Looking at the code, the handling of the DISPLAYCONFIG_DEVICE_INFO_GET_TARGET_NAME type in this function is the following: case DISPLAYCONFIG_DEVICE_INFO_GET_TARGET_NAME: { DISPLAYCONFIG_TARGET_DEVICE_NAME *target_name = (DISPLAYCONFIG_TARGET_DEVICE_NAME *)packet; FIXME("DISPLAYCONFIG_DEVICE_INFO_GET_TARGET_NAME: stub\n"); if (packet->size < sizeof(*target_name)) return ERROR_INVALID_PARAMETER; return ERROR_NOT_SUPPORTED; } This makes sense since we did not have EDID data before. If you think it is worthwhile, I can also make a patch where I parse the EDID data and populate the DISPLAYCONFIG_TARGET_DEVICE_NAME structure. The data will come from opening and reading the 'EDID' registry key that I added in my earlier patch. Then I can send this as a separate series since this is getting outside the scope of what I originally wanted to fix.
On 9/7/21 12:32 AM, Eduard Permyakov wrote:
On 2021-09-06 1:20 p.m., Zhiyi Zhang wrote:
You need to put this patch in the same series for it to apply.
On 9/3/21 10:32 PM, Eduard Permyakov wrote:
Signed-off-by: Eduard Permyakov <epermyakov(a)codeweavers.com> --- dlls/winex11.drv/xrandr.c | 43 +++++++++++++++++++++++++++++++++++---- 1 file changed, 39 insertions(+), 4 deletions(-)
diff --git a/dlls/winex11.drv/xrandr.c b/dlls/winex11.drv/xrandr.c index eb2f7021ba6..3ae1e845ed3 100644 --- a/dlls/winex11.drv/xrandr.c +++ b/dlls/winex11.drv/xrandr.c @@ -499,6 +499,43 @@ static void get_edid(RROutput output, unsigned char **prop, unsigned long *len) } } +static BOOL edid_parse_name(const unsigned char *edid, unsigned long edid_len, WCHAR name[14]) +{ + BOOL found = FALSE; + enum { DESC_OFFSET = 0x48, DESC_SIZE = 18, NDESCS = 3, MAXLEN = 13 }; You can just use normal variable names here instead of an enum;
+ static unsigned char disp_product_name_hdr[5] = {0x00, 0x00, 0x00, 0xFC, 0x00}; static const
+ const unsigned char *cursor = edid + DESC_OFFSET; + int i, name_idx = 0; + + if (edid_len < 128) + return FALSE; + + for (i = 0; i < NDESCS; i++, cursor += DESC_SIZE) + { + if (memcmp( cursor, disp_product_name_hdr, sizeof(disp_product_name_hdr) ) != 0) + continue; + + cursor += sizeof(disp_product_name_hdr); + while (*cursor != 0x0A && name_idx < MAXLEN) + name[name_idx++] = *cursor++; + name[name_idx++] = '\0'; + + found = TRUE; + break; You can return directly.
+ } + return found; +} + +static void monitor_set_name(struct x11drv_monitor *monitor, int index, const WCHAR *default_name) +{ You don't need default_name. generic_nonpnp_monitorW is the default name, you can move generic_nonpnp_monitorW to this function and remove default_name. get_monitor_name() seems better.
+ WCHAR name[14]; You can pass monitor->name directly to edid_parse_name to avoid copying.
+ if (monitor->edid && edid_parse_name(monitor->edid, monitor->edid_len, name)) + lstrcpyW( monitor->name, name ); + else + lstrcpyW( monitor->name, default_name ); + TRACE("set name of monitor %d: %s\n", index, debugstr_w(monitor->name)); +} + static void set_screen_size( int width, int height ) { int screen = default_visual.screen; @@ -1067,9 +1104,9 @@ static BOOL xrandr14_get_monitors( ULONG_PTR adapter_id, struct x11drv_monitor * /* Inactive but attached monitor, no need to check for mirrored/replica monitors */ if (!output_info->crtc || !crtc_info->mode) { - lstrcpyW( monitors[monitor_count].name, generic_nonpnp_monitorW ); monitors[monitor_count].state_flags = DISPLAY_DEVICE_ATTACHED; get_edid( adapter_id, &monitors[monitor_count].edid, &monitors[monitor_count].edid_len ); + monitor_set_name( monitors + monitor_count, monitor_count, generic_nonpnp_monitorW ); monitor_count = 1; } /* Active monitors, need to find other monitors with the same coordinates as mirrored */ @@ -1112,9 +1149,6 @@ static BOOL xrandr14_get_monitors( ULONG_PTR adapter_id, struct x11drv_monitor * enum_crtc_info->width == crtc_info->width && enum_crtc_info->height == crtc_info->height) { - /* FIXME: Read output EDID property and parse the data to get the correct name */ - lstrcpyW( monitors[monitor_count].name, generic_nonpnp_monitorW ); - SetRect( &monitors[monitor_count].rc_monitor, crtc_info->x, crtc_info->y, crtc_info->x + crtc_info->width, crtc_info->y + crtc_info->height ); monitors[monitor_count].rc_work = get_work_area( &monitors[monitor_count].rc_monitor ); @@ -1128,6 +1162,7 @@ static BOOL xrandr14_get_monitors( ULONG_PTR adapter_id, struct x11drv_monitor * get_edid( screen_resources->outputs[i], &monitors[monitor_count].edid, &monitors[monitor_count].edid_len ); + monitor_set_name( monitors + monitor_count, monitor_count, generic_nonpnp_monitorW ); monitor_count++; } I just checked on Windows and there seems to be some complications here. EnumDisplayDevice() still report a "Generic Non-PnP Monitor" for me; On my machine, the monitor EDID name "LG Ultra HD" is used for DISPLAYCONFIG_DEVICE_INFO_GET_TARGET_NAME. So either this patch needs more work to see whether DisplayConfigGetDeviceInfo() parses EDID directly, or get stored the monitor EDID name from somewhere, or we can leave this patch for now.
Hi,
Thanks for the feedback on my patches.
The parsing logic here is correct. It reports:
007c:trace:xrandr:monitor_set_name set name of monitor 0: L"Generic Non-PnP Monitor" 007c:trace:xrandr:monitor_set_name set name of monitor 1: L"E2250"
This is consistent with my monitors' EDIDs (The first monitor does not have a ModelName property, hence the default name is used).
The problem is that on my machine, the same monitor reports "Generic Non-PnP Monitor" in EnumDisplayDevices() and "LG Ultra HD" with DISPLAYCONFIG_DEVICE_INFO_GET_TARGET_NAME on Windows. The display device handlers will change the name reported to EnumDisplayDevices() so using monitor EDID names here doesn't seem right.
However, this patch does not expose the data via DisplayConfigGetDeviceInfo(). Looking at the code, the handling of the DISPLAYCONFIG_DEVICE_INFO_GET_TARGET_NAME type in this function is the following:
case DISPLAYCONFIG_DEVICE_INFO_GET_TARGET_NAME: { DISPLAYCONFIG_TARGET_DEVICE_NAME *target_name = (DISPLAYCONFIG_TARGET_DEVICE_NAME *)packet;
FIXME("DISPLAYCONFIG_DEVICE_INFO_GET_TARGET_NAME: stub\n");
if (packet->size < sizeof(*target_name)) return ERROR_INVALID_PARAMETER;
return ERROR_NOT_SUPPORTED; }
This makes sense since we did not have EDID data before. If you think it is worthwhile, I can also make a patch where I parse the EDID data and populate the DISPLAYCONFIG_TARGET_DEVICE_NAME structure. The data will come from opening and reading the 'EDID' registry key that I added in my earlier patch. Then I can send this as a separate series since this is getting outside the scope of what I originally wanted to fix.
Yes, if you have time. I think it's worthwhile because someday multiple monitor DPI settings in winecfg will need to be implemented for supporting DPI per-monitor v2 awareness. Having a correct monitor name helps distinguishing each monitor. Parsing the EDID data sounds about right. I also wonder if the name is stored somewhere in the registry. Thanks, Zhiyi
participants (3)
-
Eduard Permyakov -
Marvin -
Zhiyi Zhang