Re: [PATCH 12/18] drm/sysfb: ofdrm: Add EDID support

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

On Wed, 19 Mar 2025, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Add EDID support to sysfb connector helpers. Read the EDID property
> from the OF node in ofdrm. Without EDID, this does nothing.
>
> Some systems with OF display, such as 32-bit PPC Macintoshs, provide
> the system display's EDID data as node property in their DT. Exporting
> this information allows compositors to implement correct DPI and
> meaningful color management.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/sysfb/drm_sysfb_helper.c | 29 ++++++++++++++++++++++++
>  drivers/gpu/drm/sysfb/drm_sysfb_helper.h |  2 ++
>  drivers/gpu/drm/sysfb/ofdrm.c            | 20 ++++++++++++++++
>  3 files changed, 51 insertions(+)
>
> diff --git a/drivers/gpu/drm/sysfb/drm_sysfb_helper.c b/drivers/gpu/drm/sysfb/drm_sysfb_helper.c
> index b48e06b25305..cb65c618f8d3 100644
> --- a/drivers/gpu/drm/sysfb/drm_sysfb_helper.c
> +++ b/drivers/gpu/drm/sysfb/drm_sysfb_helper.c
> @@ -9,6 +9,7 @@
>  #include <drm/drm_atomic_state_helper.h>
>  #include <drm/drm_damage_helper.h>
>  #include <drm/drm_drv.h>
> +#include <drm/drm_edid.h>
>  #include <drm/drm_fourcc.h>
>  #include <drm/drm_framebuffer.h>
>  #include <drm/drm_gem_atomic_helper.h>
> @@ -281,10 +282,38 @@ EXPORT_SYMBOL(drm_sysfb_crtc_atomic_destroy_state);
>   * Connector
>   */
>  
> +static int drm_sysfb_get_edid_block(void *data, u8 *buf, unsigned int block, size_t len)
> +{
> +	const u8 *edid = data;
> +	size_t off = block * EDID_LENGTH;
> +	size_t end = off + len;
> +
> +	if (!edid)
> +		return -1;
> +	if (end > EDID_LENGTH)
> +		return -1;

Nitpick, I guess -1 is used elsewhere, but I think I'd prefer actual
error codes even if they're not currently propagated. It's just cleaner.

> +	memcpy(buf, &edid[off], len);
> +
> +	return 0;
> +}
> +
>  int drm_sysfb_connector_helper_get_modes(struct drm_connector *connector)
>  {
>  	struct drm_sysfb_device *sysfb = to_drm_sysfb_device(connector->dev);
> +	const struct drm_edid *drm_edid;
> +
> +	if (sysfb->edid) {
> +		drm_edid = drm_edid_read_custom(connector, drm_sysfb_get_edid_block,
> +						(void *)sysfb->edid);

Nitpick, the (void *) cast is superfluous.

> +		if (drm_edid) {
> +			drm_edid_connector_update(connector, drm_edid);
> +			drm_edid_free(drm_edid);
> +		} else {
> +			drm_edid_connector_update(connector, NULL);
> +		}

Nitpick, the above could just be

		drm_edid_connector_update(connector, drm_edid);
		drm_edid_free(drm_edid);

without the if.


Despite the nitpicks, overall LGTM.

BR,
Jani.


> +	}
>  
> +	/* Return the fixed mode even with EDID */
>  	return drm_connector_helper_get_modes_fixed(connector, &sysfb->fb_mode);
>  }
>  EXPORT_SYMBOL(drm_sysfb_connector_helper_get_modes);
> diff --git a/drivers/gpu/drm/sysfb/drm_sysfb_helper.h b/drivers/gpu/drm/sysfb/drm_sysfb_helper.h
> index 45e396bf74b7..3684bd0ef085 100644
> --- a/drivers/gpu/drm/sysfb/drm_sysfb_helper.h
> +++ b/drivers/gpu/drm/sysfb/drm_sysfb_helper.h
> @@ -24,6 +24,8 @@ struct drm_display_mode drm_sysfb_mode(unsigned int width,
>  struct drm_sysfb_device {
>  	struct drm_device dev;
>  
> +	const u8 *edid; /* can be NULL */
> +
>  	/* hardware settings */
>  	struct drm_display_mode fb_mode;
>  	const struct drm_format_info *fb_format;
> diff --git a/drivers/gpu/drm/sysfb/ofdrm.c b/drivers/gpu/drm/sysfb/ofdrm.c
> index 71e661ba9329..86c1a0c80ceb 100644
> --- a/drivers/gpu/drm/sysfb/ofdrm.c
> +++ b/drivers/gpu/drm/sysfb/ofdrm.c
> @@ -12,6 +12,7 @@
>  #include <drm/drm_damage_helper.h>
>  #include <drm/drm_device.h>
>  #include <drm/drm_drv.h>
> +#include <drm/drm_edid.h>
>  #include <drm/drm_fbdev_shmem.h>
>  #include <drm/drm_format_helper.h>
>  #include <drm/drm_framebuffer.h>
> @@ -227,6 +228,16 @@ static u64 display_get_address_of(struct drm_device *dev, struct device_node *of
>  	return address;
>  }
>  
> +static const u8 *display_get_edid_of(struct drm_device *dev, struct device_node *of_node,
> +				     u8 buf[EDID_LENGTH])
> +{
> +	int ret = of_property_read_u8_array(of_node, "EDID", buf, EDID_LENGTH);
> +
> +	if (ret)
> +		return NULL;
> +	return buf;
> +}
> +
>  static bool is_avivo(u32 vendor, u32 device)
>  {
>  	/* This will match most R5xx */
> @@ -298,6 +309,8 @@ struct ofdrm_device {
>  	/* colormap */
>  	void __iomem *cmap_base;
>  
> +	u8 edid[EDID_LENGTH];
> +
>  	/* modesetting */
>  	u32 formats[DRM_SYSFB_PLANE_NFORMATS(1)];
>  	struct drm_plane primary_plane;
> @@ -840,6 +853,7 @@ static struct ofdrm_device *ofdrm_device_create(struct drm_driver *drv,
>  	int width, height, depth, linebytes;
>  	const struct drm_format_info *format;
>  	u64 address;
> +	const u8 *edid;
>  	resource_size_t fb_size, fb_base, fb_pgbase, fb_pgsize;
>  	struct resource *res, *mem;
>  	void __iomem *screen_base;
> @@ -989,6 +1003,9 @@ static struct ofdrm_device *ofdrm_device_create(struct drm_driver *drv,
>  		}
>  	}
>  
> +	/* EDID is optional */
> +	edid = display_get_edid_of(dev, of_node, odev->edid);
> +
>  	/*
>  	 * Firmware framebuffer
>  	 */
> @@ -999,6 +1016,7 @@ static struct ofdrm_device *ofdrm_device_create(struct drm_driver *drv,
>  	sysfb->fb_pitch = linebytes;
>  	if (odev->cmap_base)
>  		sysfb->fb_gamma_lut_size = OFDRM_GAMMA_LUT_SIZE;
> +	sysfb->edid = edid;
>  
>  	drm_dbg(dev, "display mode={" DRM_MODE_FMT "}\n", DRM_MODE_ARG(&sysfb->fb_mode));
>  	drm_dbg(dev, "framebuffer format=%p4cc, size=%dx%d, linebytes=%d byte\n",
> @@ -1072,6 +1090,8 @@ static struct ofdrm_device *ofdrm_device_create(struct drm_driver *drv,
>  	drm_connector_set_panel_orientation_with_quirk(connector,
>  						       DRM_MODE_PANEL_ORIENTATION_UNKNOWN,
>  						       width, height);
> +	if (edid)
> +		drm_connector_attach_edid_property(connector);
>  
>  	ret = drm_connector_attach_encoder(connector, encoder);
>  	if (ret)

-- 
Jani Nikula, Intel