* [PATCH] drm/i915/pmu: Fix sleep under atomic in RC6 readout
@ 2018-02-06 14:31 Tvrtko Ursulin
2018-02-06 15:13 ` ✗ Fi.CI.BAT: warning for " Patchwork
` (4 more replies)
0 siblings, 5 replies; 15+ messages in thread
From: Tvrtko Ursulin @ 2018-02-06 14:31 UTC (permalink / raw)
To: Intel-gfx; +Cc: David Airlie, dri-devel, Rodrigo Vivi, intel-gfx
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
We are not allowed to call intel_runtime_pm_get from the PMU counter read
callback since the former can sleep, and the latter is running under IRQ
context.
To workaround this, we start our timer when we detect that we have failed
to obtain a runtime PM reference during read, and approximate the growing
RC6 counter from the timer.
Once the timer manages to obtain the runtime PM reference, we stop the
timer and go back to the above described behaviour.
We have to be careful not to overshoot the RC6 estimate, so once resumed
after a period of approximation, we only update the counter once it
catches up. With the observation that RC6 is increasing while the device
is suspended, this should not pose a problem and can only cause
slight inaccuracies due clock base differences.
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104943
Fixes: 6060b6aec03c ("drm/i915/pmu: Add RC6 residency metrics")
Testcase: igt/perf_pmu/rc6-runtime-pm
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: David Airlie <airlied@linux.ie>
Cc: intel-gfx@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
---
drivers/gpu/drm/i915/i915_pmu.c | 149 ++++++++++++++++++++++++++++++----------
drivers/gpu/drm/i915/i915_pmu.h | 1 +
2 files changed, 114 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 1c440460255d..dca41c072a7c 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -90,23 +90,16 @@ static unsigned int event_enabled_bit(struct perf_event *event)
return config_enabled_bit(event->attr.config);
}
-static bool pmu_needs_timer(struct drm_i915_private *i915, bool gpu_active)
+static bool
+__pmu_needs_timer(struct drm_i915_private *i915, u64 enable, bool gpu_active)
{
- u64 enable;
-
- /*
- * Only some counters need the sampling timer.
- *
- * We start with a bitmask of all currently enabled events.
- */
- enable = i915->pmu.enable;
-
/*
- * Mask out all the ones which do not need the timer, or in
+ * Mask out all events which do not need the timer, or in
* other words keep all the ones that could need the timer.
*/
enable &= config_enabled_mask(I915_PMU_ACTUAL_FREQUENCY) |
config_enabled_mask(I915_PMU_REQUESTED_FREQUENCY) |
+ config_enabled_mask(I915_PMU_RC6_RESIDENCY) |
ENGINE_SAMPLE_MASK;
/*
@@ -130,6 +123,11 @@ static bool pmu_needs_timer(struct drm_i915_private *i915, bool gpu_active)
return enable;
}
+static bool pmu_needs_timer(struct drm_i915_private *i915, bool gpu_active)
+{
+ return __pmu_needs_timer(i915, i915->pmu.enable, gpu_active);
+}
+
void i915_pmu_gt_parked(struct drm_i915_private *i915)
{
if (!i915->pmu.base.event_init)
@@ -181,20 +179,20 @@ update_sample(struct i915_pmu_sample *sample, u32 unit, u32 val)
sample->cur += mul_u32_u32(val, unit);
}
-static void engines_sample(struct drm_i915_private *dev_priv)
+static bool engines_sample(struct drm_i915_private *dev_priv)
{
struct intel_engine_cs *engine;
enum intel_engine_id id;
bool fw = false;
if ((dev_priv->pmu.enable & ENGINE_SAMPLE_MASK) == 0)
- return;
+ return false;
if (!dev_priv->gt.awake)
- return;
+ return false;
if (!intel_runtime_pm_get_if_in_use(dev_priv))
- return;
+ return false;
for_each_engine(engine, dev_priv, id) {
u32 current_seqno = intel_engine_get_seqno(engine);
@@ -225,10 +223,51 @@ static void engines_sample(struct drm_i915_private *dev_priv)
if (fw)
intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
- intel_runtime_pm_put(dev_priv);
+ return true;
+}
+
+static u64 read_rc6_residency(struct drm_i915_private *i915)
+{
+ u64 val;
+
+ val = intel_rc6_residency_ns(i915, IS_VALLEYVIEW(i915) ?
+ VLV_GT_RENDER_RC6 : GEN6_GT_GFX_RC6);
+ if (HAS_RC6p(i915))
+ val += intel_rc6_residency_ns(i915, GEN6_GT_GFX_RC6p);
+ if (HAS_RC6pp(i915))
+ val += intel_rc6_residency_ns(i915, GEN6_GT_GFX_RC6pp);
+
+ return val;
+}
+
+static void
+update_rc6_sample(struct drm_i915_private *i915, u64 val, bool locked)
+{
+ unsigned long flags;
+
+ if (!locked)
+ spin_lock_irqsave(&i915->pmu.lock, flags);
+
+ /*
+ * Update stored RC6 counter only if it is greater than the current
+ * value. This deals with periods of runtime suspend during which we are
+ * estimating the RC6 residency, so do not want to overshoot the real
+ * value read once the device is woken up.
+ */
+ if (val > i915->pmu.sample[__I915_SAMPLE_RC6].cur)
+ i915->pmu.sample[__I915_SAMPLE_RC6].cur = val;
+
+ /* We don't need to sample RC6 from the timer any more. */
+ i915->pmu.timer_enabled =
+ __pmu_needs_timer(i915,
+ i915->pmu.enable & ~config_enabled_mask(I915_PMU_RC6_RESIDENCY),
+ READ_ONCE(i915->gt.awake));
+
+ if (!locked)
+ spin_unlock_irqrestore(&i915->pmu.lock, flags);
}
-static void frequency_sample(struct drm_i915_private *dev_priv)
+static bool others_sample(struct drm_i915_private *dev_priv, bool pm)
{
if (dev_priv->pmu.enable &
config_enabled_mask(I915_PMU_ACTUAL_FREQUENCY)) {
@@ -236,10 +275,10 @@ static void frequency_sample(struct drm_i915_private *dev_priv)
val = dev_priv->gt_pm.rps.cur_freq;
if (dev_priv->gt.awake &&
- intel_runtime_pm_get_if_in_use(dev_priv)) {
+ (pm || intel_runtime_pm_get_if_in_use(dev_priv))) {
+ pm = true;
val = intel_get_cagf(dev_priv,
I915_READ_NOTRACE(GEN6_RPSTAT1));
- intel_runtime_pm_put(dev_priv);
}
update_sample(&dev_priv->pmu.sample[__I915_SAMPLE_FREQ_ACT],
@@ -252,18 +291,48 @@ static void frequency_sample(struct drm_i915_private *dev_priv)
intel_gpu_freq(dev_priv,
dev_priv->gt_pm.rps.cur_freq));
}
+
+ if (dev_priv->pmu.enable &
+ config_enabled_mask(I915_PMU_RC6_RESIDENCY)) {
+ if (pm || intel_runtime_pm_get_if_in_use(dev_priv)) {
+ update_rc6_sample(dev_priv,
+ read_rc6_residency(dev_priv),
+ false);
+ pm = true;
+ } else {
+ unsigned long flags;
+
+ /*
+ * When device is runtime suspended we assume RC6
+ * residency is increasing by the sampling timer period.
+ */
+ spin_lock_irqsave(&dev_priv->pmu.lock, flags);
+ dev_priv->pmu.sample[__I915_SAMPLE_RC6].cur += PERIOD;
+ spin_unlock_irqrestore(&dev_priv->pmu.lock, flags);
+ }
+ }
+
+ return pm;
+
}
static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer)
{
struct drm_i915_private *i915 =
container_of(hrtimer, struct drm_i915_private, pmu.timer);
+ bool pm;
if (!READ_ONCE(i915->pmu.timer_enabled))
return HRTIMER_NORESTART;
- engines_sample(i915);
- frequency_sample(i915);
+ pm = engines_sample(i915);
+ pm = others_sample(i915, pm);
+
+ if (pm)
+ intel_runtime_pm_put(i915);
+
+ if (!READ_ONCE(i915->pmu.timer_enabled))
+ return HRTIMER_NORESTART;
hrtimer_forward_now(hrtimer, ns_to_ktime(PERIOD));
return HRTIMER_RESTART;
@@ -415,7 +484,7 @@ static int i915_pmu_event_init(struct perf_event *event)
return 0;
}
-static u64 __i915_pmu_event_read(struct perf_event *event)
+static u64 __i915_pmu_event_read(struct perf_event *event, bool locked)
{
struct drm_i915_private *i915 =
container_of(event->pmu, typeof(*i915), pmu.base);
@@ -453,18 +522,26 @@ static u64 __i915_pmu_event_read(struct perf_event *event)
val = count_interrupts(i915);
break;
case I915_PMU_RC6_RESIDENCY:
- intel_runtime_pm_get(i915);
- val = intel_rc6_residency_ns(i915,
- IS_VALLEYVIEW(i915) ?
- VLV_GT_RENDER_RC6 :
- GEN6_GT_GFX_RC6);
- if (HAS_RC6p(i915))
- val += intel_rc6_residency_ns(i915,
- GEN6_GT_GFX_RC6p);
- if (HAS_RC6pp(i915))
- val += intel_rc6_residency_ns(i915,
- GEN6_GT_GFX_RC6pp);
- intel_runtime_pm_put(i915);
+ if (intel_runtime_pm_get_if_in_use(i915)) {
+ update_rc6_sample(i915,
+ read_rc6_residency(i915),
+ locked);
+ intel_runtime_pm_put(i915);
+ } else {
+ unsigned long flags;
+
+ /*
+ * If we failed to read the actual value, start
+ * the timer which will be estimating it while
+ * device is suspended.
+ */
+ if (!locked)
+ spin_lock_irqsave(&i915->pmu.lock, flags);
+ __i915_pmu_maybe_start_timer(i915);
+ if (!locked)
+ spin_unlock_irqrestore(&i915->pmu.lock, flags);
+ }
+ val = i915->pmu.sample[__I915_SAMPLE_RC6].cur;
break;
}
}
@@ -479,7 +556,7 @@ static void i915_pmu_event_read(struct perf_event *event)
again:
prev = local64_read(&hwc->prev_count);
- new = __i915_pmu_event_read(event);
+ new = __i915_pmu_event_read(event, false);
if (local64_cmpxchg(&hwc->prev_count, prev, new) != prev)
goto again;
@@ -534,7 +611,7 @@ static void i915_pmu_enable(struct perf_event *event)
* for all listeners. Even when the event was already enabled and has
* an existing non-zero value.
*/
- local64_set(&event->hw.prev_count, __i915_pmu_event_read(event));
+ local64_set(&event->hw.prev_count, __i915_pmu_event_read(event, true));
spin_unlock_irqrestore(&i915->pmu.lock, flags);
}
diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
index 5a2e013a56bb..249983ed3f08 100644
--- a/drivers/gpu/drm/i915/i915_pmu.h
+++ b/drivers/gpu/drm/i915/i915_pmu.h
@@ -27,6 +27,7 @@
enum {
__I915_SAMPLE_FREQ_ACT = 0,
__I915_SAMPLE_FREQ_REQ,
+ __I915_SAMPLE_RC6,
__I915_NUM_PMU_SAMPLERS
};
--
2.14.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 15+ messages in thread
* ✗ Fi.CI.BAT: warning for drm/i915/pmu: Fix sleep under atomic in RC6 readout
2018-02-06 14:31 [PATCH] drm/i915/pmu: Fix sleep under atomic in RC6 readout Tvrtko Ursulin
@ 2018-02-06 15:13 ` Patchwork
2018-02-06 16:04 ` [PATCH] " Chris Wilson
` (3 subsequent siblings)
4 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2018-02-06 15:13 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/pmu: Fix sleep under atomic in RC6 readout
URL : https://patchwork.freedesktop.org/series/37742/
State : warning
== Summary ==
Series 37742v1 drm/i915/pmu: Fix sleep under atomic in RC6 readout
https://patchwork.freedesktop.org/api/1.0/series/37742/revisions/1/mbox/
Test gem_ctx_create:
Subgroup basic-files:
pass -> DMESG-WARN (fi-skl-6700k2)
Test kms_pipe_crc_basic:
Subgroup suspend-read-crc-pipe-b:
pass -> INCOMPLETE (fi-snb-2520m) fdo#103713
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:415s
fi-bdw-gvtdvm total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:422s
fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:377s
fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:486s
fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:483s
fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:482s
fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:466s
fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:455s
fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:563s
fi-cnl-y3 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:572s
fi-elk-e7500 total:288 pass:229 dwarn:0 dfail:0 fail:0 skip:59 time:415s
fi-gdg-551 total:288 pass:179 dwarn:0 dfail:0 fail:1 skip:108 time:283s
fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:508s
fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:389s
fi-hsw-4770r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:402s
fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:409s
fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:459s
fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:414s
fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:455s
fi-kbl-7560u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:496s
fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:454s
fi-kbl-r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:496s
fi-pnv-d510 total:288 pass:222 dwarn:1 dfail:0 fail:0 skip:65 time:592s
fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:427s
fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:508s
fi-skl-6700hq total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:536s
fi-skl-6700k2 total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:474s
fi-skl-6770hq total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:502s
fi-skl-guc total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:413s
fi-skl-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:430s
fi-snb-2520m total:245 pass:211 dwarn:0 dfail:0 fail:0 skip:33
fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:397s
Blacklisted hosts:
fi-glk-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:470s
078873da383505cf8d6940229007115b31f1d5e0 drm-tip: 2018y-02m-06d-11h-21m-36s UTC integration manifest
a20dd79efd67 drm/i915/pmu: Fix sleep under atomic in RC6 readout
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7908/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915/pmu: Fix sleep under atomic in RC6 readout
2018-02-06 14:31 [PATCH] drm/i915/pmu: Fix sleep under atomic in RC6 readout Tvrtko Ursulin
2018-02-06 15:13 ` ✗ Fi.CI.BAT: warning for " Patchwork
@ 2018-02-06 16:04 ` Chris Wilson
2018-02-06 16:10 ` Imre Deak
2018-02-06 19:16 ` ✓ Fi.CI.BAT: success for drm/i915/pmu: Fix sleep under atomic in RC6 readout (rev2) Patchwork
` (2 subsequent siblings)
4 siblings, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2018-02-06 16:04 UTC (permalink / raw)
To: Intel-gfx
Cc: Tvrtko Ursulin, David Airlie, tursulin, dri-devel, Rodrigo Vivi,
intel-gfx
Quoting Tvrtko Ursulin (2018-02-06 14:31:07)
> +static u64 read_rc6_residency(struct drm_i915_private *i915)
> +{
> + u64 val;
> +
> + val = intel_rc6_residency_ns(i915, IS_VALLEYVIEW(i915) ?
> + VLV_GT_RENDER_RC6 : GEN6_GT_GFX_RC6);
> + if (HAS_RC6p(i915))
> + val += intel_rc6_residency_ns(i915, GEN6_GT_GFX_RC6p);
> + if (HAS_RC6pp(i915))
> + val += intel_rc6_residency_ns(i915, GEN6_GT_GFX_RC6pp);
We really should mention that these may produce interesting results
every 53 minutes. Switching to a timer will allow us to notice the
wraparound in each counter.
> +
> + return val;
> +}
> +
> +static void
> +update_rc6_sample(struct drm_i915_private *i915, u64 val, bool locked)
> +{
> + unsigned long flags;
> +
> + if (!locked)
> + spin_lock_irqsave(&i915->pmu.lock, flags);
> +
> + /*
> + * Update stored RC6 counter only if it is greater than the current
> + * value. This deals with periods of runtime suspend during which we are
> + * estimating the RC6 residency, so do not want to overshoot the real
> + * value read once the device is woken up.
> + */
> + if (val > i915->pmu.sample[__I915_SAMPLE_RC6].cur)
> + i915->pmu.sample[__I915_SAMPLE_RC6].cur = val;
64b wraparound? Maybe not today, maybe not tomorrow... ;)
> +
> + /* We don't need to sample RC6 from the timer any more. */
> + i915->pmu.timer_enabled =
> + __pmu_needs_timer(i915,
> + i915->pmu.enable & ~config_enabled_mask(I915_PMU_RC6_RESIDENCY),
> + READ_ONCE(i915->gt.awake));
But we do... :)
One thing I had in mind was to hook into runtime suspend/resume to read
the counters there and compensate, but the more I think about it, we may
as well solve the lack of resolution in the rc6 counters whilst we are
here. https://bugs.freedesktop.org/show_bug.cgi?id=94852.
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] drm/i915/pmu: Fix sleep under atomic in RC6 readout
2018-02-06 16:04 ` [PATCH] " Chris Wilson
@ 2018-02-06 16:10 ` Imre Deak
2018-02-06 17:12 ` [Intel-gfx] " Tvrtko Ursulin
2018-02-06 18:33 ` [PATCH v2] " Tvrtko Ursulin
0 siblings, 2 replies; 15+ messages in thread
From: Imre Deak @ 2018-02-06 16:10 UTC (permalink / raw)
To: Chris Wilson; +Cc: David Airlie, Intel-gfx, dri-devel, Rodrigo Vivi
On Tue, Feb 06, 2018 at 04:04:10PM +0000, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-02-06 14:31:07)
> > +static u64 read_rc6_residency(struct drm_i915_private *i915)
> > +{
> > + u64 val;
> > +
> > + val = intel_rc6_residency_ns(i915, IS_VALLEYVIEW(i915) ?
> > + VLV_GT_RENDER_RC6 : GEN6_GT_GFX_RC6);
> > + if (HAS_RC6p(i915))
> > + val += intel_rc6_residency_ns(i915, GEN6_GT_GFX_RC6p);
> > + if (HAS_RC6pp(i915))
> > + val += intel_rc6_residency_ns(i915, GEN6_GT_GFX_RC6pp);
>
> We really should mention that these may produce interesting results
> every 53 minutes. Switching to a timer will allow us to notice the
> wraparound in each counter.
>
> > +
> > + return val;
> > +}
> > +
> > +static void
> > +update_rc6_sample(struct drm_i915_private *i915, u64 val, bool locked)
> > +{
> > + unsigned long flags;
> > +
> > + if (!locked)
> > + spin_lock_irqsave(&i915->pmu.lock, flags);
> > +
> > + /*
> > + * Update stored RC6 counter only if it is greater than the current
> > + * value. This deals with periods of runtime suspend during which we are
> > + * estimating the RC6 residency, so do not want to overshoot the real
> > + * value read once the device is woken up.
> > + */
> > + if (val > i915->pmu.sample[__I915_SAMPLE_RC6].cur)
> > + i915->pmu.sample[__I915_SAMPLE_RC6].cur = val;
>
> 64b wraparound? Maybe not today, maybe not tomorrow... ;)
>
> > +
> > + /* We don't need to sample RC6 from the timer any more. */
> > + i915->pmu.timer_enabled =
> > + __pmu_needs_timer(i915,
> > + i915->pmu.enable & ~config_enabled_mask(I915_PMU_RC6_RESIDENCY),
> > + READ_ONCE(i915->gt.awake));
>
> But we do... :)
> One thing I had in mind was to hook into runtime suspend/resume to read
> the counters there and compensate, but the more I think about it, we may
> as well solve the lack of resolution in the rc6 counters whilst we are
> here. https://bugs.freedesktop.org/show_bug.cgi?id=94852.
How about using dev->power.suspended_jiffies?
--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/pmu: Fix sleep under atomic in RC6 readout
2018-02-06 16:10 ` Imre Deak
@ 2018-02-06 17:12 ` Tvrtko Ursulin
2018-02-06 18:33 ` [PATCH v2] " Tvrtko Ursulin
1 sibling, 0 replies; 15+ messages in thread
From: Tvrtko Ursulin @ 2018-02-06 17:12 UTC (permalink / raw)
To: imre.deak, Chris Wilson; +Cc: David Airlie, Intel-gfx, dri-devel, Rodrigo Vivi
On 06/02/2018 16:10, Imre Deak wrote:
> On Tue, Feb 06, 2018 at 04:04:10PM +0000, Chris Wilson wrote:
>> Quoting Tvrtko Ursulin (2018-02-06 14:31:07)
>>> +static u64 read_rc6_residency(struct drm_i915_private *i915)
>>> +{
>>> + u64 val;
>>> +
>>> + val = intel_rc6_residency_ns(i915, IS_VALLEYVIEW(i915) ?
>>> + VLV_GT_RENDER_RC6 : GEN6_GT_GFX_RC6);
>>> + if (HAS_RC6p(i915))
>>> + val += intel_rc6_residency_ns(i915, GEN6_GT_GFX_RC6p);
>>> + if (HAS_RC6pp(i915))
>>> + val += intel_rc6_residency_ns(i915, GEN6_GT_GFX_RC6pp);
>>
>> We really should mention that these may produce interesting results
>> every 53 minutes. Switching to a timer will allow us to notice the
>> wraparound in each counter.
>>
>>> +
>>> + return val;
>>> +}
>>> +
>>> +static void
>>> +update_rc6_sample(struct drm_i915_private *i915, u64 val, bool locked)
>>> +{
>>> + unsigned long flags;
>>> +
>>> + if (!locked)
>>> + spin_lock_irqsave(&i915->pmu.lock, flags);
>>> +
>>> + /*
>>> + * Update stored RC6 counter only if it is greater than the current
>>> + * value. This deals with periods of runtime suspend during which we are
>>> + * estimating the RC6 residency, so do not want to overshoot the real
>>> + * value read once the device is woken up.
>>> + */
>>> + if (val > i915->pmu.sample[__I915_SAMPLE_RC6].cur)
>>> + i915->pmu.sample[__I915_SAMPLE_RC6].cur = val;
>>
>> 64b wraparound? Maybe not today, maybe not tomorrow... ;)
But in 585 years we're in trouble? :))
>>
>>> +
>>> + /* We don't need to sample RC6 from the timer any more. */
>>> + i915->pmu.timer_enabled =
>>> + __pmu_needs_timer(i915,
>>> + i915->pmu.enable & ~config_enabled_mask(I915_PMU_RC6_RESIDENCY),
>>> + READ_ONCE(i915->gt.awake));
>>
>> But we do... :)
>> One thing I had in mind was to hook into runtime suspend/resume to read
>> the counters there and compensate, but the more I think about it, we may
>> as well solve the lack of resolution in the rc6 counters whilst we are
>> here. https://bugs.freedesktop.org/show_bug.cgi?id=94852.
>
> How about using dev->power.suspended_jiffies?
This sounds promising and I hope it ends up with a much nicer fix so
I'll give it a go.
RC6 wraparound issue I hope we can leave for a different patch. I hope
it will be possible without a timer, as long as someone is not taking
samples every 54 minutes only. (Don't mention VLV.) :)
Regards,
Tvrtko
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2] drm/i915/pmu: Fix sleep under atomic in RC6 readout
2018-02-06 16:10 ` Imre Deak
2018-02-06 17:12 ` [Intel-gfx] " Tvrtko Ursulin
@ 2018-02-06 18:33 ` Tvrtko Ursulin
2018-02-06 21:11 ` Chris Wilson
1 sibling, 1 reply; 15+ messages in thread
From: Tvrtko Ursulin @ 2018-02-06 18:33 UTC (permalink / raw)
To: Intel-gfx; +Cc: David Airlie, dri-devel, Rodrigo Vivi, intel-gfx
From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
We are not allowed to call intel_runtime_pm_get from the PMU counter read
callback since the former can sleep, and the latter is running under IRQ
context.
To workaround this, we record the last known RC6 and while runtime
suspended estimate its increase by querying the runtime PM core
timestamps.
Downside of this approach is that we can temporarily lose a chunk of RC6
time, from the last PMU read-out to runtime suspend entry, but that will
eventually catch up, once device comes back online and in the presence of
PMU queries.
Also, we have to be careful not to overshoot the RC6 estimate, so once
resumed after a period of approximation, we only update the counter once
it catches up. With the observation that RC6 is increasing while the
device is suspended, this should not pose a problem and can only cause
slight inaccuracies due clock base differences.
v2: Simplify by estimating on top of PM core counters. (Imre)
Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104943
Fixes: 6060b6aec03c ("drm/i915/pmu: Add RC6 residency metrics")
Testcase: igt/perf_pmu/rc6-runtime-pm
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Imre Deak <imre.deak@intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
Cc: David Airlie <airlied@linux.ie>
Cc: intel-gfx@lists.freedesktop.org
Cc: dri-devel@lists.freedesktop.org
---
drivers/gpu/drm/i915/i915_pmu.c | 93 ++++++++++++++++++++++++++++++++++-------
drivers/gpu/drm/i915/i915_pmu.h | 6 +++
2 files changed, 84 insertions(+), 15 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
index 1c440460255d..bfc402d47609 100644
--- a/drivers/gpu/drm/i915/i915_pmu.c
+++ b/drivers/gpu/drm/i915/i915_pmu.c
@@ -415,7 +415,81 @@ static int i915_pmu_event_init(struct perf_event *event)
return 0;
}
-static u64 __i915_pmu_event_read(struct perf_event *event)
+static u64 get_rc6(struct drm_i915_private *i915, bool locked)
+{
+ unsigned long flags;
+ u64 val;
+
+ if (intel_runtime_pm_get_if_in_use(i915)) {
+ val = intel_rc6_residency_ns(i915, IS_VALLEYVIEW(i915) ?
+ VLV_GT_RENDER_RC6 :
+ GEN6_GT_GFX_RC6);
+
+ if (HAS_RC6p(i915))
+ val += intel_rc6_residency_ns(i915, GEN6_GT_GFX_RC6p);
+
+ if (HAS_RC6pp(i915))
+ val += intel_rc6_residency_ns(i915, GEN6_GT_GFX_RC6pp);
+
+ intel_runtime_pm_put(i915);
+
+ /*
+ * If we are coming back from being runtime suspended we must
+ * be careful not to report a larger value than returned
+ * previously.
+ */
+
+ if (!locked)
+ spin_lock_irqsave(&i915->pmu.lock, flags);
+
+ if (val >= i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur) {
+ i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = 0;
+ i915->pmu.sample[__I915_SAMPLE_RC6].cur = val;
+ } else {
+ val = i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur;
+ }
+
+ if (!locked)
+ spin_unlock_irqrestore(&i915->pmu.lock, flags);
+ } else {
+ struct pci_dev *pdev = i915->drm.pdev;
+ struct device *kdev = &pdev->dev;
+ unsigned long flags2;
+
+ /*
+ * We are runtime suspended.
+ *
+ * Report the delta from when the device was suspended to now,
+ * on top of the last known real value, as the approximated RC6
+ * counter value.
+ */
+ if (!locked)
+ spin_lock_irqsave(&i915->pmu.lock, flags);
+
+ spin_lock_irqsave(&kdev->power.lock, flags2);
+
+ if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
+ i915->pmu.suspended_jiffies_last =
+ kdev->power.suspended_jiffies;
+
+ val = kdev->power.suspended_jiffies -
+ i915->pmu.suspended_jiffies_last;
+ val += jiffies - kdev->power.accounting_timestamp;
+
+ spin_unlock_irqrestore(&kdev->power.lock, flags2);
+
+ val = jiffies_to_nsecs(val);
+ val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
+ i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
+
+ if (!locked)
+ spin_unlock_irqrestore(&i915->pmu.lock, flags);
+ }
+
+ return val;
+}
+
+static u64 __i915_pmu_event_read(struct perf_event *event, bool locked)
{
struct drm_i915_private *i915 =
container_of(event->pmu, typeof(*i915), pmu.base);
@@ -453,18 +527,7 @@ static u64 __i915_pmu_event_read(struct perf_event *event)
val = count_interrupts(i915);
break;
case I915_PMU_RC6_RESIDENCY:
- intel_runtime_pm_get(i915);
- val = intel_rc6_residency_ns(i915,
- IS_VALLEYVIEW(i915) ?
- VLV_GT_RENDER_RC6 :
- GEN6_GT_GFX_RC6);
- if (HAS_RC6p(i915))
- val += intel_rc6_residency_ns(i915,
- GEN6_GT_GFX_RC6p);
- if (HAS_RC6pp(i915))
- val += intel_rc6_residency_ns(i915,
- GEN6_GT_GFX_RC6pp);
- intel_runtime_pm_put(i915);
+ val = get_rc6(i915, locked);
break;
}
}
@@ -479,7 +542,7 @@ static void i915_pmu_event_read(struct perf_event *event)
again:
prev = local64_read(&hwc->prev_count);
- new = __i915_pmu_event_read(event);
+ new = __i915_pmu_event_read(event, false);
if (local64_cmpxchg(&hwc->prev_count, prev, new) != prev)
goto again;
@@ -534,7 +597,7 @@ static void i915_pmu_enable(struct perf_event *event)
* for all listeners. Even when the event was already enabled and has
* an existing non-zero value.
*/
- local64_set(&event->hw.prev_count, __i915_pmu_event_read(event));
+ local64_set(&event->hw.prev_count, __i915_pmu_event_read(event, true));
spin_unlock_irqrestore(&i915->pmu.lock, flags);
}
diff --git a/drivers/gpu/drm/i915/i915_pmu.h b/drivers/gpu/drm/i915/i915_pmu.h
index 5a2e013a56bb..aa1b1a987ea1 100644
--- a/drivers/gpu/drm/i915/i915_pmu.h
+++ b/drivers/gpu/drm/i915/i915_pmu.h
@@ -27,6 +27,8 @@
enum {
__I915_SAMPLE_FREQ_ACT = 0,
__I915_SAMPLE_FREQ_REQ,
+ __I915_SAMPLE_RC6,
+ __I915_SAMPLE_RC6_ESTIMATED,
__I915_NUM_PMU_SAMPLERS
};
@@ -94,6 +96,10 @@ struct i915_pmu {
* struct intel_engine_cs.
*/
struct i915_pmu_sample sample[__I915_NUM_PMU_SAMPLERS];
+ /**
+ * @suspended_jiffies_last: Cached suspend time from PM core.
+ */
+ unsigned long suspended_jiffies_last;
/**
* @i915_attr: Memory block holding device attributes.
*/
--
2.14.1
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 15+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915/pmu: Fix sleep under atomic in RC6 readout (rev2)
2018-02-06 14:31 [PATCH] drm/i915/pmu: Fix sleep under atomic in RC6 readout Tvrtko Ursulin
2018-02-06 15:13 ` ✗ Fi.CI.BAT: warning for " Patchwork
2018-02-06 16:04 ` [PATCH] " Chris Wilson
@ 2018-02-06 19:16 ` Patchwork
2018-02-07 9:40 ` Patchwork
2018-02-07 12:10 ` ✗ Fi.CI.IGT: failure " Patchwork
4 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2018-02-06 19:16 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/pmu: Fix sleep under atomic in RC6 readout (rev2)
URL : https://patchwork.freedesktop.org/series/37742/
State : success
== Summary ==
Series 37742v2 drm/i915/pmu: Fix sleep under atomic in RC6 readout
https://patchwork.freedesktop.org/api/1.0/series/37742/revisions/2/mbox/
fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:418s
fi-bdw-gvtdvm total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:433s
fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:373s
fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:483s
fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:483s
fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:485s
fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:467s
fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:458s
fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:562s
fi-cnl-y3 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:580s
fi-elk-e7500 total:288 pass:229 dwarn:0 dfail:0 fail:0 skip:59 time:409s
fi-gdg-551 total:288 pass:179 dwarn:0 dfail:0 fail:1 skip:108 time:281s
fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:518s
fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:390s
fi-hsw-4770r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:396s
fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:412s
fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:455s
fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:425s
fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:455s
fi-kbl-7560u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:496s
fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:458s
fi-kbl-r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:501s
fi-pnv-d510 total:288 pass:222 dwarn:1 dfail:0 fail:0 skip:65 time:602s
fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:430s
fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:502s
fi-skl-6700hq total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:526s
fi-skl-6700k2 total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:485s
fi-skl-6770hq total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:479s
fi-skl-guc total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:421s
fi-skl-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:429s
fi-snb-2520m total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:523s
fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:395s
Blacklisted hosts:
fi-glk-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:468s
078873da383505cf8d6940229007115b31f1d5e0 drm-tip: 2018y-02m-06d-11h-21m-36s UTC integration manifest
f25697778bd9 drm/i915/pmu: Fix sleep under atomic in RC6 readout
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7912/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] drm/i915/pmu: Fix sleep under atomic in RC6 readout
2018-02-06 18:33 ` [PATCH v2] " Tvrtko Ursulin
@ 2018-02-06 21:11 ` Chris Wilson
2018-02-06 21:54 ` Imre Deak
0 siblings, 1 reply; 15+ messages in thread
From: Chris Wilson @ 2018-02-06 21:11 UTC (permalink / raw)
To: Intel-gfx
Cc: Tvrtko Ursulin, David Airlie, tursulin, dri-devel, Rodrigo Vivi,
intel-gfx
Quoting Tvrtko Ursulin (2018-02-06 18:33:11)
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> We are not allowed to call intel_runtime_pm_get from the PMU counter read
> callback since the former can sleep, and the latter is running under IRQ
> context.
>
> To workaround this, we record the last known RC6 and while runtime
> suspended estimate its increase by querying the runtime PM core
> timestamps.
>
> Downside of this approach is that we can temporarily lose a chunk of RC6
> time, from the last PMU read-out to runtime suspend entry, but that will
> eventually catch up, once device comes back online and in the presence of
> PMU queries.
>
> Also, we have to be careful not to overshoot the RC6 estimate, so once
> resumed after a period of approximation, we only update the counter once
> it catches up. With the observation that RC6 is increasing while the
> device is suspended, this should not pose a problem and can only cause
> slight inaccuracies due clock base differences.
>
> v2: Simplify by estimating on top of PM core counters. (Imre)
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104943
> Fixes: 6060b6aec03c ("drm/i915/pmu: Add RC6 residency metrics")
> Testcase: igt/perf_pmu/rc6-runtime-pm
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Imre Deak <imre.deak@intel.com>
> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> Cc: David Airlie <airlied@linux.ie>
> Cc: intel-gfx@lists.freedesktop.org
> Cc: dri-devel@lists.freedesktop.org
> ---
> drivers/gpu/drm/i915/i915_pmu.c | 93 ++++++++++++++++++++++++++++++++++-------
> drivers/gpu/drm/i915/i915_pmu.h | 6 +++
> 2 files changed, 84 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> index 1c440460255d..bfc402d47609 100644
> --- a/drivers/gpu/drm/i915/i915_pmu.c
> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> @@ -415,7 +415,81 @@ static int i915_pmu_event_init(struct perf_event *event)
> return 0;
> }
>
> -static u64 __i915_pmu_event_read(struct perf_event *event)
> +static u64 get_rc6(struct drm_i915_private *i915, bool locked)
> +{
> + unsigned long flags;
> + u64 val;
> +
> + if (intel_runtime_pm_get_if_in_use(i915)) {
> + val = intel_rc6_residency_ns(i915, IS_VALLEYVIEW(i915) ?
> + VLV_GT_RENDER_RC6 :
> + GEN6_GT_GFX_RC6);
> +
> + if (HAS_RC6p(i915))
> + val += intel_rc6_residency_ns(i915, GEN6_GT_GFX_RC6p);
> +
> + if (HAS_RC6pp(i915))
> + val += intel_rc6_residency_ns(i915, GEN6_GT_GFX_RC6pp);
> +
> + intel_runtime_pm_put(i915);
> +
> + /*
> + * If we are coming back from being runtime suspended we must
> + * be careful not to report a larger value than returned
> + * previously.
> + */
> +
> + if (!locked)
> + spin_lock_irqsave(&i915->pmu.lock, flags);
> +
> + if (val >= i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur) {
> + i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = 0;
> + i915->pmu.sample[__I915_SAMPLE_RC6].cur = val;
> + } else {
> + val = i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur;
> + }
> +
> + if (!locked)
> + spin_unlock_irqrestore(&i915->pmu.lock, flags);
> + } else {
> + struct pci_dev *pdev = i915->drm.pdev;
> + struct device *kdev = &pdev->dev;
> + unsigned long flags2;
> +
> + /*
> + * We are runtime suspended.
> + *
> + * Report the delta from when the device was suspended to now,
> + * on top of the last known real value, as the approximated RC6
> + * counter value.
> + */
> + if (!locked)
> + spin_lock_irqsave(&i915->pmu.lock, flags);
> +
> + spin_lock_irqsave(&kdev->power.lock, flags2);
> +
> + if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
> + i915->pmu.suspended_jiffies_last =
> + kdev->power.suspended_jiffies;
> +
> + val = kdev->power.suspended_jiffies -
> + i915->pmu.suspended_jiffies_last;
> + val += jiffies - kdev->power.accounting_timestamp;
> +
> + spin_unlock_irqrestore(&kdev->power.lock, flags2);
> +
> + val = jiffies_to_nsecs(val);
> + val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
> + i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
> +
> + if (!locked)
> + spin_unlock_irqrestore(&i915->pmu.lock, flags);
> + }
> +
> + return val;
> +}
I feel slightly dirty, but the dance checks out.
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] drm/i915/pmu: Fix sleep under atomic in RC6 readout
2018-02-06 21:11 ` Chris Wilson
@ 2018-02-06 21:54 ` Imre Deak
2018-02-07 9:20 ` [Intel-gfx] " Tvrtko Ursulin
2018-02-07 14:06 ` Rafael J. Wysocki
0 siblings, 2 replies; 15+ messages in thread
From: Imre Deak @ 2018-02-06 21:54 UTC (permalink / raw)
To: Rafael J. Wysocki; +Cc: David Airlie, Intel-gfx, dri-devel, Rodrigo Vivi
Hi Rafael,
On Tue, Feb 06, 2018 at 09:11:02PM +0000, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-02-06 18:33:11)
> > From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >
> > We are not allowed to call intel_runtime_pm_get from the PMU counter read
> > callback since the former can sleep, and the latter is running under IRQ
> > context.
> >
> > To workaround this, we record the last known RC6 and while runtime
> > suspended estimate its increase by querying the runtime PM core
> > timestamps.
> >
> > Downside of this approach is that we can temporarily lose a chunk of RC6
> > time, from the last PMU read-out to runtime suspend entry, but that will
> > eventually catch up, once device comes back online and in the presence of
> > PMU queries.
> >
> > Also, we have to be careful not to overshoot the RC6 estimate, so once
> > resumed after a period of approximation, we only update the counter once
> > it catches up. With the observation that RC6 is increasing while the
> > device is suspended, this should not pose a problem and can only cause
> > slight inaccuracies due clock base differences.
> >
> > v2: Simplify by estimating on top of PM core counters. (Imre)
> >
> > Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104943
> > Fixes: 6060b6aec03c ("drm/i915/pmu: Add RC6 residency metrics")
> > Testcase: igt/perf_pmu/rc6-runtime-pm
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Imre Deak <imre.deak@intel.com>
> > Cc: Jani Nikula <jani.nikula@linux.intel.com>
> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> > Cc: David Airlie <airlied@linux.ie>
> > Cc: intel-gfx@lists.freedesktop.org
> > Cc: dri-devel@lists.freedesktop.org
> > ---
> > drivers/gpu/drm/i915/i915_pmu.c | 93 ++++++++++++++++++++++++++++++++++-------
> > drivers/gpu/drm/i915/i915_pmu.h | 6 +++
> > 2 files changed, 84 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> > index 1c440460255d..bfc402d47609 100644
> > --- a/drivers/gpu/drm/i915/i915_pmu.c
> > +++ b/drivers/gpu/drm/i915/i915_pmu.c
> > @@ -415,7 +415,81 @@ static int i915_pmu_event_init(struct perf_event *event)
> > return 0;
> > }
> >
> > -static u64 __i915_pmu_event_read(struct perf_event *event)
> > +static u64 get_rc6(struct drm_i915_private *i915, bool locked)
> > +{
> > + unsigned long flags;
> > + u64 val;
> > +
> > + if (intel_runtime_pm_get_if_in_use(i915)) {
> > + val = intel_rc6_residency_ns(i915, IS_VALLEYVIEW(i915) ?
> > + VLV_GT_RENDER_RC6 :
> > + GEN6_GT_GFX_RC6);
> > +
> > + if (HAS_RC6p(i915))
> > + val += intel_rc6_residency_ns(i915, GEN6_GT_GFX_RC6p);
> > +
> > + if (HAS_RC6pp(i915))
> > + val += intel_rc6_residency_ns(i915, GEN6_GT_GFX_RC6pp);
> > +
> > + intel_runtime_pm_put(i915);
> > +
> > + /*
> > + * If we are coming back from being runtime suspended we must
> > + * be careful not to report a larger value than returned
> > + * previously.
> > + */
> > +
> > + if (!locked)
> > + spin_lock_irqsave(&i915->pmu.lock, flags);
> > +
> > + if (val >= i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur) {
> > + i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = 0;
> > + i915->pmu.sample[__I915_SAMPLE_RC6].cur = val;
> > + } else {
> > + val = i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur;
> > + }
> > +
> > + if (!locked)
> > + spin_unlock_irqrestore(&i915->pmu.lock, flags);
> > + } else {
> > + struct pci_dev *pdev = i915->drm.pdev;
> > + struct device *kdev = &pdev->dev;
> > + unsigned long flags2;
> > +
> > + /*
> > + * We are runtime suspended.
> > + *
> > + * Report the delta from when the device was suspended to now,
> > + * on top of the last known real value, as the approximated RC6
> > + * counter value.
> > + */
> > + if (!locked)
> > + spin_lock_irqsave(&i915->pmu.lock, flags);
> > +
> > + spin_lock_irqsave(&kdev->power.lock, flags2);
> > +
> > + if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
> > + i915->pmu.suspended_jiffies_last =
> > + kdev->power.suspended_jiffies;
> > +
> > + val = kdev->power.suspended_jiffies -
> > + i915->pmu.suspended_jiffies_last;
> > + val += jiffies - kdev->power.accounting_timestamp;
> > +
> > + spin_unlock_irqrestore(&kdev->power.lock, flags2);
> > +
> > + val = jiffies_to_nsecs(val);
> > + val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
> > + i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
> > +
> > + if (!locked)
> > + spin_unlock_irqrestore(&i915->pmu.lock, flags);
> > + }
> > +
> > + return val;
> > +}
>
> I feel slightly dirty, but the dance checks out.
Would it be possible to add an RPM helper that provides the device's
runtime suspend residency for the above purpose? This would be
essentially what rtpm_suspended_time_show() provides.
Thanks,
Imre
> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Intel-gfx] [PATCH v2] drm/i915/pmu: Fix sleep under atomic in RC6 readout
2018-02-06 21:54 ` Imre Deak
@ 2018-02-07 9:20 ` Tvrtko Ursulin
2018-02-07 9:36 ` Chris Wilson
2018-02-07 14:06 ` Rafael J. Wysocki
1 sibling, 1 reply; 15+ messages in thread
From: Tvrtko Ursulin @ 2018-02-07 9:20 UTC (permalink / raw)
To: imre.deak, Rafael J. Wysocki
Cc: David Airlie, Intel-gfx, dri-devel, Rodrigo Vivi
On 06/02/2018 21:54, Imre Deak wrote:
> Hi Rafael,
>
> On Tue, Feb 06, 2018 at 09:11:02PM +0000, Chris Wilson wrote:
>> Quoting Tvrtko Ursulin (2018-02-06 18:33:11)
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> We are not allowed to call intel_runtime_pm_get from the PMU counter read
>>> callback since the former can sleep, and the latter is running under IRQ
>>> context.
>>>
>>> To workaround this, we record the last known RC6 and while runtime
>>> suspended estimate its increase by querying the runtime PM core
>>> timestamps.
>>>
>>> Downside of this approach is that we can temporarily lose a chunk of RC6
>>> time, from the last PMU read-out to runtime suspend entry, but that will
>>> eventually catch up, once device comes back online and in the presence of
>>> PMU queries.
>>>
>>> Also, we have to be careful not to overshoot the RC6 estimate, so once
>>> resumed after a period of approximation, we only update the counter once
>>> it catches up. With the observation that RC6 is increasing while the
>>> device is suspended, this should not pose a problem and can only cause
>>> slight inaccuracies due clock base differences.
>>>
>>> v2: Simplify by estimating on top of PM core counters. (Imre)
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104943
>>> Fixes: 6060b6aec03c ("drm/i915/pmu: Add RC6 residency metrics")
>>> Testcase: igt/perf_pmu/rc6-runtime-pm
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Imre Deak <imre.deak@intel.com>
>>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> Cc: David Airlie <airlied@linux.ie>
>>> Cc: intel-gfx@lists.freedesktop.org
>>> Cc: dri-devel@lists.freedesktop.org
>>> ---
>>> drivers/gpu/drm/i915/i915_pmu.c | 93 ++++++++++++++++++++++++++++++++++-------
>>> drivers/gpu/drm/i915/i915_pmu.h | 6 +++
>>> 2 files changed, 84 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
>>> index 1c440460255d..bfc402d47609 100644
>>> --- a/drivers/gpu/drm/i915/i915_pmu.c
>>> +++ b/drivers/gpu/drm/i915/i915_pmu.c
>>> @@ -415,7 +415,81 @@ static int i915_pmu_event_init(struct perf_event *event)
>>> return 0;
>>> }
>>>
>>> -static u64 __i915_pmu_event_read(struct perf_event *event)
>>> +static u64 get_rc6(struct drm_i915_private *i915, bool locked)
>>> +{
>>> + unsigned long flags;
>>> + u64 val;
>>> +
>>> + if (intel_runtime_pm_get_if_in_use(i915)) {
>>> + val = intel_rc6_residency_ns(i915, IS_VALLEYVIEW(i915) ?
>>> + VLV_GT_RENDER_RC6 :
>>> + GEN6_GT_GFX_RC6);
>>> +
>>> + if (HAS_RC6p(i915))
>>> + val += intel_rc6_residency_ns(i915, GEN6_GT_GFX_RC6p);
>>> +
>>> + if (HAS_RC6pp(i915))
>>> + val += intel_rc6_residency_ns(i915, GEN6_GT_GFX_RC6pp);
>>> +
>>> + intel_runtime_pm_put(i915);
>>> +
>>> + /*
>>> + * If we are coming back from being runtime suspended we must
>>> + * be careful not to report a larger value than returned
>>> + * previously.
>>> + */
>>> +
>>> + if (!locked)
>>> + spin_lock_irqsave(&i915->pmu.lock, flags);
>>> +
>>> + if (val >= i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur) {
>>> + i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = 0;
>>> + i915->pmu.sample[__I915_SAMPLE_RC6].cur = val;
>>> + } else {
>>> + val = i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur;
>>> + }
>>> +
>>> + if (!locked)
>>> + spin_unlock_irqrestore(&i915->pmu.lock, flags);
>>> + } else {
>>> + struct pci_dev *pdev = i915->drm.pdev;
>>> + struct device *kdev = &pdev->dev;
>>> + unsigned long flags2;
>>> +
>>> + /*
>>> + * We are runtime suspended.
>>> + *
>>> + * Report the delta from when the device was suspended to now,
>>> + * on top of the last known real value, as the approximated RC6
>>> + * counter value.
>>> + */
>>> + if (!locked)
>>> + spin_lock_irqsave(&i915->pmu.lock, flags);
>>> +
>>> + spin_lock_irqsave(&kdev->power.lock, flags2);
>>> +
>>> + if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
>>> + i915->pmu.suspended_jiffies_last =
>>> + kdev->power.suspended_jiffies;
>>> +
>>> + val = kdev->power.suspended_jiffies -
>>> + i915->pmu.suspended_jiffies_last;
>>> + val += jiffies - kdev->power.accounting_timestamp;
>>> +
>>> + spin_unlock_irqrestore(&kdev->power.lock, flags2);
>>> +
>>> + val = jiffies_to_nsecs(val);
>>> + val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
>>> + i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
>>> +
>>> + if (!locked)
>>> + spin_unlock_irqrestore(&i915->pmu.lock, flags);
>>> + }
>>> +
>>> + return val;
>>> +}
>>
>> I feel slightly dirty, but the dance checks out.
>
> Would it be possible to add an RPM helper that provides the device's
> runtime suspend residency for the above purpose? This would be
> essentially what rtpm_suspended_time_show() provides.
That would indeed be much better since fishing into internals like the
above is not very nice.
However, it would also be good not to delay this fix for too long by
additional logistics, and keep it self-contained - easy to backport.
Regards,
Tvrtko
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [Intel-gfx] [PATCH v2] drm/i915/pmu: Fix sleep under atomic in RC6 readout
2018-02-07 9:20 ` [Intel-gfx] " Tvrtko Ursulin
@ 2018-02-07 9:36 ` Chris Wilson
0 siblings, 0 replies; 15+ messages in thread
From: Chris Wilson @ 2018-02-07 9:36 UTC (permalink / raw)
To: Tvrtko Ursulin, imre.deak, Rafael J. Wysocki
Cc: David Airlie, Intel-gfx, dri-devel, Rodrigo Vivi
Quoting Tvrtko Ursulin (2018-02-07 09:20:27)
>
> On 06/02/2018 21:54, Imre Deak wrote:
> > Hi Rafael,
> >
> > On Tue, Feb 06, 2018 at 09:11:02PM +0000, Chris Wilson wrote:
> >> Quoting Tvrtko Ursulin (2018-02-06 18:33:11)
> >>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>>
> >>> We are not allowed to call intel_runtime_pm_get from the PMU counter read
> >>> callback since the former can sleep, and the latter is running under IRQ
> >>> context.
> >>>
> >>> To workaround this, we record the last known RC6 and while runtime
> >>> suspended estimate its increase by querying the runtime PM core
> >>> timestamps.
> >>>
> >>> Downside of this approach is that we can temporarily lose a chunk of RC6
> >>> time, from the last PMU read-out to runtime suspend entry, but that will
> >>> eventually catch up, once device comes back online and in the presence of
> >>> PMU queries.
> >>>
> >>> Also, we have to be careful not to overshoot the RC6 estimate, so once
> >>> resumed after a period of approximation, we only update the counter once
> >>> it catches up. With the observation that RC6 is increasing while the
> >>> device is suspended, this should not pose a problem and can only cause
> >>> slight inaccuracies due clock base differences.
> >>>
> >>> v2: Simplify by estimating on top of PM core counters. (Imre)
> >>>
> >>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104943
> >>> Fixes: 6060b6aec03c ("drm/i915/pmu: Add RC6 residency metrics")
> >>> Testcase: igt/perf_pmu/rc6-runtime-pm
> >>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> >>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >>> Cc: Imre Deak <imre.deak@intel.com>
> >>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
> >>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
> >>> Cc: David Airlie <airlied@linux.ie>
> >>> Cc: intel-gfx@lists.freedesktop.org
> >>> Cc: dri-devel@lists.freedesktop.org
> >>> ---
> >>> drivers/gpu/drm/i915/i915_pmu.c | 93 ++++++++++++++++++++++++++++++++++-------
> >>> drivers/gpu/drm/i915/i915_pmu.h | 6 +++
> >>> 2 files changed, 84 insertions(+), 15 deletions(-)
> >>>
> >>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
> >>> index 1c440460255d..bfc402d47609 100644
> >>> --- a/drivers/gpu/drm/i915/i915_pmu.c
> >>> +++ b/drivers/gpu/drm/i915/i915_pmu.c
> >>> @@ -415,7 +415,81 @@ static int i915_pmu_event_init(struct perf_event *event)
> >>> return 0;
> >>> }
> >>>
> >>> -static u64 __i915_pmu_event_read(struct perf_event *event)
> >>> +static u64 get_rc6(struct drm_i915_private *i915, bool locked)
> >>> +{
> >>> + unsigned long flags;
> >>> + u64 val;
> >>> +
> >>> + if (intel_runtime_pm_get_if_in_use(i915)) {
> >>> + val = intel_rc6_residency_ns(i915, IS_VALLEYVIEW(i915) ?
> >>> + VLV_GT_RENDER_RC6 :
> >>> + GEN6_GT_GFX_RC6);
> >>> +
> >>> + if (HAS_RC6p(i915))
> >>> + val += intel_rc6_residency_ns(i915, GEN6_GT_GFX_RC6p);
> >>> +
> >>> + if (HAS_RC6pp(i915))
> >>> + val += intel_rc6_residency_ns(i915, GEN6_GT_GFX_RC6pp);
> >>> +
> >>> + intel_runtime_pm_put(i915);
> >>> +
> >>> + /*
> >>> + * If we are coming back from being runtime suspended we must
> >>> + * be careful not to report a larger value than returned
> >>> + * previously.
> >>> + */
> >>> +
> >>> + if (!locked)
> >>> + spin_lock_irqsave(&i915->pmu.lock, flags);
> >>> +
> >>> + if (val >= i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur) {
> >>> + i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = 0;
> >>> + i915->pmu.sample[__I915_SAMPLE_RC6].cur = val;
> >>> + } else {
> >>> + val = i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur;
> >>> + }
> >>> +
> >>> + if (!locked)
> >>> + spin_unlock_irqrestore(&i915->pmu.lock, flags);
> >>> + } else {
> >>> + struct pci_dev *pdev = i915->drm.pdev;
> >>> + struct device *kdev = &pdev->dev;
> >>> + unsigned long flags2;
> >>> +
> >>> + /*
> >>> + * We are runtime suspended.
> >>> + *
> >>> + * Report the delta from when the device was suspended to now,
> >>> + * on top of the last known real value, as the approximated RC6
> >>> + * counter value.
> >>> + */
> >>> + if (!locked)
> >>> + spin_lock_irqsave(&i915->pmu.lock, flags);
> >>> +
> >>> + spin_lock_irqsave(&kdev->power.lock, flags2);
> >>> +
> >>> + if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
> >>> + i915->pmu.suspended_jiffies_last =
> >>> + kdev->power.suspended_jiffies;
> >>> +
> >>> + val = kdev->power.suspended_jiffies -
> >>> + i915->pmu.suspended_jiffies_last;
> >>> + val += jiffies - kdev->power.accounting_timestamp;
> >>> +
> >>> + spin_unlock_irqrestore(&kdev->power.lock, flags2);
> >>> +
> >>> + val = jiffies_to_nsecs(val);
> >>> + val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
> >>> + i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
> >>> +
> >>> + if (!locked)
> >>> + spin_unlock_irqrestore(&i915->pmu.lock, flags);
> >>> + }
> >>> +
> >>> + return val;
> >>> +}
> >>
> >> I feel slightly dirty, but the dance checks out.
> >
> > Would it be possible to add an RPM helper that provides the device's
> > runtime suspend residency for the above purpose? This would be
> > essentially what rtpm_suspended_time_show() provides.
>
> That would indeed be much better since fishing into internals like the
> above is not very nice.
>
> However, it would also be good not to delay this fix for too long by
> additional logistics, and keep it self-contained - easy to backport.
Ok, I've failed to find any suggestion that is an improvement on the
above. (Except suspended_jiffies_last needs only the pmu.lock, and then
move the kdev->power interaction to its own function, say
device_suspended_since()? But can't solve the locked? recursion.)
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 15+ messages in thread
* ✓ Fi.CI.BAT: success for drm/i915/pmu: Fix sleep under atomic in RC6 readout (rev2)
2018-02-06 14:31 [PATCH] drm/i915/pmu: Fix sleep under atomic in RC6 readout Tvrtko Ursulin
` (2 preceding siblings ...)
2018-02-06 19:16 ` ✓ Fi.CI.BAT: success for drm/i915/pmu: Fix sleep under atomic in RC6 readout (rev2) Patchwork
@ 2018-02-07 9:40 ` Patchwork
2018-02-07 13:40 ` Tvrtko Ursulin
2018-02-07 12:10 ` ✗ Fi.CI.IGT: failure " Patchwork
4 siblings, 1 reply; 15+ messages in thread
From: Patchwork @ 2018-02-07 9:40 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/pmu: Fix sleep under atomic in RC6 readout (rev2)
URL : https://patchwork.freedesktop.org/series/37742/
State : success
== Summary ==
Series 37742v2 drm/i915/pmu: Fix sleep under atomic in RC6 readout
https://patchwork.freedesktop.org/api/1.0/series/37742/revisions/2/mbox/
Test gem_mmap_gtt:
Subgroup basic-small-bo-tiledx:
fail -> PASS (fi-gdg-551) fdo#102575
fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:420s
fi-bdw-gvtdvm total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:423s
fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:374s
fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:484s
fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:284s
fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:480s
fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:483s
fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:465s
fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:455s
fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:570s
fi-cnl-y3 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:577s
fi-elk-e7500 total:288 pass:229 dwarn:0 dfail:0 fail:0 skip:59 time:413s
fi-gdg-551 total:288 pass:180 dwarn:0 dfail:0 fail:0 skip:108 time:281s
fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:510s
fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:388s
fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:414s
fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:458s
fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:417s
fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:457s
fi-kbl-7560u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:499s
fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:451s
fi-kbl-r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:503s
fi-pnv-d510 total:288 pass:209 dwarn:1 dfail:0 fail:0 skip:78 time:566s
fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:426s
fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:506s
fi-skl-6700hq total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:532s
fi-skl-6700k2 total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:487s
fi-skl-6770hq total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:477s
fi-skl-guc total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:414s
fi-skl-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:429s
fi-snb-2520m total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:524s
fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:397s
Blacklisted hosts:
fi-glk-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:468s
e5f22cbeec1da222b22367ee3ac165188fb2a36d drm-tip: 2018y-02m-07d-08h-09m-07s UTC integration manifest
097905cd88bd drm/i915/pmu: Fix sleep under atomic in RC6 readout
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7917/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* ✗ Fi.CI.IGT: failure for drm/i915/pmu: Fix sleep under atomic in RC6 readout (rev2)
2018-02-06 14:31 [PATCH] drm/i915/pmu: Fix sleep under atomic in RC6 readout Tvrtko Ursulin
` (3 preceding siblings ...)
2018-02-07 9:40 ` Patchwork
@ 2018-02-07 12:10 ` Patchwork
4 siblings, 0 replies; 15+ messages in thread
From: Patchwork @ 2018-02-07 12:10 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/pmu: Fix sleep under atomic in RC6 readout (rev2)
URL : https://patchwork.freedesktop.org/series/37742/
State : failure
== Summary ==
Test gem_eio:
Subgroup in-flight:
fail -> PASS (shard-hsw) fdo#104676 +2
Test drv_suspend:
Subgroup forcewake-hibernate:
fail -> INCOMPLETE (shard-snb) fdo#103375
Test perf_pmu:
Subgroup frequency:
pass -> FAIL (shard-apl) fdo#104829
Test perf:
Subgroup oa-exponents:
pass -> FAIL (shard-apl) fdo#102254
Subgroup enable-disable:
pass -> FAIL (shard-apl) fdo#103715
Test kms_plane_multiple:
Subgroup atomic-pipe-b-tiling-y:
fail -> PASS (shard-apl) fdo#103166
Test kms_flip:
Subgroup 2x-plain-flip-fb-recreate-interruptible:
pass -> FAIL (shard-hsw)
Subgroup flip-vs-expired-vblank-interruptible:
pass -> FAIL (shard-hsw) fdo#102887
Test pm_rc6_residency:
Subgroup rc6-accuracy:
pass -> SKIP (shard-snb)
Test kms_sysfs_edid_timing:
pass -> WARN (shard-apl) fdo#100047
Test kms_atomic_transition:
Subgroup 2x-modeset-transitions:
skip -> PASS (shard-hsw)
fdo#104676 https://bugs.freedesktop.org/show_bug.cgi?id=104676
fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
fdo#104829 https://bugs.freedesktop.org/show_bug.cgi?id=104829
fdo#102254 https://bugs.freedesktop.org/show_bug.cgi?id=102254
fdo#103715 https://bugs.freedesktop.org/show_bug.cgi?id=103715
fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
fdo#100047 https://bugs.freedesktop.org/show_bug.cgi?id=100047
shard-apl total:3409 pass:1759 dwarn:1 dfail:0 fail:23 skip:1624 time:12358s
shard-hsw total:3442 pass:1755 dwarn:1 dfail:0 fail:14 skip:1671 time:11761s
shard-snb total:3387 pass:1331 dwarn:1 dfail:0 fail:9 skip:2045 time:6542s
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7917/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: ✓ Fi.CI.BAT: success for drm/i915/pmu: Fix sleep under atomic in RC6 readout (rev2)
2018-02-07 9:40 ` Patchwork
@ 2018-02-07 13:40 ` Tvrtko Ursulin
0 siblings, 0 replies; 15+ messages in thread
From: Tvrtko Ursulin @ 2018-02-07 13:40 UTC (permalink / raw)
To: intel-gfx, Patchwork, Tvrtko Ursulin
On 07/02/2018 09:40, Patchwork wrote:
> == Series Details ==
>
> Series: drm/i915/pmu: Fix sleep under atomic in RC6 readout (rev2)
> URL : https://patchwork.freedesktop.org/series/37742/
> State : success
>
> == Summary ==
>
> Series 37742v2 drm/i915/pmu: Fix sleep under atomic in RC6 readout
> https://patchwork.freedesktop.org/api/1.0/series/37742/revisions/2/mbox/
>
> Test gem_mmap_gtt:
> Subgroup basic-small-bo-tiledx:
> fail -> PASS (fi-gdg-551) fdo#102575
>
> fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
>
> fi-bdw-5557u total:288 pass:267 dwarn:0 dfail:0 fail:0 skip:21 time:420s
> fi-bdw-gvtdvm total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:423s
> fi-blb-e6850 total:288 pass:223 dwarn:1 dfail:0 fail:0 skip:64 time:374s
> fi-bsw-n3050 total:288 pass:242 dwarn:0 dfail:0 fail:0 skip:46 time:484s
> fi-bwr-2160 total:288 pass:183 dwarn:0 dfail:0 fail:0 skip:105 time:284s
> fi-bxt-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:480s
> fi-bxt-j4205 total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:483s
> fi-byt-j1900 total:288 pass:253 dwarn:0 dfail:0 fail:0 skip:35 time:465s
> fi-byt-n2820 total:288 pass:249 dwarn:0 dfail:0 fail:0 skip:39 time:455s
> fi-cfl-s2 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:570s
> fi-cnl-y3 total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:577s
> fi-elk-e7500 total:288 pass:229 dwarn:0 dfail:0 fail:0 skip:59 time:413s
> fi-gdg-551 total:288 pass:180 dwarn:0 dfail:0 fail:0 skip:108 time:281s
> fi-glk-1 total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:510s
> fi-hsw-4770 total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:388s
> fi-ilk-650 total:288 pass:228 dwarn:0 dfail:0 fail:0 skip:60 time:414s
> fi-ivb-3520m total:288 pass:259 dwarn:0 dfail:0 fail:0 skip:29 time:458s
> fi-ivb-3770 total:288 pass:255 dwarn:0 dfail:0 fail:0 skip:33 time:417s
> fi-kbl-7500u total:288 pass:263 dwarn:1 dfail:0 fail:0 skip:24 time:457s
> fi-kbl-7560u total:288 pass:269 dwarn:0 dfail:0 fail:0 skip:19 time:499s
> fi-kbl-7567u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:451s
> fi-kbl-r total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:503s
> fi-pnv-d510 total:288 pass:209 dwarn:1 dfail:0 fail:0 skip:78 time:566s
> fi-skl-6260u total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:426s
> fi-skl-6600u total:288 pass:261 dwarn:0 dfail:0 fail:0 skip:27 time:506s
> fi-skl-6700hq total:288 pass:262 dwarn:0 dfail:0 fail:0 skip:26 time:532s
> fi-skl-6700k2 total:288 pass:264 dwarn:0 dfail:0 fail:0 skip:24 time:487s
> fi-skl-6770hq total:288 pass:268 dwarn:0 dfail:0 fail:0 skip:20 time:477s
> fi-skl-guc total:288 pass:260 dwarn:0 dfail:0 fail:0 skip:28 time:414s
> fi-skl-gvtdvm total:288 pass:265 dwarn:0 dfail:0 fail:0 skip:23 time:429s
> fi-snb-2520m total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:524s
> fi-snb-2600 total:288 pass:248 dwarn:0 dfail:0 fail:0 skip:40 time:397s
> Blacklisted hosts:
> fi-glk-dsi total:288 pass:258 dwarn:0 dfail:0 fail:0 skip:30 time:468s
>
> e5f22cbeec1da222b22367ee3ac165188fb2a36d drm-tip: 2018y-02m-07d-08h-09m-07s UTC integration manifest
> 097905cd88bd drm/i915/pmu: Fix sleep under atomic in RC6 readout
Pushed, thanks for the review and all.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] drm/i915/pmu: Fix sleep under atomic in RC6 readout
2018-02-06 21:54 ` Imre Deak
2018-02-07 9:20 ` [Intel-gfx] " Tvrtko Ursulin
@ 2018-02-07 14:06 ` Rafael J. Wysocki
1 sibling, 0 replies; 15+ messages in thread
From: Rafael J. Wysocki @ 2018-02-07 14:06 UTC (permalink / raw)
To: imre.deak; +Cc: David Airlie, Intel-gfx, dri-devel, Rodrigo Vivi
On 2/6/2018 10:54 PM, Imre Deak wrote:
> Hi Rafael,
>
> On Tue, Feb 06, 2018 at 09:11:02PM +0000, Chris Wilson wrote:
>> Quoting Tvrtko Ursulin (2018-02-06 18:33:11)
>>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>>
>>> We are not allowed to call intel_runtime_pm_get from the PMU counter read
>>> callback since the former can sleep, and the latter is running under IRQ
>>> context.
>>>
>>> To workaround this, we record the last known RC6 and while runtime
>>> suspended estimate its increase by querying the runtime PM core
>>> timestamps.
>>>
>>> Downside of this approach is that we can temporarily lose a chunk of RC6
>>> time, from the last PMU read-out to runtime suspend entry, but that will
>>> eventually catch up, once device comes back online and in the presence of
>>> PMU queries.
>>>
>>> Also, we have to be careful not to overshoot the RC6 estimate, so once
>>> resumed after a period of approximation, we only update the counter once
>>> it catches up. With the observation that RC6 is increasing while the
>>> device is suspended, this should not pose a problem and can only cause
>>> slight inaccuracies due clock base differences.
>>>
>>> v2: Simplify by estimating on top of PM core counters. (Imre)
>>>
>>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=104943
>>> Fixes: 6060b6aec03c ("drm/i915/pmu: Add RC6 residency metrics")
>>> Testcase: igt/perf_pmu/rc6-runtime-pm
>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>> Cc: Imre Deak <imre.deak@intel.com>
>>> Cc: Jani Nikula <jani.nikula@linux.intel.com>
>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>> Cc: Rodrigo Vivi <rodrigo.vivi@intel.com>
>>> Cc: David Airlie <airlied@linux.ie>
>>> Cc: intel-gfx@lists.freedesktop.org
>>> Cc: dri-devel@lists.freedesktop.org
>>> ---
>>> drivers/gpu/drm/i915/i915_pmu.c | 93 ++++++++++++++++++++++++++++++++++-------
>>> drivers/gpu/drm/i915/i915_pmu.h | 6 +++
>>> 2 files changed, 84 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c b/drivers/gpu/drm/i915/i915_pmu.c
>>> index 1c440460255d..bfc402d47609 100644
>>> --- a/drivers/gpu/drm/i915/i915_pmu.c
>>> +++ b/drivers/gpu/drm/i915/i915_pmu.c
>>> @@ -415,7 +415,81 @@ static int i915_pmu_event_init(struct perf_event *event)
>>> return 0;
>>> }
>>>
>>> -static u64 __i915_pmu_event_read(struct perf_event *event)
>>> +static u64 get_rc6(struct drm_i915_private *i915, bool locked)
>>> +{
>>> + unsigned long flags;
>>> + u64 val;
>>> +
>>> + if (intel_runtime_pm_get_if_in_use(i915)) {
>>> + val = intel_rc6_residency_ns(i915, IS_VALLEYVIEW(i915) ?
>>> + VLV_GT_RENDER_RC6 :
>>> + GEN6_GT_GFX_RC6);
>>> +
>>> + if (HAS_RC6p(i915))
>>> + val += intel_rc6_residency_ns(i915, GEN6_GT_GFX_RC6p);
>>> +
>>> + if (HAS_RC6pp(i915))
>>> + val += intel_rc6_residency_ns(i915, GEN6_GT_GFX_RC6pp);
>>> +
>>> + intel_runtime_pm_put(i915);
>>> +
>>> + /*
>>> + * If we are coming back from being runtime suspended we must
>>> + * be careful not to report a larger value than returned
>>> + * previously.
>>> + */
>>> +
>>> + if (!locked)
>>> + spin_lock_irqsave(&i915->pmu.lock, flags);
>>> +
>>> + if (val >= i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur) {
>>> + i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = 0;
>>> + i915->pmu.sample[__I915_SAMPLE_RC6].cur = val;
>>> + } else {
>>> + val = i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur;
>>> + }
>>> +
>>> + if (!locked)
>>> + spin_unlock_irqrestore(&i915->pmu.lock, flags);
>>> + } else {
>>> + struct pci_dev *pdev = i915->drm.pdev;
>>> + struct device *kdev = &pdev->dev;
>>> + unsigned long flags2;
>>> +
>>> + /*
>>> + * We are runtime suspended.
>>> + *
>>> + * Report the delta from when the device was suspended to now,
>>> + * on top of the last known real value, as the approximated RC6
>>> + * counter value.
>>> + */
>>> + if (!locked)
>>> + spin_lock_irqsave(&i915->pmu.lock, flags);
>>> +
>>> + spin_lock_irqsave(&kdev->power.lock, flags2);
>>> +
>>> + if (!i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur)
>>> + i915->pmu.suspended_jiffies_last =
>>> + kdev->power.suspended_jiffies;
>>> +
>>> + val = kdev->power.suspended_jiffies -
>>> + i915->pmu.suspended_jiffies_last;
>>> + val += jiffies - kdev->power.accounting_timestamp;
>>> +
>>> + spin_unlock_irqrestore(&kdev->power.lock, flags2);
>>> +
>>> + val = jiffies_to_nsecs(val);
>>> + val += i915->pmu.sample[__I915_SAMPLE_RC6].cur;
>>> + i915->pmu.sample[__I915_SAMPLE_RC6_ESTIMATED].cur = val;
>>> +
>>> + if (!locked)
>>> + spin_unlock_irqrestore(&i915->pmu.lock, flags);
>>> + }
>>> +
>>> + return val;
>>> +}
>> I feel slightly dirty, but the dance checks out.
> Would it be possible to add an RPM helper that provides the device's
> runtime suspend residency for the above purpose? This would be
> essentially what rtpm_suspended_time_show() provides.
Yes, in general, but as already mentioned in this thread, I guess it's
better to do that later as an extra step for the backportability sake.
Thanks,
Rafael
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2018-02-07 14:06 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-06 14:31 [PATCH] drm/i915/pmu: Fix sleep under atomic in RC6 readout Tvrtko Ursulin
2018-02-06 15:13 ` ✗ Fi.CI.BAT: warning for " Patchwork
2018-02-06 16:04 ` [PATCH] " Chris Wilson
2018-02-06 16:10 ` Imre Deak
2018-02-06 17:12 ` [Intel-gfx] " Tvrtko Ursulin
2018-02-06 18:33 ` [PATCH v2] " Tvrtko Ursulin
2018-02-06 21:11 ` Chris Wilson
2018-02-06 21:54 ` Imre Deak
2018-02-07 9:20 ` [Intel-gfx] " Tvrtko Ursulin
2018-02-07 9:36 ` Chris Wilson
2018-02-07 14:06 ` Rafael J. Wysocki
2018-02-06 19:16 ` ✓ Fi.CI.BAT: success for drm/i915/pmu: Fix sleep under atomic in RC6 readout (rev2) Patchwork
2018-02-07 9:40 ` Patchwork
2018-02-07 13:40 ` Tvrtko Ursulin
2018-02-07 12:10 ` ✗ Fi.CI.IGT: failure " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.