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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Daniel,

On Monday, 4 September 2017 10:26:15 EEST Daniel Vetter wrote:
> On Thu, Aug 31, 2017 at 09:22:03PM +0200, 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:
> >>>>>> +
> >>>>>> +       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).

Don't forget that we need to check the disconnect flag when entering ioctls to 
return -ENODEV. I'm open to clever solutions, but I'd first aim for 
correctness with a lock and then replace the internal implementation.

-- 
Regards,

Laurent Pinchart

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel