Re: [PATCH 2/6] drm/drv: Prepare to remove drm_dev_unplug()

[Sean Paul <sean-p7yTbzM4H96eqtR555YLDQ@public.gmane.org>]

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@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.

Makes sense. I think having unregistered along with registered might cause some
confusion unless it's really clearly documented. Perhaps
s/unplugged/unregistered/ and s/registered/initialized/ but init has its own
meaning...

I'd hate to hold up the patchset over naming bikeshed, so maybe this would be
better off as a follow-on series where everyone can pile on opinions. So,

Reviewed-by: Sean Paul <sean@poorly.run>


> 
> 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
> + * 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.
>   *
> - * 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
> 

-- 
Sean Paul, Software Engineer, Google / Chromium OS
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx