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

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

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