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

["Nautiyal, Ankit K" <ankit.k.nautiyal@intel.com>]

On 2/2/2026 2:10 PM, Imre Deak wrote:
> On Fri, Jan 30, 2026 at 01:47:59PM +0530, Ankit Nautiyal 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.
>>
>> It is not possible to first decide whether DSC is needed and then compute
>> the required number of joined pipes, because the two depend on each other:
>>
>>   - DSC need is a function of the pipe count (e.g., 4‑pipe always requires
>>     DSC; 2‑pipe may require it if uncompressed joiner is unavailable).
>>
>>   - Whether a given pipe‑join configuration is sufficient depends on
>>     effective bandwidth, which itself changes when DSC is used.
>>
>> As a result, the only correct approach is to iterate candidate pipe counts.
>>
>> So, 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.
>>
>> If a forced joiner configuration is present, we just check for that
>> configuration. If it fails, we bailout and return instead of trying with
>> other joiner configurations.
>>
>> v2:
>>   - Iterate over number of pipes to be joined instead of joiner
>>     candidates. (Jani)
>>   - Document the rationale of iterating over number of joined pipes.
>>     (Imre)
>> v3:
>>   - In case the force joiner configuration doesn't work, do not fallback
>>     to the normal routine, bailout instead of trying other joiner
>>     configurations. (Imre)
>> v4:
>>   - Use num_joined_pipes instead of num_pipes. (Imre)
>>   - Inititialize status before the loops starts. (Imre)
>>
>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>> Reviewed-by: Imre Deak <imre.deak@intel.com>
> There is still one issue, see below.
>
>> ---
>>   drivers/gpu/drm/i915/display/intel_dp.c | 135 ++++++++++++++++--------
>>   1 file changed, 89 insertions(+), 46 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>> index 4c3a1b6d0015..dbe63efc1694 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -1434,6 +1434,23 @@ bool intel_dp_has_dsc(const struct intel_connector *connector)
>>   	return true;
>>   }
>>   
>> +static
>> +bool intel_dp_can_join(struct intel_display *display,
>> +		       int num_joined_pipes)
>> +{
>> +	switch (num_joined_pipes) {
>> +	case 1:
>> +		return true;
>> +	case 2:
>> +		return HAS_BIGJOINER(display) ||
>> +		       HAS_UNCOMPRESSED_JOINER(display);
>> +	case 4:
>> +		return HAS_ULTRAJOINER(display);
>> +	default:
>> +		return false;
>> +	}
>> +}
>> +
>>   static enum drm_mode_status
>>   intel_dp_mode_valid(struct drm_connector *_connector,
>>   		    const struct drm_display_mode *mode)
>> @@ -1445,7 +1462,6 @@ 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;
>> @@ -1488,66 +1504,93 @@ 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;
>> +	/*
>> +	 * We cannot determine the required pipe‑join count before knowing whether
>> +	 * DSC is needed, nor can we determine DSC need without knowing the pipe
>> +	 * count.
>> +	 * Because of this dependency cycle, the only correct approach is to iterate
>> +	 * over candidate pipe counts and evaluate each combination.
>> +	 */
>> +	status = MODE_CLOCK_HIGH;
>> +	for (num_joined_pipes = 1; num_joined_pipes <= I915_MAX_PIPES; num_joined_pipes++) {
>> +		int max_dotclk = display->cdclk.max_dotclk_freq;
>>   
>> -	if (target_clock > max_dotclk)
>> -		return MODE_CLOCK_HIGH;
>> +		if (connector->force_joined_pipes &&
>> +		    num_joined_pipes != connector->force_joined_pipes)
>> +			continue;
>>   
>> -	status = intel_pfit_mode_valid(display, mode, output_format, num_joined_pipes);
>> -	if (status != MODE_OK)
>> -		return status;
>> +		if (!intel_dp_can_join(display, num_joined_pipes))
>> +			continue;
>>   
>> -	if (intel_dp_has_dsc(connector)) {
>> -		int pipe_bpp;
>> +		if (mode->hdisplay > num_joined_pipes * intel_dp_max_hdisplay_per_pipe(display))
>> +			continue;
>>   
>> -		/*
>> -		 * 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);
>> +		status = intel_pfit_mode_valid(display, mode, output_format, num_joined_pipes);
>> +		if (status != MODE_OK)
>> +			continue;
> I missed it in my review of this particular patch, even though
> I did mention the similar issue elsewhere:
>
> status is guaranteed to be MODE_OK at this point and then ...


Oh yes this was not a problem earlier as I was setting status = 
MODE_CLOCK_HIGH inside the loop.

Thanks for catching this, will fix this in this patch and the patch#8 
and re-send.


Regards,

Ankit

>
>>   
>> -		/*
>> -		 * 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;
>> +		if (intel_dp_has_dsc(connector)) {
>> +			int pipe_bpp;
>>   
>> -			dsc_slice_count =
>> -				intel_dp_dsc_get_slice_count(connector,
>> -							     target_clock,
>> -							     mode->hdisplay,
>> -							     num_joined_pipes);
>> +			/*
>> +			 * 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);
>>   
>> -			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;
>> +			/*
>> +			 * 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;
>>   
>> -			if (!drm_dp_is_uhbr_rate(max_link_clock))
>> -				bw_overhead_flags |= DRM_DP_BW_OVERHEAD_FEC;
>> +				dsc_slice_count =
>> +					intel_dp_dsc_get_slice_count(connector,
>> +								     target_clock,
>> +								     mode->hdisplay,
>> +								     num_joined_pipes);
>>   
>> -			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);
>> +				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;
> ... this will continue with status == MODE_OK and the loop can terminate
> like that. So need a status = MODE_CLOCK_HIGH before continue.
>
>> +
>> +		if (mode_rate > max_rate && !dsc)
> This needs a status = MODE_CLOCK_HIGH as well.
>
> With the above fixed:
> Reviewed-by: Imre Deak <imre.deak@intel.com>
>
>> +			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_CLOCK_HIGH;
>> +			continue;
>> +		}
>> +
>> +		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);
>>   }
>>   
>> -- 
>> 2.45.2
>>