Re: [Intel-gfx] [CI 1/2] drm/i915/uc: Support for version reduced and multiple firmware files

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

On 9/12/2022 5:23 PM, Lucas De Marchi wrote:
> On Tue, Sep 06, 2022 at 04:01:46PM -0700, Daniele Ceraolo Spurio wrote:
>> @@ -184,49 +247,94 @@ __uc_fw_auto_select(struct drm_i915_private 
>> *i915, struct intel_uc_fw *uc_fw)
>>     fw_count = blobs_all[uc_fw->type].count;
>>
>>     for (i = 0; i < fw_count && p <= fw_blobs[i].p; i++) {
>> -        if (p == fw_blobs[i].p && rev >= fw_blobs[i].rev) {
>> -            const struct uc_fw_blob *blob = &fw_blobs[i].blob;
>> -            uc_fw->path = blob->path;
>> -            uc_fw->wanted_path = blob->path;
>> -            uc_fw->major_ver_wanted = blob->major;
>> -            uc_fw->minor_ver_wanted = blob->minor;
>> -            break;
>> -        }
>> -    }
>> +        const struct uc_fw_blob *blob = &fw_blobs[i].blob;
>>
>> -    if (uc_fw->type == INTEL_UC_FW_TYPE_GUC) {
>> -        const struct uc_fw_platform_requirement *blobs = 
>> blobs_guc_fallback;
>> -        u32 count = ARRAY_SIZE(blobs_guc_fallback);
>> +        if (p != fw_blobs[i].p)
>> +            continue;
>>
>> -        for (i = 0; i < count && p <= blobs[i].p; i++) {
>> -            if (p == blobs[i].p && rev >= blobs[i].rev) {
>> -                const struct uc_fw_blob *blob = &blobs[i].blob;
>> +        if (rev < fw_blobs[i].rev)
>> +            continue;
>>
>> -                uc_fw->fallback.path = blob->path;
>> -                uc_fw->fallback.major_ver = blob->major;
>> -                uc_fw->fallback.minor_ver = blob->minor;
>> -                break;
>> -            }
>> +        if (uc_fw->file_selected.path) {
>> +            if (uc_fw->file_selected.path == blob->path)
>> +                uc_fw->file_selected.path = NULL;
>> +
>> +            continue;
>>         }
>> +
>> +        uc_fw->file_selected.path = blob->path;
>> +        uc_fw->file_wanted.path = blob->path;
>> +        uc_fw->file_wanted.major_ver = blob->major;
>> +        uc_fw->file_wanted.minor_ver = blob->minor;
>> +        break;
>>     }
>>
>>     /* make sure the list is ordered as expected */
>> -    if (IS_ENABLED(CONFIG_DRM_I915_SELFTEST)) {
>> +    if (IS_ENABLED(CONFIG_DRM_I915_SELFTEST) && !verified) {
>> +        verified = true;
>> +
>>         for (i = 1; i < fw_count; i++) {
>> +            /* Next platform is good: */
>>             if (fw_blobs[i].p < fw_blobs[i - 1].p)
>>                 continue;
>>
>> +            /* 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;
>>
>> -            drm_err(&i915->drm, "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].rev,
>> -                intel_platform_name(fw_blobs[i].p),
>> -                fw_blobs[i].rev);
>> +            /* 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;
>> +
>> +            /* Next major version is good: */
>> +            if (fw_blobs[i].blob.major < fw_blobs[i - 1].blob.major)
>> +                continue;
>> +
>> +            /* New must be before legacy: */
>> +            if (!fw_blobs[i].blob.legacy && fw_blobs[i - 
>> 1].blob.legacy)
>> +                goto bad;
>> +
>> +            /* 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;
>> +
>> +                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;
>>
>> -            uc_fw->path = NULL;
>> +            /* 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, "\x1B[35;1mInvalid FW blob order: %s 
>> r%u %s%d.%d.%d comes before %s r%u %s%d.%d.%d\n",
>
> what is this \x1B[35;1m? Probably something that went bad while
> writing/pasting this?

Looks like it slipped in. Fix coming soon (John will send a v2 of " 
drm/i915/uc: Fix issues with overriding firmware files" and include it 
there).

Daniele

>
> Lucas De Marchi
>
>> + 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;
>>         }
>>     }
>> }
>> @@ -259,7 +367,7 @@ static void __uc_fw_user_override(struct 
>> drm_i915_private *i915, struct intel_uc