<div dir="ltr"> <div class="gmail_extra"> <div class="gmail_quote">On Wed, Nov 9, 2016 at 7:52 PM, Matthew Auld <<a href="mailto:matthew.william.auld@gmail.com" target="_blank">matthew.william.auld@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="m_-6573997344553263611HOEnZb"><div class="m_-6573997344553263611h5">On 7 November 2016 at 19:49, Robert Bragg <<a href="mailto:robert@sixbynine.org" target="_blank">robert@sixbynine.org</a>> wrote: > The maximum OA sampling frequency is now configurable via a > dev.i915.oa_max_sample_rate sysctl parameter. > > Following the precedent set by perf's similar > kernel.perf_event_max_sample_rate the default maximum rate is 100000Hz > > Signed-off-by: Robert Bragg <<a href="mailto:robert@sixbynine.org" target="_blank">robert@sixbynine.org</a>> > --- > drivers/gpu/drm/i915/i915_perf.c | 61 ++++++++++++++++++++++++++++++++-------- > 1 file changed, 50 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c > index e51c1d8..1a87fe9 100644 > --- a/drivers/gpu/drm/i915/i915_perf.c > +++ b/drivers/gpu/drm/i915/i915_perf.c > @@ -82,6 +82,21 @@ static u32 i915_perf_stream_paranoid = true; > #define INVALID_CTX_ID 0xffffffff > > > +/* For sysctl proc_dointvec_minmax of i915_oa_max_sample_rate > + * > + * 160ns is the smallest sampling period we can theoretically program the OA > + * unit with on Haswell, corresponding to 6.25MHz. > + */ > +static int oa_sample_rate_hard_limit = 6250000; > + > +/* Theoretically we can program the OA unit to sample every 160ns but don't > + * allow that by default unless root... > + * > + * The default threshold of 100000Hz is based on perf's similar > + * kernel.perf_event_max_sample_rate sysctl parameter. > + */ > +static u32 i915_oa_max_sample_rate = 100000; > + > /* XXX: beware if future OA HW adds new report formats that the current > * code assumes all reports have a power-of-two size and ~(size - 1) can > * be used as a mask to align the OA tail pointer. > @@ -1314,6 +1329,7 @@ static int read_properties_unlocked(struct drm_i915_private *dev_priv, > } > > for (i = 0; i < n_props; i++) { > + u64 oa_period, oa_freq_hz; > u64 id, value; > int ret; > > @@ -1359,21 +1375,35 @@ static int read_properties_unlocked(struct drm_i915_private *dev_priv, > return -EINVAL; > } > > - /* NB: The exponent represents a period as follows: > - * > - * 80ns * 2^(period_exponent + 1) > - * > - * Theoretically we can program the OA unit to sample > + /* Theoretically we can program the OA unit to sample > * every 160ns but don't allow that by default unless > * root. > * > - * Referring to perf's > - * kernel.perf_event_max_sample_rate for a precedent > - * (100000 by default); with an OA exponent of 6 we get > - * a period of 10.240 microseconds -just under 100000Hz > + * On Haswell the period is derived from the exponent > + * as: > + * > + * period = 80ns * 2^(exponent + 1) > + */ > + BUILD_BUG_ON(sizeof(oa_period) != 8); > + oa_period = 80ull * (2ull << value); > + > + /* This check is primarily to ensure that oa_period <= > + * UINT32_MAX (before passing to do_div which only > + * accepts a u32 denominator), but we can also skip > + * checking anything < 1Hz which implicitly can't be > + * limited via an integer oa_max_sample_rate. > */ > - if (value < 6 && !capable(CAP_SYS_ADMIN)) { > - DRM_ERROR("Minimum OA sampling exponent is 6 without root privileges\n"); > + if (oa_period <= NSEC_PER_SEC) { > + u64 tmp = NSEC_PER_SEC; > + do_div(tmp, oa_period); > + oa_freq_hz = tmp; > + } else > + oa_freq_hz = 0; > + > + if (oa_freq_hz > i915_oa_max_sample_rate && > + !capable(CAP_SYS_ADMIN)) { > + DRM_ERROR("OA exponent would exceed the max sampling frequency (sysctl dev.i915.oa_max_sample_rate) %uHz without root privileges\n", </div></div>This line is getting a little too long. </blockquote><div> </div><div>yeah, though the coding style has the exception for string constants so they remain grepable which can be useful too. Could maybe remove the bracketed naming of the corresponding sysctl parameter, though it does seem like pertinent/useful info if someone were to hit this limit. </div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> Reviewed-by: Matthew Auld <<a href="mailto:matthew.auld@intel.com" target="_blank">matthew.auld@intel.com</a>> </blockquote></div> thanks </div></div>