Re: [PATCH 4/5] drm/i915/uc: Split firmware table validation to a separate function

[John Harrison <john.c.harrison@xxxxxxxxx>]

On 4/18/2023 16:14, Ceraolo Spurio, Daniele wrote:
> On 4/14/2023 5:57 PM, John.C.Harrison@xxxxxxxxx wrote:
> > From: John Harrison <John.C.Harrison@xxxxxxxxx>
> >
> > The validation of the firmware table was being done inside the code
> > for scanning the table for the next available firmware blob. Which is
> > unnecessary. Potentially, it should be a selftest. But either way, the
> > first step is pulling it out into a separate function that can be
> > called just once rather than once per blob attempt per blob type.
> >
> > Signed-off-by: John Harrison <John.C.Harrison@xxxxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c | 164 ++++++++++++++---------
> >   1 file changed, 99 insertions(+), 65 deletions(-)
> >
> > 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 6bb45d6b8da5f..c589782467265 100644
> > --- a/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> > +++ b/drivers/gpu/drm/i915/gt/uc/intel_uc_fw.c
> > @@ -233,20 +233,22 @@ struct fw_blobs_by_type {
> >       u32 count;
> >   };
> >   +static const struct uc_fw_platform_requirement blobs_guc[] = {
> > +    INTEL_GUC_FIRMWARE_DEFS(MAKE_FW_LIST, GUC_FW_BLOB, GUC_FW_BLOB_MMP)
> > +};
> > +
> > +static const struct uc_fw_platform_requirement blobs_huc[] = {
> > +    INTEL_HUC_FIRMWARE_DEFS(MAKE_FW_LIST, HUC_FW_BLOB, 
> > HUC_FW_BLOB_MMP, HUC_FW_BLOB_GSC)
> > +};
> > +
> > +static const struct fw_blobs_by_type 
> > blobs_all[INTEL_UC_FW_NUM_TYPES] = {
> > +    [INTEL_UC_FW_TYPE_GUC] = { blobs_guc, ARRAY_SIZE(blobs_guc) },
> > +    [INTEL_UC_FW_TYPE_HUC] = { blobs_huc, ARRAY_SIZE(blobs_huc) },
> > +};
> > +
> >   static void
> >   __uc_fw_auto_select(struct drm_i915_private *i915, struct 
> > intel_uc_fw *uc_fw)
> >   {
> > -    static const struct uc_fw_platform_requirement blobs_guc[] = {
> > -        INTEL_GUC_FIRMWARE_DEFS(MAKE_FW_LIST, GUC_FW_BLOB, 
> > GUC_FW_BLOB_MMP)
> > -    };
> > -    static const struct uc_fw_platform_requirement blobs_huc[] = {
> > -        INTEL_HUC_FIRMWARE_DEFS(MAKE_FW_LIST, HUC_FW_BLOB, 
> > HUC_FW_BLOB_MMP, HUC_FW_BLOB_GSC)
> > -    };
> > -    static const struct fw_blobs_by_type 
> > blobs_all[INTEL_UC_FW_NUM_TYPES] = {
> > -        [INTEL_UC_FW_TYPE_GUC] = { blobs_guc, ARRAY_SIZE(blobs_guc) },
> > -        [INTEL_UC_FW_TYPE_HUC] = { blobs_huc, ARRAY_SIZE(blobs_huc) },
> > -    };
> > -    static bool verified[INTEL_UC_FW_NUM_TYPES];
> >       const struct uc_fw_platform_requirement *fw_blobs;
> >       enum intel_platform p = INTEL_INFO(i915)->platform;
> >       u32 fw_count;
> > @@ -286,6 +288,11 @@ __uc_fw_auto_select(struct drm_i915_private 
> > *i915, struct intel_uc_fw *uc_fw)
> >               continue;
> >             if (uc_fw->file_selected.path) {
> > +            /*
> > +             * Continuing an earlier search after a found blob 
> > failed to load.
> > +             * Once the previously chosen path has been found, clear 
> > it out
> > +             * and let the search continue from there.
> > +             */
> >               if (uc_fw->file_selected.path == blob->path)
> >                   uc_fw->file_selected.path = NULL;
> >   @@ -306,78 +313,103 @@ __uc_fw_auto_select(struct drm_i915_private 
> > *i915, struct intel_uc_fw *uc_fw)
> >           /* Failed to find a match for the last attempt?! */
> >           uc_fw->file_selected.path = NULL;
> >       }
> > +}
> >   -    /* make sure the list is ordered as expected */
> > -    if (IS_ENABLED(CONFIG_DRM_I915_SELFTEST) && 
> > !verified[uc_fw->type]) {
> > -        verified[uc_fw->type] = true;
> > +static void validate_fw_table_type(struct drm_i915_private *i915, 
> > enum intel_uc_fw_type type)
> > +{
> > +    const struct uc_fw_platform_requirement *fw_blobs;
> > +    u32 fw_count;
> > +    int i;
> >   -        for (i = 1; i < fw_count; i++) {
> > -            /* Next platform is good: */
> > -            if (fw_blobs[i].p < fw_blobs[i - 1].p)
> > -                continue;
> > +    if (type >= ARRAY_SIZE(blobs_all)) {
> > +        drm_err(&i915->drm, "No blob array for %s\n", 
> > intel_uc_fw_type_repr(type));
> > +        return;
> > +    }
> >   -            /* Next platform revision is good: */
> > -            if (fw_blobs[i].p == fw_blobs[i - 1].p &&
> > -                fw_blobs[i].rev < fw_blobs[i - 1].rev)
> > -                continue;
> > +    fw_blobs = blobs_all[type].blobs;
> > +    fw_count = blobs_all[type].count;
> >   -            /* Platform/revision must be in order: */
> > -            if (fw_blobs[i].p != fw_blobs[i - 1].p ||
> > -                fw_blobs[i].rev != fw_blobs[i - 1].rev)
> > -                goto bad;
> > +    if (!fw_count)
> > +        return;
> >   -            /* Next major version is good: */
> > -            if (fw_blobs[i].blob.major < fw_blobs[i - 1].blob.major)
> > -                continue;
> > +    /* make sure the list is ordered as expected */
> > +    for (i = 1; i < fw_count; i++) {
> > +        /* Next platform is good: */
> > +        if (fw_blobs[i].p < fw_blobs[i - 1].p)
> > +            continue;
> >   -            /* New must be before legacy: */
> > -            if (!fw_blobs[i].blob.legacy && fw_blobs[i - 
> > 1].blob.legacy)
> > -                goto bad;
> > +        /* Next platform revision is good: */
> > +        if (fw_blobs[i].p == fw_blobs[i - 1].p &&
> > +            fw_blobs[i].rev < fw_blobs[i - 1].rev)
> > +            continue;
> >   -            /* New to legacy also means 0.0 to X.Y (HuC), or X.0 
> > to X.Y (GuC) */
> > -            if (fw_blobs[i].blob.legacy && !fw_blobs[i - 
> > 1].blob.legacy) {
> > -                if (!fw_blobs[i - 1].blob.major)
> > -                    continue;
> > +        /* Platform/revision must be in order: */
> > +        if (fw_blobs[i].p != fw_blobs[i - 1].p ||
> > +            fw_blobs[i].rev != fw_blobs[i - 1].rev)
> > +            goto bad;
> >   -                if (fw_blobs[i].blob.major == fw_blobs[i - 
> > 1].blob.major)
> > -                    continue;
> > -            }
> > +        /* Next major version is good: */
> > +        if (fw_blobs[i].blob.major < fw_blobs[i - 1].blob.major)
> > +            continue;
> >   -            /* Major versions must be in order: */
> > -            if (fw_blobs[i].blob.major != fw_blobs[i - 1].blob.major)
> > -                goto bad;
> > +        /* New must be before legacy: */
> > +        if (!fw_blobs[i].blob.legacy && fw_blobs[i - 1].blob.legacy)
> > +            goto bad;
> >   -            /* Next minor version is good: */
> > -            if (fw_blobs[i].blob.minor < fw_blobs[i - 1].blob.minor)
> > +        /* New to legacy also means 0.0 to X.Y (HuC), or X.0 to X.Y 
> > (GuC) */
> > +        if (fw_blobs[i].blob.legacy && !fw_blobs[i - 1].blob.legacy) {
> > +            if (!fw_blobs[i - 1].blob.major)
> >                   continue;
> >   -            /* Minor versions must be in order: */
> > -            if (fw_blobs[i].blob.minor != fw_blobs[i - 1].blob.minor)
> > -                goto bad;
> > -
> > -            /* Patch versions must be in order: */
> > -            if (fw_blobs[i].blob.patch <= fw_blobs[i - 1].blob.patch)
> > +            if (fw_blobs[i].blob.major == fw_blobs[i - 1].blob.major)
> >                   continue;
> > +        }
> > +
> > +        /* Major versions must be in order: */
> > +        if (fw_blobs[i].blob.major != fw_blobs[i - 1].blob.major)
> > +            goto bad;
> > +
> > +        /* Next minor version is good: */
> > +        if (fw_blobs[i].blob.minor < fw_blobs[i - 1].blob.minor)
> > +            continue;
> > +
> > +        /* Minor versions must be in order: */
> > +        if (fw_blobs[i].blob.minor != fw_blobs[i - 1].blob.minor)
> > +            goto bad;
> > +
> > +        /* Patch versions must be in order: */
> > +        if (fw_blobs[i].blob.patch <= fw_blobs[i - 1].blob.patch)
> > +            continue;
> >     bad:
> > -            drm_err(&i915->drm, "Invalid %s blob order: %s r%u 
> > %s%d.%d.%d comes before %s r%u %s%d.%d.%d\n",
> > -                intel_uc_fw_type_repr(uc_fw->type),
> > -                intel_platform_name(fw_blobs[i - 1].p), fw_blobs[i - 
> > 1].rev,
> > -                fw_blobs[i - 1].blob.legacy ? "L" : "v",
> > -                fw_blobs[i - 1].blob.major,
> > -                fw_blobs[i - 1].blob.minor,
> > -                fw_blobs[i - 1].blob.patch,
> > -                intel_platform_name(fw_blobs[i].p), fw_blobs[i].rev,
> > -                fw_blobs[i].blob.legacy ? "L" : "v",
> > -                fw_blobs[i].blob.major,
> > -                fw_blobs[i].blob.minor,
> > -                fw_blobs[i].blob.patch);
> > -
> > -            uc_fw->file_selected.path = NULL;
> > -        }
> > +        drm_err(&i915->drm, "Invalid %s blob order: %s r%u 
> > %s%d.%d.%d comes before %s r%u %s%d.%d.%d\n",
> > +            intel_uc_fw_type_repr(type),
> > +            intel_platform_name(fw_blobs[i - 1].p), fw_blobs[i - 
> > 1].rev,
> > +            fw_blobs[i - 1].blob.legacy ? "L" : "v",
> > +            fw_blobs[i - 1].blob.major,
> > +            fw_blobs[i - 1].blob.minor,
> > +            fw_blobs[i - 1].blob.patch,
> > +            intel_platform_name(fw_blobs[i].p), fw_blobs[i].rev,
> > +            fw_blobs[i].blob.legacy ? "L" : "v",
> > +            fw_blobs[i].blob.major,
> > +            fw_blobs[i].blob.minor,
> > +            fw_blobs[i].blob.patch);
>
> Confirming that this big diff was just an indent change was painful :/
Yeah, not a lot one can do about that. If you pull the patches to a 
local tree then you can run 'git show -b'. Can't really post that to 
patchwork though.

> >       }
> >   }
> >   +static void validate_fw_table(struct drm_i915_private *i915)
> > +{
> > +    enum intel_uc_fw_type type;
> > +    static bool done;
> > +
> > +    if (!IS_ENABLED(CONFIG_DRM_I915_SELFTEST) || done)
> > +        return;
> > +    done = true;
> > +
> > +    for (type = 0; type < INTEL_UC_FW_NUM_TYPES; type++)
> > +        validate_fw_table_type(i915, type);
> > +}
> > +
> >   static const char *__override_guc_firmware_path(struct 
> > drm_i915_private *i915)
> >   {
> >       if (i915->params.enable_guc & ENABLE_GUC_MASK)
> > @@ -432,6 +464,8 @@ void intel_uc_fw_init_early(struct intel_uc_fw 
> > *uc_fw,
> >   {
> >       struct drm_i915_private *i915 = ____uc_fw_to_gt(uc_fw, 
> > type)->i915;
> >   +    validate_fw_table(i915);
>
> Personal preference: IMO since we're calling intel_uc_fw_init_early 
> per FW type it would've been cleaner to restrict validate_fw_table() 
> to a single blob type. This would have the negative side effect of 
> having to track the "done" status per FW type, so I can see it's not a 
> clean improvement. Not a blocker.
That's step 2 - move the validate call out of the per blob type init 
sequence completely. Either just pushing it further up the call stack or 
moving it sideways to a selftest. First step was just to separate the 
code itself out as cleanly as possible.

John.

> Reviewed-by: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
>
> Daniele
>
> > +
> >       /*
> >        * 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