Re: [PATCH 1/2] drm/amdgpu: Merge debug module parameters

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

Am 25.08.23 um 14:34 schrieb André Almeida:
> Em 25/08/2023 09:29, Christian König escreveu:
>> Am 25.08.23 um 14:24 schrieb André Almeida:
>>> Em 25/08/2023 03:56, Christian König escreveu:
>>> > Am 24.08.23 um 18:25 schrieb André Almeida:
>>> >> Merge all developer debug options available as separated module
>>> >> parameters in one, making it obvious that are for developers.
>>> >>
>>> >> Signed-off-by: André Almeida <andrealmeid@igalia.com>
>>> >> ---
>>> >>   drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c  | 24 
>>> ++++++++++++++++++++++++
>>> >>   drivers/gpu/drm/amd/include/amd_shared.h |  9 +++++++++
>>> >>   2 files changed, 33 insertions(+)
>>> >>
>>> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> >> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> >> index f5856b82605e..d53e4097acc0 100644
>>> >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
>>> >> @@ -194,6 +194,7 @@ int amdgpu_use_xgmi_p2p = 1;
>>> >>   int amdgpu_vcnfw_log;
>>> >>   int amdgpu_sg_display = -1; /* auto */
>>> >>   int amdgpu_user_partt_mode = AMDGPU_AUTO_COMPUTE_PARTITION_MODE;
>>> >> +uint amdgpu_debug_mask;
>>> >>   static void amdgpu_drv_delayed_reset_work_handler(struct 
>>> work_struct
>>> >> *work);
>>> >> @@ -938,6 +939,9 @@ module_param_named(user_partt_mode,
>>> >> amdgpu_user_partt_mode, uint, 0444);
>>> >>   module_param(enforce_isolation, bool, 0444);
>>> >>   MODULE_PARM_DESC(enforce_isolation, "enforce process isolation
>>> >> between graphics and compute . enforce_isolation = on");
>>> >> +MODULE_PARM_DESC(debug_mask, "debug options for amdgpu, disabled by
>>> >> default");
>>> >> +module_param_named(debug_mask, amdgpu_debug_mask, uint, 0444);
>>> >> +
>>> >>   /* These devices are not supported by amdgpu.
>>> >>    * They are supported by the mach64, r128, radeon drivers
>>> >>    */
>>> >> @@ -2871,6 +2875,24 @@ static struct pci_driver 
>>> amdgpu_kms_pci_driver = {
>>> >>       .dev_groups = amdgpu_sysfs_groups,
>>> >>   };
>>> >> +static void amdgpu_init_debug_options(void)
>>> >> +{
>>> >> +    if (amdgpu_debug_mask & DEBUG_VERBOSE_EVICTIONS) {
>>> >> +        pr_info("debug: eviction debug messages enabled\n");
>>> >> +        debug_evictions = true;
>>> >> +    }
>>> >> +
>>> >> +    if (amdgpu_debug_mask & DEBUG_VM) {
>>> >> +        pr_info("debug: VM handling debug enabled\n");
>>> >> +        amdgpu_vm_debug = true;
>>> >> +    }
>>> >> +
>>> >> +    if (amdgpu_debug_mask & DEBUG_LARGEBAR) {
>>> >> +        pr_info("debug: enabled simulating large-bar capability on
>>> >> non-large bar system\n");
>>> >> +        debug_largebar = true;
>>> >
>>> > How should that work???
>>>
>>> Ops, I thought it was a boolean. It should be
>>>
>>> +        debug_largebar = 1;
>>
>>
>> That's not the problem, the question is since when do we have a 
>> debug_largebar option and what should that one do?
>>
>
> It should work exactly like the other one, but instead of using 
> amdgpu.large_bar=1, one would use amdgpu.debug_mask=0x4 to activate 
> it, as the plan is to merge all current debug options in a single one 
> right?

Ah! That's the KFD debug_largebar option! Now I got what this is.

Probably best to move the booleans into amdgpu_device and deprecate the 
old options.

Not really a good approach to overwrite the globals here.

Regards,
Christian.

>
>> Regards,
>> Christian.
>