public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
* [Intel-gfx] [RFC PATCH] dma-buf/dma-fence: Use a successful read_trylock() annotation for dma_fence_begin_signalling()
@ 2023-04-28 12:52 Thomas Hellström
  2023-04-28 13:03 ` Thomas Hellström
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Thomas Hellström @ 2023-04-28 12:52 UTC (permalink / raw)
  To: dri-devel
  Cc: Thomas Hellström, Daniel Vetter, intel-gfx, Christian Koenig,
	linaro-mm-sig, intel-xe

Condsider the following call sequence:

/* Upper layer */
dma_fence_begin_signalling();
lock(tainted_shared_lock);
/* Driver callback */
dma_fence_begin_signalling();
...

The driver might here use a utility that is annotated as intended for the
dma-fence signalling critical path. Now if the upper layer isn't correctly
annotated yet for whatever reason, resulting in

/* Upper layer */
lock(tainted_shared_lock);
/* Driver callback */
dma_fence_begin_signalling();

We will receive a false lockdep locking order violation notification from
dma_fence_begin_signalling(). However entering a dma-fence signalling
critical section itself doesn't block and could not cause a deadlock.

So use a successful read_trylock() annotation instead for
dma_fence_begin_signalling(). That will make sure that the locking order
is correctly registered in the first case, and doesn't register any
locking order in the second case.

The alternative is of course to make sure that the "Upper layer" is always
correctly annotated. But experience shows that's not easily achievable
in all cases.

Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
---
 drivers/dma-buf/dma-fence.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
index f177c56269bb..17f632768ef9 100644
--- a/drivers/dma-buf/dma-fence.c
+++ b/drivers/dma-buf/dma-fence.c
@@ -308,8 +308,8 @@ bool dma_fence_begin_signalling(void)
 	if (in_atomic())
 		return true;
 
-	/* ... and non-recursive readlock */
-	lock_acquire(&dma_fence_lockdep_map, 0, 0, 1, 1, NULL, _RET_IP_);
+	/* ... and non-recursive successful read_trylock */
+	lock_acquire(&dma_fence_lockdep_map, 0, 1, 1, 1, NULL, _RET_IP_);
 
 	return false;
 }
@@ -340,7 +340,7 @@ void __dma_fence_might_wait(void)
 	lock_map_acquire(&dma_fence_lockdep_map);
 	lock_map_release(&dma_fence_lockdep_map);
 	if (tmp)
-		lock_acquire(&dma_fence_lockdep_map, 0, 0, 1, 1, NULL, _THIS_IP_);
+		lock_acquire(&dma_fence_lockdep_map, 0, 1, 1, 1, NULL, _THIS_IP_);
 }
 #endif
 
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [Intel-gfx] [RFC PATCH] dma-buf/dma-fence: Use a successful read_trylock() annotation for dma_fence_begin_signalling()
  2023-04-28 12:52 [Intel-gfx] [RFC PATCH] dma-buf/dma-fence: Use a successful read_trylock() annotation for dma_fence_begin_signalling() Thomas Hellström
@ 2023-04-28 13:03 ` Thomas Hellström
  2023-04-28 17:55 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 10+ messages in thread
From: Thomas Hellström @ 2023-04-28 13:03 UTC (permalink / raw)
  To: dri-devel
  Cc: linaro-mm-sig, Daniel Vetter, intel-gfx, Christian Koenig,
	intel-xe


On 4/28/23 14:52, Thomas Hellström wrote:
> Condsider the following call sequence:
>
> /* Upper layer */
> dma_fence_begin_signalling();
> lock(tainted_shared_lock);
> /* Driver callback */
> dma_fence_begin_signalling();
> ...

The "Upper layer" here currently being the drm scheduler and "Driver 
callback" being an xe scheduler callback.

While opt-in annotating the drm scheduler would achieve the same result, 
I think this patch should be considered anyway, as I don't think we will 
miss any true lockdep violations as a result of it.

/Thomas



^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Intel-gfx] ✓ Fi.CI.BAT: success for dma-buf/dma-fence: Use a successful read_trylock() annotation for dma_fence_begin_signalling()
  2023-04-28 12:52 [Intel-gfx] [RFC PATCH] dma-buf/dma-fence: Use a successful read_trylock() annotation for dma_fence_begin_signalling() Thomas Hellström
  2023-04-28 13:03 ` Thomas Hellström
@ 2023-04-28 17:55 ` Patchwork
  2023-04-29  2:50 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
  2023-05-26 11:11 ` [Intel-gfx] [RFC PATCH] " Thomas Hellström
  3 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2023-04-28 17:55 UTC (permalink / raw)
  To: Thomas Hellström; +Cc: intel-gfx

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

== Series Details ==

Series: dma-buf/dma-fence: Use a successful read_trylock() annotation for dma_fence_begin_signalling()
URL   : https://patchwork.freedesktop.org/series/117115/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_13073 -> Patchwork_117115v1
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

Participating hosts (39 -> 37)
------------------------------

  Missing    (2): fi-snb-2520m bat-mtlp-6 

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

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

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-apl-guc:         [PASS][1] -> [DMESG-FAIL][2] ([i915#5334])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13073/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117115v1/fi-apl-guc/igt@i915_selftest@live@gt_heartbeat.html

  * igt@i915_selftest@live@hangcheck:
    - fi-skl-guc:         [PASS][3] -> [DMESG-WARN][4] ([i915#8073])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13073/fi-skl-guc/igt@i915_selftest@live@hangcheck.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117115v1/fi-skl-guc/igt@i915_selftest@live@hangcheck.html

  * igt@i915_selftest@live@slpc:
    - bat-adln-1:         NOTRUN -> [DMESG-FAIL][5] ([i915#6997])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117115v1/bat-adln-1/igt@i915_selftest@live@slpc.html

  * igt@kms_chamelium_hpd@common-hpd-after-suspend:
    - bat-adln-1:         NOTRUN -> [SKIP][6] ([i915#7828])
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117115v1/bat-adln-1/igt@kms_chamelium_hpd@common-hpd-after-suspend.html

  * igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence:
    - bat-dg2-11:         NOTRUN -> [SKIP][7] ([i915#1845] / [i915#5354])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117115v1/bat-dg2-11/igt@kms_pipe_crc_basic@nonblocking-crc-frame-sequence.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@gt_lrc:
    - bat-adln-1:         [INCOMPLETE][8] ([i915#4983] / [i915#7609]) -> [PASS][9]
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13073/bat-adln-1/igt@i915_selftest@live@gt_lrc.html
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117115v1/bat-adln-1/igt@i915_selftest@live@gt_lrc.html

  * igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-1:
    - bat-dg2-8:          [FAIL][10] ([i915#7932]) -> [PASS][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13073/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-1.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117115v1/bat-dg2-8/igt@kms_pipe_crc_basic@nonblocking-crc@pipe-d-dp-1.html

  
#### Warnings ####

  * igt@i915_selftest@live@slpc:
    - bat-rpls-1:         [DMESG-FAIL][12] ([i915#6367] / [i915#6997]) -> [DMESG-FAIL][13] ([i915#6367] / [i915#6997] / [i915#7996])
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13073/bat-rpls-1/igt@i915_selftest@live@slpc.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117115v1/bat-rpls-1/igt@i915_selftest@live@slpc.html

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

  [i915#1845]: https://gitlab.freedesktop.org/drm/intel/issues/1845
  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
  [i915#6367]: https://gitlab.freedesktop.org/drm/intel/issues/6367
  [i915#6997]: https://gitlab.freedesktop.org/drm/intel/issues/6997
  [i915#7609]: https://gitlab.freedesktop.org/drm/intel/issues/7609
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#7920]: https://gitlab.freedesktop.org/drm/intel/issues/7920
  [i915#7932]: https://gitlab.freedesktop.org/drm/intel/issues/7932
  [i915#7996]: https://gitlab.freedesktop.org/drm/intel/issues/7996
  [i915#8073]: https://gitlab.freedesktop.org/drm/intel/issues/8073


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

  * Linux: CI_DRM_13073 -> Patchwork_117115v1

  CI-20190529: 20190529
  CI_DRM_13073: d4602c57ac02d66526e4785b80c2e01dea122f33 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7275: c284bd66d7b416b4eaca456d6085b9180ad58058 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_117115v1: d4602c57ac02d66526e4785b80c2e01dea122f33 @ git://anongit.freedesktop.org/gfx-ci/linux


### Linux commits

75770c0f4f54 dma-buf/dma-fence: Use a successful read_trylock() annotation for dma_fence_begin_signalling()

== Logs ==

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

[-- Attachment #2: Type: text/html, Size: 5858 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [Intel-gfx] ✓ Fi.CI.IGT: success for dma-buf/dma-fence: Use a successful read_trylock() annotation for dma_fence_begin_signalling()
  2023-04-28 12:52 [Intel-gfx] [RFC PATCH] dma-buf/dma-fence: Use a successful read_trylock() annotation for dma_fence_begin_signalling() Thomas Hellström
  2023-04-28 13:03 ` Thomas Hellström
  2023-04-28 17:55 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
@ 2023-04-29  2:50 ` Patchwork
  2023-05-26 11:11 ` [Intel-gfx] [RFC PATCH] " Thomas Hellström
  3 siblings, 0 replies; 10+ messages in thread
From: Patchwork @ 2023-04-29  2:50 UTC (permalink / raw)
  To: Thomas Hellström; +Cc: intel-gfx

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

== Series Details ==

Series: dma-buf/dma-fence: Use a successful read_trylock() annotation for dma_fence_begin_signalling()
URL   : https://patchwork.freedesktop.org/series/117115/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_13073_full -> Patchwork_117115v1_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

Participating hosts (8 -> 7)
------------------------------

  Missing    (1): shard-tglu0 

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_exec_fair@basic-deadline:
    - shard-apl:          [PASS][1] -> [FAIL][2] ([i915#2846])
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13073/shard-apl3/igt@gem_exec_fair@basic-deadline.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117115v1/shard-apl2/igt@gem_exec_fair@basic-deadline.html

  * igt@gem_exec_fair@basic-pace-share@rcs0:
    - shard-glk:          [PASS][3] -> [FAIL][4] ([i915#2842])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13073/shard-glk5/igt@gem_exec_fair@basic-pace-share@rcs0.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117115v1/shard-glk8/igt@gem_exec_fair@basic-pace-share@rcs0.html

  * igt@gem_spin_batch@user-each:
    - shard-apl:          [PASS][5] -> [FAIL][6] ([i915#2898])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13073/shard-apl4/igt@gem_spin_batch@user-each.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117115v1/shard-apl2/igt@gem_spin_batch@user-each.html

  * igt@kms_flip@flip-vs-expired-vblank@b-dp1:
    - shard-apl:          [PASS][7] -> [FAIL][8] ([i915#79])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13073/shard-apl2/igt@kms_flip@flip-vs-expired-vblank@b-dp1.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117115v1/shard-apl7/igt@kms_flip@flip-vs-expired-vblank@b-dp1.html

  * igt@kms_plane_scaling@plane-upscale-with-modifiers-factor-0-25@pipe-a-vga-1:
    - shard-snb:          NOTRUN -> [SKIP][9] ([fdo#109271]) +31 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117115v1/shard-snb4/igt@kms_plane_scaling@plane-upscale-with-modifiers-factor-0-25@pipe-a-vga-1.html

  
#### Possible fixes ####

  * igt@drm_fdinfo@most-busy-check-all@rcs0:
    - {shard-rkl}:        [FAIL][10] ([i915#7742]) -> [PASS][11]
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13073/shard-rkl-4/igt@drm_fdinfo@most-busy-check-all@rcs0.html
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117115v1/shard-rkl-1/igt@drm_fdinfo@most-busy-check-all@rcs0.html

  * igt@gem_exec_fair@basic-none@vecs0:
    - {shard-rkl}:        [FAIL][12] ([i915#2842]) -> [PASS][13] +1 similar issue
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13073/shard-rkl-3/igt@gem_exec_fair@basic-none@vecs0.html
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117115v1/shard-rkl-7/igt@gem_exec_fair@basic-none@vecs0.html

  * igt@gem_sync@basic-all:
    - shard-glk:          [DMESG-WARN][14] ([i915#118]) -> [PASS][15]
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13073/shard-glk3/igt@gem_sync@basic-all.html
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117115v1/shard-glk5/igt@gem_sync@basic-all.html

  * igt@i915_pm_lpsp@kms-lpsp@kms-lpsp-hdmi-a:
    - {shard-rkl}:        [SKIP][16] ([i915#1937]) -> [PASS][17]
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13073/shard-rkl-3/igt@i915_pm_lpsp@kms-lpsp@kms-lpsp-hdmi-a.html
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117115v1/shard-rkl-7/igt@i915_pm_lpsp@kms-lpsp@kms-lpsp-hdmi-a.html

  * igt@i915_pm_rc6_residency@rc6-idle@vcs0:
    - {shard-dg1}:        [FAIL][18] ([i915#3591]) -> [PASS][19]
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13073/shard-dg1-17/igt@i915_pm_rc6_residency@rc6-idle@vcs0.html
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117115v1/shard-dg1-18/igt@i915_pm_rc6_residency@rc6-idle@vcs0.html

  * igt@i915_pm_rpm@dpms-non-lpsp:
    - {shard-rkl}:        [SKIP][20] ([i915#1397]) -> [PASS][21]
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13073/shard-rkl-7/igt@i915_pm_rpm@dpms-non-lpsp.html
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117115v1/shard-rkl-6/igt@i915_pm_rpm@dpms-non-lpsp.html

  * igt@kms_cursor_legacy@single-move@pipe-b:
    - {shard-rkl}:        [INCOMPLETE][22] ([i915#8011]) -> [PASS][23] +1 similar issue
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_13073/shard-rkl-7/igt@kms_cursor_legacy@single-move@pipe-b.html
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_117115v1/shard-rkl-4/igt@kms_cursor_legacy@single-move@pipe-b.html

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

  [fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
  [fdo#109274]: https://bugs.freedesktop.org/show_bug.cgi?id=109274
  [fdo#109280]: https://bugs.freedesktop.org/show_bug.cgi?id=109280
  [fdo#109315]: https://bugs.freedesktop.org/show_bug.cgi?id=109315
  [fdo#110189]: https://bugs.freedesktop.org/show_bug.cgi?id=110189
  [fdo#111614]: https://bugs.freedesktop.org/show_bug.cgi?id=111614
  [fdo#111615]: https://bugs.freedesktop.org/show_bug.cgi?id=111615
  [fdo#111827]: https://bugs.freedesktop.org/show_bug.cgi?id=111827
  [fdo#112054]: https://bugs.freedesktop.org/show_bug.cgi?id=112054
  [i915#1072]: https://gitlab.freedesktop.org/drm/intel/issues/1072
  [i915#118]: https://gitlab.freedesktop.org/drm/intel/issues/118
  [i915#1397]: https://gitlab.freedesktop.org/drm/intel/issues/1397
  [i915#1839]: https://gitlab.freedesktop.org/drm/intel/issues/1839
  [i915#1937]: https://gitlab.freedesktop.org/drm/intel/issues/1937
  [i915#2527]: https://gitlab.freedesktop.org/drm/intel/issues/2527
  [i915#2575]: https://gitlab.freedesktop.org/drm/intel/issues/2575
  [i915#2587]: https://gitlab.freedesktop.org/drm/intel/issues/2587
  [i915#2672]: https://gitlab.freedesktop.org/drm/intel/issues/2672
  [i915#2681]: https://gitlab.freedesktop.org/drm/intel/issues/2681
  [i915#280]: https://gitlab.freedesktop.org/drm/intel/issues/280
  [i915#2842]: https://gitlab.freedesktop.org/drm/intel/issues/2842
  [i915#2846]: https://gitlab.freedesktop.org/drm/intel/issues/2846
  [i915#2856]: https://gitlab.freedesktop.org/drm/intel/issues/2856
  [i915#2898]: https://gitlab.freedesktop.org/drm/intel/issues/2898
  [i915#3555]: https://gitlab.freedesktop.org/drm/intel/issues/3555
  [i915#3591]: https://gitlab.freedesktop.org/drm/intel/issues/3591
  [i915#3689]: https://gitlab.freedesktop.org/drm/intel/issues/3689
  [i915#3886]: https://gitlab.freedesktop.org/drm/intel/issues/3886
  [i915#4078]: https://gitlab.freedesktop.org/drm/intel/issues/4078
  [i915#4270]: https://gitlab.freedesktop.org/drm/intel/issues/4270
  [i915#4349]: https://gitlab.freedesktop.org/drm/intel/issues/4349
  [i915#4579]: https://gitlab.freedesktop.org/drm/intel/issues/4579
  [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613
  [i915#5176]: https://gitlab.freedesktop.org/drm/intel/issues/5176
  [i915#5235]: https://gitlab.freedesktop.org/drm/intel/issues/5235
  [i915#5286]: https://gitlab.freedesktop.org/drm/intel/issues/5286
  [i915#5288]: https://gitlab.freedesktop.org/drm/intel/issues/5288
  [i915#5325]: https://gitlab.freedesktop.org/drm/intel/issues/5325
  [i915#5354]: https://gitlab.freedesktop.org/drm/intel/issues/5354
  [i915#5439]: https://gitlab.freedesktop.org/drm/intel/issues/5439
  [i915#6095]: https://gitlab.freedesktop.org/drm/intel/issues/6095
  [i915#658]: https://gitlab.freedesktop.org/drm/intel/issues/658
  [i915#7443]: https://gitlab.freedesktop.org/drm/intel/issues/7443
  [i915#7561]: https://gitlab.freedesktop.org/drm/intel/issues/7561
  [i915#7742]: https://gitlab.freedesktop.org/drm/intel/issues/7742
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79
  [i915#7975]: https://gitlab.freedesktop.org/drm/intel/issues/7975
  [i915#8011]: https://gitlab.freedesktop.org/drm/intel/issues/8011
  [i915#8102]: https://gitlab.freedesktop.org/drm/intel/issues/8102
  [i915#8213]: https://gitlab.freedesktop.org/drm/intel/issues/8213


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

  * Linux: CI_DRM_13073 -> Patchwork_117115v1

  CI-20190529: 20190529
  CI_DRM_13073: d4602c57ac02d66526e4785b80c2e01dea122f33 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_7275: c284bd66d7b416b4eaca456d6085b9180ad58058 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git
  Patchwork_117115v1: d4602c57ac02d66526e4785b80c2e01dea122f33 @ 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_117115v1/index.html

[-- Attachment #2: Type: text/html, Size: 7296 bytes --]

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [Intel-gfx] [RFC PATCH] dma-buf/dma-fence: Use a successful read_trylock() annotation for dma_fence_begin_signalling()
  2023-04-28 12:52 [Intel-gfx] [RFC PATCH] dma-buf/dma-fence: Use a successful read_trylock() annotation for dma_fence_begin_signalling() Thomas Hellström
                   ` (2 preceding siblings ...)
  2023-04-29  2:50 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
@ 2023-05-26 11:11 ` Thomas Hellström
  2024-08-14  7:10   ` Daniel Vetter
  3 siblings, 1 reply; 10+ messages in thread
From: Thomas Hellström @ 2023-05-26 11:11 UTC (permalink / raw)
  To: dri-devel, Daniel Vetter
  Cc: linaro-mm-sig, intel-gfx, Christian Koenig, intel-xe

Daniel,

On 4/28/23 14:52, Thomas Hellström wrote:
> Condsider the following call sequence:
>
> /* Upper layer */
> dma_fence_begin_signalling();
> lock(tainted_shared_lock);
> /* Driver callback */
> dma_fence_begin_signalling();
> ...
>
> The driver might here use a utility that is annotated as intended for the
> dma-fence signalling critical path. Now if the upper layer isn't correctly
> annotated yet for whatever reason, resulting in
>
> /* Upper layer */
> lock(tainted_shared_lock);
> /* Driver callback */
> dma_fence_begin_signalling();
>
> We will receive a false lockdep locking order violation notification from
> dma_fence_begin_signalling(). However entering a dma-fence signalling
> critical section itself doesn't block and could not cause a deadlock.
>
> So use a successful read_trylock() annotation instead for
> dma_fence_begin_signalling(). That will make sure that the locking order
> is correctly registered in the first case, and doesn't register any
> locking order in the second case.
>
> The alternative is of course to make sure that the "Upper layer" is always
> correctly annotated. But experience shows that's not easily achievable
> in all cases.
>
> Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>

Resurrecting the discussion on this one. I can't see a situation where 
we would miss *relevant* locking
order violation warnings with this patch. Ofc if we have a scheduler 
annotation patch that would work fine as well, but the lack of 
annotation in the scheduler callbacks is really starting to hurt us.

Thanks,

Thomas



> ---
>   drivers/dma-buf/dma-fence.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> index f177c56269bb..17f632768ef9 100644
> --- a/drivers/dma-buf/dma-fence.c
> +++ b/drivers/dma-buf/dma-fence.c
> @@ -308,8 +308,8 @@ bool dma_fence_begin_signalling(void)
>   	if (in_atomic())
>   		return true;
>   
> -	/* ... and non-recursive readlock */
> -	lock_acquire(&dma_fence_lockdep_map, 0, 0, 1, 1, NULL, _RET_IP_);
> +	/* ... and non-recursive successful read_trylock */
> +	lock_acquire(&dma_fence_lockdep_map, 0, 1, 1, 1, NULL, _RET_IP_);
>   
>   	return false;
>   }
> @@ -340,7 +340,7 @@ void __dma_fence_might_wait(void)
>   	lock_map_acquire(&dma_fence_lockdep_map);
>   	lock_map_release(&dma_fence_lockdep_map);
>   	if (tmp)
> -		lock_acquire(&dma_fence_lockdep_map, 0, 0, 1, 1, NULL, _THIS_IP_);
> +		lock_acquire(&dma_fence_lockdep_map, 0, 1, 1, 1, NULL, _THIS_IP_);
>   }
>   #endif
>   

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] dma-buf/dma-fence: Use a successful read_trylock() annotation for dma_fence_begin_signalling()
  2023-05-26 11:11 ` [Intel-gfx] [RFC PATCH] " Thomas Hellström
@ 2024-08-14  7:10   ` Daniel Vetter
  2024-08-14  8:37     ` Thomas Hellström
  0 siblings, 1 reply; 10+ messages in thread
From: Daniel Vetter @ 2024-08-14  7:10 UTC (permalink / raw)
  To: Thomas Hellström
  Cc: dri-devel, Daniel Vetter, linaro-mm-sig, intel-gfx,
	Christian Koenig, intel-xe

On Fri, May 26, 2023 at 01:11:28PM +0200, Thomas Hellström wrote:
> Daniel,
> 
> On 4/28/23 14:52, Thomas Hellström wrote:
> > Condsider the following call sequence:
> > 
> > /* Upper layer */
> > dma_fence_begin_signalling();
> > lock(tainted_shared_lock);
> > /* Driver callback */
> > dma_fence_begin_signalling();
> > ...
> > 
> > The driver might here use a utility that is annotated as intended for the
> > dma-fence signalling critical path. Now if the upper layer isn't correctly
> > annotated yet for whatever reason, resulting in
> > 
> > /* Upper layer */
> > lock(tainted_shared_lock);
> > /* Driver callback */
> > dma_fence_begin_signalling();
> > 
> > We will receive a false lockdep locking order violation notification from
> > dma_fence_begin_signalling(). However entering a dma-fence signalling
> > critical section itself doesn't block and could not cause a deadlock.
> > 
> > So use a successful read_trylock() annotation instead for
> > dma_fence_begin_signalling(). That will make sure that the locking order
> > is correctly registered in the first case, and doesn't register any
> > locking order in the second case.
> > 
> > The alternative is of course to make sure that the "Upper layer" is always
> > correctly annotated. But experience shows that's not easily achievable
> > in all cases.
> > 
> > Signed-off-by: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> 
> Resurrecting the discussion on this one. I can't see a situation where we
> would miss *relevant* locking
> order violation warnings with this patch. Ofc if we have a scheduler
> annotation patch that would work fine as well, but the lack of annotation in
> the scheduler callbacks is really starting to hurt us.

Yeah this is just a bit too brain-melting to review, but I concur now.

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

I think what would help is some lockdep selftests to check that we both
catch the stuff we want to, and don't incur false positives. Maybe with a
plea that lockdep should have some native form of cross-release
annotations ...

But definitely seperate patch set, since it might take a few rounds of
review by lockdep folks.
-Sima

> 
> Thanks,
> 
> Thomas
> 
> 
> 
> > ---
> >   drivers/dma-buf/dma-fence.c | 6 +++---
> >   1 file changed, 3 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-fence.c
> > index f177c56269bb..17f632768ef9 100644
> > --- a/drivers/dma-buf/dma-fence.c
> > +++ b/drivers/dma-buf/dma-fence.c
> > @@ -308,8 +308,8 @@ bool dma_fence_begin_signalling(void)
> >   	if (in_atomic())
> >   		return true;
> > -	/* ... and non-recursive readlock */
> > -	lock_acquire(&dma_fence_lockdep_map, 0, 0, 1, 1, NULL, _RET_IP_);
> > +	/* ... and non-recursive successful read_trylock */
> > +	lock_acquire(&dma_fence_lockdep_map, 0, 1, 1, 1, NULL, _RET_IP_);
> >   	return false;
> >   }
> > @@ -340,7 +340,7 @@ void __dma_fence_might_wait(void)
> >   	lock_map_acquire(&dma_fence_lockdep_map);
> >   	lock_map_release(&dma_fence_lockdep_map);
> >   	if (tmp)
> > -		lock_acquire(&dma_fence_lockdep_map, 0, 0, 1, 1, NULL, _THIS_IP_);
> > +		lock_acquire(&dma_fence_lockdep_map, 0, 1, 1, 1, NULL, _THIS_IP_);
> >   }
> >   #endif

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

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [RFC PATCH] dma-buf/dma-fence: Use a successful read_trylock() annotation for dma_fence_begin_signalling()
  2024-08-14  7:10   ` Daniel Vetter
@ 2024-08-14  8:37     ` Thomas Hellström
  2024-09-18 12:34       ` RESEND " Thomas Hellström
  0 siblings, 1 reply; 10+ messages in thread
From: Thomas Hellström @ 2024-08-14  8:37 UTC (permalink / raw)
  To: Christian Koenig
  Cc: dri-devel, Daniel Vetter, linaro-mm-sig, intel-gfx, intel-xe

Christian,

Ack to merge this through drm-misc-next, or do you want to pick it up
for dma-buf?

Thanks,
Thomas


On Wed, 2024-08-14 at 09:10 +0200, Daniel Vetter wrote:
> On Fri, May 26, 2023 at 01:11:28PM +0200, Thomas Hellström wrote:
> > Daniel,
> > 
> > On 4/28/23 14:52, Thomas Hellström wrote:
> > > Condsider the following call sequence:
> > > 
> > > /* Upper layer */
> > > dma_fence_begin_signalling();
> > > lock(tainted_shared_lock);
> > > /* Driver callback */
> > > dma_fence_begin_signalling();
> > > ...
> > > 
> > > The driver might here use a utility that is annotated as intended
> > > for the
> > > dma-fence signalling critical path. Now if the upper layer isn't
> > > correctly
> > > annotated yet for whatever reason, resulting in
> > > 
> > > /* Upper layer */
> > > lock(tainted_shared_lock);
> > > /* Driver callback */
> > > dma_fence_begin_signalling();
> > > 
> > > We will receive a false lockdep locking order violation
> > > notification from
> > > dma_fence_begin_signalling(). However entering a dma-fence
> > > signalling
> > > critical section itself doesn't block and could not cause a
> > > deadlock.
> > > 
> > > So use a successful read_trylock() annotation instead for
> > > dma_fence_begin_signalling(). That will make sure that the
> > > locking order
> > > is correctly registered in the first case, and doesn't register
> > > any
> > > locking order in the second case.
> > > 
> > > The alternative is of course to make sure that the "Upper layer"
> > > is always
> > > correctly annotated. But experience shows that's not easily
> > > achievable
> > > in all cases.
> > > 
> > > Signed-off-by: Thomas Hellström
> > > <thomas.hellstrom@linux.intel.com>
> > 
> > Resurrecting the discussion on this one. I can't see a situation
> > where we
> > would miss *relevant* locking
> > order violation warnings with this patch. Ofc if we have a
> > scheduler
> > annotation patch that would work fine as well, but the lack of
> > annotation in
> > the scheduler callbacks is really starting to hurt us.
> 
> Yeah this is just a bit too brain-melting to review, but I concur
> now.
> 
> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>










> 
> I think what would help is some lockdep selftests to check that we
> both
> catch the stuff we want to, and don't incur false positives. Maybe
> with a
> plea that lockdep should have some native form of cross-release
> annotations ...
> 
> But definitely seperate patch set, since it might take a few rounds
> of
> review by lockdep folks.
> -Sima
> 
> > 
> > Thanks,
> > 
> > Thomas
> > 
> > 
> > 
> > > ---
> > >   drivers/dma-buf/dma-fence.c | 6 +++---
> > >   1 file changed, 3 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-
> > > fence.c
> > > index f177c56269bb..17f632768ef9 100644
> > > --- a/drivers/dma-buf/dma-fence.c
> > > +++ b/drivers/dma-buf/dma-fence.c
> > > @@ -308,8 +308,8 @@ bool dma_fence_begin_signalling(void)
> > >   	if (in_atomic())
> > >   		return true;
> > > -	/* ... and non-recursive readlock */
> > > -	lock_acquire(&dma_fence_lockdep_map, 0, 0, 1, 1, NULL,
> > > _RET_IP_);
> > > +	/* ... and non-recursive successful read_trylock */
> > > +	lock_acquire(&dma_fence_lockdep_map, 0, 1, 1, 1, NULL,
> > > _RET_IP_);
> > >   	return false;
> > >   }
> > > @@ -340,7 +340,7 @@ void __dma_fence_might_wait(void)
> > >   	lock_map_acquire(&dma_fence_lockdep_map);
> > >   	lock_map_release(&dma_fence_lockdep_map);
> > >   	if (tmp)
> > > -		lock_acquire(&dma_fence_lockdep_map, 0, 0, 1, 1,
> > > NULL, _THIS_IP_);
> > > +		lock_acquire(&dma_fence_lockdep_map, 0, 1, 1, 1,
> > > NULL, _THIS_IP_);
> > >   }
> > >   #endif
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* RESEND Re: [RFC PATCH] dma-buf/dma-fence: Use a successful read_trylock() annotation for dma_fence_begin_signalling()
  2024-08-14  8:37     ` Thomas Hellström
@ 2024-09-18 12:34       ` Thomas Hellström
  2024-09-18 13:18         ` Christian König
  0 siblings, 1 reply; 10+ messages in thread
From: Thomas Hellström @ 2024-09-18 12:34 UTC (permalink / raw)
  To: Christian Koenig
  Cc: dri-devel, Daniel Vetter, linaro-mm-sig, intel-gfx, intel-xe

Christian,

Ping?


On Wed, 2024-08-14 at 10:37 +0200, Thomas Hellström wrote:
> Christian,
> 
> Ack to merge this through drm-misc-next, or do you want to pick it up
> for dma-buf?
> 
> Thanks,
> Thomas
> 
> 
> On Wed, 2024-08-14 at 09:10 +0200, Daniel Vetter wrote:
> > On Fri, May 26, 2023 at 01:11:28PM +0200, Thomas Hellström wrote:
> > > Daniel,
> > > 
> > > On 4/28/23 14:52, Thomas Hellström wrote:
> > > > Condsider the following call sequence:
> > > > 
> > > > /* Upper layer */
> > > > dma_fence_begin_signalling();
> > > > lock(tainted_shared_lock);
> > > > /* Driver callback */
> > > > dma_fence_begin_signalling();
> > > > ...
> > > > 
> > > > The driver might here use a utility that is annotated as
> > > > intended
> > > > for the
> > > > dma-fence signalling critical path. Now if the upper layer
> > > > isn't
> > > > correctly
> > > > annotated yet for whatever reason, resulting in
> > > > 
> > > > /* Upper layer */
> > > > lock(tainted_shared_lock);
> > > > /* Driver callback */
> > > > dma_fence_begin_signalling();
> > > > 
> > > > We will receive a false lockdep locking order violation
> > > > notification from
> > > > dma_fence_begin_signalling(). However entering a dma-fence
> > > > signalling
> > > > critical section itself doesn't block and could not cause a
> > > > deadlock.
> > > > 
> > > > So use a successful read_trylock() annotation instead for
> > > > dma_fence_begin_signalling(). That will make sure that the
> > > > locking order
> > > > is correctly registered in the first case, and doesn't register
> > > > any
> > > > locking order in the second case.
> > > > 
> > > > The alternative is of course to make sure that the "Upper
> > > > layer"
> > > > is always
> > > > correctly annotated. But experience shows that's not easily
> > > > achievable
> > > > in all cases.
> > > > 
> > > > Signed-off-by: Thomas Hellström
> > > > <thomas.hellstrom@linux.intel.com>
> > > 
> > > Resurrecting the discussion on this one. I can't see a situation
> > > where we
> > > would miss *relevant* locking
> > > order violation warnings with this patch. Ofc if we have a
> > > scheduler
> > > annotation patch that would work fine as well, but the lack of
> > > annotation in
> > > the scheduler callbacks is really starting to hurt us.
> > 
> > Yeah this is just a bit too brain-melting to review, but I concur
> > now.
> > 
> > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> > 
> > I think what would help is some lockdep selftests to check that we
> > both
> > catch the stuff we want to, and don't incur false positives. Maybe
> > with a
> > plea that lockdep should have some native form of cross-release
> > annotations ...
> > 
> > But definitely seperate patch set, since it might take a few rounds
> > of
> > review by lockdep folks.
> > -Sima
> > 
> > > 
> > > Thanks,
> > > 
> > > Thomas
> > > 
> > > 
> > > 
> > > > ---
> > > >   drivers/dma-buf/dma-fence.c | 6 +++---
> > > >   1 file changed, 3 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-
> > > > fence.c
> > > > index f177c56269bb..17f632768ef9 100644
> > > > --- a/drivers/dma-buf/dma-fence.c
> > > > +++ b/drivers/dma-buf/dma-fence.c
> > > > @@ -308,8 +308,8 @@ bool dma_fence_begin_signalling(void)
> > > >   	if (in_atomic())
> > > >   		return true;
> > > > -	/* ... and non-recursive readlock */
> > > > -	lock_acquire(&dma_fence_lockdep_map, 0, 0, 1, 1, NULL,
> > > > _RET_IP_);
> > > > +	/* ... and non-recursive successful read_trylock */
> > > > +	lock_acquire(&dma_fence_lockdep_map, 0, 1, 1, 1, NULL,
> > > > _RET_IP_);
> > > >   	return false;
> > > >   }
> > > > @@ -340,7 +340,7 @@ void __dma_fence_might_wait(void)
> > > >   	lock_map_acquire(&dma_fence_lockdep_map);
> > > >   	lock_map_release(&dma_fence_lockdep_map);
> > > >   	if (tmp)
> > > > -		lock_acquire(&dma_fence_lockdep_map, 0, 0, 1,
> > > > 1,
> > > > NULL, _THIS_IP_);
> > > > +		lock_acquire(&dma_fence_lockdep_map, 0, 1, 1,
> > > > 1,
> > > > NULL, _THIS_IP_);
> > > >   }
> > > >   #endif
> > 
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: RESEND Re: [RFC PATCH] dma-buf/dma-fence: Use a successful read_trylock() annotation for dma_fence_begin_signalling()
  2024-09-18 12:34       ` RESEND " Thomas Hellström
@ 2024-09-18 13:18         ` Christian König
  2024-09-20  7:46           ` Thomas Hellström
  0 siblings, 1 reply; 10+ messages in thread
From: Christian König @ 2024-09-18 13:18 UTC (permalink / raw)
  To: Thomas Hellström
  Cc: dri-devel, Daniel Vetter, linaro-mm-sig, intel-gfx, intel-xe

Sorry, somehow completely missed that. Feel free to push it to 
drm-misc-next.

Christian.

Am 18.09.24 um 14:34 schrieb Thomas Hellström:
> Christian,
>
> Ping?
>
>
> On Wed, 2024-08-14 at 10:37 +0200, Thomas Hellström wrote:
>> Christian,
>>
>> Ack to merge this through drm-misc-next, or do you want to pick it up
>> for dma-buf?
>>
>> Thanks,
>> Thomas
>>
>>
>> On Wed, 2024-08-14 at 09:10 +0200, Daniel Vetter wrote:
>>> On Fri, May 26, 2023 at 01:11:28PM +0200, Thomas Hellström wrote:
>>>> Daniel,
>>>>
>>>> On 4/28/23 14:52, Thomas Hellström wrote:
>>>>> Condsider the following call sequence:
>>>>>
>>>>> /* Upper layer */
>>>>> dma_fence_begin_signalling();
>>>>> lock(tainted_shared_lock);
>>>>> /* Driver callback */
>>>>> dma_fence_begin_signalling();
>>>>> ...
>>>>>
>>>>> The driver might here use a utility that is annotated as
>>>>> intended
>>>>> for the
>>>>> dma-fence signalling critical path. Now if the upper layer
>>>>> isn't
>>>>> correctly
>>>>> annotated yet for whatever reason, resulting in
>>>>>
>>>>> /* Upper layer */
>>>>> lock(tainted_shared_lock);
>>>>> /* Driver callback */
>>>>> dma_fence_begin_signalling();
>>>>>
>>>>> We will receive a false lockdep locking order violation
>>>>> notification from
>>>>> dma_fence_begin_signalling(). However entering a dma-fence
>>>>> signalling
>>>>> critical section itself doesn't block and could not cause a
>>>>> deadlock.
>>>>>
>>>>> So use a successful read_trylock() annotation instead for
>>>>> dma_fence_begin_signalling(). That will make sure that the
>>>>> locking order
>>>>> is correctly registered in the first case, and doesn't register
>>>>> any
>>>>> locking order in the second case.
>>>>>
>>>>> The alternative is of course to make sure that the "Upper
>>>>> layer"
>>>>> is always
>>>>> correctly annotated. But experience shows that's not easily
>>>>> achievable
>>>>> in all cases.
>>>>>
>>>>> Signed-off-by: Thomas Hellström
>>>>> <thomas.hellstrom@linux.intel.com>
>>>> Resurrecting the discussion on this one. I can't see a situation
>>>> where we
>>>> would miss *relevant* locking
>>>> order violation warnings with this patch. Ofc if we have a
>>>> scheduler
>>>> annotation patch that would work fine as well, but the lack of
>>>> annotation in
>>>> the scheduler callbacks is really starting to hurt us.
>>> Yeah this is just a bit too brain-melting to review, but I concur
>>> now.
>>>
>>> Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
>>
>>
>>
>>
>>
>>
>>
>>
>>
>>> I think what would help is some lockdep selftests to check that we
>>> both
>>> catch the stuff we want to, and don't incur false positives. Maybe
>>> with a
>>> plea that lockdep should have some native form of cross-release
>>> annotations ...
>>>
>>> But definitely seperate patch set, since it might take a few rounds
>>> of
>>> review by lockdep folks.
>>> -Sima
>>>
>>>> Thanks,
>>>>
>>>> Thomas
>>>>
>>>>
>>>>
>>>>> ---
>>>>>    drivers/dma-buf/dma-fence.c | 6 +++---
>>>>>    1 file changed, 3 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-buf/dma-
>>>>> fence.c
>>>>> index f177c56269bb..17f632768ef9 100644
>>>>> --- a/drivers/dma-buf/dma-fence.c
>>>>> +++ b/drivers/dma-buf/dma-fence.c
>>>>> @@ -308,8 +308,8 @@ bool dma_fence_begin_signalling(void)
>>>>>    	if (in_atomic())
>>>>>    		return true;
>>>>> -	/* ... and non-recursive readlock */
>>>>> -	lock_acquire(&dma_fence_lockdep_map, 0, 0, 1, 1, NULL,
>>>>> _RET_IP_);
>>>>> +	/* ... and non-recursive successful read_trylock */
>>>>> +	lock_acquire(&dma_fence_lockdep_map, 0, 1, 1, 1, NULL,
>>>>> _RET_IP_);
>>>>>    	return false;
>>>>>    }
>>>>> @@ -340,7 +340,7 @@ void __dma_fence_might_wait(void)
>>>>>    	lock_map_acquire(&dma_fence_lockdep_map);
>>>>>    	lock_map_release(&dma_fence_lockdep_map);
>>>>>    	if (tmp)
>>>>> -		lock_acquire(&dma_fence_lockdep_map, 0, 0, 1,
>>>>> 1,
>>>>> NULL, _THIS_IP_);
>>>>> +		lock_acquire(&dma_fence_lockdep_map, 0, 1, 1,
>>>>> 1,
>>>>> NULL, _THIS_IP_);
>>>>>    }
>>>>>    #endif


^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: RESEND Re: [RFC PATCH] dma-buf/dma-fence: Use a successful read_trylock() annotation for dma_fence_begin_signalling()
  2024-09-18 13:18         ` Christian König
@ 2024-09-20  7:46           ` Thomas Hellström
  0 siblings, 0 replies; 10+ messages in thread
From: Thomas Hellström @ 2024-09-20  7:46 UTC (permalink / raw)
  To: Christian König
  Cc: dri-devel, Daniel Vetter, linaro-mm-sig, intel-gfx, intel-xe

On Wed, 2024-09-18 at 15:18 +0200, Christian König wrote:
> Sorry, somehow completely missed that. Feel free to push it to 
> drm-misc-next.
> 
> Christian.

Pushed, thanks.
/Thomas


> 
> Am 18.09.24 um 14:34 schrieb Thomas Hellström:
> > Christian,
> > 
> > Ping?
> > 
> > 
> > On Wed, 2024-08-14 at 10:37 +0200, Thomas Hellström wrote:
> > > Christian,
> > > 
> > > Ack to merge this through drm-misc-next, or do you want to pick
> > > it up
> > > for dma-buf?
> > > 
> > > Thanks,
> > > Thomas
> > > 
> > > 
> > > On Wed, 2024-08-14 at 09:10 +0200, Daniel Vetter wrote:
> > > > On Fri, May 26, 2023 at 01:11:28PM +0200, Thomas Hellström
> > > > wrote:
> > > > > Daniel,
> > > > > 
> > > > > On 4/28/23 14:52, Thomas Hellström wrote:
> > > > > > Condsider the following call sequence:
> > > > > > 
> > > > > > /* Upper layer */
> > > > > > dma_fence_begin_signalling();
> > > > > > lock(tainted_shared_lock);
> > > > > > /* Driver callback */
> > > > > > dma_fence_begin_signalling();
> > > > > > ...
> > > > > > 
> > > > > > The driver might here use a utility that is annotated as
> > > > > > intended
> > > > > > for the
> > > > > > dma-fence signalling critical path. Now if the upper layer
> > > > > > isn't
> > > > > > correctly
> > > > > > annotated yet for whatever reason, resulting in
> > > > > > 
> > > > > > /* Upper layer */
> > > > > > lock(tainted_shared_lock);
> > > > > > /* Driver callback */
> > > > > > dma_fence_begin_signalling();
> > > > > > 
> > > > > > We will receive a false lockdep locking order violation
> > > > > > notification from
> > > > > > dma_fence_begin_signalling(). However entering a dma-fence
> > > > > > signalling
> > > > > > critical section itself doesn't block and could not cause a
> > > > > > deadlock.
> > > > > > 
> > > > > > So use a successful read_trylock() annotation instead for
> > > > > > dma_fence_begin_signalling(). That will make sure that the
> > > > > > locking order
> > > > > > is correctly registered in the first case, and doesn't
> > > > > > register
> > > > > > any
> > > > > > locking order in the second case.
> > > > > > 
> > > > > > The alternative is of course to make sure that the "Upper
> > > > > > layer"
> > > > > > is always
> > > > > > correctly annotated. But experience shows that's not easily
> > > > > > achievable
> > > > > > in all cases.
> > > > > > 
> > > > > > Signed-off-by: Thomas Hellström
> > > > > > <thomas.hellstrom@linux.intel.com>
> > > > > Resurrecting the discussion on this one. I can't see a
> > > > > situation
> > > > > where we
> > > > > would miss *relevant* locking
> > > > > order violation warnings with this patch. Ofc if we have a
> > > > > scheduler
> > > > > annotation patch that would work fine as well, but the lack
> > > > > of
> > > > > annotation in
> > > > > the scheduler callbacks is really starting to hurt us.
> > > > Yeah this is just a bit too brain-melting to review, but I
> > > > concur
> > > > now.
> > > > 
> > > > Reviewed-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > 
> > > > I think what would help is some lockdep selftests to check that
> > > > we
> > > > both
> > > > catch the stuff we want to, and don't incur false positives.
> > > > Maybe
> > > > with a
> > > > plea that lockdep should have some native form of cross-release
> > > > annotations ...
> > > > 
> > > > But definitely seperate patch set, since it might take a few
> > > > rounds
> > > > of
> > > > review by lockdep folks.
> > > > -Sima
> > > > 
> > > > > Thanks,
> > > > > 
> > > > > Thomas
> > > > > 
> > > > > 
> > > > > 
> > > > > > ---
> > > > > >    drivers/dma-buf/dma-fence.c | 6 +++---
> > > > > >    1 file changed, 3 insertions(+), 3 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/dma-buf/dma-fence.c b/drivers/dma-
> > > > > > buf/dma-
> > > > > > fence.c
> > > > > > index f177c56269bb..17f632768ef9 100644
> > > > > > --- a/drivers/dma-buf/dma-fence.c
> > > > > > +++ b/drivers/dma-buf/dma-fence.c
> > > > > > @@ -308,8 +308,8 @@ bool dma_fence_begin_signalling(void)
> > > > > >    	if (in_atomic())
> > > > > >    		return true;
> > > > > > -	/* ... and non-recursive readlock */
> > > > > > -	lock_acquire(&dma_fence_lockdep_map, 0, 0, 1, 1,
> > > > > > NULL,
> > > > > > _RET_IP_);
> > > > > > +	/* ... and non-recursive successful read_trylock
> > > > > > */
> > > > > > +	lock_acquire(&dma_fence_lockdep_map, 0, 1, 1, 1,
> > > > > > NULL,
> > > > > > _RET_IP_);
> > > > > >    	return false;
> > > > > >    }
> > > > > > @@ -340,7 +340,7 @@ void __dma_fence_might_wait(void)
> > > > > >    	lock_map_acquire(&dma_fence_lockdep_map);
> > > > > >    	lock_map_release(&dma_fence_lockdep_map);
> > > > > >    	if (tmp)
> > > > > > -		lock_acquire(&dma_fence_lockdep_map, 0, 0,
> > > > > > 1,
> > > > > > 1,
> > > > > > NULL, _THIS_IP_);
> > > > > > +		lock_acquire(&dma_fence_lockdep_map, 0, 1,
> > > > > > 1,
> > > > > > 1,
> > > > > > NULL, _THIS_IP_);
> > > > > >    }
> > > > > >    #endif
> 


^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2024-09-20  7:46 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-04-28 12:52 [Intel-gfx] [RFC PATCH] dma-buf/dma-fence: Use a successful read_trylock() annotation for dma_fence_begin_signalling() Thomas Hellström
2023-04-28 13:03 ` Thomas Hellström
2023-04-28 17:55 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
2023-04-29  2:50 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
2023-05-26 11:11 ` [Intel-gfx] [RFC PATCH] " Thomas Hellström
2024-08-14  7:10   ` Daniel Vetter
2024-08-14  8:37     ` Thomas Hellström
2024-09-18 12:34       ` RESEND " Thomas Hellström
2024-09-18 13:18         ` Christian König
2024-09-20  7:46           ` Thomas Hellström

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox