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 <[email protected]> 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/[email protected]?part=1
