All of lore.kernel.org
 help / color / mirror / Atom feed
* [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers
@ 2020-03-26  4:43 Ashutosh Dixit
  2020-03-26  5:29 ` [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/perf: Do not clear pollin for small user read buffers (rev2) Patchwork
                   ` (2 more replies)
  0 siblings, 3 replies; 29+ messages in thread
From: Ashutosh Dixit @ 2020-03-26  4:43 UTC (permalink / raw)
  To: intel-gfx

It is wrong to block the user thread in the next poll when OA data is
already available which could not fit in the user buffer provided in
the previous read. In several cases the exact user buffer size is not
known. Blocking user space in poll can lead to data loss when the
buffer size used is smaller than the available data.

This change fixes this issue and allows user space to read all OA data
even when using a buffer size smaller than the available data using
multiple non-blocking reads rather than staying blocked in poll till
the next timer interrupt.

v2: Fix ret value for blocking reads (Umesh)

Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
 drivers/gpu/drm/i915/i915_perf.c | 63 ++++++--------------------------
 1 file changed, 12 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 3222f6cd8255..e2d083efba6d 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -2957,49 +2957,6 @@ void i915_oa_init_reg_state(const struct intel_context *ce,
 		gen8_update_reg_state_unlocked(ce, stream);
 }
 
-/**
- * i915_perf_read_locked - &i915_perf_stream_ops->read with error normalisation
- * @stream: An i915 perf stream
- * @file: An i915 perf stream file
- * @buf: destination buffer given by userspace
- * @count: the number of bytes userspace wants to read
- * @ppos: (inout) file seek position (unused)
- *
- * Besides wrapping &i915_perf_stream_ops->read this provides a common place to
- * ensure that if we've successfully copied any data then reporting that takes
- * precedence over any internal error status, so the data isn't lost.
- *
- * For example ret will be -ENOSPC whenever there is more buffered data than
- * can be copied to userspace, but that's only interesting if we weren't able
- * to copy some data because it implies the userspace buffer is too small to
- * receive a single record (and we never split records).
- *
- * Another case with ret == -EFAULT is more of a grey area since it would seem
- * like bad form for userspace to ask us to overrun its buffer, but the user
- * knows best:
- *
- *   http://yarchive.net/comp/linux/partial_reads_writes.html
- *
- * Returns: The number of bytes copied or a negative error code on failure.
- */
-static ssize_t i915_perf_read_locked(struct i915_perf_stream *stream,
-				     struct file *file,
-				     char __user *buf,
-				     size_t count,
-				     loff_t *ppos)
-{
-	/* Note we keep the offset (aka bytes read) separate from any
-	 * error status so that the final check for whether we return
-	 * the bytes read with a higher precedence than any error (see
-	 * comment below) doesn't need to be handled/duplicated in
-	 * stream->ops->read() implementations.
-	 */
-	size_t offset = 0;
-	int ret = stream->ops->read(stream, buf, count, &offset);
-
-	return offset ?: (ret ?: -EAGAIN);
-}
-
 /**
  * i915_perf_read - handles read() FOP for i915 perf stream FDs
  * @file: An i915 perf stream file
@@ -3025,6 +2982,8 @@ static ssize_t i915_perf_read(struct file *file,
 {
 	struct i915_perf_stream *stream = file->private_data;
 	struct i915_perf *perf = stream->perf;
+	size_t offset = 0;
+	int __ret;
 	ssize_t ret;
 
 	/* To ensure it's handled consistently we simply treat all reads of a
@@ -3048,16 +3007,19 @@ static ssize_t i915_perf_read(struct file *file,
 				return ret;
 
 			mutex_lock(&perf->lock);
-			ret = i915_perf_read_locked(stream, file,
-						    buf, count, ppos);
+			__ret = stream->ops->read(stream, buf, count, &offset);
+			ret = offset ?: (__ret ?: -EAGAIN);
 			mutex_unlock(&perf->lock);
 		} while (ret == -EAGAIN);
 	} else {
 		mutex_lock(&perf->lock);
-		ret = i915_perf_read_locked(stream, file, buf, count, ppos);
+		__ret = stream->ops->read(stream, buf, count, &offset);
+		ret = offset ?: (__ret ?: -EAGAIN);
 		mutex_unlock(&perf->lock);
 	}
 
+	/* Possible values for __ret are 0, -EFAULT, -ENOSPC, -EAGAIN, ... */
+
 	/* We allow the poll checking to sometimes report false positive EPOLLIN
 	 * events where we might actually report EAGAIN on read() if there's
 	 * not really any data available. In this situation though we don't
@@ -3065,13 +3027,12 @@ static ssize_t i915_perf_read(struct file *file,
 	 * and read() returning -EAGAIN. Clearing the oa.pollin state here
 	 * effectively ensures we back off until the next hrtimer callback
 	 * before reporting another EPOLLIN event.
+	 * The exception to this is if ops->read() returned -ENOSPC which means
+	 * that more OA data is available than could fit in the user provided
+	 * buffer. In this case we want the next poll() call to not block.
 	 */
-	if (ret >= 0 || ret == -EAGAIN) {
-		/* Maybe make ->pollin per-stream state if we support multiple
-		 * concurrent streams in the future.
-		 */
+	if ((ret > 0 || ret == -EAGAIN) && __ret != -ENOSPC)
 		stream->pollin = false;
-	}
 
 	return ret;
 }
-- 
2.25.2

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

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

* [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/perf: Do not clear pollin for small user read buffers (rev2)
  2020-03-26  4:43 [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers Ashutosh Dixit
@ 2020-03-26  5:29 ` Patchwork
  2020-03-26  6:47 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
  2020-03-26  9:09 ` [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers Lionel Landwerlin
  2 siblings, 0 replies; 29+ messages in thread
From: Patchwork @ 2020-03-26  5:29 UTC (permalink / raw)
  To: Ashutosh Dixit; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/perf: Do not clear pollin for small user read buffers (rev2)
URL   : https://patchwork.freedesktop.org/series/75085/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_8190 -> Patchwork_17092
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

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

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

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

### IGT changes ###

#### Possible fixes ####

  * igt@i915_selftest@live@execlists:
    - {fi-tgl-u}:         [DMESG-FAIL][1] ([i915#656]) -> [PASS][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/fi-tgl-u/igt@i915_selftest@live@execlists.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/fi-tgl-u/igt@i915_selftest@live@execlists.html

  * igt@i915_selftest@live@hangcheck:
    - fi-icl-u2:          [INCOMPLETE][3] ([fdo#108569]) -> [PASS][4]
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/fi-icl-u2/igt@i915_selftest@live@hangcheck.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/fi-icl-u2/igt@i915_selftest@live@hangcheck.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
  [i915#656]: https://gitlab.freedesktop.org/drm/intel/issues/656


Participating hosts (40 -> 35)
------------------------------

  Additional (7): fi-glk-dsi fi-ivb-3770 fi-elk-e7500 fi-bsw-kefka fi-blb-e6850 fi-bsw-nick fi-snb-2600 
  Missing    (12): fi-hsw-4770r fi-bdw-5557u fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-bwr-2160 fi-ctg-p8600 fi-hsw-4770 fi-skl-lmem fi-bdw-samus fi-byt-clapper fi-skl-6600u 


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_8190 -> Patchwork_17092

  CI-20190529: 20190529
  CI_DRM_8190: 73f711b364bc85c8a7189487c09431eb1f515ed0 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5538: 47becbc9cd1fc7b1b78692f90fd3dcd5a9066965 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_17092: fae3ed433b36110cf2f05ca2bc9100632293334d @ git://anongit.freedesktop.org/gfx-ci/linux


== Linux commits ==

fae3ed433b36 drm/i915/perf: Do not clear pollin for small user read buffers

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* [Intel-gfx] ✓ Fi.CI.IGT: success for drm/i915/perf: Do not clear pollin for small user read buffers (rev2)
  2020-03-26  4:43 [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers Ashutosh Dixit
  2020-03-26  5:29 ` [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/perf: Do not clear pollin for small user read buffers (rev2) Patchwork
@ 2020-03-26  6:47 ` Patchwork
  2020-03-26  9:09 ` [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers Lionel Landwerlin
  2 siblings, 0 replies; 29+ messages in thread
From: Patchwork @ 2020-03-26  6:47 UTC (permalink / raw)
  To: Ashutosh Dixit; +Cc: intel-gfx

== Series Details ==

Series: drm/i915/perf: Do not clear pollin for small user read buffers (rev2)
URL   : https://patchwork.freedesktop.org/series/75085/
State : success

== Summary ==

CI Bug Log - changes from CI_DRM_8190_full -> Patchwork_17092_full
====================================================

Summary
-------

  **SUCCESS**

  No regressions found.

  

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

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

### IGT changes ###

#### Issues hit ####

  * igt@gem_ctx_isolation@bcs0-s3:
    - shard-kbl:          [PASS][1] -> [DMESG-WARN][2] ([i915#180] / [i915#93] / [i915#95]) +1 similar issue
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-kbl4/igt@gem_ctx_isolation@bcs0-s3.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-kbl1/igt@gem_ctx_isolation@bcs0-s3.html

  * igt@gem_ctx_isolation@vcs1-nonpriv:
    - shard-iclb:         [PASS][3] -> [SKIP][4] ([fdo#112080]) +5 similar issues
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-iclb1/igt@gem_ctx_isolation@vcs1-nonpriv.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-iclb5/igt@gem_ctx_isolation@vcs1-nonpriv.html

  * igt@gem_exec_schedule@implicit-read-write-bsd:
    - shard-iclb:         [PASS][5] -> [SKIP][6] ([i915#677])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-iclb8/igt@gem_exec_schedule@implicit-read-write-bsd.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-iclb4/igt@gem_exec_schedule@implicit-read-write-bsd.html

  * igt@gem_exec_schedule@implicit-write-read-bsd2:
    - shard-iclb:         [PASS][7] -> [SKIP][8] ([fdo#109276] / [i915#677])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-iclb1/igt@gem_exec_schedule@implicit-write-read-bsd2.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-iclb3/igt@gem_exec_schedule@implicit-write-read-bsd2.html

  * igt@gem_exec_schedule@preempt-bsd:
    - shard-iclb:         [PASS][9] -> [SKIP][10] ([fdo#112146]) +6 similar issues
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-iclb8/igt@gem_exec_schedule@preempt-bsd.html
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-iclb2/igt@gem_exec_schedule@preempt-bsd.html

  * igt@i915_selftest@live@requests:
    - shard-iclb:         [PASS][11] -> [INCOMPLETE][12] ([fdo#109644] / [fdo#110464])
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-iclb2/igt@i915_selftest@live@requests.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-iclb7/igt@i915_selftest@live@requests.html

  * igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size:
    - shard-skl:          [PASS][13] -> [FAIL][14] ([IGT#5])
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-skl9/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-skl5/igt@kms_cursor_legacy@flip-vs-cursor-atomic-transitions-varying-size.html

  * igt@kms_draw_crc@draw-method-rgb565-mmap-cpu-untiled:
    - shard-skl:          [PASS][15] -> [FAIL][16] ([i915#52] / [i915#54])
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-skl3/igt@kms_draw_crc@draw-method-rgb565-mmap-cpu-untiled.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-skl9/igt@kms_draw_crc@draw-method-rgb565-mmap-cpu-untiled.html

  * igt@kms_draw_crc@draw-method-rgb565-mmap-gtt-xtiled:
    - shard-glk:          [PASS][17] -> [FAIL][18] ([i915#52] / [i915#54])
   [17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-glk5/igt@kms_draw_crc@draw-method-rgb565-mmap-gtt-xtiled.html
   [18]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-glk6/igt@kms_draw_crc@draw-method-rgb565-mmap-gtt-xtiled.html

  * igt@kms_hdr@bpc-switch:
    - shard-skl:          [PASS][19] -> [FAIL][20] ([i915#1188])
   [19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-skl4/igt@kms_hdr@bpc-switch.html
   [20]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-skl1/igt@kms_hdr@bpc-switch.html

  * igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c:
    - shard-kbl:          [PASS][21] -> [DMESG-WARN][22] ([i915#180]) +2 similar issues
   [21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-kbl7/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c.html
   [22]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-kbl7/igt@kms_pipe_crc_basic@suspend-read-crc-pipe-c.html

  * igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes:
    - shard-apl:          [PASS][23] -> [DMESG-WARN][24] ([i915#180]) +1 similar issue
   [23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-apl3/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes.html
   [24]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-apl1/igt@kms_plane@plane-panning-bottom-right-suspend-pipe-c-planes.html

  * igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min:
    - shard-skl:          [PASS][25] -> [FAIL][26] ([fdo#108145]) +1 similar issue
   [25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-skl7/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html
   [26]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-skl8/igt@kms_plane_alpha_blend@pipe-a-constant-alpha-min.html

  * igt@kms_plane_alpha_blend@pipe-c-coverage-7efc:
    - shard-skl:          [PASS][27] -> [FAIL][28] ([fdo#108145] / [i915#265])
   [27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-skl2/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html
   [28]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-skl6/igt@kms_plane_alpha_blend@pipe-c-coverage-7efc.html

  * igt@kms_prime@basic-crc:
    - shard-apl:          [PASS][29] -> [FAIL][30] ([i915#1031] / [i915#95])
   [29]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-apl4/igt@kms_prime@basic-crc.html
   [30]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-apl6/igt@kms_prime@basic-crc.html
    - shard-kbl:          [PASS][31] -> [FAIL][32] ([i915#1031] / [i915#93] / [i915#95])
   [31]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-kbl7/igt@kms_prime@basic-crc.html
   [32]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-kbl3/igt@kms_prime@basic-crc.html

  * igt@kms_psr@psr2_cursor_blt:
    - shard-iclb:         [PASS][33] -> [SKIP][34] ([fdo#109441]) +1 similar issue
   [33]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-iclb2/igt@kms_psr@psr2_cursor_blt.html
   [34]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-iclb7/igt@kms_psr@psr2_cursor_blt.html

  * igt@prime_busy@hang-bsd2:
    - shard-iclb:         [PASS][35] -> [SKIP][36] ([fdo#109276]) +20 similar issues
   [35]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-iclb1/igt@prime_busy@hang-bsd2.html
   [36]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-iclb3/igt@prime_busy@hang-bsd2.html

  
#### Possible fixes ####

  * igt@gem_busy@extended-parallel-vcs1:
    - shard-iclb:         [SKIP][37] ([fdo#112080]) -> [PASS][38] +10 similar issues
   [37]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-iclb6/igt@gem_busy@extended-parallel-vcs1.html
   [38]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-iclb2/igt@gem_busy@extended-parallel-vcs1.html

  * igt@gem_exec_schedule@pi-distinct-iova-bsd:
    - shard-iclb:         [SKIP][39] ([i915#677]) -> [PASS][40] +1 similar issue
   [39]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-iclb2/igt@gem_exec_schedule@pi-distinct-iova-bsd.html
   [40]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-iclb7/igt@gem_exec_schedule@pi-distinct-iova-bsd.html

  * igt@gem_exec_schedule@preemptive-hang-bsd:
    - shard-iclb:         [SKIP][41] ([fdo#112146]) -> [PASS][42] +3 similar issues
   [41]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-iclb2/igt@gem_exec_schedule@preemptive-hang-bsd.html
   [42]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-iclb7/igt@gem_exec_schedule@preemptive-hang-bsd.html

  * igt@gen9_exec_parse@allowed-all:
    - shard-skl:          [DMESG-WARN][43] ([i915#716]) -> [PASS][44]
   [43]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-skl1/igt@gen9_exec_parse@allowed-all.html
   [44]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-skl7/igt@gen9_exec_parse@allowed-all.html

  * igt@i915_suspend@debugfs-reader:
    - shard-apl:          [DMESG-WARN][45] ([i915#180]) -> [PASS][46] +1 similar issue
   [45]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-apl6/igt@i915_suspend@debugfs-reader.html
   [46]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-apl6/igt@i915_suspend@debugfs-reader.html

  * igt@kms_cursor_crc@pipe-c-cursor-suspend:
    - shard-kbl:          [DMESG-WARN][47] ([i915#180]) -> [PASS][48] +2 similar issues
   [47]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-kbl7/igt@kms_cursor_crc@pipe-c-cursor-suspend.html
   [48]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-kbl7/igt@kms_cursor_crc@pipe-c-cursor-suspend.html

  * igt@kms_flip@plain-flip-fb-recreate:
    - shard-skl:          [FAIL][49] ([i915#34]) -> [PASS][50]
   [49]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-skl10/igt@kms_flip@plain-flip-fb-recreate.html
   [50]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-skl4/igt@kms_flip@plain-flip-fb-recreate.html

  * igt@kms_hdr@bpc-switch-dpms:
    - shard-skl:          [FAIL][51] ([i915#1188]) -> [PASS][52]
   [51]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-skl6/igt@kms_hdr@bpc-switch-dpms.html
   [52]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-skl2/igt@kms_hdr@bpc-switch-dpms.html

  * igt@kms_plane_alpha_blend@pipe-c-constant-alpha-min:
    - shard-skl:          [FAIL][53] ([fdo#108145]) -> [PASS][54] +1 similar issue
   [53]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-skl5/igt@kms_plane_alpha_blend@pipe-c-constant-alpha-min.html
   [54]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-skl6/igt@kms_plane_alpha_blend@pipe-c-constant-alpha-min.html

  * igt@kms_plane_lowres@pipe-a-tiling-y:
    - shard-glk:          [FAIL][55] ([i915#899]) -> [PASS][56]
   [55]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-glk8/igt@kms_plane_lowres@pipe-a-tiling-y.html
   [56]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-glk7/igt@kms_plane_lowres@pipe-a-tiling-y.html

  * igt@kms_psr@psr2_cursor_plane_move:
    - shard-iclb:         [SKIP][57] ([fdo#109441]) -> [PASS][58] +2 similar issues
   [57]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-iclb6/igt@kms_psr@psr2_cursor_plane_move.html
   [58]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-iclb2/igt@kms_psr@psr2_cursor_plane_move.html

  * igt@prime_vgem@wait-bsd2:
    - shard-iclb:         [SKIP][59] ([fdo#109276]) -> [PASS][60] +12 similar issues
   [59]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-iclb8/igt@prime_vgem@wait-bsd2.html
   [60]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-iclb4/igt@prime_vgem@wait-bsd2.html

  
#### Warnings ####

  * igt@kms_plane_alpha_blend@pipe-c-alpha-basic:
    - shard-apl:          [FAIL][61] ([fdo#108145]) -> [FAIL][62] ([fdo#108145] / [i915#95])
   [61]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-apl3/igt@kms_plane_alpha_blend@pipe-c-alpha-basic.html
   [62]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-apl1/igt@kms_plane_alpha_blend@pipe-c-alpha-basic.html

  * igt@kms_psr2_su@page_flip:
    - shard-iclb:         [FAIL][63] ([i915#608]) -> [SKIP][64] ([fdo#109642] / [fdo#111068])
   [63]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8190/shard-iclb2/igt@kms_psr2_su@page_flip.html
   [64]: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_17092/shard-iclb4/igt@kms_psr2_su@page_flip.html

  
  [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#109644]: https://bugs.freedesktop.org/show_bug.cgi?id=109644
  [fdo#110464]: https://bugs.freedesktop.org/show_bug.cgi?id=110464
  [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#1031]: https://gitlab.freedesktop.org/drm/intel/issues/1031
  [i915#1188]: https://gitlab.freedesktop.org/drm/intel/issues/1188
  [i915#180]: https://gitlab.freedesktop.org/drm/intel/issues/180
  [i915#265]: https://gitlab.freedesktop.org/drm/intel/issues/265
  [i915#34]: https://gitlab.freedesktop.org/drm/intel/issues/34
  [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#716]: https://gitlab.freedesktop.org/drm/intel/issues/716
  [i915#899]: https://gitlab.freedesktop.org/drm/intel/issues/899
  [i915#93]: https://gitlab.freedesktop.org/drm/intel/issues/93
  [i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95


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

  No changes in participating hosts


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

  * CI: CI-20190529 -> None
  * Linux: CI_DRM_8190 -> Patchwork_17092

  CI-20190529: 20190529
  CI_DRM_8190: 73f711b364bc85c8a7189487c09431eb1f515ed0 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGT_5538: 47becbc9cd1fc7b1b78692f90fd3dcd5a9066965 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
  Patchwork_17092: fae3ed433b36110cf2f05ca2bc9100632293334d @ 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_17092/index.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers
  2020-03-26  4:43 [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers Ashutosh Dixit
  2020-03-26  5:29 ` [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/perf: Do not clear pollin for small user read buffers (rev2) Patchwork
  2020-03-26  6:47 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
@ 2020-03-26  9:09 ` Lionel Landwerlin
  2020-03-27  3:39   ` Dixit, Ashutosh
  2 siblings, 1 reply; 29+ messages in thread
From: Lionel Landwerlin @ 2020-03-26  9:09 UTC (permalink / raw)
  To: Ashutosh Dixit, intel-gfx

On 26/03/2020 06:43, Ashutosh Dixit wrote:
> It is wrong to block the user thread in the next poll when OA data is
> already available which could not fit in the user buffer provided in
> the previous read. In several cases the exact user buffer size is not
> known. Blocking user space in poll can lead to data loss when the
> buffer size used is smaller than the available data.
>
> This change fixes this issue and allows user space to read all OA data
> even when using a buffer size smaller than the available data using
> multiple non-blocking reads rather than staying blocked in poll till
> the next timer interrupt.
>
> v2: Fix ret value for blocking reads (Umesh)
>
> Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_perf.c | 63 ++++++--------------------------
>   1 file changed, 12 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index 3222f6cd8255..e2d083efba6d 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -2957,49 +2957,6 @@ void i915_oa_init_reg_state(const struct intel_context *ce,
>   		gen8_update_reg_state_unlocked(ce, stream);
>   }
>   
> -/**
> - * i915_perf_read_locked - &i915_perf_stream_ops->read with error normalisation
> - * @stream: An i915 perf stream
> - * @file: An i915 perf stream file
> - * @buf: destination buffer given by userspace
> - * @count: the number of bytes userspace wants to read
> - * @ppos: (inout) file seek position (unused)
> - *
> - * Besides wrapping &i915_perf_stream_ops->read this provides a common place to
> - * ensure that if we've successfully copied any data then reporting that takes
> - * precedence over any internal error status, so the data isn't lost.
> - *
> - * For example ret will be -ENOSPC whenever there is more buffered data than
> - * can be copied to userspace, but that's only interesting if we weren't able
> - * to copy some data because it implies the userspace buffer is too small to
> - * receive a single record (and we never split records).
> - *
> - * Another case with ret == -EFAULT is more of a grey area since it would seem
> - * like bad form for userspace to ask us to overrun its buffer, but the user
> - * knows best:
> - *
> - *   http://yarchive.net/comp/linux/partial_reads_writes.html
> - *
> - * Returns: The number of bytes copied or a negative error code on failure.
> - */
> -static ssize_t i915_perf_read_locked(struct i915_perf_stream *stream,
> -				     struct file *file,
> -				     char __user *buf,
> -				     size_t count,
> -				     loff_t *ppos)
> -{
> -	/* Note we keep the offset (aka bytes read) separate from any
> -	 * error status so that the final check for whether we return
> -	 * the bytes read with a higher precedence than any error (see
> -	 * comment below) doesn't need to be handled/duplicated in
> -	 * stream->ops->read() implementations.
> -	 */
> -	size_t offset = 0;
> -	int ret = stream->ops->read(stream, buf, count, &offset);
> -
> -	return offset ?: (ret ?: -EAGAIN);
> -}
> -
>   /**
>    * i915_perf_read - handles read() FOP for i915 perf stream FDs
>    * @file: An i915 perf stream file
> @@ -3025,6 +2982,8 @@ static ssize_t i915_perf_read(struct file *file,
>   {
>   	struct i915_perf_stream *stream = file->private_data;
>   	struct i915_perf *perf = stream->perf;
> +	size_t offset = 0;
> +	int __ret;
>   	ssize_t ret;
>   
>   	/* To ensure it's handled consistently we simply treat all reads of a
> @@ -3048,16 +3007,19 @@ static ssize_t i915_perf_read(struct file *file,
>   				return ret;
>   
>   			mutex_lock(&perf->lock);
> -			ret = i915_perf_read_locked(stream, file,
> -						    buf, count, ppos);
> +			__ret = stream->ops->read(stream, buf, count, &offset);
> +			ret = offset ?: (__ret ?: -EAGAIN);
>   			mutex_unlock(&perf->lock);
>   		} while (ret == -EAGAIN);
>   	} else {
>   		mutex_lock(&perf->lock);
> -		ret = i915_perf_read_locked(stream, file, buf, count, ppos);
> +		__ret = stream->ops->read(stream, buf, count, &offset);
> +		ret = offset ?: (__ret ?: -EAGAIN);
>   		mutex_unlock(&perf->lock);
>   	}
>   
> +	/* Possible values for __ret are 0, -EFAULT, -ENOSPC, -EAGAIN, ... */
> +
>   	/* We allow the poll checking to sometimes report false positive EPOLLIN
>   	 * events where we might actually report EAGAIN on read() if there's
>   	 * not really any data available. In this situation though we don't
> @@ -3065,13 +3027,12 @@ static ssize_t i915_perf_read(struct file *file,
>   	 * and read() returning -EAGAIN. Clearing the oa.pollin state here
>   	 * effectively ensures we back off until the next hrtimer callback
>   	 * before reporting another EPOLLIN event.
> +	 * The exception to this is if ops->read() returned -ENOSPC which means
> +	 * that more OA data is available than could fit in the user provided
> +	 * buffer. In this case we want the next poll() call to not block.
>   	 */
> -	if (ret >= 0 || ret == -EAGAIN) {
> -		/* Maybe make ->pollin per-stream state if we support multiple
> -		 * concurrent streams in the future.
> -		 */
> +	if ((ret > 0 || ret == -EAGAIN) && __ret != -ENOSPC)
>   		stream->pollin = false;
> -	}
>   
>   	return ret;
>   }

I think this reset of the pollin field is in the wrong place in the driver.

The decision of whether pollin is true/false should be based off the 
difference between head/tail pointers.


In my opinion the best place to do this in at the end of 
gen7/8_append_oa_reports functions, under the stream->oa_buffer.ptr_lock.

If everything has been read up to the tail pointer, then there is 
nothing to wake up userspace for, otherwise leave pollin untouched.


-Lionel

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

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

* Re: [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers
  2020-03-26  9:09 ` [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers Lionel Landwerlin
@ 2020-03-27  3:39   ` Dixit, Ashutosh
  0 siblings, 0 replies; 29+ messages in thread
From: Dixit, Ashutosh @ 2020-03-27  3:39 UTC (permalink / raw)
  To: Lionel Landwerlin; +Cc: intel-gfx

On Thu, 26 Mar 2020 02:09:34 -0700, Lionel Landwerlin wrote:
>
> On 26/03/2020 06:43, Ashutosh Dixit wrote:
> > It is wrong to block the user thread in the next poll when OA data is
> > already available which could not fit in the user buffer provided in
> > the previous read. In several cases the exact user buffer size is not
> > known. Blocking user space in poll can lead to data loss when the
> > buffer size used is smaller than the available data.
> >
> > This change fixes this issue and allows user space to read all OA data
> > even when using a buffer size smaller than the available data using
> > multiple non-blocking reads rather than staying blocked in poll till
> > the next timer interrupt.
> >
> > v2: Fix ret value for blocking reads (Umesh)
> >
> > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_perf.c | 63 ++++++--------------------------
> >   1 file changed, 12 insertions(+), 51 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> > index 3222f6cd8255..e2d083efba6d 100644
> > --- a/drivers/gpu/drm/i915/i915_perf.c
> > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > @@ -2957,49 +2957,6 @@ void i915_oa_init_reg_state(const struct intel_context *ce,
> >		gen8_update_reg_state_unlocked(ce, stream);
> >   }
> >   -/**
> > - * i915_perf_read_locked - &i915_perf_stream_ops->read with error normalisation
> > - * @stream: An i915 perf stream
> > - * @file: An i915 perf stream file
> > - * @buf: destination buffer given by userspace
> > - * @count: the number of bytes userspace wants to read
> > - * @ppos: (inout) file seek position (unused)
> > - *
> > - * Besides wrapping &i915_perf_stream_ops->read this provides a common place to
> > - * ensure that if we've successfully copied any data then reporting that takes
> > - * precedence over any internal error status, so the data isn't lost.
> > - *
> > - * For example ret will be -ENOSPC whenever there is more buffered data than
> > - * can be copied to userspace, but that's only interesting if we weren't able
> > - * to copy some data because it implies the userspace buffer is too small to
> > - * receive a single record (and we never split records).
> > - *
> > - * Another case with ret == -EFAULT is more of a grey area since it would seem
> > - * like bad form for userspace to ask us to overrun its buffer, but the user
> > - * knows best:
> > - *
> > - *   http://yarchive.net/comp/linux/partial_reads_writes.html
> > - *
> > - * Returns: The number of bytes copied or a negative error code on failure.
> > - */
> > -static ssize_t i915_perf_read_locked(struct i915_perf_stream *stream,
> > -				     struct file *file,
> > -				     char __user *buf,
> > -				     size_t count,
> > -				     loff_t *ppos)
> > -{
> > -	/* Note we keep the offset (aka bytes read) separate from any
> > -	 * error status so that the final check for whether we return
> > -	 * the bytes read with a higher precedence than any error (see
> > -	 * comment below) doesn't need to be handled/duplicated in
> > -	 * stream->ops->read() implementations.
> > -	 */
> > -	size_t offset = 0;
> > -	int ret = stream->ops->read(stream, buf, count, &offset);
> > -
> > -	return offset ?: (ret ?: -EAGAIN);
> > -}
> > -
> >   /**
> >    * i915_perf_read - handles read() FOP for i915 perf stream FDs
> >    * @file: An i915 perf stream file
> > @@ -3025,6 +2982,8 @@ static ssize_t i915_perf_read(struct file *file,
> >   {
> >	struct i915_perf_stream *stream = file->private_data;
> >	struct i915_perf *perf = stream->perf;
> > +	size_t offset = 0;
> > +	int __ret;
> >	ssize_t ret;
> >		/* To ensure it's handled consistently we simply treat all reads of
> > a
> > @@ -3048,16 +3007,19 @@ static ssize_t i915_perf_read(struct file *file,
> >				return ret;
> >				mutex_lock(&perf->lock);
> > -			ret = i915_perf_read_locked(stream, file,
> > -						    buf, count, ppos);
> > +			__ret = stream->ops->read(stream, buf, count, &offset);
> > +			ret = offset ?: (__ret ?: -EAGAIN);
> >			mutex_unlock(&perf->lock);
> >		} while (ret == -EAGAIN);
> >	} else {
> >		mutex_lock(&perf->lock);
> > -		ret = i915_perf_read_locked(stream, file, buf, count, ppos);
> > +		__ret = stream->ops->read(stream, buf, count, &offset);
> > +		ret = offset ?: (__ret ?: -EAGAIN);
> >		mutex_unlock(&perf->lock);
> >	}
> >   +	/* Possible values for __ret are 0, -EFAULT, -ENOSPC, -EAGAIN,
> > ... */
> > +
> >	/* We allow the poll checking to sometimes report false positive EPOLLIN
> >	 * events where we might actually report EAGAIN on read() if there's
> >	 * not really any data available. In this situation though we don't
> > @@ -3065,13 +3027,12 @@ static ssize_t i915_perf_read(struct file *file,
> >	 * and read() returning -EAGAIN. Clearing the oa.pollin state here
> >	 * effectively ensures we back off until the next hrtimer callback
> >	 * before reporting another EPOLLIN event.
> > +	 * The exception to this is if ops->read() returned -ENOSPC which means
> > +	 * that more OA data is available than could fit in the user provided
> > +	 * buffer. In this case we want the next poll() call to not block.
> >	 */
> > -	if (ret >= 0 || ret == -EAGAIN) {
> > -		/* Maybe make ->pollin per-stream state if we support multiple
> > -		 * concurrent streams in the future.
> > -		 */
> > +	if ((ret > 0 || ret == -EAGAIN) && __ret != -ENOSPC)
> >		stream->pollin = false;
> > -	}
> >		return ret;
> >   }
>
> I think this reset of the pollin field is in the wrong place in the driver.
>
> The decision of whether pollin is true/false should be based off the
> difference between head/tail pointers.
>
> In my opinion the best place to do this in at the end of
> gen7/8_append_oa_reports functions, under the stream->oa_buffer.ptr_lock.
>
> If everything has been read up to the tail pointer, then there is nothing
> to wake up userspace for, otherwise leave pollin untouched.

Hi Lionel,

Are you seeing any problems of correctness in the code? My intention was to
use previously existing mechanisms (viz. -ENOSPC). Afais when
stream->ops->read() returns -ENOSPC it has already looked at head/tail
pointers and determined that there is data to be returned which it is
unable to because the provided buffer was too small.

Also, -ENOSPC can also be returned from append_oa_status(), though that can
probably be ignored.

Following your reasoning we should probably also say that pollin should be
set in oa_buffer_check_unlocked()?

About, stream->oa_buffer.ptr_lock, as I said previously, imo it is a lock
between a ring buffer producer (oa_buffer_check_unlocked()) and consumer
(i915_perf_read) which should not be needed, that ring buffer operation
should be lockless. Though we will need to check before removing it, maybe
I am wrong.

So unless you say there are real correctness problems in the patch or
previously existing code I am leaning towards leaving as it as is.

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

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

* Re: [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers
  2020-04-08 23:42 Ashutosh Dixit
@ 2020-04-13 22:05 ` Rodrigo Vivi
  0 siblings, 0 replies; 29+ messages in thread
From: Rodrigo Vivi @ 2020-04-13 22:05 UTC (permalink / raw)
  To: Ashutosh Dixit; +Cc: intel-gfx

On Wed, Apr 08, 2020 at 04:42:01PM -0700, Ashutosh Dixit wrote:
> It is wrong to block the user thread in the next poll when OA data is
> already available which could not fit in the user buffer provided in
> the previous read. In several cases the exact user buffer size is not
> known. Blocking user space in poll can lead to data loss when the
> buffer size used is smaller than the available data.
> 
> This change fixes this issue and allows user space to read all OA data
> even when using a buffer size smaller than the available data using
> multiple non-blocking reads rather than staying blocked in poll till
> the next timer interrupt.

Thanks for providing this.
Pushed to drm-intel-fixes targeting -rc2

> 
> v2: Fix ret value for blocking reads (Umesh)
> v3: Mistake during patch send (Ashutosh)
> v4: Remove -EAGAIN from comment (Umesh)
> v5: Improve condition for clearing pollin and return (Lionel)
> v6: Improve blocking read loop and other cleanups (Lionel)
> v7: Added Cc stable
> 
> Testcase: igt/perf/polling-small-buf
> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Link: https://patchwork.freedesktop.org/patch/msgid/20200403010120.3067-1-ashutosh.dixit@intel.com
> ---
>  drivers/gpu/drm/i915/i915_perf.c | 65 ++++++--------------------------
>  1 file changed, 11 insertions(+), 54 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index 551be589d6f4..66a46e41d5ef 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -2940,49 +2940,6 @@ void i915_oa_init_reg_state(const struct intel_context *ce,
>  		gen8_update_reg_state_unlocked(ce, stream);
>  }
>  
> -/**
> - * i915_perf_read_locked - &i915_perf_stream_ops->read with error normalisation
> - * @stream: An i915 perf stream
> - * @file: An i915 perf stream file
> - * @buf: destination buffer given by userspace
> - * @count: the number of bytes userspace wants to read
> - * @ppos: (inout) file seek position (unused)
> - *
> - * Besides wrapping &i915_perf_stream_ops->read this provides a common place to
> - * ensure that if we've successfully copied any data then reporting that takes
> - * precedence over any internal error status, so the data isn't lost.
> - *
> - * For example ret will be -ENOSPC whenever there is more buffered data than
> - * can be copied to userspace, but that's only interesting if we weren't able
> - * to copy some data because it implies the userspace buffer is too small to
> - * receive a single record (and we never split records).
> - *
> - * Another case with ret == -EFAULT is more of a grey area since it would seem
> - * like bad form for userspace to ask us to overrun its buffer, but the user
> - * knows best:
> - *
> - *   http://yarchive.net/comp/linux/partial_reads_writes.html
> - *
> - * Returns: The number of bytes copied or a negative error code on failure.
> - */
> -static ssize_t i915_perf_read_locked(struct i915_perf_stream *stream,
> -				     struct file *file,
> -				     char __user *buf,
> -				     size_t count,
> -				     loff_t *ppos)
> -{
> -	/* Note we keep the offset (aka bytes read) separate from any
> -	 * error status so that the final check for whether we return
> -	 * the bytes read with a higher precedence than any error (see
> -	 * comment below) doesn't need to be handled/duplicated in
> -	 * stream->ops->read() implementations.
> -	 */
> -	size_t offset = 0;
> -	int ret = stream->ops->read(stream, buf, count, &offset);
> -
> -	return offset ?: (ret ?: -EAGAIN);
> -}
> -
>  /**
>   * i915_perf_read - handles read() FOP for i915 perf stream FDs
>   * @file: An i915 perf stream file
> @@ -3008,7 +2965,8 @@ static ssize_t i915_perf_read(struct file *file,
>  {
>  	struct i915_perf_stream *stream = file->private_data;
>  	struct i915_perf *perf = stream->perf;
> -	ssize_t ret;
> +	size_t offset = 0;
> +	int ret;
>  
>  	/* To ensure it's handled consistently we simply treat all reads of a
>  	 * disabled stream as an error. In particular it might otherwise lead
> @@ -3031,13 +2989,12 @@ static ssize_t i915_perf_read(struct file *file,
>  				return ret;
>  
>  			mutex_lock(&perf->lock);
> -			ret = i915_perf_read_locked(stream, file,
> -						    buf, count, ppos);
> +			ret = stream->ops->read(stream, buf, count, &offset);
>  			mutex_unlock(&perf->lock);
> -		} while (ret == -EAGAIN);
> +		} while (!offset && !ret);
>  	} else {
>  		mutex_lock(&perf->lock);
> -		ret = i915_perf_read_locked(stream, file, buf, count, ppos);
> +		ret = stream->ops->read(stream, buf, count, &offset);
>  		mutex_unlock(&perf->lock);
>  	}
>  
> @@ -3048,15 +3005,15 @@ static ssize_t i915_perf_read(struct file *file,
>  	 * and read() returning -EAGAIN. Clearing the oa.pollin state here
>  	 * effectively ensures we back off until the next hrtimer callback
>  	 * before reporting another EPOLLIN event.
> +	 * The exception to this is if ops->read() returned -ENOSPC which means
> +	 * that more OA data is available than could fit in the user provided
> +	 * buffer. In this case we want the next poll() call to not block.
>  	 */
> -	if (ret >= 0 || ret == -EAGAIN) {
> -		/* Maybe make ->pollin per-stream state if we support multiple
> -		 * concurrent streams in the future.
> -		 */
> +	if (ret != -ENOSPC)
>  		stream->pollin = false;
> -	}
>  
> -	return ret;
> +	/* Possible values for ret are 0, -EFAULT, -ENOSPC, -EIO, ... */
> +	return offset ?: (ret ?: -EAGAIN);
>  }
>  
>  static enum hrtimer_restart oa_poll_check_timer_cb(struct hrtimer *hrtimer)
> -- 
> 2.25.2
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers
@ 2020-04-08 23:42 Ashutosh Dixit
  2020-04-13 22:05 ` Rodrigo Vivi
  0 siblings, 1 reply; 29+ messages in thread
From: Ashutosh Dixit @ 2020-04-08 23:42 UTC (permalink / raw)
  To: intel-gfx

It is wrong to block the user thread in the next poll when OA data is
already available which could not fit in the user buffer provided in
the previous read. In several cases the exact user buffer size is not
known. Blocking user space in poll can lead to data loss when the
buffer size used is smaller than the available data.

This change fixes this issue and allows user space to read all OA data
even when using a buffer size smaller than the available data using
multiple non-blocking reads rather than staying blocked in poll till
the next timer interrupt.

v2: Fix ret value for blocking reads (Umesh)
v3: Mistake during patch send (Ashutosh)
v4: Remove -EAGAIN from comment (Umesh)
v5: Improve condition for clearing pollin and return (Lionel)
v6: Improve blocking read loop and other cleanups (Lionel)
v7: Added Cc stable

Testcase: igt/perf/polling-small-buf
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
Cc: <stable@vger.kernel.org>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Link: https://patchwork.freedesktop.org/patch/msgid/20200403010120.3067-1-ashutosh.dixit@intel.com
---
 drivers/gpu/drm/i915/i915_perf.c | 65 ++++++--------------------------
 1 file changed, 11 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 551be589d6f4..66a46e41d5ef 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -2940,49 +2940,6 @@ void i915_oa_init_reg_state(const struct intel_context *ce,
 		gen8_update_reg_state_unlocked(ce, stream);
 }
 
-/**
- * i915_perf_read_locked - &i915_perf_stream_ops->read with error normalisation
- * @stream: An i915 perf stream
- * @file: An i915 perf stream file
- * @buf: destination buffer given by userspace
- * @count: the number of bytes userspace wants to read
- * @ppos: (inout) file seek position (unused)
- *
- * Besides wrapping &i915_perf_stream_ops->read this provides a common place to
- * ensure that if we've successfully copied any data then reporting that takes
- * precedence over any internal error status, so the data isn't lost.
- *
- * For example ret will be -ENOSPC whenever there is more buffered data than
- * can be copied to userspace, but that's only interesting if we weren't able
- * to copy some data because it implies the userspace buffer is too small to
- * receive a single record (and we never split records).
- *
- * Another case with ret == -EFAULT is more of a grey area since it would seem
- * like bad form for userspace to ask us to overrun its buffer, but the user
- * knows best:
- *
- *   http://yarchive.net/comp/linux/partial_reads_writes.html
- *
- * Returns: The number of bytes copied or a negative error code on failure.
- */
-static ssize_t i915_perf_read_locked(struct i915_perf_stream *stream,
-				     struct file *file,
-				     char __user *buf,
-				     size_t count,
-				     loff_t *ppos)
-{
-	/* Note we keep the offset (aka bytes read) separate from any
-	 * error status so that the final check for whether we return
-	 * the bytes read with a higher precedence than any error (see
-	 * comment below) doesn't need to be handled/duplicated in
-	 * stream->ops->read() implementations.
-	 */
-	size_t offset = 0;
-	int ret = stream->ops->read(stream, buf, count, &offset);
-
-	return offset ?: (ret ?: -EAGAIN);
-}
-
 /**
  * i915_perf_read - handles read() FOP for i915 perf stream FDs
  * @file: An i915 perf stream file
@@ -3008,7 +2965,8 @@ static ssize_t i915_perf_read(struct file *file,
 {
 	struct i915_perf_stream *stream = file->private_data;
 	struct i915_perf *perf = stream->perf;
-	ssize_t ret;
+	size_t offset = 0;
+	int ret;
 
 	/* To ensure it's handled consistently we simply treat all reads of a
 	 * disabled stream as an error. In particular it might otherwise lead
@@ -3031,13 +2989,12 @@ static ssize_t i915_perf_read(struct file *file,
 				return ret;
 
 			mutex_lock(&perf->lock);
-			ret = i915_perf_read_locked(stream, file,
-						    buf, count, ppos);
+			ret = stream->ops->read(stream, buf, count, &offset);
 			mutex_unlock(&perf->lock);
-		} while (ret == -EAGAIN);
+		} while (!offset && !ret);
 	} else {
 		mutex_lock(&perf->lock);
-		ret = i915_perf_read_locked(stream, file, buf, count, ppos);
+		ret = stream->ops->read(stream, buf, count, &offset);
 		mutex_unlock(&perf->lock);
 	}
 
@@ -3048,15 +3005,15 @@ static ssize_t i915_perf_read(struct file *file,
 	 * and read() returning -EAGAIN. Clearing the oa.pollin state here
 	 * effectively ensures we back off until the next hrtimer callback
 	 * before reporting another EPOLLIN event.
+	 * The exception to this is if ops->read() returned -ENOSPC which means
+	 * that more OA data is available than could fit in the user provided
+	 * buffer. In this case we want the next poll() call to not block.
 	 */
-	if (ret >= 0 || ret == -EAGAIN) {
-		/* Maybe make ->pollin per-stream state if we support multiple
-		 * concurrent streams in the future.
-		 */
+	if (ret != -ENOSPC)
 		stream->pollin = false;
-	}
 
-	return ret;
+	/* Possible values for ret are 0, -EFAULT, -ENOSPC, -EIO, ... */
+	return offset ?: (ret ?: -EAGAIN);
 }
 
 static enum hrtimer_restart oa_poll_check_timer_cb(struct hrtimer *hrtimer)
-- 
2.25.2

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

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

* Re: [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers
  2020-04-03 17:45   ` Dixit, Ashutosh
@ 2020-04-03 17:59     ` Chris Wilson
  0 siblings, 0 replies; 29+ messages in thread
From: Chris Wilson @ 2020-04-03 17:59 UTC (permalink / raw)
  To: Dixit, Ashutosh; +Cc: intel-gfx

Quoting Dixit, Ashutosh (2020-04-03 18:45:09)
> On Fri, 03 Apr 2020 09:17:14 -0700, Chris Wilson wrote:
> >
> > Quoting Ashutosh Dixit (2020-04-03 02:01:20)
> > > It is wrong to block the user thread in the next poll when OA data is
> > > already available which could not fit in the user buffer provided in
> > > the previous read. In several cases the exact user buffer size is not
> > > known. Blocking user space in poll can lead to data loss when the
> > > buffer size used is smaller than the available data.
> > >
> > > This change fixes this issue and allows user space to read all OA data
> > > even when using a buffer size smaller than the available data using
> > > multiple non-blocking reads rather than staying blocked in poll till
> > > the next timer interrupt.
> > >
> > > v2: Fix ret value for blocking reads (Umesh)
> > > v3: Mistake during patch send (Ashutosh)
> > > v4: Remove -EAGAIN from comment (Umesh)
> > > v5: Improve condition for clearing pollin and return (Lionel)
> > > v6: Improve blocking read loop and other cleanups (Lionel)
> > > v7: Added Cc stable
> > >
> > > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> > > Cc: <stable@vger.kernel.org>
> > > Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> > > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> >
> > Did you manage to devise a test case? It is nice (some might say
> > important) to pair a patch for stable with its regression test.
> 
> Yes there is a test case here:
> 
> https://patchwork.freedesktop.org/series/75100/#rev3
> 
> Lionel verified that it is fails on stable kernels here:
> 
> https://patchwork.freedesktop.org/patch/358873/?series=75100&rev=1

Ta. Pushed both,
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers
  2020-04-03 16:17 ` Chris Wilson
@ 2020-04-03 17:45   ` Dixit, Ashutosh
  2020-04-03 17:59     ` Chris Wilson
  0 siblings, 1 reply; 29+ messages in thread
From: Dixit, Ashutosh @ 2020-04-03 17:45 UTC (permalink / raw)
  To: Chris Wilson; +Cc: intel-gfx

On Fri, 03 Apr 2020 09:17:14 -0700, Chris Wilson wrote:
>
> Quoting Ashutosh Dixit (2020-04-03 02:01:20)
> > It is wrong to block the user thread in the next poll when OA data is
> > already available which could not fit in the user buffer provided in
> > the previous read. In several cases the exact user buffer size is not
> > known. Blocking user space in poll can lead to data loss when the
> > buffer size used is smaller than the available data.
> >
> > This change fixes this issue and allows user space to read all OA data
> > even when using a buffer size smaller than the available data using
> > multiple non-blocking reads rather than staying blocked in poll till
> > the next timer interrupt.
> >
> > v2: Fix ret value for blocking reads (Umesh)
> > v3: Mistake during patch send (Ashutosh)
> > v4: Remove -EAGAIN from comment (Umesh)
> > v5: Improve condition for clearing pollin and return (Lionel)
> > v6: Improve blocking read loop and other cleanups (Lionel)
> > v7: Added Cc stable
> >
> > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> > Cc: <stable@vger.kernel.org>
> > Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>
> Did you manage to devise a test case? It is nice (some might say
> important) to pair a patch for stable with its regression test.

Yes there is a test case here:

https://patchwork.freedesktop.org/series/75100/#rev3

Lionel verified that it is fails on stable kernels here:

https://patchwork.freedesktop.org/patch/358873/?series=75100&rev=1

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

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

* Re: [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers
  2020-04-03  1:01 Ashutosh Dixit
@ 2020-04-03 16:17 ` Chris Wilson
  2020-04-03 17:45   ` Dixit, Ashutosh
  0 siblings, 1 reply; 29+ messages in thread
From: Chris Wilson @ 2020-04-03 16:17 UTC (permalink / raw)
  To: Ashutosh Dixit, intel-gfx

Quoting Ashutosh Dixit (2020-04-03 02:01:20)
> It is wrong to block the user thread in the next poll when OA data is
> already available which could not fit in the user buffer provided in
> the previous read. In several cases the exact user buffer size is not
> known. Blocking user space in poll can lead to data loss when the
> buffer size used is smaller than the available data.
> 
> This change fixes this issue and allows user space to read all OA data
> even when using a buffer size smaller than the available data using
> multiple non-blocking reads rather than staying blocked in poll till
> the next timer interrupt.
> 
> v2: Fix ret value for blocking reads (Umesh)
> v3: Mistake during patch send (Ashutosh)
> v4: Remove -EAGAIN from comment (Umesh)
> v5: Improve condition for clearing pollin and return (Lionel)
> v6: Improve blocking read loop and other cleanups (Lionel)
> v7: Added Cc stable
> 
> Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> Cc: <stable@vger.kernel.org>
> Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>

Did you manage to devise a test case? It is nice (some might say
important) to pair a patch for stable with its regression test.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers
@ 2020-04-03  1:01 Ashutosh Dixit
  2020-04-03 16:17 ` Chris Wilson
  0 siblings, 1 reply; 29+ messages in thread
From: Ashutosh Dixit @ 2020-04-03  1:01 UTC (permalink / raw)
  To: intel-gfx

It is wrong to block the user thread in the next poll when OA data is
already available which could not fit in the user buffer provided in
the previous read. In several cases the exact user buffer size is not
known. Blocking user space in poll can lead to data loss when the
buffer size used is smaller than the available data.

This change fixes this issue and allows user space to read all OA data
even when using a buffer size smaller than the available data using
multiple non-blocking reads rather than staying blocked in poll till
the next timer interrupt.

v2: Fix ret value for blocking reads (Umesh)
v3: Mistake during patch send (Ashutosh)
v4: Remove -EAGAIN from comment (Umesh)
v5: Improve condition for clearing pollin and return (Lionel)
v6: Improve blocking read loop and other cleanups (Lionel)
v7: Added Cc stable

Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
Cc: <stable@vger.kernel.org>
Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
 drivers/gpu/drm/i915/i915_perf.c | 61 ++++++--------------------------
 1 file changed, 11 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 28e3d76fa2e6..2f78b147bb2d 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -2963,49 +2963,6 @@ void i915_oa_init_reg_state(const struct intel_context *ce,
 		gen8_update_reg_state_unlocked(ce, stream);
 }
 
-/**
- * i915_perf_read_locked - &i915_perf_stream_ops->read with error normalisation
- * @stream: An i915 perf stream
- * @file: An i915 perf stream file
- * @buf: destination buffer given by userspace
- * @count: the number of bytes userspace wants to read
- * @ppos: (inout) file seek position (unused)
- *
- * Besides wrapping &i915_perf_stream_ops->read this provides a common place to
- * ensure that if we've successfully copied any data then reporting that takes
- * precedence over any internal error status, so the data isn't lost.
- *
- * For example ret will be -ENOSPC whenever there is more buffered data than
- * can be copied to userspace, but that's only interesting if we weren't able
- * to copy some data because it implies the userspace buffer is too small to
- * receive a single record (and we never split records).
- *
- * Another case with ret == -EFAULT is more of a grey area since it would seem
- * like bad form for userspace to ask us to overrun its buffer, but the user
- * knows best:
- *
- *   http://yarchive.net/comp/linux/partial_reads_writes.html
- *
- * Returns: The number of bytes copied or a negative error code on failure.
- */
-static ssize_t i915_perf_read_locked(struct i915_perf_stream *stream,
-				     struct file *file,
-				     char __user *buf,
-				     size_t count,
-				     loff_t *ppos)
-{
-	/* Note we keep the offset (aka bytes read) separate from any
-	 * error status so that the final check for whether we return
-	 * the bytes read with a higher precedence than any error (see
-	 * comment below) doesn't need to be handled/duplicated in
-	 * stream->ops->read() implementations.
-	 */
-	size_t offset = 0;
-	int ret = stream->ops->read(stream, buf, count, &offset);
-
-	return offset ?: (ret ?: -EAGAIN);
-}
-
 /**
  * i915_perf_read - handles read() FOP for i915 perf stream FDs
  * @file: An i915 perf stream file
@@ -3031,7 +2988,8 @@ static ssize_t i915_perf_read(struct file *file,
 {
 	struct i915_perf_stream *stream = file->private_data;
 	struct i915_perf *perf = stream->perf;
-	ssize_t ret;
+	size_t offset = 0;
+	int ret;
 
 	/* To ensure it's handled consistently we simply treat all reads of a
 	 * disabled stream as an error. In particular it might otherwise lead
@@ -3054,13 +3012,12 @@ static ssize_t i915_perf_read(struct file *file,
 				return ret;
 
 			mutex_lock(&perf->lock);
-			ret = i915_perf_read_locked(stream, file,
-						    buf, count, ppos);
+			ret = stream->ops->read(stream, buf, count, &offset);
 			mutex_unlock(&perf->lock);
-		} while (ret == -EAGAIN);
+		} while (!offset && !ret);
 	} else {
 		mutex_lock(&perf->lock);
-		ret = i915_perf_read_locked(stream, file, buf, count, ppos);
+		ret = stream->ops->read(stream, buf, count, &offset);
 		mutex_unlock(&perf->lock);
 	}
 
@@ -3071,11 +3028,15 @@ static ssize_t i915_perf_read(struct file *file,
 	 * and read() returning -EAGAIN. Clearing the oa.pollin state here
 	 * effectively ensures we back off until the next hrtimer callback
 	 * before reporting another EPOLLIN event.
+	 * The exception to this is if ops->read() returned -ENOSPC which means
+	 * that more OA data is available than could fit in the user provided
+	 * buffer. In this case we want the next poll() call to not block.
 	 */
-	if (ret >= 0 || ret == -EAGAIN)
+	if (ret != -ENOSPC)
 		stream->pollin = false;
 
-	return ret;
+	/* Possible values for ret are 0, -EFAULT, -ENOSPC, -EIO, ... */
+	return offset ?: (ret ?: -EAGAIN);
 }
 
 static enum hrtimer_restart oa_poll_check_timer_cb(struct hrtimer *hrtimer)
-- 
2.25.2

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

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

* Re: [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers
  2020-04-01  7:43   ` Dixit, Ashutosh
@ 2020-04-01  7:51     ` Lionel Landwerlin
  0 siblings, 0 replies; 29+ messages in thread
From: Lionel Landwerlin @ 2020-04-01  7:51 UTC (permalink / raw)
  To: Dixit, Ashutosh; +Cc: intel-gfx

On 01/04/2020 10:43, Dixit, Ashutosh wrote:
> On Tue, 31 Mar 2020 23:57:57 -0700, Lionel Landwerlin wrote:
>> On 01/04/2020 02:14, Ashutosh Dixit wrote:
>>> It is wrong to block the user thread in the next poll when OA data is
>>> already available which could not fit in the user buffer provided in
>>> the previous read. In several cases the exact user buffer size is not
>>> known. Blocking user space in poll can lead to data loss when the
>>> buffer size used is smaller than the available data.
>>>
>>> This change fixes this issue and allows user space to read all OA data
>>> even when using a buffer size smaller than the available data using
>>> multiple non-blocking reads rather than staying blocked in poll till
>>> the next timer interrupt.
>>>
>>> v2: Fix ret value for blocking reads (Umesh)
>>> v3: Mistake during patch send (Ashutosh)
>>> v4: Remove -EAGAIN from comment (Umesh)
>>> v5: Improve condition for clearing pollin and return (Lionel)
>>> v6: Improve blocking read loop and other cleanups (Lionel)
>>>
>>> Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>>> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>>> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>>> ---
>>>    drivers/gpu/drm/i915/i915_perf.c | 61 ++++++--------------------------
>>>    1 file changed, 11 insertions(+), 50 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
>>> index 28e3d76fa2e6..2f78b147bb2d 100644
>>> --- a/drivers/gpu/drm/i915/i915_perf.c
>>> +++ b/drivers/gpu/drm/i915/i915_perf.c
>>> @@ -2963,49 +2963,6 @@ void i915_oa_init_reg_state(const struct intel_context *ce,
>>> 		gen8_update_reg_state_unlocked(ce, stream);
>>>    }
>>>    -/**
>>> - * i915_perf_read_locked - &i915_perf_stream_ops->read with error normalisation
>>> - * @stream: An i915 perf stream
>>> - * @file: An i915 perf stream file
>>> - * @buf: destination buffer given by userspace
>>> - * @count: the number of bytes userspace wants to read
>>> - * @ppos: (inout) file seek position (unused)
>>> - *
>>> - * Besides wrapping &i915_perf_stream_ops->read this provides a common place to
>>> - * ensure that if we've successfully copied any data then reporting that takes
>>> - * precedence over any internal error status, so the data isn't lost.
>>> - *
>>> - * For example ret will be -ENOSPC whenever there is more buffered data than
>>> - * can be copied to userspace, but that's only interesting if we weren't able
>>> - * to copy some data because it implies the userspace buffer is too small to
>>> - * receive a single record (and we never split records).
>>> - *
>>> - * Another case with ret == -EFAULT is more of a grey area since it would seem
>>> - * like bad form for userspace to ask us to overrun its buffer, but the user
>>> - * knows best:
>>> - *
>>> - *   http://yarchive.net/comp/linux/partial_reads_writes.html
>>> - *
>>> - * Returns: The number of bytes copied or a negative error code on failure.
>>> - */
>>> -static ssize_t i915_perf_read_locked(struct i915_perf_stream *stream,
>>> -				     struct file *file,
>>> -				     char __user *buf,
>>> -				     size_t count,
>>> -				     loff_t *ppos)
>>> -{
>>> -	/* Note we keep the offset (aka bytes read) separate from any
>>> -	 * error status so that the final check for whether we return
>>> -	 * the bytes read with a higher precedence than any error (see
>>> -	 * comment below) doesn't need to be handled/duplicated in
>>> -	 * stream->ops->read() implementations.
>>> -	 */
>>> -	size_t offset = 0;
>>> -	int ret = stream->ops->read(stream, buf, count, &offset);
>>> -
>>> -	return offset ?: (ret ?: -EAGAIN);
>>> -}
>>> -
>>>    /**
>>>     * i915_perf_read - handles read() FOP for i915 perf stream FDs
>>>     * @file: An i915 perf stream file
>>> @@ -3031,7 +2988,8 @@ static ssize_t i915_perf_read(struct file *file,
>>>    {
>>> 	struct i915_perf_stream *stream = file->private_data;
>>> 	struct i915_perf *perf = stream->perf;
>>> -	ssize_t ret;
>>> +	size_t offset = 0;
>>> +	int ret;
>>> 		/* To ensure it's handled consistently we simply treat all reads of
>>> a
>>> 	 * disabled stream as an error. In particular it might otherwise lead
>>> @@ -3054,13 +3012,12 @@ static ssize_t i915_perf_read(struct file *file,
>>> 				return ret;
>>> 				mutex_lock(&perf->lock);
>>> -			ret = i915_perf_read_locked(stream, file,
>>> -						    buf, count, ppos);
>>> +			ret = stream->ops->read(stream, buf, count, &offset);
>>> 			mutex_unlock(&perf->lock);
>>> -		} while (ret == -EAGAIN);
>>> +		} while (!offset && !ret);
>> This doesn't sound right, !offset means it will stop as soon as some data
>> was written.
>>
>> But for the blocking read we want to fill the buffer up to -ENOSPC.
> I don't think that's true. Here's 'man 2 read': "read() attempts to read
> /up to/ count bytes" and "It is not an error if this number is smaller than
> the number of bytes requested".
>
> The condition (!offset && !ret) is exactly equivalent to the condition (ret
> == -EAGAIN) in the original code (currently on drm-tip). The driver is free
> to unblock the blocking read whenever it determines "there is data". Our
> determination of "there is data" is "we are woken up by the OA timer and
> call ops->read() and offset > 0". (Offset will be equal to min(amount of
> data available, space in the user buffer)). The only constraint seems to be
> that the blocking read cannot return -EAGAIN (0 bytes) and the loop in the
> code guards against that.
>
>> while (ret >= 0) doesn't work?
> Because this is not the logic in the original code and I see no reason to
> change that logic. It will also change the blocking read behavior which
> according to some people is a breakage of the uAPI. The purpose of the
> patch is to fix the non blocking read path (poll + non-blocking read). It
> should not affect blocking read imo.
>
> Thanks!
> --
> Ashutosh

Ah sorry, you're right :)


Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>

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

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

* Re: [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers
  2020-04-01  6:57 ` Lionel Landwerlin
@ 2020-04-01  7:43   ` Dixit, Ashutosh
  2020-04-01  7:51     ` Lionel Landwerlin
  0 siblings, 1 reply; 29+ messages in thread
From: Dixit, Ashutosh @ 2020-04-01  7:43 UTC (permalink / raw)
  To: Lionel Landwerlin; +Cc: intel-gfx

On Tue, 31 Mar 2020 23:57:57 -0700, Lionel Landwerlin wrote:
>
> On 01/04/2020 02:14, Ashutosh Dixit wrote:
> > It is wrong to block the user thread in the next poll when OA data is
> > already available which could not fit in the user buffer provided in
> > the previous read. In several cases the exact user buffer size is not
> > known. Blocking user space in poll can lead to data loss when the
> > buffer size used is smaller than the available data.
> >
> > This change fixes this issue and allows user space to read all OA data
> > even when using a buffer size smaller than the available data using
> > multiple non-blocking reads rather than staying blocked in poll till
> > the next timer interrupt.
> >
> > v2: Fix ret value for blocking reads (Umesh)
> > v3: Mistake during patch send (Ashutosh)
> > v4: Remove -EAGAIN from comment (Umesh)
> > v5: Improve condition for clearing pollin and return (Lionel)
> > v6: Improve blocking read loop and other cleanups (Lionel)
> >
> > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > ---
> >   drivers/gpu/drm/i915/i915_perf.c | 61 ++++++--------------------------
> >   1 file changed, 11 insertions(+), 50 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> > index 28e3d76fa2e6..2f78b147bb2d 100644
> > --- a/drivers/gpu/drm/i915/i915_perf.c
> > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > @@ -2963,49 +2963,6 @@ void i915_oa_init_reg_state(const struct intel_context *ce,
> >		gen8_update_reg_state_unlocked(ce, stream);
> >   }
> >   -/**
> > - * i915_perf_read_locked - &i915_perf_stream_ops->read with error normalisation
> > - * @stream: An i915 perf stream
> > - * @file: An i915 perf stream file
> > - * @buf: destination buffer given by userspace
> > - * @count: the number of bytes userspace wants to read
> > - * @ppos: (inout) file seek position (unused)
> > - *
> > - * Besides wrapping &i915_perf_stream_ops->read this provides a common place to
> > - * ensure that if we've successfully copied any data then reporting that takes
> > - * precedence over any internal error status, so the data isn't lost.
> > - *
> > - * For example ret will be -ENOSPC whenever there is more buffered data than
> > - * can be copied to userspace, but that's only interesting if we weren't able
> > - * to copy some data because it implies the userspace buffer is too small to
> > - * receive a single record (and we never split records).
> > - *
> > - * Another case with ret == -EFAULT is more of a grey area since it would seem
> > - * like bad form for userspace to ask us to overrun its buffer, but the user
> > - * knows best:
> > - *
> > - *   http://yarchive.net/comp/linux/partial_reads_writes.html
> > - *
> > - * Returns: The number of bytes copied or a negative error code on failure.
> > - */
> > -static ssize_t i915_perf_read_locked(struct i915_perf_stream *stream,
> > -				     struct file *file,
> > -				     char __user *buf,
> > -				     size_t count,
> > -				     loff_t *ppos)
> > -{
> > -	/* Note we keep the offset (aka bytes read) separate from any
> > -	 * error status so that the final check for whether we return
> > -	 * the bytes read with a higher precedence than any error (see
> > -	 * comment below) doesn't need to be handled/duplicated in
> > -	 * stream->ops->read() implementations.
> > -	 */
> > -	size_t offset = 0;
> > -	int ret = stream->ops->read(stream, buf, count, &offset);
> > -
> > -	return offset ?: (ret ?: -EAGAIN);
> > -}
> > -
> >   /**
> >    * i915_perf_read - handles read() FOP for i915 perf stream FDs
> >    * @file: An i915 perf stream file
> > @@ -3031,7 +2988,8 @@ static ssize_t i915_perf_read(struct file *file,
> >   {
> >	struct i915_perf_stream *stream = file->private_data;
> >	struct i915_perf *perf = stream->perf;
> > -	ssize_t ret;
> > +	size_t offset = 0;
> > +	int ret;
> >		/* To ensure it's handled consistently we simply treat all reads of
> > a
> >	 * disabled stream as an error. In particular it might otherwise lead
> > @@ -3054,13 +3012,12 @@ static ssize_t i915_perf_read(struct file *file,
> >				return ret;
> >				mutex_lock(&perf->lock);
> > -			ret = i915_perf_read_locked(stream, file,
> > -						    buf, count, ppos);
> > +			ret = stream->ops->read(stream, buf, count, &offset);
> >			mutex_unlock(&perf->lock);
> > -		} while (ret == -EAGAIN);
> > +		} while (!offset && !ret);
>
> This doesn't sound right, !offset means it will stop as soon as some data
> was written.
>
> But for the blocking read we want to fill the buffer up to -ENOSPC.

I don't think that's true. Here's 'man 2 read': "read() attempts to read
/up to/ count bytes" and "It is not an error if this number is smaller than
the number of bytes requested".

The condition (!offset && !ret) is exactly equivalent to the condition (ret
== -EAGAIN) in the original code (currently on drm-tip). The driver is free
to unblock the blocking read whenever it determines "there is data". Our
determination of "there is data" is "we are woken up by the OA timer and
call ops->read() and offset > 0". (Offset will be equal to min(amount of
data available, space in the user buffer)). The only constraint seems to be
that the blocking read cannot return -EAGAIN (0 bytes) and the loop in the
code guards against that.

> while (ret >= 0) doesn't work?

Because this is not the logic in the original code and I see no reason to
change that logic. It will also change the blocking read behavior which
according to some people is a breakage of the uAPI. The purpose of the
patch is to fix the non blocking read path (poll + non-blocking read). It
should not affect blocking read imo.

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

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

* Re: [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers
  2020-03-31 23:14 Ashutosh Dixit
@ 2020-04-01  6:57 ` Lionel Landwerlin
  2020-04-01  7:43   ` Dixit, Ashutosh
  0 siblings, 1 reply; 29+ messages in thread
From: Lionel Landwerlin @ 2020-04-01  6:57 UTC (permalink / raw)
  To: Ashutosh Dixit, intel-gfx

On 01/04/2020 02:14, Ashutosh Dixit wrote:
> It is wrong to block the user thread in the next poll when OA data is
> already available which could not fit in the user buffer provided in
> the previous read. In several cases the exact user buffer size is not
> known. Blocking user space in poll can lead to data loss when the
> buffer size used is smaller than the available data.
>
> This change fixes this issue and allows user space to read all OA data
> even when using a buffer size smaller than the available data using
> multiple non-blocking reads rather than staying blocked in poll till
> the next timer interrupt.
>
> v2: Fix ret value for blocking reads (Umesh)
> v3: Mistake during patch send (Ashutosh)
> v4: Remove -EAGAIN from comment (Umesh)
> v5: Improve condition for clearing pollin and return (Lionel)
> v6: Improve blocking read loop and other cleanups (Lionel)
>
> Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_perf.c | 61 ++++++--------------------------
>   1 file changed, 11 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index 28e3d76fa2e6..2f78b147bb2d 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -2963,49 +2963,6 @@ void i915_oa_init_reg_state(const struct intel_context *ce,
>   		gen8_update_reg_state_unlocked(ce, stream);
>   }
>   
> -/**
> - * i915_perf_read_locked - &i915_perf_stream_ops->read with error normalisation
> - * @stream: An i915 perf stream
> - * @file: An i915 perf stream file
> - * @buf: destination buffer given by userspace
> - * @count: the number of bytes userspace wants to read
> - * @ppos: (inout) file seek position (unused)
> - *
> - * Besides wrapping &i915_perf_stream_ops->read this provides a common place to
> - * ensure that if we've successfully copied any data then reporting that takes
> - * precedence over any internal error status, so the data isn't lost.
> - *
> - * For example ret will be -ENOSPC whenever there is more buffered data than
> - * can be copied to userspace, but that's only interesting if we weren't able
> - * to copy some data because it implies the userspace buffer is too small to
> - * receive a single record (and we never split records).
> - *
> - * Another case with ret == -EFAULT is more of a grey area since it would seem
> - * like bad form for userspace to ask us to overrun its buffer, but the user
> - * knows best:
> - *
> - *   http://yarchive.net/comp/linux/partial_reads_writes.html
> - *
> - * Returns: The number of bytes copied or a negative error code on failure.
> - */
> -static ssize_t i915_perf_read_locked(struct i915_perf_stream *stream,
> -				     struct file *file,
> -				     char __user *buf,
> -				     size_t count,
> -				     loff_t *ppos)
> -{
> -	/* Note we keep the offset (aka bytes read) separate from any
> -	 * error status so that the final check for whether we return
> -	 * the bytes read with a higher precedence than any error (see
> -	 * comment below) doesn't need to be handled/duplicated in
> -	 * stream->ops->read() implementations.
> -	 */
> -	size_t offset = 0;
> -	int ret = stream->ops->read(stream, buf, count, &offset);
> -
> -	return offset ?: (ret ?: -EAGAIN);
> -}
> -
>   /**
>    * i915_perf_read - handles read() FOP for i915 perf stream FDs
>    * @file: An i915 perf stream file
> @@ -3031,7 +2988,8 @@ static ssize_t i915_perf_read(struct file *file,
>   {
>   	struct i915_perf_stream *stream = file->private_data;
>   	struct i915_perf *perf = stream->perf;
> -	ssize_t ret;
> +	size_t offset = 0;
> +	int ret;
>   
>   	/* To ensure it's handled consistently we simply treat all reads of a
>   	 * disabled stream as an error. In particular it might otherwise lead
> @@ -3054,13 +3012,12 @@ static ssize_t i915_perf_read(struct file *file,
>   				return ret;
>   
>   			mutex_lock(&perf->lock);
> -			ret = i915_perf_read_locked(stream, file,
> -						    buf, count, ppos);
> +			ret = stream->ops->read(stream, buf, count, &offset);
>   			mutex_unlock(&perf->lock);
> -		} while (ret == -EAGAIN);
> +		} while (!offset && !ret);

This doesn't sound right, !offset means it will stop as soon as some 
data was written.

But for the blocking read we want to fill the buffer up to -ENOSPC.


while (ret >= 0) doesn't work?


-Lionel

>   	} else {
>   		mutex_lock(&perf->lock);
> -		ret = i915_perf_read_locked(stream, file, buf, count, ppos);
> +		ret = stream->ops->read(stream, buf, count, &offset);
>   		mutex_unlock(&perf->lock);
>   	}
>   
> @@ -3071,11 +3028,15 @@ static ssize_t i915_perf_read(struct file *file,
>   	 * and read() returning -EAGAIN. Clearing the oa.pollin state here
>   	 * effectively ensures we back off until the next hrtimer callback
>   	 * before reporting another EPOLLIN event.
> +	 * The exception to this is if ops->read() returned -ENOSPC which means
> +	 * that more OA data is available than could fit in the user provided
> +	 * buffer. In this case we want the next poll() call to not block.
>   	 */
> -	if (ret >= 0 || ret == -EAGAIN)
> +	if (ret != -ENOSPC)
>   		stream->pollin = false;
>   
> -	return ret;
> +	/* Possible values for ret are 0, -EFAULT, -ENOSPC, -EIO, ... */
> +	return offset ?: (ret ?: -EAGAIN);
>   }
>   
>   static enum hrtimer_restart oa_poll_check_timer_cb(struct hrtimer *hrtimer)


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

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

* Re: [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers
  2020-03-31  7:34 ` Lionel Landwerlin
@ 2020-03-31 23:29   ` Dixit, Ashutosh
  0 siblings, 0 replies; 29+ messages in thread
From: Dixit, Ashutosh @ 2020-03-31 23:29 UTC (permalink / raw)
  To: Lionel Landwerlin; +Cc: intel-gfx

On Tue, 31 Mar 2020 00:34:10 -0700, Lionel Landwerlin wrote:
>
> On 31/03/2020 08:22, Ashutosh Dixit wrote:
> > It is wrong to block the user thread in the next poll when OA data is
> > already available which could not fit in the user buffer provided in
> > the previous read. In several cases the exact user buffer size is not
> > known. Blocking user space in poll can lead to data loss when the
> > buffer size used is smaller than the available data.
> >
> > This change fixes this issue and allows user space to read all OA data
> > even when using a buffer size smaller than the available data using
> > multiple non-blocking reads rather than staying blocked in poll till
> > the next timer interrupt.
> >
> > v2: Fix ret value for blocking reads (Umesh)
> > v3: Mistake during patch send (Ashutosh)
> > v4: Remove -EAGAIN from comment (Umesh)
> > v5: Improve condition for clearing pollin and return (Lionel)
> >
> > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>
> I forgot to mention this needs to be Cc: stable.

I will Cc stable or send them the patch after it's finalized, hope that
will be ok?

>
> Still one nit below which should make the remaining function a bit simpler.
>
> Thanks for your time.
>
> -Lionel
>
>
> > ---
> >   drivers/gpu/drm/i915/i915_perf.c | 62 +++++++-------------------------
> >   1 file changed, 13 insertions(+), 49 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> > index c74ebac50015..9c21f28f89a7 100644
> > --- a/drivers/gpu/drm/i915/i915_perf.c
> > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > @@ -2914,49 +2914,6 @@ void i915_oa_init_reg_state(const struct intel_context *ce,
> >		gen8_update_reg_state_unlocked(ce, stream);
> >   }
> >   -/**
> > - * i915_perf_read_locked - &i915_perf_stream_ops->read with error normalisation
> > - * @stream: An i915 perf stream
> > - * @file: An i915 perf stream file
> > - * @buf: destination buffer given by userspace
> > - * @count: the number of bytes userspace wants to read
> > - * @ppos: (inout) file seek position (unused)
> > - *
> > - * Besides wrapping &i915_perf_stream_ops->read this provides a common place to
> > - * ensure that if we've successfully copied any data then reporting that takes
> > - * precedence over any internal error status, so the data isn't lost.
> > - *
> > - * For example ret will be -ENOSPC whenever there is more buffered data than
> > - * can be copied to userspace, but that's only interesting if we weren't able
> > - * to copy some data because it implies the userspace buffer is too small to
> > - * receive a single record (and we never split records).
> > - *
> > - * Another case with ret == -EFAULT is more of a grey area since it would seem
> > - * like bad form for userspace to ask us to overrun its buffer, but the user
> > - * knows best:
> > - *
> > - *   http://yarchive.net/comp/linux/partial_reads_writes.html
> > - *
> > - * Returns: The number of bytes copied or a negative error code on failure.
> > - */
> > -static ssize_t i915_perf_read_locked(struct i915_perf_stream *stream,
> > -				     struct file *file,
> > -				     char __user *buf,
> > -				     size_t count,
> > -				     loff_t *ppos)
> > -{
> > -	/* Note we keep the offset (aka bytes read) separate from any
> > -	 * error status so that the final check for whether we return
> > -	 * the bytes read with a higher precedence than any error (see
> > -	 * comment below) doesn't need to be handled/duplicated in
> > -	 * stream->ops->read() implementations.
> > -	 */
> > -	size_t offset = 0;
> > -	int ret = stream->ops->read(stream, buf, count, &offset);
> > -
> > -	return offset ?: (ret ?: -EAGAIN);
> > -}
> > -
> >   /**
> >    * i915_perf_read - handles read() FOP for i915 perf stream FDs
> >    * @file: An i915 perf stream file
> > @@ -2982,7 +2939,8 @@ static ssize_t i915_perf_read(struct file *file,
> >   {
> >	struct i915_perf_stream *stream = file->private_data;
> >	struct i915_perf *perf = stream->perf;
> > -	ssize_t ret;
> > +	size_t offset = 0;
> > +	int __ret;
> >		/* To ensure it's handled consistently we simply treat all reads of
> > a
> >	 * disabled stream as an error. In particular it might otherwise lead
> > @@ -2992,6 +2950,8 @@ static ssize_t i915_perf_read(struct file *file,
> >		return -EIO;
> >		if (!(file->f_flags & O_NONBLOCK)) {
> > +		ssize_t ret;
> > +
> >		/* There's the small chance of false positives from
> >		 * stream->ops->wait_unlocked.
> >		 *
> > @@ -3005,13 +2965,13 @@ static ssize_t i915_perf_read(struct file *file,
> >				return ret;
> >				mutex_lock(&perf->lock);
> > -			ret = i915_perf_read_locked(stream, file,
> > -						    buf, count, ppos);
> > +			__ret = stream->ops->read(stream, buf, count, &offset);
>
>
> I think you can keep using ret and just change the loop to while (ret >= 0)
> (which means no failure).
>
> You will get -ENOSPC when the whole buffer is filled or some other error
> which should trigger stream closure.

Sorry, but I did not follow you here. Are you saying even when we have data
to return (offset > 0) you want to go back and block (in the
wait_unlocked())? I am not sure if that will be acceptable, I'd think the
expectation would be to return data at the rate of the OA timer.

> Finally you can 0 if nothing was written but there was nothing to read and
> that keeps the read going.

Here I really lost you. Are you talking about -ENOSPC returning after
blocking multiple times and then resetting the error to 0 somehow?

Could you please look at v6 and let me know what you think? I have
eliminated the second ret variable by changing the do-while loop to "do { }
while (!offset && !ret);" and thus mostly retain the original logic.

Thanks!
--
Ashutosh

>
>
> > +			ret = offset ?: (__ret ?: -EAGAIN);
> >			mutex_unlock(&perf->lock);
> >		} while (ret == -EAGAIN);
> >	} else {
> >		mutex_lock(&perf->lock);
> > -		ret = i915_perf_read_locked(stream, file, buf, count, ppos);
> > +		__ret = stream->ops->read(stream, buf, count, &offset);
> >		mutex_unlock(&perf->lock);
> >	}
> >   @@ -3022,11 +2982,15 @@ static ssize_t i915_perf_read(struct file
> > *file,
> >	 * and read() returning -EAGAIN. Clearing the oa.pollin state here
> >	 * effectively ensures we back off until the next hrtimer callback
> >	 * before reporting another EPOLLIN event.
> > +	 * The exception to this is if ops->read() returned -ENOSPC which means
> > +	 * that more OA data is available than could fit in the user provided
> > +	 * buffer. In this case we want the next poll() call to not block.
> >	 */
> > -	if (ret >= 0 || ret == -EAGAIN)
> > +	if (__ret != -ENOSPC)
> >		stream->pollin = false;
> >   -	return ret;
> > +	/* Possible values for __ret are 0, -EFAULT, -ENOSPC, -EIO, ... */
> > +	return offset ?: (__ret ?: -EAGAIN);
> >   }
> >     static enum hrtimer_restart oa_poll_check_timer_cb(struct hrtimer
> > *hrtimer)
>
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers
@ 2020-03-31 23:14 Ashutosh Dixit
  2020-04-01  6:57 ` Lionel Landwerlin
  0 siblings, 1 reply; 29+ messages in thread
From: Ashutosh Dixit @ 2020-03-31 23:14 UTC (permalink / raw)
  To: intel-gfx

It is wrong to block the user thread in the next poll when OA data is
already available which could not fit in the user buffer provided in
the previous read. In several cases the exact user buffer size is not
known. Blocking user space in poll can lead to data loss when the
buffer size used is smaller than the available data.

This change fixes this issue and allows user space to read all OA data
even when using a buffer size smaller than the available data using
multiple non-blocking reads rather than staying blocked in poll till
the next timer interrupt.

v2: Fix ret value for blocking reads (Umesh)
v3: Mistake during patch send (Ashutosh)
v4: Remove -EAGAIN from comment (Umesh)
v5: Improve condition for clearing pollin and return (Lionel)
v6: Improve blocking read loop and other cleanups (Lionel)

Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
 drivers/gpu/drm/i915/i915_perf.c | 61 ++++++--------------------------
 1 file changed, 11 insertions(+), 50 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 28e3d76fa2e6..2f78b147bb2d 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -2963,49 +2963,6 @@ void i915_oa_init_reg_state(const struct intel_context *ce,
 		gen8_update_reg_state_unlocked(ce, stream);
 }
 
-/**
- * i915_perf_read_locked - &i915_perf_stream_ops->read with error normalisation
- * @stream: An i915 perf stream
- * @file: An i915 perf stream file
- * @buf: destination buffer given by userspace
- * @count: the number of bytes userspace wants to read
- * @ppos: (inout) file seek position (unused)
- *
- * Besides wrapping &i915_perf_stream_ops->read this provides a common place to
- * ensure that if we've successfully copied any data then reporting that takes
- * precedence over any internal error status, so the data isn't lost.
- *
- * For example ret will be -ENOSPC whenever there is more buffered data than
- * can be copied to userspace, but that's only interesting if we weren't able
- * to copy some data because it implies the userspace buffer is too small to
- * receive a single record (and we never split records).
- *
- * Another case with ret == -EFAULT is more of a grey area since it would seem
- * like bad form for userspace to ask us to overrun its buffer, but the user
- * knows best:
- *
- *   http://yarchive.net/comp/linux/partial_reads_writes.html
- *
- * Returns: The number of bytes copied or a negative error code on failure.
- */
-static ssize_t i915_perf_read_locked(struct i915_perf_stream *stream,
-				     struct file *file,
-				     char __user *buf,
-				     size_t count,
-				     loff_t *ppos)
-{
-	/* Note we keep the offset (aka bytes read) separate from any
-	 * error status so that the final check for whether we return
-	 * the bytes read with a higher precedence than any error (see
-	 * comment below) doesn't need to be handled/duplicated in
-	 * stream->ops->read() implementations.
-	 */
-	size_t offset = 0;
-	int ret = stream->ops->read(stream, buf, count, &offset);
-
-	return offset ?: (ret ?: -EAGAIN);
-}
-
 /**
  * i915_perf_read - handles read() FOP for i915 perf stream FDs
  * @file: An i915 perf stream file
@@ -3031,7 +2988,8 @@ static ssize_t i915_perf_read(struct file *file,
 {
 	struct i915_perf_stream *stream = file->private_data;
 	struct i915_perf *perf = stream->perf;
-	ssize_t ret;
+	size_t offset = 0;
+	int ret;
 
 	/* To ensure it's handled consistently we simply treat all reads of a
 	 * disabled stream as an error. In particular it might otherwise lead
@@ -3054,13 +3012,12 @@ static ssize_t i915_perf_read(struct file *file,
 				return ret;
 
 			mutex_lock(&perf->lock);
-			ret = i915_perf_read_locked(stream, file,
-						    buf, count, ppos);
+			ret = stream->ops->read(stream, buf, count, &offset);
 			mutex_unlock(&perf->lock);
-		} while (ret == -EAGAIN);
+		} while (!offset && !ret);
 	} else {
 		mutex_lock(&perf->lock);
-		ret = i915_perf_read_locked(stream, file, buf, count, ppos);
+		ret = stream->ops->read(stream, buf, count, &offset);
 		mutex_unlock(&perf->lock);
 	}
 
@@ -3071,11 +3028,15 @@ static ssize_t i915_perf_read(struct file *file,
 	 * and read() returning -EAGAIN. Clearing the oa.pollin state here
 	 * effectively ensures we back off until the next hrtimer callback
 	 * before reporting another EPOLLIN event.
+	 * The exception to this is if ops->read() returned -ENOSPC which means
+	 * that more OA data is available than could fit in the user provided
+	 * buffer. In this case we want the next poll() call to not block.
 	 */
-	if (ret >= 0 || ret == -EAGAIN)
+	if (ret != -ENOSPC)
 		stream->pollin = false;
 
-	return ret;
+	/* Possible values for ret are 0, -EFAULT, -ENOSPC, -EIO, ... */
+	return offset ?: (ret ?: -EAGAIN);
 }
 
 static enum hrtimer_restart oa_poll_check_timer_cb(struct hrtimer *hrtimer)
-- 
2.25.2

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

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

* Re: [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers
  2020-03-31  5:22 Ashutosh Dixit
@ 2020-03-31  7:34 ` Lionel Landwerlin
  2020-03-31 23:29   ` Dixit, Ashutosh
  0 siblings, 1 reply; 29+ messages in thread
From: Lionel Landwerlin @ 2020-03-31  7:34 UTC (permalink / raw)
  To: Ashutosh Dixit, intel-gfx

On 31/03/2020 08:22, Ashutosh Dixit wrote:
> It is wrong to block the user thread in the next poll when OA data is
> already available which could not fit in the user buffer provided in
> the previous read. In several cases the exact user buffer size is not
> known. Blocking user space in poll can lead to data loss when the
> buffer size used is smaller than the available data.
>
> This change fixes this issue and allows user space to read all OA data
> even when using a buffer size smaller than the available data using
> multiple non-blocking reads rather than staying blocked in poll till
> the next timer interrupt.
>
> v2: Fix ret value for blocking reads (Umesh)
> v3: Mistake during patch send (Ashutosh)
> v4: Remove -EAGAIN from comment (Umesh)
> v5: Improve condition for clearing pollin and return (Lionel)
>
> Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>


I forgot to mention this needs to be Cc: stable.

Still one nit below which should make the remaining function a bit simpler.


Thanks for your time.


-Lionel


> ---
>   drivers/gpu/drm/i915/i915_perf.c | 62 +++++++-------------------------
>   1 file changed, 13 insertions(+), 49 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index c74ebac50015..9c21f28f89a7 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -2914,49 +2914,6 @@ void i915_oa_init_reg_state(const struct intel_context *ce,
>   		gen8_update_reg_state_unlocked(ce, stream);
>   }
>   
> -/**
> - * i915_perf_read_locked - &i915_perf_stream_ops->read with error normalisation
> - * @stream: An i915 perf stream
> - * @file: An i915 perf stream file
> - * @buf: destination buffer given by userspace
> - * @count: the number of bytes userspace wants to read
> - * @ppos: (inout) file seek position (unused)
> - *
> - * Besides wrapping &i915_perf_stream_ops->read this provides a common place to
> - * ensure that if we've successfully copied any data then reporting that takes
> - * precedence over any internal error status, so the data isn't lost.
> - *
> - * For example ret will be -ENOSPC whenever there is more buffered data than
> - * can be copied to userspace, but that's only interesting if we weren't able
> - * to copy some data because it implies the userspace buffer is too small to
> - * receive a single record (and we never split records).
> - *
> - * Another case with ret == -EFAULT is more of a grey area since it would seem
> - * like bad form for userspace to ask us to overrun its buffer, but the user
> - * knows best:
> - *
> - *   http://yarchive.net/comp/linux/partial_reads_writes.html
> - *
> - * Returns: The number of bytes copied or a negative error code on failure.
> - */
> -static ssize_t i915_perf_read_locked(struct i915_perf_stream *stream,
> -				     struct file *file,
> -				     char __user *buf,
> -				     size_t count,
> -				     loff_t *ppos)
> -{
> -	/* Note we keep the offset (aka bytes read) separate from any
> -	 * error status so that the final check for whether we return
> -	 * the bytes read with a higher precedence than any error (see
> -	 * comment below) doesn't need to be handled/duplicated in
> -	 * stream->ops->read() implementations.
> -	 */
> -	size_t offset = 0;
> -	int ret = stream->ops->read(stream, buf, count, &offset);
> -
> -	return offset ?: (ret ?: -EAGAIN);
> -}
> -
>   /**
>    * i915_perf_read - handles read() FOP for i915 perf stream FDs
>    * @file: An i915 perf stream file
> @@ -2982,7 +2939,8 @@ static ssize_t i915_perf_read(struct file *file,
>   {
>   	struct i915_perf_stream *stream = file->private_data;
>   	struct i915_perf *perf = stream->perf;
> -	ssize_t ret;
> +	size_t offset = 0;
> +	int __ret;
>   
>   	/* To ensure it's handled consistently we simply treat all reads of a
>   	 * disabled stream as an error. In particular it might otherwise lead
> @@ -2992,6 +2950,8 @@ static ssize_t i915_perf_read(struct file *file,
>   		return -EIO;
>   
>   	if (!(file->f_flags & O_NONBLOCK)) {
> +		ssize_t ret;
> +
>   		/* There's the small chance of false positives from
>   		 * stream->ops->wait_unlocked.
>   		 *
> @@ -3005,13 +2965,13 @@ static ssize_t i915_perf_read(struct file *file,
>   				return ret;
>   
>   			mutex_lock(&perf->lock);
> -			ret = i915_perf_read_locked(stream, file,
> -						    buf, count, ppos);
> +			__ret = stream->ops->read(stream, buf, count, &offset);


I think you can keep using ret and just change the loop to while (ret >= 
0) (which means no failure).

You will get -ENOSPC when the whole buffer is filled or some other error 
which should trigger stream closure.

Finally you can 0 if nothing was written but there was nothing to read 
and that keeps the read going.


> +			ret = offset ?: (__ret ?: -EAGAIN);
>   			mutex_unlock(&perf->lock);
>   		} while (ret == -EAGAIN);
>   	} else {
>   		mutex_lock(&perf->lock);
> -		ret = i915_perf_read_locked(stream, file, buf, count, ppos);
> +		__ret = stream->ops->read(stream, buf, count, &offset);
>   		mutex_unlock(&perf->lock);
>   	}
>   
> @@ -3022,11 +2982,15 @@ static ssize_t i915_perf_read(struct file *file,
>   	 * and read() returning -EAGAIN. Clearing the oa.pollin state here
>   	 * effectively ensures we back off until the next hrtimer callback
>   	 * before reporting another EPOLLIN event.
> +	 * The exception to this is if ops->read() returned -ENOSPC which means
> +	 * that more OA data is available than could fit in the user provided
> +	 * buffer. In this case we want the next poll() call to not block.
>   	 */
> -	if (ret >= 0 || ret == -EAGAIN)
> +	if (__ret != -ENOSPC)
>   		stream->pollin = false;
>   
> -	return ret;
> +	/* Possible values for __ret are 0, -EFAULT, -ENOSPC, -EIO, ... */
> +	return offset ?: (__ret ?: -EAGAIN);
>   }
>   
>   static enum hrtimer_restart oa_poll_check_timer_cb(struct hrtimer *hrtimer)


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

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

* Re: [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers
  2020-03-30  8:23 ` Lionel Landwerlin
@ 2020-03-31  5:47   ` Dixit, Ashutosh
  0 siblings, 0 replies; 29+ messages in thread
From: Dixit, Ashutosh @ 2020-03-31  5:47 UTC (permalink / raw)
  To: Lionel Landwerlin; +Cc: intel-gfx

On Mon, 30 Mar 2020 01:23:29 -0700, Lionel Landwerlin wrote:
>
> On 28/03/2020 01:16, Ashutosh Dixit wrote:
> > It is wrong to block the user thread in the next poll when OA data is
> > already available which could not fit in the user buffer provided in
> > the previous read. In several cases the exact user buffer size is not
> > known. Blocking user space in poll can lead to data loss when the
> > buffer size used is smaller than the available data.
> >
> > This change fixes this issue and allows user space to read all OA data
> > even when using a buffer size smaller than the available data using
> > multiple non-blocking reads rather than staying blocked in poll till
> > the next timer interrupt.
> >
> > v2: Fix ret value for blocking reads (Umesh)
> > v3: Mistake during patch send (Ashutosh)
> > v4: Remove -EAGAIN from comment (Umesh)
> >
> > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>
> Looks like you change makes more sense than what I suggested.
>
> I have a few nits below.

Thanks Lionel, most of what you suggested made sense so I have made those
changes and submitted a v5. Please take a look. More comments below.

>
> Thanks,
>
> -Lionel
>
>
> > ---
> >   drivers/gpu/drm/i915/i915_perf.c | 59 +++++++-------------------------
> >   1 file changed, 12 insertions(+), 47 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> > index c74ebac50015..5f6d9bff99c8 100644
> > --- a/drivers/gpu/drm/i915/i915_perf.c
> > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > @@ -2914,49 +2914,6 @@ void i915_oa_init_reg_state(const struct intel_context *ce,
> >		gen8_update_reg_state_unlocked(ce, stream);
> >   }
> >   -/**
> > - * i915_perf_read_locked - &i915_perf_stream_ops->read with error normalisation
> > - * @stream: An i915 perf stream
> > - * @file: An i915 perf stream file
> > - * @buf: destination buffer given by userspace
> > - * @count: the number of bytes userspace wants to read
> > - * @ppos: (inout) file seek position (unused)
> > - *
> > - * Besides wrapping &i915_perf_stream_ops->read this provides a common place to
> > - * ensure that if we've successfully copied any data then reporting that takes
> > - * precedence over any internal error status, so the data isn't lost.
> > - *
> > - * For example ret will be -ENOSPC whenever there is more buffered data than
> > - * can be copied to userspace, but that's only interesting if we weren't able
> > - * to copy some data because it implies the userspace buffer is too small to
> > - * receive a single record (and we never split records).
> > - *
> > - * Another case with ret == -EFAULT is more of a grey area since it would seem
> > - * like bad form for userspace to ask us to overrun its buffer, but the user
> > - * knows best:
> > - *
> > - *   http://yarchive.net/comp/linux/partial_reads_writes.html
> > - *
> > - * Returns: The number of bytes copied or a negative error code on failure.
> > - */
> > -static ssize_t i915_perf_read_locked(struct i915_perf_stream *stream,
> > -				     struct file *file,
> > -				     char __user *buf,
> > -				     size_t count,
> > -				     loff_t *ppos)
> > -{
> > -	/* Note we keep the offset (aka bytes read) separate from any
> > -	 * error status so that the final check for whether we return
> > -	 * the bytes read with a higher precedence than any error (see
> > -	 * comment below) doesn't need to be handled/duplicated in
> > -	 * stream->ops->read() implementations.
> > -	 */
> > -	size_t offset = 0;
> > -	int ret = stream->ops->read(stream, buf, count, &offset);
> > -
> > -	return offset ?: (ret ?: -EAGAIN);
> > -}
> > -
> >   /**
> >    * i915_perf_read - handles read() FOP for i915 perf stream FDs
> >    * @file: An i915 perf stream file
> > @@ -2982,6 +2939,8 @@ static ssize_t i915_perf_read(struct file *file,
> >   {
> >	struct i915_perf_stream *stream = file->private_data;
> >	struct i915_perf *perf = stream->perf;
> > +	size_t offset = 0;
> > +	int __ret;
> >	ssize_t ret;
> >		/* To ensure it's handled consistently we simply treat all reads of
> > a
> > @@ -3005,16 +2964,19 @@ static ssize_t i915_perf_read(struct file *file,
> >				return ret;
> >				mutex_lock(&perf->lock);
> > -			ret = i915_perf_read_locked(stream, file,
> > -						    buf, count, ppos);
> > +			__ret = stream->ops->read(stream, buf, count, &offset);
> > +			ret = offset ?: (__ret ?: -EAGAIN);

> I would drop this line above and move it to the end of the function.

Unfortunately, Umesh pointed this out, that can't be done because ret is
used in the loop (do { } while (ret == -EAGAIN); ).

> >			mutex_unlock(&perf->lock);
> >		} while (ret == -EAGAIN);
> >	} else {
> >		mutex_lock(&perf->lock);
> > -		ret = i915_perf_read_locked(stream, file, buf, count, ppos);
> > +		__ret = stream->ops->read(stream, buf, count, &offset);
> > +		ret = offset ?: (__ret ?: -EAGAIN);

> I would drop this line above and move it to the end of the function.

Done.

> >		mutex_unlock(&perf->lock);
> >	}
> >   +	/* Possible values for __ret are 0, -EFAULT, -ENOSPC, -EIO, ... */
> > +
> >	/* We allow the poll checking to sometimes report false positive EPOLLIN
> >	 * events where we might actually report EAGAIN on read() if there's
> >	 * not really any data available. In this situation though we don't
> > @@ -3022,8 +2984,11 @@ static ssize_t i915_perf_read(struct file *file,
> >	 * and read() returning -EAGAIN. Clearing the oa.pollin state here
> >	 * effectively ensures we back off until the next hrtimer callback
> >	 * before reporting another EPOLLIN event.
> > +	 * The exception to this is if ops->read() returned -ENOSPC which means
> > +	 * that more OA data is available than could fit in the user provided
> > +	 * buffer. In this case we want the next poll() call to not block.
> >	 */
> > -	if (ret >= 0 || ret == -EAGAIN)
> > +	if ((ret > 0 || ret == -EAGAIN) && __ret != -ENOSPC)
>
> I guess this could be simplified with
>
> if (__ret != -ENOSPC)
>
> As far as I can see in all other cases (failure of some kind, all data
> read), we should clear pollin.

Done.

>
> >		stream->pollin = false;
> >		return ret;
>
> return offset ?: (__ret ?: -EAGAIN);
>
> You could probably just retain a single ret variable.

Done, mostly. There is a second ret in a restricted scope due to the reason
mentioned above (ret in the loop).
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers
@ 2020-03-31  5:22 Ashutosh Dixit
  2020-03-31  7:34 ` Lionel Landwerlin
  0 siblings, 1 reply; 29+ messages in thread
From: Ashutosh Dixit @ 2020-03-31  5:22 UTC (permalink / raw)
  To: intel-gfx

It is wrong to block the user thread in the next poll when OA data is
already available which could not fit in the user buffer provided in
the previous read. In several cases the exact user buffer size is not
known. Blocking user space in poll can lead to data loss when the
buffer size used is smaller than the available data.

This change fixes this issue and allows user space to read all OA data
even when using a buffer size smaller than the available data using
multiple non-blocking reads rather than staying blocked in poll till
the next timer interrupt.

v2: Fix ret value for blocking reads (Umesh)
v3: Mistake during patch send (Ashutosh)
v4: Remove -EAGAIN from comment (Umesh)
v5: Improve condition for clearing pollin and return (Lionel)

Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
 drivers/gpu/drm/i915/i915_perf.c | 62 +++++++-------------------------
 1 file changed, 13 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index c74ebac50015..9c21f28f89a7 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -2914,49 +2914,6 @@ void i915_oa_init_reg_state(const struct intel_context *ce,
 		gen8_update_reg_state_unlocked(ce, stream);
 }
 
-/**
- * i915_perf_read_locked - &i915_perf_stream_ops->read with error normalisation
- * @stream: An i915 perf stream
- * @file: An i915 perf stream file
- * @buf: destination buffer given by userspace
- * @count: the number of bytes userspace wants to read
- * @ppos: (inout) file seek position (unused)
- *
- * Besides wrapping &i915_perf_stream_ops->read this provides a common place to
- * ensure that if we've successfully copied any data then reporting that takes
- * precedence over any internal error status, so the data isn't lost.
- *
- * For example ret will be -ENOSPC whenever there is more buffered data than
- * can be copied to userspace, but that's only interesting if we weren't able
- * to copy some data because it implies the userspace buffer is too small to
- * receive a single record (and we never split records).
- *
- * Another case with ret == -EFAULT is more of a grey area since it would seem
- * like bad form for userspace to ask us to overrun its buffer, but the user
- * knows best:
- *
- *   http://yarchive.net/comp/linux/partial_reads_writes.html
- *
- * Returns: The number of bytes copied or a negative error code on failure.
- */
-static ssize_t i915_perf_read_locked(struct i915_perf_stream *stream,
-				     struct file *file,
-				     char __user *buf,
-				     size_t count,
-				     loff_t *ppos)
-{
-	/* Note we keep the offset (aka bytes read) separate from any
-	 * error status so that the final check for whether we return
-	 * the bytes read with a higher precedence than any error (see
-	 * comment below) doesn't need to be handled/duplicated in
-	 * stream->ops->read() implementations.
-	 */
-	size_t offset = 0;
-	int ret = stream->ops->read(stream, buf, count, &offset);
-
-	return offset ?: (ret ?: -EAGAIN);
-}
-
 /**
  * i915_perf_read - handles read() FOP for i915 perf stream FDs
  * @file: An i915 perf stream file
@@ -2982,7 +2939,8 @@ static ssize_t i915_perf_read(struct file *file,
 {
 	struct i915_perf_stream *stream = file->private_data;
 	struct i915_perf *perf = stream->perf;
-	ssize_t ret;
+	size_t offset = 0;
+	int __ret;
 
 	/* To ensure it's handled consistently we simply treat all reads of a
 	 * disabled stream as an error. In particular it might otherwise lead
@@ -2992,6 +2950,8 @@ static ssize_t i915_perf_read(struct file *file,
 		return -EIO;
 
 	if (!(file->f_flags & O_NONBLOCK)) {
+		ssize_t ret;
+
 		/* There's the small chance of false positives from
 		 * stream->ops->wait_unlocked.
 		 *
@@ -3005,13 +2965,13 @@ static ssize_t i915_perf_read(struct file *file,
 				return ret;
 
 			mutex_lock(&perf->lock);
-			ret = i915_perf_read_locked(stream, file,
-						    buf, count, ppos);
+			__ret = stream->ops->read(stream, buf, count, &offset);
+			ret = offset ?: (__ret ?: -EAGAIN);
 			mutex_unlock(&perf->lock);
 		} while (ret == -EAGAIN);
 	} else {
 		mutex_lock(&perf->lock);
-		ret = i915_perf_read_locked(stream, file, buf, count, ppos);
+		__ret = stream->ops->read(stream, buf, count, &offset);
 		mutex_unlock(&perf->lock);
 	}
 
@@ -3022,11 +2982,15 @@ static ssize_t i915_perf_read(struct file *file,
 	 * and read() returning -EAGAIN. Clearing the oa.pollin state here
 	 * effectively ensures we back off until the next hrtimer callback
 	 * before reporting another EPOLLIN event.
+	 * The exception to this is if ops->read() returned -ENOSPC which means
+	 * that more OA data is available than could fit in the user provided
+	 * buffer. In this case we want the next poll() call to not block.
 	 */
-	if (ret >= 0 || ret == -EAGAIN)
+	if (__ret != -ENOSPC)
 		stream->pollin = false;
 
-	return ret;
+	/* Possible values for __ret are 0, -EFAULT, -ENOSPC, -EIO, ... */
+	return offset ?: (__ret ?: -EAGAIN);
 }
 
 static enum hrtimer_restart oa_poll_check_timer_cb(struct hrtimer *hrtimer)
-- 
2.25.2

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

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

* Re: [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers
  2020-03-27 23:16 Ashutosh Dixit
@ 2020-03-30  8:23 ` Lionel Landwerlin
  2020-03-31  5:47   ` Dixit, Ashutosh
  0 siblings, 1 reply; 29+ messages in thread
From: Lionel Landwerlin @ 2020-03-30  8:23 UTC (permalink / raw)
  To: Ashutosh Dixit, intel-gfx

On 28/03/2020 01:16, Ashutosh Dixit wrote:
> It is wrong to block the user thread in the next poll when OA data is
> already available which could not fit in the user buffer provided in
> the previous read. In several cases the exact user buffer size is not
> known. Blocking user space in poll can lead to data loss when the
> buffer size used is smaller than the available data.
>
> This change fixes this issue and allows user space to read all OA data
> even when using a buffer size smaller than the available data using
> multiple non-blocking reads rather than staying blocked in poll till
> the next timer interrupt.
>
> v2: Fix ret value for blocking reads (Umesh)
> v3: Mistake during patch send (Ashutosh)
> v4: Remove -EAGAIN from comment (Umesh)
>
> Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>


Looks like you change makes more sense than what I suggested.

I have a few nits below.


Thanks,


-Lionel


> ---
>   drivers/gpu/drm/i915/i915_perf.c | 59 +++++++-------------------------
>   1 file changed, 12 insertions(+), 47 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index c74ebac50015..5f6d9bff99c8 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -2914,49 +2914,6 @@ void i915_oa_init_reg_state(const struct intel_context *ce,
>   		gen8_update_reg_state_unlocked(ce, stream);
>   }
>   
> -/**
> - * i915_perf_read_locked - &i915_perf_stream_ops->read with error normalisation
> - * @stream: An i915 perf stream
> - * @file: An i915 perf stream file
> - * @buf: destination buffer given by userspace
> - * @count: the number of bytes userspace wants to read
> - * @ppos: (inout) file seek position (unused)
> - *
> - * Besides wrapping &i915_perf_stream_ops->read this provides a common place to
> - * ensure that if we've successfully copied any data then reporting that takes
> - * precedence over any internal error status, so the data isn't lost.
> - *
> - * For example ret will be -ENOSPC whenever there is more buffered data than
> - * can be copied to userspace, but that's only interesting if we weren't able
> - * to copy some data because it implies the userspace buffer is too small to
> - * receive a single record (and we never split records).
> - *
> - * Another case with ret == -EFAULT is more of a grey area since it would seem
> - * like bad form for userspace to ask us to overrun its buffer, but the user
> - * knows best:
> - *
> - *   http://yarchive.net/comp/linux/partial_reads_writes.html
> - *
> - * Returns: The number of bytes copied or a negative error code on failure.
> - */
> -static ssize_t i915_perf_read_locked(struct i915_perf_stream *stream,
> -				     struct file *file,
> -				     char __user *buf,
> -				     size_t count,
> -				     loff_t *ppos)
> -{
> -	/* Note we keep the offset (aka bytes read) separate from any
> -	 * error status so that the final check for whether we return
> -	 * the bytes read with a higher precedence than any error (see
> -	 * comment below) doesn't need to be handled/duplicated in
> -	 * stream->ops->read() implementations.
> -	 */
> -	size_t offset = 0;
> -	int ret = stream->ops->read(stream, buf, count, &offset);
> -
> -	return offset ?: (ret ?: -EAGAIN);
> -}
> -
>   /**
>    * i915_perf_read - handles read() FOP for i915 perf stream FDs
>    * @file: An i915 perf stream file
> @@ -2982,6 +2939,8 @@ static ssize_t i915_perf_read(struct file *file,
>   {
>   	struct i915_perf_stream *stream = file->private_data;
>   	struct i915_perf *perf = stream->perf;
> +	size_t offset = 0;
> +	int __ret;
>   	ssize_t ret;
>   
>   	/* To ensure it's handled consistently we simply treat all reads of a
> @@ -3005,16 +2964,19 @@ static ssize_t i915_perf_read(struct file *file,
>   				return ret;
>   
>   			mutex_lock(&perf->lock);
> -			ret = i915_perf_read_locked(stream, file,
> -						    buf, count, ppos);
> +			__ret = stream->ops->read(stream, buf, count, &offset);
> +			ret = offset ?: (__ret ?: -EAGAIN);
I would drop this line above and move it to the end of the function.
>   			mutex_unlock(&perf->lock);
>   		} while (ret == -EAGAIN);
>   	} else {
>   		mutex_lock(&perf->lock);
> -		ret = i915_perf_read_locked(stream, file, buf, count, ppos);
> +		__ret = stream->ops->read(stream, buf, count, &offset);
> +		ret = offset ?: (__ret ?: -EAGAIN);
I would drop this line above and move it to the end of the function.
>   		mutex_unlock(&perf->lock);
>   	}
>   
> +	/* Possible values for __ret are 0, -EFAULT, -ENOSPC, -EIO, ... */
> +
>   	/* We allow the poll checking to sometimes report false positive EPOLLIN
>   	 * events where we might actually report EAGAIN on read() if there's
>   	 * not really any data available. In this situation though we don't
> @@ -3022,8 +2984,11 @@ static ssize_t i915_perf_read(struct file *file,
>   	 * and read() returning -EAGAIN. Clearing the oa.pollin state here
>   	 * effectively ensures we back off until the next hrtimer callback
>   	 * before reporting another EPOLLIN event.
> +	 * The exception to this is if ops->read() returned -ENOSPC which means
> +	 * that more OA data is available than could fit in the user provided
> +	 * buffer. In this case we want the next poll() call to not block.
>   	 */
> -	if (ret >= 0 || ret == -EAGAIN)
> +	if ((ret > 0 || ret == -EAGAIN) && __ret != -ENOSPC)

I guess this could be simplified with

if (__ret != -ENOSPC)


As far as I can see in all other cases (failure of some kind, all data 
read), we should clear pollin.

>   		stream->pollin = false;
>   
>   	return ret;

return offset ?: (__ret ?: -EAGAIN);


You could probably just retain a single ret variable.

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

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

* [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers
@ 2020-03-27 23:16 Ashutosh Dixit
  2020-03-30  8:23 ` Lionel Landwerlin
  0 siblings, 1 reply; 29+ messages in thread
From: Ashutosh Dixit @ 2020-03-27 23:16 UTC (permalink / raw)
  To: intel-gfx

It is wrong to block the user thread in the next poll when OA data is
already available which could not fit in the user buffer provided in
the previous read. In several cases the exact user buffer size is not
known. Blocking user space in poll can lead to data loss when the
buffer size used is smaller than the available data.

This change fixes this issue and allows user space to read all OA data
even when using a buffer size smaller than the available data using
multiple non-blocking reads rather than staying blocked in poll till
the next timer interrupt.

v2: Fix ret value for blocking reads (Umesh)
v3: Mistake during patch send (Ashutosh)
v4: Remove -EAGAIN from comment (Umesh)

Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
 drivers/gpu/drm/i915/i915_perf.c | 59 +++++++-------------------------
 1 file changed, 12 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index c74ebac50015..5f6d9bff99c8 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -2914,49 +2914,6 @@ void i915_oa_init_reg_state(const struct intel_context *ce,
 		gen8_update_reg_state_unlocked(ce, stream);
 }
 
-/**
- * i915_perf_read_locked - &i915_perf_stream_ops->read with error normalisation
- * @stream: An i915 perf stream
- * @file: An i915 perf stream file
- * @buf: destination buffer given by userspace
- * @count: the number of bytes userspace wants to read
- * @ppos: (inout) file seek position (unused)
- *
- * Besides wrapping &i915_perf_stream_ops->read this provides a common place to
- * ensure that if we've successfully copied any data then reporting that takes
- * precedence over any internal error status, so the data isn't lost.
- *
- * For example ret will be -ENOSPC whenever there is more buffered data than
- * can be copied to userspace, but that's only interesting if we weren't able
- * to copy some data because it implies the userspace buffer is too small to
- * receive a single record (and we never split records).
- *
- * Another case with ret == -EFAULT is more of a grey area since it would seem
- * like bad form for userspace to ask us to overrun its buffer, but the user
- * knows best:
- *
- *   http://yarchive.net/comp/linux/partial_reads_writes.html
- *
- * Returns: The number of bytes copied or a negative error code on failure.
- */
-static ssize_t i915_perf_read_locked(struct i915_perf_stream *stream,
-				     struct file *file,
-				     char __user *buf,
-				     size_t count,
-				     loff_t *ppos)
-{
-	/* Note we keep the offset (aka bytes read) separate from any
-	 * error status so that the final check for whether we return
-	 * the bytes read with a higher precedence than any error (see
-	 * comment below) doesn't need to be handled/duplicated in
-	 * stream->ops->read() implementations.
-	 */
-	size_t offset = 0;
-	int ret = stream->ops->read(stream, buf, count, &offset);
-
-	return offset ?: (ret ?: -EAGAIN);
-}
-
 /**
  * i915_perf_read - handles read() FOP for i915 perf stream FDs
  * @file: An i915 perf stream file
@@ -2982,6 +2939,8 @@ static ssize_t i915_perf_read(struct file *file,
 {
 	struct i915_perf_stream *stream = file->private_data;
 	struct i915_perf *perf = stream->perf;
+	size_t offset = 0;
+	int __ret;
 	ssize_t ret;
 
 	/* To ensure it's handled consistently we simply treat all reads of a
@@ -3005,16 +2964,19 @@ static ssize_t i915_perf_read(struct file *file,
 				return ret;
 
 			mutex_lock(&perf->lock);
-			ret = i915_perf_read_locked(stream, file,
-						    buf, count, ppos);
+			__ret = stream->ops->read(stream, buf, count, &offset);
+			ret = offset ?: (__ret ?: -EAGAIN);
 			mutex_unlock(&perf->lock);
 		} while (ret == -EAGAIN);
 	} else {
 		mutex_lock(&perf->lock);
-		ret = i915_perf_read_locked(stream, file, buf, count, ppos);
+		__ret = stream->ops->read(stream, buf, count, &offset);
+		ret = offset ?: (__ret ?: -EAGAIN);
 		mutex_unlock(&perf->lock);
 	}
 
+	/* Possible values for __ret are 0, -EFAULT, -ENOSPC, -EIO, ... */
+
 	/* We allow the poll checking to sometimes report false positive EPOLLIN
 	 * events where we might actually report EAGAIN on read() if there's
 	 * not really any data available. In this situation though we don't
@@ -3022,8 +2984,11 @@ static ssize_t i915_perf_read(struct file *file,
 	 * and read() returning -EAGAIN. Clearing the oa.pollin state here
 	 * effectively ensures we back off until the next hrtimer callback
 	 * before reporting another EPOLLIN event.
+	 * The exception to this is if ops->read() returned -ENOSPC which means
+	 * that more OA data is available than could fit in the user provided
+	 * buffer. In this case we want the next poll() call to not block.
 	 */
-	if (ret >= 0 || ret == -EAGAIN)
+	if ((ret > 0 || ret == -EAGAIN) && __ret != -ENOSPC)
 		stream->pollin = false;
 
 	return ret;
-- 
2.25.2

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

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

* [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers
@ 2020-03-27 23:11 Ashutosh Dixit
  0 siblings, 0 replies; 29+ messages in thread
From: Ashutosh Dixit @ 2020-03-27 23:11 UTC (permalink / raw)
  To: intel-gfx

It is wrong to block the user thread in the next poll when OA data is
already available which could not fit in the user buffer provided in
the previous read. In several cases the exact user buffer size is not
known. Blocking user space in poll can lead to data loss when the
buffer size used is smaller than the available data.

This change fixes this issue and allows user space to read all OA data
even when using a buffer size smaller than the available data using
multiple non-blocking reads rather than staying blocked in poll till
the next timer interrupt.

v2: Fix ret value for blocking reads (Umesh)
v2: Remove -EAGAIN from comment (Umesh)

Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
 drivers/gpu/drm/i915/i915_perf.c | 59 +++++++-------------------------
 1 file changed, 12 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index c74ebac50015..5f6d9bff99c8 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -2914,49 +2914,6 @@ void i915_oa_init_reg_state(const struct intel_context *ce,
 		gen8_update_reg_state_unlocked(ce, stream);
 }
 
-/**
- * i915_perf_read_locked - &i915_perf_stream_ops->read with error normalisation
- * @stream: An i915 perf stream
- * @file: An i915 perf stream file
- * @buf: destination buffer given by userspace
- * @count: the number of bytes userspace wants to read
- * @ppos: (inout) file seek position (unused)
- *
- * Besides wrapping &i915_perf_stream_ops->read this provides a common place to
- * ensure that if we've successfully copied any data then reporting that takes
- * precedence over any internal error status, so the data isn't lost.
- *
- * For example ret will be -ENOSPC whenever there is more buffered data than
- * can be copied to userspace, but that's only interesting if we weren't able
- * to copy some data because it implies the userspace buffer is too small to
- * receive a single record (and we never split records).
- *
- * Another case with ret == -EFAULT is more of a grey area since it would seem
- * like bad form for userspace to ask us to overrun its buffer, but the user
- * knows best:
- *
- *   http://yarchive.net/comp/linux/partial_reads_writes.html
- *
- * Returns: The number of bytes copied or a negative error code on failure.
- */
-static ssize_t i915_perf_read_locked(struct i915_perf_stream *stream,
-				     struct file *file,
-				     char __user *buf,
-				     size_t count,
-				     loff_t *ppos)
-{
-	/* Note we keep the offset (aka bytes read) separate from any
-	 * error status so that the final check for whether we return
-	 * the bytes read with a higher precedence than any error (see
-	 * comment below) doesn't need to be handled/duplicated in
-	 * stream->ops->read() implementations.
-	 */
-	size_t offset = 0;
-	int ret = stream->ops->read(stream, buf, count, &offset);
-
-	return offset ?: (ret ?: -EAGAIN);
-}
-
 /**
  * i915_perf_read - handles read() FOP for i915 perf stream FDs
  * @file: An i915 perf stream file
@@ -2982,6 +2939,8 @@ static ssize_t i915_perf_read(struct file *file,
 {
 	struct i915_perf_stream *stream = file->private_data;
 	struct i915_perf *perf = stream->perf;
+	size_t offset = 0;
+	int __ret;
 	ssize_t ret;
 
 	/* To ensure it's handled consistently we simply treat all reads of a
@@ -3005,16 +2964,19 @@ static ssize_t i915_perf_read(struct file *file,
 				return ret;
 
 			mutex_lock(&perf->lock);
-			ret = i915_perf_read_locked(stream, file,
-						    buf, count, ppos);
+			__ret = stream->ops->read(stream, buf, count, &offset);
+			ret = offset ?: (__ret ?: -EAGAIN);
 			mutex_unlock(&perf->lock);
 		} while (ret == -EAGAIN);
 	} else {
 		mutex_lock(&perf->lock);
-		ret = i915_perf_read_locked(stream, file, buf, count, ppos);
+		__ret = stream->ops->read(stream, buf, count, &offset);
+		ret = offset ?: (__ret ?: -EAGAIN);
 		mutex_unlock(&perf->lock);
 	}
 
+	/* Possible values for __ret are 0, -EFAULT, -ENOSPC, -EIO, ... */
+
 	/* We allow the poll checking to sometimes report false positive EPOLLIN
 	 * events where we might actually report EAGAIN on read() if there's
 	 * not really any data available. In this situation though we don't
@@ -3022,8 +2984,11 @@ static ssize_t i915_perf_read(struct file *file,
 	 * and read() returning -EAGAIN. Clearing the oa.pollin state here
 	 * effectively ensures we back off until the next hrtimer callback
 	 * before reporting another EPOLLIN event.
+	 * The exception to this is if ops->read() returned -ENOSPC which means
+	 * that more OA data is available than could fit in the user provided
+	 * buffer. In this case we want the next poll() call to not block.
 	 */
-	if (ret >= 0 || ret == -EAGAIN)
+	if ((ret > 0 || ret == -EAGAIN) && __ret != -ENOSPC)
 		stream->pollin = false;
 
 	return ret;
-- 
2.25.2

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

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

* Re: [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers
  2020-03-26 18:02     ` Umesh Nerlige Ramappa
@ 2020-03-27  1:28       ` Dixit, Ashutosh
  0 siblings, 0 replies; 29+ messages in thread
From: Dixit, Ashutosh @ 2020-03-27  1:28 UTC (permalink / raw)
  To: Umesh Nerlige Ramappa; +Cc: intel-gfx

On Thu, 26 Mar 2020 11:02:46 -0700, Umesh Nerlige Ramappa wrote:
> On Wed, Mar 25, 2020 at 06:52:52PM -0700, Dixit, Ashutosh wrote:
> > On Wed, 25 Mar 2020 17:32:35 -0700, Umesh Nerlige Ramappa wrote:
> >>
> >> On Wed, Mar 25, 2020 at 11:20:19AM -0700, Ashutosh Dixit wrote:
> >> >
> >> > +	/* Possible values for __ret are 0, -EFAULT, -ENOSPC, -EAGAIN, ... */
> >>
> >> __ret may never be EAGAIN either (comment^). I don't see EAGAIN in the read
> >> path.
> >
> > It's here:
> >
> > gen8_append_oa_reports()
> > {
> >
> >        /*
> >         * An invalid tail pointer here means we're still waiting for the poll
> >         * hrtimer callback to give us a pointer
> >         */
> >        if (tail == INVALID_TAIL_PTR)
> >                return -EAGAIN;
> > }
>
> Oh, you are right, EAGAIN is returned here. I was looking for it with the
> poll period patch series applied and these references are removed in that
> series.

No, you are right, since that series is likely to be merged first, it is
better to remove it from this patch.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers
  2020-03-26  1:52   ` Dixit, Ashutosh
@ 2020-03-26 18:02     ` Umesh Nerlige Ramappa
  2020-03-27  1:28       ` Dixit, Ashutosh
  0 siblings, 1 reply; 29+ messages in thread
From: Umesh Nerlige Ramappa @ 2020-03-26 18:02 UTC (permalink / raw)
  To: Dixit, Ashutosh; +Cc: intel-gfx

On Wed, Mar 25, 2020 at 06:52:52PM -0700, Dixit, Ashutosh wrote:
>On Wed, 25 Mar 2020 17:32:35 -0700, Umesh Nerlige Ramappa wrote:
>>
>> On Wed, Mar 25, 2020 at 11:20:19AM -0700, Ashutosh Dixit wrote:
>> > It is wrong to block the user thread in the next poll when OA data is
>> > already available which could not fit in the user buffer provided in
>> > the previous read. In several cases the exact user buffer size is not
>> > known. Blocking user space in poll can lead to data loss when the
>> > buffer size used is smaller than the available data.
>> >
>> > This change fixes this issue and allows user space to read all OA data
>> > even when using a buffer size smaller than the available data using
>> > multiple non-blocking reads rather than staying blocked in poll till
>> > the next timer interrupt.
>> >
>> > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>> > ---
>> > drivers/gpu/drm/i915/i915_perf.c | 62 ++++++--------------------------
>> > 1 file changed, 11 insertions(+), 51 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
>> > index 3222f6cd8255..c1a47c030941 100644
>> > --- a/drivers/gpu/drm/i915/i915_perf.c
>> > +++ b/drivers/gpu/drm/i915/i915_perf.c
>> > @@ -2957,49 +2957,6 @@ void i915_oa_init_reg_state(const struct intel_context *ce,
>> >		gen8_update_reg_state_unlocked(ce, stream);
>> > }
>> >
>> > -/**
>> > - * i915_perf_read_locked - &i915_perf_stream_ops->read with error normalisation
>> > - * @stream: An i915 perf stream
>> > - * @file: An i915 perf stream file
>> > - * @buf: destination buffer given by userspace
>> > - * @count: the number of bytes userspace wants to read
>> > - * @ppos: (inout) file seek position (unused)
>> > - *
>> > - * Besides wrapping &i915_perf_stream_ops->read this provides a common place to
>> > - * ensure that if we've successfully copied any data then reporting that takes
>> > - * precedence over any internal error status, so the data isn't lost.
>> > - *
>> > - * For example ret will be -ENOSPC whenever there is more buffered data than
>> > - * can be copied to userspace, but that's only interesting if we weren't able
>> > - * to copy some data because it implies the userspace buffer is too small to
>> > - * receive a single record (and we never split records).
>> > - *
>> > - * Another case with ret == -EFAULT is more of a grey area since it would seem
>> > - * like bad form for userspace to ask us to overrun its buffer, but the user
>> > - * knows best:
>> > - *
>> > - *   http://yarchive.net/comp/linux/partial_reads_writes.html
>> > - *
>> > - * Returns: The number of bytes copied or a negative error code on failure.
>> > - */
>> > -static ssize_t i915_perf_read_locked(struct i915_perf_stream *stream,
>> > -				     struct file *file,
>> > -				     char __user *buf,
>> > -				     size_t count,
>> > -				     loff_t *ppos)
>> > -{
>> > -	/* Note we keep the offset (aka bytes read) separate from any
>> > -	 * error status so that the final check for whether we return
>> > -	 * the bytes read with a higher precedence than any error (see
>> > -	 * comment below) doesn't need to be handled/duplicated in
>> > -	 * stream->ops->read() implementations.
>> > -	 */
>> > -	size_t offset = 0;
>> > -	int ret = stream->ops->read(stream, buf, count, &offset);
>> > -
>> > -	return offset ?: (ret ?: -EAGAIN);
>> > -}
>> > -
>> > /**
>> >  * i915_perf_read - handles read() FOP for i915 perf stream FDs
>> >  * @file: An i915 perf stream file
>> > @@ -3025,6 +2982,8 @@ static ssize_t i915_perf_read(struct file *file,
>> > {
>> >	struct i915_perf_stream *stream = file->private_data;
>> >	struct i915_perf *perf = stream->perf;
>> > +	size_t offset = 0;
>> > +	int __ret;
>> >	ssize_t ret;
>> >
>> >	/* To ensure it's handled consistently we simply treat all reads of a
>> > @@ -3048,16 +3007,18 @@ static ssize_t i915_perf_read(struct file *file,
>> >				return ret;
>> >
>> >			mutex_lock(&perf->lock);
>> > -			ret = i915_perf_read_locked(stream, file,
>> > -						    buf, count, ppos);
>> > +			__ret = stream->ops->read(stream, buf, count, &offset);
>> >			mutex_unlock(&perf->lock);
>> >		} while (ret == -EAGAIN);
>>
>> ret will never be EAGAIN here in the while. EAGAIN was returned by the
>> deleted function in this patch if offset and ret are both 0.
>
>Good catch, I was so focussed on the non-blocking case that I missed the
>blocking case.
>
>> Although I don't see how that would be true.
>
>As you say above, the old function i915_perf_read_locked() was doing this:
>
>	return offset ?: (__ret ?: -EAGAIN);
>
>So -EAGAIN is returned from i915_perf_read_locked() when there is no data
>to read but otherwise there is no other error. Since this is blocking read
>we cannot return -EAGAIN to user space (since there is no data to read), we
>must go back and block again. That is the purpose of the while loop. I
>broke this logic in this patch and will need to fix this.
>
>>
>> >	} else {
>> >		mutex_lock(&perf->lock);
>> > -		ret = i915_perf_read_locked(stream, file, buf, count, ppos);
>> > +		__ret = stream->ops->read(stream, buf, count, &offset);
>> >		mutex_unlock(&perf->lock);
>> >	}
>> >
>> > +	/* Possible values for __ret are 0, -EFAULT, -ENOSPC, -EAGAIN, ... */
>>
>> __ret may never be EAGAIN either (comment^). I don't see EAGAIN in the read
>> path.
>
>It's here:
>
>gen8_append_oa_reports()
>{
>
>        /*
>         * An invalid tail pointer here means we're still waiting for the poll
>         * hrtimer callback to give us a pointer
>         */
>        if (tail == INVALID_TAIL_PTR)
>                return -EAGAIN;
>}

Oh, you are right, EAGAIN is returned here. I was looking for it with 
the poll period patch series applied and these references are removed in 
that series.

Thanks,
Umesh

>
>> That said, EAGAIN seems to have been introduced in the prior code
>> specifically for retrying the blocking read and may not have much meaning
>> otherwise.
>
>No that's not true. The kernel non-blocking read() function (in fops)
>returns -EAGAIN when there is no data to read (the function never returns 0
>except in case of EOF, in i915 perf code there is no EOF so read never
>returns 0). This logic is the same as that in the previous code and we need
>to preserve it.
>
>Will post a v2 with the fix.
>
>Thanks!
>--
>Ashutosh
>
>
>>
>> Thanks,
>> Umesh
>>
>> > +	ret = offset ?: (__ret ?: -EAGAIN);
>> > +
>> >	/* We allow the poll checking to sometimes report false positive EPOLLIN
>> >	 * events where we might actually report EAGAIN on read() if there's
>> >	 * not really any data available. In this situation though we don't
>> > @@ -3065,13 +3026,12 @@ static ssize_t i915_perf_read(struct file *file,
>> >	 * and read() returning -EAGAIN. Clearing the oa.pollin state here
>> >	 * effectively ensures we back off until the next hrtimer callback
>> >	 * before reporting another EPOLLIN event.
>> > +	 * The exception to this is if ops->read() returned -ENOSPC which means
>> > +	 * that more OA data is available than could fit in the user provided
>> > +	 * buffer. In this case we want the next poll() call to not block.
>> >	 */
>> > -	if (ret >= 0 || ret == -EAGAIN) {
>> > -		/* Maybe make ->pollin per-stream state if we support multiple
>> > -		 * concurrent streams in the future.
>> > -		 */
>> > +	if ((ret > 0 || ret == -EAGAIN) && __ret != -ENOSPC)
>> >		stream->pollin = false;
>> > -	}
>> >
>> >	return ret;
>> > }
>> > --
>> > 2.25.2
>> >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers
  2020-03-26  0:32 ` Umesh Nerlige Ramappa
@ 2020-03-26  1:52   ` Dixit, Ashutosh
  2020-03-26 18:02     ` Umesh Nerlige Ramappa
  0 siblings, 1 reply; 29+ messages in thread
From: Dixit, Ashutosh @ 2020-03-26  1:52 UTC (permalink / raw)
  To: Umesh Nerlige Ramappa; +Cc: intel-gfx

On Wed, 25 Mar 2020 17:32:35 -0700, Umesh Nerlige Ramappa wrote:
>
> On Wed, Mar 25, 2020 at 11:20:19AM -0700, Ashutosh Dixit wrote:
> > It is wrong to block the user thread in the next poll when OA data is
> > already available which could not fit in the user buffer provided in
> > the previous read. In several cases the exact user buffer size is not
> > known. Blocking user space in poll can lead to data loss when the
> > buffer size used is smaller than the available data.
> >
> > This change fixes this issue and allows user space to read all OA data
> > even when using a buffer size smaller than the available data using
> > multiple non-blocking reads rather than staying blocked in poll till
> > the next timer interrupt.
> >
> > Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> > Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> > Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> > ---
> > drivers/gpu/drm/i915/i915_perf.c | 62 ++++++--------------------------
> > 1 file changed, 11 insertions(+), 51 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> > index 3222f6cd8255..c1a47c030941 100644
> > --- a/drivers/gpu/drm/i915/i915_perf.c
> > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > @@ -2957,49 +2957,6 @@ void i915_oa_init_reg_state(const struct intel_context *ce,
> >		gen8_update_reg_state_unlocked(ce, stream);
> > }
> >
> > -/**
> > - * i915_perf_read_locked - &i915_perf_stream_ops->read with error normalisation
> > - * @stream: An i915 perf stream
> > - * @file: An i915 perf stream file
> > - * @buf: destination buffer given by userspace
> > - * @count: the number of bytes userspace wants to read
> > - * @ppos: (inout) file seek position (unused)
> > - *
> > - * Besides wrapping &i915_perf_stream_ops->read this provides a common place to
> > - * ensure that if we've successfully copied any data then reporting that takes
> > - * precedence over any internal error status, so the data isn't lost.
> > - *
> > - * For example ret will be -ENOSPC whenever there is more buffered data than
> > - * can be copied to userspace, but that's only interesting if we weren't able
> > - * to copy some data because it implies the userspace buffer is too small to
> > - * receive a single record (and we never split records).
> > - *
> > - * Another case with ret == -EFAULT is more of a grey area since it would seem
> > - * like bad form for userspace to ask us to overrun its buffer, but the user
> > - * knows best:
> > - *
> > - *   http://yarchive.net/comp/linux/partial_reads_writes.html
> > - *
> > - * Returns: The number of bytes copied or a negative error code on failure.
> > - */
> > -static ssize_t i915_perf_read_locked(struct i915_perf_stream *stream,
> > -				     struct file *file,
> > -				     char __user *buf,
> > -				     size_t count,
> > -				     loff_t *ppos)
> > -{
> > -	/* Note we keep the offset (aka bytes read) separate from any
> > -	 * error status so that the final check for whether we return
> > -	 * the bytes read with a higher precedence than any error (see
> > -	 * comment below) doesn't need to be handled/duplicated in
> > -	 * stream->ops->read() implementations.
> > -	 */
> > -	size_t offset = 0;
> > -	int ret = stream->ops->read(stream, buf, count, &offset);
> > -
> > -	return offset ?: (ret ?: -EAGAIN);
> > -}
> > -
> > /**
> >  * i915_perf_read - handles read() FOP for i915 perf stream FDs
> >  * @file: An i915 perf stream file
> > @@ -3025,6 +2982,8 @@ static ssize_t i915_perf_read(struct file *file,
> > {
> >	struct i915_perf_stream *stream = file->private_data;
> >	struct i915_perf *perf = stream->perf;
> > +	size_t offset = 0;
> > +	int __ret;
> >	ssize_t ret;
> >
> >	/* To ensure it's handled consistently we simply treat all reads of a
> > @@ -3048,16 +3007,18 @@ static ssize_t i915_perf_read(struct file *file,
> >				return ret;
> >
> >			mutex_lock(&perf->lock);
> > -			ret = i915_perf_read_locked(stream, file,
> > -						    buf, count, ppos);
> > +			__ret = stream->ops->read(stream, buf, count, &offset);
> >			mutex_unlock(&perf->lock);
> >		} while (ret == -EAGAIN);
>
> ret will never be EAGAIN here in the while. EAGAIN was returned by the
> deleted function in this patch if offset and ret are both 0.

Good catch, I was so focussed on the non-blocking case that I missed the
blocking case.

> Although I don't see how that would be true.

As you say above, the old function i915_perf_read_locked() was doing this:

	return offset ?: (__ret ?: -EAGAIN);

So -EAGAIN is returned from i915_perf_read_locked() when there is no data
to read but otherwise there is no other error. Since this is blocking read
we cannot return -EAGAIN to user space (since there is no data to read), we
must go back and block again. That is the purpose of the while loop. I
broke this logic in this patch and will need to fix this.

>
> >	} else {
> >		mutex_lock(&perf->lock);
> > -		ret = i915_perf_read_locked(stream, file, buf, count, ppos);
> > +		__ret = stream->ops->read(stream, buf, count, &offset);
> >		mutex_unlock(&perf->lock);
> >	}
> >
> > +	/* Possible values for __ret are 0, -EFAULT, -ENOSPC, -EAGAIN, ... */
>
> __ret may never be EAGAIN either (comment^). I don't see EAGAIN in the read
> path.

It's here:

gen8_append_oa_reports()
{

        /*
         * An invalid tail pointer here means we're still waiting for the poll
         * hrtimer callback to give us a pointer
         */
        if (tail == INVALID_TAIL_PTR)
                return -EAGAIN;
}

> That said, EAGAIN seems to have been introduced in the prior code
> specifically for retrying the blocking read and may not have much meaning
> otherwise.

No that's not true. The kernel non-blocking read() function (in fops)
returns -EAGAIN when there is no data to read (the function never returns 0
except in case of EOF, in i915 perf code there is no EOF so read never
returns 0). This logic is the same as that in the previous code and we need
to preserve it.

Will post a v2 with the fix.

Thanks!
--
Ashutosh


>
> Thanks,
> Umesh
>
> > +	ret = offset ?: (__ret ?: -EAGAIN);
> > +
> >	/* We allow the poll checking to sometimes report false positive EPOLLIN
> >	 * events where we might actually report EAGAIN on read() if there's
> >	 * not really any data available. In this situation though we don't
> > @@ -3065,13 +3026,12 @@ static ssize_t i915_perf_read(struct file *file,
> >	 * and read() returning -EAGAIN. Clearing the oa.pollin state here
> >	 * effectively ensures we back off until the next hrtimer callback
> >	 * before reporting another EPOLLIN event.
> > +	 * The exception to this is if ops->read() returned -ENOSPC which means
> > +	 * that more OA data is available than could fit in the user provided
> > +	 * buffer. In this case we want the next poll() call to not block.
> >	 */
> > -	if (ret >= 0 || ret == -EAGAIN) {
> > -		/* Maybe make ->pollin per-stream state if we support multiple
> > -		 * concurrent streams in the future.
> > -		 */
> > +	if ((ret > 0 || ret == -EAGAIN) && __ret != -ENOSPC)
> >		stream->pollin = false;
> > -	}
> >
> >	return ret;
> > }
> > --
> > 2.25.2
> >
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers
  2020-03-25 18:20 Ashutosh Dixit
  2020-03-25 19:25 ` Lionel Landwerlin
@ 2020-03-26  0:32 ` Umesh Nerlige Ramappa
  2020-03-26  1:52   ` Dixit, Ashutosh
  1 sibling, 1 reply; 29+ messages in thread
From: Umesh Nerlige Ramappa @ 2020-03-26  0:32 UTC (permalink / raw)
  To: Ashutosh Dixit; +Cc: intel-gfx

On Wed, Mar 25, 2020 at 11:20:19AM -0700, Ashutosh Dixit wrote:
>It is wrong to block the user thread in the next poll when OA data is
>already available which could not fit in the user buffer provided in
>the previous read. In several cases the exact user buffer size is not
>known. Blocking user space in poll can lead to data loss when the
>buffer size used is smaller than the available data.
>
>This change fixes this issue and allows user space to read all OA data
>even when using a buffer size smaller than the available data using
>multiple non-blocking reads rather than staying blocked in poll till
>the next timer interrupt.
>
>Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
>Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
>---
> drivers/gpu/drm/i915/i915_perf.c | 62 ++++++--------------------------
> 1 file changed, 11 insertions(+), 51 deletions(-)
>
>diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
>index 3222f6cd8255..c1a47c030941 100644
>--- a/drivers/gpu/drm/i915/i915_perf.c
>+++ b/drivers/gpu/drm/i915/i915_perf.c
>@@ -2957,49 +2957,6 @@ void i915_oa_init_reg_state(const struct intel_context *ce,
> 		gen8_update_reg_state_unlocked(ce, stream);
> }
>
>-/**
>- * i915_perf_read_locked - &i915_perf_stream_ops->read with error normalisation
>- * @stream: An i915 perf stream
>- * @file: An i915 perf stream file
>- * @buf: destination buffer given by userspace
>- * @count: the number of bytes userspace wants to read
>- * @ppos: (inout) file seek position (unused)
>- *
>- * Besides wrapping &i915_perf_stream_ops->read this provides a common place to
>- * ensure that if we've successfully copied any data then reporting that takes
>- * precedence over any internal error status, so the data isn't lost.
>- *
>- * For example ret will be -ENOSPC whenever there is more buffered data than
>- * can be copied to userspace, but that's only interesting if we weren't able
>- * to copy some data because it implies the userspace buffer is too small to
>- * receive a single record (and we never split records).
>- *
>- * Another case with ret == -EFAULT is more of a grey area since it would seem
>- * like bad form for userspace to ask us to overrun its buffer, but the user
>- * knows best:
>- *
>- *   http://yarchive.net/comp/linux/partial_reads_writes.html
>- *
>- * Returns: The number of bytes copied or a negative error code on failure.
>- */
>-static ssize_t i915_perf_read_locked(struct i915_perf_stream *stream,
>-				     struct file *file,
>-				     char __user *buf,
>-				     size_t count,
>-				     loff_t *ppos)
>-{
>-	/* Note we keep the offset (aka bytes read) separate from any
>-	 * error status so that the final check for whether we return
>-	 * the bytes read with a higher precedence than any error (see
>-	 * comment below) doesn't need to be handled/duplicated in
>-	 * stream->ops->read() implementations.
>-	 */
>-	size_t offset = 0;
>-	int ret = stream->ops->read(stream, buf, count, &offset);
>-
>-	return offset ?: (ret ?: -EAGAIN);
>-}
>-
> /**
>  * i915_perf_read - handles read() FOP for i915 perf stream FDs
>  * @file: An i915 perf stream file
>@@ -3025,6 +2982,8 @@ static ssize_t i915_perf_read(struct file *file,
> {
> 	struct i915_perf_stream *stream = file->private_data;
> 	struct i915_perf *perf = stream->perf;
>+	size_t offset = 0;
>+	int __ret;
> 	ssize_t ret;
>
> 	/* To ensure it's handled consistently we simply treat all reads of a
>@@ -3048,16 +3007,18 @@ static ssize_t i915_perf_read(struct file *file,
> 				return ret;
>
> 			mutex_lock(&perf->lock);
>-			ret = i915_perf_read_locked(stream, file,
>-						    buf, count, ppos);
>+			__ret = stream->ops->read(stream, buf, count, &offset);
> 			mutex_unlock(&perf->lock);
> 		} while (ret == -EAGAIN);

ret will never be EAGAIN here in the while. EAGAIN was returned by the 
deleted function in this patch if offset and ret are both 0. Although I 
don't see how that would be true.

> 	} else {
> 		mutex_lock(&perf->lock);
>-		ret = i915_perf_read_locked(stream, file, buf, count, ppos);
>+		__ret = stream->ops->read(stream, buf, count, &offset);
> 		mutex_unlock(&perf->lock);
> 	}
>
>+	/* Possible values for __ret are 0, -EFAULT, -ENOSPC, -EAGAIN, ... */

__ret may never be EAGAIN either (comment^). I don't see EAGAIN in the 
read path. 

That said, EAGAIN seems to have been introduced in the prior code 
specifically for retrying the blocking read and may not have much 
meaning otherwise.

Thanks,
Umesh

>+	ret = offset ?: (__ret ?: -EAGAIN);
>+
> 	/* We allow the poll checking to sometimes report false positive EPOLLIN
> 	 * events where we might actually report EAGAIN on read() if there's
> 	 * not really any data available. In this situation though we don't
>@@ -3065,13 +3026,12 @@ static ssize_t i915_perf_read(struct file *file,
> 	 * and read() returning -EAGAIN. Clearing the oa.pollin state here
> 	 * effectively ensures we back off until the next hrtimer callback
> 	 * before reporting another EPOLLIN event.
>+	 * The exception to this is if ops->read() returned -ENOSPC which means
>+	 * that more OA data is available than could fit in the user provided
>+	 * buffer. In this case we want the next poll() call to not block.
> 	 */
>-	if (ret >= 0 || ret == -EAGAIN) {
>-		/* Maybe make ->pollin per-stream state if we support multiple
>-		 * concurrent streams in the future.
>-		 */
>+	if ((ret > 0 || ret == -EAGAIN) && __ret != -ENOSPC)
> 		stream->pollin = false;
>-	}
>
> 	return ret;
> }
>-- 
>2.25.2
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

* Re: [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers
  2020-03-25 19:25 ` Lionel Landwerlin
@ 2020-03-25 20:44   ` Dixit, Ashutosh
  0 siblings, 0 replies; 29+ messages in thread
From: Dixit, Ashutosh @ 2020-03-25 20:44 UTC (permalink / raw)
  To: Lionel Landwerlin; +Cc: intel-gfx

On Wed, 25 Mar 2020 12:25:59 -0700, Lionel Landwerlin wrote:
>
> On 25/03/2020 20:20, Ashutosh Dixit wrote:
> > It is wrong to block the user thread in the next poll when OA data is
> > already available which could not fit in the user buffer provided in
> > the previous read. In several cases the exact user buffer size is not
> > known. Blocking user space in poll can lead to data loss when the
> > buffer size used is smaller than the available data.
> >
> > This change fixes this issue and allows user space to read all OA data
> > even when using a buffer size smaller than the available data using
> > multiple non-blocking reads rather than staying blocked in poll till
> > the next timer interrupt.
>
> Looks like you found a pretty important issue.
>
> Can you write an IGT test case so that we don't run into it again?

OK I'll see if I can come up with an IGT.

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

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

* Re: [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers
  2020-03-25 18:20 Ashutosh Dixit
@ 2020-03-25 19:25 ` Lionel Landwerlin
  2020-03-25 20:44   ` Dixit, Ashutosh
  2020-03-26  0:32 ` Umesh Nerlige Ramappa
  1 sibling, 1 reply; 29+ messages in thread
From: Lionel Landwerlin @ 2020-03-25 19:25 UTC (permalink / raw)
  To: Ashutosh Dixit, intel-gfx

On 25/03/2020 20:20, Ashutosh Dixit wrote:
> It is wrong to block the user thread in the next poll when OA data is
> already available which could not fit in the user buffer provided in
> the previous read. In several cases the exact user buffer size is not
> known. Blocking user space in poll can lead to data loss when the
> buffer size used is smaller than the available data.
>
> This change fixes this issue and allows user space to read all OA data
> even when using a buffer size smaller than the available data using
> multiple non-blocking reads rather than staying blocked in poll till
> the next timer interrupt.


Looks like you found a pretty important issue.

Can you write an IGT test case so that we don't run into it again?


Thanks a lot,


-Lionel


>
> Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_perf.c | 62 ++++++--------------------------
>   1 file changed, 11 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index 3222f6cd8255..c1a47c030941 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -2957,49 +2957,6 @@ void i915_oa_init_reg_state(const struct intel_context *ce,
>   		gen8_update_reg_state_unlocked(ce, stream);
>   }
>   
> -/**
> - * i915_perf_read_locked - &i915_perf_stream_ops->read with error normalisation
> - * @stream: An i915 perf stream
> - * @file: An i915 perf stream file
> - * @buf: destination buffer given by userspace
> - * @count: the number of bytes userspace wants to read
> - * @ppos: (inout) file seek position (unused)
> - *
> - * Besides wrapping &i915_perf_stream_ops->read this provides a common place to
> - * ensure that if we've successfully copied any data then reporting that takes
> - * precedence over any internal error status, so the data isn't lost.
> - *
> - * For example ret will be -ENOSPC whenever there is more buffered data than
> - * can be copied to userspace, but that's only interesting if we weren't able
> - * to copy some data because it implies the userspace buffer is too small to
> - * receive a single record (and we never split records).
> - *
> - * Another case with ret == -EFAULT is more of a grey area since it would seem
> - * like bad form for userspace to ask us to overrun its buffer, but the user
> - * knows best:
> - *
> - *   http://yarchive.net/comp/linux/partial_reads_writes.html
> - *
> - * Returns: The number of bytes copied or a negative error code on failure.
> - */
> -static ssize_t i915_perf_read_locked(struct i915_perf_stream *stream,
> -				     struct file *file,
> -				     char __user *buf,
> -				     size_t count,
> -				     loff_t *ppos)
> -{
> -	/* Note we keep the offset (aka bytes read) separate from any
> -	 * error status so that the final check for whether we return
> -	 * the bytes read with a higher precedence than any error (see
> -	 * comment below) doesn't need to be handled/duplicated in
> -	 * stream->ops->read() implementations.
> -	 */
> -	size_t offset = 0;
> -	int ret = stream->ops->read(stream, buf, count, &offset);
> -
> -	return offset ?: (ret ?: -EAGAIN);
> -}
> -
>   /**
>    * i915_perf_read - handles read() FOP for i915 perf stream FDs
>    * @file: An i915 perf stream file
> @@ -3025,6 +2982,8 @@ static ssize_t i915_perf_read(struct file *file,
>   {
>   	struct i915_perf_stream *stream = file->private_data;
>   	struct i915_perf *perf = stream->perf;
> +	size_t offset = 0;
> +	int __ret;
>   	ssize_t ret;
>   
>   	/* To ensure it's handled consistently we simply treat all reads of a
> @@ -3048,16 +3007,18 @@ static ssize_t i915_perf_read(struct file *file,
>   				return ret;
>   
>   			mutex_lock(&perf->lock);
> -			ret = i915_perf_read_locked(stream, file,
> -						    buf, count, ppos);
> +			__ret = stream->ops->read(stream, buf, count, &offset);
>   			mutex_unlock(&perf->lock);
>   		} while (ret == -EAGAIN);
>   	} else {
>   		mutex_lock(&perf->lock);
> -		ret = i915_perf_read_locked(stream, file, buf, count, ppos);
> +		__ret = stream->ops->read(stream, buf, count, &offset);
>   		mutex_unlock(&perf->lock);
>   	}
>   
> +	/* Possible values for __ret are 0, -EFAULT, -ENOSPC, -EAGAIN, ... */
> +	ret = offset ?: (__ret ?: -EAGAIN);
> +
>   	/* We allow the poll checking to sometimes report false positive EPOLLIN
>   	 * events where we might actually report EAGAIN on read() if there's
>   	 * not really any data available. In this situation though we don't
> @@ -3065,13 +3026,12 @@ static ssize_t i915_perf_read(struct file *file,
>   	 * and read() returning -EAGAIN. Clearing the oa.pollin state here
>   	 * effectively ensures we back off until the next hrtimer callback
>   	 * before reporting another EPOLLIN event.
> +	 * The exception to this is if ops->read() returned -ENOSPC which means
> +	 * that more OA data is available than could fit in the user provided
> +	 * buffer. In this case we want the next poll() call to not block.
>   	 */
> -	if (ret >= 0 || ret == -EAGAIN) {
> -		/* Maybe make ->pollin per-stream state if we support multiple
> -		 * concurrent streams in the future.
> -		 */
> +	if ((ret > 0 || ret == -EAGAIN) && __ret != -ENOSPC)
>   		stream->pollin = false;
> -	}
>   
>   	return ret;
>   }


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

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

* [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers
@ 2020-03-25 18:20 Ashutosh Dixit
  2020-03-25 19:25 ` Lionel Landwerlin
  2020-03-26  0:32 ` Umesh Nerlige Ramappa
  0 siblings, 2 replies; 29+ messages in thread
From: Ashutosh Dixit @ 2020-03-25 18:20 UTC (permalink / raw)
  To: intel-gfx

It is wrong to block the user thread in the next poll when OA data is
already available which could not fit in the user buffer provided in
the previous read. In several cases the exact user buffer size is not
known. Blocking user space in poll can lead to data loss when the
buffer size used is smaller than the available data.

This change fixes this issue and allows user space to read all OA data
even when using a buffer size smaller than the available data using
multiple non-blocking reads rather than staying blocked in poll till
the next timer interrupt.

Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Signed-off-by: Ashutosh Dixit <ashutosh.dixit@intel.com>
---
 drivers/gpu/drm/i915/i915_perf.c | 62 ++++++--------------------------
 1 file changed, 11 insertions(+), 51 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 3222f6cd8255..c1a47c030941 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -2957,49 +2957,6 @@ void i915_oa_init_reg_state(const struct intel_context *ce,
 		gen8_update_reg_state_unlocked(ce, stream);
 }
 
-/**
- * i915_perf_read_locked - &i915_perf_stream_ops->read with error normalisation
- * @stream: An i915 perf stream
- * @file: An i915 perf stream file
- * @buf: destination buffer given by userspace
- * @count: the number of bytes userspace wants to read
- * @ppos: (inout) file seek position (unused)
- *
- * Besides wrapping &i915_perf_stream_ops->read this provides a common place to
- * ensure that if we've successfully copied any data then reporting that takes
- * precedence over any internal error status, so the data isn't lost.
- *
- * For example ret will be -ENOSPC whenever there is more buffered data than
- * can be copied to userspace, but that's only interesting if we weren't able
- * to copy some data because it implies the userspace buffer is too small to
- * receive a single record (and we never split records).
- *
- * Another case with ret == -EFAULT is more of a grey area since it would seem
- * like bad form for userspace to ask us to overrun its buffer, but the user
- * knows best:
- *
- *   http://yarchive.net/comp/linux/partial_reads_writes.html
- *
- * Returns: The number of bytes copied or a negative error code on failure.
- */
-static ssize_t i915_perf_read_locked(struct i915_perf_stream *stream,
-				     struct file *file,
-				     char __user *buf,
-				     size_t count,
-				     loff_t *ppos)
-{
-	/* Note we keep the offset (aka bytes read) separate from any
-	 * error status so that the final check for whether we return
-	 * the bytes read with a higher precedence than any error (see
-	 * comment below) doesn't need to be handled/duplicated in
-	 * stream->ops->read() implementations.
-	 */
-	size_t offset = 0;
-	int ret = stream->ops->read(stream, buf, count, &offset);
-
-	return offset ?: (ret ?: -EAGAIN);
-}
-
 /**
  * i915_perf_read - handles read() FOP for i915 perf stream FDs
  * @file: An i915 perf stream file
@@ -3025,6 +2982,8 @@ static ssize_t i915_perf_read(struct file *file,
 {
 	struct i915_perf_stream *stream = file->private_data;
 	struct i915_perf *perf = stream->perf;
+	size_t offset = 0;
+	int __ret;
 	ssize_t ret;
 
 	/* To ensure it's handled consistently we simply treat all reads of a
@@ -3048,16 +3007,18 @@ static ssize_t i915_perf_read(struct file *file,
 				return ret;
 
 			mutex_lock(&perf->lock);
-			ret = i915_perf_read_locked(stream, file,
-						    buf, count, ppos);
+			__ret = stream->ops->read(stream, buf, count, &offset);
 			mutex_unlock(&perf->lock);
 		} while (ret == -EAGAIN);
 	} else {
 		mutex_lock(&perf->lock);
-		ret = i915_perf_read_locked(stream, file, buf, count, ppos);
+		__ret = stream->ops->read(stream, buf, count, &offset);
 		mutex_unlock(&perf->lock);
 	}
 
+	/* Possible values for __ret are 0, -EFAULT, -ENOSPC, -EAGAIN, ... */
+	ret = offset ?: (__ret ?: -EAGAIN);
+
 	/* We allow the poll checking to sometimes report false positive EPOLLIN
 	 * events where we might actually report EAGAIN on read() if there's
 	 * not really any data available. In this situation though we don't
@@ -3065,13 +3026,12 @@ static ssize_t i915_perf_read(struct file *file,
 	 * and read() returning -EAGAIN. Clearing the oa.pollin state here
 	 * effectively ensures we back off until the next hrtimer callback
 	 * before reporting another EPOLLIN event.
+	 * The exception to this is if ops->read() returned -ENOSPC which means
+	 * that more OA data is available than could fit in the user provided
+	 * buffer. In this case we want the next poll() call to not block.
 	 */
-	if (ret >= 0 || ret == -EAGAIN) {
-		/* Maybe make ->pollin per-stream state if we support multiple
-		 * concurrent streams in the future.
-		 */
+	if ((ret > 0 || ret == -EAGAIN) && __ret != -ENOSPC)
 		stream->pollin = false;
-	}
 
 	return ret;
 }
-- 
2.25.2

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

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

end of thread, other threads:[~2020-04-13 22:05 UTC | newest]

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-26  4:43 [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers Ashutosh Dixit
2020-03-26  5:29 ` [Intel-gfx] ✓ Fi.CI.BAT: success for drm/i915/perf: Do not clear pollin for small user read buffers (rev2) Patchwork
2020-03-26  6:47 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
2020-03-26  9:09 ` [Intel-gfx] [PATCH] drm/i915/perf: Do not clear pollin for small user read buffers Lionel Landwerlin
2020-03-27  3:39   ` Dixit, Ashutosh
  -- strict thread matches above, loose matches on Subject: below --
2020-04-08 23:42 Ashutosh Dixit
2020-04-13 22:05 ` Rodrigo Vivi
2020-04-03  1:01 Ashutosh Dixit
2020-04-03 16:17 ` Chris Wilson
2020-04-03 17:45   ` Dixit, Ashutosh
2020-04-03 17:59     ` Chris Wilson
2020-03-31 23:14 Ashutosh Dixit
2020-04-01  6:57 ` Lionel Landwerlin
2020-04-01  7:43   ` Dixit, Ashutosh
2020-04-01  7:51     ` Lionel Landwerlin
2020-03-31  5:22 Ashutosh Dixit
2020-03-31  7:34 ` Lionel Landwerlin
2020-03-31 23:29   ` Dixit, Ashutosh
2020-03-27 23:16 Ashutosh Dixit
2020-03-30  8:23 ` Lionel Landwerlin
2020-03-31  5:47   ` Dixit, Ashutosh
2020-03-27 23:11 Ashutosh Dixit
2020-03-25 18:20 Ashutosh Dixit
2020-03-25 19:25 ` Lionel Landwerlin
2020-03-25 20:44   ` Dixit, Ashutosh
2020-03-26  0:32 ` Umesh Nerlige Ramappa
2020-03-26  1:52   ` Dixit, Ashutosh
2020-03-26 18:02     ` Umesh Nerlige Ramappa
2020-03-27  1:28       ` Dixit, Ashutosh

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.