On Wed, Aug 16, 2017 at 03:26:43AM +0000, Zhang, Tina wrote: > > > > -----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. +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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
