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

From: "Goel, Akash" <akash.goel@intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>
Cc: akash.goel@intel.com, intel-gfx@lists.freedesktop.org
Subject: Re: [PATCH 19/19] drm/i915: Sync against the GuC log buffer flush work item on system suspend
Date: Wed, 17 Aug 2016 18:15:38 +0530	[thread overview]
Message-ID: <12d7e0d7-6fbd-c6b9-afd7-5de6a4fe128e@intel.com> (raw)
In-Reply-To: <20160817114149.GJ22696@nuc-i3427.alporthouse.com>

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@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);
>
> 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.

Best regards
Akash

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