Re: [PATCH 04/14] drm/i915/dp: Rework pipe joiner logic in mode_valid

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 21 Jan 2026, Ankit Nautiyal <ankit.k.nautiyal@intel.com> wrote:
> Currently in intel_dp_mode_valid(), we compute the number of joined pipes
> required before deciding whether DSC is needed. This ordering prevents us
> from accounting for DSC-related overhead when determining pipe
> requirements.
>
> Refactor the logic to start with a single pipe and incrementally try
> additional pipes only if needed. While DSC overhead is not yet computed
> here, this restructuring prepares the code to support that in a follow-up
> changes.
>
> Additionally, if a forced joiner configuration is present, we first check
> whether it satisfies the bandwidth and timing constraints. If it does not,
> we fall back to evaluating configurations with 1, 2, or 4 pipes joined
> and prune or keep the mode accordingly.
>
> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 144 +++++++++++++++---------
>  drivers/gpu/drm/i915/display/intel_dp.h |   7 ++
>  2 files changed, 96 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index fc7d48460a52..02381f84fa58 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -107,6 +107,13 @@
>  /* Constants for DP DSC configurations */
>  static const u8 valid_dsc_bpp[] = {6, 8, 10, 12, 15};
>  
> +static const enum joiner_type joiner_candidates[] = {
> +	FORCED_JOINER,
> +	NO_JOINER,
> +	BIG_JOINER,
> +	ULTRA_JOINER,
> +};
> +
>  /**
>   * intel_dp_is_edp - is the given port attached to an eDP panel (either CPU or PCH)
>   * @intel_dp: DP struct
> @@ -1445,13 +1452,13 @@ intel_dp_mode_valid(struct drm_connector *_connector,
>  	const struct drm_display_mode *fixed_mode;
>  	int target_clock = mode->clock;
>  	int max_rate, mode_rate, max_lanes, max_link_clock;
> -	int max_dotclk = display->cdclk.max_dotclk_freq;
>  	u16 dsc_max_compressed_bpp = 0;
>  	u8 dsc_slice_count = 0;
>  	enum drm_mode_status status;
>  	bool dsc = false;
>  	int num_joined_pipes;
>  	int link_bpp_x16;
> +	int i;
>  
>  	status = intel_cpu_transcoder_mode_valid(display, mode);
>  	if (status != MODE_OK)
> @@ -1488,67 +1495,94 @@ intel_dp_mode_valid(struct drm_connector *_connector,
>  					   target_clock, mode->hdisplay,
>  					   link_bpp_x16, 0);
>  
> -	num_joined_pipes = intel_dp_num_joined_pipes(intel_dp, connector,
> -						     mode->hdisplay, target_clock);
> -	max_dotclk *= num_joined_pipes;
> -
> -	if (target_clock > max_dotclk)
> -		return MODE_CLOCK_HIGH;
> -
> -	status = intel_pfit_mode_valid(display, mode, output_format, num_joined_pipes);
> -	if (status != MODE_OK)
> -		return status;
> -
> -	if (intel_dp_has_dsc(connector)) {
> -		int pipe_bpp;
> -
> -		/*
> -		 * TBD pass the connector BPC,
> -		 * for now U8_MAX so that max BPC on that platform would be picked
> -		 */
> -		pipe_bpp = intel_dp_dsc_compute_max_bpp(connector, U8_MAX);
> -
> -		/*
> -		 * Output bpp is stored in 6.4 format so right shift by 4 to get the
> -		 * integer value since we support only integer values of bpp.
> -		 */
> -		if (intel_dp_is_edp(intel_dp)) {
> -			dsc_max_compressed_bpp =
> -				drm_edp_dsc_sink_output_bpp(connector->dp.dsc_dpcd) >> 4;
> -
> -			dsc_slice_count =
> -				intel_dp_dsc_get_slice_count(connector,
> -							     target_clock,
> -							     mode->hdisplay,
> -							     num_joined_pipes);
> -
> -			dsc = dsc_max_compressed_bpp && dsc_slice_count;
> -		} else if (drm_dp_sink_supports_fec(connector->dp.fec_capability)) {
> -			unsigned long bw_overhead_flags = 0;
> -
> -			if (!drm_dp_is_uhbr_rate(max_link_clock))
> -				bw_overhead_flags |= DRM_DP_BW_OVERHEAD_FEC;
> -
> -			dsc = intel_dp_mode_valid_with_dsc(connector,
> -							   max_link_clock, max_lanes,
> -							   target_clock, mode->hdisplay,
> -							   num_joined_pipes,
> -							   output_format, pipe_bpp,
> -							   bw_overhead_flags);
> +	for (i = 0; i < ARRAY_SIZE(joiner_candidates); i++) {
> +		int max_dotclk = display->cdclk.max_dotclk_freq;
> +		enum joiner_type joiner = joiner_candidates[i];
> +
> +		status = MODE_CLOCK_HIGH;
> +
> +		if (joiner == FORCED_JOINER) {
> +			if (!connector->force_joined_pipes)
> +				continue;
> +			num_joined_pipes = connector->force_joined_pipes;
> +		} else {
> +			num_joined_pipes = 1 << joiner;
> +		}
> +
> +		if ((joiner >= NO_JOINER && !intel_dp_has_joiner(intel_dp)) ||
> +		    (joiner == BIG_JOINER && !HAS_BIGJOINER(display)) ||
> +		    (joiner == ULTRA_JOINER && !HAS_ULTRAJOINER(display)))

There's a bunch of superfluous braces in there.

Anyway, this makes me think if we should reconsider the HAS_BIGJOINER()
and HAS_ULTRAJOINER() naming, and the enum thing here.

We're adding a bunch of logic to enumerate combos, and to check those
against joiner availability. But really, we could have a HAS_JOINER(num)
which says whether we can join that many pipes. Maybe even have the
compression as parameter.

I know the spec talks about big/ultra joiner, but for the more casual
reader of the code, you really want to know how many pipes you're
talking about joining, not the *name* of the thing.

If we had that in place, we could turn the whole joiner loop here from
enumerating the enums to enumerating the number of joined pipes. and
whether we can join them. No need for the enum at all, and I think the
code might end up cleaner too.


> +			break;
> +
> +		if (mode->hdisplay > num_joined_pipes * intel_dp_hdisplay_limit(display))
> +			continue;
> +
> +		status = intel_pfit_mode_valid(display, mode, output_format, num_joined_pipes);
> +		if (status != MODE_OK)
> +			continue;
> +
> +		if (intel_dp_has_dsc(connector)) {
> +			int pipe_bpp;
> +
> +			/*
> +			 * TBD pass the connector BPC,
> +			 * for now U8_MAX so that max BPC on that platform would be picked
> +			 */
> +			pipe_bpp = intel_dp_dsc_compute_max_bpp(connector, U8_MAX);
> +
> +			/*
> +			 * Output bpp is stored in 6.4 format so right shift by 4 to get the
> +			 * integer value since we support only integer values of bpp.
> +			 */
> +			if (intel_dp_is_edp(intel_dp)) {
> +				dsc_max_compressed_bpp =
> +					drm_edp_dsc_sink_output_bpp(connector->dp.dsc_dpcd) >> 4;
> +
> +				dsc_slice_count =
> +					intel_dp_dsc_get_slice_count(connector,
> +								     target_clock,
> +								     mode->hdisplay,
> +								     num_joined_pipes);
> +
> +				dsc = dsc_max_compressed_bpp && dsc_slice_count;
> +			} else if (drm_dp_sink_supports_fec(connector->dp.fec_capability)) {
> +				unsigned long bw_overhead_flags = 0;
> +
> +				if (!drm_dp_is_uhbr_rate(max_link_clock))
> +					bw_overhead_flags |= DRM_DP_BW_OVERHEAD_FEC;
> +
> +				dsc = intel_dp_mode_valid_with_dsc(connector,
> +								   max_link_clock, max_lanes,
> +								   target_clock, mode->hdisplay,
> +								   num_joined_pipes,
> +								   output_format, pipe_bpp,
> +								   bw_overhead_flags);
> +			}
> +		}
> +
> +		if (intel_dp_joiner_needs_dsc(display, num_joined_pipes) && !dsc)
> +			continue;
> +
> +		if (mode_rate > max_rate && !dsc)
> +			continue;
> +
> +		status = intel_mode_valid_max_plane_size(display, mode, num_joined_pipes);
> +		if (status != MODE_OK)
> +			continue;
> +
> +		max_dotclk *= num_joined_pipes;
> +
> +		if (target_clock <= max_dotclk) {
> +			status = MODE_OK;
> +			break;
>  		}
>  	}
>  
> -	if (intel_dp_joiner_needs_dsc(display, num_joined_pipes) && !dsc)
> -		return MODE_CLOCK_HIGH;
> -
> -	status = intel_mode_valid_max_plane_size(display, mode, num_joined_pipes);
>  	if (status != MODE_OK)
>  		return status;
>  
> -	if (mode_rate > max_rate && !dsc)
> -		return MODE_CLOCK_HIGH;
> -
>  	return intel_dp_mode_valid_downstream(connector, mode, target_clock);
> +
>  }
>  
>  bool intel_dp_source_supports_tps3(struct intel_display *display)
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.h b/drivers/gpu/drm/i915/display/intel_dp.h
> index 25bfbfd291b0..a27e3b5829bd 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.h
> +++ b/drivers/gpu/drm/i915/display/intel_dp.h
> @@ -24,6 +24,13 @@ struct intel_display;
>  struct intel_dp;
>  struct intel_encoder;
>  
> +enum joiner_type {

I wrote the below *before* I wrote the comment above, which makes all of
this moot, but here goes anyway:

"joiner_type" is too generic, it needs a prefix.

> +	FORCED_JOINER = -1,
> +	NO_JOINER = 0,		/* 1 pipe */
> +	BIG_JOINER = 1,		/* 2 pipes */
> +	ULTRA_JOINER = 2,	/* 4 pipes */

Ditto for the enumerations. Why are you explicitly initializing the
values, are they somehow meaningful?

> +};

The enum is about the joiner, not about DP. intel_dp.h doesn't have
anything that requires the joiner type.


> +
>  struct link_config_limits {
>  	int min_rate, max_rate;
>  	int min_lane_count, max_lane_count;

-- 
Jani Nikula, Intel