diff mbox series

drm/i915/slpc: Set rps' min and max frequencies even with SLPC.

Message ID 20220825222315.58696-1-rodrigo.vivi@intel.com (mailing list archive)
State New, archived
Headers show
Series drm/i915/slpc: Set rps' min and max frequencies even with SLPC. | expand

Commit Message

Rodrigo Vivi Aug. 25, 2022, 10:23 p.m. UTC 
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.

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_rps.c | 18 +++++++++++++++++-
 1 file changed, 17 insertions(+), 1 deletion(-)

Comments

Rodrigo Vivi Aug. 25, 2022, 10:42 p.m. UTC | #1 
On Thu, Aug 25, 2022 at 06:23:15PM -0400, 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.
> 
> 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>

As suggested by Sushma:
Fixes: 7ba79a671568 ("drm/i915/guc/slpc: Gate Host RPS when SLPC is enabled")
Cc: <stable@vger.kernel.org> # v5.15+

> ---
>  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
>
Dixit, Ashutosh Aug. 25, 2022, 11:59 p.m. UTC | #2 
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.

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

Thanks.
--
Ashutosh

> 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_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
>
Rodrigo Vivi Aug. 26, 2022, 9:28 a.m. UTC | #3 
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
diff mbox series

Patch

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