Re: [PATCH v2 20/27] drm/connector: hdmi: Add QMS state validation and computation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] drm/connector: hdmi: NULL old_conn_state->crtc dereference in hdmi_validate_vrr()
- [Medium] drm/connector: hdmi: QMS rates rejected in hdmi_validate_qms() when vrr_max is 0
- [Medium] drm/connector: hdmi: mismatched QMS checks in hdmi_validate_vrr() and VTEM path
- [Medium] drm/connector: hdmi: broken vrr_state->dynamic tracking in hdmi_validate_vrr()
- [Medium] drm/connector: hdmi: unhandled VRR_DISPLAY_MODE_OFF and ungated cinema_vrr check
- [Low] drm/connector: hdmi: missing @conn_state in hdmi_validate_qms() kernel-doc
--

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

drm/connector: hdmi: Add QMS state validation and computation

This commit adds validation and state computation for HDMI Quick Media
Switching to hdmi_validate_vrr() and introduces hdmi_validate_qms() to
verify target frame rates and compute the signaling byte.

> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> index 892079e41a0fb..ef08a72e33f3e 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c

[ ... ]

> @@ -992,6 +1013,66 @@ static int cmp_fraction(u32 a, u32 b, u32 c, u32 d)
>  	return 0;
>  }
>  
> +/**
> + * hdmi_validate_qms - Validate HDMI QMS state
> + * @crtc_state: pointer to a &struct drm_crtc_state to run validation on
> + * @info: pointer to a &struct drm_display_info of the accompanying connector
> + *
> + * Validate that the drm_crtc_vrr_state.vrr_min_numerator and
> + * drm_crtc_vrr_state.vrr_min_denominator are acceptable for HDMI QMS, and check
> + * that the display is HDMI QMS capable.
> + *
> + * Returns:
> + *  - Negative errno on failure
> + *  - the corresponding QMS signaling byte for the target frame rate otherwise
> + */
> +static int hdmi_validate_qms(struct drm_crtc_state *crtc_state,
> +			     struct drm_connector_state *conn_state,
> +			     const struct drm_display_info *info)

[Severity: Low]
This isn't a bug, but the kernel-doc comment for hdmi_validate_qms() omits
the @conn_state parameter, which triggers a kernel-doc warning when building
with W=1. Also, the comment body refers to vrr_min_numerator and
vrr_min_denominator instead of the struct drm_crtc_vrr_state fields
vrr_min_n and vrr_min_d.

> +{
> +	u32 rate_n = crtc_state->vrr_state.vrr_min_n;
> +	u32 rate_d = crtc_state->vrr_state.vrr_min_d;
> +	u32 min_rate_n, max_rate;
> +	u32 g, i;
> +
> +	if (conn_state->vrr_display_mode != VRR_DISPLAY_MODE_MEDIA)
> +		return 0;
> +
> +	if (!drm_crtc_helper_vrr_is_fixed_rate(&crtc_state->vrr_state))
> +		return -EINVAL;
> +
> +	if (info->hdmi.vrr_cap.qms_tfr_min)
> +		min_rate_n = 24000;
> +	else
> +		min_rate_n = info->hdmi.vrr_cap.vrr_min * 1000;
> +
> +	if (info->hdmi.vrr_cap.qms_tfr_max)
> +		max_rate = info->hdmi.vrr_cap.vrr_max;
> +	else
> +		max_rate = 60;
> +
> +	g = gcd(rate_n, rate_d);
> +	if (g != 1) {
> +		rate_n /= g;
> +		rate_d /= g;
> +	}
> +
> +	if (!rate_d)
> +		return -EINVAL;
> +
> +	for (i = 0; i < ARRAY_SIZE(hdmi_qms_tfrs); i++) {
> +		if (cmp_fraction(min_rate_n, 1001, hdmi_qms_tfrs[i].rate_n,
> +				 hdmi_qms_tfrs[i].rate_d) > 0)
> +			continue;
> +		if (rate_n == hdmi_qms_tfrs[i].rate_n &&
> +		    rate_d == hdmi_qms_tfrs[i].rate_d &&
> +		    cmp_fraction(rate_n, rate_d, max_rate, 1) <= 0)
> +			return hdmi_qms_tfrs[i].tfr_byte;
> +	}
> +
> +	return -EINVAL;
> +}

[Severity: Medium]
What happens in hdmi_validate_qms() when info->hdmi.vrr_cap.qms_tfr_max is
true, but info->hdmi.vrr_cap.vrr_max is 0?

In include/drm/drm_connector.h, struct drm_hdmi_vrr_cap documents that
vrr_max == 0 is a valid value (and occurs on sinks that only support media
switching without game variable refresh rate), and that qms_tfr_min and
qms_tfr_max fall back to monitor_range:

	/**
	 * @vrr_max : maximum supported variable refresh rate in Hz (optional).
	 * Valid values are either 0 (max based on video mode) or >= 100
	 */
	u16 vrr_max;
	...
	/**
	 * @qms_tfr_min: If set, the sink uses 24/1.001Hz as its minimum rate
	 * for QMS. If unset, @drm_display_info.monitor_range.min_vfreq is used.
	 */
	bool qms_tfr_min;

	/**
	 * @qms_tfr_max: If set, @drm_display_info.monitor_range.max_vfreq is
	 * used by the sink as its maximum rate for QMS. If unset, 60Hz is used.
	 */
	bool qms_tfr_max;

When qms_tfr_max is set and vrr_max is 0, max_rate becomes 0 in
hdmi_validate_qms(), so cmp_fraction(rate_n, rate_d, max_rate, 1) <= 0
evaluates to false for every entry in hdmi_qms_tfrs[] and rejects all target
frame rates.

Similarly, on a sink that supports media switching without game variable
refresh rate (where vrr_min is 0) and has qms_tfr_min unset, neither
hdmi_validate_qms() nor hdmi_validate_vrr() consults
info->monitor_range.min_vfreq, leaving min_vfreq_n as 0 and failing validation
at if (!min_vfreq_n) in hdmi_validate_vrr().

Should hdmi_validate_qms() and hdmi_validate_vrr() fall back to
info->monitor_range.min_vfreq and info->monitor_range.max_vfreq (or
mode_refresh) when vrr_min or vrr_max is 0?

[ ... ]

> @@ -1129,8 +1210,17 @@ static int hdmi_validate_vrr(struct drm_connector *connector,
>  	if (old_conn_state)
>  		old_crtc_state = drm_atomic_get_old_crtc_state(state, old_conn_state->crtc);

[Severity: High]
Can old_conn_state->crtc be NULL here when a previously disabled connector is
enabled in the same atomic commit that enables variable refresh rate on the
crtc?

In include/drm/drm_atomic.h, drm_atomic_get_old_crtc_state() indexes
state->crtcs using drm_crtc_index(crtc) without checking if crtc is NULL:

static inline struct drm_crtc_state *
drm_atomic_get_old_crtc_state(const struct drm_atomic_commit *state,
			      struct drm_crtc *crtc)
{
	return state->crtcs[drm_crtc_index(crtc)].old_state;
}

Should this check if (old_conn_state && old_conn_state->crtc) before calling
drm_atomic_get_old_crtc_state()?

> -	if (!info->hdmi.vrr_cap.supported)
> -		return -EOPNOTSUPP;
> +	switch (new_conn_state->vrr_display_mode) {
> +	case VRR_DISPLAY_MODE_GAME:
> +		if (!info->hdmi.vrr_cap.supported)
> +			return -EOPNOTSUPP;
> +		break;
> +	case VRR_DISPLAY_MODE_MEDIA:
> +		if (!info->hdmi.vrr_cap.qms_capable ||
> +		    !connector->hdmi.qms_supported)
> +			return -EOPNOTSUPP;
> +		break;
> +	}

[Severity: Medium]
How does this capability check interact with
hdmi_generate_emp_infoframe_vtem()?

In drivers/gpu/drm/display/drm_hdmi_state_helper.c,
hdmi_generate_emp_infoframe_vtem() still returns early when
!info->hdmi.vrr_cap.supported:

	if (!info->hdmi.vrr_cap.supported)
		return 0;
	...
	case VRR_DISPLAY_MODE_MEDIA:
		if (!connector->hdmi.qms_supported)
			return -EOPNOTSUPP;
		vtem->qms_en = true;
		vtem->next_tfr = conn_state->hdmi.qms_tfr_byte;
		vtem->m_const = !vrr->dynamic;
		break;

On a sink where info->hdmi.vrr_cap.qms_capable is true but
info->hdmi.vrr_cap.supported is false, hdmi_validate_vrr() now accepts the
commit, but hdmi_generate_emp_infoframe_vtem() skips generating the infoframe.

Conversely, when new_crtc_state->vrr_enabled is false and vrr_display_mode is
set to media mode, hdmi_validate_vrr() returns 0 early at:

	if (!new_crtc_state->vrr_enabled)
		return 0;

without checking info->hdmi.vrr_cap.qms_capable or updating
new_conn_state->hdmi.qms_tfr_byte, while hdmi_generate_emp_infoframe_vtem()
still runs and sets vtem->qms_en = true with an unvalidated 0 or stale
qms_tfr_byte from a previous commit.

Should hdmi_generate_emp_infoframe_vtem() and hdmi_validate_vrr() check the
same conditions for media mode and crtc_state->vrr_enabled?

>  	mode_refresh = drm_mode_vrefresh(mode);
>  
> @@ -1145,7 +1235,10 @@ static int hdmi_validate_vrr(struct drm_connector *connector,
>  		max_vfreq_n = mode_refresh;
>  
>  	is_fixed = drm_crtc_helper_vrr_is_fixed_rate(vrr_state);
> -	if (is_fixed && info->hdmi.vrr_cap.cinema_vrr)
> +	if (is_fixed && ((info->hdmi.vrr_cap.qms_tfr_min &&
> +			  new_conn_state->vrr_display_mode == VRR_DISPLAY_MODE_MEDIA &&
> +			  connector->hdmi.qms_supported && info->hdmi.vrr_cap.qms_capable) ||
> +			 info->hdmi.vrr_cap.cinema_vrr))
>  		min_vfreq_n = 24;

[Severity: Medium]
Are there two mode-selection issues here in hdmi_validate_vrr()?

First, in the switch on new_conn_state->vrr_display_mode above, neither
VRR_DISPLAY_MODE_OFF nor a default case is handled. When
new_crtc_state->vrr_enabled is true and vrr_display_mode is
VRR_DISPLAY_MODE_OFF, hdmi_validate_vrr() no longer checks sink capability and
configures crtc vtotal adjustment even though
hdmi_generate_emp_infoframe_vtem() leaves vtem->game_vrr_en and vtem->qms_en
disabled.

Second, in the min_vfreq_n check above, info->hdmi.vrr_cap.cinema_vrr is not
restricted to VRR_DISPLAY_MODE_GAME. If vrr_display_mode is
VRR_DISPLAY_MODE_MEDIA on a sink where cinema_vrr is true and qms_tfr_min is
false, min_vfreq_n will still be lowered to 24 Hz.

Should VRR_DISPLAY_MODE_OFF be rejected (or handled) when vrr_enabled is true,
and should info->hdmi.vrr_cap.cinema_vrr only apply when vrr_display_mode is
VRR_DISPLAY_MODE_GAME?

>  
>  	/* If the VRR target changed, make sure to reset the error accumulator */
> @@ -1176,6 +1269,19 @@ static int hdmi_validate_vrr(struct drm_connector *connector,
>  		return ret;
>  	}
>  
> +	ret = hdmi_validate_qms(new_crtc_state, new_conn_state, info);
> +	if (ret < 0) {
> +		drm_dbg_kms(dev, "Invalid QMS rate: %pe\n", ERR_PTR(ret));
> +		return ret;
> +	}
> +
> +	new_conn_state->hdmi.qms_tfr_byte = ret;
> +	if (!old_conn_state ||
> +	    new_conn_state->hdmi.qms_tfr_byte != old_conn_state->hdmi.qms_tfr_byte)
> +		vrr_state->dynamic = true;
> +	else
> +		vrr_state->dynamic = false;

[Severity: Medium]
Does setting vrr_state->dynamic purely from whether qms_tfr_byte changed work
as expected for vtem->m_const in both media and game modes?

In hdmi_generate_emp_infoframe_vtem(), vtem->m_const is set from
!vrr->dynamic:

		vtem->qms_en = true;
		vtem->next_tfr = conn_state->hdmi.qms_tfr_byte;
		vtem->m_const = !vrr->dynamic;

When media mode is first enabled or its target rate changes, qms_tfr_byte
differs from old_conn_state->hdmi.qms_tfr_byte, so vrr_state->dynamic is set
to true and the infoframe is generated with m_const = false. Because the
infoframe is only generated during atomic_check and is not updated after the
rate transition completes, won't m_const remain 0 on all subsequent frames of
constant-rate playback unless another atomic commit is made with the same
target rate?

In addition, when vrr_display_mode is VRR_DISPLAY_MODE_GAME with a fixed target
rate (is_fixed == true), hdmi_validate_qms() returns 0, so both old and new
qms_tfr_byte are 0. This check then sets vrr_state->dynamic = false even when
the fixed target rate (vrr_min_n / vrr_min_d) changed in the commit.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105-vrr-limiter-uapi-v2-0-55f4215be2c2@collabora.com?part=20