Warning at drivers/gpu/drm/drm_atomic_helper.c

Commit Message

Lucas Stach Jan. 25, 2017, 11:30 a.m. UTC

Am Mittwoch, den 25.01.2017, 09:20 -0200 schrieb Fabio Estevam:
> Hi Lucas,
> 
> On Wed, Jan 25, 2017 at 8:43 AM, Lucas Stach <[e-mail]> wrote:
> 
> > This only fixes the issue, as with this change we never attach fences to
> > the plane state. I've looked into this issue a bit and if I'm not
> > mistaken, this should still be reproducible with 4.10-rc. Correct?
> 
> I could not reproduce this kernel warning with 4.10-rc, as it does not
> use imx_drm_atomic_commit() anymore.
> 
> The problem happens with 4.9.x stable, which uses imx_drm_atomic_commit().

Kernel 4.10 just moves the fence attach to the plane state. It has
nothing to do with the used commit function. The issue going away if you
change that on kernel 4.9 is a red herring.

In any case, can you try the attached patch? It's against 4.10, if you
want to test it on 4.9 you need to replace dma_fence_put(), with
fence_put().

Regards,
Lucas

Comments

Fabio Estevam Jan. 25, 2017, 11:47 a.m. UTC | #1

On Wed, Jan 25, 2017 at 9:30 AM, Lucas Stach <[e-mail]> wrote:

> Kernel 4.10 just moves the fence attach to the plane state. It has
> nothing to do with the used commit function. The issue going away if you
> change that on kernel 4.9 is a red herring.
>
> In any case, can you try the attached patch? It's against 4.10, if you
> want to test it on 4.9 you need to replace dma_fence_put(), with
> fence_put().

Just tested it on 4.9 (using fence_put()) and I can still get the
warning sometimes:
http://pastebin.com/p6acMLcY

Lucas Stach Jan. 27, 2017, 10:34 a.m. UTC | #2

Am Mittwoch, den 25.01.2017, 09:47 -0200 schrieb Fabio Estevam:
> On Wed, Jan 25, 2017 at 9:30 AM, Lucas Stach <[e-mail]> wrote:
> 
> > Kernel 4.10 just moves the fence attach to the plane state. It has
> > nothing to do with the used commit function. The issue going away if you
> > change that on kernel 4.9 is a red herring.
> >
> > In any case, can you try the attached patch? It's against 4.10, if you
> > want to test it on 4.9 you need to replace dma_fence_put(), with
> > fence_put().
> 
> Just tested it on 4.9 (using fence_put()) and I can still get the
> warning sometimes:
> http://pastebin.com/p6acMLcY
> 
Okay, after looking more at the 4.9 code, I understand what's going on.
I've just sent out a patch to fix this issue for kernel 4.9.

Regards,
Lucas

Patch

From ceaa7ab6c87ddf231024115c123868507b6d9e75 Mon Sep 17 00:00:00 2001
From: Lucas Stach <[e-mail]>
Date: Wed, 25 Jan 2017 12:12:00 +0100
Subject: [PATCH] drm/atomic: clean up fences when commit fails before fence
 wait finished

The plane_state fences are only unreferenced when the wait for them
has finished successfully. If the commit fails before or at that point
(abort from userspace) the fences are kept referenced.

As the drivers are not responsible for fence cleanup when the commit is
successful, the helper should also clean them up in the failing case.

Signed-off-by: Lucas Stach <[e-mail]>
---
 drivers/gpu/drm/drm_atomic_helper.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 34f757bcabae..2fb2aa78773e 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1957,6 +1957,17 @@  void drm_atomic_helper_cleanup_planes(struct drm_device *dev,
 		if (!drm_atomic_helper_framebuffer_changed(dev, old_state, plane_state->crtc))
 			continue;
 
+		/*
+		 * If the atomic update failed before waiting for fences
+		 * finished, the fences are kept referenced in the state. As the
+		 * drivers are not responsible for cleaning up fences when the
+		 * commit is successful, mirror this behavior when it fails.
+		 */
+		if (plane_state->fence) {
+			dma_fence_put(plane_state->fence);
+			plane_state->fence = NULL;
+		}
+
 		funcs = plane->helper_private;
 
 		if (funcs->cleanup_fb)
-- 
2.11.0