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

[Michel Thierry]

On 22/02/18 13:21, Michal Wajdeczko wrote:
> On Thu, 22 Feb 2018 21:52:39 +0100, Michel Thierry 
> <michel.thierry at intel.com> 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 at intel.com>
>>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>>> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>>> Cc: Sagar Arun Kamble <sagar.a.kamble at intel.com>
>>> Cc: Chris Wilson <chris at chris-wilson.co.uk>
>>> ---
>>>   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"
> 

Yes, guc is probably the only special one that can be reset or reset 
something itself...

> 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)
> }
> 

This looks clearer to me.

Thanks

> /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,