On Sat, 28 Mar 2026, Adriano Vero <[email protected]> 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 <[email protected]> > Cc: [email protected] > Signed-off-by: Adriano Vero <[email protected]> > --- > 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
