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

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

On 9/11/2023 5:16 PM, John Harrison wrote:
> On 9/11/2023 17:10, Daniele Ceraolo Spurio wrote:
>> 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.
> Hmm. Given the firmware paths are 'unsafe' module parameters, I would 
> argue that it is better to keep the code simple and if the user wants 
> to shoot themselves in the foot then that is their problem. That is, 
> just treat all firmware file overrides exactly the same. And if the 
> driver fails to load because the user has requested an unsupported 
> configuration then who cares.

I think allowing the override for GuC would actually make things more 
complicated. The code assumes that the GuC is enabled if the flag is set 
and that all the GuC features can be used, so having the GuC being 
disabled under the hood via this override might actually make the driver 
panic, which would be bad even if the parameter is unsafe. Not allowing 
disabling the GuC is an extra half-line of code, which IMO is overall 
simpler and cleaner compared to either risking panics or having to 
secure all the paths against a missing GuC.

Daniele

>
> John.
>
>>
>>>
>>>>
>>>> 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@intel.com>
>>>> Cc: John Harrison <John.C.Harrison@Intel.com>
>>>> Cc: Matthew Brost <matthew.brost@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;
>>>
>>
>