A couple of minor CT updates. 1 for performance, 1 for extra debug.
Signed-off-by: Matthew Brost matthew.brost@intel.com
Matthew Brost (2): drm/i915/guc: Don't check CT descriptor status before CT write / read drm/i915/guc: Print CT descriptor status in CT debug function
drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
Don't check CT descriptor status, unless CONFIG_DRM_I915_DEBUG_GUC is set, before CT write / read as this could result in a read across the PCIe bus thus adding latency to every CT write / read. On well behavied systems this vaue should always read as zero. For some reason it doesn't the CT channel is broken and will eventually recover from a GT reset, albeit the GT reset will not be triggered immediately by seeing that descriptor status is non-zero.
v2: (CI) - Fix build error (hide corrupted label in write function behind CONFIG_DRM_I915_DEBUG_GUC)
Signed-off-by: Matthew Brost matthew.brost@intel.com --- drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 6 ++++++ 1 file changed, 6 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index de89d40abd38d..948cf31429412 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -379,8 +379,10 @@ static int ct_write(struct intel_guc_ct *ct, u32 *cmds = ctb->cmds; unsigned int i;
+#ifdef CONFIG_DRM_I915_DEBUG_GUC if (unlikely(desc->status)) goto corrupted; +#endif
GEM_BUG_ON(tail > size);
@@ -445,11 +447,13 @@ static int ct_write(struct intel_guc_ct *ct,
return 0;
+#ifdef CONFIG_DRM_I915_DEBUG_GUC corrupted: CT_ERROR(ct, "Corrupted descriptor head=%u tail=%u status=%#x\n", desc->head, desc->tail, desc->status); ctb->broken = true; return -EPIPE; +#endif }
/** @@ -815,8 +819,10 @@ static int ct_read(struct intel_guc_ct *ct, struct ct_incoming_msg **msg) if (unlikely(ctb->broken)) return -EPIPE;
+#ifdef CONFIG_DRM_I915_DEBUG_GUC if (unlikely(desc->status)) goto corrupted; +#endif
GEM_BUG_ON(head > size);
On Thu, 20 Jan 2022, Matthew Brost matthew.brost@intel.com wrote:
Please don't add #ifdefs inline. You can use IS_ENABLED(CONFIG_DRM_I915_DEBUG_GUC) in if statements, but otherwise the code needs to be split out to a separate function.
BR, Jani.
On Fri, 21 Jan 2022, Matthew Brost matthew.brost@intel.com wrote:
Citation needed.
Basically never use #if/#ifdef inline. Only use them at the top level like this:
#if IS_ENABLED(CONFIG_FOO) static int bar(void) { /* implementation with foo */ } #else static int bar(void) { /* implementation without foo */ } #endif
Sometimes you can avoid the above boilerplate with IS_ENABLED() inline:
if (IS_ENABLED(CONFIG_FOO)) ...
Basically if you think #if/#ifdef inline is the easiest, you need to refactor the code to do it cleanly without them.
BR, Jani.
Noticed that the CT descriptor status was not printed in the CT debug function, add that in.
Signed-off-by: Matthew Brost matthew.brost@intel.com --- drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c index 948cf31429412..5df2e3413796e 100644 --- a/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c +++ b/drivers/gpu/drm/i915/gt/uc/intel_guc_ct.c @@ -1219,10 +1219,14 @@ void intel_guc_ct_print_info(struct intel_guc_ct *ct, ct->ctbs.send.desc->head); drm_printf(p, "Tail: %u\n", ct->ctbs.send.desc->tail); + drm_printf(p, "Status: %u\n", + ct->ctbs.send.desc->status); drm_printf(p, "G2H Space: %u\n", atomic_read(&ct->ctbs.recv.space) * 4); drm_printf(p, "Head: %u\n", ct->ctbs.recv.desc->head); drm_printf(p, "Tail: %u\n", ct->ctbs.recv.desc->tail); + drm_printf(p, "Status: %u\n", + ct->ctbs.recv.desc->status); }
dri-devel@lists.freedesktop.org
-
Jani Nikula -
Matthew Brost