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

[Jani Nikula <jani.nikula@linux.intel.com>]

[hijacking the thread a bit, sorry]

On Wed, 13 Sep 2023, Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com> wrote:
> 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,

Not related to this patch specifically, but, uh, why does xe collect all
module parameters in one file like this?

IMO there's two reasonable ways of defining module paramers:

1) Module parameters defined for static variables in each .c file where
needed, and avoid the need to access them in multiple .c files.

2) Module parameters defined in a single file, wrapped in a global
struct. (This is what i915 does.)

Putting all of the variables in a single file, and exposing them
globally is not cool. You want to limit the number of module global
variables in big drivers like this.

(Of course, primarily module parameters should be avoided altogether.)

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

Basically *every* extern in a big driver is a sign of a problem. Data is
not an interface.

Yes, also i915_modparams violates this, but it groups the stuff together
under *one* extern struct with a "namespace", if you will.


BR,
Jani.


-- 
Jani Nikula, Intel