Re: [PATCH 5/5] drm/i915: Try to print INSTDONE bits for all slice/subslice

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 2015-10-01 at 16:00 -0700, Ben Widawsky wrote:
> On Wed, Sep 30, 2015 at 11:00:46PM +0300, Imre Deak wrote:
> > From: Ben Widawsky <benjamin.widawsky@xxxxxxxxx>
> > 
> > Signed-off-by: Ben Widawsky <ben@xxxxxxxxxxxx>
> > 
> > ---
> > Changes (Imre):
> > - use the new INSTDONE capturing by default on new GENs (On Ben's request)
> > - keep printing the render ring INSTDONE to dmesg
> > - don't hard code the extra_instdone array sizes
> > - fix typo in GEN8_MCR_SLICE/GEN8_MCR_SUBSLICE
> > - fix typo when capturing to extra->row
> > - warn if the MCR selectors are non-zero
> > ---
> >  drivers/gpu/drm/i915/i915_drv.h       |  6 ++--
> >  drivers/gpu/drm/i915/i915_gpu_error.c | 62 +++++++++++++++++++++++++++++++----
> >  drivers/gpu/drm/i915/i915_irq.c       |  4 +--
> >  drivers/gpu/drm/i915/i915_reg.h       |  5 +++
> >  4 files changed, 66 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 621acf1..d1b4011 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -513,10 +513,12 @@ struct drm_i915_error_state {
> >  	u32 gam_ecochk;
> >  	u32 gab_ctl;
> >  	u32 gfx_mode;
> > +#define INSTDONE_SLICE_NUM 3
> > +#define INSTDONE_SUBSLICE_NUM 3
> >  	struct extra_instdone {
> >  		u32 slice_common;
> > -		u32 sampler;
> > -		u32 row;
> > +		u32 sampler[INSTDONE_SLICE_NUM][INSTDONE_SUBSLICE_NUM];
> > +		u32 row[INSTDONE_SLICE_NUM][INSTDONE_SUBSLICE_NUM];
> >  	} extra_instdone;
> >  
> >  	u64 fence[I915_MAX_NUM_FENCES];
> > diff --git a/drivers/gpu/drm/i915/i915_gpu_error.c b/drivers/gpu/drm/i915/i915_gpu_error.c
> > index e78e512..c6d1cbc 100644
> > --- a/drivers/gpu/drm/i915/i915_gpu_error.c
> > +++ b/drivers/gpu/drm/i915/i915_gpu_error.c
> > @@ -336,7 +336,7 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	struct drm_i915_error_state *error = error_priv->error;
> >  	struct drm_i915_error_object *obj;
> > -	int i, j, offset, elt;
> > +	int i, j, slice, subslice, offset, elt;
> >  	int max_hangcheck_score;
> >  
> >  	if (!error) {
> > @@ -385,9 +385,17 @@ int i915_error_state_to_str(struct drm_i915_error_state_buf *m,
> >  
> >  	err_printf(m, "  SC_INSTDONE (slice common): 0x%08x\n",
> >  		   error->extra_instdone.slice_common);
> > -	err_printf(m, "  SAMPLER_INTSDONE: 0x%08x\n",
> > -		   error->extra_instdone.sampler);
> > -	err_printf(m, "  ROW_INSTDONE: 0x%08x\n", error->extra_instdone.row);
> > +	for (slice = 0; slice < INSTDONE_SLICE_NUM; slice++) {
> 
> Ideally we should only be doing this for the active slices. 

Looking at it now the spec is not too clear to me:
"""
When slice 0 is disabled (when fuse reflection MMADR 0x9120[25] = 0),
this field must be set to a valid slice (slice 1 or slice 2) before
issuing a read to a register in a slice unit.
"""

Why is slice 0 special? I would understand the above if it just said
"the field must be set to a valid slice", without mentioning specific
slices.

But yes, I think it would be safer to access only active slices. I would
also limit the readout to active subslices, although there is no similar
note about it in bspec.

> I was hoping you'd have a chance to look into that.

Ok, I'll resend this patch with the above changed.

>  Do we know if this works on SKUs with fewer slices?

It worked on CHV and BXT.

> > +		for (subslice = 0; subslice < INSTDONE_SUBSLICE_NUM;
> > +		     subslice++) {
> > +			struct extra_instdone *extra = &error->extra_instdone;
> > +
> > +			err_printf(m, "  SAMPLER_INTSDONE: 0x%08x\n",
> > +				   extra->sampler[slice][subslice]);
> > +			err_printf(m, "  ROW_INSTDONE: 0x%08x\n",
> > +				   extra->row[slice][subslice]);
> > +		}
> > +	}
> >  
> >  	if (INTEL_INFO(dev)->gen >= 6) {
> >  		err_printf(m, "ERROR: 0x%08x\n", error->error);
> > @@ -1383,11 +1391,35 @@ const char *i915_cache_level_str(struct drm_i915_private *i915, int type)
> >  	}
> >  }
> >  
> > -/* NB: please notice the memset */
> > +static inline uint32_t instdone_read(struct drm_i915_private *dev_priv, int
> > +				     slice, int subslice, uint32_t offset) {
> > +	/*
> > +	 * XXX: The MCR register should be locked, but since we are only using
> > +	 * it for debug/error state, it's not terribly important to
> > +	 * synchronize it properly.
> > +	 */
> > +	uint32_t tmp = I915_READ(GEN8_MCR_SELECTOR);
> > +	uint32_t ret;
> > +
> > +	/*
> > +	 * The HW expects the slice and sublice selectors to be reset to 0
> > +	 * after reading out the registers.
> > +	 */
> > +	WARN_ON_ONCE(tmp & (GEN8_MCR_SLICE_MASK | GEN8_MCR_SUBSLICE_MASK));
> > +
> > +	I915_WRITE(GEN8_MCR_SELECTOR, tmp | GEN8_MCR_SLICE(slice) |
> > +					    GEN8_MCR_SUBSLICE(subslice));
> > +	ret = I915_READ(offset);
> > +	I915_WRITE(GEN8_MCR_SELECTOR, tmp);
> > +
> > +	return ret;
> > +}
> > +
> >  void i915_get_extra_instdone(struct drm_device *dev,
> >  			     struct extra_instdone *extra)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	int slice, subslice;
> >  
> >  	/*
> >  	 * The render INSTDONE register (GEN2_INSTDONE, RING_INSTDONE) is read
> > @@ -1396,8 +1428,24 @@ void i915_get_extra_instdone(struct drm_device *dev,
> >  	switch (INTEL_INFO(dev)->gen) {
> >  	default:
> >  		extra->slice_common = I915_READ(GEN7_SC_INSTDONE);
> > -		extra->sampler = I915_READ(GEN7_SAMPLER_INSTDONE);
> > -		extra->row = I915_READ(GEN7_ROW_INSTDONE);
> > +		for (slice = 0; slice < INSTDONE_SLICE_NUM; slice++) {
> > +			for (subslice = 0; subslice < INSTDONE_SUBSLICE_NUM;
> > +			     subslice++) {
> > +				extra->sampler[slice][subslice] =
> > +					instdone_read(dev_priv,
> > +						      slice, subslice,
> > +						      GEN7_SAMPLER_INSTDONE);
> > +				extra->row[slice][subslice] =
> > +					instdone_read(dev_priv,
> > +						      slice, subslice,
> > +						      GEN7_ROW_INSTDONE);
> > +			}
> > +		}
> > +		break;
> > +	case 7:
> > +		extra->slice_common = I915_READ(GEN7_SC_INSTDONE);
> > +		extra->sampler[0][0] = I915_READ(GEN7_SAMPLER_INSTDONE);
> > +		extra->row[0][0] = I915_READ(GEN7_ROW_INSTDONE);
> >  		break;
> >  	case 6:
> >  	case 5:
> > diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
> > index 8a3dc73..3cfcd1f 100644
> > --- a/drivers/gpu/drm/i915/i915_irq.c
> > +++ b/drivers/gpu/drm/i915/i915_irq.c
> > @@ -2478,8 +2478,8 @@ i915_err_print_instdone(uint32_t render, struct extra_instdone *extra)
> >  {
> >  	pr_err("  INSTDONE (render): 0x%08x\n", render);
> >  	pr_err("  INSTDONE (common): 0x%08x\n", extra->slice_common);
> > -	pr_err("  INSTDONE (sampler): 0x%08x\n", extra->sampler);
> > -	pr_err("  INSTDONE (row): 0x%08x\n", extra->row);
> > +	pr_err("  INSTDONE (sampler): 0x%08x\n", extra->sampler[0][0]);
> > +	pr_err("  INSTDONE (row): 0x%08x\n", extra->row[0][0]);
> >  }
> >  
> >  static void i915_report_and_clear_eir(struct drm_device *dev)
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index ac5f49a..2412ec7 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -1596,6 +1596,11 @@ enum skl_disp_power_wells {
> >  #define GEN7_SC_INSTDONE	0x07100
> >  #define GEN7_SAMPLER_INSTDONE	0x0e160
> >  #define GEN7_ROW_INSTDONE	0x0e164
> > +#define GEN8_MCR_SELECTOR	0xfdc
> > +#define   GEN8_MCR_SLICE(slice)		(((slice) & 3) << 26)
> > +#define   GEN8_MCR_SLICE_MASK		(GEN8_MCR_SLICE(3))
> > +#define   GEN8_MCR_SUBSLICE(slice)	(((slice) & 3) << 24)
> > +#define   GEN8_MCR_SUBSLICE_MASK	(GEN8_MCR_SUBSLICE(3))
> >  #define RING_IPEIR(base)	((base)+0x64)
> >  #define RING_IPEHR(base)	((base)+0x68)
> >  /*
> > -- 
> > 2.1.4
> > 
> 


_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux