[Intel-xe] [PATCH 2/2] drm/xe/uc: Add GuC/HuC firmware path overrides

[Daniele Ceraolo Spurio]

On 9/11/2023 5:00 PM, John Harrison wrote:
> On 8/2/2023 14:26, Daniele Ceraolo Spurio wrote:
>> When testing a new binary and/or debugging binary-related issues, it is
>> useful to have the option to change which binary is loaded without
>> having to update and re-compile the kernel. To support this option, this
>> patch adds 2 new modparams to override the FW path for GuC and HuC. The
>> HuC modparam can also be set to an empty string to disable HuC loading.
> Why the difference between GuC & HuC? And is 'disabled' significantly 
> different to just setting it to a non-existent file?

There is the global switch (the one you commented on in the previous 
patch) that can be used to turn off all the FWs. I see no reason for 
wanting an additional switch to turn off just the GuC, as the other FWs 
won't work without it.

Also, Xe aborts the driver load if any FW file is not found, so yes 
disabled is different than not found.

>
>>
>> Note that those modparams only take effect on platforms where we already
>> have a default FW, so we're sure there is support for FW loading and the
>> kernel isn't going to explode in an undefined path.
>>
>> Signed-off-by: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>> Cc: John Harrison <John.C.Harrison at Intel.com>
>> Cc: Matthew Brost <matthew.brost at intel.com>
>> ---
>>   drivers/gpu/drm/xe/xe_module.c | 10 ++++++++++
>>   drivers/gpu/drm/xe/xe_module.h |  2 ++
>>   drivers/gpu/drm/xe/xe_uc_fw.c  | 30 +++++++++++++++++++++++++++++-
>>   3 files changed, 41 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_module.c 
>> b/drivers/gpu/drm/xe/xe_module.c
>> index de85494e2280..0660017c3e83 100644
>> --- a/drivers/gpu/drm/xe/xe_module.c
>> +++ b/drivers/gpu/drm/xe/xe_module.c
>> @@ -30,6 +30,16 @@ int xe_guc_log_level = 5;
>>   module_param_named(guc_log_level, xe_guc_log_level, int, 0600);
>>   MODULE_PARM_DESC(guc_log_level, "GuC firmware logging level 
>> (0=disable, 1..5=enable with verbosity min..max)");
>>   +char *xe_guc_firmware_path = NULL;
>> +module_param_named_unsafe(guc_firmware_path, xe_guc_firmware_path, 
>> charp, 0400);
>> +MODULE_PARM_DESC(guc_firmware_path,
>> +         "GuC firmware path to use instead of the default one");
>> +
>> +char *xe_huc_firmware_path = NULL;
>> +module_param_named_unsafe(huc_firmware_path, xe_huc_firmware_path, 
>> charp, 0400);
>> +MODULE_PARM_DESC(huc_firmware_path,
>> +         "HuC firmware path to use instead of the default one - 
>> empty string disables");
>> +
>>   char *xe_param_force_probe = CONFIG_DRM_XE_FORCE_PROBE;
>>   module_param_named_unsafe(force_probe, xe_param_force_probe, charp, 
>> 0400);
>>   MODULE_PARM_DESC(force_probe,
>> diff --git a/drivers/gpu/drm/xe/xe_module.h 
>> b/drivers/gpu/drm/xe/xe_module.h
>> index 2c1f9199f909..e1da1e9ca5cb 100644
>> --- a/drivers/gpu/drm/xe/xe_module.h
>> +++ b/drivers/gpu/drm/xe/xe_module.h
>> @@ -10,4 +10,6 @@ extern bool force_execlist;
>>   extern bool enable_display;
>>   extern u32 xe_force_vram_bar_size;
>>   extern int xe_guc_log_level;
>> +extern char *xe_guc_firmware_path;
>> +extern char *xe_huc_firmware_path;
>>   extern char *xe_param_force_probe;
>> diff --git a/drivers/gpu/drm/xe/xe_uc_fw.c 
>> b/drivers/gpu/drm/xe/xe_uc_fw.c
>> index fd53ef9e5c99..ef873e894714 100644
>> --- a/drivers/gpu/drm/xe/xe_uc_fw.c
>> +++ b/drivers/gpu/drm/xe/xe_uc_fw.c
>> @@ -15,6 +15,7 @@
>>   #include "xe_gt.h"
>>   #include "xe_map.h"
>>   #include "xe_mmio.h"
>> +#include "xe_module.h"
>>   #include "xe_uc_fw.h"
>>     /*
>> @@ -209,6 +210,30 @@ uc_fw_auto_select(struct xe_device *xe, struct 
>> xe_uc_fw *uc_fw)
>>       }
>>   }
>>   +static void
>> +uc_fw_override(struct xe_uc_fw *uc_fw)
>> +{
>> +    char *path_override = NULL;
>> +
>> +    /* empty string disables, but it's not allowed for GuC */
>> +    switch (uc_fw->type) {
>> +    case XE_UC_FW_TYPE_GUC:
>> +        if (xe_guc_firmware_path && *xe_guc_firmware_path)
>> +            path_override = xe_guc_firmware_path;
>> +        break;
>> +    case XE_UC_FW_TYPE_HUC:
>> +        path_override = xe_huc_firmware_path;
>> +        break;
>> +    default:
>> +        break;
>> +    }
>> +
>> +    if (path_override) {
>> +        uc_fw->path = path_override;
>> +        uc_fw->user_overridden = true;
>> +    }
>> +}
>> +
>>   /**
>>    * xe_uc_fw_copy_rsa - copy fw RSA to buffer
>>    *
>> @@ -346,7 +371,10 @@ int xe_uc_fw_init(struct xe_uc_fw *uc_fw)
>>       if (!xe_uc_fw_is_supported(uc_fw))
>>           return 0;
>>   -    if (!xe_device_guc_submission_enabled(xe)) {
>> +    uc_fw_override(uc_fw);
>> +
>> +    /* user can provide an empty string via the override to disable */
> But only for HuC?

And for GSC once I get around to add support for that. I wanted to keep 
the sentence general here, since there is no visibility of which FW it 
is and there is already a proper description of the override in the 
modparam definition, but I can add a "non-GuC FWs" at the end if you 
think it makes things clearer.

Daniele

>
> John.
>
>> +    if (!xe_device_guc_submission_enabled(xe) || !(*uc_fw->path)) {
>>           xe_uc_fw_change_status(uc_fw, XE_UC_FIRMWARE_DISABLED);
>>           drm_dbg(&xe->drm, "%s disabled", 
>> xe_uc_fw_type_repr(uc_fw->type));
>>           return 0;
>