On Wed, 22 Jan 2025 07:22:25 +0200 Raag Jadav <raag.jadav@xxxxxxxxx> wrote: > On Tue, Jan 21, 2025 at 02:14:56AM +0100, Xaver Hugl wrote: > > > +It is the responsibility of the consumer to make sure that the device or > > > +its resources are not in use by any process before attempting recovery. > > I'm not convinced this is actually doable in practice, outside of > > killing all apps that aren't the one trying to recover the GPU. > > Is this just about not crashing those processes if they don't handle > > GPU hotunplugs well, about leaks, or something else? > > Correct, all of it. And since the compositor is in charge of device resources, > this way it atleast has the opportunity to recover the device and recreate > context without all the userspace violence. Hi Raag, sorry, I haven't followed this series, so I wonder, why should userspace be part of recovering the device? Why doesn't the kernel automatically load a new driver instance with a new DRM device node? Of course userspace needs to deal with stuff suddenly erroring out, and destroy existing related resources, then wait for a working device to appear and rebuild all state. The kernel driver already needs to make the existing open stuff inert and harmless, why does it need an acknowledgement from userspace to unbind and re-bind? > I'm not entirely aware of its feasibility though, perhaps something for the > consumers to experiment. If consumers mean userspace, then no, not reliably. But the kernel can do it. I see in the commit message written: "For example, if the driver supports multiple recovery methods, consumers can opt for the suitable one based on policy definition." How could consumers know what to do? How can they guess what would be enough to recover the device? Isn't that the kernel driver's job to know? (More important for userspace would be know if dmabuf fds remain pointing to valid memory retaining its contents or if the contents are lost. Userspace cannot tell which device a dmabuf originates from, AFAIK, so this would need to be added in the generic dmabuf UAPI.) "Consumers can also choose to have the device available for debugging or additional data collection before performing the recovery." Couldn't the wedged driver instance remain detached from the hardware while a new driver instance initializes? Then debug data remains until the wedged device is fully closed from userspace, or maybe devcore dump retains it. I presume that WEDGED=none case should retain the debug data somehow as well. > > > +With IOCTLs blocked and device already 'wedged', all device memory should btw. when I see "blocked" I think of the function call not returning yet. But in this patch "blocked" seems to be synonymous for "returns an error immediately". Would it be possible to avoid the word "blocked" for this? > > > +be unmapped and file descriptors should be closed to prevent leaks. > > Afaiu from a userspace POV, a rebind is just like a GPU hotunplug + > > hotplug with matching "remove" and "add" udev events. As long as the > > application cleans up resources related to the device when it receives > > the event, there should be no leaks with a normal hotunplug... Is this > > different enough that we can't have the same expectations? > > The thing about "remove" event is that it is generated *after* we opt for an > unbind, and at that point it might be already too late if userspace doesn't > get enough time to clean things up while the device is removed with a live > client resulting in unknown consequences. > > The idea here is to clean things up *before* we opt for an unbind leaving > no room for side effects. Something here feels fragile. There should not be a deadline for userspace to finish cleaning up. What was described for KMS device nodes in this same document seems like a more reliable approach: keep the dead driver instance around until userspace has closed all references to it. The device node could be removed earlier. Thanks, pq
Attachment:
pgpvZlZ8E7VL6.pgp
Description: OpenPGP digital signature
