[PATCH 3/3] drm/amd/amdgpu: Add bank selection for MMIO debugfs (v3)

[Nicolai Hähnle]

On 29.06.2016 16:22, StDenis, Tom wrote:
> Thanks.  I use vmalloc first to avoid putting the array on the stack (as
> good practice) then I chose vmalloc because there really isn't a need
> for contiguous memory and in theory it should be just as fast to
> allocate.  Wouldn't you typically use kmalloc when you need contiguous
> memory?  Though this all boils down to a single page since it's so small
> anyways.

If it's potentially non-contiguous memory, isn't there the overhead of 
page table manipulation?

Maybe somebody who's more familiar with the kernel has an opinion...

Cheers,
Nicolai

>
>
> Tom
>
>
>
>
>
> ------------------------------------------------------------------------
> *From:* Nicolai Hähnle <nhaehnle at gmail.com>
> *Sent:* Wednesday, June 29, 2016 10:20
> *To:* Tom St Denis; amd-gfx at lists.freedesktop.org
> *Cc:* StDenis, Tom
> *Subject:* Re: [PATCH 3/3] drm/amd/amdgpu: Add bank selection for MMIO
> debugfs (v3)
> Patch 1 & 3 are
>
> Reviewed-by: Nicolai Hähnle <nicolai.haehnle at amd.com>
>
> Patch 2 looks mostly fine to me, too, but I'm not sure about the
> vmalloc/vfree vs. kmalloc/kfree.
>
> Cheers,
> Nicolai
>
> On 29.06.2016 15:56, Tom St Denis wrote:
>> (v2) Added INSTANCE selector
>> (v3) Changed order of bank selectors
>>
>> Signed-off-by: Tom St Denis <tom.stdenis at amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 35 +++++++++++++++++++++++++++---
>>   1 file changed, 32 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> index 7f08ac02f9de..4a8f66c5ff43 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>> @@ -2244,20 +2244,43 @@ static ssize_t amdgpu_debugfs_regs_read(struct file *f, char __user *buf,
>>        struct amdgpu_device *adev = f->f_inode->i_private;
>>        ssize_t result = 0;
>>        int r;
>> +     bool use_bank;
>> +     unsigned instance_bank, sh_bank, se_bank;
>>
>>        if (size & 0x3 || *pos & 0x3)
>>                return -EINVAL;
>>
>> +     if (*pos & (1ULL << 62)) {
>> +             se_bank = (*pos >> 24) & 0x3FF;
>> +             sh_bank = (*pos >> 34) & 0x3FF;
>> +             instance_bank = (*pos >> 44) & 0x3FF;
>> +             use_bank = 1;
>> +             *pos &= 0xFFFFFF;
>> +     } else {
>> +             use_bank = 0;
>> +     }
>> +
>> +     if (use_bank) {
>> +             if (sh_bank >= adev->gfx.config.max_sh_per_se ||
>> +                 se_bank >= adev->gfx.config.max_shader_engines)
>> +                     return -EINVAL;
>> +             mutex_lock(&adev->grbm_idx_mutex);
>> +             amdgpu_gfx_select_se_sh(adev, se_bank,
>> +                                     sh_bank, instance_bank);
>> +     }
>> +
>>        while (size) {
>>                uint32_t value;
>>
>>                if (*pos > adev->rmmio_size)
>> -                     return result;
>> +                     goto end;
>>
>>                value = RREG32(*pos >> 2);
>>                r = put_user(value, (uint32_t *)buf);
>> -             if (r)
>> -                     return r;
>> +             if (r) {
>> +                     result = r;
>> +                     goto end;
>> +             }
>>
>>                result += 4;
>>                buf += 4;
>> @@ -2265,6 +2288,12 @@ static ssize_t amdgpu_debugfs_regs_read(struct file *f, char __user *buf,
>>                size -= 4;
>>        }
>>
>> +end:
>> +     if (use_bank) {
>> +             amdgpu_gfx_select_se_sh(adev, 0xffffffff, 0xffffffff, 0xffffffff);
>> +             mutex_unlock(&adev->grbm_idx_mutex);
>> +     }
>> +
>>        return result;
>>   }
>>
>>