[PATCH 4/5] drm/i915/uc: Split firmware table validation to a separate function
John Harrison
john.c.harrison at intel.com
Wed Apr 19 15:45:00 UTC 2023
On 4/18/2023 16:14, Ceraolo Spurio, Daniele wrote:
> On 4/14/2023 5:57 PM, John.C.Harrison at Intel.com wrote:
>> From: John Harrison <John.C.Harrison at Intel.com>
>>
>> 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 at Intel.com>
>> ---
>> 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 at intel.com>
>
> 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
>
More information about the dri-devel
mailing list