On Fri, 26 Aug 2022 03:13:18 -0700, Rodrigo Vivi wrote:

We need to inform PCODE of a desired ring frequencies so PCODE update the memory frequencies to us. rps->min_freq and rps->max_freq are the frequencies used in that request. However they were unset when SLPC was enabled and PCODE never updated the memory freq.

v2 (as Suggested by Ashutosh): if SLPC is in use, let's pick the right frequencies from the get_ia_constants instead of the fake init of rps' min and max.

Reviewed-by: Ashutosh Dixit ashutosh.dixit@intel.com

Fixes: 7ba79a671568 ("drm/i915/guc/slpc: Gate Host RPS when SLPC is enabled") Cc: stable@vger.kernel.org # v5.15+ Cc: Ashutosh Dixit ashutosh.dixit@intel.com Tested-by: Sushma Venkatesh Reddy sushma.venkatesh.reddy@intel.com Signed-off-by: Rodrigo Vivi rodrigo.vivi@intel.com

---

drivers/gpu/drm/i915/gt/intel_llc.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_llc.c b/drivers/gpu/drm/i915/gt/intel_llc.c index 14fe65812e42..766f9526da99 100644 --- a/drivers/gpu/drm/i915/gt/intel_llc.c +++ b/drivers/gpu/drm/i915/gt/intel_llc.c @@ -49,6 +49,7 @@ static unsigned int cpu_max_MHz(void) static bool get_ia_constants(struct intel_llc *llc, struct ia_constants *consts) {

• struct intel_guc_slpc *slpc = &llc_to_gt(llc)->uc.guc.slpc; struct drm_i915_private *i915 = llc_to_gt(llc)->i915; struct intel_rps *rps = &llc_to_gt(llc)->rps;

@@ -65,8 +66,13 @@ 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 (intel_uc_uses_guc_slpc(&llc_to_gt(llc)->uc)) {

• consts->min_gpu_freq = slpc->min_freq;
  

• consts->max_gpu_freq = slpc->rp0_freq;
  

• } else {

• 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;

-- 2.37.1

On Fri, 26 Aug 2022 10:44:34 -0700, Rodrigo Vivi wrote:

Fixes: 7ba79a671568 ("drm/i915/guc/slpc: Gate Host RPS when SLPC is enabled") Cc: stable@vger.kernel.org # v5.15+ Cc: Ashutosh Dixit ashutosh.dixit@intel.com Tested-by: Sushma Venkatesh Reddy sushma.venkatesh.reddy@intel.com Signed-off-by: Rodrigo Vivi rodrigo.vivi@intel.com

---

drivers/gpu/drm/i915/gt/intel_llc.c | 24 ++++++++++++++++-------- 1 file changed, 16 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_llc.c b/drivers/gpu/drm/i915/gt/intel_llc.c index 14fe65812e42..2677d62573d9 100644 --- a/drivers/gpu/drm/i915/gt/intel_llc.c +++ b/drivers/gpu/drm/i915/gt/intel_llc.c @@ -49,13 +49,28 @@ static unsigned int cpu_max_MHz(void) static bool get_ia_constants(struct intel_llc *llc, struct ia_constants *consts) {

• struct intel_guc_slpc *slpc = &llc_to_gt(llc)->uc.guc.slpc; struct drm_i915_private *i915 = llc_to_gt(llc)->i915; struct intel_rps *rps = &llc_to_gt(llc)->rps;

  if (!HAS_LLC(i915) || IS_DGFX(i915)) return false;

• if (rps->max_freq <= rps->min_freq)

• if (intel_uc_uses_guc_slpc(&llc_to_gt(llc)->uc)) {

• consts->min_gpu_freq = slpc->min_freq;
  

• consts->max_gpu_freq = slpc->rp0_freq;
  

• } else {

• 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;
  

• }
• if (consts->max_gpu_freq <= consts->min_gpu_freq) return false;

Hi Rodrigo, sorry, I missed this check previously too and the code is now equivalent to the previous code.

But now, looking at the code in gen6_update_ring_freq, I am wondering if we should return true in this case (i.e. remove the check) and we had a bug in the previous code? Because if we return false, gen6_update_ring_freq will skip the PCODE programming if 'max_gpu_freq == min_gpu_freq', but why should we skip the PCODE programming if 'max_gpu_freq == min_gpu_freq'? The case of 'max_gpu_freq < min_gpu_freq' is fine since the loop in gen6_update_ring_freq is not entered in that case.

Thanks. -- Ashutosh

We need to inform PCODE of a desired ring frequencies so PCODE update the memory frequencies to us. rps->min_freq and rps->max_freq are the frequencies used in that request. However they were unset when SLPC was enabled and PCODE never updated the memory freq.

v2 (as Suggested by Ashutosh): if SLPC is in use, let's pick the right frequencies from the get_ia_constants instead of the fake init of rps' min and max.

v3: don't forget the max <= min return

v4: Move all the freq conversion to intel_rps.c. And the max <= min check to where it belongs.

Fixes: 7ba79a671568 ("drm/i915/guc/slpc: Gate Host RPS when SLPC is enabled") Cc: stable@vger.kernel.org # v5.15+ Cc: Ashutosh Dixit ashutosh.dixit@intel.com Tested-by: Sushma Venkatesh Reddy sushma.venkatesh.reddy@intel.com Signed-off-by: Rodrigo Vivi rodrigo.vivi@intel.com --- drivers/gpu/drm/i915/gt/intel_llc.c | 19 ++++++++------- drivers/gpu/drm/i915/gt/intel_rps.c | 36 +++++++++++++++++++++++++++++ drivers/gpu/drm/i915/gt/intel_rps.h | 2 ++ 3 files changed, 47 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_llc.c b/drivers/gpu/drm/i915/gt/intel_llc.c index 14fe65812e42..1d19c073ba2e 100644 --- a/drivers/gpu/drm/i915/gt/intel_llc.c +++ b/drivers/gpu/drm/i915/gt/intel_llc.c @@ -12,6 +12,7 @@ #include "intel_llc.h" #include "intel_mchbar_regs.h" #include "intel_pcode.h" +#include "intel_rps.h"

struct ia_constants { unsigned int min_gpu_freq; @@ -55,9 +56,6 @@ static bool get_ia_constants(struct intel_llc *llc, if (!HAS_LLC(i915) || IS_DGFX(i915)) return false;

- if (rps->max_freq <= rps->min_freq) - return false; - consts->max_ia_freq = cpu_max_MHz();

consts->min_ring_freq = @@ -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; /* * For each potential GPU frequency, load a ring frequency we'd like * to use for memory access. We do this by specifying the IA frequency 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) +{ + 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 */ + freq /= GEN9_FREQ_SCALER; + } + return freq; + } +} + u32 intel_rps_get_rp0_frequency(struct intel_rps *rps) { struct intel_guc_slpc *slpc = rps_to_slpc(rps); @@ -2244,6 +2262,24 @@ u32 intel_rps_get_min_frequency(struct intel_rps *rps) return intel_gpu_freq(rps, rps->min_freq_softlimit); }

+u32 intel_rps_get_min_raw_freq(struct intel_rps *rps) +{ + struct intel_guc_slpc *slpc = rps_to_slpc(rps); + u32 freq; + + if (rps_uses_slpc(rps)) { + return DIV_ROUND_CLOSEST(slpc->min_freq, + GT_FREQUENCY_MULTIPLIER); + } else { + freq = rps->min_freq; + if (GRAPHICS_VER(rps_to_i915(rps)) >= 9) { + /* Convert GT frequency to 50 HZ units */ + freq /= GEN9_FREQ_SCALER; + } + return freq; + } +} + static int set_min_freq(struct intel_rps *rps, u32 val) { int ret = 0; diff --git a/drivers/gpu/drm/i915/gt/intel_rps.h b/drivers/gpu/drm/i915/gt/intel_rps.h index 8fe5a6bbdf66..64e4ef565e52 100644 --- a/drivers/gpu/drm/i915/gt/intel_rps.h +++ b/drivers/gpu/drm/i915/gt/intel_rps.h @@ -39,8 +39,10 @@ u32 intel_rps_get_cagf(struct intel_rps *rps, u32 rpstat1); u32 intel_rps_read_actual_frequency(struct intel_rps *rps); u32 intel_rps_get_requested_frequency(struct intel_rps *rps); u32 intel_rps_get_min_frequency(struct intel_rps *rps); +u32 intel_rps_get_min_raw_freq(struct intel_rps *rps); int intel_rps_set_min_frequency(struct intel_rps *rps, u32 val); u32 intel_rps_get_max_frequency(struct intel_rps *rps); +u32 intel_rps_get_max_raw_freq(struct intel_rps *rps); int intel_rps_set_max_frequency(struct intel_rps *rps, u32 val); u32 intel_rps_get_rp0_frequency(struct intel_rps *rps); u32 intel_rps_get_rp1_frequency(struct intel_rps *rps);

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!

On Wed, 31 Aug 2022 14:45:38 -0700, Rodrigo Vivi wrote:

Hi Rodrigo,

We need to inform PCODE of a desired ring frequencies so PCODE update the memory frequencies to us. rps->min_freq and rps->max_freq are the frequencies used in that request. However they were unset when SLPC was enabled and PCODE never updated the memory freq.

v2 (as Suggested by Ashutosh): if SLPC is in use, let's pick the right frequencies from the get_ia_constants instead of the fake init of rps' min and max.

v3: don't forget the max <= min return

v4: Move all the freq conversion to intel_rps.c. And the max <= min check to where it belongs.

v5: (Ashutosh) Fix old comment s/50 HZ/50 MHz and add a doc explaining the "raw format"

I think we both agree that mostly the way this patch is written it is to add SLPC but not risk disturbing host turbo, specially old platforms (CHV/VLV/ILK and pre-Gen 6). Also these freq units (sometimes 16.67 MHz units, sometimes 50 MHz, sometime MHz) in different places in the driver and different product generations is hugely confusing to say the least. For old platform we don't really know what units the freq's are in, we only know intel_gpu_freq will magically convert freq's to MHz. In any case let's work with what we have.

@@ -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 I would remove reference to "infinite loop", I am not seeing any infinite loop, maybe just delete the comment.

Also as I said I see the check above should be completely removed (so it is actually a pre-existing bug in the code). However since you want to carry it forward in order not to risk disturbing legacy behavior that's fine.

Rest LGTM:

Reviewed-by: Ashutosh Dixit ashutosh.dixit@intel.com