Re: [Intel-gfx] [PATCH v12 6/7] drm/i915/mtl: find the best QGV point for the SAGV configuration

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

On Mon, 2023-06-05 at 12:49 +0300, Imre Deak wrote:
> On Thu, Jun 01, 2023 at 07:03:49PM +0300, Vinod Govindapillai wrote:
> > From MTL onwards, we need to find the best QGV point based on
> > the required data rate and pass the peak BW of that point to
> > the punit to lock the corresponding QGV point.
> > 
> > v1: Fix for warning from kernel test robot
> > 
> > Bspec: 64636
> > 
> > Reported-by: kernel test robot <lkp@intel.com>
> > Closes: https://lore.kernel.org/r/202305280253.Ab8bRV2w-lkp@intel.com/
> > Reported-by: Dan Carpenter <error27@gmail.com>
> > Closes: https://lore.kernel.org/r/202305280253.Ab8bRV2w-lkp@intel.com/
> > Signed-off-by: Vinod Govindapillai <vinod.govindapillai@intel.com>
> > Reviewed-by: Stanislav Lisovskiy <stanislav.lisovskiy@intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_bw.c | 87 ++++++++++++++++++++++++-
> >  drivers/gpu/drm/i915/display/intel_bw.h |  6 ++
> >  2 files changed, 91 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_bw.c b/drivers/gpu/drm/i915/display/intel_bw.c
> > index b1cbeda0b2e3..7672963dc49c 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bw.c
> > +++ b/drivers/gpu/drm/i915/display/intel_bw.c
> > @@ -803,6 +803,85 @@ intel_atomic_get_bw_state(struct intel_atomic_state *state)
> >         return to_intel_bw_state(bw_state);
> >  }
> >  
> > +static int mtl_find_qgv_points(struct drm_i915_private *i915,
> > +                              unsigned int data_rate,
> > +                              unsigned int num_active_planes,
> > +                              const struct intel_bw_state *old_bw_state,
> > +                              struct intel_bw_state *new_bw_state)
> > +{
> > +       unsigned int best_rate = UINT_MAX;
> > +       unsigned int num_qgv_points = i915->display.bw.max[0].num_qgv_points;
> > +       unsigned int qgv_peak_bw  = 0;
> > +       int i;
> > +       int ret;
> > +
> > +       ret = intel_atomic_lock_global_state(&new_bw_state->base);
> > +       if (ret)
> > +               return ret;
> > +
> > +       /*
> > +        * If SAGV cannot be enabled, disable the pcode SAGV by passing all 1's
> > +        * for qgv peak bw in PM Demand request. So assign UINT_MAX if SAGV is
> > +        * not enabled. PM Demand code will clamp the value for the register
> > +        */
> > +       if (!intel_can_enable_sagv(i915, new_bw_state)) {
> > +               new_bw_state->qgv_point_peakbw = UINT_MAX;
> > +               drm_dbg_kms(&i915->drm, "No SAGV, use UINT_MAX as peak bw.");
> > +               goto out;
> > +       }
> > +
> > +       /*
> > +        * Find the best QGV point by comparing the data_rate with max data rate
> > +        * offered per plane group
> > +        */
> > +       for (i = 0; i < num_qgv_points; i++) {
> > +               unsigned int bw_index =
> > +                       tgl_max_bw_index(i915, num_active_planes, i);
> > +               unsigned int max_data_rate;
> > +
> > +               if (bw_index >= ARRAY_SIZE(i915->display.bw.max))
> > +                       continue;
> > +
> > +               max_data_rate = i915->display.bw.max[bw_index].deratedbw[i];
> > +
> > +               if (max_data_rate < data_rate)
> > +                       continue;
> > +
> > +               if (max_data_rate - data_rate < best_rate) {
> > +                       best_rate = max_data_rate - data_rate;
> > +                       qgv_peak_bw = i915->display.bw.max[bw_index].peakbw[i];
> > +               }
> > +
> > +               drm_dbg_kms(&i915->drm, "QGV point %d: max bw %d required %d qgv_peak_bw: %d\n",
> > +                           i, max_data_rate, data_rate, qgv_peak_bw);
> > +       }
> > +
> > +       drm_dbg_kms(&i915->drm, "Matching peaks QGV bw: %d for required data rate: %d\n",
> > +                   qgv_peak_bw, data_rate);
> > +
> > +       /*
> > +        * The display configuration cannot be supported if no QGV point
> > +        * satisfying the required data rate is found
> > +        */
> > +       if (qgv_peak_bw == 0) {
> > +               drm_dbg_kms(&i915->drm, "No QGV points for bw %d for display configuration(%d
> > active planes).\n",
> > +                           data_rate, num_active_planes);
> > +               return -EINVAL;
> > +       }
> > +
> > +       /* MTL PM DEMAND expects QGV BW parameter in multiples of 100 mbps */
> > +       new_bw_state->qgv_point_peakbw = DIV_ROUND_CLOSEST(qgv_peak_bw, 100);
> > +
> > +out:
> > +       if (new_bw_state->qgv_point_peakbw != old_bw_state->qgv_point_peakbw)  {
> > +               ret = intel_atomic_serialize_global_state(&new_bw_state->base);
> 
> qgv_point_beakbw is used as a parameter for the pmdemand command, for
> which there is a check later whether programming it is required or not.
> So why is global state serialized here?

Yes. good point. This need not to be serialized here. Will remove it.

Thanks
Vinod

> 
> > +               if (ret)
> > +                       return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +
> >  static int icl_find_qgv_points(struct drm_i915_private *i915,
> >                                unsigned int data_rate,
> >                                unsigned int num_active_planes,
> > @@ -928,8 +1007,12 @@ static int intel_bw_check_qgv_points(struct drm_i915_private *i915,
> >  
> >         data_rate = DIV_ROUND_UP(data_rate, 1000);
> >  
> > -       return icl_find_qgv_points(i915, data_rate, num_active_planes,
> > -                                  old_bw_state, new_bw_state);
> > +       if (DISPLAY_VER(i915) >= 14)
> > +               return mtl_find_qgv_points(i915, data_rate, num_active_planes,
> > +                                          old_bw_state, new_bw_state);
> > +       else
> > +               return icl_find_qgv_points(i915, data_rate, num_active_planes,
> > +                                          old_bw_state, new_bw_state);
> >  }
> >  
> >  static bool intel_bw_state_changed(struct drm_i915_private *i915,
> > diff --git a/drivers/gpu/drm/i915/display/intel_bw.h b/drivers/gpu/drm/i915/display/intel_bw.h
> > index f20292143745..67ae66a3fcdd 100644
> > --- a/drivers/gpu/drm/i915/display/intel_bw.h
> > +++ b/drivers/gpu/drm/i915/display/intel_bw.h
> > @@ -34,6 +34,12 @@ struct intel_bw_state {
> >         /* bitmask of active pipes */
> >         u8 active_pipes;
> >  
> > +       /*
> > +        * From MTL onwards, to lock a QGV point, punit expects the peak BW of
> > +        * the selected QGV point as the parameter in multiples of 100MB/s
> > +        */
> > +       unsigned int qgv_point_peakbw;
> > +
> >         /*
> >          * Current QGV points mask, which restricts
> >          * some particular SAGV states, not to confuse
> > -- 
> > 2.34.1
> >