On 8/11/25 4:26 PM, Wentland, Harry wrote: > > > On 2025-08-11 17:09, Limonciello, Mario wrote: >> On 8/11/25 4:08 PM, Hung, Alex wrote: >>> Melissa, >>> >>> The patchset passed promotion and CI. >>> >>> However, since our DC code is shared with the other OS, calling drm_* >>> functions in DC won't work for us. For example, calling >>> dc_edid_copy_edid_to_sink from dc/link/link_detection.c in patch 14. >>> >>> I don't have a good way to handle it. Does it make sense not to touch DC >>> code for now? >> >> What about if we have a set of compatibility macros in DC code? >> >> Something like this: >> >> #ifndef drm_dbg >> #define drm_dbg .... >> #endif > > DC should stay OS-agnostic. No DRM concepts in DC please. > > Why the need to change dc_edid_is_same_edid? > > I'll comment directly on the patch. > > Harry
I hadn't dug into the patches to realize that this was more than just debug prints. I figured for those using a macro would be fine, but more in depth stuff totally aligned with you. > >>> >>> On 8/11/25 13:40, Melissa Wen wrote: >>>> >>>> >>>> On 28/07/2025 20:29, Alex Hung wrote: >>>>> Thanks. I will send v6 to promotion test. >>>> Hi Alex, >>>> >>>> Any news about this round of tests? >>>> >>>> BR, >>>> >>>> Melissa >>>> >>>>> >>>>> On 7/25/25 18:33, Melissa Wen wrote: >>>>>> Hi, >>>>>> >>>>>> Siqueira and I have been working on a solution to reduce the usage of >>>>>> drm_edid_raw in the AMD display driver, since the current guideline in >>>>>> the DRM subsystem is to stop handling raw edid data in driver-specific >>>>>> implementation and use opaque `drm_edid` object with common-code >>>>>> helpers. >>>>>> >>>>>> To keep DC as an OS-agnostic component, we create a mid layer that >>>>>> isolates `drm_edid` helpers called in the DC code, while allowing other >>>>>> OSes to implement their specific implementation. >>>>>> >>>>>> This work is an extension of [1]. >>>>>> >>>>>> - Patch 1 addresses a possible leak added by previous migration to >>>>>> drm_edid. >>>>>> - Patch 2 allocates a temporary drm_edid from raw edid for parsing. >>>>>> - Patches 3-7 use common-code, drm_edid helpers to parse edid >>>>>> capabilities instead of driver-specific solutions. For this, patch 4 >>>>>> introduces a new helper that gets monitor name from drm_edid. >>>>>> - Patches 8-9 are groundwork to reduce the noise of Linux/DRM specific >>>>>> code in the DC shared code >>>>>> - Patch 10 creates a mid layer to make DC embraces different ways of >>>>>> handling EDID by platforms. >>>>>> - Patch 11 move open-coded management of raw EDID data to the mid >>>>>> layer created before. >>>>>> - Patch 12 introduces a helper that compares EDIDs from two drm_edids. >>>>>> - Patch 13 adds drm_edid to dc_sink struct and a mid-layer helper to >>>>>> free `drm_edid`. >>>>>> - Patch 14 switch dc_edid to drm_edid across the driver in a way that >>>>>> the DC shared code is little affected by Linux specific stuff. >>>>>> >>>>>> [v1] https://lore.kernel.org/dri-devel/20250411201333.151335-1- >>>>>> m...@igalia.com/ >>>>>> Changes: >>>>>> - fix broken approach to get monitor name from eld (Jani) >>>>>> - I introduced a new helper that gets monitor name from drm_edid >>>>>> - rename drm_edid_eq to drm_edid_eq_buf and doc fixes (Jani) >>>>>> - add NULL edid checks (Jani) >>>>>> - fix mishandling of product_id.manufacturer_name (Michel) >>>>>> - I directly set it to manufacturer_id since sparse didn't complain. >>>>>> - add Mario's r-b in the first fix patch and fix commit msg typo. >>>>>> >>>>>> [v2] https://lore.kernel.org/dri-devel/20250507001712.120215-1- >>>>>> m...@igalia.com/ >>>>>> Changes: >>>>>> - kernel-doc and commit msg fixes (Jani) >>>>>> - use drm_edid_legacy_init instead of open coded (Jani) >>>>>> - place drm_edid new func into the right section (Jani) >>>>>> - paramenter names fix (Jani) >>>>>> - add Jani's r-b to the patch 12 >>>>>> - remove unnecessary include (Jani) >>>>>> - call dc_edid_sink_edid_free in link_detection, instead of >>>>>> drm_edid_free >>>>>> - rebase on top of asdn >>>>>> >>>>>> [v3] https://lore.kernel.org/dri-devel/20250514202130.291324-1- >>>>>> m...@igalia.com/ >>>>>> Changes: >>>>>> - rebase to asdn >>>>>> - some kernel-doc fixes >>>>>> - move some changes to the right commit >>>>>> >>>>>> [v4] https://lore.kernel.org/amd-gfx/20250613150015.245917-1- >>>>>> m...@igalia.com/ >>>>>> Changes: >>>>>> - fix comments and commit messages (Mario) >>>>>> - remove unnecessary drm_edid dup and fix mem leak (Mario) >>>>>> - add Mario's rb to patches 5-7 >>>>>> >>>>>> [v5] https://lore.kernel.org/amd-gfx/20250618152216.948406-1- >>>>>> m...@igalia.com/ >>>>>> Changes: >>>>>> - fix NULL pointer dereference (Alex H.) with the same approach >>>>>> proposed >>>>>> by 7c3be3ce3dfae >>>>>> >>>>>> ---> >>>>>> There are three specific points where we still use drm_edid_raw() in >>>>>> the >>>>>> driver: >>>>>> 1. raw edid data for write EDID checksum in DP_TEST_EDID_CHECKSUM via >>>>>> drm_dp_dpcd_write(), that AFAIK there is no common code solution >>>>>> yet; >>>>>> 2. open-coded connectivity log for dc link detection, that maybe can be >>>>>> moved to drm (?); >>>>>> 3. open-coded parser that I suspect is a lot of duplicated code, but >>>>>> needs careful examining. >>>>>> >>>>>> I suggest to address those points in a next phase for regression >>>>>> control. >>>>>> >>>>>> [1] https://lore.kernel.org/amd-gfx/20250308142650.35920-1- >>>>>> m...@igalia.com/ >>>>>> >>>>>> Let me know yours thoughts! >>>>>> >>>>>> Melissa >>>>>> >>>>>> Melissa Wen (12): >>>>>> drm/amd/display: make sure drm_edid stored in aconnector doesn't >>>>>> leak >>>>>> drm/amd/display: start using drm_edid helpers to parse EDID caps >>>>>> drm/amd/display: use drm_edid_product_id for parsing EDID product >>>>>> info >>>>>> drm/edid: introduce a helper that gets monitor name from drm_edid >>>>>> drm/amd/display: get panel id with drm_edid helper >>>>>> drm/amd/display: get SAD from drm_eld when parsing EDID caps >>>>>> drm/amd/display: get SADB from drm_eld when parsing EDID caps >>>>>> drm/amd/display: simplify dm_helpers_parse_edid_caps signature >>>>>> drm/amd/display: change DC functions to accept private types for >>>>>> edid >>>>>> drm/edid: introduce a helper that compares edid data from two >>>>>> drm_edid >>>>>> drm/amd/display: add drm_edid to dc_sink >>>>>> drm/amd/display: move dc_sink from dc_edid to drm_edid >>>>>> >>>>>> Rodrigo Siqueira (2): >>>>>> drm/amd/display: add a mid-layer file to handle EDID in DC >>>>>> drm/amd/display: create a function to fill dc_sink with edid data >>>>>> >>>>>> .../gpu/drm/amd/display/amdgpu_dm/Makefile | 1 + >>>>>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 33 +++--- >>>>>> .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c | 109 ++++++ >>>>>> +----------- >>>>>> .../display/amdgpu_dm/amdgpu_dm_mst_types.c | 21 ++-- >>>>>> .../gpu/drm/amd/display/amdgpu_dm/dc_edid.c | 39 +++++++ >>>>>> .../gpu/drm/amd/display/amdgpu_dm/dc_edid.h | 15 +++ >>>>>> .../drm/amd/display/dc/core/dc_link_exports.c | 9 +- >>>>>> drivers/gpu/drm/amd/display/dc/core/dc_sink.c | 3 + >>>>>> drivers/gpu/drm/amd/display/dc/dc.h | 10 +- >>>>>> drivers/gpu/drm/amd/display/dc/dm_helpers.h | 7 +- >>>>>> drivers/gpu/drm/amd/display/dc/inc/link.h | 9 +- >>>>>> .../drm/amd/display/dc/link/link_detection.c | 30 ++--- >>>>>> .../drm/amd/display/dc/link/link_detection.h | 9 +- >>>>>> drivers/gpu/drm/bridge/sil-sii8620.c | 2 +- >>>>>> drivers/gpu/drm/display/drm_dp_mst_topology.c | 2 +- >>>>>> drivers/gpu/drm/drm_edid.c | 54 +++++++-- >>>>>> include/drm/drm_edid.h | 10 +- >>>>>> 17 files changed, 199 insertions(+), 164 deletions(-) >>>>>> create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.c >>>>>> create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/dc_edid.h >>>>>> >>>>> >>>> >>> >> >
