On Wed, 2015-12-09 at 20:52 +0000, Chris Wilson wrote:
> On Wed, Dec 09, 2015 at 07:47:29PM +0200, Imre Deak wrote:
> > > void gen6_rps_idle(struct drm_i915_private *dev_priv)
> > > {
> > > - struct drm_device *dev = dev_priv->dev;
> > > + /* Flush our bottom-half so that it does not race with
> > > us
> > > + * setting the idle frequency and so that it is bounded
> > > by
> > > + * our rpm wakeref.
> > > + */
> > > + flush_work(&dev_priv->rps.work);
> >
> > A (spurious) RPS interrupt could still reschedule the work, so
> > could we
> > also explicitly disable the interrupts? Meaning to use
> > gen6_{disable,enable}_rps_interrupts() in gen6_rps_{idle,busy} and
> > making sure vlv_set_rps_idle(), gen6_set_rps() would not re-enable
> > the
> > interrupts.
>
> Yes, we can do that.
>
> > That would also make it possible to
> > remove gen6_{disable,enable}_rps_interrupts() from the
> > suspend/resume path.
>
> A while back we discussed this, and I've been running with
>
> http://cgit.freedesktop.org/~ickle/linux-2.6/commit/?h=nightly&id=11f
> f1e6deceb33a5db7be31830abb46c1450755e
>
> which disables the RPS interrupt at idle time (and kills the then
> superflous
> suspend path). It works but for a few spurious interrupt warnings.
If this is about the WARNs in gen6_enable_rps_interrupts() then
gen6_disable_rps_interrupts() may leave PM IIR bits set,
but gen6_reset_rps_interrupts() would clear those. The patch you linked
calls gen6_reset_rps_interrupts(), so no idea how they could still
happen.
> Though I missed the flush_work(&rps.work) caught in this patch, which
> may just account for the errors.
There is cancel_work_sync(&rps.work) in gen6_disable_rps_interrupts(),
so we wouldn't need the flush_work() imo.
Btw, I haven't measured, but if the overhead added by all this is
significant we could use instead rpm_get_noidle() in the rps work too.
--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
