Re: [PATCH v3 1/2] drm/i915/perf: prune OA configs

[Lionel Landwerlin <lionel.g.landwerlin@xxxxxxxxx>]

On 14/07/17 13:20, Matthew Auld wrote:
> On 07/14, Lionel Landwerlin wrote:
> > In the following commit we'll introduce loadable userspace
> > configs. This change reworks how configurations are handled in the
> > perf driver and retains only the test configurations in kernel space.
> >
> > We now store the test config in dev_priv and resolve the id only once
> > when opening the perf stream. The OA config is then handled through a
> > pointer to the structure holding the configuration details.
> >
> > v2: Rework how test configs are handled (Lionel)
> >
> > v3: Use u32 to hold number of register (Matthew)
> >
> > Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin@xxxxxxxxx>
> > ---
> >   drivers/gpu/drm/i915/i915_drv.h       |   44 +-
> >   drivers/gpu/drm/i915/i915_oa_bdw.c    | 5360 +--------------------------------
> >   drivers/gpu/drm/i915/i915_oa_bdw.h    |    8 +-
> >   drivers/gpu/drm/i915/i915_oa_bxt.c    | 2623 +---------------
> >   drivers/gpu/drm/i915/i915_oa_bxt.h    |    8 +-
> >   drivers/gpu/drm/i915/i915_oa_chv.c    | 2806 +----------------
> >   drivers/gpu/drm/i915/i915_oa_chv.h    |    8 +-
> >   drivers/gpu/drm/i915/i915_oa_glk.c    | 2535 +---------------
> >   drivers/gpu/drm/i915/i915_oa_glk.h    |    8 +-
> >   drivers/gpu/drm/i915/i915_oa_hsw.c    |  764 +----
> >   drivers/gpu/drm/i915/i915_oa_hsw.h    |    8 +-
> >   drivers/gpu/drm/i915/i915_oa_kblgt2.c | 2971 +-----------------
> >   drivers/gpu/drm/i915/i915_oa_kblgt2.h |    8 +-
> >   drivers/gpu/drm/i915/i915_oa_kblgt3.c | 3020 +------------------
> >   drivers/gpu/drm/i915/i915_oa_kblgt3.h |    8 +-
> >   drivers/gpu/drm/i915/i915_oa_sklgt2.c | 3458 +--------------------
> >   drivers/gpu/drm/i915/i915_oa_sklgt2.h |    8 +-
> >   drivers/gpu/drm/i915/i915_oa_sklgt3.c | 3019 +------------------
> >   drivers/gpu/drm/i915/i915_oa_sklgt3.h |    8 +-
> >   drivers/gpu/drm/i915/i915_oa_sklgt4.c | 3073 +------------------
> >   drivers/gpu/drm/i915/i915_oa_sklgt4.h |    8 +-
> >   drivers/gpu/drm/i915/i915_perf.c      |  309 +-
> >   22 files changed, 504 insertions(+), 29558 deletions(-)
> <SNIP>
>
> > -static int gen8_emit_oa_config(struct drm_i915_gem_request *req)
> > +static int gen8_emit_oa_config(struct drm_i915_gem_request *req,
> > +			       const struct i915_oa_config *oa_config)
> >   {
> >   	struct drm_i915_private *dev_priv = req->i915;
> > -	const struct i915_oa_reg *flex_regs = dev_priv->perf.oa.flex_regs;
> > -	int n_flex_regs = dev_priv->perf.oa.flex_regs_len;
> >   	/* The MMIO offsets for Flex EU registers aren't contiguous */
> >   	u32 flex_mmio[] = {
> >   		i915_mmio_reg_offset(EU_PERF_CNTL0),
> > @@ -1601,11 +1594,14 @@ static int gen8_emit_oa_config(struct drm_i915_gem_request *req)
> >   	u32 *cs;
> >   	int i;
> >   
> > -	cs = intel_ring_begin(req, n_flex_regs * 2 + 4);
> > +	if (WARN_ON(!oa_config))
> > +		return -EINVAL;
> > +
> > +	cs = intel_ring_begin(req, oa_config->flex_regs_len * 2 + 4);
> >   	if (IS_ERR(cs))
> >   		return PTR_ERR(cs);
> >   
> > -	*cs++ = MI_LOAD_REGISTER_IMM(n_flex_regs + 1);
> > +	*cs++ = MI_LOAD_REGISTER_IMM(oa_config->flex_regs_len + 1);
> Something doesn't add up here, if we don't supply any flex registers, which
> seems to be the case for the oa test configs, or potentially any user provided
> config then we only reserve ring_begin(4)/IMM(1), but we still go off and write
> out the default flex values as per flex_mmio. Also we could potentially over
> reserve since we can only ever write out at most flex_mmio registers.
>
> My first thought is perhaps:
>
> int flex_max = ARRAY_SIZE(flex_mmio);
>
> GEM_BUG_ON(flex_regs_len > flex_max);
>
> cs = intel_ring_begin(req, flex_max * 2 + 4);
>
> *cs++ = MI_LOAD_REGISTER_IMM(flex_max + 1);
>
> for (i = 0; i < flex_max; i++) {
>      
>      for (...) {
> 	if (addr == mmio) {
> 		value = ...
> 		++count;
> 	}
>      }
> }
>
> GEM_BUG_ON(count != flex_regs_len);

Thanks for pointing this out, there is indeed a problem.

> >   
> >   	*cs++ = i915_mmio_reg_offset(GEN8_OACTXCONTROL);
> >   	*cs++ = (dev_priv->perf.oa.period_exponent << GEN8_OA_TIMER_PERIOD_SHIFT) |
> > @@ -1624,9 +1620,9 @@ static int gen8_emit_oa_config(struct drm_i915_gem_request *req)
> >   		u32 value = 0;
> >   		int j;
> >   
> > -		for (j = 0; j < n_flex_regs; j++) {
> > -			if (i915_mmio_reg_offset(flex_regs[j].addr) == mmio) {
> > -				value = flex_regs[j].value;
> > +		for (j = 0; j < oa_config->flex_regs_len; j++) {
> > +			if (i915_mmio_reg_offset(oa_config->flex_regs[j].addr) == mmio) {
> > +				value = oa_config->flex_regs[j].value;
> >   				break;
> >   			}
> >   		}
> > @@ -1641,7 +1637,8 @@ static int gen8_emit_oa_config(struct drm_i915_gem_request *req)
> >   	return 0;
> >   }
> >   
> > -static int gen8_switch_to_updated_kernel_context(struct drm_i915_private *dev_priv)
> > +static int gen8_switch_to_updated_kernel_context(struct drm_i915_private *dev_priv,
> > +						 const struct i915_oa_config *oa_config)
> >   {
> >   	struct intel_engine_cs *engine = dev_priv->engine[RCS];
> >   	struct i915_gem_timeline *timeline;
> > @@ -1656,10 +1653,12 @@ static int gen8_switch_to_updated_kernel_context(struct drm_i915_private *dev_pr
> >   	if (IS_ERR(req))
> >   		return PTR_ERR(req);
> >   
> > -	ret = gen8_emit_oa_config(req);
> > -	if (ret) {
> > -		i915_add_request(req);
> > -		return ret;
> > +	if (oa_config) {
> > +		ret = gen8_emit_oa_config(req, oa_config);
> > +		if (ret) {
> > +			i915_add_request(req);
> > +			return ret;
> > +		}
> >   	}
> >   
> >   	/* Queue this switch after all other activity */
> > @@ -1707,6 +1706,7 @@ static int gen8_switch_to_updated_kernel_context(struct drm_i915_private *dev_pr
> >    * Note: it's only the RCS/Render context that has any OA state.
> >    */
> >   static int gen8_configure_all_contexts(struct drm_i915_private *dev_priv,
> > +				       const struct i915_oa_config *oa_config,
> >   				       bool interruptible)
> >   {
> >   	struct i915_gem_context *ctx;
> > @@ -1724,7 +1724,7 @@ static int gen8_configure_all_contexts(struct drm_i915_private *dev_priv,
> >   	}
> >   
> >   	/* Switch away from any user context. */
> > -	ret = gen8_switch_to_updated_kernel_context(dev_priv);
> > +	ret = gen8_switch_to_updated_kernel_context(dev_priv, oa_config);
> >   	if (ret)
> >   		goto out;
> >   
> > @@ -1763,7 +1763,8 @@ static int gen8_configure_all_contexts(struct drm_i915_private *dev_priv,
> >   		ce->state->obj->mm.dirty = true;
> >   		regs += LRC_STATE_PN * PAGE_SIZE / sizeof(*regs);
> >   
> > -		gen8_update_reg_state_unlocked(ctx, regs);
> > +		if (oa_config)
> > +			gen8_update_reg_state_unlocked(ctx, regs, oa_config);
> >   
> >   		i915_gem_object_unpin_map(ce->state->obj);
> >   	}
> > @@ -1774,13 +1775,10 @@ static int gen8_configure_all_contexts(struct drm_i915_private *dev_priv,
> >   	return ret;
> >   }
> >   
> > -static int gen8_enable_metric_set(struct drm_i915_private *dev_priv)
> > +static int gen8_enable_metric_set(struct drm_i915_private *dev_priv,
> > +				  const struct i915_oa_config *oa_config)
> >   {
> > -	int ret = dev_priv->perf.oa.ops.select_metric_set(dev_priv);
> > -	int i;
> > -
> > -	if (ret)
> > -		return ret;
> > +	int ret;
> >   
> >   	/*
> >   	 * We disable slice/unslice clock ratio change reports on SKL since
> > @@ -1817,19 +1815,18 @@ static int gen8_enable_metric_set(struct drm_i915_private *dev_priv)
> >   	 * to make sure all slices/subslices are ON before writing to NOA
> >   	 * registers.
> >   	 */
> > -	ret = gen8_configure_all_contexts(dev_priv, true);
> > +	ret = gen8_configure_all_contexts(dev_priv, oa_config, true);
> >   	if (ret)
> >   		return ret;
> >   
> >   	I915_WRITE(GDT_CHICKEN_BITS, 0xA0);
> > -	for (i = 0; i < dev_priv->perf.oa.n_mux_configs; i++) {
> > -		config_oa_regs(dev_priv, dev_priv->perf.oa.mux_regs[i],
> > -			       dev_priv->perf.oa.mux_regs_lens[i]);
> > -	}
> > +
> > +	config_oa_regs(dev_priv, oa_config->mux_regs, oa_config->mux_regs_len);
> > +
> >   	I915_WRITE(GDT_CHICKEN_BITS, 0x80);
> >   
> > -	config_oa_regs(dev_priv, dev_priv->perf.oa.b_counter_regs,
> > -		       dev_priv->perf.oa.b_counter_regs_len);
> > +	config_oa_regs(dev_priv, oa_config->b_counter_regs,
> > +		       oa_config->b_counter_regs_len);
> >   
> >   	return 0;
> >   }
> > @@ -1837,7 +1834,7 @@ static int gen8_enable_metric_set(struct drm_i915_private *dev_priv)
> >   static void gen8_disable_metric_set(struct drm_i915_private *dev_priv)
> >   {
> >   	/* Reset all contexts' slices/subslices configurations. */
> > -	gen8_configure_all_contexts(dev_priv, false);
> > +	gen8_configure_all_contexts(dev_priv, NULL, false);
> >   }
> >   
> >   static void gen7_oa_enable(struct drm_i915_private *dev_priv)
> > @@ -2011,8 +2008,8 @@ static int i915_oa_stream_init(struct i915_perf_stream *stream,
> >   		return -EBUSY;
> >   	}
> >   
> > -	if (!props->metrics_set) {
> > -		DRM_DEBUG("OA metric set not specified\n");
> > +	if (!props->oa_config) {
> > +		DRM_DEBUG("OA configuration set not found/specified\n");
> >   		return -EINVAL;
> >   	}
> >   
> > @@ -2055,7 +2052,7 @@ static int i915_oa_stream_init(struct i915_perf_stream *stream,
> >   	dev_priv->perf.oa.oa_buffer.format =
> >   		dev_priv->perf.oa.oa_formats[props->oa_format].format;
> >   
> > -	dev_priv->perf.oa.metrics_set = props->metrics_set;
> > +	stream->oa_config = props->oa_config;
> >   
> >   	dev_priv->perf.oa.periodic = props->oa_periodic;
> >   	if (dev_priv->perf.oa.periodic)
> > @@ -2086,7 +2083,8 @@ static int i915_oa_stream_init(struct i915_perf_stream *stream,
> >   	if (ret)
> >   		goto err_oa_buf_alloc;
> >   
> > -	ret = dev_priv->perf.oa.ops.enable_metric_set(dev_priv);
> > +	ret = dev_priv->perf.oa.ops.enable_metric_set(dev_priv,
> > +						      stream->oa_config);
> >   	if (ret)
> >   		goto err_enable;
> >   
> > @@ -2113,6 +2111,7 @@ void i915_oa_init_reg_state(struct intel_engine_cs *engine,
> >   			    u32 *reg_state)
> >   {
> >   	struct drm_i915_private *dev_priv = engine->i915;
> > +	struct i915_oa_config *oa_config = dev_priv->perf.oa.current_config;
> I'm confused, isn't current_config always NULL, where do we set it exactly?
> Maybe I'm missing something. Also why not just perf.exclusive_stream->oa_config,
> is that not the current config?
Oh dear, screwed up my rebase...

current_config should have been removed.

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