| Message ID | 20220831214538.143950-1-rodrigo.vivi@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/slpc: Let's fix the PCODE min freq table setup for SLPC | expand |
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>
On Wed, Aug 31, 2022 at 03:17:26PM -0700, Dixit, Ashutosh wrote: > 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. yeap! > > > @@ -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. I know we can get the infinit loop because I faced it here on a bad config where min = max. os if min >= max, the for loop will never close. And in case we have some fused parts with min = max we will take a while to figure out what's going on during po. and who knows about older platforms and skus out there as well. I will keep the comment so we don't end up removing it from here. > > Rest LGTM: > > Reviewed-by: Ashutosh Dixit <ashutosh.dixit@intel.com> Thanks
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..318bf913c507 100644 --- a/drivers/gpu/drm/i915/gt/intel_rps.c +++ b/drivers/gpu/drm/i915/gt/intel_rps.c @@ -2156,6 +2156,31 @@ u32 intel_rps_get_max_frequency(struct intel_rps *rps) return intel_gpu_freq(rps, rps->max_freq_softlimit); } +/** + * intel_rps_get_max_raw_freq - returns the max frequency in some raw format. + * @rps: the intel_rps structure + * + * Returns the max frequency in a raw format. In newer platforms raw is in + * units of 50 MHz. + */ +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 MHz 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 +2269,31 @@ u32 intel_rps_get_min_frequency(struct intel_rps *rps) return intel_gpu_freq(rps, rps->min_freq_softlimit); } +/** + * intel_rps_get_min_raw_freq - returns the min frequency in some raw format. + * @rps: the intel_rps structure + * + * Returns the min frequency in a raw format. In newer platforms raw is in + * units of 50 MHz. + */ +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 MHz 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);
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" 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 | 50 +++++++++++++++++++++++++++++ drivers/gpu/drm/i915/gt/intel_rps.h | 2 ++ 3 files changed, 61 insertions(+), 10 deletions(-)