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

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

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?

Regards,
Christian.

>
> >
> >> +    }
> >> +}
> >> +
> >>   static int __init amdgpu_init(void)
> >>   {
> >>       int r;
> >> @@ -2893,6 +2915,8 @@ static int __init amdgpu_init(void)
> >>       /* Ignore KFD init failures. Normal when CONFIG_HSA_AMD is not
> >> set. */
> >>       amdgpu_amdkfd_init();
> >> +    amdgpu_init_debug_options();
> >> +
> >>       /* let modprobe override vga console setting */
> >>       return pci_register_driver(&amdgpu_kms_pci_driver);
> >> diff --git a/drivers/gpu/drm/amd/include/amd_shared.h
> >> b/drivers/gpu/drm/amd/include/amd_shared.h
> >> index 67d7b7ee8a2a..6fa644c249a5 100644
> >> --- a/drivers/gpu/drm/amd/include/amd_shared.h
> >> +++ b/drivers/gpu/drm/amd/include/amd_shared.h
> >> @@ -257,6 +257,15 @@ enum DC_DEBUG_MASK {
> >>   enum amd_dpm_forced_level;
> >> +/*
> >> + * amdgpu.debug module options. Are all disabled by default
> >> + */
> >> +enum AMDGPU_DEBUG_MASK {
> >> +    DEBUG_VERBOSE_EVICTIONS = (1 << 0),        // 0x1
> >> +    DEBUG_VM = (1 << 1),                // 0x2
> >> +    DEBUG_LARGEBAR = (1 << 2),            // 0x4
> >
> > Good start, but please give the symbol names an AMDGPU_ prefix. Stuff
> > like DEBUG_VM is just way to general and could clash.
> >
> > Apart from that comments on the same line and using // style comments
> > are frowned upon. You should probably rather use the BIT() macro here.
> >
>
> Ack, I'll change that for next version
>
> > Regards,
> > Christian.
> >
> >> +
> >>   /**
> >>    * struct amd_ip_funcs - general hooks for managing amdgpu IP Blocks
> >>    * @name: Name of IP block
> >
> >