On Thu, Nov 21, 2013 at 9:11 AM, Maarten Lankhorst
<maarten.lankhorst@xxxxxxxxxxxxx> 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
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/dri-devel
