[Intel-gfx] [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()

Tue Feb 5 09:11:18 UTC 2019

On Mon, Feb 04, 2019 at 06:35:28PM +0100, Noralf Trønnes wrote:
> 
> 
> Den 04.02.2019 16.41, skrev Daniel Vetter:
> > On Sun, Feb 03, 2019 at 04:41:56PM +0100, Noralf Trønnes wrote:
> >> The only thing now that makes drm_dev_unplug() special is that it sets
> >> drm_device->unplugged. Move this code to drm_dev_unregister() so that we
> >> can remove drm_dev_unplug().
> >>
> >> Signed-off-by: Noralf Trønnes <noralf at tronnes.org>
> >> ---
> 
> [...]
> 
> >>  drivers/gpu/drm/drm_drv.c | 27 +++++++++++++++------------
> >>  include/drm/drm_drv.h     | 10 ++++------
> >>  2 files changed, 19 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/drm_drv.c b/drivers/gpu/drm/drm_drv.c
> >> index 05bbc2b622fc..e0941200edc6 100644
> >> --- a/drivers/gpu/drm/drm_drv.c
> >> +++ b/drivers/gpu/drm/drm_drv.c
> >> @@ -366,15 +366,6 @@ EXPORT_SYMBOL(drm_dev_exit);
> >>   */
> >>  void drm_dev_unplug(struct drm_device *dev)
> >>  {
> >> -	/*
> >> -	 * After synchronizing any critical read section is guaranteed to see
> >> -	 * the new value of ->unplugged, and any critical section which might
> >> -	 * still have seen the old value of ->unplugged is guaranteed to have
> >> -	 * finished.
> >> -	 */
> >> -	dev->unplugged = true;
> >> -	synchronize_srcu(&drm_unplug_srcu);
> >> -
> >>  	drm_dev_unregister(dev);
> >>  	drm_dev_put(dev);
> >>  }
> >> @@ -832,11 +823,14 @@ EXPORT_SYMBOL(drm_dev_register);
> >>   * drm_dev_register() but does not deallocate the device. The caller must call
> >>   * drm_dev_put() to drop their final reference.
> >>   *
> >> - * A special form of unregistering for hotpluggable devices is drm_dev_unplug(),
> >> - * which can be called while there are still open users of @dev.
> >> + * This function can be called while there are still open users of @dev as long
> >> + * as the driver protects its device resources using drm_dev_enter() and
> >> + * drm_dev_exit().
> >>   *
> >>   * This should be called first in the device teardown code to make sure
> >> - * userspace can't access the device instance any more.
> >> + * userspace can't access the device instance any more. Drivers that support
> >> + * device unplug will probably want to call drm_atomic_helper_shutdown() first
> > 
> > Read once more with a bit more coffee, spotted this:
> > 
> > s/first/afterwards/ - shutting down the hw before we've taken it away from
> > userspace is kinda the wrong way round. It should be the inverse of driver
> > load, which is 1) allocate structures 2) prep hw 3) register driver with
> > the world (simplified ofc).
> > 
> 
> The problem is that drm_dev_unregister() sets the device as unplugged
> and if drm_atomic_helper_shutdown() is called afterwards it's not
> allowed to touch hardware.
> 
> I know it's the wrong order, but the only way to do it in the right
> order is to have a separate function that sets unplugged:
> 
> 	drm_dev_unregister();
> 	drm_atomic_helper_shutdown();
> 	drm_dev_set_unplugged();

Annoying ... but yeah calling _shutdown() before we stopped userspace is
also not going to work. Because userspace could quickly re-enable
something, and then the refcounts would be all wrong again and leaking
objects.

I get a bit the feeling we're over-optimizing here with trying to devm-ize
drm_dev_register. Just getting drm_device correctly devm-ized is a big
step forward already, and will open up a lot of TODO items across a lot of
drivers. E.g. we could add a drm_dev_kzalloc, for allocating all the drm_*
structs, which gets released together with drm_device. I think that's a
much clearer path forward, I think we all agree that getting the kfree out
of the driver codes is a good thing, and it would allow us to do this
correctly.

Then once we have that and rolled out to a few drivers we can reconsider
the entire unregister/shutdown gordian knot here. Atm I just have no idea
how to do this properly :-/

Thoughts, other ideas?

Cheers, Daniel

> Noralf.
> 
> >> + * in order to disable the hardware on regular driver module unload.
> >>   */
> >>  void drm_dev_unregister(struct drm_device *dev)
> >>  {
> >> @@ -845,6 +839,15 @@ void drm_dev_unregister(struct drm_device *dev)
> >>  	if (drm_core_check_feature(dev, DRIVER_LEGACY))
> >>  		drm_lastclose(dev);
> >>  
> >> +	/*
> >> +	 * After synchronizing any critical read section is guaranteed to see
> >> +	 * the new value of ->unplugged, and any critical section which might
> >> +	 * still have seen the old value of ->unplugged is guaranteed to have
> >> +	 * finished.
> >> +	 */
> >> +	dev->unplugged = true;
> >> +	synchronize_srcu(&drm_unplug_srcu);
> >> +
> >>  	dev->registered = false;
> >>  
> >>  	drm_client_dev_unregister(dev);
> >> diff --git a/include/drm/drm_drv.h b/include/drm/drm_drv.h
> >> index ca46a45a9cce..c50696c82a42 100644
> >> --- a/include/drm/drm_drv.h
> >> +++ b/include/drm/drm_drv.h
> >> @@ -736,13 +736,11 @@ void drm_dev_unplug(struct drm_device *dev);
> >>   * drm_dev_is_unplugged - is a DRM device unplugged
> >>   * @dev: DRM device
> >>   *
> >> - * This function can be called to check whether a hotpluggable is unplugged.
> >> - * Unplugging itself is singalled through drm_dev_unplug(). If a device is
> >> - * unplugged, these two functions guarantee that any store before calling
> >> - * drm_dev_unplug() is visible to callers of this function after it completes
> >> + * This function can be called to check whether @dev is unregistered. This can
> >> + * be used to detect that the underlying parent device is gone.
> > 
> > I think it'd be good to keep the first part, and just update the reference
> > to drm_dev_unregister. So:
> > 
> >  * This function can be called to check whether a hotpluggable is unplugged.
> >  * Unplugging itself is singalled through drm_dev_unregister(). If a device is
> >  * unplugged, these two functions guarantee that any store before calling
> >  * drm_dev_unregister() is visible to callers of this function after it
> >  * completes.
> > 
> > I think your version shrugs a few important details under the rug. With
> > those nits addressed:
> > 
> > Reviewed-by: Daniel Vetter <daniel.vetter at ffwll.ch>
> > 
> > Cheers, Daniel
> > 
> >>   *
> >> - * WARNING: This function fundamentally races against drm_dev_unplug(). It is
> >> - * recommended that drivers instead use the underlying drm_dev_enter() and
> >> + * WARNING: This function fundamentally races against drm_dev_unregister(). It
> >> + * is recommended that drivers instead use the underlying drm_dev_enter() and
> >>   * drm_dev_exit() function pairs.
> >>   */
> >>  static inline bool drm_dev_is_unplugged(struct drm_device *dev)
> >> -- 
> >> 2.20.1
> >>
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx at lists.freedesktop.org
> >> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> > 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch