Re: [PATCH v2 05/27] drm/connector: hdmi: Add VTEM EMP generation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/connector: hdmi: unpopulated adjusted_mode in hdmi_generate_emp_infoframe_vtem()
- [Medium] drm/connector: hdmi: invalid VTEM EMP state when vrr_display_mode is Off
--

commit 404c1e52433cdea0b822a43bb7471dc9ca224073
Author: Derek Foreman <derek.foreman@collabora.com>

drm/connector: hdmi: Add VTEM EMP generation

This commit adds VTEM Extended Metadata Packet (EMP) generation and state
tracking to the HDMI connector state helpers to support variable refresh
rate signalling on VRR-capable sinks.

> diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> index c1a84e30441a6..497af307a690c 100644
> --- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> +++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
> @@ -853,6 +853,61 @@ static int hdmi_generate_hdmi_vendor_infoframe(const struct drm_connector *conne
>  	return 0;
>  }
>  
> +static int hdmi_generate_emp_infoframe_vtem(const struct drm_connector *connector,
> +					    struct drm_connector_state *conn_state)
> +{
> +	const struct drm_display_info *info = &connector->display_info;
> +	const struct drm_crtc_state *crtc_state =
> +		drm_atomic_get_crtc_state(conn_state->state, conn_state->crtc);
> +	struct drm_connector_hdmi_infoframe *infoframe =
> +		&conn_state->hdmi.infoframes.vtem;
> +	struct hdmi_emp_infoframe_vtem *vtem =
> +		&infoframe->data.vtem;
> +	const struct drm_crtc_vrr_state *vrr = &crtc_state->vrr_state;
> +	int vfront;
> +	u8 avi_vic;
> +
> +	infoframe->set = false;
> +
> +	if (!connector->hdmi.funcs->vtem.write_infoframe)
> +		return 0;
> +
> +	if (!info->hdmi.vrr_cap.supported)
> +		return 0;
> +
> +	if (conn_state->hdmi.infoframes.avi.set)
> +		avi_vic = conn_state->hdmi.infoframes.avi.data.avi.video_code;
> +	else
> +		avi_vic = 0;
> +
> +	hdmi_emp_infoframe_vtem_init(vtem);
> +
> +	switch (conn_state->vrr_display_mode) {
> +	case VRR_DISPLAY_MODE_GAME:
> +		vtem->game_vrr_en = true;
> +		vtem->m_const = !vrr->dynamic;
> +		break;
> +	case VRR_DISPLAY_MODE_OFF:
> +		break;

[Severity: Medium]
When conn_state->vrr_display_mode is VRR_DISPLAY_MODE_OFF, breaking out of
the switch allows execution to reach infoframe->set = true at the end of
hdmi_generate_emp_infoframe_vtem().

Because new_frame->set is true, write_or_clear_infoframe() in
drm_atomic_helper_connector_hdmi_update_infoframes() will call
write_infoframe() instead of clear_infoframe():

drivers/gpu/drm/display/drm_hdmi_state_helper.c:write_or_clear_infoframe() {
	if (new_frame->set)
		return write_infoframe(connector, funcs, type, new_frame);

	if (old_frame->set && !new_frame->set)
		return clear_infoframe(connector, funcs, type);
...
}

In turn, hdmi_emp_infoframe_vtem_pack() packs a VTEM packet with
Data_Set_Length hardcoded to 4 (ptr[9] = 4) while game_vrr_en,
fva_factor_m1, base_vfront, and base_refresh_rate are all 0.

Should VRR_DISPLAY_MODE_OFF return 0 with infoframe->set left as false so
that write_or_clear_infoframe() invokes clear_infoframe() to stop VTEM EMP
transmission when VRR is disabled?

> +	default:
> +		return -EINVAL;
> +	}
> +
> +	if (vtem->game_vrr_en && !avi_vic) {
> +		vtem->base_refresh_rate = drm_mode_vrefresh(&crtc_state->mode);
> +		vfront = crtc_state->adjusted_mode.crtc_vsync_start -
> +			 crtc_state->adjusted_mode.crtc_vdisplay;

[Severity: High]
Is crtc_state->adjusted_mode populated when
hdmi_generate_emp_infoframe_vtem() runs during atomic_check?

In drm_atomic_helper_check_modeset(), the connector atomic_check callbacks
(drm_atomic_helper_connector_hdmi_check() -> hdmi_generate_infoframes() ->
hdmi_generate_emp_infoframe_vtem()) run before mode_fixup() copies
new_crtc_state->mode into new_crtc_state->adjusted_mode:

drivers/gpu/drm/drm_atomic_helper.c:mode_fixup() {
	for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) {
		if (!new_crtc_state->mode_changed &&
		    !new_crtc_state->connectors_changed)
			continue;

		drm_mode_copy(&new_crtc_state->adjusted_mode, &new_crtc_state->mode);
	}
...
}

When enabling a previously disabled CRTC with a non-CEA mode (!avi_vic) and
VRR_DISPLAY_MODE_GAME, crtc_state->adjusted_mode is still zero-initialized,
so vfront is calculated as 0. When switching modes on an already-active
CRTC, crtc_state->adjusted_mode still holds the previous mode's timings
from atomic_duplicate_state(), causing vtem->base_vfront to be computed
from the old mode (or causing atomic_check to fail with -EINVAL if the old
mode had vfront > U8_MAX).

Should this compute vfront from crtc_state->mode (or
connector_state_get_mode(conn_state)), matching how vtem->base_refresh_rate
and hdmi_validate_vrr() read the mode timings?

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