[PATCH 1/2] dma-buf/dma-fence: Trim dma_fence_add_callback()

[PATCH 1/2] dma-buf/dma-fence: Trim dma_fence_add_callback()

[Chris Wilson]

Rearrange the code to pull the operations beore the fence->lock critical
section, and remove a small amount of redundancy:

Function                                     old     new   delta
dma_fence_add_callback                       156     145     -11

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/dma-buf/dma-fence.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 656e9ac2d028..8d5bdfce638e 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -348,29 +348,25 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling);
 int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb,
 			   dma_fence_func_t func)
 {
-	unsigned long flags;
-	int ret = 0;
+	int ret = -ENOENT;
 
 	if (WARN_ON(!fence || !func))
 		return -EINVAL;
 
-	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
-		INIT_LIST_HEAD(&cb->node);
-		return -ENOENT;
-	}
+	cb->func = func;
+	INIT_LIST_HEAD(&cb->node);
 
-	spin_lock_irqsave(fence->lock, flags);
+	if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
+		unsigned long flags;
 
-	if (__dma_fence_enable_signaling(fence)) {
-		cb->func = func;
-		list_add_tail(&cb->node, &fence->cb_list);
-	} else {
-		INIT_LIST_HEAD(&cb->node);
-		ret = -ENOENT;
+		spin_lock_irqsave(fence->lock, flags);
+		if (__dma_fence_enable_signaling(fence)) {
+			list_add_tail(&cb->node, &fence->cb_list);
+			ret = 0;
+		}
+		spin_unlock_irqrestore(fence->lock, flags);
 	}
 
-	spin_unlock_irqrestore(fence->lock, flags);
-
 	return ret;
 }
 EXPORT_SYMBOL(dma_fence_add_callback);
-- 
2.20.1

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

[Intel-gfx] [PATCH 1/2] dma-buf/dma-fence: Trim dma_fence_add_callback()

[Chris Wilson]

Rearrange the code to pull the operations beore the fence->lock critical
section, and remove a small amount of redundancy:

Function                                     old     new   delta
dma_fence_add_callback                       156     145     -11

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/dma-buf/dma-fence.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 656e9ac2d028..8d5bdfce638e 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -348,29 +348,25 @@ EXPORT_SYMBOL(dma_fence_enable_sw_signaling);
 int dma_fence_add_callback(struct dma_fence *fence, struct dma_fence_cb *cb,
 			   dma_fence_func_t func)
 {
-	unsigned long flags;
-	int ret = 0;
+	int ret = -ENOENT;
 
 	if (WARN_ON(!fence || !func))
 		return -EINVAL;
 
-	if (test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
-		INIT_LIST_HEAD(&cb->node);
-		return -ENOENT;
-	}
+	cb->func = func;
+	INIT_LIST_HEAD(&cb->node);
 
-	spin_lock_irqsave(fence->lock, flags);
+	if (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags)) {
+		unsigned long flags;
 
-	if (__dma_fence_enable_signaling(fence)) {
-		cb->func = func;
-		list_add_tail(&cb->node, &fence->cb_list);
-	} else {
-		INIT_LIST_HEAD(&cb->node);
-		ret = -ENOENT;
+		spin_lock_irqsave(fence->lock, flags);
+		if (__dma_fence_enable_signaling(fence)) {
+			list_add_tail(&cb->node, &fence->cb_list);
+			ret = 0;
+		}
+		spin_unlock_irqrestore(fence->lock, flags);
 	}
 
-	spin_unlock_irqrestore(fence->lock, flags);
-
 	return ret;
 }
 EXPORT_SYMBOL(dma_fence_add_callback);
-- 
2.20.1

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

[PATCH 2/2] dma-buf/dma-fence: Add quick tests before dma_fence_remove_callback

[Chris Wilson]

When waiting with a callback on the stack, we must remove the callback
upon wait completion. Since this will be notified by the fence signal
callback, the removal often contends with the fence->lock being held by
the signaler. We can look at the list entry to see if the callback was
already signaled before we take the contended lock.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/dma-buf/dma-fence.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 8d5bdfce638e..b910d7bc0854 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -420,6 +420,9 @@ dma_fence_remove_callback(struct dma_fence *fence, struct dma_fence_cb *cb)
 	unsigned long flags;
 	bool ret;
 
+	if (list_empty(&cb->node))
+		return false;
+
 	spin_lock_irqsave(fence->lock, flags);
 
 	ret = !list_empty(&cb->node);
-- 
2.20.1

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

[Intel-gfx] [PATCH 2/2] dma-buf/dma-fence: Add quick tests before dma_fence_remove_callback

[Chris Wilson]

When waiting with a callback on the stack, we must remove the callback
upon wait completion. Since this will be notified by the fence signal
callback, the removal often contends with the fence->lock being held by
the signaler. We can look at the list entry to see if the callback was
already signaled before we take the contended lock.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/dma-buf/dma-fence.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index 8d5bdfce638e..b910d7bc0854 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -420,6 +420,9 @@ dma_fence_remove_callback(struct dma_fence *fence, struct dma_fence_cb *cb)
 	unsigned long flags;
 	bool ret;
 
+	if (list_empty(&cb->node))
+		return false;
+
 	spin_lock_irqsave(fence->lock, flags);
 
 	ret = !list_empty(&cb->node);
-- 
2.20.1

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

[Intel-gfx] ✓ Fi.CI.BAT: success for series starting with [1/2] dma-buf/dma-fence: Trim dma_fence_add_callback()

[Patchwork]

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

== Series Details ==

Series: series starting with [1/2] dma-buf/dma-fence: Trim dma_fence_add_callback()
URL   : https://patchwork.freedesktop.org/series/79513/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_8748 -> Patchwork_18174
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/index.html

Known issues
------------

  Here are the changes found in Patchwork_18174 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_flink_basic@basic:
    - fi-tgl-y:           [PASS][1] -> [DMESG-WARN][2] ([i915#402])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/fi-tgl-y/igt@gem_flink_basic@basic.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/fi-tgl-y/igt@gem_flink_basic@basic.html

  * igt@i915_pm_rpm@module-reload:
    - fi-byt-j1900:       [PASS][3] -> [DMESG-WARN][4] ([i915#1982]) +1 similar issue
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/fi-byt-j1900/igt@i915_pm_rpm@module-reload.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/fi-byt-j1900/igt@i915_pm_rpm@module-reload.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
    - fi-icl-u2:          [PASS][5] -> [DMESG-WARN][6] ([i915#1982])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/fi-icl-u2/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/fi-icl-u2/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html

  
#### Possible fixes ####

  * igt@gem_exec_suspend@basic-s0:
    - fi-tgl-u2:          [FAIL][7] ([i915#1888]) -> [PASS][8]
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/fi-tgl-u2/igt@gem_exec_suspend@basic-s0.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/fi-tgl-u2/igt@gem_exec_suspend@basic-s0.html

  * igt@i915_pm_rpm@basic-pci-d3-state:
    - fi-bsw-kefka:       [DMESG-WARN][9] ([i915#1982]) -> [PASS][10]
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/fi-bsw-kefka/igt@i915_pm_rpm@basic-pci-d3-state.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/fi-bsw-kefka/igt@i915_pm_rpm@basic-pci-d3-state.html

  * igt@kms_busy@basic@flip:
    - fi-tgl-y:           [DMESG-WARN][11] ([i915#1982]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/fi-tgl-y/igt@kms_busy@basic@flip.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/fi-tgl-y/igt@kms_busy@basic@flip.html

  * igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
    - {fi-kbl-7560u}:     [DMESG-WARN][13] ([i915#1982]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/fi-kbl-7560u/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/fi-kbl-7560u/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html

  * igt@kms_flip@basic-flip-vs-wf_vblank@c-edp1:
    - fi-icl-u2:          [DMESG-WARN][15] ([i915#1982]) -> [PASS][16] +1 similar issue
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/fi-icl-u2/igt@kms_flip@basic-flip-vs-wf_vblank@c-edp1.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/fi-icl-u2/igt@kms_flip@basic-flip-vs-wf_vblank@c-edp1.html

  * igt@vgem_basic@setversion:
    - fi-tgl-y:           [DMESG-WARN][17] ([i915#402]) -> [PASS][18] +1 similar issue
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/fi-tgl-y/igt@vgem_basic@setversion.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/fi-tgl-y/igt@vgem_basic@setversion.html

  
#### Warnings ####

  * igt@debugfs_test@read_all_entries:
    - fi-kbl-x1275:       [DMESG-WARN][19] ([i915#62] / [i915#92] / [i915#95]) -> [DMESG-WARN][20] ([i915#62] / [i915#92]) +2 similar issues
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/fi-kbl-x1275/igt@debugfs_test@read_all_entries.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/fi-kbl-x1275/igt@debugfs_test@read_all_entries.html

  * igt@kms_flip@basic-flip-vs-modeset@a-dp1:
    - fi-kbl-x1275:       [DMESG-WARN][21] ([i915#62] / [i915#92]) -> [DMESG-WARN][22] ([i915#62] / [i915#92] / [i915#95]) +2 similar issues
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/fi-kbl-x1275/igt@kms_flip@basic-flip-vs-modeset@a-dp1.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/fi-kbl-x1275/igt@kms_flip@basic-flip-vs-modeset@a-dp1.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#1888]: https://gitlab.freedesktop.org/drm/intel/issues/1888
  [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
  [i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402
  [i915#62]: https://gitlab.freedesktop.org/drm/intel/issues/62
  [i915#92]: https://gitlab.freedesktop.org/drm/intel/issues/92
  [i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95


Participating hosts (46 -> 40)
------------------------------

  Missing    (6): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper 


Build changes
-------------

  * Linux: CI_DRM_8748 -> Patchwork_18174

  CI-20190529: 20190529
  CI_DRM_8748: 14b60366c7e4ab1c5f3f79673951a1eda4c403e6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5735: 21f8204e54c122e4a0f8ca4b59e4b2db8d1ba687 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_18174: b42802725e73535e7d7ab6bdeaf6f2f39b97d3b7 @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

b42802725e73 dma-buf/dma-fence: Add quick tests before dma_fence_remove_callback
a3067d9a8e4e dma-buf/dma-fence: Trim dma_fence_add_callback()

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/index.html

[-- Attachment #1.2: Type: text/html, Size: 7784 bytes --]

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

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

Re: [Intel-gfx] [PATCH 2/2] dma-buf/dma-fence: Add quick tests before dma_fence_remove_callback

[Daniel Vetter]

On Wed, Jul 15, 2020 at 11:49:05AM +0100, Chris Wilson wrote:
> When waiting with a callback on the stack, we must remove the callback
> upon wait completion. Since this will be notified by the fence signal
> callback, the removal often contends with the fence->lock being held by
> the signaler. We can look at the list entry to see if the callback was
> already signaled before we take the contended lock.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/dma-buf/dma-fence.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index 8d5bdfce638e..b910d7bc0854 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -420,6 +420,9 @@ dma_fence_remove_callback(struct dma_fence *fence, struct dma_fence_cb *cb)
>  	unsigned long flags;
>  	bool ret;
>  
> +	if (list_empty(&cb->node))

I was about to say "but the races" but then noticed that Paul fixed
list_empty to use READ_ONCE like 5 years ago :-)

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +		return false;
> +
>  	spin_lock_irqsave(fence->lock, flags);
>  
>  	ret = !list_empty(&cb->node);
> -- 
> 2.20.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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: [Intel-gfx] [PATCH 2/2] dma-buf/dma-fence: Add quick tests before dma_fence_remove_callback

[Daniel Vetter]

On Wed, Jul 15, 2020 at 11:49:05AM +0100, Chris Wilson wrote:
> When waiting with a callback on the stack, we must remove the callback
> upon wait completion. Since this will be notified by the fence signal
> callback, the removal often contends with the fence->lock being held by
> the signaler. We can look at the list entry to see if the callback was
> already signaled before we take the contended lock.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/dma-buf/dma-fence.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index 8d5bdfce638e..b910d7bc0854 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -420,6 +420,9 @@ dma_fence_remove_callback(struct dma_fence *fence, struct dma_fence_cb *cb)
>  	unsigned long flags;
>  	bool ret;
>  
> +	if (list_empty(&cb->node))

I was about to say "but the races" but then noticed that Paul fixed
list_empty to use READ_ONCE like 5 years ago :-)

Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>

> +		return false;
> +
>  	spin_lock_irqsave(fence->lock, flags);
>  
>  	ret = !list_empty(&cb->node);
> -- 
> 2.20.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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: [Intel-gfx] [PATCH 2/2] dma-buf/dma-fence: Add quick tests before dma_fence_remove_callback

[Chris Wilson]

Quoting Daniel Vetter (2020-07-15 13:10:22)
> On Wed, Jul 15, 2020 at 11:49:05AM +0100, Chris Wilson wrote:
> > When waiting with a callback on the stack, we must remove the callback
> > upon wait completion. Since this will be notified by the fence signal
> > callback, the removal often contends with the fence->lock being held by
> > the signaler. We can look at the list entry to see if the callback was
> > already signaled before we take the contended lock.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/dma-buf/dma-fence.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > index 8d5bdfce638e..b910d7bc0854 100644
> > --- a/drivers/dma-buf/dma-fence.c
> > +++ b/drivers/dma-buf/dma-fence.c
> > @@ -420,6 +420,9 @@ dma_fence_remove_callback(struct dma_fence *fence, struct dma_fence_cb *cb)
> >       unsigned long flags;
> >       bool ret;
> >  
> > +     if (list_empty(&cb->node))
> 
> I was about to say "but the races" but then noticed that Paul fixed
> list_empty to use READ_ONCE like 5 years ago :-)

I'm always going "when exactly do we need list_empty_careful()"?

We can rule out a concurrent dma_fence_add_callback() for the same
dma_fence_cb, as that is a lost cause. So we only have to worry about
the concurrent list_del_init() from dma_fence_signal_locked(). So it's
the timing of
	list_del_init(): WRITE_ONCE(list->next, list)
vs
	READ_ONCE(list->next) == list
and we don't need to care about the trailing instructions in
list_del_init()...

Wait that trailing instruction is actually important here if the
dma_fence_cb is on the stack, or other imminent free.

Ok, this does need to be list_empty_careful!
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 2/2] dma-buf/dma-fence: Add quick tests before dma_fence_remove_callback

[Chris Wilson]

Quoting Daniel Vetter (2020-07-15 13:10:22)
> On Wed, Jul 15, 2020 at 11:49:05AM +0100, Chris Wilson wrote:
> > When waiting with a callback on the stack, we must remove the callback
> > upon wait completion. Since this will be notified by the fence signal
> > callback, the removal often contends with the fence->lock being held by
> > the signaler. We can look at the list entry to see if the callback was
> > already signaled before we take the contended lock.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/dma-buf/dma-fence.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > index 8d5bdfce638e..b910d7bc0854 100644
> > --- a/drivers/dma-buf/dma-fence.c
> > +++ b/drivers/dma-buf/dma-fence.c
> > @@ -420,6 +420,9 @@ dma_fence_remove_callback(struct dma_fence *fence, struct dma_fence_cb *cb)
> >       unsigned long flags;
> >       bool ret;
> >  
> > +     if (list_empty(&cb->node))
> 
> I was about to say "but the races" but then noticed that Paul fixed
> list_empty to use READ_ONCE like 5 years ago :-)

I'm always going "when exactly do we need list_empty_careful()"?

We can rule out a concurrent dma_fence_add_callback() for the same
dma_fence_cb, as that is a lost cause. So we only have to worry about
the concurrent list_del_init() from dma_fence_signal_locked(). So it's
the timing of
	list_del_init(): WRITE_ONCE(list->next, list)
vs
	READ_ONCE(list->next) == list
and we don't need to care about the trailing instructions in
list_del_init()...

Wait that trailing instruction is actually important here if the
dma_fence_cb is on the stack, or other imminent free.

Ok, this does need to be list_empty_careful!
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] dma-buf/dma-fence: Add quick tests before dma_fence_remove_callback

[Chris Wilson]

Quoting Chris Wilson (2020-07-15 13:21:43)
> Quoting Daniel Vetter (2020-07-15 13:10:22)
> > On Wed, Jul 15, 2020 at 11:49:05AM +0100, Chris Wilson wrote:
> > > When waiting with a callback on the stack, we must remove the callback
> > > upon wait completion. Since this will be notified by the fence signal
> > > callback, the removal often contends with the fence->lock being held by
> > > the signaler. We can look at the list entry to see if the callback was
> > > already signaled before we take the contended lock.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >  drivers/dma-buf/dma-fence.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > > index 8d5bdfce638e..b910d7bc0854 100644
> > > --- a/drivers/dma-buf/dma-fence.c
> > > +++ b/drivers/dma-buf/dma-fence.c
> > > @@ -420,6 +420,9 @@ dma_fence_remove_callback(struct dma_fence *fence, struct dma_fence_cb *cb)
> > >       unsigned long flags;
> > >       bool ret;
> > >  
> > > +     if (list_empty(&cb->node))
> > 
> > I was about to say "but the races" but then noticed that Paul fixed
> > list_empty to use READ_ONCE like 5 years ago :-)
> 
> I'm always going "when exactly do we need list_empty_careful()"?
> 
> We can rule out a concurrent dma_fence_add_callback() for the same
> dma_fence_cb, as that is a lost cause. So we only have to worry about
> the concurrent list_del_init() from dma_fence_signal_locked(). So it's
> the timing of
>         list_del_init(): WRITE_ONCE(list->next, list)
> vs
>         READ_ONCE(list->next) == list
> and we don't need to care about the trailing instructions in
> list_del_init()...
> 
> Wait that trailing instruction is actually important here if the
> dma_fence_cb is on the stack, or other imminent free.
> 
> Ok, this does need to be list_empty_careful!

There's a further problem in that we call INIT_LIST_HEAD on the
dma_fence_cb before the signal callback. So even if list_empty_careful()
confirms the dma_fence_cb to be completely decoupled, the containing
struct may still be inuse.
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 2/2] dma-buf/dma-fence: Add quick tests before dma_fence_remove_callback

[Chris Wilson]

Quoting Chris Wilson (2020-07-15 13:21:43)
> Quoting Daniel Vetter (2020-07-15 13:10:22)
> > On Wed, Jul 15, 2020 at 11:49:05AM +0100, Chris Wilson wrote:
> > > When waiting with a callback on the stack, we must remove the callback
> > > upon wait completion. Since this will be notified by the fence signal
> > > callback, the removal often contends with the fence->lock being held by
> > > the signaler. We can look at the list entry to see if the callback was
> > > already signaled before we take the contended lock.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > ---
> > >  drivers/dma-buf/dma-fence.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > > index 8d5bdfce638e..b910d7bc0854 100644
> > > --- a/drivers/dma-buf/dma-fence.c
> > > +++ b/drivers/dma-buf/dma-fence.c
> > > @@ -420,6 +420,9 @@ dma_fence_remove_callback(struct dma_fence *fence, struct dma_fence_cb *cb)
> > >       unsigned long flags;
> > >       bool ret;
> > >  
> > > +     if (list_empty(&cb->node))
> > 
> > I was about to say "but the races" but then noticed that Paul fixed
> > list_empty to use READ_ONCE like 5 years ago :-)
> 
> I'm always going "when exactly do we need list_empty_careful()"?
> 
> We can rule out a concurrent dma_fence_add_callback() for the same
> dma_fence_cb, as that is a lost cause. So we only have to worry about
> the concurrent list_del_init() from dma_fence_signal_locked(). So it's
> the timing of
>         list_del_init(): WRITE_ONCE(list->next, list)
> vs
>         READ_ONCE(list->next) == list
> and we don't need to care about the trailing instructions in
> list_del_init()...
> 
> Wait that trailing instruction is actually important here if the
> dma_fence_cb is on the stack, or other imminent free.
> 
> Ok, this does need to be list_empty_careful!

There's a further problem in that we call INIT_LIST_HEAD on the
dma_fence_cb before the signal callback. So even if list_empty_careful()
confirms the dma_fence_cb to be completely decoupled, the containing
struct may still be inuse.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.IGT: failure for series starting with [1/2] dma-buf/dma-fence: Trim dma_fence_add_callback()

[Patchwork]

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

== Series Details ==

Series: series starting with [1/2] dma-buf/dma-fence: Trim dma_fence_add_callback()
URL   : https://patchwork.freedesktop.org/series/79513/
State : failure

== Summary ==

CI Bug Log - changes from CI_DRM_8748_full -> Patchwork_18174_full
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with Patchwork_18174_full absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in Patchwork_18174_full, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in Patchwork_18174_full:

### IGT changes ###

#### Possible regressions ####

  * igt@gem_exec_fence@parallel@rcs0:
    - shard-iclb:         [PASS][1] -> [INCOMPLETE][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-iclb7/igt@gem_exec_fence@parallel@rcs0.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-iclb2/igt@gem_exec_fence@parallel@rcs0.html

  
Known issues
------------

  Here are the changes found in Patchwork_18174_full that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_fence@concurrent@rcs0:
    - shard-apl:          [PASS][3] -> [INCOMPLETE][4] ([i915#1635])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-apl1/igt@gem_exec_fence@concurrent@rcs0.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-apl8/igt@gem_exec_fence@concurrent@rcs0.html

  * igt@gem_exec_gttfill@all:
    - shard-glk:          [PASS][5] -> [DMESG-WARN][6] ([i915#118] / [i915#95]) +1 similar issue
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-glk7/igt@gem_exec_gttfill@all.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-glk1/igt@gem_exec_gttfill@all.html

  * igt@i915_module_load@reload:
    - shard-tglb:         [PASS][7] -> [DMESG-WARN][8] ([i915#402]) +2 similar issues
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-tglb1/igt@i915_module_load@reload.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-tglb2/igt@i915_module_load@reload.html

  * igt@kms_big_fb@y-tiled-64bpp-rotate-0:
    - shard-glk:          [PASS][9] -> [DMESG-FAIL][10] ([i915#118] / [i915#95])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-glk2/igt@kms_big_fb@y-tiled-64bpp-rotate-0.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-glk8/igt@kms_big_fb@y-tiled-64bpp-rotate-0.html

  * igt@kms_flip@basic-plain-flip@a-edp1:
    - shard-skl:          [PASS][11] -> [DMESG-WARN][12] ([i915#1982]) +8 similar issues
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-skl6/igt@kms_flip@basic-plain-flip@a-edp1.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-skl8/igt@kms_flip@basic-plain-flip@a-edp1.html

  * igt@kms_flip@flip-vs-expired-vblank-interruptible@c-edp1:
    - shard-skl:          [PASS][13] -> [FAIL][14] ([i915#79])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-skl2/igt@kms_flip@flip-vs-expired-vblank-interruptible@c-edp1.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-skl10/igt@kms_flip@flip-vs-expired-vblank-interruptible@c-edp1.html

  * igt@kms_flip@flip-vs-expired-vblank@b-hdmi-a2:
    - shard-glk:          [PASS][15] -> [FAIL][16] ([i915#79])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-glk6/igt@kms_flip@flip-vs-expired-vblank@b-hdmi-a2.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-glk9/igt@kms_flip@flip-vs-expired-vblank@b-hdmi-a2.html

  * igt@kms_frontbuffer_tracking@fbcpsr-stridechange:
    - shard-tglb:         [PASS][17] -> [DMESG-WARN][18] ([i915#1982])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-tglb2/igt@kms_frontbuffer_tracking@fbcpsr-stridechange.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-tglb2/igt@kms_frontbuffer_tracking@fbcpsr-stridechange.html

  * igt@kms_hdr@bpc-switch-suspend:
    - shard-skl:          [PASS][19] -> [FAIL][20] ([i915#1188])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-skl10/igt@kms_hdr@bpc-switch-suspend.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-skl6/igt@kms_hdr@bpc-switch-suspend.html

  * igt@kms_plane@plane-panning-top-left-pipe-a-planes:
    - shard-iclb:         [PASS][21] -> [DMESG-WARN][22] ([i915#1982]) +1 similar issue
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-iclb8/igt@kms_plane@plane-panning-top-left-pipe-a-planes.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-iclb3/igt@kms_plane@plane-panning-top-left-pipe-a-planes.html

  * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
    - shard-skl:          [PASS][23] -> [FAIL][24] ([fdo#108145] / [i915#265]) +1 similar issue
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-skl10/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-skl7/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html

  * igt@kms_psr@no_drrs:
    - shard-iclb:         [PASS][25] -> [FAIL][26] ([i915#173])
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-iclb2/igt@kms_psr@no_drrs.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-iclb1/igt@kms_psr@no_drrs.html

  * igt@kms_psr@psr2_sprite_plane_move:
    - shard-iclb:         [PASS][27] -> [SKIP][28] ([fdo#109441]) +2 similar issues
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-iclb2/igt@kms_psr@psr2_sprite_plane_move.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-iclb4/igt@kms_psr@psr2_sprite_plane_move.html

  * igt@kms_vblank@pipe-a-ts-continuation-suspend:
    - shard-kbl:          [PASS][29] -> [DMESG-WARN][30] ([i915#180]) +3 similar issues
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-kbl6/igt@kms_vblank@pipe-a-ts-continuation-suspend.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-kbl7/igt@kms_vblank@pipe-a-ts-continuation-suspend.html

  
#### Possible fixes ####

  * igt@gem_ctx_isolation@preservation-s3@vecs0:
    - shard-iclb:         [INCOMPLETE][31] ([i915#1185]) -> [PASS][32]
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-iclb3/igt@gem_ctx_isolation@preservation-s3@vecs0.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-iclb8/igt@gem_ctx_isolation@preservation-s3@vecs0.html

  * igt@gem_ctx_persistence@engines-mixed-process@vecs0:
    - shard-iclb:         [FAIL][33] ([i915#1528]) -> [PASS][34]
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-iclb5/igt@gem_ctx_persistence@engines-mixed-process@vecs0.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-iclb8/igt@gem_ctx_persistence@engines-mixed-process@vecs0.html

  * igt@gem_exec_whisper@basic-contexts-priority-all:
    - shard-glk:          [DMESG-WARN][35] ([i915#118] / [i915#95]) -> [PASS][36]
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-glk1/igt@gem_exec_whisper@basic-contexts-priority-all.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-glk9/igt@gem_exec_whisper@basic-contexts-priority-all.html

  * igt@kms_color@pipe-c-ctm-0-25:
    - shard-skl:          [DMESG-WARN][37] ([i915#1982]) -> [PASS][38] +11 similar issues
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-skl7/igt@kms_color@pipe-c-ctm-0-25.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-skl5/igt@kms_color@pipe-c-ctm-0-25.html

  * igt@kms_cursor_crc@pipe-b-cursor-64x64-sliding:
    - shard-skl:          [FAIL][39] ([i915#54]) -> [PASS][40]
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-skl3/igt@kms_cursor_crc@pipe-b-cursor-64x64-sliding.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-skl4/igt@kms_cursor_crc@pipe-b-cursor-64x64-sliding.html

  * igt@kms_draw_crc@draw-method-xrgb2101010-mmap-gtt-untiled:
    - shard-kbl:          [DMESG-WARN][41] ([i915#1982]) -> [PASS][42] +1 similar issue
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-kbl4/igt@kms_draw_crc@draw-method-xrgb2101010-mmap-gtt-untiled.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-kbl4/igt@kms_draw_crc@draw-method-xrgb2101010-mmap-gtt-untiled.html

  * igt@kms_flip@2x-flip-vs-wf_vblank-interruptible@ab-vga1-hdmi-a1:
    - shard-hsw:          [DMESG-WARN][43] ([i915#1982]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-hsw6/igt@kms_flip@2x-flip-vs-wf_vblank-interruptible@ab-vga1-hdmi-a1.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-hsw2/igt@kms_flip@2x-flip-vs-wf_vblank-interruptible@ab-vga1-hdmi-a1.html

  * igt@kms_flip@flip-vs-suspend@c-dp1:
    - shard-kbl:          [DMESG-WARN][45] ([i915#180]) -> [PASS][46] +9 similar issues
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-kbl1/igt@kms_flip@flip-vs-suspend@c-dp1.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-kbl1/igt@kms_flip@flip-vs-suspend@c-dp1.html

  * igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-blt:
    - shard-tglb:         [DMESG-WARN][47] ([i915#1982]) -> [PASS][48] +2 similar issues
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-tglb8/igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-blt.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-tglb1/igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-blt.html
    - shard-iclb:         [DMESG-WARN][49] ([i915#1982]) -> [PASS][50]
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-iclb8/igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-blt.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-iclb3/igt@kms_frontbuffer_tracking@fbc-1p-offscren-pri-shrfb-draw-blt.html

  * igt@kms_frontbuffer_tracking@fbc-rgb565-draw-mmap-gtt:
    - shard-snb:          [SKIP][51] ([fdo#109271]) -> [PASS][52]
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-snb2/igt@kms_frontbuffer_tracking@fbc-rgb565-draw-mmap-gtt.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-snb2/igt@kms_frontbuffer_tracking@fbc-rgb565-draw-mmap-gtt.html

  * igt@kms_hdr@bpc-switch-dpms:
    - shard-skl:          [FAIL][53] ([i915#1188]) -> [PASS][54]
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-skl7/igt@kms_hdr@bpc-switch-dpms.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-skl9/igt@kms_hdr@bpc-switch-dpms.html

  * igt@kms_plane_alpha_blend@pipe-b-constant-alpha-min:
    - shard-skl:          [FAIL][55] ([fdo#108145] / [i915#265]) -> [PASS][56]
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-skl3/igt@kms_plane_alpha_blend@pipe-b-constant-alpha-min.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-skl4/igt@kms_plane_alpha_blend@pipe-b-constant-alpha-min.html

  * igt@kms_psr2_su@page_flip:
    - shard-iclb:         [SKIP][57] ([fdo#109642] / [fdo#111068]) -> [PASS][58]
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-iclb7/igt@kms_psr2_su@page_flip.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-iclb2/igt@kms_psr2_su@page_flip.html

  * igt@kms_psr@psr2_primary_page_flip:
    - shard-iclb:         [SKIP][59] ([fdo#109441]) -> [PASS][60]
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-iclb4/igt@kms_psr@psr2_primary_page_flip.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-iclb2/igt@kms_psr@psr2_primary_page_flip.html

  * igt@perf@blocking-parameterized:
    - shard-iclb:         [FAIL][61] ([i915#1542]) -> [PASS][62]
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-iclb8/igt@perf@blocking-parameterized.html
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-iclb3/igt@perf@blocking-parameterized.html

  
#### Warnings ####

  * igt@gen9_exec_parse@bb-start-far:
    - shard-snb:          [TIMEOUT][63] ([i915#1958] / [i915#2119]) -> [INCOMPLETE][64] ([i915#82])
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-snb6/igt@gen9_exec_parse@bb-start-far.html
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-snb1/igt@gen9_exec_parse@bb-start-far.html

  * igt@i915_selftest@mock@requests:
    - shard-apl:          [INCOMPLETE][65] ([i915#1635]) -> [INCOMPLETE][66] ([i915#1635] / [i915#2179])
   [65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-apl6/igt@i915_selftest@mock@requests.html
   [66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-apl6/igt@i915_selftest@mock@requests.html
    - shard-glk:          [INCOMPLETE][67] ([i915#58] / [k.org#198133]) -> [INCOMPLETE][68] ([i915#2179] / [i915#58] / [k.org#198133])
   [67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8748/shard-glk3/igt@i915_selftest@mock@requests.html
   [68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/shard-glk5/igt@i915_selftest@mock@requests.html

  
  [fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
  [fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
  [fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
  [i915#118]: https://gitlab.freedesktop.org/drm/intel/issues/118
  [i915#1185]: https://gitlab.freedesktop.org/drm/intel/issues/1185
  [i915#1188]: https://gitlab.freedesktop.org/drm/intel/issues/1188
  [i915#1528]: https://gitlab.freedesktop.org/drm/intel/issues/1528
  [i915#1542]: https://gitlab.freedesktop.org/drm/intel/issues/1542
  [i915#1635]: https://gitlab.freedesktop.org/drm/intel/issues/1635
  [i915#173]: https://gitlab.freedesktop.org/drm/intel/issues/173
  [i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
  [i915#1958]: https://gitlab.freedesktop.org/drm/intel/issues/1958
  [i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
  [i915#2119]: https://gitlab.freedesktop.org/drm/intel/issues/2119
  [i915#2179]: https://gitlab.freedesktop.org/drm/intel/issues/2179
  [i915#265]: https://gitlab.freedesktop.org/drm/intel/issues/265
  [i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402
  [i915#54]: https://gitlab.freedesktop.org/drm/intel/issues/54
  [i915#58]: https://gitlab.freedesktop.org/drm/intel/issues/58
  [i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79
  [i915#82]: https://gitlab.freedesktop.org/drm/intel/issues/82
  [i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95
  [k.org#198133]: https://bugzilla.kernel.org/show_bug.cgi?id=198133


Participating hosts (10 -> 10)
------------------------------

  No changes in participating hosts


Build changes
-------------

  * Linux: CI_DRM_8748 -> Patchwork_18174

  CI-20190529: 20190529
  CI_DRM_8748: 14b60366c7e4ab1c5f3f79673951a1eda4c403e6 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5735: 21f8204e54c122e4a0f8ca4b59e4b2db8d1ba687 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_18174: b42802725e73535e7d7ab6bdeaf6f2f39b97d3b7 @ git://anongit.freedesktop.org/gfx-ci/linux
  piglit_4509: fdc5a4ca11124ab8413c7988896eec4c97336694 @ git://anongit.freedesktop.org/piglit

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_18174/index.html

[-- Attachment #1.2: Type: text/html, Size: 18829 bytes --]

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

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

Re: [Intel-gfx] [PATCH 2/2] dma-buf/dma-fence: Add quick tests before dma_fence_remove_callback

[Daniel Vetter]

On Wed, Jul 15, 2020 at 2:40 PM Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Quoting Chris Wilson (2020-07-15 13:21:43)
> > Quoting Daniel Vetter (2020-07-15 13:10:22)
> > > On Wed, Jul 15, 2020 at 11:49:05AM +0100, Chris Wilson wrote:
> > > > When waiting with a callback on the stack, we must remove the callback
> > > > upon wait completion. Since this will be notified by the fence signal
> > > > callback, the removal often contends with the fence->lock being held by
> > > > the signaler. We can look at the list entry to see if the callback was
> > > > already signaled before we take the contended lock.
> > > >
> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > ---
> > > >  drivers/dma-buf/dma-fence.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > > > index 8d5bdfce638e..b910d7bc0854 100644
> > > > --- a/drivers/dma-buf/dma-fence.c
> > > > +++ b/drivers/dma-buf/dma-fence.c
> > > > @@ -420,6 +420,9 @@ dma_fence_remove_callback(struct dma_fence *fence, struct dma_fence_cb *cb)
> > > >       unsigned long flags;
> > > >       bool ret;
> > > >
> > > > +     if (list_empty(&cb->node))
> > >
> > > I was about to say "but the races" but then noticed that Paul fixed
> > > list_empty to use READ_ONCE like 5 years ago :-)
> >
> > I'm always going "when exactly do we need list_empty_careful()"?
> >
> > We can rule out a concurrent dma_fence_add_callback() for the same
> > dma_fence_cb, as that is a lost cause. So we only have to worry about
> > the concurrent list_del_init() from dma_fence_signal_locked(). So it's
> > the timing of
> >         list_del_init(): WRITE_ONCE(list->next, list)
> > vs
> >         READ_ONCE(list->next) == list
> > and we don't need to care about the trailing instructions in
> > list_del_init()...
> >
> > Wait that trailing instruction is actually important here if the
> > dma_fence_cb is on the stack, or other imminent free.
> >
> > Ok, this does need to be list_empty_careful!

Hm, tbh I'm not really clear what list_empty_careful does on top.
Seems to lack READ_ONCE, so either some really big trickery with
dependencies is going on, or I'm not even sure how this works without
locks.

I've now stared at list_empty_careful and a bunch of users quite a
bit, and I have now idea when you'd want to use it. Lockless you want
a READ_ONCE I think and a simple check, so list_empty. And just accept
that any time you race you'll go into the locked slowpath for "list
isn't empty". Also only works if the list_empty case is the "nothing
to do, job already done" case, since the other one just isn't
guaranteed to be accurate.

list_empty_careful just wraps a bunch more magic around that will make
this both worse, and more racy (if the compiler feels creative)
because no READ_ONCE or anything like that. I don't get what that
thing is for ...

> There's a further problem in that we call INIT_LIST_HEAD on the
> dma_fence_cb before the signal callback. So even if list_empty_careful()
> confirms the dma_fence_cb to be completely decoupled, the containing
> struct may still be inuse.

The kerneldoc of dma_fence_remove_callback() already has a very stern
warning that this will blow up if you don't hold a full reference or
otherwise control the lifetime of this stuff. So I don't think we have
to worry about any of that. Or do you mean something else?
-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: [Intel-gfx] [PATCH 2/2] dma-buf/dma-fence: Add quick tests before dma_fence_remove_callback

[Daniel Vetter]

On Wed, Jul 15, 2020 at 2:40 PM Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Quoting Chris Wilson (2020-07-15 13:21:43)
> > Quoting Daniel Vetter (2020-07-15 13:10:22)
> > > On Wed, Jul 15, 2020 at 11:49:05AM +0100, Chris Wilson wrote:
> > > > When waiting with a callback on the stack, we must remove the callback
> > > > upon wait completion. Since this will be notified by the fence signal
> > > > callback, the removal often contends with the fence->lock being held by
> > > > the signaler. We can look at the list entry to see if the callback was
> > > > already signaled before we take the contended lock.
> > > >
> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > > ---
> > > >  drivers/dma-buf/dma-fence.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > > > index 8d5bdfce638e..b910d7bc0854 100644
> > > > --- a/drivers/dma-buf/dma-fence.c
> > > > +++ b/drivers/dma-buf/dma-fence.c
> > > > @@ -420,6 +420,9 @@ dma_fence_remove_callback(struct dma_fence *fence, struct dma_fence_cb *cb)
> > > >       unsigned long flags;
> > > >       bool ret;
> > > >
> > > > +     if (list_empty(&cb->node))
> > >
> > > I was about to say "but the races" but then noticed that Paul fixed
> > > list_empty to use READ_ONCE like 5 years ago :-)
> >
> > I'm always going "when exactly do we need list_empty_careful()"?
> >
> > We can rule out a concurrent dma_fence_add_callback() for the same
> > dma_fence_cb, as that is a lost cause. So we only have to worry about
> > the concurrent list_del_init() from dma_fence_signal_locked(). So it's
> > the timing of
> >         list_del_init(): WRITE_ONCE(list->next, list)
> > vs
> >         READ_ONCE(list->next) == list
> > and we don't need to care about the trailing instructions in
> > list_del_init()...
> >
> > Wait that trailing instruction is actually important here if the
> > dma_fence_cb is on the stack, or other imminent free.
> >
> > Ok, this does need to be list_empty_careful!

Hm, tbh I'm not really clear what list_empty_careful does on top.
Seems to lack READ_ONCE, so either some really big trickery with
dependencies is going on, or I'm not even sure how this works without
locks.

I've now stared at list_empty_careful and a bunch of users quite a
bit, and I have now idea when you'd want to use it. Lockless you want
a READ_ONCE I think and a simple check, so list_empty. And just accept
that any time you race you'll go into the locked slowpath for "list
isn't empty". Also only works if the list_empty case is the "nothing
to do, job already done" case, since the other one just isn't
guaranteed to be accurate.

list_empty_careful just wraps a bunch more magic around that will make
this both worse, and more racy (if the compiler feels creative)
because no READ_ONCE or anything like that. I don't get what that
thing is for ...

> There's a further problem in that we call INIT_LIST_HEAD on the
> dma_fence_cb before the signal callback. So even if list_empty_careful()
> confirms the dma_fence_cb to be completely decoupled, the containing
> struct may still be inuse.

The kerneldoc of dma_fence_remove_callback() already has a very stern
warning that this will blow up if you don't hold a full reference or
otherwise control the lifetime of this stuff. So I don't think we have
to worry about any of that. Or do you mean something else?
-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: [Intel-gfx] [PATCH 2/2] dma-buf/dma-fence: Add quick tests before dma_fence_remove_callback

[Chris Wilson]

Quoting Daniel Vetter (2020-07-15 15:03:34)
> On Wed, Jul 15, 2020 at 2:40 PM Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > There's a further problem in that we call INIT_LIST_HEAD on the
> > dma_fence_cb before the signal callback. So even if list_empty_careful()
> > confirms the dma_fence_cb to be completely decoupled, the containing
> > struct may still be inuse.
> 
> The kerneldoc of dma_fence_remove_callback() already has a very stern
> warning that this will blow up if you don't hold a full reference or
> otherwise control the lifetime of this stuff. So I don't think we have
> to worry about any of that. Or do you mean something else?

It's the struct dma_fence_cb itself that may be freed/reused. Consider
dma_fence_default_wait(). That uses struct default_wait_cb on the stack,
so in order to ensure that the callback is completed the list_empty
check has to remain under the spinlock, or else
dma_fence_default_wait_cb() can still be dereferencing wait->task as the
function returns.

So currently it is:

signed long
dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
{
        struct default_wait_cb cb;
        unsigned long flags;
        signed long ret = timeout ? timeout : 1;

        spin_lock_irqsave(fence->lock, flags);

        if (intr && signal_pending(current)) {
                ret = -ERESTARTSYS;
                goto out;
        }

        if (!__dma_fence_enable_signaling(fence))
                goto out;

        if (!timeout) {
                ret = 0;
                goto out;
        }

        cb.base.func = dma_fence_default_wait_cb;
        cb.task = current;
        list_add(&cb.base.node, &fence->cb_list);

        while (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) && ret > 0) {
                if (intr)
                        __set_current_state(TASK_INTERRUPTIBLE);
                else
                        __set_current_state(TASK_UNINTERRUPTIBLE);
                spin_unlock_irqrestore(fence->lock, flags);

                ret = schedule_timeout(ret);

                spin_lock_irqsave(fence->lock, flags);
                if (ret > 0 && intr && signal_pending(current))
                        ret = -ERESTARTSYS;
        }

        if (!list_empty(&cb.base.node))
                list_del(&cb.base.node);
        __set_current_state(TASK_RUNNING);

out:
        spin_unlock_irqrestore(fence->lock, flags);
        return ret;
}

but it could be written as:

signed long
dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
{
        struct default_wait_cb cb;
	int state = intr ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE;

        cb.task = current;
	if (dma_fence_add_callback(fence, &cb.base, dma_fence_default_wait_cb))
		return timeout ? timeout : 1;

	for (;;) {
		set_current_state(state);

		if (dma_fence_is_signaled(fence)) {
			timeout = timeout ? timeout : 1;
			break;
		}

		if (signal_pending_state(state, current)) {
			timeout = -ERESTARTSYS;
			break;
		}

		if (!timeout)
			break;

                timeout = schedule_timeout(timeout);
        }
        __set_current_state(TASK_RUNNING);

	dma_fence_remove_callback(fence, &cb.base);

        return timeout;
}
-Chris
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [Intel-gfx] [PATCH 2/2] dma-buf/dma-fence: Add quick tests before dma_fence_remove_callback

[Chris Wilson]

Quoting Daniel Vetter (2020-07-15 15:03:34)
> On Wed, Jul 15, 2020 at 2:40 PM Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > There's a further problem in that we call INIT_LIST_HEAD on the
> > dma_fence_cb before the signal callback. So even if list_empty_careful()
> > confirms the dma_fence_cb to be completely decoupled, the containing
> > struct may still be inuse.
> 
> The kerneldoc of dma_fence_remove_callback() already has a very stern
> warning that this will blow up if you don't hold a full reference or
> otherwise control the lifetime of this stuff. So I don't think we have
> to worry about any of that. Or do you mean something else?

It's the struct dma_fence_cb itself that may be freed/reused. Consider
dma_fence_default_wait(). That uses struct default_wait_cb on the stack,
so in order to ensure that the callback is completed the list_empty
check has to remain under the spinlock, or else
dma_fence_default_wait_cb() can still be dereferencing wait->task as the
function returns.

So currently it is:

signed long
dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
{
        struct default_wait_cb cb;
        unsigned long flags;
        signed long ret = timeout ? timeout : 1;

        spin_lock_irqsave(fence->lock, flags);

        if (intr && signal_pending(current)) {
                ret = -ERESTARTSYS;
                goto out;
        }

        if (!__dma_fence_enable_signaling(fence))
                goto out;

        if (!timeout) {
                ret = 0;
                goto out;
        }

        cb.base.func = dma_fence_default_wait_cb;
        cb.task = current;
        list_add(&cb.base.node, &fence->cb_list);

        while (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) && ret > 0) {
                if (intr)
                        __set_current_state(TASK_INTERRUPTIBLE);
                else
                        __set_current_state(TASK_UNINTERRUPTIBLE);
                spin_unlock_irqrestore(fence->lock, flags);

                ret = schedule_timeout(ret);

                spin_lock_irqsave(fence->lock, flags);
                if (ret > 0 && intr && signal_pending(current))
                        ret = -ERESTARTSYS;
        }

        if (!list_empty(&cb.base.node))
                list_del(&cb.base.node);
        __set_current_state(TASK_RUNNING);

out:
        spin_unlock_irqrestore(fence->lock, flags);
        return ret;
}

but it could be written as:

signed long
dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
{
        struct default_wait_cb cb;
	int state = intr ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE;

        cb.task = current;
	if (dma_fence_add_callback(fence, &cb.base, dma_fence_default_wait_cb))
		return timeout ? timeout : 1;

	for (;;) {
		set_current_state(state);

		if (dma_fence_is_signaled(fence)) {
			timeout = timeout ? timeout : 1;
			break;
		}

		if (signal_pending_state(state, current)) {
			timeout = -ERESTARTSYS;
			break;
		}

		if (!timeout)
			break;

                timeout = schedule_timeout(timeout);
        }
        __set_current_state(TASK_RUNNING);

	dma_fence_remove_callback(fence, &cb.base);

        return timeout;
}
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH 2/2] dma-buf/dma-fence: Add quick tests before dma_fence_remove_callback

[Daniel Vetter]

On Wed, Jul 15, 2020 at 4:37 PM Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> Quoting Daniel Vetter (2020-07-15 15:03:34)
> > On Wed, Jul 15, 2020 at 2:40 PM Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > There's a further problem in that we call INIT_LIST_HEAD on the
> > > dma_fence_cb before the signal callback. So even if list_empty_careful()
> > > confirms the dma_fence_cb to be completely decoupled, the containing
> > > struct may still be inuse.
> >
> > The kerneldoc of dma_fence_remove_callback() already has a very stern
> > warning that this will blow up if you don't hold a full reference or
> > otherwise control the lifetime of this stuff. So I don't think we have
> > to worry about any of that. Or do you mean something else?
>
> It's the struct dma_fence_cb itself that may be freed/reused. Consider
> dma_fence_default_wait(). That uses struct default_wait_cb on the stack,
> so in order to ensure that the callback is completed the list_empty
> check has to remain under the spinlock, or else
> dma_fence_default_wait_cb() can still be dereferencing wait->task as the
> function returns.

The current implementation of remove_callback doesn't work if you
don't own the callback structure. Or control its lifetime through some
other means.

So if we have callers removing other callback structures, that just
doesn't work, you can only remove your own.

From a quick spot check across a few callers we don't seem to have a
problem here, all current callers for this function are in various
wait functions (driver specific, or multi fence waits, stuff like
that).
-Daniel

> So currently it is:
>
> signed long
> dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
> {
>         struct default_wait_cb cb;
>         unsigned long flags;
>         signed long ret = timeout ? timeout : 1;
>
>         spin_lock_irqsave(fence->lock, flags);
>
>         if (intr && signal_pending(current)) {
>                 ret = -ERESTARTSYS;
>                 goto out;
>         }
>
>         if (!__dma_fence_enable_signaling(fence))
>                 goto out;
>
>         if (!timeout) {
>                 ret = 0;
>                 goto out;
>         }
>
>         cb.base.func = dma_fence_default_wait_cb;
>         cb.task = current;
>         list_add(&cb.base.node, &fence->cb_list);
>
>         while (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) && ret > 0) {
>                 if (intr)
>                         __set_current_state(TASK_INTERRUPTIBLE);
>                 else
>                         __set_current_state(TASK_UNINTERRUPTIBLE);
>                 spin_unlock_irqrestore(fence->lock, flags);
>
>                 ret = schedule_timeout(ret);
>
>                 spin_lock_irqsave(fence->lock, flags);
>                 if (ret > 0 && intr && signal_pending(current))
>                         ret = -ERESTARTSYS;
>         }
>
>         if (!list_empty(&cb.base.node))
>                 list_del(&cb.base.node);
>         __set_current_state(TASK_RUNNING);
>
> out:
>         spin_unlock_irqrestore(fence->lock, flags);
>         return ret;
> }
>
> but it could be written as:
>
> signed long
> dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
> {
>         struct default_wait_cb cb;
>         int state = intr ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE;
>
>         cb.task = current;
>         if (dma_fence_add_callback(fence, &cb.base, dma_fence_default_wait_cb))
>                 return timeout ? timeout : 1;
>
>         for (;;) {
>                 set_current_state(state);
>
>                 if (dma_fence_is_signaled(fence)) {
>                         timeout = timeout ? timeout : 1;
>                         break;
>                 }
>
>                 if (signal_pending_state(state, current)) {
>                         timeout = -ERESTARTSYS;
>                         break;
>                 }
>
>                 if (!timeout)
>                         break;
>
>                 timeout = schedule_timeout(timeout);
>         }
>         __set_current_state(TASK_RUNNING);
>
>         dma_fence_remove_callback(fence, &cb.base);
>
>         return timeout;
> }
> -Chris



-- 
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: [Intel-gfx] [PATCH 2/2] dma-buf/dma-fence: Add quick tests before dma_fence_remove_callback

[Daniel Vetter]

On Wed, Jul 15, 2020 at 4:37 PM Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> Quoting Daniel Vetter (2020-07-15 15:03:34)
> > On Wed, Jul 15, 2020 at 2:40 PM Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > There's a further problem in that we call INIT_LIST_HEAD on the
> > > dma_fence_cb before the signal callback. So even if list_empty_careful()
> > > confirms the dma_fence_cb to be completely decoupled, the containing
> > > struct may still be inuse.
> >
> > The kerneldoc of dma_fence_remove_callback() already has a very stern
> > warning that this will blow up if you don't hold a full reference or
> > otherwise control the lifetime of this stuff. So I don't think we have
> > to worry about any of that. Or do you mean something else?
>
> It's the struct dma_fence_cb itself that may be freed/reused. Consider
> dma_fence_default_wait(). That uses struct default_wait_cb on the stack,
> so in order to ensure that the callback is completed the list_empty
> check has to remain under the spinlock, or else
> dma_fence_default_wait_cb() can still be dereferencing wait->task as the
> function returns.

The current implementation of remove_callback doesn't work if you
don't own the callback structure. Or control its lifetime through some
other means.

So if we have callers removing other callback structures, that just
doesn't work, you can only remove your own.

From a quick spot check across a few callers we don't seem to have a
problem here, all current callers for this function are in various
wait functions (driver specific, or multi fence waits, stuff like
that).
-Daniel

> So currently it is:
>
> signed long
> dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
> {
>         struct default_wait_cb cb;
>         unsigned long flags;
>         signed long ret = timeout ? timeout : 1;
>
>         spin_lock_irqsave(fence->lock, flags);
>
>         if (intr && signal_pending(current)) {
>                 ret = -ERESTARTSYS;
>                 goto out;
>         }
>
>         if (!__dma_fence_enable_signaling(fence))
>                 goto out;
>
>         if (!timeout) {
>                 ret = 0;
>                 goto out;
>         }
>
>         cb.base.func = dma_fence_default_wait_cb;
>         cb.task = current;
>         list_add(&cb.base.node, &fence->cb_list);
>
>         while (!test_bit(DMA_FENCE_FLAG_SIGNALED_BIT, &fence->flags) && ret > 0) {
>                 if (intr)
>                         __set_current_state(TASK_INTERRUPTIBLE);
>                 else
>                         __set_current_state(TASK_UNINTERRUPTIBLE);
>                 spin_unlock_irqrestore(fence->lock, flags);
>
>                 ret = schedule_timeout(ret);
>
>                 spin_lock_irqsave(fence->lock, flags);
>                 if (ret > 0 && intr && signal_pending(current))
>                         ret = -ERESTARTSYS;
>         }
>
>         if (!list_empty(&cb.base.node))
>                 list_del(&cb.base.node);
>         __set_current_state(TASK_RUNNING);
>
> out:
>         spin_unlock_irqrestore(fence->lock, flags);
>         return ret;
> }
>
> but it could be written as:
>
> signed long
> dma_fence_default_wait(struct dma_fence *fence, bool intr, signed long timeout)
> {
>         struct default_wait_cb cb;
>         int state = intr ? TASK_INTERRUPTIBLE : TASK_UNINTERRUPTIBLE;
>
>         cb.task = current;
>         if (dma_fence_add_callback(fence, &cb.base, dma_fence_default_wait_cb))
>                 return timeout ? timeout : 1;
>
>         for (;;) {
>                 set_current_state(state);
>
>                 if (dma_fence_is_signaled(fence)) {
>                         timeout = timeout ? timeout : 1;
>                         break;
>                 }
>
>                 if (signal_pending_state(state, current)) {
>                         timeout = -ERESTARTSYS;
>                         break;
>                 }
>
>                 if (!timeout)
>                         break;
>
>                 timeout = schedule_timeout(timeout);
>         }
>         __set_current_state(TASK_RUNNING);
>
>         dma_fence_remove_callback(fence, &cb.base);
>
>         return timeout;
> }
> -Chris



-- 
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