On Mon, 01 Mar 2021 16:01:41 -0800, Nerlige Ramappa, Umesh wrote:
>
> SAMPLE_OA parameter enables sampling of OA buffer and results in a call
> to init the OA buffer which initializes the OA unit head/tail pointers.
> The OA_EXPONENT parameter controls the periodicity of the OA reports in
> the OA buffer and results in starting a hrtimer.
>
> Before gen12, all use cases required the use of the OA buffer and i915
> enforced this setting when vetting out the parameters passed. In these
> platforms the hrtimer was enabled if OA_EXPONENT was passed. This worked
> fine since it was implied that SAMPLE_OA is always passed.
>
> With gen12, this changed. Users can use perf without enabling the OA
> buffer as in OAR use cases. While an OAR use case should ideally not
> start the hrtimer, we see that passing an OA_EXPONENT parameter will
> start the hrtimer even though SAMPLE_OA is not specified. This results
> in an uninitialized OA buffer, so the head/tail pointers used to track
> the buffer are zero.
>
> This itself does not fail, but if we ran a use-case that SAMPLED the OA
> buffer previously, then the OA_TAIL register is still pointing to an old
> value. When the timer callback runs, it ends up calculating a
> wrong/large number of available reports. Since we do a spinlock_irq_save
> and start processing a large number of reports, NMI watchdog fires and
> causes a crash.
>
> Start the timer only if SAMPLE_OA is specified.
> v2:
> - Drop SAMPLE OA check when appending samples (Ashutosh)
> - Prevent read if OA buffer is not being sampled
Reviewed-by: Ashutosh Dixit <ashutosh.dixit@xxxxxxxxx>
> Fixes: 00a7f0d7155c ("drm/i915/tgl: Add perf support on TGL")
> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@xxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_perf.c | 13 +++++--------
> 1 file changed, 5 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index c15bead2dac7..2fd2c13b76ac 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -595,7 +595,6 @@ static int append_oa_sample(struct i915_perf_stream *stream,
> {
> int report_size = stream->oa_buffer.format_size;
> struct drm_i915_perf_record_header header;
> - u32 sample_flags = stream->sample_flags;
>
> header.type = DRM_I915_PERF_RECORD_SAMPLE;
> header.pad = 0;
> @@ -609,10 +608,8 @@ static int append_oa_sample(struct i915_perf_stream *stream,
> return -EFAULT;
> buf += sizeof(header);
>
> - if (sample_flags & SAMPLE_OA_REPORT) {
> - if (copy_to_user(buf, report, report_size))
> - return -EFAULT;
> - }
> + if (copy_to_user(buf, report, report_size))
> + return -EFAULT;
>
> (*offset) += header.size;
>
> @@ -2669,7 +2666,7 @@ static void i915_oa_stream_enable(struct i915_perf_stream *stream)
>
> stream->perf->ops.oa_enable(stream);
>
> - if (stream->periodic)
> + if (stream->sample_flags & SAMPLE_OA_REPORT)
> hrtimer_start(&stream->poll_check_timer,
> ns_to_ktime(stream->poll_oa_period),
> HRTIMER_MODE_REL_PINNED);
> @@ -2732,7 +2729,7 @@ static void i915_oa_stream_disable(struct i915_perf_stream *stream)
> {
> stream->perf->ops.oa_disable(stream);
>
> - if (stream->periodic)
> + if (stream->sample_flags & SAMPLE_OA_REPORT)
> hrtimer_cancel(&stream->poll_check_timer);
> }
>
> @@ -3015,7 +3012,7 @@ static ssize_t i915_perf_read(struct file *file,
> * disabled stream as an error. In particular it might otherwise lead
> * to a deadlock for blocking file descriptors...
> */
> - if (!stream->enabled)
> + if (!stream->enabled || !(stream->sample_flags & SAMPLE_OA_REPORT))
> return -EIO;
>
> if (!(file->f_flags & O_NONBLOCK)) {
> --
> 2.20.1
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
