<html> <head> <meta http-equiv="Content-Type" content="text/html; charset=utf-8"> <style type="text/css" style="display:none;"></style> </head> <body dir="ltr"> <div id="divtagdefaultwrapper" style="font-size:12pt;color:#000000;background-color:#FFFFFF;font-family:Calibri,Arial,Helvetica,sans-serif;"> Bike shedding accepted. <img alt="😊" class="EmojiInsert" style="vertical-align: bottom;" src="cid:2164fc72-30fb-46f1-8408-487a551a49cd"> Of course now I have to circle back and fix the debug tool... Tom <div style="color: rgb(0, 0, 0);"> <div> <hr tabindex="-1" style="display:inline-block; width:98%"> <div id="x_divRplyFwdMsg" dir="ltr">From: Nicolai Hähnle <nhaehnle@gmail.com> Sent: Wednesday, June 29, 2016 09:50 To: Tom St Denis; amd-gfx@lists.freedesktop.org Cc: StDenis, Tom Subject: Re: [PATCH 3/3] drm/amd/amdgpu: Add bank selection for MMIO debugfs <div> </div> </div> </div> <div class="PlainText">On 29.06.2016 12:15, Tom St Denis wrote: > (v2) Added INSTANCE selector > > Signed-off-by: Tom St Denis <tom.stdenis@amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_device.c | 35 +++++++++++++++++++++++++++--- > 1 file changed, 32 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > index 7f08ac02f9de..bea5f90f479e 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_device.c > @@ -2244,20 +2244,43 @@ static ssize_t amdgpu_debugfs_regs_read(struct file *f, char __user *buf, > struct amdgpu_device *adev = f->f_inode->i_private; > ssize_t result = 0; > int r; > + bool use_bank; > + unsigned instance_bank, sh_bank, se_bank; > > if (size & 0x3 || *pos & 0x3) > return -EINVAL; > > + if (*pos & (1ULL << 62)) { > + sh_bank = (*pos >> 24) & 0x3FF; > + se_bank = (*pos >> 34) & 0x3FF; > + instance_bank = (*pos >> 44) & 0x3FF; It's a bit of an bike-shedding quibble, but the bit shift should be ordered SE -> SH -> INSTANCE, since the SE is the highest-level concept and the instance number is the lowest level. Apart from that and the comment on #1, patch 1 and 3 look good to me. (Can't really say much about patch 2.) Cheers, Nicolai > + use_bank = 1; > + *pos &= 0xFFFFFF; > + } else { > + use_bank = 0; > + } > + > + if (use_bank) { > + if (sh_bank >= adev->gfx.config.max_sh_per_se || > + 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); > + } > + > while (size) { > uint32_t value; > > if (*pos > adev->rmmio_size) > - return result; > + goto end; > > value = RREG32(*pos >> 2); > r = put_user(value, (uint32_t *)buf); > - if (r) > - return r; > + if (r) { > + result = r; > + goto end; > + } > > result += 4; > buf += 4; > @@ -2265,6 +2288,12 @@ static ssize_t amdgpu_debugfs_regs_read(struct file *f, char __user *buf, > size -= 4; > } > > +end: > + if (use_bank) { > + amdgpu_gfx_select_se_sh(adev, 0xFFFFFFFF, 0xFFFFFFFF, 0xFFFFFFFF); > + mutex_unlock(&adev->grbm_idx_mutex); > + } > + > return result; > } > > </div> </div> </div> </body> </html>