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@xxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
