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@xxxxxxxxxx>
>>>
>>> Document the userspace ABI with kerneldoc to provide some information on
>>> how to use it.
>>>
>>> Signed-off-by: Thierry Reding <treding@xxxxxxxxxx>
>>> ---
>>> 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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
