From: Eduard Permyakov Subject: Re: [PATCH] winex11.drv: Derive the monitor device name from the EDID where available. Message-Id: <8580c10f-9409-dc22-ec7e-a4a33229d3c8@codeweavers.com> Date: Mon, 6 Sep 2021 19:32:02 +0300 In-Reply-To: References: <20210903143239.86549-1-epermyakov@codeweavers.com> 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 >> --- >> 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.