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@xxxxxxxxxx>
>>>>>>>>
>>>>>>>> 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@xxxxxxxxxx>
>>>>>>>> ---
>>>>>>>> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
