On Wed, Feb 25, 2015 at 04:47:22PM +0000, Tvrtko Ursulin wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
>
> Recent BSpect updates have changed the watermark calculation to avoid
> display flickering in some cases.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> ---
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).
- 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@xxxxxxxxx>
> 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)
>
> - 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 :)
> + res_blocks++;
>
> if (res_blocks > ddb_allocation || res_lines > 31)
res_blocks >= ddb_allocation
> return false;
> @@ -2744,23 +2755,24 @@ static void skl_compute_wm_level(const struct drm_i915_private *dev_priv,
> int num_planes,
> struct skl_wm_level *result)
> {
> - uint16_t latency = dev_priv->wm.skl_latency[level];
> uint16_t ddb_blocks;
> int i;
>
> for (i = 0; i < num_planes; i++) {
> ddb_blocks = skl_ddb_entry_size(&ddb->plane[pipe][i]);
>
> - result->plane_en[i] = skl_compute_plane_wm(p, &p->plane[i],
> + result->plane_en[i] = skl_compute_plane_wm(dev_priv,
> + p, &p->plane[i],
> ddb_blocks,
> - latency,
> + level,
> &result->plane_res_b[i],
> &result->plane_res_l[i]);
> }
>
> ddb_blocks = skl_ddb_entry_size(&ddb->cursor[pipe]);
> - result->cursor_en = skl_compute_plane_wm(p, &p->cursor, ddb_blocks,
> - latency, &result->cursor_res_b,
> + result->cursor_en = skl_compute_plane_wm(dev_priv, p, &p->cursor,
> + ddb_blocks, level,
> + &result->cursor_res_b,
> &result->cursor_res_l);
> }
>
> --
> 2.3.0
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
