Re: [PATCH v3] 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] Live hardware DPCD read over MST sideband during the atomic check phase causes severe latency and potential Denial of Service.
- [Medium] The `vrr_capable` property value cannot be updated for MST connectors because `connector->base.vrr_capable_property` is never assigned.
- [Medium] VRR and AS-SDP are incorrectly allowed for PCONs (DP-to-HDMI adapters) connected via MST hubs, bypassing the explicit restriction against branch devices.
--

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

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

This commit enables Variable Refresh Rate (VRR) and HDR metadata over MST
in the i915 driver. It retrieves the downstream port's MSA timing
capabilities to support VRR and transmits AS-SDP and HDR metadata
infoframes over MST streams.

> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 113d767e62e9c..4b196d26d27f8 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c

[ ... ]

> @@ -3107,9 +3110,12 @@ static bool intel_dp_needs_as_sdp(struct intel_dp *intel_dp,
>  		return false;
>  
>  	/*
> -	 * #TODO: Add AS SDP v1 support for PCONs (DP branch devices).
> +	 * AS-SDP is not yet supported for PCONs (DP-to-HDMI converters).
> +	 * MST hubs are also branch devices but do support AS-SDP forwarding
> +	 * to downstream sinks; allow it for MST streams.
>  	 */
> -	if (drm_dp_is_branch(intel_dp->dpcd))
> +	if (drm_dp_is_branch(intel_dp->dpcd) &&
> +	    !intel_crtc_has_type(crtc_state, INTEL_OUTPUT_DP_MST))
>  		return false;

[Severity: Medium]
Does this blanket exception for MST streams allow AS-SDP to be sent to
explicitly unsupported branch devices (like PCONs) that are downstream of an
MST hub?

[ ... ]

> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index 3be1643f8d030..e66f9508cb720 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c

[ ... ]

> @@ -1451,6 +1492,15 @@ static int mst_connector_get_ddc_modes(struct drm_connector *_connector)
>  
>  	ret = intel_connector_update_modes(&connector->base, drm_edid);
>  
> +	if (HAS_VRR(display)) {
> +		bool vrr_capable = intel_vrr_is_capable(connector);
> +
> +		drm_dbg_kms(display->drm, "[CONNECTOR:%d:%s] VRR capable: %s\n",
> +			    connector->base.base.id, connector->base.name,
> +			    str_yes_no(vrr_capable));
> +		drm_connector_set_vrr_capable_property(&connector->base, vrr_capable);
> +	}
> +

[Severity: Medium]
Will this call to drm_connector_set_vrr_capable_property() silently do
nothing because connector->base.vrr_capable_property is never assigned for
MST connectors?

The property is attached to the DRM object in
mst_topology_add_connector_properties(), but the pointer itself remains
NULL, which makes the feature invisible to userspace.

[ ... ]

> @@ -1745,6 +1799,23 @@ 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.
> +	 */
> +	if (HAS_VRR(display) && intel_dp->attached_connector &&
> +	    intel_dp->attached_connector->base.vrr_capable_property)
> +		drm_object_attach_property(&connector->base.base,
> +					   intel_dp->attached_connector->base.vrr_capable_property,
> +					   0);
> +

[Severity: Medium]
Should this also explicitly assign connector->base.vrr_capable_property so
that later updates to the property work correctly?

[ ... ]

> diff --git a/drivers/gpu/drm/i915/display/intel_vrr.c b/drivers/gpu/drm/i915/display/intel_vrr.c
> index 51e4f3309b8b1..13cd5701bf2c1 100644
> --- a/drivers/gpu/drm/i915/display/intel_vrr.c
> +++ b/drivers/gpu/drm/i915/display/intel_vrr.c

[ ... ]

> @@ -55,8 +55,27 @@ 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) {
> +			u8 dpcd[DP_RECEIVER_CAP_SIZE];
> +
> +			/*
> +			 * Use cached MSA timing ignore capability from the DFP
> +			 * sink's virtual DPCD, set during connector init.
> +			 */
> +			if (!connector->dp.mst_msa_timing_par_ignore)
> +				return false;
> +
> +			/*
> +			 * Also verify live via LCT=2 REMOTE_DPCD_READ to the
> +			 * DFP sink's actual DPCD 0x007.
> +			 */
> +			if (drm_dp_read_dpcd_caps(&connector->mst.port->aux, dpcd) < 0)
> +				return false;

[Severity: High]
Can this live hardware DPCD read over the MST sideband cause latency and
potential Denial of Service during the atomic check phase?

Since mst_stream_compute_config() calls intel_vrr_compute_config(), which
in turn calls intel_vrr_is_capable(), this adds a blocking sideband I/O
transaction to the hot KMS atomic_check path. Untrusted userspace
applications spamming DRM_IOCTL_MODE_ATOMIC with the TEST_ONLY flag could
exploit this to saturate the CPU and MST sideband communication channel.

> +			if (!drm_dp_sink_can_do_video_without_timing_msa(dpcd))
> +				return false;
> +
> +			break;

[Severity: Medium]
Is there a missing check for drm_dp_is_branch(dpcd) here?

By omitting it for MST streams, branch devices like PCONs connected behind
MST hubs might be incorrectly considered VRR-capable, bypassing the explicit
restriction against branch devices.

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