[PATCH 00/11] dma_fence critical sections annotations for atomic

[PATCH 00/11] dma_fence critical sections annotations for atomic

[Daniel Vetter]

Hi all,

Finally gotten around to refreshing all the various fence anntotions I've
hast last summer. Or well parts of it:

- entire amdgpu and drm/scheduler annotations postponed for now, because
  there's way too many splats in there that need some work

- in recent patches I've seen quite a few dma_resv_lock or kmalloc in
  atomic_commit_tail, which doesn't work in full generality with the rules
  for dma_fences we've discussed and encoded in lockdep. These annotations
  should catch stuff like this.

Review comments and testing very much welcome.

Cheers, Daniel

Daniel Vetter (11):
  drm/atomic-helper: Add dma-fence annotations
  drm/vkms: Annotate vblank timer
  drm/vblank: Annotate with dma-fence signalling section
  drm/komeda: Annotate dma-fence critical section in commit path
  drm/malidp: Annotate dma-fence critical section in commit path
  drm/atmel: Use drm_atomic_helper_commit
  drm/imx: Annotate dma-fence critical section in commit path
  drm/omapdrm: Annotate dma-fence critical section in commit path
  drm/rcar-du: Annotate dma-fence critical section in commit path
  drm/tegra: Annotate dma-fence critical section in commit path
  drm/tidss: Annotate dma-fence critical section in commit path

 .../gpu/drm/arm/display/komeda/komeda_kms.c   |   3 +
 drivers/gpu/drm/arm/malidp_drv.c              |   3 +
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c  | 107 +-----------------
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h  |   7 --
 drivers/gpu/drm/drm_atomic_helper.c           |  16 +++
 drivers/gpu/drm/drm_vblank.c                  |   8 +-
 drivers/gpu/drm/imx/imx-drm-core.c            |   2 +
 drivers/gpu/drm/omapdrm/omap_drv.c            |   9 +-
 drivers/gpu/drm/rcar-du/rcar_du_kms.c         |   2 +
 drivers/gpu/drm/tegra/drm.c                   |   3 +
 drivers/gpu/drm/tidss/tidss_kms.c             |   4 +
 drivers/gpu/drm/vkms/vkms_crtc.c              |   8 +-
 12 files changed, 54 insertions(+), 118 deletions(-)

-- 
2.30.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 01/11] drm/atomic-helper: Add dma-fence annotations

[Daniel Vetter]

This is a bit disappointing since we need to split the annotations
over all the different parts.

I was considering just leaking the critical section into the
->atomic_commit_tail callback of each driver. But that would mean we
need to pass the fence_cookie into each driver (there's a total of 13
implementations of this hook right now), so bad flag day. And also a
bit leaky abstraction.

Hence just do it function-by-function.

Cc: linux-media@vger.kernel.org
Cc: linaro-mm-sig@lists.linaro.org
Cc: linux-rdma@vger.kernel.org
Cc: amd-gfx@lists.freedesktop.org
Cc: intel-gfx@lists.freedesktop.org
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Christian König <christian.koenig@amd.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 4a66768b6057..69121d2925bd 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1567,6 +1567,7 @@ EXPORT_SYMBOL(drm_atomic_helper_wait_for_flip_done);
 void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
 {
 	struct drm_device *dev = old_state->dev;
+	bool fence_cookie = dma_fence_begin_signalling();
 
 	drm_atomic_helper_commit_modeset_disables(dev, old_state);
 
@@ -1578,6 +1579,8 @@ void drm_atomic_helper_commit_tail(struct drm_atomic_state *old_state)
 
 	drm_atomic_helper_commit_hw_done(old_state);
 
+	dma_fence_end_signalling(fence_cookie);
+
 	drm_atomic_helper_wait_for_vblanks(dev, old_state);
 
 	drm_atomic_helper_cleanup_planes(dev, old_state);
@@ -1597,6 +1600,7 @@ EXPORT_SYMBOL(drm_atomic_helper_commit_tail);
 void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
 {
 	struct drm_device *dev = old_state->dev;
+	bool fence_cookie = dma_fence_begin_signalling();
 
 	drm_atomic_helper_commit_modeset_disables(dev, old_state);
 
@@ -1609,6 +1613,8 @@ void drm_atomic_helper_commit_tail_rpm(struct drm_atomic_state *old_state)
 
 	drm_atomic_helper_commit_hw_done(old_state);
 
+	dma_fence_end_signalling(fence_cookie);
+
 	drm_atomic_helper_wait_for_vblanks(dev, old_state);
 
 	drm_atomic_helper_cleanup_planes(dev, old_state);
@@ -1624,6 +1630,9 @@ static void commit_tail(struct drm_atomic_state *old_state)
 	ktime_t start;
 	s64 commit_time_ms;
 	unsigned int i, new_self_refresh_mask = 0;
+	bool fence_cookie;
+
+	fence_cookie = dma_fence_begin_signalling();
 
 	funcs = dev->mode_config.helper_private;
 
@@ -1652,6 +1661,8 @@ static void commit_tail(struct drm_atomic_state *old_state)
 		if (new_crtc_state->self_refresh_active)
 			new_self_refresh_mask |= BIT(i);
 
+	dma_fence_end_signalling(fence_cookie);
+
 	if (funcs && funcs->atomic_commit_tail)
 		funcs->atomic_commit_tail(old_state);
 	else
@@ -1810,6 +1821,7 @@ int drm_atomic_helper_commit(struct drm_device *dev,
 			     bool nonblock)
 {
 	int ret;
+	bool fence_cookie;
 
 	if (state->async_update) {
 		ret = drm_atomic_helper_prepare_planes(dev, state);
@@ -1832,6 +1844,8 @@ int drm_atomic_helper_commit(struct drm_device *dev,
 	if (ret)
 		return ret;
 
+	fence_cookie = dma_fence_begin_signalling();
+
 	if (!nonblock) {
 		ret = drm_atomic_helper_wait_for_fences(dev, state, true);
 		if (ret)
@@ -1869,6 +1883,7 @@ int drm_atomic_helper_commit(struct drm_device *dev,
 	 */
 
 	drm_atomic_state_get(state);
+	dma_fence_end_signalling(fence_cookie);
 	if (nonblock)
 		queue_work(system_unbound_wq, &state->commit_work);
 	else
@@ -1877,6 +1892,7 @@ int drm_atomic_helper_commit(struct drm_device *dev,
 	return 0;
 
 err:
+	dma_fence_end_signalling(fence_cookie);
 	drm_atomic_helper_cleanup_planes(dev, state);
 	return ret;
 }
-- 
2.30.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 02/11] drm/vkms: Annotate vblank timer

[Daniel Vetter]

This is needed to signal the fences from page flips, annotate it
accordingly. We need to annotate entire timer callback since if we get
stuck anywhere in there, then the timer stops, and hence fences stop.
Just annotating the top part that does the vblank handling isn't
enough.

Tested-by: Melissa Wen <melissa.srw@gmail.com>
Reviewed-by: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
Cc: linux-media@vger.kernel.org
Cc: linaro-mm-sig@lists.linaro.org
Cc: linux-rdma@vger.kernel.org
Cc: amd-gfx@lists.freedesktop.org
Cc: intel-gfx@lists.freedesktop.org
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Christian König <christian.koenig@amd.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
Cc: Haneen Mohammed <hamohammed.sa@gmail.com>
Cc: Daniel Vetter <daniel@ffwll.ch>
---
 drivers/gpu/drm/vkms/vkms_crtc.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vkms/vkms_crtc.c b/drivers/gpu/drm/vkms/vkms_crtc.c
index 0443b7deeaef..6164349cdf11 100644
--- a/drivers/gpu/drm/vkms/vkms_crtc.c
+++ b/drivers/gpu/drm/vkms/vkms_crtc.c
@@ -1,5 +1,7 @@
 // SPDX-License-Identifier: GPL-2.0+
 
+#include <linux/dma-fence.h>
+
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_probe_helper.h>
@@ -14,7 +16,9 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
 	struct drm_crtc *crtc = &output->crtc;
 	struct vkms_crtc_state *state;
 	u64 ret_overrun;
-	bool ret;
+	bool ret, fence_cookie;
+
+	fence_cookie = dma_fence_begin_signalling();
 
 	ret_overrun = hrtimer_forward_now(&output->vblank_hrtimer,
 					  output->period_ns);
@@ -49,6 +53,8 @@ static enum hrtimer_restart vkms_vblank_simulate(struct hrtimer *timer)
 			DRM_DEBUG_DRIVER("Composer worker already queued\n");
 	}
 
+	dma_fence_end_signalling(fence_cookie);
+
 	return HRTIMER_RESTART;
 }
 
-- 
2.30.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 03/11] drm/vblank: Annotate with dma-fence signalling section

[Daniel Vetter]

This is rather overkill since currently all drivers call this from
hardirq (or at least timers). But maybe in the future we're going to
have thread irq handlers and what not, doesn't hurt to be prepared.
Plus this is an easy start for sprinkling these fence annotations into
shared code.

Cc: linux-media@vger.kernel.org
Cc: linaro-mm-sig@lists.linaro.org
Cc: linux-rdma@vger.kernel.org
Cc: amd-gfx@lists.freedesktop.org
Cc: intel-gfx@lists.freedesktop.org
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Christian König <christian.koenig@amd.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_vblank.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_vblank.c b/drivers/gpu/drm/drm_vblank.c
index 30912d8f82a5..f2aeb9bf325f 100644
--- a/drivers/gpu/drm/drm_vblank.c
+++ b/drivers/gpu/drm/drm_vblank.c
@@ -24,6 +24,7 @@
  * OTHER DEALINGS IN THE SOFTWARE.
  */
 
+#include <linux/dma-fence.h>
 #include <linux/export.h>
 #include <linux/kthread.h>
 #include <linux/moduleparam.h>
@@ -1922,7 +1923,7 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe)
 {
 	struct drm_vblank_crtc *vblank = &dev->vblank[pipe];
 	unsigned long irqflags;
-	bool disable_irq;
+	bool disable_irq, fence_cookie;
 
 	if (drm_WARN_ON_ONCE(dev, !drm_dev_has_vblank(dev)))
 		return false;
@@ -1930,6 +1931,8 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe)
 	if (drm_WARN_ON(dev, pipe >= dev->num_crtcs))
 		return false;
 
+	fence_cookie = dma_fence_begin_signalling();
+
 	spin_lock_irqsave(&dev->event_lock, irqflags);
 
 	/* Need timestamp lock to prevent concurrent execution with
@@ -1942,6 +1945,7 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe)
 	if (!vblank->enabled) {
 		spin_unlock(&dev->vblank_time_lock);
 		spin_unlock_irqrestore(&dev->event_lock, irqflags);
+		dma_fence_end_signalling(fence_cookie);
 		return false;
 	}
 
@@ -1968,6 +1972,8 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe)
 	if (disable_irq)
 		vblank_disable_fn(&vblank->disable_timer);
 
+	dma_fence_end_signalling(fence_cookie);
+
 	return true;
 }
 EXPORT_SYMBOL(drm_handle_vblank);
-- 
2.30.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 04/11] drm/komeda: Annotate dma-fence critical section in commit path

[Daniel Vetter]

Like the helpers, nothing special. Well except not, because we the
critical section extends until after hw_done(), since that's the last
thing which could hold up a subsequent atomic commit. That means the
wait_for_flip_done is included, but that's not a problem, we're
allowed to call dma_fence_wait() from signalling critical sections.
Even on our own fence (which this does), it's just a bit confusing.
But in a way those last 2 function calls are already part of the fence
signalling critical section for the next atomic commit.

Reading this I'm wondering why komeda waits for flip_done() before
calling hw_done(), which is a bit backwards (but hey hw can be
special). Might be good to throw a comment in there that explains why,
because the original commit that added this just doesn't.

v2: Small rebase

Reviewed-by: James Qian Wang <james.qian.wang@arm.com> (v1)
Cc: "James (Qian) Wang" <james.qian.wang@arm.com>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Mihail Atanassov <mihail.atanassov@arm.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/arm/display/komeda/komeda_kms.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
index 034ee08482e0..aeda4e5ec4f4 100644
--- a/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
+++ b/drivers/gpu/drm/arm/display/komeda/komeda_kms.c
@@ -73,6 +73,7 @@ static const struct drm_driver komeda_kms_driver = {
 static void komeda_kms_commit_tail(struct drm_atomic_state *old_state)
 {
 	struct drm_device *dev = old_state->dev;
+	bool fence_cookie = dma_fence_begin_signalling();
 
 	drm_atomic_helper_commit_modeset_disables(dev, old_state);
 
@@ -85,6 +86,8 @@ static void komeda_kms_commit_tail(struct drm_atomic_state *old_state)
 
 	drm_atomic_helper_wait_for_flip_done(dev, old_state);
 
+	dma_fence_end_signalling(fence_cookie);
+
 	drm_atomic_helper_cleanup_planes(dev, old_state);
 }
 
-- 
2.30.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 05/11] drm/malidp: Annotate dma-fence critical section in commit path

[Daniel Vetter]

Again needs to be put right after the call to
drm_atomic_helper_commit_hw_done(), since that's the last thing which
can hold up a subsequent atomic commit.

No surprises here.

Acked-by: Liviu Dudau <liviu.dudau@arm.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: "James (Qian) Wang" <james.qian.wang@arm.com>
Cc: Liviu Dudau <liviu.dudau@arm.com>
Cc: Mihail Atanassov <mihail.atanassov@arm.com>
---
 drivers/gpu/drm/arm/malidp_drv.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index fceda010d65a..d83c7366b348 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -234,6 +234,7 @@ static void malidp_atomic_commit_tail(struct drm_atomic_state *state)
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state;
 	int i;
+	bool fence_cookie = dma_fence_begin_signalling();
 
 	pm_runtime_get_sync(drm->dev);
 
@@ -260,6 +261,8 @@ static void malidp_atomic_commit_tail(struct drm_atomic_state *state)
 
 	malidp_atomic_commit_hw_done(state);
 
+	dma_fence_end_signalling(fence_cookie);
+
 	pm_runtime_put(drm->dev);
 
 	drm_atomic_helper_cleanup_planes(drm, state);
-- 
2.30.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 06/11] drm/atmel: Use drm_atomic_helper_commit

[Daniel Vetter]

One of these drivers that predates the nonblocking support in helpers,
and hand-rolled its own thing. Entirely not anything specific here, we
can just delete it all and replace it with the helper version.

Could also perhaps use the drm_mode_config_helper_suspend/resume
stuff, for another few lines deleted. But I'm not looking at that
stuff, I'm just going through all the atomic commit functions and make
sure they have properly annotated dma-fence critical sections
everywhere.

v2:
- Also delete the workqueue (Sam)
- drop the @commit kerneldoc, I missed that one.

Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Boris Brezillon <bbrezillon@kernel.org>
Cc: Nicolas Ferre <nicolas.ferre@microchip.com>
Cc: Alexandre Belloni <alexandre.belloni@bootlin.com>
Cc: Ludovic Desroches <ludovic.desroches@microchip.com>
Cc: linux-arm-kernel@lists.infradead.org
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c | 107 +------------------
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h |   7 --
 2 files changed, 2 insertions(+), 112 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
index 98fb53b75f77..65af56e47129 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c
@@ -557,103 +557,10 @@ static irqreturn_t atmel_hlcdc_dc_irq_handler(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-struct atmel_hlcdc_dc_commit {
-	struct work_struct work;
-	struct drm_device *dev;
-	struct drm_atomic_state *state;
-};
-
-static void
-atmel_hlcdc_dc_atomic_complete(struct atmel_hlcdc_dc_commit *commit)
-{
-	struct drm_device *dev = commit->dev;
-	struct atmel_hlcdc_dc *dc = dev->dev_private;
-	struct drm_atomic_state *old_state = commit->state;
-
-	/* Apply the atomic update. */
-	drm_atomic_helper_commit_modeset_disables(dev, old_state);
-	drm_atomic_helper_commit_planes(dev, old_state, 0);
-	drm_atomic_helper_commit_modeset_enables(dev, old_state);
-
-	drm_atomic_helper_wait_for_vblanks(dev, old_state);
-
-	drm_atomic_helper_cleanup_planes(dev, old_state);
-
-	drm_atomic_state_put(old_state);
-
-	/* Complete the commit, wake up any waiter. */
-	spin_lock(&dc->commit.wait.lock);
-	dc->commit.pending = false;
-	wake_up_all_locked(&dc->commit.wait);
-	spin_unlock(&dc->commit.wait.lock);
-
-	kfree(commit);
-}
-
-static void atmel_hlcdc_dc_atomic_work(struct work_struct *work)
-{
-	struct atmel_hlcdc_dc_commit *commit =
-		container_of(work, struct atmel_hlcdc_dc_commit, work);
-
-	atmel_hlcdc_dc_atomic_complete(commit);
-}
-
-static int atmel_hlcdc_dc_atomic_commit(struct drm_device *dev,
-					struct drm_atomic_state *state,
-					bool async)
-{
-	struct atmel_hlcdc_dc *dc = dev->dev_private;
-	struct atmel_hlcdc_dc_commit *commit;
-	int ret;
-
-	ret = drm_atomic_helper_prepare_planes(dev, state);
-	if (ret)
-		return ret;
-
-	/* Allocate the commit object. */
-	commit = kzalloc(sizeof(*commit), GFP_KERNEL);
-	if (!commit) {
-		ret = -ENOMEM;
-		goto error;
-	}
-
-	INIT_WORK(&commit->work, atmel_hlcdc_dc_atomic_work);
-	commit->dev = dev;
-	commit->state = state;
-
-	spin_lock(&dc->commit.wait.lock);
-	ret = wait_event_interruptible_locked(dc->commit.wait,
-					      !dc->commit.pending);
-	if (ret == 0)
-		dc->commit.pending = true;
-	spin_unlock(&dc->commit.wait.lock);
-
-	if (ret)
-		goto err_free;
-
-	/* We have our own synchronization through the commit lock. */
-	BUG_ON(drm_atomic_helper_swap_state(state, false) < 0);
-
-	/* Swap state succeeded, this is the point of no return. */
-	drm_atomic_state_get(state);
-	if (async)
-		queue_work(dc->wq, &commit->work);
-	else
-		atmel_hlcdc_dc_atomic_complete(commit);
-
-	return 0;
-
-err_free:
-	kfree(commit);
-error:
-	drm_atomic_helper_cleanup_planes(dev, state);
-	return ret;
-}
-
 static const struct drm_mode_config_funcs mode_config_funcs = {
 	.fb_create = drm_gem_fb_create,
 	.atomic_check = drm_atomic_helper_check,
-	.atomic_commit = atmel_hlcdc_dc_atomic_commit,
+	.atomic_commit = drm_atomic_helper_commit,
 };
 
 static int atmel_hlcdc_dc_modeset_init(struct drm_device *dev)
@@ -712,11 +619,6 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev)
 	if (!dc)
 		return -ENOMEM;
 
-	dc->wq = alloc_ordered_workqueue("atmel-hlcdc-dc", 0);
-	if (!dc->wq)
-		return -ENOMEM;
-
-	init_waitqueue_head(&dc->commit.wait);
 	dc->desc = match->data;
 	dc->hlcdc = dev_get_drvdata(dev->dev->parent);
 	dev->dev_private = dc;
@@ -724,7 +626,7 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev)
 	ret = clk_prepare_enable(dc->hlcdc->periph_clk);
 	if (ret) {
 		dev_err(dev->dev, "failed to enable periph_clk\n");
-		goto err_destroy_wq;
+		return ret;
 	}
 
 	pm_runtime_enable(dev->dev);
@@ -761,9 +663,6 @@ static int atmel_hlcdc_dc_load(struct drm_device *dev)
 	pm_runtime_disable(dev->dev);
 	clk_disable_unprepare(dc->hlcdc->periph_clk);
 
-err_destroy_wq:
-	destroy_workqueue(dc->wq);
-
 	return ret;
 }
 
@@ -771,7 +670,6 @@ static void atmel_hlcdc_dc_unload(struct drm_device *dev)
 {
 	struct atmel_hlcdc_dc *dc = dev->dev_private;
 
-	flush_workqueue(dc->wq);
 	drm_kms_helper_poll_fini(dev);
 	drm_atomic_helper_shutdown(dev);
 	drm_mode_config_cleanup(dev);
@@ -784,7 +682,6 @@ static void atmel_hlcdc_dc_unload(struct drm_device *dev)
 
 	pm_runtime_disable(dev->dev);
 	clk_disable_unprepare(dc->hlcdc->periph_clk);
-	destroy_workqueue(dc->wq);
 }
 
 static int atmel_hlcdc_dc_irq_postinstall(struct drm_device *dev)
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
index 469d4507e576..5b5c774e0edf 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
@@ -331,9 +331,7 @@ struct atmel_hlcdc_dc_desc {
  * @crtc: CRTC provided by the display controller
  * @planes: instantiated planes
  * @layers: active HLCDC layers
- * @wq: display controller workqueue
  * @suspend: used to store the HLCDC state when entering suspend
- * @commit: used for async commit handling
  */
 struct atmel_hlcdc_dc {
 	const struct atmel_hlcdc_dc_desc *desc;
@@ -341,15 +339,10 @@ struct atmel_hlcdc_dc {
 	struct atmel_hlcdc *hlcdc;
 	struct drm_crtc *crtc;
 	struct atmel_hlcdc_layer *layers[ATMEL_HLCDC_MAX_LAYERS];
-	struct workqueue_struct *wq;
 	struct {
 		u32 imr;
 		struct drm_atomic_state *state;
 	} suspend;
-	struct {
-		wait_queue_head_t wait;
-		bool pending;
-	} commit;
 };
 
 extern struct atmel_hlcdc_formats atmel_hlcdc_plane_rgb_formats;
-- 
2.30.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 07/11] drm/imx: Annotate dma-fence critical section in commit path

[Daniel Vetter]

drm_atomic_helper_commit_hw_done() is the last thing (no plane cleanup
apparrently), so it's the entire function. And a nice comment
explaining why thw wait_for_flip_done is ahead, unlike the usual
sequence.

Aside, I think since the atomic helpers do track plane disabling now
separately this might no longer be a real problem since:

commit 21a01abbe32a3cbeb903378a24e504bfd9fe0648
Author: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Date:   Mon Sep 4 12:48:37 2017 +0200

    drm/atomic: Fix freeing connector/plane state too early by tracking commits, v3.

Plus the subsequent bugfixes of course, this was tricky to get right.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Philipp Zabel <p.zabel@pengutronix.de>
Cc: Shawn Guo <shawnguo@kernel.org>
Cc: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>
Cc: Fabio Estevam <festevam@gmail.com>
Cc: NXP Linux Team <linux-imx@nxp.com>
Cc: linux-arm-kernel@lists.infradead.org
---
 drivers/gpu/drm/imx/imx-drm-core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/imx/imx-drm-core.c b/drivers/gpu/drm/imx/imx-drm-core.c
index d1a9841adeed..3ed1cc07cad1 100644
--- a/drivers/gpu/drm/imx/imx-drm-core.c
+++ b/drivers/gpu/drm/imx/imx-drm-core.c
@@ -81,6 +81,7 @@ static void imx_drm_atomic_commit_tail(struct drm_atomic_state *state)
 	struct drm_plane_state *old_plane_state, *new_plane_state;
 	bool plane_disabling = false;
 	int i;
+	bool fence_cookie = dma_fence_begin_signalling();
 
 	drm_atomic_helper_commit_modeset_disables(dev, state);
 
@@ -111,6 +112,7 @@ static void imx_drm_atomic_commit_tail(struct drm_atomic_state *state)
 	}
 
 	drm_atomic_helper_commit_hw_done(state);
+	dma_fence_end_signalling(fence_cookie);
 }
 
 static const struct drm_mode_config_helper_funcs imx_drm_mode_config_helpers = {
-- 
2.30.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 08/11] drm/omapdrm: Annotate dma-fence critical section in commit path

[Daniel Vetter]

Nothing special, just put the end right after hw_done(). Note that in
one path there's a wait for the flip/update to complete. But as far as
I understand from comments and code that's only relevant for modesets,
and skipped if there wasn't a modeset done on a given crtc.

For a bit more clarity pull the hw_done() call out of the if/else,
that way it's a bit clearer flow. But happy to shuffle this around as
is seen fit.

Reviewed-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/omapdrm/omap_drv.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/omapdrm/omap_drv.c b/drivers/gpu/drm/omapdrm/omap_drv.c
index 28bbad1353ee..8632139e0f01 100644
--- a/drivers/gpu/drm/omapdrm/omap_drv.c
+++ b/drivers/gpu/drm/omapdrm/omap_drv.c
@@ -68,6 +68,7 @@ static void omap_atomic_commit_tail(struct drm_atomic_state *old_state)
 {
 	struct drm_device *dev = old_state->dev;
 	struct omap_drm_private *priv = dev->dev_private;
+	bool fence_cookie = dma_fence_begin_signalling();
 
 	dispc_runtime_get(priv->dispc);
 
@@ -90,8 +91,6 @@ static void omap_atomic_commit_tail(struct drm_atomic_state *old_state)
 		omap_atomic_wait_for_completion(dev, old_state);
 
 		drm_atomic_helper_commit_planes(dev, old_state, 0);
-
-		drm_atomic_helper_commit_hw_done(old_state);
 	} else {
 		/*
 		 * OMAP3 DSS seems to have issues with the work-around above,
@@ -101,10 +100,12 @@ static void omap_atomic_commit_tail(struct drm_atomic_state *old_state)
 		drm_atomic_helper_commit_planes(dev, old_state, 0);
 
 		drm_atomic_helper_commit_modeset_enables(dev, old_state);
-
-		drm_atomic_helper_commit_hw_done(old_state);
 	}
 
+	drm_atomic_helper_commit_hw_done(old_state);
+
+	dma_fence_end_signalling(fence_cookie);
+
 	/*
 	 * Wait for completion of the page flips to ensure that old buffers
 	 * can't be touched by the hardware anymore before cleaning up planes.
-- 
2.30.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 09/11] drm/rcar-du: Annotate dma-fence critical section in commit path

[Daniel Vetter]

Ends right after drm_atomic_helper_commit_hw_done(), absolutely
nothing fancy going on here.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Kieran Bingham <kieran.bingham+renesas@ideasonboard.com>
Cc: linux-renesas-soc@vger.kernel.org
---
 drivers/gpu/drm/rcar-du/rcar_du_kms.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index fdb8a0d127ad..734e2e614fd2 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -442,6 +442,7 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
 	struct drm_crtc_state *crtc_state;
 	struct drm_crtc *crtc;
 	unsigned int i;
+	bool fence_cookie = dma_fence_begin_signalling();
 
 	/*
 	 * Store RGB routing to DPAD0 and DPAD1, the hardware will be configured
@@ -468,6 +469,7 @@ static void rcar_du_atomic_commit_tail(struct drm_atomic_state *old_state)
 	drm_atomic_helper_commit_modeset_enables(dev, old_state);
 
 	drm_atomic_helper_commit_hw_done(old_state);
+	dma_fence_end_signalling(fence_cookie);
 	drm_atomic_helper_wait_for_flip_done(dev, old_state);
 
 	drm_atomic_helper_cleanup_planes(dev, old_state);
-- 
2.30.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 10/11] drm/tegra: Annotate dma-fence critical section in commit path

[Daniel Vetter]

Again ends just after drm_atomic_helper_commit_hw_done(), but with the
twist that we need to make sure we're only annotate the custom
version. And not the other clause which just calls
drm_atomic_helper_commit_tail_rpm(), which is already annotated.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Thierry Reding <thierry.reding@gmail.com>
Cc: Jonathan Hunter <jonathanh@nvidia.com>
Cc: linux-tegra@vger.kernel.org
---
 drivers/gpu/drm/tegra/drm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/tegra/drm.c b/drivers/gpu/drm/tegra/drm.c
index e45c8414e2a3..ef2b79a903b1 100644
--- a/drivers/gpu/drm/tegra/drm.c
+++ b/drivers/gpu/drm/tegra/drm.c
@@ -65,11 +65,14 @@ static void tegra_atomic_commit_tail(struct drm_atomic_state *old_state)
 	struct tegra_drm *tegra = drm->dev_private;
 
 	if (tegra->hub) {
+		bool fence_cookie = dma_fence_begin_signalling();
+
 		drm_atomic_helper_commit_modeset_disables(drm, old_state);
 		tegra_display_hub_atomic_commit(drm, old_state);
 		drm_atomic_helper_commit_planes(drm, old_state, 0);
 		drm_atomic_helper_commit_modeset_enables(drm, old_state);
 		drm_atomic_helper_commit_hw_done(old_state);
+		dma_fence_end_signalling(fence_cookie);
 		drm_atomic_helper_wait_for_vblanks(drm, old_state);
 		drm_atomic_helper_cleanup_planes(drm, old_state);
 	} else {
-- 
2.30.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 11/11] drm/tidss: Annotate dma-fence critical section in commit path

[Daniel Vetter]

Ends right after hw_done(), totally standard case.

Acked-by: Jyri Sarha <jsarha@ti.com>
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Cc: Jyri Sarha <jsarha@ti.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/gpu/drm/tidss/tidss_kms.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
index 09485c7f0d6f..95f8e0f78e32 100644
--- a/drivers/gpu/drm/tidss/tidss_kms.c
+++ b/drivers/gpu/drm/tidss/tidss_kms.c
@@ -4,6 +4,8 @@
  * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
  */
 
+#include <linux/dma-fence.h>
+
 #include <drm/drm_atomic.h>
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_bridge.h>
@@ -26,6 +28,7 @@ static void tidss_atomic_commit_tail(struct drm_atomic_state *old_state)
 {
 	struct drm_device *ddev = old_state->dev;
 	struct tidss_device *tidss = to_tidss(ddev);
+	bool fence_cookie = dma_fence_begin_signalling();
 
 	dev_dbg(ddev->dev, "%s\n", __func__);
 
@@ -36,6 +39,7 @@ static void tidss_atomic_commit_tail(struct drm_atomic_state *old_state)
 	drm_atomic_helper_commit_modeset_enables(ddev, old_state);
 
 	drm_atomic_helper_commit_hw_done(old_state);
+	dma_fence_end_signalling(fence_cookie);
 	drm_atomic_helper_wait_for_flip_done(ddev, old_state);
 
 	drm_atomic_helper_cleanup_planes(ddev, old_state);
-- 
2.30.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/11] drm/tegra: Annotate dma-fence critical section in commit path

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 709 bytes --]

On Thu, Jan 21, 2021 at 04:29:58PM +0100, Daniel Vetter wrote:
> Again ends just after drm_atomic_helper_commit_hw_done(), but with the
> twist that we need to make sure we're only annotate the custom
> version. And not the other clause which just calls
> drm_atomic_helper_commit_tail_rpm(), which is already annotated.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Cc: Jonathan Hunter <jonathanh@nvidia.com>
> Cc: linux-tegra@vger.kernel.org
> ---
>  drivers/gpu/drm/tegra/drm.c | 3 +++
>  1 file changed, 3 insertions(+)

I assume you want to take this through drm-misc? If so:

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 00/11] dma_fence critical sections annotations for atomic

[Daniel Vetter]

On Thu, Jan 21, 2021 at 04:29:48PM +0100, Daniel Vetter wrote:
> Hi all,
> 
> Finally gotten around to refreshing all the various fence anntotions I've
> hast last summer. Or well parts of it:
> 
> - entire amdgpu and drm/scheduler annotations postponed for now, because
>   there's way too many splats in there that need some work
> 
> - in recent patches I've seen quite a few dma_resv_lock or kmalloc in
>   atomic_commit_tail, which doesn't work in full generality with the rules
>   for dma_fences we've discussed and encoded in lockdep. These annotations
>   should catch stuff like this.
> 
> Review comments and testing very much welcome.

I pulled in the acked/reviewed patches into drm-misc-next. Testing (enable
lockdep, test your driver) and review on the others very much appreciated.

Thanks, Daniel

> 
> Cheers, Daniel
> 
> Daniel Vetter (11):
>   drm/atomic-helper: Add dma-fence annotations
>   drm/vkms: Annotate vblank timer
>   drm/vblank: Annotate with dma-fence signalling section
>   drm/komeda: Annotate dma-fence critical section in commit path
>   drm/malidp: Annotate dma-fence critical section in commit path
>   drm/atmel: Use drm_atomic_helper_commit
>   drm/imx: Annotate dma-fence critical section in commit path
>   drm/omapdrm: Annotate dma-fence critical section in commit path
>   drm/rcar-du: Annotate dma-fence critical section in commit path
>   drm/tegra: Annotate dma-fence critical section in commit path
>   drm/tidss: Annotate dma-fence critical section in commit path
> 
>  .../gpu/drm/arm/display/komeda/komeda_kms.c   |   3 +
>  drivers/gpu/drm/arm/malidp_drv.c              |   3 +
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.c  | 107 +-----------------
>  drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h  |   7 --
>  drivers/gpu/drm/drm_atomic_helper.c           |  16 +++
>  drivers/gpu/drm/drm_vblank.c                  |   8 +-
>  drivers/gpu/drm/imx/imx-drm-core.c            |   2 +
>  drivers/gpu/drm/omapdrm/omap_drv.c            |   9 +-
>  drivers/gpu/drm/rcar-du/rcar_du_kms.c         |   2 +
>  drivers/gpu/drm/tegra/drm.c                   |   3 +
>  drivers/gpu/drm/tidss/tidss_kms.c             |   4 +
>  drivers/gpu/drm/vkms/vkms_crtc.c              |   8 +-
>  12 files changed, 54 insertions(+), 118 deletions(-)
> 
> -- 
> 2.30.0
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 11/11] drm/tidss: Annotate dma-fence critical section in commit path

[Tomi Valkeinen]

Hi Daniel,

On 21/01/2021 17:29, Daniel Vetter wrote:
> Ends right after hw_done(), totally standard case.
> 
> Acked-by: Jyri Sarha <jsarha@ti.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> Cc: Jyri Sarha <jsarha@ti.com>
> Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> ---
>   drivers/gpu/drm/tidss/tidss_kms.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
> index 09485c7f0d6f..95f8e0f78e32 100644
> --- a/drivers/gpu/drm/tidss/tidss_kms.c
> +++ b/drivers/gpu/drm/tidss/tidss_kms.c
> @@ -4,6 +4,8 @@
>    * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
>    */
>   
> +#include <linux/dma-fence.h>
> +
>   #include <drm/drm_atomic.h>
>   #include <drm/drm_atomic_helper.h>
>   #include <drm/drm_bridge.h>
> @@ -26,6 +28,7 @@ static void tidss_atomic_commit_tail(struct drm_atomic_state *old_state)
>   {
>   	struct drm_device *ddev = old_state->dev;
>   	struct tidss_device *tidss = to_tidss(ddev);
> +	bool fence_cookie = dma_fence_begin_signalling();
>   
>   	dev_dbg(ddev->dev, "%s\n", __func__);
>   
> @@ -36,6 +39,7 @@ static void tidss_atomic_commit_tail(struct drm_atomic_state *old_state)
>   	drm_atomic_helper_commit_modeset_enables(ddev, old_state);
>   
>   	drm_atomic_helper_commit_hw_done(old_state);
> +	dma_fence_end_signalling(fence_cookie);
>   	drm_atomic_helper_wait_for_flip_done(ddev, old_state);
>   
>   	drm_atomic_helper_cleanup_planes(ddev, old_state);
> 

I bisected v5.13 rc lockdep warnings to this patch. I see this with tidss (lockdep
report below) and omapdrm (probably caused by "drm/omapdrm: Annotate dma-fence critical
section in commit path"). The report on omapdrm is very similar, with fs_reclaim and
dma_fence_map, but hdmi bridge instead of mhdp bridge.

I'm unfamiliar with this piece of drm code, do you have any hints on what the problem
might be and where I should be looking at?

  Tomi

[   20.769286] ======================================================
[   20.776918] WARNING: possible circular locking dependency detected
[   20.783082] 5.11.0-rc2-00688-g4d56a4f08391-dirty #18 Not tainted
[   20.789072] ------------------------------------------------------
[   20.795232] kmstest/397 is trying to acquire lock:
[   20.800008] ffff800011637878 (fs_reclaim){+.+.}-{0:0}, at: fs_reclaim_acquire+0x40/0xa4
[   20.808014]
[   20.808014] but task is already holding lock:
[   20.813828] ffff000804fb41a8 (&mhdp->link_mutex){+.+.}-{3:3}, at: cdns_mhdp_atomic_enable+0x54/0x2c0 [cdns_mhdp8546]
[   20.824343]
[   20.824343] which lock already depends on the new lock.
[   20.824343]
[   20.832497]
[   20.832497] the existing dependency chain (in reverse order) is:
[   20.839958]
[   20.839958] -> #2 (&mhdp->link_mutex){+.+.}-{3:3}:
[   20.846214]        lock_acquire.part.0+0x178/0x380
[   20.850999]        lock_acquire+0x6c/0x90
[   20.854996]        __mutex_lock+0x9c/0x540
[   20.859083]        mutex_lock_nested+0x44/0x70
[   20.863513]        cdns_mhdp_atomic_enable+0x54/0x2c0 [cdns_mhdp8546]
[   20.869938]        drm_atomic_bridge_chain_enable+0x60/0xd4 [drm]
[   20.876093]        drm_atomic_helper_commit_modeset_enables+0x148/0x260 [drm_kms_helper]
[   20.884207]        tidss_atomic_commit_tail+0x64/0xd0 [tidss]
[   20.889947]        commit_tail+0xac/0x190 [drm_kms_helper]
[   20.895445]        drm_atomic_helper_commit+0x194/0x3a0 [drm_kms_helper]
[   20.902155]        drm_atomic_commit+0x58/0x70 [drm]
[   20.907165]        drm_mode_atomic_ioctl+0x9f0/0xbbc [drm]
[   20.912693]        drm_ioctl_kernel+0xd0/0x124 [drm]
[   20.917701]        drm_ioctl+0x238/0x460 [drm]
[   20.922189]        __arm64_sys_ioctl+0xb4/0x100
[   20.926707]        el0_svc_common.constprop.0+0x80/0x1e0
[   20.932007]        do_el0_svc+0x30/0xa0
[   20.935831]        el0_svc+0x20/0x30
[   20.939394]        el0_sync_handler+0x1a8/0x1b0
[   20.943910]        el0_sync+0x174/0x180
[   20.947734]
[   20.947734] -> #1 (dma_fence_map){++++}-{0:0}:
[   20.953644]        lock_acquire.part.0+0x178/0x380
[   20.958422]        lock_acquire+0x6c/0x90
[   20.962418]        __dma_fence_might_wait+0x60/0xdc
[   20.967285]        dma_resv_lockdep+0x1dc/0x2b4
[   20.971803]        do_one_initcall+0x90/0x460
[   20.976145]        kernel_init_freeable+0x2c0/0x32c
[   20.981012]        kernel_init+0x20/0x128
[   20.985007]        ret_from_fork+0x10/0x3c
[   20.989091]
[   20.989091] -> #0 (fs_reclaim){+.+.}-{0:0}:
[   20.994740]        check_noncircular+0x164/0x180
[   20.999344]        __lock_acquire+0x13dc/0x1be4
[   21.003861]        lock_acquire.part.0+0x178/0x380
[   21.008639]        lock_acquire+0x6c/0x90
[   21.012635]        fs_reclaim_acquire+0x7c/0xa4
[   21.017152]        kmem_cache_alloc_trace+0x7c/0x3c0
[   21.022103]        drm_mode_duplicate+0x34/0x70 [drm]
[   21.027199]        cdns_mhdp_atomic_enable+0x1c4/0x2c0 [cdns_mhdp8546]
[   21.033711]        drm_atomic_bridge_chain_enable+0x60/0xd4 [drm]
[   21.039845]        drm_atomic_helper_commit_modeset_enables+0x148/0x260 [drm_kms_helper]
[   21.047946]        tidss_atomic_commit_tail+0x64/0xd0 [tidss]
[   21.053681]        commit_tail+0xac/0x190 [drm_kms_helper]
[   21.059179]        drm_atomic_helper_commit+0x194/0x3a0 [drm_kms_helper]
[   21.065889]        drm_atomic_commit+0x58/0x70 [drm]
[   21.070897]        drm_mode_atomic_ioctl+0x9f0/0xbbc [drm]
[   21.076423]        drm_ioctl_kernel+0xd0/0x124 [drm]
[   21.081430]        drm_ioctl+0x238/0x460 [drm]
[   21.085917]        __arm64_sys_ioctl+0xb4/0x100
[   21.090434]        el0_svc_common.constprop.0+0x80/0x1e0
[   21.095730]        do_el0_svc+0x30/0xa0
[   21.099554]        el0_svc+0x20/0x30
[   21.103117]        el0_sync_handler+0x1a8/0x1b0
[   21.107632]        el0_sync+0x174/0x180
[   21.111454]
[   21.111454] other info that might help us debug this:
[   21.111454]
[   21.119435] Chain exists of:
[   21.119435]   fs_reclaim --> dma_fence_map --> &mhdp->link_mutex
[   21.119435]
[   21.129768]  Possible unsafe locking scenario:
[   21.129768]
[   21.135670]        CPU0                    CPU1
[   21.140184]        ----                    ----
[   21.144698]   lock(&mhdp->link_mutex);
[   21.148435]                                lock(dma_fence_map);
[   21.154340]                                lock(&mhdp->link_mutex);
[   21.160591]   lock(fs_reclaim);
[   21.163721]
[   21.163721]  *** DEADLOCK ***
[   21.163721]
[   21.169623] 4 locks held by kmstest/397:
[   21.173532]  #0: ffff8000145efc28 (crtc_ww_class_acquire){+.+.}-{0:0}, at: drm_modeset_acquire_init+0x48/0x70 [drm]
[   21.184011]  #1: ffff000805dc7888 (crtc_ww_class_mutex){+.+.}-{3:3}, at: drm_modeset_lock+0x138/0x170 [drm]
[   21.193795]  #2: ffff8000116d53f8 (dma_fence_map){++++}-{0:0}, at: tidss_atomic_commit_tail+0x2c/0xd0 [tidss]
[   21.203701]  #3: ffff000804fb41a8 (&mhdp->link_mutex){+.+.}-{3:3}, at: cdns_mhdp_atomic_enable+0x54/0x2c0 [cdns_mhdp8546]
[   21.214642]
[   21.214642] stack backtrace:
[   21.218984] CPU: 1 PID: 397 Comm: kmstest Not tainted 5.11.0-rc2-00688-g4d56a4f08391-dirty #18
[   21.227575] Hardware name: Texas Instruments K3 J721E SoC (DT)
[   21.233390] Call trace:
[   21.235825]  dump_backtrace+0x0/0x1f0
[   21.239476]  show_stack+0x24/0x80
[   21.242781]  dump_stack+0xec/0x154
[   21.246170]  print_circular_bug+0x1f8/0x200
[   21.250341]  check_noncircular+0x164/0x180
[   21.254426]  __lock_acquire+0x13dc/0x1be4
[   21.258422]  lock_acquire.part.0+0x178/0x380
[   21.262679]  lock_acquire+0x6c/0x90
[   21.266155]  fs_reclaim_acquire+0x7c/0xa4
[   21.270152]  kmem_cache_alloc_trace+0x7c/0x3c0
[   21.274582]  drm_mode_duplicate+0x34/0x70 [drm]
[   21.279159]  cdns_mhdp_atomic_enable+0x1c4/0x2c0 [cdns_mhdp8546]
[   21.285151]  drm_atomic_bridge_chain_enable+0x60/0xd4 [drm]
[   21.290764]  drm_atomic_helper_commit_modeset_enables+0x148/0x260 [drm_kms_helper]
[   21.298343]  tidss_atomic_commit_tail+0x64/0xd0 [tidss]
[   21.303559]  commit_tail+0xac/0x190 [drm_kms_helper]
[   21.308535]  drm_atomic_helper_commit+0x194/0x3a0 [drm_kms_helper]
[   21.314725]  drm_atomic_commit+0x58/0x70 [drm]
[   21.319214]  drm_mode_atomic_ioctl+0x9f0/0xbbc [drm]
[   21.324220]  drm_ioctl_kernel+0xd0/0x124 [drm]
[   21.328708]  drm_ioctl+0x238/0x460 [drm]
[   21.332676]  __arm64_sys_ioctl+0xb4/0x100
[   21.336672]  el0_svc_common.constprop.0+0x80/0x1e0
[   21.341449]  do_el0_svc+0x30/0xa0
[   21.344753]  el0_svc+0x20/0x30
[   21.347795]  el0_sync_handler+0x1a8/0x1b0
[   21.351790]  el0_sync+0x174/0x180

Re: [PATCH 11/11] drm/tidss: Annotate dma-fence critical section in commit path

[Daniel Vetter]

On Thu, May 27, 2021 at 03:15:34PM +0300, Tomi Valkeinen wrote:
> Hi Daniel,
> 
> On 21/01/2021 17:29, Daniel Vetter wrote:
> > Ends right after hw_done(), totally standard case.
> > 
> > Acked-by: Jyri Sarha <jsarha@ti.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > Cc: Jyri Sarha <jsarha@ti.com>
> > Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
> > ---
> >   drivers/gpu/drm/tidss/tidss_kms.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/tidss/tidss_kms.c b/drivers/gpu/drm/tidss/tidss_kms.c
> > index 09485c7f0d6f..95f8e0f78e32 100644
> > --- a/drivers/gpu/drm/tidss/tidss_kms.c
> > +++ b/drivers/gpu/drm/tidss/tidss_kms.c
> > @@ -4,6 +4,8 @@
> >    * Author: Tomi Valkeinen <tomi.valkeinen@ti.com>
> >    */
> > +#include <linux/dma-fence.h>
> > +
> >   #include <drm/drm_atomic.h>
> >   #include <drm/drm_atomic_helper.h>
> >   #include <drm/drm_bridge.h>
> > @@ -26,6 +28,7 @@ static void tidss_atomic_commit_tail(struct drm_atomic_state *old_state)
> >   {
> >   	struct drm_device *ddev = old_state->dev;
> >   	struct tidss_device *tidss = to_tidss(ddev);
> > +	bool fence_cookie = dma_fence_begin_signalling();
> >   	dev_dbg(ddev->dev, "%s\n", __func__);
> > @@ -36,6 +39,7 @@ static void tidss_atomic_commit_tail(struct drm_atomic_state *old_state)
> >   	drm_atomic_helper_commit_modeset_enables(ddev, old_state);
> >   	drm_atomic_helper_commit_hw_done(old_state);
> > +	dma_fence_end_signalling(fence_cookie);
> >   	drm_atomic_helper_wait_for_flip_done(ddev, old_state);
> >   	drm_atomic_helper_cleanup_planes(ddev, old_state);
> > 
> 
> I bisected v5.13 rc lockdep warnings to this patch. I see this with tidss (lockdep
> report below) and omapdrm (probably caused by "drm/omapdrm: Annotate dma-fence critical
> section in commit path"). The report on omapdrm is very similar, with fs_reclaim and
> dma_fence_map, but hdmi bridge instead of mhdp bridge.
> 
> I'm unfamiliar with this piece of drm code, do you have any hints on what the problem
> might be and where I should be looking at?

This entirely fell through cracks.

tldr; Strictly speaking, anything which can hold up a dma_fence from
signalling is not allowed to allocate memory. There's some limitations to
this (GFP_NORECLAIM is ok), but that's the gist. And atomic commits can in
theory return a dma_fence that will signal when it's all done, in practice
it's for Android only.

This is somewhat the reason why I've not pushed this further, it's a very
drastic limitation. Also I've been snowed under with tons of issues around
dma-fence from the i915 side.

Now the issue still is that there's some real deadlocks here, and it would
be nice to annotate them all. Otoh not being able to allocate any memory
(or take locks that in other paths are held while allocating memory, which
is equivalent) is really tough, and definitely not feasible for full
modesets. So the pragmatic fix I think would be to limit the dma_fence
OUT-FENCE for Android to only the page-flip case, similarly limit our
annotations. And if you do an OUT-FENCE which includes a modeset, we just
fall back to a synchronous modeset. The problem can only happen if
userspace (and hence some other kernel thread) can get at the out
dma_fence before we've finished all the work that might need allocations
and what not else.

If you're bored maybe hack this up?

Also I'm kinda disappointed since in the cover letter I asked explicitly
for testing with lockdep, since I somewhat expected issues like this. But
people just blindly throw acks around without looking at the bigger
picture.

Cheers, Daniel

> 
>  Tomi
> 
> [   20.769286] ======================================================
> [   20.776918] WARNING: possible circular locking dependency detected
> [   20.783082] 5.11.0-rc2-00688-g4d56a4f08391-dirty #18 Not tainted
> [   20.789072] ------------------------------------------------------
> [   20.795232] kmstest/397 is trying to acquire lock:
> [   20.800008] ffff800011637878 (fs_reclaim){+.+.}-{0:0}, at: fs_reclaim_acquire+0x40/0xa4
> [   20.808014]
> [   20.808014] but task is already holding lock:
> [   20.813828] ffff000804fb41a8 (&mhdp->link_mutex){+.+.}-{3:3}, at: cdns_mhdp_atomic_enable+0x54/0x2c0 [cdns_mhdp8546]
> [   20.824343]
> [   20.824343] which lock already depends on the new lock.
> [   20.824343]
> [   20.832497]
> [   20.832497] the existing dependency chain (in reverse order) is:
> [   20.839958]
> [   20.839958] -> #2 (&mhdp->link_mutex){+.+.}-{3:3}:
> [   20.846214]        lock_acquire.part.0+0x178/0x380
> [   20.850999]        lock_acquire+0x6c/0x90
> [   20.854996]        __mutex_lock+0x9c/0x540
> [   20.859083]        mutex_lock_nested+0x44/0x70
> [   20.863513]        cdns_mhdp_atomic_enable+0x54/0x2c0 [cdns_mhdp8546]
> [   20.869938]        drm_atomic_bridge_chain_enable+0x60/0xd4 [drm]
> [   20.876093]        drm_atomic_helper_commit_modeset_enables+0x148/0x260 [drm_kms_helper]
> [   20.884207]        tidss_atomic_commit_tail+0x64/0xd0 [tidss]
> [   20.889947]        commit_tail+0xac/0x190 [drm_kms_helper]
> [   20.895445]        drm_atomic_helper_commit+0x194/0x3a0 [drm_kms_helper]
> [   20.902155]        drm_atomic_commit+0x58/0x70 [drm]
> [   20.907165]        drm_mode_atomic_ioctl+0x9f0/0xbbc [drm]
> [   20.912693]        drm_ioctl_kernel+0xd0/0x124 [drm]
> [   20.917701]        drm_ioctl+0x238/0x460 [drm]
> [   20.922189]        __arm64_sys_ioctl+0xb4/0x100
> [   20.926707]        el0_svc_common.constprop.0+0x80/0x1e0
> [   20.932007]        do_el0_svc+0x30/0xa0
> [   20.935831]        el0_svc+0x20/0x30
> [   20.939394]        el0_sync_handler+0x1a8/0x1b0
> [   20.943910]        el0_sync+0x174/0x180
> [   20.947734]
> [   20.947734] -> #1 (dma_fence_map){++++}-{0:0}:
> [   20.953644]        lock_acquire.part.0+0x178/0x380
> [   20.958422]        lock_acquire+0x6c/0x90
> [   20.962418]        __dma_fence_might_wait+0x60/0xdc
> [   20.967285]        dma_resv_lockdep+0x1dc/0x2b4
> [   20.971803]        do_one_initcall+0x90/0x460
> [   20.976145]        kernel_init_freeable+0x2c0/0x32c
> [   20.981012]        kernel_init+0x20/0x128
> [   20.985007]        ret_from_fork+0x10/0x3c
> [   20.989091]
> [   20.989091] -> #0 (fs_reclaim){+.+.}-{0:0}:
> [   20.994740]        check_noncircular+0x164/0x180
> [   20.999344]        __lock_acquire+0x13dc/0x1be4
> [   21.003861]        lock_acquire.part.0+0x178/0x380
> [   21.008639]        lock_acquire+0x6c/0x90
> [   21.012635]        fs_reclaim_acquire+0x7c/0xa4
> [   21.017152]        kmem_cache_alloc_trace+0x7c/0x3c0
> [   21.022103]        drm_mode_duplicate+0x34/0x70 [drm]
> [   21.027199]        cdns_mhdp_atomic_enable+0x1c4/0x2c0 [cdns_mhdp8546]
> [   21.033711]        drm_atomic_bridge_chain_enable+0x60/0xd4 [drm]
> [   21.039845]        drm_atomic_helper_commit_modeset_enables+0x148/0x260 [drm_kms_helper]
> [   21.047946]        tidss_atomic_commit_tail+0x64/0xd0 [tidss]
> [   21.053681]        commit_tail+0xac/0x190 [drm_kms_helper]
> [   21.059179]        drm_atomic_helper_commit+0x194/0x3a0 [drm_kms_helper]
> [   21.065889]        drm_atomic_commit+0x58/0x70 [drm]
> [   21.070897]        drm_mode_atomic_ioctl+0x9f0/0xbbc [drm]
> [   21.076423]        drm_ioctl_kernel+0xd0/0x124 [drm]
> [   21.081430]        drm_ioctl+0x238/0x460 [drm]
> [   21.085917]        __arm64_sys_ioctl+0xb4/0x100
> [   21.090434]        el0_svc_common.constprop.0+0x80/0x1e0
> [   21.095730]        do_el0_svc+0x30/0xa0
> [   21.099554]        el0_svc+0x20/0x30
> [   21.103117]        el0_sync_handler+0x1a8/0x1b0
> [   21.107632]        el0_sync+0x174/0x180
> [   21.111454]
> [   21.111454] other info that might help us debug this:
> [   21.111454]
> [   21.119435] Chain exists of:
> [   21.119435]   fs_reclaim --> dma_fence_map --> &mhdp->link_mutex
> [   21.119435]
> [   21.129768]  Possible unsafe locking scenario:
> [   21.129768]
> [   21.135670]        CPU0                    CPU1
> [   21.140184]        ----                    ----
> [   21.144698]   lock(&mhdp->link_mutex);
> [   21.148435]                                lock(dma_fence_map);
> [   21.154340]                                lock(&mhdp->link_mutex);
> [   21.160591]   lock(fs_reclaim);
> [   21.163721]
> [   21.163721]  *** DEADLOCK ***
> [   21.163721]
> [   21.169623] 4 locks held by kmstest/397:
> [   21.173532]  #0: ffff8000145efc28 (crtc_ww_class_acquire){+.+.}-{0:0}, at: drm_modeset_acquire_init+0x48/0x70 [drm]
> [   21.184011]  #1: ffff000805dc7888 (crtc_ww_class_mutex){+.+.}-{3:3}, at: drm_modeset_lock+0x138/0x170 [drm]
> [   21.193795]  #2: ffff8000116d53f8 (dma_fence_map){++++}-{0:0}, at: tidss_atomic_commit_tail+0x2c/0xd0 [tidss]
> [   21.203701]  #3: ffff000804fb41a8 (&mhdp->link_mutex){+.+.}-{3:3}, at: cdns_mhdp_atomic_enable+0x54/0x2c0 [cdns_mhdp8546]
> [   21.214642]
> [   21.214642] stack backtrace:
> [   21.218984] CPU: 1 PID: 397 Comm: kmstest Not tainted 5.11.0-rc2-00688-g4d56a4f08391-dirty #18
> [   21.227575] Hardware name: Texas Instruments K3 J721E SoC (DT)
> [   21.233390] Call trace:
> [   21.235825]  dump_backtrace+0x0/0x1f0
> [   21.239476]  show_stack+0x24/0x80
> [   21.242781]  dump_stack+0xec/0x154
> [   21.246170]  print_circular_bug+0x1f8/0x200
> [   21.250341]  check_noncircular+0x164/0x180
> [   21.254426]  __lock_acquire+0x13dc/0x1be4
> [   21.258422]  lock_acquire.part.0+0x178/0x380
> [   21.262679]  lock_acquire+0x6c/0x90
> [   21.266155]  fs_reclaim_acquire+0x7c/0xa4
> [   21.270152]  kmem_cache_alloc_trace+0x7c/0x3c0
> [   21.274582]  drm_mode_duplicate+0x34/0x70 [drm]
> [   21.279159]  cdns_mhdp_atomic_enable+0x1c4/0x2c0 [cdns_mhdp8546]
> [   21.285151]  drm_atomic_bridge_chain_enable+0x60/0xd4 [drm]
> [   21.290764]  drm_atomic_helper_commit_modeset_enables+0x148/0x260 [drm_kms_helper]
> [   21.298343]  tidss_atomic_commit_tail+0x64/0xd0 [tidss]
> [   21.303559]  commit_tail+0xac/0x190 [drm_kms_helper]
> [   21.308535]  drm_atomic_helper_commit+0x194/0x3a0 [drm_kms_helper]
> [   21.314725]  drm_atomic_commit+0x58/0x70 [drm]
> [   21.319214]  drm_mode_atomic_ioctl+0x9f0/0xbbc [drm]
> [   21.324220]  drm_ioctl_kernel+0xd0/0x124 [drm]
> [   21.328708]  drm_ioctl+0x238/0x460 [drm]
> [   21.332676]  __arm64_sys_ioctl+0xb4/0x100
> [   21.336672]  el0_svc_common.constprop.0+0x80/0x1e0
> [   21.341449]  do_el0_svc+0x30/0xa0
> [   21.344753]  el0_svc+0x20/0x30
> [   21.347795]  el0_sync_handler+0x1a8/0x1b0
> [   21.351790]  el0_sync+0x174/0x180

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch