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

[Daniel Vetter]

On Thu, Aug 31, 2017 at 09:22:03PM +0200, Noralf Trønnes wrote:
> 
> Den 31.08.2017 14.59, skrev Laurent Pinchart:
> > Hi Daniel and Noralf,
> > 
> > On Wednesday, 30 August 2017 20:18:49 EEST Daniel Vetter wrote:
> > > On Wed, Aug 30, 2017 at 6:31 PM, Noralf Trønnes <noralf at tronnes.org> wrote:
> > > > Den 28.08.2017 23.56, skrev Daniel Vetter:
> > > > > On Mon, Aug 28, 2017 at 07:17:48PM +0200, Noralf Trønnes wrote:
> > > > > > +
> > > > > > +       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:
> 
> * @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;
> 
>     /* flush framebuffer */
> 
>     tinydrm_dev_exit(tdev);
> }

Yup, expect imo this should be done in drm core (as much as possible, i.e.
for ioctl at least, standard sysfs), plus then enter/exit exported to
drivers for their stuff.

And it really needs to be srcu. For kms drivers the atomic_inc/dec wont
matter, for render drivers it will show up (some of our ioctls are 100%
lock less in the fastpath, not a single atomic op in there).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch