Re: [Intel-gfx] [PATCH v3 1/2] drm/i915/uc: Start preparing GuC/HuC for reset

2018-03-08 Thread Michal Wajdeczko 
On Thu, 08 Mar 2018 06:47:35 +0100, Sagar Arun Kamble  
 wrote:





On 3/5/2018 8:13 PM, Chris Wilson wrote:

Quoting Michal Wajdeczko (2018-03-05 14:29:16)

Right after GPU reset there will be a small window of time during which
some of GuC/HuC fields will still show state before reset. Let's start
to fix that by sanitizing firmware status as we will use it shortly.

v2: s/reset_prepare/prepare_to_reset (Michel)
 don't forget about gem_sanitize path (Daniele)
v3: rebased

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

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

index a5bd073..aedb17d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2981,6 +2981,7 @@ int i915_gem_reset_prepare(struct  
drm_i915_private *dev_priv)

 }
   i915_gem_revoke_fences(dev_priv);
+   intel_uc_prepare_to_reset(dev_priv);
   return err;
  }
@@ -4882,8 +4883,10 @@ void i915_gem_sanitize(struct drm_i915_private  
*i915)
  * it may impact the display and we are uncertain about the  
stability

  * of the reset, so this could be applied to even earlier gen.
  */
-   if (INTEL_GEN(i915) >= 5 && intel_has_gpu_reset(i915))
+   if (INTEL_GEN(i915) >= 5 && intel_has_gpu_reset(i915)) {
+   intel_uc_prepare_to_reset(i915);
 WARN_ON(intel_gpu_reset(i915, ALL_ENGINES));

This still feels wrong. If we accept that we will have to reload the fw
on resume, why are we not just sanitzing the uc state and forcing the
reload?

Hi Chris,

intel_uc_prepare_to_reset() is sanitizing uc state and reload is  
happening through gem_init_hw in resume path.

what are we missing?


Maybe to make everyone happy, I'll introduce intel_uc_sanitize()
leaving to doubt how we will handle reset/suspend/resume scenarios.
And then uc_resume will likely be nop as we will do full reload on
gem_init_hw.

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

Re: [Intel-gfx] [PATCH v3 1/2] drm/i915/uc: Start preparing GuC/HuC for reset

2018-03-07 Thread Sagar Arun Kamble 



On 3/5/2018 8:13 PM, Chris Wilson wrote:

Quoting Michal Wajdeczko (2018-03-05 14:29:16)

Right after GPU reset there will be a small window of time during which
some of GuC/HuC fields will still show state before reset. Let's start
to fix that by sanitizing firmware status as we will use it shortly.

v2: s/reset_prepare/prepare_to_reset (Michel)
 don't forget about gem_sanitize path (Daniele)
v3: rebased

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

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a5bd073..aedb17d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2981,6 +2981,7 @@ int i915_gem_reset_prepare(struct drm_i915_private 
*dev_priv)
 }
  
 i915_gem_revoke_fences(dev_priv);

+   intel_uc_prepare_to_reset(dev_priv);
  
 return err;

  }
@@ -4882,8 +4883,10 @@ void i915_gem_sanitize(struct drm_i915_private *i915)
  * it may impact the display and we are uncertain about the stability
  * of the reset, so this could be applied to even earlier gen.
  */
-   if (INTEL_GEN(i915) >= 5 && intel_has_gpu_reset(i915))
+   if (INTEL_GEN(i915) >= 5 && intel_has_gpu_reset(i915)) {
+   intel_uc_prepare_to_reset(i915);
 WARN_ON(intel_gpu_reset(i915, ALL_ENGINES));

This still feels wrong. If we accept that we will have to reload the fw
on resume, why are we not just sanitzing the uc state and forcing the
reload?

Hi Chris,

intel_uc_prepare_to_reset() is sanitizing uc state and reload is 
happening through gem_init_hw in resume path.

what are we missing?

Thanks,
Sagar

-Chris


--
Thanks,
Sagar

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

Re: [Intel-gfx] [PATCH v3 1/2] drm/i915/uc: Start preparing GuC/HuC for reset

2018-03-05 Thread Chris Wilson 
Quoting Michal Wajdeczko (2018-03-05 14:29:16)
> Right after GPU reset there will be a small window of time during which
> some of GuC/HuC fields will still show state before reset. Let's start
> to fix that by sanitizing firmware status as we will use it shortly.
> 
> v2: s/reset_prepare/prepare_to_reset (Michel)
> don't forget about gem_sanitize path (Daniele)
> v3: rebased
> 
> Suggested-by: Daniele Ceraolo Spurio 
> Signed-off-by: Michal Wajdeczko 
> Cc: Daniele Ceraolo Spurio 
> Cc: Sagar Arun Kamble 
> Cc: Chris Wilson 
> Cc: Michel Thierry 
> Reviewed-by: Daniele Ceraolo Spurio 
> ---
>  drivers/gpu/drm/i915/i915_gem.c|  5 -
>  drivers/gpu/drm/i915/intel_guc.h   |  5 +
>  drivers/gpu/drm/i915/intel_huc.h   |  5 +
>  drivers/gpu/drm/i915/intel_uc.c| 14 ++
>  drivers/gpu/drm/i915/intel_uc.h|  1 +
>  drivers/gpu/drm/i915/intel_uc_fw.h |  6 ++
>  6 files changed, 35 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index a5bd073..aedb17d 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2981,6 +2981,7 @@ int i915_gem_reset_prepare(struct drm_i915_private 
> *dev_priv)
> }
>  
> i915_gem_revoke_fences(dev_priv);
> +   intel_uc_prepare_to_reset(dev_priv);
>  
> return err;
>  }
> @@ -4882,8 +4883,10 @@ void i915_gem_sanitize(struct drm_i915_private *i915)
>  * it may impact the display and we are uncertain about the stability
>  * of the reset, so this could be applied to even earlier gen.
>  */
> -   if (INTEL_GEN(i915) >= 5 && intel_has_gpu_reset(i915))
> +   if (INTEL_GEN(i915) >= 5 && intel_has_gpu_reset(i915)) {
> +   intel_uc_prepare_to_reset(i915);
> WARN_ON(intel_gpu_reset(i915, ALL_ENGINES));

This still feels wrong. If we accept that we will have to reload the fw
on resume, why are we not just sanitzing the uc state and forcing the
reload?
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] [PATCH v3 1/2] drm/i915/uc: Start preparing GuC/HuC for reset

2018-03-05 Thread Michal Wajdeczko 
Right after GPU reset there will be a small window of time during which
some of GuC/HuC fields will still show state before reset. Let's start
to fix that by sanitizing firmware status as we will use it shortly.

v2: s/reset_prepare/prepare_to_reset (Michel)
don't forget about gem_sanitize path (Daniele)
v3: rebased

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

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a5bd073..aedb17d 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2981,6 +2981,7 @@ int i915_gem_reset_prepare(struct drm_i915_private 
*dev_priv)
}
 
i915_gem_revoke_fences(dev_priv);
+   intel_uc_prepare_to_reset(dev_priv);
 
return err;
 }
@@ -4882,8 +4883,10 @@ void i915_gem_sanitize(struct drm_i915_private *i915)
 * it may impact the display and we are uncertain about the stability
 * of the reset, so this could be applied to even earlier gen.
 */
-   if (INTEL_GEN(i915) >= 5 && intel_has_gpu_reset(i915))
+   if (INTEL_GEN(i915) >= 5 && intel_has_gpu_reset(i915)) {
+   intel_uc_prepare_to_reset(i915);
WARN_ON(intel_gpu_reset(i915, ALL_ENGINES));
+   }
 }
 
 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..bdb0777 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -132,4 +132,9 @@ 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 void intel_guc_prepare_to_reset(struct intel_guc *guc)
+{
+   intel_uc_fw_prepare_to_reset(>fw);
+}
+
 #endif
diff --git a/drivers/gpu/drm/i915/intel_huc.h b/drivers/gpu/drm/i915/intel_huc.h
index 5d6e804..6f21424 100644
--- a/drivers/gpu/drm/i915/intel_huc.h
+++ b/drivers/gpu/drm/i915/intel_huc.h
@@ -38,4 +38,9 @@ struct intel_huc {
 void intel_huc_init_early(struct intel_huc *huc);
 int intel_huc_auth(struct intel_huc *huc);
 
+static inline void intel_huc_prepare_to_reset(struct intel_huc *huc)
+{
+   intel_uc_fw_prepare_to_reset(>fw);
+}
+
 #endif
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index e5bf0d3..8a64ff5 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -490,3 +490,17 @@ int intel_uc_resume(struct drm_i915_private *i915)
 
return 0;
 }
+
+void intel_uc_prepare_to_reset(struct drm_i915_private *i915)
+{
+   struct intel_huc *huc = >huc;
+   struct intel_guc *guc = >guc;
+
+   if (!USES_GUC(i915))
+   return;
+
+   GEM_BUG_ON(!HAS_GUC(i915));
+
+   intel_huc_prepare_to_reset(huc);
+   intel_guc_prepare_to_reset(guc);
+}
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index f76d51d..182a2de 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -41,6 +41,7 @@
 void intel_uc_fini(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);
+void intel_uc_prepare_to_reset(struct drm_i915_private *dev_priv);
 
 static inline bool intel_uc_is_using_guc(void)
 {
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h 
b/drivers/gpu/drm/i915/intel_uc_fw.h
index d5fd460..f1ee653 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_prepare_to_reset(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