On ke, 2016-08-17 at 18:15 +0530, Goel, Akash wrote:
>
> On 8/17/2016 5:11 PM, Chris Wilson wrote:
> > On Wed, Aug 17, 2016 at 12:27:30PM +0100, Tvrtko Ursulin wrote:
> > >
> > > On 17/08/16 11:14, akash.goel@xxxxxxxxx wrote:
> > > > From: Akash Goel <akash.goel@xxxxxxxxx>
> > > >
> > > > The GuC log buffer flush work item does a register access to send the ack
> > > > to GuC and this work item, if not synced before suspend, can potentially
> > > > get executed after the GFX device is suspended.
> > > > The work item function uses rpm_get/rpm_put calls around the Hw access,
> > > > this covers the runtime suspend case but for system suspend case (which can
> > > > be done asychronously/forcefully) sync would be required as kernel can
> > > > potentially schedule the work items even after some devices, including GFX,
> > > > have been put to suspend.
> > > > Also sync has to be done conditionally i.e. only for the system suspend
> > > > case, as sync along with rpm_get/rpm_put calls can cause a deadlock for rpm
> > > > suspend path.
> > > >
> > > > Cc: Imre Deak <imre.deak@xxxxxxxxx>
> > > > Signed-off-by: Akash Goel <akash.goel@xxxxxxxxx>
> > > > ---
> > > > drivers/gpu/drm/i915/i915_drv.c | 4 ++--
> > > > drivers/gpu/drm/i915/i915_guc_submission.c | 8 +++++++-
> > > > drivers/gpu/drm/i915/intel_guc.h | 2 +-
> > > > 3 files changed, 10 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > > > index cdee60b..2ae0ad4 100644
> > > > --- a/drivers/gpu/drm/i915/i915_drv.c
> > > > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > > > @@ -1427,7 +1427,7 @@ static int i915_drm_suspend(struct drm_device *dev)
> > > > goto out;
> > > > }
> > > >
> > > > - intel_guc_suspend(dev);
> > > > + intel_guc_suspend(dev, false);
> > > >
> > > > intel_display_suspend(dev);
> > > >
> > > > @@ -2321,7 +2321,7 @@ static int intel_runtime_suspend(struct device *device)
> > > > i915_gem_release_all_mmaps(dev_priv);
> > > > mutex_unlock(&dev->struct_mutex);
> > > >
> > > > - intel_guc_suspend(dev);
> > > > + intel_guc_suspend(dev, true);
> > > >
> > > > intel_runtime_pm_disable_interrupts(dev_priv);
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/i915_guc_submission.c b/drivers/gpu/drm/i915/i915_guc_submission.c
> > > > index ef0c116..1af8a8b 100644
> > > > --- a/drivers/gpu/drm/i915/i915_guc_submission.c
> > > > +++ b/drivers/gpu/drm/i915/i915_guc_submission.c
> > > > @@ -1519,7 +1519,7 @@ void i915_guc_submission_fini(struct drm_i915_private *dev_priv)
> > > > * intel_guc_suspend() - notify GuC entering suspend state
> > > > * @dev: drm device
> > > > */
> > > > -int intel_guc_suspend(struct drm_device *dev)
> > > > +int intel_guc_suspend(struct drm_device *dev, bool rpm_suspend)
> > > > {
> > > > struct drm_i915_private *dev_priv = to_i915(dev);
> > > > struct intel_guc *guc = &dev_priv->guc;
> > > > @@ -1530,6 +1530,12 @@ int intel_guc_suspend(struct drm_device *dev)
> > > > return 0;
> > > >
> > > > gen9_disable_guc_interrupts(dev_priv);
> > > > + /* Sync is needed only for the system suspend case, runtime suspend
> > > > + * case is covered due to rpm get/put calls used around Hw access in
> > > > + * the work item function.
> > > > + */
> > > > + if (!rpm_suspend && (i915.guc_log_level >= 0))
> > > > + flush_work(&dev_priv->guc.log.flush_work);
> >
> > In which case (rpm suspend) the flush_work is idle and this a noop. That
> > you have to pass around such state suggests that you are papering over a
> > bug?
> In case of rpm suspend the flush_work may not be a NOOP.
> Can use the flush_work for runtime suspend also but in spite of that
> can't prevent the 'RPM wakelock' asserts, as the work item can get
> executed after the rpm ref count drops to zero and before runtime
> suspend kicks in (after autosuspend delay).
>
> For that you had earlier suggested to use rpm get/put in the work item
> function, around the register access, but with that had to remove the
> flush_work from the suspend hook, otherwise a deadlock can happen.
> So doing the flush_work conditionally for system suspend case, as rpm
> get/put won't cause the resume of device in that case.
>
> Actually I had discussed about this with Imre and as per his inputs
> prepared this patch.
There would be this alternative:
in gen9_guc_irq_handler():
WARN_ON(!intel_runtime_pm_get_if_in_use());
if (!queue_work(log.flush_work))
intel_runtime_pm_put();
and dropping the reference at the end of the work item. This would make
the flush_work() a nop in case of runtime_suspend().
--Imre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
