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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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@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).

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@xxxxxxxxx>

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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx





[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux