Re: [PATCH] drm/i915/perf: Configure OAR controls for specific context

From: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
To: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>,
	intel-gfx@lists.freedesktop.org
Cc: Chris Wilson <chris.p.wilson@intel.com>
Subject: Re: [PATCH] drm/i915/perf: Configure OAR controls for specific context
Date: Fri, 8 Nov 2019 09:22:00 +0200	[thread overview]
Message-ID: <76017001-c4aa-089e-367d-11a9d45496af@intel.com> (raw)
In-Reply-To: <20191107233433.6928-1-umesh.nerlige.ramappa@intel.com>

On 08/11/2019 01:34, Umesh Nerlige Ramappa wrote:
> It turns out that the OAR CONTROL register is not getting configured
> correctly in conjunction with the context save/restore bit. When
> measuring work for a single context, the OAR counters do not increment.
>
> - Configure OAR format and enable OAR counters at the same time as
>    enabling context save/restore for OAR counters.
> - Make SAMPLE_OA_REPORT optional from gen12.
>
> v2: Update commit message
>
> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_perf.c | 23 +++++++++++++++--------
>   1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
> index 2c380aba1ce9..527a16637689 100644
> --- a/drivers/gpu/drm/i915/i915_perf.c
> +++ b/drivers/gpu/drm/i915/i915_perf.c
> @@ -2219,26 +2219,33 @@ static int gen8_configure_context(struct i915_gem_context *ctx,
>   	return err;
>   }
>   
> -static int gen12_emit_oar_config(struct intel_context *ce, bool enable)
> +static int gen12_emit_oar_config(struct i915_perf_stream *stream, bool enable)
>   {
>   	struct i915_request *rq;
> +	struct intel_context *ce = stream->pinned_ctx;
>   	u32 *cs;
> +	u32 format = stream->oa_buffer.format;
>   	int err = 0;
>   
>   	rq = i915_request_create(ce);
>   	if (IS_ERR(rq))
>   		return PTR_ERR(rq);
>   
> -	cs = intel_ring_begin(rq, 4);
> +	cs = intel_ring_begin(rq, 6);
>   	if (IS_ERR(cs)) {
>   		err = PTR_ERR(cs);
>   		goto out;
>   	}
>   
> -	*cs++ = MI_LOAD_REGISTER_IMM(1);
> +	*cs++ = MI_LOAD_REGISTER_IMM(2);
> +	/* Enable context save/restore of OAR counters */
>   	*cs++ = i915_mmio_reg_offset(RING_CONTEXT_CONTROL(ce->engine->mmio_base));
>   	*cs++ = _MASKED_FIELD(GEN12_CTX_CTRL_OAR_CONTEXT_ENABLE,
>   			      enable ? GEN12_CTX_CTRL_OAR_CONTEXT_ENABLE : 0);
> +	/* Enable OAR counters */
> +	*cs++ = i915_mmio_reg_offset(GEN12_OAR_OACONTROL);
> +	*cs++ = (format << GEN12_OAR_OACONTROL_COUNTER_FORMAT_SHIFT) |
> +		(enable ? GEN12_OAR_OACONTROL_COUNTER_ENABLE : 0);
>   	*cs++ = MI_NOOP;

It's probably a good idea to configure OAR in this function indeed :)

But we're already supposed to program it through context image 
modification in lrc_configure_all_contexts().

So it probably means we have the wrong offset?

We should probably remove it from lrc_configure_all_contexts() then. 
It's probably trashing some other bit of the context image.

>   
>   	intel_ring_advance(rq, cs);
> @@ -2474,8 +2481,7 @@ static int gen12_enable_metric_set(struct i915_perf_stream *stream)
>   	 * requested this.
>   	 */
>   	if (stream->ctx) {
> -		ret = gen12_emit_oar_config(stream->pinned_ctx,
> -					    oa_config != NULL);
> +		ret = gen12_emit_oar_config(stream, oa_config != NULL);
>   		if (ret)
>   			return ret;
>   	}
> @@ -2513,7 +2519,7 @@ static void gen12_disable_metric_set(struct i915_perf_stream *stream)
>   
>   	/* disable the context save/restore or OAR counters */
>   	if (stream->ctx)
> -		gen12_emit_oar_config(stream->pinned_ctx, false);
> +		gen12_emit_oar_config(stream, false);
>   
>   	/* Make sure we disable noa to save power. */
>   	intel_uncore_rmw(uncore, RPM_CONFIG1, GEN10_GT_NOA_ENABLE, 0);
> @@ -2713,7 +2719,8 @@ static int i915_oa_stream_init(struct i915_perf_stream *stream,
>   		return -EINVAL;
>   	}
>   
> -	if (!(props->sample_flags & SAMPLE_OA_REPORT)) {
> +	if (!(props->sample_flags & SAMPLE_OA_REPORT) &&
> +	    (INTEL_GEN(perf->i915) < 12 || !stream->ctx)) {

Good point, but this should probably go into another patch.

Note that we could also consider not sampling the OA buffer a non 
privileged operation on Gen12+, since the counters are per context 
saved/restored.

Thanks,

-Lionel

>   		DRM_DEBUG("Only OA report sampling supported\n");
>   		return -EINVAL;
>   	}
> @@ -2745,7 +2752,7 @@ static int i915_oa_stream_init(struct i915_perf_stream *stream,
>   
>   	format_size = perf->oa_formats[props->oa_format].size;
>   
> -	stream->sample_flags |= SAMPLE_OA_REPORT;
> +	stream->sample_flags = props->sample_flags;
>   	stream->sample_size += format_size;
>   
>   	stream->oa_buffer.format_size = format_size;

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