[Mesa-dev] [PATCH v2 2/8] intel: devinfo: store slice/subslice/eu masks

[Lionel Landwerlin]

On 19/03/18 23:56, Kenneth Graunke wrote:
> On Wednesday, March 14, 2018 10:19:08 AM PDT Lionel Landwerlin wrote:
>> We want to store values coming from the kernel but as a first step, we
>> can generate mask values out the numbers already stored in the
>> gen_device_info masks.
>>
>> Signed-off-by: Lionel Landwerlin <lionel.g.landwerlin at intel.com>
>> ---
>>   src/intel/dev/gen_device_info.c | 43 +++++++++++++++++++++++++++++++++++++++++
>>   src/intel/dev/gen_device_info.h | 39 ++++++++++++++++++++++++++++++++++++-
>>   2 files changed, 81 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/intel/dev/gen_device_info.c b/src/intel/dev/gen_device_info.c
>> index 26c2651f0ff..c1bdc997f2c 100644
>> --- a/src/intel/dev/gen_device_info.c
>> +++ b/src/intel/dev/gen_device_info.c
>> @@ -872,6 +872,47 @@ static const struct gen_device_info gen_device_info_icl_1x8 = {
>>      GEN11_FEATURES(1, 1, subslices(1), 6),
>>   };
>>   
>> +/* Generate slice/subslice/eu masks from number of
>> + * slices/subslices/eu_per_subslices in the per generation/gt gen_device_info
>> + * structure.
>> + *
>> + * These can be overridden with values reported by the kernel either from
>> + * getparam SLICE_MASK/SUBSLICE_MASK values or from the kernel version 4.17+
>> + * through the i915 query uapi.
>> + */
>> +static void
>> +fill_masks(struct gen_device_info *devinfo)
>> +{
>> +   devinfo->slice_masks = (1UL << devinfo->num_slices) - 1;
> L doesn't make much sense here...num_slices is 'unsigned' rather than
> 'unsigned long', and the result is a uint8_t.  I'd just use 1 or 1U.

Sure.

>
>> +   /* Subslice masks */
>> +   unsigned max_subslices = 0;
>> +   for (int s = 0; s < devinfo->num_slices; s++)
>> +      max_subslices = MAX2(devinfo->num_subslices[s], max_subslices);
>> +   devinfo->subslice_slice_stride = DIV_ROUND_UP(max_subslices, 8);
>> +
>> +   for (int s = 0; s < devinfo->num_slices; s++) {
>> +      devinfo->subslice_masks[s * devinfo->subslice_slice_stride] =
>> +         (1UL << devinfo->num_subslices[s]) - 1;
>> +   }
>> +
>> +   /* EU masks */
>> +   devinfo->eu_subslice_stride = DIV_ROUND_UP(devinfo->num_eu_per_subslice, 8);
>> +   devinfo->eu_slice_stride = max_subslices * devinfo->eu_subslice_stride;
>> +
>> +   for (int s = 0; s < devinfo->num_slices; s++) {
>> +      for (int ss = 0; ss < devinfo->num_subslices[s]; ss++) {
>> +         for (int b_eu = 0; b_eu < devinfo->eu_subslice_stride; b_eu++) {
>> +            int subslice_offset =
>> +               s * devinfo->eu_slice_stride + ss * devinfo->eu_subslice_stride;
>> +
>> +            devinfo->eu_masks[subslice_offset + b_eu] =
>> +               (((1UL << devinfo->num_eu_per_subslice) - 1) >> (b_eu * 8)) & 0xff;
>> +         }
>> +      }
>> +   }
>> +}
>> +
> This is kind of messy, both to create and to use...I almost wonder if
> what we really want is helper functions to get a particular mask...

You're right!
I even wrote the same helper in the kernel, duh! : 
https://cgit.freedesktop.org/drm-tip/tree/drivers/gpu/drm/i915/intel_device_info.h#n226

>
> But there aren't many users, so it doesn't seem -too- bad...
>
> Reviewed-by: Kenneth Graunke <kenneth at whitecape.org>