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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5780676f-7f6b-4f82-a729-b011a493a861@igalia.com>
Date:   Fri, 25 Aug 2023 09:24:33 -0300
From:   André Almeida <andrealmeid@...lia.com>
To:     Christian König <christian.koenig@....com>
Cc:     kernel-dev@...lia.com, alexander.deucher@....com,
        amd-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
        pierre-eric.pelloux-prayer@....com, linux-kernel@...r.kernel.org,
        'Marek Olšák' <maraeo@...il.com>
Subject: Re: [PATCH 1/2] drm/amdgpu: Merge debug module parameters

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@...lia.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;

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