[RFC PATCH 0/2] drm/amd/display: switch amdgpu_dm_connector to

[Melissa Wen]

On 01/29, Jani Nikula wrote:
> On Fri, 26 Jan 2024, Mario Limonciello <mario.limonciello at amd.com> wrote:
> > On 1/26/2024 10:28, Melissa Wen wrote:
> >> Hi,
> >> 
> >> I'm debugging a null-pointer dereference when running
> >> igt at kms_connector_force_edid and the way I found to solve the bug is to
> >> stop using raw edid handler in amdgpu_connector_funcs_force and
> >> create_eml_sink in favor of managing resouces via sruct drm_edid helpers
> >> (Patch 1). The proper solution seems to be switch amdgpu_dm_connector
> >> from struct edid to struct drm_edid and avoid the usage of different
> >> approaches in the driver (Patch 2). However, doing it implies a good
> >> amount of work and validation, therefore I decided to send this RFC
> >> first to collect opinions and check if there is any parallel work on
> >> this side. It's a working in progress.
> >> 
> >> The null-pointer error trigger by the igt at kms_connector_force_edid test
> >> was introduced by:
> >> - e54ed41620f ("drm/amd/display: Remove unwanted drm edid references")
> >> 
> >> You can check the error trace in the first patch.
> >> 
> >> This series was tested with kms_hdmi_inject and kms_force_connector. No
> >> null-pointer error, kms_hdmi_inject is successul and kms_force_connector
> >> is sucessful after the second execution - the force-edid subtest
> >> still fails in the first run (I'm still investigating).
> >> 
> >> There is also a couple of cast warnings to be addressed - I'm looking
> >> for the best replacement.
> >> 
> >> I appreciate any feedback and testing.
> >
> > So I'm actually a little bit worried by hardcoding EDID_LENGTH in this 
> > series.
> >
> > I have some other patches that I'm posting later on that let you get the 
> > EDID from _DDC BIOS method too.  My observation was that the EDID can be 
> > anywhere up to 512 bytes according to the ACPI spec.
> >
> > An earlier version of my patch was using EDID_LENGTH when fetching it 
> > and the EDID checksum failed.
> >
> > I'll CC you on the post, we probably want to get your changes and mine 
> > merged together.
> 
> One of the main points of struct drm_edid is that it tracks the
> allocation size separately.
> 
> We should simply not trust edid->extensions, because most of the time it
> originates from outside the kernel.
> 
> Using drm_edid and immediately drm_edid_raw() falls short. That function
> should only be used during migration to help. And yeah, it also means
> EDID parsing should be done in drm_edid.c, and not spread out all over
> the subsystem.

Hi Mario and Jani,

Thanks for the feedback.

I agree with you. I used the drm_edid_raw() as an intermediate step to
assess/validate this migration, but I'll work on removing this hack.

So, I understand that the transition from edid to drm_edid is the right
path, so I'll improve this work (keeping the points you raised in mind)
and send a version.

BR,

Melissa
> 
> 
> BR,
> Jani.
> 
> 
> >
> >> 
> >> Melissa
> >> 
> >> Melissa Wen (2):
> >>    drm/amd/display: fix null-pointer dereference on edid reading
> >>    drm/amd/display: switch amdgpu_dm_connector to use struct drm_edid
> >> 
> >>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 78 ++++++++++---------
> >>   .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  4 +-
> >>   .../amd/display/amdgpu_dm/amdgpu_dm_helpers.c |  9 ++-
> >>   .../display/amdgpu_dm/amdgpu_dm_mst_types.c   | 23 +++---
> >>   4 files changed, 60 insertions(+), 54 deletions(-)
> >> 
> >
> 
> -- 
> Jani Nikula, Intel