On 12/01/2018 13:53, Lionel Landwerlin wrote:
On 12/01/18 12:01, Tvrtko Ursulin wrote:
On 12/01/2018 10:58, Lionel Landwerlin wrote:
On 12/01/18 10:15, Tvrtko Ursulin wrote:
[snip]
+static inline int sseu_eu_idx(const struct sseu_dev_info *sseu,
+ int slice, int subslice, int eu_group)
What is eu_group for? Will it be used at some point?
In case we ever have more than 8 EUs per subslice.
I am thinking if we could hide that from the call sites, to avoid it
being passed as zeros, and to avoid having to write loops in other
patches which reference eu_groups, when it is not immediately obvious
what that means.
Could we for instance have a helper which would clear/set numbered EUs
in sseu_dev_info, and so hide all the implementation details?
sseu_enable_eus(sseu, slice, subslice, start, end);
Then when you have code like:
sseu->eu_mask[sseu_eu_idx(sseu, s, ss, 0)] = ~eu_disabled_mask;
You would write it as:
/* On this slice/subslice mark EUs 0 to N as enabled. */
sseu_enable_eus(sseu, s, ss, 0, fls(~eu_disabled_mask));
Hmm... I don't think that works if you have gaps, right?
Like a BXT 2x6 where a row of EUs has been fused off. It would be
something like 0b01110111 or 0b10111011.
Oops, you are right. I just don't like having this eu_group field which
is internal data storage detail, leaked out, is always zero, and even
unused today. So I am trying to come up with a nicer api.
What about sseu_set_eus(sseu, slice, subslice, mask)?
Then you can replace call sites like:
sseu->eu_mask[sseu_eu_idx(sseu, 0, 0, 0)] =
~((fuse & CHV_FGT_EU_DIS_SS0_R0_MASK) >>
CHV_FGT_EU_DIS_SS0_R0_SHIFT);
sseu->eu_mask[sseu_eu_idx(sseu, 0, 0, 0)] |=
~(((fuse & CHV_FGT_EU_DIS_SS0_R1_MASK) >>
CHV_FGT_EU_DIS_SS0_R1_SHIFT) << 4);
With:
mask = (fuse & CHV_FGT_EU_DIS_SS0_R0_MASK) >>
CHV_FGT_EU_DIS_SS0_R0_SHIFT;
mask |= ((fuse & CHV_FGT_EU_DIS_SS0_R1_MASK) >>
CHV_FGT_EU_DIS_SS0_R1_SHIFT) << 4);
sseu_set_eus(sseu, 0, 0, ~mask);
sseu_set_eus can then be the only place which understand the storage
format. You can even later extend the mask size past u8 and it can still
do the magic inside it.
Helper would internally know the size of the underlying storage and
dtrt. There would be no need to manually manage eu_groups. In the
initial implementation you could simply GEM_BUG_ON if the EU range
does not fit into the current storage. Later u8 could be turned into
u16 or similar. You also wouldn't have any iteration over eu_groups in
this version.
I think that would be cleaner and easier to extend in the future.
Unless I overlooked some important detail?
Or even simplify it by passing bitmask instead of start/end, and just
have no support for more than 8 EUs in this version? No eu_group etc.
When the need arises to have more, bump the eu_mask type to u16. That
would require you to put back the stride parameter in the uAPI I think.
I'm not really a fan of having the data field in userspace be
reinterpreted (as u16 or u32) based on one of the other field.
It might be easier on the kernel side, but complicates userspace.
I would prefer to stick to u8 and have everybody think of
slice/subslice/eus availability as array of u8 bit fields which you
might need to iterate more than one if there are more than 8 elements.
You are thinking about bit-ordering? Yeah, if it is easier keep the
formats the same. Even though strictly speaking internal and ABI data
representation do not need to be the same. As said above, I am just
looking for a solution which hides the eu_group parameter from the
callers and also is a bit shorter to read.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
