[PATCH 0/3] Some minor i915-perf prep changes

[PATCH 0/3] Some minor i915-perf prep changes

[Robert Bragg]

A small set of i915 perf changes that could ideally land before the gen8+
patches I hope to send out soon.

These are based on top of the gen7 tail pointer race workaround changes that
were sent out recently.

Robert Bragg (3):
  drm/i915/perf: improve invalid OA format debug message
  drm/i915/perf: better pipeline aged/aging tail updates
  drm/i915/perf: rate limit spurious oa report notice

 drivers/gpu/drm/i915/i915_perf.c | 50 +++++++++++++++++++++++-----------------
 1 file changed, 29 insertions(+), 21 deletions(-)

-- 
2.11.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/3] drm/i915/perf: improve invalid OA format debug message

[Robert Bragg]

A minor improvement to debugging output

Signed-off-by: Robert Bragg <robert@sixbynine.org>
---
 drivers/gpu/drm/i915/i915_perf.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 383b5769a851..19d0e4222974 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -1898,11 +1898,13 @@ static int read_properties_unlocked(struct drm_i915_private *dev_priv,
 			break;
 		case DRM_I915_PERF_PROP_OA_FORMAT:
 			if (value == 0 || value >= I915_OA_FORMAT_MAX) {
-				DRM_DEBUG("Invalid OA report format\n");
+				DRM_DEBUG("Out-of-range OA report format %llu\n",
+					  value);
 				return -EINVAL;
 			}
 			if (!dev_priv->perf.oa.oa_formats[value].size) {
-				DRM_DEBUG("Invalid OA report format\n");
+				DRM_DEBUG("Unsupported OA report format %llu\n",
+					  value);
 				return -EINVAL;
 			}
 			props->oa_format = value;
-- 
2.11.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/3] drm/i915/perf: better pipeline aged/aging tail updates

[Robert Bragg]

This updates the tail pointer race workaround handling to updating the
'aged' pointer before looking to start aging a new one. There's the
possibility that there is already new data available and so we can
immediately start aging a new pointer without having to first wait for a
later hrtimer callback (and then another to age).

Signed-off-by: Robert Bragg <robert@sixbynine.org>
---
 drivers/gpu/drm/i915/i915_perf.c | 41 ++++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 19d0e4222974..d04ebaa8406e 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -391,6 +391,29 @@ static bool gen7_oa_buffer_check_unlocked(struct drm_i915_private *dev_priv)
 
 	now = ktime_get_mono_fast_ns();
 
+	/* Update the aged tail
+	 *
+	 * Flip the tail pointer available for read()s once the aging tail is
+	 * old enough to trust that the corresponding data will be visible to
+	 * the CPU...
+	 *
+	 * Do this before updating the aging pointer in case we may be able to
+	 * immediately start aging a new pointer too (if new data has become
+	 * available) without needing to wait for a later hrtimer callback.
+	 */
+	if (aging_tail != INVALID_TAIL_PTR &&
+	    ((now - dev_priv->perf.oa.oa_buffer.aging_timestamp) >
+	     OA_TAIL_MARGIN_NSEC)) {
+		aged_idx ^= 1;
+		dev_priv->perf.oa.oa_buffer.aged_tail_idx = aged_idx;
+
+		aged_tail = aging_tail;
+
+		/* Mark that we need a new pointer to start aging... */
+		dev_priv->perf.oa.oa_buffer.tails[!aged_idx].offset = INVALID_TAIL_PTR;
+		aging_tail = INVALID_TAIL_PTR;
+	}
+
 	/* Update the aging tail
 	 *
 	 * We throttle aging tail updates until we have a new tail that
@@ -420,24 +443,6 @@ static bool gen7_oa_buffer_check_unlocked(struct drm_i915_private *dev_priv)
 		}
 	}
 
-	/* Update the aged tail
-	 *
-	 * Flip the tail pointer available for read()s once the aging tail is
-	 * old enough to trust that the corresponding data will be visible to
-	 * the CPU...
-	 */
-	if (aging_tail != INVALID_TAIL_PTR &&
-	    ((now - dev_priv->perf.oa.oa_buffer.aging_timestamp) >
-	     OA_TAIL_MARGIN_NSEC)) {
-		aged_idx ^= 1;
-		dev_priv->perf.oa.oa_buffer.aged_tail_idx = aged_idx;
-
-		aged_tail = aging_tail;
-
-		/* Mark that we need a new pointer to start aging... */
-		dev_priv->perf.oa.oa_buffer.tails[!aged_idx].offset = INVALID_TAIL_PTR;
-	}
-
 	spin_unlock_irqrestore(&dev_priv->perf.oa.oa_buffer.ptr_lock, flags);
 
 	return aged_tail == INVALID_TAIL_PTR ?
-- 
2.11.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/3] drm/i915/perf: rate limit spurious oa report notice

[Robert Bragg]

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.

Signed-off-by: Robert Bragg <robert@sixbynine.org>
---
 drivers/gpu/drm/i915/i915_perf.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index d04ebaa8406e..a901bcd80263 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 (printk_ratelimit())
+				DRM_NOTE("Skipping spurious, invalid OA report\n");
 			continue;
 		}
 
-- 
2.11.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for Some minor i915-perf prep changes

[Patchwork]

== Series Details ==

Series: Some minor i915-perf prep changes
URL   : https://patchwork.freedesktop.org/series/20073/
State : success

== Summary ==

Series 20073v1 Some minor i915-perf prep changes
https://patchwork.freedesktop.org/api/1.0/series/20073/revisions/1/mbox/

fi-bdw-5557u     total:253  pass:242  dwarn:0   dfail:0   fail:0   skip:11 
fi-bsw-n3050     total:253  pass:214  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:253  pass:234  dwarn:0   dfail:0   fail:0   skip:19 
fi-bxt-t5700     total:83   pass:70   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:253  pass:226  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:253  pass:222  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:253  pass:237  dwarn:0   dfail:0   fail:0   skip:16 
fi-hsw-4770r     total:253  pass:237  dwarn:0   dfail:0   fail:0   skip:16 
fi-ilk-650       total:253  pass:203  dwarn:0   dfail:0   fail:0   skip:50 
fi-ivb-3520m     total:253  pass:235  dwarn:0   dfail:0   fail:0   skip:18 
fi-ivb-3770      total:253  pass:235  dwarn:0   dfail:0   fail:0   skip:18 
fi-kbl-7500u     total:253  pass:235  dwarn:0   dfail:0   fail:0   skip:18 
fi-skl-6260u     total:253  pass:243  dwarn:0   dfail:0   fail:0   skip:10 
fi-skl-6700hq    total:253  pass:236  dwarn:0   dfail:0   fail:0   skip:17 
fi-skl-6700k     total:253  pass:231  dwarn:4   dfail:0   fail:0   skip:18 
fi-skl-6770hq    total:253  pass:243  dwarn:0   dfail:0   fail:0   skip:10 
fi-snb-2520m     total:253  pass:225  dwarn:0   dfail:0   fail:0   skip:28 
fi-snb-2600      total:253  pass:224  dwarn:0   dfail:0   fail:0   skip:29 

bf89ec45d0822835b03910371ac0baf46c4efa2d drm-tip: 2017y-02m-22d-14h-30m-04s UTC integration manifest
7c46d4c drm/i915/perf: improve invalid OA format debug message

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3933/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/3] drm/i915/perf: improve invalid OA format debug message

[Matthew Auld]

On 22 February 2017 at 15:25, Robert Bragg <robert@sixbynine.org> wrote:
> A minor improvement to debugging output
>
> Signed-off-by: Robert Bragg <robert@sixbynine.org>
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/3] drm/i915/perf: better pipeline aged/aging tail updates

[Matthew Auld]

On 22 February 2017 at 15:25, Robert Bragg <robert@sixbynine.org> wrote:
> This updates the tail pointer race workaround handling to updating the
> 'aged' pointer before looking to start aging a new one. There's the
> possibility that there is already new data available and so we can
> immediately start aging a new pointer without having to first wait for a
> later hrtimer callback (and then another to age).
>
> Signed-off-by: Robert Bragg <robert@sixbynine.org>
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915/perf: rate limit spurious oa report notice

[Matthew Auld]

On 22 February 2017 at 15:25, Robert Bragg <robert@sixbynine.org> 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.
>
> Signed-off-by: Robert Bragg <robert@sixbynine.org>
Reviewed-by: Matthew Auld <matthew.auld@intel.com>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915/perf: rate limit spurious oa report notice

[Chris Wilson]

On Tue, Feb 28, 2017 at 01:28:13PM +0000, Matthew Auld wrote:
> On 22 February 2017 at 15:25, Robert Bragg <robert@sixbynine.org> 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.
> >
> > Signed-off-by: Robert Bragg <robert@sixbynine.org>
> Reviewed-by: Matthew Auld <matthew.auld@intel.com>

printk_ratelimits emits a WARN when triggered, defeating the purpose of
using NOTE.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/3] drm/i915/perf: rate limit spurious oa report notice

[Robert Bragg]

On Tue, Feb 28, 2017 at 1:33 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Tue, Feb 28, 2017 at 01:28:13PM +0000, Matthew Auld wrote:
>> On 22 February 2017 at 15:25, Robert Bragg <robert@sixbynine.org> 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.
>> >
>> > Signed-off-by: Robert Bragg <robert@sixbynine.org>
>> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
>
> printk_ratelimits emits a WARN when triggered, defeating the purpose of
> using NOTE.

Hmm, that's a slightly awkward problem.

The warning comes from the common __ratelimit utility api that's used
for numerous things but by default will warn if the rate limiting
kicked in (once printing resumes again).

There's a RATELIMIT_MSG_ON_RELEASE flag that can be set to def the
warning until ratelimit_state_exit() is called (which looks optional
or at least the flag could also be cleared before calling it to avoid
any warnings).

In general printk_ratelimit() doesn't seem like an ideal mechanism for
throttling messages, even if they are warnings, since the rate limit
state is shared across orthogonal components, so maybe it's best
anyway to use a custom rate limit state.

Considering the _MSG_ON_RELEASE flag, I think I'll init some ratelimit
state in dev_priv->perf.oa just for this message, use
ratelimit_set_flags() to set _MSG_ON_RELEASE and have a corresponding
debug/note in i915_perf_fini after checking the rs->missed counter.

Actually at this point I'm slightly doubting whether having a warning
for throttling is that terrible in this case. Although I tend to think
it could be good to have dedicated ratelimit state here, there
conceptually is a point a which a visible warning could be appropriate
if we're really seeing a lot of these spurious reports. It's a grey
area since it's ok as note if it's rare/intermittent, but somthing's
maybe gone wrong if we see *lots* of these. hmm.

Incidentally, I suppose this same observation is relevant to many of
the DRM_*_RATELIMITED macros too - there will be an inconsistent level
used to notify about any throttling?

Br,
- Robert

> -Chris
>
> --
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Robert Bragg]

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@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);
+	}
+
 	unregister_sysctl_table(dev_priv->perf.sysctl_header);
 
 	memset(&dev_priv->perf.oa.ops, 0, sizeof(dev_priv->perf.oa.ops));
-- 
2.12.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for Some minor i915-perf prep changes (rev2)

[Patchwork]

== Series Details ==

Series: Some minor i915-perf prep changes (rev2)
URL   : https://patchwork.freedesktop.org/series/20073/
State : failure

== Summary ==

Series 20073v2 Some minor i915-perf prep changes
https://patchwork.freedesktop.org/api/1.0/series/20073/revisions/2/mbox/

Test drv_getparams_basic:
        Subgroup basic-eu-total:
                pass       -> INCOMPLETE (fi-byt-j1900)
Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                pass       -> DMESG-WARN (fi-snb-2600)

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 461s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time: 461s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 586s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 531s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 583s
fi-byt-j1900     total:3    pass:2    dwarn:0   dfail:0   fail:0   skip:0   time: 0s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 501s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 446s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 430s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 444s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 516s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 496s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 488s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 487s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 610s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 491s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 533s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time: 461s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 547s
fi-snb-2600      total:278  pass:248  dwarn:1   dfail:0   fail:0   skip:29  time: 421s

fd27e1e4c9b5dc11966b4953432bd6e0510da308 drm-tip: 2017y-03m-24d-08h-39m-20s UTC integration manifest
7eb53856 drm/i915/perf: improve invalid OA format debug message

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4283/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [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@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@intel.com>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3] drm/i915/perf: rate limit spurious oa report notice

[Robert Bragg]

Instead of initializing and summarising the number of throttled messages in
the driver _init / _fini we now do this when opening / closing an OA stream.

--- >8 ---

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 known 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()
v3: (Matt) init and summarise with stream init/close not driver init/fini

Signed-off-by: Robert Bragg <robert@sixbynine.org>
---
 drivers/gpu/drm/i915/i915_drv.h  |  6 ++++++
 drivers/gpu/drm/i915/i915_perf.c | 28 +++++++++++++++++++++++++++-
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 51a410911d81..51bd6c6034bb 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2454,6 +2454,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 5738b99caa5b..3277a52ce98e 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;
 		}
 
@@ -913,6 +914,11 @@ static void i915_oa_stream_destroy(struct i915_perf_stream *stream)
 		oa_put_render_ctx_id(stream);
 
 	dev_priv->perf.oa.exclusive_stream = NULL;
+
+	if (dev_priv->perf.oa.spurious_report_rs.missed) {
+		DRM_NOTE("%d spurious OA report notices suppressed due to ratelimiting\n",
+			 dev_priv->perf.oa.spurious_report_rs.missed);
+	}
 }
 
 static void gen7_init_oa_buffer(struct drm_i915_private *dev_priv)
@@ -1268,6 +1274,26 @@ static int i915_oa_stream_init(struct i915_perf_stream *stream,
 		return -EINVAL;
 	}
 
+	/* We set up some ratelimit state to potentially throttle any _NOTES
+	 * about spurious, invalid OA reports which we don't forward to
+	 * userspace.
+	 *
+	 * The initialization is associated with opening the stream (not driver
+	 * init) considering we print a _NOTE about any throttling when closing
+	 * the stream instead of waiting until driver _fini which no one would
+	 * ever see.
+	 *
+	 * Using the same limiting factors as printk_ratelimit()
+	 */
+	ratelimit_state_init(&dev_priv->perf.oa.spurious_report_rs,
+			     5 * HZ, 10);
+	/* Since we use a DRM_NOTE for spurious reports it would be
+	 * inconsistent to let __ratelimit() automatically print a warning for
+	 * throttling.
+	 */
+	ratelimit_set_flags(&dev_priv->perf.oa.spurious_report_rs,
+			    RATELIMIT_MSG_ON_RELEASE);
+
 	stream->sample_size = sizeof(struct drm_i915_perf_record_header);
 
 	format_size = dev_priv->perf.oa.oa_formats[props->oa_format].size;
-- 
2.12.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915/perf: rate limit spurious oa report notice

[Matthew Auld]

On 04/05, Robert Bragg wrote:
> Instead of initializing and summarising the number of throttled messages in
> the driver _init / _fini we now do this when opening / closing an OA stream.
> 
> --- >8 ---
> 
> 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 known 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()
> v3: (Matt) init and summarise with stream init/close not driver init/fini
> 
> Signed-off-by: Robert Bragg <robert@sixbynine.org>
Reviewed-by: Matthew Auld <matthew.auld@intel.com>

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for Some minor i915-perf prep changes (rev3)

[Patchwork]

== Series Details ==

Series: Some minor i915-perf prep changes (rev3)
URL   : https://patchwork.freedesktop.org/series/20073/
State : success

== Summary ==

Series 20073v3 Some minor i915-perf prep changes
https://patchwork.freedesktop.org/api/1.0/series/20073/revisions/3/mbox/

Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> FAIL       (fi-skl-6700hq) fdo#99739

fdo#99739 https://bugs.freedesktop.org/show_bug.cgi?id=99739

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 433s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time: 431s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time: 572s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 506s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 548s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time: 489s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 480s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 411s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 404s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 420s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 488s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 461s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 452s
fi-kbl-7560u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 571s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 451s
fi-skl-6700hq    total:278  pass:260  dwarn:0   dfail:0   fail:1   skip:17  time: 572s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 463s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 489s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time: 429s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 529s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 404s

39a0f48c8bc7c528cc705016dafa08a9dedfd36b drm-tip: 2017y-04m-05d-13h-22m-47s UTC integration manifest
4fd9f5e drm/i915/perf: improve invalid OA format debug message

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4410/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx