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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxx>
>
> >
> > 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
