On Sat, May 19, 2018 at 01:18:05AM +0300, Dmitry Osipenko wrote: > On 19.05.2018 01:13, Thierry Reding wrote: > > On Fri, May 18, 2018 at 11:58:19PM +0200, Thierry Reding wrote: > >> On Sat, May 19, 2018 at 12:42:22AM +0300, Dmitry Osipenko wrote: > >>> On 18.05.2018 23:33, Thierry Reding wrote: > >>>> From: Thierry Reding > >>>> > >>>> Document the userspace ABI with kerneldoc to provide some information on > >>>> how to use it. > >>>> > >>>> v2: > >>>> - keep GEM object creation flags for ABI compatibility > >>>> - fix typo in struct drm_tegra_syncpt_incr kerneldoc > >>>> - fix typos in struct drm_tegra_submit kerneldoc > >>>> - reworded some descriptions as suggested > >>>> > >>>> Signed-off-by: Thierry Reding > >>>> --- > >>>> include/uapi/drm/tegra_drm.h | 480 ++++++++++++++++++++++++++++++++++- > >>>> 1 file changed, 471 insertions(+), 9 deletions(-) > >>>> > >>>> diff --git a/include/uapi/drm/tegra_drm.h b/include/uapi/drm/tegra_drm.h > >>>> index 99e15d82d1e9..7e121c69cd9a 100644 > >>>> --- a/include/uapi/drm/tegra_drm.h > >>>> +++ b/include/uapi/drm/tegra_drm.h > >>>> @@ -32,143 +32,605 @@ extern "C" { > >>>> #define DRM_TEGRA_GEM_CREATE_TILED (1 << 0) > >>>> #define DRM_TEGRA_GEM_CREATE_BOTTOM_UP (1 << 1) > >>>> > >>>> +/** > >>>> + * struct drm_tegra_gem_create - parameters for the GEM object creation IOCTL > >>>> + */ > >>>> struct drm_tegra_gem_create { > >>>> + /** > >>>> + * @size: > >>>> + * > >>>> + * The size, in bytes, of the buffer object to be created. > >>>> + */ > >>>> __u64 size; > >>>> + > >>>> + /** > >>>> + * @flags: > >>>> + * > >>>> + * A bitmask of flags that influence the creation of GEM objects: > >>>> + * > >>>> + * DRM_TEGRA_GEM_CREATE_TILED > >>>> + * Use the 16x16 tiling format for this buffer. > >>>> + * > >>>> + * DRM_TEGRA_GEM_CREATE_BOTTOM_UP > >>>> + * The buffer has a bottom-up layout. > >>>> + */ > >>>> __u32 flags; > >>>> + > >>>> + /** > >>>> + * @handle: > >>>> + * > >>>> + * The handle of the created GEM object. Set by the kernel upon > >>>> + * successful completion of the IOCTL. > >>>> + */ > >>>> __u32 handle; > >>>> }; > >>>> > >>>> +/** > >>>> + * struct drm_tegra_gem_mmap - parameters for the GEM mmap IOCTL > >>>> + */ > >>>> struct drm_tegra_gem_mmap { > >>>> + /** > >>>> + * @handle: > >>>> + * > >>>> + * Handle of the GEM object to obtain an mmap offset for. > >>>> + */ > >>>> __u32 handle; > >>>> + > >>>> + /** > >>>> + * @pad: > >>>> + * > >>>> + * Structure padding that may be used in the future. Must be 0. > >>>> + */ > >>>> __u32 pad; > >>>> + > >>>> + /** > >>>> + * @offset: > >>>> + * > >>>> + * The mmap offset for the given GEM object. Set by the kernel upon > >>>> + * successful completion of the IOCTL. > >>>> + */ > >>>> __u64 offset; > >>>> }; > >>>> > >>>> +/** > >>>> + * struct drm_tegra_syncpt_read - parameters for the read syncpoint IOCTL > >>>> + */ > >>>> struct drm_tegra_syncpt_read { > >>>> + /** > >>>> + * @id: > >>>> + * > >>>> + * ID of the syncpoint to read the current value from. > >>>> + */ > >>>> __u32 id; > >>>> + > >>>> + /** > >>>> + * @value: > >>>> + * > >>>> + * The current syncpoint value. Set by the kernel upon successful > >>>> + * completion of the IOCTL. > >>>> + */ > >>>> __u32 value; > >>>> }; > >>>> > >>>> +/** > >>>> + * struct drm_tegra_syncpt_incr - parameters for the increment syncpoint IOCTL > >>>> + */ > >>>> struct drm_tegra_syncpt_incr { > >>>> + /** > >>>> + * @id: > >>>> + * > >>>> + * ID of the syncpoint to increment. > >>>> + */ > >>>> __u32 id; > >>>> + > >>>> + /** > >>>> + * @pad: > >>>> + * > >>>> + * Structure padding that may be used in the future. Must be 0. > >>>> + */ > >>>> __u32 pad; > >>>> }; > >>>> > >>>> +/** > >>>> + * struct drm_tegra_syncpt_wait - parameters for the wait syncpoint IOCTL > >>>> + */ > >>>> struct drm_tegra_syncpt_wait { > >>>> + /** > >>>> + * @id: > >>>> + * > >>>> + * ID of the syncpoint to wait on. > >>>> + */ > >>>> __u32 id; > >>>> + > >>>> + /** > >>>> + * @thresh: > >>>> + * > >>>> + * Threshold value for which to wait. > >>>> + */ > >>>> __u32 thresh; > >>>> + > >>>> + /** > >>>> + * @timeout: > >>>> + * > >>>> + * Timeout, in milliseconds, to wait. > >>>> + */ > >>>> __u32 timeout; > >>>> + > >>>> + /** > >>>> + * @value: > >>>> + * > >>>> + * The new syncpoint value after the wait. Set by the kernel upon > >>>> + * successful completion of the IOCTL. > >>>> + */ > >>>> __u32 value; > >>>> }; > >>>> > >>>> #define DRM_TEGRA_NO_TIMEOUT (0xffffffff) > >>>> > >>>> +/** > >>>> + * struct drm_tegra_open_channel - parameters for the open channel IOCTL > >>>> + */ > >>>> struct drm_tegra_open_channel { > >>>> + /** > >>>> + * @client: > >>>> + * > >>>> + * The client ID for this channel. > >>>> + */ > >>>> __u32 client; > >>>> + > >>>> + /** > >>>> + * @pad: > >>>> + * > >>>> + * Structure padding that may be used in the future. Must be 0. > >>>> + */ > >>>> __u32 pad; > >>>> + > >>>> + /** > >>>> + * @context: > >>>> + * > >>>> + * The application context of this channel. Set by the kernel upon > >>>> + * successful completion of the IOCTL. This context needs to be passed > >>>> + * to the DRM_TEGRA_CHANNEL_CLOSE or the DRM_TEGRA_SUBMIT IOCTLs. > >>>> + */ > >>>> __u64 context; > >>>> }; > >>>> > >>>> +/** > >>>> + * struct drm_tegra_close_channel - parameters for the close channel IOCTL > >>>> + */ > >>>> struct drm_tegra_close_channel { > >>>> + /** > >>>> + * @context: > >>>> + * > >>>> + * The application context of this channel. This is obtained from the > >>>> + * DRM_TEGRA_OPEN_CHANNEL IOCTL. > >>>> + */ > >>>> __u64 context; > >>>> }; > >>>> > >>>> +/** > >>>> + * struct drm_tegra_get_syncpt - parameters for the get syncpoint IOCTL > >>>> + */ > >>>> struct drm_tegra_get_syncpt { > >>>> + /** > >>>> + * @context: > >>>> + * > >>>> + * The application context identifying the channel for which to obtain > >>>> + * the syncpoint ID. > >>>> + */ > >>>> __u64 context; > >>>> + > >>>> + /** > >>>> + * @index: > >>>> + * > >>>> + * Index of the client syncpoint for which to obtain the ID. > >>>> + */ > >>>> __u32 index; > >>>> + > >>>> + /** > >>>> + * @id: > >>>> + * > >>>> + * The ID of the given syncpoint. Set by the kernel upon successful > >>>> + * completion of the IOCTL. > >>>> + */ > >>>> __u32 id; > >>>> }; > >>>> > >>>> +/** > >>>> + * struct drm_tegra_get_syncpt_base - parameters for the get wait base IOCTL > >>>> + */ > >>>> struct drm_tegra_get_syncpt_base { > >>>> + /** > >>>> + * @context: > >>>> + * > >>>> + * The application context identifying for which channel to obtain the > >>>> + * wait base. > >>>> + */ > >>>> __u64 context; > >>>> + > >>>> + /** > >>>> + * @syncpt: > >>>> + * > >>>> + * ID of the syncpoint for which to obtain the wait base. > >>>> + */ > >>>> __u32 syncpt; > >>>> + > >>>> + /** > >>>> + * @id: > >>>> + * > >>>> + * The ID of the wait base corresponding to the client syncpoint. Set > >>>> + * by the kernel upon successful completion of the IOCTL. > >>>> + */ > >>>> __u32 id; > >>>> }; > >>>> > >>>> +/** > >>>> + * struct drm_tegra_syncpt - syncpoint increment operation > >>>> + */ > >>>> struct drm_tegra_syncpt { > >>>> + /** > >>>> + * @id: > >>>> + * > >>>> + * ID of the syncpoint to operate on. > >>>> + */ > >>>> __u32 id; > >>>> + > >>>> + /** > >>>> + * @incrs: > >>>> + * > >>>> + * Number of increments to perform for the syncpoint. > >>>> + */ > >>>> __u32 incrs; > >>>> }; > >>>> > >>>> +/** > >>>> + * struct drm_tegra_cmdbuf - structure describing a command buffer > >>>> + */ > >>>> struct drm_tegra_cmdbuf { > >>>> + /** > >>>> + * @handle: > >>>> + * > >>>> + * Handle to a GEM object containing the command buffer. > >>>> + */ > >>>> __u32 handle; > >>>> + > >>>> + /** > >>>> + * @offset: > >>>> + * > >>>> + * Offset, in bytes, into the GEM object identified by @handle at > >>>> + * which the command buffer starts. > >>>> + */ > >>>> __u32 offset; > >>>> + > >>>> + /** > >>>> + * @words: > >>>> + * > >>>> + * Number of 32-bit words in this command buffer. > >>>> + */ > >>>> __u32 words; > >>>> + > >>>> + /** > >>>> + * @pad: > >>>> + * > >>>> + * Structure padding that may be used in the future. Must be 0. > >>>> + */ > >>>> __u32 pad; > >>>> }; > >>>> > >>>> +/** > >>>> + * struct drm_tegra_reloc - GEM object relocation structure > >>>> + */ > >>>> struct drm_tegra_reloc { > >>>> struct { > >>>> + /** > >>>> + * @cmdbuf.handle: > >>>> + * > >>>> + * Handle to the GEM object containing the command buffer for > >>>> + * which to perform this GEM object relocation. > >>>> + */ > >>>> __u32 handle; > >>>> + > >>>> + /** > >>>> + * @cmdbuf.offset: > >>>> + * > >>>> + * Offset, in bytes, into the command buffer at which to > >>>> + * insert the relocated address. > >>>> + */ > >>>> __u32 offset; > >>>> } cmdbuf; > >>>> struct { > >>>> + /** > >>>> + * @target.handle: > >>>> + * > >>>> + * Handle to the GEM object to be relocated. > >>>> + */ > >>>> __u32 handle; > >>>> + > >>>> + /** > >>>> + * @target.offset: > >>>> + * > >>>> + * Offset, in bytes, into the target GEM object at which the > >>>> + * relocated data starts. > >>>> + */ > >>>> __u32 offset; > >>>> } target; > >>>> + > >>>> + /** > >>>> + * @shift: > >>>> + * > >>>> + * The number of bits by which to shift relocated addresses. > >>>> + */ > >>>> __u32 shift; > >>>> + > >>>> + /** > >>>> + * @pad: > >>>> + * > >>>> + * Structure padding that may be used in the future. Must be 0. > >>>> + */ > >>>> __u32 pad; > >>>> }; > >>>> > >>>> +/** > >>>> + * struct drm_tegra_waitchk - wait check structure > >>>> + */ > >>>> struct drm_tegra_waitchk { > >>>> + /** > >>>> + * @handle: > >>>> + * > >>>> + * Handle to the GEM object containing a command stream on which to > >>>> + * perform the wait check. > >>>> + */ > >>>> __u32 handle; > >>>> + > >>>> + /** > >>>> + * @offset: > >>>> + * > >>>> + * Offset, in bytes, of the location in the command stream to perform > >>>> + * the wait check on. > >>>> + */ > >>>> __u32 offset; > >>>> + > >>>> + /** > >>>> + * @syncpt: > >>>> + * > >>>> + * ID of the syncpoint to wait check. > >>>> + */ > >>>> __u32 syncpt; > >>>> + > >>>> + /** > >>>> + * @thresh: > >>>> + * > >>>> + * Threshold value for which to check. > >>>> + */ > >>>> __u32 thresh; > >>>> }; > >>>> > >>>> +/** > >>>> + * struct drm_tegra_submit - job submission structure > >>>> + */ > >>>> struct drm_tegra_submit { > >>>> + /** > >>>> + * @context: > >>>> + * > >>>> + * The application context identifying the channel to use for the > >>>> + * execution of this job. > >>>> + */ > >>>> __u64 context; > >>>> + > >>>> + /** > >>>> + * @num_syncpts: > >>>> + * > >>>> + * The number of syncpoints operated on by this job. > >>>> + */ > >>>> __u32 num_syncpts; > >>>> + > >>>> + /** > >>>> + * @num_cmdbufs: > >>>> + * > >>>> + * The number of command buffers to execute as part of this job. > >>>> + */ > >>>> __u32 num_cmdbufs; > >>>> + > >>>> + /** > >>>> + * @num_relocs: > >>>> + * > >>>> + * The number of relocations to perform before executing this job. > >>>> + */ > >>>> __u32 num_relocs; > >>>> + > >>>> + /** > >>>> + * @num_waitchks: > >>>> + * > >>>> + * The number of wait checks to perform as part of this job. > >>>> + */ > >>>> __u32 num_waitchks; > >>>> + > >>>> + /** > >>>> + * @waitchk_mask: > >>>> + * > >>>> + * Bitmask of valid wait checks. > >>>> + */ > >>>> __u32 waitchk_mask; > >>>> + > >>>> + /** > >>>> + * @timeout: > >>>> + * > >>>> + * Timeout, in milliseconds, before this job is cancelled. > >>>> + */ > >>>> __u32 timeout; > >>>> + > >>>> + /** > >>>> + * @syncpts: > >>>> + * > >>>> + * A pointer to @num_syncpts &struct drm_tegra_syncpt structures that > >>> > >>> I'm not sure whether this "pointer to @num_syncpts" makes sense, shouldn't it be: > >>> > >>> A pointer to &struct drm_tegra_syncpt structures that... > >>> > >>> ? > >>> > >>> Same for the @cmdbufs/@relocs/@waitchks members. > >> > >> I wanted to have the references to those fields in the text so that it > >> becomes obvious that num_syncpts defines the number of entries in this > >> syncpts array. > >> > >> Perhaps a better formulation would be: > >> > >> A pointer to an array of @num_syncpts &struct drm_tegra_syncpt > >> structure that... > >> > >> ? > > That variant is good to me. > > > > > Another alternative may be: > > > > /** > > * @syncpts: > > * > > * A pointer to an array of &struct drm_tegra_syncpt structure that > > * specify the syncpoint operations performed as part of this job. > > * The number of elements in the array must be equal to the value > > * given by @num_syncpts. > > */ > > > > That's slightly easier to read but also very explicit in relating both > > fields to one another. Perhaps a two-way link can be established by > > adding something like this to the description of @num_syncpts: > > > > /** > > * @num_syncpts: > > * > > * The number of syncpoints operated on by this job. This defines > > * the length of the array pointed to by @syncpts. > > */ > > But this variant is even better. > > I don't mind either way, choose what you prefer. I went with the latter. I've updated all of these field descriptions and added your Reviewed-by. Pushed everything to drm/tegra/for-next and will send a pull request for that branch shortly. Thierry