subject:"Re\: \[PATCH v5 20\/27\] drm\: Scope all DRM IOCTLs with drm_dev

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

2021-05-10 Thread Daniel Vetter

On Fri, May 07, 2021 at 02:00:14PM -0400, Andrey Grodzovsky wrote:
> 
> 
> On 2021-05-07 12:24 p.m., Daniel Vetter wrote:
> > On Fri, May 07, 2021 at 11:39:49AM -0400, Andrey Grodzovsky wrote:
> > > 
> > > 
> > > On 2021-05-07 5:11 a.m., Daniel Vetter wrote:
> > > > On Thu, May 06, 2021 at 12:25:06PM -0400, Andrey Grodzovsky wrote:
> > > > > 
> > > > > 
> > > > > On 2021-05-06 5:40 a.m., Daniel Vetter wrote:
> > > > > > On Fri, Apr 30, 2021 at 01:27:37PM -0400, Andrey Grodzovsky wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > On 2021-04-30 6:25 a.m., Daniel Vetter wrote:
> > > > > > > > On Thu, Apr 29, 2021 at 04:34:55PM -0400, Andrey Grodzovsky 
> > > > > > > > wrote:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > On 2021-04-29 3:05 p.m., Daniel Vetter wrote:
> > > > > > > > > > On Thu, Apr 29, 2021 at 12:04:33PM -0400, Andrey Grodzovsky 
> > > > > > > > > > wrote:
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > On 2021-04-29 7:32 a.m., Daniel Vetter wrote:
> > > > > > > > > > > > On Thu, Apr 29, 2021 at 01:23:19PM +0200, Daniel Vetter 
> > > > > > > > > > > > wrote:
> > > > > > > > > > > > > On Wed, Apr 28, 2021 at 11:12:00AM -0400, Andrey 
> > > > > > > > > > > > > Grodzovsky wrote:
> > > > > > > > > > > > > > With this calling drm_dev_unplug will flush and 
> > > > > > > > > > > > > > block
> > > > > > > > > > > > > > all in flight IOCTLs
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Also, add feature such that if device supports 
> > > > > > > > > > > > > > graceful unplug
> > > > > > > > > > > > > > we enclose entire IOCTL in SRCU critical section.
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Signed-off-by: Andrey Grodzovsky 
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Nope.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > The idea of drm_dev_enter/exit is to mark up hw 
> > > > > > > > > > > > > access. Not entire ioctl.
> > > > > > > > > > > 
> > > > > > > > > > > Then I am confused why we have 
> > > > > > > > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.12%2Fsource%2Fdrivers%2Fgpu%2Fdrm%2Fdrm_ioctl.c%23L826&data=04%7C01%7Candrey.grodzovsky%40amd.com%7C66e4988eb341427e8b0108d91174a232%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637560014906903277%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=rdb3xesAUYYTeqU2WdoZ%2BWLzOuuRdOxBBQNTMMB%2BKB4%3D&reserved=0
> > > > > > > > > > > currently in code ?
> > > > > > > > > > 
> > > > > > > > > > I forgot about this one, again. Thanks for reminding.
> > > > > > > > > > 
> > > > > > > > > > > > > Especially not with an opt-in flag so that it could 
> > > > > > > > > > > > > be shrugged of as a
> > > > > > > > > > > > > driver hack. Most of these ioctls should have 
> > > > > > > > > > > > > absolutely no problem
> > > > > > > > > > > > > working after hotunplug.
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Also, doing this defeats the point since it pretty 
> > > > > > > > > > > > > much guarantees
> > > > > > > > > > > > > userspace will die in assert()s and stuff. E.g. on 
> > > > > > > > > > > > > i915 the rough contract
> > > > > > > > > > > > > is that only execbuf (and even that only when 
> > > > > > > > > > > > > userspace has indicated
> > > > > > > > > > > > > support for non-recoverable hw ctx) is allowed to 
> > > > > > > > > > > > > fail. Anything else
> > > > > > > > > > > > > might crash userspace.
> > > > > > > > > > > 
> > > > > > > > > > > Given that as I pointed above we already fail any IOCTls 
> > > > > > > > > > > with -ENODEV
> > > > > > > > > > > when device is unplugged, it seems those crashes don't 
> > > > > > > > > > > happen that
> > > > > > > > > > > often ? Also, in all my testing I don't think I saw a 
> > > > > > > > > > > user space crash
> > > > > > > > > > > I could attribute to this.
> > > > > > > > > > 
> > > > > > > > > > I guess it should be ok.
> > > > > > > > > 
> > > > > > > > > What should be ok ?
> > > > > > > > 
> > > > > > > > Your approach, but not your patch. If we go with this let's 
> > > > > > > > just lift it
> > > > > > > > to drm_ioctl() as the default behavior. No driver opt-in flag, 
> > > > > > > > because
> > > > > > > > that's definitely worse than any other approach because we 
> > > > > > > > really need to
> > > > > > > > get rid of driver specific behaviour for generic ioctls, 
> > > > > > > > especially
> > > > > > > > anything a compositor will use directly.
> > > > > > > > 
> > > > > > > > > > My reasons for making this work is both less trouble for 
> > > > > > > > > > userspace (did
> > > > > > > > > > you test with various wayland compositors out there, not 
> > > > > > > > > > just amdgpu x86
> > > > > > > > > 
> > > > > > > > > I didn't - will give it a try.
> > > > > > > 
> > > > > > > Weston worked without crashes, run the egl tester cube there.
> > > > > > > 
> > > > >

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

2021-05-07 Thread Andrey Grodzovsky

On 2021-05-07 12:24 p.m., Daniel Vetter wrote:

On Fri, May 07, 2021 at 11:39:49AM -0400, Andrey Grodzovsky wrote:

On 2021-05-07 5:11 a.m., Daniel Vetter wrote:

On Thu, May 06, 2021 at 12:25:06PM -0400, Andrey Grodzovsky wrote:

On 2021-05-06 5:40 a.m., Daniel Vetter wrote:

On Fri, Apr 30, 2021 at 01:27:37PM -0400, Andrey Grodzovsky wrote:

On 2021-04-30 6:25 a.m., Daniel Vetter wrote:

On Thu, Apr 29, 2021 at 04:34:55PM -0400, Andrey Grodzovsky wrote:

On 2021-04-29 3:05 p.m., Daniel Vetter wrote:

On Thu, Apr 29, 2021 at 12:04:33PM -0400, Andrey Grodzovsky wrote:

On 2021-04-29 7:32 a.m., Daniel Vetter wrote:

On Thu, Apr 29, 2021 at 01:23:19PM +0200, Daniel Vetter wrote:

On Wed, Apr 28, 2021 at 11:12:00AM -0400, Andrey Grodzovsky wrote:

With this calling drm_dev_unplug will flush and block
all in flight IOCTLs

Also, add feature such that if device supports graceful unplug
we enclose entire IOCTL in SRCU critical section.

Signed-off-by: Andrey Grodzovsky

Nope.

The idea of drm_dev_enter/exit is to mark up hw access. Not entire ioctl.

Then I am confused why we have
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.12%2Fsource%2Fdrivers%2Fgpu%2Fdrm%2Fdrm_ioctl.c%23L826&data=04%7C01%7Candrey.grodzovsky%40amd.com%7C66e4988eb341427e8b0108d91174a232%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637560014906903277%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=rdb3xesAUYYTeqU2WdoZ%2BWLzOuuRdOxBBQNTMMB%2BKB4%3D&reserved=0
currently in code ?

I forgot about this one, again. Thanks for reminding.

Especially not with an opt-in flag so that it could be shrugged of as a
driver hack. Most of these ioctls should have absolutely no problem
working after hotunplug.

Also, doing this defeats the point since it pretty much guarantees
userspace will die in assert()s and stuff. E.g. on i915 the rough contract
is that only execbuf (and even that only when userspace has indicated
support for non-recoverable hw ctx) is allowed to fail. Anything else
might crash userspace.

Given that as I pointed above we already fail any IOCTls with -ENODEV
when device is unplugged, it seems those crashes don't happen that
often ? Also, in all my testing I don't think I saw a user space crash
I could attribute to this.

I guess it should be ok.

What should be ok ?

Your approach, but not your patch. If we go with this let's just lift it
to drm_ioctl() as the default behavior. No driver opt-in flag, because
that's definitely worse than any other approach because we really need to
get rid of driver specific behaviour for generic ioctls, especially
anything a compositor will use directly.

My reasons for making this work is both less trouble for userspace (did
you test with various wayland compositors out there, not just amdgpu x86

I didn't - will give it a try.

Weston worked without crashes, run the egl tester cube there.

driver?), but also testing.

We still need a bunch of these checks in various places or you'll wait a
very long time for a pending modeset or similar to complete. Being able to
run that code easily after hotunplug has completed should help a lot with
testing.

Plus various drivers already acquired drm_dev_enter/exit and now I wonder
whether that was properly tested or not ...

I guess maybe we need a drm module option to disable this check, so that
we can exercise the code as if the ioctl has raced with hotunplug at the
worst possible moment.

Also atomic is really tricky here: I assume your testing has just done
normal synchronous commits, but anything that goes through atomic can be
done nonblocking in a separate thread. Which the ioctl catch-all here wont
capture.

Yes, async commit was on my mind and thanks for reminding me. Indeed
I forgot this but i planned to scope the entire amdgpu_dm_atomic_tail in
drm_dev_enter/exit. Note that i have a bunch of patches, all name's
starting with 'Scope' that just methodically put all the background
work items and timers the drivers schedules in drm_dev_enter/exit scope.
This was supposed to be part of the 'Scope Display code' patch.

That's too much. You still have to arrange that the flip completion event
gets sent out. So it's a bit tricky.

In other places the same problem applies, e.g. probe functions need to
make sure they report "disconnected".

I see, well, this is all part of KMS support which I defer for now
anyway. Will tackle it then.

You probably need similar (and very precisely defined) rules for amdgpu.
And those must definitely exclude any shard ioctls from randomly failing
with EIO, because that just kills the box and defeats the point of trying
to gracefully handling hotunplug and making sure userspace has a chance of
survival. E.g. for atomic everything should continue, including flip
completion, but we set all outputs to "disconnected" and send out the
uevent. Maybe crtc enabling can fail too, but that can als

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

2021-05-07 Thread Daniel Vetter

On Fri, May 07, 2021 at 11:39:49AM -0400, Andrey Grodzovsky wrote:
> 
> 
> On 2021-05-07 5:11 a.m., Daniel Vetter wrote:
> > On Thu, May 06, 2021 at 12:25:06PM -0400, Andrey Grodzovsky wrote:
> > > 
> > > 
> > > On 2021-05-06 5:40 a.m., Daniel Vetter wrote:
> > > > On Fri, Apr 30, 2021 at 01:27:37PM -0400, Andrey Grodzovsky wrote:
> > > > > 
> > > > > 
> > > > > On 2021-04-30 6:25 a.m., Daniel Vetter wrote:
> > > > > > On Thu, Apr 29, 2021 at 04:34:55PM -0400, Andrey Grodzovsky wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > On 2021-04-29 3:05 p.m., Daniel Vetter wrote:
> > > > > > > > On Thu, Apr 29, 2021 at 12:04:33PM -0400, Andrey Grodzovsky 
> > > > > > > > wrote:
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > On 2021-04-29 7:32 a.m., Daniel Vetter wrote:
> > > > > > > > > > On Thu, Apr 29, 2021 at 01:23:19PM +0200, Daniel Vetter 
> > > > > > > > > > wrote:
> > > > > > > > > > > On Wed, Apr 28, 2021 at 11:12:00AM -0400, Andrey 
> > > > > > > > > > > Grodzovsky wrote:
> > > > > > > > > > > > With this calling drm_dev_unplug will flush and block
> > > > > > > > > > > > all in flight IOCTLs
> > > > > > > > > > > > 
> > > > > > > > > > > > Also, add feature such that if device supports graceful 
> > > > > > > > > > > > unplug
> > > > > > > > > > > > we enclose entire IOCTL in SRCU critical section.
> > > > > > > > > > > > 
> > > > > > > > > > > > Signed-off-by: Andrey Grodzovsky 
> > > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > Nope.
> > > > > > > > > > > 
> > > > > > > > > > > The idea of drm_dev_enter/exit is to mark up hw access. 
> > > > > > > > > > > Not entire ioctl.
> > > > > > > > > 
> > > > > > > > > Then I am confused why we have 
> > > > > > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.12%2Fsource%2Fdrivers%2Fgpu%2Fdrm%2Fdrm_ioctl.c%23L826&data=04%7C01%7Candrey.grodzovsky%40amd.com%7Ce53ea46e66fa40a0e03f08d911381a05%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637559754928702763%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=zMlHiglnn8Vm%2BVxI9Rbk8X%2BTyuokq1x1INbhbRCWK4E%3D&reserved=0
> > > > > > > > > currently in code ?
> > > > > > > > 
> > > > > > > > I forgot about this one, again. Thanks for reminding.
> > > > > > > > 
> > > > > > > > > > > Especially not with an opt-in flag so that it could be 
> > > > > > > > > > > shrugged of as a
> > > > > > > > > > > driver hack. Most of these ioctls should have absolutely 
> > > > > > > > > > > no problem
> > > > > > > > > > > working after hotunplug.
> > > > > > > > > > > 
> > > > > > > > > > > Also, doing this defeats the point since it pretty much 
> > > > > > > > > > > guarantees
> > > > > > > > > > > userspace will die in assert()s and stuff. E.g. on i915 
> > > > > > > > > > > the rough contract
> > > > > > > > > > > is that only execbuf (and even that only when userspace 
> > > > > > > > > > > has indicated
> > > > > > > > > > > support for non-recoverable hw ctx) is allowed to fail. 
> > > > > > > > > > > Anything else
> > > > > > > > > > > might crash userspace.
> > > > > > > > > 
> > > > > > > > > Given that as I pointed above we already fail any IOCTls with 
> > > > > > > > > -ENODEV
> > > > > > > > > when device is unplugged, it seems those crashes don't happen 
> > > > > > > > > that
> > > > > > > > > often ? Also, in all my testing I don't think I saw a user 
> > > > > > > > > space crash
> > > > > > > > > I could attribute to this.
> > > > > > > > 
> > > > > > > > I guess it should be ok.
> > > > > > > 
> > > > > > > What should be ok ?
> > > > > > 
> > > > > > Your approach, but not your patch. If we go with this let's just 
> > > > > > lift it
> > > > > > to drm_ioctl() as the default behavior. No driver opt-in flag, 
> > > > > > because
> > > > > > that's definitely worse than any other approach because we really 
> > > > > > need to
> > > > > > get rid of driver specific behaviour for generic ioctls, especially
> > > > > > anything a compositor will use directly.
> > > > > > 
> > > > > > > > My reasons for making this work is both less trouble for 
> > > > > > > > userspace (did
> > > > > > > > you test with various wayland compositors out there, not just 
> > > > > > > > amdgpu x86
> > > > > > > 
> > > > > > > I didn't - will give it a try.
> > > > > 
> > > > > Weston worked without crashes, run the egl tester cube there.
> > > > > 
> > > > > > > 
> > > > > > > > driver?), but also testing.
> > > > > > > > 
> > > > > > > > We still need a bunch of these checks in various places or 
> > > > > > > > you'll wait a
> > > > > > > > very long time for a pending modeset or similar to complete. 
> > > > > > > > Being able to
> > > > > > > > run that code easily after hotunplug has completed should help 
> > > > > > > > a lot with
> > > > > > > > testing.
> > > > > > > > 
> > > > > > > > Plus various drivers already acquired drm_dev_enter/exit and 
> > > > > > > > now I wonder
> > > > > > > >

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

2021-05-07 Thread Andrey Grodzovsky

On 2021-05-07 5:11 a.m., Daniel Vetter wrote:

On Thu, May 06, 2021 at 12:25:06PM -0400, Andrey Grodzovsky wrote:

On 2021-05-06 5:40 a.m., Daniel Vetter wrote:

On Fri, Apr 30, 2021 at 01:27:37PM -0400, Andrey Grodzovsky wrote:

On 2021-04-30 6:25 a.m., Daniel Vetter wrote:

On Thu, Apr 29, 2021 at 04:34:55PM -0400, Andrey Grodzovsky wrote:

On 2021-04-29 3:05 p.m., Daniel Vetter wrote:

On Thu, Apr 29, 2021 at 12:04:33PM -0400, Andrey Grodzovsky wrote:

On 2021-04-29 7:32 a.m., Daniel Vetter wrote:

On Thu, Apr 29, 2021 at 01:23:19PM +0200, Daniel Vetter wrote:

On Wed, Apr 28, 2021 at 11:12:00AM -0400, Andrey Grodzovsky wrote:

With this calling drm_dev_unplug will flush and block
all in flight IOCTLs

Also, add feature such that if device supports graceful unplug
we enclose entire IOCTL in SRCU critical section.

Signed-off-by: Andrey Grodzovsky

Nope.

The idea of drm_dev_enter/exit is to mark up hw access. Not entire ioctl.

Then I am confused why we have
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.12%2Fsource%2Fdrivers%2Fgpu%2Fdrm%2Fdrm_ioctl.c%23L826&data=04%7C01%7Candrey.grodzovsky%40amd.com%7Ce53ea46e66fa40a0e03f08d911381a05%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637559754928702763%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=zMlHiglnn8Vm%2BVxI9Rbk8X%2BTyuokq1x1INbhbRCWK4E%3D&reserved=0
currently in code ?

I forgot about this one, again. Thanks for reminding.

Especially not with an opt-in flag so that it could be shrugged of as a
driver hack. Most of these ioctls should have absolutely no problem
working after hotunplug.

I guess it should be ok.

What should be ok ?

My reasons for making this work is both less trouble for userspace (did
you test with various wayland compositors out there, not just amdgpu x86

I didn't - will give it a try.

Weston worked without crashes, run the egl tester cube there.

driver?), but also testing.

Plus various drivers already acquired drm_dev_enter/exit and now I wonder
whether that was properly tested or not ...

I guess maybe we need a drm module option to disable this check, so that
we can exercise the code as if the ioctl has raced with hotunplug at the
worst possible moment.

That's too much. You still have to arrange that the flip completion event
gets sent out. So it's a bit tricky.

In other places the same problem applies, e.g. probe functions need to
make sure they report "disconnected".

I see, well, this is all part of KMS support which I defer for now
anyway. Will tackle it then.

As I pointed before, beca

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

2021-05-07 Thread Daniel Vetter

On Thu, May 06, 2021 at 12:25:06PM -0400, Andrey Grodzovsky wrote:
> 
> 
> On 2021-05-06 5:40 a.m., Daniel Vetter wrote:
> > On Fri, Apr 30, 2021 at 01:27:37PM -0400, Andrey Grodzovsky wrote:
> > > 
> > > 
> > > On 2021-04-30 6:25 a.m., Daniel Vetter wrote:
> > > > On Thu, Apr 29, 2021 at 04:34:55PM -0400, Andrey Grodzovsky wrote:
> > > > > 
> > > > > 
> > > > > On 2021-04-29 3:05 p.m., Daniel Vetter wrote:
> > > > > > On Thu, Apr 29, 2021 at 12:04:33PM -0400, Andrey Grodzovsky wrote:
> > > > > > > 
> > > > > > > 
> > > > > > > On 2021-04-29 7:32 a.m., Daniel Vetter wrote:
> > > > > > > > On Thu, Apr 29, 2021 at 01:23:19PM +0200, Daniel Vetter wrote:
> > > > > > > > > On Wed, Apr 28, 2021 at 11:12:00AM -0400, Andrey Grodzovsky 
> > > > > > > > > wrote:
> > > > > > > > > > With this calling drm_dev_unplug will flush and block
> > > > > > > > > > all in flight IOCTLs
> > > > > > > > > > 
> > > > > > > > > > Also, add feature such that if device supports graceful 
> > > > > > > > > > unplug
> > > > > > > > > > we enclose entire IOCTL in SRCU critical section.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Andrey Grodzovsky 
> > > > > > > > > 
> > > > > > > > > Nope.
> > > > > > > > > 
> > > > > > > > > The idea of drm_dev_enter/exit is to mark up hw access. Not 
> > > > > > > > > entire ioctl.
> > > > > > > 
> > > > > > > Then I am confused why we have 
> > > > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.12%2Fsource%2Fdrivers%2Fgpu%2Fdrm%2Fdrm_ioctl.c%23L826&data=04%7C01%7Candrey.grodzovsky%40amd.com%7Ca0ca5bdab20a4533491c08d91072fe2a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637558908355926609%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=SESZFWQEcQUHGGek8d1cNi9Iwo9XOmXqxg9MieRkxNU%3D&reserved=0
> > > > > > > currently in code ?
> > > > > > 
> > > > > > I forgot about this one, again. Thanks for reminding.
> > > > > > 
> > > > > > > > > Especially not with an opt-in flag so that it could be 
> > > > > > > > > shrugged of as a
> > > > > > > > > driver hack. Most of these ioctls should have absolutely no 
> > > > > > > > > problem
> > > > > > > > > working after hotunplug.
> > > > > > > > > 
> > > > > > > > > Also, doing this defeats the point since it pretty much 
> > > > > > > > > guarantees
> > > > > > > > > userspace will die in assert()s and stuff. E.g. on i915 the 
> > > > > > > > > rough contract
> > > > > > > > > is that only execbuf (and even that only when userspace has 
> > > > > > > > > indicated
> > > > > > > > > support for non-recoverable hw ctx) is allowed to fail. 
> > > > > > > > > Anything else
> > > > > > > > > might crash userspace.
> > > > > > > 
> > > > > > > Given that as I pointed above we already fail any IOCTls with 
> > > > > > > -ENODEV
> > > > > > > when device is unplugged, it seems those crashes don't happen that
> > > > > > > often ? Also, in all my testing I don't think I saw a user space 
> > > > > > > crash
> > > > > > > I could attribute to this.
> > > > > > 
> > > > > > I guess it should be ok.
> > > > > 
> > > > > What should be ok ?
> > > > 
> > > > Your approach, but not your patch. If we go with this let's just lift it
> > > > to drm_ioctl() as the default behavior. No driver opt-in flag, because
> > > > that's definitely worse than any other approach because we really need 
> > > > to
> > > > get rid of driver specific behaviour for generic ioctls, especially
> > > > anything a compositor will use directly.
> > > > 
> > > > > > My reasons for making this work is both less trouble for userspace 
> > > > > > (did
> > > > > > you test with various wayland compositors out there, not just 
> > > > > > amdgpu x86
> > > > > 
> > > > > I didn't - will give it a try.
> > > 
> > > Weston worked without crashes, run the egl tester cube there.
> > > 
> > > > > 
> > > > > > driver?), but also testing.
> > > > > > 
> > > > > > We still need a bunch of these checks in various places or you'll 
> > > > > > wait a
> > > > > > very long time for a pending modeset or similar to complete. Being 
> > > > > > able to
> > > > > > run that code easily after hotunplug has completed should help a 
> > > > > > lot with
> > > > > > testing.
> > > > > > 
> > > > > > Plus various drivers already acquired drm_dev_enter/exit and now I 
> > > > > > wonder
> > > > > > whether that was properly tested or not ...
> > > > > > 
> > > > > > I guess maybe we need a drm module option to disable this check, so 
> > > > > > that
> > > > > > we can exercise the code as if the ioctl has raced with hotunplug 
> > > > > > at the
> > > > > > worst possible moment.
> > > > > > 
> > > > > > Also atomic is really tricky here: I assume your testing has just 
> > > > > > done
> > > > > > normal synchronous commits, but anything that goes through atomic 
> > > > > > can be
> > > > > > done nonblocking in a separate thread. Which the ioctl catch-all 
> > > > > > here wont
> > > > > > capture.
>

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

2021-05-06 Thread Andrey Grodzovsky

On 2021-05-06 5:40 a.m., Daniel Vetter wrote:

On Fri, Apr 30, 2021 at 01:27:37PM -0400, Andrey Grodzovsky wrote:

On 2021-04-30 6:25 a.m., Daniel Vetter wrote:

On Thu, Apr 29, 2021 at 04:34:55PM -0400, Andrey Grodzovsky wrote:

On 2021-04-29 3:05 p.m., Daniel Vetter wrote:

On Thu, Apr 29, 2021 at 12:04:33PM -0400, Andrey Grodzovsky wrote:

On 2021-04-29 7:32 a.m., Daniel Vetter wrote:

On Thu, Apr 29, 2021 at 01:23:19PM +0200, Daniel Vetter wrote:

On Wed, Apr 28, 2021 at 11:12:00AM -0400, Andrey Grodzovsky wrote:

With this calling drm_dev_unplug will flush and block
all in flight IOCTLs

Also, add feature such that if device supports graceful unplug
we enclose entire IOCTL in SRCU critical section.

Signed-off-by: Andrey Grodzovsky

Nope.

The idea of drm_dev_enter/exit is to mark up hw access. Not entire ioctl.

Then I am confused why we have
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.12%2Fsource%2Fdrivers%2Fgpu%2Fdrm%2Fdrm_ioctl.c%23L826&data=04%7C01%7Candrey.grodzovsky%40amd.com%7Ca0ca5bdab20a4533491c08d91072fe2a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637558908355926609%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=SESZFWQEcQUHGGek8d1cNi9Iwo9XOmXqxg9MieRkxNU%3D&reserved=0
currently in code ?

I forgot about this one, again. Thanks for reminding.

Especially not with an opt-in flag so that it could be shrugged of as a
driver hack. Most of these ioctls should have absolutely no problem
working after hotunplug.

I guess it should be ok.

What should be ok ?

My reasons for making this work is both less trouble for userspace (did
you test with various wayland compositors out there, not just amdgpu x86

I didn't - will give it a try.

Weston worked without crashes, run the egl tester cube there.

driver?), but also testing.

Plus various drivers already acquired drm_dev_enter/exit and now I wonder
whether that was properly tested or not ...

I guess maybe we need a drm module option to disable this check, so that
we can exercise the code as if the ioctl has raced with hotunplug at the
worst possible moment.

That's too much. You still have to arrange that the flip completion event
gets sent out. So it's a bit tricky.

In other places the same problem applies, e.g. probe functions need to
make sure they report "disconnected".

I see, well, this is all part of KMS support which I defer for now
anyway. Will tackle it then.

As I pointed before, because of the complexity of the topic I prefer to
take it step by step and solve first for secondary device use case, not
fo

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

2021-05-06 Thread Daniel Vetter

On Fri, Apr 30, 2021 at 01:27:37PM -0400, Andrey Grodzovsky wrote:
> 
> 
> On 2021-04-30 6:25 a.m., Daniel Vetter wrote:
> > On Thu, Apr 29, 2021 at 04:34:55PM -0400, Andrey Grodzovsky wrote:
> > > 
> > > 
> > > On 2021-04-29 3:05 p.m., Daniel Vetter wrote:
> > > > On Thu, Apr 29, 2021 at 12:04:33PM -0400, Andrey Grodzovsky wrote:
> > > > > 
> > > > > 
> > > > > On 2021-04-29 7:32 a.m., Daniel Vetter wrote:
> > > > > > On Thu, Apr 29, 2021 at 01:23:19PM +0200, Daniel Vetter wrote:
> > > > > > > On Wed, Apr 28, 2021 at 11:12:00AM -0400, Andrey Grodzovsky wrote:
> > > > > > > > With this calling drm_dev_unplug will flush and block
> > > > > > > > all in flight IOCTLs
> > > > > > > > 
> > > > > > > > Also, add feature such that if device supports graceful unplug
> > > > > > > > we enclose entire IOCTL in SRCU critical section.
> > > > > > > > 
> > > > > > > > Signed-off-by: Andrey Grodzovsky 
> > > > > > > 
> > > > > > > Nope.
> > > > > > > 
> > > > > > > The idea of drm_dev_enter/exit is to mark up hw access. Not 
> > > > > > > entire ioctl.
> > > > > 
> > > > > Then I am confused why we have 
> > > > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.12%2Fsource%2Fdrivers%2Fgpu%2Fdrm%2Fdrm_ioctl.c%23L826&data=04%7C01%7Candrey.grodzovsky%40amd.com%7Cf4c0568093cc462f625808d90bc23a3c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637553751106596888%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=PPKrQYBrgRMjpwlL0r8n5zenIhQMFWc6gniHgUTxTAY%3D&reserved=0
> > > > > currently in code ?
> > > > 
> > > > I forgot about this one, again. Thanks for reminding.
> > > > 
> > > > > > > Especially not with an opt-in flag so that it could be shrugged 
> > > > > > > of as a
> > > > > > > driver hack. Most of these ioctls should have absolutely no 
> > > > > > > problem
> > > > > > > working after hotunplug.
> > > > > > > 
> > > > > > > Also, doing this defeats the point since it pretty much guarantees
> > > > > > > userspace will die in assert()s and stuff. E.g. on i915 the rough 
> > > > > > > contract
> > > > > > > is that only execbuf (and even that only when userspace has 
> > > > > > > indicated
> > > > > > > support for non-recoverable hw ctx) is allowed to fail. Anything 
> > > > > > > else
> > > > > > > might crash userspace.
> > > > > 
> > > > > Given that as I pointed above we already fail any IOCTls with -ENODEV
> > > > > when device is unplugged, it seems those crashes don't happen that
> > > > > often ? Also, in all my testing I don't think I saw a user space crash
> > > > > I could attribute to this.
> > > > 
> > > > I guess it should be ok.
> > > 
> > > What should be ok ?
> > 
> > Your approach, but not your patch. If we go with this let's just lift it
> > to drm_ioctl() as the default behavior. No driver opt-in flag, because
> > that's definitely worse than any other approach because we really need to
> > get rid of driver specific behaviour for generic ioctls, especially
> > anything a compositor will use directly.
> > 
> > > > My reasons for making this work is both less trouble for userspace (did
> > > > you test with various wayland compositors out there, not just amdgpu x86
> > > 
> > > I didn't - will give it a try.
> 
> Weston worked without crashes, run the egl tester cube there.
> 
> > > 
> > > > driver?), but also testing.
> > > > 
> > > > We still need a bunch of these checks in various places or you'll wait a
> > > > very long time for a pending modeset or similar to complete. Being able 
> > > > to
> > > > run that code easily after hotunplug has completed should help a lot 
> > > > with
> > > > testing.
> > > > 
> > > > Plus various drivers already acquired drm_dev_enter/exit and now I 
> > > > wonder
> > > > whether that was properly tested or not ...
> > > > 
> > > > I guess maybe we need a drm module option to disable this check, so that
> > > > we can exercise the code as if the ioctl has raced with hotunplug at the
> > > > worst possible moment.
> > > > 
> > > > Also atomic is really tricky here: I assume your testing has just done
> > > > normal synchronous commits, but anything that goes through atomic can be
> > > > done nonblocking in a separate thread. Which the ioctl catch-all here 
> > > > wont
> > > > capture.
> > > 
> > > Yes, async commit was on my mind and thanks for reminding me. Indeed
> > > I forgot this but i planned to scope the entire amdgpu_dm_atomic_tail in
> > > drm_dev_enter/exit. Note that i have a bunch of patches, all name's
> > > starting with 'Scope' that just methodically put all the background
> > > work items and timers the drivers schedules in drm_dev_enter/exit scope.
> > > This was supposed to be part of the 'Scope Display code' patch.
> > 
> > That's too much. You still have to arrange that the flip completion event
> > gets sent out. So it's a bit tricky.
> > 
> > In other places the same problem applies, e.g. probe functions need to
> > make sure the

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

2021-05-05 Thread Andrey Grodzovsky

Ping

Andrey

On 2021-04-30 1:27 p.m., Andrey Grodzovsky wrote:

On 2021-04-30 6:25 a.m., Daniel Vetter wrote:

On Thu, Apr 29, 2021 at 04:34:55PM -0400, Andrey Grodzovsky wrote:

On 2021-04-29 3:05 p.m., Daniel Vetter wrote:

On Thu, Apr 29, 2021 at 12:04:33PM -0400, Andrey Grodzovsky wrote:

On 2021-04-29 7:32 a.m., Daniel Vetter wrote:

On Thu, Apr 29, 2021 at 01:23:19PM +0200, Daniel Vetter wrote:

On Wed, Apr 28, 2021 at 11:12:00AM -0400, Andrey Grodzovsky wrote:

With this calling drm_dev_unplug will flush and block
all in flight IOCTLs

Also, add feature such that if device supports graceful unplug
we enclose entire IOCTL in SRCU critical section.

Signed-off-by: Andrey Grodzovsky

Nope.

The idea of drm_dev_enter/exit is to mark up hw access. Not
entire ioctl.

Then I am confused why we have
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.12%2Fsource%2Fdrivers%2Fgpu%2Fdrm%2Fdrm_ioctl.c%23L826&data=04%7C01%7Candrey.grodzovsky%40amd.com%7Cf4c0568093cc462f625808d90bc23a3c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637553751106596888%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=PPKrQYBrgRMjpwlL0r8n5zenIhQMFWc6gniHgUTxTAY%3D&reserved=0

currently in code ?

I forgot about this one, again. Thanks for reminding.

Especially not with an opt-in flag so that it could be shrugged
of as a

driver hack. Most of these ioctls should have absolutely no problem
working after hotunplug.

Also, doing this defeats the point since it pretty much guarantees
userspace will die in assert()s and stuff. E.g. on i915 the rough
contract
is that only execbuf (and even that only when userspace has
indicated
support for non-recoverable hw ctx) is allowed to fail. Anything
else

might crash userspace.

I guess it should be ok.

What should be ok ?

My reasons for making this work is both less trouble for userspace (did
you test with various wayland compositors out there, not just amdgpu
x86

I didn't - will give it a try.

Weston worked without crashes, run the egl tester cube there.

driver?), but also testing.

We still need a bunch of these checks in various places or you'll
wait a
very long time for a pending modeset or similar to complete. Being
able to
run that code easily after hotunplug has completed should help a lot
with

testing.

Plus various drivers already acquired drm_dev_enter/exit and now I
wonder

whether that was properly tested or not ...

I guess maybe we need a drm module option to disable this check, so
that
we can exercise the code as if the ioctl has raced with hotunplug at
the

worst possible moment.

Also atomic is really tricky here: I assume your testing has just done
normal synchronous commits, but anything that goes through atomic
can be
done nonblocking in a separate thread. Which the ioctl catch-all
here wont

capture.

That's too much. You still have to arrange that the flip completion event
gets sent out. So it's a bit tricky.

In other places the same problem applies, e.g. probe functions need to
make sure they report "disconnected".

I see, well, this is all part of KMS support which I defer for now
anyway. Will tackle it then.

survival. E.g. for atomic everything should continue, including flip
completion, but we set all outputs to "disconnected" and send out
the
uevent. Maybe crtc enabling can fail too, but that can also be
handled

through the async status we're using to signal DP link failures to
userspace.

As I pointed before, because of the complexity of the topic I
prefer to
take it step by step and solve first for secondary device use case,
not

for primary, display at

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

2021-04-30 Thread Andrey Grodzovsky

On 2021-04-30 6:25 a.m., Daniel Vetter wrote:

On Thu, Apr 29, 2021 at 04:34:55PM -0400, Andrey Grodzovsky wrote:

On 2021-04-29 3:05 p.m., Daniel Vetter wrote:

On Thu, Apr 29, 2021 at 12:04:33PM -0400, Andrey Grodzovsky wrote:

On 2021-04-29 7:32 a.m., Daniel Vetter wrote:

On Thu, Apr 29, 2021 at 01:23:19PM +0200, Daniel Vetter wrote:

On Wed, Apr 28, 2021 at 11:12:00AM -0400, Andrey Grodzovsky wrote:

With this calling drm_dev_unplug will flush and block
all in flight IOCTLs

Also, add feature such that if device supports graceful unplug
we enclose entire IOCTL in SRCU critical section.

Signed-off-by: Andrey Grodzovsky

Nope.

The idea of drm_dev_enter/exit is to mark up hw access. Not entire ioctl.

Then I am confused why we have
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.12%2Fsource%2Fdrivers%2Fgpu%2Fdrm%2Fdrm_ioctl.c%23L826&data=04%7C01%7Candrey.grodzovsky%40amd.com%7Cf4c0568093cc462f625808d90bc23a3c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637553751106596888%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=PPKrQYBrgRMjpwlL0r8n5zenIhQMFWc6gniHgUTxTAY%3D&reserved=0
currently in code ?

I forgot about this one, again. Thanks for reminding.

Especially not with an opt-in flag so that it could be shrugged of as a
driver hack. Most of these ioctls should have absolutely no problem
working after hotunplug.

I guess it should be ok.

What should be ok ?

My reasons for making this work is both less trouble for userspace (did
you test with various wayland compositors out there, not just amdgpu x86

I didn't - will give it a try.

Weston worked without crashes, run the egl tester cube there.

driver?), but also testing.

Plus various drivers already acquired drm_dev_enter/exit and now I wonder
whether that was properly tested or not ...

I guess maybe we need a drm module option to disable this check, so that
we can exercise the code as if the ioctl has raced with hotunplug at the
worst possible moment.

That's too much. You still have to arrange that the flip completion event
gets sent out. So it's a bit tricky.

In other places the same problem applies, e.g. probe functions need to
make sure they report "disconnected".

I see, well, this is all part of KMS support which I defer for now
anyway. Will tackle it then.

As I pointed before, because of the complexity of the topic I prefer to
take it step by step and solve first for secondary device use case, not
for primary, display attached device.

Yeah makes sense. But then I think the right patch is to roll this out for
all

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

2021-04-30 Thread Daniel Vetter

On Thu, Apr 29, 2021 at 04:34:55PM -0400, Andrey Grodzovsky wrote:
> 
> 
> On 2021-04-29 3:05 p.m., Daniel Vetter wrote:
> > On Thu, Apr 29, 2021 at 12:04:33PM -0400, Andrey Grodzovsky wrote:
> > > 
> > > 
> > > On 2021-04-29 7:32 a.m., Daniel Vetter wrote:
> > > > On Thu, Apr 29, 2021 at 01:23:19PM +0200, Daniel Vetter wrote:
> > > > > On Wed, Apr 28, 2021 at 11:12:00AM -0400, Andrey Grodzovsky wrote:
> > > > > > With this calling drm_dev_unplug will flush and block
> > > > > > all in flight IOCTLs
> > > > > > 
> > > > > > Also, add feature such that if device supports graceful unplug
> > > > > > we enclose entire IOCTL in SRCU critical section.
> > > > > > 
> > > > > > Signed-off-by: Andrey Grodzovsky 
> > > > > 
> > > > > Nope.
> > > > > 
> > > > > The idea of drm_dev_enter/exit is to mark up hw access. Not entire 
> > > > > ioctl.
> > > 
> > > Then I am confused why we have 
> > > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.12%2Fsource%2Fdrivers%2Fgpu%2Fdrm%2Fdrm_ioctl.c%23L826&data=04%7C01%7Candrey.grodzovsky%40amd.com%7C1821a19173a84ebae31108d90b41b2fa%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637553199084555468%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=d6kXadWHv4CEDgODsm%2FOzIIjIDA9rZDLUuV11MmEU3A%3D&reserved=0
> > > currently in code ?
> > 
> > I forgot about this one, again. Thanks for reminding.
> > 
> > > > > Especially not with an opt-in flag so that it could be shrugged of as 
> > > > > a
> > > > > driver hack. Most of these ioctls should have absolutely no problem
> > > > > working after hotunplug.
> > > > > 
> > > > > Also, doing this defeats the point since it pretty much guarantees
> > > > > userspace will die in assert()s and stuff. E.g. on i915 the rough 
> > > > > contract
> > > > > is that only execbuf (and even that only when userspace has indicated
> > > > > support for non-recoverable hw ctx) is allowed to fail. Anything else
> > > > > might crash userspace.
> > > 
> > > Given that as I pointed above we already fail any IOCTls with -ENODEV
> > > when device is unplugged, it seems those crashes don't happen that
> > > often ? Also, in all my testing I don't think I saw a user space crash
> > > I could attribute to this.
> > 
> > I guess it should be ok.
> 
> What should be ok ?

Your approach, but not your patch. If we go with this let's just lift it
to drm_ioctl() as the default behavior. No driver opt-in flag, because
that's definitely worse than any other approach because we really need to
get rid of driver specific behaviour for generic ioctls, especially
anything a compositor will use directly.

> > My reasons for making this work is both less trouble for userspace (did
> > you test with various wayland compositors out there, not just amdgpu x86
> 
> I didn't - will give it a try.
> 
> > driver?), but also testing.
> > 
> > We still need a bunch of these checks in various places or you'll wait a
> > very long time for a pending modeset or similar to complete. Being able to
> > run that code easily after hotunplug has completed should help a lot with
> > testing.
> > 
> > Plus various drivers already acquired drm_dev_enter/exit and now I wonder
> > whether that was properly tested or not ...
> > 
> > I guess maybe we need a drm module option to disable this check, so that
> > we can exercise the code as if the ioctl has raced with hotunplug at the
> > worst possible moment.
> > 
> > Also atomic is really tricky here: I assume your testing has just done
> > normal synchronous commits, but anything that goes through atomic can be
> > done nonblocking in a separate thread. Which the ioctl catch-all here wont
> > capture.
> 
> Yes, async commit was on my mind and thanks for reminding me. Indeed
> I forgot this but i planned to scope the entire amdgpu_dm_atomic_tail in
> drm_dev_enter/exit. Note that i have a bunch of patches, all name's
> starting with 'Scope' that just methodically put all the background
> work items and timers the drivers schedules in drm_dev_enter/exit scope.
> This was supposed to be part of the 'Scope Display code' patch.

That's too much. You still have to arrange that the flip completion event
gets sent out. So it's a bit tricky.

In other places the same problem applies, e.g. probe functions need to
make sure they report "disconnected".

> > > > > You probably need similar (and very precisely defined) rules for 
> > > > > amdgpu.
> > > > > And those must definitely exclude any shard ioctls from randomly 
> > > > > failing
> > > > > with EIO, because that just kills the box and defeats the point of 
> > > > > trying
> > > > > to gracefully handling hotunplug and making sure userspace has a 
> > > > > chance of
> > > > > survival. E.g. for atomic everything should continue, including flip
> > > > > completion, but we set all outputs to "disconnected" and send out the
> > > > > uevent. Maybe crtc enabling can fail too, but that can also be h

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

2021-04-29 Thread Andrey Grodzovsky

On 2021-04-29 3:05 p.m., Daniel Vetter wrote:

On Thu, Apr 29, 2021 at 12:04:33PM -0400, Andrey Grodzovsky wrote:

On 2021-04-29 7:32 a.m., Daniel Vetter wrote:

On Thu, Apr 29, 2021 at 01:23:19PM +0200, Daniel Vetter wrote:

On Wed, Apr 28, 2021 at 11:12:00AM -0400, Andrey Grodzovsky wrote:

With this calling drm_dev_unplug will flush and block
all in flight IOCTLs

Also, add feature such that if device supports graceful unplug
we enclose entire IOCTL in SRCU critical section.

Signed-off-by: Andrey Grodzovsky

Nope.

The idea of drm_dev_enter/exit is to mark up hw access. Not entire ioctl.

Then I am confused why we have
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.com%2Flinux%2Fv5.12%2Fsource%2Fdrivers%2Fgpu%2Fdrm%2Fdrm_ioctl.c%23L826&data=04%7C01%7Candrey.grodzovsky%40amd.com%7C1821a19173a84ebae31108d90b41b2fa%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637553199084555468%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=d6kXadWHv4CEDgODsm%2FOzIIjIDA9rZDLUuV11MmEU3A%3D&reserved=0
currently in code ?

I forgot about this one, again. Thanks for reminding.

Especially not with an opt-in flag so that it could be shrugged of as a
driver hack. Most of these ioctls should have absolutely no problem
working after hotunplug.

I guess it should be ok.

What should be ok ?

My reasons for making this work is both less trouble for userspace (did
you test with various wayland compositors out there, not just amdgpu x86

I didn't - will give it a try.

driver?), but also testing.

Plus various drivers already acquired drm_dev_enter/exit and now I wonder
whether that was properly tested or not ...

I guess maybe we need a drm module option to disable this check, so that
we can exercise the code as if the ioctl has raced with hotunplug at the
worst possible moment.

As I pointed before, because of the complexity of the topic I prefer to
take it step by step and solve first for secondary device use case, not
for primary, display attached device.

Yeah makes sense. But then I think the right patch is to roll this out for
all drivers, properly justified with existing code. Not behind a driver
flag, because with all these different compositors the last thing we want
is a proliferation of driver-specific behaviour. That's imo the worst
option of all of them and needs to be avoided.

So this kind of patch would be acceptable to you if I unconditionally
scope the drm_ioctl with drm_dev_enter/exit without the driver flag ?
I am worried to break other drivers with this, see patch
https://cgit.freedesktop.org/~agrodzov/linux/commit/?h=drm-misc-next&id=f0c593f35b22ca5bf60ed9e7ce2bf2b80e6c68c6

Before setting drm_dev_unplug I go through a whole process of signalling
all possible fences in the system which some one some where might be
waiting on. My concern is that in the absence of HW those fences won't
signal and so unless I signal them myself src

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

2021-04-29 Thread Daniel Vetter

On Thu, Apr 29, 2021 at 12:04:33PM -0400, Andrey Grodzovsky wrote:
> 
> 
> On 2021-04-29 7:32 a.m., Daniel Vetter wrote:
> > On Thu, Apr 29, 2021 at 01:23:19PM +0200, Daniel Vetter wrote:
> > > On Wed, Apr 28, 2021 at 11:12:00AM -0400, Andrey Grodzovsky wrote:
> > > > With this calling drm_dev_unplug will flush and block
> > > > all in flight IOCTLs
> > > > 
> > > > Also, add feature such that if device supports graceful unplug
> > > > we enclose entire IOCTL in SRCU critical section.
> > > > 
> > > > Signed-off-by: Andrey Grodzovsky 
> > > 
> > > Nope.
> > > 
> > > The idea of drm_dev_enter/exit is to mark up hw access. Not entire ioctl.
> 
> Then I am confused why we have 
> https://elixir.bootlin.com/linux/v5.12/source/drivers/gpu/drm/drm_ioctl.c#L826
> currently in code ?

I forgot about this one, again. Thanks for reminding.

> > > Especially not with an opt-in flag so that it could be shrugged of as a
> > > driver hack. Most of these ioctls should have absolutely no problem
> > > working after hotunplug.
> > > 
> > > Also, doing this defeats the point since it pretty much guarantees
> > > userspace will die in assert()s and stuff. E.g. on i915 the rough contract
> > > is that only execbuf (and even that only when userspace has indicated
> > > support for non-recoverable hw ctx) is allowed to fail. Anything else
> > > might crash userspace.
> 
> Given that as I pointed above we already fail any IOCTls with -ENODEV
> when device is unplugged, it seems those crashes don't happen that
> often ? Also, in all my testing I don't think I saw a user space crash
> I could attribute to this.

I guess it should be ok.

My reasons for making this work is both less trouble for userspace (did
you test with various wayland compositors out there, not just amdgpu x86
driver?), but also testing.

We still need a bunch of these checks in various places or you'll wait a
very long time for a pending modeset or similar to complete. Being able to
run that code easily after hotunplug has completed should help a lot with
testing.

Plus various drivers already acquired drm_dev_enter/exit and now I wonder
whether that was properly tested or not ...

I guess maybe we need a drm module option to disable this check, so that
we can exercise the code as if the ioctl has raced with hotunplug at the
worst possible moment.

Also atomic is really tricky here: I assume your testing has just done
normal synchronous commits, but anything that goes through atomic can be
done nonblocking in a separate thread. Which the ioctl catch-all here wont
capture.

> > > You probably need similar (and very precisely defined) rules for amdgpu.
> > > And those must definitely exclude any shard ioctls from randomly failing
> > > with EIO, because that just kills the box and defeats the point of trying
> > > to gracefully handling hotunplug and making sure userspace has a chance of
> > > survival. E.g. for atomic everything should continue, including flip
> > > completion, but we set all outputs to "disconnected" and send out the
> > > uevent. Maybe crtc enabling can fail too, but that can also be handled
> > > through the async status we're using to signal DP link failures to
> > > userspace.
> 
> As I pointed before, because of the complexity of the topic I prefer to
> take it step by step and solve first for secondary device use case, not
> for primary, display attached device.

Yeah makes sense. But then I think the right patch is to roll this out for
all drivers, properly justified with existing code. Not behind a driver
flag, because with all these different compositors the last thing we want
is a proliferation of driver-specific behaviour. That's imo the worst
option of all of them and needs to be avoided.

Cheers, Daniel


> 
> > > 
> > > I guess we should clarify this in the hotunplug doc?
> 
> Agree
> 
> > 
> > To clarify: I'm not against throwing an ENODEV at userspace for ioctl that
> > really make no sense, and where we're rather confident that all properly
> > implemented userspace will gracefully handle failures. Like a modeset, or
> > opening a device, or trying to import a dma-buf or stuff like that which
> > can already fail in normal operation for any kind of reason.
> > 
> > But stuff that never fails, like GETRESOURCES ioctl, really shouldn't fail
> > after hotunplug.
> 
> As I pointed above, this a bit confuses me given that we already do
> blanker rejection of IOCTLs if device is unplugged.

Well I'm confused about this too :-/

> > And then there's the middle ground, like doing a pageflip or buffer flush,
> > which I guess some userspace might handle, but risky to inflict those
> > consequences on them. atomic modeset is especially fun since depending
> > what you're doing it can be both "failures expected" and "failures not
> > really expected in normal operation".
> > 
> > Also, this really should be consistent across drivers, not solved with a
> > driver flag for every possible combination.
> > 
> > If you look at the current hot

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

2021-04-29 Thread Andrey Grodzovsky




On 2021-04-29 12:29 p.m., Felix Kuehling wrote:

Am 2021-04-29 um 12:21 p.m. schrieb Andrey Grodzovsky:



On 2021-04-29 12:15 p.m., Felix Kuehling wrote:

Am 2021-04-29 um 12:04 p.m. schrieb Andrey Grodzovsky:

So as I understand your preferred approach is that I scope any
back_end, HW specific function with drm_dev_enter/exit because that
where MMIO
access takes place. But besides explicit MMIO access thorough
register accessors in the HW back-end there is also indirect MMIO
access
taking place throughout the code in the driver because of various VRAM
BOs which provide CPU access to VRAM through the VRAM BAR. This kind of
access is spread all over in the driver and even in mid-layers such as
TTM and not limited to HW back-end functions. It means it's much harder
to spot such places to surgically scope them with drm_dev_enter/exit
and
also that any new such code introduced will immediately break hot
unplug
because the developers can't be expected to remember making their code
robust to this specific use case. That why when we discussed internally
what approach to take to protecting code with drm_dev_enter/exit we
opted for using the widest available scope.


VRAM can also be mapped in user mode. Is there anything preventing user
mode from accessing the memory after unplug? I guess the best you could
do is unmap it from the CPU page table and let the application segfault
on the next access. Or replace the mapping with a dummy page in system
memory?


We indeed unmap but instead of letting it segfault insert dummy page on
the next page fault. See here
https://cgit.freedesktop.org/~agrodzov/linux/commit/?h=drm-misc-next&id=6dde3330ffa450e2e6da4d19e2fd0bb94b66b6ce
And I am aware that this doesn't take care of KFD user mapping.
As you know, we had some discussions with you on this topic and it's on
my TODO list to follow up on this to solve it for KFD too.


ROCm user mode maps VRAM BOs using render nodes. So I'd expect
ttm_bo_vm_dummy_page to work for KFD as well.

I guess we'd need something special for KFD's doorbell and MMIO (HDP
flush) mappings. Was that the discussion about the file address space?


Yes

Andrey



Regards,
   Felix




Andrey



Regards,
    Felix




Andrey

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

2021-04-29 Thread Felix Kuehling

Am 2021-04-29 um 12:21 p.m. schrieb Andrey Grodzovsky:
>
>
> On 2021-04-29 12:15 p.m., Felix Kuehling wrote:
>> Am 2021-04-29 um 12:04 p.m. schrieb Andrey Grodzovsky:
>>> So as I understand your preferred approach is that I scope any
>>> back_end, HW specific function with drm_dev_enter/exit because that
>>> where MMIO
>>> access takes place. But besides explicit MMIO access thorough
>>> register accessors in the HW back-end there is also indirect MMIO
>>> access
>>> taking place throughout the code in the driver because of various VRAM
>>> BOs which provide CPU access to VRAM through the VRAM BAR. This kind of
>>> access is spread all over in the driver and even in mid-layers such as
>>> TTM and not limited to HW back-end functions. It means it's much harder
>>> to spot such places to surgically scope them with drm_dev_enter/exit
>>> and
>>> also that any new such code introduced will immediately break hot
>>> unplug
>>> because the developers can't be expected to remember making their code
>>> robust to this specific use case. That why when we discussed internally
>>> what approach to take to protecting code with drm_dev_enter/exit we
>>> opted for using the widest available scope.
>>
>> VRAM can also be mapped in user mode. Is there anything preventing user
>> mode from accessing the memory after unplug? I guess the best you could
>> do is unmap it from the CPU page table and let the application segfault
>> on the next access. Or replace the mapping with a dummy page in system
>> memory?
>
> We indeed unmap but instead of letting it segfault insert dummy page on
> the next page fault. See here
> https://cgit.freedesktop.org/~agrodzov/linux/commit/?h=drm-misc-next&id=6dde3330ffa450e2e6da4d19e2fd0bb94b66b6ce
> And I am aware that this doesn't take care of KFD user mapping.
> As you know, we had some discussions with you on this topic and it's on
> my TODO list to follow up on this to solve it for KFD too.

ROCm user mode maps VRAM BOs using render nodes. So I'd expect
ttm_bo_vm_dummy_page to work for KFD as well.

I guess we'd need something special for KFD's doorbell and MMIO (HDP
flush) mappings. Was that the discussion about the file address space?

Regards,
  Felix


>
> Andrey
>
>>
>> Regards,
>>    Felix
>>
>>
>>>
>>> Andrey
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

2021-04-29 Thread Andrey Grodzovsky




On 2021-04-29 12:15 p.m., Felix Kuehling wrote:

Am 2021-04-29 um 12:04 p.m. schrieb Andrey Grodzovsky:

So as I understand your preferred approach is that I scope any
back_end, HW specific function with drm_dev_enter/exit because that
where MMIO
access takes place. But besides explicit MMIO access thorough
register accessors in the HW back-end there is also indirect MMIO access
taking place throughout the code in the driver because of various VRAM
BOs which provide CPU access to VRAM through the VRAM BAR. This kind of
access is spread all over in the driver and even in mid-layers such as
TTM and not limited to HW back-end functions. It means it's much harder
to spot such places to surgically scope them with drm_dev_enter/exit and
also that any new such code introduced will immediately break hot unplug
because the developers can't be expected to remember making their code
robust to this specific use case. That why when we discussed internally
what approach to take to protecting code with drm_dev_enter/exit we
opted for using the widest available scope.


VRAM can also be mapped in user mode. Is there anything preventing user
mode from accessing the memory after unplug? I guess the best you could
do is unmap it from the CPU page table and let the application segfault
on the next access. Or replace the mapping with a dummy page in system
memory?


We indeed unmap but instead of letting it segfault insert dummy page on
the next page fault. See here 
https://cgit.freedesktop.org/~agrodzov/linux/commit/?h=drm-misc-next&id=6dde3330ffa450e2e6da4d19e2fd0bb94b66b6ce

And I am aware that this doesn't take care of KFD user mapping.
As you know, we had some discussions with you on this topic and it's on
my TODO list to follow up on this to solve it for KFD too.

Andrey



Regards,
   Felix




Andrey

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

2021-04-29 Thread Felix Kuehling

Am 2021-04-29 um 12:04 p.m. schrieb Andrey Grodzovsky:
> So as I understand your preferred approach is that I scope any
> back_end, HW specific function with drm_dev_enter/exit because that
> where MMIO
> access takes place. But besides explicit MMIO access thorough
> register accessors in the HW back-end there is also indirect MMIO access
> taking place throughout the code in the driver because of various VRAM
> BOs which provide CPU access to VRAM through the VRAM BAR. This kind of
> access is spread all over in the driver and even in mid-layers such as
> TTM and not limited to HW back-end functions. It means it's much harder
> to spot such places to surgically scope them with drm_dev_enter/exit and
> also that any new such code introduced will immediately break hot unplug
> because the developers can't be expected to remember making their code
> robust to this specific use case. That why when we discussed internally
> what approach to take to protecting code with drm_dev_enter/exit we
> opted for using the widest available scope.

VRAM can also be mapped in user mode. Is there anything preventing user
mode from accessing the memory after unplug? I guess the best you could
do is unmap it from the CPU page table and let the application segfault
on the next access. Or replace the mapping with a dummy page in system
memory?

Regards,
  Felix


>
> Andrey 
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

2021-04-29 Thread Andrey Grodzovsky





On 2021-04-29 7:32 a.m., Daniel Vetter wrote:

On Thu, Apr 29, 2021 at 01:23:19PM +0200, Daniel Vetter wrote:

On Wed, Apr 28, 2021 at 11:12:00AM -0400, Andrey Grodzovsky wrote:

With this calling drm_dev_unplug will flush and block
all in flight IOCTLs

Also, add feature such that if device supports graceful unplug
we enclose entire IOCTL in SRCU critical section.

Signed-off-by: Andrey Grodzovsky 


Nope.

The idea of drm_dev_enter/exit is to mark up hw access. Not entire ioctl.


Then I am confused why we have 
https://elixir.bootlin.com/linux/v5.12/source/drivers/gpu/drm/drm_ioctl.c#L826

currently in code ?



Especially not with an opt-in flag so that it could be shrugged of as a
driver hack. Most of these ioctls should have absolutely no problem
working after hotunplug.

Also, doing this defeats the point since it pretty much guarantees
userspace will die in assert()s and stuff. E.g. on i915 the rough contract
is that only execbuf (and even that only when userspace has indicated
support for non-recoverable hw ctx) is allowed to fail. Anything else
might crash userspace.


Given that as I pointed above we already fail any IOCTls with -ENODEV
when device is unplugged, it seems those crashes don't happen that
often ? Also, in all my testing I don't think I saw a user space crash
I could attribute to this.



You probably need similar (and very precisely defined) rules for amdgpu.
And those must definitely exclude any shard ioctls from randomly failing
with EIO, because that just kills the box and defeats the point of trying
to gracefully handling hotunplug and making sure userspace has a chance of
survival. E.g. for atomic everything should continue, including flip
completion, but we set all outputs to "disconnected" and send out the
uevent. Maybe crtc enabling can fail too, but that can also be handled
through the async status we're using to signal DP link failures to
userspace.


As I pointed before, because of the complexity of the topic I prefer to
take it step by step and solve first for secondary device use case, not
for primary, display attached device.



I guess we should clarify this in the hotunplug doc?


Agree



To clarify: I'm not against throwing an ENODEV at userspace for ioctl that
really make no sense, and where we're rather confident that all properly
implemented userspace will gracefully handle failures. Like a modeset, or
opening a device, or trying to import a dma-buf or stuff like that which
can already fail in normal operation for any kind of reason.

But stuff that never fails, like GETRESOURCES ioctl, really shouldn't fail
after hotunplug.


As I pointed above, this a bit confuses me given that we already do
blanker rejection of IOCTLs if device is unplugged.




And then there's the middle ground, like doing a pageflip or buffer flush,
which I guess some userspace might handle, but risky to inflict those
consequences on them. atomic modeset is especially fun since depending
what you're doing it can be both "failures expected" and "failures not
really expected in normal operation".

Also, this really should be consistent across drivers, not solved with a
driver flag for every possible combination.

If you look at the current hotunplug kms drivers, they have
drm_dev_enter/exit sprinkled in specific hw callback functions because of
the above problems. But maybe it makes sense to change things in a few
cases. But then we should do it across the board.


So as I understand your preferred approach is that I scope any back_end, 
HW specific function with drm_dev_enter/exit because that where MMIO

access takes place. But besides explicit MMIO access thorough
register accessors in the HW back-end there is also indirect MMIO access
taking place throughout the code in the driver because of various VRAM
BOs which provide CPU access to VRAM through the VRAM BAR. This kind of
access is spread all over in the driver and even in mid-layers such as
TTM and not limited to HW back-end functions. It means it's much harder
to spot such places to surgically scope them with drm_dev_enter/exit and
also that any new such code introduced will immediately break hot unplug
because the developers can't be expected to remember making their code
robust to this specific use case. That why when we discussed internally
what approach to take to protecting code with drm_dev_enter/exit we
opted for using the widest available scope.

Andrey



Cheers, Daniel


___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

2021-04-29 Thread Daniel Vetter

On Thu, Apr 29, 2021 at 01:23:19PM +0200, Daniel Vetter wrote:
> On Wed, Apr 28, 2021 at 11:12:00AM -0400, Andrey Grodzovsky wrote:
> > With this calling drm_dev_unplug will flush and block
> > all in flight IOCTLs
> > 
> > Also, add feature such that if device supports graceful unplug
> > we enclose entire IOCTL in SRCU critical section.
> > 
> > Signed-off-by: Andrey Grodzovsky 
> 
> Nope.
> 
> The idea of drm_dev_enter/exit is to mark up hw access. Not entire ioctl.
> 
> Especially not with an opt-in flag so that it could be shrugged of as a
> driver hack. Most of these ioctls should have absolutely no problem
> working after hotunplug.
> 
> Also, doing this defeats the point since it pretty much guarantees
> userspace will die in assert()s and stuff. E.g. on i915 the rough contract
> is that only execbuf (and even that only when userspace has indicated
> support for non-recoverable hw ctx) is allowed to fail. Anything else
> might crash userspace.
> 
> You probably need similar (and very precisely defined) rules for amdgpu.
> And those must definitely exclude any shard ioctls from randomly failing
> with EIO, because that just kills the box and defeats the point of trying
> to gracefully handling hotunplug and making sure userspace has a chance of
> survival. E.g. for atomic everything should continue, including flip
> completion, but we set all outputs to "disconnected" and send out the
> uevent. Maybe crtc enabling can fail too, but that can also be handled
> through the async status we're using to signal DP link failures to
> userspace.
> 
> I guess we should clarify this in the hotunplug doc?

To clarify: I'm not against throwing an ENODEV at userspace for ioctl that
really make no sense, and where we're rather confident that all properly
implemented userspace will gracefully handle failures. Like a modeset, or
opening a device, or trying to import a dma-buf or stuff like that which
can already fail in normal operation for any kind of reason.

But stuff that never fails, like GETRESOURCES ioctl, really shouldn't fail
after hotunplug.

And then there's the middle ground, like doing a pageflip or buffer flush,
which I guess some userspace might handle, but risky to inflict those
consequences on them. atomic modeset is especially fun since depending
what you're doing it can be both "failures expected" and "failures not
really expected in normal operation".

Also, this really should be consistent across drivers, not solved with a
driver flag for every possible combination.

If you look at the current hotunplug kms drivers, they have
drm_dev_enter/exit sprinkled in specific hw callback functions because of
the above problems. But maybe it makes sense to change things in a few
cases. But then we should do it across the board.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

2021-04-29 Thread Daniel Vetter

On Wed, Apr 28, 2021 at 11:12:00AM -0400, Andrey Grodzovsky wrote:
> With this calling drm_dev_unplug will flush and block
> all in flight IOCTLs
> 
> Also, add feature such that if device supports graceful unplug
> we enclose entire IOCTL in SRCU critical section.
> 
> Signed-off-by: Andrey Grodzovsky 

Nope.

The idea of drm_dev_enter/exit is to mark up hw access. Not entire ioctl.

Especially not with an opt-in flag so that it could be shrugged of as a
driver hack. Most of these ioctls should have absolutely no problem
working after hotunplug.

Also, doing this defeats the point since it pretty much guarantees
userspace will die in assert()s and stuff. E.g. on i915 the rough contract
is that only execbuf (and even that only when userspace has indicated
support for non-recoverable hw ctx) is allowed to fail. Anything else
might crash userspace.

You probably need similar (and very precisely defined) rules for amdgpu.
And those must definitely exclude any shard ioctls from randomly failing
with EIO, because that just kills the box and defeats the point of trying
to gracefully handling hotunplug and making sure userspace has a chance of
survival. E.g. for atomic everything should continue, including flip
completion, but we set all outputs to "disconnected" and send out the
uevent. Maybe crtc enabling can fail too, but that can also be handled
through the async status we're using to signal DP link failures to
userspace.

I guess we should clarify this in the hotunplug doc?

Cheers, Daniel

> ---
>  drivers/gpu/drm/drm_ioctl.c | 15 +--
>  include/drm/drm_drv.h   |  6 ++
>  2 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_ioctl.c b/drivers/gpu/drm/drm_ioctl.c
> index d273d1a8603a..5882ef2183bb 100644
> --- a/drivers/gpu/drm/drm_ioctl.c
> +++ b/drivers/gpu/drm/drm_ioctl.c
> @@ -815,7 +815,7 @@ long drm_ioctl(struct file *filp,
>   const struct drm_ioctl_desc *ioctl = NULL;
>   drm_ioctl_t *func;
>   unsigned int nr = DRM_IOCTL_NR(cmd);
> - int retcode = -EINVAL;
> + int idx, retcode = -EINVAL;
>   char stack_kdata[128];
>   char *kdata = NULL;
>   unsigned int in_size, out_size, drv_size, ksize;
> @@ -884,7 +884,18 @@ long drm_ioctl(struct file *filp,
>   if (ksize > in_size)
>   memset(kdata + in_size, 0, ksize - in_size);
>  
> - retcode = drm_ioctl_kernel(filp, func, kdata, ioctl->flags);
> + if (drm_core_check_feature(dev, DRIVER_HOTUNPLUG_SUPPORT)) {
> + if (drm_dev_enter(dev, &idx)) {
> + retcode = drm_ioctl_kernel(filp, func, kdata, 
> ioctl->flags);
> + drm_dev_exit(idx);
> + } else {
> + retcode = -ENODEV;
> + goto err_i1;
> + }
> + } else {
> + retcode = drm_ioctl_kernel(filp, func, kdata, ioctl->flags);
> + }
> +
>   if (copy_to_user((void __user *)arg, kdata, out_size) != 0)
>   retcode = -EFAULT;
>  
> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> index b439ae1921b8..63e05cec46c1 100644
> --- a/include/drm/drm_drv.h
> +++ b/include/drm/drm_drv.h
> @@ -94,6 +94,12 @@ enum drm_driver_feature {
>* synchronization of command submission.
>*/
>   DRIVER_SYNCOBJ_TIMELINE = BIT(6),
> + /**
> +  * @DRIVER_NO_HOTUNPLUG_SUPPORT:
> +  *
> +  * Driver support gracefull remove.
> +  */
> + DRIVER_HOTUNPLUG_SUPPORT = BIT(7),
>  
>   /* IMPORTANT: Below are all the legacy flags, add new ones above. */
>  
> -- 
> 2.25.1
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

Re: [PATCH v5 20/27] drm: Scope all DRM IOCTLs with drm_dev_enter/exit

19 matches

Site Navigation

Mail list logo

Footer information