From patchwork Thu Jan 10 20:47:57 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Vetter X-Patchwork-Id: 1962541 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-process-083081@patchwork2.kernel.org Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by patchwork2.kernel.org (Postfix) with ESMTP id C3672DF264 for ; Thu, 10 Jan 2013 21:04:12 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id A2554E651B for ; Thu, 10 Jan 2013 13:04:12 -0800 (PST) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-wi0-f182.google.com (mail-wi0-f182.google.com [209.85.212.182]) by gabe.freedesktop.org (Postfix) with ESMTP id BBCDAE6114 for ; Thu, 10 Jan 2013 12:48:43 -0800 (PST) Received: by mail-wi0-f182.google.com with SMTP id hn14so611488wib.15 for ; Thu, 10 Jan 2013 12:48:43 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=x-received:from:to:cc:subject:date:message-id:x-mailer:in-reply-to :references; bh=iV8wsjp4v7RZ+pVzSfLad2eWuFMaujhQsyyKQYaR+Ag=; b=Vo/WwTZfOEtwJFdeZFx6uJsDVxxspFdz4rtIBnZUgsQqlx9foRdTSgo1KToIENSwfb Su4cEdx6zxt/n6yFylEKELMlI8X9TjTM6BteqXOtjnh6ZYRAwqxcowzasWz3VAozuxKa j6zvfqOIfLenhyVSC38Gd31xTFq5jfU4AL//s= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-received:from:to:cc:subject:date:message-id:x-mailer:in-reply-to :references:x-gm-message-state; bh=iV8wsjp4v7RZ+pVzSfLad2eWuFMaujhQsyyKQYaR+Ag=; b=CS0lY6/30YRFOUZbKoEAm5WkdKxO1p6bZZKTw5FIk1sOmlGuUDau/o1PoqU2JQ0hUQ 7MyAR5/YjyV9cc+3eLCNGjAQYhxNt/q7I+ymhXaSUsAOdpG0U8GMPszMXySGJ5eJh5BU 80IYXTsnEWM0X8a1Ulvwj5F3PAqCStfhdXFetG68OY5eopYyfeRKcpmQg3hGiCyC59oz V3IjH5JMqk6FxEXKH5oBh0Lo/s0zm7pbOvRQqK3B9bEJ1ULsQ6/BGQ1/SdljpIs00Xs3 Lffec+U+pifRIWvkry3Xn4Iz+IIdPdA5XR3QAUXkf7+Q5bws5CiaiPbuVPmNJMVeeJeB qeCw== X-Received: by 10.194.177.199 with SMTP id cs7mr117165033wjc.41.1357850922948; Thu, 10 Jan 2013 12:48:42 -0800 (PST) Received: from biers.ffwll.local (178-83-130-250.dynamic.hispeed.ch. [178.83.130.250]) by mx.google.com with ESMTPS id hu8sm9975655wib.6.2013.01.10.12.48.42 (version=TLSv1.2 cipher=ECDHE-RSA-RC4-SHA bits=128/128); Thu, 10 Jan 2013 12:48:42 -0800 (PST) From: Daniel Vetter To: DRI Development Subject: [PATCH 16/35] drm: add per-crtc locks Date: Thu, 10 Jan 2013 21:47:57 +0100 Message-Id: <1357850897-27102-17-git-send-email-daniel.vetter@ffwll.ch> X-Mailer: git-send-email 1.7.10.4 In-Reply-To: <1357850897-27102-1-git-send-email-daniel.vetter@ffwll.ch> References: <1357850897-27102-1-git-send-email-daniel.vetter@ffwll.ch> X-Gm-Message-State: ALoCoQmTxDdHjJcXSrsE1+fmbNAXm0g21bCx1VxE2knihEN2f0Wh2BaKtTV1EI/uEbNOF6h/goL9 Cc: Daniel Vetter X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.13 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Sender: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org Errors-To: dri-devel-bounces+patchwork-dri-devel=patchwork.kernel.org@lists.freedesktop.org *drumroll* The basic idea is to protect per-crtc state which can change without touching the output configuration with separate mutexes, i.e. all the input side state to a crtc like framebuffers, cursor settings or plane configuration. Holding such a crtc lock gives a read-lock on all the other crtc state which can be changed by e.g. a modeset. All non-crtc state is still protected by the mode_config mutex. Callers that need to change modeset state of a crtc (e.g. dpms or set_mode) need to grab both the mode_config lock and nested within any crtc locks. Note that since there can only ever be one holder of the mode_config lock we can grab the subordinate crtc locks in any order (if we need to grab more than one of them). Lockdep can handle such nesting with the mutex_lock_nest_lock call correctly. With this functions that only touch connectors/encoders but not crtcs only need to take the mode_config lock. The biggest such case is the output probing, which means that we can now pageflip and move cursors while the output probe code is reading an edid. Most cases neatly fall into the three buckets: - Only touches connectors and similar output state and so only needs the mode_config lock. - Touches the global configuration and so needs all locks. - Only touches the crtc input side and so only needs the crtc lock. But a few cases that need special consideration: - Load detection which requires a crtc. The mode_config lock already prevents a modeset change, so we can use any unused crtc as we like to do load detection. The only thing to consider is that such temporary state changes don't leak out to userspace through ioctls that only take the crtc look (like a pageflip). Hence the load detect code needs to grab the crtc of any output pipes it touches (but only if it touches state used by the pageflip or cursor ioctls). - Atomic pageflip when moving planes. The first case is sane hw, where planes have a fixed association with crtcs - nothing needs to be done there. More insane^Wflexible hw needs to have plane->crtc mapping which is separately protect with a lock that nests within the crtc lock. If the plane is unused we can just assign it to the current crtc and continue. But if a plane is already in use by another crtc we can't just reassign it. Two solution present themselves: Either go back to a slow-path which takes all modeset locks, potentially incure quite a hefty delay. Or simply disallowing such changes in one atomic pageflip - in general the vblanks of two crtcs are not synced, so there's no sane way to atomically flip such plane changes accross more than one crtc. I'd heavily favour the later approach, going as far as mandating it as part of the ABI of such a new a nuclear pageflip. And if we _really_ want such semantics, we can always get them by introducing another pageflip mutex between the mode_config.mutex and the individual crtc locks. Pageflips crossing more than one crtc would then need to take that lock first, to lock out concurrent multi-crtc pageflips. - Optimized global modeset operations: We could just take the mode_config lock and then lazily lock all crtc which are affected by a modeset operation. This has the advantage that pageflip could continue unhampered on unaffected crtc. But if e.g. global resources like plls need to be reassigned and so affect unrelated crtcs we can still do that - nested locking works in any order. This patch just adds the locks and takes them in drm_modeset_lock_all, no real locking changes yet. v2: Need to initialize the new lock in crtc_init and lock it righ away, for otherwise the modeset_unlock_all below will try to unlock a not-locked mutex. Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_crtc.c | 12 ++++++++++++ include/drm/drm_crtc.h | 9 +++++++++ 2 files changed, 21 insertions(+) diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index 7c8c5cd..0494ff4 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -46,7 +46,12 @@ */ void drm_modeset_lock_all(struct drm_device *dev) { + struct drm_crtc *crtc; + 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); } EXPORT_SYMBOL(drm_modeset_lock_all); @@ -56,6 +61,11 @@ EXPORT_SYMBOL(drm_modeset_lock_all); */ 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); + mutex_unlock(&dev->mode_config.mutex); } EXPORT_SYMBOL(drm_modeset_unlock_all); @@ -449,6 +459,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); ret = drm_mode_object_get(dev, &crtc->base, DRM_MODE_OBJECT_CRTC); if (ret) diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h index 9f0524d..c89b116 100644 --- a/include/drm/drm_crtc.h +++ b/include/drm/drm_crtc.h @@ -390,6 +390,15 @@ struct drm_crtc { struct drm_device *dev; struct list_head head; + /** + * crtc mutex + * + * This provides a read lock for the overall crtc state (mode, dpms + * state, ...) and a write lock for everything which can be update + * without a full modeset (fb, cursor data, ...) + */ + struct mutex mutex; + struct drm_mode_object base; /* framebuffer the connector is currently bound to */