On Tue, Nov 22, 2022 at 07:25:36PM -0800, Dixit, Ashutosh wrote:
On Tue, 22 Nov 2022 18:07:00 -0800, Umesh Nerlige Ramappa wrote:
Hi Umesh,
An earlier commit introduced a mechanism to parse the context image to
find the OA context control offset. This resulted in an NPD on haswell
when gem_context was passed into i915_perf_open_ioctl params. Haswell
does not support logical ring contexts, so ensure that the context image
is parsed only for platforms with logical ring contexts and also
validate lrc_reg_state.
v2: Fix build failure
Closes: https://gitlab.freedesktop.org/drm/intel/-/issues/7432
Fixes: a5c3a3cbf029 ("drm/i915/perf: Determine gen12 oa ctx offset at runtime")
Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@xxxxxxxxx>
---
drivers/gpu/drm/i915/i915_perf.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i915/i915_perf.c b/drivers/gpu/drm/i915/i915_perf.c
index 00e09bb18b13..dbd785974f20 100644
--- a/drivers/gpu/drm/i915/i915_perf.c
+++ b/drivers/gpu/drm/i915/i915_perf.c
@@ -1383,6 +1383,9 @@ static u32 oa_context_image_offset(struct intel_context *ce, u32 reg)
u32 offset, len = (ce->engine->context_size - PAGE_SIZE) / 4;
u32 *state = ce->lrc_reg_state;
+ if (drm_WARN_ON(&ce->engine->i915->drm, state == NULL))
+ return U32_MAX;
+
So if we didn't add the HAS_LOGICAL_RING_CONTEXTS check below state would
be NULL correct? I couldn't figure out how it is NULL on HSW looking at the
code (with the context image pin/unpin).
Ref: intel_engines_init()
Haswell is using ring submission. While it does have a context image or
ce->state, there is no lrc state offset here. I think lrc is an execlist
concept (used only for execlist and guc submission).
/*
@@ -1447,7 +1450,8 @@ static int oa_get_render_ctx_id(struct i915_perf_stream *stream)
if (IS_ERR(ce))
return PTR_ERR(ce);
- if (engine_supports_mi_query(stream->engine)) {
+ if (engine_supports_mi_query(stream->engine) &&
+ HAS_LOGICAL_RING_CONTEXTS(stream->perf->i915)) {
This check looks fine since we seem to be looking inside ce->lrc_reg_state
for oactxctrl.
The earlier code also does not define perf->ctx_oactxctrl_offset for
haswell. Ref: i915_perf_init().
Overall looks fine so this is:
Reviewed-by: Ashutosh Dixit <ashutosh.dixit@xxxxxxxxx>
Thanks,
Umesh
/*
* We are enabling perf query here. If we don't find the context
* offset here, just return an error.
--
2.36.1
