[Intel-gfx] [PATCH v9 09/11] drm/i915: add dev.i915.oa_max_sample_rate sysctl

[Robert Bragg]

On Wed, Nov 9, 2016 at 7:52 PM, Matthew Auld <matthew.william.auld at gmail.com
> wrote:

> On 7 November 2016 at 19:49, Robert Bragg <robert at sixbynine.org> 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 <robert at sixbynine.org>
> > ---
> >  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",
> This line is getting a little too long.
>

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.



>
> Reviewed-by: Matthew Auld <matthew.auld at intel.com>
>

thanks
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/intel-gfx/attachments/20161110/2f772d08/attachment-0001.html>