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:57 PM, Imre Deak wrote:
> On Mon, Feb 02, 2026 at 02:54:25PM +0530, Nautiyal, Ankit K wrote:
>> 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.
> It was a problem even then, if this continue happened in the last
> iteration.

Ah right (face palm).


>
>> 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
>>>>