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:
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
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.
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 Fri, Jan 21, 2022 at 09:28:46AM +0200, Jani Nikula wrote:
On Thu, 20 Jan 2022, Matthew Brost matthew.brost@intel.com wrote:
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
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.
Sure, but I feel like I've actually been by someone else to not use the IS_ENABLED macro and use ifdefs inlines...
Matt
BR, Jani.
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);
-- Jani Nikula, Intel Open Source Graphics Center
On Fri, 21 Jan 2022, Matthew Brost matthew.brost@intel.com wrote:
On Fri, Jan 21, 2022 at 09:28:46AM +0200, Jani Nikula wrote:
On Thu, 20 Jan 2022, Matthew Brost matthew.brost@intel.com wrote:
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
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.
Sure, but I feel like I've actually been by someone else to not use the IS_ENABLED macro and use ifdefs inlines...
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.
Matt
BR, Jani.
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);
-- Jani Nikula, Intel Open Source Graphics Center
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