> -----Original Message----- > From: intel-gvt-dev [mailto:intel-gvt-dev-bounces@xxxxxxxxxxxxxxxxxxxxx] On > Behalf Of Chris Wilson > Sent: Tuesday, August 15, 2017 11:02 PM > To: Daniel Vetter <daniel@xxxxxxxx> > Cc: intel-gfx <intel-gfx@xxxxxxxxxxxxxxxxxxxxx>; intel-gvt-dev <intel-gvt- > dev@xxxxxxxxxxxxxxxxxxxxx>; Zhang, Tina <tina.zhang@xxxxxxxxx> > Subject: Re: [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@xxxxxxxxxxxxxxxxxx> > 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@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@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx > https://lists.freedesktop.org/mailman/listinfo/intel-gvt-dev _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
