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@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;
> };
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
Attachment:
signature.asc
Description: PGP signature
