Re: [PATCH 1/7] drm/i915: sseu: move sseu_dev_status to i915_drv.h

[Jeff McGee <jeff.mcgee@intel.com>]

On Thu, Nov 19, 2015 at 03:10:14PM -0800, Ben Widawsky wrote:
> On Wed, Oct 21, 2015 at 06:40:31PM +0300, Imre Deak wrote:
> > The data in this struct is provided both by getting the
> > slice/subslice/eu features available on a given platform and the actual
> > runtime state of these same features which depends on the HW's current
> > power saving state.
> > 
> > Atm members of this struct are duplicated in sseu_dev_status and
> I> intel_device_info. For clarity and code reuse we can share one struct
> > for both of the above purposes. This patch only moves the struct to the
> > header file, the next patch will convert users of intel_device_info to
> > use this struct too.
> > 
> > Instead of unsigned int u8 is used now, which is big enough and is used
> > anyway in intel_device_info.
> > 
> > No functional change.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_debugfs.c | 29 ++++++++++++-----------------
> >  drivers/gpu/drm/i915/i915_drv.h     |  8 ++++++++
> >  2 files changed, 20 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> > index a3b22bd..3dd7076 100644
> > --- a/drivers/gpu/drm/i915/i915_debugfs.c
> > +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> > @@ -4945,16 +4945,8 @@ DEFINE_SIMPLE_ATTRIBUTE(i915_cache_sharing_fops,
> >  			i915_cache_sharing_get, i915_cache_sharing_set,
> >  			"%llu\n");
> >  
> > -struct sseu_dev_status {
> > -	unsigned int slice_total;
> > -	unsigned int subslice_total;
> > -	unsigned int subslice_per_slice;
> > -	unsigned int eu_total;
> > -	unsigned int eu_per_subslice;
> > -};
> > -
> >  static void cherryview_sseu_device_status(struct drm_device *dev,
> > -					  struct sseu_dev_status *stat)
> > +					  struct sseu_dev_info *stat)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	int ss_max = 2;
> > @@ -4980,13 +4972,14 @@ static void cherryview_sseu_device_status(struct drm_device *dev,
> >  			 ((sig1[ss] & CHV_EU210_PG_ENABLE) ? 0 : 2) +
> >  			 ((sig2[ss] & CHV_EU311_PG_ENABLE) ? 0 : 2);
> >  		stat->eu_total += eu_cnt;
> > -		stat->eu_per_subslice = max(stat->eu_per_subslice, eu_cnt);
> > +		stat->eu_per_subslice = max_t(unsigned int,
> > +					      stat->eu_per_subslice, eu_cnt);
> >  	}
> >  	stat->subslice_total = stat->subslice_per_slice;
> >  }
> >  
> >  static void gen9_sseu_device_status(struct drm_device *dev,
> > -				    struct sseu_dev_status *stat)
> > +				    struct sseu_dev_info *stat)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	int s_max = 3, ss_max = 4;
> > @@ -5040,18 +5033,20 @@ static void gen9_sseu_device_status(struct drm_device *dev,
> >  			eu_cnt = 2 * hweight32(eu_reg[2*s + ss/2] &
> >  					       eu_mask[ss%2]);
> >  			stat->eu_total += eu_cnt;
> > -			stat->eu_per_subslice = max(stat->eu_per_subslice,
> > -						    eu_cnt);
> > +			stat->eu_per_subslice = max_t(unsigned int,
> > +						      stat->eu_per_subslice,
> > +						      eu_cnt);
> >  		}
> >  
> >  		stat->subslice_total += ss_cnt;
> > -		stat->subslice_per_slice = max(stat->subslice_per_slice,
> > -					       ss_cnt);
> > +		stat->subslice_per_slice = max_t(unsigned int,
> > +						 stat->subslice_per_slice,
> > +						 ss_cnt);
> >  	}
> >  }
> >  
> >  static void broadwell_sseu_device_status(struct drm_device *dev,
> > -					 struct sseu_dev_status *stat)
> > +					 struct sseu_dev_info *stat)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	int s;
> > @@ -5079,7 +5074,7 @@ static int i915_sseu_status(struct seq_file *m, void *unused)
> >  {
> >  	struct drm_info_node *node = (struct drm_info_node *) m->private;
> >  	struct drm_device *dev = node->minor->dev;
> > -	struct sseu_dev_status stat;
> > +	struct sseu_dev_info stat;
> 
> If you're going through this rename pain with the type anyway you may as well
> s/stat/info.
> 
> Also, I never understood what "sseu" was supposed to be short for. The spec

I started the use of sseu here as abbreviation of slice/subslice/EU, as in
encapsulation of attributes of this hierarchy. Originally we just needed a
few summarized count values but there was intent to add lists of per-component
details as needed.
-Jeff

> calls these "global attributes" which is admittedly a way too generic name. As a
> suggestion based on hindsight, I believe the following would be a bit nicer.
> struct slice_attributes {
> 	u8 slice_count;
> 	u8 eu_total; /* This is sort of useless since if eu_total isn't trivially
> 		      * eu_per_subslice * subslice_count * slice_count, then we
> 		      * need to know exactly which subslice is missing EUs. */
> 	struct {
> 		u8 subslices_per_slice;
> 		u8 eu_count; /* XXX: see above comment */
> 	} subslice;
> 
> 	#define subslice.total (subslices_per_slice * slice_count)
> }
> 
> 
> Just a thought. What you have is fine too though:
> Reviewed-by: Ben Widawsky <benjamin.widawsky@intel.com>
> 
> >  
> >  	if (INTEL_INFO(dev)->gen < 8)
> >  		return -ENODEV;
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 8afda45..73ff01f 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -782,6 +782,14 @@ struct intel_csr {
> >  #define DEFINE_FLAG(name) u8 name:1
> >  #define SEP_SEMICOLON ;
> >  
> > +struct sseu_dev_info {
> > +	u8 slice_total;
> > +	u8 subslice_total;
> > +	u8 subslice_per_slice;
> > +	u8 eu_total;
> > +	u8 eu_per_subslice;
> > +};
> > +
> >  struct intel_device_info {
> >  	u32 display_mmio_offset;
> >  	u16 device_id;
> 
> 
> -- 
> Ben Widawsky, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx