From patchwork Fri Sep 25 08:46:50 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Daniel Vetter X-Patchwork-Id: 11799359 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id A1F3F618 for ; Fri, 25 Sep 2020 08:46:59 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 45106208B6 for ; Fri, 25 Sep 2020 08:46:59 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=ffwll.ch header.i=@ffwll.ch header.b="QNqKd+5S" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 45106208B6 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ffwll.ch Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=intel-gfx-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id AAA4A6EC5C; Fri, 25 Sep 2020 08:46:58 +0000 (UTC) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mail-wm1-x341.google.com (mail-wm1-x341.google.com [IPv6:2a00:1450:4864:20::341]) by gabe.freedesktop.org (Postfix) with ESMTPS id A78556EC5C for ; Fri, 25 Sep 2020 08:46:57 +0000 (UTC) Received: by mail-wm1-x341.google.com with SMTP id b79so2351923wmb.4 for ; Fri, 25 Sep 2020 01:46:57 -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=zDzVC2Rufup1q6579KGFanX/Bfk1HNnXSNyXVJLCEfY=; b=QNqKd+5SLdgt2cysKSsjMiG2KCsl5hzKzIZpTRgCHoB4p4JPTbCKjLSzgQ5MwUGhDX bZ4XD/zhIrGv0ZHTXSlqrPpgArDxMxSDZPvNe7TFYQHiVvN1TxBNMqRbY1A7nQe7m12k XrDx4YrkobN42AtoxzlTU1gy3w2VF5x5t+TPE= 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=zDzVC2Rufup1q6579KGFanX/Bfk1HNnXSNyXVJLCEfY=; b=MxmsRbh8E8LSXinLqSJZGTBOHW/0s92HdYXgRplqkFngDjAbJ5kuriAIwGrltH4bDF hwrHI1OwS5tm2Y8o50Gb8ShnTo64nk9jZPikBeV4F3g1Tb7zGi53N1nUULgWYkBUJeyw eAGv0z4DOPC45UaNR2f6vAMyYNIRr+ogzTJQ1PSHQmWjvqALA84lMGWTmgV3HShvzUJJ NIge7d181L0KIjLOQDFX1YQr76khVELJzu+t9/+8lcrz7paZcCY4zi/bAIdSdxVwzGmD r6UcMYtC5sSxHI8gVmtxAvEgOqf7rw7eyP8llhpoZUwpnhISkio7p511eo5u5YbZ07kE FPdw== X-Gm-Message-State: AOAM531J07Yk9xbyaQFql0mFeGR4rRMUBzqEe6ncgDMnuFtItz6iP5kU 4hh26qhHeW6Cm+OdqKZJcqVdcQ== X-Google-Smtp-Source: ABdhPJxwuHQwmyF5vQ0Yy363mZBueTaylYC7VdKTKAvN5yzD0U4xYJ8Ue1ZBUnlNUYOmfhAHilEFWQ== X-Received: by 2002:a1c:f208:: with SMTP id s8mr1987668wmc.85.1601023616379; Fri, 25 Sep 2020 01:46:56 -0700 (PDT) Received: from phenom.ffwll.local ([2a02:168:57f4:0:efd0:b9e5:5ae6:c2fa]) by smtp.gmail.com with ESMTPSA id 18sm2031034wmj.28.2020.09.25.01.46.55 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 25 Sep 2020 01:46:55 -0700 (PDT) From: Daniel Vetter To: DRI Development Date: Fri, 25 Sep 2020 10:46:50 +0200 Message-Id: <20200925084651.3250104-1-daniel.vetter@ffwll.ch> X-Mailer: git-send-email 2.28.0 MIME-Version: 1.0 Subject: [Intel-gfx] [PATCH 1/2] drm/atomic: document and enforce rules around "spurious" EBUSY X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Daniel Vetter , Daniel Vetter , Intel Graphics Development , Simon Ser , Pekka Paalanen Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" When doing an atomic modeset with ALLOW_MODESET drivers are allowed to pull in arbitrary other resources, including CRTCs (e.g. when reconfiguring global resources). But in nonblocking mode userspace has then no idea this happened, which can lead to spurious EBUSY calls, both: - when that other CRTC is currently busy doing a page_flip the ALLOW_MODESET commit can fail with an EBUSY - on the other CRTC a normal atomic flip can fail with EBUSY because of the additional commit inserted by the kernel without userspace's knowledge For blocking commits this isn't a problem, because everyone else will just block until all the CRTC are reconfigured. Only thing userspace can notice is the dropped frames without any reason for why frames got dropped. Consensus is that we need new uapi to handle this properly, but no one has any idea what exactly the new uapi should look like. Since this has been shipping for years already compositors need to deal no matter what, so as a first step just try to enforce this across drivers better with some checks. v2: Add comments and a WARN_ON to enforce this only when allowed - we don't want to silently convert page flips into blocking plane updates just because the driver is buggy. v3: Fix inverted WARN_ON (Pekka). v4: Drop the uapi changes, only add a WARN_ON for now to enforce some rules for drivers. v5: Make the WARNING more informative (Daniel) v6: Add unconditional debug output for compositor hackers to figure out what's going on when they get an EBUSY (Daniel) v7: Fix up old/new_crtc_state confusion for real (Pekka/Ville) References: https://lists.freedesktop.org/archives/dri-devel/2018-July/182281.html Bugzilla: https://gitlab.freedesktop.org/wayland/weston/issues/24#note_9568 Cc: Daniel Stone Cc: Pekka Paalanen Cc: Simon Ser Cc: Ville Syrjälä Signed-off-by: Daniel Vetter --- drivers/gpu/drm/drm_atomic.c | 29 +++++++++++++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 58527f151984..aac9122f1da2 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -281,6 +281,10 @@ EXPORT_SYMBOL(__drm_atomic_state_free); * needed. It will also grab the relevant CRTC lock to make sure that the state * is consistent. * + * WARNING: Drivers may only add new CRTC states to a @state if + * drm_atomic_state.allow_modeset is set, or if it's a driver-internal commit + * not created by userspace through an IOCTL call. + * * Returns: * * Either the allocated state or the error code encoded into the pointer. When @@ -1262,10 +1266,15 @@ int drm_atomic_check_only(struct drm_atomic_state *state) struct drm_crtc_state *new_crtc_state; struct drm_connector *conn; struct drm_connector_state *conn_state; + unsigned requested_crtc = 0; + unsigned affected_crtc = 0; int i, ret = 0; DRM_DEBUG_ATOMIC("checking %p\n", state); + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) + requested_crtc |= drm_crtc_mask(crtc); + for_each_oldnew_plane_in_state(state, plane, old_plane_state, new_plane_state, i) { ret = drm_atomic_plane_check(old_plane_state, new_plane_state); if (ret) { @@ -1313,6 +1322,26 @@ int drm_atomic_check_only(struct drm_atomic_state *state) } } + for_each_new_crtc_in_state(state, crtc, new_crtc_state, i) + affected_crtc |= drm_crtc_mask(crtc); + + /* + * For commits that allow modesets drivers can add other CRTCs to the + * atomic commit, e.g. when they need to reallocate global resources. + * This can cause spurious EBUSY, which robs compositors of a very + * effective sanity check for their drawing loop. Therefor only allow + * drivers to add unrelated CRTC states for modeset commits. + * + * FIXME: Should add affected_crtc mask to the ATOMIC IOCTL as an output + * so compositors know what's going on. + */ + if (affected_crtc != requested_crtc) { + DRM_DEBUG_ATOMIC("driver added CRTC to commit: requested 0x%x, affected 0x%0x\n", + requested_crtc, affected_crtc); + WARN(!state->allow_modeset, "adding CRTC not allowed without modesets: requested 0x%x, affected 0x%0x\n", + requested_crtc, affected_crtc); + } + return 0; } EXPORT_SYMBOL(drm_atomic_check_only); From patchwork Fri Sep 25 08:46:51 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Daniel Vetter X-Patchwork-Id: 11799363 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id B37CB618 for ; Fri, 25 Sep 2020 08:47:02 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 49F9720878 for ; Fri, 25 Sep 2020 08:47:02 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (1024-bit key) header.d=ffwll.ch header.i=@ffwll.ch header.b="elDZetC9" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 49F9720878 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ffwll.ch Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=intel-gfx-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 2596D6EC62; Fri, 25 Sep 2020 08:47:00 +0000 (UTC) X-Original-To: intel-gfx@lists.freedesktop.org Delivered-To: intel-gfx@lists.freedesktop.org Received: from mail-wr1-x443.google.com (mail-wr1-x443.google.com [IPv6:2a00:1450:4864:20::443]) by gabe.freedesktop.org (Postfix) with ESMTPS id D206B6EC5D for ; Fri, 25 Sep 2020 08:46:58 +0000 (UTC) Received: by mail-wr1-x443.google.com with SMTP id c18so2638073wrm.9 for ; Fri, 25 Sep 2020 01:46:58 -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:in-reply-to:references :mime-version:content-transfer-encoding; bh=7IqZitQ9KORvFRfCpUB9I8VYakITuk7r9Yti4Q92atc=; b=elDZetC9HgJWDohrUIphsyj5A5B1tb6sEQbwJ6XFmUrEgfpQsK5ImF/JGNH66Dqth+ jwYE2NysxzDAQFSq9Bg8PPi0O72WlSWdPci6OFwLGz4RZUSmHmqS6Ku0NPzqG0k+0+Hu 40Js49057bL2TNma0cXiVUISSvCmOB/HLX7RM= 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:in-reply-to :references:mime-version:content-transfer-encoding; bh=7IqZitQ9KORvFRfCpUB9I8VYakITuk7r9Yti4Q92atc=; b=lZXyI8H8IzlqDjaUGjUJl3JunOTN3o/4Fogz1XrPy4HnqDFflqzT0N9bpERR9srN2x X3iW4V9dvJc92e0h8jty7bVAZ/SCNx0SgTMziNCWsjTXpMm4MOYcb5gIRnQkVTh35CYo aQxPmK2WCohRNT6bAYOp0H0sM5cj739tw5WiSktycAZpAK64mQB0weaGhwDWhm/sphBf tqMEyx1ZeZn3zg4DLLriICnEzX2CwyEXEjWJvRPT3yut0xucPRGZkBYt333aB1PhD7Rl WeTjlrz5z1KTVZWksmZ8D3fdhEGLtucs0tKq+JCsP7Nnna8q8npa64nVUczBTtZCN2LJ FYDg== X-Gm-Message-State: AOAM5334kmcGp2ippG3UEJRV6F8GICoq0ytRfmLuXfr8QLTCQRpoD/jf nsVfO37mFg1fsH7Tkx8dpbkn9Q== X-Google-Smtp-Source: ABdhPJy7Xm8XjOk+WDpx5dr+exmR4qUt4wJqFCWS145GRpRmJYVH4VtmGoKN5RnVMK5tgX+IL2we4g== X-Received: by 2002:a5d:46cf:: with SMTP id g15mr3289510wrs.107.1601023617530; Fri, 25 Sep 2020 01:46:57 -0700 (PDT) Received: from phenom.ffwll.local ([2a02:168:57f4:0:efd0:b9e5:5ae6:c2fa]) by smtp.gmail.com with ESMTPSA id 18sm2031034wmj.28.2020.09.25.01.46.56 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 25 Sep 2020 01:46:56 -0700 (PDT) From: Daniel Vetter To: DRI Development Date: Fri, 25 Sep 2020 10:46:51 +0200 Message-Id: <20200925084651.3250104-2-daniel.vetter@ffwll.ch> X-Mailer: git-send-email 2.28.0 In-Reply-To: <20200925084651.3250104-1-daniel.vetter@ffwll.ch> References: <20200925084651.3250104-1-daniel.vetter@ffwll.ch> MIME-Version: 1.0 Subject: [Intel-gfx] [PATCH 2/2] drm/atomic: debug output for EBUSY X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Daniel Vetter , Daniel Vetter , Intel Graphics Development , Sean Paul , Simon Ser , Pekka Paalanen Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" Hopefully we'll have the drm crash recorder RSN, but meanwhile compositors would like to know a bit better why they get an EBUSY. v2: Move misplaced hunk to the right patch (Pekka) Cc: Sean Paul Cc: Daniel Stone Cc: Pekka Paalanen Cc: Simon Ser Cc: Ville Syrjälä Signed-off-by: Daniel Vetter Acked-by: Pekka Paalanen Reviewed-by: Daniel Stone --- drivers/gpu/drm/drm_atomic_helper.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index e8abaaaa7fd1..6b3bfabac26c 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -1740,8 +1740,11 @@ int drm_atomic_helper_async_check(struct drm_device *dev, * overridden by a previous synchronous update's state. */ if (old_plane_state->commit && - !try_wait_for_completion(&old_plane_state->commit->hw_done)) + !try_wait_for_completion(&old_plane_state->commit->hw_done)) { + DRM_DEBUG_ATOMIC("[PLANE:%d:%s] inflight previous commit preventing async commit\n", + plane->base.id, plane->name); return -EBUSY; + } return funcs->atomic_async_check(plane, new_plane_state); } @@ -1964,6 +1967,9 @@ static int stall_checks(struct drm_crtc *crtc, bool nonblock) * commit with nonblocking ones. */ if (!completed && nonblock) { spin_unlock(&crtc->commit_lock); + DRM_DEBUG_ATOMIC("[CRTC:%d:%s] busy with a previous commit\n", + crtc->base.id, crtc->name); + return -EBUSY; } } else if (i == 1) { @@ -2132,8 +2138,12 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, /* Userspace is not allowed to get ahead of the previous * commit with nonblocking ones. */ if (nonblock && old_conn_state->commit && - !try_wait_for_completion(&old_conn_state->commit->flip_done)) + !try_wait_for_completion(&old_conn_state->commit->flip_done)) { + DRM_DEBUG_ATOMIC("[CONNECTOR:%d:%s] busy with a previous commit\n", + conn->base.id, conn->name); + return -EBUSY; + } /* Always track connectors explicitly for e.g. link retraining. */ commit = crtc_or_fake_commit(state, new_conn_state->crtc ?: old_conn_state->crtc); @@ -2147,8 +2157,12 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state, /* Userspace is not allowed to get ahead of the previous * commit with nonblocking ones. */ if (nonblock && old_plane_state->commit && - !try_wait_for_completion(&old_plane_state->commit->flip_done)) + !try_wait_for_completion(&old_plane_state->commit->flip_done)) { + DRM_DEBUG_ATOMIC("[PLANE:%d:%s] busy with a previous commit\n", + plane->base.id, plane->name); + return -EBUSY; + } /* Always track planes explicitly for async pageflip support. */ commit = crtc_or_fake_commit(state, new_plane_state->crtc ?: old_plane_state->crtc);