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>
