dri-devel Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] drm/doc: device hot-unplug for userspace
@ 2020-05-19 10:06 Pekka Paalanen
  2020-05-19 14:37 ` Andrey Grodzovsky
                   ` (3 more replies)
  0 siblings, 4 replies; 30+ messages in thread
From: Pekka Paalanen @ 2020-05-19 10:06 UTC (permalink / raw)
  To: dri-devel; +Cc: Sean Paul, Pekka Paalanen, Dave Airlie

From: Pekka Paalanen <pekka.paalanen@collabora.com>

Set up the expectations on how hot-unplugging a DRM device should look like to
userspace.

Written by Daniel Vetter's request and largely based on his comments in IRC and
from https://lists.freedesktop.org/archives/dri-devel/2020-May/265484.html .

Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Sean Paul <sean@poorly.run>

---

Disclaimer: I am a userspace developer writing for other userspace developers.
I took some liberties in defining what should happen without knowing what is
actually possible or what existing drivers already implement.
---
 Documentation/gpu/drm-uapi.rst | 75 ++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
index 56fec6ed1ad8..80db4abd2cbd 100644
--- a/Documentation/gpu/drm-uapi.rst
+++ b/Documentation/gpu/drm-uapi.rst
@@ -1,3 +1,5 @@
+.. Copyright 2020 DisplayLink (UK) Ltd.
+
 ===================
 Userland interfaces
 ===================
@@ -162,6 +164,79 @@ other hand, a driver requires shared state between clients which is
 visible to user-space and accessible beyond open-file boundaries, they
 cannot support render nodes.
 
+Device Hot-Unplug
+=================
+
+.. note::
+   The following is the plan. Implementation is not there yet
+   (2020 May 13).
+
+Graphics devices (display and/or render) may be connected via USB (e.g.
+display adapters or docking stations) or Thunderbolt (e.g. eGPU). An end
+user is able to hot-unplug this kind of devices while they are being
+used, and expects that the very least the machine does not crash. Any
+damage from hot-unplugging a DRM device needs to be limited as much as
+possible and userspace must be given the chance to handle it if it wants
+to. Ideally, unplugging a DRM device still lets a desktop to continue
+running, but that is going to need explicit support throughout the whole
+graphics stack: from kernel and userspace drivers, through display
+servers, via window system protocols, and in applications and libraries.
+
+Other scenarios that should lead to the same are: unrecoverable GPU
+crash, PCI device disappearing off the bus, or forced unbind of a driver
+from the physical device.
+
+In other words, from userspace perspective everything needs to keep on
+working more or less, until userspace stops using the disappeared DRM
+device and closes it completely. Userspace will learn of the device
+disappearance from the device removed uevent or in some cases specific
+ioctls returning EIO.
+
+This goal raises at least the following requirements for the kernel and
+drivers:
+
+- The kernel must not hang, crash or oops, no matter what userspace was
+  in the middle of doing when the device disappeared.
+
+- All GPU jobs that can no longer run must have their fences
+  force-signalled to avoid inflicting hangs to userspace.
+
+- KMS connectors must change their status to disconnected.
+
+- Legacy modesets and pageflips fake success.
+
+- Atomic commits, both real and TEST_ONLY, fake success.
+
+- Pending non-blocking KMS operations deliver the DRM events userspace
+  is expecting.
+
+- If underlying memory disappears, the mmaps are replaced with harmless
+  zero pages where access does not raise SIGBUS. Reads return zeros,
+  writes are ignored.
+
+- dmabuf which point to memory that has disappeared are rewritten to
+  point to harmless zero pages, similar to mmaps. Imports still succeed
+  both ways: an existing device importing a dmabuf pointing to
+  disappeared memory, and a disappeared device importing any dmabuf.
+
+- Render ioctls return EIO which is then handled in userspace drivers,
+  e.g. Mesa, to have the device disappearance handled in the way
+  specified for each API (OpenGL, GL ES: GL_KHR_robustness;
+  Vulkan: VK_ERROR_DEVICE_LOST; etc.)
+
+Raising SIGBUS is not an option, because userspace cannot realistically
+handle it.  Signal handlers are global, which makes them extremely
+difficult to use correctly from libraries like Mesa produces. Signal
+handlers are not composable, you can't have different handlers for GPU1
+and GPU2 from different vendors, and a third handler for mmapped regular
+files.  Threads cause additional pain with signal handling as well.
+
+Only after userspace has closed all relevant DRM device and dmabuf file
+descriptors and removed all mmaps, the DRM driver can tear down its
+instance for the device that no longer exists. If the same physical
+device somehow comes back in the mean time, it shall be a new DRM
+device.
+
 .. _drm_driver_ioctl:
 
 IOCTL Support on Device Nodes
-- 
2.20.1

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] drm/doc: device hot-unplug for userspace
  2020-05-19 10:06 [PATCH] drm/doc: device hot-unplug for userspace Pekka Paalanen
@ 2020-05-19 14:37 ` Andrey Grodzovsky
  2020-05-20 11:19   ` Pekka Paalanen
  2020-05-20 12:55 ` Daniel Vetter
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 30+ messages in thread
From: Andrey Grodzovsky @ 2020-05-19 14:37 UTC (permalink / raw)
  To: Pekka Paalanen, dri-devel; +Cc: Sean Paul, Dave Airlie, Pekka Paalanen

Thanks for the summary, does put things in order and makes it easier to 
comprehend all the TODOs, some questions bellow

On 5/19/20 6:06 AM, Pekka Paalanen wrote:
> From: Pekka Paalanen <pekka.paalanen@collabora.com>
>
> Set up the expectations on how hot-unplugging a DRM device should look like to
> userspace.
>
> Written by Daniel Vetter's request and largely based on his comments in IRC and
> from https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Fdri-devel%2F2020-May%2F265484.html&amp;data=02%7C01%7Candrey.grodzovsky%40amd.com%7Ce8e13dc4c85648e5fcd408d7fbdc5f2b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637254796242596783&amp;sdata=bA%2FAy3VGvzNqmV1kGigNROSZQEws2E98JibDxvEICNs%3D&amp;reserved=0 .
>
> Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Sean Paul <sean@poorly.run>
>
> ---
>
> Disclaimer: I am a userspace developer writing for other userspace developers.
> I took some liberties in defining what should happen without knowing what is
> actually possible or what existing drivers already implement.
> ---
>   Documentation/gpu/drm-uapi.rst | 75 ++++++++++++++++++++++++++++++++++
>   1 file changed, 75 insertions(+)
>
> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> index 56fec6ed1ad8..80db4abd2cbd 100644
> --- a/Documentation/gpu/drm-uapi.rst
> +++ b/Documentation/gpu/drm-uapi.rst
> @@ -1,3 +1,5 @@
> +.. Copyright 2020 DisplayLink (UK) Ltd.
> +
>   ===================
>   Userland interfaces
>   ===================
> @@ -162,6 +164,79 @@ other hand, a driver requires shared state between clients which is
>   visible to user-space and accessible beyond open-file boundaries, they
>   cannot support render nodes.
>   
> +Device Hot-Unplug
> +=================
> +
> +.. note::
> +   The following is the plan. Implementation is not there yet
> +   (2020 May 13).
> +
> +Graphics devices (display and/or render) may be connected via USB (e.g.
> +display adapters or docking stations) or Thunderbolt (e.g. eGPU). An end
> +user is able to hot-unplug this kind of devices while they are being
> +used, and expects that the very least the machine does not crash. Any
> +damage from hot-unplugging a DRM device needs to be limited as much as
> +possible and userspace must be given the chance to handle it if it wants
> +to. Ideally, unplugging a DRM device still lets a desktop to continue
> +running, but that is going to need explicit support throughout the whole
> +graphics stack: from kernel and userspace drivers, through display
> +servers, via window system protocols, and in applications and libraries.
> +
> +Other scenarios that should lead to the same are: unrecoverable GPU
> +crash, PCI device disappearing off the bus, or forced unbind of a driver
> +from the physical device.
> +
> +In other words, from userspace perspective everything needs to keep on
> +working more or less, until userspace stops using the disappeared DRM
> +device and closes it completely. Userspace will learn of the device
> +disappearance from the device removed uevent or in some cases specific
> +ioctls returning EIO.
> +
> +This goal raises at least the following requirements for the kernel and
> +drivers:
> +
> +- The kernel must not hang, crash or oops, no matter what userspace was
> +  in the middle of doing when the device disappeared.
> +
> +- All GPU jobs that can no longer run must have their fences
> +  force-signalled to avoid inflicting hangs to userspace.
> +
> +- KMS connectors must change their status to disconnected.
> +
> +- Legacy modesets and pageflips fake success.
> +
> +- Atomic commits, both real and TEST_ONLY, fake success.


Why wouldn't we return -EIO for the atommic commit IOTCL/legasy pflip 
and modeset ioctls here same way as you suggested returning -EIO for 
render ioctl ?

> +
> +- Pending non-blocking KMS operations deliver the DRM events userspace
> +  is expecting.
> +
> +- If underlying memory disappears, the mmaps are replaced with harmless
> +  zero pages where access does not raise SIGBUS. Reads return zeros,
> +  writes are ignored.
> +
> +- dmabuf which point to memory that has disappeared are rewritten to
> +  point to harmless zero pages, similar to mmaps. Imports still succeed
> +  both ways: an existing device importing a dmabuf pointing to
> +  disappeared memory, and a disappeared device importing any dmabuf.


Same as above, I would expect drmPrimeHandleToFD and drmPrimeFDToHandle 
to return -EIO in case device is detached


> +
> +- Render ioctls return EIO which is then handled in userspace drivers,
> +  e.g. Mesa, to have the device disappearance handled in the way
> +  specified for each API (OpenGL, GL ES: GL_KHR_robustness;
> +  Vulkan: VK_ERROR_DEVICE_LOST; etc.)


As far as I understood from our discussion we would reject any IOCTL 
aimed at a device which is gone and not only render ioctls.

Andrey


> +
> +Raising SIGBUS is not an option, because userspace cannot realistically
> +handle it.  Signal handlers are global, which makes them extremely
> +difficult to use correctly from libraries like Mesa produces. Signal
> +handlers are not composable, you can't have different handlers for GPU1
> +and GPU2 from different vendors, and a third handler for mmapped regular
> +files.  Threads cause additional pain with signal handling as well.
> +
> +Only after userspace has closed all relevant DRM device and dmabuf file
> +descriptors and removed all mmaps, the DRM driver can tear down its
> +instance for the device that no longer exists. If the same physical
> +device somehow comes back in the mean time, it shall be a new DRM
> +device.
> +
>   .. _drm_driver_ioctl:
>   
>   IOCTL Support on Device Nodes
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] drm/doc: device hot-unplug for userspace
  2020-05-19 14:37 ` Andrey Grodzovsky
@ 2020-05-20 11:19   ` Pekka Paalanen
  2020-05-20 12:46     ` Daniel Vetter
  0 siblings, 1 reply; 30+ messages in thread
From: Pekka Paalanen @ 2020-05-20 11:19 UTC (permalink / raw)
  To: Andrey Grodzovsky, Daniel Vetter; +Cc: Dave Airlie, Sean Paul, dri-devel

[-- Attachment #1.1: Type: text/plain, Size: 10240 bytes --]

On Tue, 19 May 2020 10:37:12 -0400
Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> wrote:

> Thanks for the summary, does put things in order and makes it easier to 
> comprehend all the TODOs, some questions bellow
> 
> On 5/19/20 6:06 AM, Pekka Paalanen wrote:
> > From: Pekka Paalanen <pekka.paalanen@collabora.com>
> >
> > Set up the expectations on how hot-unplugging a DRM device should look like to
> > userspace.
> >
> > Written by Daniel Vetter's request and largely based on his comments in IRC and
> > from https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Fdri-devel%2F2020-May%2F265484.html&amp;data=02%7C01%7Candrey.grodzovsky%40amd.com%7Ce8e13dc4c85648e5fcd408d7fbdc5f2b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637254796242596783&amp;sdata=bA%2FAy3VGvzNqmV1kGigNROSZQEws2E98JibDxvEICNs%3D&amp;reserved=0 .
> >
> > Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> > Cc: Dave Airlie <airlied@redhat.com>
> > Cc: Sean Paul <sean@poorly.run>
> >
> > ---
> >
> > Disclaimer: I am a userspace developer writing for other userspace developers.
> > I took some liberties in defining what should happen without knowing what is
> > actually possible or what existing drivers already implement.
> > ---
> >   Documentation/gpu/drm-uapi.rst | 75 ++++++++++++++++++++++++++++++++++
> >   1 file changed, 75 insertions(+)
> >
> > diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> > index 56fec6ed1ad8..80db4abd2cbd 100644
> > --- a/Documentation/gpu/drm-uapi.rst
> > +++ b/Documentation/gpu/drm-uapi.rst
> > @@ -1,3 +1,5 @@
> > +.. Copyright 2020 DisplayLink (UK) Ltd.
> > +
> >   ===================
> >   Userland interfaces
> >   ===================
> > @@ -162,6 +164,79 @@ other hand, a driver requires shared state between clients which is
> >   visible to user-space and accessible beyond open-file boundaries, they
> >   cannot support render nodes.
> >   
> > +Device Hot-Unplug
> > +=================
> > +
> > +.. note::
> > +   The following is the plan. Implementation is not there yet
> > +   (2020 May 13).
> > +
> > +Graphics devices (display and/or render) may be connected via USB (e.g.
> > +display adapters or docking stations) or Thunderbolt (e.g. eGPU). An end
> > +user is able to hot-unplug this kind of devices while they are being
> > +used, and expects that the very least the machine does not crash. Any
> > +damage from hot-unplugging a DRM device needs to be limited as much as
> > +possible and userspace must be given the chance to handle it if it wants
> > +to. Ideally, unplugging a DRM device still lets a desktop to continue
> > +running, but that is going to need explicit support throughout the whole
> > +graphics stack: from kernel and userspace drivers, through display
> > +servers, via window system protocols, and in applications and libraries.
> > +
> > +Other scenarios that should lead to the same are: unrecoverable GPU
> > +crash, PCI device disappearing off the bus, or forced unbind of a driver
> > +from the physical device.
> > +
> > +In other words, from userspace perspective everything needs to keep on
> > +working more or less, until userspace stops using the disappeared DRM
> > +device and closes it completely. Userspace will learn of the device
> > +disappearance from the device removed uevent or in some cases specific
> > +ioctls returning EIO.
> > +
> > +This goal raises at least the following requirements for the kernel and
> > +drivers:
> > +
> > +- The kernel must not hang, crash or oops, no matter what userspace was
> > +  in the middle of doing when the device disappeared.
> > +
> > +- All GPU jobs that can no longer run must have their fences
> > +  force-signalled to avoid inflicting hangs to userspace.
> > +
> > +- KMS connectors must change their status to disconnected.
> > +
> > +- Legacy modesets and pageflips fake success.
> > +
> > +- Atomic commits, both real and TEST_ONLY, fake success.  
> 
> 
> Why wouldn't we return -EIO for the atommic commit IOTCL/legasy pflip 
> and modeset ioctls here same way as you suggested returning -EIO for 
> render ioctl ?

Hi,

that is more of a question for Daniel Vetter than me. I believe he is
worried that userspace will get the error handling horribly wrong
anyway, because it needs to be handled in every single display server
project. Render ioctl errors OTOH need to be handled only in the
corresponding Mesa or other userspace driver, and for render there are
API (OpenGL/EGL, Vulkan) specs that say how it must be handled to fill
the API contract. Because of the API contract, those are more likely to
have reasonable error handling in place already.

I first thought it would be obvious for at least atomic commits to fail
appropriately, but then thinking again, it will only introduce new
failures that are currently very hard to test for (VKMS to the rescue),
and userspace would need to have code to correctly bail out for EIO
rather than attempt fallbacks. The uevent telling the device is gone is
going to come anyway. In between the device disappearance and uevent
handling, it doesn't matter much what happens: the device is abruptly
gone and the user will see a glitch anyway - he knows, he just yanked
out the cable.

To me this decision is just to make life in userspace easier by not
introducing a whole new type of error. However, if the kernel did
return EIO instead, it would be impossible for me to see that as a
kernel regression because the old behaviour was probably explosions
everywhere anyway.

I heard that UDL and maybe some other drivers in the kernel already
handle hot-unplug somehow. Maybe those are what would regress if the
kernel returned EIO?


> > +
> > +- Pending non-blocking KMS operations deliver the DRM events userspace
> > +  is expecting.
> > +
> > +- If underlying memory disappears, the mmaps are replaced with harmless
> > +  zero pages where access does not raise SIGBUS. Reads return zeros,
> > +  writes are ignored.
> > +
> > +- dmabuf which point to memory that has disappeared are rewritten to
> > +  point to harmless zero pages, similar to mmaps. Imports still succeed
> > +  both ways: an existing device importing a dmabuf pointing to
> > +  disappeared memory, and a disappeared device importing any dmabuf.  
> 
> 
> Same as above, I would expect drmPrimeHandleToFD and drmPrimeFDToHandle 
> to return -EIO in case device is detached

I imagined a use case behind this: a Wayland compositor attempting to
import a client's dmabuf. The GPU the client was using is
hot-unplugged, meaning the dmabufs lose their storage. It is up to the
client to handle its rendering GPU disappearance on its own, but also
the compositor should not disconnect it because the GPU disappeared.
It's not the client's fault the GPU disappeared.

In Wayland, failure to use a wl_buffer is considered a protocol error,
and protocol errors are always fatal: the client is disconnected. The
rationale is that the compositor must always be able to present the
client buffer somehow. If the compositor cannot, then the client did
not obey the protocol.

The fallback presentation path in a compositor is usually importing the
dmabuf to EGL, to be sampled from OpenGL. Normally the protocol
guarantees that this works, so any failure to do so is a protocol
violation. But if the GPU used by the client suddenly disappears and
the imports start to fail, that is interpreted as a protocol violation
unless the compositor can see why the import failed. Since the import
is done via EGL, getting the right error code plumbed through from
libdrm functions to the EGL caller would be a hassle. I don't see any
error code in EGL_EXT_image_dma_buf_import reserved for "the dmabuf
storage was hot-unplugged", and I doubt there is anything exclusively
for only that in the EGL base spec either.

The cost of lying that the import worked is that the compositor will
paint black or transparent where the window was supposed to be. It's a
graphical glitch that is contrary to the Wayland design principles, but
in this case a glitch is unavoidable: even if the compositor knew this
buffer is now bad, what would it paint instead? It has nothing else to
paint from. I'm assuming the compositor is using a different GPU than
what disappeared.

Ideally, the client will eventually react to losing the GPU and either
crash, quit, or switch its rendering to something that works which
simply gives the compositor a new, working buffer without losing any
window state in the process. If we risk the compositor disconnecting
the client, then the client might not recover even if it wanted to.

> > +
> > +- Render ioctls return EIO which is then handled in userspace drivers,
> > +  e.g. Mesa, to have the device disappearance handled in the way
> > +  specified for each API (OpenGL, GL ES: GL_KHR_robustness;
> > +  Vulkan: VK_ERROR_DEVICE_LOST; etc.)  
> 
> 
> As far as I understood from our discussion we would reject any IOCTL 
> aimed at a device which is gone and not only render ioctls.

Daniel?


Thanks,
pq

> 
> Andrey
> 
> 
> > +
> > +Raising SIGBUS is not an option, because userspace cannot realistically
> > +handle it.  Signal handlers are global, which makes them extremely
> > +difficult to use correctly from libraries like Mesa produces. Signal
> > +handlers are not composable, you can't have different handlers for GPU1
> > +and GPU2 from different vendors, and a third handler for mmapped regular
> > +files.  Threads cause additional pain with signal handling as well.
> > +
> > +Only after userspace has closed all relevant DRM device and dmabuf file
> > +descriptors and removed all mmaps, the DRM driver can tear down its
> > +instance for the device that no longer exists. If the same physical
> > +device somehow comes back in the mean time, it shall be a new DRM
> > +device.
> > +
> >   .. _drm_driver_ioctl:
> >   
> >   IOCTL Support on Device Nodes  


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] drm/doc: device hot-unplug for userspace
  2020-05-20 11:19   ` Pekka Paalanen
@ 2020-05-20 12:46     ` Daniel Vetter
  2020-05-20 14:50       ` Andrey Grodzovsky
  0 siblings, 1 reply; 30+ messages in thread
From: Daniel Vetter @ 2020-05-20 12:46 UTC (permalink / raw)
  To: Pekka Paalanen; +Cc: Sean Paul, dri-devel, Dave Airlie

On Wed, May 20, 2020 at 02:19:08PM +0300, Pekka Paalanen wrote:
> On Tue, 19 May 2020 10:37:12 -0400
> Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> wrote:
> 
> > Thanks for the summary, does put things in order and makes it easier to 
> > comprehend all the TODOs, some questions bellow
> > 
> > On 5/19/20 6:06 AM, Pekka Paalanen wrote:
> > > From: Pekka Paalanen <pekka.paalanen@collabora.com>
> > >
> > > Set up the expectations on how hot-unplugging a DRM device should look like to
> > > userspace.
> > >
> > > Written by Daniel Vetter's request and largely based on his comments in IRC and
> > > from https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Fdri-devel%2F2020-May%2F265484.html&amp;data=02%7C01%7Candrey.grodzovsky%40amd.com%7Ce8e13dc4c85648e5fcd408d7fbdc5f2b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637254796242596783&amp;sdata=bA%2FAy3VGvzNqmV1kGigNROSZQEws2E98JibDxvEICNs%3D&amp;reserved=0 .
> > >
> > > Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> > > Cc: Dave Airlie <airlied@redhat.com>
> > > Cc: Sean Paul <sean@poorly.run>
> > >
> > > ---
> > >
> > > Disclaimer: I am a userspace developer writing for other userspace developers.
> > > I took some liberties in defining what should happen without knowing what is
> > > actually possible or what existing drivers already implement.
> > > ---
> > >   Documentation/gpu/drm-uapi.rst | 75 ++++++++++++++++++++++++++++++++++
> > >   1 file changed, 75 insertions(+)
> > >
> > > diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> > > index 56fec6ed1ad8..80db4abd2cbd 100644
> > > --- a/Documentation/gpu/drm-uapi.rst
> > > +++ b/Documentation/gpu/drm-uapi.rst
> > > @@ -1,3 +1,5 @@
> > > +.. Copyright 2020 DisplayLink (UK) Ltd.
> > > +
> > >   ===================
> > >   Userland interfaces
> > >   ===================
> > > @@ -162,6 +164,79 @@ other hand, a driver requires shared state between clients which is
> > >   visible to user-space and accessible beyond open-file boundaries, they
> > >   cannot support render nodes.
> > >   
> > > +Device Hot-Unplug
> > > +=================
> > > +
> > > +.. note::
> > > +   The following is the plan. Implementation is not there yet
> > > +   (2020 May 13).
> > > +
> > > +Graphics devices (display and/or render) may be connected via USB (e.g.
> > > +display adapters or docking stations) or Thunderbolt (e.g. eGPU). An end
> > > +user is able to hot-unplug this kind of devices while they are being
> > > +used, and expects that the very least the machine does not crash. Any
> > > +damage from hot-unplugging a DRM device needs to be limited as much as
> > > +possible and userspace must be given the chance to handle it if it wants
> > > +to. Ideally, unplugging a DRM device still lets a desktop to continue
> > > +running, but that is going to need explicit support throughout the whole
> > > +graphics stack: from kernel and userspace drivers, through display
> > > +servers, via window system protocols, and in applications and libraries.
> > > +
> > > +Other scenarios that should lead to the same are: unrecoverable GPU
> > > +crash, PCI device disappearing off the bus, or forced unbind of a driver
> > > +from the physical device.
> > > +
> > > +In other words, from userspace perspective everything needs to keep on
> > > +working more or less, until userspace stops using the disappeared DRM
> > > +device and closes it completely. Userspace will learn of the device
> > > +disappearance from the device removed uevent or in some cases specific
> > > +ioctls returning EIO.
> > > +
> > > +This goal raises at least the following requirements for the kernel and
> > > +drivers:
> > > +
> > > +- The kernel must not hang, crash or oops, no matter what userspace was
> > > +  in the middle of doing when the device disappeared.
> > > +
> > > +- All GPU jobs that can no longer run must have their fences
> > > +  force-signalled to avoid inflicting hangs to userspace.
> > > +
> > > +- KMS connectors must change their status to disconnected.
> > > +
> > > +- Legacy modesets and pageflips fake success.
> > > +
> > > +- Atomic commits, both real and TEST_ONLY, fake success.  
> > 
> > 
> > Why wouldn't we return -EIO for the atommic commit IOTCL/legasy pflip 
> > and modeset ioctls here same way as you suggested returning -EIO for 
> > render ioctl ?
> 
> Hi,
> 
> that is more of a question for Daniel Vetter than me. I believe he is
> worried that userspace will get the error handling horribly wrong
> anyway, because it needs to be handled in every single display server
> project. Render ioctl errors OTOH need to be handled only in the
> corresponding Mesa or other userspace driver, and for render there are
> API (OpenGL/EGL, Vulkan) specs that say how it must be handled to fill
> the API contract. Because of the API contract, those are more likely to
> have reasonable error handling in place already.

Yup pretty much. Atm compositors expect an -EINVAL (especially for
TEST_ONLY), some cope with the semi-spurious -EBUSY we still throw around,
but I expect most will just die if the get an -ENOMEM or -EIO or really
anything.

I think one area where we've historically thrown some spurious errors is
also vblank ioctls, since they don't take full locks and sometimes the
kernel needs to sneak in a modeset for some reason (gpu recovery, dp link
recovery, ...).

Either way I don't think there's anything the compositor can do than just
ignore the error and carry on.

> I first thought it would be obvious for at least atomic commits to fail
> appropriately, but then thinking again, it will only introduce new
> failures that are currently very hard to test for (VKMS to the rescue),
> and userspace would need to have code to correctly bail out for EIO
> rather than attempt fallbacks. The uevent telling the device is gone is
> going to come anyway. In between the device disappearance and uevent
> handling, it doesn't matter much what happens: the device is abruptly
> gone and the user will see a glitch anyway - he knows, he just yanked
> out the cable.
> 
> To me this decision is just to make life in userspace easier by not
> introducing a whole new type of error. However, if the kernel did
> return EIO instead, it would be impossible for me to see that as a
> kernel regression because the old behaviour was probably explosions
> everywhere anyway.
> 
> I heard that UDL and maybe some other drivers in the kernel already
> handle hot-unplug somehow. Maybe those are what would regress if the
> kernel returned EIO?

I think most of the not-explodey drivers currently just (mostly?) it all
hw errors. You kinda have to if you actually want to deal with hotunplug,
stuff can disappear for any moment without warning anyway. Starting to
just skip everything is the simplest approach, plus in large parts of the
modeset helpers we don't even return values - kernel drivers suck as much
at handling the unexpected errors as userspace :-)
-Daniel

> 
> 
> > > +
> > > +- Pending non-blocking KMS operations deliver the DRM events userspace
> > > +  is expecting.
> > > +
> > > +- If underlying memory disappears, the mmaps are replaced with harmless
> > > +  zero pages where access does not raise SIGBUS. Reads return zeros,
> > > +  writes are ignored.
> > > +
> > > +- dmabuf which point to memory that has disappeared are rewritten to
> > > +  point to harmless zero pages, similar to mmaps. Imports still succeed
> > > +  both ways: an existing device importing a dmabuf pointing to
> > > +  disappeared memory, and a disappeared device importing any dmabuf.  
> > 
> > 
> > Same as above, I would expect drmPrimeHandleToFD and drmPrimeFDToHandle 
> > to return -EIO in case device is detached
> 
> I imagined a use case behind this: a Wayland compositor attempting to
> import a client's dmabuf. The GPU the client was using is
> hot-unplugged, meaning the dmabufs lose their storage. It is up to the
> client to handle its rendering GPU disappearance on its own, but also
> the compositor should not disconnect it because the GPU disappeared.
> It's not the client's fault the GPU disappeared.
> 
> In Wayland, failure to use a wl_buffer is considered a protocol error,
> and protocol errors are always fatal: the client is disconnected. The
> rationale is that the compositor must always be able to present the
> client buffer somehow. If the compositor cannot, then the client did
> not obey the protocol.
> 
> The fallback presentation path in a compositor is usually importing the
> dmabuf to EGL, to be sampled from OpenGL. Normally the protocol
> guarantees that this works, so any failure to do so is a protocol
> violation. But if the GPU used by the client suddenly disappears and
> the imports start to fail, that is interpreted as a protocol violation
> unless the compositor can see why the import failed. Since the import
> is done via EGL, getting the right error code plumbed through from
> libdrm functions to the EGL caller would be a hassle. I don't see any
> error code in EGL_EXT_image_dma_buf_import reserved for "the dmabuf
> storage was hot-unplugged", and I doubt there is anything exclusively
> for only that in the EGL base spec either.
> 
> The cost of lying that the import worked is that the compositor will
> paint black or transparent where the window was supposed to be. It's a
> graphical glitch that is contrary to the Wayland design principles, but
> in this case a glitch is unavoidable: even if the compositor knew this
> buffer is now bad, what would it paint instead? It has nothing else to
> paint from. I'm assuming the compositor is using a different GPU than
> what disappeared.
> 
> Ideally, the client will eventually react to losing the GPU and either
> crash, quit, or switch its rendering to something that works which
> simply gives the compositor a new, working buffer without losing any
> window state in the process. If we risk the compositor disconnecting
> the client, then the client might not recover even if it wanted to.
> 
> > > +
> > > +- Render ioctls return EIO which is then handled in userspace drivers,
> > > +  e.g. Mesa, to have the device disappearance handled in the way
> > > +  specified for each API (OpenGL, GL ES: GL_KHR_robustness;
> > > +  Vulkan: VK_ERROR_DEVICE_LOST; etc.)  
> > 
> > 
> > As far as I understood from our discussion we would reject any IOCTL 
> > aimed at a device which is gone and not only render ioctls.
> 
> Daniel?
> 
> 
> Thanks,
> pq
> 
> > 
> > Andrey
> > 
> > 
> > > +
> > > +Raising SIGBUS is not an option, because userspace cannot realistically
> > > +handle it.  Signal handlers are global, which makes them extremely
> > > +difficult to use correctly from libraries like Mesa produces. Signal
> > > +handlers are not composable, you can't have different handlers for GPU1
> > > +and GPU2 from different vendors, and a third handler for mmapped regular
> > > +files.  Threads cause additional pain with signal handling as well.
> > > +
> > > +Only after userspace has closed all relevant DRM device and dmabuf file
> > > +descriptors and removed all mmaps, the DRM driver can tear down its
> > > +instance for the device that no longer exists. If the same physical
> > > +device somehow comes back in the mean time, it shall be a new DRM
> > > +device.
> > > +
> > >   .. _drm_driver_ioctl:
> > >   
> > >   IOCTL Support on Device Nodes  
> 



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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] drm/doc: device hot-unplug for userspace
  2020-05-19 10:06 [PATCH] drm/doc: device hot-unplug for userspace Pekka Paalanen
  2020-05-19 14:37 ` Andrey Grodzovsky
@ 2020-05-20 12:55 ` Daniel Vetter
  2020-05-20 13:20   ` Simon Ser
  2020-05-25 12:46 ` [PATCH v2] " Pekka Paalanen
  2020-05-26 14:30 ` [PATCH] " Andrey Grodzovsky
  3 siblings, 1 reply; 30+ messages in thread
From: Daniel Vetter @ 2020-05-20 12:55 UTC (permalink / raw)
  To: Pekka Paalanen; +Cc: Pekka Paalanen, dri-devel, Dave Airlie, Sean Paul

On Tue, May 19, 2020 at 01:06:49PM +0300, Pekka Paalanen wrote:
> From: Pekka Paalanen <pekka.paalanen@collabora.com>
> 
> Set up the expectations on how hot-unplugging a DRM device should look like to
> userspace.
> 
> Written by Daniel Vetter's request and largely based on his comments in IRC and
> from https://lists.freedesktop.org/archives/dri-devel/2020-May/265484.html .
> 
> Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Sean Paul <sean@poorly.run>
> 
> ---
> 
> Disclaimer: I am a userspace developer writing for other userspace developers.
> I took some liberties in defining what should happen without knowing what is
> actually possible or what existing drivers already implement.
> ---
>  Documentation/gpu/drm-uapi.rst | 75 ++++++++++++++++++++++++++++++++++
>  1 file changed, 75 insertions(+)
> 
> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> index 56fec6ed1ad8..80db4abd2cbd 100644
> --- a/Documentation/gpu/drm-uapi.rst
> +++ b/Documentation/gpu/drm-uapi.rst
> @@ -1,3 +1,5 @@
> +.. Copyright 2020 DisplayLink (UK) Ltd.
> +
>  ===================
>  Userland interfaces
>  ===================
> @@ -162,6 +164,79 @@ other hand, a driver requires shared state between clients which is
>  visible to user-space and accessible beyond open-file boundaries, they
>  cannot support render nodes.
>  
> +Device Hot-Unplug
> +=================
> +
> +.. note::
> +   The following is the plan. Implementation is not there yet
> +   (2020 May 13).
> +
> +Graphics devices (display and/or render) may be connected via USB (e.g.
> +display adapters or docking stations) or Thunderbolt (e.g. eGPU). An end
> +user is able to hot-unplug this kind of devices while they are being
> +used, and expects that the very least the machine does not crash. Any
> +damage from hot-unplugging a DRM device needs to be limited as much as
> +possible and userspace must be given the chance to handle it if it wants
> +to. Ideally, unplugging a DRM device still lets a desktop to continue
> +running, but that is going to need explicit support throughout the whole
> +graphics stack: from kernel and userspace drivers, through display
> +servers, via window system protocols, and in applications and libraries.
> +
> +Other scenarios that should lead to the same are: unrecoverable GPU
> +crash, PCI device disappearing off the bus, or forced unbind of a driver
> +from the physical device.
> +
> +In other words, from userspace perspective everything needs to keep on
> +working more or less, until userspace stops using the disappeared DRM
> +device and closes it completely. Userspace will learn of the device
> +disappearance from the device removed uevent or in some cases specific
> +ioctls returning EIO.
> +
> +This goal raises at least the following requirements for the kernel and
> +drivers:
> +
> +- The kernel must not hang, crash or oops, no matter what userspace was
> +  in the middle of doing when the device disappeared.
> +
> +- All GPU jobs that can no longer run must have their fences
> +  force-signalled to avoid inflicting hangs to userspace.
> +
> +- KMS connectors must change their status to disconnected.
> +
> +- Legacy modesets and pageflips fake success.
> +
> +- Atomic commits, both real and TEST_ONLY, fake success.
> +
> +- Pending non-blocking KMS operations deliver the DRM events userspace
> +  is expecting.
> +
> +- If underlying memory disappears, the mmaps are replaced with harmless
> +  zero pages where access does not raise SIGBUS. Reads return zeros,
> +  writes are ignored.

Hm this is going to be expensive to implement, every write access would
cause a minor fault to catch it and throw it away. That's not going to be
fast is someone is caught trying to upload a giant texture at that moment
:-/

Could we go with just "Reads and writes will still completely without
errors, but have otherwise undefined behaviour". Then we could go with a
"single shared page for every drmfd" (to avoid leaks), set up with rw
permissions. That's dirt cheap, should be easy to implement and everything
stays fast.

> +
> +- dmabuf which point to memory that has disappeared are rewritten to
> +  point to harmless zero pages, similar to mmaps. Imports still succeed
> +  both ways: an existing device importing a dmabuf pointing to
> +  disappeared memory, and a disappeared device importing any dmabuf.

Same here about mmaps.

> +
> +- Render ioctls return EIO which is then handled in userspace drivers,
> +  e.g. Mesa, to have the device disappearance handled in the way
> +  specified for each API (OpenGL, GL ES: GL_KHR_robustness;
> +  Vulkan: VK_ERROR_DEVICE_LOST; etc.)

I'd go further and spec that driver-specific render ioctl have driver
specific behaviour, but overall must not result in crashes and failures
expect as specified in the client apis you already list.

Some drivers might just go with an uevent and have no errors from ioctls,
some might only want to have an error on a very specific ioctl, some maybe
on all of them. Since no one except the hw specific userspace drivers will
have to deal with that I think best to only specify the outcome we want.

> +
> +Raising SIGBUS is not an option, because userspace cannot realistically
> +handle it.  Signal handlers are global, which makes them extremely
> +difficult to use correctly from libraries like Mesa produces. Signal
> +handlers are not composable, you can't have different handlers for GPU1
> +and GPU2 from different vendors, and a third handler for mmapped regular
> +files.  Threads cause additional pain with signal handling as well.

Hm maybe move that to the entire mmap discussion and why SIGBUS is a
really bad idea?

> +
> +Only after userspace has closed all relevant DRM device and dmabuf file
> +descriptors and removed all mmaps, the DRM driver can tear down its
> +instance for the device that no longer exists. If the same physical
> +device somehow comes back in the mean time, it shall be a new DRM
> +device.

Maybe we should add an explicit note that there's no guarantee about the
new chardev minor this new device will get, it could both inherit the
existing one (you can't open the old one anymore anyway) or get a new one?

Or does userspace want a guarantee, i.e. as long as there's still a handle
open kernel guarantees to not recycle the chardev minor (which is what we
currently do). In that case better to add that to your list of guarantees
above.

I think overall a good start for documenting the uapi expectations of
hotunplug, we'll definitely have to refine this as we go.

Cheers, Daniel

> +
>  .. _drm_driver_ioctl:
>  
>  IOCTL Support on Device Nodes
> -- 
> 2.20.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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] drm/doc: device hot-unplug for userspace
  2020-05-20 12:55 ` Daniel Vetter
@ 2020-05-20 13:20   ` Simon Ser
  2020-05-20 14:19     ` Daniel Vetter
  0 siblings, 1 reply; 30+ messages in thread
From: Simon Ser @ 2020-05-20 13:20 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Sean Paul, Dave Airlie, Pekka Paalanen, dri-devel

On Wednesday, May 20, 2020 2:55 PM, Daniel Vetter <daniel@ffwll.ch> wrote:

> Maybe we should add an explicit note that there's no guarantee about the
> new chardev minor this new device will get, it could both inherit the
> existing one (you can't open the old one anymore anyway) or get a new one?
>
> Or does userspace want a guarantee, i.e. as long as there's still a handle
> open kernel guarantees to not recycle the chardev minor (which is what we
> currently do). In that case better to add that to your list of guarantees
> above.

The are race conditions to consider too, e.g.

- Compositor sends /dev/dri/card0 to a client
- card0 goes away
- Another device takes card0
- Client receives /dev/dri/card0 and then starts using it, but it's the
  wrong device

At first glance these seem like edge-cases that almost never happen.
However I've seen these happen in practice with connectors, especially
with docks.

One solution would be to number minor numbers like PIDs: don't recycle
card0 before we've reached the upper minor number limit.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] drm/doc: device hot-unplug for userspace
  2020-05-20 13:20   ` Simon Ser
@ 2020-05-20 14:19     ` Daniel Vetter
  2020-05-22  9:54       ` Pekka Paalanen
  0 siblings, 1 reply; 30+ messages in thread
From: Daniel Vetter @ 2020-05-20 14:19 UTC (permalink / raw)
  To: Simon Ser; +Cc: Sean Paul, Dave Airlie, Pekka Paalanen, dri-devel

On Wed, May 20, 2020 at 3:20 PM Simon Ser <contact@emersion.fr> wrote:
>
> On Wednesday, May 20, 2020 2:55 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
>
> > Maybe we should add an explicit note that there's no guarantee about the
> > new chardev minor this new device will get, it could both inherit the
> > existing one (you can't open the old one anymore anyway) or get a new one?
> >
> > Or does userspace want a guarantee, i.e. as long as there's still a handle
> > open kernel guarantees to not recycle the chardev minor (which is what we
> > currently do). In that case better to add that to your list of guarantees
> > above.
>
> The are race conditions to consider too, e.g.
>
> - Compositor sends /dev/dri/card0 to a client
> - card0 goes away
> - Another device takes card0
> - Client receives /dev/dri/card0 and then starts using it, but it's the
>   wrong device

Oh reminds me, what should we do about open() - that one will fail,
the chardev is going away after all, not failing kinda doesn't make
sense. And more tricky, about creating new leases?

I think reasonable semantics here would be that both of these "create
a new open drm fd" operations can fail as soon as the device is
unplugged. Userspace needs to be able to deal with that.
-Daniel

>
> At first glance these seem like edge-cases that almost never happen.
> However I've seen these happen in practice with connectors, especially
> with docks.
>
> One solution would be to number minor numbers like PIDs: don't recycle
> card0 before we've reached the upper minor number limit.



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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] drm/doc: device hot-unplug for userspace
  2020-05-20 12:46     ` Daniel Vetter
@ 2020-05-20 14:50       ` Andrey Grodzovsky
  2020-05-25 12:13         ` Andrey Grodzovsky
  2020-05-25 14:28         ` Daniel Vetter
  0 siblings, 2 replies; 30+ messages in thread
From: Andrey Grodzovsky @ 2020-05-20 14:50 UTC (permalink / raw)
  To: Daniel Vetter, Pekka Paalanen; +Cc: Dave Airlie, Sean Paul, dri-devel


On 5/20/20 8:46 AM, Daniel Vetter wrote:
> On Wed, May 20, 2020 at 02:19:08PM +0300, Pekka Paalanen wrote:
>> On Tue, 19 May 2020 10:37:12 -0400
>> Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> wrote:
>>
>>> Thanks for the summary, does put things in order and makes it easier to
>>> comprehend all the TODOs, some questions bellow
>>>
>>> On 5/19/20 6:06 AM, Pekka Paalanen wrote:
>>>> From: Pekka Paalanen <pekka.paalanen@collabora.com>
>>>>
>>>> Set up the expectations on how hot-unplugging a DRM device should look like to
>>>> userspace.
>>>>
>>>> Written by Daniel Vetter's request and largely based on his comments in IRC and
>>>> from https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Fdri-devel%2F2020-May%2F265484.html&amp;data=02%7C01%7CAndrey.Grodzovsky%40amd.com%7Ca4da241ff1e54610d95508d7fcbbcc11%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637255755828209496&amp;sdata=YDFoP2g3z3IiB77sRvAmPB%2Fix%2FV0Mh78YcCSAAlhXdg%3D&amp;reserved=0 .
>>>>
>>>> Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>>> Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>> Cc: Dave Airlie <airlied@redhat.com>
>>>> Cc: Sean Paul <sean@poorly.run>
>>>>
>>>> ---
>>>>
>>>> Disclaimer: I am a userspace developer writing for other userspace developers.
>>>> I took some liberties in defining what should happen without knowing what is
>>>> actually possible or what existing drivers already implement.
>>>> ---
>>>>    Documentation/gpu/drm-uapi.rst | 75 ++++++++++++++++++++++++++++++++++
>>>>    1 file changed, 75 insertions(+)
>>>>
>>>> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
>>>> index 56fec6ed1ad8..80db4abd2cbd 100644
>>>> --- a/Documentation/gpu/drm-uapi.rst
>>>> +++ b/Documentation/gpu/drm-uapi.rst
>>>> @@ -1,3 +1,5 @@
>>>> +.. Copyright 2020 DisplayLink (UK) Ltd.
>>>> +
>>>>    ===================
>>>>    Userland interfaces
>>>>    ===================
>>>> @@ -162,6 +164,79 @@ other hand, a driver requires shared state between clients which is
>>>>    visible to user-space and accessible beyond open-file boundaries, they
>>>>    cannot support render nodes.
>>>>    
>>>> +Device Hot-Unplug
>>>> +=================
>>>> +
>>>> +.. note::
>>>> +   The following is the plan. Implementation is not there yet
>>>> +   (2020 May 13).
>>>> +
>>>> +Graphics devices (display and/or render) may be connected via USB (e.g.
>>>> +display adapters or docking stations) or Thunderbolt (e.g. eGPU). An end
>>>> +user is able to hot-unplug this kind of devices while they are being
>>>> +used, and expects that the very least the machine does not crash. Any
>>>> +damage from hot-unplugging a DRM device needs to be limited as much as
>>>> +possible and userspace must be given the chance to handle it if it wants
>>>> +to. Ideally, unplugging a DRM device still lets a desktop to continue
>>>> +running, but that is going to need explicit support throughout the whole
>>>> +graphics stack: from kernel and userspace drivers, through display
>>>> +servers, via window system protocols, and in applications and libraries.
>>>> +
>>>> +Other scenarios that should lead to the same are: unrecoverable GPU
>>>> +crash, PCI device disappearing off the bus, or forced unbind of a driver
>>>> +from the physical device.
>>>> +
>>>> +In other words, from userspace perspective everything needs to keep on
>>>> +working more or less, until userspace stops using the disappeared DRM
>>>> +device and closes it completely. Userspace will learn of the device
>>>> +disappearance from the device removed uevent or in some cases specific
>>>> +ioctls returning EIO.
>>>> +
>>>> +This goal raises at least the following requirements for the kernel and
>>>> +drivers:
>>>> +
>>>> +- The kernel must not hang, crash or oops, no matter what userspace was
>>>> +  in the middle of doing when the device disappeared.
>>>> +
>>>> +- All GPU jobs that can no longer run must have their fences
>>>> +  force-signalled to avoid inflicting hangs to userspace.
>>>> +
>>>> +- KMS connectors must change their status to disconnected.
>>>> +
>>>> +- Legacy modesets and pageflips fake success.
>>>> +
>>>> +- Atomic commits, both real and TEST_ONLY, fake success.
>>>
>>> Why wouldn't we return -EIO for the atommic commit IOTCL/legasy pflip
>>> and modeset ioctls here same way as you suggested returning -EIO for
>>> render ioctl ?
>> Hi,
>>
>> that is more of a question for Daniel Vetter than me. I believe he is
>> worried that userspace will get the error handling horribly wrong
>> anyway, because it needs to be handled in every single display server
>> project. Render ioctl errors OTOH need to be handled only in the
>> corresponding Mesa or other userspace driver, and for render there are
>> API (OpenGL/EGL, Vulkan) specs that say how it must be handled to fill
>> the API contract. Because of the API contract, those are more likely to
>> have reasonable error handling in place already.
> Yup pretty much. Atm compositors expect an -EINVAL (especially for
> TEST_ONLY), some cope with the semi-spurious -EBUSY we still throw around,
> but I expect most will just die if the get an -ENOMEM or -EIO or really
> anything.
>
> I think one area where we've historically thrown some spurious errors is
> also vblank ioctls, since they don't take full locks and sometimes the
> kernel needs to sneak in a modeset for some reason (gpu recovery, dp link
> recovery, ...).
>
> Either way I don't think there's anything the compositor can do than just
> ignore the error and carry on.


So currently drm_ioctl will just check for drm_dev_is_unplugged and 
return -ENODEV at the very beginning of the function 
(https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_ioctl.c#L825) 
meaning that currently we blanket failure any IOCTL in case the device 
is unplagued (ignoring the race condition if the device unplugged after 
the check). So do we want to remove this check and put it only for 
render ioctls (which are those ? e.g. for amdgpu there is  AMDGPU_CS 
ioctl) but not for mode setting/dma_buf_import/dma_buf_export ioctls ? 
What about other types of ioctls which are non of the listed above ?

Andrey


>
>> I first thought it would be obvious for at least atomic commits to fail
>> appropriately, but then thinking again, it will only introduce new
>> failures that are currently very hard to test for (VKMS to the rescue),
>> and userspace would need to have code to correctly bail out for EIO
>> rather than attempt fallbacks. The uevent telling the device is gone is
>> going to come anyway. In between the device disappearance and uevent
>> handling, it doesn't matter much what happens: the device is abruptly
>> gone and the user will see a glitch anyway - he knows, he just yanked
>> out the cable.
>>
>> To me this decision is just to make life in userspace easier by not
>> introducing a whole new type of error. However, if the kernel did
>> return EIO instead, it would be impossible for me to see that as a
>> kernel regression because the old behaviour was probably explosions
>> everywhere anyway.
>>
>> I heard that UDL and maybe some other drivers in the kernel already
>> handle hot-unplug somehow. Maybe those are what would regress if the
>> kernel returned EIO?
> I think most of the not-explodey drivers currently just (mostly?) it all
> hw errors. You kinda have to if you actually want to deal with hotunplug,
> stuff can disappear for any moment without warning anyway. Starting to
> just skip everything is the simplest approach, plus in large parts of the
> modeset helpers we don't even return values - kernel drivers suck as much
> at handling the unexpected errors as userspace :-)
> -Daniel
>
>>
>>>> +
>>>> +- Pending non-blocking KMS operations deliver the DRM events userspace
>>>> +  is expecting.
>>>> +
>>>> +- If underlying memory disappears, the mmaps are replaced with harmless
>>>> +  zero pages where access does not raise SIGBUS. Reads return zeros,
>>>> +  writes are ignored.
>>>> +
>>>> +- dmabuf which point to memory that has disappeared are rewritten to
>>>> +  point to harmless zero pages, similar to mmaps. Imports still succeed
>>>> +  both ways: an existing device importing a dmabuf pointing to
>>>> +  disappeared memory, and a disappeared device importing any dmabuf.
>>>
>>> Same as above, I would expect drmPrimeHandleToFD and drmPrimeFDToHandle
>>> to return -EIO in case device is detached
>> I imagined a use case behind this: a Wayland compositor attempting to
>> import a client's dmabuf. The GPU the client was using is
>> hot-unplugged, meaning the dmabufs lose their storage. It is up to the
>> client to handle its rendering GPU disappearance on its own, but also
>> the compositor should not disconnect it because the GPU disappeared.
>> It's not the client's fault the GPU disappeared.
>>
>> In Wayland, failure to use a wl_buffer is considered a protocol error,
>> and protocol errors are always fatal: the client is disconnected. The
>> rationale is that the compositor must always be able to present the
>> client buffer somehow. If the compositor cannot, then the client did
>> not obey the protocol.
>>
>> The fallback presentation path in a compositor is usually importing the
>> dmabuf to EGL, to be sampled from OpenGL. Normally the protocol
>> guarantees that this works, so any failure to do so is a protocol
>> violation. But if the GPU used by the client suddenly disappears and
>> the imports start to fail, that is interpreted as a protocol violation
>> unless the compositor can see why the import failed. Since the import
>> is done via EGL, getting the right error code plumbed through from
>> libdrm functions to the EGL caller would be a hassle. I don't see any
>> error code in EGL_EXT_image_dma_buf_import reserved for "the dmabuf
>> storage was hot-unplugged", and I doubt there is anything exclusively
>> for only that in the EGL base spec either.
>>
>> The cost of lying that the import worked is that the compositor will
>> paint black or transparent where the window was supposed to be. It's a
>> graphical glitch that is contrary to the Wayland design principles, but
>> in this case a glitch is unavoidable: even if the compositor knew this
>> buffer is now bad, what would it paint instead? It has nothing else to
>> paint from. I'm assuming the compositor is using a different GPU than
>> what disappeared.
>>
>> Ideally, the client will eventually react to losing the GPU and either
>> crash, quit, or switch its rendering to something that works which
>> simply gives the compositor a new, working buffer without losing any
>> window state in the process. If we risk the compositor disconnecting
>> the client, then the client might not recover even if it wanted to.
>>
>>>> +
>>>> +- Render ioctls return EIO which is then handled in userspace drivers,
>>>> +  e.g. Mesa, to have the device disappearance handled in the way
>>>> +  specified for each API (OpenGL, GL ES: GL_KHR_robustness;
>>>> +  Vulkan: VK_ERROR_DEVICE_LOST; etc.)
>>>
>>> As far as I understood from our discussion we would reject any IOCTL
>>> aimed at a device which is gone and not only render ioctls.
>> Daniel?
>>
>>
>> Thanks,
>> pq
>>
>>> Andrey
>>>
>>>
>>>> +
>>>> +Raising SIGBUS is not an option, because userspace cannot realistically
>>>> +handle it.  Signal handlers are global, which makes them extremely
>>>> +difficult to use correctly from libraries like Mesa produces. Signal
>>>> +handlers are not composable, you can't have different handlers for GPU1
>>>> +and GPU2 from different vendors, and a third handler for mmapped regular
>>>> +files.  Threads cause additional pain with signal handling as well.
>>>> +
>>>> +Only after userspace has closed all relevant DRM device and dmabuf file
>>>> +descriptors and removed all mmaps, the DRM driver can tear down its
>>>> +instance for the device that no longer exists. If the same physical
>>>> +device somehow comes back in the mean time, it shall be a new DRM
>>>> +device.
>>>> +
>>>>    .. _drm_driver_ioctl:
>>>>    
>>>>    IOCTL Support on Device Nodes
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] drm/doc: device hot-unplug for userspace
  2020-05-20 14:19     ` Daniel Vetter
@ 2020-05-22  9:54       ` Pekka Paalanen
  2020-05-25 14:29         ` Daniel Vetter
  0 siblings, 1 reply; 30+ messages in thread
From: Pekka Paalanen @ 2020-05-22  9:54 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Sean Paul, dri-devel, Dave Airlie

[-- Attachment #1.1: Type: text/plain, Size: 1738 bytes --]

On Wed, 20 May 2020 16:19:00 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Wed, May 20, 2020 at 3:20 PM Simon Ser <contact@emersion.fr> wrote:
> >
> > On Wednesday, May 20, 2020 2:55 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> >  
> > > Maybe we should add an explicit note that there's no guarantee about the
> > > new chardev minor this new device will get, it could both inherit the
> > > existing one (you can't open the old one anymore anyway) or get a new one?
> > >
> > > Or does userspace want a guarantee, i.e. as long as there's still a handle
> > > open kernel guarantees to not recycle the chardev minor (which is what we
> > > currently do). In that case better to add that to your list of guarantees
> > > above.  
> >
> > The are race conditions to consider too, e.g.
> >
> > - Compositor sends /dev/dri/card0 to a client
> > - card0 goes away
> > - Another device takes card0
> > - Client receives /dev/dri/card0 and then starts using it, but it's the
> >   wrong device  
> 
> Oh reminds me, what should we do about open() - that one will fail,
> the chardev is going away after all, not failing kinda doesn't make
> sense. And more tricky, about creating new leases?
> 
> I think reasonable semantics here would be that both of these "create
> a new open drm fd" operations can fail as soon as the device is
> unplugged. Userspace needs to be able to deal with that.

Hi,

yeah, we can make mmap read/write end result undefined, recycle char
minors like pids, and let new open()s and new leases fail. Pretty much
everything Daniel and Simon said make sense to me.

I'll spin a v2, but maybe next week.

What about the drm_ioctl() issue Andrey pointed out?


Thanks,
pq

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] drm/doc: device hot-unplug for userspace
  2020-05-20 14:50       ` Andrey Grodzovsky
@ 2020-05-25 12:13         ` Andrey Grodzovsky
  2020-05-25 14:28         ` Daniel Vetter
  1 sibling, 0 replies; 30+ messages in thread
From: Andrey Grodzovsky @ 2020-05-25 12:13 UTC (permalink / raw)
  To: Daniel Vetter, Pekka Paalanen; +Cc: Dave Airlie, Sean Paul, dri-devel

Ping on the question bellow

Andrey

On 5/20/20 10:50 AM, Andrey Grodzovsky wrote:
>
> On 5/20/20 8:46 AM, Daniel Vetter wrote:
>> On Wed, May 20, 2020 at 02:19:08PM +0300, Pekka Paalanen wrote:
>>> On Tue, 19 May 2020 10:37:12 -0400
>>> Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> wrote:
>>>
>>>> Thanks for the summary, does put things in order and makes it 
>>>> easier to
>>>> comprehend all the TODOs, some questions bellow
>>>>
>>>> On 5/19/20 6:06 AM, Pekka Paalanen wrote:
>>>>> From: Pekka Paalanen <pekka.paalanen@collabora.com>
>>>>>
>>>>> Set up the expectations on how hot-unplugging a DRM device should 
>>>>> look like to
>>>>> userspace.
>>>>>
>>>>> Written by Daniel Vetter's request and largely based on his 
>>>>> comments in IRC and
>>>>> from 
>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Fdri-devel%2F2020-May%2F265484.html&amp;data=02%7C01%7CAndrey.Grodzovsky%40amd.com%7Ca4da241ff1e54610d95508d7fcbbcc11%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637255755828209496&amp;sdata=YDFoP2g3z3IiB77sRvAmPB%2Fix%2FV0Mh78YcCSAAlhXdg%3D&amp;reserved=0 
>>>>> .
>>>>>
>>>>> Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
>>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>>>> Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>>> Cc: Dave Airlie <airlied@redhat.com>
>>>>> Cc: Sean Paul <sean@poorly.run>
>>>>>
>>>>> ---
>>>>>
>>>>> Disclaimer: I am a userspace developer writing for other userspace 
>>>>> developers.
>>>>> I took some liberties in defining what should happen without 
>>>>> knowing what is
>>>>> actually possible or what existing drivers already implement.
>>>>> ---
>>>>>    Documentation/gpu/drm-uapi.rst | 75 
>>>>> ++++++++++++++++++++++++++++++++++
>>>>>    1 file changed, 75 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/gpu/drm-uapi.rst 
>>>>> b/Documentation/gpu/drm-uapi.rst
>>>>> index 56fec6ed1ad8..80db4abd2cbd 100644
>>>>> --- a/Documentation/gpu/drm-uapi.rst
>>>>> +++ b/Documentation/gpu/drm-uapi.rst
>>>>> @@ -1,3 +1,5 @@
>>>>> +.. Copyright 2020 DisplayLink (UK) Ltd.
>>>>> +
>>>>>    ===================
>>>>>    Userland interfaces
>>>>>    ===================
>>>>> @@ -162,6 +164,79 @@ other hand, a driver requires shared state 
>>>>> between clients which is
>>>>>    visible to user-space and accessible beyond open-file 
>>>>> boundaries, they
>>>>>    cannot support render nodes.
>>>>>    +Device Hot-Unplug
>>>>> +=================
>>>>> +
>>>>> +.. note::
>>>>> +   The following is the plan. Implementation is not there yet
>>>>> +   (2020 May 13).
>>>>> +
>>>>> +Graphics devices (display and/or render) may be connected via USB 
>>>>> (e.g.
>>>>> +display adapters or docking stations) or Thunderbolt (e.g. eGPU). 
>>>>> An end
>>>>> +user is able to hot-unplug this kind of devices while they are being
>>>>> +used, and expects that the very least the machine does not crash. 
>>>>> Any
>>>>> +damage from hot-unplugging a DRM device needs to be limited as 
>>>>> much as
>>>>> +possible and userspace must be given the chance to handle it if 
>>>>> it wants
>>>>> +to. Ideally, unplugging a DRM device still lets a desktop to 
>>>>> continue
>>>>> +running, but that is going to need explicit support throughout 
>>>>> the whole
>>>>> +graphics stack: from kernel and userspace drivers, through display
>>>>> +servers, via window system protocols, and in applications and 
>>>>> libraries.
>>>>> +
>>>>> +Other scenarios that should lead to the same are: unrecoverable GPU
>>>>> +crash, PCI device disappearing off the bus, or forced unbind of a 
>>>>> driver
>>>>> +from the physical device.
>>>>> +
>>>>> +In other words, from userspace perspective everything needs to 
>>>>> keep on
>>>>> +working more or less, until userspace stops using the disappeared 
>>>>> DRM
>>>>> +device and closes it completely. Userspace will learn of the device
>>>>> +disappearance from the device removed uevent or in some cases 
>>>>> specific
>>>>> +ioctls returning EIO.
>>>>> +
>>>>> +This goal raises at least the following requirements for the 
>>>>> kernel and
>>>>> +drivers:
>>>>> +
>>>>> +- The kernel must not hang, crash or oops, no matter what 
>>>>> userspace was
>>>>> +  in the middle of doing when the device disappeared.
>>>>> +
>>>>> +- All GPU jobs that can no longer run must have their fences
>>>>> +  force-signalled to avoid inflicting hangs to userspace.
>>>>> +
>>>>> +- KMS connectors must change their status to disconnected.
>>>>> +
>>>>> +- Legacy modesets and pageflips fake success.
>>>>> +
>>>>> +- Atomic commits, both real and TEST_ONLY, fake success.
>>>>
>>>> Why wouldn't we return -EIO for the atommic commit IOTCL/legasy pflip
>>>> and modeset ioctls here same way as you suggested returning -EIO for
>>>> render ioctl ?
>>> Hi,
>>>
>>> that is more of a question for Daniel Vetter than me. I believe he is
>>> worried that userspace will get the error handling horribly wrong
>>> anyway, because it needs to be handled in every single display server
>>> project. Render ioctl errors OTOH need to be handled only in the
>>> corresponding Mesa or other userspace driver, and for render there are
>>> API (OpenGL/EGL, Vulkan) specs that say how it must be handled to fill
>>> the API contract. Because of the API contract, those are more likely to
>>> have reasonable error handling in place already.
>> Yup pretty much. Atm compositors expect an -EINVAL (especially for
>> TEST_ONLY), some cope with the semi-spurious -EBUSY we still throw 
>> around,
>> but I expect most will just die if the get an -ENOMEM or -EIO or really
>> anything.
>>
>> I think one area where we've historically thrown some spurious errors is
>> also vblank ioctls, since they don't take full locks and sometimes the
>> kernel needs to sneak in a modeset for some reason (gpu recovery, dp 
>> link
>> recovery, ...).
>>
>> Either way I don't think there's anything the compositor can do than 
>> just
>> ignore the error and carry on.
>
>
> So currently drm_ioctl will just check for drm_dev_is_unplugged and 
> return -ENODEV at the very beginning of the function 
> (https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_ioctl.c#L825) 
> meaning that currently we blanket failure any IOCTL in case the device 
> is unplagued (ignoring the race condition if the device unplugged 
> after the check). So do we want to remove this check and put it only 
> for render ioctls (which are those ? e.g. for amdgpu there is  
> AMDGPU_CS ioctl) but not for mode 
> setting/dma_buf_import/dma_buf_export ioctls ? What about other types 
> of ioctls which are non of the listed above ?
>
> Andrey
>
>
>>
>>> I first thought it would be obvious for at least atomic commits to fail
>>> appropriately, but then thinking again, it will only introduce new
>>> failures that are currently very hard to test for (VKMS to the rescue),
>>> and userspace would need to have code to correctly bail out for EIO
>>> rather than attempt fallbacks. The uevent telling the device is gone is
>>> going to come anyway. In between the device disappearance and uevent
>>> handling, it doesn't matter much what happens: the device is abruptly
>>> gone and the user will see a glitch anyway - he knows, he just yanked
>>> out the cable.
>>>
>>> To me this decision is just to make life in userspace easier by not
>>> introducing a whole new type of error. However, if the kernel did
>>> return EIO instead, it would be impossible for me to see that as a
>>> kernel regression because the old behaviour was probably explosions
>>> everywhere anyway.
>>>
>>> I heard that UDL and maybe some other drivers in the kernel already
>>> handle hot-unplug somehow. Maybe those are what would regress if the
>>> kernel returned EIO?
>> I think most of the not-explodey drivers currently just (mostly?) it all
>> hw errors. You kinda have to if you actually want to deal with 
>> hotunplug,
>> stuff can disappear for any moment without warning anyway. Starting to
>> just skip everything is the simplest approach, plus in large parts of 
>> the
>> modeset helpers we don't even return values - kernel drivers suck as 
>> much
>> at handling the unexpected errors as userspace :-)
>> -Daniel
>>
>>>
>>>>> +
>>>>> +- Pending non-blocking KMS operations deliver the DRM events 
>>>>> userspace
>>>>> +  is expecting.
>>>>> +
>>>>> +- If underlying memory disappears, the mmaps are replaced with 
>>>>> harmless
>>>>> +  zero pages where access does not raise SIGBUS. Reads return zeros,
>>>>> +  writes are ignored.
>>>>> +
>>>>> +- dmabuf which point to memory that has disappeared are rewritten to
>>>>> +  point to harmless zero pages, similar to mmaps. Imports still 
>>>>> succeed
>>>>> +  both ways: an existing device importing a dmabuf pointing to
>>>>> +  disappeared memory, and a disappeared device importing any dmabuf.
>>>>
>>>> Same as above, I would expect drmPrimeHandleToFD and 
>>>> drmPrimeFDToHandle
>>>> to return -EIO in case device is detached
>>> I imagined a use case behind this: a Wayland compositor attempting to
>>> import a client's dmabuf. The GPU the client was using is
>>> hot-unplugged, meaning the dmabufs lose their storage. It is up to the
>>> client to handle its rendering GPU disappearance on its own, but also
>>> the compositor should not disconnect it because the GPU disappeared.
>>> It's not the client's fault the GPU disappeared.
>>>
>>> In Wayland, failure to use a wl_buffer is considered a protocol error,
>>> and protocol errors are always fatal: the client is disconnected. The
>>> rationale is that the compositor must always be able to present the
>>> client buffer somehow. If the compositor cannot, then the client did
>>> not obey the protocol.
>>>
>>> The fallback presentation path in a compositor is usually importing the
>>> dmabuf to EGL, to be sampled from OpenGL. Normally the protocol
>>> guarantees that this works, so any failure to do so is a protocol
>>> violation. But if the GPU used by the client suddenly disappears and
>>> the imports start to fail, that is interpreted as a protocol violation
>>> unless the compositor can see why the import failed. Since the import
>>> is done via EGL, getting the right error code plumbed through from
>>> libdrm functions to the EGL caller would be a hassle. I don't see any
>>> error code in EGL_EXT_image_dma_buf_import reserved for "the dmabuf
>>> storage was hot-unplugged", and I doubt there is anything exclusively
>>> for only that in the EGL base spec either.
>>>
>>> The cost of lying that the import worked is that the compositor will
>>> paint black or transparent where the window was supposed to be. It's a
>>> graphical glitch that is contrary to the Wayland design principles, but
>>> in this case a glitch is unavoidable: even if the compositor knew this
>>> buffer is now bad, what would it paint instead? It has nothing else to
>>> paint from. I'm assuming the compositor is using a different GPU than
>>> what disappeared.
>>>
>>> Ideally, the client will eventually react to losing the GPU and either
>>> crash, quit, or switch its rendering to something that works which
>>> simply gives the compositor a new, working buffer without losing any
>>> window state in the process. If we risk the compositor disconnecting
>>> the client, then the client might not recover even if it wanted to.
>>>
>>>>> +
>>>>> +- Render ioctls return EIO which is then handled in userspace 
>>>>> drivers,
>>>>> +  e.g. Mesa, to have the device disappearance handled in the way
>>>>> +  specified for each API (OpenGL, GL ES: GL_KHR_robustness;
>>>>> +  Vulkan: VK_ERROR_DEVICE_LOST; etc.)
>>>>
>>>> As far as I understood from our discussion we would reject any IOCTL
>>>> aimed at a device which is gone and not only render ioctls.
>>> Daniel?
>>>
>>>
>>> Thanks,
>>> pq
>>>
>>>> Andrey
>>>>
>>>>
>>>>> +
>>>>> +Raising SIGBUS is not an option, because userspace cannot 
>>>>> realistically
>>>>> +handle it.  Signal handlers are global, which makes them extremely
>>>>> +difficult to use correctly from libraries like Mesa produces. Signal
>>>>> +handlers are not composable, you can't have different handlers 
>>>>> for GPU1
>>>>> +and GPU2 from different vendors, and a third handler for mmapped 
>>>>> regular
>>>>> +files.  Threads cause additional pain with signal handling as well.
>>>>> +
>>>>> +Only after userspace has closed all relevant DRM device and 
>>>>> dmabuf file
>>>>> +descriptors and removed all mmaps, the DRM driver can tear down its
>>>>> +instance for the device that no longer exists. If the same physical
>>>>> +device somehow comes back in the mean time, it shall be a new DRM
>>>>> +device.
>>>>> +
>>>>>    .. _drm_driver_ioctl:
>>>>>       IOCTL Support on Device Nodes
>>
>>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2] drm/doc: device hot-unplug for userspace
  2020-05-19 10:06 [PATCH] drm/doc: device hot-unplug for userspace Pekka Paalanen
  2020-05-19 14:37 ` Andrey Grodzovsky
  2020-05-20 12:55 ` Daniel Vetter
@ 2020-05-25 12:46 ` Pekka Paalanen
  2020-05-25 13:51   ` Andrey Grodzovsky
  2020-05-26 14:30 ` [PATCH] " Andrey Grodzovsky
  3 siblings, 1 reply; 30+ messages in thread
From: Pekka Paalanen @ 2020-05-25 12:46 UTC (permalink / raw)
  To: dri-devel; +Cc: Pekka Paalanen, Dave Airlie, Sean Paul

From: Pekka Paalanen <pekka.paalanen@collabora.com>

Set up the expectations on how hot-unplugging a DRM device should look like to
userspace.

Written by Daniel Vetter's request and largely based on his comments in IRC and
from https://lists.freedesktop.org/archives/dri-devel/2020-May/265484.html .

Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Sean Paul <sean@poorly.run>
Cc: Simon Ser <contact@emersion.fr>

---

v2:
- mmap reads/writes undefined (danvet)
- make render ioctl behaviour driver-specific (danvet)
- restructure the mmap paragraphs (danvet)
- chardev minor notes (Simon)
- open behaviour (danvet)
- DRM leasing behaviour (danvet)
- added links

Disclaimer: I am a userspace developer writing for other userspace developers.
I took some liberties in defining what should happen without knowing what is
actually possible or what existing drivers already implement.
---
 Documentation/gpu/drm-uapi.rst | 102 +++++++++++++++++++++++++++++++++
 1 file changed, 102 insertions(+)

diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
index 56fec6ed1ad8..520b8e640ad1 100644
--- a/Documentation/gpu/drm-uapi.rst
+++ b/Documentation/gpu/drm-uapi.rst
@@ -1,3 +1,5 @@
+.. Copyright 2020 DisplayLink (UK) Ltd.
+
 ===================
 Userland interfaces
 ===================
@@ -162,6 +164,106 @@ other hand, a driver requires shared state between clients which is
 visible to user-space and accessible beyond open-file boundaries, they
 cannot support render nodes.
 
+Device Hot-Unplug
+=================
+
+.. note::
+   The following is the plan. Implementation is not there yet
+   (2020 May).
+
+Graphics devices (display and/or render) may be connected via USB (e.g.
+display adapters or docking stations) or Thunderbolt (e.g. eGPU). An end
+user is able to hot-unplug this kind of devices while they are being
+used, and expects that the very least the machine does not crash. Any
+damage from hot-unplugging a DRM device needs to be limited as much as
+possible and userspace must be given the chance to handle it if it wants
+to. Ideally, unplugging a DRM device still lets a desktop to continue
+running, but that is going to need explicit support throughout the whole
+graphics stack: from kernel and userspace drivers, through display
+servers, via window system protocols, and in applications and libraries.
+
+Other scenarios that should lead to the same are: unrecoverable GPU
+crash, PCI device disappearing off the bus, or forced unbind of a driver
+from the physical device.
+
+In other words, from userspace perspective everything needs to keep on
+working more or less, until userspace stops using the disappeared DRM
+device and closes it completely. Userspace will learn of the device
+disappearance from the device removed uevent or in some cases
+driver-specific ioctls returning EIO.
+
+Only after userspace has closed all relevant DRM device and dmabuf file
+descriptors and removed all mmaps, the DRM driver can tear down its
+instance for the device that no longer exists. If the same physical
+device somehow comes back in the mean time, it shall be a new DRM
+device.
+
+Similar to PIDs, chardev minor numbers are not recycled immediately. A
+new DRM device always picks the next free minor number compared to the
+previous one allocated, and wraps around when minor numbers are
+exhausted.
+
+Requirements for UAPI
+---------------------
+
+The goal raises at least the following requirements for the kernel and
+drivers:
+
+- The kernel must not hang, crash or oops, no matter what userspace was
+  in the middle of doing when the device disappeared.
+
+- All GPU jobs that can no longer run must have their fences
+  force-signalled to avoid inflicting hangs to userspace.
+
+- KMS connectors must change their status to disconnected.
+
+- Legacy modesets and pageflips fake success.
+
+- Atomic commits, both real and TEST_ONLY, fake success.
+
+- Pending non-blocking KMS operations deliver the DRM events userspace
+  is expecting.
+
+- dmabuf which point to memory that has disappeared will continue to
+  be successfully imported if it would have succeeded before the
+  disappearance.
+
+- Attempting to import a dmabuf to a disappeared device will succeed if
+  it would have succeeded without the disappearance.
+
+- Some userspace APIs already define what should happen when the device
+  disappears (OpenGL, GL ES: `GL_KHR_robustness`_; `Vulkan`_:
+  VK_ERROR_DEVICE_LOST; etc.). DRM drivers are free to implement this
+  behaviour the way they see best, e.g. returning failures in
+  driver-specific ioctls and handling those in userspace drivers, or
+  rely on uevents, and so on.
+
+- open() on a device node whose underlying device has disappeared will
+  fail.
+
+- Attempting to create a DRM lease on a disappeared DRM device will
+  fail. Existing DRM leases remain.
+
+.. _GL_KHR_robustness: https://www.khronos.org/registry/OpenGL/extensions/KHR/KHR_robustness.txt
+.. _Vulkan: https://www.khronos.org/vulkan/
+
+Requirements for memory maps
+----------------------------
+
+Memory maps have further requirements. If the underlying memory
+disappears, the mmap is modified such that reads and writes will still
+complete successfully but the result is undefined. This applies to both
+userspace mmap()'d memory and memory pointed to by dmabuf which might be
+mapped to other devices.
+
+Raising SIGBUS is not an option, because userspace cannot realistically
+handle it.  Signal handlers are global, which makes them extremely
+difficult to use correctly from libraries like those that Mesa produces.
+Signal handlers are not composable, you can't have different handlers
+for GPU1 and GPU2 from different vendors, and a third handler for
+mmapped regular files.  Threads cause additional pain with signal
+handling as well.
+
 .. _drm_driver_ioctl:
 
 IOCTL Support on Device Nodes
-- 
2.20.1

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] drm/doc: device hot-unplug for userspace
  2020-05-25 12:46 ` [PATCH v2] " Pekka Paalanen
@ 2020-05-25 13:51   ` Andrey Grodzovsky
  2020-05-25 14:30     ` Daniel Vetter
  2020-05-25 14:41     ` Pekka Paalanen
  0 siblings, 2 replies; 30+ messages in thread
From: Andrey Grodzovsky @ 2020-05-25 13:51 UTC (permalink / raw)
  To: Pekka Paalanen, dri-devel; +Cc: Sean Paul, Dave Airlie, Pekka Paalanen

On 5/25/20 8:46 AM, Pekka Paalanen wrote:

> From: Pekka Paalanen <pekka.paalanen@collabora.com>
>
> Set up the expectations on how hot-unplugging a DRM device should look like to
> userspace.
>
> Written by Daniel Vetter's request and largely based on his comments in IRC and
> from https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Fdri-devel%2F2020-May%2F265484.html&amp;data=02%7C01%7Candrey.grodzovsky%40amd.com%7Cc9676f35bbdf4d5a052808d800a9b517%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637260076178891269&amp;sdata=tbOTr7TfESohEgWspomM1sbMq4U4n7bOvdS6JlYifmM%3D&amp;reserved=0 .
>
> Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Sean Paul <sean@poorly.run>
> Cc: Simon Ser <contact@emersion.fr>
>
> ---
>
> v2:
> - mmap reads/writes undefined (danvet)
> - make render ioctl behaviour driver-specific (danvet)
> - restructure the mmap paragraphs (danvet)
> - chardev minor notes (Simon)
> - open behaviour (danvet)
> - DRM leasing behaviour (danvet)
> - added links
>
> Disclaimer: I am a userspace developer writing for other userspace developers.
> I took some liberties in defining what should happen without knowing what is
> actually possible or what existing drivers already implement.
> ---
>   Documentation/gpu/drm-uapi.rst | 102 +++++++++++++++++++++++++++++++++
>   1 file changed, 102 insertions(+)
>
> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> index 56fec6ed1ad8..520b8e640ad1 100644
> --- a/Documentation/gpu/drm-uapi.rst
> +++ b/Documentation/gpu/drm-uapi.rst
> @@ -1,3 +1,5 @@
> +.. Copyright 2020 DisplayLink (UK) Ltd.
> +
>   ===================
>   Userland interfaces
>   ===================
> @@ -162,6 +164,106 @@ other hand, a driver requires shared state between clients which is
>   visible to user-space and accessible beyond open-file boundaries, they
>   cannot support render nodes.
>   
> +Device Hot-Unplug
> +=================
> +
> +.. note::
> +   The following is the plan. Implementation is not there yet
> +   (2020 May).
> +
> +Graphics devices (display and/or render) may be connected via USB (e.g.
> +display adapters or docking stations) or Thunderbolt (e.g. eGPU). An end
> +user is able to hot-unplug this kind of devices while they are being
> +used, and expects that the very least the machine does not crash. Any
> +damage from hot-unplugging a DRM device needs to be limited as much as
> +possible and userspace must be given the chance to handle it if it wants
> +to. Ideally, unplugging a DRM device still lets a desktop to continue
> +running, but that is going to need explicit support throughout the whole
> +graphics stack: from kernel and userspace drivers, through display
> +servers, via window system protocols, and in applications and libraries.

So to support all the requirements in this document only kernel changes 
should be enough and no changes are required from user mode part of the 
stack ?

> +
> +Other scenarios that should lead to the same are: unrecoverable GPU
> +crash, PCI device disappearing off the bus, or forced unbind of a driver
> +from the physical device.
> +
> +In other words, from userspace perspective everything needs to keep on
> +working more or less, until userspace stops using the disappeared DRM
> +device and closes it completely. Userspace will learn of the device
> +disappearance from the device removed uevent


Is this uevent already implemented ? Can you point me to the code ?


> or in some cases
> +driver-specific ioctls returning EIO.
> +
> +Only after userspace has closed all relevant DRM device and dmabuf file
> +descriptors and removed all mmaps, the DRM driver can tear down its
> +instance for the device that no longer exists. If the same physical
> +device somehow comes back in the mean time, it shall be a new DRM
> +device.
> +
> +Similar to PIDs, chardev minor numbers are not recycled immediately. A
> +new DRM device always picks the next free minor number compared to the
> +previous one allocated, and wraps around when minor numbers are
> +exhausted.
> +
> +Requirements for UAPI
> +---------------------
> +
> +The goal raises at least the following requirements for the kernel and
> +drivers:
> +
> +- The kernel must not hang, crash or oops, no matter what userspace was
> +  in the middle of doing when the device disappeared.
> +
> +- All GPU jobs that can no longer run must have their fences
> +  force-signalled to avoid inflicting hangs to userspace.
> +
> +- KMS connectors must change their status to disconnected.
> +
> +- Legacy modesets and pageflips fake success.
> +
> +- Atomic commits, both real and TEST_ONLY, fake success.
> +
> +- Pending non-blocking KMS operations deliver the DRM events userspace
> +  is expecting.


The 4 points above refer to mode setting/display attached card and are 
irrelevant for secondary GPU (e.g. DRI-PRIME scenario) or no display 
system in general. Maybe we can somehow highlight this in the document 
and I on the implementing side can then decide as a first step to 
concentrate on implementing the non display case as a first step or the 
only step. In general and correct me if I am wrong, render only GPUs (or 
compute only) are the majority of cases where you would want to be able 
to detach/attach GPU on the fly (e.g attach stronger secondary graphic 
card to a laptop to get high performance in a game or add/remove a GPU 
to/from a compute cluster)

Andrey


> +
> +- dmabuf which point to memory that has disappeared will continue to
> +  be successfully imported if it would have succeeded before the
> +  disappearance.
> +
> +- Attempting to import a dmabuf to a disappeared device will succeed if
> +  it would have succeeded without the disappearance.
> +
> +- Some userspace APIs already define what should happen when the device
> +  disappears (OpenGL, GL ES: `GL_KHR_robustness`_; `Vulkan`_:
> +  VK_ERROR_DEVICE_LOST; etc.). DRM drivers are free to implement this
> +  behaviour the way they see best, e.g. returning failures in
> +  driver-specific ioctls and handling those in userspace drivers, or
> +  rely on uevents, and so on.
> +
> +- open() on a device node whose underlying device has disappeared will
> +  fail.
> +
> +- Attempting to create a DRM lease on a disappeared DRM device will
> +  fail. Existing DRM leases remain.
> +
> +.. _GL_KHR_robustness: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.khronos.org%2Fregistry%2FOpenGL%2Fextensions%2FKHR%2FKHR_robustness.txt&amp;data=02%7C01%7Candrey.grodzovsky%40amd.com%7Cc9676f35bbdf4d5a052808d800a9b517%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637260076178891269&amp;sdata=m%2FneRusoe6qGVU8Edk%2FncaD7eSJZXtPnA1IqLr7k%2Bos%3D&amp;reserved=0
> +.. _Vulkan: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.khronos.org%2Fvulkan%2F&amp;data=02%7C01%7Candrey.grodzovsky%40amd.com%7Cc9676f35bbdf4d5a052808d800a9b517%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637260076178901265&amp;sdata=WsfLduUBzRKlybOJb5PQViBWYu5DgleEeycmf76l3UU%3D&amp;reserved=0
> +
> +Requirements for memory maps
> +----------------------------
> +
> +Memory maps have further requirements. If the underlying memory
> +disappears, the mmap is modified such that reads and writes will still
> +complete successfully but the result is undefined. This applies to both
> +userspace mmap()'d memory and memory pointed to by dmabuf which might be
> +mapped to other devices.
> +
> +Raising SIGBUS is not an option, because userspace cannot realistically
> +handle it.  Signal handlers are global, which makes them extremely
> +difficult to use correctly from libraries like those that Mesa produces.
> +Signal handlers are not composable, you can't have different handlers
> +for GPU1 and GPU2 from different vendors, and a third handler for
> +mmapped regular files.  Threads cause additional pain with signal
> +handling as well.
> +
>   .. _drm_driver_ioctl:
>   
>   IOCTL Support on Device Nodes
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] drm/doc: device hot-unplug for userspace
  2020-05-20 14:50       ` Andrey Grodzovsky
  2020-05-25 12:13         ` Andrey Grodzovsky
@ 2020-05-25 14:28         ` Daniel Vetter
  2020-05-25 14:55           ` Pekka Paalanen
  1 sibling, 1 reply; 30+ messages in thread
From: Daniel Vetter @ 2020-05-25 14:28 UTC (permalink / raw)
  To: Andrey Grodzovsky; +Cc: Dave Airlie, Sean Paul, dri-devel

On Wed, May 20, 2020 at 10:50:41AM -0400, Andrey Grodzovsky wrote:
> 
> On 5/20/20 8:46 AM, Daniel Vetter wrote:
> > On Wed, May 20, 2020 at 02:19:08PM +0300, Pekka Paalanen wrote:
> > > On Tue, 19 May 2020 10:37:12 -0400
> > > Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> wrote:
> > > 
> > > > Thanks for the summary, does put things in order and makes it easier to
> > > > comprehend all the TODOs, some questions bellow
> > > > 
> > > > On 5/19/20 6:06 AM, Pekka Paalanen wrote:
> > > > > From: Pekka Paalanen <pekka.paalanen@collabora.com>
> > > > > 
> > > > > Set up the expectations on how hot-unplugging a DRM device should look like to
> > > > > userspace.
> > > > > 
> > > > > Written by Daniel Vetter's request and largely based on his comments in IRC and
> > > > > from https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Fdri-devel%2F2020-May%2F265484.html&amp;data=02%7C01%7CAndrey.Grodzovsky%40amd.com%7Ca4da241ff1e54610d95508d7fcbbcc11%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637255755828209496&amp;sdata=YDFoP2g3z3IiB77sRvAmPB%2Fix%2FV0Mh78YcCSAAlhXdg%3D&amp;reserved=0 .
> > > > > 
> > > > > Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> > > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > > Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> > > > > Cc: Dave Airlie <airlied@redhat.com>
> > > > > Cc: Sean Paul <sean@poorly.run>
> > > > > 
> > > > > ---
> > > > > 
> > > > > Disclaimer: I am a userspace developer writing for other userspace developers.
> > > > > I took some liberties in defining what should happen without knowing what is
> > > > > actually possible or what existing drivers already implement.
> > > > > ---
> > > > >    Documentation/gpu/drm-uapi.rst | 75 ++++++++++++++++++++++++++++++++++
> > > > >    1 file changed, 75 insertions(+)
> > > > > 
> > > > > diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> > > > > index 56fec6ed1ad8..80db4abd2cbd 100644
> > > > > --- a/Documentation/gpu/drm-uapi.rst
> > > > > +++ b/Documentation/gpu/drm-uapi.rst
> > > > > @@ -1,3 +1,5 @@
> > > > > +.. Copyright 2020 DisplayLink (UK) Ltd.
> > > > > +
> > > > >    ===================
> > > > >    Userland interfaces
> > > > >    ===================
> > > > > @@ -162,6 +164,79 @@ other hand, a driver requires shared state between clients which is
> > > > >    visible to user-space and accessible beyond open-file boundaries, they
> > > > >    cannot support render nodes.
> > > > > +Device Hot-Unplug
> > > > > +=================
> > > > > +
> > > > > +.. note::
> > > > > +   The following is the plan. Implementation is not there yet
> > > > > +   (2020 May 13).
> > > > > +
> > > > > +Graphics devices (display and/or render) may be connected via USB (e.g.
> > > > > +display adapters or docking stations) or Thunderbolt (e.g. eGPU). An end
> > > > > +user is able to hot-unplug this kind of devices while they are being
> > > > > +used, and expects that the very least the machine does not crash. Any
> > > > > +damage from hot-unplugging a DRM device needs to be limited as much as
> > > > > +possible and userspace must be given the chance to handle it if it wants
> > > > > +to. Ideally, unplugging a DRM device still lets a desktop to continue
> > > > > +running, but that is going to need explicit support throughout the whole
> > > > > +graphics stack: from kernel and userspace drivers, through display
> > > > > +servers, via window system protocols, and in applications and libraries.
> > > > > +
> > > > > +Other scenarios that should lead to the same are: unrecoverable GPU
> > > > > +crash, PCI device disappearing off the bus, or forced unbind of a driver
> > > > > +from the physical device.
> > > > > +
> > > > > +In other words, from userspace perspective everything needs to keep on
> > > > > +working more or less, until userspace stops using the disappeared DRM
> > > > > +device and closes it completely. Userspace will learn of the device
> > > > > +disappearance from the device removed uevent or in some cases specific
> > > > > +ioctls returning EIO.
> > > > > +
> > > > > +This goal raises at least the following requirements for the kernel and
> > > > > +drivers:
> > > > > +
> > > > > +- The kernel must not hang, crash or oops, no matter what userspace was
> > > > > +  in the middle of doing when the device disappeared.
> > > > > +
> > > > > +- All GPU jobs that can no longer run must have their fences
> > > > > +  force-signalled to avoid inflicting hangs to userspace.
> > > > > +
> > > > > +- KMS connectors must change their status to disconnected.
> > > > > +
> > > > > +- Legacy modesets and pageflips fake success.
> > > > > +
> > > > > +- Atomic commits, both real and TEST_ONLY, fake success.
> > > > 
> > > > Why wouldn't we return -EIO for the atommic commit IOTCL/legasy pflip
> > > > and modeset ioctls here same way as you suggested returning -EIO for
> > > > render ioctl ?
> > > Hi,
> > > 
> > > that is more of a question for Daniel Vetter than me. I believe he is
> > > worried that userspace will get the error handling horribly wrong
> > > anyway, because it needs to be handled in every single display server
> > > project. Render ioctl errors OTOH need to be handled only in the
> > > corresponding Mesa or other userspace driver, and for render there are
> > > API (OpenGL/EGL, Vulkan) specs that say how it must be handled to fill
> > > the API contract. Because of the API contract, those are more likely to
> > > have reasonable error handling in place already.
> > Yup pretty much. Atm compositors expect an -EINVAL (especially for
> > TEST_ONLY), some cope with the semi-spurious -EBUSY we still throw around,
> > but I expect most will just die if the get an -ENOMEM or -EIO or really
> > anything.
> > 
> > I think one area where we've historically thrown some spurious errors is
> > also vblank ioctls, since they don't take full locks and sometimes the
> > kernel needs to sneak in a modeset for some reason (gpu recovery, dp link
> > recovery, ...).
> > 
> > Either way I don't think there's anything the compositor can do than just
> > ignore the error and carry on.
> 
> 
> So currently drm_ioctl will just check for drm_dev_is_unplugged and return
> -ENODEV at the very beginning of the function (https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_ioctl.c#L825)
> meaning that currently we blanket failure any IOCTL in case the device is
> unplagued (ignoring the race condition if the device unplugged after the
> check). So do we want to remove this check and put it only for render ioctls
> (which are those ? e.g. for amdgpu there is  AMDGPU_CS ioctl) but not for
> mode setting/dma_buf_import/dma_buf_export ioctls ? What about other types
> of ioctls which are non of the listed above ?

Hm right, and this goes back all the way to first usb udl support:

commit 2c07a21d6fb0be47fda696a618b726ea258ed1dd
Author: Dave Airlie <airlied@redhat.com>
Date:   Mon Feb 20 14:18:07 2012 +0000

    drm: add core support for unplugging a device (v2)

So I guess we need to change the uapi docs to explain that. Not sure
everyone can cope, but I guess they better do. Since the use-case back
then was just udl, so simple legacy kms only, the damage was probably
rather limited. I'm not sure we can get away with that now, where kms code
has spread to funny places likey vulkan winsys code.

Or maybe we want a file priv flag you can set along the lines of "give me
less shitty hotunplug semantics for ioctls". Or maybe we can just change
the semantics, not crashing&burning shouldn't cause a regression :-)

For everything else (mmap, dma-buf fd, sync_file fd, syncobj fd) I think
the discussion is still more or less accurate.

Pekka, any thoughts?
-Daniel

> 
> Andrey
> 
> 
> > 
> > > I first thought it would be obvious for at least atomic commits to fail
> > > appropriately, but then thinking again, it will only introduce new
> > > failures that are currently very hard to test for (VKMS to the rescue),
> > > and userspace would need to have code to correctly bail out for EIO
> > > rather than attempt fallbacks. The uevent telling the device is gone is
> > > going to come anyway. In between the device disappearance and uevent
> > > handling, it doesn't matter much what happens: the device is abruptly
> > > gone and the user will see a glitch anyway - he knows, he just yanked
> > > out the cable.
> > > 
> > > To me this decision is just to make life in userspace easier by not
> > > introducing a whole new type of error. However, if the kernel did
> > > return EIO instead, it would be impossible for me to see that as a
> > > kernel regression because the old behaviour was probably explosions
> > > everywhere anyway.
> > > 
> > > I heard that UDL and maybe some other drivers in the kernel already
> > > handle hot-unplug somehow. Maybe those are what would regress if the
> > > kernel returned EIO?
> > I think most of the not-explodey drivers currently just (mostly?) it all
> > hw errors. You kinda have to if you actually want to deal with hotunplug,
> > stuff can disappear for any moment without warning anyway. Starting to
> > just skip everything is the simplest approach, plus in large parts of the
> > modeset helpers we don't even return values - kernel drivers suck as much
> > at handling the unexpected errors as userspace :-)
> > -Daniel
> > 
> > > 
> > > > > +
> > > > > +- Pending non-blocking KMS operations deliver the DRM events userspace
> > > > > +  is expecting.
> > > > > +
> > > > > +- If underlying memory disappears, the mmaps are replaced with harmless
> > > > > +  zero pages where access does not raise SIGBUS. Reads return zeros,
> > > > > +  writes are ignored.
> > > > > +
> > > > > +- dmabuf which point to memory that has disappeared are rewritten to
> > > > > +  point to harmless zero pages, similar to mmaps. Imports still succeed
> > > > > +  both ways: an existing device importing a dmabuf pointing to
> > > > > +  disappeared memory, and a disappeared device importing any dmabuf.
> > > > 
> > > > Same as above, I would expect drmPrimeHandleToFD and drmPrimeFDToHandle
> > > > to return -EIO in case device is detached
> > > I imagined a use case behind this: a Wayland compositor attempting to
> > > import a client's dmabuf. The GPU the client was using is
> > > hot-unplugged, meaning the dmabufs lose their storage. It is up to the
> > > client to handle its rendering GPU disappearance on its own, but also
> > > the compositor should not disconnect it because the GPU disappeared.
> > > It's not the client's fault the GPU disappeared.
> > > 
> > > In Wayland, failure to use a wl_buffer is considered a protocol error,
> > > and protocol errors are always fatal: the client is disconnected. The
> > > rationale is that the compositor must always be able to present the
> > > client buffer somehow. If the compositor cannot, then the client did
> > > not obey the protocol.
> > > 
> > > The fallback presentation path in a compositor is usually importing the
> > > dmabuf to EGL, to be sampled from OpenGL. Normally the protocol
> > > guarantees that this works, so any failure to do so is a protocol
> > > violation. But if the GPU used by the client suddenly disappears and
> > > the imports start to fail, that is interpreted as a protocol violation
> > > unless the compositor can see why the import failed. Since the import
> > > is done via EGL, getting the right error code plumbed through from
> > > libdrm functions to the EGL caller would be a hassle. I don't see any
> > > error code in EGL_EXT_image_dma_buf_import reserved for "the dmabuf
> > > storage was hot-unplugged", and I doubt there is anything exclusively
> > > for only that in the EGL base spec either.
> > > 
> > > The cost of lying that the import worked is that the compositor will
> > > paint black or transparent where the window was supposed to be. It's a
> > > graphical glitch that is contrary to the Wayland design principles, but
> > > in this case a glitch is unavoidable: even if the compositor knew this
> > > buffer is now bad, what would it paint instead? It has nothing else to
> > > paint from. I'm assuming the compositor is using a different GPU than
> > > what disappeared.
> > > 
> > > Ideally, the client will eventually react to losing the GPU and either
> > > crash, quit, or switch its rendering to something that works which
> > > simply gives the compositor a new, working buffer without losing any
> > > window state in the process. If we risk the compositor disconnecting
> > > the client, then the client might not recover even if it wanted to.
> > > 
> > > > > +
> > > > > +- Render ioctls return EIO which is then handled in userspace drivers,
> > > > > +  e.g. Mesa, to have the device disappearance handled in the way
> > > > > +  specified for each API (OpenGL, GL ES: GL_KHR_robustness;
> > > > > +  Vulkan: VK_ERROR_DEVICE_LOST; etc.)
> > > > 
> > > > As far as I understood from our discussion we would reject any IOCTL
> > > > aimed at a device which is gone and not only render ioctls.
> > > Daniel?
> > > 
> > > 
> > > Thanks,
> > > pq
> > > 
> > > > Andrey
> > > > 
> > > > 
> > > > > +
> > > > > +Raising SIGBUS is not an option, because userspace cannot realistically
> > > > > +handle it.  Signal handlers are global, which makes them extremely
> > > > > +difficult to use correctly from libraries like Mesa produces. Signal
> > > > > +handlers are not composable, you can't have different handlers for GPU1
> > > > > +and GPU2 from different vendors, and a third handler for mmapped regular
> > > > > +files.  Threads cause additional pain with signal handling as well.
> > > > > +
> > > > > +Only after userspace has closed all relevant DRM device and dmabuf file
> > > > > +descriptors and removed all mmaps, the DRM driver can tear down its
> > > > > +instance for the device that no longer exists. If the same physical
> > > > > +device somehow comes back in the mean time, it shall be a new DRM
> > > > > +device.
> > > > > +
> > > > >    .. _drm_driver_ioctl:
> > > > >    IOCTL Support on Device Nodes
> > 
> > 

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] drm/doc: device hot-unplug for userspace
  2020-05-22  9:54       ` Pekka Paalanen
@ 2020-05-25 14:29         ` Daniel Vetter
  0 siblings, 0 replies; 30+ messages in thread
From: Daniel Vetter @ 2020-05-25 14:29 UTC (permalink / raw)
  To: Pekka Paalanen; +Cc: Sean Paul, dri-devel, Dave Airlie

On Fri, May 22, 2020 at 12:54:32PM +0300, Pekka Paalanen wrote:
> On Wed, 20 May 2020 16:19:00 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Wed, May 20, 2020 at 3:20 PM Simon Ser <contact@emersion.fr> wrote:
> > >
> > > On Wednesday, May 20, 2020 2:55 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > >  
> > > > Maybe we should add an explicit note that there's no guarantee about the
> > > > new chardev minor this new device will get, it could both inherit the
> > > > existing one (you can't open the old one anymore anyway) or get a new one?
> > > >
> > > > Or does userspace want a guarantee, i.e. as long as there's still a handle
> > > > open kernel guarantees to not recycle the chardev minor (which is what we
> > > > currently do). In that case better to add that to your list of guarantees
> > > > above.  
> > >
> > > The are race conditions to consider too, e.g.
> > >
> > > - Compositor sends /dev/dri/card0 to a client
> > > - card0 goes away
> > > - Another device takes card0
> > > - Client receives /dev/dri/card0 and then starts using it, but it's the
> > >   wrong device  
> > 
> > Oh reminds me, what should we do about open() - that one will fail,
> > the chardev is going away after all, not failing kinda doesn't make
> > sense. And more tricky, about creating new leases?
> > 
> > I think reasonable semantics here would be that both of these "create
> > a new open drm fd" operations can fail as soon as the device is
> > unplugged. Userspace needs to be able to deal with that.
> 
> Hi,
> 
> yeah, we can make mmap read/write end result undefined, recycle char
> minors like pids, and let new open()s and new leases fail. Pretty much
> everything Daniel and Simon said make sense to me.
> 
> I'll spin a v2, but maybe next week.
> 
> What about the drm_ioctl() issue Andrey pointed out?

Dropped some thoughts there, tbh dunno, need some more discussions?
-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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] drm/doc: device hot-unplug for userspace
  2020-05-25 13:51   ` Andrey Grodzovsky
@ 2020-05-25 14:30     ` Daniel Vetter
  2020-05-25 15:02       ` Pekka Paalanen
  2020-05-25 14:41     ` Pekka Paalanen
  1 sibling, 1 reply; 30+ messages in thread
From: Daniel Vetter @ 2020-05-25 14:30 UTC (permalink / raw)
  To: Andrey Grodzovsky; +Cc: Pekka Paalanen, dri-devel, Dave Airlie, Sean Paul

On Mon, May 25, 2020 at 09:51:30AM -0400, Andrey Grodzovsky wrote:
> On 5/25/20 8:46 AM, Pekka Paalanen wrote:
> 
> > From: Pekka Paalanen <pekka.paalanen@collabora.com>
> > 
> > Set up the expectations on how hot-unplugging a DRM device should look like to
> > userspace.
> > 
> > Written by Daniel Vetter's request and largely based on his comments in IRC and
> > from https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Fdri-devel%2F2020-May%2F265484.html&amp;data=02%7C01%7Candrey.grodzovsky%40amd.com%7Cc9676f35bbdf4d5a052808d800a9b517%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637260076178891269&amp;sdata=tbOTr7TfESohEgWspomM1sbMq4U4n7bOvdS6JlYifmM%3D&amp;reserved=0 .
> > 
> > Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> > Cc: Dave Airlie <airlied@redhat.com>
> > Cc: Sean Paul <sean@poorly.run>
> > Cc: Simon Ser <contact@emersion.fr>
> > 
> > ---
> > 
> > v2:
> > - mmap reads/writes undefined (danvet)
> > - make render ioctl behaviour driver-specific (danvet)
> > - restructure the mmap paragraphs (danvet)
> > - chardev minor notes (Simon)
> > - open behaviour (danvet)
> > - DRM leasing behaviour (danvet)
> > - added links
> > 
> > Disclaimer: I am a userspace developer writing for other userspace developers.
> > I took some liberties in defining what should happen without knowing what is
> > actually possible or what existing drivers already implement.
> > ---
> >   Documentation/gpu/drm-uapi.rst | 102 +++++++++++++++++++++++++++++++++
> >   1 file changed, 102 insertions(+)
> > 
> > diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> > index 56fec6ed1ad8..520b8e640ad1 100644
> > --- a/Documentation/gpu/drm-uapi.rst
> > +++ b/Documentation/gpu/drm-uapi.rst
> > @@ -1,3 +1,5 @@
> > +.. Copyright 2020 DisplayLink (UK) Ltd.
> > +
> >   ===================
> >   Userland interfaces
> >   ===================
> > @@ -162,6 +164,106 @@ other hand, a driver requires shared state between clients which is
> >   visible to user-space and accessible beyond open-file boundaries, they
> >   cannot support render nodes.
> > +Device Hot-Unplug
> > +=================
> > +
> > +.. note::
> > +   The following is the plan. Implementation is not there yet
> > +   (2020 May).
> > +
> > +Graphics devices (display and/or render) may be connected via USB (e.g.
> > +display adapters or docking stations) or Thunderbolt (e.g. eGPU). An end
> > +user is able to hot-unplug this kind of devices while they are being
> > +used, and expects that the very least the machine does not crash. Any
> > +damage from hot-unplugging a DRM device needs to be limited as much as
> > +possible and userspace must be given the chance to handle it if it wants
> > +to. Ideally, unplugging a DRM device still lets a desktop to continue
> > +running, but that is going to need explicit support throughout the whole
> > +graphics stack: from kernel and userspace drivers, through display
> > +servers, via window system protocols, and in applications and libraries.
> 
> So to support all the requirements in this document only kernel changes
> should be enough and no changes are required from user mode part of the
> stack ?
> 
> > +
> > +Other scenarios that should lead to the same are: unrecoverable GPU
> > +crash, PCI device disappearing off the bus, or forced unbind of a driver
> > +from the physical device.
> > +
> > +In other words, from userspace perspective everything needs to keep on
> > +working more or less, until userspace stops using the disappeared DRM
> > +device and closes it completely. Userspace will learn of the device
> > +disappearance from the device removed uevent
> 
> 
> Is this uevent already implemented ? Can you point me to the code ?
> 
> 
> > or in some cases
> > +driver-specific ioctls returning EIO.
> > +
> > +Only after userspace has closed all relevant DRM device and dmabuf file
> > +descriptors and removed all mmaps, the DRM driver can tear down its
> > +instance for the device that no longer exists. If the same physical
> > +device somehow comes back in the mean time, it shall be a new DRM
> > +device.
> > +
> > +Similar to PIDs, chardev minor numbers are not recycled immediately. A
> > +new DRM device always picks the next free minor number compared to the
> > +previous one allocated, and wraps around when minor numbers are
> > +exhausted.
> > +
> > +Requirements for UAPI
> > +---------------------
> > +
> > +The goal raises at least the following requirements for the kernel and
> > +drivers:
> > +
> > +- The kernel must not hang, crash or oops, no matter what userspace was
> > +  in the middle of doing when the device disappeared.
> > +
> > +- All GPU jobs that can no longer run must have their fences
> > +  force-signalled to avoid inflicting hangs to userspace.
> > +
> > +- KMS connectors must change their status to disconnected.
> > +
> > +- Legacy modesets and pageflips fake success.
> > +
> > +- Atomic commits, both real and TEST_ONLY, fake success.
> > +
> > +- Pending non-blocking KMS operations deliver the DRM events userspace
> > +  is expecting.
> 
> 
> The 4 points above refer to mode setting/display attached card and are
> irrelevant for secondary GPU (e.g. DRI-PRIME scenario) or no display system
> in general. Maybe we can somehow highlight this in the document and I on the
> implementing side can then decide as a first step to concentrate on
> implementing the non display case as a first step or the only step. In
> general and correct me if I am wrong, render only GPUs (or compute only) are
> the majority of cases where you would want to be able to detach/attach GPU
> on the fly (e.g attach stronger secondary graphic card to a laptop to get
> high performance in a game or add/remove a GPU to/from a compute cluster)

Yeah maybe splitting this up into kms section, and rendering/cross driver
section (the dma-buf/fence stuff is relevant for both display and
rendering) would make some sense.
-Daniel

> 
> Andrey
> 
> 
> > +
> > +- dmabuf which point to memory that has disappeared will continue to
> > +  be successfully imported if it would have succeeded before the
> > +  disappearance.
> > +
> > +- Attempting to import a dmabuf to a disappeared device will succeed if
> > +  it would have succeeded without the disappearance.
> > +
> > +- Some userspace APIs already define what should happen when the device
> > +  disappears (OpenGL, GL ES: `GL_KHR_robustness`_; `Vulkan`_:
> > +  VK_ERROR_DEVICE_LOST; etc.). DRM drivers are free to implement this
> > +  behaviour the way they see best, e.g. returning failures in
> > +  driver-specific ioctls and handling those in userspace drivers, or
> > +  rely on uevents, and so on.
> > +
> > +- open() on a device node whose underlying device has disappeared will
> > +  fail.
> > +
> > +- Attempting to create a DRM lease on a disappeared DRM device will
> > +  fail. Existing DRM leases remain.
> > +
> > +.. _GL_KHR_robustness: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.khronos.org%2Fregistry%2FOpenGL%2Fextensions%2FKHR%2FKHR_robustness.txt&amp;data=02%7C01%7Candrey.grodzovsky%40amd.com%7Cc9676f35bbdf4d5a052808d800a9b517%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637260076178891269&amp;sdata=m%2FneRusoe6qGVU8Edk%2FncaD7eSJZXtPnA1IqLr7k%2Bos%3D&amp;reserved=0
> > +.. _Vulkan: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.khronos.org%2Fvulkan%2F&amp;data=02%7C01%7Candrey.grodzovsky%40amd.com%7Cc9676f35bbdf4d5a052808d800a9b517%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637260076178901265&amp;sdata=WsfLduUBzRKlybOJb5PQViBWYu5DgleEeycmf76l3UU%3D&amp;reserved=0
> > +
> > +Requirements for memory maps
> > +----------------------------
> > +
> > +Memory maps have further requirements. If the underlying memory
> > +disappears, the mmap is modified such that reads and writes will still
> > +complete successfully but the result is undefined. This applies to both
> > +userspace mmap()'d memory and memory pointed to by dmabuf which might be
> > +mapped to other devices.
> > +
> > +Raising SIGBUS is not an option, because userspace cannot realistically
> > +handle it.  Signal handlers are global, which makes them extremely
> > +difficult to use correctly from libraries like those that Mesa produces.
> > +Signal handlers are not composable, you can't have different handlers
> > +for GPU1 and GPU2 from different vendors, and a third handler for
> > +mmapped regular files.  Threads cause additional pain with signal
> > +handling as well.
> > +
> >   .. _drm_driver_ioctl:
> >   IOCTL Support on Device Nodes

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] drm/doc: device hot-unplug for userspace
  2020-05-25 13:51   ` Andrey Grodzovsky
  2020-05-25 14:30     ` Daniel Vetter
@ 2020-05-25 14:41     ` Pekka Paalanen
  1 sibling, 0 replies; 30+ messages in thread
From: Pekka Paalanen @ 2020-05-25 14:41 UTC (permalink / raw)
  To: Andrey Grodzovsky; +Cc: Dave Airlie, Sean Paul, dri-devel

[-- Attachment #1.1: Type: text/plain, Size: 9029 bytes --]

On Mon, 25 May 2020 09:51:30 -0400
Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> wrote:

> On 5/25/20 8:46 AM, Pekka Paalanen wrote:
> 
> > From: Pekka Paalanen <pekka.paalanen@collabora.com>
> >
> > Set up the expectations on how hot-unplugging a DRM device should look like to
> > userspace.
> >
> > Written by Daniel Vetter's request and largely based on his comments in IRC and
> > from https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Fdri-devel%2F2020-May%2F265484.html&amp;data=02%7C01%7Candrey.grodzovsky%40amd.com%7Cc9676f35bbdf4d5a052808d800a9b517%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637260076178891269&amp;sdata=tbOTr7TfESohEgWspomM1sbMq4U4n7bOvdS6JlYifmM%3D&amp;reserved=0 .
> >
> > Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> > Cc: Dave Airlie <airlied@redhat.com>
> > Cc: Sean Paul <sean@poorly.run>
> > Cc: Simon Ser <contact@emersion.fr>
> >
> > ---
> >
> > v2:
> > - mmap reads/writes undefined (danvet)
> > - make render ioctl behaviour driver-specific (danvet)
> > - restructure the mmap paragraphs (danvet)
> > - chardev minor notes (Simon)
> > - open behaviour (danvet)
> > - DRM leasing behaviour (danvet)
> > - added links
> >
> > Disclaimer: I am a userspace developer writing for other userspace developers.
> > I took some liberties in defining what should happen without knowing what is
> > actually possible or what existing drivers already implement.
> > ---
> >   Documentation/gpu/drm-uapi.rst | 102 +++++++++++++++++++++++++++++++++
> >   1 file changed, 102 insertions(+)
> >
> > diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> > index 56fec6ed1ad8..520b8e640ad1 100644
> > --- a/Documentation/gpu/drm-uapi.rst
> > +++ b/Documentation/gpu/drm-uapi.rst
> > @@ -1,3 +1,5 @@
> > +.. Copyright 2020 DisplayLink (UK) Ltd.
> > +
> >   ===================
> >   Userland interfaces
> >   ===================
> > @@ -162,6 +164,106 @@ other hand, a driver requires shared state between clients which is
> >   visible to user-space and accessible beyond open-file boundaries, they
> >   cannot support render nodes.
> >   
> > +Device Hot-Unplug
> > +=================
> > +
> > +.. note::
> > +   The following is the plan. Implementation is not there yet
> > +   (2020 May).
> > +
> > +Graphics devices (display and/or render) may be connected via USB (e.g.
> > +display adapters or docking stations) or Thunderbolt (e.g. eGPU). An end
> > +user is able to hot-unplug this kind of devices while they are being
> > +used, and expects that the very least the machine does not crash. Any
> > +damage from hot-unplugging a DRM device needs to be limited as much as
> > +possible and userspace must be given the chance to handle it if it wants
> > +to. Ideally, unplugging a DRM device still lets a desktop to continue
> > +running, but that is going to need explicit support throughout the whole
> > +graphics stack: from kernel and userspace drivers, through display
> > +servers, via window system protocols, and in applications and libraries.  
> 
> So to support all the requirements in this document only kernel changes 
> should be enough and no changes are required from user mode part of the 
> stack ?

Hi,

my intention is that this document describes what the kernel delivers,
or should deliver, to allow userspace to cope with hot-unplug if
userspace wishes to do so. "Userspace" here includes userspace part of
GPU drivers.

Userspace has a lot to develop to actually recover instead of just sit
in the dark after the device disappears. Handling the uevent for DRM
device removal or errors from GL/Vulkan is just the beginning of it. I
would assume that userspace drivers have things to implement as well,
before GL or Vulkan apps can recover instead of get stuck or crash.

Unplugging "secondary" DRM devices (mostly used for KMS to have more
monitors lit) should be relatively easy to implement in display
servers. Unplugging the GPU that a display server is using for
rendering is going to be really difficult and will need client
(application toolkit) support the very least, and perhaps even new
window system protocol.

I imagine this will be incremental development: first the kernel stops
crashing. Then display servers stop crashing. At some point userspace
GPU drivers stop crashing. Then display servers learn to recover
instead of sit in the dark, but disconnect most of their clients. Then
maybe with the help of window system protocol additions, some major
toolkits learn to not get killed. And so on.

Once all that works, the follow-up step could be some protocol to
switch applications from one GPU to another in flight. That's off-topic
here, but being able to handle GPU unplug is half of the switch.

> > +
> > +Other scenarios that should lead to the same are: unrecoverable GPU
> > +crash, PCI device disappearing off the bus, or forced unbind of a driver
> > +from the physical device.
> > +
> > +In other words, from userspace perspective everything needs to keep on
> > +working more or less, until userspace stops using the disappeared DRM
> > +device and closes it completely. Userspace will learn of the device
> > +disappearance from the device removed uevent  
> 
> 
> Is this uevent already implemented ? Can you point me to the code ?

I can't point to any kernel code, I'm just not familiar with it. But
it's the same uevent all Linux devices emit. You unplug a USB mouse,
this is the event that gets sent.

You can emulate it with 'udevadm trigger -c remove' IIRC, and it is the
"remove" event you can match in udev rules.

KMS hotplug event is also a uevent, but I think it is "change" rather
than "remove". Otherwise the same mechanism. Display servers already
watch for uevents to learn about monitor hotplug, and some watch for
DRM device added events too. But I don't think any really watch for DRM
device removed events, because usually everything explodes first. I
don't know, maybe X.org handles UDL unplugs?

> > or in some cases
> > +driver-specific ioctls returning EIO.
> > +
> > +Only after userspace has closed all relevant DRM device and dmabuf file
> > +descriptors and removed all mmaps, the DRM driver can tear down its
> > +instance for the device that no longer exists. If the same physical
> > +device somehow comes back in the mean time, it shall be a new DRM
> > +device.
> > +
> > +Similar to PIDs, chardev minor numbers are not recycled immediately. A
> > +new DRM device always picks the next free minor number compared to the
> > +previous one allocated, and wraps around when minor numbers are
> > +exhausted.
> > +
> > +Requirements for UAPI
> > +---------------------
> > +
> > +The goal raises at least the following requirements for the kernel and
> > +drivers:
> > +
> > +- The kernel must not hang, crash or oops, no matter what userspace was
> > +  in the middle of doing when the device disappeared.
> > +
> > +- All GPU jobs that can no longer run must have their fences
> > +  force-signalled to avoid inflicting hangs to userspace.
> > +
> > +- KMS connectors must change their status to disconnected.
> > +
> > +- Legacy modesets and pageflips fake success.
> > +
> > +- Atomic commits, both real and TEST_ONLY, fake success.
> > +
> > +- Pending non-blocking KMS operations deliver the DRM events userspace
> > +  is expecting.  
> 
> 
> The 4 points above refer to mode setting/display attached card and are 
> irrelevant for secondary GPU (e.g. DRI-PRIME scenario) or no display 
> system in general. Maybe we can somehow highlight this in the document 
> and I on the implementing side can then decide as a first step to 
> concentrate on implementing the non display case as a first step or the 
> only step. In general and correct me if I am wrong, render only GPUs (or 
> compute only) are the majority of cases where you would want to be able 
> to detach/attach GPU on the fly (e.g attach stronger secondary graphic 
> card to a laptop to get high performance in a game or add/remove a GPU 
> to/from a compute cluster)

I do think KMS-only (not rendering) devices are a major use case for
hot-unplug: docks, USB-display-adapters etc. I wrote this patch on
behalf of DisplayLink after all.

Render-only GPUs are another important use case like you describe. And
a dock might perhaps have both: a powerful GPU and a big screen
connected.

Personally, I have no expectations other than a hope that some day at
least the drivers that support hot-unpluggable hardware would implement
all of this. :-)

I would assume it's fine to work piece by piece towards the goal on
your own pace. This patch here is just for setting up the goal without
a deadline. I'm no DRM maintainer or even a DRM developer.


Thanks,
pq

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] drm/doc: device hot-unplug for userspace
  2020-05-25 14:28         ` Daniel Vetter
@ 2020-05-25 14:55           ` Pekka Paalanen
  2020-05-25 15:09             ` Daniel Vetter
  0 siblings, 1 reply; 30+ messages in thread
From: Pekka Paalanen @ 2020-05-25 14:55 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Sean Paul, dri-devel, Dave Airlie

[-- Attachment #1.1: Type: text/plain, Size: 16141 bytes --]

On Mon, 25 May 2020 16:28:04 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Wed, May 20, 2020 at 10:50:41AM -0400, Andrey Grodzovsky wrote:
> > 
> > On 5/20/20 8:46 AM, Daniel Vetter wrote:  
> > > On Wed, May 20, 2020 at 02:19:08PM +0300, Pekka Paalanen wrote:  
> > > > On Tue, 19 May 2020 10:37:12 -0400
> > > > Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> wrote:
> > > >   
> > > > > Thanks for the summary, does put things in order and makes it easier to
> > > > > comprehend all the TODOs, some questions bellow
> > > > > 
> > > > > On 5/19/20 6:06 AM, Pekka Paalanen wrote:  
> > > > > > From: Pekka Paalanen <pekka.paalanen@collabora.com>
> > > > > > 
> > > > > > Set up the expectations on how hot-unplugging a DRM device should look like to
> > > > > > userspace.
> > > > > > 
> > > > > > Written by Daniel Vetter's request and largely based on his comments in IRC and
> > > > > > from https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Fdri-devel%2F2020-May%2F265484.html&amp;data=02%7C01%7CAndrey.Grodzovsky%40amd.com%7Ca4da241ff1e54610d95508d7fcbbcc11%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637255755828209496&amp;sdata=YDFoP2g3z3IiB77sRvAmPB%2Fix%2FV0Mh78YcCSAAlhXdg%3D&amp;reserved=0 .
> > > > > > 
> > > > > > Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> > > > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > > > Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> > > > > > Cc: Dave Airlie <airlied@redhat.com>
> > > > > > Cc: Sean Paul <sean@poorly.run>
> > > > > > 
> > > > > > ---
> > > > > > 
> > > > > > Disclaimer: I am a userspace developer writing for other userspace developers.
> > > > > > I took some liberties in defining what should happen without knowing what is
> > > > > > actually possible or what existing drivers already implement.
> > > > > > ---
> > > > > >    Documentation/gpu/drm-uapi.rst | 75 ++++++++++++++++++++++++++++++++++
> > > > > >    1 file changed, 75 insertions(+)
> > > > > > 
> > > > > > diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> > > > > > index 56fec6ed1ad8..80db4abd2cbd 100644
> > > > > > --- a/Documentation/gpu/drm-uapi.rst
> > > > > > +++ b/Documentation/gpu/drm-uapi.rst
> > > > > > @@ -1,3 +1,5 @@
> > > > > > +.. Copyright 2020 DisplayLink (UK) Ltd.
> > > > > > +
> > > > > >    ===================
> > > > > >    Userland interfaces
> > > > > >    ===================
> > > > > > @@ -162,6 +164,79 @@ other hand, a driver requires shared state between clients which is
> > > > > >    visible to user-space and accessible beyond open-file boundaries, they
> > > > > >    cannot support render nodes.
> > > > > > +Device Hot-Unplug
> > > > > > +=================
> > > > > > +
> > > > > > +.. note::
> > > > > > +   The following is the plan. Implementation is not there yet
> > > > > > +   (2020 May 13).
> > > > > > +
> > > > > > +Graphics devices (display and/or render) may be connected via USB (e.g.
> > > > > > +display adapters or docking stations) or Thunderbolt (e.g. eGPU). An end
> > > > > > +user is able to hot-unplug this kind of devices while they are being
> > > > > > +used, and expects that the very least the machine does not crash. Any
> > > > > > +damage from hot-unplugging a DRM device needs to be limited as much as
> > > > > > +possible and userspace must be given the chance to handle it if it wants
> > > > > > +to. Ideally, unplugging a DRM device still lets a desktop to continue
> > > > > > +running, but that is going to need explicit support throughout the whole
> > > > > > +graphics stack: from kernel and userspace drivers, through display
> > > > > > +servers, via window system protocols, and in applications and libraries.
> > > > > > +
> > > > > > +Other scenarios that should lead to the same are: unrecoverable GPU
> > > > > > +crash, PCI device disappearing off the bus, or forced unbind of a driver
> > > > > > +from the physical device.
> > > > > > +
> > > > > > +In other words, from userspace perspective everything needs to keep on
> > > > > > +working more or less, until userspace stops using the disappeared DRM
> > > > > > +device and closes it completely. Userspace will learn of the device
> > > > > > +disappearance from the device removed uevent or in some cases specific
> > > > > > +ioctls returning EIO.
> > > > > > +
> > > > > > +This goal raises at least the following requirements for the kernel and
> > > > > > +drivers:
> > > > > > +
> > > > > > +- The kernel must not hang, crash or oops, no matter what userspace was
> > > > > > +  in the middle of doing when the device disappeared.
> > > > > > +
> > > > > > +- All GPU jobs that can no longer run must have their fences
> > > > > > +  force-signalled to avoid inflicting hangs to userspace.
> > > > > > +
> > > > > > +- KMS connectors must change their status to disconnected.
> > > > > > +
> > > > > > +- Legacy modesets and pageflips fake success.
> > > > > > +
> > > > > > +- Atomic commits, both real and TEST_ONLY, fake success.  
> > > > > 
> > > > > Why wouldn't we return -EIO for the atommic commit IOTCL/legasy pflip
> > > > > and modeset ioctls here same way as you suggested returning -EIO for
> > > > > render ioctl ?  
> > > > Hi,
> > > > 
> > > > that is more of a question for Daniel Vetter than me. I believe he is
> > > > worried that userspace will get the error handling horribly wrong
> > > > anyway, because it needs to be handled in every single display server
> > > > project. Render ioctl errors OTOH need to be handled only in the
> > > > corresponding Mesa or other userspace driver, and for render there are
> > > > API (OpenGL/EGL, Vulkan) specs that say how it must be handled to fill
> > > > the API contract. Because of the API contract, those are more likely to
> > > > have reasonable error handling in place already.  
> > > Yup pretty much. Atm compositors expect an -EINVAL (especially for
> > > TEST_ONLY), some cope with the semi-spurious -EBUSY we still throw around,
> > > but I expect most will just die if the get an -ENOMEM or -EIO or really
> > > anything.
> > > 
> > > I think one area where we've historically thrown some spurious errors is
> > > also vblank ioctls, since they don't take full locks and sometimes the
> > > kernel needs to sneak in a modeset for some reason (gpu recovery, dp link
> > > recovery, ...).
> > > 
> > > Either way I don't think there's anything the compositor can do than just
> > > ignore the error and carry on.  
> > 
> > 
> > So currently drm_ioctl will just check for drm_dev_is_unplugged and return
> > -ENODEV at the very beginning of the function (https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_ioctl.c#L825)
> > meaning that currently we blanket failure any IOCTL in case the device is
> > unplagued (ignoring the race condition if the device unplugged after the
> > check). So do we want to remove this check and put it only for render ioctls
> > (which are those ? e.g. for amdgpu there is  AMDGPU_CS ioctl) but not for
> > mode setting/dma_buf_import/dma_buf_export ioctls ? What about other types
> > of ioctls which are non of the listed above ?  
> 
> Hm right, and this goes back all the way to first usb udl support:
> 
> commit 2c07a21d6fb0be47fda696a618b726ea258ed1dd
> Author: Dave Airlie <airlied@redhat.com>
> Date:   Mon Feb 20 14:18:07 2012 +0000
> 
>     drm: add core support for unplugging a device (v2)
> 
> So I guess we need to change the uapi docs to explain that. Not sure
> everyone can cope, but I guess they better do. Since the use-case back
> then was just udl, so simple legacy kms only, the damage was probably
> rather limited. I'm not sure we can get away with that now, where kms code
> has spread to funny places likey vulkan winsys code.
> 
> Or maybe we want a file priv flag you can set along the lines of "give me
> less shitty hotunplug semantics for ioctls". Or maybe we can just change
> the semantics, not crashing&burning shouldn't cause a regression :-)
> 
> For everything else (mmap, dma-buf fd, sync_file fd, syncobj fd) I think
> the discussion is still more or less accurate.
> 
> Pekka, any thoughts?

Hi,

is ENODEV unique to this particular failure?

Returning errors instead of faking success was my first idea, but you
already convinced me that faking is at least as good if not better. :-)

So as long as the error code returned is unique to hot-unplug or other
"oops, the device is gone" conditions, I think I'm fine. Weston does
not handle ENODEV any way, it never did, and it certainly cannot be
called a kernel regression.

As a Weston developer, I don't mind adding checks for ENODEV. But if I
don't have to, even better. Weston is going to need more code to handle
DRM device unplug in any case.

Sorry, no preference from me. ;-)

I do agree that replacing ENODEV with fake success is hard to imagine
regressing anything. It's something you can do in the kernel at any
time easily, but going from fake success to error is going to be
painful. Maybe don't change things until there is a good reason to?

We need a kernel that doesn't crash before we can properly test what
would be best for userspace, fake or error.


Thanks,
pq


> -Daniel
> 
> > 
> > Andrey
> > 
> >   
> > >   
> > > > I first thought it would be obvious for at least atomic commits to fail
> > > > appropriately, but then thinking again, it will only introduce new
> > > > failures that are currently very hard to test for (VKMS to the rescue),
> > > > and userspace would need to have code to correctly bail out for EIO
> > > > rather than attempt fallbacks. The uevent telling the device is gone is
> > > > going to come anyway. In between the device disappearance and uevent
> > > > handling, it doesn't matter much what happens: the device is abruptly
> > > > gone and the user will see a glitch anyway - he knows, he just yanked
> > > > out the cable.
> > > > 
> > > > To me this decision is just to make life in userspace easier by not
> > > > introducing a whole new type of error. However, if the kernel did
> > > > return EIO instead, it would be impossible for me to see that as a
> > > > kernel regression because the old behaviour was probably explosions
> > > > everywhere anyway.
> > > > 
> > > > I heard that UDL and maybe some other drivers in the kernel already
> > > > handle hot-unplug somehow. Maybe those are what would regress if the
> > > > kernel returned EIO?  
> > > I think most of the not-explodey drivers currently just (mostly?) it all
> > > hw errors. You kinda have to if you actually want to deal with hotunplug,
> > > stuff can disappear for any moment without warning anyway. Starting to
> > > just skip everything is the simplest approach, plus in large parts of the
> > > modeset helpers we don't even return values - kernel drivers suck as much
> > > at handling the unexpected errors as userspace :-)
> > > -Daniel
> > >   
> > > >   
> > > > > > +
> > > > > > +- Pending non-blocking KMS operations deliver the DRM events userspace
> > > > > > +  is expecting.
> > > > > > +
> > > > > > +- If underlying memory disappears, the mmaps are replaced with harmless
> > > > > > +  zero pages where access does not raise SIGBUS. Reads return zeros,
> > > > > > +  writes are ignored.
> > > > > > +
> > > > > > +- dmabuf which point to memory that has disappeared are rewritten to
> > > > > > +  point to harmless zero pages, similar to mmaps. Imports still succeed
> > > > > > +  both ways: an existing device importing a dmabuf pointing to
> > > > > > +  disappeared memory, and a disappeared device importing any dmabuf.  
> > > > > 
> > > > > Same as above, I would expect drmPrimeHandleToFD and drmPrimeFDToHandle
> > > > > to return -EIO in case device is detached  
> > > > I imagined a use case behind this: a Wayland compositor attempting to
> > > > import a client's dmabuf. The GPU the client was using is
> > > > hot-unplugged, meaning the dmabufs lose their storage. It is up to the
> > > > client to handle its rendering GPU disappearance on its own, but also
> > > > the compositor should not disconnect it because the GPU disappeared.
> > > > It's not the client's fault the GPU disappeared.
> > > > 
> > > > In Wayland, failure to use a wl_buffer is considered a protocol error,
> > > > and protocol errors are always fatal: the client is disconnected. The
> > > > rationale is that the compositor must always be able to present the
> > > > client buffer somehow. If the compositor cannot, then the client did
> > > > not obey the protocol.
> > > > 
> > > > The fallback presentation path in a compositor is usually importing the
> > > > dmabuf to EGL, to be sampled from OpenGL. Normally the protocol
> > > > guarantees that this works, so any failure to do so is a protocol
> > > > violation. But if the GPU used by the client suddenly disappears and
> > > > the imports start to fail, that is interpreted as a protocol violation
> > > > unless the compositor can see why the import failed. Since the import
> > > > is done via EGL, getting the right error code plumbed through from
> > > > libdrm functions to the EGL caller would be a hassle. I don't see any
> > > > error code in EGL_EXT_image_dma_buf_import reserved for "the dmabuf
> > > > storage was hot-unplugged", and I doubt there is anything exclusively
> > > > for only that in the EGL base spec either.
> > > > 
> > > > The cost of lying that the import worked is that the compositor will
> > > > paint black or transparent where the window was supposed to be. It's a
> > > > graphical glitch that is contrary to the Wayland design principles, but
> > > > in this case a glitch is unavoidable: even if the compositor knew this
> > > > buffer is now bad, what would it paint instead? It has nothing else to
> > > > paint from. I'm assuming the compositor is using a different GPU than
> > > > what disappeared.
> > > > 
> > > > Ideally, the client will eventually react to losing the GPU and either
> > > > crash, quit, or switch its rendering to something that works which
> > > > simply gives the compositor a new, working buffer without losing any
> > > > window state in the process. If we risk the compositor disconnecting
> > > > the client, then the client might not recover even if it wanted to.
> > > >   
> > > > > > +
> > > > > > +- Render ioctls return EIO which is then handled in userspace drivers,
> > > > > > +  e.g. Mesa, to have the device disappearance handled in the way
> > > > > > +  specified for each API (OpenGL, GL ES: GL_KHR_robustness;
> > > > > > +  Vulkan: VK_ERROR_DEVICE_LOST; etc.)  
> > > > > 
> > > > > As far as I understood from our discussion we would reject any IOCTL
> > > > > aimed at a device which is gone and not only render ioctls.  
> > > > Daniel?
> > > > 
> > > > 
> > > > Thanks,
> > > > pq
> > > >   
> > > > > Andrey
> > > > > 
> > > > >   
> > > > > > +
> > > > > > +Raising SIGBUS is not an option, because userspace cannot realistically
> > > > > > +handle it.  Signal handlers are global, which makes them extremely
> > > > > > +difficult to use correctly from libraries like Mesa produces. Signal
> > > > > > +handlers are not composable, you can't have different handlers for GPU1
> > > > > > +and GPU2 from different vendors, and a third handler for mmapped regular
> > > > > > +files.  Threads cause additional pain with signal handling as well.
> > > > > > +
> > > > > > +Only after userspace has closed all relevant DRM device and dmabuf file
> > > > > > +descriptors and removed all mmaps, the DRM driver can tear down its
> > > > > > +instance for the device that no longer exists. If the same physical
> > > > > > +device somehow comes back in the mean time, it shall be a new DRM
> > > > > > +device.
> > > > > > +
> > > > > >    .. _drm_driver_ioctl:
> > > > > >    IOCTL Support on Device Nodes  
> > > 
> > >   
> 


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH v2] drm/doc: device hot-unplug for userspace
  2020-05-25 14:30     ` Daniel Vetter
@ 2020-05-25 15:02       ` Pekka Paalanen
  0 siblings, 0 replies; 30+ messages in thread
From: Pekka Paalanen @ 2020-05-25 15:02 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Sean Paul, dri-devel, Dave Airlie

[-- Attachment #1.1: Type: text/plain, Size: 10005 bytes --]

On Mon, 25 May 2020 16:30:17 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Mon, May 25, 2020 at 09:51:30AM -0400, Andrey Grodzovsky wrote:
> > On 5/25/20 8:46 AM, Pekka Paalanen wrote:
> >   
> > > From: Pekka Paalanen <pekka.paalanen@collabora.com>
> > > 
> > > Set up the expectations on how hot-unplugging a DRM device should look like to
> > > userspace.
> > > 
> > > Written by Daniel Vetter's request and largely based on his comments in IRC and
> > > from https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Fdri-devel%2F2020-May%2F265484.html&amp;data=02%7C01%7Candrey.grodzovsky%40amd.com%7Cc9676f35bbdf4d5a052808d800a9b517%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637260076178891269&amp;sdata=tbOTr7TfESohEgWspomM1sbMq4U4n7bOvdS6JlYifmM%3D&amp;reserved=0 .
> > > 
> > > Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> > > Cc: Dave Airlie <airlied@redhat.com>
> > > Cc: Sean Paul <sean@poorly.run>
> > > Cc: Simon Ser <contact@emersion.fr>
> > > 
> > > ---
> > > 
> > > v2:
> > > - mmap reads/writes undefined (danvet)
> > > - make render ioctl behaviour driver-specific (danvet)
> > > - restructure the mmap paragraphs (danvet)
> > > - chardev minor notes (Simon)
> > > - open behaviour (danvet)
> > > - DRM leasing behaviour (danvet)
> > > - added links
> > > 
> > > Disclaimer: I am a userspace developer writing for other userspace developers.
> > > I took some liberties in defining what should happen without knowing what is
> > > actually possible or what existing drivers already implement.
> > > ---
> > >   Documentation/gpu/drm-uapi.rst | 102 +++++++++++++++++++++++++++++++++
> > >   1 file changed, 102 insertions(+)
> > > 
> > > diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> > > index 56fec6ed1ad8..520b8e640ad1 100644
> > > --- a/Documentation/gpu/drm-uapi.rst
> > > +++ b/Documentation/gpu/drm-uapi.rst
> > > @@ -1,3 +1,5 @@
> > > +.. Copyright 2020 DisplayLink (UK) Ltd.
> > > +
> > >   ===================
> > >   Userland interfaces
> > >   ===================
> > > @@ -162,6 +164,106 @@ other hand, a driver requires shared state between clients which is
> > >   visible to user-space and accessible beyond open-file boundaries, they
> > >   cannot support render nodes.
> > > +Device Hot-Unplug
> > > +=================
> > > +
> > > +.. note::
> > > +   The following is the plan. Implementation is not there yet
> > > +   (2020 May).
> > > +
> > > +Graphics devices (display and/or render) may be connected via USB (e.g.
> > > +display adapters or docking stations) or Thunderbolt (e.g. eGPU). An end
> > > +user is able to hot-unplug this kind of devices while they are being
> > > +used, and expects that the very least the machine does not crash. Any
> > > +damage from hot-unplugging a DRM device needs to be limited as much as
> > > +possible and userspace must be given the chance to handle it if it wants
> > > +to. Ideally, unplugging a DRM device still lets a desktop to continue
> > > +running, but that is going to need explicit support throughout the whole
> > > +graphics stack: from kernel and userspace drivers, through display
> > > +servers, via window system protocols, and in applications and libraries.  
> > 
> > So to support all the requirements in this document only kernel changes
> > should be enough and no changes are required from user mode part of the
> > stack ?
> >   
> > > +
> > > +Other scenarios that should lead to the same are: unrecoverable GPU
> > > +crash, PCI device disappearing off the bus, or forced unbind of a driver
> > > +from the physical device.
> > > +
> > > +In other words, from userspace perspective everything needs to keep on
> > > +working more or less, until userspace stops using the disappeared DRM
> > > +device and closes it completely. Userspace will learn of the device
> > > +disappearance from the device removed uevent  
> > 
> > 
> > Is this uevent already implemented ? Can you point me to the code ?
> > 
> >   
> > > or in some cases
> > > +driver-specific ioctls returning EIO.
> > > +
> > > +Only after userspace has closed all relevant DRM device and dmabuf file
> > > +descriptors and removed all mmaps, the DRM driver can tear down its
> > > +instance for the device that no longer exists. If the same physical
> > > +device somehow comes back in the mean time, it shall be a new DRM
> > > +device.
> > > +
> > > +Similar to PIDs, chardev minor numbers are not recycled immediately. A
> > > +new DRM device always picks the next free minor number compared to the
> > > +previous one allocated, and wraps around when minor numbers are
> > > +exhausted.
> > > +
> > > +Requirements for UAPI
> > > +---------------------
> > > +
> > > +The goal raises at least the following requirements for the kernel and
> > > +drivers:
> > > +
> > > +- The kernel must not hang, crash or oops, no matter what userspace was
> > > +  in the middle of doing when the device disappeared.
> > > +
> > > +- All GPU jobs that can no longer run must have their fences
> > > +  force-signalled to avoid inflicting hangs to userspace.
> > > +
> > > +- KMS connectors must change their status to disconnected.
> > > +
> > > +- Legacy modesets and pageflips fake success.
> > > +
> > > +- Atomic commits, both real and TEST_ONLY, fake success.
> > > +
> > > +- Pending non-blocking KMS operations deliver the DRM events userspace
> > > +  is expecting.  
> > 
> > 
> > The 4 points above refer to mode setting/display attached card and are
> > irrelevant for secondary GPU (e.g. DRI-PRIME scenario) or no display system
> > in general. Maybe we can somehow highlight this in the document and I on the
> > implementing side can then decide as a first step to concentrate on
> > implementing the non display case as a first step or the only step. In
> > general and correct me if I am wrong, render only GPUs (or compute only) are
> > the majority of cases where you would want to be able to detach/attach GPU
> > on the fly (e.g attach stronger secondary graphic card to a laptop to get
> > high performance in a game or add/remove a GPU to/from a compute cluster)  
> 
> Yeah maybe splitting this up into kms section, and rendering/cross driver
> section (the dma-buf/fence stuff is relevant for both display and
> rendering) would make some sense.

Is that really something that needs spelling out?

Hmm. I guess the unwritten assumption on every "fake success" is the
condition that it would have succeeded if the device was not unplugged.

Is the problem here that one might read this as needing to fake success
for things that could never have worked at all? Like KMS on render-only
device.

The dmabuf items below have the wording.

I think splitting stuff into KMS stuff, render stuff, KMS-and-render
stuff, cross-device stuff, and mmaps gets a bit far. Or do you expect a
lot more text in here? Maybe expanding each bullet point to a paragraph?


Thanks,
pq


> -Daniel
> 
> > 
> > Andrey
> > 
> >   
> > > +
> > > +- dmabuf which point to memory that has disappeared will continue to
> > > +  be successfully imported if it would have succeeded before the
> > > +  disappearance.
> > > +
> > > +- Attempting to import a dmabuf to a disappeared device will succeed if
> > > +  it would have succeeded without the disappearance.
> > > +
> > > +- Some userspace APIs already define what should happen when the device
> > > +  disappears (OpenGL, GL ES: `GL_KHR_robustness`_; `Vulkan`_:
> > > +  VK_ERROR_DEVICE_LOST; etc.). DRM drivers are free to implement this
> > > +  behaviour the way they see best, e.g. returning failures in
> > > +  driver-specific ioctls and handling those in userspace drivers, or
> > > +  rely on uevents, and so on.
> > > +
> > > +- open() on a device node whose underlying device has disappeared will
> > > +  fail.
> > > +
> > > +- Attempting to create a DRM lease on a disappeared DRM device will
> > > +  fail. Existing DRM leases remain.
> > > +
> > > +.. _GL_KHR_robustness: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.khronos.org%2Fregistry%2FOpenGL%2Fextensions%2FKHR%2FKHR_robustness.txt&amp;data=02%7C01%7Candrey.grodzovsky%40amd.com%7Cc9676f35bbdf4d5a052808d800a9b517%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637260076178891269&amp;sdata=m%2FneRusoe6qGVU8Edk%2FncaD7eSJZXtPnA1IqLr7k%2Bos%3D&amp;reserved=0
> > > +.. _Vulkan: https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.khronos.org%2Fvulkan%2F&amp;data=02%7C01%7Candrey.grodzovsky%40amd.com%7Cc9676f35bbdf4d5a052808d800a9b517%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637260076178901265&amp;sdata=WsfLduUBzRKlybOJb5PQViBWYu5DgleEeycmf76l3UU%3D&amp;reserved=0
> > > +
> > > +Requirements for memory maps
> > > +----------------------------
> > > +
> > > +Memory maps have further requirements. If the underlying memory
> > > +disappears, the mmap is modified such that reads and writes will still
> > > +complete successfully but the result is undefined. This applies to both
> > > +userspace mmap()'d memory and memory pointed to by dmabuf which might be
> > > +mapped to other devices.
> > > +
> > > +Raising SIGBUS is not an option, because userspace cannot realistically
> > > +handle it.  Signal handlers are global, which makes them extremely
> > > +difficult to use correctly from libraries like those that Mesa produces.
> > > +Signal handlers are not composable, you can't have different handlers
> > > +for GPU1 and GPU2 from different vendors, and a third handler for
> > > +mmapped regular files.  Threads cause additional pain with signal
> > > +handling as well.
> > > +
> > >   .. _drm_driver_ioctl:
> > >   IOCTL Support on Device Nodes  
> 


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] drm/doc: device hot-unplug for userspace
  2020-05-25 14:55           ` Pekka Paalanen
@ 2020-05-25 15:09             ` Daniel Vetter
  2020-05-28 12:27               ` Pekka Paalanen
  0 siblings, 1 reply; 30+ messages in thread
From: Daniel Vetter @ 2020-05-25 15:09 UTC (permalink / raw)
  To: Pekka Paalanen; +Cc: Sean Paul, dri-devel, Dave Airlie

On Mon, May 25, 2020 at 05:55:19PM +0300, Pekka Paalanen wrote:
> On Mon, 25 May 2020 16:28:04 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Wed, May 20, 2020 at 10:50:41AM -0400, Andrey Grodzovsky wrote:
> > > 
> > > On 5/20/20 8:46 AM, Daniel Vetter wrote:  
> > > > On Wed, May 20, 2020 at 02:19:08PM +0300, Pekka Paalanen wrote:  
> > > > > On Tue, 19 May 2020 10:37:12 -0400
> > > > > Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> wrote:
> > > > >   
> > > > > > Thanks for the summary, does put things in order and makes it easier to
> > > > > > comprehend all the TODOs, some questions bellow
> > > > > > 
> > > > > > On 5/19/20 6:06 AM, Pekka Paalanen wrote:  
> > > > > > > From: Pekka Paalanen <pekka.paalanen@collabora.com>
> > > > > > > 
> > > > > > > Set up the expectations on how hot-unplugging a DRM device should look like to
> > > > > > > userspace.
> > > > > > > 
> > > > > > > Written by Daniel Vetter's request and largely based on his comments in IRC and
> > > > > > > from https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Fdri-devel%2F2020-May%2F265484.html&amp;data=02%7C01%7CAndrey.Grodzovsky%40amd.com%7Ca4da241ff1e54610d95508d7fcbbcc11%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637255755828209496&amp;sdata=YDFoP2g3z3IiB77sRvAmPB%2Fix%2FV0Mh78YcCSAAlhXdg%3D&amp;reserved=0 .
> > > > > > > 
> > > > > > > Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> > > > > > > Cc: Daniel Vetter <daniel@ffwll.ch>
> > > > > > > Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> > > > > > > Cc: Dave Airlie <airlied@redhat.com>
> > > > > > > Cc: Sean Paul <sean@poorly.run>
> > > > > > > 
> > > > > > > ---
> > > > > > > 
> > > > > > > Disclaimer: I am a userspace developer writing for other userspace developers.
> > > > > > > I took some liberties in defining what should happen without knowing what is
> > > > > > > actually possible or what existing drivers already implement.
> > > > > > > ---
> > > > > > >    Documentation/gpu/drm-uapi.rst | 75 ++++++++++++++++++++++++++++++++++
> > > > > > >    1 file changed, 75 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> > > > > > > index 56fec6ed1ad8..80db4abd2cbd 100644
> > > > > > > --- a/Documentation/gpu/drm-uapi.rst
> > > > > > > +++ b/Documentation/gpu/drm-uapi.rst
> > > > > > > @@ -1,3 +1,5 @@
> > > > > > > +.. Copyright 2020 DisplayLink (UK) Ltd.
> > > > > > > +
> > > > > > >    ===================
> > > > > > >    Userland interfaces
> > > > > > >    ===================
> > > > > > > @@ -162,6 +164,79 @@ other hand, a driver requires shared state between clients which is
> > > > > > >    visible to user-space and accessible beyond open-file boundaries, they
> > > > > > >    cannot support render nodes.
> > > > > > > +Device Hot-Unplug
> > > > > > > +=================
> > > > > > > +
> > > > > > > +.. note::
> > > > > > > +   The following is the plan. Implementation is not there yet
> > > > > > > +   (2020 May 13).
> > > > > > > +
> > > > > > > +Graphics devices (display and/or render) may be connected via USB (e.g.
> > > > > > > +display adapters or docking stations) or Thunderbolt (e.g. eGPU). An end
> > > > > > > +user is able to hot-unplug this kind of devices while they are being
> > > > > > > +used, and expects that the very least the machine does not crash. Any
> > > > > > > +damage from hot-unplugging a DRM device needs to be limited as much as
> > > > > > > +possible and userspace must be given the chance to handle it if it wants
> > > > > > > +to. Ideally, unplugging a DRM device still lets a desktop to continue
> > > > > > > +running, but that is going to need explicit support throughout the whole
> > > > > > > +graphics stack: from kernel and userspace drivers, through display
> > > > > > > +servers, via window system protocols, and in applications and libraries.
> > > > > > > +
> > > > > > > +Other scenarios that should lead to the same are: unrecoverable GPU
> > > > > > > +crash, PCI device disappearing off the bus, or forced unbind of a driver
> > > > > > > +from the physical device.
> > > > > > > +
> > > > > > > +In other words, from userspace perspective everything needs to keep on
> > > > > > > +working more or less, until userspace stops using the disappeared DRM
> > > > > > > +device and closes it completely. Userspace will learn of the device
> > > > > > > +disappearance from the device removed uevent or in some cases specific
> > > > > > > +ioctls returning EIO.
> > > > > > > +
> > > > > > > +This goal raises at least the following requirements for the kernel and
> > > > > > > +drivers:
> > > > > > > +
> > > > > > > +- The kernel must not hang, crash or oops, no matter what userspace was
> > > > > > > +  in the middle of doing when the device disappeared.
> > > > > > > +
> > > > > > > +- All GPU jobs that can no longer run must have their fences
> > > > > > > +  force-signalled to avoid inflicting hangs to userspace.
> > > > > > > +
> > > > > > > +- KMS connectors must change their status to disconnected.
> > > > > > > +
> > > > > > > +- Legacy modesets and pageflips fake success.
> > > > > > > +
> > > > > > > +- Atomic commits, both real and TEST_ONLY, fake success.  
> > > > > > 
> > > > > > Why wouldn't we return -EIO for the atommic commit IOTCL/legasy pflip
> > > > > > and modeset ioctls here same way as you suggested returning -EIO for
> > > > > > render ioctl ?  
> > > > > Hi,
> > > > > 
> > > > > that is more of a question for Daniel Vetter than me. I believe he is
> > > > > worried that userspace will get the error handling horribly wrong
> > > > > anyway, because it needs to be handled in every single display server
> > > > > project. Render ioctl errors OTOH need to be handled only in the
> > > > > corresponding Mesa or other userspace driver, and for render there are
> > > > > API (OpenGL/EGL, Vulkan) specs that say how it must be handled to fill
> > > > > the API contract. Because of the API contract, those are more likely to
> > > > > have reasonable error handling in place already.  
> > > > Yup pretty much. Atm compositors expect an -EINVAL (especially for
> > > > TEST_ONLY), some cope with the semi-spurious -EBUSY we still throw around,
> > > > but I expect most will just die if the get an -ENOMEM or -EIO or really
> > > > anything.
> > > > 
> > > > I think one area where we've historically thrown some spurious errors is
> > > > also vblank ioctls, since they don't take full locks and sometimes the
> > > > kernel needs to sneak in a modeset for some reason (gpu recovery, dp link
> > > > recovery, ...).
> > > > 
> > > > Either way I don't think there's anything the compositor can do than just
> > > > ignore the error and carry on.  
> > > 
> > > 
> > > So currently drm_ioctl will just check for drm_dev_is_unplugged and return
> > > -ENODEV at the very beginning of the function (https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_ioctl.c#L825)
> > > meaning that currently we blanket failure any IOCTL in case the device is
> > > unplagued (ignoring the race condition if the device unplugged after the
> > > check). So do we want to remove this check and put it only for render ioctls
> > > (which are those ? e.g. for amdgpu there is  AMDGPU_CS ioctl) but not for
> > > mode setting/dma_buf_import/dma_buf_export ioctls ? What about other types
> > > of ioctls which are non of the listed above ?  
> > 
> > Hm right, and this goes back all the way to first usb udl support:
> > 
> > commit 2c07a21d6fb0be47fda696a618b726ea258ed1dd
> > Author: Dave Airlie <airlied@redhat.com>
> > Date:   Mon Feb 20 14:18:07 2012 +0000
> > 
> >     drm: add core support for unplugging a device (v2)
> > 
> > So I guess we need to change the uapi docs to explain that. Not sure
> > everyone can cope, but I guess they better do. Since the use-case back
> > then was just udl, so simple legacy kms only, the damage was probably
> > rather limited. I'm not sure we can get away with that now, where kms code
> > has spread to funny places likey vulkan winsys code.
> > 
> > Or maybe we want a file priv flag you can set along the lines of "give me
> > less shitty hotunplug semantics for ioctls". Or maybe we can just change
> > the semantics, not crashing&burning shouldn't cause a regression :-)
> > 
> > For everything else (mmap, dma-buf fd, sync_file fd, syncobj fd) I think
> > the discussion is still more or less accurate.
> > 
> > Pekka, any thoughts?
> 
> Hi,
> 
> is ENODEV unique to this particular failure?

Not really sure, we'd need to audit all of drm ...

> Returning errors instead of faking success was my first idea, but you
> already convinced me that faking is at least as good if not better. :-)
> 
> So as long as the error code returned is unique to hot-unplug or other
> "oops, the device is gone" conditions, I think I'm fine. Weston does
> not handle ENODEV any way, it never did, and it certainly cannot be
> called a kernel regression.
> 
> As a Weston developer, I don't mind adding checks for ENODEV. But if I
> don't have to, even better. Weston is going to need more code to handle
> DRM device unplug in any case.
> 
> Sorry, no preference from me. ;-)
> 
> I do agree that replacing ENODEV with fake success is hard to imagine
> regressing anything. It's something you can do in the kernel at any
> time easily, but going from fake success to error is going to be
> painful. Maybe don't change things until there is a good reason to?
> 
> We need a kernel that doesn't crash before we can properly test what
> would be best for userspace, fake or error.

One upshot of faking stuff and only bailing in low-level hw code is that
it makes validating the races when you hotunplug easier - if we remove the
early bail-out check even an ioctl later on will look like it raced with
the hotunplug path in the kernel. So better assurance that things won't
blow up badly.

Otoh the early bail out in the top-level ioctl code increases the odds
that you'll survive even on a driver that's totally buggy.

So yeah I guess maybe we should just document that currently you get an
-ENODEV and maybe have the option mentioned that we might change this
going forward. See also

https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#recommended-ioctl-return-values

I think that needs an update, at least clarify that "not present" includes
"no longer present".
-Daniel

> 
> 
> Thanks,
> pq
> 
> 
> > -Daniel
> > 
> > > 
> > > Andrey
> > > 
> > >   
> > > >   
> > > > > I first thought it would be obvious for at least atomic commits to fail
> > > > > appropriately, but then thinking again, it will only introduce new
> > > > > failures that are currently very hard to test for (VKMS to the rescue),
> > > > > and userspace would need to have code to correctly bail out for EIO
> > > > > rather than attempt fallbacks. The uevent telling the device is gone is
> > > > > going to come anyway. In between the device disappearance and uevent
> > > > > handling, it doesn't matter much what happens: the device is abruptly
> > > > > gone and the user will see a glitch anyway - he knows, he just yanked
> > > > > out the cable.
> > > > > 
> > > > > To me this decision is just to make life in userspace easier by not
> > > > > introducing a whole new type of error. However, if the kernel did
> > > > > return EIO instead, it would be impossible for me to see that as a
> > > > > kernel regression because the old behaviour was probably explosions
> > > > > everywhere anyway.
> > > > > 
> > > > > I heard that UDL and maybe some other drivers in the kernel already
> > > > > handle hot-unplug somehow. Maybe those are what would regress if the
> > > > > kernel returned EIO?  
> > > > I think most of the not-explodey drivers currently just (mostly?) it all
> > > > hw errors. You kinda have to if you actually want to deal with hotunplug,
> > > > stuff can disappear for any moment without warning anyway. Starting to
> > > > just skip everything is the simplest approach, plus in large parts of the
> > > > modeset helpers we don't even return values - kernel drivers suck as much
> > > > at handling the unexpected errors as userspace :-)
> > > > -Daniel
> > > >   
> > > > >   
> > > > > > > +
> > > > > > > +- Pending non-blocking KMS operations deliver the DRM events userspace
> > > > > > > +  is expecting.
> > > > > > > +
> > > > > > > +- If underlying memory disappears, the mmaps are replaced with harmless
> > > > > > > +  zero pages where access does not raise SIGBUS. Reads return zeros,
> > > > > > > +  writes are ignored.
> > > > > > > +
> > > > > > > +- dmabuf which point to memory that has disappeared are rewritten to
> > > > > > > +  point to harmless zero pages, similar to mmaps. Imports still succeed
> > > > > > > +  both ways: an existing device importing a dmabuf pointing to
> > > > > > > +  disappeared memory, and a disappeared device importing any dmabuf.  
> > > > > > 
> > > > > > Same as above, I would expect drmPrimeHandleToFD and drmPrimeFDToHandle
> > > > > > to return -EIO in case device is detached  
> > > > > I imagined a use case behind this: a Wayland compositor attempting to
> > > > > import a client's dmabuf. The GPU the client was using is
> > > > > hot-unplugged, meaning the dmabufs lose their storage. It is up to the
> > > > > client to handle its rendering GPU disappearance on its own, but also
> > > > > the compositor should not disconnect it because the GPU disappeared.
> > > > > It's not the client's fault the GPU disappeared.
> > > > > 
> > > > > In Wayland, failure to use a wl_buffer is considered a protocol error,
> > > > > and protocol errors are always fatal: the client is disconnected. The
> > > > > rationale is that the compositor must always be able to present the
> > > > > client buffer somehow. If the compositor cannot, then the client did
> > > > > not obey the protocol.
> > > > > 
> > > > > The fallback presentation path in a compositor is usually importing the
> > > > > dmabuf to EGL, to be sampled from OpenGL. Normally the protocol
> > > > > guarantees that this works, so any failure to do so is a protocol
> > > > > violation. But if the GPU used by the client suddenly disappears and
> > > > > the imports start to fail, that is interpreted as a protocol violation
> > > > > unless the compositor can see why the import failed. Since the import
> > > > > is done via EGL, getting the right error code plumbed through from
> > > > > libdrm functions to the EGL caller would be a hassle. I don't see any
> > > > > error code in EGL_EXT_image_dma_buf_import reserved for "the dmabuf
> > > > > storage was hot-unplugged", and I doubt there is anything exclusively
> > > > > for only that in the EGL base spec either.
> > > > > 
> > > > > The cost of lying that the import worked is that the compositor will
> > > > > paint black or transparent where the window was supposed to be. It's a
> > > > > graphical glitch that is contrary to the Wayland design principles, but
> > > > > in this case a glitch is unavoidable: even if the compositor knew this
> > > > > buffer is now bad, what would it paint instead? It has nothing else to
> > > > > paint from. I'm assuming the compositor is using a different GPU than
> > > > > what disappeared.
> > > > > 
> > > > > Ideally, the client will eventually react to losing the GPU and either
> > > > > crash, quit, or switch its rendering to something that works which
> > > > > simply gives the compositor a new, working buffer without losing any
> > > > > window state in the process. If we risk the compositor disconnecting
> > > > > the client, then the client might not recover even if it wanted to.
> > > > >   
> > > > > > > +
> > > > > > > +- Render ioctls return EIO which is then handled in userspace drivers,
> > > > > > > +  e.g. Mesa, to have the device disappearance handled in the way
> > > > > > > +  specified for each API (OpenGL, GL ES: GL_KHR_robustness;
> > > > > > > +  Vulkan: VK_ERROR_DEVICE_LOST; etc.)  
> > > > > > 
> > > > > > As far as I understood from our discussion we would reject any IOCTL
> > > > > > aimed at a device which is gone and not only render ioctls.  
> > > > > Daniel?
> > > > > 
> > > > > 
> > > > > Thanks,
> > > > > pq
> > > > >   
> > > > > > Andrey
> > > > > > 
> > > > > >   
> > > > > > > +
> > > > > > > +Raising SIGBUS is not an option, because userspace cannot realistically
> > > > > > > +handle it.  Signal handlers are global, which makes them extremely
> > > > > > > +difficult to use correctly from libraries like Mesa produces. Signal
> > > > > > > +handlers are not composable, you can't have different handlers for GPU1
> > > > > > > +and GPU2 from different vendors, and a third handler for mmapped regular
> > > > > > > +files.  Threads cause additional pain with signal handling as well.
> > > > > > > +
> > > > > > > +Only after userspace has closed all relevant DRM device and dmabuf file
> > > > > > > +descriptors and removed all mmaps, the DRM driver can tear down its
> > > > > > > +instance for the device that no longer exists. If the same physical
> > > > > > > +device somehow comes back in the mean time, it shall be a new DRM
> > > > > > > +device.
> > > > > > > +
> > > > > > >    .. _drm_driver_ioctl:
> > > > > > >    IOCTL Support on Device Nodes  
> > > > 
> > > >   
> > 
> 



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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] drm/doc: device hot-unplug for userspace
  2020-05-19 10:06 [PATCH] drm/doc: device hot-unplug for userspace Pekka Paalanen
                   ` (2 preceding siblings ...)
  2020-05-25 12:46 ` [PATCH v2] " Pekka Paalanen
@ 2020-05-26 14:30 ` Andrey Grodzovsky
  2020-05-27  6:44   ` Pekka Paalanen
  3 siblings, 1 reply; 30+ messages in thread
From: Andrey Grodzovsky @ 2020-05-26 14:30 UTC (permalink / raw)
  To: Pekka Paalanen, dri-devel; +Cc: Sean Paul, Dave Airlie, Pekka Paalanen


On 5/19/20 6:06 AM, Pekka Paalanen wrote:
> From: Pekka Paalanen <pekka.paalanen@collabora.com>
>
> Set up the expectations on how hot-unplugging a DRM device should look like to
> userspace.
>
> Written by Daniel Vetter's request and largely based on his comments in IRC and
> from https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Fdri-devel%2F2020-May%2F265484.html&amp;data=02%7C01%7Candrey.grodzovsky%40amd.com%7Ce8e13dc4c85648e5fcd408d7fbdc5f2b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637254796242596783&amp;sdata=bA%2FAy3VGvzNqmV1kGigNROSZQEws2E98JibDxvEICNs%3D&amp;reserved=0 .
>
> Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> Cc: Daniel Vetter <daniel@ffwll.ch>
> Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Sean Paul <sean@poorly.run>
>
> ---
>
> Disclaimer: I am a userspace developer writing for other userspace developers.
> I took some liberties in defining what should happen without knowing what is
> actually possible or what existing drivers already implement.
> ---
>   Documentation/gpu/drm-uapi.rst | 75 ++++++++++++++++++++++++++++++++++
>   1 file changed, 75 insertions(+)
>
> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> index 56fec6ed1ad8..80db4abd2cbd 100644
> --- a/Documentation/gpu/drm-uapi.rst
> +++ b/Documentation/gpu/drm-uapi.rst
> @@ -1,3 +1,5 @@
> +.. Copyright 2020 DisplayLink (UK) Ltd.
> +
>   ===================
>   Userland interfaces
>   ===================
> @@ -162,6 +164,79 @@ other hand, a driver requires shared state between clients which is
>   visible to user-space and accessible beyond open-file boundaries, they
>   cannot support render nodes.
>   
> +Device Hot-Unplug
> +=================
> +
> +.. note::
> +   The following is the plan. Implementation is not there yet
> +   (2020 May 13).
> +
> +Graphics devices (display and/or render) may be connected via USB (e.g.
> +display adapters or docking stations) or Thunderbolt (e.g. eGPU). An end
> +user is able to hot-unplug this kind of devices while they are being
> +used, and expects that the very least the machine does not crash. Any
> +damage from hot-unplugging a DRM device needs to be limited as much as
> +possible and userspace must be given the chance to handle it if it wants
> +to. Ideally, unplugging a DRM device still lets a desktop to continue
> +running, but that is going to need explicit support throughout the whole
> +graphics stack: from kernel and userspace drivers, through display
> +servers, via window system protocols, and in applications and libraries.
> +
> +Other scenarios that should lead to the same are: unrecoverable GPU
> +crash, PCI device disappearing off the bus, or forced unbind of a driver
> +from the physical device.
> +
> +In other words, from userspace perspective everything needs to keep on
> +working more or less, until userspace stops using the disappeared DRM
> +device and closes it completely. Userspace will learn of the device
> +disappearance from the device removed uevent or in some cases specific
> +ioctls returning EIO.
> +
> +This goal raises at least the following requirements for the kernel and
> +drivers:
> +
> +- The kernel must not hang, crash or oops, no matter what userspace was
> +  in the middle of doing when the device disappeared.
> +
> +- All GPU jobs that can no longer run must have their fences
> +  force-signalled to avoid inflicting hangs to userspace.
> +
> +- KMS connectors must change their status to disconnected.
> +
> +- Legacy modesets and pageflips fake success.
> +
> +- Atomic commits, both real and TEST_ONLY, fake success.
> +
> +- Pending non-blocking KMS operations deliver the DRM events userspace
> +  is expecting.
> +
> +- If underlying memory disappears, the mmaps are replaced with harmless
> +  zero pages where access does not raise SIGBUS. Reads return zeros,
> +  writes are ignored.


Regarding this paragraph - what about exiting mappings ? In the first 
patchset we would actively invalidate all the existing CPU mappings to 
device memory and i think we still should do it otherwise we will see 
random crashes in applications as was before. I guess it's because TLBs 
and page tables are not updated to reflect the fact the device is gone.

Andrey


> +
> +- dmabuf which point to memory that has disappeared are rewritten to
> +  point to harmless zero pages, similar to mmaps. Imports still succeed
> +  both ways: an existing device importing a dmabuf pointing to
> +  disappeared memory, and a disappeared device importing any dmabuf.
> +
> +- Render ioctls return EIO which is then handled in userspace drivers,
> +  e.g. Mesa, to have the device disappearance handled in the way
> +  specified for each API (OpenGL, GL ES: GL_KHR_robustness;
> +  Vulkan: VK_ERROR_DEVICE_LOST; etc.)
> +
> +Raising SIGBUS is not an option, because userspace cannot realistically
> +handle it.  Signal handlers are global, which makes them extremely
> +difficult to use correctly from libraries like Mesa produces. Signal
> +handlers are not composable, you can't have different handlers for GPU1
> +and GPU2 from different vendors, and a third handler for mmapped regular
> +files.  Threads cause additional pain with signal handling as well.
> +
> +Only after userspace has closed all relevant DRM device and dmabuf file
> +descriptors and removed all mmaps, the DRM driver can tear down its
> +instance for the device that no longer exists. If the same physical
> +device somehow comes back in the mean time, it shall be a new DRM
> +device.
> +
>   .. _drm_driver_ioctl:
>   
>   IOCTL Support on Device Nodes
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] drm/doc: device hot-unplug for userspace
  2020-05-26 14:30 ` [PATCH] " Andrey Grodzovsky
@ 2020-05-27  6:44   ` Pekka Paalanen
  2020-05-27 13:51     ` Andrey Grodzovsky
  0 siblings, 1 reply; 30+ messages in thread
From: Pekka Paalanen @ 2020-05-27  6:44 UTC (permalink / raw)
  To: Andrey Grodzovsky; +Cc: Dave Airlie, Sean Paul, dri-devel

[-- Attachment #1.1: Type: text/plain, Size: 6403 bytes --]

On Tue, 26 May 2020 10:30:20 -0400
Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> wrote:

> On 5/19/20 6:06 AM, Pekka Paalanen wrote:
> > From: Pekka Paalanen <pekka.paalanen@collabora.com>
> >
> > Set up the expectations on how hot-unplugging a DRM device should look like to
> > userspace.
> >
> > Written by Daniel Vetter's request and largely based on his comments in IRC and
> > from https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Fdri-devel%2F2020-May%2F265484.html&amp;data=02%7C01%7Candrey.grodzovsky%40amd.com%7Ce8e13dc4c85648e5fcd408d7fbdc5f2b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637254796242596783&amp;sdata=bA%2FAy3VGvzNqmV1kGigNROSZQEws2E98JibDxvEICNs%3D&amp;reserved=0 .
> >
> > Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> > Cc: Daniel Vetter <daniel@ffwll.ch>
> > Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> > Cc: Dave Airlie <airlied@redhat.com>
> > Cc: Sean Paul <sean@poorly.run>
> >
> > ---
> >
> > Disclaimer: I am a userspace developer writing for other userspace developers.
> > I took some liberties in defining what should happen without knowing what is
> > actually possible or what existing drivers already implement.
> > ---
> >   Documentation/gpu/drm-uapi.rst | 75 ++++++++++++++++++++++++++++++++++
> >   1 file changed, 75 insertions(+)
> >
> > diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> > index 56fec6ed1ad8..80db4abd2cbd 100644
> > --- a/Documentation/gpu/drm-uapi.rst
> > +++ b/Documentation/gpu/drm-uapi.rst
> > @@ -1,3 +1,5 @@
> > +.. Copyright 2020 DisplayLink (UK) Ltd.
> > +
> >   ===================
> >   Userland interfaces
> >   ===================
> > @@ -162,6 +164,79 @@ other hand, a driver requires shared state between clients which is
> >   visible to user-space and accessible beyond open-file boundaries, they
> >   cannot support render nodes.
> >   
> > +Device Hot-Unplug
> > +=================
> > +
> > +.. note::
> > +   The following is the plan. Implementation is not there yet
> > +   (2020 May 13).
> > +
> > +Graphics devices (display and/or render) may be connected via USB (e.g.
> > +display adapters or docking stations) or Thunderbolt (e.g. eGPU). An end
> > +user is able to hot-unplug this kind of devices while they are being
> > +used, and expects that the very least the machine does not crash. Any
> > +damage from hot-unplugging a DRM device needs to be limited as much as
> > +possible and userspace must be given the chance to handle it if it wants
> > +to. Ideally, unplugging a DRM device still lets a desktop to continue
> > +running, but that is going to need explicit support throughout the whole
> > +graphics stack: from kernel and userspace drivers, through display
> > +servers, via window system protocols, and in applications and libraries.
> > +
> > +Other scenarios that should lead to the same are: unrecoverable GPU
> > +crash, PCI device disappearing off the bus, or forced unbind of a driver
> > +from the physical device.
> > +
> > +In other words, from userspace perspective everything needs to keep on
> > +working more or less, until userspace stops using the disappeared DRM
> > +device and closes it completely. Userspace will learn of the device
> > +disappearance from the device removed uevent or in some cases specific
> > +ioctls returning EIO.
> > +
> > +This goal raises at least the following requirements for the kernel and
> > +drivers:
> > +
> > +- The kernel must not hang, crash or oops, no matter what userspace was
> > +  in the middle of doing when the device disappeared.
> > +
> > +- All GPU jobs that can no longer run must have their fences
> > +  force-signalled to avoid inflicting hangs to userspace.
> > +
> > +- KMS connectors must change their status to disconnected.
> > +
> > +- Legacy modesets and pageflips fake success.
> > +
> > +- Atomic commits, both real and TEST_ONLY, fake success.
> > +
> > +- Pending non-blocking KMS operations deliver the DRM events userspace
> > +  is expecting.
> > +
> > +- If underlying memory disappears, the mmaps are replaced with harmless
> > +  zero pages where access does not raise SIGBUS. Reads return zeros,
> > +  writes are ignored.  
> 
> 
> Regarding this paragraph - what about exiting mappings ? In the first 
> patchset we would actively invalidate all the existing CPU mappings to 
> device memory and i think we still should do it otherwise we will see 
> random crashes in applications as was before. I guess it's because TLBs 
> and page tables are not updated to reflect the fact the device is gone.

Hi,

I was talking about existing mappings. What I forgot to specify is how
new mmap() calls after the device disappearance should work (the end
result should be the same still, not failure).

I'll clarify this in the next revision.


Thanks,
pq


> > +
> > +- dmabuf which point to memory that has disappeared are rewritten to
> > +  point to harmless zero pages, similar to mmaps. Imports still succeed
> > +  both ways: an existing device importing a dmabuf pointing to
> > +  disappeared memory, and a disappeared device importing any dmabuf.
> > +
> > +- Render ioctls return EIO which is then handled in userspace drivers,
> > +  e.g. Mesa, to have the device disappearance handled in the way
> > +  specified for each API (OpenGL, GL ES: GL_KHR_robustness;
> > +  Vulkan: VK_ERROR_DEVICE_LOST; etc.)
> > +
> > +Raising SIGBUS is not an option, because userspace cannot realistically
> > +handle it.  Signal handlers are global, which makes them extremely
> > +difficult to use correctly from libraries like Mesa produces. Signal
> > +handlers are not composable, you can't have different handlers for GPU1
> > +and GPU2 from different vendors, and a third handler for mmapped regular
> > +files.  Threads cause additional pain with signal handling as well.
> > +
> > +Only after userspace has closed all relevant DRM device and dmabuf file
> > +descriptors and removed all mmaps, the DRM driver can tear down its
> > +instance for the device that no longer exists. If the same physical
> > +device somehow comes back in the mean time, it shall be a new DRM
> > +device.
> > +
> >   .. _drm_driver_ioctl:
> >   
> >   IOCTL Support on Device Nodes  


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] drm/doc: device hot-unplug for userspace
  2020-05-27  6:44   ` Pekka Paalanen
@ 2020-05-27 13:51     ` Andrey Grodzovsky
  2020-05-27 14:39       ` Daniel Vetter
  0 siblings, 1 reply; 30+ messages in thread
From: Andrey Grodzovsky @ 2020-05-27 13:51 UTC (permalink / raw)
  To: Pekka Paalanen; +Cc: Dave Airlie, Sean Paul, dri-devel, Koenig, Christian


On 5/27/20 2:44 AM, Pekka Paalanen wrote:
> On Tue, 26 May 2020 10:30:20 -0400
> Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> wrote:
>
>> On 5/19/20 6:06 AM, Pekka Paalanen wrote:
>>> From: Pekka Paalanen <pekka.paalanen@collabora.com>
>>>
>>> Set up the expectations on how hot-unplugging a DRM device should look like to
>>> userspace.
>>>
>>> Written by Daniel Vetter's request and largely based on his comments in IRC and
>>> from https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Fdri-devel%2F2020-May%2F265484.html&amp;data=02%7C01%7Candrey.grodzovsky%40amd.com%7Ce8e13dc4c85648e5fcd408d7fbdc5f2b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637254796242596783&amp;sdata=bA%2FAy3VGvzNqmV1kGigNROSZQEws2E98JibDxvEICNs%3D&amp;reserved=0 .
>>>
>>> Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>> Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>> Cc: Dave Airlie <airlied@redhat.com>
>>> Cc: Sean Paul <sean@poorly.run>
>>>
>>> ---
>>>
>>> Disclaimer: I am a userspace developer writing for other userspace developers.
>>> I took some liberties in defining what should happen without knowing what is
>>> actually possible or what existing drivers already implement.
>>> ---
>>>    Documentation/gpu/drm-uapi.rst | 75 ++++++++++++++++++++++++++++++++++
>>>    1 file changed, 75 insertions(+)
>>>
>>> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
>>> index 56fec6ed1ad8..80db4abd2cbd 100644
>>> --- a/Documentation/gpu/drm-uapi.rst
>>> +++ b/Documentation/gpu/drm-uapi.rst
>>> @@ -1,3 +1,5 @@
>>> +.. Copyright 2020 DisplayLink (UK) Ltd.
>>> +
>>>    ===================
>>>    Userland interfaces
>>>    ===================
>>> @@ -162,6 +164,79 @@ other hand, a driver requires shared state between clients which is
>>>    visible to user-space and accessible beyond open-file boundaries, they
>>>    cannot support render nodes.
>>>    
>>> +Device Hot-Unplug
>>> +=================
>>> +
>>> +.. note::
>>> +   The following is the plan. Implementation is not there yet
>>> +   (2020 May 13).
>>> +
>>> +Graphics devices (display and/or render) may be connected via USB (e.g.
>>> +display adapters or docking stations) or Thunderbolt (e.g. eGPU). An end
>>> +user is able to hot-unplug this kind of devices while they are being
>>> +used, and expects that the very least the machine does not crash. Any
>>> +damage from hot-unplugging a DRM device needs to be limited as much as
>>> +possible and userspace must be given the chance to handle it if it wants
>>> +to. Ideally, unplugging a DRM device still lets a desktop to continue
>>> +running, but that is going to need explicit support throughout the whole
>>> +graphics stack: from kernel and userspace drivers, through display
>>> +servers, via window system protocols, and in applications and libraries.
>>> +
>>> +Other scenarios that should lead to the same are: unrecoverable GPU
>>> +crash, PCI device disappearing off the bus, or forced unbind of a driver
>>> +from the physical device.
>>> +
>>> +In other words, from userspace perspective everything needs to keep on
>>> +working more or less, until userspace stops using the disappeared DRM
>>> +device and closes it completely. Userspace will learn of the device
>>> +disappearance from the device removed uevent or in some cases specific
>>> +ioctls returning EIO.
>>> +
>>> +This goal raises at least the following requirements for the kernel and
>>> +drivers:
>>> +
>>> +- The kernel must not hang, crash or oops, no matter what userspace was
>>> +  in the middle of doing when the device disappeared.
>>> +
>>> +- All GPU jobs that can no longer run must have their fences
>>> +  force-signalled to avoid inflicting hangs to userspace.
>>> +
>>> +- KMS connectors must change their status to disconnected.
>>> +
>>> +- Legacy modesets and pageflips fake success.
>>> +
>>> +- Atomic commits, both real and TEST_ONLY, fake success.
>>> +
>>> +- Pending non-blocking KMS operations deliver the DRM events userspace
>>> +  is expecting.
>>> +
>>> +- If underlying memory disappears, the mmaps are replaced with harmless
>>> +  zero pages where access does not raise SIGBUS. Reads return zeros,
>>> +  writes are ignored.
>>
>> Regarding this paragraph - what about exiting mappings ? In the first
>> patchset we would actively invalidate all the existing CPU mappings to
>> device memory and i think we still should do it otherwise we will see
>> random crashes in applications as was before. I guess it's because TLBs
>> and page tables are not updated to reflect the fact the device is gone.
> Hi,
>
> I was talking about existing mappings. What I forgot to specify is how
> new mmap() calls after the device disappearance should work (the end
> result should be the same still, not failure).
>
> I'll clarify this in the next revision.
>
>
> Thanks,
> pq


I see, that ok.

Next related question is more for Daniel/Christian - about the 
implementation of this paragraph, I was thinking about something like 
checking for device disconnect in ttm_bo_vm_fault_reserved and if so 
remap the entire VA range for the VMA where the fault address belongs to 
the global zero page (i.e. (remap_pfn_range(vma, vma->vm_start, 
page_to_pfn(ZERO_PAGE(vma->vm_start), vma->vm_end - vma->vm_start, 
vma->vm_page_prot)). Question is, when the doc says 'writes are ignored' 
does it mean i should use copy on write for the vma->vm_page_prot and if 
so how i actually do it as i was not able to find what flags to set into 
vm_page_prot to force copy on write behavior.

Andrey




>
>
>>> +
>>> +- dmabuf which point to memory that has disappeared are rewritten to
>>> +  point to harmless zero pages, similar to mmaps. Imports still succeed
>>> +  both ways: an existing device importing a dmabuf pointing to
>>> +  disappeared memory, and a disappeared device importing any dmabuf.
>>> +
>>> +- Render ioctls return EIO which is then handled in userspace drivers,
>>> +  e.g. Mesa, to have the device disappearance handled in the way
>>> +  specified for each API (OpenGL, GL ES: GL_KHR_robustness;
>>> +  Vulkan: VK_ERROR_DEVICE_LOST; etc.)
>>> +
>>> +Raising SIGBUS is not an option, because userspace cannot realistically
>>> +handle it.  Signal handlers are global, which makes them extremely
>>> +difficult to use correctly from libraries like Mesa produces. Signal
>>> +handlers are not composable, you can't have different handlers for GPU1
>>> +and GPU2 from different vendors, and a third handler for mmapped regular
>>> +files.  Threads cause additional pain with signal handling as well.
>>> +
>>> +Only after userspace has closed all relevant DRM device and dmabuf file
>>> +descriptors and removed all mmaps, the DRM driver can tear down its
>>> +instance for the device that no longer exists. If the same physical
>>> +device somehow comes back in the mean time, it shall be a new DRM
>>> +device.
>>> +
>>>    .. _drm_driver_ioctl:
>>>    
>>>    IOCTL Support on Device Nodes
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] drm/doc: device hot-unplug for userspace
  2020-05-27 13:51     ` Andrey Grodzovsky
@ 2020-05-27 14:39       ` Daniel Vetter
  2020-05-27 15:23         ` Andrey Grodzovsky
  0 siblings, 1 reply; 30+ messages in thread
From: Daniel Vetter @ 2020-05-27 14:39 UTC (permalink / raw)
  To: Andrey Grodzovsky; +Cc: Dave Airlie, Sean Paul, Koenig, Christian, dri-devel

On Wed, May 27, 2020 at 3:51 PM Andrey Grodzovsky
<Andrey.Grodzovsky@amd.com> wrote:
>
>
> On 5/27/20 2:44 AM, Pekka Paalanen wrote:
> > On Tue, 26 May 2020 10:30:20 -0400
> > Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> wrote:
> >
> >> On 5/19/20 6:06 AM, Pekka Paalanen wrote:
> >>> From: Pekka Paalanen <pekka.paalanen@collabora.com>
> >>>
> >>> Set up the expectations on how hot-unplugging a DRM device should look like to
> >>> userspace.
> >>>
> >>> Written by Daniel Vetter's request and largely based on his comments in IRC and
> >>> from https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Fdri-devel%2F2020-May%2F265484.html&amp;data=02%7C01%7Candrey.grodzovsky%40amd.com%7Ce8e13dc4c85648e5fcd408d7fbdc5f2b%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637254796242596783&amp;sdata=bA%2FAy3VGvzNqmV1kGigNROSZQEws2E98JibDxvEICNs%3D&amp;reserved=0 .
> >>>
> >>> Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> >>> Cc: Daniel Vetter <daniel@ffwll.ch>
> >>> Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> >>> Cc: Dave Airlie <airlied@redhat.com>
> >>> Cc: Sean Paul <sean@poorly.run>
> >>>
> >>> ---
> >>>
> >>> Disclaimer: I am a userspace developer writing for other userspace developers.
> >>> I took some liberties in defining what should happen without knowing what is
> >>> actually possible or what existing drivers already implement.
> >>> ---
> >>>    Documentation/gpu/drm-uapi.rst | 75 ++++++++++++++++++++++++++++++++++
> >>>    1 file changed, 75 insertions(+)
> >>>
> >>> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
> >>> index 56fec6ed1ad8..80db4abd2cbd 100644
> >>> --- a/Documentation/gpu/drm-uapi.rst
> >>> +++ b/Documentation/gpu/drm-uapi.rst
> >>> @@ -1,3 +1,5 @@
> >>> +.. Copyright 2020 DisplayLink (UK) Ltd.
> >>> +
> >>>    ===================
> >>>    Userland interfaces
> >>>    ===================
> >>> @@ -162,6 +164,79 @@ other hand, a driver requires shared state between clients which is
> >>>    visible to user-space and accessible beyond open-file boundaries, they
> >>>    cannot support render nodes.
> >>>
> >>> +Device Hot-Unplug
> >>> +=================
> >>> +
> >>> +.. note::
> >>> +   The following is the plan. Implementation is not there yet
> >>> +   (2020 May 13).
> >>> +
> >>> +Graphics devices (display and/or render) may be connected via USB (e.g.
> >>> +display adapters or docking stations) or Thunderbolt (e.g. eGPU). An end
> >>> +user is able to hot-unplug this kind of devices while they are being
> >>> +used, and expects that the very least the machine does not crash. Any
> >>> +damage from hot-unplugging a DRM device needs to be limited as much as
> >>> +possible and userspace must be given the chance to handle it if it wants
> >>> +to. Ideally, unplugging a DRM device still lets a desktop to continue
> >>> +running, but that is going to need explicit support throughout the whole
> >>> +graphics stack: from kernel and userspace drivers, through display
> >>> +servers, via window system protocols, and in applications and libraries.
> >>> +
> >>> +Other scenarios that should lead to the same are: unrecoverable GPU
> >>> +crash, PCI device disappearing off the bus, or forced unbind of a driver
> >>> +from the physical device.
> >>> +
> >>> +In other words, from userspace perspective everything needs to keep on
> >>> +working more or less, until userspace stops using the disappeared DRM
> >>> +device and closes it completely. Userspace will learn of the device
> >>> +disappearance from the device removed uevent or in some cases specific
> >>> +ioctls returning EIO.
> >>> +
> >>> +This goal raises at least the following requirements for the kernel and
> >>> +drivers:
> >>> +
> >>> +- The kernel must not hang, crash or oops, no matter what userspace was
> >>> +  in the middle of doing when the device disappeared.
> >>> +
> >>> +- All GPU jobs that can no longer run must have their fences
> >>> +  force-signalled to avoid inflicting hangs to userspace.
> >>> +
> >>> +- KMS connectors must change their status to disconnected.
> >>> +
> >>> +- Legacy modesets and pageflips fake success.
> >>> +
> >>> +- Atomic commits, both real and TEST_ONLY, fake success.
> >>> +
> >>> +- Pending non-blocking KMS operations deliver the DRM events userspace
> >>> +  is expecting.
> >>> +
> >>> +- If underlying memory disappears, the mmaps are replaced with harmless
> >>> +  zero pages where access does not raise SIGBUS. Reads return zeros,
> >>> +  writes are ignored.
> >>
> >> Regarding this paragraph - what about exiting mappings ? In the first
> >> patchset we would actively invalidate all the existing CPU mappings to
> >> device memory and i think we still should do it otherwise we will see
> >> random crashes in applications as was before. I guess it's because TLBs
> >> and page tables are not updated to reflect the fact the device is gone.
> > Hi,
> >
> > I was talking about existing mappings. What I forgot to specify is how
> > new mmap() calls after the device disappearance should work (the end
> > result should be the same still, not failure).
> >
> > I'll clarify this in the next revision.
> >
> >
> > Thanks,
> > pq
>
>
> I see, that ok.
>
> Next related question is more for Daniel/Christian - about the
> implementation of this paragraph, I was thinking about something like
> checking for device disconnect in ttm_bo_vm_fault_reserved and if so
> remap the entire VA range for the VMA where the fault address belongs to
> the global zero page (i.e. (remap_pfn_range(vma, vma->vm_start,
> page_to_pfn(ZERO_PAGE(vma->vm_start), vma->vm_end - vma->vm_start,
> vma->vm_page_prot)). Question is, when the doc says 'writes are ignored'
> does it mean i should use copy on write for the vma->vm_page_prot and if
> so how i actually do it as i was not able to find what flags to set into
> vm_page_prot to force copy on write behavior.

Already discussed this with Pekka on irc, I think simply a private
page (per gpu ctx to avoid leaks) is good enough. Otherwise we need to
catch write faults and throw the writes away, and that's a) a bit
tricky to implement and b) slow, which we kinda don't want to. If the
desktop is stuck for a few seconds because we're trapping every write
of a 4k buffer that's getting uploaded, the user is going to have a
bad time :-/
-Daniel

>
> Andrey
>
>
>
>
> >
> >
> >>> +
> >>> +- dmabuf which point to memory that has disappeared are rewritten to
> >>> +  point to harmless zero pages, similar to mmaps. Imports still succeed
> >>> +  both ways: an existing device importing a dmabuf pointing to
> >>> +  disappeared memory, and a disappeared device importing any dmabuf.
> >>> +
> >>> +- Render ioctls return EIO which is then handled in userspace drivers,
> >>> +  e.g. Mesa, to have the device disappearance handled in the way
> >>> +  specified for each API (OpenGL, GL ES: GL_KHR_robustness;
> >>> +  Vulkan: VK_ERROR_DEVICE_LOST; etc.)
> >>> +
> >>> +Raising SIGBUS is not an option, because userspace cannot realistically
> >>> +handle it.  Signal handlers are global, which makes them extremely
> >>> +difficult to use correctly from libraries like Mesa produces. Signal
> >>> +handlers are not composable, you can't have different handlers for GPU1
> >>> +and GPU2 from different vendors, and a third handler for mmapped regular
> >>> +files.  Threads cause additional pain with signal handling as well.
> >>> +
> >>> +Only after userspace has closed all relevant DRM device and dmabuf file
> >>> +descriptors and removed all mmaps, the DRM driver can tear down its
> >>> +instance for the device that no longer exists. If the same physical
> >>> +device somehow comes back in the mean time, it shall be a new DRM
> >>> +device.
> >>> +
> >>>    .. _drm_driver_ioctl:
> >>>
> >>>    IOCTL Support on Device Nodes



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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] drm/doc: device hot-unplug for userspace
  2020-05-27 14:39       ` Daniel Vetter
@ 2020-05-27 15:23         ` Andrey Grodzovsky
  2020-05-27 19:44           ` Christian König
  0 siblings, 1 reply; 30+ messages in thread
From: Andrey Grodzovsky @ 2020-05-27 15:23 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Dave Airlie, Sean Paul, Koenig, Christian, dri-devel


On 5/27/20 10:39 AM, Daniel Vetter wrote:
> On Wed, May 27, 2020 at 3:51 PM Andrey Grodzovsky
> <Andrey.Grodzovsky@amd.com> wrote:
>>
>> On 5/27/20 2:44 AM, Pekka Paalanen wrote:
>>> On Tue, 26 May 2020 10:30:20 -0400
>>> Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> wrote:
>>>
>>>> On 5/19/20 6:06 AM, Pekka Paalanen wrote:
>>>>> From: Pekka Paalanen <pekka.paalanen@collabora.com>
>>>>>
>>>>> Set up the expectations on how hot-unplugging a DRM device should look like to
>>>>> userspace.
>>>>>
>>>>> Written by Daniel Vetter's request and largely based on his comments in IRC and
>>>>> from https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Fdri-devel%2F2020-May%2F265484.html&amp;data=02%7C01%7CAndrey.Grodzovsky%40amd.com%7C3c671803b2ba41b2ceac08d8024bcc9a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637261871869742519&amp;sdata=ZnhylRubOM0%2BjoreSSYMqVDzZuUdybEsoyBVcTKgxWE%3D&amp;reserved=0 .
>>>>>
>>>>> Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
>>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>>>> Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>>> Cc: Dave Airlie <airlied@redhat.com>
>>>>> Cc: Sean Paul <sean@poorly.run>
>>>>>
>>>>> ---
>>>>>
>>>>> Disclaimer: I am a userspace developer writing for other userspace developers.
>>>>> I took some liberties in defining what should happen without knowing what is
>>>>> actually possible or what existing drivers already implement.
>>>>> ---
>>>>>     Documentation/gpu/drm-uapi.rst | 75 ++++++++++++++++++++++++++++++++++
>>>>>     1 file changed, 75 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/gpu/drm-uapi.rst b/Documentation/gpu/drm-uapi.rst
>>>>> index 56fec6ed1ad8..80db4abd2cbd 100644
>>>>> --- a/Documentation/gpu/drm-uapi.rst
>>>>> +++ b/Documentation/gpu/drm-uapi.rst
>>>>> @@ -1,3 +1,5 @@
>>>>> +.. Copyright 2020 DisplayLink (UK) Ltd.
>>>>> +
>>>>>     ===================
>>>>>     Userland interfaces
>>>>>     ===================
>>>>> @@ -162,6 +164,79 @@ other hand, a driver requires shared state between clients which is
>>>>>     visible to user-space and accessible beyond open-file boundaries, they
>>>>>     cannot support render nodes.
>>>>>
>>>>> +Device Hot-Unplug
>>>>> +=================
>>>>> +
>>>>> +.. note::
>>>>> +   The following is the plan. Implementation is not there yet
>>>>> +   (2020 May 13).
>>>>> +
>>>>> +Graphics devices (display and/or render) may be connected via USB (e.g.
>>>>> +display adapters or docking stations) or Thunderbolt (e.g. eGPU). An end
>>>>> +user is able to hot-unplug this kind of devices while they are being
>>>>> +used, and expects that the very least the machine does not crash. Any
>>>>> +damage from hot-unplugging a DRM device needs to be limited as much as
>>>>> +possible and userspace must be given the chance to handle it if it wants
>>>>> +to. Ideally, unplugging a DRM device still lets a desktop to continue
>>>>> +running, but that is going to need explicit support throughout the whole
>>>>> +graphics stack: from kernel and userspace drivers, through display
>>>>> +servers, via window system protocols, and in applications and libraries.
>>>>> +
>>>>> +Other scenarios that should lead to the same are: unrecoverable GPU
>>>>> +crash, PCI device disappearing off the bus, or forced unbind of a driver
>>>>> +from the physical device.
>>>>> +
>>>>> +In other words, from userspace perspective everything needs to keep on
>>>>> +working more or less, until userspace stops using the disappeared DRM
>>>>> +device and closes it completely. Userspace will learn of the device
>>>>> +disappearance from the device removed uevent or in some cases specific
>>>>> +ioctls returning EIO.
>>>>> +
>>>>> +This goal raises at least the following requirements for the kernel and
>>>>> +drivers:
>>>>> +
>>>>> +- The kernel must not hang, crash or oops, no matter what userspace was
>>>>> +  in the middle of doing when the device disappeared.
>>>>> +
>>>>> +- All GPU jobs that can no longer run must have their fences
>>>>> +  force-signalled to avoid inflicting hangs to userspace.
>>>>> +
>>>>> +- KMS connectors must change their status to disconnected.
>>>>> +
>>>>> +- Legacy modesets and pageflips fake success.
>>>>> +
>>>>> +- Atomic commits, both real and TEST_ONLY, fake success.
>>>>> +
>>>>> +- Pending non-blocking KMS operations deliver the DRM events userspace
>>>>> +  is expecting.
>>>>> +
>>>>> +- If underlying memory disappears, the mmaps are replaced with harmless
>>>>> +  zero pages where access does not raise SIGBUS. Reads return zeros,
>>>>> +  writes are ignored.
>>>> Regarding this paragraph - what about exiting mappings ? In the first
>>>> patchset we would actively invalidate all the existing CPU mappings to
>>>> device memory and i think we still should do it otherwise we will see
>>>> random crashes in applications as was before. I guess it's because TLBs
>>>> and page tables are not updated to reflect the fact the device is gone.
>>> Hi,
>>>
>>> I was talking about existing mappings. What I forgot to specify is how
>>> new mmap() calls after the device disappearance should work (the end
>>> result should be the same still, not failure).
>>>
>>> I'll clarify this in the next revision.
>>>
>>>
>>> Thanks,
>>> pq
>>
>> I see, that ok.
>>
>> Next related question is more for Daniel/Christian - about the
>> implementation of this paragraph, I was thinking about something like
>> checking for device disconnect in ttm_bo_vm_fault_reserved and if so
>> remap the entire VA range for the VMA where the fault address belongs to
>> the global zero page (i.e. (remap_pfn_range(vma, vma->vm_start,
>> page_to_pfn(ZERO_PAGE(vma->vm_start), vma->vm_end - vma->vm_start,
>> vma->vm_page_prot)). Question is, when the doc says 'writes are ignored'
>> does it mean i should use copy on write for the vma->vm_page_prot and if
>> so how i actually do it as i was not able to find what flags to set into
>> vm_page_prot to force copy on write behavior.
> Already discussed this with Pekka on irc, I think simply a private
> page (per gpu ctx to avoid leaks) is good enough. Otherwise we need to
> catch write faults and throw the writes away, and that's a) a bit
> tricky to implement and b) slow, which we kinda don't want to. If the
> desktop is stuck for a few seconds because we're trapping every write
> of a 4k buffer that's getting uploaded, the user is going to have a
> bad time :-/
> -Daniel


So like allocating a page per process context in the driver (struct 
amdgpu_ctx in amdgpu) and mapping this page into the faulting VMAs  for 
when device is disconnected ? I am still not clear how i make the 
mapping ignore writes without catching write faults and ignoring them. I 
cannot just make it read only obviously and i can't make it writable as 
then reading back will start returning non 0's. My question is what set 
of flags in vm_area_struct.vm_flags can (if at all) give me 'ignore 
writes' behavior for the mapping of that page.

Andrey


>
>> Andrey
>>
>>
>>
>>
>>>
>>>>> +
>>>>> +- dmabuf which point to memory that has disappeared are rewritten to
>>>>> +  point to harmless zero pages, similar to mmaps. Imports still succeed
>>>>> +  both ways: an existing device importing a dmabuf pointing to
>>>>> +  disappeared memory, and a disappeared device importing any dmabuf.
>>>>> +
>>>>> +- Render ioctls return EIO which is then handled in userspace drivers,
>>>>> +  e.g. Mesa, to have the device disappearance handled in the way
>>>>> +  specified for each API (OpenGL, GL ES: GL_KHR_robustness;
>>>>> +  Vulkan: VK_ERROR_DEVICE_LOST; etc.)
>>>>> +
>>>>> +Raising SIGBUS is not an option, because userspace cannot realistically
>>>>> +handle it.  Signal handlers are global, which makes them extremely
>>>>> +difficult to use correctly from libraries like Mesa produces. Signal
>>>>> +handlers are not composable, you can't have different handlers for GPU1
>>>>> +and GPU2 from different vendors, and a third handler for mmapped regular
>>>>> +files.  Threads cause additional pain with signal handling as well.
>>>>> +
>>>>> +Only after userspace has closed all relevant DRM device and dmabuf file
>>>>> +descriptors and removed all mmaps, the DRM driver can tear down its
>>>>> +instance for the device that no longer exists. If the same physical
>>>>> +device somehow comes back in the mean time, it shall be a new DRM
>>>>> +device.
>>>>> +
>>>>>     .. _drm_driver_ioctl:
>>>>>
>>>>>     IOCTL Support on Device Nodes
>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] drm/doc: device hot-unplug for userspace
  2020-05-27 15:23         ` Andrey Grodzovsky
@ 2020-05-27 19:44           ` Christian König
  2020-05-27 20:25             ` Daniel Vetter
  0 siblings, 1 reply; 30+ messages in thread
From: Christian König @ 2020-05-27 19:44 UTC (permalink / raw)
  To: Andrey Grodzovsky, Daniel Vetter; +Cc: Dave Airlie, Sean Paul, dri-devel

Am 27.05.20 um 17:23 schrieb Andrey Grodzovsky:
>
> On 5/27/20 10:39 AM, Daniel Vetter wrote:
>> On Wed, May 27, 2020 at 3:51 PM Andrey Grodzovsky
>> <Andrey.Grodzovsky@amd.com> wrote:
>>>
>>> On 5/27/20 2:44 AM, Pekka Paalanen wrote:
>>>> On Tue, 26 May 2020 10:30:20 -0400
>>>> Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> wrote:
>>>>
>>>>> On 5/19/20 6:06 AM, Pekka Paalanen wrote:
>>>>>> From: Pekka Paalanen <pekka.paalanen@collabora.com>
>>>>>>
>>>>>> Set up the expectations on how hot-unplugging a DRM device should 
>>>>>> look like to
>>>>>> userspace.
>>>>>>
>>>>>> Written by Daniel Vetter's request and largely based on his 
>>>>>> comments in IRC and
>>>>>> from 
>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Fdri-devel%2F2020-May%2F265484.html&amp;data=02%7C01%7CAndrey.Grodzovsky%40amd.com%7C3c671803b2ba41b2ceac08d8024bcc9a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637261871869742519&amp;sdata=ZnhylRubOM0%2BjoreSSYMqVDzZuUdybEsoyBVcTKgxWE%3D&amp;reserved=0 
>>>>>> .
>>>>>>
>>>>>> Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
>>>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>>>>> Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>>>> Cc: Dave Airlie <airlied@redhat.com>
>>>>>> Cc: Sean Paul <sean@poorly.run>
>>>>>>
>>>>>> ---
>>>>>>
>>>>>> Disclaimer: I am a userspace developer writing for other 
>>>>>> userspace developers.
>>>>>> I took some liberties in defining what should happen without 
>>>>>> knowing what is
>>>>>> actually possible or what existing drivers already implement.
>>>>>> ---
>>>>>>     Documentation/gpu/drm-uapi.rst | 75 
>>>>>> ++++++++++++++++++++++++++++++++++
>>>>>>     1 file changed, 75 insertions(+)
>>>>>>
>>>>>> diff --git a/Documentation/gpu/drm-uapi.rst 
>>>>>> b/Documentation/gpu/drm-uapi.rst
>>>>>> index 56fec6ed1ad8..80db4abd2cbd 100644
>>>>>> --- a/Documentation/gpu/drm-uapi.rst
>>>>>> +++ b/Documentation/gpu/drm-uapi.rst
>>>>>> @@ -1,3 +1,5 @@
>>>>>> +.. Copyright 2020 DisplayLink (UK) Ltd.
>>>>>> +
>>>>>>     ===================
>>>>>>     Userland interfaces
>>>>>>     ===================
>>>>>> @@ -162,6 +164,79 @@ other hand, a driver requires shared state 
>>>>>> between clients which is
>>>>>>     visible to user-space and accessible beyond open-file 
>>>>>> boundaries, they
>>>>>>     cannot support render nodes.
>>>>>>
>>>>>> +Device Hot-Unplug
>>>>>> +=================
>>>>>> +
>>>>>> +.. note::
>>>>>> +   The following is the plan. Implementation is not there yet
>>>>>> +   (2020 May 13).
>>>>>> +
>>>>>> +Graphics devices (display and/or render) may be connected via 
>>>>>> USB (e.g.
>>>>>> +display adapters or docking stations) or Thunderbolt (e.g. 
>>>>>> eGPU). An end
>>>>>> +user is able to hot-unplug this kind of devices while they are 
>>>>>> being
>>>>>> +used, and expects that the very least the machine does not 
>>>>>> crash. Any
>>>>>> +damage from hot-unplugging a DRM device needs to be limited as 
>>>>>> much as
>>>>>> +possible and userspace must be given the chance to handle it if 
>>>>>> it wants
>>>>>> +to. Ideally, unplugging a DRM device still lets a desktop to 
>>>>>> continue
>>>>>> +running, but that is going to need explicit support throughout 
>>>>>> the whole
>>>>>> +graphics stack: from kernel and userspace drivers, through display
>>>>>> +servers, via window system protocols, and in applications and 
>>>>>> libraries.
>>>>>> +
>>>>>> +Other scenarios that should lead to the same are: unrecoverable GPU
>>>>>> +crash, PCI device disappearing off the bus, or forced unbind of 
>>>>>> a driver
>>>>>> +from the physical device.
>>>>>> +
>>>>>> +In other words, from userspace perspective everything needs to 
>>>>>> keep on
>>>>>> +working more or less, until userspace stops using the 
>>>>>> disappeared DRM
>>>>>> +device and closes it completely. Userspace will learn of the device
>>>>>> +disappearance from the device removed uevent or in some cases 
>>>>>> specific
>>>>>> +ioctls returning EIO.
>>>>>> +
>>>>>> +This goal raises at least the following requirements for the 
>>>>>> kernel and
>>>>>> +drivers:
>>>>>> +
>>>>>> +- The kernel must not hang, crash or oops, no matter what 
>>>>>> userspace was
>>>>>> +  in the middle of doing when the device disappeared.
>>>>>> +
>>>>>> +- All GPU jobs that can no longer run must have their fences
>>>>>> +  force-signalled to avoid inflicting hangs to userspace.
>>>>>> +
>>>>>> +- KMS connectors must change their status to disconnected.
>>>>>> +
>>>>>> +- Legacy modesets and pageflips fake success.
>>>>>> +
>>>>>> +- Atomic commits, both real and TEST_ONLY, fake success.
>>>>>> +
>>>>>> +- Pending non-blocking KMS operations deliver the DRM events 
>>>>>> userspace
>>>>>> +  is expecting.
>>>>>> +
>>>>>> +- If underlying memory disappears, the mmaps are replaced with 
>>>>>> harmless
>>>>>> +  zero pages where access does not raise SIGBUS. Reads return 
>>>>>> zeros,
>>>>>> +  writes are ignored.
>>>>> Regarding this paragraph - what about exiting mappings ? In the first
>>>>> patchset we would actively invalidate all the existing CPU 
>>>>> mappings to
>>>>> device memory and i think we still should do it otherwise we will see
>>>>> random crashes in applications as was before. I guess it's because 
>>>>> TLBs
>>>>> and page tables are not updated to reflect the fact the device is 
>>>>> gone.
>>>> Hi,
>>>>
>>>> I was talking about existing mappings. What I forgot to specify is how
>>>> new mmap() calls after the device disappearance should work (the end
>>>> result should be the same still, not failure).
>>>>
>>>> I'll clarify this in the next revision.
>>>>
>>>>
>>>> Thanks,
>>>> pq
>>>
>>> I see, that ok.
>>>
>>> Next related question is more for Daniel/Christian - about the
>>> implementation of this paragraph, I was thinking about something like
>>> checking for device disconnect in ttm_bo_vm_fault_reserved and if so
>>> remap the entire VA range for the VMA where the fault address 
>>> belongs to
>>> the global zero page (i.e. (remap_pfn_range(vma, vma->vm_start,
>>> page_to_pfn(ZERO_PAGE(vma->vm_start), vma->vm_end - vma->vm_start,
>>> vma->vm_page_prot)). Question is, when the doc says 'writes are 
>>> ignored'
>>> does it mean i should use copy on write for the vma->vm_page_prot 
>>> and if
>>> so how i actually do it as i was not able to find what flags to set 
>>> into
>>> vm_page_prot to force copy on write behavior.
>> Already discussed this with Pekka on irc, I think simply a private
>> page (per gpu ctx to avoid leaks) is good enough. Otherwise we need to
>> catch write faults and throw the writes away, and that's a) a bit
>> tricky to implement and b) slow, which we kinda don't want to. If the
>> desktop is stuck for a few seconds because we're trapping every write
>> of a 4k buffer that's getting uploaded, the user is going to have a
>> bad time :-/
>> -Daniel
>
>
> So like allocating a page per process context in the driver (struct 
> amdgpu_ctx in amdgpu) and mapping this page into the faulting VMAs  
> for when device is disconnected ? I am still not clear how i make the 
> mapping ignore writes without catching write faults and ignoring them. 
> I cannot just make it read only obviously and i can't make it writable 
> as then reading back will start returning non 0's. My question is what 
> set of flags in vm_area_struct.vm_flags can (if at all) give me 
> 'ignore writes' behavior for the mapping of that page.

I'm not aware of a possibility like that on x86 CPUs. As far as I know 
we only have something like an ignore write functionality on our GPUs 
for PRTs.

Could we use an address which points to a non allocated MMIO space or 
something like this? We would might get 0xffffffff on reads instead of 
0x0, but writes would be certainly ignored.

Christian.

>
> Andrey
>
>
>>
>>> Andrey
>>>
>>>
>>>
>>>
>>>>
>>>>>> +
>>>>>> +- dmabuf which point to memory that has disappeared are 
>>>>>> rewritten to
>>>>>> +  point to harmless zero pages, similar to mmaps. Imports still 
>>>>>> succeed
>>>>>> +  both ways: an existing device importing a dmabuf pointing to
>>>>>> +  disappeared memory, and a disappeared device importing any 
>>>>>> dmabuf.
>>>>>> +
>>>>>> +- Render ioctls return EIO which is then handled in userspace 
>>>>>> drivers,
>>>>>> +  e.g. Mesa, to have the device disappearance handled in the way
>>>>>> +  specified for each API (OpenGL, GL ES: GL_KHR_robustness;
>>>>>> +  Vulkan: VK_ERROR_DEVICE_LOST; etc.)
>>>>>> +
>>>>>> +Raising SIGBUS is not an option, because userspace cannot 
>>>>>> realistically
>>>>>> +handle it.  Signal handlers are global, which makes them extremely
>>>>>> +difficult to use correctly from libraries like Mesa produces. 
>>>>>> Signal
>>>>>> +handlers are not composable, you can't have different handlers 
>>>>>> for GPU1
>>>>>> +and GPU2 from different vendors, and a third handler for mmapped 
>>>>>> regular
>>>>>> +files.  Threads cause additional pain with signal handling as well.
>>>>>> +
>>>>>> +Only after userspace has closed all relevant DRM device and 
>>>>>> dmabuf file
>>>>>> +descriptors and removed all mmaps, the DRM driver can tear down its
>>>>>> +instance for the device that no longer exists. If the same physical
>>>>>> +device somehow comes back in the mean time, it shall be a new DRM
>>>>>> +device.
>>>>>> +
>>>>>>     .. _drm_driver_ioctl:
>>>>>>
>>>>>>     IOCTL Support on Device Nodes
>>
>>

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] drm/doc: device hot-unplug for userspace
  2020-05-27 19:44           ` Christian König
@ 2020-05-27 20:25             ` Daniel Vetter
  2020-05-28 12:13               ` Pekka Paalanen
  2020-05-28 21:28               ` Andrey Grodzovsky
  0 siblings, 2 replies; 30+ messages in thread
From: Daniel Vetter @ 2020-05-27 20:25 UTC (permalink / raw)
  To: Christian König; +Cc: Sean Paul, dri-devel, Dave Airlie

On Wed, May 27, 2020 at 9:44 PM Christian König
<christian.koenig@amd.com> wrote:
>
> Am 27.05.20 um 17:23 schrieb Andrey Grodzovsky:
> >
> > On 5/27/20 10:39 AM, Daniel Vetter wrote:
> >> On Wed, May 27, 2020 at 3:51 PM Andrey Grodzovsky
> >> <Andrey.Grodzovsky@amd.com> wrote:
> >>>
> >>> On 5/27/20 2:44 AM, Pekka Paalanen wrote:
> >>>> On Tue, 26 May 2020 10:30:20 -0400
> >>>> Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> wrote:
> >>>>
> >>>>> On 5/19/20 6:06 AM, Pekka Paalanen wrote:
> >>>>>> From: Pekka Paalanen <pekka.paalanen@collabora.com>
> >>>>>>
> >>>>>> Set up the expectations on how hot-unplugging a DRM device should
> >>>>>> look like to
> >>>>>> userspace.
> >>>>>>
> >>>>>> Written by Daniel Vetter's request and largely based on his
> >>>>>> comments in IRC and
> >>>>>> from
> >>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Fdri-devel%2F2020-May%2F265484.html&amp;data=02%7C01%7CAndrey.Grodzovsky%40amd.com%7C3c671803b2ba41b2ceac08d8024bcc9a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637261871869742519&amp;sdata=ZnhylRubOM0%2BjoreSSYMqVDzZuUdybEsoyBVcTKgxWE%3D&amp;reserved=0
> >>>>>> .
> >>>>>>
> >>>>>> Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> >>>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
> >>>>>> Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> >>>>>> Cc: Dave Airlie <airlied@redhat.com>
> >>>>>> Cc: Sean Paul <sean@poorly.run>
> >>>>>>
> >>>>>> ---
> >>>>>>
> >>>>>> Disclaimer: I am a userspace developer writing for other
> >>>>>> userspace developers.
> >>>>>> I took some liberties in defining what should happen without
> >>>>>> knowing what is
> >>>>>> actually possible or what existing drivers already implement.
> >>>>>> ---
> >>>>>>     Documentation/gpu/drm-uapi.rst | 75
> >>>>>> ++++++++++++++++++++++++++++++++++
> >>>>>>     1 file changed, 75 insertions(+)
> >>>>>>
> >>>>>> diff --git a/Documentation/gpu/drm-uapi.rst
> >>>>>> b/Documentation/gpu/drm-uapi.rst
> >>>>>> index 56fec6ed1ad8..80db4abd2cbd 100644
> >>>>>> --- a/Documentation/gpu/drm-uapi.rst
> >>>>>> +++ b/Documentation/gpu/drm-uapi.rst
> >>>>>> @@ -1,3 +1,5 @@
> >>>>>> +.. Copyright 2020 DisplayLink (UK) Ltd.
> >>>>>> +
> >>>>>>     ===================
> >>>>>>     Userland interfaces
> >>>>>>     ===================
> >>>>>> @@ -162,6 +164,79 @@ other hand, a driver requires shared state
> >>>>>> between clients which is
> >>>>>>     visible to user-space and accessible beyond open-file
> >>>>>> boundaries, they
> >>>>>>     cannot support render nodes.
> >>>>>>
> >>>>>> +Device Hot-Unplug
> >>>>>> +=================
> >>>>>> +
> >>>>>> +.. note::
> >>>>>> +   The following is the plan. Implementation is not there yet
> >>>>>> +   (2020 May 13).
> >>>>>> +
> >>>>>> +Graphics devices (display and/or render) may be connected via
> >>>>>> USB (e.g.
> >>>>>> +display adapters or docking stations) or Thunderbolt (e.g.
> >>>>>> eGPU). An end
> >>>>>> +user is able to hot-unplug this kind of devices while they are
> >>>>>> being
> >>>>>> +used, and expects that the very least the machine does not
> >>>>>> crash. Any
> >>>>>> +damage from hot-unplugging a DRM device needs to be limited as
> >>>>>> much as
> >>>>>> +possible and userspace must be given the chance to handle it if
> >>>>>> it wants
> >>>>>> +to. Ideally, unplugging a DRM device still lets a desktop to
> >>>>>> continue
> >>>>>> +running, but that is going to need explicit support throughout
> >>>>>> the whole
> >>>>>> +graphics stack: from kernel and userspace drivers, through display
> >>>>>> +servers, via window system protocols, and in applications and
> >>>>>> libraries.
> >>>>>> +
> >>>>>> +Other scenarios that should lead to the same are: unrecoverable GPU
> >>>>>> +crash, PCI device disappearing off the bus, or forced unbind of
> >>>>>> a driver
> >>>>>> +from the physical device.
> >>>>>> +
> >>>>>> +In other words, from userspace perspective everything needs to
> >>>>>> keep on
> >>>>>> +working more or less, until userspace stops using the
> >>>>>> disappeared DRM
> >>>>>> +device and closes it completely. Userspace will learn of the device
> >>>>>> +disappearance from the device removed uevent or in some cases
> >>>>>> specific
> >>>>>> +ioctls returning EIO.
> >>>>>> +
> >>>>>> +This goal raises at least the following requirements for the
> >>>>>> kernel and
> >>>>>> +drivers:
> >>>>>> +
> >>>>>> +- The kernel must not hang, crash or oops, no matter what
> >>>>>> userspace was
> >>>>>> +  in the middle of doing when the device disappeared.
> >>>>>> +
> >>>>>> +- All GPU jobs that can no longer run must have their fences
> >>>>>> +  force-signalled to avoid inflicting hangs to userspace.
> >>>>>> +
> >>>>>> +- KMS connectors must change their status to disconnected.
> >>>>>> +
> >>>>>> +- Legacy modesets and pageflips fake success.
> >>>>>> +
> >>>>>> +- Atomic commits, both real and TEST_ONLY, fake success.
> >>>>>> +
> >>>>>> +- Pending non-blocking KMS operations deliver the DRM events
> >>>>>> userspace
> >>>>>> +  is expecting.
> >>>>>> +
> >>>>>> +- If underlying memory disappears, the mmaps are replaced with
> >>>>>> harmless
> >>>>>> +  zero pages where access does not raise SIGBUS. Reads return
> >>>>>> zeros,
> >>>>>> +  writes are ignored.
> >>>>> Regarding this paragraph - what about exiting mappings ? In the first
> >>>>> patchset we would actively invalidate all the existing CPU
> >>>>> mappings to
> >>>>> device memory and i think we still should do it otherwise we will see
> >>>>> random crashes in applications as was before. I guess it's because
> >>>>> TLBs
> >>>>> and page tables are not updated to reflect the fact the device is
> >>>>> gone.
> >>>> Hi,
> >>>>
> >>>> I was talking about existing mappings. What I forgot to specify is how
> >>>> new mmap() calls after the device disappearance should work (the end
> >>>> result should be the same still, not failure).
> >>>>
> >>>> I'll clarify this in the next revision.
> >>>>
> >>>>
> >>>> Thanks,
> >>>> pq
> >>>
> >>> I see, that ok.
> >>>
> >>> Next related question is more for Daniel/Christian - about the
> >>> implementation of this paragraph, I was thinking about something like
> >>> checking for device disconnect in ttm_bo_vm_fault_reserved and if so
> >>> remap the entire VA range for the VMA where the fault address
> >>> belongs to
> >>> the global zero page (i.e. (remap_pfn_range(vma, vma->vm_start,
> >>> page_to_pfn(ZERO_PAGE(vma->vm_start), vma->vm_end - vma->vm_start,
> >>> vma->vm_page_prot)). Question is, when the doc says 'writes are
> >>> ignored'
> >>> does it mean i should use copy on write for the vma->vm_page_prot
> >>> and if
> >>> so how i actually do it as i was not able to find what flags to set
> >>> into
> >>> vm_page_prot to force copy on write behavior.
> >> Already discussed this with Pekka on irc, I think simply a private
> >> page (per gpu ctx to avoid leaks) is good enough. Otherwise we need to
> >> catch write faults and throw the writes away, and that's a) a bit
> >> tricky to implement and b) slow, which we kinda don't want to. If the
> >> desktop is stuck for a few seconds because we're trapping every write
> >> of a 4k buffer that's getting uploaded, the user is going to have a
> >> bad time :-/
> >> -Daniel
> >
> >
> > So like allocating a page per process context in the driver (struct
> > amdgpu_ctx in amdgpu) and mapping this page into the faulting VMAs
> > for when device is disconnected ? I am still not clear how i make the
> > mapping ignore writes without catching write faults and ignoring them.
> > I cannot just make it read only obviously and i can't make it writable
> > as then reading back will start returning non 0's. My question is what
> > set of flags in vm_area_struct.vm_flags can (if at all) give me
> > 'ignore writes' behavior for the mapping of that page.
>
> I'm not aware of a possibility like that on x86 CPUs. As far as I know
> we only have something like an ignore write functionality on our GPUs
> for PRTs.
>
> Could we use an address which points to a non allocated MMIO space or
> something like this? We would might get 0xffffffff on reads instead of
> 0x0, but writes would be certainly ignored.

I think just a page with garbage in, garbage out semantics is going to
be ok. I think pretty much anything has a chance to upset userspace,
so whether it's 0 or all 1s or anything else doesn't really matter.

Only thing that does matter a bit is that we have a page per fd, so
that we don't accidentally leak something between processes where we
shouldn't. I think as long as we don't crash&burn in a SIGBUS it's
good enough.
-Daniel

>
> Christian.
>
> >
> > Andrey
> >
> >
> >>
> >>> Andrey
> >>>
> >>>
> >>>
> >>>
> >>>>
> >>>>>> +
> >>>>>> +- dmabuf which point to memory that has disappeared are
> >>>>>> rewritten to
> >>>>>> +  point to harmless zero pages, similar to mmaps. Imports still
> >>>>>> succeed
> >>>>>> +  both ways: an existing device importing a dmabuf pointing to
> >>>>>> +  disappeared memory, and a disappeared device importing any
> >>>>>> dmabuf.
> >>>>>> +
> >>>>>> +- Render ioctls return EIO which is then handled in userspace
> >>>>>> drivers,
> >>>>>> +  e.g. Mesa, to have the device disappearance handled in the way
> >>>>>> +  specified for each API (OpenGL, GL ES: GL_KHR_robustness;
> >>>>>> +  Vulkan: VK_ERROR_DEVICE_LOST; etc.)
> >>>>>> +
> >>>>>> +Raising SIGBUS is not an option, because userspace cannot
> >>>>>> realistically
> >>>>>> +handle it.  Signal handlers are global, which makes them extremely
> >>>>>> +difficult to use correctly from libraries like Mesa produces.
> >>>>>> Signal
> >>>>>> +handlers are not composable, you can't have different handlers
> >>>>>> for GPU1
> >>>>>> +and GPU2 from different vendors, and a third handler for mmapped
> >>>>>> regular
> >>>>>> +files.  Threads cause additional pain with signal handling as well.
> >>>>>> +
> >>>>>> +Only after userspace has closed all relevant DRM device and
> >>>>>> dmabuf file
> >>>>>> +descriptors and removed all mmaps, the DRM driver can tear down its
> >>>>>> +instance for the device that no longer exists. If the same physical
> >>>>>> +device somehow comes back in the mean time, it shall be a new DRM
> >>>>>> +device.
> >>>>>> +
> >>>>>>     .. _drm_driver_ioctl:
> >>>>>>
> >>>>>>     IOCTL Support on Device Nodes
> >>
> >>
>


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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] drm/doc: device hot-unplug for userspace
  2020-05-27 20:25             ` Daniel Vetter
@ 2020-05-28 12:13               ` Pekka Paalanen
  2020-05-28 21:28               ` Andrey Grodzovsky
  1 sibling, 0 replies; 30+ messages in thread
From: Pekka Paalanen @ 2020-05-28 12:13 UTC (permalink / raw)
  To: Andrey Grodzovsky; +Cc: Dave Airlie, dri-devel, Sean Paul, Christian König

[-- Attachment #1.1: Type: text/plain, Size: 5022 bytes --]

On Wed, 27 May 2020 22:25:00 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Wed, May 27, 2020 at 9:44 PM Christian König
> <christian.koenig@amd.com> wrote:
> >
> > Am 27.05.20 um 17:23 schrieb Andrey Grodzovsky:  
> > >
> > > On 5/27/20 10:39 AM, Daniel Vetter wrote:  
> > >> On Wed, May 27, 2020 at 3:51 PM Andrey Grodzovsky
> > >> <Andrey.Grodzovsky@amd.com> wrote:  
> > >>>
> > >>> On 5/27/20 2:44 AM, Pekka Paalanen wrote:  
> > >>>> On Tue, 26 May 2020 10:30:20 -0400
> > >>>> Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> wrote:
> > >>>>  
> > >>>>> On 5/19/20 6:06 AM, Pekka Paalanen wrote:  
> > >>>>>> From: Pekka Paalanen <pekka.paalanen@collabora.com>
> > >>>>>>
> > >>>>>> Set up the expectations on how hot-unplugging a DRM device should
> > >>>>>> look like to
> > >>>>>> userspace.
> > >>>>>>
> > >>>>>> Written by Daniel Vetter's request and largely based on his
> > >>>>>> comments in IRC and
> > >>>>>> from
> > >>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Fdri-devel%2F2020-May%2F265484.html&amp;data=02%7C01%7CAndrey.Grodzovsky%40amd.com%7C3c671803b2ba41b2ceac08d8024bcc9a%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637261871869742519&amp;sdata=ZnhylRubOM0%2BjoreSSYMqVDzZuUdybEsoyBVcTKgxWE%3D&amp;reserved=0
> > >>>>>> .
> > >>>>>>
> > >>>>>> Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> > >>>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
> > >>>>>> Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
> > >>>>>> Cc: Dave Airlie <airlied@redhat.com>
> > >>>>>> Cc: Sean Paul <sean@poorly.run>
> > >>>>>>
> > >>>>>> ---
> > >>>>>>
> > >>>>>> Disclaimer: I am a userspace developer writing for other
> > >>>>>> userspace developers.
> > >>>>>> I took some liberties in defining what should happen without
> > >>>>>> knowing what is
> > >>>>>> actually possible or what existing drivers already implement.
> > >>>>>> ---
> > >>>>>>     Documentation/gpu/drm-uapi.rst | 75
> > >>>>>> ++++++++++++++++++++++++++++++++++
> > >>>>>>     1 file changed, 75 insertions(+)

...

> > >>> Next related question is more for Daniel/Christian - about the
> > >>> implementation of this paragraph, I was thinking about something like
> > >>> checking for device disconnect in ttm_bo_vm_fault_reserved and if so
> > >>> remap the entire VA range for the VMA where the fault address
> > >>> belongs to
> > >>> the global zero page (i.e. (remap_pfn_range(vma, vma->vm_start,
> > >>> page_to_pfn(ZERO_PAGE(vma->vm_start), vma->vm_end - vma->vm_start,
> > >>> vma->vm_page_prot)). Question is, when the doc says 'writes are
> > >>> ignored'
> > >>> does it mean i should use copy on write for the vma->vm_page_prot
> > >>> and if
> > >>> so how i actually do it as i was not able to find what flags to set
> > >>> into
> > >>> vm_page_prot to force copy on write behavior.  
> > >> Already discussed this with Pekka on irc, I think simply a private
> > >> page (per gpu ctx to avoid leaks) is good enough. Otherwise we need to
> > >> catch write faults and throw the writes away, and that's a) a bit
> > >> tricky to implement and b) slow, which we kinda don't want to. If the
> > >> desktop is stuck for a few seconds because we're trapping every write
> > >> of a 4k buffer that's getting uploaded, the user is going to have a
> > >> bad time :-/
> > >> -Daniel  
> > >
> > >
> > > So like allocating a page per process context in the driver (struct
> > > amdgpu_ctx in amdgpu) and mapping this page into the faulting VMAs
> > > for when device is disconnected ? I am still not clear how i make the
> > > mapping ignore writes without catching write faults and ignoring them.
> > > I cannot just make it read only obviously and i can't make it writable
> > > as then reading back will start returning non 0's. My question is what
> > > set of flags in vm_area_struct.vm_flags can (if at all) give me
> > > 'ignore writes' behavior for the mapping of that page.  
> >
> > I'm not aware of a possibility like that on x86 CPUs. As far as I know
> > we only have something like an ignore write functionality on our GPUs
> > for PRTs.
> >
> > Could we use an address which points to a non allocated MMIO space or
> > something like this? We would might get 0xffffffff on reads instead of
> > 0x0, but writes would be certainly ignored.  
> 
> I think just a page with garbage in, garbage out semantics is going to
> be ok. I think pretty much anything has a chance to upset userspace,
> so whether it's 0 or all 1s or anything else doesn't really matter.
> 
> Only thing that does matter a bit is that we have a page per fd, so
> that we don't accidentally leak something between processes where we
> shouldn't. I think as long as we don't crash&burn in a SIGBUS it's
> good enough.

Hi,

the v2 I sent on Monday already changed the wording to have undefined
reads/writes instead of read zero / ignore write. v3 is coming.


Thanks,
pq

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] drm/doc: device hot-unplug for userspace
  2020-05-25 15:09             ` Daniel Vetter
@ 2020-05-28 12:27               ` Pekka Paalanen
  2020-05-28 14:45                 ` Daniel Vetter
  0 siblings, 1 reply; 30+ messages in thread
From: Pekka Paalanen @ 2020-05-28 12:27 UTC (permalink / raw)
  To: Daniel Vetter; +Cc: Sean Paul, dri-devel, Dave Airlie

[-- Attachment #1.1: Type: text/plain, Size: 9423 bytes --]

On Mon, 25 May 2020 17:09:55 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Mon, May 25, 2020 at 05:55:19PM +0300, Pekka Paalanen wrote:
> > On Mon, 25 May 2020 16:28:04 +0200
> > Daniel Vetter <daniel@ffwll.ch> wrote:
> >   
> > > On Wed, May 20, 2020 at 10:50:41AM -0400, Andrey Grodzovsky wrote:  
> > > > 
> > > > On 5/20/20 8:46 AM, Daniel Vetter wrote:    
> > > > > On Wed, May 20, 2020 at 02:19:08PM +0300, Pekka Paalanen wrote:    
> > > > > > On Tue, 19 May 2020 10:37:12 -0400
> > > > > > Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> wrote:
> > > > > >     
> > > > > > > Thanks for the summary, does put things in order and makes it easier to
> > > > > > > comprehend all the TODOs, some questions bellow
> > > > > > > 
> > > > > > > On 5/19/20 6:06 AM, Pekka Paalanen wrote:    
> > > > > > > > From: Pekka Paalanen <pekka.paalanen@collabora.com>
> > > > > > > > 
> > > > > > > > Set up the expectations on how hot-unplugging a DRM device should look like to
> > > > > > > > userspace.

...

> > > > So currently drm_ioctl will just check for drm_dev_is_unplugged and return
> > > > -ENODEV at the very beginning of the function (https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_ioctl.c#L825)
> > > > meaning that currently we blanket failure any IOCTL in case the device is
> > > > unplagued (ignoring the race condition if the device unplugged after the
> > > > check). So do we want to remove this check and put it only for render ioctls
> > > > (which are those ? e.g. for amdgpu there is  AMDGPU_CS ioctl) but not for
> > > > mode setting/dma_buf_import/dma_buf_export ioctls ? What about other types
> > > > of ioctls which are non of the listed above ?    
> > > 
> > > Hm right, and this goes back all the way to first usb udl support:
> > > 
> > > commit 2c07a21d6fb0be47fda696a618b726ea258ed1dd
> > > Author: Dave Airlie <airlied@redhat.com>
> > > Date:   Mon Feb 20 14:18:07 2012 +0000
> > > 
> > >     drm: add core support for unplugging a device (v2)
> > > 
> > > So I guess we need to change the uapi docs to explain that. Not sure
> > > everyone can cope, but I guess they better do. Since the use-case back
> > > then was just udl, so simple legacy kms only, the damage was probably
> > > rather limited. I'm not sure we can get away with that now, where kms code
> > > has spread to funny places likey vulkan winsys code.
> > > 
> > > Or maybe we want a file priv flag you can set along the lines of "give me
> > > less shitty hotunplug semantics for ioctls". Or maybe we can just change
> > > the semantics, not crashing&burning shouldn't cause a regression :-)
> > > 
> > > For everything else (mmap, dma-buf fd, sync_file fd, syncobj fd) I think
> > > the discussion is still more or less accurate.
> > > 
> > > Pekka, any thoughts?  
> > 
> > Hi,
> > 
> > is ENODEV unique to this particular failure?  
> 
> Not really sure, we'd need to audit all of drm ...

$ git ngrep ENODEV -- drivers/gpu/drm | wc -l
762

Yeah, grep is not enough.

> > Returning errors instead of faking success was my first idea, but you
> > already convinced me that faking is at least as good if not better. :-)
> > 
> > So as long as the error code returned is unique to hot-unplug or other
> > "oops, the device is gone" conditions, I think I'm fine. Weston does
> > not handle ENODEV any way, it never did, and it certainly cannot be
> > called a kernel regression.
> > 
> > As a Weston developer, I don't mind adding checks for ENODEV. But if I
> > don't have to, even better. Weston is going to need more code to handle
> > DRM device unplug in any case.
> > 
> > Sorry, no preference from me. ;-)
> > 
> > I do agree that replacing ENODEV with fake success is hard to imagine
> > regressing anything. It's something you can do in the kernel at any
> > time easily, but going from fake success to error is going to be
> > painful. Maybe don't change things until there is a good reason to?
> > 
> > We need a kernel that doesn't crash before we can properly test what
> > would be best for userspace, fake or error.  
> 
> One upshot of faking stuff and only bailing in low-level hw code is that
> it makes validating the races when you hotunplug easier - if we remove the
> early bail-out check even an ioctl later on will look like it raced with
> the hotunplug path in the kernel. So better assurance that things won't
> blow up badly.
> 
> Otoh the early bail out in the top-level ioctl code increases the odds
> that you'll survive even on a driver that's totally buggy.
> 
> So yeah I guess maybe we should just document that currently you get an
> -ENODEV and maybe have the option mentioned that we might change this
> going forward. See also
> 
> https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#recommended-ioctl-return-values
> 
> I think that needs an update, at least clarify that "not present" includes
> "no longer present".

So if dmabuf import related ioctl returns ENODEV, it means a Wayland
compositor should not punish the client for giving a bad buffer.

If a compositor uses EGL to import a dmabuf, then the EGL
implementation will be calling dmabuf import related ioctls, does it
not? But I'm fairly sure that EGL has no way to signal this particular
condition to the caller uniquely. That means that either a compositor
accepts buffers it never should have, or that it punishes clients for
the DRM device disappearing.

See the below quote for a reminder:

> > > > > > > > +- dmabuf which point to memory that has disappeared are rewritten to
> > > > > > > > +  point to harmless zero pages, similar to mmaps. Imports still succeed
> > > > > > > > +  both ways: an existing device importing a dmabuf pointing to
> > > > > > > > +  disappeared memory, and a disappeared device importing any dmabuf.    
> > > > > > > 
> > > > > > > Same as above, I would expect drmPrimeHandleToFD and drmPrimeFDToHandle
> > > > > > > to return -EIO in case device is detached    
> > > > > > I imagined a use case behind this: a Wayland compositor attempting to
> > > > > > import a client's dmabuf. The GPU the client was using is
> > > > > > hot-unplugged, meaning the dmabufs lose their storage. It is up to the
> > > > > > client to handle its rendering GPU disappearance on its own, but also
> > > > > > the compositor should not disconnect it because the GPU disappeared.
> > > > > > It's not the client's fault the GPU disappeared.
> > > > > > 
> > > > > > In Wayland, failure to use a wl_buffer is considered a protocol error,
> > > > > > and protocol errors are always fatal: the client is disconnected. The
> > > > > > rationale is that the compositor must always be able to present the
> > > > > > client buffer somehow. If the compositor cannot, then the client did
> > > > > > not obey the protocol.
> > > > > > 
> > > > > > The fallback presentation path in a compositor is usually importing the
> > > > > > dmabuf to EGL, to be sampled from OpenGL. Normally the protocol
> > > > > > guarantees that this works, so any failure to do so is a protocol
> > > > > > violation. But if the GPU used by the client suddenly disappears and
> > > > > > the imports start to fail, that is interpreted as a protocol violation
> > > > > > unless the compositor can see why the import failed. Since the import
> > > > > > is done via EGL, getting the right error code plumbed through from
> > > > > > libdrm functions to the EGL caller would be a hassle. I don't see any
> > > > > > error code in EGL_EXT_image_dma_buf_import reserved for "the dmabuf
> > > > > > storage was hot-unplugged", and I doubt there is anything exclusively
> > > > > > for only that in the EGL base spec either.
> > > > > > 
> > > > > > The cost of lying that the import worked is that the compositor will
> > > > > > paint black or transparent where the window was supposed to be. It's a
> > > > > > graphical glitch that is contrary to the Wayland design principles, but
> > > > > > in this case a glitch is unavoidable: even if the compositor knew this
> > > > > > buffer is now bad, what would it paint instead? It has nothing else to
> > > > > > paint from. I'm assuming the compositor is using a different GPU than
> > > > > > what disappeared.
> > > > > > 
> > > > > > Ideally, the client will eventually react to losing the GPU and either
> > > > > > crash, quit, or switch its rendering to something that works which
> > > > > > simply gives the compositor a new, working buffer without losing any
> > > > > > window state in the process. If we risk the compositor disconnecting
> > > > > > the client, then the client might not recover even if it wanted to.

This goes for both ways: importing a good dmabuf to a bad device, and a
bad dmabuf to a good device. In neither case there should be a risk of
erroneously disconnecting the Wayland client.

Hmm. Maybe Wayland compositors should ignore all EGL import failures
that happen after the wl_buffer has been created (which implies that
the dmabuf has been validated to work initially). When import fails at
a later time, the compositor should just paint some error pattern
instead of the window. That would let the kernel keep on returning
errors.

Yeah, ok. I'll keep the ENODEV there in my next version. Let's see how
that looks then.


Thanks,
pq

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] drm/doc: device hot-unplug for userspace
  2020-05-28 12:27               ` Pekka Paalanen
@ 2020-05-28 14:45                 ` Daniel Vetter
  0 siblings, 0 replies; 30+ messages in thread
From: Daniel Vetter @ 2020-05-28 14:45 UTC (permalink / raw)
  To: Pekka Paalanen; +Cc: dri-devel, Dave Airlie, Sean Paul

On Thu, May 28, 2020 at 03:27:57PM +0300, Pekka Paalanen wrote:
> On Mon, 25 May 2020 17:09:55 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> 
> > On Mon, May 25, 2020 at 05:55:19PM +0300, Pekka Paalanen wrote:
> > > On Mon, 25 May 2020 16:28:04 +0200
> > > Daniel Vetter <daniel@ffwll.ch> wrote:
> > >   
> > > > On Wed, May 20, 2020 at 10:50:41AM -0400, Andrey Grodzovsky wrote:  
> > > > > 
> > > > > On 5/20/20 8:46 AM, Daniel Vetter wrote:    
> > > > > > On Wed, May 20, 2020 at 02:19:08PM +0300, Pekka Paalanen wrote:    
> > > > > > > On Tue, 19 May 2020 10:37:12 -0400
> > > > > > > Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> wrote:
> > > > > > >     
> > > > > > > > Thanks for the summary, does put things in order and makes it easier to
> > > > > > > > comprehend all the TODOs, some questions bellow
> > > > > > > > 
> > > > > > > > On 5/19/20 6:06 AM, Pekka Paalanen wrote:    
> > > > > > > > > From: Pekka Paalanen <pekka.paalanen@collabora.com>
> > > > > > > > > 
> > > > > > > > > Set up the expectations on how hot-unplugging a DRM device should look like to
> > > > > > > > > userspace.
> 
> ...
> 
> > > > > So currently drm_ioctl will just check for drm_dev_is_unplugged and return
> > > > > -ENODEV at the very beginning of the function (https://elixir.bootlin.com/linux/latest/source/drivers/gpu/drm/drm_ioctl.c#L825)
> > > > > meaning that currently we blanket failure any IOCTL in case the device is
> > > > > unplagued (ignoring the race condition if the device unplugged after the
> > > > > check). So do we want to remove this check and put it only for render ioctls
> > > > > (which are those ? e.g. for amdgpu there is  AMDGPU_CS ioctl) but not for
> > > > > mode setting/dma_buf_import/dma_buf_export ioctls ? What about other types
> > > > > of ioctls which are non of the listed above ?    
> > > > 
> > > > Hm right, and this goes back all the way to first usb udl support:
> > > > 
> > > > commit 2c07a21d6fb0be47fda696a618b726ea258ed1dd
> > > > Author: Dave Airlie <airlied@redhat.com>
> > > > Date:   Mon Feb 20 14:18:07 2012 +0000
> > > > 
> > > >     drm: add core support for unplugging a device (v2)
> > > > 
> > > > So I guess we need to change the uapi docs to explain that. Not sure
> > > > everyone can cope, but I guess they better do. Since the use-case back
> > > > then was just udl, so simple legacy kms only, the damage was probably
> > > > rather limited. I'm not sure we can get away with that now, where kms code
> > > > has spread to funny places likey vulkan winsys code.
> > > > 
> > > > Or maybe we want a file priv flag you can set along the lines of "give me
> > > > less shitty hotunplug semantics for ioctls". Or maybe we can just change
> > > > the semantics, not crashing&burning shouldn't cause a regression :-)
> > > > 
> > > > For everything else (mmap, dma-buf fd, sync_file fd, syncobj fd) I think
> > > > the discussion is still more or less accurate.
> > > > 
> > > > Pekka, any thoughts?  
> > > 
> > > Hi,
> > > 
> > > is ENODEV unique to this particular failure?  
> > 
> > Not really sure, we'd need to audit all of drm ...
> 
> $ git ngrep ENODEV -- drivers/gpu/drm | wc -l
> 762
> 
> Yeah, grep is not enough.
> 
> > > Returning errors instead of faking success was my first idea, but you
> > > already convinced me that faking is at least as good if not better. :-)
> > > 
> > > So as long as the error code returned is unique to hot-unplug or other
> > > "oops, the device is gone" conditions, I think I'm fine. Weston does
> > > not handle ENODEV any way, it never did, and it certainly cannot be
> > > called a kernel regression.
> > > 
> > > As a Weston developer, I don't mind adding checks for ENODEV. But if I
> > > don't have to, even better. Weston is going to need more code to handle
> > > DRM device unplug in any case.
> > > 
> > > Sorry, no preference from me. ;-)
> > > 
> > > I do agree that replacing ENODEV with fake success is hard to imagine
> > > regressing anything. It's something you can do in the kernel at any
> > > time easily, but going from fake success to error is going to be
> > > painful. Maybe don't change things until there is a good reason to?
> > > 
> > > We need a kernel that doesn't crash before we can properly test what
> > > would be best for userspace, fake or error.  
> > 
> > One upshot of faking stuff and only bailing in low-level hw code is that
> > it makes validating the races when you hotunplug easier - if we remove the
> > early bail-out check even an ioctl later on will look like it raced with
> > the hotunplug path in the kernel. So better assurance that things won't
> > blow up badly.
> > 
> > Otoh the early bail out in the top-level ioctl code increases the odds
> > that you'll survive even on a driver that's totally buggy.
> > 
> > So yeah I guess maybe we should just document that currently you get an
> > -ENODEV and maybe have the option mentioned that we might change this
> > going forward. See also
> > 
> > https://dri.freedesktop.org/docs/drm/gpu/drm-uapi.html#recommended-ioctl-return-values
> > 
> > I think that needs an update, at least clarify that "not present" includes
> > "no longer present".
> 
> So if dmabuf import related ioctl returns ENODEV, it means a Wayland
> compositor should not punish the client for giving a bad buffer.

Atm this is only for the drm fd. I think right now dma-buf keep "working",
where "working" = "probably results in some oopses". So maybe for dma-buf
we need to put down the rule that they're not allowed to go boom, and then
figure out how to do that, see below.

> If a compositor uses EGL to import a dmabuf, then the EGL
> implementation will be calling dmabuf import related ioctls, does it
> not? But I'm fairly sure that EGL has no way to signal this particular
> condition to the caller uniquely. That means that either a compositor
> accepts buffers it never should have, or that it punishes clients for
> the DRM device disappearing.
> 
> See the below quote for a reminder:
> 
> > > > > > > > > +- dmabuf which point to memory that has disappeared are rewritten to
> > > > > > > > > +  point to harmless zero pages, similar to mmaps. Imports still succeed
> > > > > > > > > +  both ways: an existing device importing a dmabuf pointing to
> > > > > > > > > +  disappeared memory, and a disappeared device importing any dmabuf.    
> > > > > > > > 
> > > > > > > > Same as above, I would expect drmPrimeHandleToFD and drmPrimeFDToHandle
> > > > > > > > to return -EIO in case device is detached    
> > > > > > > I imagined a use case behind this: a Wayland compositor attempting to
> > > > > > > import a client's dmabuf. The GPU the client was using is
> > > > > > > hot-unplugged, meaning the dmabufs lose their storage. It is up to the
> > > > > > > client to handle its rendering GPU disappearance on its own, but also
> > > > > > > the compositor should not disconnect it because the GPU disappeared.
> > > > > > > It's not the client's fault the GPU disappeared.
> > > > > > > 
> > > > > > > In Wayland, failure to use a wl_buffer is considered a protocol error,
> > > > > > > and protocol errors are always fatal: the client is disconnected. The
> > > > > > > rationale is that the compositor must always be able to present the
> > > > > > > client buffer somehow. If the compositor cannot, then the client did
> > > > > > > not obey the protocol.
> > > > > > > 
> > > > > > > The fallback presentation path in a compositor is usually importing the
> > > > > > > dmabuf to EGL, to be sampled from OpenGL. Normally the protocol
> > > > > > > guarantees that this works, so any failure to do so is a protocol
> > > > > > > violation. But if the GPU used by the client suddenly disappears and
> > > > > > > the imports start to fail, that is interpreted as a protocol violation
> > > > > > > unless the compositor can see why the import failed. Since the import
> > > > > > > is done via EGL, getting the right error code plumbed through from
> > > > > > > libdrm functions to the EGL caller would be a hassle. I don't see any
> > > > > > > error code in EGL_EXT_image_dma_buf_import reserved for "the dmabuf
> > > > > > > storage was hot-unplugged", and I doubt there is anything exclusively
> > > > > > > for only that in the EGL base spec either.
> > > > > > > 
> > > > > > > The cost of lying that the import worked is that the compositor will
> > > > > > > paint black or transparent where the window was supposed to be. It's a
> > > > > > > graphical glitch that is contrary to the Wayland design principles, but
> > > > > > > in this case a glitch is unavoidable: even if the compositor knew this
> > > > > > > buffer is now bad, what would it paint instead? It has nothing else to
> > > > > > > paint from. I'm assuming the compositor is using a different GPU than
> > > > > > > what disappeared.
> > > > > > > 
> > > > > > > Ideally, the client will eventually react to losing the GPU and either
> > > > > > > crash, quit, or switch its rendering to something that works which
> > > > > > > simply gives the compositor a new, working buffer without losing any
> > > > > > > window state in the process. If we risk the compositor disconnecting
> > > > > > > the client, then the client might not recover even if it wanted to.
> 
> This goes for both ways: importing a good dmabuf to a bad device, and a
> bad dmabuf to a good device. In neither case there should be a risk of
> erroneously disconnecting the Wayland client.
> 
> Hmm. Maybe Wayland compositors should ignore all EGL import failures
> that happen after the wl_buffer has been created (which implies that
> the dmabuf has been validated to work initially). When import fails at
> a later time, the compositor should just paint some error pattern
> instead of the window. That would let the kernel keep on returning
> errors.
> 
> Yeah, ok. I'll keep the ENODEV there in my next version. Let's see how
> that looks then.

tbh I have no idea what to do with dma-buf shared across drivers.

For dma-fence it's fairly simple: Force-complete them all, with an error
code of ENODEV. But for dma-buf I have no idea. As long as the dma-buf
sits in system memory it should keep working, plus/minus bugs in the
exporter where it tries to look at device state that might no longer be
there.

The real fun starts when the buffer is in vram, or when the mmap somehow
goes through the device (but that's more a case for integrated gpu, and
it's a bit hard to hotunplug those and consider that a real use-case).
-Daniel

> 
> 
> Thanks,
> pq



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

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH] drm/doc: device hot-unplug for userspace
  2020-05-27 20:25             ` Daniel Vetter
  2020-05-28 12:13               ` Pekka Paalanen
@ 2020-05-28 21:28               ` Andrey Grodzovsky
  1 sibling, 0 replies; 30+ messages in thread
From: Andrey Grodzovsky @ 2020-05-28 21:28 UTC (permalink / raw)
  To: Daniel Vetter, Christian König; +Cc: Dave Airlie, Sean Paul, dri-devel


On 5/27/20 4:25 PM, Daniel Vetter wrote:
> On Wed, May 27, 2020 at 9:44 PM Christian König
> <christian.koenig@amd.com> wrote:
>> Am 27.05.20 um 17:23 schrieb Andrey Grodzovsky:
>>> On 5/27/20 10:39 AM, Daniel Vetter wrote:
>>>> On Wed, May 27, 2020 at 3:51 PM Andrey Grodzovsky
>>>> <Andrey.Grodzovsky@amd.com> wrote:
>>>>> On 5/27/20 2:44 AM, Pekka Paalanen wrote:
>>>>>> On Tue, 26 May 2020 10:30:20 -0400
>>>>>> Andrey Grodzovsky <Andrey.Grodzovsky@amd.com> wrote:
>>>>>>
>>>>>>> On 5/19/20 6:06 AM, Pekka Paalanen wrote:
>>>>>>>> From: Pekka Paalanen <pekka.paalanen@collabora.com>
>>>>>>>>
>>>>>>>> Set up the expectations on how hot-unplugging a DRM device should
>>>>>>>> look like to
>>>>>>>> userspace.
>>>>>>>>
>>>>>>>> Written by Daniel Vetter's request and largely based on his
>>>>>>>> comments in IRC and
>>>>>>>> from
>>>>>>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Farchives%2Fdri-devel%2F2020-May%2F265484.html&amp;data=02%7C01%7CAndrey.Grodzovsky%40amd.com%7Cd1aab2c6fe71407a287708d8027c0f3c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637262079143242123&amp;sdata=krqBSHMfzl%2F4TMaAgEPDq8Y%2BPYWJATZyeDPfhtWQmeg%3D&amp;reserved=0
>>>>>>>> .
>>>>>>>>
>>>>>>>> Signed-off-by: Pekka Paalanen <pekka.paalanen@collabora.com>
>>>>>>>> Cc: Daniel Vetter <daniel@ffwll.ch>
>>>>>>>> Cc: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>>>>>>>> Cc: Dave Airlie <airlied@redhat.com>
>>>>>>>> Cc: Sean Paul <sean@poorly.run>
>>>>>>>>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> Disclaimer: I am a userspace developer writing for other
>>>>>>>> userspace developers.
>>>>>>>> I took some liberties in defining what should happen without
>>>>>>>> knowing what is
>>>>>>>> actually possible or what existing drivers already implement.
>>>>>>>> ---
>>>>>>>>      Documentation/gpu/drm-uapi.rst | 75
>>>>>>>> ++++++++++++++++++++++++++++++++++
>>>>>>>>      1 file changed, 75 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/Documentation/gpu/drm-uapi.rst
>>>>>>>> b/Documentation/gpu/drm-uapi.rst
>>>>>>>> index 56fec6ed1ad8..80db4abd2cbd 100644
>>>>>>>> --- a/Documentation/gpu/drm-uapi.rst
>>>>>>>> +++ b/Documentation/gpu/drm-uapi.rst
>>>>>>>> @@ -1,3 +1,5 @@
>>>>>>>> +.. Copyright 2020 DisplayLink (UK) Ltd.
>>>>>>>> +
>>>>>>>>      ===================
>>>>>>>>      Userland interfaces
>>>>>>>>      ===================
>>>>>>>> @@ -162,6 +164,79 @@ other hand, a driver requires shared state
>>>>>>>> between clients which is
>>>>>>>>      visible to user-space and accessible beyond open-file
>>>>>>>> boundaries, they
>>>>>>>>      cannot support render nodes.
>>>>>>>>
>>>>>>>> +Device Hot-Unplug
>>>>>>>> +=================
>>>>>>>> +
>>>>>>>> +.. note::
>>>>>>>> +   The following is the plan. Implementation is not there yet
>>>>>>>> +   (2020 May 13).
>>>>>>>> +
>>>>>>>> +Graphics devices (display and/or render) may be connected via
>>>>>>>> USB (e.g.
>>>>>>>> +display adapters or docking stations) or Thunderbolt (e.g.
>>>>>>>> eGPU). An end
>>>>>>>> +user is able to hot-unplug this kind of devices while they are
>>>>>>>> being
>>>>>>>> +used, and expects that the very least the machine does not
>>>>>>>> crash. Any
>>>>>>>> +damage from hot-unplugging a DRM device needs to be limited as
>>>>>>>> much as
>>>>>>>> +possible and userspace must be given the chance to handle it if
>>>>>>>> it wants
>>>>>>>> +to. Ideally, unplugging a DRM device still lets a desktop to
>>>>>>>> continue
>>>>>>>> +running, but that is going to need explicit support throughout
>>>>>>>> the whole
>>>>>>>> +graphics stack: from kernel and userspace drivers, through display
>>>>>>>> +servers, via window system protocols, and in applications and
>>>>>>>> libraries.
>>>>>>>> +
>>>>>>>> +Other scenarios that should lead to the same are: unrecoverable GPU
>>>>>>>> +crash, PCI device disappearing off the bus, or forced unbind of
>>>>>>>> a driver
>>>>>>>> +from the physical device.
>>>>>>>> +
>>>>>>>> +In other words, from userspace perspective everything needs to
>>>>>>>> keep on
>>>>>>>> +working more or less, until userspace stops using the
>>>>>>>> disappeared DRM
>>>>>>>> +device and closes it completely. Userspace will learn of the device
>>>>>>>> +disappearance from the device removed uevent or in some cases
>>>>>>>> specific
>>>>>>>> +ioctls returning EIO.
>>>>>>>> +
>>>>>>>> +This goal raises at least the following requirements for the
>>>>>>>> kernel and
>>>>>>>> +drivers:
>>>>>>>> +
>>>>>>>> +- The kernel must not hang, crash or oops, no matter what
>>>>>>>> userspace was
>>>>>>>> +  in the middle of doing when the device disappeared.
>>>>>>>> +
>>>>>>>> +- All GPU jobs that can no longer run must have their fences
>>>>>>>> +  force-signalled to avoid inflicting hangs to userspace.
>>>>>>>> +
>>>>>>>> +- KMS connectors must change their status to disconnected.
>>>>>>>> +
>>>>>>>> +- Legacy modesets and pageflips fake success.
>>>>>>>> +
>>>>>>>> +- Atomic commits, both real and TEST_ONLY, fake success.
>>>>>>>> +
>>>>>>>> +- Pending non-blocking KMS operations deliver the DRM events
>>>>>>>> userspace
>>>>>>>> +  is expecting.
>>>>>>>> +
>>>>>>>> +- If underlying memory disappears, the mmaps are replaced with
>>>>>>>> harmless
>>>>>>>> +  zero pages where access does not raise SIGBUS. Reads return
>>>>>>>> zeros,
>>>>>>>> +  writes are ignored.
>>>>>>> Regarding this paragraph - what about exiting mappings ? In the first
>>>>>>> patchset we would actively invalidate all the existing CPU
>>>>>>> mappings to
>>>>>>> device memory and i think we still should do it otherwise we will see
>>>>>>> random crashes in applications as was before. I guess it's because
>>>>>>> TLBs
>>>>>>> and page tables are not updated to reflect the fact the device is
>>>>>>> gone.
>>>>>> Hi,
>>>>>>
>>>>>> I was talking about existing mappings. What I forgot to specify is how
>>>>>> new mmap() calls after the device disappearance should work (the end
>>>>>> result should be the same still, not failure).
>>>>>>
>>>>>> I'll clarify this in the next revision.
>>>>>>
>>>>>>
>>>>>> Thanks,
>>>>>> pq
>>>>> I see, that ok.
>>>>>
>>>>> Next related question is more for Daniel/Christian - about the
>>>>> implementation of this paragraph, I was thinking about something like
>>>>> checking for device disconnect in ttm_bo_vm_fault_reserved and if so
>>>>> remap the entire VA range for the VMA where the fault address
>>>>> belongs to
>>>>> the global zero page (i.e. (remap_pfn_range(vma, vma->vm_start,
>>>>> page_to_pfn(ZERO_PAGE(vma->vm_start), vma->vm_end - vma->vm_start,
>>>>> vma->vm_page_prot)). Question is, when the doc says 'writes are
>>>>> ignored'
>>>>> does it mean i should use copy on write for the vma->vm_page_prot
>>>>> and if
>>>>> so how i actually do it as i was not able to find what flags to set
>>>>> into
>>>>> vm_page_prot to force copy on write behavior.
>>>> Already discussed this with Pekka on irc, I think simply a private
>>>> page (per gpu ctx to avoid leaks) is good enough. Otherwise we need to
>>>> catch write faults and throw the writes away, and that's a) a bit
>>>> tricky to implement and b) slow, which we kinda don't want to. If the
>>>> desktop is stuck for a few seconds because we're trapping every write
>>>> of a 4k buffer that's getting uploaded, the user is going to have a
>>>> bad time :-/
>>>> -Daniel
>>>
>>> So like allocating a page per process context in the driver (struct
>>> amdgpu_ctx in amdgpu) and mapping this page into the faulting VMAs
>>> for when device is disconnected ? I am still not clear how i make the
>>> mapping ignore writes without catching write faults and ignoring them.
>>> I cannot just make it read only obviously and i can't make it writable
>>> as then reading back will start returning non 0's. My question is what
>>> set of flags in vm_area_struct.vm_flags can (if at all) give me
>>> 'ignore writes' behavior for the mapping of that page.
>> I'm not aware of a possibility like that on x86 CPUs. As far as I know
>> we only have something like an ignore write functionality on our GPUs
>> for PRTs.
>>
>> Could we use an address which points to a non allocated MMIO space or
>> something like this? We would might get 0xffffffff on reads instead of
>> 0x0, but writes would be certainly ignored.
> I think just a page with garbage in, garbage out semantics is going to
> be ok. I think pretty much anything has a chance to upset userspace,
> so whether it's 0 or all 1s or anything else doesn't really matter.
>
> Only thing that does matter a bit is that we have a page per fd, so
> that we don't accidentally leak something between processes where we
> shouldn't. I think as long as we don't crash&burn in a SIGBUS it's
> good enough.
> -Daniel


To use non allocated MMIO space i would need first to know which range 
is currently not used (how ?) and then reserve it (and free later) to 
avoid other devices start using it. I think the interface for this is 
https://elixir.bootlin.com/linux/v5.7-rc7/source/include/linux/ioport.h#L233. 
But still i like the zero page approach more where we map the zero page 
during new page faults into the faulting process page table with setting 
adding ~(VM_SHARED | VM_MAYSHARE) to vma->vm_flags or at least to 
pgprot_t for this particular maping which i think makes the mapping copy 
on write and so each process (each FD) will also not leak data to other 
processes.

Andrey


>
>> Christian.
>>
>>> Andrey
>>>
>>>
>>>>> Andrey
>>>>>
>>>>>
>>>>>
>>>>>
>>>>>>>> +
>>>>>>>> +- dmabuf which point to memory that has disappeared are
>>>>>>>> rewritten to
>>>>>>>> +  point to harmless zero pages, similar to mmaps. Imports still
>>>>>>>> succeed
>>>>>>>> +  both ways: an existing device importing a dmabuf pointing to
>>>>>>>> +  disappeared memory, and a disappeared device importing any
>>>>>>>> dmabuf.
>>>>>>>> +
>>>>>>>> +- Render ioctls return EIO which is then handled in userspace
>>>>>>>> drivers,
>>>>>>>> +  e.g. Mesa, to have the device disappearance handled in the way
>>>>>>>> +  specified for each API (OpenGL, GL ES: GL_KHR_robustness;
>>>>>>>> +  Vulkan: VK_ERROR_DEVICE_LOST; etc.)
>>>>>>>> +
>>>>>>>> +Raising SIGBUS is not an option, because userspace cannot
>>>>>>>> realistically
>>>>>>>> +handle it.  Signal handlers are global, which makes them extremely
>>>>>>>> +difficult to use correctly from libraries like Mesa produces.
>>>>>>>> Signal
>>>>>>>> +handlers are not composable, you can't have different handlers
>>>>>>>> for GPU1
>>>>>>>> +and GPU2 from different vendors, and a third handler for mmapped
>>>>>>>> regular
>>>>>>>> +files.  Threads cause additional pain with signal handling as well.
>>>>>>>> +
>>>>>>>> +Only after userspace has closed all relevant DRM device and
>>>>>>>> dmabuf file
>>>>>>>> +descriptors and removed all mmaps, the DRM driver can tear down its
>>>>>>>> +instance for the device that no longer exists. If the same physical
>>>>>>>> +device somehow comes back in the mean time, it shall be a new DRM
>>>>>>>> +device.
>>>>>>>> +
>>>>>>>>      .. _drm_driver_ioctl:
>>>>>>>>
>>>>>>>>      IOCTL Support on Device Nodes
>>>>
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, back to index

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-19 10:06 [PATCH] drm/doc: device hot-unplug for userspace Pekka Paalanen
2020-05-19 14:37 ` Andrey Grodzovsky
2020-05-20 11:19   ` Pekka Paalanen
2020-05-20 12:46     ` Daniel Vetter
2020-05-20 14:50       ` Andrey Grodzovsky
2020-05-25 12:13         ` Andrey Grodzovsky
2020-05-25 14:28         ` Daniel Vetter
2020-05-25 14:55           ` Pekka Paalanen
2020-05-25 15:09             ` Daniel Vetter
2020-05-28 12:27               ` Pekka Paalanen
2020-05-28 14:45                 ` Daniel Vetter
2020-05-20 12:55 ` Daniel Vetter
2020-05-20 13:20   ` Simon Ser
2020-05-20 14:19     ` Daniel Vetter
2020-05-22  9:54       ` Pekka Paalanen
2020-05-25 14:29         ` Daniel Vetter
2020-05-25 12:46 ` [PATCH v2] " Pekka Paalanen
2020-05-25 13:51   ` Andrey Grodzovsky
2020-05-25 14:30     ` Daniel Vetter
2020-05-25 15:02       ` Pekka Paalanen
2020-05-25 14:41     ` Pekka Paalanen
2020-05-26 14:30 ` [PATCH] " Andrey Grodzovsky
2020-05-27  6:44   ` Pekka Paalanen
2020-05-27 13:51     ` Andrey Grodzovsky
2020-05-27 14:39       ` Daniel Vetter
2020-05-27 15:23         ` Andrey Grodzovsky
2020-05-27 19:44           ` Christian König
2020-05-27 20:25             ` Daniel Vetter
2020-05-28 12:13               ` Pekka Paalanen
2020-05-28 21:28               ` Andrey Grodzovsky

dri-devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/dri-devel/0 dri-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dri-devel dri-devel/ https://lore.kernel.org/dri-devel \
		dri-devel@lists.freedesktop.org
	public-inbox-index dri-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.freedesktop.lists.dri-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git