On Wed, 19 Mar 2025, Thomas Zimmermann <tzimmermann@xxxxxxx> 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@xxxxxxx>
> ---
> 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
