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