Re: [Intel-gfx] [PATCH] i915/perf: Start hrtimer only if sampling the OA buffer

["Dixit, Ashutosh" <ashutosh.dixit@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