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

[Dmitry Osipenko]

On 17.05.2018 18:41, Thierry Reding wrote:
> From: Thierry Reding 
> 
> Document the userspace ABI with kerneldoc to provide some information on
> how to use it.
> 
> Signed-off-by: Thierry Reding 
> ---
>  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.

>  
> +/**
> + * 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_TILED
> +  *   Use the 16x16 tiling format for this buffer.
> +  *
> +  * DRM_TEGRA_GEM_BOTTOM_UP
> +  *   The buffer has a bottom-up layout.
> +  */
>   __u32 flags;
> +
> + /**
> +  * @handle:
> +  *
> +  * Return location for the handle of the created GEM object.
> +  */
>   __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:
> +  *
> +  * Return location for the mmap offset for the given GEM object.
> +  */
>   __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:
> +  *
> +  * 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.

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

>   __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.

>   

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

[Dmitry Osipenko]

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 
>>>
>>> Document the userspace ABI with kerneldoc to provide some information on
>>> how to use it.
>>>
>>> Signed-off-by: Thierry Reding 
>>> ---
>>>  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   (0x)
>>>  
>>> +/**
>>> + * 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;
>>> +

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

[Thierry Reding]

On Sat, May 19, 2018 at 12:07:15AM +0300, Dmitry Osipenko wrote:
> 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 
> >>>
> >>> Document the userspace ABI with kerneldoc to provide some information on
> >>> how to use it.
> >>>
> >>> Signed-off-by: Thierry Reding 
> >>> ---
> >>>  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;
> };

The per-field description blocks are preferred in the DRM subsystem. I
think primarily this is to decrease the chances of anyone forgetting to
update the documentation when the code changes.

Thierry


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Thierry Reding]

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 
> > 
> > Document the userspace ABI with kerneldoc to provide some information on
> > how to use it.
> > 
> > Signed-off-by: Thierry Reding 
> > ---
> >  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   (0x)
> >  
> > +/**
> > + * 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;
> >  };
> >  
>