Re: [PATCH 1/2] drm/amdgpu: Merge debug module parameters
From: Christian König
Date: Fri Aug 25 2023 - 08:30:55 EST
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@xxxxxxxxxx>
>> ---
>> 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
>
>