Re: [Intel-gfx] [PATCH v13 14/21] drm/i915/uc: Update GEM runtime resume with need for reload of GuC/HuC

[Sagar Arun Kamble]

On 10/11/2017 10:49 PM, Michal Wajdeczko wrote:
On Wed, 11 Oct 2017 10:54:09 +0200, Sagar Arun Kamble 
 wrote:



On resume from drm sleep/suspend, we have gem_init_hw path to reload
the GuC/HuC firmware. However, on resume from runtime suspend we needed
to add support to reload the GuC/HuC firmware and resume.
We can leverage intel_uc_init_hw for this based on skip_load_on_resume.

Signed-off-by: Sagar Arun Kamble 
Cc: Chris Wilson 
Cc: Michal Wajdeczko 
Cc: Michał Winiarski 
Cc: Joonas Lahtinen 
---
 drivers/gpu/drm/i915/i915_gem.c |  2 +-
 drivers/gpu/drm/i915/intel_uc.c | 28 
 drivers/gpu/drm/i915/intel_uc.h |  1 +
 3 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c 
b/drivers/gpu/drm/i915/i915_gem.c

index 7d1b7e1..9e257e2 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2113,7 +2113,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_uc_resume(dev_priv);
+    intel_uc_runtime_resume(dev_priv);
mutex_unlock(_priv->drm.struct_mutex);
 }
diff --git a/drivers/gpu/drm/i915/intel_uc.c 
b/drivers/gpu/drm/i915/intel_uc.c

index f641872..25acf8f 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -410,3 +410,31 @@ void intel_uc_resume(struct drm_i915_private 
*dev_priv)

 guc->skip_load_on_resume = false;
 }
 }
+
+/**
+ * intel_uc_runtime_resume() - Resume uC operation.
+ * @dev_priv: i915 device private
+ *
+ * This function invokes intel_uc_suspend that will if GuC is loaded

    
Please focus on tasks rather than function names.

Sure.



+ * enable communication with GuC, enable GuC interrupts, invoke GuC OS
+ * resumption and enable GuC submission.
+ * If GuC is not loaded, GuC needs to be loaded and do the entire setup
+ * by leveraging intel_uc_init_hw.
+ *
+ */
+void intel_uc_runtime_resume(struct drm_i915_private *dev_priv)
+{
+    struct intel_guc *guc = _priv->guc;
+
+    if (!guc->suspended)
+    return;
+
+    intel_uc_resume(dev_priv);
+
+    if (guc->skip_load_on_resume)


Hmm, I may be lost, but I feel that some changes from 13/21 done
in intel_uc_resume() looks like good candidate for this function.

What I'm missing is clear distinction what each function will do,
due to lot of conditions and cross calls.
Have introduced separate runtime and drm uc_suspend/resume functions in 
v14 and will help understand this better.



+    return;
+
+    WARN_ON(guc_wopcm_locked(guc));


Why here?

will remove.



+
+    intel_uc_init_hw(dev_priv);
+}
diff --git a/drivers/gpu/drm/i915/intel_uc.h 
b/drivers/gpu/drm/i915/intel_uc.h

index 7d9dd9c..f741ccc 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -36,5 +36,6 @@
 void intel_uc_cleanup(struct drm_i915_private *dev_priv);
 int intel_uc_suspend(struct drm_i915_private *dev_priv);
 void intel_uc_resume(struct drm_i915_private *dev_priv);
+void intel_uc_runtime_resume(struct drm_i915_private *dev_priv);
#endif


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

Re: [Intel-gfx] [PATCH v13 14/21] drm/i915/uc: Update GEM runtime resume with need for reload of GuC/HuC

[Michal Wajdeczko]

On Wed, 11 Oct 2017 10:54:09 +0200, Sagar Arun Kamble  
 wrote:



On resume from drm sleep/suspend, we have gem_init_hw path to reload
the GuC/HuC firmware. However, on resume from runtime suspend we needed
to add support to reload the GuC/HuC firmware and resume.
We can leverage intel_uc_init_hw for this based on skip_load_on_resume.

Signed-off-by: Sagar Arun Kamble 
Cc: Chris Wilson 
Cc: Michal Wajdeczko 
Cc: Michał Winiarski 
Cc: Joonas Lahtinen 
---
 drivers/gpu/drm/i915/i915_gem.c |  2 +-
 drivers/gpu/drm/i915/intel_uc.c | 28 
 drivers/gpu/drm/i915/intel_uc.h |  1 +
 3 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c  
b/drivers/gpu/drm/i915/i915_gem.c

index 7d1b7e1..9e257e2 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2113,7 +2113,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_uc_resume(dev_priv);
+   intel_uc_runtime_resume(dev_priv);
mutex_unlock(_priv->drm.struct_mutex);
 }
diff --git a/drivers/gpu/drm/i915/intel_uc.c  
b/drivers/gpu/drm/i915/intel_uc.c

index f641872..25acf8f 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -410,3 +410,31 @@ void intel_uc_resume(struct drm_i915_private  
*dev_priv)

guc->skip_load_on_resume = false;
}
 }
+
+/**
+ * intel_uc_runtime_resume() - Resume uC operation.
+ * @dev_priv: i915 device private
+ *
+ * This function invokes intel_uc_suspend that will if GuC is loaded


Please focus on tasks rather than function names.


+ * enable communication with GuC, enable GuC interrupts, invoke GuC OS
+ * resumption and enable GuC submission.
+ * If GuC is not loaded, GuC needs to be loaded and do the entire setup
+ * by leveraging intel_uc_init_hw.
+ *
+ */
+void intel_uc_runtime_resume(struct drm_i915_private *dev_priv)
+{
+   struct intel_guc *guc = _priv->guc;
+
+   if (!guc->suspended)
+   return;
+
+   intel_uc_resume(dev_priv);
+
+   if (guc->skip_load_on_resume)


Hmm, I may be lost, but I feel that some changes from 13/21 done
in intel_uc_resume() looks like good candidate for this function.

What I'm missing is clear distinction what each function will do,
due to lot of conditions and cross calls.


+   return;
+
+   WARN_ON(guc_wopcm_locked(guc));


Why here?


+
+   intel_uc_init_hw(dev_priv);
+}
diff --git a/drivers/gpu/drm/i915/intel_uc.h  
b/drivers/gpu/drm/i915/intel_uc.h

index 7d9dd9c..f741ccc 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -36,5 +36,6 @@
 void intel_uc_cleanup(struct drm_i915_private *dev_priv);
 int intel_uc_suspend(struct drm_i915_private *dev_priv);
 void intel_uc_resume(struct drm_i915_private *dev_priv);
+void intel_uc_runtime_resume(struct drm_i915_private *dev_priv);
#endif

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

[Intel-gfx] [PATCH v13 14/21] drm/i915/uc: Update GEM runtime resume with need for reload of GuC/HuC

[Sagar Arun Kamble]

On resume from drm sleep/suspend, we have gem_init_hw path to reload
the GuC/HuC firmware. However, on resume from runtime suspend we needed
to add support to reload the GuC/HuC firmware and resume.
We can leverage intel_uc_init_hw for this based on skip_load_on_resume.

Signed-off-by: Sagar Arun Kamble 
Cc: Chris Wilson 
Cc: Michal Wajdeczko 
Cc: Michał Winiarski 
Cc: Joonas Lahtinen 
---
 drivers/gpu/drm/i915/i915_gem.c |  2 +-
 drivers/gpu/drm/i915/intel_uc.c | 28 
 drivers/gpu/drm/i915/intel_uc.h |  1 +
 3 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 7d1b7e1..9e257e2 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2113,7 +2113,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_uc_resume(dev_priv);
+   intel_uc_runtime_resume(dev_priv);
 
mutex_unlock(_priv->drm.struct_mutex);
 }
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index f641872..25acf8f 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -410,3 +410,31 @@ void intel_uc_resume(struct drm_i915_private *dev_priv)
guc->skip_load_on_resume = false;
}
 }
+
+/**
+ * intel_uc_runtime_resume() - Resume uC operation.
+ * @dev_priv: i915 device private
+ *
+ * This function invokes intel_uc_suspend that will if GuC is loaded
+ * enable communication with GuC, enable GuC interrupts, invoke GuC OS
+ * resumption and enable GuC submission.
+ * If GuC is not loaded, GuC needs to be loaded and do the entire setup
+ * by leveraging intel_uc_init_hw.
+ *
+ */
+void intel_uc_runtime_resume(struct drm_i915_private *dev_priv)
+{
+   struct intel_guc *guc = _priv->guc;
+
+   if (!guc->suspended)
+   return;
+
+   intel_uc_resume(dev_priv);
+
+   if (guc->skip_load_on_resume)
+   return;
+
+   WARN_ON(guc_wopcm_locked(guc));
+
+   intel_uc_init_hw(dev_priv);
+}
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 7d9dd9c..f741ccc 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -36,5 +36,6 @@
 void intel_uc_cleanup(struct drm_i915_private *dev_priv);
 int intel_uc_suspend(struct drm_i915_private *dev_priv);
 void intel_uc_resume(struct drm_i915_private *dev_priv);
+void intel_uc_runtime_resume(struct drm_i915_private *dev_priv);
 
 #endif
-- 
1.9.1

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