[Intel-gfx] [PATCH v2 1/6] drm/i915: store all subslice masks

[Lionel Landwerlin]

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.

>
> 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.

>
> Regards,
>
> Tvrtko
>