Messages in this thread | | | From | Arnd Bergmann <> | Date | Tue, 26 Apr 2022 12:50:50 +0200 | Subject | Re: [Linaro-mm-sig] Re: [PATCH v2 4/4] drm/nvdla/uapi: Add UAPI of NVDLA driver |
| |
On Tue, Apr 26, 2022 at 8:31 AM Christian König <christian.koenig@amd.com> wrote: > Am 26.04.22 um 08:08 schrieb Cai Huoqing: > > The NVIDIA Deep Learning Accelerator (NVDLA) is an open source IP > > which is integrated into NVIDIA Jetson AGX Xavier, > > so add UAPI of this driver. > > > > Signed-off-by: Cai Huoqing <cai.huoqing@linux.dev>
I saw the reply but no the original mail, so I'll comment here.
> > + > > +#if !defined(__KERNEL__) > > +#define __user > > +#endif
This is done in the 'make headers_install' step, no need to define it separately.
> > +#define NVDLA_NO_TIMEOUT (0xffffffff) > > + __u32 timeout; > > What format does that timeout value have? > > In general it is best practice to have absolute 64bit nanosecond > timeouts (to be used with ktime inside the kernel) so that restarting > interrupted IOCTLs works smooth.
When using absolute values, one also needs to decide whether this should be realtime, monotonic or boottime and document the decision.
> > + * struct nvdla_submit_args structure for task submit > > + * > > + * @tasks pointer to array of struct nvdla_ioctl_submit_task > > + * @num_tasks number of entries in tasks > > + * @flags flags for task submit, no flags defined yet > > + * @version version of task structure > > + * > > + */ > > +struct nvdla_submit_args { > > + __u64 tasks; > > + __u16 num_tasks; > > +#define NVDLA_MAX_TASKS_PER_SUBMIT 24 > > +#define NVDLA_SUBMIT_FLAGS_ATOMIC (1 << 0) > > Well that "no flags defined yet" from the comment above is probably > outdated :)
> > + __u16 flags; > > + __u32 version; > > +};
Versioned interfaces are usually a bad idea. If you introduce an ioctl command, it should generally keep working. If you ever need to change the interface, just use a new command number for the new version.
> > +/** > > + * struct nvdla_gem_create_args for allocating DMA buffer through GEM > > + * > > + * @handle handle updated by kernel after allocation > > + * @flags implementation specific flags > > + * @size size of buffer to allocate > > + */ > > +struct nvdla_gem_create_args { > > + __u32 handle; > > + __u32 flags; > > + __u64 size; > > +}; > > + > > +/** > > + * struct nvdla_gem_map_offset_args for mapping DMA buffer > > + * > > + * @handle handle of the buffer > > + * @reserved reserved for padding > > + * @offset offset updated by kernel after mapping > > + */ > > +struct nvdla_gem_map_offset_args { > > + __u32 handle; > > + __u32 reserved; > > + __u64 offset; > > +}; > > + > > +#define DRM_NVDLA_SUBMIT 0x00 > > +#define DRM_NVDLA_GEM_CREATE 0x01 > > +#define DRM_NVDLA_GEM_MMAP 0x02
Is this an actual mmap() call, or something that needs to be done before the mmap()? Is the 'handle' a file descriptor or some internal number?
Arnd
| |