[PATCH 12/18] drm/sysfb: ofdrm: Add EDID support
Thomas Zimmermann
tzimmermann at suse.de
Thu Mar 20 13:08:56 UTC 2025
Hi
Am 20.03.25 um 13:50 schrieb Jani Nikula:
> On Wed, 19 Mar 2025, Thomas Zimmermann <tzimmermann at 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 at 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.
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)
More information about the dri-devel
mailing list