[Intel-gfx] [PATCH v2 3/8] drm/i915/uc: Unify uc_fw status tracking

[Michal Wajdeczko]

On Wed, 24 Jul 2019 18:37:52 +0200, Daniele Ceraolo Spurio  
<daniele.ceraolospurio at 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.

>
>>
>>> +    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 ?

>>> +    case INTEL_UC_FIRMWARE_FETCH_FAIL:
>>> +        return "FETCH FAIL";

same here, "fetch" it's name of our internal step,
"MISSING" sounds better imno

>>>      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 ?

>>>      }
>>>      return "<invalid>";
>>>  }