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

[John Harrison]

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?

>
> 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?

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;