[v2] drm/tegra: Add kerneldoc for UAPI

Message ID	20180518203337.26476-1-thierry.reding@gmail.com
State	Deferred
Headers	show Return-Path: <linux-tegra-owner@vger.kernel.org> From: Thierry Reding <thierry.reding@gmail.com> To: Thierry Reding <thierry.reding@gmail.com> Cc: Dmitry Osipenko <digetx@gmail.com>, Mikko Perttunen <mperttunen@nvidia.com>, dri-devel@lists.freedesktop.org, linux-tegra@vger.kernel.org Subject: [PATCH v2] drm/tegra: Add kerneldoc for UAPI Date: Fri, 18 May 2018 22:33:37 +0200 Message-Id: <20180518203337.26476-1-thierry.reding@gmail.com> In-Reply-To: <20180517154132.10058-8-thierry.reding@gmail.com> References: <20180517154132.10058-8-thierry.reding@gmail.com> Sender: linux-tegra-owner@vger.kernel.org Precedence: bulk
Series	[v2] drm/tegra: Add kerneldoc for UAPI \| expand [v2] drm/tegra: Add kerneldoc for UAPI

Thierry Reding May 18, 2018, 8:33 p.m. UTC

From: Thierry Reding <treding@nvidia.com>

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 <treding@nvidia.com>
---
 include/uapi/drm/tegra_drm.h | 480 ++++++++++++++++++++++++++++++++++-
 1 file changed, 471 insertions(+), 9 deletions(-)

Dmitry Osipenko May 18, 2018, 9:42 p.m. UTC | #1

On 18.05.2018 23:33, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> 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 <treding@nvidia.com>
> ---
>  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.


The rest looks good to be, well done! And with the above nit being resolved:

Reviewed-by: Dmitry Osipenko <digetx@gmail.com>

> +	 * specify the syncpoint operations performed as part of this job.
> +	 */
>  	__u64 syncpts;
> +
> +	/**
> +	 * @cmdbufs:
> +	 *
> +	 * A pointer to @num_cmdbufs &struct drm_tegra_cmdbuf structures that
> +	 * define the command buffers to execute as part of this job.
> +	 */
>  	__u64 cmdbufs;
> +
> +	/**
> +	 * @relocs:
> +	 *
> +	 * A pointer to @num_relocs &struct drm_tegra_reloc structures that
> +	 * specify the relocations that need to be performed before executing
> +	 * this job.
> +	 */
>  	__u64 relocs;
> +
> +	/**
> +	 * @waitchks:
> +	 *
> +	 * A pointer to @num_waitchks &struct drm_tegra_waitchk structures
> +	 * that specify the wait checks to be performed while executing this
> +	 * job.
> +	 */
>  	__u64 waitchks;
> -	__u32 fence;		/* Return value */
>  
> -	__u32 reserved[5];	/* future expansion */
> +	/**
> +	 * @fence:
> +	 *
> +	 * The threshold of the syncpoint associated with this job after it
> +	 * has been completed. Set by the kernel upon successful completion of
> +	 * the IOCTL. This can be used with the DRM_TEGRA_SYNCPT_WAIT IOCTL to
> +	 * wait for this job to be finished.
> +	 */
> +	__u32 fence;
> +
> +	/**
> +	 * @reserved:
> +	 *
> +	 * This field is reserved for future use. Must be 0.
> +	 */
> +	__u32 reserved[5];
>  };
>  
>  #define DRM_TEGRA_GEM_TILING_MODE_PITCH 0
>  #define DRM_TEGRA_GEM_TILING_MODE_TILED 1
>  #define DRM_TEGRA_GEM_TILING_MODE_BLOCK 2
>  
> +/**
> + * struct drm_tegra_gem_set_tiling - parameters for the set tiling IOCTL
> + */
>  struct drm_tegra_gem_set_tiling {
> -	/* input */
> +	/**
> +	 * @handle:
> +	 *
> +	 * Handle to the GEM object for which to set the tiling parameters.
> +	 */
>  	__u32 handle;
> +
> +	/**
> +	 * @mode:
> +	 *
> +	 * The tiling mode to set. Must be one of:
> +	 *
> +	 * DRM_TEGRA_GEM_TILING_MODE_PITCH
> +	 *   pitch linear format
> +	 *
> +	 * DRM_TEGRA_GEM_TILING_MODE_TILED
> +	 *   16x16 tiling format
> +	 *
> +	 * DRM_TEGRA_GEM_TILING_MODE_BLOCK
> +	 *   16Bx2 tiling format
> +	 */
>  	__u32 mode;
> +
> +	/**
> +	 * @value:
> +	 *
> +	 * The value to set for the tiling mode parameter.
> +	 */
>  	__u32 value;
> +
> +	/**
> +	 * @pad:
> +	 *
> +	 * Structure padding that may be used in the future. Must be 0.
> +	 */
>  	__u32 pad;
>  };
>  
> +/**
> + * struct drm_tegra_gem_get_tiling - parameters for the get tiling IOCTL
> + */
>  struct drm_tegra_gem_get_tiling {
> -	/* input */
> +	/**
> +	 * @handle:
> +	 *
> +	 * Handle to the GEM object for which to query the tiling parameters.
> +	 */
>  	__u32 handle;
> -	/* output */
> +
> +	/**
> +	 * @mode:
> +	 *
> +	 * The tiling mode currently associated with the GEM object. Set by
> +	 * the kernel upon successful completion of the IOCTL.
> +	 */
>  	__u32 mode;
> +
> +	/**
> +	 * @value:
> +	 *
> +	 * The tiling mode parameter currently associated with the GEM object.
> +	 * Set by the kernel upon successful completion of the IOCTL.
> +	 */
>  	__u32 value;
> +
> +	/**
> +	 * @pad:
> +	 *
> +	 * Structure padding that may be used in the future. Must be 0.
> +	 */
>  	__u32 pad;
>  };
>  
>  #define DRM_TEGRA_GEM_BOTTOM_UP		(1 << 0)
>  #define DRM_TEGRA_GEM_FLAGS		(DRM_TEGRA_GEM_BOTTOM_UP)
>  
> +/**
> + * struct drm_tegra_gem_set_flags - parameters for the set flags IOCTL
> + */
>  struct drm_tegra_gem_set_flags {
> -	/* input */
> +	/**
> +	 * @handle:
> +	 *
> +	 * Handle to the GEM object for which to set the flags.
> +	 */
>  	__u32 handle;
> -	/* output */
> +
> +	/**
> +	 * @flags:
> +	 *
> +	 * The flags to set for the GEM object.
> +	 */
>  	__u32 flags;
>  };
>  
> +/**
> + * struct drm_tegra_gem_get_flags - parameters for the get flags IOCTL
> + */
>  struct drm_tegra_gem_get_flags {
> -	/* input */
> +	/**
> +	 * @handle:
> +	 *
> +	 * Handle to the GEM object for which to query the flags.
> +	 */
>  	__u32 handle;
> -	/* output */
> +
> +	/**
> +	 * @flags:
> +	 *
> +	 * The flags currently associated with the GEM object. Set by the
> +	 * kernel upon successful completion of the IOCTL.
> +	 */
>  	__u32 flags;
>  };
>  
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thierry Reding May 18, 2018, 9:58 p.m. UTC | #2

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 <treding@nvidia.com>
> > 
> > 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 <treding@nvidia.com>
> > ---
> >  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...

?

Thierry

Thierry Reding May 18, 2018, 10:13 p.m. UTC | #3

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 <treding@nvidia.com>
> > > 
> > > 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 <treding@nvidia.com>
> > > ---
> > >  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...
> 
> ?

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.
	 */

Thierry

Dmitry Osipenko May 18, 2018, 10:18 p.m. UTC | #4

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 <treding@nvidia.com>
>>>>
>>>> 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 <treding@nvidia.com>
>>>> ---
>>>>  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.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thierry Reding May 18, 2018, 10:24 p.m. UTC | #5

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 <treding@nvidia.com>
> >>>>
> >>>> 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 <treding@nvidia.com>
> >>>> ---
> >>>>  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

Dmitry Osipenko May 18, 2018, 10:28 p.m. UTC | #6

On 19.05.2018 01:24, Thierry Reding wrote:
> 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 <treding@nvidia.com>
>>>>>>
>>>>>> 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 <treding@nvidia.com>
>>>>>> ---
>>>>>>  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.

Awesome! I think Mikko was going to make a patch to the validation bug in the
submit code that he spotted recently, so maybe it would worth to postpone the
pull request a tad.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Thierry Reding May 18, 2018, 10:35 p.m. UTC | #7

On Sat, May 19, 2018 at 01:28:17AM +0300, Dmitry Osipenko wrote:
> On 19.05.2018 01:24, Thierry Reding wrote:
> > 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 <treding@nvidia.com>
> >>>>>>
> >>>>>> 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 <treding@nvidia.com>
> >>>>>> ---
> >>>>>>  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.
> 
> Awesome! I think Mikko was going to make a patch to the validation bug in the
> submit code that he spotted recently, so maybe it would worth to postpone the
> pull request a tad.

This is for the main feature pull request for v4.18-rc1, for which the
deadline is usually -rc6 of the previous version, so this is already
cutting it rather close. If Mikko has a bugfix patch that's something
that can go into a separate -fixes pull request.

Thierry

Dmitry Osipenko May 18, 2018, 10:45 p.m. UTC | #8

On 19.05.2018 01:35, Thierry Reding wrote:
> On Sat, May 19, 2018 at 01:28:17AM +0300, Dmitry Osipenko wrote:
>> On 19.05.2018 01:24, Thierry Reding wrote:
>>> 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 <treding@nvidia.com>
>>>>>>>>
>>>>>>>> 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 <treding@nvidia.com>
>>>>>>>> ---
>>>>>>>>  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.
>>
>> Awesome! I think Mikko was going to make a patch to the validation bug in the
>> submit code that he spotted recently, so maybe it would worth to postpone the
>> pull request a tad.
> 
> This is for the main feature pull request for v4.18-rc1, for which the
> deadline is usually -rc6 of the previous version, so this is already
> cutting it rather close. If Mikko has a bugfix patch that's something
> that can go into a separate -fixes pull request.

Okay.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Daniel Vetter May 23, 2018, 8:59 a.m. UTC | #9

On Fri, May 18, 2018 at 10:33:37PM +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
> 
> 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 <treding@nvidia.com>
> ---
>  include/uapi/drm/tegra_drm.h | 480 ++++++++++++++++++++++++++++++++++-

Would be great to include that somewhere in Documentation/gpu/drivers.rst
under a tegra chapter. Otherwise 0day won't check your docs at all
unfortunately.
-Daniel

>  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
> +	 * specify the syncpoint operations performed as part of this job.
> +	 */
>  	__u64 syncpts;
> +
> +	/**
> +	 * @cmdbufs:
> +	 *
> +	 * A pointer to @num_cmdbufs &struct drm_tegra_cmdbuf structures that
> +	 * define the command buffers to execute as part of this job.
> +	 */
>  	__u64 cmdbufs;
> +
> +	/**
> +	 * @relocs:
> +	 *
> +	 * A pointer to @num_relocs &struct drm_tegra_reloc structures that
> +	 * specify the relocations that need to be performed before executing
> +	 * this job.
> +	 */
>  	__u64 relocs;
> +
> +	/**
> +	 * @waitchks:
> +	 *
> +	 * A pointer to @num_waitchks &struct drm_tegra_waitchk structures
> +	 * that specify the wait checks to be performed while executing this
> +	 * job.
> +	 */
>  	__u64 waitchks;
> -	__u32 fence;		/* Return value */
>  
> -	__u32 reserved[5];	/* future expansion */
> +	/**
> +	 * @fence:
> +	 *
> +	 * The threshold of the syncpoint associated with this job after it
> +	 * has been completed. Set by the kernel upon successful completion of
> +	 * the IOCTL. This can be used with the DRM_TEGRA_SYNCPT_WAIT IOCTL to
> +	 * wait for this job to be finished.
> +	 */
> +	__u32 fence;
> +
> +	/**
> +	 * @reserved:
> +	 *
> +	 * This field is reserved for future use. Must be 0.
> +	 */
> +	__u32 reserved[5];
>  };
>  
>  #define DRM_TEGRA_GEM_TILING_MODE_PITCH 0
>  #define DRM_TEGRA_GEM_TILING_MODE_TILED 1
>  #define DRM_TEGRA_GEM_TILING_MODE_BLOCK 2
>  
> +/**
> + * struct drm_tegra_gem_set_tiling - parameters for the set tiling IOCTL
> + */
>  struct drm_tegra_gem_set_tiling {
> -	/* input */
> +	/**
> +	 * @handle:
> +	 *
> +	 * Handle to the GEM object for which to set the tiling parameters.
> +	 */
>  	__u32 handle;
> +
> +	/**
> +	 * @mode:
> +	 *
> +	 * The tiling mode to set. Must be one of:
> +	 *
> +	 * DRM_TEGRA_GEM_TILING_MODE_PITCH
> +	 *   pitch linear format
> +	 *
> +	 * DRM_TEGRA_GEM_TILING_MODE_TILED
> +	 *   16x16 tiling format
> +	 *
> +	 * DRM_TEGRA_GEM_TILING_MODE_BLOCK
> +	 *   16Bx2 tiling format
> +	 */
>  	__u32 mode;
> +
> +	/**
> +	 * @value:
> +	 *
> +	 * The value to set for the tiling mode parameter.
> +	 */
>  	__u32 value;
> +
> +	/**
> +	 * @pad:
> +	 *
> +	 * Structure padding that may be used in the future. Must be 0.
> +	 */
>  	__u32 pad;
>  };
>  
> +/**
> + * struct drm_tegra_gem_get_tiling - parameters for the get tiling IOCTL
> + */
>  struct drm_tegra_gem_get_tiling {
> -	/* input */
> +	/**
> +	 * @handle:
> +	 *
> +	 * Handle to the GEM object for which to query the tiling parameters.
> +	 */
>  	__u32 handle;
> -	/* output */
> +
> +	/**
> +	 * @mode:
> +	 *
> +	 * The tiling mode currently associated with the GEM object. Set by
> +	 * the kernel upon successful completion of the IOCTL.
> +	 */
>  	__u32 mode;
> +
> +	/**
> +	 * @value:
> +	 *
> +	 * The tiling mode parameter currently associated with the GEM object.
> +	 * Set by the kernel upon successful completion of the IOCTL.
> +	 */
>  	__u32 value;
> +
> +	/**
> +	 * @pad:
> +	 *
> +	 * Structure padding that may be used in the future. Must be 0.
> +	 */
>  	__u32 pad;
>  };
>  
>  #define DRM_TEGRA_GEM_BOTTOM_UP		(1 << 0)
>  #define DRM_TEGRA_GEM_FLAGS		(DRM_TEGRA_GEM_BOTTOM_UP)
>  
> +/**
> + * struct drm_tegra_gem_set_flags - parameters for the set flags IOCTL
> + */
>  struct drm_tegra_gem_set_flags {
> -	/* input */
> +	/**
> +	 * @handle:
> +	 *
> +	 * Handle to the GEM object for which to set the flags.
> +	 */
>  	__u32 handle;
> -	/* output */
> +
> +	/**
> +	 * @flags:
> +	 *
> +	 * The flags to set for the GEM object.
> +	 */
>  	__u32 flags;
>  };
>  
> +/**
> + * struct drm_tegra_gem_get_flags - parameters for the get flags IOCTL
> + */
>  struct drm_tegra_gem_get_flags {
> -	/* input */
> +	/**
> +	 * @handle:
> +	 *
> +	 * Handle to the GEM object for which to query the flags.
> +	 */
>  	__u32 handle;
> -	/* output */
> +
> +	/**
> +	 * @flags:
> +	 *
> +	 * The flags currently associated with the GEM object. Set by the
> +	 * kernel upon successful completion of the IOCTL.
> +	 */
>  	__u32 flags;
>  };
>  
> -- 
> 2.17.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

[v2] drm/tegra: Add kerneldoc for UAPI

Commit Message

Comments

Patch