On Tue, 2019-05-07 at 14:16 -0700, Daniele Ceraolo Spurio wrote:
> <snip>
>
> > >
> > > > --- a/drivers/gpu/drm/i915/intel_device_info.c
> > > > +++ b/drivers/gpu/drm/i915/intel_device_info.c
> > > > @@ -84,17 +84,46 @@ void intel_device_info_dump_flags(const
> > > > struct
> > > > intel_device_info *info,
> > > > #undef PRINT_FLAG
> > > > }
> > > >
> > > > +#define SS_STR_MAX_SIZE (GEN_MAX_SUBSLICE_STRIDE * 2 + 1)
> > > > +
> > > > +static char *
> > > > +subslice_per_slice_str(char *buf, u8 size, const struct
> > > > sseu_dev_info *sseu,
> > > > + u8 slice)
> > > > +{
> > > > + int i;
> > > > + u8 ss_offset = slice * sseu->ss_stride;
> > > > +
> > > > + GEM_BUG_ON(slice >= sseu->max_slices);
> > > > +
> > > > + /* Two ASCII character hex plus null terminator */
> > > > + GEM_BUG_ON(size < sseu->ss_stride * 2 + 1);
> > > > +
> > > > + memset(buf, 0, size);
> > > > +
> > > > + /*
> > > > + * Print subslice information in reverse order to match
> > > > + * userspace expectations.
> > > > + */
> > > > + for (i = 0; i < sseu->ss_stride; i++)
> > > > + sprintf(&buf[i * 2], "%02x",
> > > > + sseu->subslice_mask[ss_offset + sseu-
> > > > >ss_stride
> > > > -
> > > > + (i + 1)]);
> > > > +
> > > > + return buf;
> > > > +}
> > > > +
> > > > static void sseu_dump(const struct sseu_dev_info *sseu,
> > > > struct
> > > > drm_printer *p)
> > > > {
> > > > int s;
> > > > + char buf[SS_STR_MAX_SIZE];
> > > >
> > > > drm_printf(p, "slice total: %u, mask=%04x\n",
> > > > hweight8(sseu->slice_mask), sseu-
> > > > >slice_mask);
> > > > drm_printf(p, "subslice total: %u\n",
> > > > intel_sseu_subslice_total(sseu));
> > > > for (s = 0; s < sseu->max_slices; s++) {
> > > > - drm_printf(p, "slice%d: %u subslices,
> > > > mask=%04x\n",
> > > > + drm_printf(p, "slice%d: %u subslices,
> > > > mask=%s\n",
> > > > s,
> > > > intel_sseu_subslices_per_slice(sseu, s),
> > > > - sseu->subslice_mask[s]);
> > > > + subslice_per_slice_str(buf,
> > > > ARRAY_SIZE(buf),
> > > > sseu, s));
> > >
> > > Now that we have intel_sseu_get_subslices() can't we just print
> > > the
> > > return from that instead of using the buffer?
> >
> > I personally would prefer we keep the stringify function as it
> > gives a
> > little more flexibility. Do you have a strong preference to move to
> > a
> > direct printk formatted string?
> >
>
> I do not, it just seemed like duplication since you're not really
> using
> any extra formatting or other flexibility in filling the buffer.
> This
> isn't a lot of code, so maybe we can switch to just using the u32
> for
> now and add this back if/when we do require the flexibility?
Makes sense and thanks for the feedback. I'll revert back to the printk
format.
>
> > >
> > >
> > > > }
> > > > drm_printf(p, "EU total: %u\n", sseu->eu_total);
> > > > drm_printf(p, "EU per subslice: %u\n", sseu-
> > > > >eu_per_subslice);
> > >
> > > <snip>
> > >
> > > > @@ -555,6 +570,7 @@ static void haswell_sseu_info_init(struct
> > > > drm_i915_private *dev_priv)
> > > > struct sseu_dev_info *sseu = &RUNTIME_INFO(dev_priv)-
> > > > >sseu;
> > > > u32 fuse1;
> > > > int s, ss;
> > > > + u32 subslice_mask;
> > > >
> > > > /*
> > > > * There isn't a register to tell us how many
> > > > slices/subslices.
> > > > We
> > > > @@ -566,22 +582,18 @@ static void haswell_sseu_info_init(struct
> > > > drm_i915_private *dev_priv)
> > > > /* fall through */
> > > > case 1:
> > > > sseu->slice_mask = BIT(0);
> > > > - sseu->subslice_mask[0] = BIT(0);
> > > > + subslice_mask = BIT(0);
> > > > break;
> > > > case 2:
> > > > sseu->slice_mask = BIT(0);
> > > > - sseu->subslice_mask[0] = BIT(0) | BIT(1);
> > > > + subslice_mask = BIT(0) | BIT(1);
> > > > break;
> > > > case 3:
> > > > sseu->slice_mask = BIT(0) | BIT(1);
> > > > - sseu->subslice_mask[0] = BIT(0) | BIT(1);
> > > > - sseu->subslice_mask[1] = BIT(0) | BIT(1);
> > > > + subslice_mask = BIT(0) | BIT(1);
> > > > break;
> > > > }
> > > >
> > > > - sseu->max_slices = hweight8(sseu->slice_mask);
> > > > - sseu->max_subslices = hweight8(sseu->subslice_mask[0]);
> > > > -
> > > > fuse1 = I915_READ(HSW_PAVP_FUSE1);
> > > > switch ((fuse1 & HSW_F1_EU_DIS_MASK) >>
> > > > HSW_F1_EU_DIS_SHIFT) {
> > > > default:
> > > > @@ -598,9 +610,14 @@ static void haswell_sseu_info_init(struct
> > > > drm_i915_private *dev_priv)
> > > > sseu->eu_per_subslice = 6;
> > > > break;
> > > > }
> > > > - sseu->max_eus_per_subslice = sseu->eu_per_subslice;
> > > > +
> > > > + intel_sseu_set_info(sseu, hweight8(sseu->slice_mask),
> > > > + hweight8(subslice_mask),
> > > > + sseu->eu_per_subslice);
> > >
> > > I'd still prefer this to use a local variable so that we always
> > > only
> > > set
> > > sseu->eu_per_subslice from within intel_sseu_set_info.
> >
> > So the reason I kept this is in intel_sseu_set_info we are really
> > just
> > setting the max_eus_per_subslice, not the eu_per_subslice. Are you
> > saying you'd also like to move the code that sets eu_per_subslice
> > in
> > each generation's handler to local variables and/or just passed
> > directly as an argument to intel_sseu_set_info?
>
> My bad, I confused eu_per_subslice and max_eus_per_subslice as the
> same
> variable. Just ignore this comment :)
No problem, thanks!
-Stuart
>
> Daniele
>
> >
> > I.e. should we use intel_sseu_set_info to set most or all of the
> > members of the intel_sseu structure? Or is it OK to keep the
> > current
> > implementation of only using this to set default maximums per
> > platform?
> >
> > -Stuart
> >
> > >
> > > Daniele
> > >
> > > >
> > > > for (s = 0; s < sseu->max_slices; s++) {
> > > > + intel_sseu_set_subslices(sseu, s,
> > > > subslice_mask);
> > > > +
> > > > for (ss = 0; ss < sseu->max_subslices; ss++) {
> > > > intel_sseu_set_eus(sseu, s, ss,
> > > > (1UL << sseu-
> > > > > eu_per_subslice) - 1);
smime.p7s
Description: S/MIME cryptographic signature
_______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx
