Re: [PATCH] drm/i915: Flush the RPS bottom-half when the GPU idles

From: Imre Deak <imre.deak@intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH] drm/i915: Flush the RPS bottom-half when the GPU idles
Date: Thu, 10 Dec 2015 00:02:55 +0200	[thread overview]
Message-ID: <1449698575.19914.18.camel@intel.com> (raw)
In-Reply-To: <20151209205249.GA12761@nuc-i3427.alporthouse.com>

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@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx