Using u8 is ok as well, just make sure that you don't have any hidden 
padding.

Nirmoy had a tool to double check for paddings which I once more forgot 
the name of.

Christian.

Am 25.08.21 um 12:40 schrieb Tom St Denis:
> The struct works as is but I'll change them to u32.  The offset is an 
> artefact of the fact this was an IOCTL originally.  I'm working both 
> ends in parallel trying to make the changes at the same time because 
> I'm only submitting the kernel patch if I've tested it in userspace.
>
> I'll send a v4 in a bit this morning....
>
> Tom
>
> On Wed, Aug 25, 2021 at 2:35 AM Christian König 
> <ckoenig.leichtzumerken@gmail.com 
> <mailto:ckoenig.leichtzumerken@gmail.com>> wrote:
>
>
>
>     Am 24.08.21 um 15:36 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.
>     >
>     > (v2): Move read/write to .read/.write, fix style, add comment
>     >        for IOCTL data structure
>     >
>     > Signed-off-by: Tom St Denis <tom.stdenis@amd.com
>     <mailto:tom.stdenis@amd.com>>
>     > ---
>     >   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 162
>     ++++++++++++++++++++
>     >   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h |  32 ++++
>     >   2 files changed, 194 insertions(+)
>     >
>     > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>     b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>     > index 277128846dd1..8e8f5743c8f5 100644
>     > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>     > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>     > @@ -279,6 +279,156 @@ 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 ssize_t amdgpu_debugfs_regs2_op(struct file *f, char
>     __user *buf, size_t size, int write_en)
>     > +{
>     > +     struct amdgpu_debugfs_regs2_data *rd = f->private_data;
>     > +     struct amdgpu_device *adev = rd->adev;
>     > +     ssize_t result = 0;
>     > +     int r;
>     > +     uint32_t value;
>     > +
>     > +     if (size & 0x3 || rd->state.offset & 0x3)
>     > +             return -EINVAL;
>     > +
>     > +     if (rd->state.id.use_grbm) {
>     > +             if (rd->state.id.grbm.se <http://state.id.grbm.se>
>     == 0x3FF)
>     > +                     rd->state.id.grbm.se
>     <http://state.id.grbm.se> = 0xFFFFFFFF;
>     > +             if (rd->state.id.grbm.sh <http://state.id.grbm.sh>
>     == 0x3FF)
>     > +                     rd->state.id.grbm.sh
>     <http://state.id.grbm.sh> = 0xFFFFFFFF;
>     > +             if (rd->state.id.grbm.instance == 0x3FF)
>     > +                     rd->state.id.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.id.use_grbm) {
>     > +             if ((rd->state.id.grbm.sh
>     <http://state.id.grbm.sh> != 0xFFFFFFFF && rd->state.id.grbm.sh
>     <http://state.id.grbm.sh> >= adev->gfx.config.max_sh_per_se) ||
>     > +                 (rd->state.id.grbm.se
>     <http://state.id.grbm.se> != 0xFFFFFFFF && rd->state.id.grbm.se
>     <http://state.id.grbm.se> >= 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->state.id.grbm.se
>     <http://state.id.grbm.se>,
>     > +      rd->state.id.grbm.sh <http://state.id.grbm.sh>,
>     > +      rd->state.id.grbm.instance);
>     > +     }
>     > +
>     > +     if (rd->state.id.use_srbm) {
>     > +             mutex_lock(&adev->srbm_mutex);
>     > +             amdgpu_gfx_select_me_pipe_q(adev,
>     rd->state.id.srbm.me <http://state.id.srbm.me>,
>     rd->state.id.srbm.pipe,
>     > +              rd->state.id.srbm.queue, rd->state.id.srbm.vmid);
>     > +     }
>     > +
>     > +     if (rd->state.id.pg_lock)
>     > +             mutex_lock(&adev->pm.mutex);
>     > +
>     > +     while (size) {
>     > +             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;
>     > +             }
>     > +             rd->state.offset += 4;
>     > +             size -= 4;
>     > +             result += 4;
>     > +             buf += 4;
>     > +     }
>     > +end:
>     > +     if (rd->state.id.use_grbm) {
>     > +             amdgpu_gfx_select_se_sh(adev, 0xffffffff,
>     0xffffffff, 0xffffffff);
>     > +             mutex_unlock(&adev->grbm_idx_mutex);
>     > +     }
>     > +
>     > +     if (rd->state.id.use_srbm) {
>     > +             amdgpu_gfx_select_me_pipe_q(adev, 0, 0, 0, 0);
>     > +             mutex_unlock(&adev->srbm_mutex);
>     > +     }
>     > +
>     > +     if (rd->state.id.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.id.use_grbm = 0;
>     > +     rd->state.id.use_srbm = 0;
>     > +     rd->state.id.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;
>     > +
>     > +     switch (cmd) {
>     > +     case AMDGPU_DEBUGFS_REGS2_IOC_SET_STATE:
>     > +             if (copy_from_user(&rd->state.id
>     <http://state.id>, (struct amdgpu_debugfs_regs2_iocdata *)data,
>     sizeof rd->state.id <http://state.id>))
>     > +                     return -EINVAL;
>     > +             break;
>     > +     default:
>     > +             return -EINVAL;
>     > +     }
>     > +     return 0;
>     > +}
>     > +
>     > +static ssize_t amdgpu_debugfs_regs2_read(struct file *f, char
>     __user *buf, size_t size, loff_t *pos)
>     > +{
>     > +     struct amdgpu_debugfs_regs2_data *rd = f->private_data;
>     > +     rd->state.offset = *pos;
>     > +     return amdgpu_debugfs_regs2_op(f, buf, size, 0);
>     > +}
>     > +
>     > +static ssize_t amdgpu_debugfs_regs2_write(struct file *f, const
>     char __user *buf, size_t size, loff_t *pos)
>     > +{
>     > +     struct amdgpu_debugfs_regs2_data *rd = f->private_data;
>     > +     rd->state.offset = *pos;
>     > +     return amdgpu_debugfs_regs2_op(f, (char __user *)buf,
>     size, 1);
>     > +}
>     > +
>     >
>     >   /**
>     >    * amdgpu_debugfs_regs_pcie_read - Read from a PCIE register
>     > @@ -1091,6 +1241,16 @@ 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,
>     > +     .read = amdgpu_debugfs_regs2_read,
>     > +     .write = amdgpu_debugfs_regs2_write,
>     > +     .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 +1308,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 +1321,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..ec044df5d428 100644
>     > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h
>     > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.h
>     > @@ -22,6 +22,8 @@
>     >    * OTHER DEALINGS IN THE SOFTWARE.
>     >    *
>     >    */
>     > +#include <linux/ioctl.h>
>     > +#include <uapi/drm/amdgpu_drm.h>
>     >
>     >   /*
>     >    * Debugfs
>     > @@ -38,3 +40,33 @@ 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);
>     > +
>     > +/*
>     > + * MMIO debugfs IOCTL structure
>     > + */
>     > +struct amdgpu_debugfs_regs2_iocdata {
>     > +     __u8 use_srbm, use_grbm, pg_lock;
>
>     You should consider using u32 here as well or add explicitly padding.
>
>     > +     struct {
>     > +             __u32 se, sh, instance;
>     > +     } grbm;
>     > +     struct {
>     > +             __u32 me, pipe, queue, vmid;
>     > +     } srbm;
>     > +};
>     > +
>     > +/*
>     > + * MMIO debugfs state data (per file* handle)
>     > + */
>     > +struct amdgpu_debugfs_regs2_data {
>     > +     struct amdgpu_device *adev;
>     > +     struct {
>     > +             struct amdgpu_debugfs_regs2_iocdata id;
>     > +             __u32 offset;
>
>     What is the offset good for here?
>
>     Regards,
>     Christian.
>
>     > +     } state;
>     > +};
>     > +
>     > +enum AMDGPU_DEBUGFS_REGS2_CMDS {
>     > +     AMDGPU_DEBUGFS_REGS2_CMD_SET_STATE=0,
>     > +};
>     > +
>     > +#define AMDGPU_DEBUGFS_REGS2_IOC_SET_STATE _IOWR(0x20,
>     AMDGPU_DEBUGFS_REGS2_CMD_SET_STATE, struct
>     amdgpu_debugfs_regs2_iocdata)
>