[PATCH] drm/amd/amdgpu: Make debugfs write compliment read
Christian König
deathsimple at vodafone.de
Tue Oct 11 12:35:14 UTC 2016
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>
More information about the amd-gfx
mailing list