Re: [PATCH v8 0/8] drm/amd/display: more drm_edid to AMD display driver (partial)

[Jani Nikula <jani.nikula@linux.intel.com>]

On Tue, 14 Apr 2026, Leo Li <sunpeng.li@amd.com> wrote:
> On 2026-04-13 13:51, Melissa Wen wrote:
>> 
>> 
>> On 31/03/2026 12:45, Leo Li wrote:
>>>
>>> On 2026-02-27 03:52, Timur Kristóf wrote:
>>>> On Wednesday, February 25, 2026 1:04:29 PM Central European Standard Time
>>>> Melissa Wen wrote:
>>>>> Hi,
>>>>>
>>>>> This is a reduced version of `drm/amd/display: more drm_edid to AMD
>>>>> display driver` [1] sent a few months ago only with the less invasive
>>>>> changes, i.e., those changes that don't affect DC. This partial focus on
>>>>> using more drm_edid helpers instead of raw EDID helpers. Most patches
>>>>> here are already reviewed by someone and I think they can be merged
>>>>> without major concerns. They can also prevent new initiatives of keeping
>>>>> parsing raw EDID as driver specific code, which can make harder for us
>>>>> to remove `drm_edid_raw()`. They should focus on moving raw EDID
>>>>> handling to the DRM common code instead.
>>>>>
>>>>> Regarding code changes from previous version, here I added some r-b tags
>>>>> (from Mario and Timur), fixed commit message syntax (Mario) and
>>>>> centralized error handling (Timur). I'll follow up the changes in DC to
>>>>> accept the Linux/DRM opaque object in a separate series since those look
>>>>> more sensitive.
>>>>>
>>>>> [1] https://lore.kernel.org/amd-gfx/20251106165536.161662-1-mwen@igalia.com/
>>>>>
>>>>> BR,
>>>>>
>>>>> Melissa
>>>>>
>>>> Nice work, Melissa.
>>>>
>>>> Reviewed-by: Timur Kristóf <timur.kristof@gmail.com>
>>>> For this series.
>>> Thanks for your patience Melissa, series is also
>>> Reviewed-by: Leo Li <sunpeng.li@amd.com>
>>>
>>> We'll include it in this week's testing.
>> 
>> Hi Leo,
>> 
>> In the end, was this series applied to AMD's branch or do I need to rebase it?
>> 
>> Melissa
>
> Hi Melissa,
>
> During testing, it seems we're failing drm_edid_valid() in this callpath:
>
>     dm_helpers_read_local_edid > dm_helpers_parse_edid_caps > drm_edid_valid.
>
> I'm 90% sure it's because recreating the drm_edid from dc_edid ignores the
> HF-EEODB (HDMI Forum EDID Extension Override Data Block), which can override the
> number of extensions dictated by the base EDID. However, drm_edid_valid() does
> consider the HF-EEODB, causing this line(*) to return false. Commenting it out
> "fixes" the issue
>
> (*)https://elixir.bootlin.com/linux/v7.0/source/drivers/gpu/drm/drm_edid.c#L2076
>
> This whole drm_edid to dc_edid conversion in dm_helpers_read_local_edid(), then
> immediately back to drm_edid in parse_edid_caps(), is really not nice... We lose
> the HF-EEODB-aware size by creating dc_edid based on the EDID-advertised
> extension count. It should work if drm_edid is simply passed straight through.
>
> The challenge is dm_helpers* are DC's interface back to DM, and there is another
> OS that depends on it. Plumbing drm_edid through DC would be quite challenging.

I find this to be in stark contrast with stable-api-nonsense.rst [1].
Why do we have internal kernel code that we can't change based on the
*outside* needs even if it makes total sense from the kernel POV? Nobody
outside of AMD has any idea of the constraints or how they apply.

[1] https://docs.kernel.org/process/stable-api-nonsense.html

> Alternatively, It should work if we carry over the drm_edid.size value to
> dc_edid.length. Do you know the motivation behind keeping drm_edid->size
> private? Would it be a good idea to implement a drm_edid_get_size()?

struct drm_edid is opaque specifically to prevent drivers from trying to
parse EDID directly or trying to calculate the EDID size. I did this to
enforce centralized handling of HF-EEODB. With the old struct edid,
there used to be huge amounts of code computing EDID size, and you
couldn't be sure if whoever allocated it was aware of HF-EEODB or
not. struct drm_edid dissociates the EDID from the allocated buffer
size.

And, really, all EDID parsing should happen in drm_edid.c the same way
for all drivers, and making struct drm_edid opaque is pretty much the
only way to block quick hacks from proliferating.

BR,
Jani.

-- 
Jani Nikula, Intel