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));
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.
Regards,
Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
