[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 <treding at nvidia.com>
> >>>
> >>> Document the userspace ABI with kerneldoc to provide some information on
> >>> how to use it.
> >>>
> >>> Signed-off-by: Thierry Reding <treding at 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;
> };

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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20180519/7ea2ba42/attachment.sig>