On 14/09/15 10:21, Daniel Vetter wrote:
On Thu, Sep 10, 2015 at 03:07:00PM -0700, Yu Dai wrote:
Agree. The LRC prefix is confusing. Thanks for the patch. -Alex
Care to do an official r-b?
Thanks, Daniel
On 09/10/2015 02:58 PM, Jesse Barnes wrote:
That looks like it would, but I think it's still confusing to reference LRC state when we haven't initialized execlists at all...
Jesse
On 09/10/2015 02:56 PM, Yu Dai wrote:
Jesse,
Will the patch here fix the issue? It should help other cases where LRC_PPHWSP_PN is referenced on non-execlist / guc platforms.
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 4cc54b3..233a930 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -71,7 +71,7 @@ static inline void intel_logical_ring_emit(struct intel_ringbuffer *ringbuf,
/* One extra page is added before LRC for GuC as shared data */
#define LRC_GUCSHR_PN (0)
-#define LRC_PPHWSP_PN (LRC_GUCSHR_PN + 1)
+#define LRC_PPHWSP_PN (LRC_GUCSHR_PN + i915.enable_guc_submission ? 1 : 0)
#define LRC_STATE_PN (LRC_PPHWSP_PN + 1)
I don't like this approach of hiding the runtime conditional offset in
the macro. I think it would be better to leave this alone and use the
technique in the code below where the (const) macros named 'LRC_*' are
used only inside clauses guarded by an "if (i915.enable_execlists)".
That way the code won't mention the LRC_* symbols except where they're
known tp be meaningful.
Of these symbols, LRC_GUCSHR_PN is not used except in GuC-specific code,
and LRC_STATE_PN is used only in GuC and/or LRC (execlist-mode) specific
code. So it's only LRC_PPHWSP_PN that needs to be used selectively
(specifically, added to the base of a context object when using execlist
mode). And the code below already accomplishes that without changing the
macro.
void intel_lr_context_free(struct intel_context *ctx);
Thanks,
Alex
On 09/10/2015 02:40 PM, Jesse Barnes wrote:
Looks like this was introduced in:
commit d1675198ed1f21aec6e036336e4340c40b726497
Author: Alex Dai <yu.dai@xxxxxxxxx>
Date: Wed Aug 12 15:43:43 2015 +0100
drm/i915: Integrate GuC-based command submission
This patch assumed LRC contexts and HWS layout, which is incorrect on
platforms without execlists. This can lead to a crash in GPU error
state readout on those platforms.
I don't see a bug filed for this, but there may be one that I haven't
found.
Cc: Alex Dai <yu.dai@xxxxxxxxx>
Signed-off-by: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>
---
drivers/gpu/drm/i915/i915_gpu_error.c | 19 +++++++++++++------
1 file changed, 13 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
index 3379f9c..d0822f8 100644
--- a/drivers/gpu/drm/i915/i915_gpu_error.c
+++ b/drivers/gpu/drm/i915/i915_gpu_error.c
@@ -457,17 +457,24 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
}
if ((obj = error->ring[i].hws_page)) {
+ u64 hws_offset = lower_32_bits(obj->gtt_offset);
No need to take only lower 32 bits here, the variable is already u64 and
the format below is 0x%08llx, so we'll get a 32-bit value in the output
(unless it actually is bigger than 32 bits, which shouldn't happen and
which we'd want to know about!).
+ u32 *hws_page = &obj->pages[0][0];
+
+ if (i915.enable_execlists) {
+ hws_offset = obj->gtt_offset + LRC_PPHWSP_PN *
+ PAGE_SIZE;
If we keep the full 64-bit value (or even if we don't), this should
simplify to "hws_offset += LRC_PPHWSP_PN*PAGE_SIZE;"
+ hws_page = &obj->pages[LRC_PPHWSP_PN][0];
+ }
err_printf(m, "%s --- HW Status = 0x%08llx\n",
- dev_priv->ring[i].name,
- obj->gtt_offset + LRC_PPHWSP_PN * PAGE_SIZE);
+ dev_priv->ring[i].name, hws_offset);
offset = 0;
for (elt = 0; elt < PAGE_SIZE/16; elt += 4) {
err_printf(m, "[%04x] %08x %08x %08x %08x\n",
offset,
- obj->pages[LRC_PPHWSP_PN][elt],
- obj->pages[LRC_PPHWSP_PN][elt+1],
- obj->pages[LRC_PPHWSP_PN][elt+2],
- obj->pages[LRC_PPHWSP_PN][elt+3]);
+ hws_page[elt],
+ hws_page[elt+1],
+ hws_page[elt+2],
+ hws_page[elt+3]);
offset += 16;
}
}
With the 64-bit handling changed, I'd be happy to r-b Jesse's original
patch, but not Alex's redefinition of LRC_PPHWSP_PN.
Cheers,
.Dave.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
