Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider: - [High] Logic Error: `align_max_sink_dsc_input_bpp()` unconditionally overrides `limits->pipe.max_bpp` to 0 for PCON passthrough. - [High] Logic Error / State Mismatch: Inconsistent connector context (`intel_dp->attached_connector`) used for MST streams breaks passthrough skipping and EDID derivations. - [High] Functional Bug: PCON passthrough DSC computation aborts due to missing bypasses for zeroed DPCD capabilities in `intel_dp_dsc_compute_config()` and `intel_dp_dsc_compute_params()`. - [Low] Compilation Failure: `intel_hdmi_dsc_get_num_slices()` is called with 5 arguments instead of 6. --
commit bcc24c289e2fb41a9b6176abaf305ef6fb96a79e 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, derives DSC parameters from the HDMI sink's EDID, and explicitly enables DP_DSC_PASSTHROUGH_EN on the PCON. > diff --git a/drivers/gpu/drm/i915/display/intel_dp.c > b/drivers/gpu/drm/i915/display/intel_dp.c > index 0cd5e6b5034cf..98602964b2339 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c [ ... ] > @@ -2393,7 +2403,19 @@ int intel_dp_dsc_compute_config(struct intel_dp > *intel_dp, > } > } > > - if (!intel_dp_dsc_get_slice_config(connector, adjusted_mode->crtc_clock, > + if (intel_dp->attached_connector->dp.pcon_dsc_passthrough) { [Severity: High] Will this configuration abort early before reaching the new passthrough logic? Earlier in intel_dp_dsc_compute_config(), there is an existing check: if (!intel_dp_dsc_supports_format(connector, pipe_config->output_format)) return -EINVAL; Since PCON passthrough devices have an all-zero DSC DPCD, does intel_dp_dsc_supports_format() evaluate to false and return -EINVAL here? Similarly, in intel_dp_dsc_compute_params(), line_buf_depth is extracted from the zeroed DPCD, which also results in 0 and an immediate abort: vdsc_cfg->line_buf_depth = min(INTEL_DP_DSC_MAX_LINE_BUF_DEPTH, drm_dp_dsc_sink_line_buf_depth(connector->dp.dsc_dpcd)); if (!vdsc_cfg->line_buf_depth) { ... return -EINVAL; } Are bypasses needed for these checks when doing PCON passthrough? > + /* > + * PCON passthrough: no PCON encoder DPCD, derive slice config > + * from the downstream HDMI 2.1 sink DSC capabilities in EDID. > + */ > + if (!intel_dp_pcon_passthrough_dsc_slice_config(intel_dp, > + pipe_config, > + > &pipe_config->dsc.slice_config)) { > + drm_dbg_kms(display->drm, > + "PCON DSC passthrough: no valid slice > config\n"); > + return -EINVAL; > + } > + } else if (!intel_dp_dsc_get_slice_config(connector, > adjusted_mode->crtc_clock, > adjusted_mode->crtc_hdisplay, > num_joined_pipes, > &pipe_config->dsc.slice_config)) > return -EINVAL; [ ... ] > @@ -2722,8 +2744,33 @@ intel_dp_compute_config_limits(struct intel_dp > *intel_dp, > return false; > } > > - if (dsc && !intel_dp_dsc_compute_pipe_bpp_limits(connector, limits)) > - return false; > + if (dsc) { > + if (connector->dp.pcon_dsc_passthrough) { > + /* > + * PCON passthrough: PCON has no DSC encoder DPCD so > + * intel_dp_dsc_compute_pipe_bpp_limits() would see > + * all-zero dsc_dpcd and fail. Use the downstream HDMI > + * sink DSC bpc cap to constrain the pipe bpp instead. > + */ > + const struct drm_display_info *info = > + &connector->base.display_info; > + u8 hdmi_max_bpc = info->hdmi.dsc_cap.bpc_supported ?: 8; > + int dsc_min_bpc = intel_dp_dsc_min_src_input_bpc(); > + int dsc_max_bpc = min_t(int, > + > intel_dp_dsc_max_src_input_bpc(display), > + hdmi_max_bpc); > + > + limits->pipe.min_bpp = max(limits->pipe.min_bpp, > + dsc_min_bpc * 3); > + limits->pipe.max_bpp = min(limits->pipe.max_bpp, > + dsc_max_bpc * 3); > + > + if (limits->pipe.min_bpp > limits->pipe.max_bpp) > + return false; > + } else if (!intel_dp_dsc_compute_pipe_bpp_limits(connector, > limits)) { > + return false; > + } > + } [Severity: High] Will the calculated limits->pipe.max_bpp be overridden to 0 later in this function? At the end of intel_dp_compute_config_limits(), there is an unconditional check: if (dsc) limits->pipe.max_bpp = align_max_sink_dsc_input_bpp(connector, limits->pipe.max_bpp); Since align_max_sink_dsc_input_bpp() reads the empty PCON dsc_dpcd, won't it return 0 and override the valid passthrough max_bpp calculation? [ ... ] > +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] Does this correctly identify MST topologies? Since intel_dp->attached_connector points to the root SST connector, connector->mst.port will always be NULL here. Will this cause the intended MST bypass to fail and incorrectly attempt passthrough on MST streams? > + > + if (!intel_dp_is_hdmi_2_1_sink(intel_dp)) > + return; > + > + if (!(connector->dp.dsc_dpcd[0] & DP_DSC_PASSTHROUGH_IS_SUPPORTED)) > + return; > + > + info = &connector->base.display_info; > + connector->dp.pcon_dsc_passthrough = info->hdmi.dsc_cap.v_1p2; > +} > + > +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] Does this fetch the correct EDID for MST streams? Using intel_dp->attached_connector fetches the display info from the root connector rather than the active MST branch connector. If the MST bypass above fails, will this derive capabilities from the wrong EDID? > + 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; > + int target_slices; > + int slices_per_pipe; > + > + /* > + * Derive the required slice count from the downstream HDMI 2.1 sink > + * DSC capabilities in EDID (not from the PCON encoder DPCD which is > + * all-zero for passthrough-only devices). > + */ > + target_slices = > intel_hdmi_dsc_get_num_slices(&crtc_state->hw.adjusted_mode, > + hdmi_max_slices, > + > crtc_state->hw.adjusted_mode.hdisplay, > + hdmi_max_slices, > + hdmi_throughput); [Severity: Low] Does this call have the correct number of arguments? intel_hdmi_dsc_get_num_slices() requires 6 arguments (including output_format), but only 5 are provided here. Will this cause a compilation failure? > + if (!target_slices) > + return false; > + > + drm_dbg_kms(display->drm, > + "PCON DSC passthrough: target %d slices from HDMI sink > EDID cap" > + " (max_slices=%d clk_per_slice=%d MHz)\n", > + target_slices, hdmi_max_slices, hdmi_throughput); -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
