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

[Stéphane Marchesin]

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 :)
>
> 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.

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

Stéphane

>
> If you mmap() your filesystem, and you rip out your block device, then
> you also will get SIGBUS if you access pages that are not in
> pagecache. Why are graphics buffers different?
>
> Thanks
> David