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

[Chris Wilson]

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