From patchwork Tue Apr 15 08:02:43 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Daniel Vetter X-Patchwork-Id: 3989871 Return-Path: X-Original-To: patchwork-intel-gfx@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork1.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.19.201]) by patchwork1.web.kernel.org (Postfix) with ESMTP id BD2669F375 for ; Tue, 15 Apr 2014 08:02:57 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id C6F5E20200 for ; Tue, 15 Apr 2014 08:02:56 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id B3710201FB for ; Tue, 15 Apr 2014 08:02:55 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id AF2B56E880; Tue, 15 Apr 2014 01:02:54 -0700 (PDT) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mail-ee0-f48.google.com (mail-ee0-f48.google.com [74.125.83.48]) by gabe.freedesktop.org (Postfix) with ESMTP id 873196E87E for ; Tue, 15 Apr 2014 01:02:52 -0700 (PDT) Received: by mail-ee0-f48.google.com with SMTP id b57so7325421eek.21 for ; Tue, 15 Apr 2014 01:02:51 -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; bh=qNoiiL1SInR5xJ/TY9aQ0JgfbVBmYnLdgZcinsR1trQ=; b=EKBQYw4BANhe9IL46KQBRCQpI43fQXLxCrnfZ44/6GVMLzkcVXShBsFJu2uFDQX8UE JgXw6c0knl+P4jh4jqf4DA1lz4OMecMdQ/dLwQmADglnpFXMBlZr7mKqbqHgbC6deaA2 DPTLqqYjDnPU4HL0HbTbkZN6chYAab3hq9XPQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:from:to:cc:subject:date:message-id; bh=qNoiiL1SInR5xJ/TY9aQ0JgfbVBmYnLdgZcinsR1trQ=; b=g0dZN+UT7QpYzyW4LvF3G2R+Q4XC8RXpVB3CJRK0+UOFMDPrkNF2j6Pyp5Qy3KjxTc JtwuZlQsRDa5r+QpegdpNPDz4RQUNlGebQ7Tpwf8aH95ADxzfuHHjeIrqFuuJ0kgasHp BNUURtbrzNbOZXwYtUhCKNR3/3YWtYK/fzcTWqshck5zd8zpnJnSJ+iDwoUekqWDcY9O EzWmKGQ2lO1jZhcI0ODq6ghfEcph3BS4bGURUqLmG0b31SgAB5L6zZVn1+dRJ4YkmVcz k+yGxpQqo98r7xrNnR63ox5E1rE23WROidcqtYSPn8NN06Sdmlu3FD2TDnGPynTtI/mZ l1mg== X-Gm-Message-State: ALoCoQkkbPddKeGUakc8E+QuzEj5U6oRjDQdUwYOolWlyVQOQTKYYn5OulHs2AzI39HJTOzMFimW X-Received: by 10.14.126.73 with SMTP id a49mr622751eei.46.1397548971471; Tue, 15 Apr 2014 01:02:51 -0700 (PDT) Received: from phenom.ffwll.local (84-73-67-144.dclient.hispeed.ch. [84.73.67.144]) by mx.google.com with ESMTPSA id u46sm47233377eel.1.2014.04.15.01.02.49 for (version=TLSv1.2 cipher=ECDHE-RSA-AES128-SHA bits=128/128); Tue, 15 Apr 2014 01:02:50 -0700 (PDT) From: Daniel Vetter To: DRI Development Date: Tue, 15 Apr 2014 10:02:43 +0200 Message-Id: <1397548963-18189-1-git-send-email-daniel.vetter@ffwll.ch> X-Mailer: git-send-email 1.9.2 Cc: Daniel Vetter , Intel Graphics Development Subject: [Intel-gfx] [PATCH] drm/plane-helper: Don't fake-implement primary plane disabling X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , MIME-Version: 1.0 Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" X-Spam-Status: No, score=-4.7 required=5.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,RP_MATCHES_RCVD,T_DKIM_INVALID,UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Virus-Scanned: ClamAV using ClamSMTP After thinking about this topic a bit more I've reached the conclusion that implementing this doesn't make sense: - The locking is all wrong: set_config(NULL) will also unlink encoders and connectors, but those links are protected with the mode_config mutex. In the ->disable_plane callback we only hold all modeset locks, but eventually we want to switch to just grabbing the per-crtc (and maybe per-plane) locks as needed, maybe based on ww_mutexes. Having a callback which absolutely needs all modeset locks is bad for this conversion. Note that the same isn't true for the provided ->update_plane since we've audited the crtc helpers to make sure that not encoder or connector links are changed. - There's no way to re-enable the plane with an ->update_plane: The connectors/encoder links are lost and so we can't re-enable the CRTC. Even without that issue the driver might have reassigned some shared resources (as opposed to e.g. DPMS off, where drivers are not allowed to do that to make sure the CRTC can be enabled again). - The semantics don't make much sense: Userspace asked to scan out black (or some other color if the driver supports a background color), not that the screen be disabled. - Implementing proper primary plane support (i.e. actually disabling the primary plane without disabling the CRTC) is really simple, at least if all the hw needs is flipping a bit. The big task is auditing all the interactions with other ioctls when the CRTC is on but there's no primary plane (e.g. pageflips). And some of that work still needs to be done. Cc: Matt Roper Signed-off-by: Daniel Vetter Reviewed-by: Ville Syrjälä Reviewed-by: Matt Roper --- drivers/gpu/drm/drm_plane_helper.c | 33 +++++---------------------------- 1 file changed, 5 insertions(+), 28 deletions(-) diff --git a/drivers/gpu/drm/drm_plane_helper.c b/drivers/gpu/drm/drm_plane_helper.c index 9263fd5efa58..9540ff9f97fe 100644 --- a/drivers/gpu/drm/drm_plane_helper.c +++ b/drivers/gpu/drm/drm_plane_helper.c @@ -205,9 +205,9 @@ EXPORT_SYMBOL(drm_primary_helper_update); * * Provides a default plane disable handler for primary planes. This is handler * is called in response to a userspace SetPlane operation on the plane with a - * NULL framebuffer parameter. We call the driver's modeset handler with a NULL - * framebuffer to disable the CRTC if no other planes are currently enabled. - * If other planes are still enabled on the same CRTC, we return -EBUSY. + * NULL framebuffer parameter. It unconditionally fails the disable call with + * -EINVAL the only way to disable the primary plane without driver support is + * to disable the entier CRTC. Which does not match the plane ->disable hook. * * Note that some hardware may be able to disable the primary plane without * disabling the whole CRTC. Drivers for such hardware should provide their @@ -216,34 +216,11 @@ EXPORT_SYMBOL(drm_primary_helper_update); * disabled primary plane). * * RETURNS: - * Zero on success, error code on failure + * Unconditionally returns -EINVAL. */ int drm_primary_helper_disable(struct drm_plane *plane) { - struct drm_plane *p; - struct drm_mode_set set = { - .crtc = plane->crtc, - .fb = NULL, - }; - - if (plane->crtc == NULL || plane->fb == NULL) - /* Already disabled */ - return 0; - - list_for_each_entry(p, &plane->dev->mode_config.plane_list, head) - if (p != plane && p->fb) { - DRM_DEBUG_KMS("Cannot disable primary plane while other planes are still active on CRTC.\n"); - return -EBUSY; - } - - /* - * N.B. We call set_config() directly here rather than - * drm_mode_set_config_internal() since drm_mode_setplane() already - * handles the basic refcounting and we don't need the special - * cross-CRTC refcounting (no chance of stealing connectors from - * other CRTC's with this update). - */ - return plane->crtc->funcs->set_config(&set); + return -EINVAL; } EXPORT_SYMBOL(drm_primary_helper_disable);