| Message ID | 1449681017-16722-1-git-send-email-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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>
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);
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
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
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
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
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);
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(-)