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

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

On Thu, 14 Sep 2023, Rodrigo Vivi <rodrigo.vivi@intel.com> wrote:
> 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?

I think that's exactly 2) above. i915 having macro wrappers for this is
an implementation detail that's neither here nor there.

BR,
Jani.

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

-- 
Jani Nikula, Intel