From mboxrd@z Thu Jan 1 00:00:00 1970 From: Maarten Lankhorst Subject: Re: [RFCv3 02/14] drm: convert crtc to ww_mutex Date: Thu, 21 Nov 2013 15:11:46 +0100 Message-ID: <528E14A2.6010206@canonical.com> References: <1384980493-25499-1-git-send-email-robdclark@gmail.com> <1384980493-25499-3-git-send-email-robdclark@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from youngberry.canonical.com (youngberry.canonical.com [91.189.89.112]) by gabe.freedesktop.org (Postfix) with ESMTP id 5C342FD3FB for ; Thu, 21 Nov 2013 06:11:48 -0800 (PST) In-Reply-To: <1384980493-25499-3-git-send-email-robdclark@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces@lists.freedesktop.org Errors-To: dri-devel-bounces@lists.freedesktop.org To: Rob Clark Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org 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