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

[Matthew Brost <matthew.brost@intel.com>]

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