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

[Zhang, Tina]

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

BR,
Tina

> -Chris
> _______________________________________________
> intel-gvt-dev mailing list
> intel-gvt-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev