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

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

[Jani Nikula]

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(-)

-- 
2.39.2

[PATCH 1/4] drm/edid: add drm_edid_get_product_id()

[Jani Nikula]

Add a struct drm_edid based function to get the vendor and product ID
from an EDID. Add a separate struct for defining this part of the EDID,
with defined byte order for product code and serial number.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 15 +++++++++++++++
 include/drm/drm_edid.h     | 25 ++++++++++++++++++++-----
 2 files changed, 35 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index ea77577a3786..626a0e24e66a 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -2756,6 +2756,21 @@ const struct drm_edid *drm_edid_read(struct drm_connector *connector)
 }
 EXPORT_SYMBOL(drm_edid_read);
 
+/**
+ * drm_edid_get_product_id - Get the vendor and product identification
+ * @drm_edid: EDID
+ * @id: Where to place the product id
+ */
+void drm_edid_get_product_id(const struct drm_edid *drm_edid,
+			     struct drm_edid_product_id *id)
+{
+	if (drm_edid && drm_edid->edid && drm_edid->size >= EDID_LENGTH)
+		memcpy(id, &drm_edid->edid->product_id, sizeof(*id));
+	else
+		memset(id, 0, sizeof(*id));
+}
+EXPORT_SYMBOL(drm_edid_get_product_id);
+
 /**
  * drm_edid_get_panel_id - Get a panel's ID from EDID
  * @drm_edid: EDID that contains panel ID.
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 6f65bbf655a1..7911a2f8a672 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -272,14 +272,27 @@ struct detailed_timing {
 #define DRM_EDID_DSC_MAX_SLICES			0xf
 #define DRM_EDID_DSC_TOTAL_CHUNK_KBYTES		0x3f
 
+struct drm_edid_product_id {
+	u8 manufacturer_name[2];
+	__le16 product_code;
+	__le32 serial_number;
+	u8 week_of_manufacture;
+	u8 year_of_manufacture;
+} __packed;
+
 struct edid {
 	u8 header[8];
 	/* Vendor & product info */
-	u8 mfg_id[2];
-	u8 prod_code[2];
-	u32 serial; /* FIXME: byte order */
-	u8 mfg_week;
-	u8 mfg_year;
+	union {
+		struct drm_edid_product_id product_id;
+		struct {
+			u8 mfg_id[2];
+			u8 prod_code[2];
+			u32 serial; /* FIXME: byte order */
+			u8 mfg_week;
+			u8 mfg_year;
+		} __packed;
+	} __packed;
 	/* EDID version */
 	u8 version;
 	u8 revision;
@@ -466,6 +479,8 @@ int drm_edid_connector_update(struct drm_connector *connector,
 			      const struct drm_edid *edid);
 int drm_edid_connector_add_modes(struct drm_connector *connector);
 bool drm_edid_is_digital(const struct drm_edid *drm_edid);
+void drm_edid_get_product_id(const struct drm_edid *drm_edid,
+			     struct drm_edid_product_id *id);
 
 const u8 *drm_find_edid_extension(const struct drm_edid *drm_edid,
 				  int ext_id, int *ext_index);
-- 
2.39.2

[PATCH 2/4] drm/edid: add drm_edid_print_product_id()

[Jani Nikula]

Add a function to print a decoded EDID vendor and product id to a drm
printer, optinally with the raw data.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/drm_edid.c | 35 +++++++++++++++++++++++++++++++++++
 include/drm/drm_edid.h     |  3 +++
 2 files changed, 38 insertions(+)

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 626a0e24e66a..198986f0eb8b 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -29,6 +29,7 @@
  */
 
 #include <linux/bitfield.h>
+#include <linux/byteorder/generic.h>
 #include <linux/cec.h>
 #include <linux/hdmi.h>
 #include <linux/i2c.h>
@@ -2771,6 +2772,40 @@ void drm_edid_get_product_id(const struct drm_edid *drm_edid,
 }
 EXPORT_SYMBOL(drm_edid_get_product_id);
 
+/**
+ * drm_edid_print_product_id - Print decoded product id to printer
+ * @p: drm printer
+ * @id: EDID product id
+ * @raw: If true, also print the raw hex
+ */
+void drm_edid_print_product_id(struct drm_printer *p,
+			       const struct drm_edid_product_id *id, bool raw)
+{
+	u16 mfg_id = id->manufacturer_name[0] << 8 | id->manufacturer_name[1];
+	char *date;
+	char vend[4];
+
+	drm_edid_decode_mfg_id(mfg_id, vend);
+
+	if (id->week_of_manufacture == 0xff)
+		date = kasprintf(GFP_KERNEL, "model year: %d",
+				 id->year_of_manufacture + 1990);
+	else
+		date = kasprintf(GFP_KERNEL, "week: %d, year of manufacture: %d",
+				 id->week_of_manufacture,
+				 id->year_of_manufacture + 1990);
+
+	drm_printf(p, "manufacturer name: %s, product code: %u, serial number: %u, %s\n",
+		   vend, le16_to_cpu(id->product_code),
+		   le32_to_cpu(id->serial_number), date ?: "");
+
+	if (raw)
+		drm_printf(p, "raw product id: %*ph\n", (int)sizeof(*id), id);
+
+	kfree(date);
+}
+EXPORT_SYMBOL(drm_edid_print_product_id);
+
 /**
  * drm_edid_get_panel_id - Get a panel's ID from EDID
  * @drm_edid: EDID that contains panel ID.
diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
index 7911a2f8a672..c763ba1a0bbd 100644
--- a/include/drm/drm_edid.h
+++ b/include/drm/drm_edid.h
@@ -30,6 +30,7 @@ struct drm_connector;
 struct drm_device;
 struct drm_display_mode;
 struct drm_edid;
+struct drm_printer;
 struct hdmi_avi_infoframe;
 struct hdmi_vendor_infoframe;
 struct i2c_adapter;
@@ -481,6 +482,8 @@ int drm_edid_connector_add_modes(struct drm_connector *connector);
 bool drm_edid_is_digital(const struct drm_edid *drm_edid);
 void drm_edid_get_product_id(const struct drm_edid *drm_edid,
 			     struct drm_edid_product_id *id);
+void drm_edid_print_product_id(struct drm_printer *p,
+			       const struct drm_edid_product_id *id, bool raw);
 
 const u8 *drm_find_edid_extension(const struct drm_edid *drm_edid,
 				  int ext_id, int *ext_index);
-- 
2.39.2

[PATCH 3/4] drm/i915/bios: switch to struct drm_edid and struct drm_edid_product_id

[Jani Nikula]

To avoid accessing and parsing the raw EDID with drm_edid_raw(), switch
to the struct drm_edid based function to extract product id, and use the
drm printer function to debug log it.

The underlying assumption is that struct drm_edid_product_id and struct
lvds_pnp_id describe identical data, albeit with slightly different
member definitions.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_bios.c | 43 ++++++++++-------------
 1 file changed, 18 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
index c7841b3eede8..5e111a8cce66 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -600,6 +600,9 @@ get_lvds_pnp_id(const struct bdb_lvds_lfp_data *data,
 		const struct bdb_lvds_lfp_data_ptrs *ptrs,
 		int index)
 {
+	/* These two are supposed to have the same layout in memory. */
+	BUILD_BUG_ON(sizeof(struct lvds_pnp_id) != sizeof(struct drm_edid_product_id));
+
 	return (const void *)data + ptrs->ptr[index].panel_pnp_id.offset;
 }
 
@@ -613,19 +616,6 @@ get_lfp_data_tail(const struct bdb_lvds_lfp_data *data,
 		return NULL;
 }
 
-static void dump_pnp_id(struct drm_i915_private *i915,
-			const struct lvds_pnp_id *pnp_id,
-			const char *name)
-{
-	u16 mfg_name = be16_to_cpu((__force __be16)pnp_id->mfg_name);
-	char vend[4];
-
-	drm_dbg_kms(&i915->drm, "%s PNPID mfg: %s (0x%x), prod: %u, serial: %u, week: %d, year: %d\n",
-		    name, drm_edid_decode_mfg_id(mfg_name, vend),
-		    pnp_id->mfg_name, pnp_id->product_code, pnp_id->serial,
-		    pnp_id->mfg_week, pnp_id->mfg_year + 1990);
-}
-
 static int opregion_get_panel_type(struct drm_i915_private *i915,
 				   const struct intel_bios_encoder_data *devdata,
 				   const struct drm_edid *drm_edid, bool use_fallback)
@@ -664,21 +654,21 @@ static int pnpid_get_panel_type(struct drm_i915_private *i915,
 {
 	const struct bdb_lvds_lfp_data *data;
 	const struct bdb_lvds_lfp_data_ptrs *ptrs;
-	const struct lvds_pnp_id *edid_id;
-	struct lvds_pnp_id edid_id_nodate;
-	const struct edid *edid = drm_edid_raw(drm_edid); /* FIXME */
+	struct drm_edid_product_id product_id, product_id_nodate;
+	struct drm_printer p;
 	int i, best = -1;
 
-	if (!edid)
+	if (!drm_edid)
 		return -1;
 
-	edid_id = (const void *)&edid->mfg_id[0];
+	drm_edid_get_product_id(drm_edid, &product_id);
 
-	edid_id_nodate = *edid_id;
-	edid_id_nodate.mfg_week = 0;
-	edid_id_nodate.mfg_year = 0;
+	product_id_nodate = product_id;
+	product_id_nodate.week_of_manufacture = 0;
+	product_id_nodate.year_of_manufacture = 0;
 
-	dump_pnp_id(i915, edid_id, "EDID");
+	p = drm_dbg_printer(&i915->drm, DRM_UT_KMS, "EDID");
+	drm_edid_print_product_id(&p, &product_id, true);
 
 	ptrs = bdb_find_section(i915, BDB_LVDS_LFP_DATA_PTRS);
 	if (!ptrs)
@@ -693,7 +683,7 @@ static int pnpid_get_panel_type(struct drm_i915_private *i915,
 			get_lvds_pnp_id(data, ptrs, i);
 
 		/* full match? */
-		if (!memcmp(vbt_id, edid_id, sizeof(*vbt_id)))
+		if (!memcmp(vbt_id, &product_id, sizeof(*vbt_id)))
 			return i;
 
 		/*
@@ -701,7 +691,7 @@ static int pnpid_get_panel_type(struct drm_i915_private *i915,
 		 * and the VBT entry does not specify a date.
 		 */
 		if (best < 0 &&
-		    !memcmp(vbt_id, &edid_id_nodate, sizeof(*vbt_id)))
+		    !memcmp(vbt_id, &product_id_nodate, sizeof(*vbt_id)))
 			best = i;
 	}
 
@@ -888,6 +878,7 @@ parse_lfp_data(struct drm_i915_private *i915,
 	const struct bdb_lvds_lfp_data_tail *tail;
 	const struct bdb_lvds_lfp_data_ptrs *ptrs;
 	const struct lvds_pnp_id *pnp_id;
+	struct drm_printer p;
 	int panel_type = panel->vbt.panel_type;
 
 	ptrs = bdb_find_section(i915, BDB_LVDS_LFP_DATA_PTRS);
@@ -902,7 +893,9 @@ parse_lfp_data(struct drm_i915_private *i915,
 		parse_lfp_panel_dtd(i915, panel, data, ptrs);
 
 	pnp_id = get_lvds_pnp_id(data, ptrs, panel_type);
-	dump_pnp_id(i915, pnp_id, "Panel");
+
+	p = drm_dbg_printer(&i915->drm, DRM_UT_KMS, "Panel");
+	drm_edid_print_product_id(&p, (const struct drm_edid_product_id *)pnp_id, false);
 
 	tail = get_lfp_data_tail(data, ptrs);
 	if (!tail)
-- 
2.39.2

[PATCH 4/4] drm/i915/bios: return drm_edid_product_id from get_lvds_pnp_id()

[Jani Nikula]

Use a more suitable type to avoid the cast.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/display/intel_bios.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/display/intel_bios.c b/drivers/gpu/drm/i915/display/intel_bios.c
index 5e111a8cce66..300c1acf5de2 100644
--- a/drivers/gpu/drm/i915/display/intel_bios.c
+++ b/drivers/gpu/drm/i915/display/intel_bios.c
@@ -595,7 +595,7 @@ get_lvds_fp_timing(const struct bdb_lvds_lfp_data *data,
 	return (const void *)data + ptrs->ptr[index].fp_timing.offset;
 }
 
-static const struct lvds_pnp_id *
+static const struct drm_edid_product_id *
 get_lvds_pnp_id(const struct bdb_lvds_lfp_data *data,
 		const struct bdb_lvds_lfp_data_ptrs *ptrs,
 		int index)
@@ -679,7 +679,7 @@ static int pnpid_get_panel_type(struct drm_i915_private *i915,
 		return -1;
 
 	for (i = 0; i < 16; i++) {
-		const struct lvds_pnp_id *vbt_id =
+		const struct drm_edid_product_id *vbt_id =
 			get_lvds_pnp_id(data, ptrs, i);
 
 		/* full match? */
@@ -877,7 +877,7 @@ parse_lfp_data(struct drm_i915_private *i915,
 	const struct bdb_lvds_lfp_data *data;
 	const struct bdb_lvds_lfp_data_tail *tail;
 	const struct bdb_lvds_lfp_data_ptrs *ptrs;
-	const struct lvds_pnp_id *pnp_id;
+	const struct drm_edid_product_id *pnp_id;
 	struct drm_printer p;
 	int panel_type = panel->vbt.panel_type;
 
@@ -895,7 +895,7 @@ parse_lfp_data(struct drm_i915_private *i915,
 	pnp_id = get_lvds_pnp_id(data, ptrs, panel_type);
 
 	p = drm_dbg_printer(&i915->drm, DRM_UT_KMS, "Panel");
-	drm_edid_print_product_id(&p, (const struct drm_edid_product_id *)pnp_id, false);
+	drm_edid_print_product_id(&p, pnp_id, false);
 
 	tail = get_lfp_data_tail(data, ptrs);
 	if (!tail)
-- 
2.39.2

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

[Jani Nikula]

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(-)

-- 
Jani Nikula, Intel

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

[Melissa Wen]

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

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

[Jani Nikula]

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

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

[Melissa Wen]

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

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

[Jani Nikula]

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, Intel

Re: [PATCH 2/4] drm/edid: add drm_edid_print_product_id()

[Ville Syrjälä]

On Thu, Mar 21, 2024 at 12:05:10PM +0200, Jani Nikula wrote:
> Add a function to print a decoded EDID vendor and product id to a drm
> printer, optinally with the raw data.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 35 +++++++++++++++++++++++++++++++++++
>  include/drm/drm_edid.h     |  3 +++
>  2 files changed, 38 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index 626a0e24e66a..198986f0eb8b 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -29,6 +29,7 @@
>   */
>  
>  #include <linux/bitfield.h>
> +#include <linux/byteorder/generic.h>
>  #include <linux/cec.h>
>  #include <linux/hdmi.h>
>  #include <linux/i2c.h>
> @@ -2771,6 +2772,40 @@ void drm_edid_get_product_id(const struct drm_edid *drm_edid,
>  }
>  EXPORT_SYMBOL(drm_edid_get_product_id);
>  
> +/**
> + * drm_edid_print_product_id - Print decoded product id to printer
> + * @p: drm printer
> + * @id: EDID product id
> + * @raw: If true, also print the raw hex
> + */
> +void drm_edid_print_product_id(struct drm_printer *p,
> +			       const struct drm_edid_product_id *id, bool raw)
> +{
> +	u16 mfg_id = id->manufacturer_name[0] << 8 | id->manufacturer_name[1];
> +	char *date;
> +	char vend[4];
> +
> +	drm_edid_decode_mfg_id(mfg_id, vend);
> +
> +	if (id->week_of_manufacture == 0xff)

Didn't realize this had a loaded meaning. Maybe we should also
skip the week printout if week==0? Otherwise people might think
week==0 means the first week.

> +		date = kasprintf(GFP_KERNEL, "model year: %d",
> +				 id->year_of_manufacture + 1990);
> +	else
> +		date = kasprintf(GFP_KERNEL, "week: %d, year of manufacture: %d",

The "week: %d" part feels a bit left out here. Maybe this should be
formatted as "week/year of manufacture: %d/%d"? 

Not sure I like the kasprintf(). Maybe use an on-stack buffer?

> +				 id->week_of_manufacture,
> +				 id->year_of_manufacture + 1990);
> +
> +	drm_printf(p, "manufacturer name: %s, product code: %u, serial number: %u, %s\n",
> +		   vend, le16_to_cpu(id->product_code),
> +		   le32_to_cpu(id->serial_number), date ?: "");
> +
> +	if (raw)
> +		drm_printf(p, "raw product id: %*ph\n", (int)sizeof(*id), id);
> +
> +	kfree(date);
> +}
> +EXPORT_SYMBOL(drm_edid_print_product_id);
> +
>  /**
>   * drm_edid_get_panel_id - Get a panel's ID from EDID
>   * @drm_edid: EDID that contains panel ID.
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 7911a2f8a672..c763ba1a0bbd 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -30,6 +30,7 @@ struct drm_connector;
>  struct drm_device;
>  struct drm_display_mode;
>  struct drm_edid;
> +struct drm_printer;
>  struct hdmi_avi_infoframe;
>  struct hdmi_vendor_infoframe;
>  struct i2c_adapter;
> @@ -481,6 +482,8 @@ int drm_edid_connector_add_modes(struct drm_connector *connector);
>  bool drm_edid_is_digital(const struct drm_edid *drm_edid);
>  void drm_edid_get_product_id(const struct drm_edid *drm_edid,
>  			     struct drm_edid_product_id *id);
> +void drm_edid_print_product_id(struct drm_printer *p,
> +			       const struct drm_edid_product_id *id, bool raw);
>  
>  const u8 *drm_find_edid_extension(const struct drm_edid *drm_edid,
>  				  int ext_id, int *ext_index);
> -- 
> 2.39.2

-- 
Ville Syrjälä
Intel

Re: [PATCH 1/4] drm/edid: add drm_edid_get_product_id()

[Ville Syrjälä]

On Thu, Mar 21, 2024 at 12:05:09PM +0200, Jani Nikula wrote:
> Add a struct drm_edid based function to get the vendor and product ID
> from an EDID. Add a separate struct for defining this part of the EDID,
> with defined byte order for product code and serial number.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/drm_edid.c | 15 +++++++++++++++
>  include/drm/drm_edid.h     | 25 ++++++++++++++++++++-----
>  2 files changed, 35 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index ea77577a3786..626a0e24e66a 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -2756,6 +2756,21 @@ const struct drm_edid *drm_edid_read(struct drm_connector *connector)
>  }
>  EXPORT_SYMBOL(drm_edid_read);
>  
> +/**
> + * drm_edid_get_product_id - Get the vendor and product identification
> + * @drm_edid: EDID
> + * @id: Where to place the product id
> + */
> +void drm_edid_get_product_id(const struct drm_edid *drm_edid,
> +			     struct drm_edid_product_id *id)
> +{
> +	if (drm_edid && drm_edid->edid && drm_edid->size >= EDID_LENGTH)
> +		memcpy(id, &drm_edid->edid->product_id, sizeof(*id));
> +	else
> +		memset(id, 0, sizeof(*id));
> +}
> +EXPORT_SYMBOL(drm_edid_get_product_id);
> +
>  /**
>   * drm_edid_get_panel_id - Get a panel's ID from EDID
>   * @drm_edid: EDID that contains panel ID.
> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
> index 6f65bbf655a1..7911a2f8a672 100644
> --- a/include/drm/drm_edid.h
> +++ b/include/drm/drm_edid.h
> @@ -272,14 +272,27 @@ struct detailed_timing {
>  #define DRM_EDID_DSC_MAX_SLICES			0xf
>  #define DRM_EDID_DSC_TOTAL_CHUNK_KBYTES		0x3f
>  
> +struct drm_edid_product_id {
> +	u8 manufacturer_name[2];

__be16?

> +	__le16 product_code;
> +	__le32 serial_number;
> +	u8 week_of_manufacture;
> +	u8 year_of_manufacture;
> +} __packed;
> +
>  struct edid {
>  	u8 header[8];
>  	/* Vendor & product info */
> -	u8 mfg_id[2];
> -	u8 prod_code[2];
> -	u32 serial; /* FIXME: byte order */
> -	u8 mfg_week;
> -	u8 mfg_year;
> +	union {
> +		struct drm_edid_product_id product_id;
> +		struct {
> +			u8 mfg_id[2];
> +			u8 prod_code[2];
> +			u32 serial; /* FIXME: byte order */
> +			u8 mfg_week;
> +			u8 mfg_year;
> +		} __packed;
> +	} __packed;
>  	/* EDID version */
>  	u8 version;
>  	u8 revision;
> @@ -466,6 +479,8 @@ int drm_edid_connector_update(struct drm_connector *connector,
>  			      const struct drm_edid *edid);
>  int drm_edid_connector_add_modes(struct drm_connector *connector);
>  bool drm_edid_is_digital(const struct drm_edid *drm_edid);
> +void drm_edid_get_product_id(const struct drm_edid *drm_edid,
> +			     struct drm_edid_product_id *id);
>  
>  const u8 *drm_find_edid_extension(const struct drm_edid *drm_edid,
>  				  int ext_id, int *ext_index);
> -- 
> 2.39.2

-- 
Ville Syrjälä
Intel

Re: [PATCH 1/4] drm/edid: add drm_edid_get_product_id()

[Jani Nikula]

On Mon, 08 Apr 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Mar 21, 2024 at 12:05:09PM +0200, Jani Nikula wrote:
>> Add a struct drm_edid based function to get the vendor and product ID
>> from an EDID. Add a separate struct for defining this part of the EDID,
>> with defined byte order for product code and serial number.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/drm_edid.c | 15 +++++++++++++++
>>  include/drm/drm_edid.h     | 25 ++++++++++++++++++++-----
>>  2 files changed, 35 insertions(+), 5 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index ea77577a3786..626a0e24e66a 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -2756,6 +2756,21 @@ const struct drm_edid *drm_edid_read(struct drm_connector *connector)
>>  }
>>  EXPORT_SYMBOL(drm_edid_read);
>>  
>> +/**
>> + * drm_edid_get_product_id - Get the vendor and product identification
>> + * @drm_edid: EDID
>> + * @id: Where to place the product id
>> + */
>> +void drm_edid_get_product_id(const struct drm_edid *drm_edid,
>> +			     struct drm_edid_product_id *id)
>> +{
>> +	if (drm_edid && drm_edid->edid && drm_edid->size >= EDID_LENGTH)
>> +		memcpy(id, &drm_edid->edid->product_id, sizeof(*id));
>> +	else
>> +		memset(id, 0, sizeof(*id));
>> +}
>> +EXPORT_SYMBOL(drm_edid_get_product_id);
>> +
>>  /**
>>   * drm_edid_get_panel_id - Get a panel's ID from EDID
>>   * @drm_edid: EDID that contains panel ID.
>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>> index 6f65bbf655a1..7911a2f8a672 100644
>> --- a/include/drm/drm_edid.h
>> +++ b/include/drm/drm_edid.h
>> @@ -272,14 +272,27 @@ struct detailed_timing {
>>  #define DRM_EDID_DSC_MAX_SLICES			0xf
>>  #define DRM_EDID_DSC_TOTAL_CHUNK_KBYTES		0x3f
>>  
>> +struct drm_edid_product_id {
>> +	u8 manufacturer_name[2];
>
> __be16?

Yeah, why not.

BR,
Jani.

>
>> +	__le16 product_code;
>> +	__le32 serial_number;
>> +	u8 week_of_manufacture;
>> +	u8 year_of_manufacture;
>> +} __packed;
>> +
>>  struct edid {
>>  	u8 header[8];
>>  	/* Vendor & product info */
>> -	u8 mfg_id[2];
>> -	u8 prod_code[2];
>> -	u32 serial; /* FIXME: byte order */
>> -	u8 mfg_week;
>> -	u8 mfg_year;
>> +	union {
>> +		struct drm_edid_product_id product_id;
>> +		struct {
>> +			u8 mfg_id[2];
>> +			u8 prod_code[2];
>> +			u32 serial; /* FIXME: byte order */
>> +			u8 mfg_week;
>> +			u8 mfg_year;
>> +		} __packed;
>> +	} __packed;
>>  	/* EDID version */
>>  	u8 version;
>>  	u8 revision;
>> @@ -466,6 +479,8 @@ int drm_edid_connector_update(struct drm_connector *connector,
>>  			      const struct drm_edid *edid);
>>  int drm_edid_connector_add_modes(struct drm_connector *connector);
>>  bool drm_edid_is_digital(const struct drm_edid *drm_edid);
>> +void drm_edid_get_product_id(const struct drm_edid *drm_edid,
>> +			     struct drm_edid_product_id *id);
>>  
>>  const u8 *drm_find_edid_extension(const struct drm_edid *drm_edid,
>>  				  int ext_id, int *ext_index);
>> -- 
>> 2.39.2

-- 
Jani Nikula, Intel

Re: [PATCH 2/4] drm/edid: add drm_edid_print_product_id()

[Jani Nikula]

On Mon, 08 Apr 2024, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Mar 21, 2024 at 12:05:10PM +0200, Jani Nikula wrote:
>> Add a function to print a decoded EDID vendor and product id to a drm
>> printer, optinally with the raw data.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/drm_edid.c | 35 +++++++++++++++++++++++++++++++++++
>>  include/drm/drm_edid.h     |  3 +++
>>  2 files changed, 38 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 626a0e24e66a..198986f0eb8b 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -29,6 +29,7 @@
>>   */
>>  
>>  #include <linux/bitfield.h>
>> +#include <linux/byteorder/generic.h>
>>  #include <linux/cec.h>
>>  #include <linux/hdmi.h>
>>  #include <linux/i2c.h>
>> @@ -2771,6 +2772,40 @@ void drm_edid_get_product_id(const struct drm_edid *drm_edid,
>>  }
>>  EXPORT_SYMBOL(drm_edid_get_product_id);
>>  
>> +/**
>> + * drm_edid_print_product_id - Print decoded product id to printer
>> + * @p: drm printer
>> + * @id: EDID product id
>> + * @raw: If true, also print the raw hex
>> + */
>> +void drm_edid_print_product_id(struct drm_printer *p,
>> +			       const struct drm_edid_product_id *id, bool raw)
>> +{
>> +	u16 mfg_id = id->manufacturer_name[0] << 8 | id->manufacturer_name[1];
>> +	char *date;
>> +	char vend[4];
>> +
>> +	drm_edid_decode_mfg_id(mfg_id, vend);
>> +
>> +	if (id->week_of_manufacture == 0xff)
>
> Didn't realize this had a loaded meaning. Maybe we should also
> skip the week printout if week==0? Otherwise people might think
> week==0 means the first week.

Agreed.

>
>> +		date = kasprintf(GFP_KERNEL, "model year: %d",
>> +				 id->year_of_manufacture + 1990);
>> +	else
>> +		date = kasprintf(GFP_KERNEL, "week: %d, year of manufacture: %d",
>
> The "week: %d" part feels a bit left out here. Maybe this should be
> formatted as "week/year of manufacture: %d/%d"? 

Agreed.

> Not sure I like the kasprintf(). Maybe use an on-stack buffer?

Refactored using struct seq_buf and a helper function; v2 out shortly.

BR,
Jani.

>
>> +				 id->week_of_manufacture,
>> +				 id->year_of_manufacture + 1990);
>> +
>> +	drm_printf(p, "manufacturer name: %s, product code: %u, serial number: %u, %s\n",
>> +		   vend, le16_to_cpu(id->product_code),
>> +		   le32_to_cpu(id->serial_number), date ?: "");
>> +
>> +	if (raw)
>> +		drm_printf(p, "raw product id: %*ph\n", (int)sizeof(*id), id);
>> +
>> +	kfree(date);
>> +}
>> +EXPORT_SYMBOL(drm_edid_print_product_id);
>> +
>>  /**
>>   * drm_edid_get_panel_id - Get a panel's ID from EDID
>>   * @drm_edid: EDID that contains panel ID.
>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>> index 7911a2f8a672..c763ba1a0bbd 100644
>> --- a/include/drm/drm_edid.h
>> +++ b/include/drm/drm_edid.h
>> @@ -30,6 +30,7 @@ struct drm_connector;
>>  struct drm_device;
>>  struct drm_display_mode;
>>  struct drm_edid;
>> +struct drm_printer;
>>  struct hdmi_avi_infoframe;
>>  struct hdmi_vendor_infoframe;
>>  struct i2c_adapter;
>> @@ -481,6 +482,8 @@ int drm_edid_connector_add_modes(struct drm_connector *connector);
>>  bool drm_edid_is_digital(const struct drm_edid *drm_edid);
>>  void drm_edid_get_product_id(const struct drm_edid *drm_edid,
>>  			     struct drm_edid_product_id *id);
>> +void drm_edid_print_product_id(struct drm_printer *p,
>> +			       const struct drm_edid_product_id *id, bool raw);
>>  
>>  const u8 *drm_find_edid_extension(const struct drm_edid *drm_edid,
>>  				  int ext_id, int *ext_index);
>> -- 
>> 2.39.2

-- 
Jani Nikula, Intel