From patchwork Wed Sep 23 10:57:36 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Daniel Vetter X-Patchwork-Id: 11794831 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 92D98618 for ; Wed, 23 Sep 2020 10:57:52 +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 52B9E235FC for ; Wed, 23 Sep 2020 10:57:52 +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="J09Es2y2" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 52B9E235FC Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=ffwll.ch Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 373286E941; Wed, 23 Sep 2020 10:57:47 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from mail-wm1-x342.google.com (mail-wm1-x342.google.com [IPv6:2a00:1450:4864:20::342]) by gabe.freedesktop.org (Postfix) with ESMTPS id 1C7AA6E939 for ; Wed, 23 Sep 2020 10:57:46 +0000 (UTC) Received: by mail-wm1-x342.google.com with SMTP id e17so6683439wme.0 for ; Wed, 23 Sep 2020 03:57:46 -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=jH21pZs5HmBz+4VEG5XyFZvmILO//Vy8yo2HNuGYWUw=; b=J09Es2y211HW75cKmq7GQLDvhZJHtwxgB4lHHf1cBaNdbWxsXyE/c3OgNC2qMVfSpi 7Cq9g7dnfq7tZySaZA4D+8MEab3z1zdKoFMVQzU5sARnVmYq+Ujk3rwSkKUEdRliZn8q PaQ/KBWre8tyDX2xDNKqRIsFsm5ua7klHFEcQ= 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=jH21pZs5HmBz+4VEG5XyFZvmILO//Vy8yo2HNuGYWUw=; b=EnRcgh4st8uJFzkrs6ANcPYn01wM1PBBhkT0nCkP3Myron1Jal44+qJlxyB9LfKDiZ zGJQtfU4CMsRD+wSBrFN11CkpLHHiGhvP8kzRwA8z+WVoXmluWQCxiNSzZpDjSCPe4lK j3R5zOOLrcCYsej5gkYt2zrW78+CtRSBAXpRWLwdmkcir27p0ENsfSRLpSyly1xsM+Zj Zgmcj6yIPNeXJ8SuvHZfX6uVRFqYf44Lq9FzMiX22FNze3EunCCBwxPawW2QsE7P8O1D 0a7lddMv7W4ejdfehhb5G5qcIKseGe7epT83f8RGgu8q2LjoxkFu1iarPX6alWZOqfkx YwTw== X-Gm-Message-State: AOAM531yb4trJ3YhFgmJ08mJYJJijN0WZ7XiOBhmWZ6mYIcAPyxm9yER gMbmo/Rq/UrrkLYIvi3A64brfEVwlYJkVzdK X-Google-Smtp-Source: ABdhPJyAj0nK+95p+rvWiTDhGzaJsNgMG/8gzuv2M7xc+eVbmCp3dLXD/S3nXRZtSs/bQKCiUYxvfQ== X-Received: by 2002:a1c:408a:: with SMTP id n132mr5626193wma.45.1600858664388; Wed, 23 Sep 2020 03:57:44 -0700 (PDT) Received: from phenom.ffwll.local ([2a02:168:57f4:0:efd0:b9e5:5ae6:c2fa]) by smtp.gmail.com with ESMTPSA id i14sm5678758wro.96.2020.09.23.03.57.43 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 23 Sep 2020 03:57:43 -0700 (PDT) From: Daniel Vetter To: DRI Development Subject: [PATCH 1/2] drm/atomic: document and enforce rules around "spurious" EBUSY Date: Wed, 23 Sep 2020 12:57:36 +0200 Message-Id: <20200923105737.2943649-1-daniel.vetter@ffwll.ch> X-Mailer: git-send-email 2.28.0 MIME-Version: 1.0 X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Daniel Vetter , Daniel Vetter , Intel Graphics Development , Pekka Paalanen Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" 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) 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 | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 58527f151984..e22669b64521 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, old_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,24 @@ int drm_atomic_check_only(struct drm_atomic_state *state) } } + for_each_new_crtc_in_state(state, crtc, old_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) { + 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);