On 03/23, Robert Bragg wrote:
> This change is pre-emptively aiming to avoid a potential cause of kernel
> logging noise in case some condition were to result in us seeing invalid
> OA reports.
>
> The workaround for the OA unit's tail pointer race condition is what
> avoids the primary know cause of invalid reports being seen and with
> that in place we aren't expecting to see this notice but it can't be
> entirely ruled out.
>
> Just in case some condition does lead to the notice then it's likely
> that it will be triggered repeatedly while attempting to append a
> sequence of reports and depending on the configured OA sampling
> frequency that might be a large number of repeat notices.
>
> v2: (Chris) avoid inconsistent warning on throttle with
> printk_ratelimit()
>
> Signed-off-by: Robert Bragg <robert@xxxxxxxxxxxxx>
> ---
> drivers/gpu/drm/i915/i915_drv.h | 6 ++++++
> drivers/gpu/drm/i915/i915_perf.c | 17 ++++++++++++++++-
> 2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index a7b49cad6ab2..a7986c0c29ad 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2471,6 +2471,12 @@ struct drm_i915_private {
> wait_queue_head_t poll_wq;
> bool pollin;
>
> + /**
> + * For rate limiting any notifications of spurious
> + * invalid OA reports
> + */
> + struct ratelimit_state spurious_report_rs;
> +
> bool periodic;
> int period_exponent;
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index c09a7c9b61d9..36d07ca68029 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -632,7 +632,8 @@ static int gen7_append_oa_reports(struct i915_perf_stream *stream,
> * copying it to userspace...
> */
> if (report32[0] == 0) {
> - DRM_NOTE("Skipping spurious, invalid OA report\n");
> + if (__ratelimit(&dev_priv->perf.oa.spurious_report_rs))
> + DRM_NOTE("Skipping spurious, invalid OA report\n");
> continue;
> }
>
> @@ -2144,6 +2145,15 @@ void i915_perf_init(struct drm_i915_private *dev_priv)
> if (!IS_HASWELL(dev_priv))
> return;
>
> + /* Using the same limiting factors as printk_ratelimit() */
> + ratelimit_state_init(&dev_priv->perf.oa.spurious_report_rs,
> + 5 * HZ, 10);
> + /* We use a DRM_NOTE for spurious reports so it would be
> + * inconsistent to print a warning for throttling.
> + */
> + ratelimit_set_flags(&dev_priv->perf.oa.spurious_report_rs,
> + RATELIMIT_MSG_ON_RELEASE);
> +
> hrtimer_init(&dev_priv->perf.oa.poll_check_timer,
> CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> dev_priv->perf.oa.poll_check_timer.function = oa_poll_check_timer_cb;
> @@ -2182,6 +2192,11 @@ void i915_perf_fini(struct drm_i915_private *dev_priv)
> if (!dev_priv->perf.initialized)
> return;
>
> + if (dev_priv->perf.oa.spurious_report_rs.missed) {
> + DRM_NOTE("%d spurious OA report notices suppressed due to ratelimiting",
> + dev_priv->perf.oa.spurious_report_rs.missed);
Missing newline for DRM_NOTE. I would have expected to see this notice
when the stream is closed.
Either way seems to make sense:
Reviewed-by: Matthew Auld <matthew.auld@xxxxxxxxx>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
