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.nik...@intel.com/ >>> [1] >>> Signed-off-by: Melissa Wen <m...@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. -- Earthling Michel Dänzer \ GNOME / Xwayland / Mesa developer https://redhat.com \ Libre software enthusiast
