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

Commit Message

[e-mail] Aug. 17, 2016, 10:14 a.m. UTC

From: Akash Goel <[e-mail]>

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 <[e-mail]>
Signed-off-by: Akash Goel <[e-mail]>
---
 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(-)

Comments

Tvrtko Ursulin Aug. 17, 2016, 11:27 a.m. UTC | #1

On 17/08/16 11:14, [e-mail] wrote:
> From: Akash Goel <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: Akash Goel <[e-mail]>
> ---
>   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 <[e-mail]>

Regards,

Tvrtko

Chris Wilson Aug. 17, 2016, 11:41 a.m. UTC | #2

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

[e-mail] Aug. 17, 2016, 12:45 p.m. UTC | #3

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, [e-mail] wrote:
>>> From: Akash Goel <[e-mail]>
>>>
>>> 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 <[e-mail]>
>>> Signed-off-by: Akash Goel <[e-mail]>
>>> ---
>>>  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
>

Imre Deak Aug. 17, 2016, 1:11 p.m. UTC | #4

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, [e-mail] wrote:
> > > > From: Akash Goel <[e-mail]>
> > > > 
> > > > 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 <[e-mail]>
> > > > Signed-off-by: Akash Goel <[e-mail]>
> > > > ---
> > > >  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

[e-mail] Aug. 17, 2016, 3:37 p.m. UTC | #5

On 8/17/2016 6:41 PM, Imre Deak wrote:
> 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, [e-mail] wrote:
>>>>> From: Akash Goel <[e-mail]>
>>>>>
>>>>> 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 <[e-mail]>
>>>>> Signed-off-by: Akash Goel <[e-mail]>
>>>>> ---
>>>>>  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:
>
Thanks much for suggesting the alternate approach.

Just to confirm whether I understood everything correctly,

> in gen9_guc_irq_handler():
>    WARN_ON(!intel_runtime_pm_get_if_in_use());
Used WARN, as we don't expect the device to be suspended at this 
juncture, so intel_runtime_pm_get_if_in_use() should return true.

>    if (!queue_work(log.flush_work))
If queue_work returns 0, then work item is already pending, so it won't
be queued hence can release the rpm ref count now only.
>        intel_runtime_pm_put();

>
> and dropping the reference at the end of the work item.
This will be just like the __intel_autoenable_gt_powersave

> This would make the flush_work() a nop in case of runtime_suspend().
So can call the flush_work unconditionally.

Hope I understood it correctly.

Best regards
Akash

> --Imre
>

[e-mail] Aug. 18, 2016, 3:45 a.m. UTC | #6

On 8/17/2016 9:07 PM, Goel, Akash wrote:
>
>
> On 8/17/2016 6:41 PM, Imre Deak wrote:
>> 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:
>>>>>

>>>>>> +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:
>>
> Thanks much for suggesting the alternate approach.
>
> Just to confirm whether I understood everything correctly,
>
>> in gen9_guc_irq_handler():
>>    WARN_ON(!intel_runtime_pm_get_if_in_use());
> Used WARN, as we don't expect the device to be suspended at this
> juncture, so intel_runtime_pm_get_if_in_use() should return true.
>
>>    if (!queue_work(log.flush_work))
> If queue_work returns 0, then work item is already pending, so it won't
> be queued hence can release the rpm ref count now only.
>>        intel_runtime_pm_put();
>
>>
>> and dropping the reference at the end of the work item.
> This will be just like the __intel_autoenable_gt_powersave
>
>> This would make the flush_work() a nop in case of runtime_suspend().
> So can call the flush_work unconditionally.
>
> Hope I understood it correctly.
>
Hi Imre,

You had suggested to use the below code from irq handler, suspecting 
that intel_runtime_pm_get_if_in_use() can return false, if interrupt 
gets handled just after device goes out of use.

	if (intel_runtime_pm_get_if_in_use()) {
		if (!queue_work(log.flush_work))
			intel_runtime_pm_put();
	}

Do you mean to say that interrupt can come when rpm suspend has already 
started but before the interrupt is disabled from the suspend hook ?
Like if interrupt comes b/w 1) & 4), then runtime_pm_get_if_in_use()
will return false.
1)	Autosuspend delay elapses (device is marked as suspending)
2)		intel_runtime_suspend
3)			intel_guc_suspend
4)				gen9_disable_guc_interrupts(dev_priv);

If the above hypothesis is correct, then it implies that interrupt has 
to come after autosuspend delay has elapsed for the above scenario to arise.

I think it would be unlikely for the interrupt to come so late because 
device would have gone idle just before the autosuspend period started 
and so no GuC submissions would have been done after that.
So the probability of missing a work item could be very less and we
can bear that.

Best regards
Akash

> Best regards
> Akash
>
>> --Imre
>>

Imre Deak Aug. 18, 2016, 10:55 a.m. UTC | #7

On to, 2016-08-18 at 09:15 +0530, Goel, Akash wrote:
> 
> On 8/17/2016 9:07 PM, Goel, Akash wrote:
> > 
> > 
> > On 8/17/2016 6:41 PM, Imre Deak wrote:
> > > 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:
> > > > > > 
> 
> > > > > > > +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:
> > > 
> > Thanks much for suggesting the alternate approach.
> > 
> > Just to confirm whether I understood everything correctly,
> > 
> > > in gen9_guc_irq_handler():
> > >    WARN_ON(!intel_runtime_pm_get_if_in_use());
> > Used WARN, as we don't expect the device to be suspended at this
> > juncture, so intel_runtime_pm_get_if_in_use() should return true.
> > 
> > >    if (!queue_work(log.flush_work))
> > If queue_work returns 0, then work item is already pending, so it
> > won't
> > be queued hence can release the rpm ref count now only.
> > >        intel_runtime_pm_put();
> > 
> > > 
> > > and dropping the reference at the end of the work item.
> > This will be just like the __intel_autoenable_gt_powersave
> > 
> > > This would make the flush_work() a nop in case of
> > > runtime_suspend().
> > So can call the flush_work unconditionally.
> > 
> > Hope I understood it correctly.

Yes, the above is correct except for my mistake in
handling intel_runtime_pm_get_if_in_use() returning false as discussed
below.

> > 
> Hi Imre,
> 
> You had suggested to use the below code from irq handler, suspecting 
> that intel_runtime_pm_get_if_in_use() can return false, if interrupt 
> gets handled just after device goes out of use.
> 
> 	if (intel_runtime_pm_get_if_in_use()) {
> 		if (!queue_work(log.flush_work))
> 			intel_runtime_pm_put();
> 	}
> 
> Do you mean to say that interrupt can come when rpm suspend has
> already 
> started but before the interrupt is disabled from the suspend hook ?
> Like if interrupt comes b/w 1) & 4), then runtime_pm_get_if_in_use()
> will return false.
> 1)	Autosuspend delay elapses (device is marked as suspending)
> 2)		intel_runtime_suspend
> 3)			intel_guc_suspend
> 4)				gen9_disable_guc_interrupts(dev_pri
> v);

No, it can return false anytime the last RPM reference is dropped, that
is even before the autosuspend delay elapses. But that still makes the
likelihood for a missed work item scheduling small, because 1) we want
to reduce the autosuspend delay considerably from the current 10 sec
and 2) because what you say below about the GPU actually idling before
the RPM refcount going to 0.

> If the above hypothesis is correct, then it implies that interrupt
> has 
> to come after autosuspend delay has elapsed for the above scenario to
> arise.
> 
> I think it would be unlikely for the interrupt to come so late
> because 
> device would have gone idle just before the autosuspend period
> started 
> and so no GuC submissions would have been done after that.

Right.

> So the probability of missing a work item could be very less and we
> can bear that.

I haven't looked into what is the consequence of missing a work item,
you know this better. In any case - since it is still a possibility -
if it's a problem you could still make sure in intel_guc_suspend() that
any pending work is completed by calling guc_read_update_log_buffer(),
host2guc_logbuffer_flush_complete() if necessary after disabling
interrupts in intel_guc_suspend().

--Imre

[e-mail] Aug. 18, 2016, 11:24 a.m. UTC | #8

On 8/18/2016 4:25 PM, Imre Deak wrote:
> On to, 2016-08-18 at 09:15 +0530, Goel, Akash wrote:
>>
>> On 8/17/2016 9:07 PM, Goel, Akash wrote:
>>>
>>>
>>> On 8/17/2016 6:41 PM, Imre Deak wrote:
>>>> 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:
>>>>>>>
>>
>>>>>>>> +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:
>>>>
>>> Thanks much for suggesting the alternate approach.
>>>
>>> Just to confirm whether I understood everything correctly,
>>>
>>>> in gen9_guc_irq_handler():
>>>>    WARN_ON(!intel_runtime_pm_get_if_in_use());
>>> Used WARN, as we don't expect the device to be suspended at this
>>> juncture, so intel_runtime_pm_get_if_in_use() should return true.
>>>
>>>>    if (!queue_work(log.flush_work))
>>> If queue_work returns 0, then work item is already pending, so it
>>> won't
>>> be queued hence can release the rpm ref count now only.
>>>>        intel_runtime_pm_put();
>>>
>>>>
>>>> and dropping the reference at the end of the work item.
>>> This will be just like the __intel_autoenable_gt_powersave
>>>
>>>> This would make the flush_work() a nop in case of
>>>> runtime_suspend().
>>> So can call the flush_work unconditionally.
>>>
>>> Hope I understood it correctly.
>
> Yes, the above is correct except for my mistake in
> handling intel_runtime_pm_get_if_in_use() returning false as discussed
> below.
>
>>>
>> Hi Imre,
>>
>> You had suggested to use the below code from irq handler, suspecting
>> that intel_runtime_pm_get_if_in_use() can return false, if interrupt
>> gets handled just after device goes out of use.
>>
>> 	if (intel_runtime_pm_get_if_in_use()) {
>> 		if (!queue_work(log.flush_work))
>> 			intel_runtime_pm_put();
>> 	}
>>
>> Do you mean to say that interrupt can come when rpm suspend has
>> already
>> started but before the interrupt is disabled from the suspend hook ?
>> Like if interrupt comes b/w 1) & 4), then runtime_pm_get_if_in_use()
>> will return false.
>> 1)	Autosuspend delay elapses (device is marked as suspending)
>> 2)		intel_runtime_suspend
>> 3)			intel_guc_suspend
>> 4)				gen9_disable_guc_interrupts(dev_pri
>> v);
>
> No, it can return false anytime the last RPM reference is dropped, that
> is even before the autosuspend delay elapses.

Sorry I missed that pm_runtime_get_if_in_use() will return 0 if RPM ref 
count has dropped to 0, even if device is still in runtime active state 
(as autosuspend delay has not elapsed).

 > But that still makes the
> likelihood for a missed work item scheduling small, because 1) we want
> to reduce the autosuspend delay considerably from the current 10 sec
> and 2) because what you say below about the GPU actually idling before
> the RPM refcount going to 0.
>
>> If the above hypothesis is correct, then it implies that interrupt
>> has
>> to come after autosuspend delay has elapsed for the above scenario to
>> arise.
>>
>> I think it would be unlikely for the interrupt to come so late
>> because
>> device would have gone idle just before the autosuspend period
>> started
>> and so no GuC submissions would have been done after that.
>
> Right.
>
>> So the probability of missing a work item could be very less and we
>> can bear that.
>
> I haven't looked into what is the consequence of missing a work item,
> you know this better. In any case - since it is still a possibility -
> if it's a problem you could still make sure in intel_guc_suspend() that
> any pending work is completed by calling guc_read_update_log_buffer(),
> host2guc_logbuffer_flush_complete() if necessary after disabling
> interrupts in intel_guc_suspend().
Actually ideally guc_read_update_log_buffer() and 
host2guc_logbuffer_flush_complete() should be called only if the work
item was actually missed. So will have to detect the missing of work item.

Isn't the original implementation i.e. conditional flushing of work item 
for the system suspend case, a simpler & cleaner solution ?

Best regards
Akash

>
> --Imre
>

Imre Deak Aug. 18, 2016, 12:59 p.m. UTC | #9

On to, 2016-08-18 at 16:54 +0530, Goel, Akash wrote:
> 
> On 8/18/2016 4:25 PM, Imre Deak wrote:
> > On to, 2016-08-18 at 09:15 +0530, Goel, Akash wrote:
> > > 
> > > On 8/17/2016 9:07 PM, Goel, Akash wrote:
> > > > 
> > > > 
> > > > On 8/17/2016 6:41 PM, Imre Deak wrote:
> > > > > 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:
> > > > > > > > 
> > > 
> > > > > > > > > +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:
> > > > > 
> > > > Thanks much for suggesting the alternate approach.
> > > > 
> > > > Just to confirm whether I understood everything correctly,
> > > > 
> > > > > in gen9_guc_irq_handler():
> > > > >    WARN_ON(!intel_runtime_pm_get_if_in_use());
> > > > Used WARN, as we don't expect the device to be suspended at this
> > > > juncture, so intel_runtime_pm_get_if_in_use() should return true.
> > > > 
> > > > >    if (!queue_work(log.flush_work))
> > > > If queue_work returns 0, then work item is already pending, so it
> > > > won't
> > > > be queued hence can release the rpm ref count now only.
> > > > >        intel_runtime_pm_put();
> > > > 
> > > > > 
> > > > > and dropping the reference at the end of the work item.
> > > > This will be just like the __intel_autoenable_gt_powersave
> > > > 
> > > > > This would make the flush_work() a nop in case of
> > > > > runtime_suspend().
> > > > So can call the flush_work unconditionally.
> > > > 
> > > > Hope I understood it correctly.
> > 
> > Yes, the above is correct except for my mistake in
> > handling intel_runtime_pm_get_if_in_use() returning false as discussed
> > below.
> > 
> > > > 
> > > Hi Imre,
> > > 
> > > You had suggested to use the below code from irq handler, suspecting
> > > that intel_runtime_pm_get_if_in_use() can return false, if interrupt
> > > gets handled just after device goes out of use.
> > > 
> > > 	if (intel_runtime_pm_get_if_in_use()) {
> > > 		if (!queue_work(log.flush_work))
> > > 			intel_runtime_pm_put();
> > > 	}
> > > 
> > > Do you mean to say that interrupt can come when rpm suspend has
> > > already
> > > started but before the interrupt is disabled from the suspend hook ?
> > > Like if interrupt comes b/w 1) & 4), then runtime_pm_get_if_in_use()
> > > will return false.
> > > 1)	Autosuspend delay elapses (device is marked as suspending)
> > > 2)		intel_runtime_suspend
> > > 3)			intel_guc_suspend
> > > 4)				gen9_disable_guc_interrupts(dev_pri
> > > v);
> > 
> > No, it can return false anytime the last RPM reference is dropped, that
> > is even before the autosuspend delay elapses.
> 
> Sorry I missed that pm_runtime_get_if_in_use() will return 0 if RPM ref 
> count has dropped to 0, even if device is still in runtime active state 
> (as autosuspend delay has not elapsed).
> 
>  > But that still makes the
> > likelihood for a missed work item scheduling small, because 1) we want
> > to reduce the autosuspend delay considerably from the current 10 sec
> > and 2) because what you say below about the GPU actually idling before
> > the RPM refcount going to 0.
> > 
> > > If the above hypothesis is correct, then it implies that interrupt
> > > has
> > > to come after autosuspend delay has elapsed for the above scenario to
> > > arise.
> > > 
> > > I think it would be unlikely for the interrupt to come so late
> > > because
> > > device would have gone idle just before the autosuspend period
> > > started
> > > and so no GuC submissions would have been done after that.
> > 
> > Right.
> > 
> > > So the probability of missing a work item could be very less and we
> > > can bear that.
> > 
> > I haven't looked into what is the consequence of missing a work item,
> > you know this better. In any case - since it is still a possibility -
> > if it's a problem you could still make sure in intel_guc_suspend() that
> > any pending work is completed by calling guc_read_update_log_buffer(),
> > host2guc_logbuffer_flush_complete() if necessary after disabling
> > interrupts in intel_guc_suspend().
> Actually ideally guc_read_update_log_buffer() and 
> host2guc_logbuffer_flush_complete() should be called only if the work
> item was actually missed. So will have to detect the missing of work item.

Ok. But note that missing an interrupt when runtime suspending is not
unimaginable in any case, since interrupts can get disabled (and
cleared) before they would get serviced.

> Isn't the original implementation i.e. conditional flushing of work item 
> for the system suspend case, a simpler & cleaner solution ?

Yes, perhaps, especially with the missed work item detection. How about
making the log.flush_wq freezable? Then we could forgo the flush in
both runtime and system suspend.

--Imre

[e-mail] Aug. 18, 2016, 1:47 p.m. UTC | #10

On 8/18/2016 6:29 PM, Imre Deak wrote:
> On to, 2016-08-18 at 16:54 +0530, Goel, Akash wrote:
>>
>> On 8/18/2016 4:25 PM, Imre Deak wrote:
>>> On to, 2016-08-18 at 09:15 +0530, Goel, Akash wrote:
>>>>
>>>> On 8/17/2016 9:07 PM, Goel, Akash wrote:
>>>>>
>>>>>
>>>>> On 8/17/2016 6:41 PM, Imre Deak wrote:
>>>>>> 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:
>>>>>>>>>
>>>>
>>>>>>>>>> +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:
>>>>>>
>>>>> Thanks much for suggesting the alternate approach.
>>>>>
>>>>> Just to confirm whether I understood everything correctly,
>>>>>
>>>>>> in gen9_guc_irq_handler():
>>>>>>    WARN_ON(!intel_runtime_pm_get_if_in_use());
>>>>> Used WARN, as we don't expect the device to be suspended at this
>>>>> juncture, so intel_runtime_pm_get_if_in_use() should return true.
>>>>>
>>>>>>    if (!queue_work(log.flush_work))
>>>>> If queue_work returns 0, then work item is already pending, so it
>>>>> won't
>>>>> be queued hence can release the rpm ref count now only.
>>>>>>        intel_runtime_pm_put();
>>>>>
>>>>>>
>>>>>> and dropping the reference at the end of the work item.
>>>>> This will be just like the __intel_autoenable_gt_powersave
>>>>>
>>>>>> This would make the flush_work() a nop in case of
>>>>>> runtime_suspend().
>>>>> So can call the flush_work unconditionally.
>>>>>
>>>>> Hope I understood it correctly.
>>>
>>> Yes, the above is correct except for my mistake in
>>> handling intel_runtime_pm_get_if_in_use() returning false as discussed
>>> below.
>>>
>>>>>
>>>> Hi Imre,
>>>>
>>>> You had suggested to use the below code from irq handler, suspecting
>>>> that intel_runtime_pm_get_if_in_use() can return false, if interrupt
>>>> gets handled just after device goes out of use.
>>>>
>>>> 	if (intel_runtime_pm_get_if_in_use()) {
>>>> 		if (!queue_work(log.flush_work))
>>>> 			intel_runtime_pm_put();
>>>> 	}
>>>>
>>>> Do you mean to say that interrupt can come when rpm suspend has
>>>> already
>>>> started but before the interrupt is disabled from the suspend hook ?
>>>> Like if interrupt comes b/w 1) & 4), then runtime_pm_get_if_in_use()
>>>> will return false.
>>>> 1)	Autosuspend delay elapses (device is marked as suspending)
>>>> 2)		intel_runtime_suspend
>>>> 3)			intel_guc_suspend
>>>> 4)				gen9_disable_guc_interrupts(dev_pri
>>>> v);
>>>
>>> No, it can return false anytime the last RPM reference is dropped, that
>>> is even before the autosuspend delay elapses.
>>
>> Sorry I missed that pm_runtime_get_if_in_use() will return 0 if RPM ref
>> count has dropped to 0, even if device is still in runtime active state
>> (as autosuspend delay has not elapsed).
>>
>>  > But that still makes the
>>> likelihood for a missed work item scheduling small, because 1) we want
>>> to reduce the autosuspend delay considerably from the current 10 sec
>>> and 2) because what you say below about the GPU actually idling before
>>> the RPM refcount going to 0.
>>>
>>>> If the above hypothesis is correct, then it implies that interrupt
>>>> has
>>>> to come after autosuspend delay has elapsed for the above scenario to
>>>> arise.
>>>>
>>>> I think it would be unlikely for the interrupt to come so late
>>>> because
>>>> device would have gone idle just before the autosuspend period
>>>> started
>>>> and so no GuC submissions would have been done after that.
>>>
>>> Right.
>>>
>>>> So the probability of missing a work item could be very less and we
>>>> can bear that.
>>>
>>> I haven't looked into what is the consequence of missing a work item,
>>> you know this better. In any case - since it is still a possibility -
>>> if it's a problem you could still make sure in intel_guc_suspend() that
>>> any pending work is completed by calling guc_read_update_log_buffer(),
>>> host2guc_logbuffer_flush_complete() if necessary after disabling
>>> interrupts in intel_guc_suspend().
>> Actually ideally guc_read_update_log_buffer() and
>> host2guc_logbuffer_flush_complete() should be called only if the work
>> item was actually missed. So will have to detect the missing of work item.
>
> Ok. But note that missing an interrupt when runtime suspending is not
> unimaginable in any case, since interrupts can get disabled (and
> cleared) before they would get serviced.
>
Fine, missing of interrupt will always be a possibility.

>> Isn't the original implementation i.e. conditional flushing of work item
>> for the system suspend case, a simpler & cleaner solution ?
>
> Yes, perhaps, especially with the missed work item detection. How about
> making the log.flush_wq freezable? Then we could forgo the flush in
> both runtime and system suspend.
>
Thanks for the inputs. Sorry not familiar with freezable WQ semantics.
But after looking at code, this is what I understood :-
1. freezable Workqueues will be frozen before the system suspend
    callbacks are invoked for the devices.
2. Any work item queued after the WQ is marked frozen will be scheduled
    later, on resume.
3. But if a work item was already present in the freezable Workqueue,
    before it was frozen and it did not complete, then system suspend
    itself will be aborted.
4. So if the log.flush_wq is marked as freezable, then flush of
    work item will not be required for the system suspend case.
    And runtime suspend case is already covered with rpm get/put
    around register access in work item function.

It seems there are 2 config options CONFIG_SUSPEND_FREEZER and
CONFIG_FREEZER which have to be enabled for all the above to happen.
If these config options will always be enabled then probably marking
log.flush_wq would work.

Please kindly confirm whether I understood correctly or not, accordingly 
will proceed further.

Best regards
Akash



> --Imre
>

Imre Deak Aug. 18, 2016, 2:18 p.m. UTC | #11

On to, 2016-08-18 at 19:17 +0530, Goel, Akash wrote:
> [...]
> Thanks for the inputs. Sorry not familiar with freezable WQ semantics.
> But after looking at code, this is what I understood :-
> 1. freezable Workqueues will be frozen before the system suspend
>     callbacks are invoked for the devices.

Yes.

> 2. Any work item queued after the WQ is marked frozen will be scheduled
>     later, on resume.

Yes.

> 3. But if a work item was already present in the freezable Workqueue,
>     before it was frozen and it did not complete, then system suspend
>     itself will be aborted.

System suspend will be aborted only if any kernel thread didn't
complete within a reasonable amount of time (freeze_timeout_msecs, 20
sec by default). Otherwise already queued items will be properly
waited upon and suspend will proceed.

> 4. So if the log.flush_wq is marked as freezable, then flush of
>     work item will not be required for the system suspend case.
>     And runtime suspend case is already covered with rpm get/put
>     around register access in work item function.

Yes.

> 
> It seems there are 2 config options CONFIG_SUSPEND_FREEZER

This is set whenever system suspend is enabled.

> and
> CONFIG_FREEZER 

This is set except for one platform (powerpc), where I assume freezing
of the tasks is achieved in a different way. In any case it doesn't
matter for us.

--Imre

> which have to be enabled for all the above to happen.
> If these config options will always be enabled then probably marking
> log.flush_wq would work.
> 
> Please kindly confirm whether I understood correctly or not, accordingly 
> will proceed further.
> 
> Best regards
> Akash
> 
> 
> 
> > --Imre
> >

[e-mail] Aug. 18, 2016, 2:35 p.m. UTC | #12

On 8/18/2016 7:48 PM, Imre Deak wrote:
> On to, 2016-08-18 at 19:17 +0530, Goel, Akash wrote:
>> [...]
>> Thanks for the inputs. Sorry not familiar with freezable WQ semantics.
>> But after looking at code, this is what I understood :-
>> 1. freezable Workqueues will be frozen before the system suspend
>>     callbacks are invoked for the devices.
>
> Yes.
>
>> 2. Any work item queued after the WQ is marked frozen will be scheduled
>>     later, on resume.
>
> Yes.
>
>> 3. But if a work item was already present in the freezable Workqueue,
>>     before it was frozen and it did not complete, then system suspend
>>     itself will be aborted.
>
> System suspend will be aborted only if any kernel thread didn't
> complete within a reasonable amount of time (freeze_timeout_msecs, 20
> sec by default). Otherwise already queued items will be properly
> waited upon and suspend will proceed.
Sorry for getting this wrong.
What I understood is that even if there are pending work items on
freezable WQ after freeze_timeout_msecs, then also system suspend would 
be performed.
Sorry couldn't find an explicit/synchronous wait in kernel for the 
pending work items for freezable WQs, but it doesn't matter.

>
>> 4. So if the log.flush_wq is marked as freezable, then flush of
>>     work item will not be required for the system suspend case.
>>     And runtime suspend case is already covered with rpm get/put
>>     around register access in work item function.
>
> Yes.
>
>>
>> It seems there are 2 config options CONFIG_SUSPEND_FREEZER
>
> This is set whenever system suspend is enabled.
>
>> and
>> CONFIG_FREEZER
>
> This is set except for one platform (powerpc), where I assume freezing
> of the tasks is achieved in a different way. In any case it doesn't
> matter for us.
>
Many thanks for providing all this info.

Will then mark the log.flush_wq as freezable.

Best regards
Akash
> --Imre
>
>> which have to be enabled for all the above to happen.
>> If these config options will always be enabled then probably marking
>> log.flush_wq would work.
>>
>> Please kindly confirm whether I understood correctly or not, accordingly
>> will proceed further.
>>
>> Best regards
>> Akash
>>
>>
>>
>>> --Imre
>>>

Imre Deak Aug. 18, 2016, 2:55 p.m. UTC | #13

On to, 2016-08-18 at 20:05 +0530, Goel, Akash wrote:
> 
> On 8/18/2016 7:48 PM, Imre Deak wrote:
> > On to, 2016-08-18 at 19:17 +0530, Goel, Akash wrote:
> > > [...]
> > > Thanks for the inputs. Sorry not familiar with freezable WQ semantics.
> > > But after looking at code, this is what I understood :-
> > > 1. freezable Workqueues will be frozen before the system suspend
> > >     callbacks are invoked for the devices.
> > 
> > Yes.
> > 
> > > 2. Any work item queued after the WQ is marked frozen will be scheduled
> > >     later, on resume.
> > 
> > Yes.
> > 
> > > 3. But if a work item was already present in the freezable Workqueue,
> > >     before it was frozen and it did not complete, then system suspend
> > >     itself will be aborted.
> > 
> > System suspend will be aborted only if any kernel thread didn't
> > complete within a reasonable amount of time (freeze_timeout_msecs, 20
> > sec by default). Otherwise already queued items will be properly
> > waited upon and suspend will proceed.
> Sorry for getting this wrong.
> What I understood is that even if there are pending work items on
> freezable WQ after freeze_timeout_msecs, then also system suspend would 
> be performed.

In case of timeout suspend_prepare()->suspend_freeze_processes()
->freeze_kernel_threads()->try_to_freeze_tasks() will return -EBUSY and
suspend will fail.

> Sorry couldn't find an explicit/synchronous wait in kernel for the 
> pending work items for freezable WQs, but it doesn't matter.

The above try_to_freeze_tasks() will wait until
freeze_workqueues_busy() indicates that there are no work items active
on any freezable queues.

--Imre

> 
> > 
> > > 4. So if the log.flush_wq is marked as freezable, then flush of
> > >     work item will not be required for the system suspend case.
> > >     And runtime suspend case is already covered with rpm get/put
> > >     around register access in work item function.
> > 
> > Yes.
> > 
> > > 
> > > It seems there are 2 config options CONFIG_SUSPEND_FREEZER
> > 
> > This is set whenever system suspend is enabled.
> > 
> > > and
> > > CONFIG_FREEZER
> > 
> > This is set except for one platform (powerpc), where I assume freezing
> > of the tasks is achieved in a different way. In any case it doesn't
> > matter for us.
> > 
> Many thanks for providing all this info.
> 
> Will then mark the log.flush_wq as freezable.
> 
> Best regards
> Akash
> > --Imre
> > 
> > > which have to be enabled for all the above to happen.
> > > If these config options will always be enabled then probably marking
> > > log.flush_wq would work.
> > > 
> > > Please kindly confirm whether I understood correctly or not, accordingly
> > > will proceed further.
> > > 
> > > Best regards
> > > Akash
> > > 
> > > 
> > > 
> > > > --Imre
> > > >

[e-mail] Aug. 18, 2016, 3:01 p.m. UTC | #14

On 8/18/2016 8:25 PM, Imre Deak wrote:
> On to, 2016-08-18 at 20:05 +0530, Goel, Akash wrote:
>>
>> On 8/18/2016 7:48 PM, Imre Deak wrote:
>>> On to, 2016-08-18 at 19:17 +0530, Goel, Akash wrote:
>>>> [...]
>>>> Thanks for the inputs. Sorry not familiar with freezable WQ semantics.
>>>> But after looking at code, this is what I understood :-
>>>> 1. freezable Workqueues will be frozen before the system suspend
>>>>     callbacks are invoked for the devices.
>>>
>>> Yes.
>>>
>>>> 2. Any work item queued after the WQ is marked frozen will be scheduled
>>>>     later, on resume.
>>>
>>> Yes.
>>>
>>>> 3. But if a work item was already present in the freezable Workqueue,
>>>>     before it was frozen and it did not complete, then system suspend
>>>>     itself will be aborted.
>>>
>>> System suspend will be aborted only if any kernel thread didn't
>>> complete within a reasonable amount of time (freeze_timeout_msecs, 20
>>> sec by default). Otherwise already queued items will be properly
>>> waited upon and suspend will proceed.
>> Sorry for getting this wrong.
>> What I understood is that even if there are pending work items on
>> freezable WQ after freeze_timeout_msecs, then also system suspend would
>> be performed.
>
> In case of timeout suspend_prepare()->suspend_freeze_processes()
> ->freeze_kernel_threads()->try_to_freeze_tasks() will return -EBUSY and
> suspend will fail.
>
So sorry, there was a typo in my last mail, instead of writing 'system 
suspend would be aborted', I wrote 'system suspend would be performed'.

>> Sorry couldn't find an explicit/synchronous wait in kernel for the
>> pending work items for freezable WQs, but it doesn't matter.
>
> The above try_to_freeze_tasks() will wait until
> freeze_workqueues_busy() indicates that there are no work items active
> on any freezable queues.
>
Thanks much for clarifying. I will go through that function again.

Best regards
Akash
> --Imre
>
>>
>>>
>>>> 4. So if the log.flush_wq is marked as freezable, then flush of
>>>>     work item will not be required for the system suspend case.
>>>>     And runtime suspend case is already covered with rpm get/put
>>>>     around register access in work item function.
>>>
>>> Yes.
>>>
>>>>
>>>> It seems there are 2 config options CONFIG_SUSPEND_FREEZER
>>>
>>> This is set whenever system suspend is enabled.
>>>
>>>> and
>>>> CONFIG_FREEZER
>>>
>>> This is set except for one platform (powerpc), where I assume freezing
>>> of the tasks is achieved in a different way. In any case it doesn't
>>> matter for us.
>>>
>> Many thanks for providing all this info.
>>
>> Will then mark the log.flush_wq as freezable.
>>
>> Best regards
>> Akash
>>> --Imre
>>>
>>>> which have to be enabled for all the above to happen.
>>>> If these config options will always be enabled then probably marking
>>>> log.flush_wq would work.
>>>>
>>>> Please kindly confirm whether I understood correctly or not, accordingly
>>>> will proceed further.
>>>>
>>>> Best regards
>>>> Akash
>>>>
>>>>
>>>>
>>>>> --Imre
>>>>>

Patch

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 */