Re: [PATCH v2 2/8] drm/i915/uc: Unify uC FW selection

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

On 7/24/2019 4:31 AM, Michal Wajdeczko wrote:
> On Wed, 24 Jul 2019 04:21:47 +0200, Daniele Ceraolo Spurio 
> <daniele.ceraolospurio@xxxxxxxxx> wrote:
>
> > Instead of having 2 identical functions for GuC and HuC firmware
> > selection, we can unify the selection logic and just use different lists
> > based on FW type.
> >
> > Note that the revid is not relevant for current blobs, but the upcoming
> > CML will be identified as CFL rev 5, so by considering the revid we're
> > ready for that.
> >
> > v2: rework blob list defs (Michal), add order check (Chris), fuse GuC
> >     and HuC lists into one.
> >
> > Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> > Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> > Cc: Anusha Srivatsa <anusha.srivatsa@xxxxxxxxx>
> > Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> > ---
> >  drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c |  86 +-----------
> >  drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c |  88 +-----------
> >  drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c  | 156 ++++++++++++++++++++++
> >  drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h  |  29 ++--
> >  4 files changed, 167 insertions(+), 192 deletions(-)
> >
> > 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 87169e826747..a027deb80330 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_fw.c
> > @@ -31,89 +31,6 @@
> >  #include "intel_guc_fw.h"
> >  #include "i915_drv.h"
> > -#define __MAKE_GUC_FW_PATH(KEY) \
> > -    "i915/" \
> > -    __stringify(KEY##_GUC_FW_PREFIX) "_guc_" \
> > -    __stringify(KEY##_GUC_FW_MAJOR) "." \
> > -    __stringify(KEY##_GUC_FW_MINOR) "." \
> > -    __stringify(KEY##_GUC_FW_PATCH) ".bin"
> > -
> > -#define SKL_GUC_FW_PREFIX skl
> > -#define SKL_GUC_FW_MAJOR 33
> > -#define SKL_GUC_FW_MINOR 0
> > -#define SKL_GUC_FW_PATCH 0
> > -#define SKL_GUC_FIRMWARE_PATH __MAKE_GUC_FW_PATH(SKL)
> > -MODULE_FIRMWARE(SKL_GUC_FIRMWARE_PATH);
> > -
> > -#define BXT_GUC_FW_PREFIX bxt
> > -#define BXT_GUC_FW_MAJOR 33
> > -#define BXT_GUC_FW_MINOR 0
> > -#define BXT_GUC_FW_PATCH 0
> > -#define BXT_GUC_FIRMWARE_PATH __MAKE_GUC_FW_PATH(BXT)
> > -MODULE_FIRMWARE(BXT_GUC_FIRMWARE_PATH);
> > -
> > -#define KBL_GUC_FW_PREFIX kbl
> > -#define KBL_GUC_FW_MAJOR 33
> > -#define KBL_GUC_FW_MINOR 0
> > -#define KBL_GUC_FW_PATCH 0
> > -#define KBL_GUC_FIRMWARE_PATH __MAKE_GUC_FW_PATH(KBL)
> > -MODULE_FIRMWARE(KBL_GUC_FIRMWARE_PATH);
> > -
> > -#define GLK_GUC_FW_PREFIX glk
> > -#define GLK_GUC_FW_MAJOR 33
> > -#define GLK_GUC_FW_MINOR 0
> > -#define GLK_GUC_FW_PATCH 0
> > -#define GLK_GUC_FIRMWARE_PATH __MAKE_GUC_FW_PATH(GLK)
> > -MODULE_FIRMWARE(GLK_GUC_FIRMWARE_PATH);
> > -
> > -#define ICL_GUC_FW_PREFIX icl
> > -#define ICL_GUC_FW_MAJOR 33
> > -#define ICL_GUC_FW_MINOR 0
> > -#define ICL_GUC_FW_PATCH 0
> > -#define ICL_GUC_FIRMWARE_PATH __MAKE_GUC_FW_PATH(ICL)
> > -MODULE_FIRMWARE(ICL_GUC_FIRMWARE_PATH);
> > -
> > -static void guc_fw_select(struct intel_uc_fw *guc_fw)
> > -{
> > -    struct intel_guc *guc = container_of(guc_fw, struct intel_guc, fw);
> > -    struct drm_i915_private *i915 = guc_to_gt(guc)->i915;
> > -
> > -    GEM_BUG_ON(guc_fw->type != INTEL_UC_FW_TYPE_GUC);
> > -
> > -    if (!HAS_GT_UC(i915)) {
> > -        guc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
> > -        return;
> > -    }
> > -
> > -    guc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED;
> > -
> > -    if (i915_modparams.guc_firmware_path) {
> > -        guc_fw->path = i915_modparams.guc_firmware_path;
> > -        guc_fw->major_ver_wanted = 0;
> > -        guc_fw->minor_ver_wanted = 0;
> > -    } else if (IS_ICELAKE(i915)) {
> > -        guc_fw->path = ICL_GUC_FIRMWARE_PATH;
> > -        guc_fw->major_ver_wanted = ICL_GUC_FW_MAJOR;
> > -        guc_fw->minor_ver_wanted = ICL_GUC_FW_MINOR;
> > -    } else if (IS_GEMINILAKE(i915)) {
> > -        guc_fw->path = GLK_GUC_FIRMWARE_PATH;
> > -        guc_fw->major_ver_wanted = GLK_GUC_FW_MAJOR;
> > -        guc_fw->minor_ver_wanted = GLK_GUC_FW_MINOR;
> > -    } else if (IS_KABYLAKE(i915) || IS_COFFEELAKE(i915)) {
> > -        guc_fw->path = KBL_GUC_FIRMWARE_PATH;
> > -        guc_fw->major_ver_wanted = KBL_GUC_FW_MAJOR;
> > -        guc_fw->minor_ver_wanted = KBL_GUC_FW_MINOR;
> > -    } else if (IS_BROXTON(i915)) {
> > -        guc_fw->path = BXT_GUC_FIRMWARE_PATH;
> > -        guc_fw->major_ver_wanted = BXT_GUC_FW_MAJOR;
> > -        guc_fw->minor_ver_wanted = BXT_GUC_FW_MINOR;
> > -    } else if (IS_SKYLAKE(i915)) {
> > -        guc_fw->path = SKL_GUC_FIRMWARE_PATH;
> > -        guc_fw->major_ver_wanted = SKL_GUC_FW_MAJOR;
> > -        guc_fw->minor_ver_wanted = SKL_GUC_FW_MINOR;
> > -    }
> > -}
> > -
> >  /**
> >   * intel_guc_fw_init_early() - initializes GuC firmware struct
> >   * @guc: intel_guc struct
> > @@ -124,8 +41,7 @@ void intel_guc_fw_init_early(struct intel_guc *guc)
> >  {
> >      struct intel_uc_fw *guc_fw = &guc->fw;
> > -    intel_uc_fw_init_early(guc_fw, INTEL_UC_FW_TYPE_GUC);
> > -    guc_fw_select(guc_fw);
> > +    intel_uc_fw_init_early(guc_to_gt(guc)->i915, guc_fw, 
> > INTEL_UC_FW_TYPE_GUC);
> >  }
> > static void guc_prepare_xfer(struct intel_guc *guc)
> > 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 ff6f7b157ecb..fa2151fa3a13 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_huc_fw.c
> > @@ -23,91 +23,6 @@
> >   * Note that HuC firmware loading must be done before GuC loading.
> >   */
> > -#define BXT_HUC_FW_MAJOR 01
> > -#define BXT_HUC_FW_MINOR 8
> > -#define BXT_BLD_NUM 2893
> > -
> > -#define SKL_HUC_FW_MAJOR 01
> > -#define SKL_HUC_FW_MINOR 07
> > -#define SKL_BLD_NUM 1398
> > -
> > -#define KBL_HUC_FW_MAJOR 02
> > -#define KBL_HUC_FW_MINOR 00
> > -#define KBL_BLD_NUM 1810
> > -
> > -#define GLK_HUC_FW_MAJOR 03
> > -#define GLK_HUC_FW_MINOR 01
> > -#define GLK_BLD_NUM 2893
> > -
> > -#define ICL_HUC_FW_MAJOR 8
> > -#define ICL_HUC_FW_MINOR 4
> > -#define ICL_BLD_NUM 3238
> > -
> > -#define HUC_FW_PATH(platform, major, minor, bld_num) \
> > -    "i915/" __stringify(platform) "_huc_ver" __stringify(major) "_" \
> > -    __stringify(minor) "_" __stringify(bld_num) ".bin"
> > -
> > -#define I915_SKL_HUC_UCODE HUC_FW_PATH(skl, SKL_HUC_FW_MAJOR, \
> > -    SKL_HUC_FW_MINOR, SKL_BLD_NUM)
> > -MODULE_FIRMWARE(I915_SKL_HUC_UCODE);
> > -
> > -#define I915_BXT_HUC_UCODE HUC_FW_PATH(bxt, BXT_HUC_FW_MAJOR, \
> > -    BXT_HUC_FW_MINOR, BXT_BLD_NUM)
> > -MODULE_FIRMWARE(I915_BXT_HUC_UCODE);
> > -
> > -#define I915_KBL_HUC_UCODE HUC_FW_PATH(kbl, KBL_HUC_FW_MAJOR, \
> > -    KBL_HUC_FW_MINOR, KBL_BLD_NUM)
> > -MODULE_FIRMWARE(I915_KBL_HUC_UCODE);
> > -
> > -#define I915_GLK_HUC_UCODE HUC_FW_PATH(glk, GLK_HUC_FW_MAJOR, \
> > -    GLK_HUC_FW_MINOR, GLK_BLD_NUM)
> > -MODULE_FIRMWARE(I915_GLK_HUC_UCODE);
> > -
> > -#define I915_ICL_HUC_UCODE HUC_FW_PATH(icl, ICL_HUC_FW_MAJOR, \
> > -    ICL_HUC_FW_MINOR, ICL_BLD_NUM)
> > -MODULE_FIRMWARE(I915_ICL_HUC_UCODE);
> > -
> > -static void huc_fw_select(struct intel_uc_fw *huc_fw)
> > -{
> > -    struct intel_huc *huc = container_of(huc_fw, struct intel_huc, fw);
> > -    struct drm_i915_private *dev_priv = huc_to_gt(huc)->i915;
> > -
> > -    GEM_BUG_ON(huc_fw->type != INTEL_UC_FW_TYPE_HUC);
> > -
> > -    if (!HAS_GT_UC(dev_priv)) {
> > -        huc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
> > -        return;
> > -    }
> > -
> > -    huc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED;
> > -
> > -    if (i915_modparams.huc_firmware_path) {
> > -        huc_fw->path = i915_modparams.huc_firmware_path;
> > -        huc_fw->major_ver_wanted = 0;
> > -        huc_fw->minor_ver_wanted = 0;
> > -    } else if (IS_SKYLAKE(dev_priv)) {
> > -        huc_fw->path = I915_SKL_HUC_UCODE;
> > -        huc_fw->major_ver_wanted = SKL_HUC_FW_MAJOR;
> > -        huc_fw->minor_ver_wanted = SKL_HUC_FW_MINOR;
> > -    } else if (IS_BROXTON(dev_priv)) {
> > -        huc_fw->path = I915_BXT_HUC_UCODE;
> > -        huc_fw->major_ver_wanted = BXT_HUC_FW_MAJOR;
> > -        huc_fw->minor_ver_wanted = BXT_HUC_FW_MINOR;
> > -    } else if (IS_KABYLAKE(dev_priv) || IS_COFFEELAKE(dev_priv)) {
> > -        huc_fw->path = I915_KBL_HUC_UCODE;
> > -        huc_fw->major_ver_wanted = KBL_HUC_FW_MAJOR;
> > -        huc_fw->minor_ver_wanted = KBL_HUC_FW_MINOR;
> > -    } else if (IS_GEMINILAKE(dev_priv)) {
> > -        huc_fw->path = I915_GLK_HUC_UCODE;
> > -        huc_fw->major_ver_wanted = GLK_HUC_FW_MAJOR;
> > -        huc_fw->minor_ver_wanted = GLK_HUC_FW_MINOR;
> > -    } else if (IS_ICELAKE(dev_priv)) {
> > -        huc_fw->path = I915_ICL_HUC_UCODE;
> > -        huc_fw->major_ver_wanted = ICL_HUC_FW_MAJOR;
> > -        huc_fw->minor_ver_wanted = ICL_HUC_FW_MINOR;
> > -    }
> > -}
> > -
> >  /**
> >   * intel_huc_fw_init_early() - initializes HuC firmware struct
> >   * @huc: intel_huc struct
> > @@ -118,8 +33,7 @@ void intel_huc_fw_init_early(struct intel_huc *huc)
> >  {
> >      struct intel_uc_fw *huc_fw = &huc->fw;
> > -    intel_uc_fw_init_early(huc_fw, INTEL_UC_FW_TYPE_HUC);
> > -    huc_fw_select(huc_fw);
> > +    intel_uc_fw_init_early(huc_to_gt(huc)->i915, huc_fw, 
> > INTEL_UC_FW_TYPE_HUC);
> >  }
> > static void huc_xfer_rsa(struct intel_huc *huc)
> > 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 8ce7210907c0..48100dff466d 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> > @@ -29,6 +29,162 @@
> >  #include "intel_uc_fw.h"
> >  #include "i915_drv.h"
> > +/*
> > + * List of required GuC and HuC binaries per-platform.
> > + * Must be ordered based on platform + revid, from newer to older.
> > + */
> > +#define INTEL_UC_FIRMWARE_DEFS(fw_def, guc_def, huc_def) \
> > +    fw_def(ICELAKE,    0, guc_def(icl, 33, 0, 0), huc_def(icl, 8,  
> > 4, 3238)) \
> > +    fw_def(COFFEELAKE, 0, guc_def(kbl, 33, 0, 0), huc_def(kbl, 02, 
> > 00, 1810)) \
> > +    fw_def(GEMINILAKE, 0, guc_def(glk, 33, 0, 0), huc_def(glk, 03, 
> > 01, 2893)) \
> > +    fw_def(KABYLAKE,   0, guc_def(kbl, 33, 0, 0), huc_def(kbl, 02, 
> > 00, 1810)) \
> > +    fw_def(BROXTON,    0, guc_def(bxt, 33, 0, 0), huc_def(bxt, 01,  
> > 8, 2893)) \
> > +    fw_def(SKYLAKE,    0, guc_def(skl, 33, 0, 0), huc_def(skl, 01, 
> > 07, 1398))
> > +
> > +#define __MAKE_UC_FW_PATH(prefix_, name_, separator_, major_, 
> > minor_, patch_) \
> > +    "i915/" \
> > +    __stringify(prefix_) name_ \
> > +    __stringify(major_) separator_ \
> > +    __stringify(minor_) separator_ \
> > +    __stringify(patch_) ".bin"
> > +
> > +#define MAKE_GUC_FW_PATH(prefix_, major_, minor_, patch_) \
> > +    __MAKE_UC_FW_PATH(prefix_, "_guc_", ".", major_, minor_, patch_)
> > +
> > +#define MAKE_HUC_FW_PATH(prefix_, major_, minor_, bld_num_) \
> > +    __MAKE_UC_FW_PATH(prefix_, "_huc_ver", "_", major_, minor_, 
> > bld_num_)
> > +
> > +/* All blobs need to be declared via MODULE_FIRMWARE() */
> > +#define INTEL_UC_MODULE_FW(platform_, revid_, guc_, huc_) \
> > +    MODULE_FIRMWARE(guc_); \
> > +    MODULE_FIRMWARE(huc_);
> > +
> > +INTEL_UC_FIRMWARE_DEFS(INTEL_UC_MODULE_FW, MAKE_GUC_FW_PATH, 
> > MAKE_HUC_FW_PATH)
> > +
> > +/*
> > + * The below defs and macros are used to iterate across the list of 
> > blobs. See
> > + * __uc_fw_select() below for details.
> > + */
> > +struct __packed intel_uc_fw_blob {
> > +    u8 major;
> > +    u8 minor;
>
> HuC firmware is using 16 bits for major/minor but I guess we have
> some time until we get to version 255
>
> > +    const char *path;
> > +};
> > +
> > +#define UC_FW_BLOB(major_, minor_, path_) \
> > +    { .major = major_, .minor = minor_, .path = path_ }
> > +
> > +#define GUC_FW_BLOB(prefix_, major_, minor_, patch_) \
> > +    UC_FW_BLOB(major_, minor_, \
> > +           MAKE_GUC_FW_PATH(prefix_, major_, minor_, patch_))
> > +
> > +#define HUC_FW_BLOB(prefix_, major_, minor_, bld_num_) \
> > +    UC_FW_BLOB(major_, minor_, \
> > +           MAKE_HUC_FW_PATH(prefix_, major_, minor_, bld_num_))
> > +
> > +#define MAKE_FW_LIST(platform_, revid_, guc_, huc_) \
> > +{ \
> > +    .p = INTEL_##platform_, \
> > +    .first_rev = revid_, \
> > +    .blobs[INTEL_UC_FW_TYPE_GUC] = guc_, \
> > +    .blobs[INTEL_UC_FW_TYPE_HUC] = huc_, \
> > +},
>
> nit: unnamed struct on which above macro operates is defined inside
> function below - either keep this macro private to the function or
> move struct definition outside function,
>
> btw, above you've already provided definition for struct intel_uc_fw_blob
>
> > +
> > +static void
> > +__uc_fw_select(struct intel_uc_fw *uc_fw, enum intel_platform p, u8 
> > rev)
> > +{
> > +    static const struct __packed {
> > +        enum intel_platform p;
>
> nit: using full "platform" word instead of "p" will not hurt
>
> > +        u8 first_rev;
>
> nit: maybe just "rev", from comment above we know that this is first one
>
> > +        const struct intel_uc_fw_blob blobs[INTEL_UC_NUM_TYPES];
> > +    } fw_blobs[] = {
> > +        INTEL_UC_FIRMWARE_DEFS(MAKE_FW_LIST, GUC_FW_BLOB, HUC_FW_BLOB)
> > +    };
> > +    int i;
> > +
> > +    for (i = 0; i < ARRAY_SIZE(fw_blobs) && p <= fw_blobs[i].p; i++) {
> > +        if (p == fw_blobs[i].p && rev >= fw_blobs[i].first_rev) {
> > +            const struct intel_uc_fw_blob *blob =
> > +                    &fw_blobs[i].blobs[uc_fw->type];
> > +            uc_fw->path = blob->path;
> > +            uc_fw->major_ver_wanted = blob->major;
> > +            uc_fw->minor_ver_wanted = blob->minor;
> > +            break;
> > +        }
> > +    }
> > +
> > +    /* make sure the list is ordered as expected */
> > +    if (IS_ENABLED(CONFIG_DRM_I915_SELFTEST)) {
> > +        for (i = 1; i < ARRAY_SIZE(fw_blobs); i++) {
> > +            if (fw_blobs[i].p < fw_blobs[i - 1].p)
> > +                continue;
> > +
> > +            if (fw_blobs[i].p == fw_blobs[i - 1].p &&
> > +                fw_blobs[i].first_rev < fw_blobs[i - 1].first_rev)
> > +                continue;
> > +
> > +            pr_err("invalid FW blob order: %s r%u comes before %s 
> > r%u\n",
> > +                   intel_platform_name(fw_blobs[i - 1].p),
> > +                   fw_blobs[i - 1].first_rev,
> > +                   intel_platform_name(fw_blobs[i].p),
> > +                   fw_blobs[i].first_rev);
> > +
> > +            uc_fw->path = NULL;
> > +            uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
>
> if we reorder this selftest with actual search then maybe we can skip
> resetting path (btw, major/minor are untouched) and return

Actually better to always scan the whole table (as you mentioned below)

> > +            return;
>
> maybe for full selftest we should scan whole table instead of returning
> on first mismatch ?
>
> > +        }
> > +    }
> > +}
> > +
> > +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);
> > +
> > +    if (!HAS_GT_UC(i915)) {
> > +        uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
>
> maybe we should check this in 'init_early' before calling 'select' 
> function ?
>
> > +        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;
> > +    else if (unlikely(i915_modparams.huc_firmware_path &&
> > +              uc_fw->type == INTEL_UC_FW_TYPE_HUC))
> > +        uc_fw->path = i915_modparams.huc_firmware_path;
> > +    else
> > +        __uc_fw_select(uc_fw, INTEL_INFO(i915)->platform, 
> > INTEL_REVID(i915));
>
> if we don't select anything (no override, no hardcoded path) then we
> will stay with fetch_status = NOT_STARTED, but I think for such case
> we should use NOT_SUPPORTED

I'm not changing any of the status flow in this patch ;)

Anyway, I agree with you, but we can't do that at the moment as we still 
need a way to differentiate between no uC HW (NOT_SUPPORTED) and HW 
available but no FW (NOT_STARTED/SELECTION_DONE && path == NULL) for the 
error logging in sanitize options. As we've discussed offline, Once 
we've cleaned up the sanitizing we can make those 2 states converge into 
one. Alternatively, we can just drop the log difference since the user 
won't really care why the support is not there and a developer should be 
able to quickly spot the reason. Thoughts?

> > +}
> > +
> > +/**
> > + * intel_uc_fw_init_early - initialize the uC object and select the 
> > firmware
> > + * @i915: device private
> > + * @uc_fw: uC firmware
> > + * @type: type of uC
> > + *
> > + * Initialize the state of our uC object and relevant tracking and 
> > select the
> > + * firmware to fetch and load.
> > + */
> > +void intel_uc_fw_init_early(struct drm_i915_private *i915,
> > +                struct intel_uc_fw *uc_fw,
> > +                enum intel_uc_fw_type type)
>
> void intel_uc_fw_init_early(
>     struct intel_uc_fw *uc_fw,
>     enum intel_uc_fw_type type,
>     struct drm_i915_private *i915)
>
> to use correct object/subject ordering

Will flip. I had it like this to match the intel_uc_fw_fetch function.

> > +{
> > +    /*
> > +     * we use FIRMWARE_UNINITIALIZED to detect checks against 
> > fetch_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->type = type;
> > +
> > +    uc_fw_select(i915, uc_fw);
>
> maybe:
>     BUILD_BUG_ON(INTEL_UC_FIRMWARE_UNINITIALIZED);
>     GEM_BUG_ON(uc_fw->fetch_status);
>     GEM_BUG_ON(uc_fw->load_status);
>     GEM_BUG_ON(uc_fw->path);
>
>     uc_fw->type = type;
>
>     if (HAS_GT_UC(i915) && !__uc_fw_override(uc_fw))
>         __uc_fw_auto_select(uc_fw, INTEL_INFO(i915)->platform, 
> INTEL_REVID(i915));
>
>     if (uc_fw->path) {
>         uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED;
>         uc_fw->load_status = INTEL_UC_FIRMWARE_NOT_STARTED;
>     } else {
>         uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
>         uc_fw->load_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
>     }

I can't do it like this for the logging reasons mentioned above, but I 
can rework it as:

    if (!HAS_GT_UC(i915)) {
        uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_SUPPORTED;
        return;
    }

    if(!__uc_fw_override(uc_fw))
        __uc_fw_auto_select(uc_fw, INTEL_INFO(i915)->platform, 
INTEL_REVID(i915));

    uc_fw->fetch_status = INTEL_UC_FIRMWARE_NOT_STARTED;

    return;

> > +}
> > +
> >  /**
> >   * intel_uc_fw_fetch - fetch uC firmware
> >   *
> > 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 833d04d06576..c2868ef15eda 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.h
> > @@ -44,8 +44,9 @@ enum intel_uc_fw_status {
> >  };
> > enum intel_uc_fw_type {
> > -    INTEL_UC_FW_TYPE_GUC,
> > -    INTEL_UC_FW_TYPE_HUC
> > +    INTEL_UC_FW_TYPE_GUC = 0,
> > +    INTEL_UC_FW_TYPE_HUC,
> > +    INTEL_UC_NUM_TYPES
>
> this is bad idea, as now we have NUM_TYPES as valid fw type
> #define INTEL_UC_NUM_TYPES 2

ok

Daniele

> >  };
> > /*
> > @@ -105,24 +106,9 @@ static inline const char 
> > *intel_uc_fw_type_repr(enum intel_uc_fw_type type)
> >          return "GuC";
> >      case INTEL_UC_FW_TYPE_HUC:
> >          return "HuC";
> > +    default:
> > +        return "uC";
> >      }
> > -    return "uC";
> > -}
> > -
> > -static inline
> > -void intel_uc_fw_init_early(struct intel_uc_fw *uc_fw,
> > -                enum intel_uc_fw_type type)
> > -{
> > -    /*
> > -     * we use FIRMWARE_UNINITIALIZED to detect checks against 
> > fetch_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->type = type;
> >  }
> > static inline bool intel_uc_fw_is_selected(struct intel_uc_fw *uc_fw)
> > @@ -164,7 +150,10 @@ static inline u32 
> > intel_uc_fw_get_upload_size(struct intel_uc_fw *uc_fw)
> >      return uc_fw->header_size + uc_fw->ucode_size;
> >  }
> > -void intel_uc_fw_fetch(struct drm_i915_private *dev_priv,
> > +void intel_uc_fw_init_early(struct drm_i915_private *i915,
> > +                struct intel_uc_fw *uc_fw,
> > +                enum intel_uc_fw_type type);
> > +void intel_uc_fw_fetch(struct drm_i915_private *i915,
> >                 struct intel_uc_fw *uc_fw);
> >  void intel_uc_fw_cleanup_fetch(struct intel_uc_fw *uc_fw);
> >  int intel_uc_fw_upload(struct intel_uc_fw *uc_fw,

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