Re: [PATCH] drm/i915/uc: Start preparing GuC/HuC for reset

["Michal Wajdeczko" <michal.wajdeczko@xxxxxxxxx>]

On Thu, 22 Feb 2018 21:52:39 +0100, Michel Thierry  
<michel.thierry@xxxxxxxxx> wrote:

> On 22/02/18 10:45, Michal Wajdeczko wrote:
> > 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.
> >  Suggested-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > Signed-off-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> > Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > Cc: Sagar Arun Kamble <sagar.a.kamble@xxxxxxxxx>
> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/i915_gem.c    |  1 +
> >   drivers/gpu/drm/i915/intel_guc.h   |  5 +++++
> >   drivers/gpu/drm/i915/intel_huc.h   |  5 +++++
> >   drivers/gpu/drm/i915/intel_uc.c    | 11 +++++++++++
> >   drivers/gpu/drm/i915/intel_uc.h    |  1 +
> >   drivers/gpu/drm/i915/intel_uc_fw.h |  6 ++++++
> >   6 files changed, 29 insertions(+)
> >  diff --git a/drivers/gpu/drm/i915/i915_gem.c  
> > b/drivers/gpu/drm/i915/i915_gem.c
> > index 14c855b..9f91625 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_reset_prepare(dev_priv);
> >     	return err;
> >   }
> > diff --git a/drivers/gpu/drm/i915/intel_guc.h  
> > b/drivers/gpu/drm/i915/intel_guc.h
> > index 52856a9..ffa528f 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_reset_prepare(struct intel_guc *guc)
>
> Hi,
>
> Would intel_reset_guc_prepare be better?

I was following existing naming from "i915_gem_reset_prepare"

But if we want to diverge and still follow obj-verb-subject pattern
then maybe something like this:

intel_uc_prepare_to_reset(i915)
{
  intel_huc_prepare_to_reset(huc)
  intel_guc_prepare_to_reset(guc)
}

/Michal

> Here you're preparing the guc for reset, not asking guc to prepare a  
> reset.
>
> For example, we have:
> intel_guc_reset_engine  -> h2g requesting an engine reset
> intel_reset_guc 	-> i915 will reset guc
> gen9_reset_guc_interrupts -> i915 is resetting the iir reg
>
> -Michel
>
> > +{
> > +	intel_uc_fw_reset_prepare(&guc->fw);
> > +}
> > +
> >   #endif
> > diff --git a/drivers/gpu/drm/i915/intel_huc.h  
> > b/drivers/gpu/drm/i915/intel_huc.h
> > index 40039db..c1ee5b5 100644
> > --- a/drivers/gpu/drm/i915/intel_huc.h
> > +++ b/drivers/gpu/drm/i915/intel_huc.h
> > @@ -38,4 +38,9 @@ struct intel_huc {
> >   int intel_huc_init_hw(struct intel_huc *huc);
> >   int intel_huc_auth(struct intel_huc *huc);
> >   +static inline void intel_huc_reset_prepare(struct intel_huc *huc)
>
> same here (for consistency).
>
> > +{
> > +	intel_uc_fw_reset_prepare(&huc->fw);
> > +}
> > +
> >   #endif
> > diff --git a/drivers/gpu/drm/i915/intel_uc.c  
> > b/drivers/gpu/drm/i915/intel_uc.c
> > index 9f1bac6..64ae0a9 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.c
> > +++ b/drivers/gpu/drm/i915/intel_uc.c
> > @@ -445,3 +445,14 @@ void intel_uc_fini_hw(struct drm_i915_private  
> > *dev_priv)
> >   	if (USES_GUC_SUBMISSION(dev_priv))
> >   		gen9_disable_guc_interrupts(dev_priv);
> >   }
> > +
> > +void intel_uc_reset_prepare(struct drm_i915_private *i915)
> > +{
> > +	if (!USES_GUC(i915))
> > +		return;
> > +
> > +	GEM_BUG_ON(!HAS_GUC(i915));
> > +
> > +	intel_huc_reset_prepare(&i915->huc);
> > +	intel_guc_reset_prepare(&i915->guc);
> > +}
> > diff --git a/drivers/gpu/drm/i915/intel_uc.h  
> > b/drivers/gpu/drm/i915/intel_uc.h
> > index f2984e0..5c10253 100644
> > --- a/drivers/gpu/drm/i915/intel_uc.h
> > +++ b/drivers/gpu/drm/i915/intel_uc.h
> > @@ -39,6 +39,7 @@
> >   void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
> >   int intel_uc_init(struct drm_i915_private *dev_priv);
> >   void intel_uc_fini(struct drm_i915_private *dev_priv);
> > +void intel_uc_reset_prepare(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..68bc304 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_reset_prepare(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,
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx