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

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

On 1/29/2026 2:54 PM, Imre Deak wrote:
> On Thu, Jan 29, 2026 at 11:18:28AM +0530, Nautiyal, Ankit K wrote:
>> On 1/29/2026 10:51 AM, Nautiyal, Ankit K wrote:
>>> On 1/28/2026 10:16 PM, Imre Deak wrote:
>>>> On Wed, Jan 28, 2026 at 07:36:24PM +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.
>>>>>
>>>>> 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.
>>>>>
>>>>> 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)
>>>>>
>>>>> Signed-off-by: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
>>>>> ---
>>>>>    drivers/gpu/drm/i915/display/intel_dp.c | 158
>>>>> +++++++++++++++---------
>>>>>    1 file changed, 103 insertions(+), 55 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
>>>>> b/drivers/gpu/drm/i915/display/intel_dp.c
>>>>> index 4c3a1b6d0015..599965a6e1a6 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,13 +1462,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 num_pipes;
>>>>>          status = intel_cpu_transcoder_mode_valid(display, mode);
>>>>>        if (status != MODE_OK)
>>>>> @@ -1488,67 +1505,98 @@ 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);
>>>>> +    /*
>>>>> +     * 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.
>>>>> +     */
>>>>> +    for (num_pipes = 0; num_pipes < I915_MAX_PIPES; num_pipes++) {
>>>>> +        int max_dotclk = display->cdclk.max_dotclk_freq;
>>>>> +
>>>>> +        status = MODE_CLOCK_HIGH;
>>>>> +
>>>>> +        if (num_pipes == 0) {
>>>>> +            if (!connector->force_joined_pipes)
>>>>> +                continue;
>>>>> +            num_joined_pipes = connector->force_joined_pipes;
>>>>> +        } else {
>>>>> +            num_joined_pipes = num_pipes;
>>>>> +        }
>>>> The current way is to try connector->force_joined_pipes and fail the
>>>> commit if that doesn't work. Here you'd change that to fall back trying
>>>> non-forced pipe-joined configs in that case. If that's needed (not sure
>>>> if that's a good idea, since then the user wouldn't know which case
>>>> succeeded or failed), it should be a separate change. Here it could be
>>>> simply an if (forced_joined_pipes && num_pipes != forced_joined_pipes)
>>>> continue and then use num_pipes instead of num_joined_pipes later in the
>>>> loop.
>>> Hmm Yeah the initial thought process was to try force joiner thing
>>> first, if succeed well and good, if not we do our usual thing.
>>>
>>> But I see your point. I think we can just fail the commit if forced
>>> joiner doesn't work.
>>>
>>> This will also help simplify the loop and will allow to have an iterator
>>> macro as you have mentioned in the later patch.
>> For mode_valid phase can there be a corner case where force joiner is set to
>> some high value with which all modes somehow fail?
>>
>> Can hdisplay/htotal or clock  for a mode go below some acceptable value if
>> joiner is used?
> I.e. hdisplay/htotal/clock per-pipe becoming too low, right? I don't
> recall that the driver would check for these anywhere. The mode timing
> itself is checked and then (for instance) MODE_CLOCK_LOW is returned,
> but that's not a per-pipe thing. One thing that can fail is the DSC
> slice count, which can get too high for the sink.
>
> In any case there's not much we can do about any such failure scenario,
> just fail the commit?


Hmm makes sense. I have already started working on this, will send the 
next revision with this change and other suggested changes soon.

Regards,

Ankit

>
>> Regards,
>>
>> Ankit
>>
>>>
>>>>> +
>>>>> +        if (!intel_dp_can_join(display, num_joined_pipes))
>>>>> +            continue;
>>>>> +
>>>>> +        if (mode->hdisplay > num_joined_pipes *
>>>>> intel_dp_max_hdisplay_per_pipe(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;
>>>> status stays MODE_OK if target_clock > max_dotclk.
>>> Oh yes.. will fix this.
>>>
>>>
>>>>> +            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);
>>>>> +
>>>> Extra w/s.
>>> Will drop this extra line.
>>>
>>>
>>> Thanks & Regards,
>>>
>>> Ankit
>>>
>>>>>    }
>>>>>      bool intel_dp_source_supports_tps3(struct intel_display *display)
>>>>> -- 
>>>>> 2.45.2
>>>>>