From patchwork Tue Mar 21 11:22:06 2017 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Daniel Vetter X-Patchwork-Id: 9636415 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 6102D602CC for ; Tue, 21 Mar 2017 11:22:18 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 6349426224 for ; Tue, 21 Mar 2017 11:22:18 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 581672768C; Tue, 21 Mar 2017 11:22:18 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.1 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 6F0A126224 for ; Tue, 21 Mar 2017 11:22:17 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 7EDDB6E377; Tue, 21 Mar 2017 11:22:15 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-wr0-x241.google.com (mail-wr0-x241.google.com [IPv6:2a00:1450:400c:c0c::241]) by gabe.freedesktop.org (Postfix) with ESMTPS id D9D8B6E377 for ; Tue, 21 Mar 2017 11:22:13 +0000 (UTC) Received: by mail-wr0-x241.google.com with SMTP id g10so22017786wrg.0 for ; Tue, 21 Mar 2017 04:22:13 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ffwll.ch; s=google; h=from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=jufuT1r8VSXE1Dw1r1ApQA4hyqJSs8h+cYW9rKHACDI=; b=i6kOKfPlQ3VHYv60aZWxCPUM5av7KO2RjB/QL5Ptw6XwMxsRXQwmU5uxW4nemWLCGb YlBjmT4KO+r7ceA09qTtjKL2/uPaYyh7Ua1FYnD10QNuw5T7H6Jro1HyuGZN8gqktApW sVtXvdONbBrodFvDDOi7lm7A2itD1gyR+jXec= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=jufuT1r8VSXE1Dw1r1ApQA4hyqJSs8h+cYW9rKHACDI=; b=Z2YdQSOcXYs4uUgEmybG2CAYn4m7lq7Lj583uC8O8JCNoPelDjhmPWGraI0ENfvYr/ a2cBvL4FtP064s3LuVXPzOGB6UUrAAs6EOwPDv+Nhute/9SgeGaRU4yMtEg3gTV0jm+4 OeTQZQByzY4UX8BlHZZ9FFhSEhq3y/wOqqtWpVOTZk6FS3cT/nKSgg/xSz88YjGKHCDE y+3yk0Gy9o3B/50UNjVoMdI/uTDk9OsbeuAU0iRathiFxf8qbD83W1RpW1LZTSZSzRKc AtTtJOu/yyrbhXZQRMjtri/r0JYyt6jJ6xGfxRZ9ylIm2Kadi7YWDSo135OW0mvjSTP4 XrLg== X-Gm-Message-State: AFeK/H0cfUZy5vsOTstPsP93F+1gr9g2dRqltCKxDrG7YRR3mOqeRsnNJg07UuXUGdBm9w== X-Received: by 10.223.163.145 with SMTP id l17mr29719463wrb.103.1490095332453; Tue, 21 Mar 2017 04:22:12 -0700 (PDT) Received: from phenom.ffwll.local ([2a02:168:56c9:0:decc:6e78:7e96:b452]) by smtp.gmail.com with ESMTPSA id z88sm24671735wrb.26.2017.03.21.04.22.11 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Tue, 21 Mar 2017 04:22:11 -0700 (PDT) From: Daniel Vetter To: Intel Graphics Development Subject: [PATCH] drm/atomic: Introduce drm_atomic_helper_shutdown Date: Tue, 21 Mar 2017 12:22:06 +0100 Message-Id: <20170321112206.19867-1-daniel.vetter@ffwll.ch> X-Mailer: git-send-email 2.11.0 MIME-Version: 1.0 Cc: Daniel Vetter , DRI Development , Ben Skeggs , Daniel Vetter X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Virus-Scanned: ClamAV using ClamSMTP The trouble here is that it does multiple atomic commits under one drm_modeset_lock_all, which breaks the behind-the-scenes acquire context magic that function pulls off. It's much better to have one overall atomic commit. That we still have multiple atomic commits prevents us from adding some pretty useful debug checks to the atomic machinery. Hence it is really a bad idea to call the legacy drm_crtc_force_disable_all() function. There's 2 atomic drivers using this still, nouveau and tinydrm. To fix this, introduce a new drm_atomic_helper_shutdown() by extracting the code from i915. While at it improve kernel-doc and catch future offenders by sprinkling a WARN_ON into the legacy function. We should probably move those into the legacy modeset helpers, too ... Cc: Maarten Lankhorst Cc: Noralf Trønnes Cc: Ben Skeggs Signed-off-by: Daniel Vetter Acked-by: Noralf Trønnes --- drivers/gpu/drm/drm_atomic_helper.c | 42 +++++++++++++++++++++++++++-- drivers/gpu/drm/drm_crtc.c | 7 +++++ drivers/gpu/drm/i915/i915_drv.c | 20 +------------- drivers/gpu/drm/nouveau/nouveau_display.c | 8 ++++-- drivers/gpu/drm/tinydrm/core/tinydrm-core.c | 4 +-- include/drm/drm_atomic_helper.h | 1 + 6 files changed, 57 insertions(+), 25 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index b4dcb2559e09..5f401519d03c 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -2444,7 +2444,8 @@ int __drm_atomic_helper_set_config(struct drm_mode_set *set, * that they are connected to. * * This is used for example in suspend/resume to disable all currently active - * functions when suspending. + * functions when suspending. If you just want to shut down everything at e.g. + * driver unload, look at drm_atomic_helper_shutdown(). * * Note that if callers haven't already acquired all modeset locks this might * return -EDEADLK, which must be handled by calling drm_modeset_backoff(). @@ -2453,7 +2454,8 @@ int __drm_atomic_helper_set_config(struct drm_mode_set *set, * 0 on success or a negative error code on failure. * * See also: - * drm_atomic_helper_suspend(), drm_atomic_helper_resume() + * drm_atomic_helper_suspend(), drm_atomic_helper_resume() and + * drm_atomic_helper_shutdown(). */ int drm_atomic_helper_disable_all(struct drm_device *dev, struct drm_modeset_acquire_ctx *ctx) @@ -2518,6 +2520,42 @@ int drm_atomic_helper_disable_all(struct drm_device *dev, EXPORT_SYMBOL(drm_atomic_helper_disable_all); /** + * drm_atomic_helper_shutdown - shutdown all CRTC + * @dev: DRM device + * + * This shuts down all CRTC, which is useful for driver unloading. Shutdown on + * suspend should instead be handled with drm_atomic_helper_suspend(), since + * that also takes a snapshot of the modeset state to be restored on resume. + * + * This is just a convenience wrapper around drm_atomic_helper_disable_all(), + * and it is the atomic version of drm_crtc_force_disable(). + */ +void drm_atomic_helper_shutdown(struct drm_device *dev) +{ + struct drm_modeset_acquire_ctx ctx; + int ret; + + drm_modeset_acquire_init(&ctx, 0); + while (1) { + ret = drm_modeset_lock_all_ctx(dev, &ctx); + if (!ret) + ret = drm_atomic_helper_disable_all(dev, &ctx); + + if (ret != -EDEADLK) + break; + + drm_modeset_backoff(&ctx); + } + + if (ret) + DRM_ERROR("Disabling all crtc's during unload failed with %i\n", ret); + + drm_modeset_drop_locks(&ctx); + drm_modeset_acquire_fini(&ctx); +} +EXPORT_SYMBOL(drm_atomic_helper_shutdown); + +/** * drm_atomic_helper_suspend - subsystem-level suspend helper * @dev: DRM device * diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c index e2974d3c92e7..660b4c8715de 100644 --- a/drivers/gpu/drm/drm_crtc.c +++ b/drivers/gpu/drm/drm_crtc.c @@ -94,6 +94,8 @@ EXPORT_SYMBOL(drm_crtc_from_index); * drm_crtc_force_disable - Forcibly turn off a CRTC * @crtc: CRTC to turn off * + * Note: This should only be used by non-atomic legacy drivers. + * * Returns: * Zero on success, error code on failure. */ @@ -103,6 +105,8 @@ int drm_crtc_force_disable(struct drm_crtc *crtc) .crtc = crtc, }; + WARN_ON(drm_drv_uses_atomic_modeset(crtc->dev)); + return drm_mode_set_config_internal(&set); } EXPORT_SYMBOL(drm_crtc_force_disable); @@ -114,6 +118,9 @@ EXPORT_SYMBOL(drm_crtc_force_disable); * Drivers may want to call this on unload to ensure that all displays are * unlit and the GPU is in a consistent, low power state. Takes modeset locks. * + * Note: This should only be used by non-atomic legacy drivers. For an atomic + * version look at drm_atomic_helper_shutdown(). + * * Returns: * Zero on success, error code on failure. */ diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 95fb7d391788..e54800a3e52d 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1306,8 +1306,6 @@ void i915_driver_unload(struct drm_device *dev) { struct drm_i915_private *dev_priv = to_i915(dev); struct pci_dev *pdev = dev_priv->drm.pdev; - struct drm_modeset_acquire_ctx ctx; - int ret; intel_fbdev_fini(dev); @@ -1316,23 +1314,7 @@ void i915_driver_unload(struct drm_device *dev) intel_display_power_get(dev_priv, POWER_DOMAIN_INIT); - drm_modeset_acquire_init(&ctx, 0); - while (1) { - ret = drm_modeset_lock_all_ctx(dev, &ctx); - if (!ret) - ret = drm_atomic_helper_disable_all(dev, &ctx); - - if (ret != -EDEADLK) - break; - - drm_modeset_backoff(&ctx); - } - - if (ret) - DRM_ERROR("Disabling all crtc's during unload failed with %i\n", ret); - - drm_modeset_drop_locks(&ctx); - drm_modeset_acquire_fini(&ctx); + drm_atomic_helper_shutdown(dev); intel_gvt_cleanup(dev_priv); diff --git a/drivers/gpu/drm/nouveau/nouveau_display.c b/drivers/gpu/drm/nouveau/nouveau_display.c index 8b3622afa530..b5a92386cc8e 100644 --- a/drivers/gpu/drm/nouveau/nouveau_display.c +++ b/drivers/gpu/drm/nouveau/nouveau_display.c @@ -413,8 +413,12 @@ nouveau_display_fini(struct drm_device *dev, bool suspend) struct drm_connector *connector; struct drm_crtc *crtc; - if (!suspend) - drm_crtc_force_disable_all(dev); + if (!suspend) { + if (drm_drv_uses_atomic_modeset(dev)) + drm_atomic_helper_shutdown(dev); + else + drm_crtc_force_disable_all(dev); + } /* Make sure that drm and hw vblank irqs get properly disabled. */ drm_for_each_crtc(crtc, dev) diff --git a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c index 6a257dd08ee0..bbb85e0fc153 100644 --- a/drivers/gpu/drm/tinydrm/core/tinydrm-core.c +++ b/drivers/gpu/drm/tinydrm/core/tinydrm-core.c @@ -251,7 +251,7 @@ static void tinydrm_unregister(struct tinydrm_device *tdev) { struct drm_fbdev_cma *fbdev_cma = tdev->fbdev_cma; - drm_crtc_force_disable_all(tdev->drm); + drm_atomic_helper_shutdown(tdev->dev); /* don't restore fbdev in lastclose, keep pipeline disabled */ tdev->fbdev_cma = NULL; drm_dev_unregister(tdev->drm); @@ -302,7 +302,7 @@ EXPORT_SYMBOL(devm_tinydrm_register); */ void tinydrm_shutdown(struct tinydrm_device *tdev) { - drm_crtc_force_disable_all(tdev->drm); + drm_atomic_helper_shutdown(tdev->dev); } EXPORT_SYMBOL(tinydrm_shutdown); diff --git a/include/drm/drm_atomic_helper.h b/include/drm/drm_atomic_helper.h index dc16274987c7..969f7237f1fc 100644 --- a/include/drm/drm_atomic_helper.h +++ b/include/drm/drm_atomic_helper.h @@ -104,6 +104,7 @@ int __drm_atomic_helper_set_config(struct drm_mode_set *set, int drm_atomic_helper_disable_all(struct drm_device *dev, struct drm_modeset_acquire_ctx *ctx); +void drm_atomic_helper_shutdown(struct drm_device *dev); struct drm_atomic_state *drm_atomic_helper_suspend(struct drm_device *dev); int drm_atomic_helper_commit_duplicated_state(struct drm_atomic_state *state, struct drm_modeset_acquire_ctx *ctx);