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

["Christian König" <christian.koenig@amd.com>]

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

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

Regards,
Christian.

> +
>   /**
>    * struct amd_ip_funcs - general hooks for managing amdgpu IP Blocks
>    * @name: Name of IP block