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

From: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
To: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Cc: intel-gfx@lists.freedesktop.org, Chris Wilson <chris.p.wilson@intel.com>
Subject: Re: [PATCH] drm/i915/perf: Configure OAR controls for specific context
Date: Sun, 10 Nov 2019 09:14:44 -0800	[thread overview]
Message-ID: <20191110171444.GB65581@orsosgc001.amr.corp.intel.com> (raw)
In-Reply-To: <76017001-c4aa-089e-367d-11a9d45496af@intel.com>

On Fri, Nov 08, 2019 at 09:22:00AM +0200, Lionel Landwerlin wrote:
>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.
>

Offset is correct.

When I removed the configuration from lrc_configure_all_contexts(), the 
test broke :( 

Debugging further, it looks like OAR config (OAR_OACONTROL and 
RING_CONTEXT_CONTROL) must be applied to pinned ctx image as well as 
with LRI using kernel_context

For gen12, lrc_configure_all_contexts only needs to configure the stable 
power state.

Posting rev2, verified with test as posted here - 
https://patchwork.freedesktop.org/patch/339514/?series=69164&rev=1 

The expectation is that oar counters are saved and restored only for the 
context passed in perf open ioctl. Also, if some other context issues 
MI_RPC, it should get valid timestamps, context id etc.., but zeroed 
counter reports. Let me know if this is not the right understanding. 

>
>
>>  	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.
>

The TGL support patch already makes 'not sampling the OA buffer' 
non-privileged. These changes are only clearing the path. I have moved 
them to a separate patch in v2.

Thanks,
Umesh

>
>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