Re: [PATCH v2 1/6] drm/i915: store all subslice masks

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx