Re: [Intel-gfx] [PATCH v8 08/12] drm/i915/uc: Fully sanitize uC within intel_uc_fini_hw

[Sagar Arun Kamble]

On 4/9/2018 5:53 PM, Michal Wajdeczko wrote:

As we always call intel_uc_sanitize after every call to
intel_uc_fini_hw we may drop redundant call and sanitize
uC from the fini_hw function.

Signed-off-by: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Chris Wilson 

With change to sanitize during uc_init_mmio this looks good to me.
Reviewed-by: Sagar Arun Kamble 

---
  drivers/gpu/drm/i915/i915_gem.c | 2 --
  drivers/gpu/drm/i915/intel_uc.c | 9 +++--
  drivers/gpu/drm/i915/intel_uc.h | 1 -
  3 files changed, 3 insertions(+), 9 deletions(-)

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

-   intel_uc_sanitize(dev_priv);
  
  	return err;

  }
@@ -5062,7 +5061,6 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
 * machine in an unusable condition.
 */
i915_gem_fini_hw(dev_priv);
-   intel_uc_sanitize(dev_priv);
i915_gem_sanitize(dev_priv);
  
  	intel_runtime_pm_put(dev_priv);

diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 1cffaf7..0439966 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -322,18 +322,13 @@ void intel_uc_fini(struct drm_i915_private *dev_priv)
intel_guc_fini(guc);
  }
  
-void intel_uc_sanitize(struct drm_i915_private *i915)

+static void __uc_sanitize(struct drm_i915_private *i915)
  {
struct intel_guc *guc = >guc;
struct intel_huc *huc = >huc;
  
-	if (!USES_GUC(i915))

-   return;
-
GEM_BUG_ON(!HAS_GUC(i915));
  
-	guc_disable_communication(guc);

-
intel_huc_sanitize(huc);
intel_guc_sanitize(guc);
  
@@ -445,6 +440,8 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)

intel_guc_submission_disable(guc);
  
  	guc_disable_communication(guc);

+
+   __uc_sanitize(dev_priv);
  }
  
  int intel_uc_suspend(struct drm_i915_private *i915)

diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 25d73ad..64aaf93 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -33,7 +33,6 @@
  void intel_uc_init_mmio(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);


--
Thanks,
Sagar

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

Re: [Intel-gfx] [PATCH v8 08/12] drm/i915/uc: Fully sanitize uC within intel_uc_fini_hw

[Michal Wajdeczko]

On Mon, 09 Apr 2018 14:47:24 +0200, Chris Wilson  
 wrote:



Quoting Michal Wajdeczko (2018-04-09 13:23:28)

As we always call intel_uc_sanitize after every call to
intel_uc_fini_hw we may drop redundant call and sanitize
uC from the fini_hw function.

Signed-off-by: Michal Wajdeczko 
Cc: Sagar Arun Kamble 
Cc: Chris Wilson 


Not that it matters, since doing it before losing control or on resume
is the same from our pov, but I've always pencilled in sanitize as being
done on takeover (i.e. before init).


In intel_uc_init_hw we are already doing some semi-sanitization (thanks
to __intel_uc_reset_hw), but maybe to be more explicit, we should add
call to __uc_sanitize() in intel_uc_init_mmio() ?


Why do you favour after fini?


Hmm, not at all, I would call it just more explicit



Gut feeling prefers keeping it as a separate step rather rolling it up
into init/fini. But that's just because before we did sanitize
elsewhere, we had many strange bugs and those bugs have left their
scars (so I like seeing sanitize, it reminds me of dead bugs).


As now we have symmetrical inits/finis that cover all critical paths,
I don't think we need separate 'sanitize' step that could be called
any time/any place.

I assume extra __sanitize in uc_init_mmio should be enough reminder.

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

Re: [Intel-gfx] [PATCH v8 08/12] drm/i915/uc: Fully sanitize uC within intel_uc_fini_hw

[Chris Wilson]

Quoting Michal Wajdeczko (2018-04-09 13:23:28)
> As we always call intel_uc_sanitize after every call to
> intel_uc_fini_hw we may drop redundant call and sanitize
> uC from the fini_hw function.
> 
> Signed-off-by: Michal Wajdeczko 
> Cc: Sagar Arun Kamble 
> Cc: Chris Wilson 

Not that it matters, since doing it before losing control or on resume
is the same from our pov, but I've always pencilled in sanitize as being
done on takeover (i.e. before init). Why do you favour after fini?

Gut feeling prefers keeping it as a separate step rather rolling it up
into init/fini. But that's just because before we did sanitize
elsewhere, we had many strange bugs and those bugs have left their
scars (so I like seeing sanitize, it reminds me of dead bugs).
-Chris
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx