On Thu, May 29, 2014 at 7:22 AM, Daniel Vetter <daniel@xxxxxxxx> wrote: > On Wed, May 28, 2014 at 07:57:25PM -0400, Rob Clark wrote: >> For atomic, it will be quite necessary to not need to care so much >> about locking order. And 'state' gives us a convenient place to stash a >> ww_ctx for any sort of update that needs to grab multiple crtc locks. >> >> Because we will want to eventually make locking even more fine grained >> (giving locks to planes, connectors, etc), split out drm_modeset_lock >> and drm_modeset_acquire_ctx to track acquired locks. >> >> Atomic will use this to keep track of which locks have been acquired >> in a transaction. >> >> Signed-off-by: Rob Clark <robdclark@xxxxxxxxx> > > I still would like to see all the additional magic for atomic updates > removed from the drm_mode_lock functions. Getting the w/w dance right > isn't that simple and all these complications confuse. there are something like 10 lines of additional magic in this patch which is not used yet. I managed to move most of it to the now-later atomic patch (which was previously earlier in the series). But running out of time/energy to care about juggling things around. So, meh. BR, -R > Also I simply can't review whether these added features make sense without > their users ;-) > -Daniel > >> --- >> drivers/gpu/drm/Makefile | 2 +- >> drivers/gpu/drm/drm_crtc.c | 87 +++++++++++----- >> drivers/gpu/drm/drm_crtc_helper.c | 3 +- >> drivers/gpu/drm/drm_fb_helper.c | 4 + >> drivers/gpu/drm/drm_modeset_lock.c | 187 +++++++++++++++++++++++++++++++++++ >> drivers/gpu/drm/drm_plane_helper.c | 2 +- >> drivers/gpu/drm/i915/intel_crt.c | 5 +- >> drivers/gpu/drm/i915/intel_display.c | 58 +++++++---- >> drivers/gpu/drm/i915/intel_drv.h | 6 +- >> drivers/gpu/drm/i915/intel_sprite.c | 2 +- >> drivers/gpu/drm/i915/intel_tv.c | 5 +- >> drivers/gpu/drm/omapdrm/omap_crtc.c | 10 +- >> drivers/gpu/drm/vmwgfx/vmwgfx_kms.c | 8 +- >> include/drm/drmP.h | 5 - >> include/drm/drm_crtc.h | 15 +-- >> include/drm/drm_modeset_lock.h | 139 ++++++++++++++++++++++++++ >> include/uapi/drm/drm_mode.h | 3 + >> 17 files changed, 468 insertions(+), 73 deletions(-) >> create mode 100644 drivers/gpu/drm/drm_modeset_lock.c >> create mode 100644 include/drm/drm_modeset_lock.h >> >> diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile >> index 48e38ba..bf4c12d 100644 >> --- a/drivers/gpu/drm/Makefile >> +++ b/drivers/gpu/drm/Makefile >> @@ -14,7 +14,7 @@ drm-y := drm_auth.o drm_buffer.o drm_bufs.o drm_cache.o \ >> drm_info.o drm_debugfs.o drm_encoder_slave.o \ >> drm_trace_points.o drm_global.o drm_prime.o \ >> drm_rect.o drm_vma_manager.o drm_flip_work.o \ >> - drm_plane_helper.o >> + drm_plane_helper.o drm_modeset_lock.o >> >> drm-$(CONFIG_COMPAT) += drm_ioc32.o >> drm-$(CONFIG_DRM_GEM_CMA_HELPER) += drm_gem_cma_helper.o >> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c >> index de1520c..cf622f6 100644 >> --- a/drivers/gpu/drm/drm_crtc.c >> +++ b/drivers/gpu/drm/drm_crtc.c >> @@ -37,8 +37,8 @@ >> #include <drm/drm_crtc.h> >> #include <drm/drm_edid.h> >> #include <drm/drm_fourcc.h> >> +#include <drm/drm_modeset_lock.h> >> >> -#include "drm_crtc_internal.h" >> >> /** >> * drm_modeset_lock_all - take all modeset locks >> @@ -50,14 +50,42 @@ >> */ >> void drm_modeset_lock_all(struct drm_device *dev) >> { >> - struct drm_crtc *crtc; >> + struct drm_mode_config *config = &dev->mode_config; >> + struct drm_modeset_acquire_ctx *ctx; >> + int ret; >> >> - mutex_lock(&dev->mode_config.mutex); >> + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL); >> + if (WARN_ON(!ctx)) >> + return; >> >> - mutex_lock(&dev->mode_config.connection_mutex); >> + mutex_lock(&config->mutex); >> >> - list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) >> - mutex_lock_nest_lock(&crtc->mutex, &dev->mode_config.mutex); >> + drm_modeset_acquire_init(ctx, false, false); >> + >> +retry: >> + ret = drm_modeset_lock(&config->connection_mutex, ctx); >> + if (ret) >> + goto fail; >> + ret = drm_modeset_lock_all_crtcs(dev, ctx); >> + if (ret) >> + goto fail; >> + >> + WARN_ON(config->acquire_ctx); >> + >> + /* now we hold the locks, so now that it is safe, stash the >> + * ctx for drm_modeset_unlock_all(): >> + */ >> + config->acquire_ctx = ctx; >> + >> + drm_warn_on_modeset_not_all_locked(dev); >> + >> + return; >> + >> +fail: >> + if (ret == -EDEADLK) { >> + drm_modeset_backoff(ctx); >> + goto retry; >> + } >> } >> EXPORT_SYMBOL(drm_modeset_lock_all); >> >> @@ -69,12 +97,18 @@ EXPORT_SYMBOL(drm_modeset_lock_all); >> */ >> void drm_modeset_unlock_all(struct drm_device *dev) >> { >> - struct drm_crtc *crtc; >> + struct drm_mode_config *config = &dev->mode_config; >> + struct drm_modeset_acquire_ctx *ctx = config->acquire_ctx; >> >> - list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) >> - mutex_unlock(&crtc->mutex); >> + if (WARN_ON(!ctx)) >> + return; >> + >> + config->acquire_ctx = NULL; >> + drm_modeset_drop_locks(ctx); >> + ww_acquire_fini(&ctx->ww_ctx); >> + drm_modeset_acquire_fini(ctx); >> >> - mutex_unlock(&dev->mode_config.connection_mutex); >> + kfree(ctx); >> >> mutex_unlock(&dev->mode_config.mutex); >> } >> @@ -95,9 +129,9 @@ 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(!drm_modeset_is_locked(&crtc->mutex)); >> >> - WARN_ON(!mutex_is_locked(&dev->mode_config.connection_mutex)); >> + WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); >> WARN_ON(!mutex_is_locked(&dev->mode_config.mutex)); >> } >> EXPORT_SYMBOL(drm_warn_on_modeset_not_all_locked); >> @@ -677,6 +711,8 @@ void drm_framebuffer_remove(struct drm_framebuffer *fb) >> } >> EXPORT_SYMBOL(drm_framebuffer_remove); >> >> +DEFINE_WW_CLASS(crtc_ww_class); >> + >> /** >> * drm_crtc_init_with_planes - Initialise a new CRTC object with >> * specified primary and cursor planes. >> @@ -696,6 +732,7 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, >> void *cursor, >> const struct drm_crtc_funcs *funcs) >> { >> + struct drm_mode_config *config = &dev->mode_config; >> int ret; >> >> crtc->dev = dev; >> @@ -703,8 +740,9 @@ int drm_crtc_init_with_planes(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); >> + drm_modeset_lock_init(&crtc->mutex); >> + /* dropped by _unlock_all(): */ >> + drm_modeset_lock(&crtc->mutex, config->acquire_ctx); >> >> ret = drm_mode_object_get(dev, &crtc->base, DRM_MODE_OBJECT_CRTC); >> if (ret) >> @@ -712,8 +750,8 @@ int drm_crtc_init_with_planes(struct drm_device *dev, struct drm_crtc *crtc, >> >> crtc->base.properties = &crtc->properties; >> >> - list_add_tail(&crtc->head, &dev->mode_config.crtc_list); >> - dev->mode_config.num_crtc++; >> + list_add_tail(&crtc->head, &config->crtc_list); >> + config->num_crtc++; >> >> crtc->primary = primary; >> if (primary) >> @@ -741,6 +779,8 @@ void drm_crtc_cleanup(struct drm_crtc *crtc) >> kfree(crtc->gamma_store); >> crtc->gamma_store = NULL; >> >> + drm_modeset_lock_fini(&crtc->mutex); >> + >> drm_mode_object_put(dev, &crtc->base); >> list_del(&crtc->head); >> dev->mode_config.num_crtc--; >> @@ -1804,7 +1844,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, >> DRM_DEBUG_KMS("[CONNECTOR:%d:?]\n", out_resp->connector_id); >> >> mutex_lock(&dev->mode_config.mutex); >> - mutex_lock(&dev->mode_config.connection_mutex); >> + drm_modeset_lock(&dev->mode_config.connection_mutex, NULL); >> >> connector = drm_connector_find(dev, out_resp->connector_id); >> if (!connector) { >> @@ -1903,7 +1943,7 @@ int drm_mode_getconnector(struct drm_device *dev, void *data, >> out_resp->count_encoders = encoders_count; >> >> out: >> - mutex_unlock(&dev->mode_config.connection_mutex); >> + drm_modeset_unlock(&dev->mode_config.connection_mutex); >> mutex_unlock(&dev->mode_config.mutex); >> >> return ret; >> @@ -2487,7 +2527,7 @@ static int drm_mode_cursor_common(struct drm_device *dev, >> return -ENOENT; >> } >> >> - mutex_lock(&crtc->mutex); >> + drm_modeset_lock(&crtc->mutex, NULL); >> if (req->flags & DRM_MODE_CURSOR_BO) { >> if (!crtc->funcs->cursor_set && !crtc->funcs->cursor_set2) { >> ret = -ENXIO; >> @@ -2511,7 +2551,7 @@ static int drm_mode_cursor_common(struct drm_device *dev, >> } >> } >> out: >> - mutex_unlock(&crtc->mutex); >> + drm_modeset_unlock(&crtc->mutex); >> >> return ret; >> >> @@ -4195,7 +4235,7 @@ int drm_mode_page_flip_ioctl(struct drm_device *dev, >> if (!crtc) >> return -ENOENT; >> >> - mutex_lock(&crtc->mutex); >> + drm_modeset_lock(&crtc->mutex, NULL); >> if (crtc->primary->fb == NULL) { >> /* The framebuffer is currently unbound, presumably >> * due to a hotplug event, that userspace has not >> @@ -4279,7 +4319,7 @@ out: >> drm_framebuffer_unreference(fb); >> if (old_fb) >> drm_framebuffer_unreference(old_fb); >> - mutex_unlock(&crtc->mutex); >> + drm_modeset_unlock(&crtc->mutex); >> >> return ret; >> } >> @@ -4644,7 +4684,7 @@ EXPORT_SYMBOL(drm_format_vert_chroma_subsampling); >> void drm_mode_config_init(struct drm_device *dev) >> { >> mutex_init(&dev->mode_config.mutex); >> - mutex_init(&dev->mode_config.connection_mutex); >> + drm_modeset_lock_init(&dev->mode_config.connection_mutex); >> mutex_init(&dev->mode_config.idr_mutex); >> mutex_init(&dev->mode_config.fb_lock); >> INIT_LIST_HEAD(&dev->mode_config.fb_list); >> @@ -4744,5 +4784,6 @@ void drm_mode_config_cleanup(struct drm_device *dev) >> } >> >> idr_destroy(&dev->mode_config.crtc_idr); >> + drm_modeset_lock_fini(&dev->mode_config.connection_mutex); >> } >> EXPORT_SYMBOL(drm_mode_config_cleanup); >> diff --git a/drivers/gpu/drm/drm_crtc_helper.c b/drivers/gpu/drm/drm_crtc_helper.c >> index 5b93caf..4eaded8 100644 >> --- a/drivers/gpu/drm/drm_crtc_helper.c >> +++ b/drivers/gpu/drm/drm_crtc_helper.c >> @@ -89,8 +89,7 @@ bool drm_helper_encoder_in_use(struct drm_encoder *encoder) >> struct drm_device *dev = encoder->dev; >> >> WARN_ON(!mutex_is_locked(&dev->mode_config.mutex)); >> - WARN_ON(!mutex_is_locked(&dev->mode_config.connection_mutex)); >> - >> + WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); >> list_for_each_entry(connector, &dev->mode_config.connector_list, head) >> if (connector->encoder == encoder) >> return true; >> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c >> index 1e28ba6..25f687d 100644 >> --- a/drivers/gpu/drm/drm_fb_helper.c >> +++ b/drivers/gpu/drm/drm_fb_helper.c >> @@ -355,6 +355,10 @@ 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: >> + */ >> if (!mutex_trylock(&dev->mode_config.mutex)) { >> error = true; >> continue; >> diff --git a/drivers/gpu/drm/drm_modeset_lock.c b/drivers/gpu/drm/drm_modeset_lock.c >> new file mode 100644 >> index 0000000..6580d6c >> --- /dev/null >> +++ b/drivers/gpu/drm/drm_modeset_lock.c >> @@ -0,0 +1,187 @@ >> +/* >> + * Copyright (C) 2014 Red Hat >> + * Author: Rob Clark <robdclark@xxxxxxxxx> >> + * >> + * Permission is hereby granted, free of charge, to any person obtaining a >> + * copy of this software and associated documentation files (the "Software"), >> + * to deal in the Software without restriction, including without limitation >> + * the rights to use, copy, modify, merge, publish, distribute, sublicense, >> + * and/or sell copies of the Software, and to permit persons to whom the >> + * Software is furnished to do so, subject to the following conditions: >> + * >> + * The above copyright notice and this permission notice shall be included in >> + * all copies or substantial portions of the Software. >> + * >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL >> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR >> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, >> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR >> + * OTHER DEALINGS IN THE SOFTWARE. >> + */ >> + >> +#include <drm/drmP.h> >> +#include <drm/drm_crtc.h> >> +#include <drm/drm_modeset_lock.h> >> + >> + >> +void drm_modeset_acquire_init(struct drm_modeset_acquire_ctx *ctx, >> + bool nolock, bool nonblock) >> +{ >> + ww_acquire_init(&ctx->ww_ctx, &crtc_ww_class); >> + INIT_LIST_HEAD(&ctx->locked); >> + mutex_init(&ctx->mutex); >> + ctx->nolock = nolock; >> + ctx->nonblock = nonblock; >> +} >> +EXPORT_SYMBOL(drm_modeset_acquire_init); >> + >> +void drm_modeset_acquire_fini(struct drm_modeset_acquire_ctx *ctx) >> +{ >> + WARN_ON(ctx->contended); >> + /* >> + * NOTE: it is intentional that ww_acquire_fini() is not called >> + * here.. due to the way lock handover works in drm_atomic >> + */ >> + mutex_destroy(&ctx->mutex); >> +} >> +EXPORT_SYMBOL(drm_modeset_acquire_fini); >> + >> +void drm_modeset_drop_locks(struct drm_modeset_acquire_ctx *ctx) >> +{ >> + WARN_ON(ctx->contended); >> + mutex_lock(&ctx->mutex); >> + while (!list_empty(&ctx->locked)) { >> + struct drm_modeset_lock *lock; >> + >> + lock = list_first_entry(&ctx->locked, >> + struct drm_modeset_lock, head); >> + >> + drm_modeset_unlock(lock); >> + } >> + mutex_unlock(&ctx->mutex); >> +} >> +EXPORT_SYMBOL(drm_modeset_drop_locks); >> + >> +static int modeset_lock(struct drm_modeset_lock *lock, >> + struct drm_modeset_acquire_ctx *ctx, >> + bool interruptible, bool slow) >> +{ >> + int ret; >> + >> + if (ctx->nolock) >> + return 0; >> + >> + WARN_ON(ctx->frozen); /* all locks should be held by now! */ >> + WARN_ON(ctx->contended); >> + >> +retry: >> + if (interruptible) { >> + ret = ww_mutex_lock_interruptible(&lock->mutex, &ctx->ww_ctx); >> + } else if (slow) { >> + ww_mutex_lock_slow(&lock->mutex, &ctx->ww_ctx); >> + ret = 0; >> + } else { >> + ret = ww_mutex_lock(&lock->mutex, &ctx->ww_ctx); >> + } >> + if (!ret) { >> + if (lock->atomic_pending) { >> + /* some other pending update with dropped locks */ >> + ww_mutex_unlock(&lock->mutex); >> + if (ctx->nonblock) >> + return -EBUSY; >> + wait_event(lock->event, !lock->atomic_pending); >> + goto retry; >> + } >> + lock->atomic_pending = true; >> + WARN_ON(!list_empty(&lock->head)); >> + list_add(&lock->head, &ctx->locked); >> + } else if (ret == -EALREADY) { >> + /* we already hold the lock.. this is fine */ >> + ret = 0; >> + } else if (ret == -EDEADLK) { >> + ctx->contended = lock; >> + } >> + >> + return ret; >> +} >> + >> +void drm_modeset_backoff(struct drm_modeset_acquire_ctx *ctx) >> +{ >> + struct drm_modeset_lock *contended = ctx->contended; >> + >> + ctx->contended = NULL; >> + >> + if (WARN_ON(!contended)) >> + return; >> + >> + drm_modeset_drop_locks(ctx); >> + >> + modeset_lock(contended, ctx, false, true); >> +} >> +EXPORT_SYMBOL(drm_modeset_backoff); >> + >> +/** >> + * drm_modeset_lock - take modeset lock >> + * @lock: lock to take >> + * @ctx: acquire ctx >> + * >> + * If ctx is not NULL, then then it's acquire context is used >> + * and the lock does not need to be explicitly unlocked, it >> + * will be automatically unlocked when the atomic update is >> + * complete >> + */ >> +int drm_modeset_lock(struct drm_modeset_lock *lock, >> + struct drm_modeset_acquire_ctx *ctx) >> +{ >> + if (ctx) >> + return modeset_lock(lock, ctx, false, false); >> + >> + ww_mutex_lock(&lock->mutex, NULL); >> + return 0; >> +} >> +EXPORT_SYMBOL(drm_modeset_lock); >> + >> +int drm_modeset_lock_interruptible(struct drm_modeset_lock *lock, >> + struct drm_modeset_acquire_ctx *ctx) >> +{ >> + if (ctx) >> + return modeset_lock(lock, ctx, true, false); >> + >> + return ww_mutex_lock_interruptible(&lock->mutex, NULL); >> +} >> +EXPORT_SYMBOL(drm_modeset_lock_interruptible); >> + >> +/** >> + * drm_modeset_unlock - drop modeset lock >> + * @lock: lock to release >> + */ >> +void drm_modeset_unlock(struct drm_modeset_lock *lock) >> +{ >> + list_del_init(&lock->head); >> + lock->atomic_pending = false; >> + ww_mutex_unlock(&lock->mutex); >> + wake_up_all(&lock->event); >> +} >> +EXPORT_SYMBOL(drm_modeset_unlock); >> + >> +/** >> + * drm_modeset_lock_all_crtcs - helper to drm_modeset_lock() all CRTCs >> + */ >> +int drm_modeset_lock_all_crtcs(struct drm_device *dev, >> + struct drm_modeset_acquire_ctx *ctx) >> +{ >> + struct drm_mode_config *config = &dev->mode_config; >> + struct drm_crtc *crtc; >> + int ret = 0; >> + >> + list_for_each_entry(crtc, &config->crtc_list, head) { >> + ret = drm_modeset_lock(&crtc->mutex, ctx); >> + if (ret) >> + return ret; >> + } >> + >> + return 0; >> +} >> +EXPORT_SYMBOL(drm_modeset_lock_all_crtcs); >> diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c >> index 458d9bf..6301ec6 100644 >> --- a/drivers/gpu/drm/drm_plane_helper.c >> +++ b/drivers/gpu/drm/drm_plane_helper.c >> @@ -59,7 +59,7 @@ static int get_connectors_for_crtc(struct drm_crtc *crtc, >> * need to grab the connection_mutex here to be able to make these >> * checks. >> */ >> - WARN_ON(!mutex_is_locked(&dev->mode_config.connection_mutex)); >> + WARN_ON(!drm_modeset_is_locked(&dev->mode_config.connection_mutex)); >> >> list_for_each_entry(connector, &dev->mode_config.connector_list, head) >> if (connector->encoder && connector->encoder->crtc == crtc) { >> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c >> index 22d8347..33328fc 100644 >> --- a/drivers/gpu/drm/i915/intel_crt.c >> +++ b/drivers/gpu/drm/i915/intel_crt.c >> @@ -630,6 +630,7 @@ intel_crt_detect(struct drm_connector *connector, bool force) >> enum intel_display_power_domain power_domain; >> enum drm_connector_status status; >> struct intel_load_detect_pipe tmp; >> + struct drm_modeset_acquire_ctx ctx; >> >> intel_runtime_pm_get(dev_priv); >> >> @@ -673,12 +674,12 @@ intel_crt_detect(struct drm_connector *connector, bool force) >> } >> >> /* for pre-945g platforms use load detect */ >> - if (intel_get_load_detect_pipe(connector, NULL, &tmp)) { >> + if (intel_get_load_detect_pipe(connector, NULL, &tmp, &ctx)) { >> if (intel_crt_detect_ddc(connector)) >> status = connector_status_connected; >> else >> status = intel_crt_load_detect(crt); >> - intel_release_load_detect_pipe(connector, &tmp); >> + intel_release_load_detect_pipe(connector, &tmp, &ctx); >> } else >> status = connector_status_unknown; >> >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index 5844f07..ba6cbbb 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -2363,7 +2363,7 @@ void intel_display_handle_reset(struct drm_device *dev) >> list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) { >> struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> >> - mutex_lock(&crtc->mutex); >> + drm_modeset_lock(&crtc->mutex, NULL); >> /* >> * FIXME: Once we have proper support for primary planes (and >> * disabling them without disabling the entire crtc) allow again >> @@ -2374,7 +2374,7 @@ void intel_display_handle_reset(struct drm_device *dev) >> crtc->primary->fb, >> crtc->x, >> crtc->y); >> - mutex_unlock(&crtc->mutex); >> + drm_modeset_unlock(&crtc->mutex); >> } >> } >> >> @@ -7972,7 +7972,8 @@ mode_fits_in_fbdev(struct drm_device *dev, >> >> bool intel_get_load_detect_pipe(struct drm_connector *connector, >> struct drm_display_mode *mode, >> - struct intel_load_detect_pipe *old) >> + struct intel_load_detect_pipe *old, >> + struct drm_modeset_acquire_ctx *ctx) >> { >> struct intel_crtc *intel_crtc; >> struct intel_encoder *intel_encoder = >> @@ -7982,13 +7983,19 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, >> struct drm_crtc *crtc = NULL; >> struct drm_device *dev = encoder->dev; >> struct drm_framebuffer *fb; >> - int i = -1; >> + struct drm_mode_config *config = &dev->mode_config; >> + int ret, i = -1; >> >> DRM_DEBUG_KMS("[CONNECTOR:%d:%s], [ENCODER:%d:%s]\n", >> connector->base.id, drm_get_connector_name(connector), >> encoder->base.id, drm_get_encoder_name(encoder)); >> >> - mutex_lock(&dev->mode_config.connection_mutex); >> + drm_modeset_acquire_init(ctx, false, false); >> + >> +retry: >> + ret = drm_modeset_lock(&config->connection_mutex, ctx); >> + if (ret) >> + goto fail_unlock; >> >> /* >> * Algorithm gets a little messy: >> @@ -8004,7 +8011,9 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, >> if (encoder->crtc) { >> crtc = encoder->crtc; >> >> - mutex_lock(&crtc->mutex); >> + ret = drm_modeset_lock(&crtc->mutex, ctx); >> + if (ret) >> + goto fail_unlock; >> >> old->dpms_mode = connector->dpms; >> old->load_detect_temp = false; >> @@ -8017,7 +8026,7 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, >> } >> >> /* Find an unused one (if possible) */ >> - list_for_each_entry(possible_crtc, &dev->mode_config.crtc_list, head) { >> + list_for_each_entry(possible_crtc, &config->crtc_list, head) { >> i++; >> if (!(encoder->possible_crtcs & (1 << i))) >> continue; >> @@ -8032,10 +8041,12 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, >> */ >> if (!crtc) { >> DRM_DEBUG_KMS("no pipe available for load-detect\n"); >> - goto fail_unlock_connector; >> + goto fail_unlock; >> } >> >> - mutex_lock(&crtc->mutex); >> + ret = drm_modeset_lock(&crtc->mutex, ctx); >> + if (ret) >> + goto fail_unlock; >> intel_encoder->new_crtc = to_intel_crtc(crtc); >> to_intel_connector(connector)->new_encoder = intel_encoder; >> >> @@ -8085,15 +8096,22 @@ bool intel_get_load_detect_pipe(struct drm_connector *connector, >> intel_crtc->new_config = &intel_crtc->config; >> else >> intel_crtc->new_config = NULL; >> - mutex_unlock(&crtc->mutex); >> -fail_unlock_connector: >> - mutex_unlock(&dev->mode_config.connection_mutex); >> +fail_unlock: >> + if (ret == -EDEADLK) { >> + drm_modeset_backoff(ctx); >> + goto retry; >> + } >> + >> + drm_modeset_drop_locks(ctx); >> + ww_acquire_fini(&ctx->ww_ctx); >> + drm_modeset_acquire_fini(ctx); >> >> return false; >> } >> >> void intel_release_load_detect_pipe(struct drm_connector *connector, >> - struct intel_load_detect_pipe *old) >> + struct intel_load_detect_pipe *old, >> + struct drm_modeset_acquire_ctx *ctx) >> { >> struct intel_encoder *intel_encoder = >> intel_attached_encoder(connector); >> @@ -8117,8 +8135,7 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, >> drm_framebuffer_unreference(old->release_fb); >> } >> >> - mutex_unlock(&crtc->mutex); >> - mutex_unlock(&connector->dev->mode_config.connection_mutex); >> + goto unlock; >> return; >> } >> >> @@ -8126,8 +8143,10 @@ void intel_release_load_detect_pipe(struct drm_connector *connector, >> if (old->dpms_mode != DRM_MODE_DPMS_ON) >> connector->funcs->dpms(connector, old->dpms_mode); >> >> - mutex_unlock(&crtc->mutex); >> - mutex_unlock(&connector->dev->mode_config.connection_mutex); >> +unlock: >> + drm_modeset_drop_locks(ctx); >> + ww_acquire_fini(&ctx->ww_ctx); >> + drm_modeset_acquire_fini(ctx); >> } >> >> static int i9xx_pll_refclk(struct drm_device *dev, >> @@ -11385,6 +11404,7 @@ static void intel_enable_pipe_a(struct drm_device *dev) >> struct intel_connector *connector; >> struct drm_connector *crt = NULL; >> struct intel_load_detect_pipe load_detect_temp; >> + struct drm_modeset_acquire_ctx ctx; >> >> /* We can't just switch on the pipe A, we need to set things up with a >> * proper mode and output configuration. As a gross hack, enable pipe A >> @@ -11401,8 +11421,8 @@ static void intel_enable_pipe_a(struct drm_device *dev) >> if (!crt) >> return; >> >> - if (intel_get_load_detect_pipe(crt, NULL, &load_detect_temp)) >> - intel_release_load_detect_pipe(crt, &load_detect_temp); >> + if (intel_get_load_detect_pipe(crt, NULL, &load_detect_temp, &ctx)) >> + intel_release_load_detect_pipe(crt, &load_detect_temp, &ctx); >> >> >> } >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index d8b540b..824f8ae 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -721,9 +721,11 @@ void vlv_wait_port_ready(struct drm_i915_private *dev_priv, >> struct intel_digital_port *dport); >> bool intel_get_load_detect_pipe(struct drm_connector *connector, >> struct drm_display_mode *mode, >> - struct intel_load_detect_pipe *old); >> + struct intel_load_detect_pipe *old, >> + struct drm_modeset_acquire_ctx *ctx); >> void intel_release_load_detect_pipe(struct drm_connector *connector, >> - struct intel_load_detect_pipe *old); >> + struct intel_load_detect_pipe *old, >> + struct drm_modeset_acquire_ctx *ctx); >> int intel_pin_and_fence_fb_obj(struct drm_device *dev, >> struct drm_i915_gem_object *obj, >> struct intel_ring_buffer *pipelined); >> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c >> index 213cd58..c235546 100644 >> --- a/drivers/gpu/drm/i915/intel_sprite.c >> +++ b/drivers/gpu/drm/i915/intel_sprite.c >> @@ -55,7 +55,7 @@ static bool intel_pipe_update_start(struct intel_crtc *crtc, uint32_t *start_vbl >> int scanline, min, max, vblank_start; >> DEFINE_WAIT(wait); >> >> - WARN_ON(!mutex_is_locked(&crtc->base.mutex)); >> + WARN_ON(!drm_modeset_is_locked(&crtc->base.mutex)); >> >> vblank_start = mode->crtc_vblank_start; >> if (mode->flags & DRM_MODE_FLAG_INTERLACE) >> diff --git a/drivers/gpu/drm/i915/intel_tv.c b/drivers/gpu/drm/i915/intel_tv.c >> index e0193e8..97d9fc9 100644 >> --- a/drivers/gpu/drm/i915/intel_tv.c >> +++ b/drivers/gpu/drm/i915/intel_tv.c >> @@ -1321,10 +1321,11 @@ intel_tv_detect(struct drm_connector *connector, bool force) >> >> if (force) { >> struct intel_load_detect_pipe tmp; >> + struct drm_modeset_acquire_ctx ctx; >> >> - if (intel_get_load_detect_pipe(connector, &mode, &tmp)) { >> + if (intel_get_load_detect_pipe(connector, &mode, &tmp, &ctx)) { >> type = intel_tv_detect_type(intel_tv, connector); >> - intel_release_load_detect_pipe(connector, &tmp); >> + intel_release_load_detect_pipe(connector, &tmp, &ctx); >> } else >> return connector_status_unknown; >> } else >> diff --git a/drivers/gpu/drm/omapdrm/omap_crtc.c b/drivers/gpu/drm/omapdrm/omap_crtc.c >> index e3c47a8..2d28dc3 100644 >> --- a/drivers/gpu/drm/omapdrm/omap_crtc.c >> +++ b/drivers/gpu/drm/omapdrm/omap_crtc.c >> @@ -319,13 +319,13 @@ static void page_flip_worker(struct work_struct *work) >> struct drm_display_mode *mode = &crtc->mode; >> struct drm_gem_object *bo; >> >> - mutex_lock(&crtc->mutex); >> + drm_modeset_lock(&crtc->mutex, NULL); >> omap_plane_mode_set(omap_crtc->plane, crtc, crtc->primary->fb, >> 0, 0, mode->hdisplay, mode->vdisplay, >> crtc->x << 16, crtc->y << 16, >> mode->hdisplay << 16, mode->vdisplay << 16, >> vblank_cb, crtc); >> - mutex_unlock(&crtc->mutex); >> + drm_modeset_unlock(&crtc->mutex); >> >> bo = omap_framebuffer_bo(crtc->primary->fb, 0); >> drm_gem_object_unreference_unlocked(bo); >> @@ -465,7 +465,7 @@ static void apply_worker(struct work_struct *work) >> * the callbacks and list modification all serialized >> * with respect to modesetting ioctls from userspace. >> */ >> - mutex_lock(&crtc->mutex); >> + drm_modeset_lock(&crtc->mutex, NULL); >> dispc_runtime_get(); >> >> /* >> @@ -510,7 +510,7 @@ static void apply_worker(struct work_struct *work) >> >> out: >> dispc_runtime_put(); >> - mutex_unlock(&crtc->mutex); >> + drm_modeset_unlock(&crtc->mutex); >> } >> >> int omap_crtc_apply(struct drm_crtc *crtc, >> @@ -518,7 +518,7 @@ int omap_crtc_apply(struct drm_crtc *crtc, >> { >> struct omap_crtc *omap_crtc = to_omap_crtc(crtc); >> >> - WARN_ON(!mutex_is_locked(&crtc->mutex)); >> + WARN_ON(!drm_modeset_is_locked(&crtc->mutex)); >> >> /* no need to queue it again if it is already queued: */ >> if (apply->queued) >> diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c >> index e7199b4..8f3edc4 100644 >> --- a/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c >> +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_kms.c >> @@ -187,7 +187,7 @@ int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv, >> * can do this since the caller in the drm core doesn't check anything >> * which is protected by any looks. >> */ >> - mutex_unlock(&crtc->mutex); >> + drm_modeset_unlock(&crtc->mutex); >> drm_modeset_lock_all(dev_priv->dev); >> >> /* A lot of the code assumes this */ >> @@ -252,7 +252,7 @@ int vmw_du_crtc_cursor_set(struct drm_crtc *crtc, struct drm_file *file_priv, >> ret = 0; >> out: >> drm_modeset_unlock_all(dev_priv->dev); >> - mutex_lock(&crtc->mutex); >> + drm_modeset_lock(&crtc->mutex, NULL); >> >> return ret; >> } >> @@ -273,7 +273,7 @@ int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y) >> * can do this since the caller in the drm core doesn't check anything >> * which is protected by any looks. >> */ >> - mutex_unlock(&crtc->mutex); >> + drm_modeset_unlock(&crtc->mutex); >> drm_modeset_lock_all(dev_priv->dev); >> >> vmw_cursor_update_position(dev_priv, shown, >> @@ -281,7 +281,7 @@ int vmw_du_crtc_cursor_move(struct drm_crtc *crtc, int x, int y) >> du->cursor_y + du->hotspot_y); >> >> drm_modeset_unlock_all(dev_priv->dev); >> - mutex_lock(&crtc->mutex); >> + drm_modeset_lock(&crtc->mutex, NULL); >> >> return 0; >> } >> diff --git a/include/drm/drmP.h b/include/drm/drmP.h >> index 12f10bc..a9b8a5d 100644 >> --- a/include/drm/drmP.h >> +++ b/include/drm/drmP.h >> @@ -1184,11 +1184,6 @@ static inline int drm_device_is_unplugged(struct drm_device *dev) >> return ret; >> } >> >> -static inline bool drm_modeset_is_locked(struct drm_device *dev) >> -{ >> - return mutex_is_locked(&dev->mode_config.mutex); >> -} >> - >> static inline bool drm_is_render_client(const struct drm_file *file_priv) >> { >> return file_priv->minor->type == DRM_MINOR_RENDER; >> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h >> index 1bd6708..deadb32 100644 >> --- a/include/drm/drm_crtc.h >> +++ b/include/drm/drm_crtc.h >> @@ -33,6 +33,7 @@ >> #include <linux/hdmi.h> >> #include <drm/drm_mode.h> >> #include <drm/drm_fourcc.h> >> +#include <drm/drm_modeset_lock.h> >> >> struct drm_device; >> struct drm_mode_set; >> @@ -205,6 +206,10 @@ struct drm_property { >> struct list_head enum_blob_list; >> }; >> >> +void drm_modeset_lock_all(struct drm_device *dev); >> +void drm_modeset_unlock_all(struct drm_device *dev); >> +void drm_warn_on_modeset_not_all_locked(struct drm_device *dev); >> + >> struct drm_crtc; >> struct drm_connector; >> struct drm_encoder; >> @@ -280,6 +285,7 @@ struct drm_crtc_funcs { >> * drm_crtc - central CRTC control structure >> * @dev: parent DRM device >> * @head: list management >> + * @mutex: per-CRTC locking >> * @base: base KMS object for ID tracking etc. >> * @primary: primary plane for this CRTC >> * @cursor: cursor plane for this CRTC >> @@ -314,7 +320,7 @@ struct drm_crtc { >> * state, ...) and a write lock for everything which can be update >> * without a full modeset (fb, cursor data, ...) >> */ >> - struct mutex mutex; >> + struct drm_modeset_lock mutex; >> >> struct drm_mode_object base; >> >> @@ -738,7 +744,8 @@ struct drm_mode_group { >> */ >> struct drm_mode_config { >> struct mutex mutex; /* protects configuration (mode lists etc.) */ >> - struct mutex connection_mutex; /* protects connector->encoder and encoder->crtc links */ >> + struct drm_modeset_lock connection_mutex; /* protects connector->encoder and encoder->crtc links */ >> + struct drm_modeset_acquire_ctx *acquire_ctx; /* for legacy _lock_all() / _unlock_all() */ >> struct mutex idr_mutex; /* for IDR management */ >> struct idr crtc_idr; /* use this idr for all IDs, fb, crtc, connector, modes - just makes life easier */ >> /* this is limited to one for now */ >> @@ -839,10 +846,6 @@ struct drm_prop_enum_list { >> char *name; >> }; >> >> -extern void drm_modeset_lock_all(struct drm_device *dev); >> -extern void drm_modeset_unlock_all(struct drm_device *dev); >> -extern void drm_warn_on_modeset_not_all_locked(struct drm_device *dev); >> - >> extern int drm_crtc_init_with_planes(struct drm_device *dev, >> struct drm_crtc *crtc, >> struct drm_plane *primary, >> diff --git a/include/drm/drm_modeset_lock.h b/include/drm/drm_modeset_lock.h >> new file mode 100644 >> index 0000000..2630da3 >> --- /dev/null >> +++ b/include/drm/drm_modeset_lock.h >> @@ -0,0 +1,139 @@ >> +/* >> + * Copyright (C) 2014 Red Hat >> + * Author: Rob Clark <robdclark@xxxxxxxxx> >> + * >> + * Permission is hereby granted, free of charge, to any person obtaining a >> + * copy of this software and associated documentation files (the "Software"), >> + * to deal in the Software without restriction, including without limitation >> + * the rights to use, copy, modify, merge, publish, distribute, sublicense, >> + * and/or sell copies of the Software, and to permit persons to whom the >> + * Software is furnished to do so, subject to the following conditions: >> + * >> + * The above copyright notice and this permission notice shall be included in >> + * all copies or substantial portions of the Software. >> + * >> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR >> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, >> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL >> + * THE COPYRIGHT HOLDER(S) OR AUTHOR(S) BE LIABLE FOR ANY CLAIM, DAMAGES OR >> + * OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, >> + * ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR >> + * OTHER DEALINGS IN THE SOFTWARE. >> + */ >> + >> +#ifndef DRM_MODESET_LOCK_H_ >> +#define DRM_MODESET_LOCK_H_ >> + >> +#include <linux/ww_mutex.h> >> + >> +struct drm_modeset_lock; >> + >> +struct drm_modeset_acquire_ctx { >> + >> + struct ww_acquire_ctx ww_ctx; >> + >> + bool nolock : 1; >> + bool nonblock : 1; >> + >> + /* just for debugging, the context is 'frozen' in drm_atomic_check() >> + * to catch anyone who might be trying to acquire a lock after it is >> + * too late. >> + */ >> + bool frozen : 1; >> + >> + /* contended lock: if a lock is contended you should only call >> + * drm_modeset_backoff() which drops locks and slow-locks the >> + * contended lock. >> + */ >> + struct drm_modeset_lock *contended; >> + >> + /* list of 'struct drm_modeset_lock': */ >> + struct list_head locked; >> + >> + /* currently simply for protecting against 'locked' list manipulation >> + * between original thread calling atomic->end() and driver thread >> + * calling back drm_atomic_commit_unlocked(). >> + * >> + * Other spots are sufficiently synchronized by virtue of holding >> + * the lock's ww_mutex. But during the lock/resource hand-over to the >> + * driver thread (drop_locks()/grab_locks()), we cannot rely on this. >> + */ >> + struct mutex mutex; >> +}; >> + >> +/** >> + * drm_modeset_lock - used for locking modeset resources. >> + * @mutex: resource locking >> + * @atomic_pending: is this resource part of a still-pending >> + * atomic update >> + * @head: used to hold it's place on state->locked list when >> + * part of an atomic update >> + * >> + * Used for locking CRTCs and other modeset resources. >> + */ >> +struct drm_modeset_lock { >> + /** >> + * modeset lock >> + */ >> + struct ww_mutex mutex; >> + >> + /** >> + * Are we busy (pending asynchronous/NONBLOCK update)? Any further >> + * asynchronous update will return -EBUSY if it also needs to acquire >> + * this lock. While a synchronous update will block until the pending >> + * async update completes. >> + * >> + * Drivers must ensure the update is completed before sending vblank >> + * event to userspace. Typically this just means don't send event >> + * before drm_atomic_commit_unlocked() returns. >> + */ >> + bool atomic_pending; >> + >> + /** >> + * Resources that are locked as part of an atomic update are added >> + * to a list (so we know what to unlock at the end). >> + */ >> + struct list_head head; >> + >> + /** >> + * For waiting on atomic_pending locks, if not a NONBLOCK operation. >> + */ >> + wait_queue_head_t event; >> +}; >> + >> +extern struct ww_class crtc_ww_class; >> + >> +void drm_modeset_acquire_init(struct drm_modeset_acquire_ctx *ctx, >> + bool nolock, bool nonblock); >> +void drm_modeset_acquire_fini(struct drm_modeset_acquire_ctx *ctx); >> +void drm_modeset_drop_locks(struct drm_modeset_acquire_ctx *ctx); >> +void drm_modeset_backoff(struct drm_modeset_acquire_ctx *ctx); >> + >> +static inline void drm_modeset_lock_init(struct drm_modeset_lock *lock) >> +{ >> + ww_mutex_init(&lock->mutex, &crtc_ww_class); >> + INIT_LIST_HEAD(&lock->head); >> + init_waitqueue_head(&lock->event); >> +} >> + >> +static inline void drm_modeset_lock_fini(struct drm_modeset_lock *lock) >> +{ >> + WARN_ON(!list_empty(&lock->head)); >> +} >> + >> +static inline bool drm_modeset_is_locked(struct drm_modeset_lock *lock) >> +{ >> + return ww_mutex_is_locked(&lock->mutex); >> +} >> + >> +int drm_modeset_lock(struct drm_modeset_lock *lock, >> + struct drm_modeset_acquire_ctx *ctx); >> +int drm_modeset_lock_interruptible(struct drm_modeset_lock *lock, >> + struct drm_modeset_acquire_ctx *ctx); >> +void drm_modeset_unlock(struct drm_modeset_lock *lock); >> + >> +struct drm_device; >> +int drm_modeset_lock_all_crtcs(struct drm_device *dev, >> + struct drm_modeset_acquire_ctx *ctx); >> + >> +#endif /* DRM_MODESET_LOCK_H_ */ >> diff --git a/include/uapi/drm/drm_mode.h b/include/uapi/drm/drm_mode.h >> index ded505e..6421edc 100644 >> --- a/include/uapi/drm/drm_mode.h >> +++ b/include/uapi/drm/drm_mode.h >> @@ -511,4 +511,7 @@ struct drm_mode_destroy_dumb { >> uint32_t handle; >> }; >> >> +#define DRM_MODE_ATOMIC_NONBLOCK 0x0200 >> +#define DRM_MODE_ATOMIC_NOLOCK 0x8000 /* only used internally */ >> + >> #endif >> -- >> 1.9.3 >> > > -- > Daniel Vetter > Software Engineer, Intel Corporation > +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/dri-devel