From: Zhiyi Zhang <zzhang@codeweavers.com>
Subject: Re: [PATCH] winex11.drv: Derive the monitor device name from the EDID where available.
Message-Id: <e9ec9c86-f269-f5cb-3a53-aa813427fd34@codeweavers.com>
Date: Mon, 6 Sep 2021 18:20:54 +0800
In-Reply-To: <20210903143239.86549-1-epermyakov@codeweavers.com>
References: <20210903143239.86549-1-epermyakov@codeweavers.com>

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@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.