[Intel-gfx] [PATCH v2] drm/i915/bdw: Check for slice, subslice and EU count for BDW

[Daniluk, Lukasz]

> -----Original Message-----
> From: Chris Wilson [mailto:chris at chris-wilson.co.uk]
> Sent: Wednesday, September 2, 2015 16:03
> To: Daniluk, Lukasz
> Cc: intel-gfx at lists.freedesktop.org
> Subject: Re: [Intel-gfx] [PATCH v2] drm/i915/bdw: Check for slice, subslice and
> EU count for BDW
> 
> On Wed, Sep 02, 2015 at 05:47:58PM +0200, Łukasz Daniluk wrote:
> > +static void broadwell_sseu_device_status(struct drm_device *dev,
> 
> Why pass in dev if you only use dev_priv (and commit the sin of repeatedly
> retrieving dev->dev_priv)?

I was following the style of other _sseu_device_status functions. 
Also, I use INTEL_INFO that wants dev argument.

> 
> > +					 struct sseu_dev_status *stat)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +	int s;
> > +	u32 slice_info = I915_READ(GEN8_GT_SLICE_INFO);
> > +
> > +	stat->slice_total =
> > +		hweight32(slice_info & GEN8_LSLICESTAT_MASK);
> > +
> > +	if (stat->slice_total)
> > +	{
> Missed.

Do you mean anything else than bracket here?

> 
> > +		stat->subslice_per_slice = INTEL_INFO(dev)->subslice_per_slice;
> > +		stat->eu_per_subslice = INTEL_INFO(dev)->eu_per_subslice;
> > +		stat->subslice_total = stat->slice_total * stat-
> >subslice_per_slice;
> > +		stat->eu_total = stat->eu_per_subslice * stat->subslice_total;
> > +
> > +		/* subtract fused off EU(s) from enabled slice(s) */
> > +		for (s = 0; s < stat->slice_total; s++) {
> > +			u8 subslice_7eu = INTEL_INFO(dev)->subslice_7eu[s];
> > +			stat->eu_total -= hweight8(subslice_7eu);
> > +		}
> 
> So why are we computing this twice? Is the debugfs to show the hw registers or
> the sw tracking? Where do I see what we actually report? If it is for the hw
> registers, report them. A perfect function here would report the actual register
> values (possibly decoding them) and show what the internal values are.
> -Chris
> 
> --
> Chris Wilson, Intel Open Source Technology Centre

Nothing is computed twice here - all values that are unchanged are read using INTEL_INFO macro.
What is computed however is amount of active (not simply present on the system) subslices and EUs. 
These values depend on number of active slices. The for loop checks if the one of the active slice is 
the one with EU fused off for die recovery. When you read i915_sseu_status the first part (SSEU Device 
Info) gives you information on general capabilities (things that can be read using INTEL_INFO(dev)), 
the second part (SSEU Device Status) is giving you the contents of stat struct.

---
Łukasz Daniluk