Re: [PATCH RFC 12/25] drm/edid: Parse CinemaVRR flag from HDMI SCDS

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/edid: redundant cinema_vrr field introduces logic duplication
- [Medium] drm/edid: off-by-one payload length check in drm_parse_vrr_info()
--

commit 2696b9f8c9224d0c1b191489f857db21f792e625
Author: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>

drm/edid: Parse CinemaVRR flag from HDMI SCDS

HDMI's extension to EDID contains a flag which is used by sinks to
signal that they support CinemaVRR. When CinemaVRR is enabled, some
VRR is possible even if the minimum VRR range is set to 0.

> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index a1494028e0f5e..cc6f2ba8c9f4d 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -6314,6 +6314,11 @@ static void drm_parse_vrr_info(struct drm_display_info *info, const u8 *hf_scds)
>  	struct drm_monitor_range_info *range = &info->monitor_range;
>  	const int pld_len = cea_db_payload_len(hf_scds);
>  
> +	if (pld_len < 9)
> +		return;

[Severity: Medium]
Does this length check prematurely abort parsing for valid HDMI Forum VSDBs
that provide exactly an 8-byte payload?

Since cea_db_payload_len() returns the number of payload bytes following the
tag, and payload bytes are indexed starting from 1 up to pld_len, accessing
hf_scds[8] requires pld_len to be at least 8.

When pld_len == 8, it seems the valid 8th payload byte containing the
CinemaVRR flag would be incorrectly skipped.

> +
> +	info->hdmi.cinema_vrr = !!(hf_scds[8] & DRM_EDID_CINEMA_VRR);
> +
>  	if (pld_len < 10)
>  		return;

[ ... ]

> @@ -6392,7 +6397,7 @@ static void drm_parse_hdmi_forum_scds(struct drm_connector *connector,
>  
>  	drm_parse_vrr_info(info, hf_scds);
>  
> -	if (info->monitor_range.min_vfreq > 0)
> +	if (info->monitor_range.min_vfreq > 0 || hdmi->cinema_vrr)

[Severity: Medium]
Are we consuming a newly added redundant field here instead of the existing
vrr_cap.cinema_vrr?

Since the driver already correctly parses the CinemaVRR capability from the
exact same EDID bit in drm_parse_hdmi_gaming_info() into
hdmi->vrr_cap.cinema_vrr, could using this new duplicate field cause driver
state inconsistencies if the two parsing logics diverge?

>  		hdmi->vrr_capable = true;
>  	else
>  		hdmi->vrr_capable = false;
> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
> index b18684d039d5e..b147b8474f1a9 100644
> --- a/include/drm/drm_connector.h
> +++ b/include/drm/drm_connector.h
> @@ -383,6 +383,9 @@ struct drm_hdmi_info {
>  
>  	/** @vrr_capable: The sink supports variable refresh rate */
>  	bool vrr_capable;
> +
> +	/** @cinema_vrr: The sink supports "CinemaVRR" */
> +	bool cinema_vrr;

[Severity: Medium]
Could this introduce a duplicate field when vrr_cap.cinema_vrr already exists
in the same structure?

>  };
>  
>  /**

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260921-vrr-limiter-uapi-v1-0-2fcd7d011646@collabora.com?part=12