<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=utf-8">
</head>
<body>
<div>On Thu, 2022-08-25 at 16:59 -0700, Dixit, Ashutosh wrote:</div>
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<div>On Thu, 25 Aug 2022 15:23:15 -0700, Rodrigo Vivi wrote:<br>
</div>
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<div><br>
</div>
<div>We need to inform PCODE of a desired ring frequencies so PCODE update<br>
</div>
<div>the memory frequencies to us. rps->min_freq and rps->max_freq are the<br>
</div>
<div>frequencies used in that request. However they were unset when SLPC was<br>
</div>
<div>enabled and PCODE never updated the memory freq.<br>
</div>
<div><br>
</div>
<div>Let's at least for now get these freq set up so we can inform PCODE.<br>
</div>
</blockquote>
<div><br>
</div>
<div>Hi Rodrigo,<br>
</div>
<div><br>
</div>
<div>Great find. Though may I propose a more direct patch below for fixing this:<br>
</div>
<div><br>
</div>
<div>+++++++++++++++++++++++++++++++++++++++++++++<br>
</div>
<div>diff --git a/drivers/gpu/drm/i915/gt/intel_llc.c b/drivers/gpu/drm/i915/gt/intel_llc.c<br>
</div>
<div>index 14fe65812e42..a1791b6c7e04 100644<br>
</div>
<div>--- a/drivers/gpu/drm/i915/gt/intel_llc.c<br>
</div>
<div>+++ b/drivers/gpu/drm/i915/gt/intel_llc.c<br>
</div>
<div>@@ -49,6 +49,7 @@ static unsigned int cpu_max_MHz(void)<br>
</div>
<div> static bool get_ia_constants(struct intel_llc *llc,<br>
</div>
<div> struct ia_constants *consts)<br>
</div>
<div> {<br>
</div>
<div>+ struct intel_guc_slpc *slpc = &llc_to_gt(llc)->uc.guc.slpc;<br>
</div>
<div> struct drm_i915_private *i915 = llc_to_gt(llc)->i915;<br>
</div>
<div> struct intel_rps *rps = &llc_to_gt(llc)->rps;<br>
</div>
<div><br>
</div>
<div>@@ -65,8 +66,14 @@ static bool get_ia_constants(struct intel_llc *llc,<br>
</div>
<div> /* convert DDR frequency from units of 266.6MHz to bandwidth */<br>
</div>
<div> consts->min_ring_freq = mult_frac(consts->min_ring_freq, 8, 3);<br>
</div>
<div><br>
</div>
<div>- consts->min_gpu_freq = rps->min_freq;<br>
</div>
<div>- consts->max_gpu_freq = rps->max_freq;<br>
</div>
<div>+ if (intel_uc_uses_guc_slpc(&llc_to_gt(llc)->uc)) {<br>
</div>
<div>+ consts->min_gpu_freq = slpc->min_freq;<br>
</div>
<div>+ consts->max_gpu_freq = slpc->rp0_freq;<br>
</div>
<div>+ } else {<br>
</div>
<div>+ consts->min_gpu_freq = rps->min_freq;<br>
</div>
<div>+ consts->max_gpu_freq = rps->max_freq;<br>
</div>
<div>+ }<br>
</div>
<div>+<br>
</div>
<div> if (GRAPHICS_VER(i915) >= 9) {<br>
</div>
<div> /* Convert GT frequency to 50 HZ units */<br>
</div>
<div> consts->min_gpu_freq /= GEN9_FREQ_SCALER;<br>
</div>
<div>+++++++++++++++++++++++++++++++++++++++++++++<br>
</div>
<div><br>
</div>
<div>I have only compile tested the patch but it looks like everything is set up<br>
</div>
<div>so the patch above should work. The call stack for slpc initialization is<br>
</div>
<div>the following (I am writing here due to the rather opaque uc macros):<br>
</div>
<div><br>
</div>
<div>intel_gt_resume<br>
</div>
<div>-> intel_gt_init_hw<br>
</div>
<div>-> intel_uc_init_hw/__uc_init_hw<br>
</div>
<div>-> intel_guc_slpc_enable<br>
</div>
<div>-> slpc_get_rp_values<br>
</div>
<div><br>
</div>
<div>As we can see intel_llc_enable() is called after intel_gt_init_hw() in<br>
</div>
<div>intel_gt_resume() so SLPC params should be set up.<br>
</div>
</blockquote>
<div><br>
</div>
<div>Yeap, I took that path worried about timing, but you are right this should</div>
<div>be there already and it would be cleaner. </div>
<div><br>
</div>
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<div><br>
</div>
<div>What you have is fine too, I can R-b that if you prefer that.<br>
</div>
</blockquote>
<div><br>
</div>
<div>Your is better and cleaner. Let me try that first here and then I will resend it.</div>
<div><br>
</div>
<div>Thank you!</div>
<div><br>
</div>
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<div><br>
</div>
<div>Thanks.<br>
</div>
<div>--<br>
</div>
<div>Ashutosh<br>
</div>
<div><br>
</div>
<blockquote type="cite" style="margin:0 0 0 .8ex; border-left:2px #729fcf solid;padding-left:1ex">
<div>Cc: Ashutosh Dixit <<a href="mailto:ashutosh.dixit@intel.com">ashutosh.dixit@intel.com</a>><br>
</div>
<div>Tested-by: Sushma Venkatesh Reddy <<a href="mailto:sushma.venkatesh.reddy@intel.com">sushma.venkatesh.reddy@intel.com</a>><br>
</div>
<div>Signed-off-by: Rodrigo Vivi <<a href="mailto:rodrigo.vivi@intel.com">rodrigo.vivi@intel.com</a>><br>
</div>
<div>---<br>
</div>
<div> drivers/gpu/drm/i915/gt/intel_rps.c | 18 +++++++++++++++++-<br>
</div>
<div> 1 file changed, 17 insertions(+), 1 deletion(-)<br>
</div>
<div><br>
</div>
<div>diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c<br>
</div>
<div>index 8c289a032103..58a82978d5df 100644<br>
</div>
<div>--- a/drivers/gpu/drm/i915/gt/intel_rps.c<br>
</div>
<div>+++ b/drivers/gpu/drm/i915/gt/intel_rps.c<br>
</div>
<div>@@ -1128,6 +1128,20 @@ void gen6_rps_get_freq_caps(struct intel_rps *rps, struct intel_rps_freq_caps *c<br>
</div>
<div> }<br>
</div>
<div> }<br>
</div>
<div><br>
</div>
<div>+static void rps_basic_init_for_slpc(struct intel_rps *rps)<br>
</div>
<div>+{<br>
</div>
<div>+ struct intel_rps_freq_caps caps;<br>
</div>
<div>+<br>
</div>
<div>+ /*<br>
</div>
<div>+ * Even with SLPC we need to initialize at least a basic min and max<br>
</div>
<div>+ * frequency so we can inform pcode a desired IA ring frequency in<br>
</div>
<div>+ * gen6_update_ring_freq<br>
</div>
<div>+ */<br>
</div>
<div>+ gen6_rps_get_freq_caps(rps, &caps);<br>
</div>
<div>+ rps->min_freq = caps.min_freq;<br>
</div>
<div>+ rps->max_freq = caps.rp0_freq;<br>
</div>
<div>+}<br>
</div>
<div>+<br>
</div>
<div> static void gen6_rps_init(struct intel_rps *rps)<br>
</div>
<div> {<br>
</div>
<div> struct drm_i915_private *i915 = rps_to_i915(rps);<br>
</div>
<div>@@ -1970,8 +1984,10 @@ void intel_rps_init(struct intel_rps *rps)<br>
</div>
<div> {<br>
</div>
<div> struct drm_i915_private *i915 = rps_to_i915(rps);<br>
</div>
<div><br>
</div>
<div>- if (rps_uses_slpc(rps))<br>
</div>
<div>+ if (rps_uses_slpc(rps)) {<br>
</div>
<div>+ rps_basic_init_for_slpc(rps);<br>
</div>
<div> return;<br>
</div>
<div>+ }<br>
</div>
<div><br>
</div>
<div> if (IS_CHERRYVIEW(i915))<br>
</div>
<div> chv_rps_init(rps);<br>
</div>
<div>--<br>
</div>
<div>2.37.1<br>
</div>
<div><br>
</div>
</blockquote>
</blockquote>
<div><br>
</div>
<div><span></span></div>
</body>
</html>