[PATCH] winex11.drv: Derive the monitor device name from the EDID where available.

[Zhiyi Zhang]

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 at 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