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?
-Chris
--
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
