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
