| Message ID | 20210302000141.63020-1-umesh.nerlige.ramappa@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | i915/perf: Start hrtimer only if sampling the OA buffer | expand |
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@intel.com> > Fixes: 00a7f0d7155c ("drm/i915/tgl: Add perf support on TGL") > Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> > --- > 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 >
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)) {
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 Fixes: 00a7f0d7155c ("drm/i915/tgl: Add perf support on TGL") Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> --- drivers/gpu/drm/i915/i915_perf.c | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-)