Re: [Intel-gfx] [PATCH 4/6] drm/i915: Use GEN12 RPSTAT register
On 14/09/2022 17:11, Dixit, Ashutosh wrote: On Wed, 14 Sep 2022 02:56:26 -0700, Nilawar, Badal wrote: On 13-09-2022 13:17, Tvrtko Ursulin wrote: On 13/09/2022 01:09, Dixit, Ashutosh wrote: On Mon, 12 Sep 2022 04:29:38 -0700, Nilawar, Badal wrote: diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c index 958b37123bf1..a24704ec2c18 100644 --- a/drivers/gpu/drm/i915/i915_pmu.c +++ b/drivers/gpu/drm/i915/i915_pmu.c @@ -371,7 +371,6 @@ static void frequency_sample(struct intel_gt *gt, unsigned int period_ns) { struct drm_i915_private *i915 = gt->i915; - struct intel_uncore *uncore = gt->uncore; struct i915_pmu *pmu = &i915->pmu; struct intel_rps *rps = >->rps; @@ -394,7 +393,7 @@ frequency_sample(struct intel_gt *gt, unsigned int period_ns) * case we assume the system is running at the intended * frequency. Fortunately, the read should rarely fail! */ - val = intel_uncore_read_fw(uncore, GEN6_RPSTAT1); + val = intel_rps_read_rpstat(rps); Hmm, we got rid of _fw which the comment above refers to. Maybe we need a fw flag to intel_rps_read_rpstat? Above function before reading rpstat it checks if gt is awake. Ok, so you are referring to intel_gt_pm_get_if_awake check in frequency_sample. So when gt is awake shouldn't matter if we read GEN6_RPSTAT1 with forcewake.In that case we can remove above comment. Let me know your thoughts on this. I am not entirely sure about this. For example in c1c82d267ae8 intel_uncore_read_fw was introduced with the same intel_gt_pm_get_if_awake check. So this would mean even if gt is awake not taking forcewake makes a difference. The same code pattern was retained in b66ecd0438bf. Maybe it's because there are no locks? Its about power. As c1c82d267ae8 ("drm/i915/pmu: Cheat when reading the actual frequency to avoid fw") explains the _fw variant is to avoid preventing RC6, and so increased GPU power draw, just because someone has PMU open. (Because of the 200Hz sampling timer that is needed for PMU frequency reporting.) Under the circumstances I think we could do one of two things: 1. If we want to drop _fw, we should do it as a separate patch with its own justification so it can be reviewed separately. 2. Otherwise as I mentioned we should retain the _fw and add a fw flag to intel_rps_read_rpstat. Agreed. Or instead of the flag, the usual pattern of having intel_rps_read_rpstat_fw and make intel_rps_read_rpsstat get the forcewake. Also, may I ask, this patch is in the MTL enablement series but the commit message and patch content seem like it is fixing a wider Gen12 issue? What is the extent of incorrect behaviour without it? Should it be tagged for stable for first Tigerlake supporting kernel? GEN6_RPSTAT1(0xa01c) and GEN12_RPSTAT1(0x1381b4) both are supported by gen12 and above. The difference between two is GEN6_RPSTAT1 falls under RENDER forcewake domain and GEN12_RPSTAT1 does not require forcewake to access. GEN12_RPSTAT1 is punit register and when GT is in RC6 it will give frequency as 0. Correct, so no changes needed for stable kernels. But going forward Badal is proposing (which I sort of agree with but may need some discussion) that we change i915 behavior to return 0 freq (instead of cur_freq or RPn) when GT is idle or in RC6 (so we don't take forcewake to read freq when GT is in RC6). We already report zero when GT is idle (as considered by software tracking) so I guess the only part you'd like to change is drop the else branch in: val = intel_uncore_read_fw(uncore, GEN6_RPSTAT1); if (val) val = intel_rps_get_cagf(rps, val); else val = rps->cur_freq; ? What would be pros and cons? Regards, Tvrtko
Re: [Intel-gfx] [PATCH 4/6] drm/i915: Use GEN12 RPSTAT register
On Wed, 14 Sep 2022 02:56:26 -0700, Nilawar, Badal wrote: > > On 13-09-2022 13:17, Tvrtko Ursulin wrote: > > > > On 13/09/2022 01:09, Dixit, Ashutosh wrote: > >> On Mon, 12 Sep 2022 04:29:38 -0700, Nilawar, Badal wrote: > >>> > > diff --git a/drivers/gpu/drm/i915/i915_pmu.c > > b/drivers/gpu/drm/i915/i915_pmu.c > > index 958b37123bf1..a24704ec2c18 100644 > > --- a/drivers/gpu/drm/i915/i915_pmu.c > > +++ b/drivers/gpu/drm/i915/i915_pmu.c > > @@ -371,7 +371,6 @@ static void > > frequency_sample(struct intel_gt *gt, unsigned int period_ns) > > { > > struct drm_i915_private *i915 = gt->i915; > > - struct intel_uncore *uncore = gt->uncore; > > struct i915_pmu *pmu = &i915->pmu; > > struct intel_rps *rps = >->rps; > > > > @@ -394,7 +393,7 @@ frequency_sample(struct intel_gt *gt, unsigned > > int period_ns) > > * case we assume the system is running at the intended > > * frequency. Fortunately, the read should rarely fail! > > */ > > - val = intel_uncore_read_fw(uncore, GEN6_RPSTAT1); > > + val = intel_rps_read_rpstat(rps); > > Hmm, we got rid of _fw which the comment above refers to. Maybe we > need a > fw flag to intel_rps_read_rpstat? > >>> > >>> Above function before reading rpstat it checks if gt is awake. > >> > >> Ok, so you are referring to intel_gt_pm_get_if_awake check in > >> frequency_sample. > >> > >>> So when gt is awake shouldn't matter if we read GEN6_RPSTAT1 with > >>> forcewake.In that case we can remove above comment. Let me know your > >>> thoughts on this. > >> > >> I am not entirely sure about this. For example in c1c82d267ae8 > >> intel_uncore_read_fw was introduced with the same > >> intel_gt_pm_get_if_awake > >> check. So this would mean even if gt is awake not taking forcewake makes > >> a > >> difference. The same code pattern was retained in b66ecd0438bf. Maybe > >> it's > >> because there are no locks? > > > > Its about power. As c1c82d267ae8 ("drm/i915/pmu: Cheat when reading the > > actual frequency to avoid fw") explains the _fw variant is to avoid > > preventing RC6, and so increased GPU power draw, just because someone has > > PMU open. (Because of the 200Hz sampling timer that is needed for PMU > > frequency reporting.) > > > >> Under the circumstances I think we could do one of two things: > >> 1. If we want to drop _fw, we should do it as a separate patch with its > >> own > >> justification so it can be reviewed separately. > >> 2. Otherwise as I mentioned we should retain the _fw and add a fw flag to > >> intel_rps_read_rpstat. > > > > Agreed. Or instead of the flag, the usual pattern of having > > intel_rps_read_rpstat_fw and make intel_rps_read_rpsstat get the > > forcewake. > > > > Also, may I ask, this patch is in the MTL enablement series but the > > commit message and patch content seem like it is fixing a wider Gen12 > > issue? What is the extent of incorrect behaviour without it? Should it be > > tagged for stable for first Tigerlake supporting kernel? > > GEN6_RPSTAT1(0xa01c) and GEN12_RPSTAT1(0x1381b4) both are supported by > gen12 and above. The difference between two is GEN6_RPSTAT1 falls under > RENDER forcewake domain and GEN12_RPSTAT1 does not require forcewake to > access. GEN12_RPSTAT1 is punit register and when GT is in RC6 it will give > frequency as 0. Correct, so no changes needed for stable kernels. But going forward Badal is proposing (which I sort of agree with but may need some discussion) that we change i915 behavior to return 0 freq (instead of cur_freq or RPn) when GT is idle or in RC6 (so we don't take forcewake to read freq when GT is in RC6). > Reason for clubbing this patch with MTL series is due to common function > intel_rps_read_rpstat. I think I should send this patch in separate series. Agree! Thanks. -- Ashutosh
Re: [Intel-gfx] [PATCH 4/6] drm/i915: Use GEN12 RPSTAT register
On 13-09-2022 13:17, Tvrtko Ursulin wrote: On 13/09/2022 01:09, Dixit, Ashutosh wrote: On Mon, 12 Sep 2022 04:29:38 -0700, Nilawar, Badal wrote: diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c index 958b37123bf1..a24704ec2c18 100644 --- a/drivers/gpu/drm/i915/i915_pmu.c +++ b/drivers/gpu/drm/i915/i915_pmu.c @@ -371,7 +371,6 @@ static void frequency_sample(struct intel_gt *gt, unsigned int period_ns) { struct drm_i915_private *i915 = gt->i915; - struct intel_uncore *uncore = gt->uncore; struct i915_pmu *pmu = &i915->pmu; struct intel_rps *rps = >->rps; @@ -394,7 +393,7 @@ frequency_sample(struct intel_gt *gt, unsigned int period_ns) * case we assume the system is running at the intended * frequency. Fortunately, the read should rarely fail! */ - val = intel_uncore_read_fw(uncore, GEN6_RPSTAT1); + val = intel_rps_read_rpstat(rps); Hmm, we got rid of _fw which the comment above refers to. Maybe we need a fw flag to intel_rps_read_rpstat? Above function before reading rpstat it checks if gt is awake. Ok, so you are referring to intel_gt_pm_get_if_awake check in frequency_sample. So when gt is awake shouldn't matter if we read GEN6_RPSTAT1 with forcewake.In that case we can remove above comment. Let me know your thoughts on this. I am not entirely sure about this. For example in c1c82d267ae8 intel_uncore_read_fw was introduced with the same intel_gt_pm_get_if_awake check. So this would mean even if gt is awake not taking forcewake makes a difference. The same code pattern was retained in b66ecd0438bf. Maybe it's because there are no locks? Its about power. As c1c82d267ae8 ("drm/i915/pmu: Cheat when reading the actual frequency to avoid fw") explains the _fw variant is to avoid preventing RC6, and so increased GPU power draw, just because someone has PMU open. (Because of the 200Hz sampling timer that is needed for PMU frequency reporting.) Under the circumstances I think we could do one of two things: 1. If we want to drop _fw, we should do it as a separate patch with its own justification so it can be reviewed separately. 2. Otherwise as I mentioned we should retain the _fw and add a fw flag to intel_rps_read_rpstat. Agreed. Or instead of the flag, the usual pattern of having intel_rps_read_rpstat_fw and make intel_rps_read_rpsstat get the forcewake. Also, may I ask, this patch is in the MTL enablement series but the commit message and patch content seem like it is fixing a wider Gen12 issue? What is the extent of incorrect behaviour without it? Should it be tagged for stable for first Tigerlake supporting kernel? GEN6_RPSTAT1(0xa01c) and GEN12_RPSTAT1(0x1381b4) both are supported by gen12 and above. The difference between two is GEN6_RPSTAT1 falls under RENDER forcewake domain and GEN12_RPSTAT1 does not require forcewake to access. GEN12_RPSTAT1 is punit register and when GT is in RC6 it will give frequency as 0. Reason for clubbing this patch with MTL series is due to common function intel_rps_read_rpstat. I think I should send this patch in separate series. Regards, Badal Regards, Tvrtko
Re: [Intel-gfx] [PATCH 4/6] drm/i915: Use GEN12 RPSTAT register
On 13/09/2022 01:09, Dixit, Ashutosh wrote: On Mon, 12 Sep 2022 04:29:38 -0700, Nilawar, Badal wrote: diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c index 958b37123bf1..a24704ec2c18 100644 --- a/drivers/gpu/drm/i915/i915_pmu.c +++ b/drivers/gpu/drm/i915/i915_pmu.c @@ -371,7 +371,6 @@ static void frequency_sample(struct intel_gt *gt, unsigned int period_ns) { struct drm_i915_private *i915 = gt->i915; - struct intel_uncore *uncore = gt->uncore; struct i915_pmu *pmu = &i915->pmu; struct intel_rps *rps = >->rps; @@ -394,7 +393,7 @@ frequency_sample(struct intel_gt *gt, unsigned int period_ns) * case we assume the system is running at the intended * frequency. Fortunately, the read should rarely fail! */ - val = intel_uncore_read_fw(uncore, GEN6_RPSTAT1); + val = intel_rps_read_rpstat(rps); Hmm, we got rid of _fw which the comment above refers to. Maybe we need a fw flag to intel_rps_read_rpstat? Above function before reading rpstat it checks if gt is awake. Ok, so you are referring to intel_gt_pm_get_if_awake check in frequency_sample. So when gt is awake shouldn't matter if we read GEN6_RPSTAT1 with forcewake.In that case we can remove above comment. Let me know your thoughts on this. I am not entirely sure about this. For example in c1c82d267ae8 intel_uncore_read_fw was introduced with the same intel_gt_pm_get_if_awake check. So this would mean even if gt is awake not taking forcewake makes a difference. The same code pattern was retained in b66ecd0438bf. Maybe it's because there are no locks? Its about power. As c1c82d267ae8 ("drm/i915/pmu: Cheat when reading the actual frequency to avoid fw") explains the _fw variant is to avoid preventing RC6, and so increased GPU power draw, just because someone has PMU open. (Because of the 200Hz sampling timer that is needed for PMU frequency reporting.) Under the circumstances I think we could do one of two things: 1. If we want to drop _fw, we should do it as a separate patch with its own justification so it can be reviewed separately. 2. Otherwise as I mentioned we should retain the _fw and add a fw flag to intel_rps_read_rpstat. Agreed. Or instead of the flag, the usual pattern of having intel_rps_read_rpstat_fw and make intel_rps_read_rpsstat get the forcewake. Also, may I ask, this patch is in the MTL enablement series but the commit message and patch content seem like it is fixing a wider Gen12 issue? What is the extent of incorrect behaviour without it? Should it be tagged for stable for first Tigerlake supporting kernel? Regards, Tvrtko
Re: [Intel-gfx] [PATCH 4/6] drm/i915: Use GEN12 RPSTAT register
On Mon, 12 Sep 2022 04:29:38 -0700, Nilawar, Badal wrote: > > >> diff --git a/drivers/gpu/drm/i915/i915_pmu.c > >> b/drivers/gpu/drm/i915/i915_pmu.c > >> index 958b37123bf1..a24704ec2c18 100644 > >> --- a/drivers/gpu/drm/i915/i915_pmu.c > >> +++ b/drivers/gpu/drm/i915/i915_pmu.c > >> @@ -371,7 +371,6 @@ static void > >> frequency_sample(struct intel_gt *gt, unsigned int period_ns) > >> { > >>struct drm_i915_private *i915 = gt->i915; > >> - struct intel_uncore *uncore = gt->uncore; > >>struct i915_pmu *pmu = &i915->pmu; > >>struct intel_rps *rps = >->rps; > >> > >> @@ -394,7 +393,7 @@ frequency_sample(struct intel_gt *gt, unsigned int > >> period_ns) > >> * case we assume the system is running at the intended > >> * frequency. Fortunately, the read should rarely fail! > >> */ > >> - val = intel_uncore_read_fw(uncore, GEN6_RPSTAT1); > >> + val = intel_rps_read_rpstat(rps); > > > > Hmm, we got rid of _fw which the comment above refers to. Maybe we need a > > fw flag to intel_rps_read_rpstat? > > Above function before reading rpstat it checks if gt is awake. Ok, so you are referring to intel_gt_pm_get_if_awake check in frequency_sample. > So when gt is awake shouldn't matter if we read GEN6_RPSTAT1 with > forcewake.In that case we can remove above comment. Let me know your > thoughts on this. I am not entirely sure about this. For example in c1c82d267ae8 intel_uncore_read_fw was introduced with the same intel_gt_pm_get_if_awake check. So this would mean even if gt is awake not taking forcewake makes a difference. The same code pattern was retained in b66ecd0438bf. Maybe it's because there are no locks? Under the circumstances I think we could do one of two things: 1. If we want to drop _fw, we should do it as a separate patch with its own justification so it can be reviewed separately. 2. Otherwise as I mentioned we should retain the _fw and add a fw flag to intel_rps_read_rpstat. Thanks. -- Ashutosh > >>if (val) > >>val = intel_rps_get_cagf(rps, val); > >>else > >> -- > >> 2.25.1 > >>
Re: [Intel-gfx] [PATCH 4/6] drm/i915: Use GEN12 RPSTAT register
On 10-09-2022 07:19, Dixit, Ashutosh wrote: On Thu, 08 Sep 2022 19:56:44 -0700, Badal Nilawar wrote: From: Don Hiatt On GEN12, use the correct GEN12 RPSTAT register mask/shift. HSD: 1409538411 I think let's remove this. Sure. Cc: Don Hiatt Cc: Andi Shyti Signed-off-by: Don Hiatt Signed-off-by: Badal Nilawar --- drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 2 +- drivers/gpu/drm/i915/gt/intel_gt_regs.h | 5 + drivers/gpu/drm/i915/gt/intel_rps.c | 17 - drivers/gpu/drm/i915/gt/intel_rps.h | 1 + drivers/gpu/drm/i915/i915_pmu.c | 3 +-- 5 files changed, 24 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c index 108b9e76c32e..96c03a1258e1 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c @@ -380,7 +380,7 @@ void intel_gt_pm_frequency_dump(struct intel_gt *gt, struct drm_printer *p) rpinclimit = intel_uncore_read(uncore, GEN6_RP_UP_THRESHOLD); rpdeclimit = intel_uncore_read(uncore, GEN6_RP_DOWN_THRESHOLD); - rpstat = intel_uncore_read(uncore, GEN6_RPSTAT1); + rpstat = intel_rps_read_rpstat(rps); rpcurupei = intel_uncore_read(uncore, GEN6_RP_CUR_UP_EI) & GEN6_CURICONT_MASK; rpcurup = intel_uncore_read(uncore, GEN6_RP_CUR_UP) & GEN6_CURBSYTAVG_MASK; rpprevup = intel_uncore_read(uncore, GEN6_RP_PREV_UP) & GEN6_CURBSYTAVG_MASK; diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h index fb2c56777480..dac59c3e68db 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h @@ -1510,6 +1510,11 @@ #define VLV_RENDER_C0_COUNT _MMIO(0x138118) #define VLV_MEDIA_C0_COUNT_MMIO(0x13811c) +#define GEN12_RPSTAT1 _MMIO(0x1381b4) +#define GEN12_CAGF_SHIFT 11 +#define GEN12_CAGF_MASK REG_GENMASK(19, 11) +#define GEN12_VOLTAGE_MASK REG_GENMASK(10, 0) Let's remove GEN12_VOLTAGE_MASK, looks like it's not being used. Yes, not used. I will remove this. + #define GEN11_GT_INTR_DW(x) _MMIO(0x190018 + ((x) * 4)) #define GEN11_CSME (31) #define GEN11_GUNIT (28) diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c index 6fadde4ee7bf..341f96f536e8 100644 --- a/drivers/gpu/drm/i915/gt/intel_rps.c +++ b/drivers/gpu/drm/i915/gt/intel_rps.c @@ -2040,6 +2040,19 @@ void intel_rps_sanitize(struct intel_rps *rps) rps_disable_interrupts(rps); } +u32 intel_rps_read_rpstat(struct intel_rps *rps) +{ + struct drm_i915_private *i915 = rps_to_i915(rps); + u32 rpstat; + + if (GRAPHICS_VER(i915) >= 12) + rpstat = intel_uncore_read(rps_to_gt(rps)->uncore, GEN12_RPSTAT1); + else + rpstat = intel_uncore_read(rps_to_gt(rps)->uncore, GEN6_RPSTAT1); Probably nit but how about something like this: i915_reg_t rpstat; if (GRAPHICS_VER(i915) >= 12) rpstat = GEN12_RPSTAT1; else rpstat = GEN6_RPSTAT1; return intel_uncore_read(rps_to_gt(rps)->uncore, rpstat); Ok + + return rpstat; +} + u32 intel_rps_get_cagf(struct intel_rps *rps, u32 rpstat) { struct drm_i915_private *i915 = rps_to_i915(rps); @@ -2047,6 +2060,8 @@ u32 intel_rps_get_cagf(struct intel_rps *rps, u32 rpstat) if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915)) cagf = (rpstat >> 8) & 0xff; + else if (GRAPHICS_VER(i915) >= 12) + cagf = (rpstat & GEN12_CAGF_MASK) >> GEN12_CAGF_SHIFT; else if (GRAPHICS_VER(i915) >= 9) cagf = (rpstat & GEN9_CAGF_MASK) >> GEN9_CAGF_SHIFT; else if (IS_HASWELL(i915) || IS_BROADWELL(i915)) @@ -2071,7 +2086,7 @@ static u32 read_cagf(struct intel_rps *rps) freq = vlv_punit_read(i915, PUNIT_REG_GPU_FREQ_STS); vlv_punit_put(i915); } else if (GRAPHICS_VER(i915) >= 6) { - freq = intel_uncore_read(uncore, GEN6_RPSTAT1); + freq = intel_rps_read_rpstat(rps); } else { freq = intel_uncore_read(uncore, MEMSTAT_ILK); } diff --git a/drivers/gpu/drm/i915/gt/intel_rps.h b/drivers/gpu/drm/i915/gt/intel_rps.h index 4509dfdc52e0..08bae6d97870 100644 --- a/drivers/gpu/drm/i915/gt/intel_rps.h +++ b/drivers/gpu/drm/i915/gt/intel_rps.h @@ -47,6 +47,7 @@ u32 intel_rps_get_rp1_frequency(struct intel_rps *rps); u32 intel_rps_get_rpn_frequency(struct intel_rps *rps); u32 intel_rps_read_punit_req(struct intel_rps *rps); u32 intel_rps_read_punit_req_frequency(struct intel_rps *rps); +u32 intel_rps_re
Re: [Intel-gfx] [PATCH 4/6] drm/i915: Use GEN12 RPSTAT register
On Thu, 08 Sep 2022 19:56:44 -0700, Badal Nilawar wrote: > > From: Don Hiatt > > On GEN12, use the correct GEN12 RPSTAT register mask/shift. > > HSD: 1409538411 I think let's remove this. > Cc: Don Hiatt > Cc: Andi Shyti > Signed-off-by: Don Hiatt > Signed-off-by: Badal Nilawar > --- > drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 2 +- > drivers/gpu/drm/i915/gt/intel_gt_regs.h | 5 + > drivers/gpu/drm/i915/gt/intel_rps.c | 17 - > drivers/gpu/drm/i915/gt/intel_rps.h | 1 + > drivers/gpu/drm/i915/i915_pmu.c | 3 +-- > 5 files changed, 24 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c > b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c > index 108b9e76c32e..96c03a1258e1 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c > +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c > @@ -380,7 +380,7 @@ void intel_gt_pm_frequency_dump(struct intel_gt *gt, > struct drm_printer *p) > rpinclimit = intel_uncore_read(uncore, GEN6_RP_UP_THRESHOLD); > rpdeclimit = intel_uncore_read(uncore, GEN6_RP_DOWN_THRESHOLD); > > - rpstat = intel_uncore_read(uncore, GEN6_RPSTAT1); > + rpstat = intel_rps_read_rpstat(rps); > rpcurupei = intel_uncore_read(uncore, GEN6_RP_CUR_UP_EI) & > GEN6_CURICONT_MASK; > rpcurup = intel_uncore_read(uncore, GEN6_RP_CUR_UP) & > GEN6_CURBSYTAVG_MASK; > rpprevup = intel_uncore_read(uncore, GEN6_RP_PREV_UP) & > GEN6_CURBSYTAVG_MASK; > diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h > b/drivers/gpu/drm/i915/gt/intel_gt_regs.h > index fb2c56777480..dac59c3e68db 100644 > --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h > +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h > @@ -1510,6 +1510,11 @@ > #define VLV_RENDER_C0_COUNT _MMIO(0x138118) > #define VLV_MEDIA_C0_COUNT _MMIO(0x13811c) > > +#define GEN12_RPSTAT1_MMIO(0x1381b4) > +#define GEN12_CAGF_SHIFT 11 > +#define GEN12_CAGF_MASKREG_GENMASK(19, 11) > +#define GEN12_VOLTAGE_MASK REG_GENMASK(10, 0) Let's remove GEN12_VOLTAGE_MASK, looks like it's not being used. > + > #define GEN11_GT_INTR_DW(x) _MMIO(0x190018 + ((x) * 4)) > #define GEN11_CSME (31) > #define GEN11_GUNIT(28) > diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c > b/drivers/gpu/drm/i915/gt/intel_rps.c > index 6fadde4ee7bf..341f96f536e8 100644 > --- a/drivers/gpu/drm/i915/gt/intel_rps.c > +++ b/drivers/gpu/drm/i915/gt/intel_rps.c > @@ -2040,6 +2040,19 @@ void intel_rps_sanitize(struct intel_rps *rps) > rps_disable_interrupts(rps); > } > > +u32 intel_rps_read_rpstat(struct intel_rps *rps) > +{ > + struct drm_i915_private *i915 = rps_to_i915(rps); > + u32 rpstat; > + > + if (GRAPHICS_VER(i915) >= 12) > + rpstat = intel_uncore_read(rps_to_gt(rps)->uncore, > GEN12_RPSTAT1); > + else > + rpstat = intel_uncore_read(rps_to_gt(rps)->uncore, > GEN6_RPSTAT1); Probably nit but how about something like this: i915_reg_t rpstat; if (GRAPHICS_VER(i915) >= 12) rpstat = GEN12_RPSTAT1; else rpstat = GEN6_RPSTAT1; return intel_uncore_read(rps_to_gt(rps)->uncore, rpstat); > + > + return rpstat; > +} > + > u32 intel_rps_get_cagf(struct intel_rps *rps, u32 rpstat) > { > struct drm_i915_private *i915 = rps_to_i915(rps); > @@ -2047,6 +2060,8 @@ u32 intel_rps_get_cagf(struct intel_rps *rps, u32 > rpstat) > > if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915)) > cagf = (rpstat >> 8) & 0xff; > + else if (GRAPHICS_VER(i915) >= 12) > + cagf = (rpstat & GEN12_CAGF_MASK) >> GEN12_CAGF_SHIFT; > else if (GRAPHICS_VER(i915) >= 9) > cagf = (rpstat & GEN9_CAGF_MASK) >> GEN9_CAGF_SHIFT; > else if (IS_HASWELL(i915) || IS_BROADWELL(i915)) > @@ -2071,7 +2086,7 @@ static u32 read_cagf(struct intel_rps *rps) > freq = vlv_punit_read(i915, PUNIT_REG_GPU_FREQ_STS); > vlv_punit_put(i915); > } else if (GRAPHICS_VER(i915) >= 6) { > - freq = intel_uncore_read(uncore, GEN6_RPSTAT1); > + freq = intel_rps_read_rpstat(rps); > } else { > freq = intel_uncore_read(uncore, MEMSTAT_ILK); > } > diff --git a/drivers/gpu/drm/i915/gt/intel_rps.h > b/drivers/gpu/drm/i915/gt/intel_rps.h > index 4509dfdc52e0..08bae6d97870 100644 > --- a/drivers/gpu/drm/i915/gt/intel_rps.h > +++ b/drivers/gpu/drm/i915/gt/intel_rps.h > @@ -47,6 +47,7 @@ u32 intel_rps_get_rp1_frequency(struct intel_rps *rps); > u32 intel_rps_get_rpn_frequency(struct intel_rps *rps); > u32 intel_rps_read_punit_req(struct intel_rps *rps); > u32 intel_rps_read_punit_req_frequency(struct intel_
[Intel-gfx] [PATCH 4/6] drm/i915: Use GEN12 RPSTAT register
From: Don Hiatt On GEN12, use the correct GEN12 RPSTAT register mask/shift. HSD: 1409538411 Cc: Don Hiatt Cc: Andi Shyti Signed-off-by: Don Hiatt Signed-off-by: Badal Nilawar --- drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c | 2 +- drivers/gpu/drm/i915/gt/intel_gt_regs.h | 5 + drivers/gpu/drm/i915/gt/intel_rps.c | 17 - drivers/gpu/drm/i915/gt/intel_rps.h | 1 + drivers/gpu/drm/i915/i915_pmu.c | 3 +-- 5 files changed, 24 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c index 108b9e76c32e..96c03a1258e1 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c +++ b/drivers/gpu/drm/i915/gt/intel_gt_pm_debugfs.c @@ -380,7 +380,7 @@ void intel_gt_pm_frequency_dump(struct intel_gt *gt, struct drm_printer *p) rpinclimit = intel_uncore_read(uncore, GEN6_RP_UP_THRESHOLD); rpdeclimit = intel_uncore_read(uncore, GEN6_RP_DOWN_THRESHOLD); - rpstat = intel_uncore_read(uncore, GEN6_RPSTAT1); + rpstat = intel_rps_read_rpstat(rps); rpcurupei = intel_uncore_read(uncore, GEN6_RP_CUR_UP_EI) & GEN6_CURICONT_MASK; rpcurup = intel_uncore_read(uncore, GEN6_RP_CUR_UP) & GEN6_CURBSYTAVG_MASK; rpprevup = intel_uncore_read(uncore, GEN6_RP_PREV_UP) & GEN6_CURBSYTAVG_MASK; diff --git a/drivers/gpu/drm/i915/gt/intel_gt_regs.h b/drivers/gpu/drm/i915/gt/intel_gt_regs.h index fb2c56777480..dac59c3e68db 100644 --- a/drivers/gpu/drm/i915/gt/intel_gt_regs.h +++ b/drivers/gpu/drm/i915/gt/intel_gt_regs.h @@ -1510,6 +1510,11 @@ #define VLV_RENDER_C0_COUNT_MMIO(0x138118) #define VLV_MEDIA_C0_COUNT _MMIO(0x13811c) +#define GEN12_RPSTAT1 _MMIO(0x1381b4) +#define GEN12_CAGF_SHIFT 11 +#define GEN12_CAGF_MASK REG_GENMASK(19, 11) +#define GEN12_VOLTAGE_MASK REG_GENMASK(10, 0) + #define GEN11_GT_INTR_DW(x)_MMIO(0x190018 + ((x) * 4)) #define GEN11_CSME (31) #define GEN11_GUNIT (28) diff --git a/drivers/gpu/drm/i915/gt/intel_rps.c b/drivers/gpu/drm/i915/gt/intel_rps.c index 6fadde4ee7bf..341f96f536e8 100644 --- a/drivers/gpu/drm/i915/gt/intel_rps.c +++ b/drivers/gpu/drm/i915/gt/intel_rps.c @@ -2040,6 +2040,19 @@ void intel_rps_sanitize(struct intel_rps *rps) rps_disable_interrupts(rps); } +u32 intel_rps_read_rpstat(struct intel_rps *rps) +{ + struct drm_i915_private *i915 = rps_to_i915(rps); + u32 rpstat; + + if (GRAPHICS_VER(i915) >= 12) + rpstat = intel_uncore_read(rps_to_gt(rps)->uncore, GEN12_RPSTAT1); + else + rpstat = intel_uncore_read(rps_to_gt(rps)->uncore, GEN6_RPSTAT1); + + return rpstat; +} + u32 intel_rps_get_cagf(struct intel_rps *rps, u32 rpstat) { struct drm_i915_private *i915 = rps_to_i915(rps); @@ -2047,6 +2060,8 @@ u32 intel_rps_get_cagf(struct intel_rps *rps, u32 rpstat) if (IS_VALLEYVIEW(i915) || IS_CHERRYVIEW(i915)) cagf = (rpstat >> 8) & 0xff; + else if (GRAPHICS_VER(i915) >= 12) + cagf = (rpstat & GEN12_CAGF_MASK) >> GEN12_CAGF_SHIFT; else if (GRAPHICS_VER(i915) >= 9) cagf = (rpstat & GEN9_CAGF_MASK) >> GEN9_CAGF_SHIFT; else if (IS_HASWELL(i915) || IS_BROADWELL(i915)) @@ -2071,7 +2086,7 @@ static u32 read_cagf(struct intel_rps *rps) freq = vlv_punit_read(i915, PUNIT_REG_GPU_FREQ_STS); vlv_punit_put(i915); } else if (GRAPHICS_VER(i915) >= 6) { - freq = intel_uncore_read(uncore, GEN6_RPSTAT1); + freq = intel_rps_read_rpstat(rps); } else { freq = intel_uncore_read(uncore, MEMSTAT_ILK); } diff --git a/drivers/gpu/drm/i915/gt/intel_rps.h b/drivers/gpu/drm/i915/gt/intel_rps.h index 4509dfdc52e0..08bae6d97870 100644 --- a/drivers/gpu/drm/i915/gt/intel_rps.h +++ b/drivers/gpu/drm/i915/gt/intel_rps.h @@ -47,6 +47,7 @@ u32 intel_rps_get_rp1_frequency(struct intel_rps *rps); u32 intel_rps_get_rpn_frequency(struct intel_rps *rps); u32 intel_rps_read_punit_req(struct intel_rps *rps); u32 intel_rps_read_punit_req_frequency(struct intel_rps *rps); +u32 intel_rps_read_rpstat(struct intel_rps *rps); void gen6_rps_get_freq_caps(struct intel_rps *rps, struct intel_rps_freq_caps *caps); void intel_rps_raise_unslice(struct intel_rps *rps); void intel_rps_lower_unslice(struct intel_rps *rps); diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c index 958b37123bf1..a24704ec2c18 100644 --- a/drivers/gpu/drm/i915/i915_pmu.c +++ b/drivers/gpu/drm/i915/i915_pmu.c @@ -371,7 +371,6 @@ static void frequency_sample(struct intel_gt *gt, unsigned int period_ns) { struct drm