Re: [PATCH 1/2] drm/edid: populate monitor_range from DisplayID Dynamic Video Timing block

[Jani Nikula]

On Sat, 28 Mar 2026, Adriano Vero <litaliano00.contact@xxxxxxxxx> wrote:
> Some eDP panels report their VRR limits only via a DisplayID v1r2
> Dynamic Video Timing Range Limits block (tag 0x25), without setting

That's a contradiction right there. DisplayID v1r2 does not have tag
0x25. DisplayID v2r0 does. All the DATA_BLOCK_2_* are only defined for
v2r0.

> DRM_EDID_FEATURE_CONTINUOUS_FREQ. drm_get_monitor_range() returns
> early for such panels, leaving monitor_range zeroed.
>
> Add drm_get_monitor_range_displayid() and call it from
> update_display_info() immediately after drm_get_monitor_range(). It
> uses displayid_iter_edid_begin() to locate Dynamic Video Timing blocks
> and extracts min/max refresh rates, including the 10-bit max_vfreq
> encoding signalled by block->rev bits [2:0]. It is a no-op when
> monitor_range is already populated by the classic EDID path.
>
> All drivers reading display_info.monitor_range now receive correct VRR
> limits from DisplayID-only panels without any raw EDID access.
>
> Byte offsets verified against parse_edid_displayid_vrr() in amdgpu_dm.c.
>
> Cc: Jani Nikula <jani.nikula@xxxxxxxxxxxxxxx>
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx
> Signed-off-by: Adriano Vero <litaliano00.contact@xxxxxxxxx>
> ---
>  drivers/gpu/drm/drm_edid.c | 68 ++++++++++++++++++++++++++++++++++++--
>  1 file changed, 66 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index ff432ac6b..d2c360178 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -6504,6 +6504,71 @@ void get_monitor_range(const struct detailed_timing *timing, void *c)
>  	}
>  }
>  
> +/**
> + * drm_get_monitor_range_displayid - populate monitor_range from a

Please no kernel-doc for static functions here.

> + * DisplayID v1r2 Dynamic Video Timing Range Limits block (tag 0x25).
> + *
> + * Some eDP panels report their VRR limits only in a DisplayID block,
> + * without setting DRM_EDID_FEATURE_CONTINUOUS_FREQ. drm_get_monitor_range()
> + * returns early for such panels, leaving monitor_range zeroed. This
> + * function is called separately from update_display_info() as a fallback.

Useless info. Please don't repeat what's obvious from the code.

> + *
> + * Block payload layout (block->num_bytes == 9):
> + *   data[6]       min_vfreq in Hz
> + *   data[7]       max_vfreq low 8 bits
> + *   data[8][1:0]  max_vfreq high bits (when block->rev & 7 is nonzero)
> + *
> + * Byte offsets verified against parse_edid_displayid_vrr() in amdgpu_dm.c.

Useless info.

> + */
> +static void
> +drm_get_monitor_range_displayid(struct drm_connector *connector,
> +				const struct drm_edid *drm_edid)
> +{
> +	struct drm_display_info *info = &connector->display_info;
> +	const struct displayid_block *block;
> +	struct displayid_iter iter;
> +
> +	/* Only run when the classic EDID path left these zeroed */
> +	if (info->monitor_range.min_vfreq && info->monitor_range.max_vfreq)
> +		return;
> +
> +	displayid_iter_edid_begin(drm_edid, &iter);
> +	displayid_iter_for_each(block, &iter) {
> +		const u8 *data;
> +		u16 min_vfreq, max_vfreq;
> +
> +		if (block->tag != DATA_BLOCK_2_DYNAMIC_VIDEO_TIMING)
> +			continue;

It would be pedantically correct to also check the
displayid_version(). See displayid_is_tiled_block() for an example.

> +
> +		/* rev bits [7:1] must be zero; payload must be exactly 9 bytes */

I can see all of that from the code. The comment doesn't add anything
helpful. If it explained why, it would go a long way.

> +		if ((block->rev & 0xFE) != 0 || block->num_bytes != 9)
> +			continue;
> +
> +		data = (const u8 *)(block + 1);
> +
> +		min_vfreq = data[6];
> +
> +		/* rev bits [2:0] nonzero: max_vfreq is 10-bit */
> +		if (block->rev & 7)
> +			max_vfreq = data[7] | ((u16)(data[8] & 3) << 8);
> +		else
> +			max_vfreq = data[7];

You might as well add a __packed struct for the data, similar to struct
displayid_tiled_block, to help parsing.

> +
> +		if (!min_vfreq || !max_vfreq)
> +			continue;
> +
> +		info->monitor_range.min_vfreq = min_vfreq;
> +		info->monitor_range.max_vfreq = max_vfreq;
> +
> +		drm_dbg_kms(connector->dev,
> +			    "[CONNECTOR:%d:%s] DisplayID dynamic video timing range: %u-%u Hz\n",
> +			    connector->base.id, connector->name,
> +			    min_vfreq, max_vfreq);

So I'd like all of the above to be in the form:

		if (displayid_is_dynamic_video_timing_block(...))
			displayid_parse_dynamic_video_timing_block(...);

Or something similar. Again, see displayid_is_tiled_block() and its use.

The main point is that I think we should group these displayid_iter*
blocks together more, iterating fewer times and handling all blocks at
once. Note that I'm *not* asking you to do that here, but separating the
parsing from the looping goes a long way in doing that in the future.

BR,
Jani.

> +		break;
> +	}
> +	displayid_iter_end(&iter);
> +}
> +
>  static void drm_get_monitor_range(struct drm_connector *connector,
>  				  const struct drm_edid *drm_edid)
>  {
> @@ -6691,10 +6756,9 @@ static void update_display_info(struct drm_connector *connector,
>  	info->height_mm = edid->height_cm * 10;
>  
>  	drm_get_monitor_range(connector, drm_edid);
> -
> +	drm_get_monitor_range_displayid(connector, drm_edid);

This is way too early. Somewhere near drm_update_mso() call is much
better.

>  	if (edid->revision < 3)
>  		goto out;
> -
>  	if (!drm_edid_is_digital(drm_edid))
>  		goto out;

-- 
Jani Nikula, Intel