[PATCH] drm/amdgpu: convert bios_hardcoded_edid to drm_edid

[Alex Deucher]

On Tue, Jul 23, 2024 at 12:49 PM Alex Deucher <alexdeucher at gmail.com> wrote:
>
> On Sun, Jun 16, 2024 at 2:32 PM Thomas Weißschuh <linux at weissschuh.net> wrote:
> >
> > On 2024-06-16 11:12:03+0000, Thomas Weißschuh wrote:
> > > Instead of manually passing around 'struct edid *' and its size,
> > > use 'struct drm_edid', which encapsulates a validated combination of
> > > both.
> > >
> > > As the drm_edid_ can handle NULL gracefully, the explicit checks can be
> > > dropped.
> > >
> > > Also save a few characters by transforming '&array[0]' to the equivalent
> > > 'array' and using 'max_t(int, ...)' instead of manual casts.
> > >
> > > Signed-off-by: Thomas Weißschuh <linux at weissschuh.net>
> > > ---
> > > While this patch introduces a new user for drm_edid_raw(),
> > > if amdgpu proper gets migrated to 'struct drm_edid', that usage will go
> > > away.
> > >
> > > This is only compile-tested.
> > >
> > > I have some more patches for the rest of amdgpu,
> > > to move to 'struct drm_edid'.
> > > This patch is a test-balloon for the general idea.
> > >
> > > The same can also be done for drm/radeon.
> > > ---
> > >  drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c |  6 +-----
> > >  drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h       |  4 ++--
> > >  drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c       |  2 +-
> > >  drivers/gpu/drm/amd/amdgpu/atombios_encoders.c | 21 +++++++--------------
> > >  drivers/gpu/drm/amd/amdgpu/dce_v10_0.c         |  2 +-
> > >  drivers/gpu/drm/amd/amdgpu/dce_v11_0.c         |  2 +-
> > >  drivers/gpu/drm/amd/amdgpu/dce_v6_0.c          |  2 +-
> > >  drivers/gpu/drm/amd/amdgpu/dce_v8_0.c          |  2 +-
> > >  8 files changed, 15 insertions(+), 26 deletions(-)
> >
> > <snip>
> >
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c b/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
> > > index 25feab188dfe..90383094ed1e 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/atombios_encoders.c
> > > @@ -2064,20 +2064,13 @@ amdgpu_atombios_encoder_get_lcd_info(struct amdgpu_encoder *encoder)
> > >                               case LCD_FAKE_EDID_PATCH_RECORD_TYPE:
> > >                                       fake_edid_record = (ATOM_FAKE_EDID_PATCH_RECORD *)record;
> > >                                       if (fake_edid_record->ucFakeEDIDLength) {
> > > -                                             struct edid *edid;
> > > -                                             int edid_size =
> > > -                                                     max((int)EDID_LENGTH, (int)fake_edid_record->ucFakeEDIDLength);
> > > -                                             edid = kmalloc(edid_size, GFP_KERNEL);
> > > -                                             if (edid) {
> > > -                                                     memcpy((u8 *)edid, (u8 *)&fake_edid_record->ucFakeEDIDString[0],
> > > -                                                            fake_edid_record->ucFakeEDIDLength);
> > > -
> > > -                                                     if (drm_edid_is_valid(edid)) {
> > > -                                                             adev->mode_info.bios_hardcoded_edid = edid;
> > > -                                                             adev->mode_info.bios_hardcoded_edid_size = edid_size;
> > > -                                                     } else
> > > -                                                             kfree(edid);
> > > -                                             }
> > > +                                             const struct drm_edid *edid;
> > > +                                             edid = drm_edid_alloc(fake_edid_record->ucFakeEDIDString,
> > > +                                                                   max_t(int, EDID_LENGTH, fake_edid_record->ucFakeEDIDLength));
> > > +                                             if (drm_edid_valid(edid))
> > > +                                                     adev->mode_info.bios_hardcoded_edid = edid;
> > > +                                             else
> > > +                                                     drm_edid_free(edid);
> >
> > The old code here seems broken in general.
> > In drivers/gpu/drm/amd/include/atombios.h the comment for ucFakeEDIDLength says:
> > (I expect the same field in the same struct for amdgpu to have the same semantics)
> >
> >     UCHAR ucFakeEDIDLength;       // = 128 means EDID length is 128 bytes, otherwise the EDID length = ucFakeEDIDLength*128
> >
> > So as soon as the EDID from the BIOS has extensions, only the first few
> > bytes will be copied into the allocated memory. drm_edid_is_valid() will
> > then read the uninitialized memory and if the "extensions" field ends up
> > non-zero it will happily "validate" past the allocated buffer.
>
> I guess the allocation should be changed to something like:
> if (ucFakeEDIDLength == 128)
>     edid_size = ucFakeEDIDLength;
> else
>     edid_size = ucFakeEDIDLength * 128;

The record size handling in atombios_encoders.c would also need to be fixed.

Alex

>
> That said, I don't know how many systems actually used this.  IIRC
> this was only used in GPUs from 15-20 years ago.  No objections to the
> patch in general.
>
> Alex
>
>
> >
> > The new code won't work either but at least it won't read uninitialized
> > memory nor will it read past the buffer bounds.
> >
> > >                                       }
> > >                                       record += fake_edid_record->ucFakeEDIDLength ?
> > >                                                 struct_size(fake_edid_record,
> >
> > <snip>