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 <nor...@tronnes.org>
> ---
>
> Maybe s/unplugged/unregistered/ ?
>
> I looked at drm_device->registered, but using that would mean that
> drm_dev_is_unplugged() would return before drm_device is registered.
> And given that its current purpose is to prevent race against connector
> registration, I stayed away from it.
Yeah I think we need to keep the registered state separate from unplugged.
Iirc this exact scenario is what we discussed when you revamped the
unplug infrastructure.
>
> Noralf.
>
>
> 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).
> + * 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.vet...@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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
