Re: [PATCH 6/8] drm/i915/skl: Updated watermark programming

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

On 02/26/2015 04:45 PM, Damien Lespiau wrote:
> On Wed, Feb 25, 2015 at 04:47:22PM +0000, Tvrtko Ursulin wrote:
>> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>>
>> Recent BSpect updates have changed the watermark calculation to avoid
>> display flickering in some cases.
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>
> There are really several changes in this patch, it would have been
> easier for the review to split them (and that's usually how we are
> supposed to split patches).

 From my point of view - this patch changes the computation from what it 
was documented two weeks ago, to what was documented one week ago. I 
don't see that it would be natural to be so inventive to start splitting 
that.

>    - Convert the inner computations to number of blocks
>    - W/A to increase the nb of blocks for level 1-7 by 1
>    - Move max block test to >= instead of >
>
> Anyway, which the couple of comments below addressd (of which only the
> '>=' is a real problem), this is:
>
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

Will fix up the details and tag it then, thanks!

>>   drivers/gpu/drm/i915/intel_pm.c | 52 +++++++++++++++++++++++++----------------
>>   1 file changed, 32 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
>> index f7c9938..626c3c2 100644
>> --- a/drivers/gpu/drm/i915/intel_pm.c
>> +++ b/drivers/gpu/drm/i915/intel_pm.c
>> @@ -2595,7 +2595,7 @@ static uint32_t skl_wm_method1(uint32_t pixel_rate, uint8_t bytes_per_pixel,
>>   	if (latency == 0)
>>   		return UINT_MAX;
>>
>> -	wm_intermediate_val = latency * pixel_rate * bytes_per_pixel;
>> +	wm_intermediate_val = latency * pixel_rate * bytes_per_pixel / 512;
>>   	ret = DIV_ROUND_UP(wm_intermediate_val, 1000);
>>
>>   	return ret;
>> @@ -2605,15 +2605,18 @@ static uint32_t skl_wm_method2(uint32_t pixel_rate, uint32_t pipe_htotal,
>>   			       uint32_t horiz_pixels, uint8_t bytes_per_pixel,
>>   			       uint32_t latency)
>>   {
>> -	uint32_t ret, plane_bytes_per_line, wm_intermediate_val;
>> +	uint32_t ret;
>> +	uint32_t plane_bytes_per_line, plane_blocks_per_line;
>> +	uint32_t wm_intermediate_val;
>>
>>   	if (latency == 0)
>>   		return UINT_MAX;
>>
>>   	plane_bytes_per_line = horiz_pixels * bytes_per_pixel;
>> +	plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
>>   	wm_intermediate_val = latency * pixel_rate;
>>   	ret = DIV_ROUND_UP(wm_intermediate_val, pipe_htotal * 1000) *
>> -				plane_bytes_per_line;
>> +				plane_blocks_per_line;
>>
>>   	return ret;
>>   }
>> @@ -2693,39 +2696,47 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
>>   	}
>>   }
>>
>> -static bool skl_compute_plane_wm(struct skl_pipe_wm_parameters *p,
>> +static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>> +				 struct skl_pipe_wm_parameters *p,
>>   				 struct intel_plane_wm_parameters *p_params,
>>   				 uint16_t ddb_allocation,
>> -				 uint32_t mem_value,
>> +				 int level,
>>   				 uint16_t *out_blocks, /* out */
>>   				 uint8_t *out_lines /* out */)
>>   {
>> -	uint32_t method1, method2, plane_bytes_per_line, res_blocks, res_lines;
>> -	uint32_t result_bytes;
>> +	uint32_t latency = dev_priv->wm.skl_latency[level];
>> +	uint32_t method1, method2;
>> +	uint32_t plane_bytes_per_line, plane_blocks_per_line;
>> +	uint32_t res_blocks, res_lines;
>> +	uint32_t result_blocks;
>
> we now have res_blocks and result_blocks, a bit confusing. Maybe rename
> result_blocks to selected_result (which happens to be in blocks)

Will change.

>>
>> -	if (mem_value == 0 || !p->active || !p_params->enabled)
>> +	if (latency == 0 || !p->active || !p_params->enabled)
>>   		return false;
>>
>>   	method1 = skl_wm_method1(p->pixel_rate,
>>   				 p_params->bytes_per_pixel,
>> -				 mem_value);
>> +				 latency);
>>   	method2 = skl_wm_method2(p->pixel_rate,
>>   				 p->pipe_htotal,
>>   				 p_params->horiz_pixels,
>>   				 p_params->bytes_per_pixel,
>> -				 mem_value);
>> +				 latency);
>>
>>   	plane_bytes_per_line = p_params->horiz_pixels *
>>   					p_params->bytes_per_pixel;
>> +	plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
>>
>>   	/* For now xtile and linear */
>> -	if (((ddb_allocation * 512) / plane_bytes_per_line) >= 1)
>> -		result_bytes = min(method1, method2);
>> +	if ((ddb_allocation / plane_blocks_per_line) >= 1)
>> +		result_blocks = min(method1, method2);
>>   	else
>> -		result_bytes = method1;
>> +		result_blocks = method1;
>> +
>> +	res_blocks = result_blocks + 1;
>> +	res_lines = DIV_ROUND_UP(result_blocks, plane_blocks_per_line);
>>
>> -	res_blocks = DIV_ROUND_UP(result_bytes, 512) + 1;
>> -	res_lines = DIV_ROUND_UP(result_bytes, plane_bytes_per_line);
>> +	if (level >=1 && level <= 7)
>
> a space is missing before the 1 :)

Weird, ok. :)

>> +		res_blocks++;
>>
>>   	if (res_blocks > ddb_allocation || res_lines > 31)
>
> res_blocks >= ddb_allocation

Well spotted.

Regards,

Tvrtko
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx