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

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

[Tina Zhang]

Prime objects have no backing filp to GEM mmap pages from. So, for Prime
objects, such operation is not permitted.

v1:
- Separate this patch from dma-buf patch set. (Joonas)

Signed-off-by: Tina Zhang <tina.zhang@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0628fb1..bb9d5ed 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1704,7 +1704,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
 	 */
 	if (!obj->base.filp) {
 		i915_gem_object_put(obj);
-		return -EINVAL;
+		return -EPERM;
 	}
 
 	addr = vm_mmap(obj->base.filp, 0, args->size,
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915: Return -EPERM when i915_gem_mmap_ioctl handling prime objects

[Patchwork]

== Series Details ==

Series: drm/i915: Return -EPERM when i915_gem_mmap_ioctl handling prime objects
URL   : https://patchwork.freedesktop.org/series/28784/
State : success

== Summary ==

Series 28784v1 drm/i915: Return -EPERM when i915_gem_mmap_ioctl handling prime objects
https://patchwork.freedesktop.org/api/1.0/series/28784/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                pass       -> DMESG-WARN (fi-byt-n2820) fdo#101705

fdo#101705 https://bugs.freedesktop.org/show_bug.cgi?id=101705

fi-bdw-5557u     total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:444s
fi-bdw-gvtdvm    total:279  pass:265  dwarn:0   dfail:0   fail:0   skip:14  time:440s
fi-blb-e6850     total:279  pass:224  dwarn:1   dfail:0   fail:0   skip:54  time:358s
fi-bsw-n3050     total:279  pass:243  dwarn:0   dfail:0   fail:0   skip:36  time:551s
fi-bxt-j4205     total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:513s
fi-byt-j1900     total:279  pass:254  dwarn:1   dfail:0   fail:0   skip:24  time:517s
fi-byt-n2820     total:279  pass:250  dwarn:1   dfail:0   fail:0   skip:28  time:508s
fi-glk-2a        total:279  pass:260  dwarn:0   dfail:0   fail:0   skip:19  time:603s
fi-hsw-4770      total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:435s
fi-hsw-4770r     total:279  pass:263  dwarn:0   dfail:0   fail:0   skip:16  time:417s
fi-ilk-650       total:279  pass:229  dwarn:0   dfail:0   fail:0   skip:50  time:419s
fi-ivb-3520m     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:509s
fi-ivb-3770      total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:479s
fi-kbl-7500u     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:473s
fi-kbl-7560u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:582s
fi-kbl-r         total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:589s
fi-pnv-d510      total:279  pass:223  dwarn:1   dfail:0   fail:0   skip:55  time:524s
fi-skl-6260u     total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:468s
fi-skl-6700k     total:279  pass:261  dwarn:0   dfail:0   fail:0   skip:18  time:476s
fi-skl-6770hq    total:279  pass:269  dwarn:0   dfail:0   fail:0   skip:10  time:482s
fi-skl-gvtdvm    total:279  pass:266  dwarn:0   dfail:0   fail:0   skip:13  time:448s
fi-skl-x1585l    total:279  pass:268  dwarn:0   dfail:0   fail:0   skip:11  time:482s
fi-snb-2520m     total:279  pass:251  dwarn:0   dfail:0   fail:0   skip:28  time:549s
fi-snb-2600      total:279  pass:250  dwarn:0   dfail:0   fail:0   skip:29  time:410s

919fe89189f91de8ab10a4821ca88f10df24411d drm-tip: 2017y-08m-15d-08h-06m-07s UTC integration manifest
6dc861eb8aa1 drm/i915: Return -EPERM when i915_gem_mmap_ioctl handling prime objects

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_5399/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Tue, Aug 15, 2017 at 10:42 AM, Tina Zhang <tina.zhang@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.
-Daniel

>
> v1:
> - Separate this patch from dma-buf patch set. (Joonas)
>
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 0628fb1..bb9d5ed 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1704,7 +1704,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
>          */
>         if (!obj->base.filp) {
>                 i915_gem_object_put(obj);
> -               return -EINVAL;
> +               return -EPERM;
>         }
>
>         addr = vm_mmap(obj->base.filp, 0, args->size,
> --
> 2.7.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

Quoting Daniel Vetter (2017-08-15 11:49:51)
> On Tue, Aug 15, 2017 at 10:42 AM, Tina Zhang <tina.zhang@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.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

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@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
...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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@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/

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.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

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@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.
> -Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Tue, Aug 15, 2017 at 4:35 PM, Chris Wilson <chris@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@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@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? 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.

- 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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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@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@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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

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

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

[Joonas Lahtinen]

On Fri, 2017-08-18 at 10:03 +0200, Daniel Vetter wrote:
> On Wed, Aug 16, 2017 at 03:26:43AM +0000, Zhang, Tina wrote:

<SNIP>

> > > Quoting Daniel Vetter (2017-08-15 15:48:03)
> > > > On Tue, Aug 15, 2017 at 4:35 PM, Chris Wilson <chris@chris-wilson.co.uk>

<SNIP>

> > > > - 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, I think you've just signed up for writing a nice section of
kerneldoc about this ;) Include the reasoning too, then it'll be easier
to be consistent in the future.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Joonas Lahtinen]

On Tue, 2017-08-15 at 16:42 +0800, Tina Zhang wrote:
> Prime objects have no backing filp to GEM mmap pages from. So, for Prime
> objects, such operation is not permitted.
> 
> v1:
> - Separate this patch from dma-buf patch set. (Joonas)
> 
> Signed-off-by: Tina Zhang <tina.zhang@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 0628fb1..bb9d5ed 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1704,7 +1704,7 @@ i915_gem_mmap_ioctl(struct drm_device *dev, void *data,
>  	 */
>  	if (!obj->base.filp) {
>  		i915_gem_object_put(obj);
> -		return -EINVAL;
> +		return -EPERM;

+ Daniel (in case he wants to explain in more detail)

The existing usecases in the driver for -EPERM are all conditional to
not being privileged user or DRM master, so we should rather stick to
"-EINVAL" and augment with DRM_DEBUG_DRIVER in case the developer may
want to know read more about the -EINVAL.

Lets stick to that convention here and in the GVT dma-buf patches.

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx