On 10/11/2019 19:14, Umesh Nerlige Ramappa wrote:
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@xxxxxxxxx>
---
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
We must be missing something, because that doesn't make sense.
OAR_OACONTROL ought to be needed only for pinned_ctx.
If we program it through gen12_emit_oar_config() there is no need to do
it in lrc_configure_all_contexts() as well.
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.
If I remember correctly, the documentation says MI_RPC results are
undefined if OAR is not enabled.
So I wouldn't even expect timestamps/context-id to be valid.
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.
Oh... Thanks I didn't remember that.
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
