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 <[email protected]>
> ---
> 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 <[email protected]>
>
> 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
[email protected]
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
