diff mbox

[3/4] drm/tegra: Add SET/GET_FLAGS IOCTLs

Message ID 1402398294-10252-3-git-send-email-thierry.reding@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Thierry Reding June 10, 2014, 11:04 a.m. UTC 
From: Thierry Reding <treding@nvidia.com>

The DRM_TEGRA_GEM_SET_FLAGS IOCTL can be used to set the flags of a
buffer object after it has been allocated or imported. Flags associated
with a buffer object can be queried using the DRM_TEGRA_GEM_GET_FLAGS
IOCTL.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/gpu/drm/tegra/drm.c  | 48 ++++++++++++++++++++++++++++++++++++++++++++
 include/uapi/drm/tegra_drm.h | 21 +++++++++++++++++++
 2 files changed, 69 insertions(+)

Comments

Stéphane Marchesin June 11, 2014, 4:21 p.m. UTC | #1 
On Tue, Jun 10, 2014 at 4:04 AM, Thierry Reding
<thierry.reding@gmail.com> wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> The DRM_TEGRA_GEM_SET_FLAGS IOCTL can be used to set the flags of a
> buffer object after it has been allocated or imported. Flags associated
> with a buffer object can be queried using the DRM_TEGRA_GEM_GET_FLAGS
> IOCTL.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
>  drivers/gpu/drm/tegra/drm.c  | 48 ++++++++++++++++++++++++++++++++++++++++++++
>  include/uapi/drm/tegra_drm.h | 21 +++++++++++++++++++
>  2 files changed, 69 insertions(+)
>
> diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> index 5dca20982f3b..f292c29ef62f 100644
> --- a/drivers/gpu/drm/tegra/drm.c
> +++ b/drivers/gpu/drm/tegra/drm.c
> @@ -548,6 +548,52 @@ static int tegra_gem_get_tiling(struct drm_device *drm, void *data,
>
>         return err;
>  }
> +
> +static int tegra_gem_set_flags(struct drm_device *drm, void *data,
> +                              struct drm_file *file)
> +{
> +       struct drm_tegra_gem_set_flags *args = data;
> +       struct drm_tegra_bo *bo;
> +       unsigned long flags = 0;
> +
> +       if (args->flags & ~DRM_TEGRA_GEM_FLAGS)
> +               return -EINVAL;
> +
> +       gem = drm_gem_object_lookup(drm, file, args->handle);
> +       if (!gem)
> +               return -EINVAL;

Usually -ENOENT is returned for unknown objects I think?

> +
> +       bo = to_tegra_bo(gem);
> +       bo->flags = 0;
> +
> +       if (args->flags & DRM_TEGRA_GEM_BOTTOM_UP)
> +               bo->flags |= TEGRA_BO_BOTTOM_UP;
> +
> +       drm_gem_object_unreference(gem);
> +
> +       return 0;
> +}
> +
> +static int tegra_gem_get_flags(struct drm_device *drm, void *data,
> +                              struct drm_file *file)
> +{
> +       struct drm_tegra_gem_get_flags *args = data;
> +       struct drm_tegra_bo *bo;
> +
> +       gem = drm_gem_object_lookup(drm, file, args->handle);
> +       if (!gem)
> +               return -EINVAL;
> +
> +       bo = to_tegra_bo(gem);
> +       args->flags = 0;
> +
> +       if (bo->flags & TEGRA_BO_BOTTOM_UP)
> +               args->flags |= DRM_TEGRA_GEM_BOTTOM_UP;
> +
> +       drm_gem_object_unreference(gem);
> +
> +       return 0;
> +}
>  #endif
>
>  static const struct drm_ioctl_desc tegra_drm_ioctls[] = {
> @@ -564,6 +610,8 @@ static const struct drm_ioctl_desc tegra_drm_ioctls[] = {
>         DRM_IOCTL_DEF_DRV(TEGRA_GET_SYNCPT_BASE, tegra_get_syncpt_base, DRM_UNLOCKED),
>         DRM_IOCTL_DEF_DRV(TEGRA_GEM_SET_TILING, tegra_gem_set_tiling, DRM_UNLOCKED),
>         DRM_IOCTL_DEF_DRV(TEGRA_GEM_GET_TILING, tegra_gem_get_tiling, DRM_UNLOCKED),
> +       DRM_IOCTL_DEF_DRV(TEGRA_GEM_SET_FLAGS, tegra_gem_set_flags, DRM_UNLOCKED),
> +       DRM_IOCTL_DEF_DRV(TEGRA_GEM_GET_FLAGS, tegra_gem_get_flags, DRM_UNLOCKED),
>  #endif
>  };
>
> diff --git a/include/uapi/drm/tegra_drm.h b/include/uapi/drm/tegra_drm.h
> index 0829f75eb986..c15d781ecc0f 100644
> --- a/include/uapi/drm/tegra_drm.h
> +++ b/include/uapi/drm/tegra_drm.h
> @@ -150,6 +150,23 @@ struct drm_tegra_gem_get_tiling {
>         __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 {
> +       /* input */
> +       __u32 handle;
> +       /* output */
> +       __u32 flags;
> +};
> +
> +struct drm_tegra_gem_get_flags {
> +       /* input */
> +       __u32 handle;
> +       /* output */
> +       __u32 flags;
> +};
> +
>  #define DRM_TEGRA_GEM_CREATE           0x00
>  #define DRM_TEGRA_GEM_MMAP             0x01
>  #define DRM_TEGRA_SYNCPT_READ          0x02
> @@ -162,6 +179,8 @@ struct drm_tegra_gem_get_tiling {
>  #define DRM_TEGRA_GET_SYNCPT_BASE      0x09
>  #define DRM_TEGRA_GEM_SET_TILING       0x0a
>  #define DRM_TEGRA_GEM_GET_TILING       0x0b
> +#define DRM_TEGRA_GEM_SET_FLAGS                0x0c
> +#define DRM_TEGRA_GEM_GET_FLAGS                0x0d
>
>  #define DRM_IOCTL_TEGRA_GEM_CREATE DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_GEM_CREATE, struct drm_tegra_gem_create)
>  #define DRM_IOCTL_TEGRA_GEM_MMAP DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_GEM_MMAP, struct drm_tegra_gem_mmap)
> @@ -175,5 +194,7 @@ struct drm_tegra_gem_get_tiling {
>  #define DRM_IOCTL_TEGRA_GET_SYNCPT_BASE DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_GET_SYNCPT_BASE, struct drm_tegra_get_syncpt_base)
>  #define DRM_IOCTL_TEGRA_GEM_SET_TILING DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_GEM_SET_TILING, struct drm_tegra_gem_set_tiling)
>  #define DRM_IOCTL_TEGRA_GEM_GET_TILING DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_GEM_GET_TILING, struct drm_tegra_gem_get_tiling)
> +#define DRM_IOCTL_TEGRA_GEM_SET_FLAGS DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_GEM_SET_FLAGS, struct drm_tegra_gem_set_flags)
> +#define DRM_IOCTL_TEGRA_GEM_GET_FLAGS DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_GEM_GET_FLAGS, struct drm_tegra_gem_get_flags)
>
>  #endif
> --
> 1.9.2
>
Thierry Reding June 12, 2014, 7:18 a.m. UTC | #2 
On Wed, Jun 11, 2014 at 09:21:21AM -0700, Stéphane Marchesin wrote:
> On Tue, Jun 10, 2014 at 4:04 AM, Thierry Reding
> <thierry.reding@gmail.com> wrote:
> > From: Thierry Reding <treding@nvidia.com>
> >
> > The DRM_TEGRA_GEM_SET_FLAGS IOCTL can be used to set the flags of a
> > buffer object after it has been allocated or imported. Flags associated
> > with a buffer object can be queried using the DRM_TEGRA_GEM_GET_FLAGS
> > IOCTL.
> >
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> >  drivers/gpu/drm/tegra/drm.c  | 48 ++++++++++++++++++++++++++++++++++++++++++++
> >  include/uapi/drm/tegra_drm.h | 21 +++++++++++++++++++
> >  2 files changed, 69 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> > index 5dca20982f3b..f292c29ef62f 100644
> > --- a/drivers/gpu/drm/tegra/drm.c
> > +++ b/drivers/gpu/drm/tegra/drm.c
> > @@ -548,6 +548,52 @@ static int tegra_gem_get_tiling(struct drm_device *drm, void *data,
> >
> >         return err;
> >  }
> > +
> > +static int tegra_gem_set_flags(struct drm_device *drm, void *data,
> > +                              struct drm_file *file)
> > +{
> > +       struct drm_tegra_gem_set_flags *args = data;
> > +       struct drm_tegra_bo *bo;
> > +       unsigned long flags = 0;
> > +
> > +       if (args->flags & ~DRM_TEGRA_GEM_FLAGS)
> > +               return -EINVAL;
> > +
> > +       gem = drm_gem_object_lookup(drm, file, args->handle);
> > +       if (!gem)
> > +               return -EINVAL;
> 
> Usually -ENOENT is returned for unknown objects I think?

We've had that discussion on IRC a little while back. I came across a
few places where this was returned to userspace, and since I remember
a particular email[0] about this error code so well I thought it should
be discussed, but I can't remember the outcome (if any). Quoting from
that email:

	ENOENT is not a valid error return from an ioctl. Never has been,
	never will be. ENOENT means "No such file and directory", and is
	for path operations. ioctl's are done on files that have already
	been opened, there's no way in hell that ENOENT would ever be
	valid.

Given that -ENOENT is used *a lot* in DRM (many, if not most, of the
instances returning the error from an IOCTL), I wonder how this can be
resolved. Given that userspace may rely on this error code and that we
can't break userspace I don't see a way out.

Thierry

[0]: https://lkml.org/lkml/2012/12/23/75
Daniel Vetter June 12, 2014, 9:28 a.m. UTC | #3 
On Thu, Jun 12, 2014 at 09:18:40AM +0200, Thierry Reding wrote:
> On Wed, Jun 11, 2014 at 09:21:21AM -0700, Stéphane Marchesin wrote:
> > On Tue, Jun 10, 2014 at 4:04 AM, Thierry Reding
> > <thierry.reding@gmail.com> wrote:
> > > From: Thierry Reding <treding@nvidia.com>
> > >
> > > The DRM_TEGRA_GEM_SET_FLAGS IOCTL can be used to set the flags of a
> > > buffer object after it has been allocated or imported. Flags associated
> > > with a buffer object can be queried using the DRM_TEGRA_GEM_GET_FLAGS
> > > IOCTL.
> > >
> > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > ---
> > >  drivers/gpu/drm/tegra/drm.c  | 48 ++++++++++++++++++++++++++++++++++++++++++++
> > >  include/uapi/drm/tegra_drm.h | 21 +++++++++++++++++++
> > >  2 files changed, 69 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> > > index 5dca20982f3b..f292c29ef62f 100644
> > > --- a/drivers/gpu/drm/tegra/drm.c
> > > +++ b/drivers/gpu/drm/tegra/drm.c
> > > @@ -548,6 +548,52 @@ static int tegra_gem_get_tiling(struct drm_device *drm, void *data,
> > >
> > >         return err;
> > >  }
> > > +
> > > +static int tegra_gem_set_flags(struct drm_device *drm, void *data,
> > > +                              struct drm_file *file)
> > > +{
> > > +       struct drm_tegra_gem_set_flags *args = data;
> > > +       struct drm_tegra_bo *bo;
> > > +       unsigned long flags = 0;
> > > +
> > > +       if (args->flags & ~DRM_TEGRA_GEM_FLAGS)
> > > +               return -EINVAL;
> > > +
> > > +       gem = drm_gem_object_lookup(drm, file, args->handle);
> > > +       if (!gem)
> > > +               return -EINVAL;
> > 
> > Usually -ENOENT is returned for unknown objects I think?
> 
> We've had that discussion on IRC a little while back. I came across a
> few places where this was returned to userspace, and since I remember
> a particular email[0] about this error code so well I thought it should
> be discussed, but I can't remember the outcome (if any). Quoting from
> that email:
> 
> 	ENOENT is not a valid error return from an ioctl. Never has been,
> 	never will be. ENOENT means "No such file and directory", and is
> 	for path operations. ioctl's are done on files that have already
> 	been opened, there's no way in hell that ENOENT would ever be
> 	valid.
> 
> Given that -ENOENT is used *a lot* in DRM (many, if not most, of the
> instances returning the error from an IOCTL), I wonder how this can be
> resolved. Given that userspace may rely on this error code and that we
> can't break userspace I don't see a way out.

Imo returning -ENOENT for lookup failures is perfectly ok, after all the
error code spelled out means "No entity" or so. So might as well extend
the meaning. Without slight bending of error codes the only thing we'd
ever be able to return is -EINVAL for ioctls, which is completely
pointless.

So my approach is to throw the vfs experts opinion into the wind and
continue with the well-established rules we have in drm.
-Daniel
Thierry Reding June 12, 2014, 11:02 a.m. UTC | #4 
On Thu, Jun 12, 2014 at 11:28:43AM +0200, Daniel Vetter wrote:
> On Thu, Jun 12, 2014 at 09:18:40AM +0200, Thierry Reding wrote:
> > On Wed, Jun 11, 2014 at 09:21:21AM -0700, Stéphane Marchesin wrote:
> > > On Tue, Jun 10, 2014 at 4:04 AM, Thierry Reding
> > > <thierry.reding@gmail.com> wrote:
> > > > From: Thierry Reding <treding@nvidia.com>
> > > >
> > > > The DRM_TEGRA_GEM_SET_FLAGS IOCTL can be used to set the flags of a
> > > > buffer object after it has been allocated or imported. Flags associated
> > > > with a buffer object can be queried using the DRM_TEGRA_GEM_GET_FLAGS
> > > > IOCTL.
> > > >
> > > > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > > > ---
> > > >  drivers/gpu/drm/tegra/drm.c  | 48 ++++++++++++++++++++++++++++++++++++++++++++
> > > >  include/uapi/drm/tegra_drm.h | 21 +++++++++++++++++++
> > > >  2 files changed, 69 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
> > > > index 5dca20982f3b..f292c29ef62f 100644
> > > > --- a/drivers/gpu/drm/tegra/drm.c
> > > > +++ b/drivers/gpu/drm/tegra/drm.c
> > > > @@ -548,6 +548,52 @@ static int tegra_gem_get_tiling(struct drm_device *drm, void *data,
> > > >
> > > >         return err;
> > > >  }
> > > > +
> > > > +static int tegra_gem_set_flags(struct drm_device *drm, void *data,
> > > > +                              struct drm_file *file)
> > > > +{
> > > > +       struct drm_tegra_gem_set_flags *args = data;
> > > > +       struct drm_tegra_bo *bo;
> > > > +       unsigned long flags = 0;
> > > > +
> > > > +       if (args->flags & ~DRM_TEGRA_GEM_FLAGS)
> > > > +               return -EINVAL;
> > > > +
> > > > +       gem = drm_gem_object_lookup(drm, file, args->handle);
> > > > +       if (!gem)
> > > > +               return -EINVAL;
> > > 
> > > Usually -ENOENT is returned for unknown objects I think?
> > 
> > We've had that discussion on IRC a little while back. I came across a
> > few places where this was returned to userspace, and since I remember
> > a particular email[0] about this error code so well I thought it should
> > be discussed, but I can't remember the outcome (if any). Quoting from
> > that email:
> > 
> > 	ENOENT is not a valid error return from an ioctl. Never has been,
> > 	never will be. ENOENT means "No such file and directory", and is
> > 	for path operations. ioctl's are done on files that have already
> > 	been opened, there's no way in hell that ENOENT would ever be
> > 	valid.
> > 
> > Given that -ENOENT is used *a lot* in DRM (many, if not most, of the
> > instances returning the error from an IOCTL), I wonder how this can be
> > resolved. Given that userspace may rely on this error code and that we
> > can't break userspace I don't see a way out.
> 
> Imo returning -ENOENT for lookup failures is perfectly ok, after all the
> error code spelled out means "No entity" or so. So might as well extend
> the meaning. Without slight bending of error codes the only thing we'd
> ever be able to return is -EINVAL for ioctls, which is completely
> pointless.

I disagree. -EINVAL makes perfect sense because one of the arguments
passed into the IOCTL (the handle) is invalid.

> So my approach is to throw the vfs experts opinion into the wind and
> continue with the well-established rules we have in drm.

The quoted comments was from Linus himself and I didn't feel like being
shouted at. Given his above explanation it doesn't seem like the thing
that he'd consider subsystem specific. But then again, ENOENT usage has
gone unnoticed in DRM for some time, so maybe we'll get away with it.

I'll update the patch to make it consistent with the rest of DRM.

Thierry
diff mbox

Patch

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index 5dca20982f3b..f292c29ef62f 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -548,6 +548,52 @@  static int tegra_gem_get_tiling(struct drm_device *drm, void *data,
 
 	return err;
 }
+
+static int tegra_gem_set_flags(struct drm_device *drm, void *data,
+			       struct drm_file *file)
+{
+	struct drm_tegra_gem_set_flags *args = data;
+	struct drm_tegra_bo *bo;
+	unsigned long flags = 0;
+
+	if (args->flags & ~DRM_TEGRA_GEM_FLAGS)
+		return -EINVAL;
+
+	gem = drm_gem_object_lookup(drm, file, args->handle);
+	if (!gem)
+		return -EINVAL;
+
+	bo = to_tegra_bo(gem);
+	bo->flags = 0;
+
+	if (args->flags & DRM_TEGRA_GEM_BOTTOM_UP)
+		bo->flags |= TEGRA_BO_BOTTOM_UP;
+
+	drm_gem_object_unreference(gem);
+
+	return 0;
+}
+
+static int tegra_gem_get_flags(struct drm_device *drm, void *data,
+			       struct drm_file *file)
+{
+	struct drm_tegra_gem_get_flags *args = data;
+	struct drm_tegra_bo *bo;
+
+	gem = drm_gem_object_lookup(drm, file, args->handle);
+	if (!gem)
+		return -EINVAL;
+
+	bo = to_tegra_bo(gem);
+	args->flags = 0;
+
+	if (bo->flags & TEGRA_BO_BOTTOM_UP)
+		args->flags |= DRM_TEGRA_GEM_BOTTOM_UP;
+
+	drm_gem_object_unreference(gem);
+
+	return 0;
+}
 #endif
 
 static const struct drm_ioctl_desc tegra_drm_ioctls[] = {
@@ -564,6 +610,8 @@  static const struct drm_ioctl_desc tegra_drm_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(TEGRA_GET_SYNCPT_BASE, tegra_get_syncpt_base, DRM_UNLOCKED),
 	DRM_IOCTL_DEF_DRV(TEGRA_GEM_SET_TILING, tegra_gem_set_tiling, DRM_UNLOCKED),
 	DRM_IOCTL_DEF_DRV(TEGRA_GEM_GET_TILING, tegra_gem_get_tiling, DRM_UNLOCKED),
+	DRM_IOCTL_DEF_DRV(TEGRA_GEM_SET_FLAGS, tegra_gem_set_flags, DRM_UNLOCKED),
+	DRM_IOCTL_DEF_DRV(TEGRA_GEM_GET_FLAGS, tegra_gem_get_flags, DRM_UNLOCKED),
 #endif
 };
 
diff --git a/include/uapi/drm/tegra_drm.h b/include/uapi/drm/tegra_drm.h
index 0829f75eb986..c15d781ecc0f 100644
--- a/include/uapi/drm/tegra_drm.h
+++ b/include/uapi/drm/tegra_drm.h
@@ -150,6 +150,23 @@  struct drm_tegra_gem_get_tiling {
 	__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 {
+	/* input */
+	__u32 handle;
+	/* output */
+	__u32 flags;
+};
+
+struct drm_tegra_gem_get_flags {
+	/* input */
+	__u32 handle;
+	/* output */
+	__u32 flags;
+};
+
 #define DRM_TEGRA_GEM_CREATE		0x00
 #define DRM_TEGRA_GEM_MMAP		0x01
 #define DRM_TEGRA_SYNCPT_READ		0x02
@@ -162,6 +179,8 @@  struct drm_tegra_gem_get_tiling {
 #define DRM_TEGRA_GET_SYNCPT_BASE	0x09
 #define DRM_TEGRA_GEM_SET_TILING	0x0a
 #define DRM_TEGRA_GEM_GET_TILING	0x0b
+#define DRM_TEGRA_GEM_SET_FLAGS		0x0c
+#define DRM_TEGRA_GEM_GET_FLAGS		0x0d
 
 #define DRM_IOCTL_TEGRA_GEM_CREATE DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_GEM_CREATE, struct drm_tegra_gem_create)
 #define DRM_IOCTL_TEGRA_GEM_MMAP DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_GEM_MMAP, struct drm_tegra_gem_mmap)
@@ -175,5 +194,7 @@  struct drm_tegra_gem_get_tiling {
 #define DRM_IOCTL_TEGRA_GET_SYNCPT_BASE DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_GET_SYNCPT_BASE, struct drm_tegra_get_syncpt_base)
 #define DRM_IOCTL_TEGRA_GEM_SET_TILING DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_GEM_SET_TILING, struct drm_tegra_gem_set_tiling)
 #define DRM_IOCTL_TEGRA_GEM_GET_TILING DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_GEM_GET_TILING, struct drm_tegra_gem_get_tiling)
+#define DRM_IOCTL_TEGRA_GEM_SET_FLAGS DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_GEM_SET_FLAGS, struct drm_tegra_gem_set_flags)
+#define DRM_IOCTL_TEGRA_GEM_GET_FLAGS DRM_IOWR(DRM_COMMAND_BASE + DRM_TEGRA_GEM_GET_FLAGS, struct drm_tegra_gem_get_flags)
 
 #endif