[Intel-gfx] [PATCH] drm/i915: Return -EPERM when i915_gem_mmap_ioctl handling prime objects

[Daniel Vetter]

On Wed, Aug 16, 2017 at 03:26:43AM +0000, Zhang, Tina wrote:
> 
> 
> > -----Original Message-----
> > From: intel-gvt-dev [mailto:intel-gvt-dev-bounces at lists.freedesktop.org] On
> > Behalf Of Chris Wilson
> > Sent: Tuesday, August 15, 2017 11:02 PM
> > To: Daniel Vetter <daniel at ffwll.ch>
> > Cc: intel-gfx <intel-gfx at lists.freedesktop.org>; intel-gvt-dev <intel-gvt-
> > dev at lists.freedesktop.org>; Zhang, Tina <tina.zhang at intel.com>
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Return -EPERM when
> > i915_gem_mmap_ioctl handling prime objects
> > 
> > Quoting Daniel Vetter (2017-08-15 15:48:03)
> > > On Tue, Aug 15, 2017 at 4:35 PM, Chris Wilson <chris at chris-wilson.co.uk>
> > wrote:
> > > > Quoting Chris Wilson (2017-08-15 15:32:41)
> > > >> Quoting Daniel Vetter (2017-08-15 15:25:46)
> > > >> > On Tue, Aug 15, 2017 at 3:18 PM, Chris Wilson <chris at chris-
> > wilson.co.uk> wrote:
> > > >> > > Quoting Daniel Vetter (2017-08-15 11:49:51)
> > > >> > >> On Tue, Aug 15, 2017 at 10:42 AM, Tina Zhang
> > <tina.zhang at intel.com> wrote:
> > > >> > >> > Prime objects have no backing filp to GEM mmap pages from.
> > > >> > >> > So, for Prime objects, such operation is not permitted.
> > > >> > >>
> > > >> > >> EPERM is when you don't have enough permissions, but it's
> > > >> > >> possible (e.g. a feature requiring root, or drm master).
> > > >> > >> -EINVAL is if something is invalid, and not even root can
> > > >> > >> change that. So from a consistency pov, EINVAL is the right error code
> > here I think.
> > > >> > >
> > > >> > > Consistency is that we wanted the same error code for all
> > > >> > > objects where you did not have the ability to change the underlying
> > storage.
> > > >> > >
> > > >> > > The question is that an access issue or a permission issue. But
> > > >> > > at the very least, mmap_ioctl is the odd one out. Which the
> > > >> > > changelog did not explain and being sent out of context does not help.
> > > >> >
> > > >> > Which other ioctl give you an EPERM for something that doesn't
> > > >> > even work when you're root and/or drm master or whatever it is
> > > >> > that gives you permission? I thought we've been pretty consistent
> > > >> > with that one ...
> > > >>
> > > >> https://patchwork.freedesktop.org/series/28709/
> > > >
> > > > Oops, https://patchwork.freedesktop.org/series/27844/
> > > >
> > > >> Short story is that we add a new set of second class GEM objects
> > > >> that are not allowed to change the backing storage or details of the PTE.
> > > >>
> > > >> Not happy about the dysfunctional GEM objects, but we do want a
> > > >> clear and consistent indication as to why we start rejecting certain ioctls.
> > >
> > > I guess two questions:
> > > - Does userspace really care about the different return value?
> > 
> > I'm expecting to get a bug report as soon as someone tries to mix it with an EGL
> > image. Once we hand out dma-bufs to partial objects, they will show up
> > everywhere and randomly break. Trying to save hassle later.
> > 
> > > Or is
> > > the use case more that we have very specific userspace which knows not
> > > to do stupid things with these special gvt dma-bufs? If no, then I'd
> > > go with EINVAL + DRM_DEBUG_DRIVER.
> > 
> > DRM_DEBUG, for user error not driver and because we don't have a dedicated
> > channel for complete error messages to give to the user. :| Behind a private
> > channel we could report more details that only make sense to the caller, or may
> > simply need to be kept private. Dream on!
> > 
> > > - If we need that special errno, can we take something else? EPERM imo
> > > has fairly specific meaning. ENODEV/ENOTTY are more the "not supported
> > > on this thing" error codes, if we need a special one. They also have
> > > other meanings attached already, but then everything excpe EINVAL has
> > > when we do an ioctl, since the vfs can already throw these at you
> > > anyway.
> > 
> > ENODEV at the ioctl level we already have to mean that the device doesn't
> > support the operation, but not the object. (Then internally we've used ENODEV
> > to indicate programmer error.)
> > 
> > ENOTTY too easy to confuse with the absent ioctl?
> > 
> > ENXIO is not bad, basically says the remote channel does not support the
> > operation.
> "ENXIO" looks fine to me. If everyone is onboard with this, we will replace "EINVAL" with it
> in this patch, and also the ones in https://patchwork.freedesktop.org/patch/168810/
> Thanks.

+1 on ENXIO. I reviewed usage in drm, and mostly it's used for probe time
cases where the other endpoint wasn't found/didn't reply. It's a bit a
stretch, but seems to fit best at least.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch