Re: [Intel-gfx] [PATCH 1/2] drm/i915/pmu: Turn off the timer to sample frequencies when GT is parked
On 24/05/2023 22:46, Dixit, Ashutosh wrote: On Wed, 24 May 2023 02:12:31 -0700, Andrzej Hajda wrote: Hi Andrzej, On 23.05.2023 17:19, Ashutosh Dixit wrote: pmu_needs_timer() keeps the timer running even when GT is parked, ostensibly to sample requested/actual frequencies. However frequency_sample() has the following: /* Report 0/0 (actual/requested) frequency while parked. */ if (!intel_gt_pm_get_if_awake(gt)) return; The above code prevents frequencies to be sampled while the GT is parked. So we might as well turn off the sampling timer itself in this case and save CPU cycles/power. v2: Instead of turning freq bits off, return false, since no counters will run after this change when GT is parked (Tvrtko) Signed-off-by: Ashutosh Dixit Reviewed-by: Tvrtko Ursulin --- drivers/gpu/drm/i915/i915_pmu.c | 12 +--- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c index a814583e19fd7..b47d890d4ada1 100644 --- a/drivers/gpu/drm/i915/i915_pmu.c +++ b/drivers/gpu/drm/i915/i915_pmu.c @@ -144,6 +144,10 @@ static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active) struct drm_i915_private *i915 = container_of(pmu, typeof(*i915), pmu); u32 enable; +/* When GPU is idle, at present no counters need to run */ + if (!gpu_active) + return false; + What is then purpose of calling pmu_needs_timer with 2nd arg false? Why not just replace all occurrences of pmu_needs_timer(.., false) with false? And remove the 2nd argument. OK, this didn't seem unreasonable so I went ahead and made this change in Patch v3. Copying Tvrtko too in case he prefers v2 for any reason. Please review. It is all fine by me (the latest version and all)! Regards, Tvrtko
Re: [PATCH 1/2] drm/i915/pmu: Turn off the timer to sample frequencies when GT is parked
On 24.05.2023 23:56, Ashutosh Dixit wrote:
pmu_needs_timer() keeps the timer running even when GT is parked,
ostensibly to sample requested/actual frequencies. However
frequency_sample() has the following:
/* Report 0/0 (actual/requested) frequency while parked. */
if (!intel_gt_pm_get_if_awake(gt))
return;
The above code prevents frequencies to be sampled while the GT is
parked. So we might as well turn off the sampling timer itself in this
case and save CPU cycles/power.
v2: Instead of turning freq bits off, return false, since no counters will
run after this change when GT is parked (Tvrtko)
v3: Remove gpu_active argument of pmu_needs_timer (Andrzej)
Signed-off-by: Ashutosh Dixit
Reviewed-by: Tvrtko Ursulin
Reviewed-by: Andrzej Hajda
Regards
Andrzej
---
drivers/gpu/drm/i915/i915_pmu.c | 16 +---
1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index a814583e19fd7..09313cf9316b4 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -139,7 +139,7 @@ static u32 frequency_enabled_mask(void)
return mask;
}
-static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active)
+static bool pmu_needs_timer(struct i915_pmu *pmu)
{
struct drm_i915_private *i915 = container_of(pmu, typeof(*i915), pmu);
u32 enable;
@@ -157,17 +157,11 @@ static bool pmu_needs_timer(struct i915_pmu *pmu, bool
gpu_active)
*/
enable &= frequency_enabled_mask() | ENGINE_SAMPLE_MASK;
- /*
-* When the GPU is idle per-engine counters do not need to be
-* running so clear those bits out.
-*/
- if (!gpu_active)
- enable &= ~ENGINE_SAMPLE_MASK;
/*
* Also there is software busyness tracking available we do not
* need the timer for I915_SAMPLE_BUSY counter.
*/
- else if (i915->caps.scheduler & I915_SCHEDULER_CAP_ENGINE_BUSY_STATS)
+ if (i915->caps.scheduler & I915_SCHEDULER_CAP_ENGINE_BUSY_STATS)
enable &= ~BIT(I915_SAMPLE_BUSY);
/*
@@ -295,7 +289,7 @@ static void park_rc6(struct intel_gt *gt)
static void __i915_pmu_maybe_start_timer(struct i915_pmu *pmu)
{
- if (!pmu->timer_enabled && pmu_needs_timer(pmu, true)) {
+ if (!pmu->timer_enabled && pmu_needs_timer(pmu)) {
pmu->timer_enabled = true;
pmu->timer_last = ktime_get();
hrtimer_start_range_ns(>timer,
@@ -321,7 +315,7 @@ void i915_pmu_gt_parked(struct intel_gt *gt)
*/
pmu->unparked &= ~BIT(gt->info.id);
if (pmu->unparked == 0)
- pmu->timer_enabled = pmu_needs_timer(pmu, false);
+ pmu->timer_enabled = false;
spin_unlock_irq(>lock);
}
@@ -827,7 +821,7 @@ static void i915_pmu_disable(struct perf_event *event)
*/
if (--pmu->enable_count[bit] == 0) {
pmu->enable &= ~BIT(bit);
- pmu->timer_enabled &= pmu_needs_timer(pmu, true);
+ pmu->timer_enabled &= pmu_needs_timer(pmu);
}
spin_unlock_irqrestore(>lock, flags);
Re: [Intel-gfx] [PATCH 1/2] drm/i915/pmu: Turn off the timer to sample frequencies when GT is parked
On Wed, 24 May 2023 02:12:31 -0700, Andrzej Hajda wrote: > Hi Andrzej, > On 23.05.2023 17:19, Ashutosh Dixit wrote: > > pmu_needs_timer() keeps the timer running even when GT is parked, > > ostensibly to sample requested/actual frequencies. However > > frequency_sample() has the following: > > > > /* Report 0/0 (actual/requested) frequency while parked. */ > > if (!intel_gt_pm_get_if_awake(gt)) > > return; > > > > The above code prevents frequencies to be sampled while the GT is > > parked. So we might as well turn off the sampling timer itself in this > > case and save CPU cycles/power. > > > > v2: Instead of turning freq bits off, return false, since no counters will > > run after this change when GT is parked (Tvrtko) > > > > Signed-off-by: Ashutosh Dixit > > Reviewed-by: Tvrtko Ursulin > > --- > > drivers/gpu/drm/i915/i915_pmu.c | 12 +--- > > 1 file changed, 5 insertions(+), 7 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_pmu.c > > b/drivers/gpu/drm/i915/i915_pmu.c > > index a814583e19fd7..b47d890d4ada1 100644 > > --- a/drivers/gpu/drm/i915/i915_pmu.c > > +++ b/drivers/gpu/drm/i915/i915_pmu.c > > @@ -144,6 +144,10 @@ static bool pmu_needs_timer(struct i915_pmu *pmu, bool > > gpu_active) > > struct drm_i915_private *i915 = container_of(pmu, typeof(*i915), pmu); > > u32 enable; > > + /* When GPU is idle, at present no counters need to run */ > > + if (!gpu_active) > > + return false; > > + > > What is then purpose of calling pmu_needs_timer with 2nd arg false? > Why not just replace all occurrences of pmu_needs_timer(.., false) with > false? And remove the 2nd argument. OK, this didn't seem unreasonable so I went ahead and made this change in Patch v3. Copying Tvrtko too in case he prefers v2 for any reason. Please review. Thanks. -- Ashutosh > > > > > /* > > * Only some counters need the sampling timer. > > * > > @@ -157,17 +161,11 @@ static bool pmu_needs_timer(struct i915_pmu *pmu, > > bool gpu_active) > > */ > > enable &= frequency_enabled_mask() | ENGINE_SAMPLE_MASK; > > - /* > > -* When the GPU is idle per-engine counters do not need to be > > -* running so clear those bits out. > > -*/ > > - if (!gpu_active) > > - enable &= ~ENGINE_SAMPLE_MASK; > > /* > > * Also there is software busyness tracking available we do not > > * need the timer for I915_SAMPLE_BUSY counter. > > */ > > - else if (i915->caps.scheduler & I915_SCHEDULER_CAP_ENGINE_BUSY_STATS) > > + if (i915->caps.scheduler & I915_SCHEDULER_CAP_ENGINE_BUSY_STATS) > > enable &= ~BIT(I915_SAMPLE_BUSY); > > /* >
[PATCH 1/2] drm/i915/pmu: Turn off the timer to sample frequencies when GT is parked
pmu_needs_timer() keeps the timer running even when GT is parked,
ostensibly to sample requested/actual frequencies. However
frequency_sample() has the following:
/* Report 0/0 (actual/requested) frequency while parked. */
if (!intel_gt_pm_get_if_awake(gt))
return;
The above code prevents frequencies to be sampled while the GT is
parked. So we might as well turn off the sampling timer itself in this
case and save CPU cycles/power.
v2: Instead of turning freq bits off, return false, since no counters will
run after this change when GT is parked (Tvrtko)
v3: Remove gpu_active argument of pmu_needs_timer (Andrzej)
Signed-off-by: Ashutosh Dixit
Reviewed-by: Tvrtko Ursulin
---
drivers/gpu/drm/i915/i915_pmu.c | 16 +---
1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index a814583e19fd7..09313cf9316b4 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -139,7 +139,7 @@ static u32 frequency_enabled_mask(void)
return mask;
}
-static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active)
+static bool pmu_needs_timer(struct i915_pmu *pmu)
{
struct drm_i915_private *i915 = container_of(pmu, typeof(*i915), pmu);
u32 enable;
@@ -157,17 +157,11 @@ static bool pmu_needs_timer(struct i915_pmu *pmu, bool
gpu_active)
*/
enable &= frequency_enabled_mask() | ENGINE_SAMPLE_MASK;
- /*
-* When the GPU is idle per-engine counters do not need to be
-* running so clear those bits out.
-*/
- if (!gpu_active)
- enable &= ~ENGINE_SAMPLE_MASK;
/*
* Also there is software busyness tracking available we do not
* need the timer for I915_SAMPLE_BUSY counter.
*/
- else if (i915->caps.scheduler & I915_SCHEDULER_CAP_ENGINE_BUSY_STATS)
+ if (i915->caps.scheduler & I915_SCHEDULER_CAP_ENGINE_BUSY_STATS)
enable &= ~BIT(I915_SAMPLE_BUSY);
/*
@@ -295,7 +289,7 @@ static void park_rc6(struct intel_gt *gt)
static void __i915_pmu_maybe_start_timer(struct i915_pmu *pmu)
{
- if (!pmu->timer_enabled && pmu_needs_timer(pmu, true)) {
+ if (!pmu->timer_enabled && pmu_needs_timer(pmu)) {
pmu->timer_enabled = true;
pmu->timer_last = ktime_get();
hrtimer_start_range_ns(>timer,
@@ -321,7 +315,7 @@ void i915_pmu_gt_parked(struct intel_gt *gt)
*/
pmu->unparked &= ~BIT(gt->info.id);
if (pmu->unparked == 0)
- pmu->timer_enabled = pmu_needs_timer(pmu, false);
+ pmu->timer_enabled = false;
spin_unlock_irq(>lock);
}
@@ -827,7 +821,7 @@ static void i915_pmu_disable(struct perf_event *event)
*/
if (--pmu->enable_count[bit] == 0) {
pmu->enable &= ~BIT(bit);
- pmu->timer_enabled &= pmu_needs_timer(pmu, true);
+ pmu->timer_enabled &= pmu_needs_timer(pmu);
}
spin_unlock_irqrestore(>lock, flags);
--
2.38.0
Re: [Intel-gfx] [PATCH 1/2] drm/i915/pmu: Turn off the timer to sample frequencies when GT is parked
On 23.05.2023 17:19, Ashutosh Dixit wrote: pmu_needs_timer() keeps the timer running even when GT is parked, ostensibly to sample requested/actual frequencies. However frequency_sample() has the following: /* Report 0/0 (actual/requested) frequency while parked. */ if (!intel_gt_pm_get_if_awake(gt)) return; The above code prevents frequencies to be sampled while the GT is parked. So we might as well turn off the sampling timer itself in this case and save CPU cycles/power. v2: Instead of turning freq bits off, return false, since no counters will run after this change when GT is parked (Tvrtko) Signed-off-by: Ashutosh Dixit Reviewed-by: Tvrtko Ursulin --- drivers/gpu/drm/i915/i915_pmu.c | 12 +--- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c index a814583e19fd7..b47d890d4ada1 100644 --- a/drivers/gpu/drm/i915/i915_pmu.c +++ b/drivers/gpu/drm/i915/i915_pmu.c @@ -144,6 +144,10 @@ static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active) struct drm_i915_private *i915 = container_of(pmu, typeof(*i915), pmu); u32 enable; + /* When GPU is idle, at present no counters need to run */ + if (!gpu_active) + return false; + What is then purpose of calling pmu_needs_timer with 2nd arg false? Why not just replace all occurrences of pmu_needs_timer(.., false) with false? And remove the 2nd argument. Regards Andrzej /* * Only some counters need the sampling timer. * @@ -157,17 +161,11 @@ static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active) */ enable &= frequency_enabled_mask() | ENGINE_SAMPLE_MASK; - /* -* When the GPU is idle per-engine counters do not need to be -* running so clear those bits out. -*/ - if (!gpu_active) - enable &= ~ENGINE_SAMPLE_MASK; /* * Also there is software busyness tracking available we do not * need the timer for I915_SAMPLE_BUSY counter. */ - else if (i915->caps.scheduler & I915_SCHEDULER_CAP_ENGINE_BUSY_STATS) + if (i915->caps.scheduler & I915_SCHEDULER_CAP_ENGINE_BUSY_STATS) enable &= ~BIT(I915_SAMPLE_BUSY); /*
[PATCH 1/2] drm/i915/pmu: Turn off the timer to sample frequencies when GT is parked
pmu_needs_timer() keeps the timer running even when GT is parked, ostensibly to sample requested/actual frequencies. However frequency_sample() has the following: /* Report 0/0 (actual/requested) frequency while parked. */ if (!intel_gt_pm_get_if_awake(gt)) return; The above code prevents frequencies to be sampled while the GT is parked. So we might as well turn off the sampling timer itself in this case and save CPU cycles/power. v2: Instead of turning freq bits off, return false, since no counters will run after this change when GT is parked (Tvrtko) Signed-off-by: Ashutosh Dixit Reviewed-by: Tvrtko Ursulin --- drivers/gpu/drm/i915/i915_pmu.c | 12 +--- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c index a814583e19fd7..b47d890d4ada1 100644 --- a/drivers/gpu/drm/i915/i915_pmu.c +++ b/drivers/gpu/drm/i915/i915_pmu.c @@ -144,6 +144,10 @@ static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active) struct drm_i915_private *i915 = container_of(pmu, typeof(*i915), pmu); u32 enable; + /* When GPU is idle, at present no counters need to run */ + if (!gpu_active) + return false; + /* * Only some counters need the sampling timer. * @@ -157,17 +161,11 @@ static bool pmu_needs_timer(struct i915_pmu *pmu, bool gpu_active) */ enable &= frequency_enabled_mask() | ENGINE_SAMPLE_MASK; - /* -* When the GPU is idle per-engine counters do not need to be -* running so clear those bits out. -*/ - if (!gpu_active) - enable &= ~ENGINE_SAMPLE_MASK; /* * Also there is software busyness tracking available we do not * need the timer for I915_SAMPLE_BUSY counter. */ - else if (i915->caps.scheduler & I915_SCHEDULER_CAP_ENGINE_BUSY_STATS) + if (i915->caps.scheduler & I915_SCHEDULER_CAP_ENGINE_BUSY_STATS) enable &= ~BIT(I915_SAMPLE_BUSY); /* -- 2.38.0
