[PATCH 00/15] drm/i915: Adding NV12 for skylake display

[PATCH 00/15] drm/i915: Adding NV12 for skylake display

[Chandra Konduru]

This patch series is adding initial NV12 support for Skylake display
after rebasing on latest drm-intel-nightly. Earlier I had two patch
series one for 0/180 and another for 90/270. Some of the patches
were already merged. This is combined series to support 0/90/180/270
and removing the ones that are already merged.

Feature is tested with igt/kms_nv12 testcases.
Feature is unit tested for linear/X/Y formats in 0, 90, 180, 270
orientations with combinations of 1 or 2 planes enabled along with
scaling. Also negatively tested for enabling NV12 on unsupported
plane.

The last patch in this series depends on Tvrtko's GEM remapping
for NV12 format patch series.

First two patches fixing couple things in dbuf logic to allocate
correct min number of dbuf blocks and use correct source width
and height in 90/270 rotation cases.

Chandra Konduru (15):
  drm/i915: Allocate min dbuf blocks per bspec
  drm/i915: In DBUF/WM calcs for 90/270, swap w & h
  drm/i915: Add register definitions for NV12 support
  drm/i915: Set scaler mode for NV12
  drm/i915: Stage scaler request for NV12 as src format
  drm/i915: Update format_is_yuv() to include NV12
  drm/i915: Upscale scaler max scale for NV12.
  drm/i915: Add NV12 as supported format for primary plane
  drm/i915: Add NV12 as supported format for sprite plane
  drm/i915: Add NV12 support to intel_framebuffer_init
  drm/i915: Add NV12 to primary plane programming.
  drm/i915: Add NV12 to sprite plane programming.
  drm/i915: Set initial phase & trip for NV12 scaler
  drm/i915: skl nv12 workarounds
  drm/i915: Add 90/270 rotation for NV12 format.

 drivers/gpu/drm/i915/i915_reg.h      |   47 +++++++++
 drivers/gpu/drm/i915/intel_atomic.c  |    5 +-
 drivers/gpu/drm/i915/intel_csr.c     |    2 +-
 drivers/gpu/drm/i915/intel_display.c |  178 +++++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_drv.h     |    5 +-
 drivers/gpu/drm/i915/intel_pm.c      |   71 ++++++++++++--
 drivers/gpu/drm/i915/intel_sprite.c  |  121 +++++++++++++++++++----
 7 files changed, 387 insertions(+), 42 deletions(-)

-- 
1.7.9.5

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

[PATCH 01/15] drm/i915: Allocate min dbuf blocks per bspec

[Chandra Konduru]

Properly allocate min blocks per hw requirements.

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c |   39 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index fff0c22..da3046f 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -2959,6 +2959,41 @@ skl_get_total_relative_data_rate(struct intel_crtc *intel_crtc,
 	return total_data_rate;
 }
 
+static uint16_t
+skl_dbuf_min_alloc(const struct intel_plane_wm_parameters *p, int y_plane)
+{
+	uint16_t min_alloc;
+
+	/* For packed formats, no y-plane, return 0 */
+	if (y_plane && !p->y_bytes_per_pixel)
+		return 0;
+
+
+	if (p->tiling == I915_FORMAT_MOD_Y_TILED ||
+	    p->tiling == I915_FORMAT_MOD_Yf_TILED) {
+		uint32_t min_scanlines = 8;
+		uint8_t bytes_per_pixel =
+			y_plane ? p->y_bytes_per_pixel : p->bytes_per_pixel;
+
+		switch (bytes_per_pixel) {
+		case 1:
+			min_scanlines = 32;
+			break;
+		case 2:
+			min_scanlines = 16;
+			break;
+		case 8:
+			WARN(1, "Unsupported pixel depth for rotation");
+		}
+		min_alloc = DIV_ROUND_UP((4 * p->horiz_pixels/(y_plane ? 1 : 2) *
+			bytes_per_pixel), 512) * min_scanlines/4 + 3;
+	} else {
+		min_alloc = 8;
+	}
+
+	return min_alloc;
+}
+
 static void
 skl_allocate_pipe_ddb(struct drm_crtc *crtc,
 		      const struct intel_wm_config *config,
@@ -2999,9 +3034,9 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
 		if (!p->enabled)
 			continue;
 
-		minimum[plane] = 8;
+		minimum[plane] = skl_dbuf_min_alloc(p, 0);    /* uv-plane/packed */
 		alloc_size -= minimum[plane];
-		y_minimum[plane] = p->y_bytes_per_pixel ? 8 : 0;
+		y_minimum[plane] = skl_dbuf_min_alloc(p, 1);  /* y-plane */
 		alloc_size -= y_minimum[plane];
 	}
 
-- 
1.7.9.5

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

Re: [PATCH 01/15] drm/i915: Allocate min dbuf blocks per bspec

[Ville Syrjälä]

On Wed, Aug 19, 2015 at 06:02:22PM -0700, Chandra Konduru wrote:
> Properly allocate min blocks per hw requirements.
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c |   39 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 37 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index fff0c22..da3046f 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -2959,6 +2959,41 @@ skl_get_total_relative_data_rate(struct intel_crtc *intel_crtc,
>  	return total_data_rate;
>  }
>  
> +static uint16_t
> +skl_dbuf_min_alloc(const struct intel_plane_wm_parameters *p, int y_plane)

bool y_plane

In general I dislike the fact that the code treats Y as the special case
instead of CbCr. The oppostire would be far more natural IMO, and,
I believe, would result in less checks in the code all around. Althouh
I think in general we should just pass around the format and plane index.

> +{
> +	uint16_t min_alloc;
> +
> +	/* For packed formats, no y-plane, return 0 */
> +	if (y_plane && !p->y_bytes_per_pixel)
> +		return 0;
> +
> +

Extra line.

> +	if (p->tiling == I915_FORMAT_MOD_Y_TILED ||
> +	    p->tiling == I915_FORMAT_MOD_Yf_TILED) {
> +		uint32_t min_scanlines = 8;
> +		uint8_t bytes_per_pixel =
> +			y_plane ? p->y_bytes_per_pixel : p->bytes_per_pixel;
> +
> +		switch (bytes_per_pixel) {
> +		case 1:
> +			min_scanlines = 32;
> +			break;
> +		case 2:
> +			min_scanlines = 16;
> +			break;
> +		case 8:
> +			WARN(1, "Unsupported pixel depth for rotation");
> +		}

Could be just 32/cpp.

> +		min_alloc = DIV_ROUND_UP((4 * p->horiz_pixels/(y_plane ? 1 : 2) *
> +			bytes_per_pixel), 512) * min_scanlines/4 + 3;

Another case that could be simplified by removing the y_plane special
casing. In fact just passing in the format and plane index in we could get:

cpp = drm_format_plane_cpp(format, plane);
width = width / drm_format_horiz_subsampling(format);
min_scanlines = 32 / cpp;
min_alloc = DIV_ROUND_UP(4 * width * cpp, 512) * min_scanlines / 4 + 3;


We could even move the width/subsampling (and height too) thing into a
common helper in a drm header, eg.:

static inline int drm_format_plane_width(format, plane, width)
{
	if (plane)
		return width / drm_format_plane_horiz_subsampling(format);
	else
		return width;
}
static inline int drm_format_plane_height(format, plane, height);
{
	if (plane)
		return height / drm_format_plane_vert_subsampling(format);
	else
		return height;
}

> +	} else {
> +		min_alloc = 8;
> +	}
> +
> +	return min_alloc;
> +}
> +
>  static void
>  skl_allocate_pipe_ddb(struct drm_crtc *crtc,
>  		      const struct intel_wm_config *config,
> @@ -2999,9 +3034,9 @@ skl_allocate_pipe_ddb(struct drm_crtc *crtc,
>  		if (!p->enabled)
>  			continue;
>  
> -		minimum[plane] = 8;
> +		minimum[plane] = skl_dbuf_min_alloc(p, 0);    /* uv-plane/packed */
>  		alloc_size -= minimum[plane];
> -		y_minimum[plane] = p->y_bytes_per_pixel ? 8 : 0;
> +		y_minimum[plane] = skl_dbuf_min_alloc(p, 1);  /* y-plane */

false/true instead of 0/1.

With the bool things changed this does what it says so:
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

>  		alloc_size -= y_minimum[plane];
>  	}
>  
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 02/15] drm/i915: In DBUF/WM calcs for 90/270, swap w & h

[Chandra Konduru]

This patch swaps src width and height for dbuf/wm calculations
when rotation is 90/270 as per hw requirements.

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/intel_pm.c |   32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index da3046f..c455946 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -3193,6 +3193,8 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
 	enum pipe pipe = intel_crtc->pipe;
 	struct drm_plane *plane;
 	struct drm_framebuffer *fb;
+	struct intel_plane_state *plane_state;
+	int src_w, src_h;
 	int i = 1; /* Index for sprite planes start */
 
 	p->active = intel_crtc->active;
@@ -3201,6 +3203,7 @@ 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;
+		plane_state = to_intel_plane_state(crtc->primary->state);
 		/* For planar: Bpp is for uv plane, y_Bpp is for y plane */
 		if (fb) {
 			p->plane[0].enabled = true;
@@ -3215,8 +3218,22 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
 			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;
-		p->plane[0].vert_pixels = intel_crtc->config->pipe_src_h;
+
+		if (drm_rect_width(&plane_state->src)) {
+			src_w = drm_rect_width(&plane_state->src) >> 16;
+			src_h = drm_rect_height(&plane_state->src) >> 16;
+		} else {
+			src_w = intel_crtc->config->pipe_src_w;
+			src_h = intel_crtc->config->pipe_src_h;
+		}
+
+		if (intel_rotation_90_or_270(crtc->primary->state->rotation)) {
+			p->plane[0].horiz_pixels = src_h;
+			p->plane[0].vert_pixels = src_w;
+		} else {
+			p->plane[0].horiz_pixels = src_w;
+			p->plane[0].vert_pixels = src_h;
+		}
 		p->plane[0].rotation = crtc->primary->state->rotation;
 
 		fb = crtc->cursor->state->fb;
@@ -3750,8 +3767,15 @@ skl_update_sprite_wm(struct drm_plane *plane, struct drm_crtc *crtc,
 
 	intel_plane->wm.enabled = enabled;
 	intel_plane->wm.scaled = scaled;
-	intel_plane->wm.horiz_pixels = sprite_width;
-	intel_plane->wm.vert_pixels = sprite_height;
+
+	if (intel_rotation_90_or_270(plane->state->rotation)) {
+		intel_plane->wm.horiz_pixels = sprite_height;
+		intel_plane->wm.vert_pixels = sprite_width;
+	} else {
+		intel_plane->wm.horiz_pixels = sprite_width;
+		intel_plane->wm.vert_pixels = sprite_height;
+	}
+
 	intel_plane->wm.tiling = DRM_FORMAT_MOD_NONE;
 
 	/* For planar: Bpp is for UV plane, y_Bpp is for Y plane */
-- 
1.7.9.5

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

Re: [PATCH 02/15] drm/i915: In DBUF/WM calcs for 90/270, swap w & h

[Ville Syrjälä]

On Wed, Aug 19, 2015 at 06:02:23PM -0700, Chandra Konduru wrote:
> This patch swaps src width and height for dbuf/wm calculations
> when rotation is 90/270 as per hw requirements.

The spec is rather unclear about this. It only says:
"If plane 90 or 270 rotation is enabled, use the rotated width and
height in pixel rate calculations."

Although it does make sense based on the fact that the display
fetch will happen in the rotated direction.

I tried to reconcile some if this stuff in my head but when I went to
read the Yf/Ys tiling documentation I got even more confused. What it
says there is that a single cacheline always contains 4 rows of pixels,
but then in the next sentence it claims the aspect ratio will be 1:1
for 8,32,128 bpp, and 2:1 for 16,64 bpp. Those two things can't both be
true. Can someone tell me which one is true?

> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c |   32 ++++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index da3046f..c455946 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -3193,6 +3193,8 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
>  	enum pipe pipe = intel_crtc->pipe;
>  	struct drm_plane *plane;
>  	struct drm_framebuffer *fb;
> +	struct intel_plane_state *plane_state;
> +	int src_w, src_h;

These can be moved to a narrower scope. plane_state could be const I
believe.

>  	int i = 1; /* Index for sprite planes start */
>  
>  	p->active = intel_crtc->active;
> @@ -3201,6 +3203,7 @@ 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;
> +		plane_state = to_intel_plane_state(crtc->primary->state);
>  		/* For planar: Bpp is for uv plane, y_Bpp is for y plane */
>  		if (fb) {
>  			p->plane[0].enabled = true;
> @@ -3215,8 +3218,22 @@ static void skl_compute_wm_pipe_parameters(struct drm_crtc *crtc,
>  			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;
> -		p->plane[0].vert_pixels = intel_crtc->config->pipe_src_h;
> +
> +		if (drm_rect_width(&plane_state->src)) {
> +			src_w = drm_rect_width(&plane_state->src) >> 16;
> +			src_h = drm_rect_height(&plane_state->src) >> 16;
> +		} else {
> +			src_w = intel_crtc->config->pipe_src_w;
> +			src_h = intel_crtc->config->pipe_src_h;
> +		}

Are we still not having a consistent plane state all the time, or why is
this kludge needed?

> +
> +		if (intel_rotation_90_or_270(crtc->primary->state->rotation)) {
> +			p->plane[0].horiz_pixels = src_h;
> +			p->plane[0].vert_pixels = src_w;
> +		} else {
> +			p->plane[0].horiz_pixels = src_w;
> +			p->plane[0].vert_pixels = src_h;
> +		}
>  		p->plane[0].rotation = crtc->primary->state->rotation;
>  
>  		fb = crtc->cursor->state->fb;
> @@ -3750,8 +3767,15 @@ skl_update_sprite_wm(struct drm_plane *plane, struct drm_crtc *crtc,
>  
>  	intel_plane->wm.enabled = enabled;
>  	intel_plane->wm.scaled = scaled;
> -	intel_plane->wm.horiz_pixels = sprite_width;
> -	intel_plane->wm.vert_pixels = sprite_height;
> +
> +	if (intel_rotation_90_or_270(plane->state->rotation)) {
> +		intel_plane->wm.horiz_pixels = sprite_height;
> +		intel_plane->wm.vert_pixels = sprite_width;
> +	} else {
> +		intel_plane->wm.horiz_pixels = sprite_width;
> +		intel_plane->wm.vert_pixels = sprite_height;
> +	}
> +
>  	intel_plane->wm.tiling = DRM_FORMAT_MOD_NONE;
>  
>  	/* For planar: Bpp is for UV plane, y_Bpp is for Y plane */
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 03/15] drm/i915: Add register definitions for NV12 support

[Chandra Konduru]

This patch adds register definitions for skylake
display NV12 support.

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h |   27 +++++++++++++++++++++++++++
 1 file changed, 27 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 1fa0554..c4d732f 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5498,6 +5498,7 @@ enum skl_disp_power_wells {
 #define PS_SCALER_MODE_MASK (3 << 28)
 #define PS_SCALER_MODE_DYN  (0 << 28)
 #define PS_SCALER_MODE_HQ  (1 << 28)
+#define PS_SCALER_MODE_NV12 (2 << 28)
 #define PS_PLANE_SEL_MASK  (7 << 25)
 #define PS_PLANE_SEL(plane) ((plane + 1) << 25)
 #define PS_FILTER_MASK         (3 << 23)
@@ -5601,6 +5602,32 @@ enum skl_disp_power_wells {
 			_ID(id, _PS_ECC_STAT_1A, _PS_ECC_STAT_2A),   \
 			_ID(id, _PS_ECC_STAT_1B, _PS_ECC_STAT_2B)
 
+
+/*
+ * Skylake  NV12 Register
+ */
+#define PLANE_AUX_DIST_1_A		0x701c0
+#define PLANE_AUX_DIST_2_A		0x702c0
+#define PLANE_AUX_DIST_1_B		0x711c0
+#define PLANE_AUX_DIST_2_B		0x712c0
+#define _PLANE_AUX_DIST_1(pipe)	\
+   _PIPE(pipe, PLANE_AUX_DIST_1_A, PLANE_AUX_DIST_1_B)
+#define _PLANE_AUX_DIST_2(pipe)	\
+   _PIPE(pipe, PLANE_AUX_DIST_2_A, PLANE_AUX_DIST_2_B)
+#define PLANE_AUX_DIST(pipe, plane)	\
+	_PLANE(plane, _PLANE_AUX_DIST_1(pipe), _PLANE_AUX_DIST_2(pipe))
+
+#define PLANE_AUX_OFFSET_1_A		0x701c4
+#define PLANE_AUX_OFFSET_2_A		0x702c4
+#define PLANE_AUX_OFFSET_1_B		0x711c4
+#define PLANE_AUX_OFFSET_2_B		0x712c4
+#define _PLANE_AUX_OFFSET_1(pipe)	\
+   _PIPE(pipe, PLANE_AUX_OFFSET_1_A, PLANE_AUX_OFFSET_1_B)
+#define _PLANE_AUX_OFFSET_2(pipe)	\
+   _PIPE(pipe, PLANE_AUX_OFFSET_2_A, PLANE_AUX_OFFSET_2_B)
+#define PLANE_AUX_OFFSET(pipe, plane)	\
+	_PLANE(plane, _PLANE_AUX_OFFSET_1(pipe), _PLANE_AUX_OFFSET_2(pipe))
+
 /* legacy palette */
 #define _LGC_PALETTE_A           0x4a000
 #define _LGC_PALETTE_B           0x4a800
-- 
1.7.9.5

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

Re: [PATCH 03/15] drm/i915: Add register definitions for NV12 support

[Ville Syrjälä]

On Wed, Aug 19, 2015 at 06:02:24PM -0700, Chandra Konduru wrote:
> This patch adds register definitions for skylake
> display NV12 support.
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h |   27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 1fa0554..c4d732f 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5498,6 +5498,7 @@ enum skl_disp_power_wells {
>  #define PS_SCALER_MODE_MASK (3 << 28)
>  #define PS_SCALER_MODE_DYN  (0 << 28)
>  #define PS_SCALER_MODE_HQ  (1 << 28)
> +#define PS_SCALER_MODE_NV12 (2 << 28)
>  #define PS_PLANE_SEL_MASK  (7 << 25)
>  #define PS_PLANE_SEL(plane) ((plane + 1) << 25)
>  #define PS_FILTER_MASK         (3 << 23)
> @@ -5601,6 +5602,32 @@ enum skl_disp_power_wells {
>  			_ID(id, _PS_ECC_STAT_1A, _PS_ECC_STAT_2A),   \
>  			_ID(id, _PS_ECC_STAT_1B, _PS_ECC_STAT_2B)
>  
> +
> +/*
> + * Skylake  NV12 Register
> + */
> +#define PLANE_AUX_DIST_1_A		0x701c0
> +#define PLANE_AUX_DIST_2_A		0x702c0
> +#define PLANE_AUX_DIST_1_B		0x711c0
> +#define PLANE_AUX_DIST_2_B		0x712c0
> +#define _PLANE_AUX_DIST_1(pipe)	\
> +   _PIPE(pipe, PLANE_AUX_DIST_1_A, PLANE_AUX_DIST_1_B)
> +#define _PLANE_AUX_DIST_2(pipe)	\
> +   _PIPE(pipe, PLANE_AUX_DIST_2_A, PLANE_AUX_DIST_2_B)
> +#define PLANE_AUX_DIST(pipe, plane)	\
> +	_PLANE(plane, _PLANE_AUX_DIST_1(pipe), _PLANE_AUX_DIST_2(pipe))
> +
> +#define PLANE_AUX_OFFSET_1_A		0x701c4
> +#define PLANE_AUX_OFFSET_2_A		0x702c4
> +#define PLANE_AUX_OFFSET_1_B		0x711c4
> +#define PLANE_AUX_OFFSET_2_B		0x712c4
> +#define _PLANE_AUX_OFFSET_1(pipe)	\
> +   _PIPE(pipe, PLANE_AUX_OFFSET_1_A, PLANE_AUX_OFFSET_1_B)
> +#define _PLANE_AUX_OFFSET_2(pipe)	\
> +   _PIPE(pipe, PLANE_AUX_OFFSET_2_A, PLANE_AUX_OFFSET_2_B)
> +#define PLANE_AUX_OFFSET(pipe, plane)	\
> +	_PLANE(plane, _PLANE_AUX_OFFSET_1(pipe), _PLANE_AUX_OFFSET_2(pipe))
> +

Bspec seems entirely confused about these registers, not showing
AUX_OFFSET for SKL at all, and even the normal OFFSET has lost all the
information about the register offsets. But it does match what's there
for gen10, so I suppose it's all good.

But this stuff could be squashed to whichever patch(es) actually use the
new defines.

>  /* legacy palette */
>  #define _LGC_PALETTE_A           0x4a000
>  #define _LGC_PALETTE_B           0x4a800
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 04/15] drm/i915: Set scaler mode for NV12

[Chandra Konduru]

This patch sets appropriate scaler mode for NV12 format.
In this mode, skylake scaler does either chroma-upsampling or
chroma-upsampling and resolution scaling.

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/intel_atomic.c |    5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
index 9336e80..fd3972c 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -247,7 +247,10 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
 		}
 
 		/* set scaler mode */
-		if (num_scalers_need == 1 && intel_crtc->pipe != PIPE_C) {
+		if (plane_state && plane_state->base.fb &&
+			plane_state->base.fb->pixel_format == DRM_FORMAT_NV12) {
+			scaler_state->scalers[*scaler_id].mode = PS_SCALER_MODE_NV12;
+		} else if (num_scalers_need == 1 && intel_crtc->pipe != PIPE_C) {
 			/*
 			 * when only 1 scaler is in use on either pipe A or B,
 			 * scaler 0 operates in high quality (HQ) mode.
-- 
1.7.9.5

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

Re: [PATCH 04/15] drm/i915: Set scaler mode for NV12

[Ville Syrjälä]

On Wed, Aug 19, 2015 at 06:02:25PM -0700, Chandra Konduru wrote:
> This patch sets appropriate scaler mode for NV12 format.
> In this mode, skylake scaler does either chroma-upsampling or
> chroma-upsampling and resolution scaling.
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_atomic.c |    5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_atomic.c b/drivers/gpu/drm/i915/intel_atomic.c
> index 9336e80..fd3972c 100644
> --- a/drivers/gpu/drm/i915/intel_atomic.c
> +++ b/drivers/gpu/drm/i915/intel_atomic.c
> @@ -247,7 +247,10 @@ int intel_atomic_setup_scalers(struct drm_device *dev,
>  		}
>  
>  		/* set scaler mode */
> -		if (num_scalers_need == 1 && intel_crtc->pipe != PIPE_C) {
> +		if (plane_state && plane_state->base.fb &&
> +			plane_state->base.fb->pixel_format == DRM_FORMAT_NV12) {
> +			scaler_state->scalers[*scaler_id].mode = PS_SCALER_MODE_NV12;
> +		} else if (num_scalers_need == 1 && intel_crtc->pipe != PIPE_C) {
>  			/*
>  			 * when only 1 scaler is in use on either pipe A or B,
>  			 * scaler 0 operates in high quality (HQ) mode.

I was almost going to say that we don't have code like this, but then I
found it hiding in intel_atomic.c for whatever reason.

Anyway, the patch looks good so
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

On a further note, this function could use some cleaning to move
various variables into narrower scope. Now it's rather hard to see what
is valid per iteration and what is valid across the entire loop.

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 04/15] drm/i915: Set scaler mode for NV12

[Daniel Vetter]

On Fri, Sep 04, 2015 at 11:53:36AM +0300, Ville Syrjälä wrote:
> On a further note, this function could use some cleaning to move
> various variables into narrower scope. Now it's rather hard to see what
> is valid per iteration and what is valid across the entire loop.

It's also rather big, so might just want to split out the inner parts
perhaps into helper functions.
-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

[PATCH 05/15] drm/i915: Stage scaler request for NV12 as src format

[Chandra Konduru]

This patch stages a scaler request when input format
is NV12. The same scaler does both chroma-upsampling
and resolution scaling as needed.

v2:
-Added helper function for need_scaling (Ville)

v3:
-Rebased to current kernel version 4.2.0.rc4 (me)

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3ee1c17..411b211 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4341,10 +4341,27 @@ static void cpt_verify_modeset(struct drm_device *dev, int pipe)
 	}
 }
 
+static int skl_need_scaling(int src_w, int dst_w, int src_h, int dst_h,
+	int rotation, uint32_t pixel_format)
+{
+	/* scaling is required when src dst sizes doesn't match or format is NV12 */
+	if (src_w != dst_w || src_h != dst_h)
+		return 1;
+
+	if (intel_rotation_90_or_270(rotation) &&
+			(src_h != dst_w || src_w != dst_h))
+		return 1;
+
+	if (pixel_format == DRM_FORMAT_NV12)
+		return 1;
+
+	return 0;
+}
+
 static int
 skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
 		  unsigned scaler_user, int *scaler_id, unsigned int rotation,
-		  int src_w, int src_h, int dst_w, int dst_h)
+		  int src_w, int src_h, int dst_w, int dst_h, uint32_t pixel_format)
 {
 	struct intel_crtc_scaler_state *scaler_state =
 		&crtc_state->scaler_state;
@@ -4352,9 +4369,8 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
 		to_intel_crtc(crtc_state->base.crtc);
 	int need_scaling;
 
-	need_scaling = intel_rotation_90_or_270(rotation) ?
-		(src_h != dst_w || src_w != dst_h):
-		(src_w != dst_w || src_h != dst_h);
+	need_scaling = skl_need_scaling(src_w, dst_w, src_h, dst_h, rotation,
+		pixel_format);
 
 	/*
 	 * if plane is being disabled or scaler is no more required or force detach
@@ -4423,7 +4439,7 @@ int skl_update_scaler_crtc(struct intel_crtc_state *state)
 	return skl_update_scaler(state, !state->base.active, SKL_CRTC_INDEX,
 		&state->scaler_state.scaler_id, DRM_ROTATE_0,
 		state->pipe_src_w, state->pipe_src_h,
-		adjusted_mode->hdisplay, adjusted_mode->vdisplay);
+		adjusted_mode->hdisplay, adjusted_mode->vdisplay, 0);
 }
 
 /**
@@ -4459,7 +4475,8 @@ static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
 				drm_rect_width(&plane_state->src) >> 16,
 				drm_rect_height(&plane_state->src) >> 16,
 				drm_rect_width(&plane_state->dst),
-				drm_rect_height(&plane_state->dst));
+				drm_rect_height(&plane_state->dst),
+				fb ? fb->pixel_format : 0);
 
 	if (ret || plane_state->scaler_id < 0)
 		return ret;
@@ -4484,6 +4501,7 @@ static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
 	case DRM_FORMAT_YVYU:
 	case DRM_FORMAT_UYVY:
 	case DRM_FORMAT_VYUY:
+	case DRM_FORMAT_NV12:
 		break;
 	default:
 		DRM_DEBUG_KMS("[PLANE:%d] FB:%d unsupported scaling format 0x%x\n",
-- 
1.7.9.5

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

Re: [PATCH 05/15] drm/i915: Stage scaler request for NV12 as src format

[Ville Syrjälä]

On Wed, Aug 19, 2015 at 06:02:26PM -0700, Chandra Konduru wrote:
> This patch stages a scaler request when input format
> is NV12. The same scaler does both chroma-upsampling
> and resolution scaling as needed.
> 
> v2:
> -Added helper function for need_scaling (Ville)
> 
> v3:
> -Rebased to current kernel version 4.2.0.rc4 (me)
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   30 ++++++++++++++++++++++++------
>  1 file changed, 24 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3ee1c17..411b211 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4341,10 +4341,27 @@ static void cpt_verify_modeset(struct drm_device *dev, int pipe)
>  	}
>  }
>  
> +static int skl_need_scaling(int src_w, int dst_w, int src_h, int dst_h,

Make it bool.

I'd reorder the arguments to group src_w and src_h together,
and dst_w and dst_h together.

> +	int rotation, uint32_t pixel_format)

Indentation is weird. rotation should be unsigned int.

> +{
> +	/* scaling is required when src dst sizes doesn't match or format is NV12 */
> +	if (src_w != dst_w || src_h != dst_h)
> +		return 1;
> +
> +	if (intel_rotation_90_or_270(rotation) &&
> +			(src_h != dst_w || src_w != dst_h))
> +		return 1;

I would make this something like:

{
	if (format == NV12)
		return true;
	
	if (90_270)
		return ...;
	else
		return ...;
}

> +
> +	if (pixel_format == DRM_FORMAT_NV12)
> +		return 1;
> +
> +	return 0;
> +}
> +
>  static int
>  skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>  		  unsigned scaler_user, int *scaler_id, unsigned int rotation,
> -		  int src_w, int src_h, int dst_w, int dst_h)
> +		  int src_w, int src_h, int dst_w, int dst_h, uint32_t pixel_format)
>  {
>  	struct intel_crtc_scaler_state *scaler_state =
>  		&crtc_state->scaler_state;
> @@ -4352,9 +4369,8 @@ skl_update_scaler(struct intel_crtc_state *crtc_state, bool force_detach,
>  		to_intel_crtc(crtc_state->base.crtc);
>  	int need_scaling;
>  
> -	need_scaling = intel_rotation_90_or_270(rotation) ?
> -		(src_h != dst_w || src_w != dst_h):
> -		(src_w != dst_w || src_h != dst_h);
> +	need_scaling = skl_need_scaling(src_w, dst_w, src_h, dst_h, rotation,
> +		pixel_format);
>  
>  	/*
>  	 * if plane is being disabled or scaler is no more required or force detach
> @@ -4423,7 +4439,7 @@ int skl_update_scaler_crtc(struct intel_crtc_state *state)
>  	return skl_update_scaler(state, !state->base.active, SKL_CRTC_INDEX,
>  		&state->scaler_state.scaler_id, DRM_ROTATE_0,
>  		state->pipe_src_w, state->pipe_src_h,
> -		adjusted_mode->hdisplay, adjusted_mode->vdisplay);
> +		adjusted_mode->hdisplay, adjusted_mode->vdisplay, 0);
>  }
>  
>  /**
> @@ -4459,7 +4475,8 @@ static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
>  				drm_rect_width(&plane_state->src) >> 16,
>  				drm_rect_height(&plane_state->src) >> 16,
>  				drm_rect_width(&plane_state->dst),
> -				drm_rect_height(&plane_state->dst));
> +				drm_rect_height(&plane_state->dst),
> +				fb ? fb->pixel_format : 0);
>  
>  	if (ret || plane_state->scaler_id < 0)
>  		return ret;
> @@ -4484,6 +4501,7 @@ static int skl_update_scaler_plane(struct intel_crtc_state *crtc_state,
>  	case DRM_FORMAT_YVYU:
>  	case DRM_FORMAT_UYVY:
>  	case DRM_FORMAT_VYUY:
> +	case DRM_FORMAT_NV12:
>  		break;
>  	default:
>  		DRM_DEBUG_KMS("[PLANE:%d] FB:%d unsupported scaling format 0x%x\n",
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 06/15] drm/i915: Update format_is_yuv() to include NV12

[Chandra Konduru]

This patch adds NV12 to format_is_yuv() function
and made it available for both primary and sprite
planes.

v2:
-Use intel_ prefix for format_is_yuv (Ville)

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h    |    1 +
 drivers/gpu/drm/i915/intel_sprite.c |    9 +++++----
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f44941b..18632a4 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1394,6 +1394,7 @@ int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
 void intel_pipe_update_start(struct intel_crtc *crtc,
 			     uint32_t *start_vbl_count);
 void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count);
+bool intel_format_is_yuv(uint32_t format);
 
 /* intel_tv.c */
 void intel_tv_init(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index c13c529..8b73bb8 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -39,14 +39,15 @@
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 
-static bool
-format_is_yuv(uint32_t format)
+bool
+intel_format_is_yuv(uint32_t format)
 {
 	switch (format) {
 	case DRM_FORMAT_YUYV:
 	case DRM_FORMAT_UYVY:
 	case DRM_FORMAT_VYUY:
 	case DRM_FORMAT_YVYU:
+	case DRM_FORMAT_NV12:
 		return true;
 	default:
 		return false;
@@ -293,7 +294,7 @@ chv_update_csc(struct intel_plane *intel_plane, uint32_t format)
 	int plane = intel_plane->plane;
 
 	/* Seems RGB data bypasses the CSC always */
-	if (!format_is_yuv(format))
+	if (!intel_format_is_yuv(format))
 		return;
 
 	/*
@@ -857,7 +858,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
 		src_y = src->y1 >> 16;
 		src_h = drm_rect_height(src) >> 16;
 
-		if (format_is_yuv(fb->pixel_format)) {
+		if (intel_format_is_yuv(fb->pixel_format)) {
 			src_x &= ~1;
 			src_w &= ~1;
 
-- 
1.7.9.5

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

Re: [PATCH 06/15] drm/i915: Update format_is_yuv() to include NV12

[Ville Syrjälä]

On Wed, Aug 19, 2015 at 06:02:27PM -0700, Chandra Konduru wrote:
> This patch adds NV12 to format_is_yuv() function
> and made it available for both primary and sprite
> planes.
> 
> v2:
> -Use intel_ prefix for format_is_yuv (Ville)
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_drv.h    |    1 +
>  drivers/gpu/drm/i915/intel_sprite.c |    9 +++++----
>  2 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index f44941b..18632a4 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1394,6 +1394,7 @@ int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
>  void intel_pipe_update_start(struct intel_crtc *crtc,
>  			     uint32_t *start_vbl_count);
>  void intel_pipe_update_end(struct intel_crtc *crtc, u32 start_vbl_count);
> +bool intel_format_is_yuv(uint32_t format);
>  
>  /* intel_tv.c */
>  void intel_tv_init(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index c13c529..8b73bb8 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -39,14 +39,15 @@
>  #include <drm/i915_drm.h>
>  #include "i915_drv.h"
>  
> -static bool
> -format_is_yuv(uint32_t format)
> +bool
> +intel_format_is_yuv(uint32_t format)
>  {
>  	switch (format) {
>  	case DRM_FORMAT_YUYV:
>  	case DRM_FORMAT_UYVY:
>  	case DRM_FORMAT_VYUY:
>  	case DRM_FORMAT_YVYU:
> +	case DRM_FORMAT_NV12:
>  		return true;
>  	default:
>  		return false;
> @@ -293,7 +294,7 @@ chv_update_csc(struct intel_plane *intel_plane, uint32_t format)
>  	int plane = intel_plane->plane;
>  
>  	/* Seems RGB data bypasses the CSC always */
> -	if (!format_is_yuv(format))
> +	if (!intel_format_is_yuv(format))
>  		return;
>  
>  	/*
> @@ -857,7 +858,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  		src_y = src->y1 >> 16;
>  		src_h = drm_rect_height(src) >> 16;
>  
> -		if (format_is_yuv(fb->pixel_format)) {
> +		if (intel_format_is_yuv(fb->pixel_format)) {
>  			src_x &= ~1;
>  			src_w &= ~1;
>  
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 07/15] drm/i915: Upscale scaler max scale for NV12.

[Chandra Konduru]

This patch updates max supported scaler limits for NV12.

v2:
-Rebased to current kernel version 4.2.0.rc4 (me)

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   13 +++++++++----
 drivers/gpu/drm/i915/intel_drv.h     |    3 ++-
 drivers/gpu/drm/i915/intel_sprite.c  |    2 +-
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 411b211..b1d9edf 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13421,7 +13421,9 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
 }
 
 int
-skl_max_scale(struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state)
+skl_max_scale(struct intel_crtc *intel_crtc,
+	struct intel_crtc_state *crtc_state,
+	uint32_t pixel_format)
 {
 	int max_scale;
 	struct drm_device *dev;
@@ -13441,11 +13443,13 @@ skl_max_scale(struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state
 
 	/*
 	 * skl max scale is lower of:
-	 *    close to 3 but not 3, -1 is for that purpose
+	 *    close to 2 or 3 (NV12: 2, other formats: 3) but not equal,
+	 *          -1 is for that purpose
 	 *            or
 	 *    cdclk/crtc_clock
 	 */
-	max_scale = min((1 << 16) * 3 - 1, (1 << 8) * ((cdclk << 8) / crtc_clock));
+	max_scale = min((1 << 16) * (pixel_format == DRM_FORMAT_NV12 ? 2 : 3) - 1,
+		(1 << 8) * ((cdclk << 8) / crtc_clock));
 
 	return max_scale;
 }
@@ -13465,7 +13469,8 @@ intel_check_primary_plane(struct drm_plane *plane,
 	if (INTEL_INFO(plane->dev)->gen >= 9 &&
 	    state->ckey.flags == I915_SET_COLORKEY_NONE) {
 		min_scale = 1;
-		max_scale = skl_max_scale(to_intel_crtc(crtc), crtc_state);
+		max_scale = skl_max_scale(to_intel_crtc(crtc), crtc_state,
+				fb ? fb->pixel_format : 0);
 		can_position = true;
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 18632a4..d50b8cb 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1140,7 +1140,8 @@ void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
 void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
 
 int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state);
-int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state);
+int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state,
+	uint32_t pixel_format);
 
 unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
 				     struct drm_i915_gem_object *obj,
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 8b73bb8..66d60ae 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -780,7 +780,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
 		if (state->ckey.flags == I915_SET_COLORKEY_NONE) {
 			can_scale = 1;
 			min_scale = 1;
-			max_scale = skl_max_scale(intel_crtc, crtc_state);
+			max_scale = skl_max_scale(intel_crtc, crtc_state, fb->pixel_format);
 		} else {
 			can_scale = 0;
 			min_scale = DRM_PLANE_HELPER_NO_SCALING;
-- 
1.7.9.5

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

Re: [PATCH 07/15] drm/i915: Upscale scaler max scale for NV12.

[Ville Syrjälä]

On Wed, Aug 19, 2015 at 06:02:28PM -0700, Chandra Konduru wrote:
> This patch updates max supported scaler limits for NV12.
> 
> v2:
> -Rebased to current kernel version 4.2.0.rc4 (me)
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   13 +++++++++----
>  drivers/gpu/drm/i915/intel_drv.h     |    3 ++-
>  drivers/gpu/drm/i915/intel_sprite.c  |    2 +-
>  3 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 411b211..b1d9edf 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13421,7 +13421,9 @@ intel_cleanup_plane_fb(struct drm_plane *plane,
>  }
>  
>  int
> -skl_max_scale(struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state)
> +skl_max_scale(struct intel_crtc *intel_crtc,
> +	struct intel_crtc_state *crtc_state,
> +	uint32_t pixel_format)
>  {
>  	int max_scale;
>  	struct drm_device *dev;
> @@ -13441,11 +13443,13 @@ skl_max_scale(struct intel_crtc *intel_crtc, struct intel_crtc_state *crtc_state
>  
>  	/*
>  	 * skl max scale is lower of:
> -	 *    close to 3 but not 3, -1 is for that purpose
> +	 *    close to 2 or 3 (NV12: 2, other formats: 3) but not equal,
> +	 *          -1 is for that purpose
>  	 *            or
>  	 *    cdclk/crtc_clock
>  	 */
> -	max_scale = min((1 << 16) * 3 - 1, (1 << 8) * ((cdclk << 8) / crtc_clock));
> +	max_scale = min((1 << 16) * (pixel_format == DRM_FORMAT_NV12 ? 2 : 3) - 1,
> +		(1 << 8) * ((cdclk << 8) / crtc_clock));

Starting to be a bit messy.

Maybe something like

if (nv12)
	max_scale = (2 << 16) - 1;
else
	max_scale = (3 << 16) - 1;
max_scale = min(max_scale, ...);

?

>  
>  	return max_scale;
>  }
> @@ -13465,7 +13469,8 @@ intel_check_primary_plane(struct drm_plane *plane,
>  	if (INTEL_INFO(plane->dev)->gen >= 9 &&
>  	    state->ckey.flags == I915_SET_COLORKEY_NONE) {
>  		min_scale = 1;
> -		max_scale = skl_max_scale(to_intel_crtc(crtc), crtc_state);
> +		max_scale = skl_max_scale(to_intel_crtc(crtc), crtc_state,
> +				fb ? fb->pixel_format : 0);
>  		can_position = true;
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 18632a4..d50b8cb 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1140,7 +1140,8 @@ void intel_crtc_wait_for_pending_flips(struct drm_crtc *crtc);
>  void intel_modeset_preclose(struct drm_device *dev, struct drm_file *file);
>  
>  int skl_update_scaler_crtc(struct intel_crtc_state *crtc_state);
> -int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state);
> +int skl_max_scale(struct intel_crtc *crtc, struct intel_crtc_state *crtc_state,
> +	uint32_t pixel_format);
>  
>  unsigned long intel_plane_obj_offset(struct intel_plane *intel_plane,
>  				     struct drm_i915_gem_object *obj,
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 8b73bb8..66d60ae 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -780,7 +780,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  		if (state->ckey.flags == I915_SET_COLORKEY_NONE) {
>  			can_scale = 1;
>  			min_scale = 1;
> -			max_scale = skl_max_scale(intel_crtc, crtc_state);
> +			max_scale = skl_max_scale(intel_crtc, crtc_state, fb->pixel_format);
>  		} else {
>  			can_scale = 0;
>  			min_scale = DRM_PLANE_HELPER_NO_SCALING;
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 08/15] drm/i915: Add NV12 as supported format for primary plane

[Chandra Konduru]

This patch adds NV12 to list of supported formats for
primary plane.

v2:
-Rebased (me)

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
Testcase: igt/kms_nv12
---
 drivers/gpu/drm/i915/intel_display.c |   22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b1d9edf..e4a6a91 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -74,6 +74,19 @@ static const uint32_t skl_primary_formats[] = {
 	DRM_FORMAT_XBGR2101010,
 };
 
+/* Primary plane formats for gen >= 9 with NV12 */
+static const uint32_t skl_primary_formats_with_nv12[] = {
+	DRM_FORMAT_C8,
+	DRM_FORMAT_RGB565,
+	DRM_FORMAT_XRGB8888,
+	DRM_FORMAT_XBGR8888,
+	DRM_FORMAT_ARGB8888,
+	DRM_FORMAT_ABGR8888,
+	DRM_FORMAT_XRGB2101010,
+	DRM_FORMAT_XBGR2101010,
+	DRM_FORMAT_NV12,
+};
+
 /* Cursor formats */
 static const uint32_t intel_cursor_formats[] = {
 	DRM_FORMAT_ARGB8888,
@@ -13606,8 +13619,13 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
 		primary->plane = !pipe;
 
 	if (INTEL_INFO(dev)->gen >= 9) {
-		intel_primary_formats = skl_primary_formats;
-		num_formats = ARRAY_SIZE(skl_primary_formats);
+		if (pipe == PIPE_A || pipe == PIPE_B) {
+			intel_primary_formats = skl_primary_formats_with_nv12;
+			num_formats = ARRAY_SIZE(skl_primary_formats_with_nv12);
+		} else {
+			intel_primary_formats = skl_primary_formats;
+			num_formats = ARRAY_SIZE(skl_primary_formats);
+		}
 	} else if (INTEL_INFO(dev)->gen >= 4) {
 		intel_primary_formats = i965_primary_formats;
 		num_formats = ARRAY_SIZE(i965_primary_formats);
-- 
1.7.9.5

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

Re: [PATCH 08/15] drm/i915: Add NV12 as supported format for primary plane

[Daniel Vetter]

On Wed, Aug 19, 2015 at 06:02:29PM -0700, Chandra Konduru wrote:
> This patch adds NV12 to list of supported formats for
> primary plane.
> 
> v2:
> -Rebased (me)
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> Testcase: igt/kms_nv12

I think it's time to unify the separate primary/cursor code we have for
skl and just use the sprite one everywhere. Doesn't need to happen
necessarily before this patch series, but really needs to happen rather
sooner than later. We've already broken some of the scaler tests because
we had to hack away the 3rd plane because we couldn't remove the cursor
stuff due to some oddball (and undebugged) reason. Iirc Damien has some
patches somewhere for this.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c |   22 ++++++++++++++++++++--
>  1 file changed, 20 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b1d9edf..e4a6a91 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -74,6 +74,19 @@ static const uint32_t skl_primary_formats[] = {
>  	DRM_FORMAT_XBGR2101010,
>  };
>  
> +/* Primary plane formats for gen >= 9 with NV12 */
> +static const uint32_t skl_primary_formats_with_nv12[] = {
> +	DRM_FORMAT_C8,
> +	DRM_FORMAT_RGB565,
> +	DRM_FORMAT_XRGB8888,
> +	DRM_FORMAT_XBGR8888,
> +	DRM_FORMAT_ARGB8888,
> +	DRM_FORMAT_ABGR8888,
> +	DRM_FORMAT_XRGB2101010,
> +	DRM_FORMAT_XBGR2101010,
> +	DRM_FORMAT_NV12,
> +};
> +
>  /* Cursor formats */
>  static const uint32_t intel_cursor_formats[] = {
>  	DRM_FORMAT_ARGB8888,
> @@ -13606,8 +13619,13 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
>  		primary->plane = !pipe;
>  
>  	if (INTEL_INFO(dev)->gen >= 9) {
> -		intel_primary_formats = skl_primary_formats;
> -		num_formats = ARRAY_SIZE(skl_primary_formats);
> +		if (pipe == PIPE_A || pipe == PIPE_B) {
> +			intel_primary_formats = skl_primary_formats_with_nv12;
> +			num_formats = ARRAY_SIZE(skl_primary_formats_with_nv12);
> +		} else {
> +			intel_primary_formats = skl_primary_formats;
> +			num_formats = ARRAY_SIZE(skl_primary_formats);
> +		}
>  	} else if (INTEL_INFO(dev)->gen >= 4) {
>  		intel_primary_formats = i965_primary_formats;
>  		num_formats = ARRAY_SIZE(i965_primary_formats);
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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 08/15] drm/i915: Add NV12 as supported format for primary plane

[Konduru, Chandra]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Wednesday, August 26, 2015 1:40 AM
> To: Konduru, Chandra
> Cc: intel-gfx@lists.freedesktop.org; Vetter, Daniel; Syrjala, Ville
> Subject: Re: [Intel-gfx] [PATCH 08/15] drm/i915: Add NV12 as supported format
> for primary plane
> 
> On Wed, Aug 19, 2015 at 06:02:29PM -0700, Chandra Konduru wrote:
> > This patch adds NV12 to list of supported formats for
> > primary plane.
> >
> > v2:
> > -Rebased (me)
> >
> > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > Testcase: igt/kms_nv12
> 
> I think it's time to unify the separate primary/cursor code we have for
> skl and just use the sprite one everywhere. Doesn't need to happen
> necessarily before this patch series, but really needs to happen rather
> sooner than later. We've already broken some of the scaler tests because
> we had to hack away the 3rd plane because we couldn't remove the cursor
> stuff due to some oddball (and undebugged) reason. Iirc Damien has some
> patches somewhere for this.
> -Daniel
> 
I didn't planned before this patch series, but fully agree with 
your assessment to unify.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 09/15] drm/i915: Add NV12 as supported format for sprite plane

[Chandra Konduru]

This patch adds NV12 to list of supported formats for
sprite plane.

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
Testcase: igt/kms_nv12
---
 drivers/gpu/drm/i915/intel_sprite.c |   23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 66d60ae..4e8c020 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1041,6 +1041,19 @@ static uint32_t skl_plane_formats[] = {
 	DRM_FORMAT_VYUY,
 };
 
+static uint32_t skl_plane_formats_with_nv12[] = {
+	DRM_FORMAT_RGB565,
+	DRM_FORMAT_ABGR8888,
+	DRM_FORMAT_ARGB8888,
+	DRM_FORMAT_XBGR8888,
+	DRM_FORMAT_XRGB8888,
+	DRM_FORMAT_YUYV,
+	DRM_FORMAT_YVYU,
+	DRM_FORMAT_UYVY,
+	DRM_FORMAT_VYUY,
+	DRM_FORMAT_NV12,
+};
+
 int
 intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 {
@@ -1112,8 +1125,14 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
 		intel_plane->disable_plane = skl_disable_plane;
 		state->scaler_id = -1;
 
-		plane_formats = skl_plane_formats;
-		num_plane_formats = ARRAY_SIZE(skl_plane_formats);
+		if ((pipe == PIPE_A || pipe == PIPE_B) && (plane == 0)) {
+			plane_formats = skl_plane_formats_with_nv12;
+			num_plane_formats = ARRAY_SIZE(skl_plane_formats_with_nv12);
+		} else {
+			plane_formats = skl_plane_formats;
+			num_plane_formats = ARRAY_SIZE(skl_plane_formats) - 1;
+		}
+
 		break;
 	default:
 		kfree(intel_plane);
-- 
1.7.9.5

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

Re: [PATCH 09/15] drm/i915: Add NV12 as supported format for sprite plane

[Ville Syrjälä]

On Wed, Aug 19, 2015 at 06:02:30PM -0700, Chandra Konduru wrote:
> This patch adds NV12 to list of supported formats for
> sprite plane.
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> Testcase: igt/kms_nv12
> ---
>  drivers/gpu/drm/i915/intel_sprite.c |   23 +++++++++++++++++++++--
>  1 file changed, 21 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 66d60ae..4e8c020 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -1041,6 +1041,19 @@ static uint32_t skl_plane_formats[] = {
>  	DRM_FORMAT_VYUY,
>  };
>  
> +static uint32_t skl_plane_formats_with_nv12[] = {

static const ...

> +	DRM_FORMAT_RGB565,
> +	DRM_FORMAT_ABGR8888,
> +	DRM_FORMAT_ARGB8888,
> +	DRM_FORMAT_XBGR8888,
> +	DRM_FORMAT_XRGB8888,
> +	DRM_FORMAT_YUYV,
> +	DRM_FORMAT_YVYU,
> +	DRM_FORMAT_UYVY,
> +	DRM_FORMAT_VYUY,
> +	DRM_FORMAT_NV12,
> +};
> +
>  int
>  intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>  {
> @@ -1112,8 +1125,14 @@ intel_plane_init(struct drm_device *dev, enum pipe pipe, int plane)
>  		intel_plane->disable_plane = skl_disable_plane;
>  		state->scaler_id = -1;
>  
> -		plane_formats = skl_plane_formats;
> -		num_plane_formats = ARRAY_SIZE(skl_plane_formats);
> +		if ((pipe == PIPE_A || pipe == PIPE_B) && (plane == 0)) {
> +			plane_formats = skl_plane_formats_with_nv12;
> +			num_plane_formats = ARRAY_SIZE(skl_plane_formats_with_nv12);
> +		} else {
> +			plane_formats = skl_plane_formats;
> +			num_plane_formats = ARRAY_SIZE(skl_plane_formats) - 1;

Stray '-1'

> +		}

Only the first two planes have NV12. So primary and sprite 0 for us.
And none on pipe C. Yep, matches the spec. 

With the const and -1 fixed this is
Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> +
>  		break;
>  	default:
>  		kfree(intel_plane);
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 10/15] drm/i915: Add NV12 support to intel_framebuffer_init

[Chandra Konduru]

This patch adds NV12 as supported format to
intel_framebuffer_init and performs various checks.

v2:
-Fix an issue in checks added (me)

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
Testcase: igt/kms_nv12
---
 drivers/gpu/drm/i915/intel_display.c |   28 ++++++++++++++++++++++++++++
 1 file changed, 28 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e4a6a91..4df4d77 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14343,6 +14343,34 @@ static int intel_framebuffer_init(struct drm_device *dev,
 			return -EINVAL;
 		}
 		break;
+	case DRM_FORMAT_NV12:
+		if (INTEL_INFO(dev)->gen < 9) {
+			DRM_DEBUG("unsupported pixel format: %s\n",
+				  drm_get_format_name(mode_cmd->pixel_format));
+			return -EINVAL;
+		}
+		if (!mode_cmd->offsets[1]) {
+			DRM_DEBUG("uv start offset not set\n");
+			return -EINVAL;
+		}
+		if (mode_cmd->pitches[0] != mode_cmd->pitches[1] ||
+			mode_cmd->handles[0] != mode_cmd->handles[1]) {
+			DRM_DEBUG("y and uv subplanes have different parameters\n");
+			return -EINVAL;
+		}
+		if (mode_cmd->modifier[1] == I915_FORMAT_MOD_Yf_TILED &&
+			(mode_cmd->offsets[1] & 0xFFF)) {
+			DRM_DEBUG("tile-Yf uv offset 0x%x isn't starting on new tile-row\n",
+				mode_cmd->offsets[1]);
+			return -EINVAL;
+		}
+		if (mode_cmd->modifier[1] == I915_FORMAT_MOD_Y_TILED &&
+			((mode_cmd->offsets[1] / mode_cmd->pitches[1]) % 4)) {
+			DRM_DEBUG("tile-Y uv offset 0x%x isn't 4-line aligned\n",
+				mode_cmd->offsets[1]);
+			return -EINVAL;
+		}
+		break;
 	default:
 		DRM_DEBUG("unsupported pixel format: %s\n",
 			  drm_get_format_name(mode_cmd->pixel_format));
-- 
1.7.9.5

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

Re: [PATCH 10/15] drm/i915: Add NV12 support to intel_framebuffer_init

[Ville Syrjälä]

On Wed, Aug 19, 2015 at 06:02:31PM -0700, Chandra Konduru wrote:
> This patch adds NV12 as supported format to
> intel_framebuffer_init and performs various checks.
> 
> v2:
> -Fix an issue in checks added (me)
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> Testcase: igt/kms_nv12
> ---
>  drivers/gpu/drm/i915/intel_display.c |   28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e4a6a91..4df4d77 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14343,6 +14343,34 @@ static int intel_framebuffer_init(struct drm_device *dev,
>  			return -EINVAL;
>  		}
>  		break;
> +	case DRM_FORMAT_NV12:
> +		if (INTEL_INFO(dev)->gen < 9) {
> +			DRM_DEBUG("unsupported pixel format: %s\n",
> +				  drm_get_format_name(mode_cmd->pixel_format));
> +			return -EINVAL;
> +		}
> +		if (!mode_cmd->offsets[1]) {
> +			DRM_DEBUG("uv start offset not set\n");
> +			return -EINVAL;
> +		}

I'm not sure this check makes much sense. We should perhaps either reject
all cases where the planes overlap, or allow it all.

Some years ago I was thinking of adding overlap check to drm core, but
then I figured maybe someone wants to interleave the planes in memory,
so the overlap checks would then have to check each line for overlap
so it gets a bit nasty. So I'm not sure there's any point in disallowing
overlapping planes.

> +		if (mode_cmd->pitches[0] != mode_cmd->pitches[1] ||
> +			mode_cmd->handles[0] != mode_cmd->handles[1]) {
> +			DRM_DEBUG("y and uv subplanes have different parameters\n");
> +			return -EINVAL;
> +		}

Maybe we'd want a separate debug message for these two. I think the
shared stride limitation would be fairly common in other hardware, but
the fact that we require the bo to be the same is unfortunately quite
a special "feature" of our latest hardware. So having a clear debug
message for it might help people figure out why their seemingly valid
code doesn't work on i915.

> +		if (mode_cmd->modifier[1] == I915_FORMAT_MOD_Yf_TILED &&
> +			(mode_cmd->offsets[1] & 0xFFF)) {

Bunch of indentation problems in this patch as well.

> +			DRM_DEBUG("tile-Yf uv offset 0x%x isn't starting on new tile-row\n",
> +				mode_cmd->offsets[1]);
> +			return -EINVAL;
> +		}
> +		if (mode_cmd->modifier[1] == I915_FORMAT_MOD_Y_TILED &&
> +			((mode_cmd->offsets[1] / mode_cmd->pitches[1]) % 4)) {
> +			DRM_DEBUG("tile-Y uv offset 0x%x isn't 4-line aligned\n",
> +				mode_cmd->offsets[1]);
> +			return -EINVAL;
> +		}
> +		break;
>  	default:
>  		DRM_DEBUG("unsupported pixel format: %s\n",
>  			  drm_get_format_name(mode_cmd->pixel_format));
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 11/15] drm/i915: Add NV12 to primary plane programming.

[Chandra Konduru]

This patch is adding NV12 support to skylake primary plane
programming. It is covering linear/X/Y/Yf tiling formats
for 0 and 180 rotations.

For 90/270 rotation, Y and UV subplanes should be treated
as separate surfaces and GTT remapping for rotation should
be done separately for each subplane. Once GEM adds support
for seperate remappings for two subplanes, 90/270 support
to be added to plane programming.

v2:
-Use regular int instead of 16.16 in aux_offset calculations (me)

v3:
-Allow 90/270 for NV12 as its remapping is now supported (me)

v4:
-Rebased to current kernel version 4.2.0.rc4 (me)

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
Testcase: igt/kms_nv12
---
 drivers/gpu/drm/i915/intel_display.c |   37 ++++++++++++++++++++++++++++++++++
 1 file changed, 37 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 4df4d77..329651e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3026,6 +3026,8 @@ u32 skl_plane_ctl_format(uint32_t pixel_format)
 		return PLANE_CTL_FORMAT_YUV422 | PLANE_CTL_YUV422_UYVY;
 	case DRM_FORMAT_VYUY:
 		return PLANE_CTL_FORMAT_YUV422 | PLANE_CTL_YUV422_VYUY;
+	case DRM_FORMAT_NV12:
+		return PLANE_CTL_FORMAT_NV12;
 	default:
 		MISSING_CASE(pixel_format);
 	}
@@ -3094,6 +3096,8 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
 	int src_x = 0, src_y = 0, src_w = 0, src_h = 0;
 	int dst_x = 0, dst_y = 0, dst_w = 0, dst_h = 0;
 	int scaler_id = -1;
+	u32 aux_dist = 0, aux_x_offset = 0, aux_y_offset = 0, aux_stride = 0;
+	u32 tile_row_adjustment = 0;
 
 	plane_state = to_intel_plane_state(plane->state);
 
@@ -3150,11 +3154,34 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
 		x_offset = stride * tile_height - y - src_h;
 		y_offset = x;
 		plane_size = (src_w - 1) << 16 | (src_h - 1);
+		/*
+		 * TBD: For NV12 90/270 rotation, Y and UV subplanes should
+		 * be treated as separate surfaces and GTT remapping for
+		 * rotation should be done separately for each subplane.
+		 * Enable support once seperate remappings are available.
+		 */
 	} else {
 		stride = fb->pitches[0] / stride_div;
 		x_offset = x;
 		y_offset = y;
 		plane_size = (src_h - 1) << 16 | (src_w - 1);
+		tile_height = PAGE_SIZE / stride_div;
+
+		if (fb->pixel_format == DRM_FORMAT_NV12) {
+			int height_in_mem = (fb->offsets[1]/fb->pitches[0]);
+			/*
+			 * If UV starts from middle of a page, then UV start should
+			 * be programmed to beginning of that page. And offset into that
+			 * page to be programmed into y-offset
+			 */
+			tile_row_adjustment = height_in_mem % tile_height;
+			aux_dist = fb->pitches[0] * (height_in_mem - tile_row_adjustment);
+			aux_x_offset = DIV_ROUND_UP(x, 2);
+			aux_y_offset = DIV_ROUND_UP(y, 2) + tile_row_adjustment;
+			/* For tile-Yf, uv-subplane tile width is 2x of Y-subplane */
+			aux_stride = fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED ?
+				stride / 2 : stride;
+		}
 	}
 	plane_offset = y_offset << 16 | x_offset;
 
@@ -3162,11 +3189,14 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
 	I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset);
 	I915_WRITE(PLANE_SIZE(pipe, 0), plane_size);
 	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
+	I915_WRITE(PLANE_AUX_DIST(pipe, 0), aux_dist | aux_stride);
+	I915_WRITE(PLANE_AUX_OFFSET(pipe, 0), aux_y_offset << 16 | aux_x_offset);
 
 	if (scaler_id >= 0) {
 		uint32_t ps_ctrl = 0;
 
 		WARN_ON(!dst_w || !dst_h);
+
 		ps_ctrl = PS_SCALER_EN | PS_PLANE_SEL(0) |
 			crtc_state->scaler_state.scalers[scaler_id].mode;
 		I915_WRITE(SKL_PS_CTRL(pipe, scaler_id), ps_ctrl);
@@ -3175,6 +3205,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
 		I915_WRITE(SKL_PS_WIN_SZ(pipe, scaler_id), (dst_w << 16) | dst_h);
 		I915_WRITE(PLANE_POS(pipe, 0), 0);
 	} else {
+		WARN_ON(fb->pixel_format == DRM_FORMAT_NV12);
 		I915_WRITE(PLANE_POS(pipe, 0), (dst_y << 16) | dst_x);
 	}
 
@@ -11626,6 +11657,12 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 	bool turn_off, turn_on, visible, was_visible;
 	struct drm_framebuffer *fb = plane_state->fb;
 
+	/* Adjust (macro)pixel boundary */
+	if (fb && intel_format_is_yuv(fb->pixel_format)) {
+		to_intel_plane_state(plane_state)->src.x1 &= ~0x10000;
+		to_intel_plane_state(plane_state)->src.x2 &= ~0x10000;
+	}
+
 	if (crtc_state && INTEL_INFO(dev)->gen >= 9 &&
 	    plane->type != DRM_PLANE_TYPE_CURSOR) {
 		ret = skl_update_scaler_plane(
-- 
1.7.9.5

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

Re: [PATCH 11/15] drm/i915: Add NV12 to primary plane programming.

[Ville Syrjälä]

On Wed, Aug 19, 2015 at 06:02:32PM -0700, Chandra Konduru wrote:
> This patch is adding NV12 support to skylake primary plane
> programming. It is covering linear/X/Y/Yf tiling formats
> for 0 and 180 rotations.
> 
> For 90/270 rotation, Y and UV subplanes should be treated
> as separate surfaces and GTT remapping for rotation should
> be done separately for each subplane. Once GEM adds support
> for seperate remappings for two subplanes, 90/270 support
> to be added to plane programming.
> 
> v2:
> -Use regular int instead of 16.16 in aux_offset calculations (me)
> 
> v3:
> -Allow 90/270 for NV12 as its remapping is now supported (me)
> 
> v4:
> -Rebased to current kernel version 4.2.0.rc4 (me)
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> Testcase: igt/kms_nv12
> ---
>  drivers/gpu/drm/i915/intel_display.c |   37 ++++++++++++++++++++++++++++++++++
>  1 file changed, 37 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 4df4d77..329651e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3026,6 +3026,8 @@ u32 skl_plane_ctl_format(uint32_t pixel_format)
>  		return PLANE_CTL_FORMAT_YUV422 | PLANE_CTL_YUV422_UYVY;
>  	case DRM_FORMAT_VYUY:
>  		return PLANE_CTL_FORMAT_YUV422 | PLANE_CTL_YUV422_VYUY;
> +	case DRM_FORMAT_NV12:
> +		return PLANE_CTL_FORMAT_NV12;
>  	default:
>  		MISSING_CASE(pixel_format);
>  	}
> @@ -3094,6 +3096,8 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  	int src_x = 0, src_y = 0, src_w = 0, src_h = 0;
>  	int dst_x = 0, dst_y = 0, dst_w = 0, dst_h = 0;
>  	int scaler_id = -1;
> +	u32 aux_dist = 0, aux_x_offset = 0, aux_y_offset = 0, aux_stride = 0;
> +	u32 tile_row_adjustment = 0;
>  
>  	plane_state = to_intel_plane_state(plane->state);
>  
> @@ -3150,11 +3154,34 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  		x_offset = stride * tile_height - y - src_h;
>  		y_offset = x;
>  		plane_size = (src_w - 1) << 16 | (src_h - 1);
> +		/*
> +		 * TBD: For NV12 90/270 rotation, Y and UV subplanes should
> +		 * be treated as separate surfaces and GTT remapping for
> +		 * rotation should be done separately for each subplane.
> +		 * Enable support once seperate remappings are available.
> +		 */
>  	} else {
>  		stride = fb->pitches[0] / stride_div;
>  		x_offset = x;
>  		y_offset = y;
>  		plane_size = (src_h - 1) << 16 | (src_w - 1);
> +		tile_height = PAGE_SIZE / stride_div;
> +
> +		if (fb->pixel_format == DRM_FORMAT_NV12) {
> +			int height_in_mem = (fb->offsets[1]/fb->pitches[0]);
> +			/*
> +			 * If UV starts from middle of a page, then UV start should
> +			 * be programmed to beginning of that page. And offset into that
> +			 * page to be programmed into y-offset
> +			 */
> +			tile_row_adjustment = height_in_mem % tile_height;
> +			aux_dist = fb->pitches[0] * (height_in_mem - tile_row_adjustment);
> +			aux_x_offset = DIV_ROUND_UP(x, 2);
> +			aux_y_offset = DIV_ROUND_UP(y, 2) + tile_row_adjustment;
> +			/* For tile-Yf, uv-subplane tile width is 2x of Y-subplane */
> +			aux_stride = fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED ?
> +				stride / 2 : stride;

The 2x part was rather well hidden in the spec. How do we deal with
cases when the Y stride is an odd number of tiles?

> +		}
>  	}
>  	plane_offset = y_offset << 16 | x_offset;
>  
> @@ -3162,11 +3189,14 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  	I915_WRITE(PLANE_OFFSET(pipe, 0), plane_offset);
>  	I915_WRITE(PLANE_SIZE(pipe, 0), plane_size);
>  	I915_WRITE(PLANE_STRIDE(pipe, 0), stride);
> +	I915_WRITE(PLANE_AUX_DIST(pipe, 0), aux_dist | aux_stride);
> +	I915_WRITE(PLANE_AUX_OFFSET(pipe, 0), aux_y_offset << 16 | aux_x_offset);
>  
>  	if (scaler_id >= 0) {
>  		uint32_t ps_ctrl = 0;
>  
>  		WARN_ON(!dst_w || !dst_h);
> +
>  		ps_ctrl = PS_SCALER_EN | PS_PLANE_SEL(0) |
>  			crtc_state->scaler_state.scalers[scaler_id].mode;
>  		I915_WRITE(SKL_PS_CTRL(pipe, scaler_id), ps_ctrl);
> @@ -3175,6 +3205,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  		I915_WRITE(SKL_PS_WIN_SZ(pipe, scaler_id), (dst_w << 16) | dst_h);
>  		I915_WRITE(PLANE_POS(pipe, 0), 0);
>  	} else {
> +		WARN_ON(fb->pixel_format == DRM_FORMAT_NV12);
>  		I915_WRITE(PLANE_POS(pipe, 0), (dst_y << 16) | dst_x);
>  	}
>  
> @@ -11626,6 +11657,12 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  	bool turn_off, turn_on, visible, was_visible;
>  	struct drm_framebuffer *fb = plane_state->fb;
>  
> +	/* Adjust (macro)pixel boundary */
> +	if (fb && intel_format_is_yuv(fb->pixel_format)) {
> +		to_intel_plane_state(plane_state)->src.x1 &= ~0x10000;
> +		to_intel_plane_state(plane_state)->src.x2 &= ~0x10000;
> +	}

That can increase the size of the source rectangle, so it needs to be done
in a way that avois that. And we already do this for sprites.

Really, someone should just finally fix the mess we have made with handling
different types of planes in totally different ways and unify things as
much as possible.

> +
>  	if (crtc_state && INTEL_INFO(dev)->gen >= 9 &&
>  	    plane->type != DRM_PLANE_TYPE_CURSOR) {
>  		ret = skl_update_scaler_plane(
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/15] drm/i915: Add NV12 to primary plane programming.

[Daniel Vetter]

On Fri, Sep 04, 2015 at 02:09:41PM +0300, Ville Syrjälä wrote:
> Really, someone should just finally fix the mess we have made with handling
> different types of planes in totally different ways and unify things as
> much as possible.

Yeah I want universal planes for skl finally, and I'd like to have someon
on the hook and commited to do it before I pull in this series. We pushed
this task off for almost a year by now, and it's been endlessly pain.
-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 11/15] drm/i915: Add NV12 to primary plane programming.

[Konduru, Chandra]

> > +
> > +		if (fb->pixel_format == DRM_FORMAT_NV12) {
> > +			int height_in_mem = (fb->offsets[1]/fb->pitches[0]);
> > +			/*
> > +			 * If UV starts from middle of a page, then UV start
> should
> > +			 * be programmed to beginning of that page. And offset
> into that
> > +			 * page to be programmed into y-offset
> > +			 */
> > +			tile_row_adjustment = height_in_mem % tile_height;
> > +			aux_dist = fb->pitches[0] * (height_in_mem -
> tile_row_adjustment);
> > +			aux_x_offset = DIV_ROUND_UP(x, 2);
> > +			aux_y_offset = DIV_ROUND_UP(y, 2) +
> tile_row_adjustment;
> > +			/* For tile-Yf, uv-subplane tile width is 2x of Y-subplane
> */
> > +			aux_stride = fb->modifier[0] ==
> I915_FORMAT_MOD_Yf_TILED ?
> > +				stride / 2 : stride;
> 
> The 2x part was rather well hidden in the spec. How do we deal with
> cases when the Y stride is an odd number of tiles?

It should be a round up division to take care of that scenario. 
Fixing this and resolving your other comments. 
Will be sending updated patches shortly.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/15] drm/i915: Add NV12 to primary plane programming.

[Ville Syrjälä]

On Sat, Sep 05, 2015 at 01:10:11AM +0000, Konduru, Chandra wrote:
> > > +
> > > +		if (fb->pixel_format == DRM_FORMAT_NV12) {
> > > +			int height_in_mem = (fb->offsets[1]/fb->pitches[0]);
> > > +			/*
> > > +			 * If UV starts from middle of a page, then UV start
> > should
> > > +			 * be programmed to beginning of that page. And offset
> > into that
> > > +			 * page to be programmed into y-offset
> > > +			 */
> > > +			tile_row_adjustment = height_in_mem % tile_height;
> > > +			aux_dist = fb->pitches[0] * (height_in_mem -
> > tile_row_adjustment);
> > > +			aux_x_offset = DIV_ROUND_UP(x, 2);
> > > +			aux_y_offset = DIV_ROUND_UP(y, 2) +
> > tile_row_adjustment;
> > > +			/* For tile-Yf, uv-subplane tile width is 2x of Y-subplane
> > */
> > > +			aux_stride = fb->modifier[0] ==
> > I915_FORMAT_MOD_Yf_TILED ?
> > > +				stride / 2 : stride;
> > 
> > The 2x part was rather well hidden in the spec. How do we deal with
> > cases when the Y stride is an odd number of tiles?
> 
> It should be a round up division to take care of that scenario. 

That would stil lresult in a corrupted picture I think. So I was
thinking that we should just refuse to create NCV12 framebuffers with a
poorly aligned stride.

> Fixing this and resolving your other comments. 
> Will be sending updated patches shortly.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/15] drm/i915: Add NV12 to primary plane programming.

[Konduru, Chandra]

> > > > +		if (fb->pixel_format == DRM_FORMAT_NV12) {
> > > > +			int height_in_mem = (fb->offsets[1]/fb->pitches[0]);
> > > > +			/*
> > > > +			 * If UV starts from middle of a page, then UV start
> > > should
> > > > +			 * be programmed to beginning of that page. And offset
> > > into that
> > > > +			 * page to be programmed into y-offset
> > > > +			 */
> > > > +			tile_row_adjustment = height_in_mem % tile_height;
> > > > +			aux_dist = fb->pitches[0] * (height_in_mem -
> > > tile_row_adjustment);
> > > > +			aux_x_offset = DIV_ROUND_UP(x, 2);
> > > > +			aux_y_offset = DIV_ROUND_UP(y, 2) +
> > > tile_row_adjustment;
> > > > +			/* For tile-Yf, uv-subplane tile width is 2x of Y-subplane
> > > */
> > > > +			aux_stride = fb->modifier[0] ==
> > > I915_FORMAT_MOD_Yf_TILED ?
> > > > +				stride / 2 : stride;
> > >
> > > The 2x part was rather well hidden in the spec. How do we deal with
> > > cases when the Y stride is an odd number of tiles?
> >
> > It should be a round up division to take care of that scenario.
> 
> That would stil lresult in a corrupted picture I think. So I was
> thinking that we should just refuse to create NCV12 framebuffers with a
> poorly aligned stride.
> 
I added a check in intel_framebuffer_init() which should catch them:
        if (mode_cmd->pitches[0] != mode_cmd->pitches[1]) {
            DRM_DEBUG("y and uv subplanes have different pitches\n");
            return -EINVAL;
        }


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

Re: [PATCH 11/15] drm/i915: Add NV12 to primary plane programming.

[Ville Syrjälä]

On Wed, Sep 09, 2015 at 02:30:58AM +0300, Konduru, Chandra wrote:
> > > > > +		if (fb->pixel_format == DRM_FORMAT_NV12) {
> > > > > +			int height_in_mem = (fb->offsets[1]/fb->pitches[0]);
> > > > > +			/*
> > > > > +			 * If UV starts from middle of a page, then UV start
> > > > should
> > > > > +			 * be programmed to beginning of that page. And offset
> > > > into that
> > > > > +			 * page to be programmed into y-offset
> > > > > +			 */
> > > > > +			tile_row_adjustment = height_in_mem % tile_height;
> > > > > +			aux_dist = fb->pitches[0] * (height_in_mem -
> > > > tile_row_adjustment);
> > > > > +			aux_x_offset = DIV_ROUND_UP(x, 2);
> > > > > +			aux_y_offset = DIV_ROUND_UP(y, 2) +
> > > > tile_row_adjustment;
> > > > > +			/* For tile-Yf, uv-subplane tile width is 2x of Y-subplane
> > > > */
> > > > > +			aux_stride = fb->modifier[0] ==
> > > > I915_FORMAT_MOD_Yf_TILED ?
> > > > > +				stride / 2 : stride;
> > > >
> > > > The 2x part was rather well hidden in the spec. How do we deal with
> > > > cases when the Y stride is an odd number of tiles?
> > >
> > > It should be a round up division to take care of that scenario.
> > 
> > That would stil lresult in a corrupted picture I think. So I was
> > thinking that we should just refuse to create NCV12 framebuffers with a
> > poorly aligned stride.
> > 
> I added a check in intel_framebuffer_init() which should catch them:
>         if (mode_cmd->pitches[0] != mode_cmd->pitches[1]) {
>             DRM_DEBUG("y and uv subplanes have different pitches\n");
>             return -EINVAL;
>         }

That won't catch the case I'm worried about. We would also need to make
sure pitches[1] is aligned to the UV tile width.

-- 
Ville Syrjälä
Intel OTC
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

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

Re: [PATCH 11/15] drm/i915: Add NV12 to primary plane programming.

[Konduru, Chandra]

> > > > > > +			/* For tile-Yf, uv-subplane tile width is 2x of Y-
> subplane
> > > > > */
> > > > > > +			aux_stride = fb->modifier[0] ==
> > > > > I915_FORMAT_MOD_Yf_TILED ?
> > > > > > +				stride / 2 : stride;
> > > > >
> > > > > The 2x part was rather well hidden in the spec. How do we deal with
> > > > > cases when the Y stride is an odd number of tiles?
> > > >
> > > > It should be a round up division to take care of that scenario.
> > >
> > > That would stil lresult in a corrupted picture I think. So I was
> > > thinking that we should just refuse to create NCV12 framebuffers with a
> > > poorly aligned stride.
> > >
> > I added a check in intel_framebuffer_init() which should catch them:
> >         if (mode_cmd->pitches[0] != mode_cmd->pitches[1]) {
> >             DRM_DEBUG("y and uv subplanes have different pitches\n");
> >             return -EINVAL;
> >         }
> 
> That won't catch the case I'm worried about. We would also need to make
> sure pitches[1] is aligned to the UV tile width.

If caller is following tile/row/pitch alignments properly for sub-planes of 
NV12 Yf buffer, above check will catch. 
But are you referring a case where userland isn't following tile/row size 
alignments properly? In that case, above may not catch. But
isn't that is the case even with other FB formats if user land not
following tile/row/pitch alignments?

> 
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/15] drm/i915: Add NV12 to primary plane programming.

[Ville Syrjälä]

On Wed, Sep 09, 2015 at 05:12:06PM +0000, Konduru, Chandra wrote:
> > > > > > > +			/* For tile-Yf, uv-subplane tile width is 2x of Y-
> > subplane
> > > > > > */
> > > > > > > +			aux_stride = fb->modifier[0] ==
> > > > > > I915_FORMAT_MOD_Yf_TILED ?
> > > > > > > +				stride / 2 : stride;
> > > > > >
> > > > > > The 2x part was rather well hidden in the spec. How do we deal with
> > > > > > cases when the Y stride is an odd number of tiles?
> > > > >
> > > > > It should be a round up division to take care of that scenario.
> > > >
> > > > That would stil lresult in a corrupted picture I think. So I was
> > > > thinking that we should just refuse to create NCV12 framebuffers with a
> > > > poorly aligned stride.
> > > >
> > > I added a check in intel_framebuffer_init() which should catch them:
> > >         if (mode_cmd->pitches[0] != mode_cmd->pitches[1]) {
> > >             DRM_DEBUG("y and uv subplanes have different pitches\n");
> > >             return -EINVAL;
> > >         }
> > 
> > That won't catch the case I'm worried about. We would also need to make
> > sure pitches[1] is aligned to the UV tile width.
> 
> If caller is following tile/row/pitch alignments properly for sub-planes of 
> NV12 Yf buffer, above check will catch. 
> But are you referring a case where userland isn't following tile/row size 
> alignments properly? In that case, above may not catch. But
> isn't that is the case even with other FB formats if user land not
> following tile/row/pitch alignments?

We reject any attempt to create a framebuffer with a poorly aligned
stride.

Hmm. Actually I suppose we should just handle it in 
intel_fb_stride_alignment(). Eg.:

case Yf:
	if (cpp != 1 || pixel_format == DRM_FORMAT_NV12)
		return 128;
	else
		return 64;

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/15] drm/i915: Add NV12 to primary plane programming.

[Konduru, Chandra]

> -----Original Message-----
> From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> Sent: Wednesday, September 09, 2015 11:06 AM
> To: Konduru, Chandra
> Cc: Syrjala, Ville; intel-gfx@lists.freedesktop.org; Vetter, Daniel
> Subject: Re: [Intel-gfx] [PATCH 11/15] drm/i915: Add NV12 to primary plane
> programming.
> 
> On Wed, Sep 09, 2015 at 05:12:06PM +0000, Konduru, Chandra wrote:
> > > > > > > > +			/* For tile-Yf, uv-subplane tile width is 2x of Y-
> > > subplane
> > > > > > > */
> > > > > > > > +			aux_stride = fb->modifier[0] ==
> > > > > > > I915_FORMAT_MOD_Yf_TILED ?
> > > > > > > > +				stride / 2 : stride;
> > > > > > >
> > > > > > > The 2x part was rather well hidden in the spec. How do we deal with
> > > > > > > cases when the Y stride is an odd number of tiles?
> > > > > >
> > > > > > It should be a round up division to take care of that scenario.
> > > > >
> > > > > That would stil lresult in a corrupted picture I think. So I was
> > > > > thinking that we should just refuse to create NCV12 framebuffers with a
> > > > > poorly aligned stride.
> > > > >
> > > > I added a check in intel_framebuffer_init() which should catch them:
> > > >         if (mode_cmd->pitches[0] != mode_cmd->pitches[1]) {
> > > >             DRM_DEBUG("y and uv subplanes have different pitches\n");
> > > >             return -EINVAL;
> > > >         }
> > >
> > > That won't catch the case I'm worried about. We would also need to make
> > > sure pitches[1] is aligned to the UV tile width.
> >
> > If caller is following tile/row/pitch alignments properly for sub-planes of
> > NV12 Yf buffer, above check will catch.
> > But are you referring a case where userland isn't following tile/row size
> > alignments properly? In that case, above may not catch. But
> > isn't that is the case even with other FB formats if user land not
> > following tile/row/pitch alignments?
> 
> We reject any attempt to create a framebuffer with a poorly aligned
> stride.
> 
> Hmm. Actually I suppose we should just handle it in
> intel_fb_stride_alignment(). Eg.:
> 
> case Yf:
> 	if (cpp != 1 || pixel_format == DRM_FORMAT_NV12)
> 		return 128;
> 	else
> 		return 64;

This check is already there in intel_fb_stride_alignment()
which is called from intel_framebuffer_init(). But it always
does for 1st sub-plane which means does for Y.
It needs an update to pass a sub-plane (for UV) parameter, 
and made another call for UV-plane alignment check.
Will this be ok?

> 
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/15] drm/i915: Add NV12 to primary plane programming.

[Ville Syrjälä]

On Wed, Sep 09, 2015 at 08:10:30PM +0000, Konduru, Chandra wrote:
> 
> 
> > -----Original Message-----
> > From: Ville Syrjälä [mailto:ville.syrjala@linux.intel.com]
> > Sent: Wednesday, September 09, 2015 11:06 AM
> > To: Konduru, Chandra
> > Cc: Syrjala, Ville; intel-gfx@lists.freedesktop.org; Vetter, Daniel
> > Subject: Re: [Intel-gfx] [PATCH 11/15] drm/i915: Add NV12 to primary plane
> > programming.
> > 
> > On Wed, Sep 09, 2015 at 05:12:06PM +0000, Konduru, Chandra wrote:
> > > > > > > > > +			/* For tile-Yf, uv-subplane tile width is 2x of Y-
> > > > subplane
> > > > > > > > */
> > > > > > > > > +			aux_stride = fb->modifier[0] ==
> > > > > > > > I915_FORMAT_MOD_Yf_TILED ?
> > > > > > > > > +				stride / 2 : stride;
> > > > > > > >
> > > > > > > > The 2x part was rather well hidden in the spec. How do we deal with
> > > > > > > > cases when the Y stride is an odd number of tiles?
> > > > > > >
> > > > > > > It should be a round up division to take care of that scenario.
> > > > > >
> > > > > > That would stil lresult in a corrupted picture I think. So I was
> > > > > > thinking that we should just refuse to create NCV12 framebuffers with a
> > > > > > poorly aligned stride.
> > > > > >
> > > > > I added a check in intel_framebuffer_init() which should catch them:
> > > > >         if (mode_cmd->pitches[0] != mode_cmd->pitches[1]) {
> > > > >             DRM_DEBUG("y and uv subplanes have different pitches\n");
> > > > >             return -EINVAL;
> > > > >         }
> > > >
> > > > That won't catch the case I'm worried about. We would also need to make
> > > > sure pitches[1] is aligned to the UV tile width.
> > >
> > > If caller is following tile/row/pitch alignments properly for sub-planes of
> > > NV12 Yf buffer, above check will catch.
> > > But are you referring a case where userland isn't following tile/row size
> > > alignments properly? In that case, above may not catch. But
> > > isn't that is the case even with other FB formats if user land not
> > > following tile/row/pitch alignments?
> > 
> > We reject any attempt to create a framebuffer with a poorly aligned
> > stride.
> > 
> > Hmm. Actually I suppose we should just handle it in
> > intel_fb_stride_alignment(). Eg.:
> > 
> > case Yf:
> > 	if (cpp != 1 || pixel_format == DRM_FORMAT_NV12)
> > 		return 128;
> > 	else
> > 		return 64;
> 
> This check is already there in intel_fb_stride_alignment()
> which is called from intel_framebuffer_init(). But it always
> does for 1st sub-plane which means does for Y.
> It needs an update to pass a sub-plane (for UV) parameter, 
> and made another call for UV-plane alignment check.
> Will this be ok?

Yeah. You could in fact just have it loop over all the planes
and call it for each. Something like this perhaps:
for (i = 0; i < drm_format_num_planes(formt); i++) {
	intel_fb_stride_alignment(modifier[i], format, i);
	...
}

Or you could pass the cpp/bits_per_pixel instead of the plane index,
since that's the only thing for which you need the plane index within
the function.

I also started to wonder whether we should repeat most of the other
checks in intel_framebuffer_init() for each plane. But it's probably
just easier to check that handle, pitch and modifier matches for
both planes in the NV12 case. I think you actually missed the
modifier check. You just checked that modifier[1] is Yf, but that
leaves modifier[0] unchecked. I failed to notice it as well during
my review of the relevant patch.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/15] drm/i915: Add NV12 to primary plane programming.

[Konduru, Chandra]

> > > > > > > > > > +			/* For tile-Yf, uv-subplane tile width is
> 2x of Y-
> > > > > subplane
> > > > > > > > > */
> > > > > > > > > > +			aux_stride = fb->modifier[0] ==
> > > > > > > > > I915_FORMAT_MOD_Yf_TILED ?
> > > > > > > > > > +				stride / 2 : stride;
> > > > > > > > >
> > > > > > > > > The 2x part was rather well hidden in the spec. How do we deal
> with
> > > > > > > > > cases when the Y stride is an odd number of tiles?
> > > > > > > >
> > > > > > > > It should be a round up division to take care of that scenario.
> > > > > > >
> > > > > > > That would stil lresult in a corrupted picture I think. So I was
> > > > > > > thinking that we should just refuse to create NCV12 framebuffers
> with a
> > > > > > > poorly aligned stride.
> > > > > > >
> > > > > > I added a check in intel_framebuffer_init() which should catch them:
> > > > > >         if (mode_cmd->pitches[0] != mode_cmd->pitches[1]) {
> > > > > >             DRM_DEBUG("y and uv subplanes have different pitches\n");
> > > > > >             return -EINVAL;
> > > > > >         }
> > > > >
> > > > > That won't catch the case I'm worried about. We would also need to
> make
> > > > > sure pitches[1] is aligned to the UV tile width.
> > > >
> > > > If caller is following tile/row/pitch alignments properly for sub-planes of
> > > > NV12 Yf buffer, above check will catch.
> > > > But are you referring a case where userland isn't following tile/row size
> > > > alignments properly? In that case, above may not catch. But
> > > > isn't that is the case even with other FB formats if user land not
> > > > following tile/row/pitch alignments?
> > >
> > > We reject any attempt to create a framebuffer with a poorly aligned
> > > stride.
> > >
> > > Hmm. Actually I suppose we should just handle it in
> > > intel_fb_stride_alignment(). Eg.:
> > >
> > > case Yf:
> > > 	if (cpp != 1 || pixel_format == DRM_FORMAT_NV12)
> > > 		return 128;
> > > 	else
> > > 		return 64;
> >
> > This check is already there in intel_fb_stride_alignment()
> > which is called from intel_framebuffer_init(). But it always
> > does for 1st sub-plane which means does for Y.
> > It needs an update to pass a sub-plane (for UV) parameter,
> > and made another call for UV-plane alignment check.
> > Will this be ok?
> 
> Yeah. You could in fact just have it loop over all the planes
> and call it for each. Something like this perhaps:
> for (i = 0; i < drm_format_num_planes(formt); i++) {
> 	intel_fb_stride_alignment(modifier[i], format, i);
> 	...
> }
I planned the same, looping for all subplanes.

> 
> Or you could pass the cpp/bits_per_pixel instead of the plane index,
> since that's the only thing for which you need the plane index within
> the function.
> 
> I also started to wonder whether we should repeat most of the other
> checks in intel_framebuffer_init() for each plane. But it's probably
> just easier to check that handle, pitch and modifier matches for
> both planes in the NV12 case. I think you actually missed the
> modifier check. You just checked that modifier[1] is Yf, but that
> leaves modifier[0] unchecked. I failed to notice it as well during
> my review of the relevant patch.

Added modifier check.
Made these changes to " drm/i915: Add NV12 support to intel_framebuffer_init"
patch. Also sending out updated WA patch to move to init clockgating.
Rest of your feedback is already addressed. 
Before sending out these 2 patches, any other comments?

> 
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/15] drm/i915: Add NV12 to primary plane programming.

[Ville Syrjälä]

On Wed, Sep 09, 2015 at 09:09:27PM +0000, Konduru, Chandra wrote:
> 
> > > > > > > > > > > +			/* For tile-Yf, uv-subplane tile width is
> > 2x of Y-
> > > > > > subplane
> > > > > > > > > > */
> > > > > > > > > > > +			aux_stride = fb->modifier[0] ==
> > > > > > > > > > I915_FORMAT_MOD_Yf_TILED ?
> > > > > > > > > > > +				stride / 2 : stride;
> > > > > > > > > >
> > > > > > > > > > The 2x part was rather well hidden in the spec. How do we deal
> > with
> > > > > > > > > > cases when the Y stride is an odd number of tiles?
> > > > > > > > >
> > > > > > > > > It should be a round up division to take care of that scenario.
> > > > > > > >
> > > > > > > > That would stil lresult in a corrupted picture I think. So I was
> > > > > > > > thinking that we should just refuse to create NCV12 framebuffers
> > with a
> > > > > > > > poorly aligned stride.
> > > > > > > >
> > > > > > > I added a check in intel_framebuffer_init() which should catch them:
> > > > > > >         if (mode_cmd->pitches[0] != mode_cmd->pitches[1]) {
> > > > > > >             DRM_DEBUG("y and uv subplanes have different pitches\n");
> > > > > > >             return -EINVAL;
> > > > > > >         }
> > > > > >
> > > > > > That won't catch the case I'm worried about. We would also need to
> > make
> > > > > > sure pitches[1] is aligned to the UV tile width.
> > > > >
> > > > > If caller is following tile/row/pitch alignments properly for sub-planes of
> > > > > NV12 Yf buffer, above check will catch.
> > > > > But are you referring a case where userland isn't following tile/row size
> > > > > alignments properly? In that case, above may not catch. But
> > > > > isn't that is the case even with other FB formats if user land not
> > > > > following tile/row/pitch alignments?
> > > >
> > > > We reject any attempt to create a framebuffer with a poorly aligned
> > > > stride.
> > > >
> > > > Hmm. Actually I suppose we should just handle it in
> > > > intel_fb_stride_alignment(). Eg.:
> > > >
> > > > case Yf:
> > > > 	if (cpp != 1 || pixel_format == DRM_FORMAT_NV12)
> > > > 		return 128;
> > > > 	else
> > > > 		return 64;
> > >
> > > This check is already there in intel_fb_stride_alignment()
> > > which is called from intel_framebuffer_init(). But it always
> > > does for 1st sub-plane which means does for Y.
> > > It needs an update to pass a sub-plane (for UV) parameter,
> > > and made another call for UV-plane alignment check.
> > > Will this be ok?
> > 
> > Yeah. You could in fact just have it loop over all the planes
> > and call it for each. Something like this perhaps:
> > for (i = 0; i < drm_format_num_planes(formt); i++) {
> > 	intel_fb_stride_alignment(modifier[i], format, i);
> > 	...
> > }
> I planned the same, looping for all subplanes.
> 
> > 
> > Or you could pass the cpp/bits_per_pixel instead of the plane index,
> > since that's the only thing for which you need the plane index within
> > the function.
> > 
> > I also started to wonder whether we should repeat most of the other
> > checks in intel_framebuffer_init() for each plane. But it's probably
> > just easier to check that handle, pitch and modifier matches for
> > both planes in the NV12 case. I think you actually missed the
> > modifier check. You just checked that modifier[1] is Yf, but that
> > leaves modifier[0] unchecked. I failed to notice it as well during
> > my review of the relevant patch.
> 
> Added modifier check.
> Made these changes to " drm/i915: Add NV12 support to intel_framebuffer_init"
> patch. Also sending out updated WA patch to move to init clockgating.
> Rest of your feedback is already addressed. 
> Before sending out these 2 patches, any other comments?

Looking at what was said, I think we covered most open items
reasonably well, so fire away. I'll start going through the
updated patches tomorrow.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 11/15] drm/i915: Add NV12 to primary plane programming.

[Konduru, Chandra]

> Looking at what was said, I think we covered most open items
> reasonably well, so fire away. I'll start going through the
> updated patches tomorrow.
> 
Updates patches:
http://lists.freedesktop.org/archives/intel-gfx/2015-September/075235.html

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

[PATCH 12/15] drm/i915: Add NV12 to sprite plane programming.

[Chandra Konduru]

This patch is adding NV12 support to skylake sprite plane
programming. It is covering linear/X/Y/Yf tiling formats
for 0 and 180 rotations.

For 90/270 rotation, Y and UV subplanes should be treated
as separate surfaces and GTT remapping for rotation should
be done separately for each subplane. Once GEM adds support
for seperate remappings for two subplanes, 90/270 support
to be added to plane programming.

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
Testcase: igt/kms_nv12
---
 drivers/gpu/drm/i915/intel_sprite.c |   31 +++++++++++++++++++++++++++++--
 1 file changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 4e8c020..a1384a7 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -188,6 +188,8 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 	int x_offset, y_offset;
 	struct intel_crtc_state *crtc_state = to_intel_crtc(crtc)->config;
 	int scaler_id;
+	u32 aux_dist = 0, aux_x_offset = 0, aux_y_offset = 0, aux_stride = 0;
+	u32 tile_row_adjustment = 0;
 
 	plane_ctl = PLANE_CTL_ENABLE |
 		PLANE_CTL_PIPE_CSC_ENABLE;
@@ -234,24 +236,48 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 		plane_size = (src_w << 16) | src_h;
 		x_offset = stride * tile_height - y - (src_h + 1);
 		y_offset = x;
+
+		/*
+		 * TBD: For NV12 90/270 rotation, Y and UV subplanes should
+		 * be treated as separate surfaces and GTT remapping for
+		 * rotation should be done separately for each subplane.
+		 * Enable support once seperate remappings are available.
+		 */
 	} else {
 		stride = fb->pitches[0] / stride_div;
 		plane_size = (src_h << 16) | src_w;
 		x_offset = x;
 		y_offset = y;
+		tile_height = PAGE_SIZE / stride_div;
+
+		if (fb->pixel_format == DRM_FORMAT_NV12) {
+			int height_in_mem = (fb->offsets[1]/fb->pitches[0]);
+			/*
+			 * If UV starts from middle of a page, then UV start should
+			 * be programmed to beginning of that page. And offset into that
+			 * page to be programmed into y-offset
+			 */
+			tile_row_adjustment = height_in_mem % tile_height;
+			aux_dist = fb->pitches[0] * (height_in_mem - tile_row_adjustment);
+			aux_x_offset = DIV_ROUND_UP(x, 2);
+			aux_y_offset = DIV_ROUND_UP(y, 2) + tile_row_adjustment;
+			/* For tile-Yf, uv-subplane tile width is 2x of Y-subplane */
+			aux_stride = fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED ?
+				stride / 2 : stride;
+		}
 	}
 	plane_offset = y_offset << 16 | x_offset;
 
 	I915_WRITE(PLANE_OFFSET(pipe, plane), plane_offset);
 	I915_WRITE(PLANE_STRIDE(pipe, plane), stride);
 	I915_WRITE(PLANE_SIZE(pipe, plane), plane_size);
+	I915_WRITE(PLANE_AUX_DIST(pipe, plane), aux_dist | aux_stride);
+	I915_WRITE(PLANE_AUX_OFFSET(pipe, plane), aux_y_offset<<16 | aux_x_offset);
 
 	/* program plane scaler */
 	if (scaler_id >= 0) {
 		uint32_t ps_ctrl = 0;
 
-		DRM_DEBUG_KMS("plane = %d PS_PLANE_SEL(plane) = 0x%x\n", plane,
-			PS_PLANE_SEL(plane));
 		ps_ctrl = PS_SCALER_EN | PS_PLANE_SEL(plane) |
 			crtc_state->scaler_state.scalers[scaler_id].mode;
 		I915_WRITE(SKL_PS_CTRL(pipe, scaler_id), ps_ctrl);
@@ -262,6 +288,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 
 		I915_WRITE(PLANE_POS(pipe, plane), 0);
 	} else {
+		WARN_ON(fb->pixel_format == DRM_FORMAT_NV12);
 		I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x);
 	}
 
-- 
1.7.9.5

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

[PATCH 13/15] drm/i915: Set initial phase & trip for NV12 scaler

[Chandra Konduru]

This patch sets default initial phase and trip to scale NV12
content. In future, if needed these can be set via properties
or other means depending on incoming stream request. Until then
defaults are fine.

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    7 +++++++
 drivers/gpu/drm/i915/intel_sprite.c  |    7 +++++++
 2 files changed, 14 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 329651e..419660d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3098,6 +3098,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
 	int scaler_id = -1;
 	u32 aux_dist = 0, aux_x_offset = 0, aux_y_offset = 0, aux_stride = 0;
 	u32 tile_row_adjustment = 0;
+	u32 hphase = 0, vphase = 0;
 
 	plane_state = to_intel_plane_state(plane->state);
 
@@ -3181,6 +3182,9 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
 			/* For tile-Yf, uv-subplane tile width is 2x of Y-subplane */
 			aux_stride = fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED ?
 				stride / 2 : stride;
+
+			hphase = 0x00010001;  /* use trip for both Y and UV */
+			vphase = 0x00012000;  /* use trip for Y and phase 0.5 for UV */
 		}
 	}
 	plane_offset = y_offset << 16 | x_offset;
@@ -3209,6 +3213,9 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
 		I915_WRITE(PLANE_POS(pipe, 0), (dst_y << 16) | dst_x);
 	}
 
+	I915_WRITE(SKL_PS_HPHASE(pipe, scaler_id), hphase);
+	I915_WRITE(SKL_PS_VPHASE(pipe, scaler_id), vphase);
+
 	I915_WRITE(PLANE_SURF(pipe, 0), surf_addr);
 
 	POSTING_READ(PLANE_SURF(pipe, 0));
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index a1384a7..0ea9273 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -190,6 +190,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 	int scaler_id;
 	u32 aux_dist = 0, aux_x_offset = 0, aux_y_offset = 0, aux_stride = 0;
 	u32 tile_row_adjustment = 0;
+	u32 hphase = 0, vphase = 0;
 
 	plane_ctl = PLANE_CTL_ENABLE |
 		PLANE_CTL_PIPE_CSC_ENABLE;
@@ -264,6 +265,9 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 			/* For tile-Yf, uv-subplane tile width is 2x of Y-subplane */
 			aux_stride = fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED ?
 				stride / 2 : stride;
+
+			hphase = 0x00010001;  /* use trip for both Y and UV */
+			vphase = 0x00012000;  /* use trip for Y and phase 0.5 for UV */
 		}
 	}
 	plane_offset = y_offset << 16 | x_offset;
@@ -292,6 +296,9 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 		I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x);
 	}
 
+	I915_WRITE(SKL_PS_HPHASE(pipe, scaler_id), hphase);
+	I915_WRITE(SKL_PS_VPHASE(pipe, scaler_id), vphase);
+
 	I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl);
 	I915_WRITE(PLANE_SURF(pipe, plane), surf_addr);
 	POSTING_READ(PLANE_SURF(pipe, plane));
-- 
1.7.9.5

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

Re: [PATCH 13/15] drm/i915: Set initial phase & trip for NV12 scaler

[Ville Syrjälä]

On Wed, Aug 19, 2015 at 06:02:34PM -0700, Chandra Konduru wrote:
> This patch sets default initial phase and trip to scale NV12
> content. In future, if needed these can be set via properties
> or other means depending on incoming stream request. Until then
> defaults are fine.

We should set it according to the sub-pixel coordinates. At the moment
we just throw away the sub-pixel parts, but we should change that and do
it right(tm).

And yes some chroma siting properties would also be nice in the future.
I proposed something for that long ago, but that predates even plane
properties IIRC, so it was some ioctl based junk.

> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    7 +++++++
>  drivers/gpu/drm/i915/intel_sprite.c  |    7 +++++++
>  2 files changed, 14 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 329651e..419660d 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3098,6 +3098,7 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  	int scaler_id = -1;
>  	u32 aux_dist = 0, aux_x_offset = 0, aux_y_offset = 0, aux_stride = 0;
>  	u32 tile_row_adjustment = 0;
> +	u32 hphase = 0, vphase = 0;
>  
>  	plane_state = to_intel_plane_state(plane->state);
>  
> @@ -3181,6 +3182,9 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  			/* For tile-Yf, uv-subplane tile width is 2x of Y-subplane */
>  			aux_stride = fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED ?
>  				stride / 2 : stride;
> +
> +			hphase = 0x00010001;  /* use trip for both Y and UV */
> +			vphase = 0x00012000;  /* use trip for Y and phase 0.5 for UV */
>  		}
>  	}
>  	plane_offset = y_offset << 16 | x_offset;
> @@ -3209,6 +3213,9 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  		I915_WRITE(PLANE_POS(pipe, 0), (dst_y << 16) | dst_x);
>  	}
>  
> +	I915_WRITE(SKL_PS_HPHASE(pipe, scaler_id), hphase);
> +	I915_WRITE(SKL_PS_VPHASE(pipe, scaler_id), vphase);
> +
>  	I915_WRITE(PLANE_SURF(pipe, 0), surf_addr);
>  
>  	POSTING_READ(PLANE_SURF(pipe, 0));
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index a1384a7..0ea9273 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -190,6 +190,7 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  	int scaler_id;
>  	u32 aux_dist = 0, aux_x_offset = 0, aux_y_offset = 0, aux_stride = 0;
>  	u32 tile_row_adjustment = 0;
> +	u32 hphase = 0, vphase = 0;
>  
>  	plane_ctl = PLANE_CTL_ENABLE |
>  		PLANE_CTL_PIPE_CSC_ENABLE;
> @@ -264,6 +265,9 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  			/* For tile-Yf, uv-subplane tile width is 2x of Y-subplane */
>  			aux_stride = fb->modifier[0] == I915_FORMAT_MOD_Yf_TILED ?
>  				stride / 2 : stride;
> +
> +			hphase = 0x00010001;  /* use trip for both Y and UV */
> +			vphase = 0x00012000;  /* use trip for Y and phase 0.5 for UV */
>  		}
>  	}
>  	plane_offset = y_offset << 16 | x_offset;
> @@ -292,6 +296,9 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  		I915_WRITE(PLANE_POS(pipe, plane), (crtc_y << 16) | crtc_x);
>  	}
>  
> +	I915_WRITE(SKL_PS_HPHASE(pipe, scaler_id), hphase);
> +	I915_WRITE(SKL_PS_VPHASE(pipe, scaler_id), vphase);
> +
>  	I915_WRITE(PLANE_CTL(pipe, plane), plane_ctl);
>  	I915_WRITE(PLANE_SURF(pipe, plane), surf_addr);
>  	POSTING_READ(PLANE_SURF(pipe, plane));
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 14/15] drm/i915: skl nv12 workarounds

[Chandra Konduru]

Adding driver workarounds for nv12.

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      |   20 ++++++++++++++++++++
 drivers/gpu/drm/i915/intel_csr.c     |    2 +-
 drivers/gpu/drm/i915/intel_display.c |   31 +++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |    1 +
 drivers/gpu/drm/i915/intel_sprite.c  |    7 +++++++
 5 files changed, 60 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index c4d732f..3192837 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5354,6 +5354,26 @@ enum skl_disp_power_wells {
 #define PLANE_NV12_BUF_CFG(pipe, plane)	\
 	_PLANE(plane, _PLANE_NV12_BUF_CFG_1(pipe), _PLANE_NV12_BUF_CFG_2(pipe))
 
+/*
+ * Skylake Chicken registers
+ */
+#define _CHICKEN_PIPESL_A          0x420B0
+#define _CHICKEN_PIPESL_B          0x420B4
+#define _CHICKEN_PIPESL_C          0x420B8
+#define  DISABLE_STREAMER_FIX      (1 << 22)
+#define CHICKEN_PIPESL(pipe) _PIPE(pipe, _CHICKEN_PIPESL_A, _CHICKEN_PIPESL_B)
+
+#define CHICKEN_DCPR_1             0x46430
+#define IDLE_WAKEMEM_MASK          (1 << 13)
+
+#define CLKGATE_DIS_PSL_A        0x46520
+#define CLKGATE_DIS_PSL_B        0x46524
+#define CLKGATE_DIS_PSL_C        0x46528
+#define DUPS1_GATING_DIS         (1 << 15)
+#define DUPS2_GATING_DIS         (1 << 19)
+#define DUPS3_GATING_DIS         (1 << 23)
+#define CLKGATE_DIS_PSL(pipe)  _PIPE(pipe, CLKGATE_DIS_PSL_A, CLKGATE_DIS_PSL_B)
+
 /* SKL new cursor registers */
 #define _CUR_BUF_CFG_A				0x7017c
 #define _CUR_BUF_CFG_B				0x7117c
diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
index ba1ae03..559a7f5 100644
--- a/drivers/gpu/drm/i915/intel_csr.c
+++ b/drivers/gpu/drm/i915/intel_csr.c
@@ -181,7 +181,7 @@ static const struct stepping_info skl_stepping_info[] = {
 		{'G', '0'}, {'H', '0'}, {'I', '0'}
 };
 
-static char intel_get_stepping(struct drm_device *dev)
+char intel_get_stepping(struct drm_device *dev)
 {
 	if (IS_SKYLAKE(dev) && (dev->pdev->revision <
 			ARRAY_SIZE(skl_stepping_info)))
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 419660d..2158b8f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3196,6 +3196,16 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
 	I915_WRITE(PLANE_AUX_DIST(pipe, 0), aux_dist | aux_stride);
 	I915_WRITE(PLANE_AUX_OFFSET(pipe, 0), aux_y_offset << 16 | aux_x_offset);
 
+	DRM_DEBUG_KMS("KCM: is_skl = %d is_bxt = %d\n",
+		IS_SKYLAKE(dev), IS_BROXTON(dev));
+
+	if (((IS_SKYLAKE(dev) && intel_get_stepping(dev) == 'C') ||
+		(IS_BROXTON(dev) && intel_get_stepping(dev) == 'A')) &&
+		fb->pixel_format == DRM_FORMAT_NV12) {
+			I915_WRITE(CHICKEN_PIPESL(pipe),
+				I915_READ(CHICKEN_PIPESL(pipe)) | DISABLE_STREAMER_FIX);
+	}
+
 	if (scaler_id >= 0) {
 		uint32_t ps_ctrl = 0;
 
@@ -5004,6 +5014,21 @@ static bool hsw_crtc_supports_ips(struct intel_crtc *crtc)
 	return HAS_IPS(crtc->base.dev) && crtc->pipe == PIPE_A;
 }
 
+
+static void skl_wa_clkgate(struct drm_i915_private *dev_priv,
+	int pipe, int enable)
+{
+	if (pipe == PIPE_A || pipe == PIPE_B) {
+		if (enable)
+			I915_WRITE(CLKGATE_DIS_PSL(pipe),
+				DUPS1_GATING_DIS | DUPS2_GATING_DIS);
+		else
+			I915_WRITE(CLKGATE_DIS_PSL(pipe),
+				I915_READ(CLKGATE_DIS_PSL(pipe) &
+				~(DUPS1_GATING_DIS|DUPS2_GATING_DIS)));
+	}
+}
+
 static void haswell_crtc_enable(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -5094,6 +5119,9 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 		intel_wait_for_vblank(dev, hsw_workaround_pipe);
 		intel_wait_for_vblank(dev, hsw_workaround_pipe);
 	}
+
+	/* workaround for NV12 */
+	skl_wa_clkgate(dev_priv, pipe, 1);
 }
 
 static void ironlake_pfit_disable(struct intel_crtc *crtc)
@@ -5211,6 +5239,9 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 
 	intel_crtc->active = false;
 	intel_update_watermarks(crtc);
+
+	/* workaround for NV12 */
+	skl_wa_clkgate(dev_priv, intel_crtc->pipe, 0);
 }
 
 static void i9xx_pfit_enable(struct intel_crtc *crtc)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index d50b8cb..63750d5 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1158,6 +1158,7 @@ void intel_csr_load_status_set(struct drm_i915_private *dev_priv,
 					enum csr_state state);
 void intel_csr_load_program(struct drm_device *dev);
 void intel_csr_ucode_fini(struct drm_device *dev);
+char intel_get_stepping(struct drm_device *dev);
 void assert_csr_loaded(struct drm_i915_private *dev_priv);
 
 /* intel_dp.c */
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 0ea9273..9d1c5b9 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -278,6 +278,13 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 	I915_WRITE(PLANE_AUX_DIST(pipe, plane), aux_dist | aux_stride);
 	I915_WRITE(PLANE_AUX_OFFSET(pipe, plane), aux_y_offset<<16 | aux_x_offset);
 
+	if (((IS_SKYLAKE(dev) && intel_get_stepping(dev) == 'C') ||
+		(IS_BROXTON(dev) && intel_get_stepping(dev) == 'A')) &&
+		fb->pixel_format == DRM_FORMAT_NV12) {
+			I915_WRITE(CHICKEN_PIPESL(pipe),
+				I915_READ(CHICKEN_PIPESL(pipe)) | DISABLE_STREAMER_FIX);
+	}
+
 	/* program plane scaler */
 	if (scaler_id >= 0) {
 		uint32_t ps_ctrl = 0;
-- 
1.7.9.5

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

Re: [PATCH 14/15] drm/i915: skl nv12 workarounds

[Daniel Vetter]

On Wed, Aug 19, 2015 at 06:02:35PM -0700, Chandra Konduru wrote:
> Adding driver workarounds for nv12.
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |   20 ++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_csr.c     |    2 +-
>  drivers/gpu/drm/i915/intel_display.c |   31 +++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |    1 +
>  drivers/gpu/drm/i915/intel_sprite.c  |    7 +++++++
>  5 files changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index c4d732f..3192837 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5354,6 +5354,26 @@ enum skl_disp_power_wells {
>  #define PLANE_NV12_BUF_CFG(pipe, plane)	\
>  	_PLANE(plane, _PLANE_NV12_BUF_CFG_1(pipe), _PLANE_NV12_BUF_CFG_2(pipe))
>  
> +/*
> + * Skylake Chicken registers
> + */
> +#define _CHICKEN_PIPESL_A          0x420B0
> +#define _CHICKEN_PIPESL_B          0x420B4
> +#define _CHICKEN_PIPESL_C          0x420B8
> +#define  DISABLE_STREAMER_FIX      (1 << 22)
> +#define CHICKEN_PIPESL(pipe) _PIPE(pipe, _CHICKEN_PIPESL_A, _CHICKEN_PIPESL_B)
> +
> +#define CHICKEN_DCPR_1             0x46430
> +#define IDLE_WAKEMEM_MASK          (1 << 13)
> +
> +#define CLKGATE_DIS_PSL_A        0x46520
> +#define CLKGATE_DIS_PSL_B        0x46524
> +#define CLKGATE_DIS_PSL_C        0x46528
> +#define DUPS1_GATING_DIS         (1 << 15)
> +#define DUPS2_GATING_DIS         (1 << 19)
> +#define DUPS3_GATING_DIS         (1 << 23)
> +#define CLKGATE_DIS_PSL(pipe)  _PIPE(pipe, CLKGATE_DIS_PSL_A, CLKGATE_DIS_PSL_B)
> +
>  /* SKL new cursor registers */
>  #define _CUR_BUF_CFG_A				0x7017c
>  #define _CUR_BUF_CFG_B				0x7117c
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index ba1ae03..559a7f5 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -181,7 +181,7 @@ static const struct stepping_info skl_stepping_info[] = {
>  		{'G', '0'}, {'H', '0'}, {'I', '0'}
>  };
>  
> -static char intel_get_stepping(struct drm_device *dev)
> +char intel_get_stepping(struct drm_device *dev)

I guess we should have a new home for this now that it's used outside of
intel_csr.c Plus kerneldoc, as usual.

>  {
>  	if (IS_SKYLAKE(dev) && (dev->pdev->revision <
>  			ARRAY_SIZE(skl_stepping_info)))
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 419660d..2158b8f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3196,6 +3196,16 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  	I915_WRITE(PLANE_AUX_DIST(pipe, 0), aux_dist | aux_stride);
>  	I915_WRITE(PLANE_AUX_OFFSET(pipe, 0), aux_y_offset << 16 | aux_x_offset);
>  
> +	DRM_DEBUG_KMS("KCM: is_skl = %d is_bxt = %d\n",
> +		IS_SKYLAKE(dev), IS_BROXTON(dev));
> +

Wa comments are missing here. Also generally we do 1 wa per commit.
-Daniel

> +	if (((IS_SKYLAKE(dev) && intel_get_stepping(dev) == 'C') ||
> +		(IS_BROXTON(dev) && intel_get_stepping(dev) == 'A')) &&
> +		fb->pixel_format == DRM_FORMAT_NV12) {
> +			I915_WRITE(CHICKEN_PIPESL(pipe),
> +				I915_READ(CHICKEN_PIPESL(pipe)) | DISABLE_STREAMER_FIX);
> +	}
> +
>  	if (scaler_id >= 0) {
>  		uint32_t ps_ctrl = 0;
>  
> @@ -5004,6 +5014,21 @@ static bool hsw_crtc_supports_ips(struct intel_crtc *crtc)
>  	return HAS_IPS(crtc->base.dev) && crtc->pipe == PIPE_A;
>  }
>  
> +
> +static void skl_wa_clkgate(struct drm_i915_private *dev_priv,
> +	int pipe, int enable)
> +{
> +	if (pipe == PIPE_A || pipe == PIPE_B) {
> +		if (enable)
> +			I915_WRITE(CLKGATE_DIS_PSL(pipe),
> +				DUPS1_GATING_DIS | DUPS2_GATING_DIS);
> +		else
> +			I915_WRITE(CLKGATE_DIS_PSL(pipe),
> +				I915_READ(CLKGATE_DIS_PSL(pipe) &
> +				~(DUPS1_GATING_DIS|DUPS2_GATING_DIS)));
> +	}
> +}
> +
>  static void haswell_crtc_enable(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> @@ -5094,6 +5119,9 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  		intel_wait_for_vblank(dev, hsw_workaround_pipe);
>  		intel_wait_for_vblank(dev, hsw_workaround_pipe);
>  	}
> +
> +	/* workaround for NV12 */
> +	skl_wa_clkgate(dev_priv, pipe, 1);
>  }
>  
>  static void ironlake_pfit_disable(struct intel_crtc *crtc)
> @@ -5211,6 +5239,9 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  
>  	intel_crtc->active = false;
>  	intel_update_watermarks(crtc);
> +
> +	/* workaround for NV12 */
> +	skl_wa_clkgate(dev_priv, intel_crtc->pipe, 0);
>  }
>  
>  static void i9xx_pfit_enable(struct intel_crtc *crtc)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d50b8cb..63750d5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1158,6 +1158,7 @@ void intel_csr_load_status_set(struct drm_i915_private *dev_priv,
>  					enum csr_state state);
>  void intel_csr_load_program(struct drm_device *dev);
>  void intel_csr_ucode_fini(struct drm_device *dev);
> +char intel_get_stepping(struct drm_device *dev);
>  void assert_csr_loaded(struct drm_i915_private *dev_priv);
>  
>  /* intel_dp.c */
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 0ea9273..9d1c5b9 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -278,6 +278,13 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  	I915_WRITE(PLANE_AUX_DIST(pipe, plane), aux_dist | aux_stride);
>  	I915_WRITE(PLANE_AUX_OFFSET(pipe, plane), aux_y_offset<<16 | aux_x_offset);
>  
> +	if (((IS_SKYLAKE(dev) && intel_get_stepping(dev) == 'C') ||
> +		(IS_BROXTON(dev) && intel_get_stepping(dev) == 'A')) &&
> +		fb->pixel_format == DRM_FORMAT_NV12) {
> +			I915_WRITE(CHICKEN_PIPESL(pipe),
> +				I915_READ(CHICKEN_PIPESL(pipe)) | DISABLE_STREAMER_FIX);
> +	}
> +
>  	/* program plane scaler */
>  	if (scaler_id >= 0) {
>  		uint32_t ps_ctrl = 0;
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
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 14/15] drm/i915: skl nv12 workarounds

[Konduru, Chandra]

> > -static char intel_get_stepping(struct drm_device *dev)
> > +char intel_get_stepping(struct drm_device *dev)
> 
> I guess we should have a new home for this now that it's used outside of
> intel_csr.c Plus kerneldoc, as usual.

Will add kerneldoc header and respun, but where do you want to move to?

> 
> >  {
> >  	if (IS_SKYLAKE(dev) && (dev->pdev->revision <
> >  			ARRAY_SIZE(skl_stepping_info)))
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> > index 419660d..2158b8f 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -3196,6 +3196,16 @@ static void skylake_update_primary_plane(struct
> drm_crtc *crtc,
> >  	I915_WRITE(PLANE_AUX_DIST(pipe, 0), aux_dist | aux_stride);
> >  	I915_WRITE(PLANE_AUX_OFFSET(pipe, 0), aux_y_offset << 16 |
> aux_x_offset);
> >
> > +	DRM_DEBUG_KMS("KCM: is_skl = %d is_bxt = %d\n",
> > +		IS_SKYLAKE(dev), IS_BROXTON(dev));
> > +
> 
> Wa comments are missing here. Also generally we do 1 wa per commit.
> -Daniel

Sure, will split and respun.

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

Re: [PATCH 14/15] drm/i915: skl nv12 workarounds

[Daniel Vetter]

On Thu, Aug 27, 2015 at 01:44:06AM +0000, Konduru, Chandra wrote:
> > > -static char intel_get_stepping(struct drm_device *dev)
> > > +char intel_get_stepping(struct drm_device *dev)
> > 
> > I guess we should have a new home for this now that it's used outside of
> > intel_csr.c Plus kerneldoc, as usual.
> 
> Will add kerneldoc header and respun, but where do you want to move to?

If you want my dice-roll, I'd shovel it into intel_uncore.c for lack of
better home.
-Daniel

> 
> > 
> > >  {
> > >  	if (IS_SKYLAKE(dev) && (dev->pdev->revision <
> > >  			ARRAY_SIZE(skl_stepping_info)))
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > > index 419660d..2158b8f 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -3196,6 +3196,16 @@ static void skylake_update_primary_plane(struct
> > drm_crtc *crtc,
> > >  	I915_WRITE(PLANE_AUX_DIST(pipe, 0), aux_dist | aux_stride);
> > >  	I915_WRITE(PLANE_AUX_OFFSET(pipe, 0), aux_y_offset << 16 |
> > aux_x_offset);
> > >
> > > +	DRM_DEBUG_KMS("KCM: is_skl = %d is_bxt = %d\n",
> > > +		IS_SKYLAKE(dev), IS_BROXTON(dev));
> > > +
> > 
> > Wa comments are missing here. Also generally we do 1 wa per commit.
> > -Daniel
> 
> Sure, will split and respun.
> 

-- 
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 14/15] drm/i915: skl nv12 workarounds

[Konduru, Chandra]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Wednesday, September 02, 2015 1:02 AM
> To: Konduru, Chandra
> Cc: Daniel Vetter; intel-gfx@lists.freedesktop.org; Vetter, Daniel; Syrjala, Ville
> Subject: Re: [Intel-gfx] [PATCH 14/15] drm/i915: skl nv12 workarounds
> 
> On Thu, Aug 27, 2015 at 01:44:06AM +0000, Konduru, Chandra wrote:
> > > > -static char intel_get_stepping(struct drm_device *dev)
> > > > +char intel_get_stepping(struct drm_device *dev)
> > >
> > > I guess we should have a new home for this now that it's used outside of
> > > intel_csr.c Plus kerneldoc, as usual.
> >
> > Will add kerneldoc header and respun, but where do you want to move to?
> 
> If you want my dice-roll, I'd shovel it into intel_uncore.c for lack of
> better home.
> -Daniel
I sent updated patch series couple days ago moving to intel_display.c.
I hope that is fine. 
By the way, can you start giving RB tags for the patches you reviewed?

> 
> >
> > >
> > > >  {
> > > >  	if (IS_SKYLAKE(dev) && (dev->pdev->revision <
> > > >  			ARRAY_SIZE(skl_stepping_info)))
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > > index 419660d..2158b8f 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -3196,6 +3196,16 @@ static void
> skylake_update_primary_plane(struct
> > > drm_crtc *crtc,
> > > >  	I915_WRITE(PLANE_AUX_DIST(pipe, 0), aux_dist | aux_stride);
> > > >  	I915_WRITE(PLANE_AUX_OFFSET(pipe, 0), aux_y_offset << 16 |
> > > aux_x_offset);
> > > >
> > > > +	DRM_DEBUG_KMS("KCM: is_skl = %d is_bxt = %d\n",
> > > > +		IS_SKYLAKE(dev), IS_BROXTON(dev));
> > > > +
> > >
> > > Wa comments are missing here. Also generally we do 1 wa per commit.
> > > -Daniel
> >
> > Sure, will split and respun.
> >
> 
> --
> 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 14/15] drm/i915: skl nv12 workarounds

[Daniel Vetter]

On Thu, Sep 03, 2015 at 06:33:25PM +0000, Konduru, Chandra wrote:
> 
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> > Sent: Wednesday, September 02, 2015 1:02 AM
> > To: Konduru, Chandra
> > Cc: Daniel Vetter; intel-gfx@lists.freedesktop.org; Vetter, Daniel; Syrjala, Ville
> > Subject: Re: [Intel-gfx] [PATCH 14/15] drm/i915: skl nv12 workarounds
> > 
> > On Thu, Aug 27, 2015 at 01:44:06AM +0000, Konduru, Chandra wrote:
> > > > > -static char intel_get_stepping(struct drm_device *dev)
> > > > > +char intel_get_stepping(struct drm_device *dev)
> > > >
> > > > I guess we should have a new home for this now that it's used outside of
> > > > intel_csr.c Plus kerneldoc, as usual.
> > >
> > > Will add kerneldoc header and respun, but where do you want to move to?
> > 
> > If you want my dice-roll, I'd shovel it into intel_uncore.c for lack of
> > better home.
> > -Daniel
> I sent updated patch series couple days ago moving to intel_display.c.
> I hope that is fine. 

Imo intel_display.c is the wrong place since this isn't display-specific
really - stepping is for the entire device. intel_uncore.c, i915_drv.c or
i915_dma.c would all be suitable places.

> By the way, can you start giving RB tags for the patches you reviewed?

This isn't a full review, just a small thing I spotted while reading
patches.
-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 14/15] drm/i915: skl nv12 workarounds

[Konduru, Chandra]

> > > On Thu, Aug 27, 2015 at 01:44:06AM +0000, Konduru, Chandra wrote:
> > > > > > -static char intel_get_stepping(struct drm_device *dev)
> > > > > > +char intel_get_stepping(struct drm_device *dev)
> > > > >
> > > > > I guess we should have a new home for this now that it's used outside of
> > > > > intel_csr.c Plus kerneldoc, as usual.
> > > >
> > > > Will add kerneldoc header and respun, but where do you want to move to?
> > >
> > > If you want my dice-roll, I'd shovel it into intel_uncore.c for lack of
> > > better home.
> > > -Daniel
> > I sent updated patch series couple days ago moving to intel_display.c.
> > I hope that is fine.
> 
> Imo intel_display.c is the wrong place since this isn't display-specific
> really - stepping is for the entire device. intel_uncore.c, i915_drv.c or
> i915_dma.c would all be suitable places.

Moved to i915_drv.c. Sending out updated patches shortly.

> 
> > By the way, can you start giving RB tags for the patches you reviewed?
> 
> This isn't a full review, just a small thing I spotted while reading
> patches.
OK, Ville did/is doing full review. So covered.


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

Re: [PATCH 14/15] drm/i915: skl nv12 workarounds

[Ville Syrjälä]

On Wed, Aug 19, 2015 at 06:02:35PM -0700, Chandra Konduru wrote:
> Adding driver workarounds for nv12.
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_reg.h      |   20 ++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_csr.c     |    2 +-
>  drivers/gpu/drm/i915/intel_display.c |   31 +++++++++++++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |    1 +
>  drivers/gpu/drm/i915/intel_sprite.c  |    7 +++++++
>  5 files changed, 60 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index c4d732f..3192837 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -5354,6 +5354,26 @@ enum skl_disp_power_wells {
>  #define PLANE_NV12_BUF_CFG(pipe, plane)	\
>  	_PLANE(plane, _PLANE_NV12_BUF_CFG_1(pipe), _PLANE_NV12_BUF_CFG_2(pipe))
>  
> +/*
> + * Skylake Chicken registers
> + */
> +#define _CHICKEN_PIPESL_A          0x420B0
> +#define _CHICKEN_PIPESL_B          0x420B4
> +#define _CHICKEN_PIPESL_C          0x420B8
> +#define  DISABLE_STREAMER_FIX      (1 << 22)
> +#define CHICKEN_PIPESL(pipe) _PIPE(pipe, _CHICKEN_PIPESL_A, _CHICKEN_PIPESL_B)
> +
> +#define CHICKEN_DCPR_1             0x46430
> +#define IDLE_WAKEMEM_MASK          (1 << 13)
> +
> +#define CLKGATE_DIS_PSL_A        0x46520
> +#define CLKGATE_DIS_PSL_B        0x46524
> +#define CLKGATE_DIS_PSL_C        0x46528
> +#define DUPS1_GATING_DIS         (1 << 15)
> +#define DUPS2_GATING_DIS         (1 << 19)
> +#define DUPS3_GATING_DIS         (1 << 23)
> +#define CLKGATE_DIS_PSL(pipe)  _PIPE(pipe, CLKGATE_DIS_PSL_A, CLKGATE_DIS_PSL_B)
> +
>  /* SKL new cursor registers */
>  #define _CUR_BUF_CFG_A				0x7017c
>  #define _CUR_BUF_CFG_B				0x7117c
> diff --git a/drivers/gpu/drm/i915/intel_csr.c b/drivers/gpu/drm/i915/intel_csr.c
> index ba1ae03..559a7f5 100644
> --- a/drivers/gpu/drm/i915/intel_csr.c
> +++ b/drivers/gpu/drm/i915/intel_csr.c
> @@ -181,7 +181,7 @@ static const struct stepping_info skl_stepping_info[] = {
>  		{'G', '0'}, {'H', '0'}, {'I', '0'}
>  };
>  
> -static char intel_get_stepping(struct drm_device *dev)
> +char intel_get_stepping(struct drm_device *dev)
>  {
>  	if (IS_SKYLAKE(dev) && (dev->pdev->revision <
>  			ARRAY_SIZE(skl_stepping_info)))
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 419660d..2158b8f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3196,6 +3196,16 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  	I915_WRITE(PLANE_AUX_DIST(pipe, 0), aux_dist | aux_stride);
>  	I915_WRITE(PLANE_AUX_OFFSET(pipe, 0), aux_y_offset << 16 | aux_x_offset);
>  
> +	DRM_DEBUG_KMS("KCM: is_skl = %d is_bxt = %d\n",
> +		IS_SKYLAKE(dev), IS_BROXTON(dev));
> +
> +	if (((IS_SKYLAKE(dev) && intel_get_stepping(dev) == 'C') ||
> +		(IS_BROXTON(dev) && intel_get_stepping(dev) == 'A')) &&
> +		fb->pixel_format == DRM_FORMAT_NV12) {
> +			I915_WRITE(CHICKEN_PIPESL(pipe),
> +				I915_READ(CHICKEN_PIPESL(pipe)) | DISABLE_STREAMER_FIX);
> +	}

According to Bspec this would need to be disabled for render
compression. And to do that we'd need to add some vblank waits to make
sure we don't disable it too soon. But since it's pre-production
hardware anyway I guess we might not care too much.

I would probably drop SKL from these since I'd assume almost everyone
has D+ by now. And maybe just stuff it in init_clock_gating for BXT
since we're going to eliminate it soonish anyway.

> +
>  	if (scaler_id >= 0) {
>  		uint32_t ps_ctrl = 0;
>  
> @@ -5004,6 +5014,21 @@ static bool hsw_crtc_supports_ips(struct intel_crtc *crtc)
>  	return HAS_IPS(crtc->base.dev) && crtc->pipe == PIPE_A;
>  }
>  
> +
> +static void skl_wa_clkgate(struct drm_i915_private *dev_priv,
> +	int pipe, int enable)
> +{
> +	if (pipe == PIPE_A || pipe == PIPE_B) {
> +		if (enable)
> +			I915_WRITE(CLKGATE_DIS_PSL(pipe),
> +				DUPS1_GATING_DIS | DUPS2_GATING_DIS);
> +		else
> +			I915_WRITE(CLKGATE_DIS_PSL(pipe),
> +				I915_READ(CLKGATE_DIS_PSL(pipe) &
> +				~(DUPS1_GATING_DIS|DUPS2_GATING_DIS)));
> +	}
> +}
> +
>  static void haswell_crtc_enable(struct drm_crtc *crtc)
>  {
>  	struct drm_device *dev = crtc->dev;
> @@ -5094,6 +5119,9 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
>  		intel_wait_for_vblank(dev, hsw_workaround_pipe);
>  		intel_wait_for_vblank(dev, hsw_workaround_pipe);
>  	}
> +
> +	/* workaround for NV12 */
> +	skl_wa_clkgate(dev_priv, pipe, 1);

I wonder what's the cost of having this
a) always enabled
b) enabled when the pipe is enabled
c) enabled only when NV12 is used
?

>  }
>  
>  static void ironlake_pfit_disable(struct intel_crtc *crtc)
> @@ -5211,6 +5239,9 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
>  
>  	intel_crtc->active = false;
>  	intel_update_watermarks(crtc);
> +
> +	/* workaround for NV12 */
> +	skl_wa_clkgate(dev_priv, intel_crtc->pipe, 0);
>  }
>  
>  static void i9xx_pfit_enable(struct intel_crtc *crtc)
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index d50b8cb..63750d5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1158,6 +1158,7 @@ void intel_csr_load_status_set(struct drm_i915_private *dev_priv,
>  					enum csr_state state);
>  void intel_csr_load_program(struct drm_device *dev);
>  void intel_csr_ucode_fini(struct drm_device *dev);
> +char intel_get_stepping(struct drm_device *dev);
>  void assert_csr_loaded(struct drm_i915_private *dev_priv);
>  
>  /* intel_dp.c */
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 0ea9273..9d1c5b9 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -278,6 +278,13 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  	I915_WRITE(PLANE_AUX_DIST(pipe, plane), aux_dist | aux_stride);
>  	I915_WRITE(PLANE_AUX_OFFSET(pipe, plane), aux_y_offset<<16 | aux_x_offset);
>  
> +	if (((IS_SKYLAKE(dev) && intel_get_stepping(dev) == 'C') ||
> +		(IS_BROXTON(dev) && intel_get_stepping(dev) == 'A')) &&
> +		fb->pixel_format == DRM_FORMAT_NV12) {
> +			I915_WRITE(CHICKEN_PIPESL(pipe),
> +				I915_READ(CHICKEN_PIPESL(pipe)) | DISABLE_STREAMER_FIX);
> +	}
> +
>  	/* program plane scaler */
>  	if (scaler_id >= 0) {
>  		uint32_t ps_ctrl = 0;
> -- 
> 1.7.9.5
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 14/15] drm/i915: skl nv12 workarounds

[Konduru, Chandra]

> > +
> > +	if (((IS_SKYLAKE(dev) && intel_get_stepping(dev) == 'C') ||
> > +		(IS_BROXTON(dev) && intel_get_stepping(dev) == 'A')) &&
> > +		fb->pixel_format == DRM_FORMAT_NV12) {
> > +			I915_WRITE(CHICKEN_PIPESL(pipe),
> > +				I915_READ(CHICKEN_PIPESL(pipe)) |
> DISABLE_STREAMER_FIX);
> > +	}
> 
> According to Bspec this would need to be disabled for render
> compression. And to do that we'd need to add some vblank waits to make
> sure we don't disable it too soon. But since it's pre-production
> hardware anyway I guess we might not care too much.

Render compression related checks will be coming as part of that patch.

> 
> I would probably drop SKL from these since I'd assume almost everyone
> has D+ by now. And maybe just stuff it in init_clock_gating for BXT
> since we're going to eliminate it soonish anyway.

Last checked some folks are still using, so keep it intact for completeness.

> > +static void skl_wa_clkgate(struct drm_i915_private *dev_priv,
> > +	int pipe, int enable)
> > +{
> > +	if (pipe == PIPE_A || pipe == PIPE_B) {
> > +		if (enable)
> > +			I915_WRITE(CLKGATE_DIS_PSL(pipe),
> > +				DUPS1_GATING_DIS | DUPS2_GATING_DIS);
> > +		else
> > +			I915_WRITE(CLKGATE_DIS_PSL(pipe),
> > +				I915_READ(CLKGATE_DIS_PSL(pipe) &
> > +				~(DUPS1_GATING_DIS|DUPS2_GATING_DIS)));
> > +	}
> > +}
> > +
> >  static void haswell_crtc_enable(struct drm_crtc *crtc)
> >  {
> >  	struct drm_device *dev = crtc->dev;
> > @@ -5094,6 +5119,9 @@ static void haswell_crtc_enable(struct drm_crtc
> *crtc)
> >  		intel_wait_for_vblank(dev, hsw_workaround_pipe);
> >  		intel_wait_for_vblank(dev, hsw_workaround_pipe);
> >  	}
> > +
> > +	/* workaround for NV12 */
> > +	skl_wa_clkgate(dev_priv, pipe, 1);
> 
> I wonder what's the cost of having this
> a) always enabled
> b) enabled when the pipe is enabled
> c) enabled only when NV12 is used
> ?

Initially optimized to enable only when nv12 is used,
but there are some corner cases when planes switch to and 
from nv12 to non-nv12 and SV recommendation is to enable
always; and SV evaluated cost, and it isn't a big concern.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 14/15] drm/i915: skl nv12 workarounds

[Ville Syrjälä]

On Sat, Sep 05, 2015 at 01:28:56AM +0000, Konduru, Chandra wrote:
> > > +
> > > +	if (((IS_SKYLAKE(dev) && intel_get_stepping(dev) == 'C') ||
> > > +		(IS_BROXTON(dev) && intel_get_stepping(dev) == 'A')) &&
> > > +		fb->pixel_format == DRM_FORMAT_NV12) {
> > > +			I915_WRITE(CHICKEN_PIPESL(pipe),
> > > +				I915_READ(CHICKEN_PIPESL(pipe)) |
> > DISABLE_STREAMER_FIX);
> > > +	}
> > 
> > According to Bspec this would need to be disabled for render
> > compression. And to do that we'd need to add some vblank waits to make
> > sure we don't disable it too soon. But since it's pre-production
> > hardware anyway I guess we might not care too much.
> 
> Render compression related checks will be coming as part of that patch.
> 
> > 
> > I would probably drop SKL from these since I'd assume almost everyone
> > has D+ by now. And maybe just stuff it in init_clock_gating for BXT
> > since we're going to eliminate it soonish anyway.
> 
> Last checked some folks are still using, so keep it intact for completeness.
> 
> > > +static void skl_wa_clkgate(struct drm_i915_private *dev_priv,
> > > +	int pipe, int enable)
> > > +{
> > > +	if (pipe == PIPE_A || pipe == PIPE_B) {
> > > +		if (enable)
> > > +			I915_WRITE(CLKGATE_DIS_PSL(pipe),
> > > +				DUPS1_GATING_DIS | DUPS2_GATING_DIS);
> > > +		else
> > > +			I915_WRITE(CLKGATE_DIS_PSL(pipe),
> > > +				I915_READ(CLKGATE_DIS_PSL(pipe) &
> > > +				~(DUPS1_GATING_DIS|DUPS2_GATING_DIS)));
> > > +	}
> > > +}
> > > +
> > >  static void haswell_crtc_enable(struct drm_crtc *crtc)
> > >  {
> > >  	struct drm_device *dev = crtc->dev;
> > > @@ -5094,6 +5119,9 @@ static void haswell_crtc_enable(struct drm_crtc
> > *crtc)
> > >  		intel_wait_for_vblank(dev, hsw_workaround_pipe);
> > >  		intel_wait_for_vblank(dev, hsw_workaround_pipe);
> > >  	}
> > > +
> > > +	/* workaround for NV12 */
> > > +	skl_wa_clkgate(dev_priv, pipe, 1);
> > 
> > I wonder what's the cost of having this
> > a) always enabled
> > b) enabled when the pipe is enabled
> > c) enabled only when NV12 is used
> > ?
> 
> Initially optimized to enable only when nv12 is used,
> but there are some corner cases when planes switch to and 
> from nv12 to non-nv12 and SV recommendation is to enable
> always; and SV evaluated cost, and it isn't a big concern.

So, based on that we could just stuff it into init_clock_gating and
forget about it.

But we'll run into problems as soon as render compression enters the
picure. But I don't have a problem leaving it up to the render
compression patches to solve.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 14/15] drm/i915: skl nv12 workarounds

[Konduru, Chandra]

> > > > +static void skl_wa_clkgate(struct drm_i915_private *dev_priv,
> > > > +	int pipe, int enable)
> > > > +{
> > > > +	if (pipe == PIPE_A || pipe == PIPE_B) {
> > > > +		if (enable)
> > > > +			I915_WRITE(CLKGATE_DIS_PSL(pipe),
> > > > +				DUPS1_GATING_DIS | DUPS2_GATING_DIS);
> > > > +		else
> > > > +			I915_WRITE(CLKGATE_DIS_PSL(pipe),
> > > > +				I915_READ(CLKGATE_DIS_PSL(pipe) &
> > > > +				~(DUPS1_GATING_DIS|DUPS2_GATING_DIS)));
> > > > +	}
> > > > +}
> > > > +
> > > >  static void haswell_crtc_enable(struct drm_crtc *crtc)
> > > >  {
> > > >  	struct drm_device *dev = crtc->dev;
> > > > @@ -5094,6 +5119,9 @@ static void haswell_crtc_enable(struct drm_crtc
> > > *crtc)
> > > >  		intel_wait_for_vblank(dev, hsw_workaround_pipe);
> > > >  		intel_wait_for_vblank(dev, hsw_workaround_pipe);
> > > >  	}
> > > > +
> > > > +	/* workaround for NV12 */
> > > > +	skl_wa_clkgate(dev_priv, pipe, 1);
> > >
> > > I wonder what's the cost of having this
> > > a) always enabled
> > > b) enabled when the pipe is enabled
> > > c) enabled only when NV12 is used
> > > ?
> >
> > Initially optimized to enable only when nv12 is used,
> > but there are some corner cases when planes switch to and
> > from nv12 to non-nv12 and SV recommendation is to enable
> > always; and SV evaluated cost, and it isn't a big concern.
> 
> So, based on that we could just stuff it into init_clock_gating and
> forget about it.

Couldn't include into init_clock_gating because this requires
a pipe based check.

By the way, so far 4 patches got RB tags.
In the respun series (http://lists.freedesktop.org/archives/intel-gfx/2015-September/075235.html
addressed your feedback), those 4 tags goes to 1, 3, 5 and 8 of 15.
Can you check updated patches and issue R-B tags for remaining ones?

> 
> But we'll run into problems as soon as render compression enters the
> picure. But I don't have a problem leaving it up to the render
> compression patches to solve.
> 
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 14/15] drm/i915: skl nv12 workarounds

[Ville Syrjälä]

On Wed, Sep 09, 2015 at 02:51:58AM +0300, Konduru, Chandra wrote:
> > > > > +static void skl_wa_clkgate(struct drm_i915_private *dev_priv,
> > > > > +	int pipe, int enable)
> > > > > +{
> > > > > +	if (pipe == PIPE_A || pipe == PIPE_B) {
> > > > > +		if (enable)
> > > > > +			I915_WRITE(CLKGATE_DIS_PSL(pipe),
> > > > > +				DUPS1_GATING_DIS | DUPS2_GATING_DIS);
> > > > > +		else
> > > > > +			I915_WRITE(CLKGATE_DIS_PSL(pipe),
> > > > > +				I915_READ(CLKGATE_DIS_PSL(pipe) &
> > > > > +				~(DUPS1_GATING_DIS|DUPS2_GATING_DIS)));
> > > > > +	}
> > > > > +}
> > > > > +
> > > > >  static void haswell_crtc_enable(struct drm_crtc *crtc)
> > > > >  {
> > > > >  	struct drm_device *dev = crtc->dev;
> > > > > @@ -5094,6 +5119,9 @@ static void haswell_crtc_enable(struct drm_crtc
> > > > *crtc)
> > > > >  		intel_wait_for_vblank(dev, hsw_workaround_pipe);
> > > > >  		intel_wait_for_vblank(dev, hsw_workaround_pipe);
> > > > >  	}
> > > > > +
> > > > > +	/* workaround for NV12 */
> > > > > +	skl_wa_clkgate(dev_priv, pipe, 1);
> > > >
> > > > I wonder what's the cost of having this
> > > > a) always enabled
> > > > b) enabled when the pipe is enabled
> > > > c) enabled only when NV12 is used
> > > > ?
> > >
> > > Initially optimized to enable only when nv12 is used,
> > > but there are some corner cases when planes switch to and
> > > from nv12 to non-nv12 and SV recommendation is to enable
> > > always; and SV evaluated cost, and it isn't a big concern.
> > 
> > So, based on that we could just stuff it into init_clock_gating and
> > forget about it.
> 
> Couldn't include into init_clock_gating because this requires
> a pipe based check.

init_clock_gating()
{
	...
	enable for pipe A
	enable for pipe B
	...
}

or

for_each_pipe(pipe)
	if (pipe != C)
		enable w/a


> 
> By the way, so far 4 patches got RB tags.
> In the respun series (http://lists.freedesktop.org/archives/intel-gfx/2015-September/075235.html
> addressed your feedback), those 4 tags goes to 1, 3, 5 and 8 of 15.
> Can you check updated patches and issue R-B tags for remaining ones?

I think your trigger finger is a bit overly sensitive :) We still had
open issues in this series so posting another one makes things somewhat
messy.

-- 
Ville Syrjälä
Intel OTC
---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.

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

Re: [PATCH 14/15] drm/i915: skl nv12 workarounds

[Konduru, Chandra]

> > > > > > +
> > > > > > +	/* workaround for NV12 */
> > > > > > +	skl_wa_clkgate(dev_priv, pipe, 1);
> > > > >
> > > > > I wonder what's the cost of having this
> > > > > a) always enabled
> > > > > b) enabled when the pipe is enabled
> > > > > c) enabled only when NV12 is used
> > > > > ?
> > > >
> > > > Initially optimized to enable only when nv12 is used,
> > > > but there are some corner cases when planes switch to and
> > > > from nv12 to non-nv12 and SV recommendation is to enable
> > > > always; and SV evaluated cost, and it isn't a big concern.
> > >
> > > So, based on that we could just stuff it into init_clock_gating and
> > > forget about it.
> >
> > Couldn't include into init_clock_gating because this requires
> > a pipe based check.
> 
> init_clock_gating()
> {
> 	...
> 	enable for pipe A
> 	enable for pipe B
> 	...
> }
> 
> or
> 
> for_each_pipe(pipe)
> 	if (pipe != C)
> 		enable w/a
> 

Oh yeah, it can be done that way.
OK, will move it there. 

> 
> >
> > By the way, so far 4 patches got RB tags.
> > In the respun series (http://lists.freedesktop.org/archives/intel-gfx/2015-
> September/075235.html
> > addressed your feedback), those 4 tags goes to 1, 3, 5 and 8 of 15.
> > Can you check updated patches and issue R-B tags for remaining ones?
> 
> I think your trigger finger is a bit overly sensitive :) We still had
> open issues in this series so posting another one makes things somewhat
> messy.
There is one other open relate to tile-Yf Y, UV plane pitch/stride check. 
I just responded to that one. 

> 
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 15/15] drm/i915: Add 90/270 rotation for NV12 format.

[Chandra Konduru]

Adding NV12 90/270 rotation support for primary and sprite planes.

v2:
-For 90/270 adjust pixel boundary only in Y-direction (bspec)

v3:
-Rebased (me)

Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
Testcase: igt/kms_nv12
---
 drivers/gpu/drm/i915/intel_display.c |   28 +++++++++++------
 drivers/gpu/drm/i915/intel_sprite.c  |   56 ++++++++++++++++++++++------------
 2 files changed, 56 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2158b8f..19d0f8b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3096,7 +3096,8 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
 	int src_x = 0, src_y = 0, src_w = 0, src_h = 0;
 	int dst_x = 0, dst_y = 0, dst_w = 0, dst_h = 0;
 	int scaler_id = -1;
-	u32 aux_dist = 0, aux_x_offset = 0, aux_y_offset = 0, aux_stride = 0;
+	unsigned long aux_dist = 0;
+	u32 aux_x_offset = 0, aux_y_offset = 0, aux_stride = 0;
 	u32 tile_row_adjustment = 0;
 	u32 hphase = 0, vphase = 0;
 
@@ -3155,12 +3156,16 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
 		x_offset = stride * tile_height - y - src_h;
 		y_offset = x;
 		plane_size = (src_w - 1) << 16 | (src_h - 1);
-		/*
-		 * TBD: For NV12 90/270 rotation, Y and UV subplanes should
-		 * be treated as separate surfaces and GTT remapping for
-		 * rotation should be done separately for each subplane.
-		 * Enable support once seperate remappings are available.
-		 */
+
+		if (fb->pixel_format == DRM_FORMAT_NV12) {
+			u32 uv_tile_height = intel_tile_height(dev, fb->pixel_format,
+						fb->modifier[0], 1);
+			aux_stride = DIV_ROUND_UP(fb->height / 2, uv_tile_height);
+			aux_dist = intel_plane_obj_offset(to_intel_plane(plane), obj, 1) -
+				surf_addr;
+			aux_x_offset = aux_stride * uv_tile_height - y / 2 - fb->height / 2;
+			aux_y_offset = x / 2;
+		}
 	} else {
 		stride = fb->pitches[0] / stride_div;
 		x_offset = x;
@@ -11697,8 +11702,13 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
 
 	/* Adjust (macro)pixel boundary */
 	if (fb && intel_format_is_yuv(fb->pixel_format)) {
-		to_intel_plane_state(plane_state)->src.x1 &= ~0x10000;
-		to_intel_plane_state(plane_state)->src.x2 &= ~0x10000;
+		if (intel_rotation_90_or_270(plane_state->rotation)) {
+			to_intel_plane_state(plane_state)->src.y1 &= ~0x10000;
+			to_intel_plane_state(plane_state)->src.y2 &= ~0x10000;
+		} else {
+			to_intel_plane_state(plane_state)->src.x1 &= ~0x10000;
+			to_intel_plane_state(plane_state)->src.x2 &= ~0x10000;
+		}
 	}
 
 	if (crtc_state && INTEL_INFO(dev)->gen >= 9 &&
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 9d1c5b9..3522cb0 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -188,7 +188,8 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 	int x_offset, y_offset;
 	struct intel_crtc_state *crtc_state = to_intel_crtc(crtc)->config;
 	int scaler_id;
-	u32 aux_dist = 0, aux_x_offset = 0, aux_y_offset = 0, aux_stride = 0;
+	unsigned long aux_dist = 0;
+	u32 aux_x_offset = 0, aux_y_offset = 0, aux_stride = 0;
 	u32 tile_row_adjustment = 0;
 	u32 hphase = 0, vphase = 0;
 
@@ -238,12 +239,14 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
 		x_offset = stride * tile_height - y - (src_h + 1);
 		y_offset = x;
 
-		/*
-		 * TBD: For NV12 90/270 rotation, Y and UV subplanes should
-		 * be treated as separate surfaces and GTT remapping for
-		 * rotation should be done separately for each subplane.
-		 * Enable support once seperate remappings are available.
-		 */
+		if (fb->pixel_format == DRM_FORMAT_NV12) {
+			u32 uv_tile_height = intel_tile_height(dev, fb->pixel_format,
+						fb->modifier[0], 1);
+			aux_stride = DIV_ROUND_UP(fb->height / 2, uv_tile_height);
+			aux_dist = intel_plane_obj_offset(intel_plane, obj, 1) - surf_addr;
+			aux_x_offset = aux_stride * uv_tile_height - y / 2 - fb->height / 2;
+			aux_y_offset = x / 2;
+		}
 	} else {
 		stride = fb->pitches[0] / stride_div;
 		plane_size = (src_h << 16) | src_w;
@@ -900,18 +903,33 @@ intel_check_sprite_plane(struct drm_plane *plane,
 		src_h = drm_rect_height(src) >> 16;
 
 		if (intel_format_is_yuv(fb->pixel_format)) {
-			src_x &= ~1;
-			src_w &= ~1;
-
-			/*
-			 * Must keep src and dst the
-			 * same if we can't scale.
-			 */
-			if (!can_scale)
-				crtc_w &= ~1;
-
-			if (crtc_w == 0)
-				state->visible = false;
+			if (intel_rotation_90_or_270(state->base.rotation)) {
+				src_y &= ~1;
+				src_h &= ~1;
+
+				/*
+				 * Must keep src and dst the
+				 * same if we can't scale.
+				 */
+				if (!can_scale)
+					crtc_h &= ~1;
+
+				if (crtc_h == 0)
+					state->visible = false;
+			} else {
+				src_x &= ~1;
+				src_w &= ~1;
+
+				/*
+				 * Must keep src and dst the
+				 * same if we can't scale.
+				 */
+				if (!can_scale)
+					crtc_w &= ~1;
+
+				if (crtc_w == 0)
+					state->visible = false;
+			}
 		}
 	}
 
-- 
1.7.9.5

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

Re: [PATCH 15/15] drm/i915: Add 90/270 rotation for NV12 format.

[Ville Syrjälä]

On Wed, Aug 19, 2015 at 06:02:36PM -0700, Chandra Konduru wrote:
> Adding NV12 90/270 rotation support for primary and sprite planes.
> 
> v2:
> -For 90/270 adjust pixel boundary only in Y-direction (bspec)
> 
> v3:
> -Rebased (me)
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> Testcase: igt/kms_nv12
> ---
>  drivers/gpu/drm/i915/intel_display.c |   28 +++++++++++------
>  drivers/gpu/drm/i915/intel_sprite.c  |   56 ++++++++++++++++++++++------------
>  2 files changed, 56 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2158b8f..19d0f8b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3096,7 +3096,8 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  	int src_x = 0, src_y = 0, src_w = 0, src_h = 0;
>  	int dst_x = 0, dst_y = 0, dst_w = 0, dst_h = 0;
>  	int scaler_id = -1;
> -	u32 aux_dist = 0, aux_x_offset = 0, aux_y_offset = 0, aux_stride = 0;
> +	unsigned long aux_dist = 0;
> +	u32 aux_x_offset = 0, aux_y_offset = 0, aux_stride = 0;
>  	u32 tile_row_adjustment = 0;
>  	u32 hphase = 0, vphase = 0;
>  
> @@ -3155,12 +3156,16 @@ static void skylake_update_primary_plane(struct drm_crtc *crtc,
>  		x_offset = stride * tile_height - y - src_h;
>  		y_offset = x;
>  		plane_size = (src_w - 1) << 16 | (src_h - 1);
> -		/*
> -		 * TBD: For NV12 90/270 rotation, Y and UV subplanes should
> -		 * be treated as separate surfaces and GTT remapping for
> -		 * rotation should be done separately for each subplane.
> -		 * Enable support once seperate remappings are available.
> -		 */
> +
> +		if (fb->pixel_format == DRM_FORMAT_NV12) {
> +			u32 uv_tile_height = intel_tile_height(dev, fb->pixel_format,
> +						fb->modifier[0], 1);
> +			aux_stride = DIV_ROUND_UP(fb->height / 2, uv_tile_height);
> +			aux_dist = intel_plane_obj_offset(to_intel_plane(plane), obj, 1) -
> +				surf_addr;
> +			aux_x_offset = aux_stride * uv_tile_height - y / 2 - fb->height / 2;
> +			aux_y_offset = x / 2;
> +		}
>  	} else {
>  		stride = fb->pitches[0] / stride_div;
>  		x_offset = x;
> @@ -11697,8 +11702,13 @@ int intel_plane_atomic_calc_changes(struct drm_crtc_state *crtc_state,
>  
>  	/* Adjust (macro)pixel boundary */
>  	if (fb && intel_format_is_yuv(fb->pixel_format)) {
> -		to_intel_plane_state(plane_state)->src.x1 &= ~0x10000;
> -		to_intel_plane_state(plane_state)->src.x2 &= ~0x10000;
> +		if (intel_rotation_90_or_270(plane_state->rotation)) {
> +			to_intel_plane_state(plane_state)->src.y1 &= ~0x10000;
> +			to_intel_plane_state(plane_state)->src.y2 &= ~0x10000;
> +		} else {
> +			to_intel_plane_state(plane_state)->src.x1 &= ~0x10000;
> +			to_intel_plane_state(plane_state)->src.x2 &= ~0x10000;
> +		}

IIRC we concluded (with Art's help) that this is not needed. We always
want to align in src.x.

>  	}
>  
>  	if (crtc_state && INTEL_INFO(dev)->gen >= 9 &&
> diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
> index 9d1c5b9..3522cb0 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -188,7 +188,8 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  	int x_offset, y_offset;
>  	struct intel_crtc_state *crtc_state = to_intel_crtc(crtc)->config;
>  	int scaler_id;
> -	u32 aux_dist = 0, aux_x_offset = 0, aux_y_offset = 0, aux_stride = 0;
> +	unsigned long aux_dist = 0;
> +	u32 aux_x_offset = 0, aux_y_offset = 0, aux_stride = 0;
>  	u32 tile_row_adjustment = 0;
>  	u32 hphase = 0, vphase = 0;
>  
> @@ -238,12 +239,14 @@ skl_update_plane(struct drm_plane *drm_plane, struct drm_crtc *crtc,
>  		x_offset = stride * tile_height - y - (src_h + 1);
>  		y_offset = x;
>  
> -		/*
> -		 * TBD: For NV12 90/270 rotation, Y and UV subplanes should
> -		 * be treated as separate surfaces and GTT remapping for
> -		 * rotation should be done separately for each subplane.
> -		 * Enable support once seperate remappings are available.
> -		 */
> +		if (fb->pixel_format == DRM_FORMAT_NV12) {
> +			u32 uv_tile_height = intel_tile_height(dev, fb->pixel_format,
> +						fb->modifier[0], 1);
> +			aux_stride = DIV_ROUND_UP(fb->height / 2, uv_tile_height);
> +			aux_dist = intel_plane_obj_offset(intel_plane, obj, 1) - surf_addr;
> +			aux_x_offset = aux_stride * uv_tile_height - y / 2 - fb->height / 2;
> +			aux_y_offset = x / 2;
> +		}
>  	} else {
>  		stride = fb->pitches[0] / stride_div;
>  		plane_size = (src_h << 16) | src_w;
> @@ -900,18 +903,33 @@ intel_check_sprite_plane(struct drm_plane *plane,
>  		src_h = drm_rect_height(src) >> 16;
>  
>  		if (intel_format_is_yuv(fb->pixel_format)) {
> -			src_x &= ~1;
> -			src_w &= ~1;
> -
> -			/*
> -			 * Must keep src and dst the
> -			 * same if we can't scale.
> -			 */
> -			if (!can_scale)
> -				crtc_w &= ~1;
> -
> -			if (crtc_w == 0)
> -				state->visible = false;
> +			if (intel_rotation_90_or_270(state->base.rotation)) {
> +				src_y &= ~1;
> +				src_h &= ~1;
> +
> +				/*
> +				 * Must keep src and dst the
> +				 * same if we can't scale.
> +				 */
> +				if (!can_scale)
> +					crtc_h &= ~1;
> +
> +				if (crtc_h == 0)
> +					state->visible = false;
> +			} else {
> +				src_x &= ~1;
> +				src_w &= ~1;
> +
> +				/*
> +				 * Must keep src and dst the
> +				 * same if we can't scale.
> +				 */
> +				if (!can_scale)
> +					crtc_w &= ~1;
> +
> +				if (crtc_w == 0)
> +					state->visible = false;
> +			}

Same here.

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

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 15/15] drm/i915: Add 90/270 rotation for NV12 format.

[Konduru, Chandra]

> >  	/* Adjust (macro)pixel boundary */
> >  	if (fb && intel_format_is_yuv(fb->pixel_format)) {
> > -		to_intel_plane_state(plane_state)->src.x1 &= ~0x10000;
> > -		to_intel_plane_state(plane_state)->src.x2 &= ~0x10000;
> > +		if (intel_rotation_90_or_270(plane_state->rotation)) {
> > +			to_intel_plane_state(plane_state)->src.y1 &=
> ~0x10000;
> > +			to_intel_plane_state(plane_state)->src.y2 &=
> ~0x10000;
> > +		} else {
> > +			to_intel_plane_state(plane_state)->src.x1 &=
> ~0x10000;
> > +			to_intel_plane_state(plane_state)->src.x2 &=
> ~0x10000;
> > +		}
> 
> IIRC we concluded (with Art's help) that this is not needed. We always
> want to align in src.x.

Initial code that I added was making both X and Y offsets as even 
when 90/270 as per bspec at that time. 

But later Art update is as below:
  >> " the X offset must always be even for YUV422+NV12, and 
  >>   the Y offset must be even when rotated 90/270 degrees."

So, above code change is needed.


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

Re: [PATCH 15/15] drm/i915: Add 90/270 rotation for NV12 format.

[Ville Syrjälä]

On Sat, Sep 05, 2015 at 01:38:14AM +0000, Konduru, Chandra wrote:
> > >  	/* Adjust (macro)pixel boundary */
> > >  	if (fb && intel_format_is_yuv(fb->pixel_format)) {
> > > -		to_intel_plane_state(plane_state)->src.x1 &= ~0x10000;
> > > -		to_intel_plane_state(plane_state)->src.x2 &= ~0x10000;
> > > +		if (intel_rotation_90_or_270(plane_state->rotation)) {
> > > +			to_intel_plane_state(plane_state)->src.y1 &=
> > ~0x10000;
> > > +			to_intel_plane_state(plane_state)->src.y2 &=
> > ~0x10000;
> > > +		} else {
> > > +			to_intel_plane_state(plane_state)->src.x1 &=
> > ~0x10000;
> > > +			to_intel_plane_state(plane_state)->src.x2 &=
> > ~0x10000;
> > > +		}
> > 
> > IIRC we concluded (with Art's help) that this is not needed. We always
> > want to align in src.x.
> 
> Initial code that I added was making both X and Y offsets as even 
> when 90/270 as per bspec at that time. 
> 
> But later Art update is as below:
>   >> " the X offset must always be even for YUV422+NV12, and 
>   >>   the Y offset must be even when rotated 90/270 degrees."
> 
> So, above code change is needed.

That's the orignal text, later update from Art was:
 "They don't both have to be even when roated.  The text should say " the
  X offset must be even for YUV422+NV12 ***when not rotated 90/270***, and
  the Y offset must be even when rotated 90/270 degrees.""

And the spec has been updated since to include the same information. For
us we just need to align src.x? since that's what gets programmed into
the X offset for 0/180, and into the Y offset for 90/270. So the current
code is fine as is.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx