Re: [PATCH v2 2/3] drm/edid: Add drm_edid_get_preferred_size()

[Thomas Zimmermann <tzimmermann@suse.de>]

Hi

Am 31.08.26 um 15:13 schrieb Jani Nikula:
> On Mon, 31 Aug 2026, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Add drm_edid_get_preferred_size() to extract the panel's preferred
>> display resolution from a given EDID. Required for DRM's panel
>> orientation quirks.
>>
>> v2:
>> - handle EDID without pixel timing descriptor (Sashiko)
>> - fix checks for width and height pointers
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Acked-by: Ard Biesheuvel <ardb@kernel.org>
>> ---
>>   drivers/gpu/drm/drm_edid.c | 46 ++++++++++++++++++++++++++++++++++++++
>>   include/drm/drm_edid.h     |  2 ++
>>   2 files changed, 48 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
>> index 9990a836d0b6..53c7eed7a73e 100644
>> --- a/drivers/gpu/drm/drm_edid.c
>> +++ b/drivers/gpu/drm/drm_edid.c
>> @@ -2872,6 +2872,52 @@ u32 drm_edid_get_panel_id(const struct drm_edid *drm_edid)
>>   }
>>   EXPORT_SYMBOL(drm_edid_get_panel_id);
>>   
>> +/**
>> + * drm_edid_get_preferred_size - Get a panel's preferred size from EDID
>> + * @drm_edid: EDID that contains panel ID.
>> + * @width: Returns the panel's width in pixels per scanline, if given
>> + * @height: Returns the panel's height in scanlines, if given
>> + *
>> + * This function detects the preferred size of a panel from the given
>> + * EDID. There is no such information stored in the EDID block directly,
>> + * but the preferred mode often corresponds to the panel's native geometry.
> I think the special use case for this function should be mentioned. In
> the regular EDID parsing case, the connector will include the modes, one
> of them will be preferred, and that should be, uh, preferred.
>
> I dislike adding multiple functions for parsing the same thing, but
> looks like it can't be helped here. Let's at least avoid adding users
> for this in the paths that can use the normal stuff?

Absolutely.  The only reason I added this helper is that the driver 
needs the size for setting up the display pipeline before the connector 
exists. I looked into using the existing code without the connector, but 
that refactoring seemed to do more harm than good. Hence the new helper.

Should the new function be named drm_edid_detect_panel_size() to signal 
the use case?


>
>> + *
>> + * Return: Zero on success, or a negative errno code otherwise.
>> + */
>> +int drm_edid_get_preferred_size(const struct drm_edid *drm_edid,
>> +				unsigned int *width, unsigned int *height)
>> +{
>> +	const struct edid *edid = drm_edid->edid;
>> +	const struct detailed_timing *dt;
>> +	const struct detailed_pixel_timing *pt;
>> +
>> +	if (drm_edid->size < EDID_LENGTH)
>> +		return -EINVAL;
> This is guaranteed. drm_edid_alloc() intentionally won't hand you out a
> struct drm_edid with size smaller than EDID_LENGTH. Maybe check for
> !drm_edid instead.

Ok.

>
>> +
>> +	/*
>> +	 * Use display mode from the Preferred Timing Descriptor. For old
>> +	 * and obscure displays, we might need better heuristics.
>> +	 */
>> +
>> +	if (edid->revision < 4 && !(edid->features & DRM_EDID_FEATURE_PREFERRED_TIMING))
> The preferred timing mode should always be there starting from EDID
> 1.3. The bit doesn't indicate whether it's there or not. It should be
> always set for EDID 1.3, and for EDID 1.4 it indicates whether the
> preferred timing includes the native pixel format and the preferred
> refresh rate.
>
> I think just if (edid->revision < 3) should be enough.

Ok

>
>> +		return -EINVAL; /* no Preferred Timing Descriptor */
>> +
>> +	dt = &edid->detailed_timings[0];
>> +
>> +	if (!le16_to_cpu(dt->pixel_clock))
> There's is_detailed_timing_descriptor() for this.

Great!

Best regards
Thomas

>
>> +		return -EINVAL;
>> +
>> +	pt = &dt->data.pixel_data;
>> +
>> +	if (width)
>> +		*width = (pt->hactive_hblank_hi & 0xf0) << 4 | pt->hactive_lo;
>> +	if (height)
>> +		*height = (pt->vactive_vblank_hi & 0xf0) << 4 | pt->vactive_lo;
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL(drm_edid_get_preferred_size);
>> +
>>   /**
>>    * drm_edid_read_base_block - Get a panel's EDID base block
>>    * @adapter: I2C adapter to use for DDC
>> diff --git a/include/drm/drm_edid.h b/include/drm/drm_edid.h
>> index 04f7a7f1f108..981aeeb6ddf9 100644
>> --- a/include/drm/drm_edid.h
>> +++ b/include/drm/drm_edid.h
>> @@ -487,6 +487,8 @@ void drm_edid_get_product_id(const struct drm_edid *drm_edid,
>>   void drm_edid_print_product_id(struct drm_printer *p,
>>   			       const struct drm_edid_product_id *id, bool raw);
>>   u32 drm_edid_get_panel_id(const struct drm_edid *drm_edid);
>> +int drm_edid_get_preferred_size(const struct drm_edid *drm_edid,
>> +				unsigned int *width, unsigned int *height);
>>   bool drm_edid_match(const struct drm_edid *drm_edid,
>>   		    const struct drm_edid_ident *ident);
>>   bool drm_edid_has_quirk(struct drm_connector *connector, enum drm_edid_quirk quirk);

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, (HRB 36809, AG Nürnberg)