Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] Flawed implementation of PCON DSC passthrough for MST topologies incorrectly targets the root SST connector, leading to broken MST support, data races, and lockless accesses. - [High] Mode validation unconditionally rejects all DSC modes for passthrough-only PCONs by capping the slice count with an all-zero DPCD capability. --
commit f1a37f1f267d3517b3c9a3c61b716d9649b3222e Author: Xiao Lu <[email protected]> drm/i915/dp: enable DSC passthrough for HDMI 2.1 PCON devices This patch adds explicit driver control of PCON DSC passthrough. It detects passthrough capability at connect time and uses the downstream HDMI 2.1 sink DSC capabilities instead of the PCON's encoder constraints to derive DSC parameters. > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > b/drivers/gpu/drm/i915/display/intel_dp.c > index 0cd5e6b5034cf..e23e4e8b32bf2 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c [ ... ] > @@ -2524,10 +2567,20 @@ bool intel_dp_mode_valid_with_dsc(struct > intel_connector *connector, > num_joined_pipes, > output_format, > pipe_bpp, INT_MAX); > - int dsc_slice_count = intel_dp_dsc_get_slice_count(connector, > - mode_clock, > - mode_hdisplay, > - num_joined_pipes); > + int dsc_slice_count; > + > + if (connector->dp.pcon_dsc_passthrough) { > + const struct drm_display_info *info = > &connector->base.display_info; > + > + dsc_slice_count = min_t(int, info->hdmi.dsc_cap.max_slices, > + > drm_dp_dsc_sink_max_slice_count(connector->dp.dsc_dpcd, > + false)); [Severity: High] Does this unconditionally reject all modes requiring DSC for passthrough-only PCONs? The commit message notes that the PCON encoder DPCD is all-zero for passthrough-only devices. When connector->dp.dsc_dpcd is all-zero, drm_dp_dsc_sink_max_slice_count() evaluates to 0. As a result, dsc_slice_count becomes 0 due to the min_t() cap, and the subsequent check in intel_dp_mode_valid_with_dsc() (if dsc_slice_count == 0) will return false, failing mode validation. [ ... ] > @@ -3755,6 +3833,10 @@ intel_dp_sink_set_dsc_decompression(struct > intel_connector *connector, > { > struct intel_display *display = to_intel_display(connector); > > + /* PCON passthrough: PCON forwards the stream, sink decompresses. */ > + if (!connector->mst.dp && connector->dp.pcon_dsc_passthrough) > + return; [Severity: High] Will this short-circuit logic fail to bypass decompression for MST leaves? For an MST leaf connector, connector->mst.dp points to the root intel_dp, meaning the !connector->mst.dp check evaluates to false. This appears to contradict the commit message's note about ensuring intel_dp_sink_set_dsc_decompression() is correctly bypassed for PCON passthrough in MST topologies. [ ... ] > @@ -4298,6 +4391,116 @@ void intel_dp_check_frl_training(struct intel_dp > *intel_dp) > } > } > > +/* > + * intel_dp_pcon_set_dsc_passthrough_cap - cache PCON DSC passthrough > capability [ ... ] > + */ > +static void > +intel_dp_pcon_set_dsc_passthrough_cap(struct intel_dp *intel_dp) > +{ > + struct intel_connector *connector = intel_dp->attached_connector; > + const struct drm_display_info *info; > + > + if (!connector) > + return; > + > + connector->dp.pcon_dsc_passthrough = false; > + > + /* > + * PCON DSC passthrough is only supported for SST direct connections. > + * In MST topologies the intermediate hub may report incorrect virtual > + * DPCD for the downstream PCON port, leading to mismatched DSC > + * parameters. Skip passthrough for MST until hub firmware correctly > + * reflects the PCON's capabilities. > + */ > + if (connector->mst.port) > + return; [Severity: High] When called during MST connector mode validation, does this locklessly mutate the root SST connector's state instead of evaluating the MST leaf? Since mst_connector_get_ddc_modes() passes the root intel_dp (connector->mst.dp), intel_dp->attached_connector resolves to the root SST connector. Consequently, connector->mst.port is NULL for the root connector, the early return is skipped, and it locklessly modifies the root connector's pcon_dsc_passthrough flag using potentially stale display_info data. [ ... ] > +static bool > +intel_dp_pcon_passthrough_dsc_slice_config(struct intel_dp *intel_dp, > + const struct intel_crtc_state > *crtc_state, > + struct intel_dsc_slice_config > *config_ret) > +{ > + struct intel_display *display = to_intel_display(intel_dp); > + struct intel_connector *connector = intel_dp->attached_connector; > + const struct drm_display_info *info = &connector->base.display_info; [Severity: High] Could this result in using the wrong display_info during an MST stream's atomic check? If this is called for an MST stream, intel_dp is the root DP device, making intel_dp->attached_connector the root SST connector. This causes the function to read the root connector's display_info rather than the MST leaf's display_info. > diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c > b/drivers/gpu/drm/i915/display/intel_dp_mst.c > index 6a869d0f6ffc0..2653f2347f235 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c > +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c [ ... ] > @@ -1451,6 +1451,13 @@ static int mst_connector_get_ddc_modes(struct > drm_connector *_connector) > > ret = intel_connector_update_modes(&connector->base, drm_edid); > > + /* > + * Re-evaluate PCON DSC passthrough capability now that the MST > + * connector's EDID has been parsed and display_info.hdmi.dsc_cap > + * is populated. > + */ > + intel_dp_pcon_set_dsc_passthrough_cap(intel_dp); [Severity: High] Is intel_dp pointing to the root SST port here rather than the MST leaf? As noted above, passing the root intel_dp into intel_dp_pcon_set_dsc_passthrough_cap() causes the capability evaluation to incorrectly target the root connector rather than the MST stream connector. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
