[PATCH] drm/amd/amdgpu: Make debugfs write compliment read

[Christian König]

> The only downside to taking the locks all the time is for 99% of debug 
> reads it's unnecessary and in theory could lead to 
> performance/behaviour problems.
>
Yeah, but performance isn't critical here and taking two locks compared 
to the rest of what we (IOCTL etc...) do is completely negligibly.

> I've been using this on read for a while now and haven't noticed a 
> problem.
>
My concern is rather that somebody tries to write to the PM regs on 
accident or purpose without taking the lock.

See, the kernel is usually responsible to keep people from doing stupid 
things with the hardware, even if those people are root and could do 
stupid things anyway. Well, you know what I mean?

Regards,
Christian.

Am 10.10.2016 um 22:21 schrieb StDenis, Tom:
>
> The only downside to taking the locks all the time is for 99% of debug 
> reads it's unnecessary and in theory could lead to 
> performance/behaviour problems.
>
>
> I've been using this on read for a while now and haven't noticed a 
> problem.
>
>
> Tom
>
>
>
> ------------------------------------------------------------------------
> *From:* Christian König <deathsimple at vodafone.de>
> *Sent:* Monday, October 10, 2016 09:49
> *To:* Tom St Denis; amd-gfx at lists.freedesktop.org
> *Cc:* StDenis, Tom
> *Subject:* Re: [PATCH] drm/amd/amdgpu: Make debugfs write compliment read
> Would it hurt us to always take the looks (both the pm as well as grbm 
> idx lock)?
>
> I don't think that would be much of an issue and I would have a better 
> feeling with that.
>
> Christian.
>
> Am 10.10.2016 um 15:38 schrieb Tom St Denis:
>>
>> that's kind of the point.  Its for debugging only.  Also reads can 
>> already lock
>>
>>
>> On Mon, Oct 10, 2016, 09:25 Christian König <deathsimple at vodafone.de 
>> <mailto:deathsimple at vodafone.de>> wrote:
>>
>>     Mhm, that userspace controls if the lock is taken or not is a bit
>>     problematic, on the other hand only root could access that.
>>
>>     Christian.
>>
>>     Am 10.10.2016 um 14:51 schrieb Tom St Denis:
>>     > Add PG lock support as well as bank selection to
>>     > the MMIO write function.
>>     >
>>     > Signed-off-by: Tom St Denis <tom.stdenis at amd.com
>>     <mailto:tom.stdenis at amd.com>>
>>     > ---
>>     >   drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 43
>>     ++++++++++++++++++++++++++++++
>>     >   1 file changed, 43 insertions(+)
>>     >
>>     > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>     b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>     > index 04c4aee70452..a9d83c1d7a43 100644
>>     > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>     > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c
>>     > @@ -2637,10 +2637,45 @@ static ssize_t
>>     amdgpu_debugfs_regs_write(struct file *f, const char __user *buf,
>>     >       struct amdgpu_device *adev = f->f_inode->i_private;
>>     >       ssize_t result = 0;
>>     >       int r;
>>     > +     bool pm_pg_lock, use_bank;
>>     > +     unsigned instance_bank, sh_bank, se_bank;
>>     >
>>     >       if (size & 0x3 || *pos & 0x3)
>>     >               return -EINVAL;
>>     >
>>     > +     /* are we reading registers for which a PG lock is
>>     necessary? */
>>     > +     pm_pg_lock = (*pos >> 23) & 1;
>>     > +
>>     > +     if (*pos & (1ULL << 62)) {
>>     > +             se_bank = (*pos >> 24) & 0x3FF;
>>     > +             sh_bank = (*pos >> 34) & 0x3FF;
>>     > +             instance_bank = (*pos >> 44) & 0x3FF;
>>     > +
>>     > +             if (se_bank == 0x3FF)
>>     > +                     se_bank = 0xFFFFFFFF;
>>     > +             if (sh_bank == 0x3FF)
>>     > +                     sh_bank = 0xFFFFFFFF;
>>     > +             if (instance_bank == 0x3FF)
>>     > +                     instance_bank = 0xFFFFFFFF;
>>     > +             use_bank = 1;
>>     > +     } else {
>>     > +             use_bank = 0;
>>     > +     }
>>     > +
>>     > +     *pos &= 0x3FFFF;
>>     > +
>>     > +     if (use_bank) {
>>     > +             if ((sh_bank != 0xFFFFFFFF && sh_bank >=
>>     adev->gfx.config.max_sh_per_se) ||
>>     > +                 (se_bank != 0xFFFFFFFF && 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);
>>     > +     }
>>     > +
>>     > +     if (pm_pg_lock)
>>     > +             mutex_lock(&adev->pm.mutex);
>>     > +
>>     >       while (size) {
>>     >               uint32_t value;
>>     >
>>     > @@ -2659,6 +2694,14 @@ static ssize_t
>>     amdgpu_debugfs_regs_write(struct file *f, const char __user *buf,
>>     >               size -= 4;
>>     >       }
>>     >
>>     > +     if (use_bank) {
>>     > +             amdgpu_gfx_select_se_sh(adev, 0xffffffff,
>>     0xffffffff, 0xffffffff);
>>     > +  mutex_unlock(&adev->grbm_idx_mutex);
>>     > +     }
>>     > +
>>     > +     if (pm_pg_lock)
>>     > +  mutex_unlock(&adev->pm.mutex);
>>     > +
>>     >       return result;
>>     >   }
>>     >
>>
>>
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20161011/be72902e/attachment-0001.html>