Re: [PATCH v7 2/2] drm/i915/dp: enable DSC passthrough for HDMI 2.1 PCON devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Incorrect connector context used for MST streams, causing root connector state corruption and broken MST passthrough.
- [High] Mode rejection due to unhandled all-zero PCON `dsc_dpcd` evaluating to 0 `dsc_slice_count`.
- [High] Stale early return guards prevent PCON passthrough from functioning over MST and risk hardware misconfiguration.
- [High] `intel_dp_dsc_compute_config` unconditionally fails because it calls `intel_dp_dsc_supports_format` with an all-zero DPCD.
- [Low] Implicit declaration of static function causing a build failure across translation units.
- [Low] Unused static helper function `intel_dp_pcon_dsc_enc_slice_height`.
--

commit e900995cfc29f9b4c87a5f8c6c73b97d623e0e33
Author: Xiao Lu <xiaolu.xie@intel.com>

drm/i915/dp: enable DSC passthrough for HDMI 2.1 PCON devices

This commit enables explicit driver control of PCON DSC passthrough for
HDMI 2.1 sinks that decompress the stream themselves. It adds paths to
detect capability, compute DSC parameters from the EDID instead of PCON
DPCD, and correctly configure passthrough via DPCD.

> 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
[ ... ]
> @@ -2393,7 +2424,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 (connector->dp.pcon_dsc_passthrough) {

[Severity: High]
Earlier in this function, intel_dp_dsc_supports_format() is called. Since
passthrough-only PCONs have an all-zero cached dsc_dpcd, does that check
unconditionally return false and abort the configuration with -EINVAL
before reaching this passthrough logic?

> +		/*
> +		 * 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;
[ ... ]
> @@ -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]
Since passthrough-only PCONs lack a DSC decoder, their dsc_dpcd array is
all zeros, which makes drm_dp_dsc_sink_max_slice_count() return 0. 

Does this cause the min_t() to evaluate to 0, resulting in valid modes
being unconditionally rejected? 

The v7 changelog mentions falling back to EDID max_slices when pcon_max is
zero, but the code appears to use a direct min_t comparison.

> +	} else {
> +		dsc_slice_count = intel_dp_dsc_get_slice_count(connector,
> +							       mode_clock,
> +							       mode_hdisplay,
> +							       num_joined_pipes);
> +	}
[ ... ]
> @@ -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]
Does this guard inadvertently prevent the short-circuit for PCON
passthrough over MST? 

The v7 changelog indicates the !connector->mst.dp check was removed to
support MST, but it appears to still be present here. If an MST connection
is used, DP_DECOMPRESSION_EN might be erroneously written to a passthrough
device that does not support decompression.

> +
>  	if (write_dsc_decompression_flag(connector->dp.dsc_decompression_aux,
>  					 DP_DECOMPRESSION_EN, enable) < 0)
>  		drm_dbg_kms(display->drm,
[ ... ]
> @@ -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
> + *
> + * Called once at connect time (from intel_dp_get_dsc_sink_cap()) after
> + * dsc_dpcd and the HDMI sink EDID have been read. Caches the result in
> + * connector->dp.pcon_dsc_passthrough so modeset paths can read it cheaply
> + * without repeating the DPCD and EDID lookups.
> + *
> + * A PCON supports DSC passthrough when it explicitly advertises
> + * DP_DSC_PASSTHROUGH_IS_SUPPORTED (DPCD 0x060 bit1) and the downstream
> + * HDMI 2.1 sink supports DSC 1.2. The compressed stream produced by the
> + * source VDSC engine is forwarded unchanged through the PCON to the HDMI
> + * sink for decompression (DP_DSC_ENABLE bit1 on the PCON).
> + */
> +static void
> +intel_dp_pcon_set_dsc_passthrough_cap(struct intel_dp *intel_dp)

[Severity: Low]
Does intel_dp_pcon_set_dsc_passthrough_cap() need to be non-static? 

The v7 changelog mentions it was made non-static to fix a build failure,
but it remains static here. Compiling this might fail with an implicit
declaration error when called from
drivers/gpu/drm/i915/display/intel_dp_mst.c.

> +{
> +	struct intel_connector *connector = intel_dp->attached_connector;

[Severity: High]
Could this cause state corruption for MST streams?

In an MST topology, intel_dp->attached_connector resolves to the root SST
connector, not the leaf MST connector. The v7 changelog notes that this
was fixed by accepting an explicit connector parameter, but it seems
intel_dp->attached_connector is still being used here.

> +	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 early return explicitly disable passthrough capability checking
for MST connectors?

The v7 changelog indicates the !connector->mst.port early-return was
removed to support MST topologies, but it is still present in this patch.

[ ... ]
> +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]
Similar to the issue above, does this read the root connector's display
info instead of the MST sink's EDID?

If this is executed for an MST stream, it will use the root connector's
HDMI capabilities (or lack thereof) instead of the actual downstream sink,
which might lead to invalid DSC slice configurations.

[ ... ]
> +static int
> +intel_dp_pcon_dsc_enc_slice_height(const struct intel_crtc_state *crtc_state)
> +{
> +	int vactive = crtc_state->hw.adjusted_mode.vdisplay;
> +
> +	return intel_hdmi_dsc_get_slice_height(vactive);
> +}

[Severity: Low]
Is intel_dp_pcon_dsc_enc_slice_height() intended to be used? 

It appears to be newly defined as a static helper but is not called
anywhere in this patch, which might increase binary size or cause compiler
warnings for unused functions.

[ ... ]
> 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: Low]
Because intel_dp_pcon_set_dsc_passthrough_cap() remains static in
intel_dp.c, will this cross-file call trigger a build failure?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909041041.244531-1-xiaolu.xie@intel.com?part=2