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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Tom,

yeah, the timing issue is a good argument. In this case just go ahead 
and add my Acked-by to the patch.

Regards,
Christian.

Am 11.10.2016 um 14:19 schrieb StDenis, Tom:
>
> Hi Christian,
>
>
> I agree with the crux of your argument but you can crash the system 
> with carefully timed read/writes from the GFX registers, or MC, or ... 
> That's why the entries are root only.
>
>
> The issue with locking excessively is you might change behaviour of 
> something you're trying to debug because the kernel then has to wait 
> on the lock (or you're waiting on the lock).  For instance, suppose 
> you want to read DCE registers and the kernel is doing some gfx/grbm 
> related your DCE read will either block the GFX activity or you're 
> waiting on it which means the behaviour of the system could be 
> different than from the case where you're not probing the registers.
>
>
> The goal is to be as unobtrusive as possible.  Heck I often mmap the 
> BAR so I can read MMIO registers directly and not waste CPU cycles on 
> syscalls.
>
>
> The PM lock is because reading PGFSM registers during a powerup/down 
> results in a GPU hang, and we only take the GRBM lock when users want 
> to read from specific banks.  For any other case we don't need a lock. 
>  In the kernel they should be mutually exclusive.  The PM lock is only 
> taken in PP when performing a transition (or initializing the core) 
> and I have yet to see a place where PP/PM related code also grabs the 
> grbm index lock.
>
> I don't have strong feelings against your proposal but I do prefer not 
> to take the locks when I don't need to.  Ultimately I'll leave the 
> decision up to you and Alex.  I'd like to get the 2nd patch through 
> though as it makes the write side of the mmio reg complimentary (e.g. 
> bank selection/pm lock).
>
> Cheers,
> Tom
>
> ------------------------------------------------------------------------
> *From:* Christian König <deathsimple at vodafone.de>
> *Sent:* Tuesday, October 11, 2016 08:11
> *To:* StDenis, Tom; amd-gfx at lists.freedesktop.org
> *Subject:* Re: [PATCH] drm/amd/amdgpu: Make debugfs write compliment read
>>
>> 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;
>>>     >   }
>>>     >
>>>
>>>
>>
>
>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


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


[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux