[PATCH 2/2] drm: make unplugged flag specific to udl driver

[Daniel Vetter]

On Wed, Feb 10, 2016 at 02:02:46PM -0800, Stéphane Marchesin wrote:
> On Wed, Feb 10, 2016 at 1:54 PM, David Herrmann <dh.herrmann at gmail.com> wrote:
> > Hi
> >
> > On Wed, Feb 10, 2016 at 10:46 PM, Stéphane Marchesin
> > <stephane.marchesin at gmail.com> wrote:
> >> On Wed, Feb 10, 2016 at 1:38 PM, David Herrmann <dh.herrmann at gmail.com> wrote:
> >>> Hi
> >>>
> >>> On Wed, Feb 10, 2016 at 9:39 PM, Haixia Shi <hshi at chromium.org> wrote:
> >>>>
> >>>>> +       if (udl_device_is_unplugged(dev) &&
> >>>>> +               nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_SETCRTC) &&
> >>>>> +               nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_RMFB) &&
> >>>>> +               nr != DRM_IOCTL_NR(DRM_IOCTL_MODE_DESTROY_DUMB))
> >>>>> +               return -ENODEV;
> >>>>>
> >>>>>Why?
> >>>>>
> >>>>>Just do:
> >>>>>
> >>>>>        if (udl_device_is_unplugged(dev))
> >>>>>                return -ENODEV;
> >>>>>
> >>>>>Why this complex logic here?
> >>>>
> >>>> Because there are legitimate ioctl calls after UDL is unplugged. See
> >>>> crbug.com/583508 and crbug.com/583758 for some background.
> >>>>
> >>>> The userspace (Chrome in this case) has allocated FBs and Dumb buffers on
> >>>> the drm device and needs to be given a chance to properly deallocate them
> >>>> (via RMFB and DESTROY_DUMB). In addition, it needs to call SETCRTC with
> >>>> fb_id = 0 to properly release the last refcount on the primary fb.
> >>>>
> >>>> I initially proposed adding an "UNPLUG_DISALLOW" flag to ioctls so that we
> >>>> can whitelist them on a case-by-case basis but that proposal got shot down
> >>>> as being unnecessary, but you can see my original patch at
> >>>> https://chromium-review.googlesource.com/#/c/326160/
> >>>
> >>> If a device is unplugged, you should consider all your resources to be
> >>> destroyed. There is no reason to release them manually. User-space
> >>> *must* be able to deal with asynchronous unplugs.
> >>
> >> So the problem if you do that is that things like a buffer's memory
> >> pages can disappear from under you. How would you deal with that case?
> >> User space certainly can't have a segfault handler catch that just in
> >> case :)

Yeah, dma-buf and fence lifetime is entirely unsolved. I agree that they
/should/ keep being alive. Of course the actual data in it might be toast,
but that's not any different from a gpu hang. At least i915 only gives you
asynchronous signalling for "bad stuff happened" in that case, buffer
access to corrupted data continues to work. And must do so, because
X/compositors/clients would just die if we don't do that, and that's Not
Good(tm).

> > If you rip out hardware resources, then you better be able to deal
> > with it. Sure, UDL is an exception as it doesn't have memory resources
> > on the chip. But it sounds fishy to me, if you base your API on it. On
> > a lot of other devices, the memory will simply not be there. So you
> > cannot keep it around.
> 
> The thing is, you are not unplugging a device here; you are unplugging
> a USB monitor. As a proof that this is just a monitor, I can plug
> another USB monitor with the same driver and pick up where I left off.
> I guess I am saying that the concept of unplugging a device is not
> applicable here (or to any driver that I know, for that matter).
> 
> Other drivers already handle all this by, for example, failing page
> flips if the monitor is gone. We basically want to do the same for
> UDL; I don't think we need to invent a new level of unplug here.

Just an aside: Imo failing pageflips is really bad behaviour of some
kernel drivers (yes mst does it by force-disabling sinks). Imo userspace
should ask for things to get disabled explicitly, much less potential for
races. For mst I think the right solution is to send out the uevent, stope
enumerating the port, but keep it internally alive until it's all gone.

Something similar could make sense for uld.

> > There are many ways to invalidate memory mappings. You either unmap
> > the entire range (and user-space must deal with SIGBUS, which is
> > completely feasible and a lot of code already does it), or you replace
> > all with a zero page, or you duplicate all pages, ... IMO, user-space
> > has to start dealing with hardware unplug properly and stop pretending
> > it cannot happen.
> 
> What you are suggesting is much more complicated than you claim, for
> example if you destroy the dmabuf which is shared with another driver,
> what happens? User space definitely can't deal with that.
> 
> I think we should wait until we have unpluggable display hardware
> before inventing really complex support for it.

I agree that for now we probably should just have hacks for udl (and yeah
fixing up mst to no longer just go poof is on my todo list somewhere), and
leave the larger issue of drivers disappearing unfixed. Atm module unload
is the only real user for that (except udl ofc), and that's a developer
feature.

Fixing all the dma-buf/fence/drm device lifetime issues properly is super
hard I think. And tons of work.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch