Re: [PATCH v2 1/4] drm/i915/perf: reuse timestamp frequency from device info

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Nov 13, 2017 at 07:03:30PM +0000, Matthew Auld wrote:
> On 13 November 2017 at 18:18, Lionel Landwerlin
> <lionel.g.landwerlin@intel.com> wrote:
> > Now that we have this stored in the device info, we can drop it from perf
> > part of the driver.
> >
> > Note that this requires to init perf after we've computed the frequency,
> > hence why we move i915_perf_init() from i915_driver_init_early() to after
> > intel_device_info_runtime_init().
> >
> > v2: Use udiv_u64 (Chris)
> >
> > Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c  |  4 ++--
> >  drivers/gpu/drm/i915/i915_drv.h  |  1 -
> >  drivers/gpu/drm/i915/i915_perf.c | 32 +++-----------------------------
> >  3 files changed, 5 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> > index 42813f4247e2..8ce95fd9d313 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -931,8 +931,6 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
> >
> >         intel_detect_preproduction_hw(dev_priv);
> >
> > -       i915_perf_init(dev_priv);
> > -
> >         return 0;
> >
> >  err_irq:
> > @@ -1096,6 +1094,8 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv)
> >
> >         intel_sanitize_options(dev_priv);
> >
> > +       i915_perf_init(dev_priv);
> > +
> If we are moving this to init_hw, we should move i915_perf_fini to
> i915_driver_cleanup_hw, to keep the symmetry.
> 
> >         ret = i915_ggtt_probe_hw(dev_priv);
> >         if (ret)
> >                 return ret;
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index b538df740ac3..036e197f6824 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -2619,7 +2619,6 @@ struct drm_i915_private {
> >
> >                         bool periodic;
> >                         int period_exponent;
> > -                       int timestamp_frequency;
> >
> >                         struct i915_oa_config test_config;
> >
> > diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> > index 00be015e01df..292ad3e2c307 100644
> > --- a/drivers/gpu/drm/i915/i915_perf.c
> > +++ b/drivers/gpu/drm/i915/i915_perf.c
> > @@ -2692,7 +2692,7 @@ i915_perf_open_ioctl_locked(struct drm_i915_private *dev_priv,
> >  static u64 oa_exponent_to_ns(struct drm_i915_private *dev_priv, int exponent)
> >  {
> >         return div_u64(1000000000ULL * (2ULL << exponent),
> > -                      dev_priv->perf.oa.timestamp_frequency);
> > +                      INTEL_INFO(dev_priv)->cs_timestamp_frequency);
> s/div_u64/div64_u64/

I wonder if these u64/u64 divisisions are actually necessary.
Is u32 not good enough for the timestamp frequency? I see a lot
of trailing zeroes on the values below...

> 
> >  }
> >
> >  /**
> > @@ -3415,8 +3415,6 @@ static struct ctl_table dev_root[] = {
> >   */
> >  void i915_perf_init(struct drm_i915_private *dev_priv)
> >  {
> > -       dev_priv->perf.oa.timestamp_frequency = 0;
> > -
> >         if (IS_HASWELL(dev_priv)) {
> >                 dev_priv->perf.oa.ops.is_valid_b_counter_reg =
> >                         gen7_is_valid_b_counter_addr;
> > @@ -3432,8 +3430,6 @@ void i915_perf_init(struct drm_i915_private *dev_priv)
> >                 dev_priv->perf.oa.ops.oa_hw_tail_read =
> >                         gen7_oa_hw_tail_read;
> >
> > -               dev_priv->perf.oa.timestamp_frequency = 12500000;
> > -
> >                 dev_priv->perf.oa.oa_formats = hsw_oa_formats;
> >         } else if (i915_modparams.enable_execlists) {
> >                 /* Note: that although we could theoretically also support the
> > @@ -3477,23 +3473,6 @@ void i915_perf_init(struct drm_i915_private *dev_priv)
> >
> >                                 dev_priv->perf.oa.gen8_valid_ctx_bit = (1<<16);
> >                         }
> > -
> > -                       switch (dev_priv->info.platform) {
> > -                       case INTEL_BROADWELL:
> > -                               dev_priv->perf.oa.timestamp_frequency = 12500000;
> > -                               break;
> > -                       case INTEL_BROXTON:
> > -                       case INTEL_GEMINILAKE:
> > -                               dev_priv->perf.oa.timestamp_frequency = 19200000;
> > -                               break;
> > -                       case INTEL_SKYLAKE:
> > -                       case INTEL_KABYLAKE:
> > -                       case INTEL_COFFEELAKE:
> > -                               dev_priv->perf.oa.timestamp_frequency = 12000000;
> > -                               break;
> > -                       default:
> > -                               break;
> > -                       }
> >                 } else if (IS_GEN10(dev_priv)) {
> >                         dev_priv->perf.oa.ops.is_valid_b_counter_reg =
> >                                 gen7_is_valid_b_counter_addr;
> > @@ -3509,15 +3488,10 @@ void i915_perf_init(struct drm_i915_private *dev_priv)
> >                         dev_priv->perf.oa.ctx_flexeu0_offset = 0x3de;
> >
> >                         dev_priv->perf.oa.gen8_valid_ctx_bit = (1<<16);
> > -
> > -                       /* Default frequency, although we need to read it from
> > -                        * the register as it might vary between parts.
> > -                        */
> > -                       dev_priv->perf.oa.timestamp_frequency = 12000000;
> >                 }
> >         }
> >
> > -       if (dev_priv->perf.oa.timestamp_frequency) {
> > +       if (dev_priv->perf.oa.ops.enable_metric_set) {
> Maybe sprinkle a GEM_BUG_ON(!dev_priv->perf.oa.timestamp_frequency) or
> something?
> 
> Otherwise:
> 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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx