From: "Dixit, Ashutosh" <ashutosh.dixit@intel.com>
To: "Nerlige Ramappa, Umesh" <umesh.nerlige.ramappa@intel.com>
Cc: "intel-gfx@lists.freedesktop.org" <intel-gfx@lists.freedesktop.org>
Subject: Re: [Intel-gfx] [PATCH] i915/perf: Start hrtimer only if sampling the OA buffer
Date: Mon, 01 Mar 2021 16:26:39 -0800 [thread overview]
Message-ID: <878s768kow.wl-ashutosh.dixit@intel.com> (raw)
In-Reply-To: <20210302000141.63020-1-umesh.nerlige.ramappa@intel.com>
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
>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
next prev parent reply other threads:[~2021-03-02 0:26 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-02 0:01 [Intel-gfx] [PATCH] i915/perf: Start hrtimer only if sampling the OA buffer Umesh Nerlige Ramappa
2021-03-02 0:26 ` Dixit, Ashutosh [this message]
2021-03-02 1:11 ` [Intel-gfx] ✓ Fi.CI.BAT: success for " Patchwork
2021-03-02 1:17 ` [Intel-gfx] ✓ Fi.CI.IGT: " Patchwork
2021-03-05 21:09 [Intel-gfx] [PATCH] " Umesh Nerlige Ramappa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=878s768kow.wl-ashutosh.dixit@intel.com \
--to=ashutosh.dixit@intel.com \
--cc=intel-gfx@lists.freedesktop.org \
--cc=umesh.nerlige.ramappa@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).