On Tue, Jul 29, 2014 at 11:32:20PM +0200, Daniel Vetter wrote: > In the fbdev code we want to do trylocks only to avoid deadlocks and > other ugly issues. Thus far we've only grabbed the overall modeset > lock, but that already failed to exclude a pile of potential > concurrent operations. With proper atomic support this will be worse. > > So add a trylock mode to the modeset locking code which attempts all > locks only with trylocks, if possible. We need to track this in the > locking functions themselves and can't restrict this to drivers since > driver-private w/w mutexes must be treated the same way. > > There's still the issue that other driver private locks aren't handled > here at all, but well can't have everything. With this we will at > least not regress, even once atomic allows lots of concurrent kms > activity. > > Aside: We should move the acquire context to stack-based allocation in > the callers to get rid of that awful WARN_ON(kmalloc_failed) control > flow which just blows up when memory is short. But that's material for > separate patches. > > v2: > - Fix logic inversion fumble in the fb helper. > - Add proper kerneldoc. > > Signed-off-by: Daniel Vetter <daniel.vetter at ffwll.ch> > --- > drivers/gpu/drm/drm_fb_helper.c | 10 +++---- > drivers/gpu/drm/drm_modeset_lock.c | 56 > ++++++++++++++++++++++++++++++-------- > include/drm/drm_modeset_lock.h | 6 ++++ > 3 files changed, 55 insertions(+), 17 deletions(-) > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > index 3144db9dc0f1..841e039ba028 100644 > --- a/drivers/gpu/drm/drm_fb_helper.c > +++ b/drivers/gpu/drm/drm_fb_helper.c > @@ -419,11 +419,11 @@ static bool drm_fb_helper_force_kernel_mode(void) > if (dev->switch_power_state == DRM_SWITCH_POWER_OFF) > continue; > > - /* NOTE: we use lockless flag below to avoid grabbing other > - * modeset locks. So just trylock the underlying mutex > - * directly: > + /* > + * NOTE: Use trylock mode to avoid deadlocks and sleeping in > + * panic context. > */ > - if (!mutex_trylock(&dev->mode_config.mutex)) { > + if (__drm_modeset_lock_all(dev, true) != 0) { > error = true; > continue; > }
Can we succeed locking config->mutex and connection_mutex inside __drm_modeset_lock_all(), but then fail to lock one of the CRTC mutexes in drm_modeset_lock_all_crtcs()? If so, __drm_modeset_lock_all() will return -EBUSY, but not drop the locks it acquired, and then it seems like we can return from the function here without ever dropping locks. Matt > @@ -432,7 +432,7 @@ static bool drm_fb_helper_force_kernel_mode(void) > if (ret) > error = true; > > - mutex_unlock(&dev->mode_config.mutex); > + drm_modeset_unlock_all(dev); > } > return error; > } > diff --git a/drivers/gpu/drm/drm_modeset_lock.c > b/drivers/gpu/drm/drm_modeset_lock.c > index 4d2aa549c614..acfe187609b0 100644 > --- a/drivers/gpu/drm/drm_modeset_lock.c > +++ b/drivers/gpu/drm/drm_modeset_lock.c > @@ -57,26 +57,37 @@ > > > /** > - * drm_modeset_lock_all - take all modeset locks > - * @dev: drm device > + * __drm_modeset_lock_all - internal helper to grab all modeset locks > + * @dev: DRM device > + * @trylock: trylock mode for atomic contexts > * > - * This function takes all modeset locks, suitable where a more fine-grained > - * scheme isn't (yet) implemented. Locks must be dropped with > - * drm_modeset_unlock_all. > + * This is a special version of drm_modeset_lock_all() which can also be > used in > + * atomic contexts. Then @trylock must be set to true. > + * > + * Returns: > + * 0 on success or negative error code on failure. > */ > -void drm_modeset_lock_all(struct drm_device *dev) > +int __drm_modeset_lock_all(struct drm_device *dev, > + bool trylock) > { > struct drm_mode_config *config = &dev->mode_config; > struct drm_modeset_acquire_ctx *ctx; > int ret; > > - ctx = kzalloc(sizeof(*ctx), GFP_KERNEL); > - if (WARN_ON(!ctx)) > - return; > + ctx = kzalloc(sizeof(*ctx), > + trylock ? GFP_ATOMIC : GFP_KERNEL); > + if (!ctx) > + return -ENOMEM; > > - mutex_lock(&config->mutex); > + if (trylock) { > + if (!mutex_trylock(&config->mutex)) > + return -EBUSY; > + } else { > + mutex_lock(&config->mutex); > + } > > drm_modeset_acquire_init(ctx, 0); > + ctx->trylock_only = trylock; > > retry: > ret = drm_modeset_lock(&config->connection_mutex, ctx); > @@ -95,13 +106,29 @@ retry: > > drm_warn_on_modeset_not_all_locked(dev); > > - return; > + return 0; > > fail: > if (ret == -EDEADLK) { > drm_modeset_backoff(ctx); > goto retry; > } > + > + return ret; > +} > +EXPORT_SYMBOL(__drm_modeset_lock_all); > + > +/** > + * drm_modeset_lock_all - take all modeset locks > + * @dev: drm device > + * > + * This function takes all modeset locks, suitable where a more fine-grained > + * scheme isn't (yet) implemented. Locks must be dropped with > + * drm_modeset_unlock_all. > + */ > +void drm_modeset_lock_all(struct drm_device *dev) > +{ > + WARN_ON(__drm_modeset_lock_all(dev, false) != 0); > } > EXPORT_SYMBOL(drm_modeset_lock_all); > > @@ -287,7 +314,12 @@ static inline int modeset_lock(struct drm_modeset_lock > *lock, > > WARN_ON(ctx->contended); > > - if (interruptible && slow) { > + if (ctx->trylock_only) { > + if (!ww_mutex_trylock(&lock->mutex)) > + return -EBUSY; > + else > + return 0; > + } else if (interruptible && slow) { > ret = ww_mutex_lock_slow_interruptible(&lock->mutex, > &ctx->ww_ctx); > } else if (interruptible) { > ret = ww_mutex_lock_interruptible(&lock->mutex, &ctx->ww_ctx); > diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h > index d38e1508f11a..a3f736d24382 100644 > --- a/include/drm/drm_modeset_lock.h > +++ b/include/drm/drm_modeset_lock.h > @@ -53,6 +53,11 @@ struct drm_modeset_acquire_ctx { > * list of held locks (drm_modeset_lock) > */ > struct list_head locked; > + > + /** > + * Trylock mode, use only for panic handlers! > + */ > + bool trylock_only; > }; > > /** > @@ -123,6 +128,7 @@ struct drm_device; > struct drm_crtc; > > void drm_modeset_lock_all(struct drm_device *dev); > +int __drm_modeset_lock_all(struct drm_device *dev, bool trylock); > void drm_modeset_unlock_all(struct drm_device *dev); > void drm_modeset_lock_crtc(struct drm_crtc *crtc); > void drm_modeset_unlock_crtc(struct drm_crtc *crtc); > -- > 2.0.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx at lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795