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

[John Harrison]

On 9/14/2023 07:22, Jani Nikula wrote:
> On Thu, 14 Sep 2023, Rodrigo Vivi <rodrigo.vivi at 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 at 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.
Personally, I think 1) is the better technical approach - hidden static 
data with a publicly exported helper function if needed from more than 
one file. The 'data is not an interface' is a very valid point.

However, from a driver maintenance view, I think having them all 
collected in one file and living inside a parent structure is the safer 
option. It makes it much simpler to ensure consistency of 
implementation. Maybe make that parent structure static and wrap it with 
a helper function for retrieval to help reduce the global data issue.

Although neither of those options helps with the bigger problem that the 
module parameters themselves are fundamentally global no matter how you 
code them. As in, you can't have guc_firmware_path=igfx_guc_x_bin and 
guc_firmware_path=dgfx_guc_y.bin at the same time. So when you have the 
latest cutting edge discrete card plugged into a older (but still Xe 
supported) host system, you end up breaking the host in order to tweak 
the discrete.

John.

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