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

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 17/08/16 11:14, akash.goel@intel.com wrote:
> From: Akash Goel <akash.goel@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@intel.com>
> Signed-off-by: Akash Goel <akash.goel@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);
>
>   	ctx = dev_priv->kernel_context;
>
> diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
> index b56fe24..1367314 100644
> --- a/drivers/gpu/drm/i915/intel_guc.h
> +++ b/drivers/gpu/drm/i915/intel_guc.h
> @@ -172,7 +172,7 @@ extern void intel_guc_init(struct drm_device *dev);
>   extern int intel_guc_setup(struct drm_device *dev);
>   extern void intel_guc_fini(struct drm_device *dev);
>   extern const char *intel_guc_fw_status_repr(enum intel_guc_fw_status status);
> -extern int intel_guc_suspend(struct drm_device *dev);
> +extern int intel_guc_suspend(struct drm_device *dev, bool rpm_suspend);
>   extern int intel_guc_resume(struct drm_device *dev);
>
>   /* i915_guc_submission.c */
>

Sounds believable but would prefer is someone with better knowledge of 
suspend/resume paths would gave it a look as well.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

Regards,

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