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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Sleeping in Atomic Context via DPCD reads/writes
- [Medium] Stale Cached Hardware State for MSA timing par ignore capability
- [Low] Duplicated block of comments for vrr_capable_property
--

commit 3a17e2d16d11206d1e9f75d12cdff011a2baaa07
Author: Xiao Lu <xiaolu.xie@intel.com>

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

This commit enables Variable Refresh Rate (VRR) and High Dynamic Range (HDR)
metadata over DisplayPort Multi-Stream Transport (MST). It removes the hard
MST block in the VRR capability check, utilizes DPCD reads to determine MSA
timing ignore capability, and manages related connector properties.

> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c

[ ... ]

> @@ -1157,6 +1160,24 @@ static void mst_stream_post_disable(struct intel_atomic_state *state,
>  	 * Power down mst path before disabling the port, otherwise we end
>  	 * up getting interrupts from the sink upon detecting link loss.
>  	 */
> +	if (old_crtc_state->vrr.enable) {
> +		u8 val;
> +		ssize_t ret;
> +
> +		ret = drm_dp_mst_dpcd_read(&connector->mst.port->aux,
> +					   DP_DOWNSPREAD_CTRL, &val, 1);

[Severity: High]
Encoder atomic callbacks like mst_stream_post_disable() execute in atomic
context where sleeping is forbidden. Does the new drm_dp_mst_dpcd_read()
function sleep?

It appears to rely on a call chain that can sleep:

drm_dp_mst_dpcd_read()
   drm_dp_send_dpcd_read()
      kzalloc_obj() // Uses implicit GFP_KERNEL allocations
      drm_dp_mst_wait_tx_reply() // Uses sleepable wait primitives

Will this cause a kernel splat (e.g., "BUG: sleeping function called from
invalid context") or system hang during a modeset?

[ ... ]

> @@ -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 (pipe_config->vrr.enable) {
> +		u8 val;
> +		ssize_t ret;
> +
> +		ret = drm_dp_mst_dpcd_read(&connector->mst.port->aux,
> +					   DP_DOWNSPREAD_CTRL, &val, 1);

[Severity: High]
Similarly, mst_stream_pre_enable() executes in atomic context. Does calling
drm_dp_mst_dpcd_read() here also violate the atomic context sleeping
prohibition?

[ ... ]

> @@ -1745,6 +1811,34 @@ 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.
> +	 */
> +	/*
> +	 * Reuse the vrr_capable property from the root intel_dp SST connector
> +	 * which was created during driver load (before device registration).
> +	 * MST connectors are created after device registration, so we cannot
> +	 * create new properties here - we must attach the existing one.
> +	 */
> +	/*
> +	 * Reuse the vrr_capable property from the root intel_dp SST connector
> +	 * which was created during driver load (before device registration).
> +	 * MST connectors are created after device registration, so we cannot
> +	 * create new properties here - we must attach the existing one.
> +	 * Also assign connector->vrr_capable_property so that
> +	 * drm_connector_set_vrr_capable_property() can update the value.
> +	 */

[Severity: Low]
this isn't a bug, but you've duplicated this comment block multiple times.
Was this pasted by accident?

[ ... ]

> @@ -1755,6 +1849,18 @@ intel_dp_mst_read_decompression_port_dsc_caps(struct intel_dp *intel_dp,
>  	u8 dpcd_caps[DP_RECEIVER_CAP_SIZE];
>  	struct drm_dp_desc desc;
>  
> +	/*
> +	 * Read MSA timing par ignore capability from the sink's virtual DPCD.
> +	 * This is independent of DSC support so read it before the DSC check.
> +	 */
> +	{
> +		u8 mst_dpcd[DP_RECEIVER_CAP_SIZE];
> +
> +		if (drm_dp_read_dpcd_caps(&connector->mst.port->aux, mst_dpcd) == 0)
> +			connector->dp.mst_msa_timing_par_ignore =
> +				drm_dp_sink_can_do_video_without_timing_msa(mst_dpcd);
> +	}

[Severity: Medium]
Because intel_dp_mst_read_decompression_port_dsc_caps() is only called during
connector initialization, can this result in stale capabilities being cached?

If a user hot-swaps a downstream sink on an MST branch device, the MST
topology manager might reuse the existing connector without destroying it or
re-running its initialization callbacks.

Will connector->dp.mst_msa_timing_par_ignore retain the stale capability of
the previously connected monitor?

> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c

[ ... ]

> @@ -55,8 +55,19 @@ bool intel_vrr_is_capable(struct intel_connector *connector)
>  			return false;
>  		fallthrough;
>  	case DRM_MODE_CONNECTOR_DisplayPort:
> -		if (connector->mst.dp)
> -			return false;
> +		if (connector->mst.dp) {
> +			/*
> +			 * Use cached MSA timing ignore capability from the DFP
> +			 * sink's virtual DPCD, set during connector init in
> +			 * intel_dp_mst_read_decompression_port_dsc_caps().
> +			 * Avoid live sideband reads here as this function is
> +			 * called from the atomic check path.
> +			 */
> +			if (!connector->dp.mst_msa_timing_par_ignore)
> +				return false;

[Severity: Medium]
As mentioned above, if the cached mst_msa_timing_par_ignore value becomes
stale due to a hotplug event, could intel_vrr_is_capable() mistakenly enable
or disable VRR functionality for the new monitor based on the old monitor's
capabilities?

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