[PATCH v1 0/2] Clean the debugfs register_dump_register_list

[PATCH v1 0/2] Clean the debugfs register_dump_register_list

[Sunil Khatri]

Debugfs and devcoredump cleanup with removal of register list for dump

Sunil Khatri (2):
  drm/amdgpu: Remove debugfs amdgpu_reset_dump_register_list
  drm/amdgpu: Clean up the register dump via debugfs list

 drivers/gpu/drm/amd/amdgpu/amdgpu.h           | 13 ---
 drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c   | 96 -------------------
 .../gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c  | 10 +-
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c    | 19 ----
 4 files changed, 1 insertion(+), 137 deletions(-)

-- 
2.34.1

[PATCH v2 1/2] drm/amdgpu: Remove debugfs amdgpu_reset_dump_register_list

[Sunil Khatri]

There are some problem with existing amdgpu_reset_dump_register_list
debugfs node. It is supposed to read a list of registers but there
could be cases when the IP is not in correct power state. Register
read in such cases could lead to more problems.

We are taking care of all such power states in devcoredump and
dumping the registers of need for debugging. So cleaning this code
and we dont need this functionality via debugfs anymore.

Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 96 ---------------------
 1 file changed, 96 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
index 0e1a11b6b989..cbef720de779 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
@@ -2026,100 +2026,6 @@ DEFINE_DEBUGFS_ATTRIBUTE(fops_ib_preempt, NULL,
 DEFINE_DEBUGFS_ATTRIBUTE(fops_sclk_set, NULL,
 			amdgpu_debugfs_sclk_set, "%llu\n");
 
-static ssize_t amdgpu_reset_dump_register_list_read(struct file *f,
-				char __user *buf, size_t size, loff_t *pos)
-{
-	struct amdgpu_device *adev = (struct amdgpu_device *)file_inode(f)->i_private;
-	char reg_offset[12];
-	int i, ret, len = 0;
-
-	if (*pos)
-		return 0;
-
-	memset(reg_offset, 0, 12);
-	ret = down_read_killable(&adev->reset_domain->sem);
-	if (ret)
-		return ret;
-
-	for (i = 0; i < adev->reset_info.num_regs; i++) {
-		sprintf(reg_offset, "0x%x\n", adev->reset_info.reset_dump_reg_list[i]);
-		up_read(&adev->reset_domain->sem);
-		if (copy_to_user(buf + len, reg_offset, strlen(reg_offset)))
-			return -EFAULT;
-
-		len += strlen(reg_offset);
-		ret = down_read_killable(&adev->reset_domain->sem);
-		if (ret)
-			return ret;
-	}
-
-	up_read(&adev->reset_domain->sem);
-	*pos += len;
-
-	return len;
-}
-
-static ssize_t amdgpu_reset_dump_register_list_write(struct file *f,
-			const char __user *buf, size_t size, loff_t *pos)
-{
-	struct amdgpu_device *adev = (struct amdgpu_device *)file_inode(f)->i_private;
-	char reg_offset[11];
-	uint32_t *new = NULL, *tmp = NULL;
-	unsigned int len = 0;
-	int ret, i = 0;
-
-	do {
-		memset(reg_offset, 0, 11);
-		if (copy_from_user(reg_offset, buf + len,
-					min(10, (size-len)))) {
-			ret = -EFAULT;
-			goto error_free;
-		}
-
-		new = krealloc_array(tmp, i + 1, sizeof(uint32_t), GFP_KERNEL);
-		if (!new) {
-			ret = -ENOMEM;
-			goto error_free;
-		}
-		tmp = new;
-		if (sscanf(reg_offset, "%X %n", &tmp[i], &ret) != 1) {
-			ret = -EINVAL;
-			goto error_free;
-		}
-
-		len += ret;
-		i++;
-	} while (len < size);
-
-	new = kmalloc_array(i, sizeof(uint32_t), GFP_KERNEL);
-	if (!new) {
-		ret = -ENOMEM;
-		goto error_free;
-	}
-	ret = down_write_killable(&adev->reset_domain->sem);
-	if (ret)
-		goto error_free;
-
-	swap(adev->reset_info.reset_dump_reg_list, tmp);
-	swap(adev->reset_info.reset_dump_reg_value, new);
-	adev->reset_info.num_regs = i;
-	up_write(&adev->reset_domain->sem);
-	ret = size;
-
-error_free:
-	if (tmp != new)
-		kfree(tmp);
-	kfree(new);
-	return ret;
-}
-
-static const struct file_operations amdgpu_reset_dump_register_list = {
-	.owner = THIS_MODULE,
-	.read = amdgpu_reset_dump_register_list_read,
-	.write = amdgpu_reset_dump_register_list_write,
-	.llseek = default_llseek
-};
-
 int amdgpu_debugfs_init(struct amdgpu_device *adev)
 {
 	struct dentry *root = adev_to_drm(adev)->primary->debugfs_root;
@@ -2204,8 +2110,6 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev)
 			    &amdgpu_debugfs_vm_info_fops);
 	debugfs_create_file("amdgpu_benchmark", 0200, root, adev,
 			    &amdgpu_benchmark_fops);
-	debugfs_create_file("amdgpu_reset_dump_register_list", 0644, root, adev,
-			    &amdgpu_reset_dump_register_list);
 
 	adev->debugfs_vbios_blob.data = adev->bios;
 	adev->debugfs_vbios_blob.size = adev->bios_size;
-- 
2.34.1

Re: [PATCH v2 1/2] drm/amdgpu: Remove debugfs amdgpu_reset_dump_register_list

[Christian König]

Am 30.07.24 um 08:44 schrieb Sunil Khatri:
> There are some problem with existing amdgpu_reset_dump_register_list
> debugfs node. It is supposed to read a list of registers but there
> could be cases when the IP is not in correct power state. Register
> read in such cases could lead to more problems.

You could be a bit more specific here, e.g. "could lead to ASIC hangs.".

>
> We are taking care of all such power states in devcoredump and
> dumping the registers of need for debugging. So cleaning this code
> and we dont need this functionality via debugfs anymore.
>
> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>

This patch and #2 in the series could potentially be squashed together, 
but either way is fine with me.

Reviewed-by: Christian König <christian.koenig@amd.com> for both patches.

Regards,
Christian.

> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 96 ---------------------
>   1 file changed, 96 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> index 0e1a11b6b989..cbef720de779 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> @@ -2026,100 +2026,6 @@ DEFINE_DEBUGFS_ATTRIBUTE(fops_ib_preempt, NULL,
>   DEFINE_DEBUGFS_ATTRIBUTE(fops_sclk_set, NULL,
>   			amdgpu_debugfs_sclk_set, "%llu\n");
>   
> -static ssize_t amdgpu_reset_dump_register_list_read(struct file *f,
> -				char __user *buf, size_t size, loff_t *pos)
> -{
> -	struct amdgpu_device *adev = (struct amdgpu_device *)file_inode(f)->i_private;
> -	char reg_offset[12];
> -	int i, ret, len = 0;
> -
> -	if (*pos)
> -		return 0;
> -
> -	memset(reg_offset, 0, 12);
> -	ret = down_read_killable(&adev->reset_domain->sem);
> -	if (ret)
> -		return ret;
> -
> -	for (i = 0; i < adev->reset_info.num_regs; i++) {
> -		sprintf(reg_offset, "0x%x\n", adev->reset_info.reset_dump_reg_list[i]);
> -		up_read(&adev->reset_domain->sem);
> -		if (copy_to_user(buf + len, reg_offset, strlen(reg_offset)))
> -			return -EFAULT;
> -
> -		len += strlen(reg_offset);
> -		ret = down_read_killable(&adev->reset_domain->sem);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	up_read(&adev->reset_domain->sem);
> -	*pos += len;
> -
> -	return len;
> -}
> -
> -static ssize_t amdgpu_reset_dump_register_list_write(struct file *f,
> -			const char __user *buf, size_t size, loff_t *pos)
> -{
> -	struct amdgpu_device *adev = (struct amdgpu_device *)file_inode(f)->i_private;
> -	char reg_offset[11];
> -	uint32_t *new = NULL, *tmp = NULL;
> -	unsigned int len = 0;
> -	int ret, i = 0;
> -
> -	do {
> -		memset(reg_offset, 0, 11);
> -		if (copy_from_user(reg_offset, buf + len,
> -					min(10, (size-len)))) {
> -			ret = -EFAULT;
> -			goto error_free;
> -		}
> -
> -		new = krealloc_array(tmp, i + 1, sizeof(uint32_t), GFP_KERNEL);
> -		if (!new) {
> -			ret = -ENOMEM;
> -			goto error_free;
> -		}
> -		tmp = new;
> -		if (sscanf(reg_offset, "%X %n", &tmp[i], &ret) != 1) {
> -			ret = -EINVAL;
> -			goto error_free;
> -		}
> -
> -		len += ret;
> -		i++;
> -	} while (len < size);
> -
> -	new = kmalloc_array(i, sizeof(uint32_t), GFP_KERNEL);
> -	if (!new) {
> -		ret = -ENOMEM;
> -		goto error_free;
> -	}
> -	ret = down_write_killable(&adev->reset_domain->sem);
> -	if (ret)
> -		goto error_free;
> -
> -	swap(adev->reset_info.reset_dump_reg_list, tmp);
> -	swap(adev->reset_info.reset_dump_reg_value, new);
> -	adev->reset_info.num_regs = i;
> -	up_write(&adev->reset_domain->sem);
> -	ret = size;
> -
> -error_free:
> -	if (tmp != new)
> -		kfree(tmp);
> -	kfree(new);
> -	return ret;
> -}
> -
> -static const struct file_operations amdgpu_reset_dump_register_list = {
> -	.owner = THIS_MODULE,
> -	.read = amdgpu_reset_dump_register_list_read,
> -	.write = amdgpu_reset_dump_register_list_write,
> -	.llseek = default_llseek
> -};
> -
>   int amdgpu_debugfs_init(struct amdgpu_device *adev)
>   {
>   	struct dentry *root = adev_to_drm(adev)->primary->debugfs_root;
> @@ -2204,8 +2110,6 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev)
>   			    &amdgpu_debugfs_vm_info_fops);
>   	debugfs_create_file("amdgpu_benchmark", 0200, root, adev,
>   			    &amdgpu_benchmark_fops);
> -	debugfs_create_file("amdgpu_reset_dump_register_list", 0644, root, adev,
> -			    &amdgpu_reset_dump_register_list);
>   
>   	adev->debugfs_vbios_blob.data = adev->bios;
>   	adev->debugfs_vbios_blob.size = adev->bios_size;

Re: [PATCH v2 1/2] drm/amdgpu: Remove debugfs amdgpu_reset_dump_register_list

[Lazar, Lijo]

On 7/30/2024 12:14 PM, Sunil Khatri wrote:
> There are some problem with existing amdgpu_reset_dump_register_list
> debugfs node. It is supposed to read a list of registers but there
> could be cases when the IP is not in correct power state. Register
> read in such cases could lead to more problems.
> 
> We are taking care of all such power states in devcoredump and
> dumping the registers of need for debugging. So cleaning this code
> and we dont need this functionality via debugfs anymore.
> 
> Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>

Series is -

	Reviewed-by: Lijo Lazar <lijo.lazar@amd.com>

Thanks,
Lijo
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 96 ---------------------
>  1 file changed, 96 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> index 0e1a11b6b989..cbef720de779 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
> @@ -2026,100 +2026,6 @@ DEFINE_DEBUGFS_ATTRIBUTE(fops_ib_preempt, NULL,
>  DEFINE_DEBUGFS_ATTRIBUTE(fops_sclk_set, NULL,
>  			amdgpu_debugfs_sclk_set, "%llu\n");
>  
> -static ssize_t amdgpu_reset_dump_register_list_read(struct file *f,
> -				char __user *buf, size_t size, loff_t *pos)
> -{
> -	struct amdgpu_device *adev = (struct amdgpu_device *)file_inode(f)->i_private;
> -	char reg_offset[12];
> -	int i, ret, len = 0;
> -
> -	if (*pos)
> -		return 0;
> -
> -	memset(reg_offset, 0, 12);
> -	ret = down_read_killable(&adev->reset_domain->sem);
> -	if (ret)
> -		return ret;
> -
> -	for (i = 0; i < adev->reset_info.num_regs; i++) {
> -		sprintf(reg_offset, "0x%x\n", adev->reset_info.reset_dump_reg_list[i]);
> -		up_read(&adev->reset_domain->sem);
> -		if (copy_to_user(buf + len, reg_offset, strlen(reg_offset)))
> -			return -EFAULT;
> -
> -		len += strlen(reg_offset);
> -		ret = down_read_killable(&adev->reset_domain->sem);
> -		if (ret)
> -			return ret;
> -	}
> -
> -	up_read(&adev->reset_domain->sem);
> -	*pos += len;
> -
> -	return len;
> -}
> -
> -static ssize_t amdgpu_reset_dump_register_list_write(struct file *f,
> -			const char __user *buf, size_t size, loff_t *pos)
> -{
> -	struct amdgpu_device *adev = (struct amdgpu_device *)file_inode(f)->i_private;
> -	char reg_offset[11];
> -	uint32_t *new = NULL, *tmp = NULL;
> -	unsigned int len = 0;
> -	int ret, i = 0;
> -
> -	do {
> -		memset(reg_offset, 0, 11);
> -		if (copy_from_user(reg_offset, buf + len,
> -					min(10, (size-len)))) {
> -			ret = -EFAULT;
> -			goto error_free;
> -		}
> -
> -		new = krealloc_array(tmp, i + 1, sizeof(uint32_t), GFP_KERNEL);
> -		if (!new) {
> -			ret = -ENOMEM;
> -			goto error_free;
> -		}
> -		tmp = new;
> -		if (sscanf(reg_offset, "%X %n", &tmp[i], &ret) != 1) {
> -			ret = -EINVAL;
> -			goto error_free;
> -		}
> -
> -		len += ret;
> -		i++;
> -	} while (len < size);
> -
> -	new = kmalloc_array(i, sizeof(uint32_t), GFP_KERNEL);
> -	if (!new) {
> -		ret = -ENOMEM;
> -		goto error_free;
> -	}
> -	ret = down_write_killable(&adev->reset_domain->sem);
> -	if (ret)
> -		goto error_free;
> -
> -	swap(adev->reset_info.reset_dump_reg_list, tmp);
> -	swap(adev->reset_info.reset_dump_reg_value, new);
> -	adev->reset_info.num_regs = i;
> -	up_write(&adev->reset_domain->sem);
> -	ret = size;
> -
> -error_free:
> -	if (tmp != new)
> -		kfree(tmp);
> -	kfree(new);
> -	return ret;
> -}
> -
> -static const struct file_operations amdgpu_reset_dump_register_list = {
> -	.owner = THIS_MODULE,
> -	.read = amdgpu_reset_dump_register_list_read,
> -	.write = amdgpu_reset_dump_register_list_write,
> -	.llseek = default_llseek
> -};
> -
>  int amdgpu_debugfs_init(struct amdgpu_device *adev)
>  {
>  	struct dentry *root = adev_to_drm(adev)->primary->debugfs_root;
> @@ -2204,8 +2110,6 @@ int amdgpu_debugfs_init(struct amdgpu_device *adev)
>  			    &amdgpu_debugfs_vm_info_fops);
>  	debugfs_create_file("amdgpu_benchmark", 0200, root, adev,
>  			    &amdgpu_benchmark_fops);
> -	debugfs_create_file("amdgpu_reset_dump_register_list", 0644, root, adev,
> -			    &amdgpu_reset_dump_register_list);
>  
>  	adev->debugfs_vbios_blob.data = adev->bios;
>  	adev->debugfs_vbios_blob.size = adev->bios_size;

[PATCH v2 2/2] drm/amdgpu: Clean up the register dump via debugfs list

[Sunil Khatri]

debugfs register list for dump is cleaned as it have
some issues related to proper power state of the IP
before register read.

Since the above mentioned is removed we no longer want
this to be dumped part of the devcoredump and hence
removed.

Signed-off-by: Sunil Khatri <sunil.khatri@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h           | 13 -------------
 .../gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c  | 10 +---------
 drivers/gpu/drm/amd/amdgpu/amdgpu_device.c    | 19 -------------------
 3 files changed, 1 insertion(+), 41 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 137a88b8de45..c54ddd3e68aa 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -823,17 +823,6 @@ struct amdgpu_mqd {
 struct amdgpu_reset_domain;
 struct amdgpu_fru_info;
 
-struct amdgpu_reset_info {
-	/* reset dump register */
-	u32 *reset_dump_reg_list;
-	u32 *reset_dump_reg_value;
-	int num_regs;
-
-#ifdef CONFIG_DEV_COREDUMP
-	struct amdgpu_coredump_info *coredump_info;
-#endif
-};
-
 /*
  * Non-zero (true) if the GPU has VRAM. Zero (false) otherwise.
  */
@@ -1157,8 +1146,6 @@ struct amdgpu_device {
 
 	struct mutex			benchmark_mutex;
 
-	struct amdgpu_reset_info	reset_info;
-
 	bool                            scpm_enabled;
 	uint32_t                        scpm_status;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c
index f6806ae1c061..cf2b4dd4d865 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dev_coredump.c
@@ -203,7 +203,7 @@ amdgpu_devcoredump_read(char *buffer, loff_t offset, size_t count,
 	struct amdgpu_coredump_info *coredump = data;
 	struct drm_print_iterator iter;
 	struct amdgpu_vm_fault_info *fault_info;
-	int i, ver;
+	int ver;
 
 	iter.data = buffer;
 	iter.offset = 0;
@@ -317,14 +317,6 @@ amdgpu_devcoredump_read(char *buffer, loff_t offset, size_t count,
 
 	if (coredump->reset_vram_lost)
 		drm_printf(&p, "VRAM is lost due to GPU reset!\n");
-	if (coredump->adev->reset_info.num_regs) {
-		drm_printf(&p, "AMDGPU register dumps:\nOffset:     Value:\n");
-
-		for (i = 0; i < coredump->adev->reset_info.num_regs; i++)
-			drm_printf(&p, "0x%08x: 0x%08x\n",
-				   coredump->adev->reset_info.reset_dump_reg_list[i],
-				   coredump->adev->reset_info.reset_dump_reg_value[i]);
-	}
 
 	return count - iter.remain;
 }
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
index a2a1a3da17e3..3a43754e7f10 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
@@ -5277,23 +5277,6 @@ int amdgpu_device_mode1_reset(struct amdgpu_device *adev)
 	return ret;
 }
 
-static int amdgpu_reset_reg_dumps(struct amdgpu_device *adev)
-{
-	int i;
-
-	lockdep_assert_held(&adev->reset_domain->sem);
-
-	for (i = 0; i < adev->reset_info.num_regs; i++) {
-		adev->reset_info.reset_dump_reg_value[i] =
-			RREG32(adev->reset_info.reset_dump_reg_list[i]);
-
-		trace_amdgpu_reset_reg_dumps(adev->reset_info.reset_dump_reg_list[i],
-					     adev->reset_info.reset_dump_reg_value[i]);
-	}
-
-	return 0;
-}
-
 int amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
 				 struct amdgpu_reset_context *reset_context)
 {
@@ -5359,8 +5342,6 @@ int amdgpu_device_pre_asic_reset(struct amdgpu_device *adev,
 		}
 
 		if (!test_bit(AMDGPU_SKIP_COREDUMP, &reset_context->flags)) {
-			amdgpu_reset_reg_dumps(tmp_adev);
-
 			dev_info(tmp_adev->dev, "Dumping IP State\n");
 			/* Trigger ip dump before we reset the asic */
 			for (i = 0; i < tmp_adev->num_ip_blocks; i++)
-- 
2.34.1