All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/4] drm/i915: some perf cleanups (& fixes!)
@ 2017-11-13 18:18 Lionel Landwerlin
  2017-11-13 18:18 ` [PATCH v2 1/4] drm/i915/perf: reuse timestamp frequency from device info Lionel Landwerlin
                   ` (5 more replies)
  0 siblings, 6 replies; 13+ messages in thread
From: Lionel Landwerlin @ 2017-11-13 18:18 UTC (permalink / raw)
  To: intel-gfx

Another round of issues reported mostly by Matthew.

Thanks,

Lionel Landwerlin (4):
  drm/i915/perf: reuse timestamp frequency from device info
  drm/i915/perf: replace .reg accesses with i915_mmio_reg_offset
  drm/i915: fix 64bit divide
  drm/i915/cnl: only divide up base frequency with crystal source

 drivers/gpu/drm/i915/i915_drv.c          |  4 +-
 drivers/gpu/drm/i915/i915_drv.h          |  1 -
 drivers/gpu/drm/i915/i915_perf.c         | 71 ++++++++++++--------------------
 drivers/gpu/drm/i915/intel_device_info.c | 23 +++++------
 4 files changed, 40 insertions(+), 59 deletions(-)

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 1/4] drm/i915/perf: reuse timestamp frequency from device info
  2017-11-13 18:18 [PATCH v2 0/4] drm/i915: some perf cleanups (& fixes!) Lionel Landwerlin
@ 2017-11-13 18:18 ` Lionel Landwerlin
  2017-11-13 19:03   ` Matthew Auld
  2017-11-13 18:19 ` [PATCH v2 2/4] drm/i915/perf: replace .reg accesses with i915_mmio_reg_offset Lionel Landwerlin
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 13+ messages in thread
From: Lionel Landwerlin @ 2017-11-13 18:18 UTC (permalink / raw)
  To: intel-gfx

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);
+
 	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);
-- 
2.15.0

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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 2/4] drm/i915/perf: replace .reg accesses with i915_mmio_reg_offset
  2017-11-13 18:18 [PATCH v2 0/4] drm/i915: some perf cleanups (& fixes!) Lionel Landwerlin
  2017-11-13 18:18 ` [PATCH v2 1/4] drm/i915/perf: reuse timestamp frequency from device info Lionel Landwerlin
@ 2017-11-13 18:19 ` Lionel Landwerlin
  2017-11-13 18:19 ` [PATCH v2 3/4] drm/i915: fix 64bit divide Lionel Landwerlin
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 13+ messages in thread
From: Lionel Landwerlin @ 2017-11-13 18:19 UTC (permalink / raw)
  To: intel-gfx

This replaces accesses to the reg field of the i915_reg_t structure
with the i915_mmio_reg_offset() inline function.

Suggested-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
---
 drivers/gpu/drm/i915/i915_perf.c | 39 ++++++++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 292ad3e2c307..cffab6fc39fa 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -3007,7 +3007,7 @@ static bool gen8_is_valid_flex_addr(struct drm_i915_private *dev_priv, u32 addr)
 	int i;
 
 	for (i = 0; i < ARRAY_SIZE(flex_eu_regs); i++) {
-		if (flex_eu_regs[i].reg == addr)
+		if (i915_mmio_reg_offset(flex_eu_regs[i]) == addr)
 			return true;
 	}
 	return false;
@@ -3015,38 +3015,47 @@ static bool gen8_is_valid_flex_addr(struct drm_i915_private *dev_priv, u32 addr)
 
 static bool gen7_is_valid_b_counter_addr(struct drm_i915_private *dev_priv, u32 addr)
 {
-	return (addr >= OASTARTTRIG1.reg && addr <= OASTARTTRIG8.reg) ||
-		(addr >= OAREPORTTRIG1.reg && addr <= OAREPORTTRIG8.reg) ||
-		(addr >= OACEC0_0.reg && addr <= OACEC7_1.reg);
+	return (addr >= i915_mmio_reg_offset(OASTARTTRIG1) &&
+		addr <= i915_mmio_reg_offset(OASTARTTRIG8)) ||
+		(addr >= i915_mmio_reg_offset(OAREPORTTRIG1) &&
+		 addr <= i915_mmio_reg_offset(OAREPORTTRIG8)) ||
+		(addr >= i915_mmio_reg_offset(OACEC0_0) &&
+		 addr <= i915_mmio_reg_offset(OACEC7_1));
 }
 
 static bool gen7_is_valid_mux_addr(struct drm_i915_private *dev_priv, u32 addr)
 {
-	return addr == HALF_SLICE_CHICKEN2.reg ||
-		(addr >= MICRO_BP0_0.reg && addr <= NOA_WRITE.reg) ||
-		(addr >= OA_PERFCNT1_LO.reg && addr <= OA_PERFCNT2_HI.reg) ||
-		(addr >= OA_PERFMATRIX_LO.reg && addr <= OA_PERFMATRIX_HI.reg);
+	return addr == i915_mmio_reg_offset(HALF_SLICE_CHICKEN2) ||
+		(addr >= i915_mmio_reg_offset(MICRO_BP0_0) &&
+		 addr <= i915_mmio_reg_offset(NOA_WRITE)) ||
+		(addr >= i915_mmio_reg_offset(OA_PERFCNT1_LO) &&
+		 addr <= i915_mmio_reg_offset(OA_PERFCNT2_HI)) ||
+		(addr >= i915_mmio_reg_offset(OA_PERFMATRIX_LO) &&
+		 addr <= i915_mmio_reg_offset(OA_PERFMATRIX_HI));
 }
 
 static bool gen8_is_valid_mux_addr(struct drm_i915_private *dev_priv, u32 addr)
 {
 	return gen7_is_valid_mux_addr(dev_priv, addr) ||
-		addr == WAIT_FOR_RC6_EXIT.reg ||
-		(addr >= RPM_CONFIG0.reg && addr <= NOA_CONFIG(8).reg);
+		addr == i915_mmio_reg_offset(WAIT_FOR_RC6_EXIT) ||
+		(addr >= i915_mmio_reg_offset(RPM_CONFIG0) &&
+		 addr <= i915_mmio_reg_offset(NOA_CONFIG(8)));
 }
 
 static bool gen10_is_valid_mux_addr(struct drm_i915_private *dev_priv, u32 addr)
 {
 	return gen8_is_valid_mux_addr(dev_priv, addr) ||
-		(addr >= OA_PERFCNT3_LO.reg && addr <= OA_PERFCNT4_HI.reg);
+		(addr >= i915_mmio_reg_offset(OA_PERFCNT3_LO) &&
+		 addr <= i915_mmio_reg_offset(OA_PERFCNT4_HI));
 }
 
 static bool hsw_is_valid_mux_addr(struct drm_i915_private *dev_priv, u32 addr)
 {
 	return gen7_is_valid_mux_addr(dev_priv, addr) ||
 		(addr >= 0x25100 && addr <= 0x2FF90) ||
-		(addr >= HSW_MBVID2_NOA0.reg && addr <= HSW_MBVID2_NOA9.reg) ||
-		addr == HSW_MBVID2_MISR0.reg;
+		(addr >= i915_mmio_reg_offset(HSW_MBVID2_NOA0) &&
+		 addr <= i915_mmio_reg_offset(HSW_MBVID2_NOA9)) ||
+		addr == i915_mmio_reg_offset(HSW_MBVID2_MISR0);
 }
 
 static bool chv_is_valid_mux_addr(struct drm_i915_private *dev_priv, u32 addr)
@@ -3061,14 +3070,14 @@ static uint32_t mask_reg_value(u32 reg, u32 val)
 	 * WaDisableSTUnitPowerOptimization workaround. Make sure the value
 	 * programmed by userspace doesn't change this.
 	 */
-	if (HALF_SLICE_CHICKEN2.reg == reg)
+	if (i915_mmio_reg_offset(HALF_SLICE_CHICKEN2) == reg)
 		val = val & ~_MASKED_BIT_ENABLE(GEN8_ST_PO_DISABLE);
 
 	/* WAIT_FOR_RC6_EXIT has only one bit fullfilling the function
 	 * indicated by its name and a bunch of selection fields used by OA
 	 * configs.
 	 */
-	if (WAIT_FOR_RC6_EXIT.reg == reg)
+	if (i915_mmio_reg_offset(WAIT_FOR_RC6_EXIT) == reg)
 		val = val & ~_MASKED_BIT_ENABLE(HSW_WAIT_FOR_RC6_EXIT_ENABLE);
 
 	return val;
-- 
2.15.0

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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 3/4] drm/i915: fix 64bit divide
  2017-11-13 18:18 [PATCH v2 0/4] drm/i915: some perf cleanups (& fixes!) Lionel Landwerlin
  2017-11-13 18:18 ` [PATCH v2 1/4] drm/i915/perf: reuse timestamp frequency from device info Lionel Landwerlin
  2017-11-13 18:19 ` [PATCH v2 2/4] drm/i915/perf: replace .reg accesses with i915_mmio_reg_offset Lionel Landwerlin
@ 2017-11-13 18:19 ` Lionel Landwerlin
  2017-11-13 18:44   ` Matthew Auld
  2017-11-13 18:19 ` [PATCH v2 4/4] drm/i915/cnl: only divide up base frequency with crystal source Lionel Landwerlin
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 13+ messages in thread
From: Lionel Landwerlin @ 2017-11-13 18:19 UTC (permalink / raw)
  To: intel-gfx

ERROR: "__udivdi3" [drivers/gpu/drm/i915/i915.ko] undefined!
ERROR: "__divdi3" [drivers/gpu/drm/i915/i915.ko] undefined!

We can also drop an if() as we divide by (value + 1) only if value is
not 0.

Fixes: dab9178333 ("drm/i915: expose command stream timestamp frequency to userspace")
Reported-by: Matthew Auld <matthew.auld@intel.com>
Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
---
 drivers/gpu/drm/i915/intel_device_info.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
index 78bf7374fbdd..992ae1bdfb3b 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -336,13 +336,12 @@ static u64 read_reference_ts_freq(struct drm_i915_private *dev_priv)
 
 	base_freq = ((ts_override & GEN9_TIMESTAMP_OVERRIDE_US_COUNTER_DIVIDER_MASK) >>
 		     GEN9_TIMESTAMP_OVERRIDE_US_COUNTER_DIVIDER_SHIFT) + 1;
-	base_freq *= 1000000;
+	base_freq *= 1000000ULL;
 
 	frac_freq = ((ts_override &
 		      GEN9_TIMESTAMP_OVERRIDE_US_COUNTER_DENOMINATOR_MASK) >>
 		     GEN9_TIMESTAMP_OVERRIDE_US_COUNTER_DENOMINATOR_SHIFT);
-	if (frac_freq != 0)
-		frac_freq = 1000000 / (frac_freq + 1);
+	frac_freq = div_u64(1000000ULL, frac_freq + 1);
 
 	return base_freq + frac_freq;
 }
@@ -360,7 +359,7 @@ static u64 read_timestamp_frequency(struct drm_i915_private *dev_priv)
 		 *      hclks." (through the “Clocking Configuration”
 		 *      (“CLKCFG”) MCHBAR register)
 		 */
-		return (dev_priv->rawclk_freq * 1000) / 16;
+		return div_u64(dev_priv->rawclk_freq * 1000ULL, 16);
 	} else if (INTEL_GEN(dev_priv) <= 8) {
 		/* PRMs say:
 		 *
-- 
2.15.0

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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v2 4/4] drm/i915/cnl: only divide up base frequency with crystal source
  2017-11-13 18:18 [PATCH v2 0/4] drm/i915: some perf cleanups (& fixes!) Lionel Landwerlin
                   ` (2 preceding siblings ...)
  2017-11-13 18:19 ` [PATCH v2 3/4] drm/i915: fix 64bit divide Lionel Landwerlin
@ 2017-11-13 18:19 ` Lionel Landwerlin
  2017-11-13 19:01 ` ✓ Fi.CI.BAT: success for drm/i915: some perf cleanups (& fixes!) Patchwork
  2017-11-13 20:33 ` ✓ Fi.CI.IGT: " Patchwork
  5 siblings, 0 replies; 13+ messages in thread
From: Lionel Landwerlin @ 2017-11-13 18:19 UTC (permalink / raw)
  To: intel-gfx

We apply this logic to Gen9 as well. We didn't notice this issue as
most part we've encountered so far only use the crystal as source for
their timestamp registers.

Fixes: dab9178333 ("drm/i915: expose command stream timestamp frequency to userspace")
Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
---
 drivers/gpu/drm/i915/intel_device_info.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
index 992ae1bdfb3b..943b3b5399eb 100644
--- a/drivers/gpu/drm/i915/intel_device_info.c
+++ b/drivers/gpu/drm/i915/intel_device_info.c
@@ -413,15 +413,15 @@ static u64 read_timestamp_frequency(struct drm_i915_private *dev_priv)
 				freq = f24_mhz;
 				break;
 			}
-		}
 
-		/* Now figure out how the command stream's timestamp register
-		 * increments from this frequency (it might increment only
-		 * every few clock cycle).
-		 */
-		freq >>= 3 - ((rpm_config_reg &
-			       GEN10_RPM_CONFIG0_CTC_SHIFT_PARAMETER_MASK) >>
-			      GEN10_RPM_CONFIG0_CTC_SHIFT_PARAMETER_SHIFT);
+			/* Now figure out how the command stream's timestamp
+			 * register increments from this frequency (it might
+			 * increment only every few clock cycle).
+			 */
+			freq >>= 3 - ((rpm_config_reg &
+				       GEN10_RPM_CONFIG0_CTC_SHIFT_PARAMETER_MASK) >>
+				      GEN10_RPM_CONFIG0_CTC_SHIFT_PARAMETER_SHIFT);
+		}
 
 		return freq;
 	}
-- 
2.15.0

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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 3/4] drm/i915: fix 64bit divide
  2017-11-13 18:19 ` [PATCH v2 3/4] drm/i915: fix 64bit divide Lionel Landwerlin
@ 2017-11-13 18:44   ` Matthew Auld
  2017-11-13 18:50     ` Lionel Landwerlin
  2017-11-13 20:50     ` Ville Syrjälä
  0 siblings, 2 replies; 13+ messages in thread
From: Matthew Auld @ 2017-11-13 18:44 UTC (permalink / raw)
  To: Lionel Landwerlin; +Cc: Intel Graphics Development

On 13 November 2017 at 18:19, Lionel Landwerlin
<lionel.g.landwerlin@intel.com> wrote:
> ERROR: "__udivdi3" [drivers/gpu/drm/i915/i915.ko] undefined!
> ERROR: "__divdi3" [drivers/gpu/drm/i915/i915.ko] undefined!
>
> We can also drop an if() as we divide by (value + 1) only if value is
> not 0.
>
> Fixes: dab9178333 ("drm/i915: expose command stream timestamp frequency to userspace")
> Reported-by: Matthew Auld <matthew.auld@intel.com>
> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_device_info.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> index 78bf7374fbdd..992ae1bdfb3b 100644
> --- a/drivers/gpu/drm/i915/intel_device_info.c
> +++ b/drivers/gpu/drm/i915/intel_device_info.c
> @@ -336,13 +336,12 @@ static u64 read_reference_ts_freq(struct drm_i915_private *dev_priv)
>
>         base_freq = ((ts_override & GEN9_TIMESTAMP_OVERRIDE_US_COUNTER_DIVIDER_MASK) >>
>                      GEN9_TIMESTAMP_OVERRIDE_US_COUNTER_DIVIDER_SHIFT) + 1;
> -       base_freq *= 1000000;
> +       base_freq *= 1000000ULL;
>
>         frac_freq = ((ts_override &
>                       GEN9_TIMESTAMP_OVERRIDE_US_COUNTER_DENOMINATOR_MASK) >>
>                      GEN9_TIMESTAMP_OVERRIDE_US_COUNTER_DENOMINATOR_SHIFT);
> -       if (frac_freq != 0)
> -               frac_freq = 1000000 / (frac_freq + 1);
> +       frac_freq = div_u64(1000000ULL, frac_freq + 1);
s/div_u64/div64_u64/ ?

>
>         return base_freq + frac_freq;
>  }
> @@ -360,7 +359,7 @@ static u64 read_timestamp_frequency(struct drm_i915_private *dev_priv)
>                  *      hclks." (through the “Clocking Configuration”
>                  *      (“CLKCFG”) MCHBAR register)
>                  */
> -               return (dev_priv->rawclk_freq * 1000) / 16;
> +               return div_u64(dev_priv->rawclk_freq * 1000ULL, 16);
Why do we need this?

>         } else if (INTEL_GEN(dev_priv) <= 8) {
>                 /* PRMs say:
>                  *
> --
> 2.15.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 3/4] drm/i915: fix 64bit divide
  2017-11-13 18:44   ` Matthew Auld
@ 2017-11-13 18:50     ` Lionel Landwerlin
  2017-11-13 20:50     ` Ville Syrjälä
  1 sibling, 0 replies; 13+ messages in thread
From: Lionel Landwerlin @ 2017-11-13 18:50 UTC (permalink / raw)
  To: Matthew Auld; +Cc: Intel Graphics Development

On 13/11/17 18:44, Matthew Auld wrote:
> On 13 November 2017 at 18:19, Lionel Landwerlin
> <lionel.g.landwerlin@intel.com> wrote:
>> ERROR: "__udivdi3" [drivers/gpu/drm/i915/i915.ko] undefined!
>> ERROR: "__divdi3" [drivers/gpu/drm/i915/i915.ko] undefined!
>>
>> We can also drop an if() as we divide by (value + 1) only if value is
>> not 0.
>>
>> Fixes: dab9178333 ("drm/i915: expose command stream timestamp frequency to userspace")
>> Reported-by: Matthew Auld <matthew.auld@intel.com>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
>> ---
>>   drivers/gpu/drm/i915/intel_device_info.c | 7 +++----
>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
>> index 78bf7374fbdd..992ae1bdfb3b 100644
>> --- a/drivers/gpu/drm/i915/intel_device_info.c
>> +++ b/drivers/gpu/drm/i915/intel_device_info.c
>> @@ -336,13 +336,12 @@ static u64 read_reference_ts_freq(struct drm_i915_private *dev_priv)
>>
>>          base_freq = ((ts_override & GEN9_TIMESTAMP_OVERRIDE_US_COUNTER_DIVIDER_MASK) >>
>>                       GEN9_TIMESTAMP_OVERRIDE_US_COUNTER_DIVIDER_SHIFT) + 1;
>> -       base_freq *= 1000000;
>> +       base_freq *= 1000000ULL;
>>
>>          frac_freq = ((ts_override &
>>                        GEN9_TIMESTAMP_OVERRIDE_US_COUNTER_DENOMINATOR_MASK) >>
>>                       GEN9_TIMESTAMP_OVERRIDE_US_COUNTER_DENOMINATOR_SHIFT);
>> -       if (frac_freq != 0)
>> -               frac_freq = 1000000 / (frac_freq + 1);
>> +       frac_freq = div_u64(1000000ULL, frac_freq + 1);
> s/div_u64/div64_u64/ ?

Oh right!

>
>>          return base_freq + frac_freq;
>>   }
>> @@ -360,7 +359,7 @@ static u64 read_timestamp_frequency(struct drm_i915_private *dev_priv)
>>                   *      hclks." (through the “Clocking Configuration”
>>                   *      (“CLKCFG”) MCHBAR register)
>>                   */
>> -               return (dev_priv->rawclk_freq * 1000) / 16;
>> +               return div_u64(dev_priv->rawclk_freq * 1000ULL, 16);
> Why do we need this?

It was just for consistency as we return a u64. I can drop it if you want.

>
>>          } else if (INTEL_GEN(dev_priv) <= 8) {
>>                  /* PRMs say:
>>                   *
>> --
>> 2.15.0
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx


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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* ✓ Fi.CI.BAT: success for drm/i915: some perf cleanups (& fixes!)
  2017-11-13 18:18 [PATCH v2 0/4] drm/i915: some perf cleanups (& fixes!) Lionel Landwerlin
                   ` (3 preceding siblings ...)
  2017-11-13 18:19 ` [PATCH v2 4/4] drm/i915/cnl: only divide up base frequency with crystal source Lionel Landwerlin
@ 2017-11-13 19:01 ` Patchwork
  2017-11-13 20:33 ` ✓ Fi.CI.IGT: " Patchwork
  5 siblings, 0 replies; 13+ messages in thread
From: Patchwork @ 2017-11-13 19:01 UTC (permalink / raw)
  To: Lionel Landwerlin; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: some perf cleanups (& fixes!)
URL   : https://patchwork.freedesktop.org/series/33736/
State : success

== Summary ==

Series 33736v1 drm/i915: some perf cleanups (& fixes!)
https://patchwork.freedesktop.org/api/1.0/series/33736/revisions/1/mbox/

Test chamelium:
        Subgroup dp-crc-fast:
                dmesg-fail -> FAIL       (fi-kbl-7500u) fdo#102514
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-b:
                pass       -> INCOMPLETE (fi-snb-2520m) fdo#103713
Test vgem_basic:
        Subgroup dmabuf-export:
                incomplete -> PASS       (fi-byt-n2820) fdo#103714
                incomplete -> PASS       (fi-glk-1) fdo#103706
        Subgroup unload:
                notrun     -> INCOMPLETE (fi-byt-n2820) fdo#103702 +1

fdo#102514 https://bugs.freedesktop.org/show_bug.cgi?id=102514
fdo#103713 https://bugs.freedesktop.org/show_bug.cgi?id=103713
fdo#103714 https://bugs.freedesktop.org/show_bug.cgi?id=103714
fdo#103706 https://bugs.freedesktop.org/show_bug.cgi?id=103706
fdo#103702 https://bugs.freedesktop.org/show_bug.cgi?id=103702

fi-bdw-5557u     total:285  pass:263  dwarn:0   dfail:1   fail:0   skip:20 
fi-bdw-gvtdvm    total:285  pass:260  dwarn:0   dfail:1   fail:0   skip:23 
fi-blb-e6850     total:285  pass:218  dwarn:1   dfail:1   fail:0   skip:64 
fi-bsw-n3050     total:285  pass:238  dwarn:0   dfail:1   fail:0   skip:45 
fi-bwr-2160      total:285  pass:178  dwarn:0   dfail:1   fail:0   skip:105
fi-bxt-dsi       total:285  pass:254  dwarn:0   dfail:1   fail:0   skip:29 
fi-bxt-j4205     total:285  pass:255  dwarn:0   dfail:1   fail:0   skip:28 
fi-byt-j1900     total:285  pass:249  dwarn:0   dfail:1   fail:0   skip:34 
fi-byt-n2820     total:285  pass:245  dwarn:0   dfail:1   fail:0   skip:38 
fi-elk-e7500     total:285  pass:224  dwarn:0   dfail:1   fail:0   skip:59 
fi-gdg-551       total:285  pass:174  dwarn:0   dfail:1   fail:1   skip:108
fi-glk-1         total:285  pass:256  dwarn:0   dfail:1   fail:0   skip:27 
fi-hsw-4770      total:285  pass:257  dwarn:0   dfail:1   fail:0   skip:26 
fi-hsw-4770r     total:285  pass:257  dwarn:0   dfail:1   fail:0   skip:26 
fi-ilk-650       total:285  pass:223  dwarn:0   dfail:1   fail:0   skip:60 
fi-ivb-3520m     total:285  pass:255  dwarn:0   dfail:1   fail:0   skip:28 
fi-ivb-3770      total:285  pass:255  dwarn:0   dfail:1   fail:0   skip:28 
fi-kbl-7500u     total:285  pass:258  dwarn:1   dfail:1   fail:1   skip:23 
fi-kbl-7560u     total:285  pass:265  dwarn:0   dfail:1   fail:0   skip:18 
fi-kbl-7567u     total:285  pass:264  dwarn:0   dfail:1   fail:0   skip:19 
fi-kbl-r         total:285  pass:257  dwarn:0   dfail:1   fail:0   skip:26 
fi-pnv-d510      total:285  pass:217  dwarn:1   dfail:1   fail:0   skip:65 
fi-skl-6260u     total:285  pass:264  dwarn:0   dfail:1   fail:0   skip:19 
fi-skl-6600u     total:285  pass:257  dwarn:0   dfail:1   fail:0   skip:26 
fi-skl-6700hq    total:285  pass:258  dwarn:0   dfail:1   fail:0   skip:25 
fi-skl-6700k     total:285  pass:260  dwarn:0   dfail:1   fail:0   skip:23 
fi-skl-6770hq    total:285  pass:264  dwarn:0   dfail:1   fail:0   skip:19 
fi-skl-gvtdvm    total:285  pass:261  dwarn:0   dfail:1   fail:0   skip:22 
fi-snb-2520m     total:246  pass:212  dwarn:0   dfail:0   fail:0   skip:33 
fi-snb-2600      total:285  pass:244  dwarn:0   dfail:1   fail:0   skip:39 
Blacklisted hosts:
fi-cfl-s         total:285  pass:252  dwarn:0   dfail:1   fail:0   skip:31 

9891b4090c6e6a5299a4134f7bd6c463fb2cd485 drm-tip: 2017y-11m-13d-16h-04m-46s UTC integration manifest
747f9fcd5d77 drm/i915/cnl: only divide up base frequency with crystal source
cdf4e62ea174 drm/i915: fix 64bit divide
7e06e7b72dfb drm/i915/perf: replace .reg accesses with i915_mmio_reg_offset
1f3c84bd9413 drm/i915/perf: reuse timestamp frequency from device info

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7101/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/4] drm/i915/perf: reuse timestamp frequency from device info
  2017-11-13 18:18 ` [PATCH v2 1/4] drm/i915/perf: reuse timestamp frequency from device info Lionel Landwerlin
@ 2017-11-13 19:03   ` Matthew Auld
  2017-11-13 20:53     ` Ville Syrjälä
  0 siblings, 1 reply; 13+ messages in thread
From: Matthew Auld @ 2017-11-13 19:03 UTC (permalink / raw)
  To: Lionel Landwerlin; +Cc: Intel Graphics Development

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>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 13+ messages in thread

* ✓ Fi.CI.IGT: success for drm/i915: some perf cleanups (& fixes!)
  2017-11-13 18:18 [PATCH v2 0/4] drm/i915: some perf cleanups (& fixes!) Lionel Landwerlin
                   ` (4 preceding siblings ...)
  2017-11-13 19:01 ` ✓ Fi.CI.BAT: success for drm/i915: some perf cleanups (& fixes!) Patchwork
@ 2017-11-13 20:33 ` Patchwork
  5 siblings, 0 replies; 13+ messages in thread
From: Patchwork @ 2017-11-13 20:33 UTC (permalink / raw)
  To: Lionel Landwerlin; +Cc: intel-gfx

== Series Details ==

Series: drm/i915: some perf cleanups (& fixes!)
URL   : https://patchwork.freedesktop.org/series/33736/
State : success

== Summary ==

Test drv_module_reload:
        Subgroup basic-reload:
                pass       -> DMESG-WARN (shard-hsw) fdo#102707

fdo#102707 https://bugs.freedesktop.org/show_bug.cgi?id=102707

shard-hsw        total:2584 pass:1471 dwarn:3   dfail:2   fail:9   skip:1099 time:9518s
Blacklisted hosts:
shard-apl        total:2544 pass:1596 dwarn:3   dfail:2   fail:24  skip:918 time:13012s
shard-kbl        total:2504 pass:1637 dwarn:26  dfail:11  fail:24  skip:803 time:9974s
shard-snb        total:2584 pass:1206 dwarn:2   dfail:2   fail:12  skip:1362 time:7731s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7101/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 3/4] drm/i915: fix 64bit divide
  2017-11-13 18:44   ` Matthew Auld
  2017-11-13 18:50     ` Lionel Landwerlin
@ 2017-11-13 20:50     ` Ville Syrjälä
  1 sibling, 0 replies; 13+ messages in thread
From: Ville Syrjälä @ 2017-11-13 20:50 UTC (permalink / raw)
  To: Matthew Auld; +Cc: Intel Graphics Development

On Mon, Nov 13, 2017 at 06:44:47PM +0000, Matthew Auld wrote:
> On 13 November 2017 at 18:19, Lionel Landwerlin
> <lionel.g.landwerlin@intel.com> wrote:
> > ERROR: "__udivdi3" [drivers/gpu/drm/i915/i915.ko] undefined!
> > ERROR: "__divdi3" [drivers/gpu/drm/i915/i915.ko] undefined!
> >
> > We can also drop an if() as we divide by (value + 1) only if value is
> > not 0.
> >
> > Fixes: dab9178333 ("drm/i915: expose command stream timestamp frequency to userspace")
> > Reported-by: Matthew Auld <matthew.auld@intel.com>
> > Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_device_info.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_device_info.c b/drivers/gpu/drm/i915/intel_device_info.c
> > index 78bf7374fbdd..992ae1bdfb3b 100644
> > --- a/drivers/gpu/drm/i915/intel_device_info.c
> > +++ b/drivers/gpu/drm/i915/intel_device_info.c
> > @@ -336,13 +336,12 @@ static u64 read_reference_ts_freq(struct drm_i915_private *dev_priv)
> >
> >         base_freq = ((ts_override & GEN9_TIMESTAMP_OVERRIDE_US_COUNTER_DIVIDER_MASK) >>
> >                      GEN9_TIMESTAMP_OVERRIDE_US_COUNTER_DIVIDER_SHIFT) + 1;
> > -       base_freq *= 1000000;
> > +       base_freq *= 1000000ULL;
> >
> >         frac_freq = ((ts_override &
> >                       GEN9_TIMESTAMP_OVERRIDE_US_COUNTER_DENOMINATOR_MASK) >>
> >                      GEN9_TIMESTAMP_OVERRIDE_US_COUNTER_DENOMINATOR_SHIFT);
> > -       if (frac_freq != 0)
> > -               frac_freq = 1000000 / (frac_freq + 1);
> > +       frac_freq = div_u64(1000000ULL, frac_freq + 1);
> s/div_u64/div64_u64/ ?

Rather s/u64 frac_freq/u32 frac_freq/

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/4] drm/i915/perf: reuse timestamp frequency from device info
  2017-11-13 19:03   ` Matthew Auld
@ 2017-11-13 20:53     ` Ville Syrjälä
  2017-11-13 21:12       ` Chris Wilson
  0 siblings, 1 reply; 13+ messages in thread
From: Ville Syrjälä @ 2017-11-13 20:53 UTC (permalink / raw)
  To: Matthew Auld; +Cc: Intel Graphics Development

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 1/4] drm/i915/perf: reuse timestamp frequency from device info
  2017-11-13 20:53     ` Ville Syrjälä
@ 2017-11-13 21:12       ` Chris Wilson
  0 siblings, 0 replies; 13+ messages in thread
From: Chris Wilson @ 2017-11-13 21:12 UTC (permalink / raw)
  To: Ville Syrjälä, Matthew Auld; +Cc: Intel Graphics Development

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2017-11-13 21:12 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-11-13 18:18 [PATCH v2 0/4] drm/i915: some perf cleanups (& fixes!) Lionel Landwerlin
2017-11-13 18:18 ` [PATCH v2 1/4] drm/i915/perf: reuse timestamp frequency from device info Lionel Landwerlin
2017-11-13 19:03   ` Matthew Auld
2017-11-13 20:53     ` Ville Syrjälä
2017-11-13 21:12       ` Chris Wilson
2017-11-13 18:19 ` [PATCH v2 2/4] drm/i915/perf: replace .reg accesses with i915_mmio_reg_offset Lionel Landwerlin
2017-11-13 18:19 ` [PATCH v2 3/4] drm/i915: fix 64bit divide Lionel Landwerlin
2017-11-13 18:44   ` Matthew Auld
2017-11-13 18:50     ` Lionel Landwerlin
2017-11-13 20:50     ` Ville Syrjälä
2017-11-13 18:19 ` [PATCH v2 4/4] drm/i915/cnl: only divide up base frequency with crystal source Lionel Landwerlin
2017-11-13 19:01 ` ✓ Fi.CI.BAT: success for drm/i915: some perf cleanups (& fixes!) Patchwork
2017-11-13 20:33 ` ✓ Fi.CI.IGT: " Patchwork

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.