Re: [PATCH 5/5] drm/i915: Expand subslice mask

[Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>]

<snip>

>>
>>> --- a/drivers/gpu/drm/i915/intel_device_info.c
>>> +++ b/drivers/gpu/drm/i915/intel_device_info.c
>>> @@ -84,17 +84,46 @@ void intel_device_info_dump_flags(const struct
>>> intel_device_info *info,
>>>    #undef PRINT_FLAG
>>>    }
>>>    
>>> +#define SS_STR_MAX_SIZE (GEN_MAX_SUBSLICE_STRIDE * 2 + 1)
>>> +
>>> +static char *
>>> +subslice_per_slice_str(char *buf, u8 size, const struct
>>> sseu_dev_info *sseu,
>>> +		       u8 slice)
>>> +{
>>> +	int i;
>>> +	u8 ss_offset = slice * sseu->ss_stride;
>>> +
>>> +	GEM_BUG_ON(slice >= sseu->max_slices);
>>> +
>>> +	/* Two ASCII character hex plus null terminator */
>>> +	GEM_BUG_ON(size < sseu->ss_stride * 2 + 1);
>>> +
>>> +	memset(buf, 0, size);
>>> +
>>> +	/*
>>> +	 * Print subslice information in reverse order to match
>>> +	 * userspace expectations.
>>> +	 */
>>> +	for (i = 0; i < sseu->ss_stride; i++)
>>> +		sprintf(&buf[i * 2], "%02x",
>>> +			sseu->subslice_mask[ss_offset + sseu->ss_stride
>>> -
>>> +					    (i + 1)]);
>>> +
>>> +	return buf;
>>> +}
>>> +
>>>    static void sseu_dump(const struct sseu_dev_info *sseu, struct
>>> drm_printer *p)
>>>    {
>>>    	int s;
>>> +	char buf[SS_STR_MAX_SIZE];
>>>    
>>>    	drm_printf(p, "slice total: %u, mask=%04x\n",
>>>    		   hweight8(sseu->slice_mask), sseu->slice_mask);
>>>    	drm_printf(p, "subslice total: %u\n",
>>> intel_sseu_subslice_total(sseu));
>>>    	for (s = 0; s < sseu->max_slices; s++) {
>>> -		drm_printf(p, "slice%d: %u subslices, mask=%04x\n",
>>> +		drm_printf(p, "slice%d: %u subslices, mask=%s\n",
>>>    			   s, intel_sseu_subslices_per_slice(sseu, s),
>>> -			   sseu->subslice_mask[s]);
>>> +			   subslice_per_slice_str(buf, ARRAY_SIZE(buf),
>>> sseu, s));
>>
>> Now that we have intel_sseu_get_subslices() can't we just print the
>> return from that instead of using the buffer?
> 
> I personally would prefer we keep the stringify function as it gives a
> little more flexibility. Do you have a strong preference to move to a
> direct printk formatted string?
> 

I do not, it just seemed like duplication since you're not really using 
any extra formatting or other flexibility in filling the buffer. This 
isn't a lot of code, so maybe we can switch to just using the u32 for 
now and add this back if/when we do require the flexibility?

>>
>>
>>>    	}
>>>    	drm_printf(p, "EU total: %u\n", sseu->eu_total);
>>>    	drm_printf(p, "EU per subslice: %u\n", sseu->eu_per_subslice);
>>
>> <snip>
>>
>>> @@ -555,6 +570,7 @@ static void haswell_sseu_info_init(struct
>>> drm_i915_private *dev_priv)
>>>    	struct sseu_dev_info *sseu = &RUNTIME_INFO(dev_priv)->sseu;
>>>    	u32 fuse1;
>>>    	int s, ss;
>>> +	u32 subslice_mask;
>>>    
>>>    	/*
>>>    	 * There isn't a register to tell us how many slices/subslices.
>>> We
>>> @@ -566,22 +582,18 @@ static void haswell_sseu_info_init(struct
>>> drm_i915_private *dev_priv)
>>>    		/* fall through */
>>>    	case 1:
>>>    		sseu->slice_mask = BIT(0);
>>> -		sseu->subslice_mask[0] = BIT(0);
>>> +		subslice_mask = BIT(0);
>>>    		break;
>>>    	case 2:
>>>    		sseu->slice_mask = BIT(0);
>>> -		sseu->subslice_mask[0] = BIT(0) | BIT(1);
>>> +		subslice_mask = BIT(0) | BIT(1);
>>>    		break;
>>>    	case 3:
>>>    		sseu->slice_mask = BIT(0) | BIT(1);
>>> -		sseu->subslice_mask[0] = BIT(0) | BIT(1);
>>> -		sseu->subslice_mask[1] = BIT(0) | BIT(1);
>>> +		subslice_mask = BIT(0) | BIT(1);
>>>    		break;
>>>    	}
>>>    
>>> -	sseu->max_slices = hweight8(sseu->slice_mask);
>>> -	sseu->max_subslices = hweight8(sseu->subslice_mask[0]);
>>> -
>>>    	fuse1 = I915_READ(HSW_PAVP_FUSE1);
>>>    	switch ((fuse1 & HSW_F1_EU_DIS_MASK) >> HSW_F1_EU_DIS_SHIFT) {
>>>    	default:
>>> @@ -598,9 +610,14 @@ static void haswell_sseu_info_init(struct
>>> drm_i915_private *dev_priv)
>>>    		sseu->eu_per_subslice = 6;
>>>    		break;
>>>    	}
>>> -	sseu->max_eus_per_subslice = sseu->eu_per_subslice;
>>> +
>>> +	intel_sseu_set_info(sseu, hweight8(sseu->slice_mask),
>>> +			    hweight8(subslice_mask),
>>> +			    sseu->eu_per_subslice);
>>
>> I'd still prefer this to use a local variable so that we always only
>> set
>> sseu->eu_per_subslice from within intel_sseu_set_info.
> 
> So the reason I kept this is in intel_sseu_set_info we are really just
> setting the max_eus_per_subslice, not the eu_per_subslice. Are you
> saying you'd also like to move the code that sets eu_per_subslice in
> each generation's handler to local variables and/or just passed
> directly as an argument to intel_sseu_set_info?

My bad, I confused eu_per_subslice and max_eus_per_subslice as the same 
variable. Just ignore this comment :)

Daniele

> 
> I.e. should we use intel_sseu_set_info to set most or all of the
> members of the intel_sseu structure? Or is it OK to keep the current
> implementation of only using this to set default maximums per platform?
> 
> -Stuart
> 
>>
>> Daniele
>>
>>>    
>>>    	for (s = 0; s < sseu->max_slices; s++) {
>>> +		intel_sseu_set_subslices(sseu, s, subslice_mask);
>>> +
>>>    		for (ss = 0; ss < sseu->max_subslices; ss++) {
>>>    			intel_sseu_set_eus(sseu, s, ss,
>>>    					   (1UL << sseu-
>>>> eu_per_subslice) - 1);
>>>
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx