ACK/Cmnt: ACK/cmnt: [PATCH 1/1][SRU][Focal] drm/i915/perf: Do not clear pollin for small user read buffers
Stefan Bader
stefan.bader at canonical.com
Wed May 13 09:42:44 UTC 2020
On 13.05.20 11:30, Kleber Souza wrote:
> On 06.05.20 04:50, AceLan Kao wrote:
>> From: Ashutosh Dixit <ashutosh.dixit at intel.com>
>>
>> BugLink: https://bugs.launchpad.net/bugs/1877013
>>
>> 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 at intel.com>
>> Signed-off-by: Ashutosh Dixit <ashutosh.dixit at intel.com>
>> Cc: Umesh Nerlige Ramappa <umesh.nerlige.ramappa at intel.com>
>> Cc: <stable at vger.kernel.org>
>> Signed-off-by: Chris Wilson <chris at chris-wilson.co.uk>
>> Link: https://patchwork.freedesktop.org/patch/msgid/20200403010120.3067-1-ashutosh.dixit@intel.com
>> (cherry-picked from commit 6352219c39c04ed3f9a8d1cf93f87c21753a213e)
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at intel.com>
>> (backported from commit bcad588dea538a4fc173d16a90a005536ec8dbf2)
>> Signed-off-by: AceLan Kao <acelan.kao at canonical.com>
>
> This patch seems to have been applied to 5.5.y and 5.6.y only, so I'm wondering
> why it hasn't been applied to 5.4.y which is LTS.
>
> Anyway, this is a clean cherry-pick and we have good test results.
>
>
> Acked-by: Kleber Sacilotto de Souza <kleber.souza at canonical.com>
Acked-by: Stefan Bader <stefan.bader at canonical.com>
>
>> ---
I can only guess that the answer is that it does not apply cleanly (thus was
backported to focal). I can not see obvious differences and with good testing
results it seems acceptable.
>> 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 e42b86827d6b..bf917fc1837d 100644
>> --- a/drivers/gpu/drm/i915/i915_perf.c
>> +++ b/drivers/gpu/drm/i915/i915_perf.c
>> @@ -2312,49 +2312,6 @@ void i915_oa_init_reg_state(struct intel_engine_cs *engine,
>> gen8_update_reg_state_unlocked(stream, ce, regs, stream->oa_config);
>> }
>>
>> -/**
>> - * 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
>> @@ -2380,7 +2337,8 @@ static ssize_t i915_perf_read(struct file *file,
>> {
>> struct i915_perf_stream *stream = file->private_data;
>> struct drm_i915_private *dev_priv = stream->dev_priv;
>> - 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
>> @@ -2403,13 +2361,12 @@ static ssize_t i915_perf_read(struct file *file,
>> return ret;
>>
>> mutex_lock(&dev_priv->perf.lock);
>> - ret = i915_perf_read_locked(stream, file,
>> - buf, count, ppos);
>> + ret = stream->ops->read(stream, buf, count, &offset);
>> mutex_unlock(&dev_priv->perf.lock);
>> - } while (ret == -EAGAIN);
>> + } while (!offset && !ret);
>> } else {
>> mutex_lock(&dev_priv->perf.lock);
>> - ret = i915_perf_read_locked(stream, file, buf, count, ppos);
>> + ret = stream->ops->read(stream, buf, count, &offset);
>> mutex_unlock(&dev_priv->perf.lock);
>> }
>>
>> @@ -2420,15 +2377,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)
>>
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.ubuntu.com/archives/kernel-team/attachments/20200513/6a7caa60/attachment.sig>
More information about the kernel-team
mailing list