Re: [RFC v1 3/3] drm/i915/vrr: Allow VRR on HDMI 2.1 sinks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] drm/i915/dip: emp_as_sdp_tl not computed for HDMI VRR due to DP-only call
- [Medium] drm/i915/hdmi: stale vrr_capable property on disconnect in intel_hdmi_detect()
- [Medium] drm/i915/vrr: fallback to monitor_range.max_vfreq fails when vrr_max is 0
- [Medium] drm/i915/vrr: VRR enabled in DVI mode in intel_vrr_compute_config()
- [Low] drm/i915/dip: undeclared HDMI_EMP_DATA() macro in intel_dip_write_hdmi_vtem()
--

commit d2e831520b52e253f435c04bcef565c5c658d639
Author: Alan Stead <alan.stead@intel.com>

drm/i915/vrr: Allow VRR on HDMI 2.1 sinks

This commit enables VRR support on HDMI 2.1 sinks for display version 20+
by using the VRR range from the HDMI Forum VSDB in display_info.hdmi.vrr_cap
and attaching and updating the vrr_capable property on HDMI connectors.

> diff --git a/drivers/gpu/drm/i915/display/intel_hdmi.c b/drivers/gpu/drm/i915/display/intel_hdmi.c
> index b6fa1a1558b82..4854e86c1762a 100644
> --- a/drivers/gpu/drm/i915/display/intel_hdmi.c
> +++ b/drivers/gpu/drm/i915/display/intel_hdmi.c
> @@ -2490,6 +2490,8 @@ intel_hdmi_unset_edid(struct drm_connector *_connector)
>  
>  	drm_edid_free(connector->detect_edid);
>  	connector->detect_edid = NULL;
> +
> +	drm_connector_set_vrr_capable_property(&connector->base, false);
>  }

[Severity: Medium]
When an HDMI 2.1 VRR-capable sink is unplugged on display version 20+
platforms, will intel_hdmi_unset_edid() actually be reached to reset the
vrr_capable property to false?

In drivers/gpu/drm/i915/display/intel_hdmi.c, intel_hdmi_detect() checks if
the port is connected before calling intel_hdmi_unset_edid():

	if (DISPLAY_VER(display) >= 11 &&
	    !intel_digital_port_connected(encoder))
		goto out;

	intel_hdmi_unset_edid(&connector->base);

	if (intel_hdmi_set_edid(&connector->base))
		status = connector_status_connected;

out:
	intel_display_power_put(display, POWER_DOMAIN_GMBUS, wakeref);

	if (status != connector_status_connected)
		cec_notifier_phys_addr_invalidate(intel_hdmi->cec_notifier);

Because DISPLAY_VER(display) >= 11 is true for all display version 20+
hardware, unplugging the sink causes intel_hdmi_detect() to jump directly to
out without calling intel_hdmi_unset_edid(), leaving the connector's
vrr_capable property stuck at true while disconnected.

> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 142ba2db52fda..c2774fd690473 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c
> @@ -39,11 +39,33 @@
>  #define DCB_CORRECTION_AGGRESSIVENESS	1000 /* ms × 100; 10 ms */
>  #define DCB_BLANK_TARGET		50
>  
> +/*
> + * The refresh rate range the sink supports VRR in. HDMI 2.1 sinks advertise
> + * it in the HDMI Forum VSDB, where VRRmax = 0 means "the refresh rate of the
> + * video timing" (fall back to the EDID range descriptor for the maximum).
> + */
> +static void intel_vrr_sink_range(struct intel_connector *connector,
> +				 int *min_vfreq, int *max_vfreq)
> +{
> +	const struct drm_display_info *info = &connector->base.display_info;
> +	const struct drm_hdmi_vrr_cap *vrr_cap = &info->hdmi.vrr_cap;
> +
> +	*min_vfreq = info->monitor_range.min_vfreq;
> +	*max_vfreq = info->monitor_range.max_vfreq;
> +
> +	if (connector->base.connector_type == DRM_MODE_CONNECTOR_HDMIA &&
> +	    vrr_cap->supported) {
> +		*min_vfreq = vrr_cap->vrr_min;
> +		if (vrr_cap->vrr_max)
> +			*max_vfreq = vrr_cap->vrr_max;
> +	}
> +}

[Severity: Medium]
Can falling back to info->monitor_range.max_vfreq when vrr_cap->vrr_max is 0
reject valid HDMI 2.1 VRR sinks?

Per the HDMI 2.1 specification and struct drm_hdmi_vrr_cap, vrr_max == 0
indicates that the upper VRR bound is the refresh rate of the active video
timing (and vrr_max == 0 is required for any sink whose maximum VRR rate is
below 100 Hz, since HF-VSDB only encodes 0 or values >= 100).

However, drm_get_monitor_range() in drivers/gpu/drm/drm_edid.c only populates
info->monitor_range.max_vfreq when the EDID revision is >= 4,
DRM_EDID_FEATURE_CONTINUOUS_FREQ is set, and a 0xfd Display Range Limits
descriptor with DRM_EDID_RANGE_LIMITS_ONLY_FLAG is present. On HDMI 2.1 sinks
that advertise vrr_max == 0 in the HF-VSDB without a DisplayPort-style
continuous-frequency Range Limits Only descriptor,
info->monitor_range.max_vfreq remains 0, causing intel_vrr_sink_range() to
set *max_vfreq = 0 and both intel_vrr_is_capable() and
intel_vrr_is_in_range() to return false.

[ ... ]

> @@ -78,24 +100,36 @@ bool intel_vrr_is_capable(struct intel_connector *connector)
>  			return false;
>  
>  		break;
> +	case DRM_MODE_CONNECTOR_HDMIA:
> +		/*
> +		 * HDMI 2.1 VRR is signalled to the sink with the VTEM, which
> +		 * is sent using the EMP hardware of display version 20+.
> +		 */
> +		if (DISPLAY_VER(display) < 20 ||
> +		    !connector->base.display_info.hdmi.vrr_cap.supported)
> +			return false;
> +		break;

[Severity: High]
When VRR is enabled on an HDMI connector, does crtc_state->dip.emp_as_sdp_tl
ever get computed?

In drivers/gpu/drm/i915/display/intel_dip.c,
intel_dip_get_as_sdp_transmission_line() has an HDMI VRR branch:

	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_HDMI) &&
	    crtc_state->vrr.in_range) {
		const struct drm_display_mode *mode = &crtc_state->hw.adjusted_mode;
		int max_tl = crtc_state->set_context_latency +
			crtc_state->vrr.guardband - 1;
		int tl = mode->crtc_vtotal - mode->crtc_vdisplay - 1;

		if (max_tl > 0)
			tl = min(tl, max_tl);

		return tl;
	}

That helper is only called from intel_dip_sdp_tl_compute_config_late(), which
in turn is only called via intel_ddi_compute_config_late() ->
intel_dp_compute_config_late() -> intel_dp_sdp_compute_config_late(). In
drivers/gpu/drm/i915/display/intel_ddi.c, intel_ddi_compute_config_late()
guards that call behind intel_crtc_has_dp_encoder():

	if (intel_crtc_has_dp_encoder(crtc_state))
		ret = intel_dp_compute_config_late(encoder, crtc_state, conn_state);

For INTEL_OUTPUT_HDMI, intel_dip_sdp_tl_compute_config_late() is never
called, so crtc_state->dip.emp_as_sdp_tl stays 0 and
intel_vrr_set_transcoder_timings() -> intel_dip_write_emp_as_sdp_tl()
programs EMP_AS_SDP_TL to 0 instead of the calculated transmission line.

[Severity: Low]
Also in the HDMI VTEM support enabled by this path, does
intel_dip_write_hdmi_vtem() in drivers/gpu/drm/i915/display/intel_dip.c build
when CONFIG_DRM_I915 or CONFIG_DRM_XE is enabled?

intel_dip_write_hdmi_vtem() writes the packet body using HDMI_EMP_DATA():

	for (i = 0; i < ARRAY_SIZE(pb); i += 4)
		intel_de_write(display, HDMI_EMP_DATA(display, cpu_transcoder),
			       pb[i] | pb[i + 1] << 8 | pb[i + 2] << 16 |
			       pb[i + 3] << 24);

However, drivers/gpu/drm/i915/display/intel_dip_regs.h only defines
HDMI_EMP_CTL and HDMI_EMP_HEADER:

/* HDMI 2.1 Extended Metadata Packet (EMP) control/header; payload is HDMI_EMP_DATA */
#define _HDMI_EMP_CTL_A				0x600d0
#define HDMI_EMP_CTL(display, trans)		_MMIO_TRANS2((display), (trans), _HDMI_EMP_CTL_A)
...
#define _HDMI_EMP_HEADER_A			0x600d4
#define HDMI_EMP_HEADER(display, trans)		_MMIO_TRANS2((display), (trans), _HDMI_EMP_HEADER_A)

HDMI_EMP_DATA is not defined in intel_dip_regs.h or anywhere else in the
tree, resulting in an implicit declaration build failure in
intel_dip_write_hdmi_vtem().

>  	default:
>  		return false;
>  	}
>  
> -	if (!info->monitor_range.min_vfreq || !info->monitor_range.max_vfreq ||
> -	    info->monitor_range.min_vfreq > info->monitor_range.max_vfreq)
> +	intel_vrr_sink_range(connector, &min_vfreq, &max_vfreq);
> +
> +	if (!min_vfreq || !max_vfreq || min_vfreq > max_vfreq)
>  		return false;
>  
> -	return info->monitor_range.max_vfreq - info->monitor_range.min_vfreq > 10;
> +	return max_vfreq - min_vfreq > 10;
>  }
>  
>  bool intel_vrr_is_in_range(struct intel_connector *connector, int vrefresh)
>  {
> -	const struct drm_display_info *info = &connector->base.display_info;
> +	int min_vfreq, max_vfreq;
> +
> +	intel_vrr_sink_range(connector, &min_vfreq, &max_vfreq);
>  
>  	return intel_vrr_is_capable(connector) &&
> -		vrefresh >= info->monitor_range.min_vfreq &&
> -		vrefresh <= info->monitor_range.max_vfreq;
> +		vrefresh >= min_vfreq &&
> +		vrefresh <= max_vfreq;
>  }

[Severity: Medium]
Should intel_vrr_compute_config() also verify that crtc_state->has_hdmi_sink
(or crtc_state->has_infoframe) is true before enabling VRR on an HDMI
connector?

If userspace forces DVI mode via the connector audio property (setting
force_audio to HDMI_AUDIO_OFF_DVI) while an HDMI 2.1 VRR sink is connected,
intel_hdmi_compute_config() leaves pipe_config->has_hdmi_sink and
pipe_config->has_infoframe as false, and intel_ddi_enable_transcoder_func()
programs TRANS_DDI_MODE_SELECT_DVI:

	if (crtc_state->has_hdmi_sink)
		temp |= TRANS_DDI_MODE_SELECT_HDMI;
	else
		temp |= TRANS_DDI_MODE_SELECT_DVI;

Because intel_vrr_compute_config() only checks intel_vrr_is_in_range()
against connector->base.display_info, it still sets crtc_state->vrr.in_range
and crtc_state->vrr.enable to true in DVI mode, even though the AVI InfoFrame
is not computed and the transcoder cannot transmit HDMI data island packets
such as the VTEM.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261030203314.1085262-1-alan.stead@intel.com?part=3