<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=iso-8859-1"> <style type="text/css" style="display:none;"></style> </head> <body dir="ltr"> <div id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;font-family:Calibri,Arial,Helvetica,sans-serif;"> 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 <div style="color: rgb(0, 0, 0);"> <hr tabindex="-1" style="display:inline-block; width:98%"> <div id="divRplyFwdMsg" dir="ltr">From: Christian König <deathsimple@vodafone.de> Sent: Monday, October 10, 2016 09:49 To: Tom St Denis; amd-gfx@lists.freedesktop.org Cc: StDenis, Tom Subject: Re: [PATCH] drm/amd/amdgpu: Make debugfs write compliment read <div> </div> </div> <div> <div class="moz-cite-prefix">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: </div> <blockquote type="cite"> that's kind of the point. Its for debugging only. Also reads can already lock <div class="gmail_quote"> <div dir="ltr">On Mon, Oct 10, 2016, 09:25 Christian König <<a href="mailto:deathsimple@vodafone.de">deathsimple@vodafone.de</a>> wrote: </div> <blockquote class="gmail_quote" style="margin:0 0 0 .8ex; border-left:1px #ccc solid; padding-left:1ex"> 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 <<a href="mailto:tom.stdenis@amd.com" class="gmail_msg" target="_blank">tom.stdenis@amd.com</a>> > --- > 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; > } > </blockquote> </div> </blockquote> </div> </div> </div> </body> </html>