Re: [RFCv3 02/14] drm: convert crtc to ww_mutex

[Maarten Lankhorst <maarten.lankhorst@canonical.com>]

op 20-11-13 21:48, Rob Clark schreef:
> At the moment, this doesn't do anything.  But for atomic we will have an
> ww_acquire_ctx associated with the state, to simplify the locking and
> avoid potential deadlock when we cannot control the locking order.
Nack. :-)

Please don't split this out. ww_mutex may be backed by a mutex, but that's an implementation detail you must not rely on.
> ---
>  drivers/gpu/drm/drm_crtc.c           | 20 +++++++++++---------
>  drivers/gpu/drm/i915/intel_display.c | 16 ++++++++--------
>  drivers/gpu/drm/omapdrm/omap_crtc.c  | 10 +++++-----
>  drivers/gpu/drm/vmwgfx/vmwgfx_kms.c  | 12 ++++++++----
>  include/drm/drm_crtc.h               |  3 ++-
>  5 files changed, 34 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 81ac351..55f37db 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -52,7 +52,7 @@ void drm_modeset_lock_all(struct drm_device *dev)
>  	mutex_lock(&dev->mode_config.mutex);
>  
>  	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
> -		mutex_lock_nest_lock(&crtc->mutex, &dev->mode_config.mutex);
> +		mutex_lock_nest_lock(&crtc->mutex.base, &dev->mode_config.mutex);
>  }
This breaks ww_mutex semantics, for example. What if someone holding a ww_ctx acquires has one mutex, and tries to acquire a second crtc mutex?
If lockdep was smart it would notice that this lock is nested in different locks, but I don't think lockdep is that smart.
>  EXPORT_SYMBOL(drm_modeset_lock_all);
>  
> @@ -65,7 +65,7 @@ void drm_modeset_unlock_all(struct drm_device *dev)
>  	struct drm_crtc *crtc;
>  
>  	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
> -		mutex_unlock(&crtc->mutex);
> +		ww_mutex_unlock(&crtc->mutex);
>  
>  	mutex_unlock(&dev->mode_config.mutex);
>  }
> @@ -84,7 +84,7 @@ void drm_warn_on_modeset_not_all_locked(struct drm_device *dev)
>  		return;
>  
>  	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head)
> -		WARN_ON(!mutex_is_locked(&crtc->mutex));
> +		WARN_ON(!ww_mutex_is_locked(&crtc->mutex));
>  
>  	WARN_ON(!mutex_is_locked(&dev->mode_config.mutex));
>  }
> @@ -613,6 +613,8 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb)
>  }
>  EXPORT_SYMBOL(drm_framebuffer_remove);
>  
> +static DEFINE_WW_CLASS(crtc_ww_class);
> +
>  /**
>   * drm_crtc_init - Initialise a new CRTC object
>   * @dev: DRM device
> @@ -634,8 +636,8 @@ int drm_crtc_init(struct drm_device *dev, struct drm_crtc *crtc,
>  	crtc->invert_dimensions = false;
>  
>  	drm_modeset_lock_all(dev);
> -	mutex_init(&crtc->mutex);
> -	mutex_lock_nest_lock(&crtc->mutex, &dev->mode_config.mutex);
> +	ww_mutex_init(&crtc->mutex, &crtc_ww_class);
> +	mutex_lock_nest_lock(&crtc->mutex.base, &dev->mode_config.mutex);
In a later patch you keep this snippet, please make this a trylock instead. It removes
the assumption that ww_mutex has mutex as base.

~Maarten