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

[Ben Widawsky <benjamin.widawsky@intel.com>]

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
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