Re: [PATCH v8 11/12] drm/i915: Add more Haswell OA metric sets

From: Chris Wilson <chris@chris-wilson.co.uk>
To: Robert Bragg <robert@sixbynine.org>
Cc: dri-devel@lists.freedesktop.org, intel-gfx@lists.freedesktop.org,
	Matthew Auld <matthew.william.auld@gmail.com>,
	Sourab Gupta <sourab.gupta@intel.com>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH v8 11/12] drm/i915: Add more Haswell OA metric sets
Date: Tue, 1 Nov 2016 14:57:05 +0000	[thread overview]
Message-ID: <20161101145705.GD26576@nuc-i3427.alporthouse.com> (raw)
In-Reply-To: <20161028021430.2177-12-robert@sixbynine.org>

On Fri, Oct 28, 2016 at 03:14:29AM +0100, Robert Bragg wrote:
> This adds 'compute', 'compute extended', 'memory reads', 'memory writes'
> and 'sampler balance' metric sets for Haswell.
> 
> The code is auto generated from an XML description of metric sets,
> currently maintained in gputop, ref:
> 
>  https://github.com/rib/gputop
>  > gputop-data/oa-*.xml
>  > scripts/i915-perf-kernelgen.py
> 
>  $ make -C gputop-data -f Makefile.xml
> 
> Signed-off-by: Robert Bragg <robert@sixbynine.org>
> Reviewed-by: Matthew Auld <matthew.auld@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_oa_hsw.c | 559 ++++++++++++++++++++++++++++++++++++-
>  1 file changed, 558 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_oa_hsw.c b/drivers/gpu/drm/i915/i915_oa_hsw.c
> index 6af25cf..4ddf756 100644
> --- a/drivers/gpu/drm/i915/i915_oa_hsw.c
> +++ b/drivers/gpu/drm/i915/i915_oa_hsw.c
> @@ -31,9 +31,14 @@
>  
>  enum metric_set_id {
>  	METRIC_SET_ID_RENDER_BASIC = 1,
> +	METRIC_SET_ID_COMPUTE_BASIC,
> +	METRIC_SET_ID_COMPUTE_EXTENDED,
> +	METRIC_SET_ID_MEMORY_READS,
> +	METRIC_SET_ID_MEMORY_WRITES,
> +	METRIC_SET_ID_SAMPLER_BALANCE,
>  };
>  
> -int i915_oa_n_builtin_metric_sets_hsw = 1;
> +int i915_oa_n_builtin_metric_sets_hsw = 6;
>  
>  static const struct i915_oa_reg b_counter_config_render_basic[] = {
>  	{ _MMIO(0x2724), 0x00800000 },
> @@ -112,6 +117,298 @@ get_render_basic_mux_config(struct drm_i915_private *dev_priv,
>  	return mux_config_render_basic;
>  }
>  
> +static const struct i915_oa_reg b_counter_config_compute_basic[] = {
> +	{ _MMIO(0x2710), 0x00000000 },
> +	{ _MMIO(0x2714), 0x00800000 },
> +	{ _MMIO(0x2718), 0xaaaaaaaa },
> +	{ _MMIO(0x271c), 0xaaaaaaaa },
> +	{ _MMIO(0x2720), 0x00000000 },
> +	{ _MMIO(0x2724), 0x00800000 },
> +	{ _MMIO(0x2728), 0xaaaaaaaa },
> +	{ _MMIO(0x272c), 0xaaaaaaaa },
> +	{ _MMIO(0x2740), 0x00000000 },
> +	{ _MMIO(0x2744), 0x00000000 },
> +	{ _MMIO(0x2748), 0x00000000 },
> +	{ _MMIO(0x274c), 0x00000000 },
> +	{ _MMIO(0x2750), 0x00000000 },
> +	{ _MMIO(0x2754), 0x00000000 },
> +	{ _MMIO(0x2758), 0x00000000 },
> +	{ _MMIO(0x275c), 0x00000000 },
> +	{ _MMIO(0x236c), 0x00000000 },
> +};
> +
> +static const struct i915_oa_reg mux_config_compute_basic[] = {
> +	{ _MMIO(0x253a4), 0x00000000 },
> +	{ _MMIO(0x2681c), 0x01f00800 },
> +	{ _MMIO(0x26820), 0x00001000 },
> +	{ _MMIO(0x2781c), 0x01f00800 },
> +	{ _MMIO(0x26520), 0x00000007 },
> +	{ _MMIO(0x265a0), 0x00000007 },
> +	{ _MMIO(0x25380), 0x00000010 },
> +	{ _MMIO(0x2538c), 0x00300000 },
> +	{ _MMIO(0x25384), 0xaa8aaaaa },
> +	{ _MMIO(0x25404), 0xffffffff },
> +	{ _MMIO(0x26800), 0x00004202 },
> +	{ _MMIO(0x26808), 0x00605817 },
> +	{ _MMIO(0x2680c), 0x10001005 },
> +	{ _MMIO(0x26804), 0x00000000 },
> +	{ _MMIO(0x27800), 0x00000102 },
> +	{ _MMIO(0x27808), 0x0c0701e0 },
> +	{ _MMIO(0x2780c), 0x000200a0 },
> +	{ _MMIO(0x27804), 0x00000000 },
> +	{ _MMIO(0x26484), 0x44000000 },
> +	{ _MMIO(0x26704), 0x44000000 },
> +	{ _MMIO(0x26500), 0x00000006 },
> +	{ _MMIO(0x26510), 0x00000001 },
> +	{ _MMIO(0x26504), 0x88000000 },
> +	{ _MMIO(0x26580), 0x00000006 },
> +	{ _MMIO(0x26590), 0x00000020 },
> +	{ _MMIO(0x26584), 0x00000000 },
> +	{ _MMIO(0x26104), 0x55822222 },
> +	{ _MMIO(0x26184), 0xaa866666 },
> +	{ _MMIO(0x25420), 0x08320c83 },
> +	{ _MMIO(0x25424), 0x06820c83 },
> +	{ _MMIO(0x2541c), 0x00000000 },
> +	{ _MMIO(0x25428), 0x00000c03 },
> +};
> +
> +static const struct i915_oa_reg *
> +get_compute_basic_mux_config(struct drm_i915_private *dev_priv,
> +			     int *len)
> +{
> +	*len = ARRAY_SIZE(mux_config_compute_basic);
> +	return mux_config_compute_basic;
> +}

> @@ -140,6 +437,106 @@ int i915_oa_select_metric_set_hsw(struct drm_i915_private *dev_priv)
>  			ARRAY_SIZE(b_counter_config_render_basic);
>  
>  		return 0;
> +	case METRIC_SET_ID_COMPUTE_BASIC:
> +		dev_priv->perf.oa.mux_regs =
> +			get_compute_basic_mux_config(dev_priv,
> +						     &dev_priv->perf.oa.mux_regs_len);
> +		if (!dev_priv->perf.oa.mux_regs) {
> +			DRM_DEBUG_DRIVER("No suitable MUX config for \"COMPUTE_BASIC\" metric set");
> +
> +			/* EINVAL because *_register_sysfs already checked this
> +			 * and so it wouldn't have been advertised so userspace and
> +			 * so shouldn't have been requested
> +			 */
> +			return -EINVAL;
> +		}
> +
> +		dev_priv->perf.oa.b_counter_regs =
> +			b_counter_config_compute_basic;
> +		dev_priv->perf.oa.b_counter_regs_len =
> +			ARRAY_SIZE(b_counter_config_compute_basic);
> +
> +		return 0;

>  int
>  i915_perf_register_sysfs_hsw(struct drm_i915_private *dev_priv)
>  {
> @@ -178,9 +685,49 @@ i915_perf_register_sysfs_hsw(struct drm_i915_private *dev_priv)
>  		if (ret)
>  			goto error_render_basic;
>  	}
> +	if (get_compute_basic_mux_config(dev_priv, &mux_len)) {

Why not use the derived state in dev_priv->perf.oa.mux_regs? Then we
only expose what is initialised.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel