Re: [PATCH v2] drm/i915/display: Enable VRR/HDR over MST

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] AS-SDP computation is broken for MST streams because the branch device check unconditionally returns false.
- [High] VRR capability caching is incorrectly tied to DSC support, silently breaking VRR on MST monitors that lack DSC.
- [High] DP_MSA_TIMING_PAR_IGNORE_EN is unconditionally enabled based on capability rather than active VRR state.
- [High] Dynamic creation of `vrr_capable_property` after device registration during MST hotplug triggers a kernel WARN_ON and leaks memory.
--

commit 67043be5215717a8902d6610575b918e36e9945a
Author: Xiao Lu <xiaolu.xie@intel.com>

drm/i915/display: Enable VRR/HDR over MST

This commit introduces VRR and HDR metadata support over MST display streams.
It caches the downstream port's MSA timing par ignore capability and enables
AS-SDP and VSC SDP computation and transmission for MST connectors.

> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 6a869d0f6ffc0..657440e2cb648 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c

[ ... ]

> @@ -793,6 +793,9 @@ static int mst_stream_compute_config(struct intel_atomic_state *state,
>  			bxt_dpio_phy_calc_lane_lat_optim_mask(pipe_config->lane_count);
>  
>  	intel_vrr_compute_config(pipe_config, conn_state);
> +	intel_dp_compute_as_sdp(intel_dp, pipe_config);

[Severity: High]
Will this AS-SDP computation actually run for MST topologies?

In intel_dp_compute_as_sdp() (which calls intel_dp_needs_as_sdp()), there is
an existing check that unconditionally disables AS-SDP for branch devices:

drivers/gpu/drm/i915/display/intel_dp.c:intel_dp_needs_as_sdp()
    if (drm_dp_is_branch(intel_dp->dpcd))
        return false;

Since the primary DP device in an MST topology is always a branch device
(the MST hub), this check will always return false, silently aborting the
AS-SDP logic you are enabling here.

[ ... ]

> @@ -1274,6 +1295,24 @@ static void mst_stream_pre_enable(struct intel_atomic_state *state,
>  
>  	drm_dp_send_power_updown_phy(&intel_dp->mst.mgr, connector->mst.port, true);
>  
> +	if (intel_vrr_is_capable(connector)) {
> +		u8 val;
> +		ssize_t ret;
> +
> +		ret = drm_dp_mst_dpcd_read(&connector->mst.port->aux,
> +					   DP_DOWNSPREAD_CTRL, &val, 1);
> +		if (ret < 0) {
> +			drm_dbg_kms(display->drm,
> +				    "[CONNECTOR:%d:%s] Failed to read DP_DOWNSPREAD_CTRL: %zd\n",
> +				    connector->base.base.id,
> +				    connector->base.name, ret);
> +		} else {
> +			val |= DP_MSA_TIMING_PAR_IGNORE_EN;
> +			drm_dp_mst_dpcd_write(&connector->mst.port->aux,
> +					      DP_DOWNSPREAD_CTRL, &val, 1);
> +		}
> +	}

[Severity: High]
Does this alter the sink behavior unconditionally, even when VRR is not active?

The DP_MSA_TIMING_PAR_IGNORE_EN bit is being written based solely on
intel_vrr_is_capable(connector), bypassing any check for whether VRR is
actually enabled for this atomic state (e.g. crtc_state->vrr.enable).

This will program the sink to ignore MSA video timings even when a fixed
refresh rate is active. Could this lead to display corruption or sync loss
on monitors that expect strict MSA timings during fixed-refresh operation?

[ ... ]

> @@ -1745,6 +1799,14 @@ static int mst_topology_add_connector_properties(struct intel_dp *intel_dp,
>  	if (connector->base.max_bpc_property)
>  		drm_connector_attach_max_bpc_property(&connector->base, 6, 12);
>  
> +	/*
> +	 * Reuse the vrr_capable prop from the eDP connector (which is always
> +	 * initialized during driver load before device registration), because
> +	 * we're not allowed to create new props after device registration.
> +	 */
> +	if (HAS_VRR(display))
> +		drm_connector_attach_vrr_capable_property(&connector->base);

[Severity: High]
Does this safely reuse the property without triggering a kernel WARN_ON?

Although the comment mentions reusing the vrr_capable property, the code
does not seed connector->base.vrr_capable_property with the shared property
pointer before calling drm_connector_attach_vrr_capable_property().

Because it is unseeded, drm_connector_attach_vrr_capable_property() will
try to allocate a new property via drm_property_create_bool(). Creating
new properties after device registration is forbidden and triggers a
WARN_ON in __drm_mode_object_add().

This will also permanently leak a DRM property object in the device's
global property list on every MST hotplug.

[ ... ]

> @@ -1761,6 +1823,9 @@ intel_dp_mst_read_decompression_port_dsc_caps(struct intel_dp *intel_dp,
>  	if (drm_dp_read_dpcd_caps(connector->dp.dsc_decompression_aux, dpcd_caps) < 0)
>  		return;
>  
> +	connector->dp.mst_msa_timing_par_ignore =
> +		drm_dp_sink_can_do_video_without_timing_msa(dpcd_caps);

[Severity: High]
Will VRR be disabled on MST monitors that do not support DSC decompression?

The flag mst_msa_timing_par_ignore is populated here, but this function has
an early return higher up:

    if (!connector->dp.dsc_decompression_aux)
        return;

For non-DSC MST displays, this flag remains uninitialized (false). Later,
intel_vrr_is_capable() relies on this cached value to determine VRR
support:

drivers/gpu/drm/i915/display/intel_vrr.c:intel_vrr_is_capable()
    if (!connector->dp.mst_msa_timing_par_ignore)
        return false;

This will cause intel_vrr_is_capable() to unconditionally reject VRR for
any MST display that lacks a DSC decompression AUX port.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902012852.250993-1-xiaolu.xie@intel.com?part=1