[PATCH 1/1] drm/i915: Move i915_gem_suspend_gtt_mappings to i915_gem_suspend

[PATCH 1/1] drm/i915: Move i915_gem_suspend_gtt_mappings to i915_gem_suspend

[Sagar Arun Kamble]

suspend_gtt_mappings was happening post i915_gem_suspend where
GPU is reset in the end. Call it before i915_gem_sanitize.
This also makes portion of i915_gem_suspend symmetrical to
i915_gem_resume and will facilitate proper ordering of GuC resume
where ggtt.invalidate gets updated.

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 2 --
 drivers/gpu/drm/i915/i915_gem.c | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 66fc156..05c71a9 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1558,8 +1558,6 @@ static int i915_drm_suspend(struct drm_device *dev)
 
 	intel_suspend_hw(dev_priv);
 
-	i915_gem_suspend_gtt_mappings(dev_priv);
-
 	i915_save_state(dev_priv);
 
 	opregion_target_state = suspend_to_idle(dev_priv) ? PCI_D1 : PCI_D3cold;
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ab8c694..4abd6fd 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4557,6 +4557,8 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
 	if (WARN_ON(!intel_engines_are_idle(dev_priv)))
 		i915_gem_set_wedged(dev_priv); /* no hope, discard everything */
 
+	i915_gem_suspend_gtt_mappings(dev_priv);
+
 	/*
 	 * Neither the BIOS, ourselves or any other kernel
 	 * expects the system to be in execlists mode on startup,
-- 
1.9.1

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

Re: [PATCH 1/1] drm/i915: Move i915_gem_suspend_gtt_mappings to i915_gem_suspend

[Chris Wilson]

Quoting Sagar Arun Kamble (2017-10-05 19:07:37)
> suspend_gtt_mappings was happening post i915_gem_suspend where
> GPU is reset in the end. Call it before i915_gem_sanitize.
> This also makes portion of i915_gem_suspend symmetrical to
> i915_gem_resume and will facilitate proper ordering of GuC resume
> where ggtt.invalidate gets updated.
> 
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c | 2 --
>  drivers/gpu/drm/i915/i915_gem.c | 2 ++
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 66fc156..05c71a9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1558,8 +1558,6 @@ static int i915_drm_suspend(struct drm_device *dev)
>  
>         intel_suspend_hw(dev_priv);
>  
> -       i915_gem_suspend_gtt_mappings(dev_priv);
> -
>         i915_save_state(dev_priv);
>  
>         opregion_target_state = suspend_to_idle(dev_priv) ? PCI_D1 : PCI_D3cold;
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index ab8c694..4abd6fd 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4557,6 +4557,8 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
>         if (WARN_ON(!intel_engines_are_idle(dev_priv)))
>                 i915_gem_set_wedged(dev_priv); /* no hope, discard everything */
>  
> +       i915_gem_suspend_gtt_mappings(dev_priv);

Hmm. suspend_gtt_mappings zaps the GGTT. We shouldn't do that before we
shutdown the display. Maybe we need to move the i915_gem_sanitize instead?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/1] drm/i915: Move i915_gem_suspend_gtt_mappings to i915_gem_suspend

[Sagar Arun Kamble]

On 10/5/2017 11:41 PM, Chris Wilson wrote:
> Quoting Sagar Arun Kamble (2017-10-05 19:07:37)
>> suspend_gtt_mappings was happening post i915_gem_suspend where
>> GPU is reset in the end. Call it before i915_gem_sanitize.
>> This also makes portion of i915_gem_suspend symmetrical to
>> i915_gem_resume and will facilitate proper ordering of GuC resume
>> where ggtt.invalidate gets updated.
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> ---
>>   drivers/gpu/drm/i915/i915_drv.c | 2 --
>>   drivers/gpu/drm/i915/i915_gem.c | 2 ++
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index 66fc156..05c71a9 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -1558,8 +1558,6 @@ static int i915_drm_suspend(struct drm_device *dev)
>>   
>>          intel_suspend_hw(dev_priv);
>>   
>> -       i915_gem_suspend_gtt_mappings(dev_priv);
>> -
>>          i915_save_state(dev_priv);
>>   
>>          opregion_target_state = suspend_to_idle(dev_priv) ? PCI_D1 : PCI_D3cold;
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index ab8c694..4abd6fd 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -4557,6 +4557,8 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
>>          if (WARN_ON(!intel_engines_are_idle(dev_priv)))
>>                  i915_gem_set_wedged(dev_priv); /* no hope, discard everything */
>>   
>> +       i915_gem_suspend_gtt_mappings(dev_priv);
> Hmm. suspend_gtt_mappings zaps the GGTT. We shouldn't do that before we
> shutdown the display. Maybe we need to move the i915_gem_sanitize instead?
> -Chris
Any particular reason gem_suspend is being done prior to display 
suspend? ...
if we could change that order current gem_suspend (i915_gem_suspend post 
intel_suspend*) with this fix should be fine ...
Else will have to call gem_sanitize separately.

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

Re: [PATCH 1/1] drm/i915: Move i915_gem_suspend_gtt_mappings to i915_gem_suspend

[Chris Wilson]

Quoting Sagar Arun Kamble (2017-10-05 19:17:46)
> 
> 
> On 10/5/2017 11:41 PM, Chris Wilson wrote:
> > Quoting Sagar Arun Kamble (2017-10-05 19:07:37)
> >> suspend_gtt_mappings was happening post i915_gem_suspend where
> >> GPU is reset in the end. Call it before i915_gem_sanitize.
> >> This also makes portion of i915_gem_suspend symmetrical to
> >> i915_gem_resume and will facilitate proper ordering of GuC resume
> >> where ggtt.invalidate gets updated.
> >>
> >> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> >> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >> Cc: Michał Winiarski <michal.winiarski@intel.com>
> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >> ---
> >>   drivers/gpu/drm/i915/i915_drv.c | 2 --
> >>   drivers/gpu/drm/i915/i915_gem.c | 2 ++
> >>   2 files changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >> index 66fc156..05c71a9 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.c
> >> +++ b/drivers/gpu/drm/i915/i915_drv.c
> >> @@ -1558,8 +1558,6 @@ static int i915_drm_suspend(struct drm_device *dev)
> >>   
> >>          intel_suspend_hw(dev_priv);
> >>   
> >> -       i915_gem_suspend_gtt_mappings(dev_priv);
> >> -
> >>          i915_save_state(dev_priv);
> >>   
> >>          opregion_target_state = suspend_to_idle(dev_priv) ? PCI_D1 : PCI_D3cold;
> >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >> index ab8c694..4abd6fd 100644
> >> --- a/drivers/gpu/drm/i915/i915_gem.c
> >> +++ b/drivers/gpu/drm/i915/i915_gem.c
> >> @@ -4557,6 +4557,8 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
> >>          if (WARN_ON(!intel_engines_are_idle(dev_priv)))
> >>                  i915_gem_set_wedged(dev_priv); /* no hope, discard everything */
> >>   
> >> +       i915_gem_suspend_gtt_mappings(dev_priv);
> > Hmm. suspend_gtt_mappings zaps the GGTT. We shouldn't do that before we
> > shutdown the display. Maybe we need to move the i915_gem_sanitize instead?
> > -Chris
> Any particular reason gem_suspend is being done prior to display 
> suspend? ...

It grew from idling the GPU as the first action - which makes sure that
user updates to the screen stop etc.

> if we could change that order current gem_suspend (i915_gem_suspend post 
> intel_suspend*) with this fix should be fine ...
> Else will have to call gem_sanitize separately.

I guess we are looking at splitting gem_suspend into the user quiescing
section and the hw cleanup.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/1] drm/i915: Move i915_gem_suspend_gtt_mappings to i915_gem_suspend

[Sagar Arun Kamble]

On 10/5/2017 11:52 PM, Chris Wilson wrote:
> Quoting Sagar Arun Kamble (2017-10-05 19:17:46)
>>
>> On 10/5/2017 11:41 PM, Chris Wilson wrote:
>>> Quoting Sagar Arun Kamble (2017-10-05 19:07:37)
>>>> suspend_gtt_mappings was happening post i915_gem_suspend where
>>>> GPU is reset in the end. Call it before i915_gem_sanitize.
>>>> This also makes portion of i915_gem_suspend symmetrical to
>>>> i915_gem_resume and will facilitate proper ordering of GuC resume
>>>> where ggtt.invalidate gets updated.
>>>>
>>>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>> Cc: Michał Winiarski <michal.winiarski@intel.com>
>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_drv.c | 2 --
>>>>    drivers/gpu/drm/i915/i915_gem.c | 2 ++
>>>>    2 files changed, 2 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>>>> index 66fc156..05c71a9 100644
>>>> --- a/drivers/gpu/drm/i915/i915_drv.c
>>>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>>>> @@ -1558,8 +1558,6 @@ static int i915_drm_suspend(struct drm_device *dev)
>>>>    
>>>>           intel_suspend_hw(dev_priv);
>>>>    
>>>> -       i915_gem_suspend_gtt_mappings(dev_priv);
>>>> -
>>>>           i915_save_state(dev_priv);
>>>>    
>>>>           opregion_target_state = suspend_to_idle(dev_priv) ? PCI_D1 : PCI_D3cold;
>>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>>>> index ab8c694..4abd6fd 100644
>>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>>> @@ -4557,6 +4557,8 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
>>>>           if (WARN_ON(!intel_engines_are_idle(dev_priv)))
>>>>                   i915_gem_set_wedged(dev_priv); /* no hope, discard everything */
>>>>    
>>>> +       i915_gem_suspend_gtt_mappings(dev_priv);
>>> Hmm. suspend_gtt_mappings zaps the GGTT. We shouldn't do that before we
>>> shutdown the display. Maybe we need to move the i915_gem_sanitize instead?
>>> -Chris
>> Any particular reason gem_suspend is being done prior to display
>> suspend? ...
> It grew from idling the GPU as the first action - which makes sure that
> user updates to the screen stop etc.
>
>> if we could change that order current gem_suspend (i915_gem_suspend post
>> intel_suspend*) with this fix should be fine ...
>> Else will have to call gem_sanitize separately.
> I guess we are looking at splitting gem_suspend into the user quiescing
> section and the hw cleanup.
Ok. Will split then.
> -Chris

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

[PATCH v2 1/1] drm/i915: Split i915_gem_suspend into user quiescing and HW cleanup/suspend

[Sagar Arun Kamble]

GTT mappings are to be suspended post i915_gem_suspend and display
suspend. GPU is reset in i915_gem_suspend. This separated the HW suspend
across reset. To achieve the complete HW cleanup prior to GPU reset we
need to separate i915_gem_suspend functionality into user quiescing
and HW cleanup. With this patch new function i915_gem_quiescent is created
that will ensure GEM is idle. i915_gem_suspend will ensure all GPU HW
operations are suspended.

With this patch drm suspend sequence is gem_quiescent followed by
gem_suspend. Unload sequence is gem_quiescent->gem_fini->gem_suspend.
This unload sequence ensures GuC HW access to destroy client in gem_fini
succeeds.

v2. Split i915_gem_suspend to accomodate GTT mappings suspend as that is
not to be done till display suspend. (Chris)

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 10 ++++++----
 drivers/gpu/drm/i915/i915_drv.h |  3 ++-
 drivers/gpu/drm/i915/i915_gem.c | 34 +++++++++++++++++++++++++---------
 3 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 66fc156..875ce94 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -693,9 +693,10 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	return 0;
 
 cleanup_gem:
-	if (i915_gem_suspend(dev_priv))
+	if (i915_gem_quiescent(dev_priv))
 		DRM_ERROR("failed to idle hardware; continuing to unload!\n");
 	i915_gem_fini(dev_priv);
+	i915_gem_suspend(dev_priv);
 cleanup_uc:
 	intel_uc_fini_fw(dev_priv);
 cleanup_irq:
@@ -1395,7 +1396,7 @@ void i915_driver_unload(struct drm_device *dev)
 
 	i915_driver_unregister(dev_priv);
 
-	if (i915_gem_suspend(dev_priv))
+	if (i915_gem_quiescent(dev_priv))
 		DRM_ERROR("failed to idle hardware; continuing to unload!\n");
 
 	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
@@ -1430,6 +1431,7 @@ void i915_driver_unload(struct drm_device *dev)
 	i915_reset_error_state(dev_priv);
 
 	i915_gem_fini(dev_priv);
+	i915_gem_suspend(dev_priv);
 	intel_uc_fini_fw(dev_priv);
 	intel_fbc_cleanup_cfb(dev_priv);
 
@@ -1540,7 +1542,7 @@ static int i915_drm_suspend(struct drm_device *dev)
 
 	pci_save_state(pdev);
 
-	error = i915_gem_suspend(dev_priv);
+	error = i915_gem_quiescent(dev_priv);
 	if (error) {
 		dev_err(&pdev->dev,
 			"GEM idle failed, resume might fail\n");
@@ -1558,7 +1560,7 @@ static int i915_drm_suspend(struct drm_device *dev)
 
 	intel_suspend_hw(dev_priv);
 
-	i915_gem_suspend_gtt_mappings(dev_priv);
+	i915_gem_suspend(dev_priv);
 
 	i915_save_state(dev_priv);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1fc7080..982e37e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3688,7 +3688,8 @@ void i915_gem_reset_engine(struct intel_engine_cs *engine,
 void i915_gem_cleanup_engines(struct drm_i915_private *dev_priv);
 int i915_gem_wait_for_idle(struct drm_i915_private *dev_priv,
 			   unsigned int flags);
-int __must_check i915_gem_suspend(struct drm_i915_private *dev_priv);
+int __must_check i915_gem_quiescent(struct drm_i915_private *dev_priv);
+void i915_gem_suspend(struct drm_i915_private *dev_priv);
 void i915_gem_resume(struct drm_i915_private *dev_priv);
 int i915_gem_fault(struct vm_fault *vmf);
 int i915_gem_object_wait(struct drm_i915_gem_object *obj,
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ab8c694..d5ca218 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4507,13 +4507,12 @@ void i915_gem_sanitize(struct drm_i915_private *i915)
 	}
 }
 
-int i915_gem_suspend(struct drm_i915_private *dev_priv)
+int i915_gem_quiescent(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = &dev_priv->drm;
 	int ret;
 
 	intel_runtime_pm_get(dev_priv);
-	intel_suspend_gt_powersave(dev_priv);
 
 	mutex_lock(&dev->struct_mutex);
 
@@ -4539,8 +4538,6 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
 	i915_gem_contexts_lost(dev_priv);
 	mutex_unlock(&dev->struct_mutex);
 
-	intel_guc_suspend(dev_priv);
-
 	cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
 	cancel_delayed_work_sync(&dev_priv->gt.retire_work);
 
@@ -4557,6 +4554,29 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
 	if (WARN_ON(!intel_engines_are_idle(dev_priv)))
 		i915_gem_set_wedged(dev_priv); /* no hope, discard everything */
 
+	intel_runtime_pm_put(dev_priv);
+	return 0;
+
+err_unlock:
+	mutex_unlock(&dev->struct_mutex);
+
+	intel_runtime_pm_put(dev_priv);
+	return ret;
+}
+
+void i915_gem_suspend(struct drm_i915_private *dev_priv)
+{
+	struct drm_device *dev = &dev_priv->drm;
+
+	intel_runtime_pm_get(dev_priv);
+	intel_suspend_gt_powersave(dev_priv);
+
+	mutex_lock(&dev->struct_mutex);
+
+	i915_gem_suspend_gtt_mappings(dev_priv);
+
+	intel_guc_suspend(dev_priv);
+
 	/*
 	 * Neither the BIOS, ourselves or any other kernel
 	 * expects the system to be in execlists mode on startup,
@@ -4578,13 +4598,9 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
 	 */
 	i915_gem_sanitize(dev_priv);
 
-	intel_runtime_pm_put(dev_priv);
-	return 0;
-
-err_unlock:
 	mutex_unlock(&dev->struct_mutex);
+
 	intel_runtime_pm_put(dev_priv);
-	return ret;
 }
 
 void i915_gem_resume(struct drm_i915_private *dev_priv)
-- 
1.9.1

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

Re: [PATCH v2 1/1] drm/i915: Split i915_gem_suspend into user quiescing and HW cleanup/suspend

[Sagar Arun Kamble]

On 10/6/2017 12:07 PM, Sagar Arun Kamble wrote:
> GTT mappings are to be suspended post i915_gem_suspend and display
I think I should say this "post idling GPU and display suspend". Will 
update in next rev.
> suspend. GPU is reset in i915_gem_suspend. This separated the HW suspend
> across reset. To achieve the complete HW cleanup prior to GPU reset we
> need to separate i915_gem_suspend functionality into user quiescing
> and HW cleanup. With this patch new function i915_gem_quiescent
And this should have been named "i915_gem_quiesce"?
Chris, Joonas, does i915_gem_quiesce  for user quiescing and 
i915_gem_suspend for HW suspend look good?
>   is created
> that will ensure GEM is idle. i915_gem_suspend will ensure all GPU HW
> operations are suspended.
>
> With this patch drm suspend sequence is gem_quiescent followed by
> gem_suspend. Unload sequence is gem_quiescent->gem_fini->gem_suspend.
> This unload sequence ensures GuC HW access to destroy client in gem_fini
> succeeds.
>
> v2. Split i915_gem_suspend to accomodate GTT mappings suspend as that is
> not to be done till display suspend. (Chris)
>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.c | 10 ++++++----
>   drivers/gpu/drm/i915/i915_drv.h |  3 ++-
>   drivers/gpu/drm/i915/i915_gem.c | 34 +++++++++++++++++++++++++---------
>   3 files changed, 33 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 66fc156..875ce94 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -693,9 +693,10 @@ static int i915_load_modeset_init(struct drm_device *dev)
>   	return 0;
>   
>   cleanup_gem:
> -	if (i915_gem_suspend(dev_priv))
> +	if (i915_gem_quiescent(dev_priv))
>   		DRM_ERROR("failed to idle hardware; continuing to unload!\n");
>   	i915_gem_fini(dev_priv);
> +	i915_gem_suspend(dev_priv);
>   cleanup_uc:
>   	intel_uc_fini_fw(dev_priv);
>   cleanup_irq:
> @@ -1395,7 +1396,7 @@ void i915_driver_unload(struct drm_device *dev)
>   
>   	i915_driver_unregister(dev_priv);
>   
> -	if (i915_gem_suspend(dev_priv))
> +	if (i915_gem_quiescent(dev_priv))
>   		DRM_ERROR("failed to idle hardware; continuing to unload!\n");
>   
>   	intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> @@ -1430,6 +1431,7 @@ void i915_driver_unload(struct drm_device *dev)
>   	i915_reset_error_state(dev_priv);
>   
>   	i915_gem_fini(dev_priv);
> +	i915_gem_suspend(dev_priv);
>   	intel_uc_fini_fw(dev_priv);
>   	intel_fbc_cleanup_cfb(dev_priv);
>   
> @@ -1540,7 +1542,7 @@ static int i915_drm_suspend(struct drm_device *dev)
>   
>   	pci_save_state(pdev);
>   
> -	error = i915_gem_suspend(dev_priv);
> +	error = i915_gem_quiescent(dev_priv);
>   	if (error) {
>   		dev_err(&pdev->dev,
>   			"GEM idle failed, resume might fail\n");
> @@ -1558,7 +1560,7 @@ static int i915_drm_suspend(struct drm_device *dev)
>   
>   	intel_suspend_hw(dev_priv);
>   
> -	i915_gem_suspend_gtt_mappings(dev_priv);
> +	i915_gem_suspend(dev_priv);
>   
>   	i915_save_state(dev_priv);
>   
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 1fc7080..982e37e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3688,7 +3688,8 @@ void i915_gem_reset_engine(struct intel_engine_cs *engine,
>   void i915_gem_cleanup_engines(struct drm_i915_private *dev_priv);
>   int i915_gem_wait_for_idle(struct drm_i915_private *dev_priv,
>   			   unsigned int flags);
> -int __must_check i915_gem_suspend(struct drm_i915_private *dev_priv);
> +int __must_check i915_gem_quiescent(struct drm_i915_private *dev_priv);
> +void i915_gem_suspend(struct drm_i915_private *dev_priv);
>   void i915_gem_resume(struct drm_i915_private *dev_priv);
>   int i915_gem_fault(struct vm_fault *vmf);
>   int i915_gem_object_wait(struct drm_i915_gem_object *obj,
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index ab8c694..d5ca218 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4507,13 +4507,12 @@ void i915_gem_sanitize(struct drm_i915_private *i915)
>   	}
>   }
>   
> -int i915_gem_suspend(struct drm_i915_private *dev_priv)
> +int i915_gem_quiescent(struct drm_i915_private *dev_priv)
>   {
>   	struct drm_device *dev = &dev_priv->drm;
>   	int ret;
>   
>   	intel_runtime_pm_get(dev_priv);
> -	intel_suspend_gt_powersave(dev_priv);
>   
>   	mutex_lock(&dev->struct_mutex);
>   
> @@ -4539,8 +4538,6 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
>   	i915_gem_contexts_lost(dev_priv);
>   	mutex_unlock(&dev->struct_mutex);
>   
> -	intel_guc_suspend(dev_priv);
> -
>   	cancel_delayed_work_sync(&dev_priv->gpu_error.hangcheck_work);
>   	cancel_delayed_work_sync(&dev_priv->gt.retire_work);
>   
> @@ -4557,6 +4554,29 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
>   	if (WARN_ON(!intel_engines_are_idle(dev_priv)))
>   		i915_gem_set_wedged(dev_priv); /* no hope, discard everything */
>   
> +	intel_runtime_pm_put(dev_priv);
> +	return 0;
> +
> +err_unlock:
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	intel_runtime_pm_put(dev_priv);
> +	return ret;
> +}
> +
> +void i915_gem_suspend(struct drm_i915_private *dev_priv)
> +{
> +	struct drm_device *dev = &dev_priv->drm;
> +
> +	intel_runtime_pm_get(dev_priv);
> +	intel_suspend_gt_powersave(dev_priv);
> +
> +	mutex_lock(&dev->struct_mutex);
> +
> +	i915_gem_suspend_gtt_mappings(dev_priv);
> +
> +	intel_guc_suspend(dev_priv);
> +
>   	/*
>   	 * Neither the BIOS, ourselves or any other kernel
>   	 * expects the system to be in execlists mode on startup,
> @@ -4578,13 +4598,9 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
>   	 */
>   	i915_gem_sanitize(dev_priv);
>   
> -	intel_runtime_pm_put(dev_priv);
> -	return 0;
> -
> -err_unlock:
>   	mutex_unlock(&dev->struct_mutex);
> +
>   	intel_runtime_pm_put(dev_priv);
> -	return ret;
>   }
>   
>   void i915_gem_resume(struct drm_i915_private *dev_priv)

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

✗ Fi.CI.BAT: failure for series starting with [v2,1/1] drm/i915: Split i915_gem_suspend into user quiescing and HW cleanup/suspend (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [v2,1/1] drm/i915: Split i915_gem_suspend into user quiescing and HW cleanup/suspend (rev2)
URL   : https://patchwork.freedesktop.org/series/31450/
State : failure

== Summary ==

Series 31450v2 series starting with [v2,1/1] drm/i915: Split i915_gem_suspend into user quiescing and HW cleanup/suspend
https://patchwork.freedesktop.org/api/1.0/series/31450/revisions/2/mbox/

Test gem_exec_suspend:
        Subgroup basic-s3:
                pass       -> DMESG-WARN (fi-cfl-s) fdo#103026
Test drv_module_reload:
        Subgroup basic-reload:
                pass       -> DMESG-FAIL (fi-snb-2520m)
                pass       -> DMESG-FAIL (fi-snb-2600)
                pass       -> DMESG-FAIL (fi-ivb-3520m)
                pass       -> DMESG-FAIL (fi-ivb-3770)
                pass       -> INCOMPLETE (fi-byt-j1900)
                pass       -> DMESG-FAIL (fi-byt-n2820)
                pass       -> DMESG-FAIL (fi-hsw-4770)
                pass       -> DMESG-FAIL (fi-hsw-4770r)
                pass       -> DMESG-FAIL (fi-bdw-5557u)
                pass       -> DMESG-FAIL (fi-bdw-gvtdvm)
                pass       -> DMESG-FAIL (fi-bsw-n3050)
                pass       -> DMESG-FAIL (fi-skl-6260u)
                pass       -> DMESG-FAIL (fi-skl-6700hq)
                pass       -> DMESG-FAIL (fi-skl-6700k)
                pass       -> DMESG-FAIL (fi-skl-6770hq)
                pass       -> DMESG-FAIL (fi-skl-gvtdvm)
                pass       -> DMESG-FAIL (fi-bxt-dsi)
                pass       -> DMESG-FAIL (fi-bxt-j4205)
                pass       -> DMESG-FAIL (fi-kbl-7500u)
                pass       -> DMESG-FAIL (fi-kbl-7560u)
                dmesg-warn -> DMESG-FAIL (fi-kbl-7567u) fdo#102294 +2
                pass       -> DMESG-FAIL (fi-kbl-r)
                pass       -> DMESG-FAIL (fi-glk-1)
                pass       -> DMESG-FAIL (fi-cfl-s)
                pass       -> DMESG-FAIL (fi-cnl-y)
        Subgroup basic-no-display:
                pass       -> FAIL       (fi-snb-2520m)
                pass       -> FAIL       (fi-snb-2600)
                pass       -> FAIL       (fi-ivb-3520m)
                pass       -> FAIL       (fi-ivb-3770)
                pass       -> FAIL       (fi-byt-n2820)
                pass       -> FAIL       (fi-hsw-4770)
                pass       -> FAIL       (fi-hsw-4770r)
                pass       -> FAIL       (fi-bdw-5557u)
                pass       -> FAIL       (fi-bdw-gvtdvm)
                pass       -> FAIL       (fi-bsw-n3050)
                pass       -> FAIL       (fi-skl-6260u)
                pass       -> FAIL       (fi-skl-6700hq)
                pass       -> FAIL       (fi-skl-6700k)
                pass       -> FAIL       (fi-skl-6770hq)
                pass       -> FAIL       (fi-skl-gvtdvm)
                pass       -> FAIL       (fi-bxt-dsi)
                pass       -> FAIL       (fi-bxt-j4205)
                pass       -> FAIL       (fi-kbl-7500u)
                pass       -> FAIL       (fi-kbl-7560u)
                pass       -> FAIL       (fi-kbl-r)
                pass       -> FAIL       (fi-glk-1)
                pass       -> FAIL       (fi-cfl-s) fdo#103022 +1
                pass       -> FAIL       (fi-cnl-y)
        Subgroup basic-reload-inject:
                pass       -> FAIL       (fi-snb-2520m)
                pass       -> FAIL       (fi-snb-2600)
                pass       -> FAIL       (fi-ivb-3520m)
                pass       -> FAIL       (fi-ivb-3770)
                pass       -> FAIL       (fi-byt-n2820)
                pass       -> FAIL       (fi-hsw-4770)
                pass       -> FAIL       (fi-hsw-4770r)
                pass       -> FAIL       (fi-bdw-5557u)
                pass       -> FAIL       (fi-bdw-gvtdvm)
                pass       -> FAIL       (fi-bsw-n3050)
                pass       -> FAIL       (fi-skl-6260u)
                pass       -> FAIL       (fi-skl-6700hq)
                pass       -> FAIL       (fi-skl-6700k)
                pass       -> FAIL       (fi-skl-6770hq)
                pass       -> FAIL       (fi-skl-gvtdvm)
                pass       -> FAIL       (fi-bxt-dsi)
                pass       -> FAIL       (fi-bxt-j4205)
                pass       -> FAIL       (fi-kbl-7500u)
                pass       -> FAIL       (fi-kbl-7560u)
                pass       -> FAIL       (fi-kbl-r)
                pass       -> FAIL       (fi-glk-1)
                pass       -> FAIL       (fi-cnl-y)

fdo#103026 https://bugs.freedesktop.org/show_bug.cgi?id=103026
fdo#102294 https://bugs.freedesktop.org/show_bug.cgi?id=102294
fdo#103022 https://bugs.freedesktop.org/show_bug.cgi?id=103022

fi-bdw-5557u     total:289  pass:265  dwarn:0   dfail:1   fail:2   skip:21  time:452s
fi-bdw-gvtdvm    total:289  pass:262  dwarn:0   dfail:1   fail:2   skip:24  time:468s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:393s
fi-bsw-n3050     total:289  pass:240  dwarn:0   dfail:1   fail:2   skip:46  time:555s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:287s
fi-bxt-dsi       total:289  pass:256  dwarn:0   dfail:1   fail:2   skip:30  time:520s
fi-bxt-j4205     total:289  pass:257  dwarn:0   dfail:1   fail:2   skip:29  time:529s
fi-byt-j1900     total:286  pass:250  dwarn:1   dfail:0   fail:0   skip:34 
fi-byt-n2820     total:289  pass:246  dwarn:1   dfail:1   fail:2   skip:39  time:523s
fi-cfl-s         total:289  pass:253  dwarn:1   dfail:1   fail:2   skip:32  time:531s
fi-cnl-y         total:289  pass:259  dwarn:0   dfail:1   fail:2   skip:27  time:604s
fi-glk-1         total:289  pass:258  dwarn:0   dfail:1   fail:2   skip:28  time:585s
fi-hsw-4770      total:289  pass:259  dwarn:0   dfail:1   fail:2   skip:27  time:430s
fi-hsw-4770r     total:289  pass:259  dwarn:0   dfail:1   fail:2   skip:27  time:412s
fi-ivb-3520m     total:289  pass:257  dwarn:0   dfail:1   fail:2   skip:29  time:505s
WARNING: Long output truncated

97c9e99b242fe40bbda48ba2bcaed07c47fba085 drm-tip: 2017y-10m-06d-09h-07m-21s UTC integration manifest
c75b40bb75e3 drm/i915: Split i915_gem_suspend into user quiescing and HW cleanup/suspend

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5921/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/1] drm/i915: Split i915_gem_suspend into user quiescing and HW cleanup/suspend

[Joonas Lahtinen]

On Fri, 2017-10-06 at 13:39 +0530, Sagar Arun Kamble wrote:
> 
> On 10/6/2017 12:07 PM, Sagar Arun Kamble wrote:
> > GTT mappings are to be suspended post i915_gem_suspend and display
> 
> I think I should say this "post idling GPU and display suspend". Will 
> update in next rev.
> > suspend. GPU is reset in i915_gem_suspend. This separated the HW suspend
> > across reset. To achieve the complete HW cleanup prior to GPU reset we
> > need to separate i915_gem_suspend functionality into user quiescing
> > and HW cleanup. With this patch new function i915_gem_quiescent
> 
> And this should have been named "i915_gem_quiesce"?
> Chris, Joonas, does i915_gem_quiesce  for user quiescing and 
> i915_gem_suspend for HW suspend look good?

The BAT results seemed quite negative.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v2 1/1] drm/i915: Split i915_gem_suspend into user quiescing and HW cleanup/suspend

[Sagar Arun Kamble]

On 10/12/2017 2:43 PM, Joonas Lahtinen wrote:
> On Fri, 2017-10-06 at 13:39 +0530, Sagar Arun Kamble wrote:
>> On 10/6/2017 12:07 PM, Sagar Arun Kamble wrote:
>>> GTT mappings are to be suspended post i915_gem_suspend and display
>> I think I should say this "post idling GPU and display suspend". Will
>> update in next rev.
>>> suspend. GPU is reset in i915_gem_suspend. This separated the HW suspend
>>> across reset. To achieve the complete HW cleanup prior to GPU reset we
>>> need to separate i915_gem_suspend functionality into user quiescing
>>> and HW cleanup. With this patch new function i915_gem_quiescent
>> And this should have been named "i915_gem_quiesce"?
>> Chris, Joonas, does i915_gem_quiesce  for user quiescing and
>> i915_gem_suspend for HW suspend look good?
> The BAT results seemed quite negative.
Yes. Addressed as part of GEM/GuC fixes series.
>
> Regards, Joonas

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