<div dir="ltr">The IOCTL data is in the debugfs header as it is. I could move that to the amdgpu_drm.h and include it from amdgpu_debugfs.h.<div> </div><div>I'll re-write the STATE IOCTL to use a struct and then test against what I have in umr.</div><div> </div><div>Refactoring the read/write is trivial and I'll do that no problem (with style fixes).</div><div> </div><div>Cheers,</div><div>Tom</div></div> <div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Aug 24, 2021 at 8:34 AM Christian König <<a href="mailto:ckoenig.leichtzumerken@gmail.com">ckoenig.leichtzumerken@gmail.com</a>> wrote: </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Am 24.08.21 um 14:27 schrieb StDenis, Tom: > [AMD Official Use Only] > > What do you mean a "shared header?" How would they be shared between kernel and user? Somewhere in the include/uapi/drm/ folder I think. Either add that to amdgpu_drm.h or maybe amdgpu_debugfs.h? Or just keep it as a structure in amdgpu_debugfs.h with a comment that it is used for an IOCTL. Just not something hard coded like first byte is this, second that etc.... > As for why not read/write. Jus wanted to keep it simple. It's not really performance bound. umr never does reads/writes larger than 32-bits anyways. It doesn't have to be this way though but I figured the fewer LOC the better. I think it would be cleaner if we would have separate functions for this. As far as I can see you also don't need the dance with the power managerment etc for the IOCTL. It's just get_user() into your structure. BTW: In the kernel coding style put "switch" and "case" on the same column, otherwise <a href="http://checkpatch.pl" rel="noreferrer" target="_blank">checkpatch.pl</a> might complain. Christian. > > Tom > > ________________________________________ > From: Christian König <<a href="mailto:ckoenig.leichtzumerken@gmail.com" target="_blank">ckoenig.leichtzumerken@gmail.com</a>> > Sent: Tuesday, August 24, 2021 08:20 > To: StDenis, Tom; <a href="mailto:amd-gfx@lists.freedesktop.org" target="_blank">amd-gfx@lists.freedesktop.org</a> > Subject: Re: [PATCH] drm/amd/amdgpu: New debugfs interface for MMIO registers > > > > Am 24.08.21 um 14:16 schrieb Tom St Denis: >> This new debugfs interface uses an IOCTL interface in order to pass >> along state information like SRBM and GRBM bank switching. This >> new interface also allows a full 32-bit MMIO address range which >> the previous didn't. With this new design we have room to grow >> the flexibility of the file as need be. >> >> Signed-off-by: Tom St Denis <<a href="mailto:tom.stdenis@amd.com" target="_blank">tom.stdenis@amd.com</a>> >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 177 ++++++++++++++++++++ >> drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h | 32 ++++ >> 2 files changed, 209 insertions(+) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >> index 277128846dd1..ab2d92f84da5 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c >> @@ -279,6 +279,173 @@ static ssize_t amdgpu_debugfs_regs_write(struct file *f, const char __user *buf, >> return amdgpu_debugfs_process_reg_op(false, f, (char __user *)buf, size, pos); >> } >> >> +static int amdgpu_debugfs_regs2_open(struct inode *inode, struct file *file) >> +{ >> + struct amdgpu_debugfs_regs2_data *rd; >> + >> + rd = kzalloc(sizeof *rd, GFP_KERNEL); >> + if (!rd) >> + return -ENOMEM; >> + rd->adev = file_inode(file)->i_private; >> + file->private_data = rd; >> + >> + return 0; >> +} >> + >> +static int amdgpu_debugfs_regs2_release(struct inode *inode, struct file *file) >> +{ >> + kfree(file->private_data); >> + return 0; >> +} >> + >> +static int amdgpu_debugfs_regs2_op(struct file *f, char __user *buf, int write_en) >> +{ >> + struct amdgpu_debugfs_regs2_data *rd = f->private_data; >> + struct amdgpu_device *adev = rd->adev; >> + int result = 0, r; >> + uint32_t value; >> + >> + if (rd->state.offset & 0x3) >> + return -EINVAL; >> + >> + if (rd->state.use_grbm) { >> + if (rd-><a href="http://state.grbm.se" rel="noreferrer" target="_blank">state.grbm.se</a> == 0x3FF) >> + rd-><a href="http://state.grbm.se" rel="noreferrer" target="_blank">state.grbm.se</a> = 0xFFFFFFFF; >> + if (rd-><a href="http://state.grbm.sh" rel="noreferrer" target="_blank">state.grbm.sh</a> == 0x3FF) >> + rd-><a href="http://state.grbm.sh" rel="noreferrer" target="_blank">state.grbm.sh</a> = 0xFFFFFFFF; >> + if (rd->state.grbm.instance == 0x3FF) >> + rd->state.grbm.instance = 0xFFFFFFFF; >> + } >> + >> + r = pm_runtime_get_sync(adev_to_drm(adev)->dev); >> + if (r < 0) { >> + pm_runtime_put_autosuspend(adev_to_drm(adev)->dev); >> + return r; >> + } >> + >> + r = amdgpu_virt_enable_access_debugfs(adev); >> + if (r < 0) { >> + pm_runtime_put_autosuspend(adev_to_drm(adev)->dev); >> + return r; >> + } >> + >> + if (rd->state.use_grbm) { >> + if ((rd-><a href="http://state.grbm.sh" rel="noreferrer" target="_blank">state.grbm.sh</a> != 0xFFFFFFFF && rd-><a href="http://state.grbm.sh" rel="noreferrer" target="_blank">state.grbm.sh</a> >= adev->gfx.config.max_sh_per_se) || >> + (rd-><a href="http://state.grbm.se" rel="noreferrer" target="_blank">state.grbm.se</a> != 0xFFFFFFFF && rd-><a href="http://state.grbm.se" rel="noreferrer" target="_blank">state.grbm.se</a> >= adev->gfx.config.max_shader_engines)) { >> + pm_runtime_mark_last_busy(adev_to_drm(adev)->dev); >> + pm_runtime_put_autosuspend(adev_to_drm(adev)->dev); >> + amdgpu_virt_disable_access_debugfs(adev); >> + return -EINVAL; >> + } >> + mutex_lock(&adev->grbm_idx_mutex); >> + amdgpu_gfx_select_se_sh(adev, rd-><a href="http://state.grbm.se" rel="noreferrer" target="_blank">state.grbm.se</a>, >> + rd-><a href="http://state.grbm.sh" rel="noreferrer" target="_blank">state.grbm.sh</a>, >> + rd->state.grbm.instance); >> + } else if (rd->state.use_grbm) { >> + mutex_lock(&adev->srbm_mutex); >> + amdgpu_gfx_select_me_pipe_q(adev, rd-><a href="http://state.srbm.me" rel="noreferrer" target="_blank">state.srbm.me</a>, rd->state.srbm.pipe, >> + rd->state.srbm.queue, rd->state.srbm.vmid); >> + } >> + >> + if (rd->state.pg_lock) >> + mutex_lock(&adev->pm.mutex); >> + >> + if (!write_en) { >> + value = RREG32(rd->state.offset >> 2); >> + r = put_user(value, (uint32_t *)buf); >> + } else { >> + r = get_user(value, (uint32_t *)buf); >> + if (!r) >> + amdgpu_mm_wreg_mmio_rlc(adev, rd->state.offset >> 2, value); >> + } >> + if (r) { >> + result = r; >> + goto end; >> + } >> + result = 0; >> +end: >> + if (rd->state.use_grbm) { >> + amdgpu_gfx_select_se_sh(adev, 0xffffffff, 0xffffffff, 0xffffffff); >> + mutex_unlock(&adev->grbm_idx_mutex); >> + } else if (rd->state.use_srbm) { >> + amdgpu_gfx_select_me_pipe_q(adev, 0, 0, 0, 0); >> + mutex_unlock(&adev->srbm_mutex); >> + } >> + >> + if (rd->state.pg_lock) >> + mutex_unlock(&adev->pm.mutex); >> + >> + // in umr (the likely user of this) flags are set per file operation >> + // which means they're never "unset" explicitly. To avoid breaking >> + // this convention we unset the flags after each operation >> + // flags are for a single call (need to be set for every read/write) >> + rd->state.use_grbm = 0; >> + rd->state.use_srbm = 0; >> + rd->state.pg_lock = 0; >> + >> + pm_runtime_mark_last_busy(adev_to_drm(adev)->dev); >> + pm_runtime_put_autosuspend(adev_to_drm(adev)->dev); >> + >> + amdgpu_virt_disable_access_debugfs(adev); >> + return result; >> +} >> + >> +static long amdgpu_debugfs_regs2_ioctl(struct file *f, unsigned int cmd, unsigned long data) >> +{ >> + struct amdgpu_debugfs_regs2_data *rd = f->private_data; >> + unsigned char st[32], v; >> + int r, x; >> + >> + // always read first 4 bytes of data >> + for (x = 0; x < 4; x++) { >> + if ((r = get_user(v, (unsigned char *)data))) { >> + return r; >> + } >> + ++data; >> + st[x] = v; >> + } >> + >> + // first 4 bytes are offset >> + rd->state.offset = ((u32)st[0]) | ((u32)st[1] << 8) | >> + ((u32)st[2] << 16) | ((u32)st[3] << 24); >> + >> + switch (cmd) { >> + case AMDGPU_DEBUGFS_REGS2_IOC_SET_STATE: >> + for (x = 4; x < 32; x++) { >> + if ((r = get_user(v, (unsigned char *)data))) { >> + return r; >> + } >> + ++data; >> + st[x] = v; >> + } >> + >> + // next byte contains the flag >> + // we only consume the remainder of the state if bit 1 is set >> + // this allows the subsequent read/write >> + rd->state.use_grbm = (st[4] & 1) ? 1 : 0; >> + rd->state.use_srbm = (st[4] & 2) ? 1 : 0; >> + rd->state.pg_lock = (st[4] & 4) ? 1 : 0; >> + >> + // next 6 bytes are grbm data >> + rd-><a href="http://state.grbm.se" rel="noreferrer" target="_blank">state.grbm.se</a> = ((u32)st[5]) | ((u32)st[6] << 8); >> + rd-><a href="http://state.grbm.sh" rel="noreferrer" target="_blank">state.grbm.sh</a> = ((u32)st[7]) | ((u32)st[8] << 8); >> + rd->state.grbm.instance = ((u32)st[9]) | ((u32)st[10] << 8); >> + >> + // next 8 are srbm data >> + rd-><a href="http://state.srbm.me" rel="noreferrer" target="_blank">state.srbm.me</a> = ((u32)st[11]) | ((u32)st[12] << 8); >> + rd->state.srbm.pipe = ((u32)st[13]) | ((u32)st[14] << 8); >> + rd->state.srbm.queue = ((u32)st[15]) | ((u32)st[16] << 8); >> + rd->state.srbm.vmid = ((u32)st[17]) | ((u32)st[18] << 8); >> + break; >> + case AMDGPU_DEBUGFS_REGS2_IOC_READ: >> + return amdgpu_debugfs_regs2_op(f, (char __user *)data, 0); >> + case AMDGPU_DEBUGFS_REGS2_IOC_WRITE: >> + return amdgpu_debugfs_regs2_op(f, (char __user *)data, 1); >> + default: >> + return -EINVAL; >> + } >> + return 0; >> +} >> >> /** >> * amdgpu_debugfs_regs_pcie_read - Read from a PCIE register >> @@ -1091,6 +1258,14 @@ static ssize_t amdgpu_debugfs_gfxoff_read(struct file *f, char __user *buf, >> return result; >> } >> >> +static const struct file_operations amdgpu_debugfs_regs2_fops = { >> + .owner = THIS_MODULE, >> + .unlocked_ioctl = amdgpu_debugfs_regs2_ioctl, >> + .open = amdgpu_debugfs_regs2_open, >> + .release = amdgpu_debugfs_regs2_release, >> + .llseek = default_llseek >> +}; >> + >> static const struct file_operations amdgpu_debugfs_regs_fops = { >> .owner = THIS_MODULE, >> .read = amdgpu_debugfs_regs_read, >> @@ -1148,6 +1323,7 @@ static const struct file_operations amdgpu_debugfs_gfxoff_fops = { >> >> static const struct file_operations *debugfs_regs[] = { >> &amdgpu_debugfs_regs_fops, >> + &amdgpu_debugfs_regs2_fops, >> &amdgpu_debugfs_regs_didt_fops, >> &amdgpu_debugfs_regs_pcie_fops, >> &amdgpu_debugfs_regs_smc_fops, >> @@ -1160,6 +1336,7 @@ static const struct file_operations *debugfs_regs[] = { >> >> static const char *debugfs_regs_names[] = { >> "amdgpu_regs", >> + "amdgpu_regs2", >> "amdgpu_regs_didt", >> "amdgpu_regs_pcie", >> "amdgpu_regs_smc", >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h >> index 141a8474e24f..04c81cd4bcc7 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h >> @@ -22,6 +22,7 @@ >> * OTHER DEALINGS IN THE SOFTWARE. >> * >> */ >> +#include <linux/ioctl.h> >> >> /* >> * Debugfs >> @@ -38,3 +39,34 @@ void amdgpu_debugfs_fence_init(struct amdgpu_device *adev); >> void amdgpu_debugfs_firmware_init(struct amdgpu_device *adev); >> void amdgpu_debugfs_gem_init(struct amdgpu_device *adev); >> int amdgpu_debugfs_wait_dump(struct amdgpu_device *adev); >> + >> +struct amdgpu_debugfs_regs2_data { >> + struct amdgpu_device *adev; >> + struct { >> + // regs state >> + int use_srbm, >> + use_grbm, >> + pg_lock; >> + struct { >> + u32 se, sh, instance; >> + } grbm; >> + struct { >> + u32 me, pipe, queue, vmid; >> + } srbm; >> + u32 offset; >> + } state; >> +}; > This stuff needs to be in some shared header then. > >> + >> +enum AMDGPU_DEBUGFS_REGS2_CMDS { >> + AMDGPU_DEBUGFS_REGS2_CMD_SET_STATE=0, >> + AMDGPU_DEBUGFS_REGS2_CMD_READ, >> + AMDGPU_DEBUGFS_REGS2_CMD_WRITE, > Why not just using the normal read and write functions? > > Christian. > >> +}; >> + >> +struct amdgpu_debugfs_regs2_iocdata { >> + unsigned char t[32]; >> +}; >> + >> +#define AMDGPU_DEBUGFS_REGS2_IOC_SET_STATE _IOWR(0x20, AMDGPU_DEBUGFS_REGS2_CMD_SET_STATE, struct amdgpu_debugfs_regs2_iocdata) >> +#define AMDGPU_DEBUGFS_REGS2_IOC_READ _IOWR(0x20, AMDGPU_DEBUGFS_REGS2_CMD_WRITE, struct amdgpu_debugfs_regs2_iocdata) >> +#define AMDGPU_DEBUGFS_REGS2_IOC_WRITE _IOWR(0x20, AMDGPU_DEBUGFS_REGS2_CMD_READ, struct amdgpu_debugfs_regs2_iocdata) </blockquote></div>