Re: [PATCH 1/2] drm/i915/guc: Don't check CT descriptor status before CT write / read

[Jani Nikula <jani.nikula@linux.intel.com>]

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);

-- 
Jani Nikula, Intel Open Source Graphics Center