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

[Rob Clark]

On Thu, Nov 21, 2013 at 9:11 AM, Maarten Lankhorst
<maarten.lankhorst at canonical.com> wrote:
> 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.

well, everywhere (but mutex_lock_nest_lock()) is using the ww_mutex
fxns, so once the mutex_lock_nest_lock() thing is sorted, that
shouldn't be a problem.  The reason I was thinking about either
squashing this, or re-juggling a bit is because it doesn't make much
sense to change everything to ww_mutex and then a couple patches later
to drm_modeset_{lock,unlock}_crtc().

BR,
-R

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