Re: [Intel-gfx] [PATCH 2/3] drm/i915/uc: Sanitize uC together with GEM

2018-03-09 Thread Sagar Arun Kamble 



On 3/9/2018 2:30 AM, Michal Wajdeczko wrote:

Instead of dancing around uC on reset/suspend/resume scenarios,
explicitly sanitize uC when we sanitize GEM to force uC reload
and start from known beginning.

Signed-off-by: Michal Wajdeczko 
Cc: Daniele Ceraolo Spurio 
Cc: Sagar Arun Kamble 
Cc: Chris Wilson 
Cc: Michel Thierry 
---
  drivers/gpu/drm/i915/i915_gem.c|  2 ++
  drivers/gpu/drm/i915/intel_guc.h   |  6 ++
  drivers/gpu/drm/i915/intel_huc.h   |  6 ++
  drivers/gpu/drm/i915/intel_uc.c| 18 ++
  drivers/gpu/drm/i915/intel_uc.h|  1 +
  drivers/gpu/drm/i915/intel_uc_fw.h |  6 ++
  6 files changed, 39 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ab88ca5..49c81ae 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4892,6 +4892,8 @@ void i915_gem_sanitize(struct drm_i915_private *i915)
 */
if (INTEL_GEN(i915) >= 5 && intel_has_gpu_reset(i915))
WARN_ON(intel_gpu_reset(i915, ALL_ENGINES));
above intel_gpu_reset also resets uC. Should we just let it reset only 
real engines with this change then?

+
+   intel_uc_sanitize(i915);
  }
  
  int i915_gem_suspend(struct drm_i915_private *dev_priv)

diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index b9424ac..ec8569f 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -132,4 +132,10 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma)
  struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
  u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
  
+static inline int intel_guc_sanitize(struct intel_guc *guc)

+{
+   intel_uc_fw_sanitize(&guc->fw);
+   return 0;
+}
+
  #endif
diff --git a/drivers/gpu/drm/i915/intel_huc.h b/drivers/gpu/drm/i915/intel_huc.h
index 5d6e804..b185850 100644
--- a/drivers/gpu/drm/i915/intel_huc.h
+++ b/drivers/gpu/drm/i915/intel_huc.h
@@ -38,4 +38,10 @@ struct intel_huc {
  void intel_huc_init_early(struct intel_huc *huc);
  int intel_huc_auth(struct intel_huc *huc);
  
+static inline int intel_huc_sanitize(struct intel_huc *huc)

+{
+   intel_uc_fw_sanitize(&huc->fw);
+   return 0;
+}
+
  #endif
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index a45171c..abd1f7b 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -327,6 +327,24 @@ void intel_uc_fini(struct drm_i915_private *dev_priv)
intel_guc_fini(guc);
  }
  
+void intel_uc_sanitize(struct drm_i915_private *i915)

+{
+   struct intel_guc *guc = &i915->guc;
+   struct intel_huc *huc = &i915->huc;
+
+   if (!USES_GUC(i915))
+   return;
+
+   GEM_BUG_ON(!HAS_GUC(i915));
+
+   guc_disable_communication(guc);
+
+   intel_huc_sanitize(huc);
+   intel_guc_sanitize(guc);
+
+   __intel_uc_reset_hw(i915);
+}
+
  int intel_uc_init_hw(struct drm_i915_private *dev_priv)
  {
struct intel_guc *guc = &dev_priv->guc;
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index dce4813..937e611 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -34,6 +34,7 @@
  void intel_uc_fini_fw(struct drm_i915_private *dev_priv);
  int intel_uc_init_misc(struct drm_i915_private *dev_priv);
  void intel_uc_fini_misc(struct drm_i915_private *dev_priv);
+void intel_uc_sanitize(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_init(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h 
b/drivers/gpu/drm/i915/intel_uc_fw.h
index d5fd460..2601521 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.h
+++ b/drivers/gpu/drm/i915/intel_uc_fw.h
@@ -115,6 +115,12 @@ static inline bool intel_uc_fw_is_selected(struct 
intel_uc_fw *uc_fw)
return uc_fw->path != NULL;
  }
  
+static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw)

+{
+   if (uc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS)
+   uc_fw->load_status = INTEL_UC_FIRMWARE_PENDING;
+}
+
  void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
   struct intel_uc_fw *uc_fw);
  int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,


--
Thanks,
Sagar

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

Re: [Intel-gfx] [PATCH 2/3] drm/i915/uc: Sanitize uC together with GEM

2018-03-08 Thread Daniele Ceraolo Spurio 



On 08/03/18 13:00, Michal Wajdeczko wrote:

Instead of dancing around uC on reset/suspend/resume scenarios,
explicitly sanitize uC when we sanitize GEM to force uC reload
and start from known beginning.

Signed-off-by: Michal Wajdeczko 
Cc: Daniele Ceraolo Spurio 
Cc: Sagar Arun Kamble 
Cc: Chris Wilson 
Cc: Michel Thierry 
---
  drivers/gpu/drm/i915/i915_gem.c|  2 ++
  drivers/gpu/drm/i915/intel_guc.h   |  6 ++
  drivers/gpu/drm/i915/intel_huc.h   |  6 ++
  drivers/gpu/drm/i915/intel_uc.c| 18 ++
  drivers/gpu/drm/i915/intel_uc.h|  1 +
  drivers/gpu/drm/i915/intel_uc_fw.h |  6 ++
  6 files changed, 39 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ab88ca5..49c81ae 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4892,6 +4892,8 @@ void i915_gem_sanitize(struct drm_i915_private *i915)
 */
if (INTEL_GEN(i915) >= 5 && intel_has_gpu_reset(i915))
WARN_ON(intel_gpu_reset(i915, ALL_ENGINES));
+
+   intel_uc_sanitize(i915);
  }
  
  int i915_gem_suspend(struct drm_i915_private *dev_priv)

diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index b9424ac..ec8569f 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -132,4 +132,10 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma)
  struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
  u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
  
+static inline int intel_guc_sanitize(struct intel_guc *guc)

+{
+   intel_uc_fw_sanitize(&guc->fw);
+   return 0;
+}
+
  #endif
diff --git a/drivers/gpu/drm/i915/intel_huc.h b/drivers/gpu/drm/i915/intel_huc.h
index 5d6e804..b185850 100644
--- a/drivers/gpu/drm/i915/intel_huc.h
+++ b/drivers/gpu/drm/i915/intel_huc.h
@@ -38,4 +38,10 @@ struct intel_huc {
  void intel_huc_init_early(struct intel_huc *huc);
  int intel_huc_auth(struct intel_huc *huc);
  
+static inline int intel_huc_sanitize(struct intel_huc *huc)

+{
+   intel_uc_fw_sanitize(&huc->fw);
+   return 0;
+}
+
  #endif
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index a45171c..abd1f7b 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -327,6 +327,24 @@ void intel_uc_fini(struct drm_i915_private *dev_priv)
intel_guc_fini(guc);
  }
  
+void intel_uc_sanitize(struct drm_i915_private *i915)

+{
+   struct intel_guc *guc = &i915->guc;
+   struct intel_huc *huc = &i915->huc;
+
+   if (!USES_GUC(i915))
+   return;
+
+   GEM_BUG_ON(!HAS_GUC(i915));
+
+   guc_disable_communication(guc);


If you want to do this here for CT buffers you need to call this before 
resetting the GPU otherwise GuC will already be reset and won't be able 
to unregister the buffers. We can also just wipe the shared memory where 
the CT buffer info is stored but that doesn't sound too clean, although 
we might have to add the code for it anyway if we want to support the 
(unlikely) case where GuC dies on some error.


Might also be interesting to release doorbells here to sync that state 
as well, but that's a task for a another time ;)


BTW, are you planning to call this in the i915_reset flow as well or is 
the plan to add another dedicated function?


Thanks,
Daniele


+
+   intel_huc_sanitize(huc);
+   intel_guc_sanitize(guc);
+
+   __intel_uc_reset_hw(i915);
+}
+
  int intel_uc_init_hw(struct drm_i915_private *dev_priv)
  {
struct intel_guc *guc = &dev_priv->guc;
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index dce4813..937e611 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -34,6 +34,7 @@
  void intel_uc_fini_fw(struct drm_i915_private *dev_priv);
  int intel_uc_init_misc(struct drm_i915_private *dev_priv);
  void intel_uc_fini_misc(struct drm_i915_private *dev_priv);
+void intel_uc_sanitize(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_init(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h 
b/drivers/gpu/drm/i915/intel_uc_fw.h
index d5fd460..2601521 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.h
+++ b/drivers/gpu/drm/i915/intel_uc_fw.h
@@ -115,6 +115,12 @@ static inline bool intel_uc_fw_is_selected(struct 
intel_uc_fw *uc_fw)
return uc_fw->path != NULL;
  }
  
+static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw)

+{
+   if (uc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS)
+   uc_fw->load_status = INTEL_UC_FIRMWARE_PENDING;
+}
+
  void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
   struct intel_uc_fw *uc_fw);
  int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,

Re: [Intel-gfx] [PATCH 2/3] drm/i915/uc: Sanitize uC together with GEM

2018-03-08 Thread Chris Wilson 
Quoting Michal Wajdeczko (2018-03-08 21:00:35)
> Instead of dancing around uC on reset/suspend/resume scenarios,
> explicitly sanitize uC when we sanitize GEM to force uC reload
> and start from known beginning.
> 
> Signed-off-by: Michal Wajdeczko 
> Cc: Daniele Ceraolo Spurio 
> Cc: Sagar Arun Kamble 
> Cc: Chris Wilson 
> Cc: Michel Thierry 

Lgtm,
Reviewed-by: Chris Wilson 

Second opinions are welcome.
> ---
>  drivers/gpu/drm/i915/i915_gem.c|  2 ++
>  drivers/gpu/drm/i915/intel_guc.h   |  6 ++
>  drivers/gpu/drm/i915/intel_huc.h   |  6 ++
>  drivers/gpu/drm/i915/intel_uc.c| 18 ++
>  drivers/gpu/drm/i915/intel_uc.h|  1 +
>  drivers/gpu/drm/i915/intel_uc_fw.h |  6 ++
>  6 files changed, 39 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index ab88ca5..49c81ae 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4892,6 +4892,8 @@ void i915_gem_sanitize(struct drm_i915_private *i915)
>  */
> if (INTEL_GEN(i915) >= 5 && intel_has_gpu_reset(i915))
> WARN_ON(intel_gpu_reset(i915, ALL_ENGINES));

I think we can say that our reset works for gen3+ well enough to use
everywhere. (Certainly gen3/gen4 are no worse than gen5!)
Future task for the brave.
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH 2/3] drm/i915/uc: Sanitize uC together with GEM

2018-03-08 Thread Michal Wajdeczko 
Instead of dancing around uC on reset/suspend/resume scenarios,
explicitly sanitize uC when we sanitize GEM to force uC reload
and start from known beginning.

Signed-off-by: Michal Wajdeczko 
Cc: Daniele Ceraolo Spurio 
Cc: Sagar Arun Kamble 
Cc: Chris Wilson 
Cc: Michel Thierry 
---
 drivers/gpu/drm/i915/i915_gem.c|  2 ++
 drivers/gpu/drm/i915/intel_guc.h   |  6 ++
 drivers/gpu/drm/i915/intel_huc.h   |  6 ++
 drivers/gpu/drm/i915/intel_uc.c| 18 ++
 drivers/gpu/drm/i915/intel_uc.h|  1 +
 drivers/gpu/drm/i915/intel_uc_fw.h |  6 ++
 6 files changed, 39 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ab88ca5..49c81ae 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4892,6 +4892,8 @@ void i915_gem_sanitize(struct drm_i915_private *i915)
 */
if (INTEL_GEN(i915) >= 5 && intel_has_gpu_reset(i915))
WARN_ON(intel_gpu_reset(i915, ALL_ENGINES));
+
+   intel_uc_sanitize(i915);
 }
 
 int i915_gem_suspend(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index b9424ac..ec8569f 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -132,4 +132,10 @@ static inline u32 guc_ggtt_offset(struct i915_vma *vma)
 struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 size);
 u32 intel_guc_wopcm_size(struct drm_i915_private *dev_priv);
 
+static inline int intel_guc_sanitize(struct intel_guc *guc)
+{
+   intel_uc_fw_sanitize(&guc->fw);
+   return 0;
+}
+
 #endif
diff --git a/drivers/gpu/drm/i915/intel_huc.h b/drivers/gpu/drm/i915/intel_huc.h
index 5d6e804..b185850 100644
--- a/drivers/gpu/drm/i915/intel_huc.h
+++ b/drivers/gpu/drm/i915/intel_huc.h
@@ -38,4 +38,10 @@ struct intel_huc {
 void intel_huc_init_early(struct intel_huc *huc);
 int intel_huc_auth(struct intel_huc *huc);
 
+static inline int intel_huc_sanitize(struct intel_huc *huc)
+{
+   intel_uc_fw_sanitize(&huc->fw);
+   return 0;
+}
+
 #endif
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index a45171c..abd1f7b 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -327,6 +327,24 @@ void intel_uc_fini(struct drm_i915_private *dev_priv)
intel_guc_fini(guc);
 }
 
+void intel_uc_sanitize(struct drm_i915_private *i915)
+{
+   struct intel_guc *guc = &i915->guc;
+   struct intel_huc *huc = &i915->huc;
+
+   if (!USES_GUC(i915))
+   return;
+
+   GEM_BUG_ON(!HAS_GUC(i915));
+
+   guc_disable_communication(guc);
+
+   intel_huc_sanitize(huc);
+   intel_guc_sanitize(guc);
+
+   __intel_uc_reset_hw(i915);
+}
+
 int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 {
struct intel_guc *guc = &dev_priv->guc;
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index dce4813..937e611 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -34,6 +34,7 @@
 void intel_uc_fini_fw(struct drm_i915_private *dev_priv);
 int intel_uc_init_misc(struct drm_i915_private *dev_priv);
 void intel_uc_fini_misc(struct drm_i915_private *dev_priv);
+void intel_uc_sanitize(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_init(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h 
b/drivers/gpu/drm/i915/intel_uc_fw.h
index d5fd460..2601521 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.h
+++ b/drivers/gpu/drm/i915/intel_uc_fw.h
@@ -115,6 +115,12 @@ static inline bool intel_uc_fw_is_selected(struct 
intel_uc_fw *uc_fw)
return uc_fw->path != NULL;
 }
 
+static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw)
+{
+   if (uc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS)
+   uc_fw->load_status = INTEL_UC_FIRMWARE_PENDING;
+}
+
 void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
   struct intel_uc_fw *uc_fw);
 int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
-- 
1.9.1

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