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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Thu, Sep 14, 2023 at 10:53:40AM +0300, Jani Nikula wrote:
> 
> [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.)

Well, I believe this is my fault. I explicitly asked folks to avoid doing
what i915 was doing. Well, when I asked that I was more focused on avoiding
the macros and doing in the same way that other drivers were doing.

I really hated the explosion of parameters in our internal version of i915.
We need to do our best to avoid, or at least minimize the amount of params
and I believe the macros is like an incentive/invitation to bring more.

Then while checking other big drivers around we see that there are many
drivers doing the same extern exports as xe.

Would something like iwlwifi_mod_params from
drivers/net/wireless/intel/iwlwifi/iwl-modparams.h ease your concerns?

Thanks,
Rodrigo.

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