Re: [PATCH v9 09/11] drm/i915: add dev.i915.oa_max_sample_rate sysctl

From: sourab gupta <sourab.gupta@intel.com>
To: Robert Bragg <robert@sixbynine.org>
Cc: "dri-devel@lists.freedesktop.org"
	<dri-devel@lists.freedesktop.org>,
	David Airlie <airlied@linux.ie>,
	"intel-gfx@lists.freedesktop.org"
	<intel-gfx@lists.freedesktop.org>,
	"Vetter, Daniel" <daniel.vetter@intel.com>
Subject: Re: [PATCH v9 09/11] drm/i915: add dev.i915.oa_max_sample_rate sysctl
Date: Tue, 08 Nov 2016 17:44:02 +0530	[thread overview]
Message-ID: <1478607242.3013.2.camel@sourab-desktop> (raw)
In-Reply-To: <CAMou1-3yP9Wpks7gmgqvW3JRKmX7DY38mwm_Su4K5YOoO3-Wyw@mail.gmail.com>

On Tue, 2016-11-08 at 03:47 -0800, Robert Bragg wrote:
> 
> 
> On Tue, Nov 8, 2016 at 6:19 AM, sourab gupta <sourab.gupta@intel.com>
> wrote:
>         On Mon, 2016-11-07 at 11:49 -0800, Robert Bragg 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@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;
>         There's no check for 'oa_sample_rate_hard_limit' anywhere
>         below.
> 
> 
> It's in the struct ctl_table oa_table[] declaration of the
> "oa_max_sample_rate" paramater, assigned to .extra2 which is
> referenced by the proc_dointvec_minmax validation handler for the
> parameter.
> 
Ok. Seems fine then.
>  
>         
>         > +
>         > +/* 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);
>         
>         I assume now that there'll be a platform specific check for
>         80ull, while
>         programming oa_period, for subquent Gen8+ platforms, which
>         should be
>         fine.
> 
> 
> Yeah, this code will need adapting for gen9+. I guess we'll change it
> to work in terms of ((2<<exp) * NSEC_PER_SEC) / timestamp_frequency.
> 
Seems reasonable.
>  
>         
>         > +
>         > +                     /* 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",
>         > +
>          i915_oa_max_sample_rate);
>         >                               return -EACCES;
>         >                       }
>         >
>         > @@ -1481,6 +1511,15 @@ static struct ctl_table oa_table[] =
>         {
>         >        .extra1 = &zero,
>         >        .extra2 = &one,
>         >        },
>         > +     {
>         > +      .procname = "oa_max_sample_rate",
>         > +      .data = &i915_oa_max_sample_rate,
>         > +      .maxlen = sizeof(i915_oa_max_sample_rate),
>         > +      .mode = 0644,
>         > +      .proc_handler = proc_dointvec_minmax,
>         > +      .extra1 = &zero,
>         > +      .extra2 = &oa_sample_rate_hard_limit,
>         > +      },
>         >       {}
>         >  };
>         >
>         
The patch looks good to me.
Reviewed-by: Sourab Gupta <sourab.gupta@intel.com>
>         
>         
> 
> 


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