[PATCH] drm/amd/amdgpu: Merge the smc/pcie/didt debugfs read/write methods

[tstdenis@xxxxxxx (Tom St Denis)]

Hi Christian,

I was thinking of merging the didt/smc/pcie ops into one themselves. 
I'll see what I can come up with without spending too much time on it.

Cheers,
Tom

On 04/04/2018 10:02 AM, Christian König wrote:
> Am 04.04.2018 um 15:35 schrieb Tom St Denis:
>> Fold the read/write methods of the various register access
>> debugfs entries into op functions.
>>
>> Signed-off-by: Tom St Denis <tom.stdenis at amd.com>
> 
> I would rather split out a dword helper. E.g. something like:
> 
> amdgpu_debugfs_regs_dword_op(struct file *f, char __user *buf, size_t 
> size, loff_t *pos,
>  Â Â Â  Â Â Â  Â Â Â  Â Â Â  Â Â Â  Â Â Â  Â Â Â  Â Â Â  Â Â Â  Â Â Â  Â Â Â  Â Â Â  Â Â Â  Â Â Â  int 
> (*op)(struct amdgpu_device *adev, char __user *buf))
> {
> ...
> }
> 
> And then you have op callbacks for PCIE, didt, SMC each in a read/write 
> variant.
> 
> Would save quite a bunch more of loc if I'm not completely mistaken.
> 
> Christian.
> 
>> ---
>> Â  drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 140 
>> +++++++++++-----------------
>> Â  1 file changed, 57 insertions(+), 83 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c 
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>> index c98e59721444..b80e18469d25 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>> @@ -177,8 +177,8 @@ static ssize_t amdgpu_debugfs_regs_write(struct 
>> file *f, const char __user *buf,
>> Â Â Â Â Â  return amdgpu_debugfs_process_reg_op(false, f, (char __user 
>> *)buf, size, pos);
>> Â  }
>> -static ssize_t amdgpu_debugfs_regs_pcie_read(struct file *f, char 
>> __user *buf,
>> -Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  size_t size, loff_t *pos)
>> +static ssize_t amdgpu_debugfs_regs_pcie_op(bool read, struct file *f,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  char __user *buf, size_t size, loff_t *pos)
>> Â  {
>> Â Â Â Â Â  struct amdgpu_device *adev = file_inode(f)->i_private;
>> Â Â Â Â Â  ssize_t result = 0;
>> @@ -190,11 +190,18 @@ static ssize_t 
>> amdgpu_debugfs_regs_pcie_read(struct file *f, char __user *buf,
>> Â Â Â Â Â  while (size) {
>> Â Â Â Â Â Â Â Â Â  uint32_t value;
>> -Â Â Â Â Â Â Â  value = RREG32_PCIE(*pos >> 2);
>> -Â Â Â Â Â Â Â  r = put_user(value, (uint32_t *)buf);
>> -Â Â Â Â Â Â Â  if (r)
>> -Â Â Â Â Â Â Â Â Â Â Â  return r;
>> +Â Â Â Â Â Â Â  if (read) {
>> +Â Â Â Â Â Â Â Â Â Â Â  value = RREG32_PCIE(*pos >> 2);
>> +Â Â Â Â Â Â Â Â Â Â Â  r = put_user(value, (uint32_t *)buf);
>> +Â Â Â Â Â Â Â Â Â Â Â  if (r)
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  return r;
>> +Â Â Â Â Â Â Â  } else {
>> +Â Â Â Â Â Â Â Â Â Â Â  r = get_user(value, (uint32_t *)buf);
>> +Â Â Â Â Â Â Â Â Â Â Â  if (r)
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  return r;
>> +Â Â Â Â Â Â Â Â Â Â Â  WREG32_PCIE(*pos >> 2, value);
>> +Â Â Â Â Â Â Â  }
>> Â Â Â Â Â Â Â Â Â  result += 4;
>> Â Â Â Â Â Â Â Â Â  buf += 4;
>> Â Â Â Â Â Â Â Â Â  *pos += 4;
>> @@ -204,8 +211,20 @@ static ssize_t 
>> amdgpu_debugfs_regs_pcie_read(struct file *f, char __user *buf,
>> Â Â Â Â Â  return result;
>> Â  }
>> +static ssize_t amdgpu_debugfs_regs_pcie_read(struct file *f, char 
>> __user *buf,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  size_t size, loff_t *pos)
>> +{
>> +Â Â Â  return amdgpu_debugfs_regs_pcie_op(true, f, buf, size, pos);
>> +}
>> +
>> Â  static ssize_t amdgpu_debugfs_regs_pcie_write(struct file *f, const 
>> char __user *buf,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  size_t size, loff_t *pos)
>> +{
>> +Â Â Â  return amdgpu_debugfs_regs_pcie_op(false, f, (char __user *)buf, 
>> size, pos);
>> +}
>> +
>> +static ssize_t amdgpu_debugfs_regs_didt_op(bool read, struct file *f,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  char __user *buf, size_t size, loff_t *pos)
>> Â  {
>> Â Â Â Â Â  struct amdgpu_device *adev = file_inode(f)->i_private;
>> Â Â Â Â Â  ssize_t result = 0;
>> @@ -217,12 +236,18 @@ static ssize_t 
>> amdgpu_debugfs_regs_pcie_write(struct file *f, const char __user
>> Â Â Â Â Â  while (size) {
>> Â Â Â Â Â Â Â Â Â  uint32_t value;
>> -Â Â Â Â Â Â Â  r = get_user(value, (uint32_t *)buf);
>> -Â Â Â Â Â Â Â  if (r)
>> -Â Â Â Â Â Â Â Â Â Â Â  return r;
>> -
>> -Â Â Â Â Â Â Â  WREG32_PCIE(*pos >> 2, value);
>> +Â Â Â Â Â Â Â  if (read) {
>> +Â Â Â Â Â Â Â Â Â Â Â  value = RREG32_DIDT(*pos >> 2);
>> +Â Â Â Â Â Â Â Â Â Â Â  r = put_user(value, (uint32_t *)buf);
>> +Â Â Â Â Â Â Â Â Â Â Â  if (r)
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  return r;
>> +Â Â Â Â Â Â Â  } else {
>> +Â Â Â Â Â Â Â Â Â Â Â  r = get_user(value, (uint32_t *)buf);
>> +Â Â Â Â Â Â Â Â Â Â Â  if (r)
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  return r;
>> +Â Â Â Â Â Â Â Â Â Â Â  WREG32_DIDT(*pos >> 2, value);
>> +Â Â Â Â Â Â Â  }
>> Â Â Â Â Â Â Â Â Â  result += 4;
>> Â Â Â Â Â Â Â Â Â  buf += 4;
>> Â Â Â Â Â Â Â Â Â  *pos += 4;
>> @@ -235,32 +260,18 @@ static ssize_t 
>> amdgpu_debugfs_regs_pcie_write(struct file *f, const char __user
>> Â  static ssize_t amdgpu_debugfs_regs_didt_read(struct file *f, char 
>> __user *buf,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  size_t size, loff_t *pos)
>> Â  {
>> -Â Â Â  struct amdgpu_device *adev = file_inode(f)->i_private;
>> -Â Â Â  ssize_t result = 0;
>> -Â Â Â  int r;
>> -
>> -Â Â Â  if (size & 0x3 || *pos & 0x3)
>> -Â Â Â Â Â Â Â  return -EINVAL;
>> -
>> -Â Â Â  while (size) {
>> -Â Â Â Â Â Â Â  uint32_t value;
>> -
>> -Â Â Â Â Â Â Â  value = RREG32_DIDT(*pos >> 2);
>> -Â Â Â Â Â Â Â  r = put_user(value, (uint32_t *)buf);
>> -Â Â Â Â Â Â Â  if (r)
>> -Â Â Â Â Â Â Â Â Â Â Â  return r;
>> -
>> -Â Â Â Â Â Â Â  result += 4;
>> -Â Â Â Â Â Â Â  buf += 4;
>> -Â Â Â Â Â Â Â  *pos += 4;
>> -Â Â Â Â Â Â Â  size -= 4;
>> -Â Â Â  }
>> -
>> -Â Â Â  return result;
>> +Â Â Â  return amdgpu_debugfs_regs_didt_op(true, f, buf, size, pos);
>> Â  }
>> Â  static ssize_t amdgpu_debugfs_regs_didt_write(struct file *f, const 
>> char __user *buf,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  size_t size, loff_t *pos)
>> +{
>> +Â Â Â  return amdgpu_debugfs_regs_didt_op(false, f, (char __user *)buf, 
>> size, pos);
>> +}
>> +
>> +static ssize_t amdgpu_debugfs_regs_smc_op(bool read,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  struct file *f, char __user *buf,
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  size_t size, loff_t *pos)
>> Â  {
>> Â Â Â Â Â  struct amdgpu_device *adev = file_inode(f)->i_private;
>> Â Â Â Â Â  ssize_t result = 0;
>> @@ -272,11 +283,17 @@ static ssize_t 
>> amdgpu_debugfs_regs_didt_write(struct file *f, const char __user
>> Â Â Â Â Â  while (size) {
>> Â Â Â Â Â Â Â Â Â  uint32_t value;
>> -Â Â Â Â Â Â Â  r = get_user(value, (uint32_t *)buf);
>> -Â Â Â Â Â Â Â  if (r)
>> -Â Â Â Â Â Â Â Â Â Â Â  return r;
>> -
>> -Â Â Â Â Â Â Â  WREG32_DIDT(*pos >> 2, value);
>> +Â Â Â Â Â Â Â  if (read) {
>> +Â Â Â Â Â Â Â Â Â Â Â  value = RREG32_SMC(*pos);
>> +Â Â Â Â Â Â Â Â Â Â Â  r = put_user(value, (uint32_t *)buf);
>> +Â Â Â Â Â Â Â Â Â Â Â  if (r)
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  return r;
>> +Â Â Â Â Â Â Â  } else {
>> +Â Â Â Â Â Â Â Â Â Â Â  r = get_user(value, (uint32_t *)buf);
>> +Â Â Â Â Â Â Â Â Â Â Â  if (r)
>> +Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  return r;
>> +Â Â Â Â Â Â Â Â Â Â Â  WREG32_SMC(*pos, value);
>> +Â Â Â Â Â Â Â  }
>> Â Â Â Â Â Â Â Â Â  result += 4;
>> Â Â Â Â Â Â Â Â Â  buf += 4;
>> @@ -290,56 +307,13 @@ static ssize_t 
>> amdgpu_debugfs_regs_didt_write(struct file *f, const char __user
>> Â  static ssize_t amdgpu_debugfs_regs_smc_read(struct file *f, char 
>> __user *buf,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  size_t size, loff_t *pos)
>> Â  {
>> -Â Â Â  struct amdgpu_device *adev = file_inode(f)->i_private;
>> -Â Â Â  ssize_t result = 0;
>> -Â Â Â  int r;
>> -
>> -Â Â Â  if (size & 0x3 || *pos & 0x3)
>> -Â Â Â Â Â Â Â  return -EINVAL;
>> -
>> -Â Â Â  while (size) {
>> -Â Â Â Â Â Â Â  uint32_t value;
>> -
>> -Â Â Â Â Â Â Â  value = RREG32_SMC(*pos);
>> -Â Â Â Â Â Â Â  r = put_user(value, (uint32_t *)buf);
>> -Â Â Â Â Â Â Â  if (r)
>> -Â Â Â Â Â Â Â Â Â Â Â  return r;
>> -
>> -Â Â Â Â Â Â Â  result += 4;
>> -Â Â Â Â Â Â Â  buf += 4;
>> -Â Â Â Â Â Â Â  *pos += 4;
>> -Â Â Â Â Â Â Â  size -= 4;
>> -Â Â Â  }
>> -
>> -Â Â Â  return result;
>> +Â Â Â  return amdgpu_debugfs_regs_smc_op(true, f, buf, size, pos);
>> Â  }
>> Â  static ssize_t amdgpu_debugfs_regs_smc_write(struct file *f, const 
>> char __user *buf,
>> Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â Â  size_t size, loff_t *pos)
>> Â  {
>> -Â Â Â  struct amdgpu_device *adev = file_inode(f)->i_private;
>> -Â Â Â  ssize_t result = 0;
>> -Â Â Â  int r;
>> -
>> -Â Â Â  if (size & 0x3 || *pos & 0x3)
>> -Â Â Â Â Â Â Â  return -EINVAL;
>> -
>> -Â Â Â  while (size) {
>> -Â Â Â Â Â Â Â  uint32_t value;
>> -
>> -Â Â Â Â Â Â Â  r = get_user(value, (uint32_t *)buf);
>> -Â Â Â Â Â Â Â  if (r)
>> -Â Â Â Â Â Â Â Â Â Â Â  return r;
>> -
>> -Â Â Â Â Â Â Â  WREG32_SMC(*pos, value);
>> -
>> -Â Â Â Â Â Â Â  result += 4;
>> -Â Â Â Â Â Â Â  buf += 4;
>> -Â Â Â Â Â Â Â  *pos += 4;
>> -Â Â Â Â Â Â Â  size -= 4;
>> -Â Â Â  }
>> -
>> -Â Â Â  return result;
>> +Â Â Â  return amdgpu_debugfs_regs_smc_op(false, f, (char __user *)buf, 
>> size, pos);
>> Â  }
>> Â  static ssize_t amdgpu_debugfs_gca_config_read(struct file *f, char 
>> __user *buf,
>