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

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

[Chris Wilson]

Make sure that the RPS bottom-half is flushed before we set the idle
frequency when we decide the GPU is idle. This should prevent any races
with the bottom-half and setting the idle frequency, and ensures that
the bottom-half is bounded by the GPU's rpm reference taken for when it
is active (i.e. between gen6_rps_busy() and gen6_rps_idle()).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index e655321385e2..bb796d4e9a3a 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -4401,11 +4401,15 @@ void gen6_rps_busy(struct drm_i915_private *dev_priv)
 
 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);
 
 	mutex_lock(&dev_priv->rps.hw_lock);
 	if (dev_priv->rps.enabled) {
-		if (IS_VALLEYVIEW(dev))
+		if (IS_VALLEYVIEW(dev_priv))
 			vlv_set_rps_idle(dev_priv);
 		else
 			gen6_set_rps(dev_priv->dev, dev_priv->rps.idle_freq);
-- 
2.6.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Jesse Barnes]

On 12/09/2015 09:10 AM, Chris Wilson wrote:
> Make sure that the RPS bottom-half is flushed before we set the idle
> frequency when we decide the GPU is idle. This should prevent any races
> with the bottom-half and setting the idle frequency, and ensures that
> the bottom-half is bounded by the GPU's rpm reference taken for when it
> is active (i.e. between gen6_rps_busy() and gen6_rps_idle()).
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index e655321385e2..bb796d4e9a3a 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4401,11 +4401,15 @@ void gen6_rps_busy(struct drm_i915_private *dev_priv)
>  
>  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);
>  
>  	mutex_lock(&dev_priv->rps.hw_lock);
>  	if (dev_priv->rps.enabled) {
> -		if (IS_VALLEYVIEW(dev))
> +		if (IS_VALLEYVIEW(dev_priv))
>  			vlv_set_rps_idle(dev_priv);
>  		else
>  			gen6_set_rps(dev_priv->dev, dev_priv->rps.idle_freq);
> 

Hah and a consistency fix snuck in there... nice.

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Imre Deak]

On ke, 2015-12-09 at 17:10 +0000, Chris Wilson wrote:
> Make sure that the RPS bottom-half is flushed before we set the idle
> frequency when we decide the GPU is idle. This should prevent any
> races
> with the bottom-half and setting the idle frequency, and ensures that
> the bottom-half is bounded by the GPU's rpm reference taken for when
> it
> is active (i.e. between gen6_rps_busy() and gen6_rps_idle()).
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c
> b/drivers/gpu/drm/i915/intel_pm.c
> index e655321385e2..bb796d4e9a3a 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -4401,11 +4401,15 @@ void gen6_rps_busy(struct drm_i915_private
> *dev_priv)
>  
>  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.

That would also make it possible to
remove gen6_{disable,enable}_rps_interrupts() from the 
suspend/resume path.

>  	mutex_lock(&dev_priv->rps.hw_lock);
>  	if (dev_priv->rps.enabled) {
> -		if (IS_VALLEYVIEW(dev))
> +		if (IS_VALLEYVIEW(dev_priv))
>  			vlv_set_rps_idle(dev_priv);
>  		else
>  			gen6_set_rps(dev_priv->dev, dev_priv-
> >rps.idle_freq);
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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=11ff1e6deceb33a5db7be31830abb46c1450755e

which disables the RPS interrupt at idle time (and kills the then superflous
suspend path). It works but for a few spurious interrupt warnings.
Though I missed the flush_work(&rps.work) caught in this patch, which
may just account for the errors.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Imre Deak]

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

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

[Chris Wilson]

On Thu, Dec 10, 2015 at 12:02:55AM +0200, Imre Deak wrote:
> 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.

Maybe self-inflicted by a later patch to remove reset-rps-interrupts. I
was under the impression that we didn't actually need to do the reset.
 
> > 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.

Right.
 
> 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.

I was anticipating the irq locks and synchronize_irq being the worst
offender. However, rps busy/idle don't impact much on GPU intensive
workloads so tend to stay away from the usual measurements, and are so
hopefully insignificant.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

On Wed, Dec 09, 2015 at 09:30:42AM -0800, Jesse Barnes wrote:
> On 12/09/2015 09:10 AM, Chris Wilson wrote:
> > Make sure that the RPS bottom-half is flushed before we set the idle
> > frequency when we decide the GPU is idle. This should prevent any races
> > with the bottom-half and setting the idle frequency, and ensures that
> > the bottom-half is bounded by the GPU's rpm reference taken for when it
> > is active (i.e. between gen6_rps_busy() and gen6_rps_idle()).
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_pm.c | 8 ++++++--
> >  1 file changed, 6 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> > index e655321385e2..bb796d4e9a3a 100644
> > --- a/drivers/gpu/drm/i915/intel_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_pm.c
> > @@ -4401,11 +4401,15 @@ void gen6_rps_busy(struct drm_i915_private *dev_priv)
> >  
> >  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);
> >  
> >  	mutex_lock(&dev_priv->rps.hw_lock);
> >  	if (dev_priv->rps.enabled) {
> > -		if (IS_VALLEYVIEW(dev))
> > +		if (IS_VALLEYVIEW(dev_priv))
> >  			vlv_set_rps_idle(dev_priv);
> >  		else
> >  			gen6_set_rps(dev_priv->dev, dev_priv->rps.idle_freq);
> > 
> 
> Hah and a consistency fix snuck in there... nice.

TIL one cannot flush work from inside a workqueue.

The good news is we can quite happily squash our usage of i915_wq and
just use the regular system_wq.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx