| Message ID | 20171113181902.12411-2-lionel.g.landwerlin@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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/ > } > > /** > @@ -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>
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
Quoting Ville Syrjälä (2017-11-13 20:53:39) > On Mon, Nov 13, 2017 at 07:03:30PM +0000, Matthew Auld wrote: > > On 13 November 2017 at 18:18, Lionel Landwerlin > > > 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... Especially when the new ABI introduced to expose the frequency is a s32... I suspect we may want to change that to KHz to have sufficient headroom for wacky GPUs? -Chris
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); + 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); } /** @@ -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) { hrtimer_init(&dev_priv->perf.oa.poll_check_timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL); dev_priv->perf.oa.poll_check_timer.function = oa_poll_check_timer_cb; @@ -3528,7 +3502,7 @@ void i915_perf_init(struct drm_i915_private *dev_priv) spin_lock_init(&dev_priv->perf.oa.oa_buffer.ptr_lock); oa_sample_rate_hard_limit = - dev_priv->perf.oa.timestamp_frequency / 2; + div_u64(INTEL_INFO(dev_priv)->cs_timestamp_frequency, 2); dev_priv->perf.sysctl_header = register_sysctl_table(dev_root); mutex_init(&dev_priv->perf.metrics_lock);
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(-)