RE: [PATCH] drm/i915/dp: Add support for DP UHBR SST DSC

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

On Tue, 04 Feb 2025, "Saarinen, Jani" <jani.saarinen@intel.com> wrote:
> Hi,
>
>> -----Original Message-----
>> From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Jani
>> Nikula
>> Sent: Tuesday, 4 February 2025 18.40
>> To: Deak, Imre <imre.deak@intel.com>
>> Cc: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
>> Subject: Re: [PATCH] drm/i915/dp: Add support for DP UHBR SST DSC
>>
>> On Tue, 04 Feb 2025, Imre Deak <imre.deak@intel.com> wrote:
>> > On Mon, Feb 03, 2025 at 06:08:34PM +0200, Jani Nikula wrote:
>> >> Drop the UHBR limitation from DP SST DSC, and handle SST DSC
>> >> bandwidth computation for UHBR using
>> intel_dp_mtp_tu_compute_config().
>> >>
>> >> Cc: Imre Deak <imre.deak@intel.com>
>> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> >
>> > Reviewed-by: Imre Deak <imre.deak@intel.com>
>> >
>> > With the DPT bpp and bpp_step fixes on the list, this seems to work on
>> > a
>> > DP2.0 dock on (SST) UHBR link/DSC mode.
>>
>> \o/
>>
>> Thanks for the review and testing!
> Awesome, thanks Jani N and Imre!

And pushed to din, thanks for the review!

>
>> >
> Br,
> Jani
>> >> ---
>> >>  drivers/gpu/drm/i915/display/intel_dp.c | 35
>> >> +++++++++++++++++++------
>> >>  1 file changed, 27 insertions(+), 8 deletions(-)
>> >>
>> >> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c
>> >> b/drivers/gpu/drm/i915/display/intel_dp.c
>> >> index cc6aba353c11..eb8f6806166c 100644
>> >> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> >> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> >> @@ -1958,15 +1958,37 @@ static int dsc_compute_link_config(struct
>> intel_dp *intel_dp,
>> >>            for (lane_count = limits->min_lane_count;
>> >>                 lane_count <= limits->max_lane_count;
>> >>                 lane_count <<= 1) {
>> >> -                  if (!is_bw_sufficient_for_dsc_config(dsc_bpp_x16,
>> link_rate,
>> >> -                                                       lane_count,
>> adjusted_mode->clock,
>> >> -                                                       pipe_config-
>> >output_format,
>> >> -                                                       timeslots))
>> >> -                          continue;
>> >>
>> >> +                  /*
>> >> +                   * FIXME: intel_dp_mtp_tu_compute_config()
>> requires
>> >> +                   * ->lane_count and ->port_clock set before we know
>> >> +                   * they'll work. If we end up failing altogether,
>> >> +                   * they'll remain in crtc state. This shouldn't matter,
>> >> +                   * as we'd then bail out from compute config, but it's
>> >> +                   * just ugly.
>> >> +                   */
>> >>                    pipe_config->lane_count = lane_count;
>> >>                    pipe_config->port_clock = link_rate;
>> >>
>> >> +                  if (drm_dp_is_uhbr_rate(link_rate)) {
>> >> +                          int ret;
>> >> +
>> >> +                          ret =
>> intel_dp_mtp_tu_compute_config(intel_dp,
>> >> +
>> pipe_config,
>> >> +                                                               conn_state,
>> >> +
>> dsc_bpp_x16,
>> >> +
>> dsc_bpp_x16,
>> >> +                                                               0, true);
>> >> +                          if (ret)
>> >> +                                  continue;
>> >> +                  } else {
>> >> +                          if
>> (!is_bw_sufficient_for_dsc_config(dsc_bpp_x16, link_rate,
>> >> +                                                               lane_count,
>> adjusted_mode->clock,
>> >> +
>> pipe_config->output_format,
>> >> +                                                               timeslots))
>> >> +                                  continue;
>> >> +                  }
>> >> +
>> >>                    return 0;
>> >>            }
>> >>    }
>> >> @@ -2493,9 +2515,6 @@ intel_dp_compute_config_limits(struct intel_dp
>> *intel_dp,
>> >>    limits->min_rate = intel_dp_min_link_rate(intel_dp);
>> >>    limits->max_rate = intel_dp_max_link_rate(intel_dp);
>> >>
>> >> -  /* FIXME 128b/132b SST+DSC support missing */
>> >> -  if (!is_mst && dsc)
>> >> -          limits->max_rate = min(limits->max_rate, 810000);
>> >>    limits->min_rate = min(limits->min_rate, limits->max_rate);
>> >>
>> >>    limits->min_lane_count = intel_dp_min_lane_count(intel_dp);
>> >> --
>> >> 2.39.5
>> >>
>>
>> --
>> Jani Nikula, Intel

-- 
Jani Nikula, Intel