diff mbox series

drm/amdgpu: convert bios_hardcoded_edid to drm_edid

Message ID 20240616-amdgpu-edid-bios-v1-1-2874f212b365@weissschuh.net (mailing list archive)
State New, archived
Headers show
Series drm/amdgpu: convert bios_hardcoded_edid to drm_edid | expand

Commit Message

Thomas Weißschuh June 16, 2024, 9:12 a.m. UTC
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@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(-)


---
base-commit: a3e18a540541325a8c8848171f71e0d45ad30b2c
change-id: 20240616-amdgpu-edid-bios-a31aa16b0321

Best regards,

Comments

Thomas Weißschuh June 16, 2024, 6:14 p.m. UTC | #1
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@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.

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>
Thomas Weißschuh July 20, 2024, 7:35 a.m. UTC | #2
Hi amdgpu maintainers,

did you get a chance to look at the patch and the suspected bug?

Thanks,
Thomas

On 2024-06-16 20:14:45+0000, Thomas Weißschuh 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@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.
> 
> 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>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
index 9caba10315a8..f1b11b27cce0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_connectors.c
@@ -265,11 +265,7 @@  struct edid *amdgpu_connector_edid(struct drm_connector *connector)
 static struct edid *
 amdgpu_connector_get_hardcoded_edid(struct amdgpu_device *adev)
 {
-	if (adev->mode_info.bios_hardcoded_edid) {
-		return kmemdup((unsigned char *)adev->mode_info.bios_hardcoded_edid,
-			       adev->mode_info.bios_hardcoded_edid_size, GFP_KERNEL);
-	}
-	return NULL;
+	return drm_edid_duplicate(drm_edid_raw(adev->mode_info.bios_hardcoded_edid));
 }
 
 static void amdgpu_connector_get_edid(struct drm_connector *connector)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
index 1fe21a70ddd0..928ac3f1e2ba 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h
@@ -51,6 +51,7 @@  struct amdgpu_encoder;
 struct amdgpu_router;
 struct amdgpu_hpd;
 struct edid;
+struct drm_edid;
 
 #define to_amdgpu_crtc(x) container_of(x, struct amdgpu_crtc, base)
 #define to_amdgpu_connector(x) container_of(x, struct amdgpu_connector, base)
@@ -325,8 +326,7 @@  struct amdgpu_mode_info {
 	/* FMT dithering */
 	struct drm_property *dither_property;
 	/* hardcoded DFP edid from BIOS */
-	struct edid *bios_hardcoded_edid;
-	int bios_hardcoded_edid_size;
+	const struct drm_edid *bios_hardcoded_edid;
 
 	/* firmware flags */
 	u32 firmware_flags;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c
index e30eecd02ae1..543275db8302 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vkms.c
@@ -539,7 +539,7 @@  static int amdgpu_vkms_sw_fini(void *handle)
 
 	adev->mode_info.mode_config_initialized = false;
 
-	kfree(adev->mode_info.bios_hardcoded_edid);
+	drm_edid_free(adev->mode_info.bios_hardcoded_edid);
 	kfree(adev->amdgpu_vkms_output);
 	return 0;
 }
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);
 					}
 					record += fake_edid_record->ucFakeEDIDLength ?
 						  struct_size(fake_edid_record,
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
index b44fce44c066..11d648e688ce 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v10_0.c
@@ -2846,7 +2846,7 @@  static int dce_v10_0_sw_fini(void *handle)
 {
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
-	kfree(adev->mode_info.bios_hardcoded_edid);
+	drm_edid_free(adev->mode_info.bios_hardcoded_edid);
 
 	drm_kms_helper_poll_fini(adev_to_drm(adev));
 
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
index 80b2e7f79acf..01536f523032 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v11_0.c
@@ -2973,7 +2973,7 @@  static int dce_v11_0_sw_fini(void *handle)
 {
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
-	kfree(adev->mode_info.bios_hardcoded_edid);
+	drm_edid_free(adev->mode_info.bios_hardcoded_edid);
 
 	drm_kms_helper_poll_fini(adev_to_drm(adev));
 
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
index db20012600f5..0e5b568a96fc 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v6_0.c
@@ -2745,7 +2745,7 @@  static int dce_v6_0_sw_fini(void *handle)
 {
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
-	kfree(adev->mode_info.bios_hardcoded_edid);
+	drm_edid_free(adev->mode_info.bios_hardcoded_edid);
 
 	drm_kms_helper_poll_fini(adev_to_drm(adev));
 
diff --git a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
index 5b56100ec902..895f050a3e62 100644
--- a/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/dce_v8_0.c
@@ -2766,7 +2766,7 @@  static int dce_v8_0_sw_fini(void *handle)
 {
 	struct amdgpu_device *adev = (struct amdgpu_device *)handle;
 
-	kfree(adev->mode_info.bios_hardcoded_edid);
+	drm_edid_free(adev->mode_info.bios_hardcoded_edid);
 
 	drm_kms_helper_poll_fini(adev_to_drm(adev));