| Message ID | cover.1711015462.git.jani.nikula@intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | drm/edid & drm/i915: vendor and product id logging improvements | expand |
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. 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(-)
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`? (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
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
On 04/08, Jani Nikula wrote: > 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? I have no strong opinion, I was only thinking that two parts would make this `edid_buf->prod_code[0] | edid_buf->prod_code[1] << 8` operation more intuitive. As you mentioned the currrent product_code format fits specs better, I understand we can get the same result on amd with: le16_to_cpu() --> split --> second part shift. Anyway, it's certainly not a blocker. The series LGTM. Acked-by: Melissa Wen <mwen@igalia.com> > > 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
On Mon, 08 Apr 2024, Melissa Wen <mwen@igalia.com> wrote: > On 04/08, Jani Nikula wrote: >> 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? > > I have no strong opinion, I was only thinking that two parts would make > this `edid_buf->prod_code[0] | edid_buf->prod_code[1] << 8` operation > more intuitive. > > As you mentioned the currrent product_code format fits specs better, I > understand we can get the same result on amd with: > le16_to_cpu() --> split --> second part shift. Wait, what? No. le16_to_cpu() directly gives you what you want. That's the whole point. No splits, no shifts, no OR-ing. One macro that forces the right byte order for you, as the member is defined __le16. > Anyway, it's certainly not a blocker. The series LGTM. > > Acked-by: Melissa Wen <mwen@igalia.com> > >> >> This is how you'd use it: >> >> edid_caps->product_id = le16_to_cpu(id->product_code); This is intended to be an example how to deal with your URL 1 above. BR, Jani. >> >> 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
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() 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(-)