Re: [Intel-gfx] [PATCH v3 06/10] drm/i915/uc: Improve tracking of uC init status

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

On 2/13/20 4:04 PM, John Harrison wrote:
> On 2/13/2020 15:44, Daniele Ceraolo Spurio wrote:
>> On 2/13/20 3:36 PM, John Harrison wrote:
>>> On 2/11/2020 16:31, Daniele Ceraolo Spurio wrote:
>>>> To be able to setup GuC submission functions during engine init we need
>>>> to commit to using GuC as soon as possible.
>>>> Currently, the only thing that can stop us from using the
>>>> microcontrollers once we've fetched the blobs is a fundamental
>>>> error (e.g. OOM); given that if we hit such an error we can't really
>>>> fall-back to anything, we can "officialize" the FW fetching completion
>>>> as the moment at which we're committing to using GuC.
>>>>
>>>> To better differentiate this case, the uses_guc check, which indicates
>>>> that GuC is supported and was selected in modparam, is renamed to
>>>> wants_guc and a new uses_guc is introduced to represent the case were
>>>> we're committed to using the GuC. Note that uses_guc does still not 
>>>> imply
>>>> that the blob is actually loaded on the HW (is_running is the check for
>>>> that). Also, since we need to have attempted the fetch for the result
>>>> of uses_guc to be meaningful, we need to make sure we've moved away
>>>> from INTEL_UC_FIRMWARE_SELECTED.
>>>>
>>>> All the GuC changes have been mirrored on the HuC for coherency.
>>>>
>>>> v2: split fetch return changes and new macros to their own patches,
>>>>      support HuC only if GuC is wanted, improve "used" state
>>>>      description (Michal)
>>>>
>>>> v3: s/wants_huc/uses_huc in uc_init_wopcm
>>>>
>>>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
>>>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>>>> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
>>>> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
>>>> Cc: John Harrison <John.C.Harrison@Intel.com>
>>>> Cc: Matthew Brost <matthew.brost@intel.com>
>>>> Reviewed-by: Fernando Pacheco <fernando.pacheco@intel.com> #v1
>>>> ---
>>>>   drivers/gpu/drm/i915/gt/uc/intel_guc.h    |  8 +++++++-
>>>>   drivers/gpu/drm/i915/gt/uc/intel_huc.h    |  8 +++++++-
>>>>   drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c |  2 +-
>>>>   drivers/gpu/drm/i915/gt/uc/intel_uc.c     | 21 +++++++++++---------
>>>>   drivers/gpu/drm/i915/gt/uc/intel_uc.h     | 24 
>>>> ++++++++++++++++++++++-
>>>>   5 files changed, 50 insertions(+), 13 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h 
>>>> b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
>>>> index 668b067b71e2..b51adaac8752 100644
>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
>>>> @@ -143,11 +143,17 @@ static inline bool 
>>>> intel_guc_is_supported(struct intel_guc *guc)
>>>>       return intel_uc_fw_is_supported(&guc->fw);
>>>>   }
>>>> -static inline bool intel_guc_is_enabled(struct intel_guc *guc)
>>>> +static inline bool intel_guc_is_wanted(struct intel_guc *guc)
>>>>   {
>>>>       return intel_uc_fw_is_enabled(&guc->fw);
>>>>   }
>>>> +static inline bool intel_guc_is_used(struct intel_guc *guc)
>>>> +{
>>>> +    GEM_BUG_ON(__intel_uc_fw_status(&guc->fw) == 
>>>> INTEL_UC_FIRMWARE_SELECTED);
>>>> +    return intel_uc_fw_is_available(&guc->fw);
>>>> +}
>>>> +
>>>>   static inline bool intel_guc_is_fw_running(struct intel_guc *guc)
>>>>   {
>>>>       return intel_uc_fw_is_running(&guc->fw);
>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.h 
>>>> b/drivers/gpu/drm/i915/gt/uc/intel_huc.h
>>>> index 644c059fe01d..a40b9cfc6c22 100644
>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h
>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h
>>>> @@ -41,11 +41,17 @@ static inline bool intel_huc_is_supported(struct 
>>>> intel_huc *huc)
>>>>       return intel_uc_fw_is_supported(&huc->fw);
>>>>   }
>>>> -static inline bool intel_huc_is_enabled(struct intel_huc *huc)
>>>> +static inline bool intel_huc_is_wanted(struct intel_huc *huc)
>>>>   {
>>>>       return intel_uc_fw_is_enabled(&huc->fw);
>>>>   }
>>>> +static inline bool intel_huc_is_used(struct intel_huc *huc)
>>>> +{
>>>> +    GEM_BUG_ON(__intel_uc_fw_status(&huc->fw) == 
>>>> INTEL_UC_FIRMWARE_SELECTED);
>>>> +    return intel_uc_fw_is_available(&huc->fw);
>>>> +}
>>>> +
>>>>   static inline bool intel_huc_is_authenticated(struct intel_huc *huc)
>>>>   {
>>>>       return intel_uc_fw_is_running(&huc->fw);
>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c 
>>>> b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
>>>> index eee193bf2cc4..9cdf4cbe691c 100644
>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
>>>> @@ -20,7 +20,7 @@ void intel_huc_fw_init_early(struct intel_huc *huc)
>>>>       struct drm_i915_private *i915 = gt->i915;
>>>>       intel_uc_fw_init_early(&huc->fw, INTEL_UC_FW_TYPE_HUC,
>>>> -                   intel_uc_uses_guc(uc),
>>>> +                   intel_uc_wants_guc(uc),
>>>>                      INTEL_INFO(i915)->platform, INTEL_REVID(i915));
>>>>   }
>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c 
>>>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>>> index affc4d6f9ead..5825a6c3e0a0 100644
>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
>>>> @@ -48,17 +48,17 @@ static void __confirm_options(struct intel_uc *uc)
>>>>       DRM_DEV_DEBUG_DRIVER(i915->drm.dev,
>>>>                    "enable_guc=%d (guc:%s submission:%s huc:%s)\n",
>>>>                    i915_modparams.enable_guc,
>>>> -                 yesno(intel_uc_uses_guc(uc)),
>>>> +                 yesno(intel_uc_wants_guc(uc)),
>>>>                    yesno(intel_uc_uses_guc_submission(uc)),
>>>> -                 yesno(intel_uc_uses_huc(uc)));
>>>> +                 yesno(intel_uc_wants_huc(uc)));
>>>>       if (i915_modparams.enable_guc == -1)
>>>>           return;
>>>>       if (i915_modparams.enable_guc == 0) {
>>>> -        GEM_BUG_ON(intel_uc_uses_guc(uc));
>>>> +        GEM_BUG_ON(intel_uc_wants_guc(uc));
>>>>           GEM_BUG_ON(intel_uc_uses_guc_submission(uc));
>>>> -        GEM_BUG_ON(intel_uc_uses_huc(uc));
>>>> +        GEM_BUG_ON(intel_uc_wants_huc(uc));
>>>>           return;
>>>>       }
>>>> @@ -93,7 +93,7 @@ void intel_uc_init_early(struct intel_uc *uc)
>>>>       __confirm_options(uc);
>>>> -    if (intel_uc_uses_guc(uc))
>>>> +    if (intel_uc_wants_guc(uc))
>>>>           uc->ops = &uc_ops_on;
>>>>       else
>>>>           uc->ops = &uc_ops_off;
>>>> @@ -257,13 +257,13 @@ static void __uc_fetch_firmwares(struct 
>>>> intel_uc *uc)
>>>>   {
>>>>       int err;
>>>> -    GEM_BUG_ON(!intel_uc_uses_guc(uc));
>>>> +    GEM_BUG_ON(!intel_uc_wants_guc(uc));
>>>>       err = intel_uc_fw_fetch(&uc->guc.fw);
>>>>       if (err)
>>>>           return;
>>>> -    if (intel_uc_uses_huc(uc))
>>>> +    if (intel_uc_wants_huc(uc))
>>>>           intel_uc_fw_fetch(&uc->huc.fw);
>>>>   }
>>>> @@ -279,7 +279,10 @@ static void __uc_init(struct intel_uc *uc)
>>>>       struct intel_huc *huc = &uc->huc;
>>>>       int ret;
>>>> -    GEM_BUG_ON(!intel_uc_uses_guc(uc));
>>>> +    GEM_BUG_ON(!intel_uc_wants_guc(uc));
>>>> +
>>>> +    if (!intel_uc_uses_guc(uc))
>>>> +        return;
>>>>       /* XXX: GuC submission is unavailable for now */
>>>>       GEM_BUG_ON(intel_uc_supports_guc_submission(uc));
>>>> @@ -402,7 +405,7 @@ static int __uc_init_hw(struct intel_uc *uc)
>>>>       int ret, attempts;
>>>>       GEM_BUG_ON(!intel_uc_supports_guc(uc));
>>>> -    GEM_BUG_ON(!intel_uc_uses_guc(uc));
>>>> +    GEM_BUG_ON(!intel_uc_wants_guc(uc));
>>>>       if (!intel_uc_fw_is_available(&guc->fw)) {
>>>>           ret = __uc_check_hw(uc) ||
>>>> diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.h 
>>>> b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
>>>> index 2ce993ceb60a..a41aaf353f88 100644
>>>> --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.h
>>>> +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.h
>>>> @@ -40,6 +40,27 @@ void intel_uc_runtime_suspend(struct intel_uc *uc);
>>>>   int intel_uc_resume(struct intel_uc *uc);
>>>>   int intel_uc_runtime_resume(struct intel_uc *uc);
>>>> +/*
>>>> + * We need to know as early as possible if we're going to use GuC 
>>>> or not to
>>>> + * take the correct setup paths. Additionally, once we've started 
>>>> loading the
>>>> + * GuC, it is unsafe to keep executing without it because some 
>>>> parts of the HW,
>>>> + * a subset of which is not cleaned on GT reset, will start 
>>>> expecting the GuC FW
>>>> + * to be running.
>>>> + * To solve both these requirements, we commit to using the 
>>>> microcontrollers if
>>>> + * the relevant modparam is set and the blobs are found on the 
>>>> system. At this
>>>> + * stage, the only thing that can stop us from attempting to load 
>>>> the blobs on
>>>> + * the HW and use them is a fundamental issue (e.g. no memory for our
>>>> + * structures); if we hit such a problem during driver load we're 
>>>> broken even
>>>> + * without GuC, so there is no point in trying to fall back.
>>>> + *
>>>> + * Given the above, we can be in one of 4 states, with the last one 
>>>> implying
>>>> + * we're committed to using the microcontroller:
>>>> + * - Not supported: not available in HW and/or firmware not defined.
>>>> + * - Supported: available in HW and firmware defined.
>>>> + * - Wanted: supported + enabled in modparam.
>>>> + * - In use: wanted + firmware found on the system and successfully 
>>>> fetched.
>>> Should be another line for 'Running'? I guess the definition of 
>>> 'is_running' comes from elsewhere but it would make sense to include 
>>> it in the description here.
>>
>> "Running" belongs to a different set of states, indicating the current 
>> state of the uC. The ones added here are about what we plan to do with 
>> the GuC, not what the current state is, so I disagree that it makes 
>> sense to add "running" here.
>>
>>>
>>> Also, it would be good to include the actual function names 
>>> themselves. That way if someone searches on 'intel_uc_uses_guc', for 
>>> example, they will find this description. Especially as searching for 
>>> them by either text or symbol will not find the definition!
>>>
>>
>> How do you want them added? We don't usually do that for autogenerated 
>> functions (there is an example of that further down in this file).
>>
>> Daniele
>>
> An example of documentation? Or an example of more autogenerated 
> functions? The intel_uc.h currently in the tree has no documentation in 
> it at all! I was just thinking that I personally get frustrated when I 
> try to search for a function definition and can't find anything because 
> it is some fancy autogenerated thing with no documentation at all. If 
> you feel that including the expanded names is too verbose then fine, 
> it's hardly a show stopper issue. At least you have added some 
> documentation about what the various states mean!
> 

I meant to ask how did you want them documented. There isn't a 1:1 
mapping between the function and the states (4 theoretical states, but 3 
actual recorded states/functions) and the names are subject to change as 
well, which is difficult to mirror in detached documentation. And yes, 
it does also feel a bit too verbose to me :P

Daniele

> John.
> 
>>> With that tweak:
>>> Reviewed-by: John Harrison <John.C.Harrison@Intel.com>
>>>
>>>
>>>> + */
>>>> +
>>>>   #define __uc_state_checker(x, state, required) \
>>>>   static inline bool intel_uc_##state##_##x(struct intel_uc *uc) \
>>>>   { \
>>>> @@ -48,7 +69,8 @@ static inline bool intel_uc_##state##_##x(struct 
>>>> intel_uc *uc) \
>>>>   #define uc_state_checkers(x) \
>>>>   __uc_state_checker(x, supports, supported) \
>>>> -__uc_state_checker(x, uses, enabled)
>>>> +__uc_state_checker(x, wants, wanted) \
>>>> +__uc_state_checker(x, uses, used)
>>>>   uc_state_checkers(guc);
>>>>   uc_state_checkers(huc);
>>>
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx