Re: [PATCH] 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] Bypassing `intel_dp_dsc_get_slice_config()` leaves `slice_config` zero-initialized, causing a divide-by-zero kernel panic in `intel_dsc_compute_params()`.
- [High] An existing early return prevents the new DSC passthrough capability detection from executing for passthrough-only PCONs.
- [High] Unconditional DPCD capability checks will fail and abort modesets or zero limits for passthrough PCONs with an all-zero `dsc_dpcd`.
- [High] Bypassing `intel_dp_dsc_get_slice_config()` ignores Intel VDSC hardware constraints by selecting slice counts based exclusively on the HDMI sink.
- [High] The `pcon_dsc_passthrough` capability flag is evaluated and cached before the EDID is parsed, using uninitialized or stale display data.
- [Medium] Assignment to a nonexistent struct member `slice_count` in `struct intel_crtc_state`'s `dsc` struct causes a compilation failure.
--

commit 542b41d19d507f8a0d3cdb73bcd6ff8c7a8d22d5
Author: Xiao Lu <xiaolu.xie@intel.com>
Subject: drm/i915/dp: enable DSC passthrough for HDMI 2.1 PCON devices

This patch adds explicit driver control of PCON DSC passthrough for HDMI 2.1
devices. It detects the passthrough capability from DPCD and the HDMI sink
EDID, skipping the PCON's own DSC decompression to forward the stream
unchanged.

> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 113d767e62e9c..ad55b08ecdea6 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c

[ ... ]

> @@ -2403,7 +2412,21 @@ 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]
Since passthrough-only PCONs have an all-zero dsc_dpcd, won't the 
unconditional call to intel_dp_dsc_supports_format() at the beginning of 
intel_dp_dsc_compute_config() parse the empty DPCD and return -EINVAL
immediately? 

Similarly, in intel_dp_compute_config_limits(), won't the unconditional call
to align_max_sink_dsc_input_bpp() parse the empty dsc_dpcd and clamp the
limits to 0?

> +		/*
> +		 * PCON passthrough: no PCON encoder DPCD, derive slice count
> +		 * from the downstream HDMI 2.1 sink DSC capabilities.
> +		 */
> +		u8 pt_slice_count =
> +			intel_dp_pcon_passthrough_dsc_slice_count(intel_dp,
> +								  pipe_config);
> +		if (!pt_slice_count) {
> +			drm_dbg_kms(display->drm,
> +				    "PCON DSC passthrough: no valid slice count\n");
> +			return -EINVAL;
> +		}
> +		pipe_config->dsc.slice_count = pt_slice_count;

[Severity: Medium]
Does pipe_config->dsc contain a member named slice_count? It looks like
the slice_count member actually resides in the nested struct
drm_dsc_config config, which would cause a build failure here.

[Severity: High]
By taking this branch and bypassing intel_dp_dsc_get_slice_config(),
pipe_config->dsc.slice_config is left zero-initialized. Could this cause a
divide-by-zero kernel panic later in intel_dsc_compute_params() when it
executes:

vdsc_cfg->slice_width = DIV_ROUND_UP(vdsc_cfg->pic_width,
    intel_dsc_line_slice_count(&pipe_config->dsc.slice_config));

because intel_dsc_line_slice_count() returns 0 for a zeroed struct?

> +	} 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;

[ ... ]

> @@ -4297,6 +4360,71 @@ 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)
> +{
> +	struct intel_connector *connector = intel_dp->attached_connector;
> +	const struct drm_display_info *info;
> +
> +	if (!connector)
> +		return;
> +
> +	connector->dp.pcon_dsc_passthrough = false;
> +
> +	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;
> +}

[Severity: High]
This capability cache relies on connector->base.display_info, but it is
called from intel_dp_detect_dsc_caps() which executes before 
intel_dp_set_edid() populates display_info. Does this cause the check to
read uninitialized memory or stale EDID data from a previously connected
monitor?

> +static u8
> +intel_dp_pcon_passthrough_dsc_slice_count(struct intel_dp *intel_dp,
> +					  const struct intel_crtc_state *crtc_state)
> +{
> +	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 hdmi_throughput = info->hdmi.dsc_cap.clk_per_slice;
> +	int hdmi_max_slices = info->hdmi.dsc_cap.max_slices;
> +	int num_slices;
> +
> +	/*
> +	 * Passthrough: no PCON encoder constraints, use only the HDMI sink
> +	 * DSC caps from EDID. Pass hdisplay for pcon_max_slice_width so that
> +	 * constraint is effectively unconstrained.
> +	 */
> +	num_slices = intel_hdmi_dsc_get_num_slices(crtc_state,
> +						   hdmi_max_slices,
> +						   crtc_state->hw.adjusted_mode.hdisplay,
> +						   hdmi_max_slices,
> +						   hdmi_throughput);

[Severity: High]
By passing hdmi_max_slices as the src_max_slices parameter, this discards
all source VDSC constraints. If the HDMI sink supports up to 16 slices but
the source VDSC only supports a maximum of 4 slices per pipe, could this
result in misconfiguring the source VDSC with a slice count it physically
cannot generate?

[ ... ]

> @@ -4565,6 +4694,8 @@ void intel_dp_get_dsc_sink_cap(u8 dpcd_rev,
>  	if (drm_dp_has_quirk(desc, DP_DPCD_QUIRK_DSC_THROUGHPUT_BPP_LIMIT) &&
>  	    desc->ident.hw_rev == 0x10)
>  		connector->dp.dsc_throughput_quirk = true;
> +
> +	intel_dp_pcon_set_dsc_passthrough_cap(intel_attached_dp(connector));
>  }

[Severity: High]
Earlier in intel_dp_get_dsc_sink_cap(), there is an early return:

    if (!(connector->dp.dsc_dpcd[0] & DP_DSC_DECOMPRESSION_IS_SUPPORTED))
        return;
        
Since passthrough-only PCONs lack a DSC decoder and do not set
DP_DSC_DECOMPRESSION_IS_SUPPORTED, doesn't this early return prevent
intel_dp_pcon_set_dsc_passthrough_cap() from ever executing for these
devices?

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