Re: [PATCH 7/7] drm/tegra: Add kerneldoc for UAPI

[Thierry Reding <thierry.reding@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 15703 bytes --]

On Fri, May 18, 2018 at 08:19:55PM +0300, Dmitry Osipenko wrote:
> On 17.05.2018 18:41, Thierry Reding wrote:
> > From: Thierry Reding <treding@nvidia.com>
> > 
> > Document the userspace ABI with kerneldoc to provide some information on
> > how to use it.
> > 
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  drivers/gpu/drm/tegra/gem.c  |   4 +-
> >  include/uapi/drm/tegra_drm.h | 480 ++++++++++++++++++++++++++++++++++-
> >  2 files changed, 468 insertions(+), 16 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/tegra/gem.c b/drivers/gpu/drm/tegra/gem.c
> > index 387ba1dfbe0d..e2987a19541d 100644
> > --- a/drivers/gpu/drm/tegra/gem.c
> > +++ b/drivers/gpu/drm/tegra/gem.c
> > @@ -291,10 +291,10 @@ struct tegra_bo *tegra_bo_create(struct drm_device *drm, size_t size,
> >  	if (err < 0)
> >  		goto release;
> >  
> > -	if (flags & DRM_TEGRA_GEM_CREATE_TILED)
> > +	if (flags & DRM_TEGRA_GEM_TILED)
> >  		bo->tiling.mode = TEGRA_BO_TILING_MODE_TILED;
> >  
> > -	if (flags & DRM_TEGRA_GEM_CREATE_BOTTOM_UP)
> > +	if (flags & DRM_TEGRA_GEM_BOTTOM_UP)
> >  		bo->flags |= TEGRA_BO_BOTTOM_UP;
> >  
> >  	return bo;
> > diff --git a/include/uapi/drm/tegra_drm.h b/include/uapi/drm/tegra_drm.h
> > index 99e15d82d1e9..86a7b1e7559d 100644
> > --- a/include/uapi/drm/tegra_drm.h
> > +++ b/include/uapi/drm/tegra_drm.h
> > @@ -29,146 +29,598 @@
> >  extern "C" {
> >  #endif
> >  
> > -#define DRM_TEGRA_GEM_CREATE_TILED     (1 << 0)
> > -#define DRM_TEGRA_GEM_CREATE_BOTTOM_UP (1 << 1)
> > +#define DRM_TEGRA_GEM_TILED		(1 << 0)
> > +#define DRM_TEGRA_GEM_BOTTOM_UP		(1 << 1)
> > +#define DRM_TEGRA_GEM_FLAGS		(DRM_TEGRA_GEM_TILED | \
> > +					 DRM_TEGRA_GEM_BOTTOM_UP)
> 
> The current userspace won't compile with the above changes, so this is the ABI
> breakage. Please keep the old DRM_TEGRA_GEM_CREATE_* DRM_TEGRA_GEM_* flags for now.

It looks like I fumbled this during a rebase and didn't catch it. I've
left the original flags in.

[...]
> > +/**
> > + * 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:
> > +	 *
> > +	 * Return location for the current syncpoint value.
> > +	 */>  	__u32 value;
> >  };
> 
> What about:
> 
> Returned value of the given syncpoint.
> 
> Could we replace "return location for the.." with just "Returned.." in other
> places too? My mind is stuttering while reading "location for the value", though
> I know that my english isn't ideal and maybe it's only me.

How about something a little more explicit, like:

	The current value of the syncpoint. Will be set by the kernel
	upon successful completion of the IOCTL.

?

> >  
> > +/**
> > + * struct drm_tegra_syncpt_incr - parameters for the increment syncpoint IOCTL
> > + */
> >  struct drm_tegra_syncpt_incr {
> > +	/**
> > +	 * @id:
> > +	 *
> > +	 * ID of the syncpoint to read the current value from.
> > +	 */
> 
> Cut-n-pasted comment. Change to:
> 
> ID of the syncpoint to increment.

Good catch. Fixed.

> >  	__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:
> > +	 *
> > +	 * Return value for the current syncpoint value.
> > +	 */
> Just:
> 
> Returned value of the syncpoint.

Will reword this similar to the above.

> >  	__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:
> > +	 *
> > +	 * Return location for the application context of this channel. 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:
> > +	 *
> > +	 * Channel-relative index of the syncpoint for which to obtain the ID.
> > +	 */
> >  	__u32 index;
> 
> Client-relative. And what about:
> 
> Clients syncpoint index for which to obtain the ID.

I'll try to find some better wording for this.

> > +
> > +	/**
> > +	 * @id:
> > +	 *
> > +	 * Return location for the ID of the given syncpoint.
> > +	 */
> >  	__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:
> > +	 *
> > +	 * Return location for the ID of the wait base.
> > +	 */
> >  	__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 into the command buffer at which to insert the the
> 
> Offset, in bytes,

Done.

> > +		 * relocated address.
> > +		 */
> >  		__u32 offset;
> >  	} cmdbuf;
> >  	struct {
> > +		/**
> > +		 * @target.handle:
> > +		 *
> > +		 * Handle to the GEM object to be relocated.
> > +		 */
> >  		__u32 handle;
> > +
> > +		/**
> > +		 * @target.offset:
> > +		 *
> > +		 * Offset into the target GEM object at which the relocated
> 
> Offset, in bytes,

Done.

> > +		 * 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_legacy
> 
> We don't have drm_tegra_cmdbuf_legacy yet, should be drm_tegra_cmdbuf.

Done.

> > +	 * 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_legacy structures
> > +	 * that specify the relocations that need to be performed before
> > +	 * executing this job.
> > +	 */
> 
> Same as for drm_tegra_cmdbuf_legacy.

Done.

> >  	__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:
> > +	 *
> > +	 * Return location for the threshold of the syncpoint associated with
> > +	 * this job. 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:
> > +	 *
> > +	 * Return location for the tiling mode.
> > +	 */
> >  	__u32 mode;
> > +
> > +	/**
> > +	 * @value:
> > +	 *
> > +	 * Return location for the tiling mode parameter.
> > +	 */
> >  	__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:
> > +	 *
> > +	 * Return location for the flags queried from the GEM object.
> > +	 */
> >  	__u32 flags;
> >  };
> >  

Thanks for the review. I'm going to think about a better wording for
some of the descriptions and send out a new version of this patch
individually, for review.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel