[PATCH 1/2] drm: refernce count event->completion

[PATCH 1/2] drm: refernce count event->completion

[Daniel Vetter]

When writing the generic nonblocking commit code I assumed that
through clever lifetime management I can assure that the completion
(stored in drm_crtc_commit) only gets freed after it is completed. And
that worked.

I also wanted to make nonblocking helpers resilient against driver
bugs, by having timeouts everywhere. And that worked too.

Unfortunately taking boths things together results in oopses :( Well,
at least sometimes: What seems to happen is that the drm event hangs
around forever stuck in limbo land. The nonblocking helpers eventually
time out, move on and release it. Now the bug I tested all this
against is drivers that just entirely fail to deliver the vblank
events like they should, and in those cases the event is simply
leaked. But what seems to happen, at least sometimes, on i915 is that
the event is set up correctly, but somohow the vblank fails to fire in
time. Which means the event isn't leaked, it's still there waiting for
eventually a vblank to fire. That tends to happen when re-enabling the
pipe, and then the trap springs and the kernel oopses.

The correct fix here is simply to refcount the crtc commit to make
sure that the event sticks around even for drivers which only
sometimes fail to deliver vblanks for some arbitrary reasons. Since
crtc commits are already refcounted that's easy to do.

References: https://bugs.freedesktop.org/show_bug.cgi?id=96781
Cc: Jim Rees <rees@umich.edu>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: stable@vger.kernel.org
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 11 +++++++++++
 drivers/gpu/drm/drm_fops.c          |  2 +-
 include/drm/drmP.h                  |  1 +
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 799c1564a4f8..b4dfd1e1a4f0 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1355,6 +1355,15 @@ static int stall_checks(struct drm_crtc *crtc, bool nonblock)
 	return ret < 0 ? ret : 0;
 }
 
+void release_crtc_commit(struct completion *completion)
+{
+	struct drm_crtc_commit *commit = container_of(completion,
+						      typeof(*commit),
+						      flip_done);
+
+	drm_crtc_commit_put(commit);
+}
+
 /**
  * drm_atomic_helper_setup_commit - setup possibly nonblocking commit
  * @state: new modeset state to be committed
@@ -1447,6 +1456,8 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 		}
 
 		crtc_state->event->base.completion = &commit->flip_done;
+		crtc_state->event->base.completion_release = release_crtc_commit;
+		drm_crtc_commit_get(commit);
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 48e106557c92..e22645375e60 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -689,8 +689,8 @@ void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e)
 	assert_spin_locked(&dev->event_lock);
 
 	if (e->completion) {
-		/* ->completion might disappear as soon as it signalled. */
 		complete_all(e->completion);
+		e->completion_release(e->completion);
 		e->completion = NULL;
 	}
 
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index a9cfd33c7b1a..e821a8f142d9 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -360,6 +360,7 @@ struct drm_ioctl_desc {
 /* Event queued up for userspace to read */
 struct drm_pending_event {
 	struct completion *completion;
+	void (*completion_release)(struct completion *completion);
 	struct drm_event *event;
 	struct dma_fence *fence;
 	struct list_head link;
-- 
2.11.0

[PATCH 1/2] drm: refernce count event->completion

[Daniel Vetter]

When writing the generic nonblocking commit code I assumed that
through clever lifetime management I can assure that the completion
(stored in drm_crtc_commit) only gets freed after it is completed. And
that worked.

I also wanted to make nonblocking helpers resilient against driver
bugs, by having timeouts everywhere. And that worked too.

Unfortunately taking boths things together results in oopses :( Well,
at least sometimes: What seems to happen is that the drm event hangs
around forever stuck in limbo land. The nonblocking helpers eventually
time out, move on and release it. Now the bug I tested all this
against is drivers that just entirely fail to deliver the vblank
events like they should, and in those cases the event is simply
leaked. But what seems to happen, at least sometimes, on i915 is that
the event is set up correctly, but somohow the vblank fails to fire in
time. Which means the event isn't leaked, it's still there waiting for
eventually a vblank to fire. That tends to happen when re-enabling the
pipe, and then the trap springs and the kernel oopses.

The correct fix here is simply to refcount the crtc commit to make
sure that the event sticks around even for drivers which only
sometimes fail to deliver vblanks for some arbitrary reasons. Since
crtc commits are already refcounted that's easy to do.

References: https://bugs.freedesktop.org/show_bug.cgi?id=96781
Cc: Jim Rees <rees@umich.edu>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Jani Nikula <jani.nikula@linux.intel.com>
Cc: stable@vger.kernel.org
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic_helper.c | 11 +++++++++++
 drivers/gpu/drm/drm_fops.c          |  2 +-
 include/drm/drmP.h                  |  1 +
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
index 799c1564a4f8..b4dfd1e1a4f0 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -1355,6 +1355,15 @@ static int stall_checks(struct drm_crtc *crtc, bool nonblock)
 	return ret < 0 ? ret : 0;
 }
 
+void release_crtc_commit(struct completion *completion)
+{
+	struct drm_crtc_commit *commit = container_of(completion,
+						      typeof(*commit),
+						      flip_done);
+
+	drm_crtc_commit_put(commit);
+}
+
 /**
  * drm_atomic_helper_setup_commit - setup possibly nonblocking commit
  * @state: new modeset state to be committed
@@ -1447,6 +1456,8 @@ int drm_atomic_helper_setup_commit(struct drm_atomic_state *state,
 		}
 
 		crtc_state->event->base.completion = &commit->flip_done;
+		crtc_state->event->base.completion_release = release_crtc_commit;
+		drm_crtc_commit_get(commit);
 	}
 
 	return 0;
diff --git a/drivers/gpu/drm/drm_fops.c b/drivers/gpu/drm/drm_fops.c
index 48e106557c92..e22645375e60 100644
--- a/drivers/gpu/drm/drm_fops.c
+++ b/drivers/gpu/drm/drm_fops.c
@@ -689,8 +689,8 @@ void drm_send_event_locked(struct drm_device *dev, struct drm_pending_event *e)
 	assert_spin_locked(&dev->event_lock);
 
 	if (e->completion) {
-		/* ->completion might disappear as soon as it signalled. */
 		complete_all(e->completion);
+		e->completion_release(e->completion);
 		e->completion = NULL;
 	}
 
diff --git a/include/drm/drmP.h b/include/drm/drmP.h
index a9cfd33c7b1a..e821a8f142d9 100644
--- a/include/drm/drmP.h
+++ b/include/drm/drmP.h
@@ -360,6 +360,7 @@ struct drm_ioctl_desc {
 /* Event queued up for userspace to read */
 struct drm_pending_event {
 	struct completion *completion;
+	void (*completion_release)(struct completion *completion);
 	struct drm_event *event;
 	struct dma_fence *fence;
 	struct list_head link;
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/2] drm: Add kernel-doc for drm_crtc_commit_get/put

[Daniel Vetter]

I was lazy, rectify that! Also align with drm_atomic_state_get/put for
ocd.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic.c |  9 ++-------
 include/drm/drm_atomic.h     | 21 ++++++++++++++++++++-
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index b1b54011a92c..26a4cfdf7777 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -35,19 +35,14 @@
 
 #include "drm_crtc_internal.h"
 
-static void crtc_commit_free(struct kref *kref)
+void __drm_crtc_commit_free(struct kref *kref)
 {
 	struct drm_crtc_commit *commit =
 		container_of(kref, struct drm_crtc_commit, ref);
 
 	kfree(commit);
 }
-
-void drm_crtc_commit_put(struct drm_crtc_commit *commit)
-{
-	kref_put(&commit->ref, crtc_commit_free);
-}
-EXPORT_SYMBOL(drm_crtc_commit_put);
+EXPORT_SYMBOL(__drm_crtc_commit_free);
 
 /**
  * drm_atomic_state_default_release -
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 91b18bd7cb50..e8ed6f707497 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -199,12 +199,31 @@ struct drm_atomic_state {
 	struct work_struct commit_work;
 };
 
-void drm_crtc_commit_put(struct drm_crtc_commit *commit);
+void __drm_crtc_commit_free(struct kref *kref);
+
+/**
+ * drm_crtc_commit_get - acquire a reference to the CRTC commit
+ * @commit: CRTC commit
+ *
+ * Increases the reference of @commit.
+ */
 static inline void drm_crtc_commit_get(struct drm_crtc_commit *commit)
 {
 	kref_get(&commit->ref);
 }
 
+/**
+ * drm_crtc_commit_put - release a reference to the CRTC commmit
+ * @commit: CRTC commit
+ *
+ * This releases a reference to @commit which is freed after removing the
+ * final reference. No locking required and callable from any context.
+ */
+void drm_crtc_commit_put(struct drm_crtc_commit *commit)
+{
+	kref_put(&commit->ref, __drm_crtc_commit_free);
+}
+
 struct drm_atomic_state * __must_check
 drm_atomic_state_alloc(struct drm_device *dev);
 void drm_atomic_state_clear(struct drm_atomic_state *state);
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm: refernce count event->completion

[Chris Wilson]

On Wed, Dec 21, 2016 at 11:23:30AM +0100, Daniel Vetter wrote:
> When writing the generic nonblocking commit code I assumed that
> through clever lifetime management I can assure that the completion
> (stored in drm_crtc_commit) only gets freed after it is completed. And
> that worked.
> 
> I also wanted to make nonblocking helpers resilient against driver
> bugs, by having timeouts everywhere. And that worked too.
> 
> Unfortunately taking boths things together results in oopses :( Well,
> at least sometimes: What seems to happen is that the drm event hangs
> around forever stuck in limbo land. The nonblocking helpers eventually
> time out, move on and release it. Now the bug I tested all this
> against is drivers that just entirely fail to deliver the vblank
> events like they should, and in those cases the event is simply
> leaked. But what seems to happen, at least sometimes, on i915 is that
> the event is set up correctly, but somohow the vblank fails to fire in
> time. Which means the event isn't leaked, it's still there waiting for
> eventually a vblank to fire. That tends to happen when re-enabling the
> pipe, and then the trap springs and the kernel oopses.
> 
> The correct fix here is simply to refcount the crtc commit to make
> sure that the event sticks around even for drivers which only
> sometimes fail to deliver vblanks for some arbitrary reasons. Since
> crtc commits are already refcounted that's easy to do.

Or make the event a part of the atomic state?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

✗ Fi.CI.BAT: failure for series starting with [1/2] drm: refernce count event->completion

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm: refernce count event->completion
URL   : https://patchwork.freedesktop.org/series/17087/
State : failure

== Summary ==

drivers/gpu/drm/i915/intel_dvo.o: In function `atomic_sub_and_test':
/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: multiple definition of `drm_crtc_commit_put'
drivers/gpu/drm/i915/i915_drv.o:/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: first defined here
drivers/gpu/drm/i915/intel_hdmi.o: In function `atomic_sub_and_test':
/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: multiple definition of `drm_crtc_commit_put'
drivers/gpu/drm/i915/i915_drv.o:/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: first defined here
drivers/gpu/drm/i915/intel_i2c.o: In function `atomic_sub_and_test':
/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: multiple definition of `drm_crtc_commit_put'
drivers/gpu/drm/i915/i915_drv.o:/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: first defined here
drivers/gpu/drm/i915/intel_lspcon.o: In function `atomic_sub_and_test':
/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: multiple definition of `drm_crtc_commit_put'
drivers/gpu/drm/i915/i915_drv.o:/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: first defined here
drivers/gpu/drm/i915/intel_lvds.o: In function `atomic_sub_and_test':
/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: multiple definition of `drm_crtc_commit_put'
drivers/gpu/drm/i915/i915_drv.o:/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: first defined here
drivers/gpu/drm/i915/intel_panel.o: In function `atomic_sub_and_test':
/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: multiple definition of `drm_crtc_commit_put'
drivers/gpu/drm/i915/i915_drv.o:/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: first defined here
drivers/gpu/drm/i915/intel_sdvo.o: In function `atomic_sub_and_test':
/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: multiple definition of `drm_crtc_commit_put'
drivers/gpu/drm/i915/i915_drv.o:/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: first defined here
drivers/gpu/drm/i915/intel_tv.o: In function `atomic_sub_and_test':
/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: multiple definition of `drm_crtc_commit_put'
drivers/gpu/drm/i915/i915_drv.o:/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: first defined here
drivers/gpu/drm/i915/i915_gpu_error.o: In function `atomic_sub_and_test':
/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: multiple definition of `drm_crtc_commit_put'
drivers/gpu/drm/i915/i915_drv.o:/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: first defined here
drivers/gpu/drm/i915/i915_vgpu.o: In function `atomic_sub_and_test':
/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: multiple definition of `drm_crtc_commit_put'
drivers/gpu/drm/i915/i915_drv.o:/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: first defined here
drivers/gpu/drm/i915/i915_perf.o: In function `atomic_sub_and_test':
/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: multiple definition of `drm_crtc_commit_put'
drivers/gpu/drm/i915/i915_drv.o:/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: first defined here
drivers/gpu/drm/i915/i915_oa_hsw.o: In function `atomic_sub_and_test':
/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: multiple definition of `drm_crtc_commit_put'
drivers/gpu/drm/i915/i915_drv.o:/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: first defined here
drivers/gpu/drm/i915/intel_gvt.o: In function `atomic_sub_and_test':
/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: multiple definition of `drm_crtc_commit_put'
drivers/gpu/drm/i915/i915_drv.o:/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: first defined here
drivers/gpu/drm/i915/gvt/gvt.o: In function `atomic_sub_and_test':
/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: multiple definition of `drm_crtc_commit_put'
drivers/gpu/drm/i915/i915_drv.o:/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: first defined here
drivers/gpu/drm/i915/gvt/aperture_gm.o: In function `atomic_sub_and_test':
/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: multiple definition of `drm_crtc_commit_put'
drivers/gpu/drm/i915/i915_drv.o:/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: first defined here
drivers/gpu/drm/i915/gvt/handlers.o: In function `atomic_sub_and_test':
/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: multiple definition of `drm_crtc_commit_put'
drivers/gpu/drm/i915/i915_drv.o:/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: first defined here
drivers/gpu/drm/i915/gvt/vgpu.o: In function `atomic_sub_and_test':
/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: multiple definition of `drm_crtc_commit_put'
drivers/gpu/drm/i915/i915_drv.o:/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: first defined here
drivers/gpu/drm/i915/gvt/firmware.o: In function `atomic_sub_and_test':
/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: multiple definition of `drm_crtc_commit_put'
drivers/gpu/drm/i915/i915_drv.o:/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: first defined here
drivers/gpu/drm/i915/gvt/interrupt.o: In function `atomic_sub_and_test':
/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: multiple definition of `drm_crtc_commit_put'
drivers/gpu/drm/i915/i915_drv.o:/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: first defined here
drivers/gpu/drm/i915/gvt/gtt.o: In function `atomic_sub_and_test':
/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: multiple definition of `drm_crtc_commit_put'
drivers/gpu/drm/i915/i915_drv.o:/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: first defined here
drivers/gpu/drm/i915/gvt/cfg_space.o: In function `atomic_sub_and_test':
/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: multiple definition of `drm_crtc_commit_put'
drivers/gpu/drm/i915/i915_drv.o:/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: first defined here
drivers/gpu/drm/i915/gvt/opregion.o: In function `atomic_sub_and_test':
/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: multiple definition of `drm_crtc_commit_put'
drivers/gpu/drm/i915/i915_drv.o:/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: first defined here
drivers/gpu/drm/i915/gvt/mmio.o: In function `atomic_sub_and_test':
/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: multiple definition of `drm_crtc_commit_put'
drivers/gpu/drm/i915/i915_drv.o:/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: first defined here
drivers/gpu/drm/i915/gvt/display.o: In function `atomic_sub_and_test':
/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: multiple definition of `drm_crtc_commit_put'
drivers/gpu/drm/i915/i915_drv.o:/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: first defined here
drivers/gpu/drm/i915/gvt/edid.o: In function `atomic_sub_and_test':
/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: multiple definition of `drm_crtc_commit_put'
drivers/gpu/drm/i915/i915_drv.o:/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: first defined here
drivers/gpu/drm/i915/gvt/execlist.o: In function `atomic_sub_and_test':
/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: multiple definition of `drm_crtc_commit_put'
drivers/gpu/drm/i915/i915_drv.o:/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: first defined here
drivers/gpu/drm/i915/gvt/scheduler.o: In function `atomic_sub_and_test':
/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: multiple definition of `drm_crtc_commit_put'
drivers/gpu/drm/i915/i915_drv.o:/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: first defined here
drivers/gpu/drm/i915/gvt/sched_policy.o: In function `atomic_sub_and_test':
/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: multiple definition of `drm_crtc_commit_put'
drivers/gpu/drm/i915/i915_drv.o:/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: first defined here
drivers/gpu/drm/i915/gvt/render.o: In function `atomic_sub_and_test':
/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: multiple definition of `drm_crtc_commit_put'
drivers/gpu/drm/i915/i915_drv.o:/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: first defined here
drivers/gpu/drm/i915/gvt/cmd_parser.o: In function `atomic_sub_and_test':
/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: multiple definition of `drm_crtc_commit_put'
drivers/gpu/drm/i915/i915_drv.o:/home/cidrm/kernel/./arch/x86/include/asm/atomic.h:80: first defined here
scripts/Makefile.build:531: recipe for target 'drivers/gpu/drm/i915/i915.o' failed
make[4]: *** [drivers/gpu/drm/i915/i915.o] Error 1
scripts/Makefile.build:544: recipe for target 'drivers/gpu/drm/i915' failed
make[3]: *** [drivers/gpu/drm/i915] Error 2
scripts/Makefile.build:544: recipe for target 'drivers/gpu/drm' failed
make[2]: *** [drivers/gpu/drm] Error 2
scripts/Makefile.build:544: recipe for target 'drivers/gpu' failed
make[1]: *** [drivers/gpu] Error 2
Makefile:988: recipe for target 'drivers' failed
make: *** [drivers] Error 2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm: refernce count event->completion

[Maarten Lankhorst]

Op 21-12-16 om 11:36 schreef Chris Wilson:
> On Wed, Dec 21, 2016 at 11:23:30AM +0100, Daniel Vetter wrote:
>> When writing the generic nonblocking commit code I assumed that
>> through clever lifetime management I can assure that the completion
>> (stored in drm_crtc_commit) only gets freed after it is completed. And
>> that worked.
>>
>> I also wanted to make nonblocking helpers resilient against driver
>> bugs, by having timeouts everywhere. And that worked too.
>>
>> Unfortunately taking boths things together results in oopses :( Well,
>> at least sometimes: What seems to happen is that the drm event hangs
>> around forever stuck in limbo land. The nonblocking helpers eventually
>> time out, move on and release it. Now the bug I tested all this
>> against is drivers that just entirely fail to deliver the vblank
>> events like they should, and in those cases the event is simply
>> leaked. But what seems to happen, at least sometimes, on i915 is that
>> the event is set up correctly, but somohow the vblank fails to fire in
>> time. Which means the event isn't leaked, it's still there waiting for
>> evevntually a vblank to fire. That tends to happen when re-enabling the
>> pipe, and then the trap springs and the kernel oopses.
>>
>> The correct fix here is simply to refcount the crtc commit to make
>> sure that the event sticks around even for drivers which only
>> sometimes fail to deliver vblanks for some arbitrary reasons. Since
>> crtc commits are already refcounted that's easy to do.
> Or make the event a part of the atomic state?
> -Chris
>
afaict crtc commit is already taken to wait for completion, so this patch makes sense.

There's just a minor typo in the subject. :)
Not sure that release_commit should be a function pointer, regardless..

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Re: [PATCH 1/2] drm: refernce count event->completion

[Daniel Vetter]

On Wed, Dec 21, 2016 at 10:36:41AM +0000, Chris Wilson wrote:
> On Wed, Dec 21, 2016 at 11:23:30AM +0100, Daniel Vetter wrote:
> > When writing the generic nonblocking commit code I assumed that
> > through clever lifetime management I can assure that the completion
> > (stored in drm_crtc_commit) only gets freed after it is completed. And
> > that worked.
> > 
> > I also wanted to make nonblocking helpers resilient against driver
> > bugs, by having timeouts everywhere. And that worked too.
> > 
> > Unfortunately taking boths things together results in oopses :( Well,
> > at least sometimes: What seems to happen is that the drm event hangs
> > around forever stuck in limbo land. The nonblocking helpers eventually
> > time out, move on and release it. Now the bug I tested all this
> > against is drivers that just entirely fail to deliver the vblank
> > events like they should, and in those cases the event is simply
> > leaked. But what seems to happen, at least sometimes, on i915 is that
> > the event is set up correctly, but somohow the vblank fails to fire in
> > time. Which means the event isn't leaked, it's still there waiting for
> > eventually a vblank to fire. That tends to happen when re-enabling the
> > pipe, and then the trap springs and the kernel oopses.
> > 
> > The correct fix here is simply to refcount the crtc commit to make
> > sure that the event sticks around even for drivers which only
> > sometimes fail to deliver vblanks for some arbitrary reasons. Since
> > crtc commits are already refcounted that's easy to do.
> 
> Or make the event a part of the atomic state?

I guess we could do that, but I wanted the most minimal thing for
backporting. And reference-counted atomic state is new, and the patch
would be a bit bigger.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/2] drm: refernce count event->completion

[Daniel Vetter]

On Wed, Dec 21, 2016 at 10:36:41AM +0000, Chris Wilson wrote:
> On Wed, Dec 21, 2016 at 11:23:30AM +0100, Daniel Vetter wrote:
> > When writing the generic nonblocking commit code I assumed that
> > through clever lifetime management I can assure that the completion
> > (stored in drm_crtc_commit) only gets freed after it is completed. And
> > that worked.
> > 
> > I also wanted to make nonblocking helpers resilient against driver
> > bugs, by having timeouts everywhere. And that worked too.
> > 
> > Unfortunately taking boths things together results in oopses :( Well,
> > at least sometimes: What seems to happen is that the drm event hangs
> > around forever stuck in limbo land. The nonblocking helpers eventually
> > time out, move on and release it. Now the bug I tested all this
> > against is drivers that just entirely fail to deliver the vblank
> > events like they should, and in those cases the event is simply
> > leaked. But what seems to happen, at least sometimes, on i915 is that
> > the event is set up correctly, but somohow the vblank fails to fire in
> > time. Which means the event isn't leaked, it's still there waiting for
> > eventually a vblank to fire. That tends to happen when re-enabling the
> > pipe, and then the trap springs and the kernel oopses.
> > 
> > The correct fix here is simply to refcount the crtc commit to make
> > sure that the event sticks around even for drivers which only
> > sometimes fail to deliver vblanks for some arbitrary reasons. Since
> > crtc commits are already refcounted that's easy to do.
> 
> Or make the event a part of the atomic state?

I guess we could do that, but I wanted the most minimal thing for
backporting. And reference-counted atomic state is new, and the patch
would be a bit bigger.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm: Add kernel-doc for drm_crtc_commit_get/put

[Daniel Vetter]

I was lazy, rectify that! Also align with drm_atomic_state_get/put for
ocd.

v2: Git add helps.

Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/drm_atomic.c |  9 ++-------
 include/drm/drm_atomic.h     | 21 ++++++++++++++++++++-
 2 files changed, 22 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index b1b54011a92c..26a4cfdf7777 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -35,19 +35,14 @@
 
 #include "drm_crtc_internal.h"
 
-static void crtc_commit_free(struct kref *kref)
+void __drm_crtc_commit_free(struct kref *kref)
 {
 	struct drm_crtc_commit *commit =
 		container_of(kref, struct drm_crtc_commit, ref);
 
 	kfree(commit);
 }
-
-void drm_crtc_commit_put(struct drm_crtc_commit *commit)
-{
-	kref_put(&commit->ref, crtc_commit_free);
-}
-EXPORT_SYMBOL(drm_crtc_commit_put);
+EXPORT_SYMBOL(__drm_crtc_commit_free);
 
 /**
  * drm_atomic_state_default_release -
diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
index 91b18bd7cb50..97587ec36eae 100644
--- a/include/drm/drm_atomic.h
+++ b/include/drm/drm_atomic.h
@@ -199,12 +199,31 @@ struct drm_atomic_state {
 	struct work_struct commit_work;
 };
 
-void drm_crtc_commit_put(struct drm_crtc_commit *commit);
+void __drm_crtc_commit_free(struct kref *kref);
+
+/**
+ * drm_crtc_commit_get - acquire a reference to the CRTC commit
+ * @commit: CRTC commit
+ *
+ * Increases the reference of @commit.
+ */
 static inline void drm_crtc_commit_get(struct drm_crtc_commit *commit)
 {
 	kref_get(&commit->ref);
 }
 
+/**
+ * drm_crtc_commit_put - release a reference to the CRTC commmit
+ * @commit: CRTC commit
+ *
+ * This releases a reference to @commit which is freed after removing the
+ * final reference. No locking required and callable from any context.
+ */
+static inline void drm_crtc_commit_put(struct drm_crtc_commit *commit)
+{
+	kref_put(&commit->ref, __drm_crtc_commit_free);
+}
+
 struct drm_atomic_state * __must_check
 drm_atomic_state_alloc(struct drm_device *dev);
 void drm_atomic_state_clear(struct drm_atomic_state *state);
-- 
2.11.0

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

✓ Fi.CI.BAT: success for series starting with [1/2] drm: refernce count event->completion (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm: refernce count event->completion (rev2)
URL   : https://patchwork.freedesktop.org/series/17087/
State : success

== Summary ==

Series 17087v2 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/17087/revisions/2/mbox/

Test kms_force_connector_basic:
        Subgroup force-connector-state:
                dmesg-warn -> PASS       (fi-ivb-3770)

fi-bdw-5557u     total:247  pass:233  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:247  pass:208  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:247  pass:225  dwarn:1   dfail:0   fail:0   skip:21 
fi-bxt-t5700     total:247  pass:220  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-j1900     total:247  pass:220  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:247  pass:216  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:247  pass:228  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:247  pass:228  dwarn:0   dfail:0   fail:0   skip:19 
fi-ilk-650       total:247  pass:195  dwarn:0   dfail:0   fail:0   skip:52 
fi-ivb-3520m     total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:247  pass:226  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6260u     total:247  pass:234  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:247  pass:227  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:247  pass:224  dwarn:3   dfail:0   fail:0   skip:20 
fi-skl-6770hq    total:247  pass:234  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:247  pass:216  dwarn:0   dfail:0   fail:0   skip:31 
fi-snb-2600      total:247  pass:215  dwarn:0   dfail:0   fail:0   skip:32 

f45701ec502e5ee9682561be7578bd39741ad4bb drm-tip: 2016y-12m-21d-11h-52m-22s UTC integration manifest
c68032e drm: Add kernel-doc for drm_crtc_commit_get/put
e2ba7c9 drm: refernce count event->completion

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3353/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] drm: Add kernel-doc for drm_crtc_commit_get/put

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3926 bytes --]

Hi Daniel,

[auto build test ERROR on drm/drm-next]
[also build test ERROR on v4.9 next-20161221]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Daniel-Vetter/drm-refernce-count-event-completion/20161222-101700
base:   git://people.freedesktop.org/~airlied/linux.git drm-next
config: i386-randconfig-r0-201651 (attached as .config)
compiler: gcc-5 (Debian 5.4.1-2) 5.4.1 20160904
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/gpu/drm/virtio/virtgpu_kms.o: In function `drm_crtc_commit_put':
>> virtgpu_kms.c:(.text+0x80): multiple definition of `drm_crtc_commit_put'
   drivers/gpu/drm/virtio/virtgpu_drv.o:virtgpu_drv.c:(.text+0x80): first defined here
   drivers/gpu/drm/virtio/virtgpu_drm_bus.o: In function `drm_crtc_commit_put':
   virtgpu_drm_bus.c:(.text+0x0): multiple definition of `drm_crtc_commit_put'
   drivers/gpu/drm/virtio/virtgpu_drv.o:virtgpu_drv.c:(.text+0x80): first defined here
   drivers/gpu/drm/virtio/virtgpu_gem.o: In function `drm_crtc_commit_put':
   virtgpu_gem.c:(.text+0x0): multiple definition of `drm_crtc_commit_put'
   drivers/gpu/drm/virtio/virtgpu_drv.o:virtgpu_drv.c:(.text+0x80): first defined here
   drivers/gpu/drm/virtio/virtgpu_fb.o: In function `drm_crtc_commit_put':
   virtgpu_fb.c:(.text+0x620): multiple definition of `drm_crtc_commit_put'
   drivers/gpu/drm/virtio/virtgpu_drv.o:virtgpu_drv.c:(.text+0x80): first defined here
   drivers/gpu/drm/virtio/virtgpu_display.o: In function `drm_crtc_commit_put':
   virtgpu_display.c:(.text+0x320): multiple definition of `drm_crtc_commit_put'
   drivers/gpu/drm/virtio/virtgpu_drv.o:virtgpu_drv.c:(.text+0x80): first defined here
   drivers/gpu/drm/virtio/virtgpu_vq.o: In function `drm_crtc_commit_put':
   virtgpu_vq.c:(.text+0x660): multiple definition of `drm_crtc_commit_put'
   drivers/gpu/drm/virtio/virtgpu_drv.o:virtgpu_drv.c:(.text+0x80): first defined here
   drivers/gpu/drm/virtio/virtgpu_ttm.o: In function `drm_crtc_commit_put':
   virtgpu_ttm.c:(.text+0x400): multiple definition of `drm_crtc_commit_put'
   drivers/gpu/drm/virtio/virtgpu_drv.o:virtgpu_drv.c:(.text+0x80): first defined here
   drivers/gpu/drm/virtio/virtgpu_fence.o: In function `drm_crtc_commit_put':
   virtgpu_fence.c:(.text+0xc0): multiple definition of `drm_crtc_commit_put'
   drivers/gpu/drm/virtio/virtgpu_drv.o:virtgpu_drv.c:(.text+0x80): first defined here
   drivers/gpu/drm/virtio/virtgpu_object.o: In function `drm_crtc_commit_put':
   virtgpu_object.c:(.text+0x60): multiple definition of `drm_crtc_commit_put'
   drivers/gpu/drm/virtio/virtgpu_drv.o:virtgpu_drv.c:(.text+0x80): first defined here
   drivers/gpu/drm/virtio/virtgpu_debugfs.o: In function `drm_crtc_commit_put':
   virtgpu_debugfs.c:(.text+0x50): multiple definition of `drm_crtc_commit_put'
   drivers/gpu/drm/virtio/virtgpu_drv.o:virtgpu_drv.c:(.text+0x80): first defined here
   drivers/gpu/drm/virtio/virtgpu_plane.o: In function `drm_crtc_commit_put':
   virtgpu_plane.c:(.text+0x4c0): multiple definition of `drm_crtc_commit_put'
   drivers/gpu/drm/virtio/virtgpu_drv.o:virtgpu_drv.c:(.text+0x80): first defined here
   drivers/gpu/drm/virtio/virtgpu_ioctl.o: In function `drm_crtc_commit_put':
   virtgpu_ioctl.c:(.text+0xed0): multiple definition of `drm_crtc_commit_put'
   drivers/gpu/drm/virtio/virtgpu_drv.o:virtgpu_drv.c:(.text+0x80): first defined here
   drivers/gpu/drm/virtio/virtgpu_prime.o: In function `drm_crtc_commit_put':
   virtgpu_prime.c:(.text+0x0): multiple definition of `drm_crtc_commit_put'
   drivers/gpu/drm/virtio/virtgpu_drv.o:virtgpu_drv.c:(.text+0x80): first defined here

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 22623 bytes --]

[-- Attachment #3: 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 1/2] drm: refernce count event->completion

[Daniel Vetter]

On Wed, Dec 21, 2016 at 12:08:45PM +0100, Maarten Lankhorst wrote:
> Op 21-12-16 om 11:36 schreef Chris Wilson:
> > On Wed, Dec 21, 2016 at 11:23:30AM +0100, Daniel Vetter wrote:
> >> When writing the generic nonblocking commit code I assumed that
> >> through clever lifetime management I can assure that the completion
> >> (stored in drm_crtc_commit) only gets freed after it is completed. And
> >> that worked.
> >>
> >> I also wanted to make nonblocking helpers resilient against driver
> >> bugs, by having timeouts everywhere. And that worked too.
> >>
> >> Unfortunately taking boths things together results in oopses :( Well,
> >> at least sometimes: What seems to happen is that the drm event hangs
> >> around forever stuck in limbo land. The nonblocking helpers eventually
> >> time out, move on and release it. Now the bug I tested all this
> >> against is drivers that just entirely fail to deliver the vblank
> >> events like they should, and in those cases the event is simply
> >> leaked. But what seems to happen, at least sometimes, on i915 is that
> >> the event is set up correctly, but somohow the vblank fails to fire in
> >> time. Which means the event isn't leaked, it's still there waiting for
> >> evevntually a vblank to fire. That tends to happen when re-enabling the
> >> pipe, and then the trap springs and the kernel oopses.
> >>
> >> The correct fix here is simply to refcount the crtc commit to make
> >> sure that the event sticks around even for drivers which only
> >> sometimes fail to deliver vblanks for some arbitrary reasons. Since
> >> crtc commits are already refcounted that's easy to do.
> > Or make the event a part of the atomic state?
> > -Chris
> >
> afaict crtc commit is already taken to wait for completion, so this patch makes sense.
> 
> There's just a minor typo in the subject. :)
> Not sure that release_commit should be a function pointer, regardless..
> 
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

It didn't help the bug reporters against oopses (but the reporters are
supremely confusing, I have no idea what's really being tested, the
bugzilla is a mess), but I still think the patch is useful for more
robuestness, I dropped the cc: stable and applied it to drm-misc.

Thanks for the review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/2] drm: refernce count event->completion

[Daniel Vetter]

On Wed, Dec 21, 2016 at 12:08:45PM +0100, Maarten Lankhorst wrote:
> Op 21-12-16 om 11:36 schreef Chris Wilson:
> > On Wed, Dec 21, 2016 at 11:23:30AM +0100, Daniel Vetter wrote:
> >> When writing the generic nonblocking commit code I assumed that
> >> through clever lifetime management I can assure that the completion
> >> (stored in drm_crtc_commit) only gets freed after it is completed. And
> >> that worked.
> >>
> >> I also wanted to make nonblocking helpers resilient against driver
> >> bugs, by having timeouts everywhere. And that worked too.
> >>
> >> Unfortunately taking boths things together results in oopses :( Well,
> >> at least sometimes: What seems to happen is that the drm event hangs
> >> around forever stuck in limbo land. The nonblocking helpers eventually
> >> time out, move on and release it. Now the bug I tested all this
> >> against is drivers that just entirely fail to deliver the vblank
> >> events like they should, and in those cases the event is simply
> >> leaked. But what seems to happen, at least sometimes, on i915 is that
> >> the event is set up correctly, but somohow the vblank fails to fire in
> >> time. Which means the event isn't leaked, it's still there waiting for
> >> evevntually a vblank to fire. That tends to happen when re-enabling the
> >> pipe, and then the trap springs and the kernel oopses.
> >>
> >> The correct fix here is simply to refcount the crtc commit to make
> >> sure that the event sticks around even for drivers which only
> >> sometimes fail to deliver vblanks for some arbitrary reasons. Since
> >> crtc commits are already refcounted that's easy to do.
> > Or make the event a part of the atomic state?
> > -Chris
> >
> afaict crtc commit is already taken to wait for completion, so this patch makes sense.
> 
> There's just a minor typo in the subject. :)
> Not sure that release_commit should be a function pointer, regardless..
> 
> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

It didn't help the bug reporters against oopses (but the reporters are
supremely confusing, I have no idea what's really being tested, the
bugzilla is a mess), but I still think the patch is useful for more
robuestness, I dropped the cc: stable and applied it to drm-misc.

Thanks for the review.
-Daniel
-- 
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] drm: Add kernel-doc for drm_crtc_commit_get/put

[Daniel Vetter]

On Wed, Dec 21, 2016 at 02:03:35PM +0100, Daniel Vetter wrote:
> I was lazy, rectify that! Also align with drm_atomic_state_get/put for
> ocd.
> 
> v2: Git add helps.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>

Anyone feel like acking this, pretty please? ;-)
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic.c |  9 ++-------
>  include/drm/drm_atomic.h     | 21 ++++++++++++++++++++-
>  2 files changed, 22 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index b1b54011a92c..26a4cfdf7777 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -35,19 +35,14 @@
>  
>  #include "drm_crtc_internal.h"
>  
> -static void crtc_commit_free(struct kref *kref)
> +void __drm_crtc_commit_free(struct kref *kref)
>  {
>  	struct drm_crtc_commit *commit =
>  		container_of(kref, struct drm_crtc_commit, ref);
>  
>  	kfree(commit);
>  }
> -
> -void drm_crtc_commit_put(struct drm_crtc_commit *commit)
> -{
> -	kref_put(&commit->ref, crtc_commit_free);
> -}
> -EXPORT_SYMBOL(drm_crtc_commit_put);
> +EXPORT_SYMBOL(__drm_crtc_commit_free);
>  
>  /**
>   * drm_atomic_state_default_release -
> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
> index 91b18bd7cb50..97587ec36eae 100644
> --- a/include/drm/drm_atomic.h
> +++ b/include/drm/drm_atomic.h
> @@ -199,12 +199,31 @@ struct drm_atomic_state {
>  	struct work_struct commit_work;
>  };
>  
> -void drm_crtc_commit_put(struct drm_crtc_commit *commit);
> +void __drm_crtc_commit_free(struct kref *kref);
> +
> +/**
> + * drm_crtc_commit_get - acquire a reference to the CRTC commit
> + * @commit: CRTC commit
> + *
> + * Increases the reference of @commit.
> + */
>  static inline void drm_crtc_commit_get(struct drm_crtc_commit *commit)
>  {
>  	kref_get(&commit->ref);
>  }
>  
> +/**
> + * drm_crtc_commit_put - release a reference to the CRTC commmit
> + * @commit: CRTC commit
> + *
> + * This releases a reference to @commit which is freed after removing the
> + * final reference. No locking required and callable from any context.
> + */
> +static inline void drm_crtc_commit_put(struct drm_crtc_commit *commit)
> +{
> +	kref_put(&commit->ref, __drm_crtc_commit_free);
> +}
> +
>  struct drm_atomic_state * __must_check
>  drm_atomic_state_alloc(struct drm_device *dev);
>  void drm_atomic_state_clear(struct drm_atomic_state *state);
> -- 
> 2.11.0
> 

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

Re: [PATCH] drm: Add kernel-doc for drm_crtc_commit_get/put

[Alex Deucher]

On Wed, Jan 4, 2017 at 11:11 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Dec 21, 2016 at 02:03:35PM +0100, Daniel Vetter wrote:
>> I was lazy, rectify that! Also align with drm_atomic_state_get/put for
>> ocd.
>>
>> v2: Git add helps.
>>
>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>
> Anyone feel like acking this, pretty please? ;-)

Acked-by: Alex Deucher <alexander.deucher@amd.com>


> -Daniel
>
>> ---
>>  drivers/gpu/drm/drm_atomic.c |  9 ++-------
>>  include/drm/drm_atomic.h     | 21 ++++++++++++++++++++-
>>  2 files changed, 22 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index b1b54011a92c..26a4cfdf7777 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -35,19 +35,14 @@
>>
>>  #include "drm_crtc_internal.h"
>>
>> -static void crtc_commit_free(struct kref *kref)
>> +void __drm_crtc_commit_free(struct kref *kref)
>>  {
>>       struct drm_crtc_commit *commit =
>>               container_of(kref, struct drm_crtc_commit, ref);
>>
>>       kfree(commit);
>>  }
>> -
>> -void drm_crtc_commit_put(struct drm_crtc_commit *commit)
>> -{
>> -     kref_put(&commit->ref, crtc_commit_free);
>> -}
>> -EXPORT_SYMBOL(drm_crtc_commit_put);
>> +EXPORT_SYMBOL(__drm_crtc_commit_free);
>>
>>  /**
>>   * drm_atomic_state_default_release -
>> diff --git a/include/drm/drm_atomic.h b/include/drm/drm_atomic.h
>> index 91b18bd7cb50..97587ec36eae 100644
>> --- a/include/drm/drm_atomic.h
>> +++ b/include/drm/drm_atomic.h
>> @@ -199,12 +199,31 @@ struct drm_atomic_state {
>>       struct work_struct commit_work;
>>  };
>>
>> -void drm_crtc_commit_put(struct drm_crtc_commit *commit);
>> +void __drm_crtc_commit_free(struct kref *kref);
>> +
>> +/**
>> + * drm_crtc_commit_get - acquire a reference to the CRTC commit
>> + * @commit: CRTC commit
>> + *
>> + * Increases the reference of @commit.
>> + */
>>  static inline void drm_crtc_commit_get(struct drm_crtc_commit *commit)
>>  {
>>       kref_get(&commit->ref);
>>  }
>>
>> +/**
>> + * drm_crtc_commit_put - release a reference to the CRTC commmit
>> + * @commit: CRTC commit
>> + *
>> + * This releases a reference to @commit which is freed after removing the
>> + * final reference. No locking required and callable from any context.
>> + */
>> +static inline void drm_crtc_commit_put(struct drm_crtc_commit *commit)
>> +{
>> +     kref_put(&commit->ref, __drm_crtc_commit_free);
>> +}
>> +
>>  struct drm_atomic_state * __must_check
>>  drm_atomic_state_alloc(struct drm_device *dev);
>>  void drm_atomic_state_clear(struct drm_atomic_state *state);
>> --
>> 2.11.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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm: Add kernel-doc for drm_crtc_commit_get/put

[Daniel Vetter]

On Wed, Jan 04, 2017 at 03:26:48PM -0500, Alex Deucher wrote:
> On Wed, Jan 4, 2017 at 11:11 AM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Dec 21, 2016 at 02:03:35PM +0100, Daniel Vetter wrote:
> >> I was lazy, rectify that! Also align with drm_atomic_state_get/put for
> >> ocd.
> >>
> >> v2: Git add helps.
> >>
> >> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> >
> > Anyone feel like acking this, pretty please? ;-)
> 
> Acked-by: Alex Deucher <alexander.deucher@amd.com>

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

Re: [PATCH 1/2] drm: refernce count event->completion

[Jani Nikula]

On Wed, 04 Jan 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Dec 21, 2016 at 12:08:45PM +0100, Maarten Lankhorst wrote:
>> Op 21-12-16 om 11:36 schreef Chris Wilson:
>> > On Wed, Dec 21, 2016 at 11:23:30AM +0100, Daniel Vetter wrote:
>> >> When writing the generic nonblocking commit code I assumed that
>> >> through clever lifetime management I can assure that the completion
>> >> (stored in drm_crtc_commit) only gets freed after it is completed. And
>> >> that worked.
>> >>
>> >> I also wanted to make nonblocking helpers resilient against driver
>> >> bugs, by having timeouts everywhere. And that worked too.
>> >>
>> >> Unfortunately taking boths things together results in oopses :( Well,
>> >> at least sometimes: What seems to happen is that the drm event hangs
>> >> around forever stuck in limbo land. The nonblocking helpers eventually
>> >> time out, move on and release it. Now the bug I tested all this
>> >> against is drivers that just entirely fail to deliver the vblank
>> >> events like they should, and in those cases the event is simply
>> >> leaked. But what seems to happen, at least sometimes, on i915 is that
>> >> the event is set up correctly, but somohow the vblank fails to fire in
>> >> time. Which means the event isn't leaked, it's still there waiting for
>> >> evevntually a vblank to fire. That tends to happen when re-enabling the
>> >> pipe, and then the trap springs and the kernel oopses.
>> >>
>> >> The correct fix here is simply to refcount the crtc commit to make
>> >> sure that the event sticks around even for drivers which only
>> >> sometimes fail to deliver vblanks for some arbitrary reasons. Since
>> >> crtc commits are already refcounted that's easy to do.
>> > Or make the event a part of the atomic state?
>> > -Chris
>> >
>> afaict crtc commit is already taken to wait for completion, so this patch makes sense.
>> 
>> There's just a minor typo in the subject. :)
>> Not sure that release_commit should be a function pointer, regardless..
>> 
>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>
> It didn't help the bug reporters against oopses (but the reporters are
> supremely confusing, I have no idea what's really being tested, the
> bugzilla is a mess), but I still think the patch is useful for more
> robuestness, I dropped the cc: stable and applied it to drm-misc.

Agreed on the bug [1] being a mess. However, the bug has a reliable
bisect result, the revert was posted by some of the reporters on the
lists and in the bug, and now something that will not help anyone in
v4.9 or v4.10 was pushed. :(

BR,
Jani.


[1] https://bugs.freedesktop.org/show_bug.cgi?id=96781

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 1/2] drm: refernce count event->completion

[Jani Nikula]

On Wed, 04 Jan 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Dec 21, 2016 at 12:08:45PM +0100, Maarten Lankhorst wrote:
>> Op 21-12-16 om 11:36 schreef Chris Wilson:
>> > On Wed, Dec 21, 2016 at 11:23:30AM +0100, Daniel Vetter wrote:
>> >> When writing the generic nonblocking commit code I assumed that
>> >> through clever lifetime management I can assure that the completion
>> >> (stored in drm_crtc_commit) only gets freed after it is completed. And
>> >> that worked.
>> >>
>> >> I also wanted to make nonblocking helpers resilient against driver
>> >> bugs, by having timeouts everywhere. And that worked too.
>> >>
>> >> Unfortunately taking boths things together results in oopses :( Well,
>> >> at least sometimes: What seems to happen is that the drm event hangs
>> >> around forever stuck in limbo land. The nonblocking helpers eventually
>> >> time out, move on and release it. Now the bug I tested all this
>> >> against is drivers that just entirely fail to deliver the vblank
>> >> events like they should, and in those cases the event is simply
>> >> leaked. But what seems to happen, at least sometimes, on i915 is that
>> >> the event is set up correctly, but somohow the vblank fails to fire in
>> >> time. Which means the event isn't leaked, it's still there waiting for
>> >> evevntually a vblank to fire. That tends to happen when re-enabling the
>> >> pipe, and then the trap springs and the kernel oopses.
>> >>
>> >> The correct fix here is simply to refcount the crtc commit to make
>> >> sure that the event sticks around even for drivers which only
>> >> sometimes fail to deliver vblanks for some arbitrary reasons. Since
>> >> crtc commits are already refcounted that's easy to do.
>> > Or make the event a part of the atomic state?
>> > -Chris
>> >
>> afaict crtc commit is already taken to wait for completion, so this patch makes sense.
>> 
>> There's just a minor typo in the subject. :)
>> Not sure that release_commit should be a function pointer, regardless..
>> 
>> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
>
> It didn't help the bug reporters against oopses (but the reporters are
> supremely confusing, I have no idea what's really being tested, the
> bugzilla is a mess), but I still think the patch is useful for more
> robuestness, I dropped the cc: stable and applied it to drm-misc.

Agreed on the bug [1] being a mess. However, the bug has a reliable
bisect result, the revert was posted by some of the reporters on the
lists and in the bug, and now something that will not help anyone in
v4.9 or v4.10 was pushed. :(

BR,
Jani.


[1] https://bugs.freedesktop.org/show_bug.cgi?id=96781

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm: refernce count event->completion

[Daniel Vetter]

On Thu, Feb 09, 2017 at 04:39:29PM +0200, Jani Nikula wrote:
> On Wed, 04 Jan 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Dec 21, 2016 at 12:08:45PM +0100, Maarten Lankhorst wrote:
> >> Op 21-12-16 om 11:36 schreef Chris Wilson:
> >> > On Wed, Dec 21, 2016 at 11:23:30AM +0100, Daniel Vetter wrote:
> >> >> When writing the generic nonblocking commit code I assumed that
> >> >> through clever lifetime management I can assure that the completion
> >> >> (stored in drm_crtc_commit) only gets freed after it is completed. And
> >> >> that worked.
> >> >>
> >> >> I also wanted to make nonblocking helpers resilient against driver
> >> >> bugs, by having timeouts everywhere. And that worked too.
> >> >>
> >> >> Unfortunately taking boths things together results in oopses :( Well,
> >> >> at least sometimes: What seems to happen is that the drm event hangs
> >> >> around forever stuck in limbo land. The nonblocking helpers eventually
> >> >> time out, move on and release it. Now the bug I tested all this
> >> >> against is drivers that just entirely fail to deliver the vblank
> >> >> events like they should, and in those cases the event is simply
> >> >> leaked. But what seems to happen, at least sometimes, on i915 is that
> >> >> the event is set up correctly, but somohow the vblank fails to fire in
> >> >> time. Which means the event isn't leaked, it's still there waiting for
> >> >> evevntually a vblank to fire. That tends to happen when re-enabling the
> >> >> pipe, and then the trap springs and the kernel oopses.
> >> >>
> >> >> The correct fix here is simply to refcount the crtc commit to make
> >> >> sure that the event sticks around even for drivers which only
> >> >> sometimes fail to deliver vblanks for some arbitrary reasons. Since
> >> >> crtc commits are already refcounted that's easy to do.
> >> > Or make the event a part of the atomic state?
> >> > -Chris
> >> >
> >> afaict crtc commit is already taken to wait for completion, so this patch makes sense.
> >> 
> >> There's just a minor typo in the subject. :)
> >> Not sure that release_commit should be a function pointer, regardless..
> >> 
> >> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >
> > It didn't help the bug reporters against oopses (but the reporters are
> > supremely confusing, I have no idea what's really being tested, the
> > bugzilla is a mess), but I still think the patch is useful for more
> > robuestness, I dropped the cc: stable and applied it to drm-misc.
> 
> Agreed on the bug [1] being a mess. However, the bug has a reliable
> bisect result, the revert was posted by some of the reporters on the
> lists and in the bug, and now something that will not help anyone in
> v4.9 or v4.10 was pushed. :(

Latest report just says that the revert isn't helping either. I suspect
the report is a giantic conflagration of everything ever that kills
various reporters boxes. I still believe that the patch here fixes the
original bug, but there might be a lot more hiding.

It's at least seen quite a pile of testing, so I think it's sounds, and we
could cherry-pick it to dinf with cc: stable for 4.9+. Worst case it's not
going to help for the other problems.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 1/2] drm: refernce count event->completion

[Daniel Vetter]

On Thu, Feb 09, 2017 at 04:39:29PM +0200, Jani Nikula wrote:
> On Wed, 04 Jan 2017, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Wed, Dec 21, 2016 at 12:08:45PM +0100, Maarten Lankhorst wrote:
> >> Op 21-12-16 om 11:36 schreef Chris Wilson:
> >> > On Wed, Dec 21, 2016 at 11:23:30AM +0100, Daniel Vetter wrote:
> >> >> When writing the generic nonblocking commit code I assumed that
> >> >> through clever lifetime management I can assure that the completion
> >> >> (stored in drm_crtc_commit) only gets freed after it is completed. And
> >> >> that worked.
> >> >>
> >> >> I also wanted to make nonblocking helpers resilient against driver
> >> >> bugs, by having timeouts everywhere. And that worked too.
> >> >>
> >> >> Unfortunately taking boths things together results in oopses :( Well,
> >> >> at least sometimes: What seems to happen is that the drm event hangs
> >> >> around forever stuck in limbo land. The nonblocking helpers eventually
> >> >> time out, move on and release it. Now the bug I tested all this
> >> >> against is drivers that just entirely fail to deliver the vblank
> >> >> events like they should, and in those cases the event is simply
> >> >> leaked. But what seems to happen, at least sometimes, on i915 is that
> >> >> the event is set up correctly, but somohow the vblank fails to fire in
> >> >> time. Which means the event isn't leaked, it's still there waiting for
> >> >> evevntually a vblank to fire. That tends to happen when re-enabling the
> >> >> pipe, and then the trap springs and the kernel oopses.
> >> >>
> >> >> The correct fix here is simply to refcount the crtc commit to make
> >> >> sure that the event sticks around even for drivers which only
> >> >> sometimes fail to deliver vblanks for some arbitrary reasons. Since
> >> >> crtc commits are already refcounted that's easy to do.
> >> > Or make the event a part of the atomic state?
> >> > -Chris
> >> >
> >> afaict crtc commit is already taken to wait for completion, so this patch makes sense.
> >> 
> >> There's just a minor typo in the subject. :)
> >> Not sure that release_commit should be a function pointer, regardless..
> >> 
> >> Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> >
> > It didn't help the bug reporters against oopses (but the reporters are
> > supremely confusing, I have no idea what's really being tested, the
> > bugzilla is a mess), but I still think the patch is useful for more
> > robuestness, I dropped the cc: stable and applied it to drm-misc.
> 
> Agreed on the bug [1] being a mess. However, the bug has a reliable
> bisect result, the revert was posted by some of the reporters on the
> lists and in the bug, and now something that will not help anyone in
> v4.9 or v4.10 was pushed. :(

Latest report just says that the revert isn't helping either. I suspect
the report is a giantic conflagration of everything ever that kills
various reporters boxes. I still believe that the patch here fixes the
original bug, but there might be a lot more hiding.

It's at least seen quite a pile of testing, so I think it's sounds, and we
could cherry-pick it to dinf with cc: stable for 4.9+. Worst case it's not
going to help for the other problems.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm: refernce count event->completion

[Jim Rees]

Daniel Vetter wrote:

  Latest report just says that the revert isn't helping either. I suspect
  the report is a giantic conflagration of everything ever that kills
  various reporters boxes. I still believe that the patch here fixes the
  original bug, but there might be a lot more hiding.
  
  It's at least seen quite a pile of testing, so I think it's sounds, and we
  could cherry-pick it to dinf with cc: stable for 4.9+. Worst case it's not
  going to help for the other problems.

No, that's not what the latest report says. It says, "running for 2 weeks
... This is certainly way, way better than the current stock experience,
which results in my T460s entirely locking up daily." and "Less than a day
after I made that comment I got a hard lockup". So reverting the buggy
helper nonblock tracking commit took this reporter from locking up daily to
locking up once in two weeks. For everyone else, reverting the buggy commit
fixes all bugs. Also note that this most recent lockup appears to be a
different bug ("GPU HANG: ecode").

So we have a commit that is causing hard lockups and flip_done timeouts for
multiple users. Reverting this commit fixes the problem. But we did not push
the revert up for 4.9, and it looks like we're not going to push it up for
4.10 either.

Re: [PATCH 1/2] drm: refernce count event->completion

[Jim Rees]

Daniel Vetter wrote:

  Latest report just says that the revert isn't helping either. I suspect
  the report is a giantic conflagration of everything ever that kills
  various reporters boxes. I still believe that the patch here fixes the
  original bug, but there might be a lot more hiding.
  
  It's at least seen quite a pile of testing, so I think it's sounds, and we
  could cherry-pick it to dinf with cc: stable for 4.9+. Worst case it's not
  going to help for the other problems.

No, that's not what the latest report says. It says, "running for 2 weeks
... This is certainly way, way better than the current stock experience,
which results in my T460s entirely locking up daily." and "Less than a day
after I made that comment I got a hard lockup". So reverting the buggy
helper nonblock tracking commit took this reporter from locking up daily to
locking up once in two weeks. For everyone else, reverting the buggy commit
fixes all bugs. Also note that this most recent lockup appears to be a
different bug ("GPU HANG: ecode").

So we have a commit that is causing hard lockups and flip_done timeouts for
multiple users. Reverting this commit fixes the problem. But we did not push
the revert up for 4.9, and it looks like we're not going to push it up for
4.10 either.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx