Re: [Intel-gfx] [PATCH] drm/i915/slpc: Let's fix the PCODE min freq table setup for SLPC

["Vivi, Rodrigo" <rodrigo.vivi@intel.com>]

On Tue, 2022-08-30 at 17:45 -0700, Dixit, Ashutosh wrote:
> On Tue, 30 Aug 2022 12:16:20 -0700, Rodrigo Vivi wrote:
> > 
> 
> Hi Rodrigo,
> 
> > @@ -65,13 +63,8 @@ static bool get_ia_constants(struct intel_llc
> > *llc,
> >         /* convert DDR frequency from units of 266.6MHz to
> > bandwidth */
> >         consts->min_ring_freq = mult_frac(consts->min_ring_freq, 8,
> > 3);
> > 
> > -       consts->min_gpu_freq = rps->min_freq;
> > -       consts->max_gpu_freq = rps->max_freq;
> > -       if (GRAPHICS_VER(i915) >= 9) {
> > -               /* Convert GT frequency to 50 HZ units */
> > -               consts->min_gpu_freq /= GEN9_FREQ_SCALER;
> > -               consts->max_gpu_freq /= GEN9_FREQ_SCALER;
> > -       }
> > +       consts->min_gpu_freq = intel_rps_get_min_raw_freq(rps);
> > +       consts->max_gpu_freq = intel_rps_get_max_raw_freq(rps);
> > 
> >         return true;
> >  }
> > @@ -130,6 +123,12 @@ static void gen6_update_ring_freq(struct
> > intel_llc *llc)
> >         if (!get_ia_constants(llc, &consts))
> >                 return;
> > 
> > +       /*
> > +        * Although this is unlikely on any platform during
> > initialization,
> > +        * let's ensure we don't get accidentally into infinite
> > loop
> > +        */
> > +       if (consts.max_gpu_freq <= consts.min_gpu_freq)
> > +               return;
> 
> As I said this is not correct and is not needed. If
> 'consts.max_gpu_freq ==
> consts.min_gpu_freq' we would *want* to program PCODE. If
> 'consts.max_gpu_freq < consts.min_gpu_freq' the loop will
> automatically
> skip (and also it is not an infinite loop).

yeap, but if we change this condition in the loop we will
miss one entry in the case they are equal.
Since we are doing this generically for 15 years of hardware
I didn't want to take the risk of having some out there
where the min = max and the 1 entry in the table is needed.

> 
> > diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c
> > b/drivers/gpu/drm/i915/gt/intel_rps.c
> > index de794f5f8594..26af974292c7 100644
> > --- a/drivers/gpu/drm/i915/gt/intel_rps.c
> > +++ b/drivers/gpu/drm/i915/gt/intel_rps.c
> > @@ -2156,6 +2156,24 @@ u32 intel_rps_get_max_frequency(struct
> > intel_rps *rps)
> >                 return intel_gpu_freq(rps, rps-
> > >max_freq_softlimit);
> >  }
> > 
> > +u32 intel_rps_get_max_raw_freq(struct intel_rps *rps)
> 
> What does "raw" mean? Or are we introducing a new concept here then
> we need
> to explain the new concept? I was previously told there is a concept
> of "hw
> units" of freq and intel_gpu_freq will convert from "hw units" to
> MHz.

yeap, it is the hw units, some folks also calling FID of the freqs.
I couldn't find a better name.

> 
> Also, Is the return value in units of 50 MHz in all cases (we know it
> is
> for SLPC and Gen 9+)? In that case we should name such a function to
> something like intel_rps_get_max_freq_in_50mhz_units?

yeap, that would work... at least until in some future platform our hw
folks need to find another base...

> 
> > +{
> > +       struct intel_guc_slpc *slpc = rps_to_slpc(rps);
> > +       u32 freq;
> > +
> > +       if (rps_uses_slpc(rps)) {
> > +               return DIV_ROUND_CLOSEST(slpc->rp0_freq,
> > +                                        GT_FREQUENCY_MULTIPLIER);
> > +       } else {
> > +               freq = rps->max_freq;
> > +               if (GRAPHICS_VER(rps_to_i915(rps)) >= 9) {
> > +                       /* Convert GT frequency to 50 HZ units */
> 
> 50 MHz and not 50 Hz. Also the comment should be moved to above
> rps_uses_slpc() line if returned freq is always in units of 50 MHz.

yeap, this comment was already there and probably wrong...

> 
> > +                       freq /= GEN9_FREQ_SCALER;
> > +               }
> > +               return freq;
> > +       }
> > +}
> 
> Also is this function equivalent to this:
> 
> u32 intel_rps_get_max_freq_in_50mhz_units(struct intel_rps *rps)
> {
>         struct intel_guc_slpc *slpc = rps_to_slpc(rps);
>         u32 freq;
> 
>         /* freq in MHz */
>         freq = rps_uses_slpc(rps) ? slpc->rp0_freq :
> intel_gpu_freq(rps->max_freq);

do you really want to convert forth and back? Can we minimize the math?

> 
>         return DIV_ROUND_CLOSEST(freq, GT_FREQUENCY_MULTIPLIER);
> }
> 
> Sorry I don't have a lot of history in how these frequencies are
> scaled
> specially for old platforms like CHV/VLV/Gen6+. But afaiu
> intel_gpu_freq()
> will convert the freq to MHz for all platforms.

yeap, old platforms also concern me... another reason to avoid doing
something new and only using the conversion that was already there.

> 
> And then does get_ia_constants() accept freq in 50 MHz units for all
> platforms?

Please notice that there's absolutely no change for the non-slpc
platforms.

> 
> If we are not sure about this, we can go with your version which is
> closer
> to the original version in get_ia_constants() and so "safer" I guess.

so you mean this version? ;)

> 
> Thanks.
> --
> Ashutosh

Thank you so much!