| Message ID | 20250411201333.151335-3-mwen@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/amd/display: more drm_edid to AMD display driver | expand |
On 2025-04-11 22:08, Melissa Wen wrote: > Since [1], we can use drm_edid_product_id to get debug info from > drm_edid instead of directly parsing EDID. > > Link: https://lore.kernel.org/dri-devel/cover.1712655867.git.jani.nikula@intel.com/ [1] > Signed-off-by: Melissa Wen <mwen@igalia.com> > --- > .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 16 +++++++++------- > 1 file changed, 9 insertions(+), 7 deletions(-) > > diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c > index 62954b351ebd..e93adb7e48a5 100644 > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c > [...] > @@ -122,13 +124,13 @@ enum dc_edid_status dm_helpers_parse_edid_caps( > if (!drm_edid_is_valid(edid_buf)) > result = EDID_BAD_CHECKSUM; > > - edid_caps->manufacturer_id = (uint16_t) edid_buf->mfg_id[0] | > - ((uint16_t) edid_buf->mfg_id[1])<<8; > - edid_caps->product_id = (uint16_t) edid_buf->prod_code[0] | > - ((uint16_t) edid_buf->prod_code[1])<<8; > - edid_caps->serial_number = edid_buf->serial; > - edid_caps->manufacture_week = edid_buf->mfg_week; > - edid_caps->manufacture_year = edid_buf->mfg_year; > + drm_edid_get_product_id(drm_edid, &product_id); > + > + edid_caps->manufacturer_id = le16_to_cpu(product_id.manufacturer_name); struct drm_edid_product_id has __be16 manufacturer_name; so shouldn't this use be16_to_cpu? (Though I see that would be a change in behaviour from the existing code...)
On 15/04/2025 06:32, Michel Dänzer wrote: > On 2025-04-11 22:08, Melissa Wen wrote: >> Since [1], we can use drm_edid_product_id to get debug info from >> drm_edid instead of directly parsing EDID. >> >> Link: https://lore.kernel.org/dri-devel/cover.1712655867.git.jani.nikula@intel.com/ [1] >> Signed-off-by: Melissa Wen <mwen@igalia.com> >> --- >> .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 16 +++++++++------- >> 1 file changed, 9 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c >> index 62954b351ebd..e93adb7e48a5 100644 >> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c >> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c >> [...] >> @@ -122,13 +124,13 @@ enum dc_edid_status dm_helpers_parse_edid_caps( >> if (!drm_edid_is_valid(edid_buf)) >> result = EDID_BAD_CHECKSUM; >> >> - edid_caps->manufacturer_id = (uint16_t) edid_buf->mfg_id[0] | >> - ((uint16_t) edid_buf->mfg_id[1])<<8; >> - edid_caps->product_id = (uint16_t) edid_buf->prod_code[0] | >> - ((uint16_t) edid_buf->prod_code[1])<<8; >> - edid_caps->serial_number = edid_buf->serial; >> - edid_caps->manufacture_week = edid_buf->mfg_week; >> - edid_caps->manufacture_year = edid_buf->mfg_year; >> + drm_edid_get_product_id(drm_edid, &product_id); >> + >> + edid_caps->manufacturer_id = le16_to_cpu(product_id.manufacturer_name); > struct drm_edid_product_id has > > __be16 manufacturer_name; > > so shouldn't this use be16_to_cpu? (Though I see that would be a change in behaviour from the existing code...) Hi Michel, thanks for reviewing this patch. It changes the behaviour, yes. But as you pointed it out I realized I can just assign product_id.manufacturer_name directly. It also noticed that I screwed up on rebasing and there is a patch missing here [1], let me fix all these things in the next version. [1] https://lore.kernel.org/amd-gfx/20250308142650.35920-3-mwen@igalia.com Melissa > >
On 2025-04-17 15:27, Melissa Wen wrote: > On 15/04/2025 06:32, Michel Dänzer wrote: >> On 2025-04-11 22:08, Melissa Wen wrote: >>> Since [1], we can use drm_edid_product_id to get debug info from >>> drm_edid instead of directly parsing EDID. >>> >>> Link: https://lore.kernel.org/dri-devel/cover.1712655867.git.jani.nikula@intel.com/ [1] >>> Signed-off-by: Melissa Wen <mwen@igalia.com> >>> --- >>> .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 16 +++++++++------- >>> 1 file changed, 9 insertions(+), 7 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c >>> index 62954b351ebd..e93adb7e48a5 100644 >>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c >>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c >>> [...] >>> @@ -122,13 +124,13 @@ enum dc_edid_status dm_helpers_parse_edid_caps( >>> if (!drm_edid_is_valid(edid_buf)) >>> result = EDID_BAD_CHECKSUM; >>> - edid_caps->manufacturer_id = (uint16_t) edid_buf->mfg_id[0] | >>> - ((uint16_t) edid_buf->mfg_id[1])<<8; >>> - edid_caps->product_id = (uint16_t) edid_buf->prod_code[0] | >>> - ((uint16_t) edid_buf->prod_code[1])<<8; >>> - edid_caps->serial_number = edid_buf->serial; >>> - edid_caps->manufacture_week = edid_buf->mfg_week; >>> - edid_caps->manufacture_year = edid_buf->mfg_year; >>> + drm_edid_get_product_id(drm_edid, &product_id); >>> + >>> + edid_caps->manufacturer_id = le16_to_cpu(product_id.manufacturer_name); >> struct drm_edid_product_id has >> >> __be16 manufacturer_name; >> >> so shouldn't this use be16_to_cpu? (Though I see that would be a change in behaviour from the existing code...) > Hi Michel, thanks for reviewing this patch. > > It changes the behaviour, yes. But as you pointed it out I realized I can just assign product_id.manufacturer_name directly. That's a third option, with its own issues: struct dc_edid_caps::manufacturer_id doesn't have any endianness annotation and is otherwise accessed directly, not via [bl]e16_to_cpu. It's also assigned directly to struct audio_info::manufacture_id, which is programmed to the AZALIA_F0_CODEC_PIN_CONTROL_SINK_INFO0 register. This means different behaviour (and specifically a different value being written to the register) on little vs big endian hosts. That can't be right. P.S. Independently from the above, AFAIK sparse will complain if a field marked with __be16 isn't accessed via be16_to_cpu / cpu_to_be16.
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c index 62954b351ebd..e93adb7e48a5 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm_helpers.c @@ -108,6 +108,8 @@ enum dc_edid_status dm_helpers_parse_edid_caps( struct drm_connector *connector = &aconnector->base; struct drm_device *dev = connector->dev; struct edid *edid_buf = edid ? (struct edid *) edid->raw_edid : NULL; + struct drm_edid *drm_edid; + struct drm_edid_product_id product_id; struct cea_sad *sads; int sad_count = -1; int sadb_count = -1; @@ -122,13 +124,13 @@ enum dc_edid_status dm_helpers_parse_edid_caps( if (!drm_edid_is_valid(edid_buf)) result = EDID_BAD_CHECKSUM; - edid_caps->manufacturer_id = (uint16_t) edid_buf->mfg_id[0] | - ((uint16_t) edid_buf->mfg_id[1])<<8; - edid_caps->product_id = (uint16_t) edid_buf->prod_code[0] | - ((uint16_t) edid_buf->prod_code[1])<<8; - edid_caps->serial_number = edid_buf->serial; - edid_caps->manufacture_week = edid_buf->mfg_week; - edid_caps->manufacture_year = edid_buf->mfg_year; + drm_edid_get_product_id(drm_edid, &product_id); + + edid_caps->manufacturer_id = le16_to_cpu(product_id.manufacturer_name); + edid_caps->product_id = le16_to_cpu(product_id.product_code); + edid_caps->serial_number = le32_to_cpu(product_id.serial_number); + edid_caps->manufacture_week = product_id.week_of_manufacture; + edid_caps->manufacture_year = product_id.year_of_manufacture; drm_edid_get_monitor_name(edid_buf, edid_caps->display_name,
Since [1], we can use drm_edid_product_id to get debug info from drm_edid instead of directly parsing EDID. Link: https://lore.kernel.org/dri-devel/cover.1712655867.git.jani.nikula@intel.com/ [1] Signed-off-by: Melissa Wen <mwen@igalia.com> --- .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-)