diff mbox

[v12,04/11] drm/i915: Create uC runtime and system suspend/resume helpers

Message ID 1506581329-29720-5-git-send-email-sagar.a.kamble@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

sagar.a.kamble@intel.com Sept. 28, 2017, 6:48 a.m. UTC
Prepared generic helpers intel_uc_suspend, intel_uc_resume,
intel_uc_runtime_suspend, intel_uc_runtime_resume. These are
called from respective GEM functions.
Only exception is intel_uc_resume that needs to be called
w/ or w/o GuC loaded in i915_drm_resume path. Changes to
add WOPCM condition check to load GuC during resume will be added
in later patches.

v2: Rebase w.r.t removal of GuC code restructuring.

v3: Calling intel_uc_resume from i915_gem_resume post resuming
i915 gem setup. This is symmetrical with i915_gem_suspend.
Removed error messages from i915 suspend/resume routines as
uC suspend/resume routines will have those. (Michal Wajdeczko)
Declare wedged on uc_suspend failure and uc_resume failure.
(Michał Winiarski)
Keeping the uC suspend/resume function definitions close to other
uC functions.

v4: Added implementation to intel_uc_resume as GuC resume is
needed to be triggered post reloading the firmware as well. Added
comments about semantics of GuC resume with the firmware reload.

v5: Updated return from i915_gem_runtime_suspend. Moved the comment
about GuC reload optimization to intel_uc_init_hw. (Michal Wajdeczko)
Updated comments as FIXME.

Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Michał Winiarski <michal.winiarski@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 12 ++++++++++++
 drivers/gpu/drm/i915/i915_gem.c | 24 +++++++++++++++++++-----
 drivers/gpu/drm/i915/intel_uc.c | 27 +++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_uc.h |  4 ++++
 4 files changed, 62 insertions(+), 5 deletions(-)

Comments

Joonas Lahtinen Sept. 29, 2017, 12:12 p.m. UTC | #1
+ Michal,

On the principle of code motion first, changes second, I'd like to see
the clean split-up from Michal before touching the files much. That way
 git history will be easier to examine.

Few comments below.

On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble wrote:
> Prepared generic helpers intel_uc_suspend, intel_uc_resume,
> intel_uc_runtime_suspend, intel_uc_runtime_resume. These are
> called from respective GEM functions.
> Only exception is intel_uc_resume that needs to be called
> w/ or w/o GuC loaded in i915_drm_resume path. Changes to
> add WOPCM condition check to load GuC during resume will be added
> in later patches.
> 
> v2: Rebase w.r.t removal of GuC code restructuring.
> 
> v3: Calling intel_uc_resume from i915_gem_resume post resuming
> i915 gem setup. This is symmetrical with i915_gem_suspend.
> Removed error messages from i915 suspend/resume routines as
> uC suspend/resume routines will have those. (Michal Wajdeczko)
> Declare wedged on uc_suspend failure and uc_resume failure.
> (Michał Winiarski)
> Keeping the uC suspend/resume function definitions close to other
> uC functions.
> 
> v4: Added implementation to intel_uc_resume as GuC resume is
> needed to be triggered post reloading the firmware as well. Added
> comments about semantics of GuC resume with the firmware reload.
> 
> v5: Updated return from i915_gem_runtime_suspend. Moved the comment
> about GuC reload optimization to intel_uc_init_hw. (Michal Wajdeczko)
> Updated comments as FIXME.
> 
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michał Winiarski <michal.winiarski@intel.com>

<SNIP>

> @@ -1698,6 +1698,18 @@ static int i915_drm_resume(struct drm_device *dev)
>  	}
>  	mutex_unlock(&dev->struct_mutex);
>  
> +	/*
> +	 * FIXME: Currently we know that at the end of suspend we have done Full

I think "beginning of resume" is the one that bites us more.

> +	 * GPU reset and GuC is loaded again during i915_gem_init_hw.
> +	 * Now, send action to GuC to resume back again as earlier call to
> +	 * intel_uc_resume from i915_gem_resume would have done nothing.
> +	 * This needs to be skipped if GuC was not loaded during resume as
> +	 * intel_uc_resume would have been already called from i915_gem_resume.
> +	 */
> +	ret = intel_uc_resume(dev_priv);
> +	if (ret)
> +		i915_gem_set_wedged(dev_priv);

I'm very sure we want to bring the system up with a backup of ELSP only
submission *even* if we used GuC when going to sleep.

> @@ -4571,7 +4573,9 @@ 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_guc_suspend(dev_priv);
> +	ret = intel_uc_suspend(dev_priv);
> +	if (ret)
> +		i915_gem_set_wedged(dev_priv); /* no hope, discard everything */

Isn't that an exxaragation when we still may have functional ELSP?

It may not be a bad idea to first bring up the ELSP submission and when
GuC has loaded, switch over. To get rid of the latency.

> @@ -4619,7 +4624,16 @@ int i915_gem_resume(struct drm_i915_private *dev_priv)
>  	 */
>  	dev_priv->gt.resume(dev_priv);
>  
> -	intel_guc_resume(dev_priv);
> +	/*
> +	 * FIXME: At the end of suspend, Full GPU reset is done which unloads
> +	 * the GuC firmware. If reset is avoided there, we can check the WOPCM
> +	 * status here to see if GuC is still loaded and just do GuC resume
> +	 * without reloading the firmware back.
> +	 */

Again, we're resetting at both directions, going to suspend and on
resume too. I'd classify these bot as more of a TODOs, because we only
get some latency that can be improved on, no broken behaviour to fix.

Regards, Joonas
sagar.a.kamble@intel.com Sept. 29, 2017, 2:13 p.m. UTC | #2
On 9/29/2017 5:42 PM, Joonas Lahtinen wrote:
> + Michal,
>
> On the principle of code motion first, changes second, I'd like to see
> the clean split-up from Michal before touching the files much. That way
>   git history will be easier to examine.
I think we wanted to get these fixes in prior to restructuring as that 
might take time.
Currently this v12 series has some bit of restructuring included along 
with fixes which has increased patches.
I have separated that today with new series. Will get that in first and 
then follow with fixes series or fixes series based on Michal's split-up 
series.
This is the minor restructuring series that is required for fixes 
series: https://patchwork.freedesktop.org/series/31140/

Michal, please suggest.

>
> Few comments below.
>
> On Thu, 2017-09-28 at 12:18 +0530, Sagar Arun Kamble wrote:
>> Prepared generic helpers intel_uc_suspend, intel_uc_resume,
>> intel_uc_runtime_suspend, intel_uc_runtime_resume. These are
>> called from respective GEM functions.
>> Only exception is intel_uc_resume that needs to be called
>> w/ or w/o GuC loaded in i915_drm_resume path. Changes to
>> add WOPCM condition check to load GuC during resume will be added
>> in later patches.
>>
>> v2: Rebase w.r.t removal of GuC code restructuring.
>>
>> v3: Calling intel_uc_resume from i915_gem_resume post resuming
>> i915 gem setup. This is symmetrical with i915_gem_suspend.
>> Removed error messages from i915 suspend/resume routines as
>> uC suspend/resume routines will have those. (Michal Wajdeczko)
>> Declare wedged on uc_suspend failure and uc_resume failure.
>> (Michał Winiarski)
>> Keeping the uC suspend/resume function definitions close to other
>> uC functions.
>>
>> v4: Added implementation to intel_uc_resume as GuC resume is
>> needed to be triggered post reloading the firmware as well. Added
>> comments about semantics of GuC resume with the firmware reload.
>>
>> v5: Updated return from i915_gem_runtime_suspend. Moved the comment
>> about GuC reload optimization to intel_uc_init_hw. (Michal Wajdeczko)
>> Updated comments as FIXME.
>>
>> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Michał Winiarski <michal.winiarski@intel.com>
> <SNIP>
>
>> @@ -1698,6 +1698,18 @@ static int i915_drm_resume(struct drm_device *dev)
>>   	}
>>   	mutex_unlock(&dev->struct_mutex);
>>   
>> +	/*
>> +	 * FIXME: Currently we know that at the end of suspend we have done Full
> I think "beginning of resume" is the one that bites us more.
Yes. I have actually rephrased these comments in my next rev but its 
held up before below series is merged:
https://patchwork.freedesktop.org/series/31140/
>
>> +	 * GPU reset and GuC is loaded again during i915_gem_init_hw.
>> +	 * Now, send action to GuC to resume back again as earlier call to
>> +	 * intel_uc_resume from i915_gem_resume would have done nothing.
>> +	 * This needs to be skipped if GuC was not loaded during resume as
>> +	 * intel_uc_resume would have been already called from i915_gem_resume.
>> +	 */
>> +	ret = intel_uc_resume(dev_priv);
>> +	if (ret)
>> +		i915_gem_set_wedged(dev_priv);
> I'm very sure we want to bring the system up with a backup of ELSP only
> submission *even* if we used GuC when going to sleep.
Yes. In the next rev I am removing this wedged.
>
>> @@ -4571,7 +4573,9 @@ 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_guc_suspend(dev_priv);
>> +	ret = intel_uc_suspend(dev_priv);
>> +	if (ret)
>> +		i915_gem_set_wedged(dev_priv); /* no hope, discard everything */
> Isn't that an exxaragation when we still may have functional ELSP?
>
> It may not be a bad idea to first bring up the ELSP submission and when
> GuC has loaded, switch over. To get rid of the latency.
Yes. will be removing this wedged.
>
>> @@ -4619,7 +4624,16 @@ int i915_gem_resume(struct drm_i915_private *dev_priv)
>>   	 */
>>   	dev_priv->gt.resume(dev_priv);
>>   
>> -	intel_guc_resume(dev_priv);
>> +	/*
>> +	 * FIXME: At the end of suspend, Full GPU reset is done which unloads
>> +	 * the GuC firmware. If reset is avoided there, we can check the WOPCM
>> +	 * status here to see if GuC is still loaded and just do GuC resume
>> +	 * without reloading the firmware back.
>> +	 */
> Again, we're resetting at both directions, going to suspend and on
> resume too. I'd classify these bot as more of a TODOs, because we only
> get some latency that can be improved on, no broken behaviour to fix.
Yes. Will mark these as TODO.
Thanks for the review.
> Regards, Joonas
Michal Wajdeczko Sept. 29, 2017, 2:39 p.m. UTC | #3
On Fri, 29 Sep 2017 16:13:54 +0200, Sagar Arun Kamble  
<sagar.a.kamble@intel.com> wrote:

>
>
> On 9/29/2017 5:42 PM, Joonas Lahtinen wrote:
>> + Michal,
>>
>> On the principle of code motion first, changes second, I'd like to see
>> the clean split-up from Michal before touching the files much. That way
>>   git history will be easier to examine.
> I think we wanted to get these fixes in prior to restructuring as that  
> might take time.
> Currently this v12 series has some bit of restructuring included along  
> with fixes which has increased patches.
> I have separated that today with new series. Will get that in first and  
> then follow with fixes series or fixes series based on Michal's split-up  
> series.
> This is the minor restructuring series that is required for fixes  
> series: https://patchwork.freedesktop.org/series/31140/
>
> Michal, please suggest.

Joonas suggestion was to start with all code moves first, then apply your
fixes from your changes to preserve better git history. To speed up I've
already started new 'moves-only' series that should be ready by EOD.

Michal
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index a9821ae..66701ee 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1698,6 +1698,18 @@  static int i915_drm_resume(struct drm_device *dev)
 	}
 	mutex_unlock(&dev->struct_mutex);
 
+	/*
+	 * FIXME: Currently we know that at the end of suspend we have done Full
+	 * GPU reset and GuC is loaded again during i915_gem_init_hw.
+	 * Now, send action to GuC to resume back again as earlier call to
+	 * intel_uc_resume from i915_gem_resume would have done nothing.
+	 * This needs to be skipped if GuC was not loaded during resume as
+	 * intel_uc_resume would have been already called from i915_gem_resume.
+	 */
+	ret = intel_uc_resume(dev_priv);
+	if (ret)
+		i915_gem_set_wedged(dev_priv);
+
 	intel_modeset_init_hw(dev);
 
 	spin_lock_irq(&dev_priv->irq_lock);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 889b35f..1f61d9c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2025,9 +2025,11 @@  int i915_gem_fault(struct vm_fault *vmf)
 int i915_gem_runtime_suspend(struct drm_i915_private *dev_priv)
 {
 	struct drm_i915_gem_object *obj, *on;
-	int i;
+	int i, ret;
 
-	intel_guc_suspend(dev_priv);
+	ret = intel_uc_runtime_suspend(dev_priv);
+	if (ret)
+		return ret;
 
 	/*
 	 * Only called during RPM suspend. All users of the userfault_list
@@ -2080,7 +2082,7 @@  void i915_gem_runtime_resume(struct drm_i915_private *dev_priv)
 	i915_gem_init_swizzling(dev_priv);
 	i915_gem_restore_fences(dev_priv);
 
-	intel_guc_resume(dev_priv);
+	intel_uc_runtime_resume(dev_priv);
 }
 
 static int i915_gem_object_create_mmap_offset(struct drm_i915_gem_object *obj)
@@ -4571,7 +4573,9 @@  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_guc_suspend(dev_priv);
+	ret = intel_uc_suspend(dev_priv);
+	if (ret)
+		i915_gem_set_wedged(dev_priv); /* no hope, discard everything */
 
 	/*
 	 * Neither the BIOS, ourselves or any other kernel
@@ -4606,6 +4610,7 @@  int i915_gem_suspend(struct drm_i915_private *dev_priv)
 int i915_gem_resume(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = &dev_priv->drm;
+	int ret;
 
 	WARN_ON(dev_priv->gt.awake);
 
@@ -4619,7 +4624,16 @@  int i915_gem_resume(struct drm_i915_private *dev_priv)
 	 */
 	dev_priv->gt.resume(dev_priv);
 
-	intel_guc_resume(dev_priv);
+	/*
+	 * FIXME: At the end of suspend, Full GPU reset is done which unloads
+	 * the GuC firmware. If reset is avoided there, we can check the WOPCM
+	 * status here to see if GuC is still loaded and just do GuC resume
+	 * without reloading the firmware back.
+	 */
+	ret = intel_uc_resume(dev_priv);
+	if (ret)
+		i915_gem_set_wedged(dev_priv);
+
 	mutex_unlock(&dev->struct_mutex);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 2774778..80251ec 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -373,6 +373,13 @@  int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 			goto err_guc;
 	}
 
+	/*
+	 * FIXME: Currently, GuC is loaded unconditionally without checking
+	 * if it is already available in WOPCM (generally available after resume
+	 * from sleep state). We can skip the load based on WOPCM status (To be
+	 * decided based on bit 0 of GUC_WOPCM_SIZE).
+	 */
+
 	/* init WOPCM */
 	I915_WRITE(GUC_WOPCM_SIZE, intel_guc_wopcm_size(dev_priv));
 	I915_WRITE(DMA_GUC_WOPCM_OFFSET,
@@ -481,6 +488,26 @@  void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 	i915_ggtt_disable_guc(dev_priv);
 }
 
+int intel_uc_runtime_suspend(struct drm_i915_private *dev_priv)
+{
+	return intel_guc_suspend(dev_priv);
+}
+
+int intel_uc_runtime_resume(struct drm_i915_private *dev_priv)
+{
+	return intel_guc_resume(dev_priv);
+}
+
+int intel_uc_suspend(struct drm_i915_private *dev_priv)
+{
+	return intel_guc_suspend(dev_priv);
+}
+
+int intel_uc_resume(struct drm_i915_private *dev_priv)
+{
+	return intel_guc_resume(dev_priv);
+}
+
 int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len)
 {
 	WARN(1, "Unexpected send: action=%#x\n", *action);
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 6966349..0a79e17 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -208,6 +208,10 @@  struct intel_huc {
 void intel_uc_fini_fw(struct drm_i915_private *dev_priv);
 int intel_uc_init_hw(struct drm_i915_private *dev_priv);
 void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
+int intel_uc_runtime_suspend(struct drm_i915_private *dev_priv);
+int intel_uc_runtime_resume(struct drm_i915_private *dev_priv);
+int intel_uc_suspend(struct drm_i915_private *dev_priv);
+int intel_uc_resume(struct drm_i915_private *dev_priv);
 int intel_guc_sample_forcewake(struct intel_guc *guc);
 int intel_guc_send_nop(struct intel_guc *guc, const u32 *action, u32 len);
 int intel_guc_send_mmio(struct intel_guc *guc, const u32 *action, u32 len);