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

[Dmitry Osipenko <digetx@gmail.com>]

On 18.05.2018 23:12, Thierry Reding wrote:
> 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.
> 
> ?

That's better.

Maybe we could also use format like this:

/**
 * struct drm_tegra_syncpt_read - parameters for the read syncpoint IOCTL
 * @id:		ID of the syncpoint to read the current value from.
 * @value:	Return current value of the syncpoint.
 */
struct drm_tegra_syncpt_read {
	__u32 id;
	__u32 value;
};

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

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