Re: [Intel-gfx] [PATCH] drm/i915: Implement UHBR bandwidth check

["Govindapillai, Vinod" <vinod.govindapillai@intel.com>]

Hi Stan,

On Fri, 2023-03-24 at 15:51 +0200, Stanislav Lisovskiy wrote:
> According to spec, we should check if output_bpp * pixel_rate is less
> than DDI clock * 72, if UHBR is used.
> 
> v2: - s/pipe_config/crtc_state/ (Jani Nikula)
>     - Merged previous patch into that one, to remove empty function(Jani Nikula)
> 
> v3: - Make that constraint check to be DSC-related only
>     - Limit this to only DISPLAY_VER <= 13
> 
> HSDES: 1406899791
> BSPEC: 49259
> 
> Signed-off-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp_mst.c | 29 +++++++++++++++++++--
>  1 file changed, 27 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> index a860cbc5dbea..4c0edb760b8e 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c
> @@ -45,6 +45,27 @@
>  #include "intel_hotplug.h"
>  #include "skl_scaler.h"
>  
> +static int intel_dp_mst_check_constraints(struct drm_i915_private *i915, int bpp,
> +                                         const struct drm_display_mode *adjusted_mode,
> +                                         struct intel_crtc_state *crtc_state,
> +                                         bool dsc)
> +{
> +       if (intel_dp_is_uhbr(crtc_state) && DISPLAY_VER(i915) <= 13 && dsc) {
> +               int output_bpp = bpp;
> +               /* DisplayPort 2 128b/132b, bits per lane is always 32 */
> +               int symbol_clock = crtc_state->port_clock / 32;
> +
> +               if (output_bpp * adjusted_mode->crtc_clock >=
> +                   symbol_clock * 72) {
> +                       drm_dbg_kms(&i915->drm, "UHBR check failed(required bw %d available
> %d)\n",
> +                                   output_bpp * adjusted_mode->crtc_clock, symbol_clock * 72);
> +                       return -EINVAL;
> +               }
> +       }
> +
> +       return 0;
> +}
> +
>  static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder,
>                                                 struct intel_crtc_state *crtc_state,
>                                                 int max_bpp,
> @@ -87,6 +108,10 @@ static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder,
>  
>                 drm_dbg_kms(&i915->drm, "Trying bpp %d\n", bpp);
>  
> +               ret = intel_dp_mst_check_constraints(i915, bpp, adjusted_mode, crtc_state, dsc);
> +               if (ret)
> +                       continue;
> +

One suggestion, if you move this constraints check to the top of the "for loop", you may be able to
save an unnecessary "drm_dp_calc_pbn_mode" call in case constraints doesn't match for a bpp.

Reviewed-by: Vinod Govindapillai <vinod.govindapillai@intel.com>

>                 slots = drm_dp_atomic_find_time_slots(state, &intel_dp->mst_mgr,
>                                                       connector->port,
>                                                       crtc_state->pbn);
> @@ -104,8 +129,8 @@ static int intel_dp_mst_find_vcpi_slots_for_bpp(struct intel_encoder *encoder,
>                 }
>         }
>  
> -       /* Despite slots are non-zero, we still failed the atomic check */
> -       if (ret && slots >= 0)
> +       /* We failed to find a proper bpp/timeslots, return error */
> +       if (ret)
>                 slots = ret;
>  
>         if (slots < 0) {