[PATCH 6/6] drm/tinydrm: Support device unplug

[Noralf Trønnes]

Den 01.09.2017 10.38, skrev Laurent Pinchart:
> Hi Noralf,
>
> On Thursday, 31 August 2017 22:22:03 EEST Noralf Trønnes wrote:
>> Den 31.08.2017 14.59, skrev Laurent Pinchart:
>>> On Wednesday, 30 August 2017 20:18:49 EEST Daniel Vetter wrote:
>>>> On Wed, Aug 30, 2017 at 6:31 PM, Noralf Trønnes wrote:
>>>>> Den 28.08.2017 23.56, skrev Daniel Vetter:
>>>>>> On Mon, Aug 28, 2017 at 07:17:48PM +0200, Noralf Trønnes wrote:
>>>>>>> Support device unplugging to make tinydrm suitable for USB devices.
>>>>>>>
>>>>>>> Cc: David Lechner <david at lechnology.com>
>>>>>>> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
>>>>>>> ---
>>>>>>>
>>>>>>> drivers/gpu/drm/tinydrm/core/tinydrm-core.c | 69 ++++++++++++++++++---
>>>>>>> drivers/gpu/drm/tinydrm/mi0283qt.c          |  4 ++
>>>>>>> drivers/gpu/drm/tinydrm/mipi-dbi.c          |  5 ++-
>>>>>>> drivers/gpu/drm/tinydrm/repaper.c           |  9 +++-
>>>>>>> drivers/gpu/drm/tinydrm/st7586.c            |  9 +++-
>>>>>>> include/drm/tinydrm/tinydrm.h               |  5 +++
>>>>>>> 6 files changed, 87 insertions(+), 14 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c

[snip]

>>>>>>> +
>>>>>>> +       drm_fbdev_cma_dev_unplug(tdev->fbdev_cma);
>>>>>>> +       drm_dev_unplug(&tdev->drm);
>>>>>>> +
>>>>>>> +       /* Make sure framebuffer flushing is done */
>>>>>>> +       mutex_lock(&tdev->dirty_lock);
>>>>>>> +       mutex_unlock(&tdev->dirty_lock);
>>>>>> Is this really needed? Or, doesn't it just paper over a driver bug you
>>>>>> have already anyway, since native kms userspace can directly call
>>>>>> fb->funcs->dirty too, and you already protect against that.
>>>>>>
>>>>>> This definitely looks like the fbdev helper is leaking implementation
>>>>>> details to callers where it shouldn't do that.
>>>>> Flushing can happen while drm_dev_unplug() is called, and when we leave
>>>>> this function the device facing resources controlled by devres will be
>>>>> removed. Thus I have to make sure any such flushing is done before
>>>>> leaving so the next flush is stopped by the drm_dev_is_unplugged()
>>>>> check. I don't see any other way of ensuring that.
>>>>>
>>>>> I see now that I should move the call to drm_atomic_helper_shutdown()
>>>>> after drm_dev_unplug() to properly protect the pipe .enable/.disable
>>>>> callbacks.
>>>> Hm, calling _shutdown when the hw is gone already won't end well.
>>>> Fundamentally this race exists for all use-cases, and I'm somewhat
>>>> leaning towards plugging it in the core.
>>>>
>>>> The general solution probably involves something that smells a lot
>>>> like srcu, i.e. at every possible entry point into a drm driver
>>>> (ioctl, fbdev, dma-buf sharing, everything really) we take that
>>>> super-cheap read-side look, and drop it when we leave.
>>> That's similar to what we plan to do in V4L2. The idea is to set a device
>>> removed flag at the beginning of the .remove() handler and wait for all
>>> pending operations to complete. The core will reject any new operation
>>> when the flag is set. To wait for completion, every entry point would
>>> increase a use count, and decrease it on exit. When the use count is
>>> decreased to 0 waiters will be woken up. This should solve the unplug/user
>>> race.
>> Ah, such a simple solution, easy to understand and difficult to get wrong!
>> And it's even nestable, no danger of deadlocking.
>>
>> Maybe I can use it with tinydrm:
> It would be better to implement that in the DRM core to reject all ioctls
> early, as some of them could call drm_driver operations that can't return an
> error (such as .disable_vblank() for instance).

I'll look at that.

drm_ioctl() already rejects calls when unplugged:

     if (drm_device_is_unplugged(dev))
         return -ENODEV;


>> * @dev_use: Tracks use of functions acessing the parent device.
>> *           If it is zero, the device is gone. See ...
>> struct tinydrm_device {
>>       atomic_t dev_use;
>> };
>>
>> /**
>>    * tinydrm_dev_enter - Enter device accessing function
>>    * @tdev: tinydrm device
>>    *
>>    * This function protects against using a device and it's resources after
>>    * it's removed. Should be called at the beginning of the function.
>>    *
>>    * Returns:
>>    * False if the device is still present, true if it is gone.
>>    */
>> static inline bool tinydrm_dev_enter(struct tinydrm_device *tdev)
>> {
>>       return !atomic_inc_not_zero(&tdev->dev_use);
>> }
>>
>> static inline void tinydrm_dev_exit(struct tinydrm_device *tdev)
>> {
>>       atomic_dec(&tdev->dev_use);
>> }
>>
>> static inline bool tinydrm_is_unplugged(struct tinydrm_device *tdev)
>> {
>>       bool ret = !atomic_read(&tdev->dev_use);
>>       smp_rmb();
>>       return ret;
>> }
>>
>>
>> static int tinydrm_init(...)
>> {
>>       /* initialize */
>>
>>       /* Set device is present */
>>       atomic_set(&tdev->dev_use, 1);
>> }
>>
>> static void tinydrm_unregister(...)
>> {
>>       /* Set device gone */
>>       atomic_dec(&tdev->dev_use);
>>
>>       /* Wait for all device facing functions to finish */
>>       while (!tinydrm_is_unplugged(tdev)) {
>>           cond_resched();
>>       }
>>
>>       /* proceed with unregistering */
>> }
>>
>> static int mipi_dbi_fb_dirty(...)
>> {
>>       if (tinydrm_dev_enter(tdev))
>>           return -ENODEV;
> Nitpicking, isn't the right error code -ENXIO ?

I'm using the pattern as shown in the drm_ioctl example above.

>>       /* flush framebuffer */
>>
>>       tinydrm_dev_exit(tdev);
>> }

[snip]