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

[Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>]

On 7/24/2019 5:35 AM, Michal Wajdeczko wrote:
> On Wed, 24 Jul 2019 04:21:48 +0200, Daniele Ceraolo Spurio 
> <daniele.ceraolospurio@xxxxxxxxx> wrote:
>
> > We currently track fetch and load status separately, but the 2 are
> > actually sequential in the uc lifetime (fetch must complete before we
> > can attempt the load!). Unifying the 2 variables we can better follow
> > the sequential states and improve our trackng of the uC state.
> >
> > Also, sprinkle some GEM_BUG_ON to make sure we transition correctly
> > between states.
> >
> > v2: rename states, add the running state (Michal), drop some logs in
> >     the fetch path (Michal, Chris)
> >
> > Suggested-by: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/i915/gt/uc/intel_guc.h        |  4 +-
> >  drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c     |  6 +-
> >  .../gpu/drm/i915/gt/uc/intel_guc_submission.c |  2 +-
> >  drivers/gpu/drm/i915/gt/uc/intel_huc.c        |  8 +-
> >  drivers/gpu/drm/i915/gt/uc/intel_huc.h        |  5 ++
> >  drivers/gpu/drm/i915/gt/uc/intel_uc.c         | 10 +--
> >  drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c      | 86 +++++++------------
> >  drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h      | 58 ++++++++-----
> >  8 files changed, 89 insertions(+), 90 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc.h 
> > b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > index 6852352381ce..f51c4c3c1d0b 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc.h
> > @@ -169,9 +169,9 @@ int intel_guc_suspend(struct intel_guc *guc);
> >  int intel_guc_resume(struct intel_guc *guc);
> >  struct i915_vma *intel_guc_allocate_vma(struct intel_guc *guc, u32 
> > size);
> > -static inline bool intel_guc_is_loaded(struct intel_guc *guc)
> > +static inline bool intel_guc_is_running(struct intel_guc *guc)
> >  {
> > -    return intel_uc_fw_is_loaded(&guc->fw);
> > +    return intel_uc_fw_is_running(&guc->fw);
> >  }
> > static inline int intel_guc_sanitize(struct intel_guc *guc)
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c 
> > b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
> > index a027deb80330..085e7842ef8a 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
> > @@ -232,5 +232,9 @@ static int guc_fw_xfer(struct intel_uc_fw *guc_fw)
> >   */
> >  int intel_guc_fw_upload(struct intel_guc *guc)
> >  {
> > -    return intel_uc_fw_upload(&guc->fw, guc_fw_xfer);
> > +    int ret = intel_uc_fw_upload(&guc->fw, guc_fw_xfer);
> > +    if (!ret)
> > +        guc->fw.status = INTEL_UC_FIRMWARE_RUNNING;
>
> we should already know that in guc_fw_xfer/guc_xfer_ucode
> see below for details
>
> > +
> > +    return ret;
> >  }
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c 
> > b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > index a0f2a01365bc..b4238fe16a03 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_submission.c
> > @@ -941,7 +941,7 @@ static void __guc_client_disable(struct 
> > intel_guc_client *client)
> >       * the case, instead of trying (in vain) to communicate with it, 
> > let's
> >       * just cleanup the doorbell HW and our internal state.
> >       */
> > -    if (intel_guc_is_loaded(client->guc))
> > +    if (intel_guc_is_running(client->guc))
> >          destroy_doorbell(client);
> >      else
> >          __fini_doorbell(client);
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.c 
> > b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> > index ab6c1564b6a7..7804ea5f699c 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.c
> > @@ -117,8 +117,8 @@ int intel_huc_auth(struct intel_huc *huc)
> >      struct intel_guc *guc = &gt->uc.guc;
> >      int ret;
> > -    if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
> > -        return -ENOEXEC;
> > +    GEM_BUG_ON(!intel_uc_fw_is_loaded(&huc->fw));
> > +    GEM_BUG_ON(intel_huc_is_authenticated(huc));
> >     ret = intel_guc_auth_huc(guc,
> >                   intel_guc_ggtt_offset(guc, huc->rsa_data));
> > @@ -138,10 +138,12 @@ int intel_huc_auth(struct intel_huc *huc)
> >          goto fail;
> >      }
> > +    huc->fw.status = INTEL_UC_FIRMWARE_RUNNING;
> > +
> >      return 0;
> > fail:
> > -    huc->fw.load_status = INTEL_UC_FIRMWARE_FAIL;
> > +    huc->fw.status = INTEL_UC_FIRMWARE_LOAD_FAIL;
> >     DRM_ERROR("HuC: Authentication failed %d\n", ret);
> >      return ret;
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_huc.h 
> > b/drivers/gpu/drm/i915/gt/uc/intel_huc.h
> > index 9fa3d4629f2e..ea340f85bc46 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_huc.h
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc.h
> > @@ -56,4 +56,9 @@ static inline int intel_huc_sanitize(struct 
> > intel_huc *huc)
> >      return 0;
> >  }
> > +static inline bool intel_huc_is_authenticated(struct intel_huc *huc)
> > +{
> > +    return intel_uc_fw_is_running(&huc->fw);
> > +}
> > +
> >  #endif
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc.c 
> > b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> > index d60c56fd72e5..b761809946b1 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc.c
> > @@ -559,7 +559,7 @@ void intel_uc_fini_hw(struct intel_uc *uc)
> >  {
> >      struct intel_guc *guc = &uc->guc;
> > -    if (!intel_guc_is_loaded(guc))
> > +    if (!intel_guc_is_running(guc))
> >          return;
> >     GEM_BUG_ON(!intel_uc_fw_supported(&guc->fw));
> > @@ -581,7 +581,7 @@ void intel_uc_reset_prepare(struct intel_uc *uc)
> >  {
> >      struct intel_guc *guc = &uc->guc;
> > -    if (!intel_guc_is_loaded(guc))
> > +    if (!intel_guc_is_running(guc))
> >          return;
> >     guc_stop_communication(guc);
> > @@ -593,7 +593,7 @@ void intel_uc_runtime_suspend(struct intel_uc *uc)
> >      struct intel_guc *guc = &uc->guc;
> >      int err;
> > -    if (!intel_guc_is_loaded(guc))
> > +    if (!intel_guc_is_running(guc))
> >          return;
> >     err = intel_guc_suspend(guc);
> > @@ -608,7 +608,7 @@ void intel_uc_suspend(struct intel_uc *uc)
> >      struct intel_guc *guc = &uc->guc;
> >      intel_wakeref_t wakeref;
> > -    if (!intel_guc_is_loaded(guc))
> > +    if (!intel_guc_is_running(guc))
> >          return;
> >     with_intel_runtime_pm(&uc_to_gt(uc)->i915->runtime_pm, wakeref)
> > @@ -620,7 +620,7 @@ int intel_uc_resume(struct intel_uc *uc)
> >      struct intel_guc *guc = &uc->guc;
> >      int err;
> > -    if (!intel_guc_is_loaded(guc))
> > +    if (!intel_guc_is_running(guc))
> >          return 0;
> >     guc_enable_communication(guc);
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c 
> > b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> > index 48100dff466d..9fc72c2e50d1 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> > @@ -130,7 +130,7 @@ __uc_fw_select(struct intel_uc_fw *uc_fw, enum 
> > intel_platform p, u8 rev)
> >                     fw_blobs[i].first_rev);
> >             uc_fw->path = NULL;
> > -            uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
> > +            uc_fw->status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
> >              return;
> >          }
> >      }
> > @@ -139,15 +139,13 @@ __uc_fw_select(struct intel_uc_fw *uc_fw, enum 
> > intel_platform p, u8 rev)
> >  static void
> >  uc_fw_select(struct drm_i915_private *i915, struct intel_uc_fw *uc_fw)
> >  {
> > -    GEM_BUG_ON(uc_fw->fetch_status != INTEL_UC_FIRMWARE_UNINITIALIZED);
> > +    GEM_BUG_ON(uc_fw->status != INTEL_UC_FIRMWARE_UNINITIALIZED);
> >     if (!HAS_GT_UC(i915)) {
> > -        uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
> > +        uc_fw->status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
> >          return;
> >      }
> > -    uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED;
> > -
> >      if (unlikely(i915_modparams.guc_firmware_path &&
> >               uc_fw->type == INTEL_UC_FW_TYPE_GUC))
> >          uc_fw->path = i915_modparams.guc_firmware_path;
> > @@ -156,6 +154,8 @@ uc_fw_select(struct drm_i915_private *i915, 
> > struct intel_uc_fw *uc_fw)
> >          uc_fw->path = i915_modparams.huc_firmware_path;
> >      else
> >          __uc_fw_select(uc_fw, INTEL_INFO(i915)->platform, 
> > INTEL_REVID(i915));
> > +
> > +    uc_fw->status = INTEL_UC_FIRMWARE_SELECTION_DONE;
> >  }
> > /**
> > @@ -172,14 +172,13 @@ void intel_uc_fw_init_early(struct 
> > drm_i915_private *i915,
> >                  enum intel_uc_fw_type type)
> >  {
> >      /*
> > -     * we use FIRMWARE_UNINITIALIZED to detect checks against 
> > fetch_status
> > +     * we use FIRMWARE_UNINITIALIZED to detect checks against 
> > uc_fw->status
> >       * before we're looked at the HW caps to see if we have uc support
> >       */
> >      BUILD_BUG_ON(INTEL_UC_FIRMWARE_UNINITIALIZED);
> >     uc_fw->path = NULL;
> > -    uc_fw->fetch_status = INTEL_UC_FIRMWARE_UNINITIALIZED;
> > -    uc_fw->load_status = INTEL_UC_FIRMWARE_NOT_STARTED;
> > +    uc_fw->status = INTEL_UC_FIRMWARE_UNINITIALIZED;
> >      uc_fw->type = type;
> >     uc_fw_select(i915, uc_fw);
> > @@ -204,29 +203,11 @@ void intel_uc_fw_fetch(struct drm_i915_private 
> > *dev_priv,
> >      int err;
> >     GEM_BUG_ON(!intel_uc_fw_supported(uc_fw));
> > -
> > -    if (!uc_fw->path) {
> > -        dev_info(dev_priv->drm.dev,
> > -             "%s: No firmware was defined for %s!\n",
> > -             intel_uc_fw_type_repr(uc_fw->type),
> > - intel_platform_name(INTEL_INFO(dev_priv)->platform));
> > -        return;
> > -    }
> > -
> > -    DRM_DEBUG_DRIVER("%s fw fetch %s\n",
> > -             intel_uc_fw_type_repr(uc_fw->type), uc_fw->path);
> > -
> > -    uc_fw->fetch_status = INTEL_UC_FIRMWARE_PENDING;
> > -    DRM_DEBUG_DRIVER("%s fw fetch %s\n",
> > -             intel_uc_fw_type_repr(uc_fw->type),
> > -             intel_uc_fw_status_repr(uc_fw->fetch_status));
> > +    GEM_BUG_ON(!intel_uc_fw_is_selected(uc_fw));
> >     err = request_firmware(&fw, uc_fw->path, &pdev->dev);
> > -    if (err) {
> > -        DRM_DEBUG_DRIVER("%s fw request_firmware err=%d\n",
> > -                 intel_uc_fw_type_repr(uc_fw->type), err);
> > +    if (err)
> >          goto fail;
> > -    }
> >     DRM_DEBUG_DRIVER("%s fw size %zu ptr %p\n",
> >               intel_uc_fw_type_repr(uc_fw->type), fw->size, fw);
> > @@ -328,19 +309,13 @@ void intel_uc_fw_fetch(struct drm_i915_private 
> > *dev_priv,
> >     uc_fw->obj = obj;
> >      uc_fw->size = fw->size;
> > -    uc_fw->fetch_status = INTEL_UC_FIRMWARE_SUCCESS;
> > -    DRM_DEBUG_DRIVER("%s fw fetch %s\n",
> > -             intel_uc_fw_type_repr(uc_fw->type),
> > -             intel_uc_fw_status_repr(uc_fw->fetch_status));
> > +    uc_fw->status = INTEL_UC_FIRMWARE_AVAILABLE;
> >     release_firmware(fw);
> >      return;
> > fail:
> > -    uc_fw->fetch_status = INTEL_UC_FIRMWARE_FAIL;
> > -    DRM_DEBUG_DRIVER("%s fw fetch %s\n",
> > -             intel_uc_fw_type_repr(uc_fw->type),
> > -             intel_uc_fw_status_repr(uc_fw->fetch_status));
> > +    uc_fw->status = INTEL_UC_FIRMWARE_FETCH_FAIL;
> >     DRM_WARN("%s: Failed to fetch firmware %s (error %d)\n",
> >           intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, err);
> > @@ -396,14 +371,11 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
> >      DRM_DEBUG_DRIVER("%s fw load %s\n",
> >               intel_uc_fw_type_repr(uc_fw->type), uc_fw->path);
> > -    if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> > -        return -ENOEXEC;
> > -
> > -    uc_fw->load_status = INTEL_UC_FIRMWARE_PENDING;
> > -    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));
> > +    /* make sure the status was cleared the last time we reset the 
> > uc */
> > +    GEM_BUG_ON(intel_uc_fw_is_loaded(uc_fw));
> > +    if (!intel_uc_fw_is_available(uc_fw))
> > +        return -ENOEXEC;
> >      /* Call custom loader */
> >      intel_uc_fw_ggtt_bind(uc_fw);
> >      err = xfer(uc_fw);
> > @@ -411,10 +383,9 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
> >      if (err)
> >          goto fail;
> > -    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.

> > +    DRM_DEBUG_DRIVER("%s fw load completed\n",
> > +             intel_uc_fw_type_repr(uc_fw->type));
> >     DRM_INFO("%s: Loaded firmware %s (version %u.%u)\n",
> >           intel_uc_fw_type_repr(uc_fw->type),
> > @@ -424,10 +395,9 @@ int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,
> >      return 0;
> > fail:
> > -    uc_fw->load_status = INTEL_UC_FIRMWARE_FAIL;
> > -    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_LOAD_FAIL;
> > +    DRM_DEBUG_DRIVER("%s fw load failed\n",
> > +             intel_uc_fw_type_repr(uc_fw->type));
> >     DRM_WARN("%s: Failed to load firmware %s (error %d)\n",
> >           intel_uc_fw_type_repr(uc_fw->type), uc_fw->path, err);
> > @@ -439,7 +409,10 @@ int intel_uc_fw_init(struct intel_uc_fw *uc_fw)
> >  {
> >      int err;
> > -    if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> > +    /* this should happen before the load! */
> > +    GEM_BUG_ON(intel_uc_fw_is_loaded(uc_fw));
> > +
> > +    if (!intel_uc_fw_is_available(uc_fw))
> >          return -ENOEXEC;
> >     err = i915_gem_object_pin_pages(uc_fw->obj);
> > @@ -452,7 +425,7 @@ int intel_uc_fw_init(struct intel_uc_fw *uc_fw)
> > void intel_uc_fw_fini(struct intel_uc_fw *uc_fw)
> >  {
> > -    if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> > +    if (!intel_uc_fw_is_available(uc_fw))
> >          return;
> >     i915_gem_object_unpin_pages(uc_fw->obj);
> > @@ -486,7 +459,7 @@ void intel_uc_fw_cleanup_fetch(struct intel_uc_fw 
> > *uc_fw)
> >      if (obj)
> >          i915_gem_object_put(obj);
> > -    uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED;
> > +    uc_fw->status = INTEL_UC_FIRMWARE_SELECTION_DONE;
> >  }
> > /**
> > @@ -500,9 +473,8 @@ void intel_uc_fw_dump(const struct intel_uc_fw 
> > *uc_fw, struct drm_printer *p)
> >  {
> >      drm_printf(p, "%s firmware: %s\n",
> >             intel_uc_fw_type_repr(uc_fw->type), uc_fw->path);
> > -    drm_printf(p, "\tstatus: fetch %s, load %s\n",
> > -           intel_uc_fw_status_repr(uc_fw->fetch_status),
> > -           intel_uc_fw_status_repr(uc_fw->load_status));
> > +    drm_printf(p, "\tstatus: %s\n",
> > +           intel_uc_fw_status_repr(uc_fw->status));
> >      drm_printf(p, "\tversion: wanted %u.%u, found %u.%u\n",
> >             uc_fw->major_ver_wanted, uc_fw->minor_ver_wanted,
> >             uc_fw->major_ver_found, uc_fw->minor_ver_found);
> > diff --git a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h 
> > b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> > index c2868ef15eda..ecdec4320260 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> > @@ -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.

> > +    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";
> > +    case INTEL_UC_FIRMWARE_FETCH_FAIL:
> > +        return "FETCH FAIL";
> >      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.

> > +    case INTEL_UC_FIRMWARE_AVAILABLE:
> > +        return "AVAILABLE";
> > +    case INTEL_UC_FIRMWARE_LOADED:
> > +        return "LOADED";
> > +    case INTEL_UC_FIRMWARE_RUNNING:
> > +        return "RUNNING";
> >      }
> >      return "<invalid>";
> >  }
> > @@ -113,25 +118,36 @@ static inline const char 
> > *intel_uc_fw_type_repr(enum intel_uc_fw_type type)
> > static inline bool intel_uc_fw_is_selected(struct intel_uc_fw *uc_fw)
> >  {
> > +    GEM_BUG_ON(uc_fw->path && uc_fw->status < 
> > INTEL_UC_FIRMWARE_SELECTION_DONE);
> >      return uc_fw->path != NULL;
> >  }
> > +static inline bool intel_uc_fw_is_available(struct intel_uc_fw *uc_fw)
> > +{
> > +    return uc_fw->status >= INTEL_UC_FIRMWARE_AVAILABLE;
> > +}
> > +
> >  static inline bool intel_uc_fw_is_loaded(struct intel_uc_fw *uc_fw)
> >  {
> > -    return uc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS;
> > +    return uc_fw->status >= INTEL_UC_FIRMWARE_LOADED;
> > +}
> > +
> > +static inline bool intel_uc_fw_is_running(struct intel_uc_fw *uc_fw)
> > +{
> > +    return uc_fw->status == INTEL_UC_FIRMWARE_RUNNING;
> >  }
> > static inline bool intel_uc_fw_supported(struct intel_uc_fw *uc_fw)
> >  {
> >      /* shouldn't call this before checking hw/blob availability */
> > -    GEM_BUG_ON(uc_fw->fetch_status == INTEL_UC_FIRMWARE_UNINITIALIZED);
> > -    return uc_fw->fetch_status != INTEL_UC_FIRMWARE_NOT_SUPPORTED;
> > +    GEM_BUG_ON(uc_fw->status == INTEL_UC_FIRMWARE_UNINITIALIZED);
>
> shouldn't we have this check on all uc_fw_is_xxx() functions ?

I can add that in.

Daniele

> > +    return uc_fw->status != INTEL_UC_FIRMWARE_NOT_SUPPORTED;
> >  }
> > static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw)
> >  {
> >      if (intel_uc_fw_is_loaded(uc_fw))
> > -        uc_fw->load_status = INTEL_UC_FIRMWARE_PENDING;
> > +        uc_fw->status = INTEL_UC_FIRMWARE_AVAILABLE;
> >  }
> > /**
> > @@ -144,7 +160,7 @@ static inline void intel_uc_fw_sanitize(struct 
> > intel_uc_fw *uc_fw)
> >   */
> >  static inline u32 intel_uc_fw_get_upload_size(struct intel_uc_fw 
> > *uc_fw)
> >  {
> > -    if (uc_fw->fetch_status != INTEL_UC_FIRMWARE_SUCCESS)
> > +    if (!intel_uc_fw_is_available(uc_fw))
> >          return 0;
> >     return uc_fw->header_size + uc_fw->ucode_size;

_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx