Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] `intel_dp_pcon_passthrough_dsc_slice_config()` unconditionally fails for passthrough-only PCONs due to passing a zero slice count to `intel_hdmi_dsc_get_num_slices()`. - [High] `intel_dp_sink_set_dsc_passthrough()` skips enabling DSC passthrough on intermediate MST hubs when a PCON is connected. - [High] Incorrect uncompressed dotclk validation applied to DSC passthrough modes due to zero slice count. --
commit 8ca09416b752ce965e92cfcd6a72b6d5b9efeae5 Author: Xiao Lu <[email protected]> drm/i915/dp: enable DSC passthrough for HDMI 2.1 PCON devices This commit enables explicit driver control of DSC passthrough for HDMI 2.1 PCON devices that lack an internal DSC encoder. It derives DSC parameters from the HDMI sink's EDID and writes the appropriate passthrough enable bits. > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > b/drivers/gpu/drm/i915/display/intel_dp.c > index 0cd5e6b5034cf..939c707d34316 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c [ ... ] > @@ -2524,10 +2569,27 @@ 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) { > + /* > + * For PCON passthrough, use the HDMI sink EDID max_slices > + * capped by the PCON's own slice capability from dsc_dpcd. > + * If the PCON dsc_dpcd is all-zero, fall back to EDID only. > + */ > + const struct drm_display_info *info = > &connector->base.display_info; > + int pcon_max = > drm_dp_dsc_sink_max_slice_count(connector->dp.dsc_dpcd, > + false); > + > + dsc_slice_count = pcon_max ? > + min_t(int, info->hdmi.dsc_cap.max_slices, pcon_max) : > + info->hdmi.dsc_cap.max_slices; [Severity: High] While the slice count is calculated correctly here with a fallback, does this leave callers like intel_dp_mode_valid_format() and mst_connector_mode_valid_ctx() with an incorrect slice count? Those outer functions obtain the slice count by calling intel_dp_dsc_get_slice_count() directly. For passthrough-only PCONs with an all-zero DPCD, this returns 0. When intel_dp_dotclk_valid() is later called with a slice count of 0, will it incorrectly evaluate DSC passthrough modes against uncompressed dotclock limits? > + } else { > + dsc_slice_count = intel_dp_dsc_get_slice_count(connector, > + mode_clock, > + mode_hdisplay, > + > num_joined_pipes); > + } [ ... ] > @@ -3767,8 +3858,22 @@ intel_dp_sink_set_dsc_passthrough(const struct > intel_connector *connector, > bool enable) > { > struct intel_display *display = to_intel_display(connector); > - struct drm_dp_aux *aux = connector->mst.port ? > - connector->mst.port->passthrough_aux : NULL; > + struct drm_dp_aux *aux = NULL; > + > + if (connector->dp.pcon_dsc_passthrough) { > + /* > + * HDMI 2.1 PCON passthrough: write DP_DSC_PASSTHROUGH_EN > + * directly to the PCON so it forwards the compressed stream > + * to the HDMI sink. For SST use the root port aux; for MST > + * use the MST port aux which routes via REMOTE_DPCD_WRITE. > + */ > + if (connector->mst.port) > + aux = &connector->mst.port->aux; > + else > + aux = &intel_attached_dp((struct intel_connector > *)connector)->aux; > + } else if (connector->mst.port) { > + aux = connector->mst.port->passthrough_aux; > + } [Severity: High] When connecting a PCON through an MST hub, doesn't the driver need to instruct both the intermediate MST hub (passthrough_aux) and the PCON (connector->mst.port->aux) to pass through the DSC stream? The mutually exclusive if/else if structure here appears to force the driver to write DP_DSC_PASSTHROUGH_EN only to the PCON, bypassing the MST hub configuration entirely. Can this prevent the compressed stream from being properly forwarded? > > if (!aux) > return; [ ... ] > @@ -4298,6 +4403,112 @@ void intel_dp_check_frl_training(struct intel_dp > *intel_dp) > +static bool > +intel_dp_pcon_passthrough_dsc_slice_config(struct intel_dp *intel_dp, > + const struct intel_connector > *connector, > + const struct intel_crtc_state > *crtc_state, > + struct intel_dsc_slice_config > *config_ret) > +{ > + struct intel_display *display = to_intel_display(intel_dp); > + const struct drm_display_info *info = &connector->base.display_info; > + int num_joined_pipes = intel_crtc_num_joined_pipes(crtc_state); > + int hdmi_throughput = info->hdmi.dsc_cap.clk_per_slice; > + int hdmi_max_slices = info->hdmi.dsc_cap.max_slices; > + /* > + * Use the PCON's own DSC passthrough capability (from its virtual > + * DPCD) as the source constraint, mirroring what > + * intel_dp_pcon_dsc_enc_slices() does for encoder mode. This handles > + * multi-hop topologies (e.g. MST hub -> PCON) where the intermediate > + * hub virtual DPCD may be more restrictive than the HDMI sink EDID. > + */ > + int pcon_max_slices = > drm_dp_dsc_sink_max_slice_count(connector->dp.dsc_dpcd, false); > + int target_slices; > + int slices_per_pipe; > + > + /* > + * src_max_slice_width: pass INT_MAX so the HDMI spec limit of 2720px > + * applies; the source VDSC HW limit (5120px) is not the binding > constraint. > + * src_max_slices: use PCON dsc_dpcd slice capability as upper bound. > + */ > + target_slices = > intel_hdmi_dsc_get_num_slices(&crtc_state->hw.adjusted_mode, > + crtc_state->output_format, > + pcon_max_slices, INT_MAX, > + hdmi_max_slices, > + hdmi_throughput); [Severity: High] If a passthrough-only PCON has an all-zero DPCD DSC capability, drm_dp_dsc_sink_max_slice_count() will evaluate to 0 for pcon_max_slices. Does this cause intel_hdmi_dsc_get_num_slices() to unconditionally fail and return 0, since it requires src_max_slices >= 1? Should there be a fallback to EDID slice limits (e.g. pcon_max_slices ? pcon_max_slices : hdmi_max_slices) to prevent this? > + if (!target_slices) > + return false; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
