[Intel-gfx] [PATCH 8/9] drm/i915: Make intel_calculate_wm return unsigned int

[Tvrtko Ursulin]

On 10/10/2016 09:02, Joonas Lahtinen wrote:
> On pe, 2016-10-07 at 14:34 +0100, Tvrtko Ursulin wrote:
>> @@ -595,18 +595,17 @@ static unsigned long intel_calculate_wm(unsigned int clock_in_khz,
>>   	 * clocks go from a few thousand to several hundred thousand.
>>   	 * latency is usually a few thousand
>>   	 */
>> -	entries_required = ((clock_in_khz / 1000) * cpp * latency_ns) /
>> -		1000;
>> +	entries_required = ((clock_in_khz / 1000) * cpp * latency_ns) / 1000;
> Is the intermediary result always within int?

I certainly hope so! Display clock in MHz * cpp * latency which is u16. 
So 2^31 / 2^16 / cpp=8 leaves 4GHz for the clock before it would overflow.

>>   	entries_required = DIV_ROUND_UP(entries_required, wm->cacheline_size);
>>   
>> -	DRM_DEBUG_KMS("FIFO entries required for mode: %ld\n", entries_required);
>> +	DRM_DEBUG_KMS("FIFO entries required for mode: %d\n", entries_required);
>>   
>>   	wm_size = fifo_size - (entries_required + wm->guard_size);
>>   
>> -	DRM_DEBUG_KMS("FIFO watermark level: %ld\n", wm_size);
>> +	DRM_DEBUG_KMS("FIFO watermark level: %d\n", wm_size);
>>   
>>   	/* Don't promote wm_size to unsigned... */
>> -	if (wm_size > (long)wm->max_wm)
>> +	if (wm_size > (int)wm->max_wm)
>>   		wm_size = wm->max_wm;
>>   	if (wm_size <= 0)
>>   		wm_size = wm->default_wm;
> This could be
>
> 	if (wm_size <= 0)
> 		wm_size = wm->default_wm;
> 	else if (wm_size > U16_MAX || (u16)wm_size > wm->max_wm)
> 		wm_size = wm->max_wm;
>
> or something?
>
> Other than that, types look better that they used to.
>
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
>
> The whole type landscape in watermark code seems bit sloppy to me.

Yes, but I would also like not to introduce regressions.  So I am a bit 
reluctant to push forward with the second half of this series. :I

Regards,

Tvrtko