Re: [Intel-gfx] [PATCH 05/13] drm/i915/dsb: Dump the DSB command buffer when DSB fails

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Thu, Feb 02, 2023 at 05:09:05PM +0000, Manna, Animesh wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-gfx <intel-gfx-bounces@lists.freedesktop.org> On Behalf Of Ville
> > Syrjala
> > Sent: Wednesday, January 18, 2023 10:01 PM
> > To: intel-gfx@lists.freedesktop.org
> > Subject: [Intel-gfx] [PATCH 05/13] drm/i915/dsb: Dump the DSB command
> > buffer when DSB fails
> > 
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > 
> > Dump the full DSB command buffers and head/tail pointers if the the DSB
> > hasn't completed its job in time.
> > 
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> >  drivers/gpu/drm/i915/display/intel_dsb.c | 33 +++++++++++++++++++++---
> >  1 file changed, 30 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c
> > b/drivers/gpu/drm/i915/display/intel_dsb.c
> > index 9e25b1345927..f454329b6901 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> > @@ -92,6 +92,22 @@ static bool assert_dsb_has_room(struct intel_dsb
> > *dsb)
> >  			 crtc->base.base.id, crtc->base.name, dsb->id);  }
> > 
> > +static void intel_dsb_dump(struct intel_dsb *dsb) {
> > +	struct intel_crtc *crtc = dsb->crtc;
> > +	struct drm_i915_private *i915 = to_i915(crtc->base.dev);
> > +	const u32 *buf = dsb->cmd_buf;
> > +	int i;
> > +
> > +	drm_dbg_kms(&i915->drm, "[CRTC:%d:%s] DSB %d commands {\n",
> > +		    crtc->base.base.id, crtc->base.name, dsb->id);
> > +	for (i = 0; i < ALIGN(dsb->free_pos, 64 / 4); i += 4)
> > +		drm_dbg_kms(&i915->drm,
> > +			    " 0x%08x: 0x%08x 0x%08x 0x%08x 0x%08x\n",
> > +			    i * 4, buf[i], buf[i+1], buf[i+2], buf[i+3]);
> > +	drm_dbg_kms(&i915->drm, "}\n");
> > +}
> > +
> >  static bool is_dsb_busy(struct drm_i915_private *i915, enum pipe pipe,
> >  			enum dsb_id id)
> >  {
> > @@ -260,10 +276,21 @@ void intel_dsb_wait(struct intel_dsb *dsb)
> >  	struct drm_i915_private *dev_priv = to_i915(crtc->base.dev);
> >  	enum pipe pipe = crtc->pipe;
> > 
> > -	if (wait_for(!is_dsb_busy(dev_priv, pipe, dsb->id), 1))
> > +	if (wait_for(!is_dsb_busy(dev_priv, pipe, dsb->id), 1)) {
> > +		u32 offset = i915_ggtt_offset(dsb->vma);
> > +
> > +		intel_de_write(dev_priv, DSB_CTRL(pipe, dsb->id),
> > +			       DSB_ENABLE | DSB_HALT);
> 
> One doubt - Why DSB_ENABLE bit is set here? Is setting DSB_HALT not sufficient.
> Other than above the changes look good to me.

Clearing the enable with would reset the DSB, so the halt would
seem superfluous in that case. And I *think* a reset does clear
DSB_CURRENT_HEAD so the debugs wouldn't provide any helpful data
in that case. I'll need to double check that I suppose, but at
least right now my DSB_CURRENT_HEAD does read zero while the DSB
isn't actively executing.

Though I suppose we could make do without the halt entirely and
just read the registers while the DSB might still be making
progress. Admittedly I suppose it still might be even with the
halt as I don't think the halt is instantaneous.

-- 
Ville Syrjälä
Intel