Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider: - [High] PCON DSC passthrough slice calculation bypasses source and DP link minimum slice requirements. - [High] `intel_dp_pcon_set_dsc_passthrough_cap` incorrectly updates the root connector's state when probing MST streams, causing data races. - [High] Unconditional RGB conversion for all PCON passthrough streams corrupts YCbCr formats. - [High] Passing `hdisplay` as `src_max_slice_width` prevents 1-slice DSC configurations for all standard resolutions. - [High] PCON DSC passthrough modes are incorrectly rejected during mode validation because `intel_dp_mode_valid` relies on the all-zero PCON DPCD. - [High] Missing horizontal display width divisibility check for DSC slice count in PCON DSC passthrough. --
commit 78dd30c6b107d4f8cce9fa9760f0a389a8f18b26 Author: Xiao Lu <[email protected]> drm/i915/dp: enable DSC passthrough for HDMI 2.1 PCON devices HDMI 2.1 PCON devices (e.g. Parade PS196) that do not have an internal DSC encoder can still forward a DSC-compressed stream from the DP source to the downstream HDMI 2.1 sink unchanged (passthrough mode). The HDMI sink (e.g. Lenovo ThinkVision Pro 27UD-10) then decompresses the stream itself. This mode is signalled by DP_DSC_PASSTHROUGH_IS_SUPPORTED (DPCD 0x060 bit1) 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..90a2ba6122e46 100644 > --- a/drivers/gpu/drm/i915/display/intel_dp.c > +++ b/drivers/gpu/drm/i915/display/intel_dp.c > @@ -1259,8 +1259,14 @@ bool intel_dp_has_dsc(const struct intel_connector > *connector) > connector->panel.vbt.edp.dsc_disable) > return false; > > - if (!drm_dp_sink_supports_dsc(connector->dp.dsc_dpcd)) > - return false; > + if (!drm_dp_sink_supports_dsc(connector->dp.dsc_dpcd)) { > + /* > + * PCON passthrough: PCON has no DSC decoder so dsc_dpcd is > + * zero, but if pcon_dsc_passthrough is set (cached at connect > + * time) the downstream HDMI 2.1 sink will decompress instead. > + */ > + return connector->dp.pcon_dsc_passthrough; > + } > > return true; > } [Severity: High] This update handles the all-zero PCON DPCD for capabilities, but is there a similar update missing in intel_dp_mode_valid() and intel_dp_mode_valid_with_dsc()? During mode validation, intel_dp_mode_valid() calls intel_dp_dsc_compute_max_bpp() and intel_dp_dsc_get_slice_count(), which still rely on the all-zero PCON DPCD. For passthrough-only PCONs, connector->dp.dsc_dpcd is entirely zeroes, causing intel_dp_dsc_get_slice_count() to find 0 slices. This results in dsc_slice_count == 0 inside intel_dp_mode_valid_with_dsc(), which immediately returns false and silently rejects valid DSC modes during EDID probing. [ ... ] > @@ -1866,28 +1872,42 @@ static int intel_dp_dsc_compute_params(const struct > intel_connector *connector, > if (ret) > return ret; > > - vdsc_cfg->dsc_version_major = > - (connector->dp.dsc_dpcd[DP_DSC_REV - DP_DSC_SUPPORT] & > - DP_DSC_MAJOR_MASK) >> DP_DSC_MAJOR_SHIFT; > - vdsc_cfg->dsc_version_minor = > - min(intel_dp_source_dsc_version_minor(display), > - intel_dp_sink_dsc_version_minor(connector->dp.dsc_dpcd)); > - if (vdsc_cfg->convert_rgb) > - vdsc_cfg->convert_rgb = > - connector->dp.dsc_dpcd[DP_DSC_DEC_COLOR_FORMAT_CAP - > DP_DSC_SUPPORT] & > - DP_DSC_RGB; > - > - 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) { > - drm_dbg_kms(display->drm, > - "DSC Sink Line Buffer Depth invalid\n"); > - return -EINVAL; > - } > + if (connector->dp.pcon_dsc_passthrough) { > + /* > + * For PCON passthrough the HDMI 2.1 sink decompresses, not the > + * PCON. The PCON dsc_dpcd fields reflect passthrough device > + * constraints and are not valid for configuring the source VDSC > + * engine. Use DSC 1.2 sink defaults instead. > + */ > + vdsc_cfg->dsc_version_major = 1; > + vdsc_cfg->dsc_version_minor = > min(intel_dp_source_dsc_version_minor(display), 2); > + vdsc_cfg->convert_rgb = true; [Severity: High] Does this unconditional assignment override the correctly computed convert_rgb value for YCbCr pipelines? intel_dsc_compute_params() correctly sets vdsc_cfg->convert_rgb = false for YCbCr formats. Unconditionally overwriting it with true here will cause the VDSC engine to apply RGB-to-YCoCg conversion to data that is already in YCbCr format, corrupting the colors. [ ... ] > +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 incorrectly update the root connector's state when probing MST streams, causing data races? When intel_dp_get_dsc_sink_cap() is called during MST port probing, it passes intel_attached_dp(connector), which returns the root port's intel_dp. Here, intel_dp->attached_connector resolves to the root SST connector instead of the MST connector. Because the root connector isn't an MST port, the connector->mst.port early return fails. This causes the code to read and modify the shared root connector's pcon_dsc_passthrough flag, potentially concurrently with other operations on every MST display hotplug or EDID read. [ ... ] > +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; > + 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, > + crtc_state->output_format, > + hdmi_max_slices, > + > crtc_state->hw.adjusted_mode.hdisplay, [Severity: High] Does passing hdisplay as src_max_slice_width prevent 1-slice DSC configurations for all standard resolutions? intel_hdmi_dsc_get_num_slices() limits max_slice_width to min(MAX_HDMI_SLICE_WIDTH, src_max_slice_width). By passing hdisplay as src_max_slice_width, max_slice_width becomes exactly hdisplay. When evaluating 1 slice, slice_width = hdisplay. The loop condition if (slice_width >= max_slice_width) evaluates to if (hdisplay >= hdisplay), which is always true. This immediately increments min_slices and rejects the 1-slice configuration for any resolution where hdisplay <= 2720 (e.g., 1080p, 1440p), forcing an unnecessary fallback to 2+ slices. > + hdmi_max_slices, > + hdmi_throughput); > + 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); > + > + for (slices_per_pipe = 1; slices_per_pipe <= 4; slices_per_pipe++) { > + struct intel_dsc_slice_config config; > + > + if (!intel_dsc_get_slice_config(display, > + num_joined_pipes, > slices_per_pipe, > + &config)) > + continue; > + > + if (intel_dsc_line_slice_count(&config) == target_slices) { [Severity: High] Are we missing a validation against the source VDSC engine's minimum slice requirements here? This strict equality check enforces the HDMI sink's minimum slice count, but it completely bypasses intel_dp_dsc_min_slice_count() which enforces Intel's source VDSC hardware limits. Any display mode where the VDSC engine requires more slices than the HDMI link minimum will be misconfigured. [Severity: High] Is a horizontal display width divisibility check also missing here? The loop accepts the configuration without verifying that the active display width is perfectly divisible by the slice count (i.e. mode_hdisplay % target_slices == 0). If the width isn't divisible, fractional slice widths will be programmed into the VDSC hardware, leading to unrecoverable pipe underruns and GPU hangs. > + *config_ret = config; > + return true; > + } > + } > + > + return false; > +} -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
