Re: [PATCH 0/4] drm/edid & drm/i915: vendor and product id logging improvements

[Jani Nikula <jani.nikula@intel.com>]

On Mon, 08 Apr 2024, Melissa Wen <mwen@igalia.com> wrote:
> On 04/02, Jani Nikula wrote:
>> On Thu, 21 Mar 2024, Jani Nikula <jani.nikula@intel.com> wrote:
>> > Jani Nikula (4):
>> >   drm/edid: add drm_edid_get_product_id()
>> >   drm/edid: add drm_edid_print_product_id()
>> >   drm/i915/bios: switch to struct drm_edid and struct
>> >     drm_edid_product_id
>> >   drm/i915/bios: return drm_edid_product_id from get_lvds_pnp_id()
>> 
>> Ping for reviews please? This should be helpful in eradicating one class
>> of drm_edid_raw() uses.
>
> Hi Jani,
>
> I took a look at the series. AFAIU your solution with
> `drm_edid_product_id` mostly fits AMD display driver needs, except that
> it needs the `product_code` split into two parts (like manufacturer
> name) because the driver handles prod_code parts to configure a register
> for audio, as in the path below:
>
> 1. https://gitlab.freedesktop.org/drm/kernel/-/blob/drm-next/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c#L113
> 2. https://gitlab.freedesktop.org/drm/kernel/-/blob/drm-next/drivers/gpu/drm/amd/display/dc/core/dc_stream.c#L90
> 3. https://gitlab.freedesktop.org/drm/kernel/-/blob/drm-next/drivers/gpu/drm/amd/display/dc/dce/dce_audio.c#L873
>
> What do you think on keeping `prod_code` split into two part in
> `drm_edid_product_id`?

I think having it as "__le16 product_code" is better and
self-documenting. This is what the spec says it is, so why split it to
two parts where you always need to wonder about the byte order?

This is how you'd use it:

	edid_caps->product_id = le16_to_cpu(id->product_code);

BR,
Jani.

>
> (cc'ing some AMD devs that might have a better understanding of this use case)
>
> Thanks a lot for addressing this pending issue!
>
> Melissa
>
>> 
>> BR,
>> Jani.
>> 
>> 
>> >
>> >  drivers/gpu/drm/drm_edid.c                | 50 +++++++++++++++++++++++
>> >  drivers/gpu/drm/i915/display/intel_bios.c | 49 ++++++++++------------
>> >  include/drm/drm_edid.h                    | 28 ++++++++++---
>> >  3 files changed, 94 insertions(+), 33 deletions(-)
>> 
>> -- 
>> Jani Nikula, Intel

-- 
Jani Nikula, Intel