[PATCH] drm/i915: Adding dbuf support for skl nv12 format.

[PATCH] drm/i915: Adding dbuf support for skl nv12 format.

[Chandra Konduru]

Skylake nv12 format requires dbuf (aka. ddb) calculations
and programming for each of y and uv sub-planes. Made minor
changes to reuse current dbuf calculations and programming
for uv plane. i.e., with this change, existing computation
is used for either packed format or uv portion of nv12
depending on incoming format. Added new code for dbuf
computation and programming for y plane.

This patch is a pre-requisite for adding NV12 format support.
Actual nv12 support is coming in later patches.

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h  |    3 +-
 drivers/gpu/drm/i915/i915_reg.h  |   11 ++++++
 drivers/gpu/drm/i915/intel_drv.h |    8 ++++
 drivers/gpu/drm/i915/intel_pm.c  |   79 ++++++++++++++++++++++++++++++++------
 4 files changed, 88 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index b13c552..75eb3a5 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1450,7 +1450,8 @@ static inline bool skl_ddb_entry_equal(const struct skl_ddb_entry *e1,
 
 struct skl_ddb_allocation {
 	struct skl_ddb_entry pipe[I915_MAX_PIPES];
-	struct skl_ddb_entry plane[I915_MAX_PIPES][I915_MAX_PLANES];
+	struct skl_ddb_entry plane[I915_MAX_PIPES][I915_MAX_PLANES]; /* packed/uv */
+	struct skl_ddb_entry y_plane[I915_MAX_PIPES][I915_MAX_PLANES]; /* y-plane */
 	struct skl_ddb_entry cursor[I915_MAX_PIPES];
 };
 
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0865ec7..aa2a0de 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -4880,6 +4880,8 @@ enum skl_disp_power_wells {
 #define _PLANE_KEYMAX_2_A			0x702a0
 #define _PLANE_BUF_CFG_1_A			0x7027c
 #define _PLANE_BUF_CFG_2_A			0x7037c
+#define _PLANE_NV12_BUF_CFG_1_A		0x70278
+#define _PLANE_NV12_BUF_CFG_2_A		0x70378
 
 #define _PLANE_CTL_1_B				0x71180
 #define _PLANE_CTL_2_B				0x71280
@@ -4966,6 +4968,15 @@ enum skl_disp_power_wells {
 #define PLANE_BUF_CFG(pipe, plane)	\
 	_PLANE(plane, _PLANE_BUF_CFG_1(pipe), _PLANE_BUF_CFG_2(pipe))
 
+#define _PLANE_NV12_BUF_CFG_1_B		0x71278
+#define _PLANE_NV12_BUF_CFG_2_B		0x71378
+#define _PLANE_NV12_BUF_CFG_1(pipe)	\
+	_PIPE(pipe, _PLANE_NV12_BUF_CFG_1_A, _PLANE_NV12_BUF_CFG_1_B)
+#define _PLANE_NV12_BUF_CFG_2(pipe)	\
+	_PIPE(pipe, _PLANE_NV12_BUF_CFG_2_A, _PLANE_NV12_BUF_CFG_2_B)
+#define PLANE_NV12_BUF_CFG(pipe, plane)	\
+	_PLANE(plane, _PLANE_NV12_BUF_CFG_1(pipe), _PLANE_NV12_BUF_CFG_2(pipe))
+
 /* SKL new cursor registers */
 #define _CUR_BUF_CFG_A				0x7017c
 #define _CUR_BUF_CFG_B				0x7117c
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c9bc975..8079865 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -566,7 +566,15 @@ struct intel_crtc {
 struct intel_plane_wm_parameters {
 	uint32_t horiz_pixels;
 	uint32_t vert_pixels;
+	/*
+	 *   For packed pixel formats:
+	 *     bytes_per_pixel - holds bytes per pixel
+	 *   For planar pixel formats:
+	 *     bytes_per_pixel - holds bytes per pixel for uv-plane
+	 *     y_bytes_per_pixel - holds bytes per pixel for y-plane
+	 */
 	uint8_t bytes_per_pixel;
+	uint8_t y_bytes_per_pixel;
 	bool enabled;
 	bool scaled;
 	u64 tiling;
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 0f4391e..b8df120 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2616,8 +2616,18 @@ void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
 }
 
 static unsigned int
-skl_plane_relative_data_rate(const struct intel_plane_wm_parameters *p)
+skl_plane_relative_data_rate(const struct intel_plane_wm_parameters *p, int y)
 {
+
+	/* for planar format */
+	if (p->y_bytes_per_pixel) {
+		if (y)  /* y-plane data rate */
+			return p->horiz_pixels * p->vert_pixels * p->y_bytes_per_pixel;
+		else    /* uv-plane data rate */
+			return (p->horiz_pixels/2) * (p->vert_pixels/2) * p->bytes_per_pixel;
+	}
+
+	/* for packed formats */
 	return p->horiz_pixels * p->vert_pixels * p->bytes_per_pixel;
 }
 
@@ -2640,7 +2650,10 @@ skl_get_total_relative_data_rate(struct intel_crtc *intel_crtc,
 		if (!p->enabled)
 			continue;
 
-		total_data_rate += skl_plane_relative_data_rate(p);
+		total_data_rate += skl_plane_relative_data_rate(p, 0); /* packed/uv */
+		if (p->y_bytes_per_pixel) {
+			total_data_rate += skl_plane_relative_data_rate(p, 1); /* y-plane */
+		}
 	}
 
 	return total_data_rate;
@@ -2659,6 +2672,7 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
 	struct skl_ddb_entry *alloc = &ddb->pipe[pipe];
 	uint16_t alloc_size, start, cursor_blocks;
 	uint16_t minimum[I915_MAX_PLANES];
+	uint16_t y_minimum[I915_MAX_PLANES];
 	unsigned int total_data_rate;
 	int plane;
 
@@ -2687,6 +2701,8 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
 
 		minimum[plane] = 8;
 		alloc_size -= minimum[plane];
+		y_minimum[plane] = p->y_bytes_per_pixel ? 8 : 0;
+		alloc_size -= y_minimum[plane];
 	}
 
 	/*
@@ -2700,16 +2716,17 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
 	start = alloc->start;
 	for (plane = 0; plane < intel_num_planes(intel_crtc); plane++) {
 		const struct intel_plane_wm_parameters *p;
-		unsigned int data_rate;
-		uint16_t plane_blocks;
+		unsigned int data_rate, y_data_rate;
+		uint16_t plane_blocks, y_plane_blocks = 0;
 
 		p = &params->plane[plane];
 		if (!p->enabled)
 			continue;
 
-		data_rate = skl_plane_relative_data_rate(p);
+		data_rate = skl_plane_relative_data_rate(p, 0);
 
 		/*
+		 * allocation for (packed formats) or (uv-plane part of planar format):
 		 * promote the expression to 64 bits to avoid overflowing, the
 		 * result is < available as data_rate / total_data_rate < 1
 		 */
@@ -2721,6 +2738,22 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
 		ddb->plane[pipe][plane].end = start + plane_blocks;
 
 		start += plane_blocks;
+
+		/*
+		 * allocation for y_plane part of planar format:
+		 */
+		if (p->y_bytes_per_pixel) {
+			y_data_rate = skl_plane_relative_data_rate(p, 1);
+			y_plane_blocks = y_minimum[plane];
+			y_plane_blocks += div_u64((uint64_t)alloc_size * y_data_rate,
+						total_data_rate);
+
+			ddb->y_plane[pipe][plane].start = start;
+			ddb->y_plane[pipe][plane].end = start + y_plane_blocks;
+
+			start += y_plane_blocks;
+		}
+
 	}
 
 }
@@ -2833,13 +2866,18 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
 		p->pixel_rate = skl_pipe_pixel_rate(intel_crtc->config);
 
 		fb = crtc->primary->state->fb;
+		/* For planar: Bpp is for uv plane, y_Bpp is for y plane */
 		if (fb) {
 			p->plane[0].enabled = true;
-			p->plane[0].bytes_per_pixel = fb->bits_per_pixel / 8;
+			p->plane[0].bytes_per_pixel = fb->pixel_format == DRM_FORMAT_NV12 ?
+				drm_format_plane_cpp(fb->pixel_format, 1) : fb->bits_per_pixel / 8;
+			p->plane[0].y_bytes_per_pixel = fb->pixel_format == DRM_FORMAT_NV12 ?
+				drm_format_plane_cpp(fb->pixel_format, 0) : 0;
 			p->plane[0].tiling = fb->modifier[0];
 		} else {
 			p->plane[0].enabled = false;
 			p->plane[0].bytes_per_pixel = 0;
+			p->plane[0].y_bytes_per_pixel = 0;
 			p->plane[0].tiling = DRM_FORMAT_MOD_NONE;
 		}
 		p->plane[0].horiz_pixels = intel_crtc->config->pipe_src_w;
@@ -2847,6 +2885,7 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
 		p->plane[0].rotation = crtc->primary->state->rotation;
 
 		fb = crtc->cursor->state->fb;
+		p->cursor.y_bytes_per_pixel = 0;
 		if (fb) {
 			p->cursor.enabled = true;
 			p->cursor.bytes_per_pixel = fb->bits_per_pixel / 8;
@@ -2882,22 +2921,25 @@ static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
 	uint32_t plane_bytes_per_line, plane_blocks_per_line;
 	uint32_t res_blocks, res_lines;
 	uint32_t selected_result;
+	uint8_t bytes_per_pixel;
 
 	if (latency == 0 || !p->active || !p_params->enabled)
 		return false;
 
+	bytes_per_pixel = p_params->y_bytes_per_pixel ?
+		p_params->y_bytes_per_pixel :
+		p_params->bytes_per_pixel;
 	method1 = skl_wm_method1(p->pixel_rate,
-				 p_params->bytes_per_pixel,
+				 bytes_per_pixel,
 				 latency);
 	method2 = skl_wm_method2(p->pixel_rate,
 				 p->pipe_htotal,
 				 p_params->horiz_pixels,
-				 p_params->bytes_per_pixel,
+				 bytes_per_pixel,
 				 p_params->tiling,
 				 latency);
 
-	plane_bytes_per_line = p_params->horiz_pixels *
-					p_params->bytes_per_pixel;
+	plane_bytes_per_line = p_params->horiz_pixels * bytes_per_pixel;
 	plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
 
 	if (p_params->tiling == I915_FORMAT_MOD_Y_TILED ||
@@ -3114,10 +3156,14 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
 				   new->plane_trans[pipe][i]);
 		I915_WRITE(CUR_WM_TRANS(pipe), new->cursor_trans[pipe]);
 
-		for (i = 0; i < intel_num_planes(crtc); i++)
+		for (i = 0; i < intel_num_planes(crtc); i++) {
 			skl_ddb_entry_write(dev_priv,
 					    PLANE_BUF_CFG(pipe, i),
 					    &new->ddb.plane[pipe][i]);
+			skl_ddb_entry_write(dev_priv,
+					    PLANE_NV12_BUF_CFG(pipe, i),
+					    &new->ddb.y_plane[pipe][i]);
+		}
 
 		skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
 				    &new->ddb.cursor[pipe]);
@@ -3275,6 +3321,7 @@ static bool skl_update_pipe_wm(struct drm_crtc *crtc,
 		return false;
 
 	intel_crtc->wm.skl_active = *pipe_wm;
+
 	return true;
 }
 
@@ -3368,8 +3415,16 @@ skl_update_sprite_wm(struct drm_plane *plane, struct drm_crtc *crtc,
 	intel_plane->wm.scaled = scaled;
 	intel_plane->wm.horiz_pixels = sprite_width;
 	intel_plane->wm.vert_pixels = sprite_height;
-	intel_plane->wm.bytes_per_pixel = pixel_size;
 	intel_plane->wm.tiling = DRM_FORMAT_MOD_NONE;
+
+	/* For planar: Bpp is for UV plane, y_Bpp is for Y plane */
+	intel_plane->wm.bytes_per_pixel =
+		(fb && fb->pixel_format == DRM_FORMAT_NV12) ?
+		drm_format_plane_cpp(plane->state->fb->pixel_format, 1) : pixel_size;
+	intel_plane->wm.y_bytes_per_pixel =
+		(fb && fb->pixel_format == DRM_FORMAT_NV12) ?
+		drm_format_plane_cpp(plane->state->fb->pixel_format, 0) : 0;
+
 	/*
 	 * Framebuffer can be NULL on plane disable, but it does not
 	 * matter for watermarks if we assume no tiling in that case.
-- 
1.7.9.5

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

Re: [PATCH] drm/i915: Adding dbuf support for skl nv12 format.

[shuang.he]

Tested-By: Intel Graphics QA PRTS (Patch Regression Test System Contact: shuang.he@intel.com)
Task id: 6273
-------------------------------------Summary-------------------------------------
Platform          Delta          drm-intel-nightly          Series Applied
PNV                                  276/276              276/276
ILK                                  302/302              302/302
SNB                                  318/318              318/318
IVB                                  341/341              341/341
BYT                                  287/287              287/287
BDW                                  318/318              318/318
-------------------------------------Detailed-------------------------------------
Platform  Test                                drm-intel-nightly          Series Applied
Note: You need to pay more attention to line start with '*'
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Adding dbuf support for skl nv12 format.

[Daniel Vetter]

On Mon, Apr 27, 2015 at 03:47:37PM -0700, Chandra Konduru wrote:
> Skylake nv12 format requires dbuf (aka. ddb) calculations
> and programming for each of y and uv sub-planes. Made minor
> changes to reuse current dbuf calculations and programming
> for uv plane. i.e., with this change, existing computation
> is used for either packed format or uv portion of nv12
> depending on incoming format. Added new code for dbuf
> computation and programming for y plane.
> 
> This patch is a pre-requisite for adding NV12 format support.
> Actual nv12 support is coming in later patches.
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>

Just aside (and not a blocker for this patch really). Do we have nv12
subtest in all the relevant plane tests (rotation, scaling, ...) to
exercise this code? Not sure whether we can redo the yuv->rgb conversion
correctly in software to match hw, might need to pick special colors that
convert accurately.

Anyway given how often we've broken the sprite code I really think we need
full functional tests (i.e. including crc checks).
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Adding dbuf support for skl nv12 format.

[Konduru, Chandra]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Monday, May 04, 2015 8:01 AM
> To: Konduru, Chandra
> Cc: intel-gfx@lists.freedesktop.org; Vetter, Daniel; Syrjala, Ville
> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Adding dbuf support for skl nv12
> format.
> 
> On Mon, Apr 27, 2015 at 03:47:37PM -0700, Chandra Konduru wrote:
> > Skylake nv12 format requires dbuf (aka. ddb) calculations and
> > programming for each of y and uv sub-planes. Made minor changes to
> > reuse current dbuf calculations and programming for uv plane. i.e.,
> > with this change, existing computation is used for either packed
> > format or uv portion of nv12 depending on incoming format. Added new
> > code for dbuf computation and programming for y plane.
> >
> > This patch is a pre-requisite for adding NV12 format support.
> > Actual nv12 support is coming in later patches.
> >
> > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> 
> Just aside (and not a blocker for this patch really). Do we have nv12 subtest in all
> the relevant plane tests (rotation, scaling, ...) to exercise this code? Not sure
> whether we can redo the yuv->rgb conversion correctly in software to match
> hw, might need to pick special colors that convert accurately.
> 
> Anyway given how often we've broken the sprite code I really think we need full
> functional tests (i.e. including crc checks).

I have written an i-g-t test kms_nv12 to test/exercise this patch and 
subsequent NV12 patch series which was submitted last week after
this patch. It tests nv12 along with rotation, scaling and does
some crc checks. 
But because chrome upsampling is done using scaler which uses multiple taps,
crcs aren't going to match with original reference flip with RGB format. I tried with 
simple solid color. Can you please take a look that?

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Adding dbuf support for skl nv12 format.

[Daniel Vetter]

On Mon, May 04, 2015 at 11:40:49PM +0000, Konduru, Chandra wrote:
> 
> 
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> > Sent: Monday, May 04, 2015 8:01 AM
> > To: Konduru, Chandra
> > Cc: intel-gfx@lists.freedesktop.org; Vetter, Daniel; Syrjala, Ville
> > Subject: Re: [Intel-gfx] [PATCH] drm/i915: Adding dbuf support for skl nv12
> > format.
> > 
> > On Mon, Apr 27, 2015 at 03:47:37PM -0700, Chandra Konduru wrote:
> > > Skylake nv12 format requires dbuf (aka. ddb) calculations and
> > > programming for each of y and uv sub-planes. Made minor changes to
> > > reuse current dbuf calculations and programming for uv plane. i.e.,
> > > with this change, existing computation is used for either packed
> > > format or uv portion of nv12 depending on incoming format. Added new
> > > code for dbuf computation and programming for y plane.
> > >
> > > This patch is a pre-requisite for adding NV12 format support.
> > > Actual nv12 support is coming in later patches.
> > >
> > > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > 
> > Just aside (and not a blocker for this patch really). Do we have nv12 subtest in all
> > the relevant plane tests (rotation, scaling, ...) to exercise this code? Not sure
> > whether we can redo the yuv->rgb conversion correctly in software to match
> > hw, might need to pick special colors that convert accurately.
> > 
> > Anyway given how often we've broken the sprite code I really think we need full
> > functional tests (i.e. including crc checks).
> 
> I have written an i-g-t test kms_nv12 to test/exercise this patch and 
> subsequent NV12 patch series which was submitted last week after
> this patch. It tests nv12 along with rotation, scaling and does
> some crc checks. 
> But because chrome upsampling is done using scaler which uses multiple taps,
> crcs aren't going to match with original reference flip with RGB format. I tried with 
> simple solid color. Can you please take a look that?

Oh right the upsampling will result in a smudge when we change visible
colors, so the 4 color pattern used for checking rotation doesn't work.
Just an idea (without yet looking at your code):
- Create a tall&narrow sprite view, and a big buffer which would fit that
  sprite view in any orentiation.
- Only draw a slice exactly matching the sprite view (horizontal/vertical)
  in one solid color, leave the remaining buffer black.

That way we can at least do some basic checks to make sure the
rotation/alignment is correct. We won't be able to detect when the buffer
is mirrored wrongly, but at least we'll have some coverage.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Adding dbuf support for skl nv12 format.

[Damien Lespiau]

Hi Chandra,

On Mon, Apr 27, 2015 at 03:47:37PM -0700, Chandra Konduru wrote:
> Skylake nv12 format requires dbuf (aka. ddb) calculations
> and programming for each of y and uv sub-planes. Made minor
> changes to reuse current dbuf calculations and programming
> for uv plane. i.e., with this change, existing computation
> is used for either packed format or uv portion of nv12
> depending on incoming format. Added new code for dbuf
> computation and programming for y plane.
> 
> This patch is a pre-requisite for adding NV12 format support.
> Actual nv12 support is coming in later patches.
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>

I found a few things to talk about. Nothing major, except maybe that the
cof for Y tiled buffers minimum allocation that is not there and would
need to take the UV plane size into account.

A more general remark though, is that I believe we should make the code
more generic and talk about fb planes 0 and 1 instead of having the
  if (y_bytes_per_pixel)
everywhere. It'd be a good fit for the WM code as we're always
interested in fb plane 0 (either RGB or Y plane). Concretely it means
having per-fb plane variables in the structures, ie bytes_per_pixel[2],
indexed by the fb plane number.

Otherwise the rest looks good to me. I'd love to see the more generic
version (especially the remark about deriving the sub-sampling factors
from the format), but this can be done in tree if you want.

> ---
>  drivers/gpu/drm/i915/i915_drv./h  |    3 +-
>  drivers/gpu/drm/i915/i915_reg.h  |   11 ++++++
>  drivers/gpu/drm/i915/intel_drv.h |    8 ++++
>  drivers/gpu/drm/i915/intel_pm.c  |   79 ++++++++++++++++++++++++++++++++------
>  4 files changed, 88 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b13c552..75eb3a5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1450,7 +1450,8 @@ static inline bool skl_ddb_entry_equal(const struct skl_ddb_entry *e1,
>  
>  struct skl_ddb_allocation {
>  	struct skl_ddb_entry pipe[I915_MAX_PIPES];
> -	struct skl_ddb_entry plane[I915_MAX_PIPES][I915_MAX_PLANES];
> +	struct skl_ddb_entry plane[I915_MAX_PIPES][I915_MAX_PLANES]; /* packed/uv */
> +	struct skl_ddb_entry y_plane[I915_MAX_PIPES][I915_MAX_PLANES]; /* y-plane */
>  	struct skl_ddb_entry cursor[I915_MAX_PIPES];
>  };
>  
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0865ec7..aa2a0de 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4880,6 +4880,8 @@ enum skl_disp_power_wells {
>  #define _PLANE_KEYMAX_2_A			0x702a0
>  #define _PLANE_BUF_CFG_1_A			0x7027c
>  #define _PLANE_BUF_CFG_2_A			0x7037c
> +#define _PLANE_NV12_BUF_CFG_1_A		0x70278
> +#define _PLANE_NV12_BUF_CFG_2_A		0x70378
>  
>  #define _PLANE_CTL_1_B				0x71180
>  #define _PLANE_CTL_2_B				0x71280
> @@ -4966,6 +4968,15 @@ enum skl_disp_power_wells {
>  #define PLANE_BUF_CFG(pipe, plane)	\
>  	_PLANE(plane, _PLANE_BUF_CFG_1(pipe), _PLANE_BUF_CFG_2(pipe))
>  
> +#define _PLANE_NV12_BUF_CFG_1_B		0x71278
> +#define _PLANE_NV12_BUF_CFG_2_B		0x71378
> +#define _PLANE_NV12_BUF_CFG_1(pipe)	\
> +	_PIPE(pipe, _PLANE_NV12_BUF_CFG_1_A, _PLANE_NV12_BUF_CFG_1_B)
> +#define _PLANE_NV12_BUF_CFG_2(pipe)	\
> +	_PIPE(pipe, _PLANE_NV12_BUF_CFG_2_A, _PLANE_NV12_BUF_CFG_2_B)
> +#define PLANE_NV12_BUF_CFG(pipe, plane)	\
> +	_PLANE(plane, _PLANE_NV12_BUF_CFG_1(pipe), _PLANE_NV12_BUF_CFG_2(pipe))
> +
>  /* SKL new cursor registers */
>  #define _CUR_BUF_CFG_A				0x7017c
>  #define _CUR_BUF_CFG_B				0x7117c
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index c9bc975..8079865 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -566,7 +566,15 @@ struct intel_crtc {
>  struct intel_plane_wm_parameters {
>  	uint32_t horiz_pixels;
>  	uint32_t vert_pixels;
> +	/*
> +	 *   For packed pixel formats:
> +	 *     bytes_per_pixel - holds bytes per pixel
> +	 *   For planar pixel formats:
> +	 *     bytes_per_pixel - holds bytes per pixel for uv-plane
> +	 *     y_bytes_per_pixel - holds bytes per pixel for y-plane
> +	 */
>  	uint8_t bytes_per_pixel;
> +	uint8_t y_bytes_per_pixel;
>  	bool enabled;
>  	bool scaled;
>  	u64 tiling;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 0f4391e..b8df120 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2616,8 +2616,18 @@ void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
>  }
>  
>  static unsigned int
> -skl_plane_relative_data_rate(const struct intel_plane_wm_parameters *p)
> +skl_plane_relative_data_rate(const struct intel_plane_wm_parameters *p, int y)
>  {
> +
> +	/* for planar format */
> +	if (p->y_bytes_per_pixel) {
> +		if (y)  /* y-plane data rate */
> +			return p->horiz_pixels * p->vert_pixels * p->y_bytes_per_pixel;
> +		else    /* uv-plane data rate */
> +			return (p->horiz_pixels/2) * (p->vert_pixels/2) * p->bytes_per_pixel;

The knowledge that the UV plane is sub-sampled by 2 in both directions
is hidden there, we may want to add a new drm_ function to give the
sub-sample factors depending on the format and the plane number. This
way, the code would more generic if the hw ever adds support for more
planar formats. If we still want to cache the parameters in
intel_plane_wm_parameters, I think we should have per-plane (ie the
surfaces in the fb) bytes_per_pixel, width, height, ...

I'd also try use something a bit more general rather than the 'y'
parameter, maybe the plane number (0->y, 1->UV in the NV12 case 0->RGB
otherwise).

> +	}
> +
> +	/* for packed formats */
>  	return p->horiz_pixels * p->vert_pixels * p->bytes_per_pixel;
>  }

Oh, and something else I noticed, we apparently need to compute a
scaling factor in there. As you've looked at the scalers for SKL, you
may be interested in looking into it.

>  
> @@ -2640,7 +2650,10 @@ skl_get_total_relative_data_rate(struct intel_crtc *intel_crtc,
>  		if (!p->enabled)
>  			continue;
>  
> -		total_data_rate += skl_plane_relative_data_rate(p);
> +		total_data_rate += skl_plane_relative_data_rate(p, 0); /* packed/uv */
> +		if (p->y_bytes_per_pixel) {
> +			total_data_rate += skl_plane_relative_data_rate(p, 1); /* y-plane */
> +		}
>  	}
>  
>  	return total_data_rate;
> @@ -2659,6 +2672,7 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
>  	struct skl_ddb_entry *alloc = &ddb->pipe[pipe];
>  	uint16_t alloc_size, start, cursor_blocks;
>  	uint16_t minimum[I915_MAX_PLANES];
> +	uint16_t y_minimum[I915_MAX_PLANES];
>  	unsigned int total_data_rate;
>  	int plane;
>  
> @@ -2687,6 +2701,8 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
>  
>  		minimum[plane] = 8;
>  		alloc_size -= minimum[plane];
> +		y_minimum[plane] = p->y_bytes_per_pixel ? 8 : 0;
> +		alloc_size -= y_minimum[plane];
>  	}

Hum, this made me look at the DDB allocation page and it seems we're
missing the code to compute the minimum allocate for Y tiled FBs. It's
somewhat important here as that code is using the width of the plane,
width that would be halved for a Y tiled UV plane.

>  
>  	/*
> @@ -2700,16 +2716,17 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
>  	start = alloc->start;
>  	for (plane = 0; plane < intel_num_planes(intel_crtc); plane++) {
>  		const struct intel_plane_wm_parameters *p;
> -		unsigned int data_rate;
> -		uint16_t plane_blocks;
> +		unsigned int data_rate, y_data_rate;
> +		uint16_t plane_blocks, y_plane_blocks = 0;
>  
>  		p = &params->plane[plane];
>  		if (!p->enabled)
>  			continue;
>  
> -		data_rate = skl_plane_relative_data_rate(p);
> +		data_rate = skl_plane_relative_data_rate(p, 0);
>  
>  		/*
> +		 * allocation for (packed formats) or (uv-plane part of planar format):
>  		 * promote the expression to 64 bits to avoid overflowing, the
>  		 * result is < available as data_rate / total_data_rate < 1
>  		 */
> @@ -2721,6 +2738,22 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
>  		ddb->plane[pipe][plane].end = start + plane_blocks;
>  
>  		start += plane_blocks;
> +
> +		/*
> +		 * allocation for y_plane part of planar format:
> +		 */
> +		if (p->y_bytes_per_pixel) {
> +			y_data_rate = skl_plane_relative_data_rate(p, 1);
> +			y_plane_blocks = y_minimum[plane];
> +			y_plane_blocks += div_u64((uint64_t)alloc_size * y_data_rate,
> +						total_data_rate);
> +
> +			ddb->y_plane[pipe][plane].start = start;
> +			ddb->y_plane[pipe][plane].end = start + y_plane_blocks;
> +
> +			start += y_plane_blocks;
> +		}
> +
>  	}
>  
>  }
> @@ -2833,13 +2866,18 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
>  		p->pixel_rate = skl_pipe_pixel_rate(intel_crtc->config);
>  
>  		fb = crtc->primary->state->fb;
> +		/* For planar: Bpp is for uv plane, y_Bpp is for y plane */
>  		if (fb) {
>  			p->plane[0].enabled = true;
> -			p->plane[0].bytes_per_pixel = fb->bits_per_pixel / 8;
> +			p->plane[0].bytes_per_pixel = fb->pixel_format == DRM_FORMAT_NV12 ?
> +				drm_format_plane_cpp(fb->pixel_format, 1) : fb->bits_per_pixel / 8;
> +			p->plane[0].y_bytes_per_pixel = fb->pixel_format == DRM_FORMAT_NV12 ?
> +				drm_format_plane_cpp(fb->pixel_format, 0) : 0;

drm_format_plane_cpp() seems general enough to always work?

>  			p->plane[0].tiling = fb->modifier[0];
>  		} else {
>  			p->plane[0].enabled = false;
>  			p->plane[0].bytes_per_pixel = 0;
> +			p->plane[0].y_bytes_per_pixel = 0;
>  			p->plane[0].tiling = DRM_FORMAT_MOD_NONE;
>  		}
>  		p->plane[0].horiz_pixels = intel_crtc->config->pipe_src_w;
> @@ -2847,6 +2885,7 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
>  		p->plane[0].rotation = crtc->primary->state->rotation;
>  
>  		fb = crtc->cursor->state->fb;
> +		p->cursor.y_bytes_per_pixel = 0;
>  		if (fb) {
>  			p->cursor.enabled = true;
>  			p->cursor.bytes_per_pixel = fb->bits_per_pixel / 8;
> @@ -2882,22 +2921,25 @@ static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>  	uint32_t plane_bytes_per_line, plane_blocks_per_line;
>  	uint32_t res_blocks, res_lines;
>  	uint32_t selected_result;
> +	uint8_t bytes_per_pixel;
>  
>  	if (latency == 0 || !p->active || !p_params->enabled)
>  		return false;
>  
> +	bytes_per_pixel = p_params->y_bytes_per_pixel ?
> +		p_params->y_bytes_per_pixel :
> +		p_params->bytes_per_pixel;
>  	method1 = skl_wm_method1(p->pixel_rate,
> -				 p_params->bytes_per_pixel,
> +				 bytes_per_pixel,
>  				 latency);
>  	method2 = skl_wm_method2(p->pixel_rate,
>  				 p->pipe_htotal,
>  				 p_params->horiz_pixels,
> -				 p_params->bytes_per_pixel,
> +				 bytes_per_pixel,
>  				 p_params->tiling,
>  				 latency);
>  
> -	plane_bytes_per_line = p_params->horiz_pixels *
> -					p_params->bytes_per_pixel;
> +	plane_bytes_per_line = p_params->horiz_pixels * bytes_per_pixel;
>  	plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
>  
>  	if (p_params->tiling == I915_FORMAT_MOD_Y_TILED ||
> @@ -3114,10 +3156,14 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
>  				   new->plane_trans[pipe][i]);
>  		I915_WRITE(CUR_WM_TRANS(pipe), new->cursor_trans[pipe]);
>  
> -		for (i = 0; i < intel_num_planes(crtc); i++)
> +		for (i = 0; i < intel_num_planes(crtc); i++) {
>  			skl_ddb_entry_write(dev_priv,
>  					    PLANE_BUF_CFG(pipe, i),
>  					    &new->ddb.plane[pipe][i]);
> +			skl_ddb_entry_write(dev_priv,
> +					    PLANE_NV12_BUF_CFG(pipe, i),
> +					    &new->ddb.y_plane[pipe][i]);
> +		}
>  
>  		skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
>  				    &new->ddb.cursor[pipe]);
> @@ -3275,6 +3321,7 @@ static bool skl_update_pipe_wm(struct drm_crtc *crtc,
>  		return false;
>  
>  	intel_crtc->wm.skl_active = *pipe_wm;
> +
>  	return true;
>  }
>  
> @@ -3368,8 +3415,16 @@ skl_update_sprite_wm(struct drm_plane *plane, struct drm_crtc *crtc,
>  	intel_plane->wm.scaled = scaled;
>  	intel_plane->wm.horiz_pixels = sprite_width;
>  	intel_plane->wm.vert_pixels = sprite_height;
> -	intel_plane->wm.bytes_per_pixel = pixel_size;
>  	intel_plane->wm.tiling = DRM_FORMAT_MOD_NONE;
> +
> +	/* For planar: Bpp is for UV plane, y_Bpp is for Y plane */
> +	intel_plane->wm.bytes_per_pixel =
> +		(fb && fb->pixel_format == DRM_FORMAT_NV12) ?
> +		drm_format_plane_cpp(plane->state->fb->pixel_format, 1) : pixel_size;
> +	intel_plane->wm.y_bytes_per_pixel =
> +		(fb && fb->pixel_format == DRM_FORMAT_NV12) ?
> +		drm_format_plane_cpp(plane->state->fb->pixel_format, 0) : 0;
> +

drm_format_plane_cpp() seems general enough to always work?

>  	/*
>  	 * Framebuffer can be NULL on plane disable, but it does not
>  	 * matter for watermarks if we assume no tiling in that case.
> -- 
> 1.7.9.5
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Adding dbuf support for skl nv12 format.

[Damien Lespiau]

On Mon, Apr 27, 2015 at 03:47:37PM -0700, Chandra Konduru wrote:
> Skylake nv12 format requires dbuf (aka. ddb) calculations
> and programming for each of y and uv sub-planes. Made minor
> changes to reuse current dbuf calculations and programming
> for uv plane. i.e., with this change, existing computation
> is used for either packed format or uv portion of nv12
> depending on incoming format. Added new code for dbuf
> computation and programming for y plane.
> 
> This patch is a pre-requisite for adding NV12 format support.
> Actual nv12 support is coming in later patches.
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>

Talked with Chandra about the previous comments. The Y tiling problem I
was mentioning isn't quite part of NV12 and can be fixed on top of this.
I'll probaby try my suggestions and create a series on top of this
patch.

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

> ---
>  drivers/gpu/drm/i915/i915_drv.h  |    3 +-
>  drivers/gpu/drm/i915/i915_reg.h  |   11 ++++++
>  drivers/gpu/drm/i915/intel_drv.h |    8 ++++
>  drivers/gpu/drm/i915/intel_pm.c  |   79 ++++++++++++++++++++++++++++++++------
>  4 files changed, 88 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index b13c552..75eb3a5 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1450,7 +1450,8 @@ static inline bool skl_ddb_entry_equal(const struct skl_ddb_entry *e1,
>  
>  struct skl_ddb_allocation {
>  	struct skl_ddb_entry pipe[I915_MAX_PIPES];
> -	struct skl_ddb_entry plane[I915_MAX_PIPES][I915_MAX_PLANES];
> +	struct skl_ddb_entry plane[I915_MAX_PIPES][I915_MAX_PLANES]; /* packed/uv */
> +	struct skl_ddb_entry y_plane[I915_MAX_PIPES][I915_MAX_PLANES]; /* y-plane */
>  	struct skl_ddb_entry cursor[I915_MAX_PIPES];
>  };
>  
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0865ec7..aa2a0de 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -4880,6 +4880,8 @@ enum skl_disp_power_wells {
>  #define _PLANE_KEYMAX_2_A			0x702a0
>  #define _PLANE_BUF_CFG_1_A			0x7027c
>  #define _PLANE_BUF_CFG_2_A			0x7037c
> +#define _PLANE_NV12_BUF_CFG_1_A		0x70278
> +#define _PLANE_NV12_BUF_CFG_2_A		0x70378
>  
>  #define _PLANE_CTL_1_B				0x71180
>  #define _PLANE_CTL_2_B				0x71280
> @@ -4966,6 +4968,15 @@ enum skl_disp_power_wells {
>  #define PLANE_BUF_CFG(pipe, plane)	\
>  	_PLANE(plane, _PLANE_BUF_CFG_1(pipe), _PLANE_BUF_CFG_2(pipe))
>  
> +#define _PLANE_NV12_BUF_CFG_1_B		0x71278
> +#define _PLANE_NV12_BUF_CFG_2_B		0x71378
> +#define _PLANE_NV12_BUF_CFG_1(pipe)	\
> +	_PIPE(pipe, _PLANE_NV12_BUF_CFG_1_A, _PLANE_NV12_BUF_CFG_1_B)
> +#define _PLANE_NV12_BUF_CFG_2(pipe)	\
> +	_PIPE(pipe, _PLANE_NV12_BUF_CFG_2_A, _PLANE_NV12_BUF_CFG_2_B)
> +#define PLANE_NV12_BUF_CFG(pipe, plane)	\
> +	_PLANE(plane, _PLANE_NV12_BUF_CFG_1(pipe), _PLANE_NV12_BUF_CFG_2(pipe))
> +
>  /* SKL new cursor registers */
>  #define _CUR_BUF_CFG_A				0x7017c
>  #define _CUR_BUF_CFG_B				0x7117c
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index c9bc975..8079865 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -566,7 +566,15 @@ struct intel_crtc {
>  struct intel_plane_wm_parameters {
>  	uint32_t horiz_pixels;
>  	uint32_t vert_pixels;
> +	/*
> +	 *   For packed pixel formats:
> +	 *     bytes_per_pixel - holds bytes per pixel
> +	 *   For planar pixel formats:
> +	 *     bytes_per_pixel - holds bytes per pixel for uv-plane
> +	 *     y_bytes_per_pixel - holds bytes per pixel for y-plane
> +	 */
>  	uint8_t bytes_per_pixel;
> +	uint8_t y_bytes_per_pixel;
>  	bool enabled;
>  	bool scaled;
>  	u64 tiling;
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 0f4391e..b8df120 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2616,8 +2616,18 @@ void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv,
>  }
>  
>  static unsigned int
> -skl_plane_relative_data_rate(const struct intel_plane_wm_parameters *p)
> +skl_plane_relative_data_rate(const struct intel_plane_wm_parameters *p, int y)
>  {
> +
> +	/* for planar format */
> +	if (p->y_bytes_per_pixel) {
> +		if (y)  /* y-plane data rate */
> +			return p->horiz_pixels * p->vert_pixels * p->y_bytes_per_pixel;
> +		else    /* uv-plane data rate */
> +			return (p->horiz_pixels/2) * (p->vert_pixels/2) * p->bytes_per_pixel;
> +	}
> +
> +	/* for packed formats */
>  	return p->horiz_pixels * p->vert_pixels * p->bytes_per_pixel;
>  }
>  
> @@ -2640,7 +2650,10 @@ skl_get_total_relative_data_rate(struct intel_crtc *intel_crtc,
>  		if (!p->enabled)
>  			continue;
>  
> -		total_data_rate += skl_plane_relative_data_rate(p);
> +		total_data_rate += skl_plane_relative_data_rate(p, 0); /* packed/uv */
> +		if (p->y_bytes_per_pixel) {
> +			total_data_rate += skl_plane_relative_data_rate(p, 1); /* y-plane */
> +		}
>  	}
>  
>  	return total_data_rate;
> @@ -2659,6 +2672,7 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
>  	struct skl_ddb_entry *alloc = &ddb->pipe[pipe];
>  	uint16_t alloc_size, start, cursor_blocks;
>  	uint16_t minimum[I915_MAX_PLANES];
> +	uint16_t y_minimum[I915_MAX_PLANES];
>  	unsigned int total_data_rate;
>  	int plane;
>  
> @@ -2687,6 +2701,8 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
>  
>  		minimum[plane] = 8;
>  		alloc_size -= minimum[plane];
> +		y_minimum[plane] = p->y_bytes_per_pixel ? 8 : 0;
> +		alloc_size -= y_minimum[plane];
>  	}
>  
>  	/*
> @@ -2700,16 +2716,17 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
>  	start = alloc->start;
>  	for (plane = 0; plane < intel_num_planes(intel_crtc); plane++) {
>  		const struct intel_plane_wm_parameters *p;
> -		unsigned int data_rate;
> -		uint16_t plane_blocks;
> +		unsigned int data_rate, y_data_rate;
> +		uint16_t plane_blocks, y_plane_blocks = 0;
>  
>  		p = &params->plane[plane];
>  		if (!p->enabled)
>  			continue;
>  
> -		data_rate = skl_plane_relative_data_rate(p);
> +		data_rate = skl_plane_relative_data_rate(p, 0);
>  
>  		/*
> +		 * allocation for (packed formats) or (uv-plane part of planar format):
>  		 * promote the expression to 64 bits to avoid overflowing, the
>  		 * result is < available as data_rate / total_data_rate < 1
>  		 */
> @@ -2721,6 +2738,22 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
>  		ddb->plane[pipe][plane].end = start + plane_blocks;
>  
>  		start += plane_blocks;
> +
> +		/*
> +		 * allocation for y_plane part of planar format:
> +		 */
> +		if (p->y_bytes_per_pixel) {
> +			y_data_rate = skl_plane_relative_data_rate(p, 1);
> +			y_plane_blocks = y_minimum[plane];
> +			y_plane_blocks += div_u64((uint64_t)alloc_size * y_data_rate,
> +						total_data_rate);
> +
> +			ddb->y_plane[pipe][plane].start = start;
> +			ddb->y_plane[pipe][plane].end = start + y_plane_blocks;
> +
> +			start += y_plane_blocks;
> +		}
> +
>  	}
>  
>  }
> @@ -2833,13 +2866,18 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
>  		p->pixel_rate = skl_pipe_pixel_rate(intel_crtc->config);
>  
>  		fb = crtc->primary->state->fb;
> +		/* For planar: Bpp is for uv plane, y_Bpp is for y plane */
>  		if (fb) {
>  			p->plane[0].enabled = true;
> -			p->plane[0].bytes_per_pixel = fb->bits_per_pixel / 8;
> +			p->plane[0].bytes_per_pixel = fb->pixel_format == DRM_FORMAT_NV12 ?
> +				drm_format_plane_cpp(fb->pixel_format, 1) : fb->bits_per_pixel / 8;
> +			p->plane[0].y_bytes_per_pixel = fb->pixel_format == DRM_FORMAT_NV12 ?
> +				drm_format_plane_cpp(fb->pixel_format, 0) : 0;
>  			p->plane[0].tiling = fb->modifier[0];
>  		} else {
>  			p->plane[0].enabled = false;
>  			p->plane[0].bytes_per_pixel = 0;
> +			p->plane[0].y_bytes_per_pixel = 0;
>  			p->plane[0].tiling = DRM_FORMAT_MOD_NONE;
>  		}
>  		p->plane[0].horiz_pixels = intel_crtc->config->pipe_src_w;
> @@ -2847,6 +2885,7 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
>  		p->plane[0].rotation = crtc->primary->state->rotation;
>  
>  		fb = crtc->cursor->state->fb;
> +		p->cursor.y_bytes_per_pixel = 0;
>  		if (fb) {
>  			p->cursor.enabled = true;
>  			p->cursor.bytes_per_pixel = fb->bits_per_pixel / 8;
> @@ -2882,22 +2921,25 @@ static bool skl_compute_plane_wm(const struct drm_i915_private *dev_priv,
>  	uint32_t plane_bytes_per_line, plane_blocks_per_line;
>  	uint32_t res_blocks, res_lines;
>  	uint32_t selected_result;
> +	uint8_t bytes_per_pixel;
>  
>  	if (latency == 0 || !p->active || !p_params->enabled)
>  		return false;
>  
> +	bytes_per_pixel = p_params->y_bytes_per_pixel ?
> +		p_params->y_bytes_per_pixel :
> +		p_params->bytes_per_pixel;
>  	method1 = skl_wm_method1(p->pixel_rate,
> -				 p_params->bytes_per_pixel,
> +				 bytes_per_pixel,
>  				 latency);
>  	method2 = skl_wm_method2(p->pixel_rate,
>  				 p->pipe_htotal,
>  				 p_params->horiz_pixels,
> -				 p_params->bytes_per_pixel,
> +				 bytes_per_pixel,
>  				 p_params->tiling,
>  				 latency);
>  
> -	plane_bytes_per_line = p_params->horiz_pixels *
> -					p_params->bytes_per_pixel;
> +	plane_bytes_per_line = p_params->horiz_pixels * bytes_per_pixel;
>  	plane_blocks_per_line = DIV_ROUND_UP(plane_bytes_per_line, 512);
>  
>  	if (p_params->tiling == I915_FORMAT_MOD_Y_TILED ||
> @@ -3114,10 +3156,14 @@ static void skl_write_wm_values(struct drm_i915_private *dev_priv,
>  				   new->plane_trans[pipe][i]);
>  		I915_WRITE(CUR_WM_TRANS(pipe), new->cursor_trans[pipe]);
>  
> -		for (i = 0; i < intel_num_planes(crtc); i++)
> +		for (i = 0; i < intel_num_planes(crtc); i++) {
>  			skl_ddb_entry_write(dev_priv,
>  					    PLANE_BUF_CFG(pipe, i),
>  					    &new->ddb.plane[pipe][i]);
> +			skl_ddb_entry_write(dev_priv,
> +					    PLANE_NV12_BUF_CFG(pipe, i),
> +					    &new->ddb.y_plane[pipe][i]);
> +		}
>  
>  		skl_ddb_entry_write(dev_priv, CUR_BUF_CFG(pipe),
>  				    &new->ddb.cursor[pipe]);
> @@ -3275,6 +3321,7 @@ static bool skl_update_pipe_wm(struct drm_crtc *crtc,
>  		return false;
>  
>  	intel_crtc->wm.skl_active = *pipe_wm;
> +
>  	return true;
>  }
>  
> @@ -3368,8 +3415,16 @@ skl_update_sprite_wm(struct drm_plane *plane, struct drm_crtc *crtc,
>  	intel_plane->wm.scaled = scaled;
>  	intel_plane->wm.horiz_pixels = sprite_width;
>  	intel_plane->wm.vert_pixels = sprite_height;
> -	intel_plane->wm.bytes_per_pixel = pixel_size;
>  	intel_plane->wm.tiling = DRM_FORMAT_MOD_NONE;
> +
> +	/* For planar: Bpp is for UV plane, y_Bpp is for Y plane */
> +	intel_plane->wm.bytes_per_pixel =
> +		(fb && fb->pixel_format == DRM_FORMAT_NV12) ?
> +		drm_format_plane_cpp(plane->state->fb->pixel_format, 1) : pixel_size;
> +	intel_plane->wm.y_bytes_per_pixel =
> +		(fb && fb->pixel_format == DRM_FORMAT_NV12) ?
> +		drm_format_plane_cpp(plane->state->fb->pixel_format, 0) : 0;
> +
>  	/*
>  	 * Framebuffer can be NULL on plane disable, but it does not
>  	 * matter for watermarks if we assume no tiling in that case.
> -- 
> 1.7.9.5
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Adding dbuf support for skl nv12 format.

[Daniel Vetter]

On Fri, May 08, 2015 at 06:23:45PM +0100, Damien Lespiau wrote:
> On Mon, Apr 27, 2015 at 03:47:37PM -0700, Chandra Konduru wrote:
> > Skylake nv12 format requires dbuf (aka. ddb) calculations
> > and programming for each of y and uv sub-planes. Made minor
> > changes to reuse current dbuf calculations and programming
> > for uv plane. i.e., with this change, existing computation
> > is used for either packed format or uv portion of nv12
> > depending on incoming format. Added new code for dbuf
> > computation and programming for y plane.
> > 
> > This patch is a pre-requisite for adding NV12 format support.
> > Actual nv12 support is coming in later patches.
> > 
> > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> 
> Talked with Chandra about the previous comments. The Y tiling problem I
> was mentioning isn't quite part of NV12 and can be fixed on top of this.
> I'll probaby try my suggestions and create a series on top of this
> patch.

Wrt unifying the wm code, I honestly wonder whether we need our current
indirection. I kinda made sense before universal planes and atomic. But
know we have drm_plane_state subclassed. So imo the better approach would
be to directly fish things out of there, or if _really_ need add something
to intel_plane_state. But on a quick look a few weeks ago most of the wm
structs seemed redundant with drm_plane_state or plane_state->fb.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: Adding dbuf support for skl nv12 format.

[Daniel Vetter]

On Fri, May 08, 2015 at 06:23:45PM +0100, Damien Lespiau wrote:
> On Mon, Apr 27, 2015 at 03:47:37PM -0700, Chandra Konduru wrote:
> > Skylake nv12 format requires dbuf (aka. ddb) calculations
> > and programming for each of y and uv sub-planes. Made minor
> > changes to reuse current dbuf calculations and programming
> > for uv plane. i.e., with this change, existing computation
> > is used for either packed format or uv portion of nv12
> > depending on incoming format. Added new code for dbuf
> > computation and programming for y plane.
> > 
> > This patch is a pre-requisite for adding NV12 format support.
> > Actual nv12 support is coming in later patches.
> > 
> > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> 
> Talked with Chandra about the previous comments. The Y tiling problem I
> was mentioning isn't quite part of NV12 and can be fixed on top of this.
> I'll probaby try my suggestions and create a series on top of this
> patch.
> 
> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

Oh and ofc: Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx