Re: [PATCH 2/3] drm/amdgpu: add debug fs for AMD GPU reset register dumps

[Christian König <christian.koenig@xxxxxxx>]

Am 11.01.22 um 08:11 schrieb Somalapuram Amaranath:
> Implementation of debug fs “amdgpu_reset_reg_dumps”
> to capture reset register status
>
> Signed-off-by: Somalapuram Amaranath <Amaranath.Somalapuram@xxxxxxx>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 64 +++++++++++++++++++++
>   1 file changed, 64 insertions(+)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> index 164d6a9e9fbb..c56d7b451aea 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> @@ -1609,6 +1609,68 @@ DEFINE_DEBUGFS_ATTRIBUTE(fops_ib_preempt, NULL,
>   DEFINE_DEBUGFS_ATTRIBUTE(fops_sclk_set, NULL,
>   			amdgpu_debugfs_sclk_set, "%llu\n");
>   
> +static DECLARE_RWSEM(reset_reg_dumps);

Please never use a static lock like this. That would belong into the 
adev structure, but on the other hand I don't see any need for it.

Regards,
Christian.

> +
> +static void seq_reg_dump(struct seq_file *m,
> +			 uint32_t (*dump)[2], uint32_t n_regs, char **names)
> +{
> +	uint32_t i, count;
> +
> +	for (i = 0, count = 0; i < n_regs; i++) {
> +		if (count == 0 ||
> +		    dump[i-1][0] + sizeof(uint32_t) != dump[i][0]) {
> +			seq_printf(m, "%s%s:	%08x: %08x",
> +				   i ? "\n" : "", names[i],
> +				   dump[i][0], dump[i][1]);
> +			count = 7;
> +		} else {
> +			seq_printf(m, " %08x", dump[i][1]);
> +			count--;
> +		}
> +	}
> +
> +	seq_puts(m, "\n");
> +}
> +
> +int amdgpu_debugfs_reset_reg_dumps_show(struct seq_file *m, void *unused)
> +{
> +	struct amdgpu_device *adev = (struct amdgpu_device *)m->private;
> +	uint32_t (*dump)[2], n_regs = 0;
> +	int r = 0, i;
> +	char **reg_names;
> +
> +	down_read(&reset_reg_dumps);
> +
> +	for (i = 0; i < adev->num_ip_blocks; i++) {
> +		if (!adev->ip_blocks[i].status.valid)
> +			continue;
> +		if (!adev->ip_blocks[i].version->funcs->reset_reg_dumps)
> +			continue;
> +		r = adev->ip_blocks[i].version->funcs->reset_reg_dumps(
> +				adev, &dump, &n_regs, &reg_names);
> +
> +		if (r)
> +			DRM_ERROR("reset_reg_dumps of IP block <%s> failed %d\n",
> +				  adev->ip_blocks[i].version->funcs->name, r);
> +
> +		if (!r) {
> +			seq_reg_dump(m, dump, n_regs, reg_names);
> +			kfree(dump);
> +		}
> +	}
> +
> +	up_read(&reset_reg_dumps);
> +
> +	for (i = 0; i < n_regs; i++)
> +		kfree(reg_names[i]);
> +
> +	kfree(reg_names);
> +
> +	return 0;
> +}
> +
> +DEFINE_SHOW_ATTRIBUTE(amdgpu_debugfs_reset_reg_dumps);
> +
>   int amdgpu_debugfs_init(struct amdgpu_device *adev)
>   {
>   	struct dentry *root = adev_to_drm(adev)->primary->debugfs_root;
> @@ -1672,6 +1734,8 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev)
>   			    &amdgpu_debugfs_test_ib_fops);
>   	debugfs_create_file("amdgpu_vm_info", 0444, root, adev,
>   			    &amdgpu_debugfs_vm_info_fops);
> +	debugfs_create_file("amdgpu_reset_reg_dumps", S_IFREG | 0444, root,
> +			    adev, &amdgpu_debugfs_reset_reg_dumps_fops);
>   
>   	adev->debugfs_vbios_blob.data = adev->bios;
>   	adev->debugfs_vbios_blob.size = adev->bios_size;