[Intel-gfx] [PATCH v2] drm/i915/perf: rate limit spurious oa report notice

[Matthew Auld]

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 at sixbynine.org>
> ---
>  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 at intel.com>