Re: [Intel-gfx] [PATCH] drm/i915/slpc: Set rps' min and max frequencies even with SLPC.

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

[-- Attachment #1: Type: text/plain, Size: 4471 bytes --]

On Thu, 2022-08-25 at 16:59 -0700, Dixit, Ashutosh wrote:
On Thu, 25 Aug 2022 15:23:15 -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.

Let's at least for now get these freq set up so we can inform PCODE.

Hi Rodrigo,

Great find. Though may I propose a more direct patch below for fixing this:

+++++++++++++++++++++++++++++++++++++++++++++
diff --git a/drivers/gpu/drm/i915/gt/intel_llc.c b/drivers/gpu/drm/i915/gt/intel_llc.c
index 14fe65812e42..a1791b6c7e04 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,14 @@ 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;
+++++++++++++++++++++++++++++++++++++++++++++

I have only compile tested the patch but it looks like everything is set up
so the patch above should work. The call stack for slpc initialization is
the following (I am writing here due to the rather opaque uc macros):

intel_gt_resume
-> intel_gt_init_hw
-> intel_uc_init_hw/__uc_init_hw
-> intel_guc_slpc_enable
-> slpc_get_rp_values

As we can see intel_llc_enable() is called after intel_gt_init_hw() in
intel_gt_resume() so SLPC params should be set up.

Yeap, I took that path worried about timing, but you are right this should
be there already and it would be cleaner.


What you have is fine too, I can R-b that if you prefer that.

Your is better and cleaner. Let me try that first here and then I will resend it.

Thank you!


Thanks.
--
Ashutosh

Cc: Ashutosh Dixit <ashutosh.dixit@intel.com<mailto:ashutosh.dixit@intel.com>>
Tested-by: Sushma Venkatesh Reddy <sushma.venkatesh.reddy@intel.com<mailto:sushma.venkatesh.reddy@intel.com>>
Signed-off-by: Rodrigo Vivi <rodrigo.vivi@intel.com<mailto:rodrigo.vivi@intel.com>>
---
 drivers/gpu/drm/i915/gt/intel_rps.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c
index 8c289a032103..58a82978d5df 100644
--- a/drivers/gpu/drm/i915/gt/intel_rps.c
+++ b/drivers/gpu/drm/i915/gt/intel_rps.c
@@ -1128,6 +1128,20 @@ void gen6_rps_get_freq_caps(struct intel_rps *rps, struct intel_rps_freq_caps *c
        }
 }

+static void rps_basic_init_for_slpc(struct intel_rps *rps)
+{
+       struct intel_rps_freq_caps caps;
+
+       /*
+        * Even with SLPC we need to initialize at least a basic min and max
+        * frequency so we can inform pcode a desired IA ring frequency in
+        * gen6_update_ring_freq
+        */
+       gen6_rps_get_freq_caps(rps, &caps);
+       rps->min_freq = caps.min_freq;
+       rps->max_freq = caps.rp0_freq;
+}
+
 static void gen6_rps_init(struct intel_rps *rps)
 {
        struct drm_i915_private *i915 = rps_to_i915(rps);
@@ -1970,8 +1984,10 @@ void intel_rps_init(struct intel_rps *rps)
 {
        struct drm_i915_private *i915 = rps_to_i915(rps);

-       if (rps_uses_slpc(rps))
+       if (rps_uses_slpc(rps)) {
+               rps_basic_init_for_slpc(rps);
                return;
+       }

        if (IS_CHERRYVIEW(i915))
                chv_rps_init(rps);
--
2.37.1



[-- Attachment #2: Type: text/html, Size: 9264 bytes --]