[Intel-gfx] [PATCH 19/19] drm/i915: Sync against the GuC log buffer flush work item on system suspend

[Chris Wilson]

On Wed, Aug 17, 2016 at 12:27:30PM +0100, Tvrtko Ursulin wrote:
> 
> On 17/08/16 11:14, akash.goel at intel.com wrote:
> >From: Akash Goel <akash.goel at intel.com>
> >
> >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 at intel.com>
> >Signed-off-by: Akash Goel <akash.goel at intel.com>
> >---
> >  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