Re: [PATCH v2 3/3] drm/sysfb: Use preferred panel size for panel orientation quirks

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

On Mon, 31 Aug 2026, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Hi
>
> Am 31.08.26 um 15:23 schrieb Jani Nikula:
>> On Mon, 31 Aug 2026, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>>> Sysfb drivers currently use the given display mode for looking up the
>>> panel orientation. But the look-up table stores the native geometry of
>>> the panels, so the lookup fails if the current mode sizes differs.
>>>
>>> Get the panel's native geometry with drm_edid_get_preferred_size() from
>>> the EDID and use it for looking up the panel orientation.
>>>
>>> v2:
>>> - ofdrm: validate EDID header before using it (Sashiko)
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Acked-by: Ard Biesheuvel <ardb@kernel.org>
>>> ---
>>>   drivers/gpu/drm/sysfb/efidrm.c  | 17 ++++++++++++++++-
>>>   drivers/gpu/drm/sysfb/ofdrm.c   | 18 ++++++++++++++++--
>>>   drivers/gpu/drm/sysfb/vesadrm.c | 17 ++++++++++++++++-
>>>   3 files changed, 48 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/sysfb/efidrm.c b/drivers/gpu/drm/sysfb/efidrm.c
>>> index 3f9cd5d03efb..7a7d12003304 100644
>>> --- a/drivers/gpu/drm/sysfb/efidrm.c
>>> +++ b/drivers/gpu/drm/sysfb/efidrm.c
>>> @@ -152,6 +152,7 @@ static struct efidrm_device *efidrm_device_create(struct drm_driver *drv,
>>>   	const struct screen_info *si;
>>>   	const struct drm_format_info *format;
>>>   	int width, height, stride;
>>> +	unsigned int panel_width, panel_height;
>>>   	s64 vsize;
>>>   	u64 mem_flags;
>>>   	struct resource resbuf;
>>> @@ -217,6 +218,20 @@ static struct efidrm_device *efidrm_device_create(struct drm_driver *drv,
>>>   	if (drm_edid_header_is_valid(dpy->edid.dummy) == 8)
>>>   		sysfb->edid = dpy->edid.dummy;
>>>   #endif
>>> +
>>> +	panel_width = width;
>>> +	panel_height = height;
>>> +
>>> +	if (sysfb->edid) {
>>> +		const struct drm_edid *drm_edid;
>>> +
>>> +		drm_edid = drm_edid_alloc(sysfb->edid, EDID_LENGTH);
>>> +		if (drm_edid) {
>>> +			drm_edid_get_preferred_size(drm_edid, &panel_width, &panel_height);
>>> +			drm_edid_free(drm_edid);
>>> +		}
>>> +	}
>>> +
>> Unrelated to the patch at hand, but does all of this code assume EDID is
>> always just one block? Scary. The whole sysfs->edid being a u8 pointer
>> without size feels like a footgun.
>
> For UEFI and VESA it's always one block of 128 bytes in size. The boot 
> parameters hard-code this value and there's no space for additional 
> blocks. [1]  On OpenFirmware, ofdrm checks the size to be EDID_LENGTH. 
> [2] I guess there could more blocks there, but AFAIK the DT property 
> only exists on ancient PPC MACs. I have no means of testing, but the 
> test file I obtained from such a system is 128 bytes.
>
> [1] 
> https://elixir.bootlin.com/linux/v7.2.2/source/arch/x86/include/uapi/asm/bootparam.h#L133
> [2] 
> https://elixir.bootlin.com/linux/v7.2.2/source/drivers/gpu/drm/sysfb/ofdrm.c#L229
>
>
>>
>> Here, the above should work, but I've been kind of hoping to eradicate
>> EDID_LENGTH assumptions and calculations from everywhere outside of
>> drm_edid.c, because it's complicated.
>
> We need some way of importing the raw data block into drm_edid.c, which 
> in turn needs the buffer size. The drivers could do this without 
> EDID_LENGTH if that helps. Note that the drivers use the regular 
> machinery from drm_edid.c for dissecting the EDID block. See [3].
>
> [3] 
> https://elixir.bootlin.com/linux/v7.2.2/source/drivers/gpu/drm/sysfb/drm_sysfb_modeset.c#L594

Okay, thanks for the pointers.

BR,
Jani.


>
> Best regards
> Thomas
>
>
>>
>>
>> BR,
>> Jani.
>>
>>
>>>   	sysfb->fb_mode = drm_sysfb_mode(width, height, 0, 0);
>>>   	sysfb->fb_format = format;
>>>   	sysfb->fb_pitch = stride;
>>> @@ -340,7 +355,7 @@ static struct efidrm_device *efidrm_device_create(struct drm_driver *drv,
>>>   	drm_connector_helper_add(connector, &efidrm_connector_helper_funcs);
>>>   	drm_connector_set_panel_orientation_with_quirk(connector,
>>>   						       DRM_MODE_PANEL_ORIENTATION_UNKNOWN,
>>> -						       width, height);
>>> +						       panel_width, panel_height);
>>>   	if (sysfb->edid)
>>>   		drm_connector_attach_edid_property(connector);
>>>   
>>> diff --git a/drivers/gpu/drm/sysfb/ofdrm.c b/drivers/gpu/drm/sysfb/ofdrm.c
>>> index 9d60db45139c..a0a865aee098 100644
>>> --- a/drivers/gpu/drm/sysfb/ofdrm.c
>>> +++ b/drivers/gpu/drm/sysfb/ofdrm.c
>>> @@ -229,7 +229,7 @@ static const u8 *display_get_edid_of(struct drm_device *dev, struct device_node
>>>   {
>>>   	int ret = of_property_read_u8_array(of_node, "EDID", buf, EDID_LENGTH);
>>>   
>>> -	if (ret)
>>> +	if (ret || drm_edid_header_is_valid(buf) != 8)
>>>   		return NULL;
>>>   	return buf;
>>>   }
>>> @@ -828,6 +828,7 @@ static struct ofdrm_device *ofdrm_device_create(struct drm_driver *drv,
>>>   	enum ofdrm_model model;
>>>   	bool big_endian;
>>>   	int width, height, depth, linebytes;
>>> +	unsigned int panel_width, panel_height;
>>>   	const struct drm_format_info *format;
>>>   	u64 address;
>>>   	const u8 *edid;
>>> @@ -998,6 +999,19 @@ static struct ofdrm_device *ofdrm_device_create(struct drm_driver *drv,
>>>   		sysfb->fb_gamma_lut_size = OFDRM_GAMMA_LUT_SIZE;
>>>   	sysfb->edid = edid;
>>>   
>>> +	panel_width = width;
>>> +	panel_height = height;
>>> +
>>> +	if (sysfb->edid) {
>>> +		const struct drm_edid *drm_edid;
>>> +
>>> +		drm_edid = drm_edid_alloc(sysfb->edid, EDID_LENGTH);
>>> +		if (drm_edid) {
>>> +			drm_edid_get_preferred_size(drm_edid, &panel_width, &panel_height);
>>> +			drm_edid_free(drm_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",
>>>   		&format->format, width, height, linebytes);
>>> @@ -1069,7 +1083,7 @@ static struct ofdrm_device *ofdrm_device_create(struct drm_driver *drv,
>>>   	drm_connector_helper_add(connector, &ofdrm_connector_helper_funcs);
>>>   	drm_connector_set_panel_orientation_with_quirk(connector,
>>>   						       DRM_MODE_PANEL_ORIENTATION_UNKNOWN,
>>> -						       width, height);
>>> +						       panel_width, panel_height);
>>>   	if (edid)
>>>   		drm_connector_attach_edid_property(connector);
>>>   
>>> diff --git a/drivers/gpu/drm/sysfb/vesadrm.c b/drivers/gpu/drm/sysfb/vesadrm.c
>>> index 6a67b2d2e451..8c52bbac6d2f 100644
>>> --- a/drivers/gpu/drm/sysfb/vesadrm.c
>>> +++ b/drivers/gpu/drm/sysfb/vesadrm.c
>>> @@ -402,6 +402,7 @@ static struct vesadrm_device *vesadrm_device_create(struct drm_driver *drv,
>>>   	const struct screen_info *si;
>>>   	const struct drm_format_info *format;
>>>   	int width, height, stride;
>>> +	unsigned int panel_width, panel_height;
>>>   	s64 vsize;
>>>   	struct resource resbuf;
>>>   	struct resource *res;
>>> @@ -484,6 +485,20 @@ static struct vesadrm_device *vesadrm_device_create(struct drm_driver *drv,
>>>   	if (drm_edid_header_is_valid(dpy->edid.dummy) == 8)
>>>   		sysfb->edid = dpy->edid.dummy;
>>>   #endif
>>> +
>>> +	panel_width = width;
>>> +	panel_height = height;
>>> +
>>> +	if (sysfb->edid) {
>>> +		const struct drm_edid *drm_edid;
>>> +
>>> +		drm_edid = drm_edid_alloc(sysfb->edid, EDID_LENGTH);
>>> +		if (drm_edid) {
>>> +			drm_edid_get_preferred_size(drm_edid, &panel_width, &panel_height);
>>> +			drm_edid_free(drm_edid);
>>> +		}
>>> +	}
>>> +
>>>   	sysfb->fb_mode = drm_sysfb_mode(width, height, 0, 0);
>>>   	sysfb->fb_format = format;
>>>   	sysfb->fb_pitch = stride;
>>> @@ -585,7 +600,7 @@ static struct vesadrm_device *vesadrm_device_create(struct drm_driver *drv,
>>>   	drm_connector_helper_add(connector, &vesadrm_connector_helper_funcs);
>>>   	drm_connector_set_panel_orientation_with_quirk(connector,
>>>   						       DRM_MODE_PANEL_ORIENTATION_UNKNOWN,
>>> -						       width, height);
>>> +						       panel_width, panel_height);
>>>   	if (sysfb->edid)
>>>   		drm_connector_attach_edid_property(connector);

-- 
Jani Nikula, Intel