Hi Am 20.03.25 um 13:50 schrieb Jani Nikula:
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.
Sure. Somehow I was under the impression that errno codes wouldn't be welcome here.
+ 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.
This is a const cast.
+ 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.
Make sense.
Despite the nitpicks, overall LGTM.
Thanks for reviewing.Since I have your attention and you're knowledgeable wrt EDID: byte 20 of the EDID header indicates the type of output (analog, HDMI, DP, etc). I intent to use this for setting the connector type to something better then UNKNOWN. Does that make sense?
Best regards Thomas
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)
-- -- Thomas Zimmermann Graphics Driver Developer SUSE Software Solutions Germany GmbH Frankenstrasse 146, 90461 Nuernberg, Germany GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman HRB 36809 (AG Nuernberg)
