Re: [Intel-gfx] [PATCH] drm/i915/pmu: Fix sleep under atomic in RC6 readout
On 06/02/2018 16:10, Imre Deak wrote:
On Tue, Feb 06, 2018 at 04:04:10PM +, 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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH] drm/i915/pmu: Fix sleep under atomic in RC6 readout
On Tue, Feb 06, 2018 at 04:04:10PM +, 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
Re: [Intel-gfx] [PATCH] drm/i915/pmu: Fix sleep under atomic in RC6 readout
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
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
