* [Intel-gfx] [PATCH] drm/i915/perf: don't read head/tail pointers outside critical section
@ 2020-03-30 9:14 Lionel Landwerlin
2020-03-30 9:26 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Lionel Landwerlin @ 2020-03-30 9:14 UTC (permalink / raw)
To: intel-gfx
Reading or writing those fields should only happen under
stream->oa_buffer.ptr_lock.
Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Fixes: d1df41eb72ef ("drm/i915/perf: rework aging tail workaround")
---
drivers/gpu/drm/i915/i915_perf.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index c74ebac50015..ec9421f02ebd 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -463,6 +463,7 @@ static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream)
u32 gtt_offset = i915_ggtt_offset(stream->oa_buffer.vma);
int report_size = stream->oa_buffer.format_size;
unsigned long flags;
+ bool pollin;
u32 hw_tail;
u64 now;
@@ -532,10 +533,13 @@ static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream)
stream->oa_buffer.aging_timestamp = now;
}
+ pollin = OA_TAKEN(stream->oa_buffer.tail - gtt_offset,
+ stream->oa_buffer.head - gtt_offset) >= report_size;
+
+
spin_unlock_irqrestore(&stream->oa_buffer.ptr_lock, flags);
- return OA_TAKEN(stream->oa_buffer.tail - gtt_offset,
- stream->oa_buffer.head - gtt_offset) >= report_size;
+ return pollin;
}
/**
--
2.26.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply related [flat|nested] 8+ messages in thread
* [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for drm/i915/perf: don't read head/tail pointers outside critical section
2020-03-30 9:14 [Intel-gfx] [PATCH] drm/i915/perf: don't read head/tail pointers outside critical section Lionel Landwerlin
@ 2020-03-30 9:26 ` Patchwork
2020-03-30 9:56 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
` (2 subsequent siblings)
3 siblings, 0 replies; 8+ messages in thread
From: Patchwork @ 2020-03-30 9:26 UTC (permalink / raw)
To: Lionel Landwerlin; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/perf: don't read head/tail pointers outside critical section
URL : https://patchwork.freedesktop.org/series/75220/
State : warning
== Summary ==
$ dim checkpatch origin/drm-tip
97a65e4f417f drm/i915/perf: don't read head/tail pointers outside critical section
-:32: CHECK:LINE_SPACING: Please don't use multiple blank lines
#32: FILE: drivers/gpu/drm/i915/i915_perf.c:539:
+
+
total: 0 errors, 0 warnings, 1 checks, 22 lines checked
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/perf: don't read head/tail pointers outside critical section
2020-03-30 9:14 [Intel-gfx] [PATCH] drm/i915/perf: don't read head/tail pointers outside critical section Lionel Landwerlin
2020-03-30 9:26 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
@ 2020-03-30 9:56 ` Patchwork
2020-03-30 10:09 ` [Intel-gfx] [PATCH] " Chris Wilson
2020-03-30 11:04 ` [Intel-gfx] ✓ Fi.CI.IGT: success for " Patchwork
3 siblings, 0 replies; 8+ messages in thread
From: Patchwork @ 2020-03-30 9:56 UTC (permalink / raw)
To: Lionel Landwerlin; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/perf: don't read head/tail pointers outside critical section
URL : https://patchwork.freedesktop.org/series/75220/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_8212 -> Patchwork_17124
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/index.html
Known issues
------------
Here are the changes found in Patchwork_17124 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@i915_selftest@live@execlists:
- fi-icl-y: [PASS][1] -> [DMESG-FAIL][2] ([fdo#108569])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/fi-icl-y/igt@i915_selftest@live@execlists.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/fi-icl-y/igt@i915_selftest@live@execlists.html
- fi-kbl-soraka: [PASS][3] -> [INCOMPLETE][4] ([fdo#112259] / [i915#656])
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/fi-kbl-soraka/igt@i915_selftest@live@execlists.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/fi-kbl-soraka/igt@i915_selftest@live@execlists.html
#### Possible fixes ####
* igt@i915_selftest@live@gt_timelines:
- {fi-tgl-u}: [DMESG-FAIL][5] -> [PASS][6]
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/fi-tgl-u/igt@i915_selftest@live@gt_timelines.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/fi-tgl-u/igt@i915_selftest@live@gt_timelines.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#108569]: https://bugs.freedesktop.org/show_bug.cgi?id=108569
[fdo#112259]: https://bugs.freedesktop.org/show_bug.cgi?id=112259
[i915#656]: https://gitlab.freedesktop.org/drm/intel/issues/656
Participating hosts (45 -> 44)
------------------------------
Additional (5): fi-cml-u2 fi-cml-s fi-skl-6770hq fi-cfl-8700k fi-cfl-8109u
Missing (6): fi-byt-squawks fi-bsw-cyan fi-apl-guc fi-ctg-p8600 fi-byt-clapper fi-bdw-samus
Build changes
-------------
* CI: CI-20190529 -> None
* Linux: CI_DRM_8212 -> Patchwork_17124
CI-20190529: 20190529
CI_DRM_8212: 68b152390f915c189e2dd0b29eec557d5d8be9a8 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_5544: 477c562fc9932939083d732b77dd7b083c6bc0a1 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_17124: 97a65e4f417fc62caa75e155ad3d50179e5db9dd @ git://anongit.freedesktop.org/gfx-ci/linux
== Linux commits ==
97a65e4f417f drm/i915/perf: don't read head/tail pointers outside critical section
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/perf: don't read head/tail pointers outside critical section
2020-03-30 9:14 [Intel-gfx] [PATCH] drm/i915/perf: don't read head/tail pointers outside critical section Lionel Landwerlin
2020-03-30 9:26 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
2020-03-30 9:56 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
@ 2020-03-30 10:09 ` Chris Wilson
2020-03-30 15:55 ` Dixit, Ashutosh
2020-03-30 11:04 ` [Intel-gfx] ✓ Fi.CI.IGT: success for " Patchwork
3 siblings, 1 reply; 8+ messages in thread
From: Chris Wilson @ 2020-03-30 10:09 UTC (permalink / raw)
To: Lionel Landwerlin, intel-gfx
Quoting Lionel Landwerlin (2020-03-30 10:14:11)
> Reading or writing those fields should only happen under
> stream->oa_buffer.ptr_lock.
Writing, yes. Reading as a pair, sure. There are other ways you can
ensure that the tail/head are read as one, but fair enough.
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Fixes: d1df41eb72ef ("drm/i915/perf: rework aging tail workaround")
> ---
> drivers/gpu/drm/i915/i915_perf.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index c74ebac50015..ec9421f02ebd 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -463,6 +463,7 @@ static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream)
> u32 gtt_offset = i915_ggtt_offset(stream->oa_buffer.vma);
> int report_size = stream->oa_buffer.format_size;
> unsigned long flags;
> + bool pollin;
> u32 hw_tail;
> u64 now;
>
> @@ -532,10 +533,13 @@ static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream)
> stream->oa_buffer.aging_timestamp = now;
> }
>
> + pollin = OA_TAKEN(stream->oa_buffer.tail - gtt_offset,
> + stream->oa_buffer.head - gtt_offset) >= report_size;
> +
> +
Bonus \n
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> spin_unlock_irqrestore(&stream->oa_buffer.ptr_lock, flags);
>
> - return OA_TAKEN(stream->oa_buffer.tail - gtt_offset,
> - stream->oa_buffer.head - gtt_offset) >= report_size;
> + return pollin;
You could always leave the calculation here, and just have the read
inside.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
* [Intel-gfx] ✓ Fi.CI.IGT: success for drm/i915/perf: don't read head/tail pointers outside critical section
2020-03-30 9:14 [Intel-gfx] [PATCH] drm/i915/perf: don't read head/tail pointers outside critical section Lionel Landwerlin
` (2 preceding siblings ...)
2020-03-30 10:09 ` [Intel-gfx] [PATCH] " Chris Wilson
@ 2020-03-30 11:04 ` Patchwork
3 siblings, 0 replies; 8+ messages in thread
From: Patchwork @ 2020-03-30 11:04 UTC (permalink / raw)
To: Lionel Landwerlin; +Cc: intel-gfx
== Series Details ==
Series: drm/i915/perf: don't read head/tail pointers outside critical section
URL : https://patchwork.freedesktop.org/series/75220/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_8212_full -> Patchwork_17124_full
====================================================
Summary
-------
**SUCCESS**
No regressions found.
Known issues
------------
Here are the changes found in Patchwork_17124_full that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@gem_exec_balancer@smoke:
- shard-iclb: [PASS][1] -> [SKIP][2] ([fdo#110854])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-iclb1/igt@gem_exec_balancer@smoke.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-iclb7/igt@gem_exec_balancer@smoke.html
* igt@gem_exec_schedule@implicit-both-bsd:
- shard-iclb: [PASS][3] -> [SKIP][4] ([i915#677]) +1 similar issue
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-iclb8/igt@gem_exec_schedule@implicit-both-bsd.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-iclb4/igt@gem_exec_schedule@implicit-both-bsd.html
* igt@gem_exec_schedule@implicit-read-write-bsd1:
- shard-iclb: [PASS][5] -> [SKIP][6] ([fdo#109276] / [i915#677]) +1 similar issue
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-iclb4/igt@gem_exec_schedule@implicit-read-write-bsd1.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-iclb6/igt@gem_exec_schedule@implicit-read-write-bsd1.html
* igt@gem_exec_schedule@independent-bsd2:
- shard-iclb: [PASS][7] -> [SKIP][8] ([fdo#109276]) +15 similar issues
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-iclb2/igt@gem_exec_schedule@independent-bsd2.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-iclb7/igt@gem_exec_schedule@independent-bsd2.html
* igt@gem_exec_schedule@preempt-bsd:
- shard-iclb: [PASS][9] -> [SKIP][10] ([fdo#112146]) +7 similar issues
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-iclb3/igt@gem_exec_schedule@preempt-bsd.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-iclb1/igt@gem_exec_schedule@preempt-bsd.html
* igt@gem_exec_store@pages-vcs1:
- shard-iclb: [PASS][11] -> [SKIP][12] ([fdo#112080]) +10 similar issues
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-iclb4/igt@gem_exec_store@pages-vcs1.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-iclb6/igt@gem_exec_store@pages-vcs1.html
* igt@gen9_exec_parse@allowed-all:
- shard-glk: [PASS][13] -> [DMESG-WARN][14] ([i915#716])
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-glk8/igt@gen9_exec_parse@allowed-all.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-glk1/igt@gen9_exec_parse@allowed-all.html
* igt@i915_pm_rc6_residency@rc6-idle:
- shard-glk: [PASS][15] -> [FAIL][16] ([i915#1527])
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-glk1/igt@i915_pm_rc6_residency@rc6-idle.html
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-glk2/igt@i915_pm_rc6_residency@rc6-idle.html
* igt@kms_cursor_crc@pipe-b-cursor-64x64-sliding:
- shard-skl: [PASS][17] -> [FAIL][18] ([i915#54])
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-skl3/igt@kms_cursor_crc@pipe-b-cursor-64x64-sliding.html
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-skl1/igt@kms_cursor_crc@pipe-b-cursor-64x64-sliding.html
* igt@kms_cursor_legacy@2x-long-cursor-vs-flip-atomic:
- shard-hsw: [PASS][19] -> [FAIL][20] ([i915#96])
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-hsw1/igt@kms_cursor_legacy@2x-long-cursor-vs-flip-atomic.html
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-hsw8/igt@kms_cursor_legacy@2x-long-cursor-vs-flip-atomic.html
* igt@kms_fbcon_fbt@fbc-suspend:
- shard-kbl: [PASS][21] -> [DMESG-WARN][22] ([i915#180] / [i915#93] / [i915#95])
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-kbl3/igt@kms_fbcon_fbt@fbc-suspend.html
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-kbl7/igt@kms_fbcon_fbt@fbc-suspend.html
* igt@kms_flip_tiling@flip-changes-tiling-yf:
- shard-kbl: [PASS][23] -> [FAIL][24] ([i915#699] / [i915#93] / [i915#95])
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-kbl4/igt@kms_flip_tiling@flip-changes-tiling-yf.html
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-kbl7/igt@kms_flip_tiling@flip-changes-tiling-yf.html
* igt@kms_hdr@bpc-switch-dpms:
- shard-skl: [PASS][25] -> [FAIL][26] ([i915#1188])
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-skl4/igt@kms_hdr@bpc-switch-dpms.html
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-skl9/igt@kms_hdr@bpc-switch-dpms.html
* igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes:
- shard-kbl: [PASS][27] -> [DMESG-WARN][28] ([i915#180]) +2 similar issues
[27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-kbl6/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes.html
[28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-kbl2/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes.html
- shard-apl: [PASS][29] -> [DMESG-WARN][30] ([i915#180])
[29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-apl4/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes.html
[30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-apl6/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes.html
* igt@kms_plane_alpha_blend@pipe-a-constant-alpha-mid:
- shard-kbl: [PASS][31] -> [FAIL][32] ([fdo#108145] / [i915#93] / [i915#95])
[31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-kbl4/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-mid.html
[32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-kbl7/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-mid.html
* igt@kms_plane_alpha_blend@pipe-b-coverage-7efc:
- shard-skl: [PASS][33] -> [FAIL][34] ([fdo#108145] / [i915#265])
[33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-skl6/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html
[34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-skl7/igt@kms_plane_alpha_blend@pipe-b-coverage-7efc.html
* igt@kms_plane_alpha_blend@pipe-c-constant-alpha-min:
- shard-skl: [PASS][35] -> [FAIL][36] ([fdo#108145]) +2 similar issues
[35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-skl9/igt@kms_plane_alpha_blend@pipe-c-constant-alpha-min.html
[36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-skl3/igt@kms_plane_alpha_blend@pipe-c-constant-alpha-min.html
* igt@kms_psr@psr2_primary_page_flip:
- shard-iclb: [PASS][37] -> [SKIP][38] ([fdo#109441]) +2 similar issues
[37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-iclb2/igt@kms_psr@psr2_primary_page_flip.html
[38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-iclb7/igt@kms_psr@psr2_primary_page_flip.html
* igt@kms_setmode@basic:
- shard-apl: [PASS][39] -> [FAIL][40] ([i915#31])
[39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-apl6/igt@kms_setmode@basic.html
[40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-apl7/igt@kms_setmode@basic.html
#### Possible fixes ####
* igt@gem_exec_schedule@implicit-write-read-bsd1:
- shard-iclb: [SKIP][41] ([fdo#109276] / [i915#677]) -> [PASS][42] +1 similar issue
[41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-iclb5/igt@gem_exec_schedule@implicit-write-read-bsd1.html
[42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-iclb1/igt@gem_exec_schedule@implicit-write-read-bsd1.html
* igt@gem_exec_schedule@pi-distinct-iova-bsd:
- shard-iclb: [SKIP][43] ([i915#677]) -> [PASS][44] +2 similar issues
[43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-iclb1/igt@gem_exec_schedule@pi-distinct-iova-bsd.html
[44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-iclb7/igt@gem_exec_schedule@pi-distinct-iova-bsd.html
* igt@gem_exec_schedule@preempt-other-chain-bsd:
- shard-iclb: [SKIP][45] ([fdo#112146]) -> [PASS][46] +6 similar issues
[45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-iclb1/igt@gem_exec_schedule@preempt-other-chain-bsd.html
[46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-iclb7/igt@gem_exec_schedule@preempt-other-chain-bsd.html
* igt@i915_hangman@error-state-capture-vcs1:
- shard-iclb: [SKIP][47] ([fdo#112080]) -> [PASS][48] +9 similar issues
[47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-iclb3/igt@i915_hangman@error-state-capture-vcs1.html
[48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-iclb1/igt@i915_hangman@error-state-capture-vcs1.html
* igt@i915_pm_dc@dc6-psr:
- shard-iclb: [FAIL][49] ([i915#454]) -> [PASS][50]
[49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-iclb8/igt@i915_pm_dc@dc6-psr.html
[50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-iclb5/igt@i915_pm_dc@dc6-psr.html
* igt@kms_cursor_crc@pipe-a-cursor-suspend:
- shard-kbl: [DMESG-WARN][51] ([i915#180]) -> [PASS][52] +6 similar issues
[51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-kbl1/igt@kms_cursor_crc@pipe-a-cursor-suspend.html
[52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-kbl4/igt@kms_cursor_crc@pipe-a-cursor-suspend.html
* igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions:
- shard-skl: [FAIL][53] ([IGT#5]) -> [PASS][54]
[53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-skl7/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions.html
[54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-skl9/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions.html
* igt@kms_draw_crc@draw-method-xrgb2101010-mmap-gtt-xtiled:
- shard-skl: [FAIL][55] ([i915#52] / [i915#54]) -> [PASS][56]
[55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-skl1/igt@kms_draw_crc@draw-method-xrgb2101010-mmap-gtt-xtiled.html
[56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-skl8/igt@kms_draw_crc@draw-method-xrgb2101010-mmap-gtt-xtiled.html
* igt@kms_flip@flip-vs-expired-vblank:
- shard-apl: [FAIL][57] ([i915#79]) -> [PASS][58]
[57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-apl2/igt@kms_flip@flip-vs-expired-vblank.html
[58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-apl8/igt@kms_flip@flip-vs-expired-vblank.html
* igt@kms_frontbuffer_tracking@psr-suspend:
- shard-skl: [INCOMPLETE][59] ([i915#123] / [i915#69]) -> [PASS][60]
[59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-skl2/igt@kms_frontbuffer_tracking@psr-suspend.html
[60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-skl5/igt@kms_frontbuffer_tracking@psr-suspend.html
* igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes:
- shard-apl: [DMESG-WARN][61] ([i915#180]) -> [PASS][62] +2 similar issues
[61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-apl1/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes.html
[62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-apl8/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-b-planes.html
* igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min:
- shard-skl: [FAIL][63] ([fdo#108145]) -> [PASS][64]
[63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-skl8/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html
[64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-skl4/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html
* igt@kms_psr@psr2_suspend:
- shard-iclb: [SKIP][65] ([fdo#109441]) -> [PASS][66]
[65]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-iclb4/igt@kms_psr@psr2_suspend.html
[66]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-iclb2/igt@kms_psr@psr2_suspend.html
* {igt@perf@blocking-parameterized}:
- shard-hsw: [FAIL][67] ([i915#1542]) -> [PASS][68] +1 similar issue
[67]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-hsw8/igt@perf@blocking-parameterized.html
[68]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-hsw7/igt@perf@blocking-parameterized.html
* igt@prime_busy@hang-bsd2:
- shard-iclb: [SKIP][69] ([fdo#109276]) -> [PASS][70] +21 similar issues
[69]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-iclb6/igt@prime_busy@hang-bsd2.html
[70]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-iclb4/igt@prime_busy@hang-bsd2.html
#### Warnings ####
* igt@kms_psr2_su@page_flip:
- shard-iclb: [SKIP][71] ([fdo#109642] / [fdo#111068]) -> [FAIL][72] ([i915#608])
[71]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8212/shard-iclb7/igt@kms_psr2_su@page_flip.html
[72]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17124/shard-iclb2/igt@kms_psr2_su@page_flip.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[IGT#5]: https://gitlab.freedesktop.org/drm/igt-gpu-tools/issues/5
[fdo#108145]: https://bugs.freedesktop.org/show_bug.cgi?id=108145
[fdo#109276]: https://bugs.freedesktop.org/show_bug.cgi?id=109276
[fdo#109441]: https://bugs.freedesktop.org/show_bug.cgi?id=109441
[fdo#109642]: https://bugs.freedesktop.org/show_bug.cgi?id=109642
[fdo#110854]: https://bugs.freedesktop.org/show_bug.cgi?id=110854
[fdo#111068]: https://bugs.freedesktop.org/show_bug.cgi?id=111068
[fdo#112080]: https://bugs.freedesktop.org/show_bug.cgi?id=112080
[fdo#112146]: https://bugs.freedesktop.org/show_bug.cgi?id=112146
[i915#1188]: https://gitlab.freedesktop.org/drm/intel/issues/1188
[i915#123]: https://gitlab.freedesktop.org/drm/intel/issues/123
[i915#1527]: https://gitlab.freedesktop.org/drm/intel/issues/1527
[i915#1542]: https://gitlab.freedesktop.org/drm/intel/issues/1542
[i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
[i915#265]: https://gitlab.freedesktop.org/drm/intel/issues/265
[i915#31]: https://gitlab.freedesktop.org/drm/intel/issues/31
[i915#454]: https://gitlab.freedesktop.org/drm/intel/issues/454
[i915#52]: https://gitlab.freedesktop.org/drm/intel/issues/52
[i915#54]: https://gitlab.freedesktop.org/drm/intel/issues/54
[i915#608]: https://gitlab.freedesktop.org/drm/intel/issues/608
[i915#677]: https://gitlab.freedesktop.org/drm/intel/issues/677
[i915#69]: https://gitlab.freedesktop.org/drm/intel/issues/69
[i915#699]: https://gitlab.freedesktop.org/drm/intel/issues/699
[i915#716]: https://gitlab.freedesktop.org/drm/intel/issues/716
[i915#79]: https://gitlab.freedesktop.org/drm/intel/issues/79
[i915#93]: https://gitlab.freedesktop.org/drm/intel/issues/93
[i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95
[i915#96]: https://gitlab.freedesktop.org/drm/intel/issues/96
Participating hosts (10 -> 10)
------------------------------
No changes in participating hosts
Build changes
-------------
* CI: CI-20190529 -> None
* Linux: CI_DRM_8212 -> Patchwork_17124
CI-20190529: 20190529
CI_DRM_8212: 68b152390f915c189e2dd0b29eec557d5d8be9a8 @ git://anongit.freedesktop.org/gfx-ci/linux
IGT_5544: 477c562fc9932939083d732b77dd7b083c6bc0a1 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
Patchwork_17124: 97a65e4f417fc62caa75e155ad3d50179e5db9dd @ 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_17124/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/perf: don't read head/tail pointers outside critical section
2020-03-30 10:09 ` [Intel-gfx] [PATCH] " Chris Wilson
@ 2020-03-30 15:55 ` Dixit, Ashutosh
2020-03-30 16:38 ` Chris Wilson
0 siblings, 1 reply; 8+ messages in thread
From: Dixit, Ashutosh @ 2020-03-30 15:55 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Mon, 30 Mar 2020 03:09:20 -0700, Chris Wilson wrote:
>
> Quoting Lionel Landwerlin (2020-03-30 10:14:11)
> > Reading or writing those fields should only happen under
> > stream->oa_buffer.ptr_lock.
>
> Writing, yes. Reading as a pair, sure. There are other ways you can
> ensure that the tail/head are read as one, but fair enough.
Sorry but I am trying to understand exactly what the purpose of
stream->oa_buffer.ptr_lock is? This is a classic ring buffer producer
consumer situation where producer updates tail and consumer updates
head. Since both are u32's can't those operations be done without requiring
a lock?
>
> > Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> > Fixes: d1df41eb72ef ("drm/i915/perf: rework aging tail workaround")
> > ---
> > drivers/gpu/drm/i915/i915_perf.c | 8 ++++++--
> > 1 file changed, 6 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> > index c74ebac50015..ec9421f02ebd 100644
> > --- a/drivers/gpu/drm/i915/i915_perf.c
> > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > @@ -463,6 +463,7 @@ static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream)
> > u32 gtt_offset = i915_ggtt_offset(stream->oa_buffer.vma);
> > int report_size = stream->oa_buffer.format_size;
> > unsigned long flags;
> > + bool pollin;
> > u32 hw_tail;
> > u64 now;
> >
> > @@ -532,10 +533,13 @@ static bool oa_buffer_check_unlocked(struct i915_perf_stream *stream)
> > stream->oa_buffer.aging_timestamp = now;
> > }
> >
> > + pollin = OA_TAKEN(stream->oa_buffer.tail - gtt_offset,
> > + stream->oa_buffer.head - gtt_offset) >= report_size;
> > +
> > +
>
> Bonus \n
>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> > spin_unlock_irqrestore(&stream->oa_buffer.ptr_lock, flags);
> >
> > - return OA_TAKEN(stream->oa_buffer.tail - gtt_offset,
> > - stream->oa_buffer.head - gtt_offset) >= report_size;
> > + return pollin;
In what way is the original code incorrect? As I mentioned head is u32 and
can be read atomically without requiring a lock? We had deliberately moved
this code outside the lock so as to pick up the the latest value of head if
it had been updated in the consumer (read).
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/perf: don't read head/tail pointers outside critical section
2020-03-30 15:55 ` Dixit, Ashutosh
@ 2020-03-30 16:38 ` Chris Wilson
2020-03-31 1:40 ` Dixit, Ashutosh
0 siblings, 1 reply; 8+ messages in thread
From: Chris Wilson @ 2020-03-30 16:38 UTC (permalink / raw)
To: Dixit, Ashutosh; +Cc: intel-gfx
Quoting Dixit, Ashutosh (2020-03-30 16:55:32)
> On Mon, 30 Mar 2020 03:09:20 -0700, Chris Wilson wrote:
> >
> > Quoting Lionel Landwerlin (2020-03-30 10:14:11)
> > > Reading or writing those fields should only happen under
> > > stream->oa_buffer.ptr_lock.
> >
> > Writing, yes. Reading as a pair, sure. There are other ways you can
> > ensure that the tail/head are read as one, but fair enough.
>
> Sorry but I am trying to understand exactly what the purpose of
> stream->oa_buffer.ptr_lock is? This is a classic ring buffer producer
> consumer situation where producer updates tail and consumer updates
> head. Since both are u32's can't those operations be done without requiring
> a lock?
> > > spin_unlock_irqrestore(&stream->oa_buffer.ptr_lock, flags);
> > >
> > > - return OA_TAKEN(stream->oa_buffer.tail - gtt_offset,
> > > - stream->oa_buffer.head - gtt_offset) >= report_size;
> > > + return pollin;
>
> In what way is the original code incorrect? As I mentioned head is u32 and
> can be read atomically without requiring a lock? We had deliberately moved
> this code outside the lock so as to pick up the the latest value of head if
> it had been updated in the consumer (read).
It's the pair of reads here. What's the synchronisation between the read
of tail/head with the update? There's no sync between the reads so
order is not determined here.
So we may see the head updated for an old tail, and so think we have
plenty to report, when in fact there's none (or someother convolution).
Normal ringbuffer is to sample the head/tail pointers, smp_rmb(), then
consume the data between head/tail (with the write doing the smp_wmb()
after updating the data and before moving the tail). [So the normal
usage of barriers is around access to one of tail/head (the other is
under your control) and the shared contents.]
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [Intel-gfx] [PATCH] drm/i915/perf: don't read head/tail pointers outside critical section
2020-03-30 16:38 ` Chris Wilson
@ 2020-03-31 1:40 ` Dixit, Ashutosh
0 siblings, 0 replies; 8+ messages in thread
From: Dixit, Ashutosh @ 2020-03-31 1:40 UTC (permalink / raw)
To: Chris Wilson; +Cc: intel-gfx
On Mon, 30 Mar 2020 09:38:23 -0700, Chris Wilson wrote:
>
> Quoting Dixit, Ashutosh (2020-03-30 16:55:32)
> > On Mon, 30 Mar 2020 03:09:20 -0700, Chris Wilson wrote:
> > >
> > > Quoting Lionel Landwerlin (2020-03-30 10:14:11)
> > > > Reading or writing those fields should only happen under
> > > > stream->oa_buffer.ptr_lock.
> > >
> > > Writing, yes. Reading as a pair, sure. There are other ways you can
> > > ensure that the tail/head are read as one, but fair enough.
> >
> > Sorry but I am trying to understand exactly what the purpose of
> > stream->oa_buffer.ptr_lock is? This is a classic ring buffer producer
> > consumer situation where producer updates tail and consumer updates
> > head. Since both are u32's can't those operations be done without requiring
> > a lock?
>
> > > > spin_unlock_irqrestore(&stream->oa_buffer.ptr_lock, flags);
> > > >
> > > > - return OA_TAKEN(stream->oa_buffer.tail - gtt_offset,
> > > > - stream->oa_buffer.head - gtt_offset) >= report_size;
> > > > + return pollin;
> >
> > In what way is the original code incorrect? As I mentioned head is u32 and
> > can be read atomically without requiring a lock? We had deliberately moved
> > this code outside the lock so as to pick up the the latest value of head if
> > it had been updated in the consumer (read).
>
> It's the pair of reads here. What's the synchronisation between the read
> of tail/head with the update? There's no sync between the reads so
> order is not determined here.
>
> So we may see the head updated for an old tail, and so think we have
> plenty to report, when in fact there's none (or someother convolution).
>
> Normal ringbuffer is to sample the head/tail pointers, smp_rmb(), then
> consume the data between head/tail (with the write doing the smp_wmb()
> after updating the data and before moving the tail). [So the normal
> usage of barriers is around access to one of tail/head (the other is
> under your control) and the shared contents.]
Ok, thanks for explanantion Chris, I think I understand how barriers are
used with ring buffers but I am still not sure if the previous code was
incorrect. It is almost consumer side code running in the producer
thread. Anyway, let's just go with this patch for now:
Acked-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2020-03-31 1:40 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-30 9:14 [Intel-gfx] [PATCH] drm/i915/perf: don't read head/tail pointers outside critical section Lionel Landwerlin
2020-03-30 9:26 ` [Intel-gfx] ✗ Fi.CI.CHECKPATCH: warning for " Patchwork
2020-03-30 9:56 ` [Intel-gfx] ✓ Fi.CI.BAT: success " Patchwork
2020-03-30 10:09 ` [Intel-gfx] [PATCH] " Chris Wilson
2020-03-30 15:55 ` Dixit, Ashutosh
2020-03-30 16:38 ` Chris Wilson
2020-03-31 1:40 ` Dixit, Ashutosh
2020-03-30 11:04 ` [Intel-gfx] ✓ Fi.CI.IGT: success for " Patchwork
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.