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

[Lucas De Marchi <lucas.demarchi@xxxxxxxxx>]

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?

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