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