| Message ID | 20220826174434.157513-1-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/slpc: Fix PCODE IA Freq requests when using SLPC | expand |
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
On Fri, 26 Aug 2022 13:03:05 -0700, Dixit, Ashutosh wrote: > > 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; Sorry, this also doesn't work because slpc freq are converted using intel_gpu_freq(). How about calling gen6_rps_get_freq_caps() directly in the SLPC case? Or we could go with the original patch for now with a FIXME? Thanks. > > + } 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; > > + }
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; consts->max_ia_freq = cpu_max_MHz(); @@ -65,13 +80,6 @@ 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; - } return true; }
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 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(-)