Re: [PATCH v2 3/8] drm/i915/uc: Unify uc_fw status tracking

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

On 7/24/19 10:24 AM, Michal Wajdeczko wrote:
> On Wed, 24 Jul 2019 18:37:52 +0200, Daniele Ceraolo Spurio 
> <daniele.ceraolospurio@intel.com> wrote:
> 
>>>> -    uc_fw->load_status = INTEL_UC_FIRMWARE_SUCCESS;
>>>> -    DRM_DEBUG_DRIVER("%s fw load %s\n",
>>>> -             intel_uc_fw_type_repr(uc_fw->type),
>>>> -             intel_uc_fw_status_repr(uc_fw->load_status));
>>>> +    uc_fw->status = INTEL_UC_FIRMWARE_LOADED;
>>>
>>> maybe we can slightly modify xfer function agreement and use
>>> -EINPROGRESS to indicate whether fw is just loaded (HuC) or
>>> is already authenticated and running (GuC):
>>>
>>>     if (!err)
>>>         uc_fw->status = INTEL_UC_FIRMWARE_RUNNING;
>>>     else if (err == -EINPROGRESS)
>>>         uc_fw->status = INTEL_UC_FIRMWARE_LOADED;
>>>     else
>>>         goto fail;
>>>
>>
>> I've purposely kept the RUNNING state outside because in patch 8 I 
>> move the wait outside the xfer, so the switch to the running state 
>> will be done outside of here for both uC. Seemed like less churn to go 
>> directly with that.
> 
> ok, I missed that move in diff 8/8
> 
> 
>>>> @@ -35,12 +35,14 @@ struct drm_i915_private;
>>>>  #define INTEL_UC_FIRMWARE_URL 
>>>> "https://git.kernel.org/pub/scm/linux/kernel/git/firmware/linux-firmware.git/tree/i915" 
>>>>
>>>> enum intel_uc_fw_status {
>>>> -    INTEL_UC_FIRMWARE_NOT_SUPPORTED = -2, /* no uc HW */
>>>> -    INTEL_UC_FIRMWARE_FAIL = -1,
>>>> +    INTEL_UC_FIRMWARE_LOAD_FAIL = -3,
>>>> +    INTEL_UC_FIRMWARE_FETCH_FAIL = -2,
>>>> +    INTEL_UC_FIRMWARE_NOT_SUPPORTED = -1, /* no uc HW */
>>>>      INTEL_UC_FIRMWARE_UNINITIALIZED = 0, /* used to catch checks 
>>>> done too early */
>>>> -    INTEL_UC_FIRMWARE_NOT_STARTED = 1,
>>>> -    INTEL_UC_FIRMWARE_PENDING,
>>>> -    INTEL_UC_FIRMWARE_SUCCESS
>>>> +    INTEL_UC_FIRMWARE_SELECTION_DONE, /* selection include the "no 
>>>> FW" case */
>>>
>>> why do you want to keep "No FW" case here ?
>>> when we know that there is no fw, we should not attempt to fetch it.
>>> so this is different state than "fw was selected, awaiting fetch"
>>
>> We need a way to differentiate for the logging and I didn't want an 
>> extra state since we check fw->path anyway to make sure the fw was 
>> actually selected.
> 
> But "N/A" state also means that we already pass over init_early step
> that includes selection, so we don't need to add any extra state.
> 

Yes, but we wouldn't know if N/A was set because we are on a platform 
with no uC HW or because the FW was not defined. I'm going to drop that 
distinction in the logs and be done with it, it's quite easy to find out 
based on the gen anyway (anything gen9+ has GuC/HuC HW)

>>
>>>
>>>> +    INTEL_UC_FIRMWARE_AVAILABLE, /* fetch done */
>>>> +    INTEL_UC_FIRMWARE_LOADED, /* dma xfer done */
>>>> +    INTEL_UC_FIRMWARE_RUNNING /* fw init/auth done */
>>>>  };
>>>> enum intel_uc_fw_type {
>>>> @@ -57,8 +59,7 @@ struct intel_uc_fw {
>>>>      const char *path;
>>>>      size_t size;
>>>>      struct drm_i915_gem_object *obj;
>>>> -    enum intel_uc_fw_status fetch_status;
>>>> -    enum intel_uc_fw_status load_status;
>>>> +    enum intel_uc_fw_status status;
>>>>     /*
>>>>       * The firmware build process will generate a version header 
>>>> file with major and
>>>> @@ -83,18 +84,22 @@ static inline
>>>>  const char *intel_uc_fw_status_repr(enum intel_uc_fw_status status)
>>>>  {
>>>>      switch (status) {
>>>> +    case INTEL_UC_FIRMWARE_LOAD_FAIL:
>>>> +        return "LOAD FAIL";
> 
> sorry for second thoughts, but with these names we could have:
> 
> LOADED (user: hurray!) --> NOT_LOADED (user: but we were already loaded?!?)
> 
> so maybe plain "FAIL" as this is user facing status ?
> 

ok

>>>> +    case INTEL_UC_FIRMWARE_FETCH_FAIL:
>>>> +        return "FETCH FAIL";
> 
> same here, "fetch" it's name of our internal step,
> "MISSING" sounds better imno
> 

ok

>>>>      case INTEL_UC_FIRMWARE_NOT_SUPPORTED:
>>>> -        return "N/A - uc HW not available";
>>>> -    case INTEL_UC_FIRMWARE_FAIL:
>>>> -        return "FAIL";
>>>> +        return "N/A";
>>>>      case INTEL_UC_FIRMWARE_UNINITIALIZED:
>>>>          return "UNINITIALIZED";
>>>> -    case INTEL_UC_FIRMWARE_NOT_STARTED:
>>>> -        return "NOT_STARTED";
>>>> -    case INTEL_UC_FIRMWARE_PENDING:
>>>> -        return "PENDING";
>>>> -    case INTEL_UC_FIRMWARE_SUCCESS:
>>>> -        return "SUCCESS";
>>>> +    case INTEL_UC_FIRMWARE_SELECTION_DONE:
>>>> +        return "SELECTION DONE";
>>>
>>> nit: this is not my favorite, what was wrong with
>>> "PENDING" (known, awaiting fetch/load, look it's transient state!)
>>> "SELECTED" (shorter, applies to this fw object vs step)
>>
>> I wanted to highlight the fact that the selection included the "no FW" 
>> case, the fw wasn't necessarily "selected". We just know that we've 
>> run through the selection code.
> 
> but from the user pov this is internal detail, not sure if we should expose
> that, on other hand, PENDING clearly indicates that we are still going 
> to do
> something with that firmware (fetch/xfer/auth) until we reach end state.
> 
>>
>>>
>>>> +    case INTEL_UC_FIRMWARE_AVAILABLE:
>>>> +        return "AVAILABLE";
>>>> +    case INTEL_UC_FIRMWARE_LOADED:
>>>> +        return "LOADED";
>>>> +    case INTEL_UC_FIRMWARE_RUNNING:
>>>> +        return "RUNNING";
> 
> hmm, the difference between LOADED/RUNNING might be unnoticed by the
> user, as he may also treat LOADED as full success.
> 
> so maybe s/LOADED/TRANSFERRED ?
> 

ok

Daniele

>>>>      }
>>>>      return "<invalid>";
>>>>  }
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx