From: Zhiyi Zhang Subject: Re: [PATCH] winex11.drv: Derive the monitor device name from the EDID where available. Message-Id: Date: Tue, 7 Sep 2021 09:46:13 +0800 In-Reply-To: <8580c10f-9409-dc22-ec7e-a4a33229d3c8@codeweavers.com> References: <20210903143239.86549-1-epermyakov@codeweavers.com> <8580c10f-9409-dc22-ec7e-a4a33229d3c8@codeweavers.com> 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 >>> --- >>>   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