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

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

[Chandra Konduru]

This patch series is adding 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 testcase.
Feature is unit tested for linear/X/Y/Yf formats in 0, 90, 180, 270
orientations with combinations of 1, 2 or 3 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.

Chandra Konduru (12):
  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: Enable NV12 primary plane via crtc set config
  drm/i915: Add NV12 to primary plane programming.
  drm/i915: Add NV12 to sprite plane programming.
  drm/i915: Add 90/270 rotation for NV12 format.

 drivers/gpu/drm/i915/i915_reg.h      |   27 ++++++++
 drivers/gpu/drm/i915/intel_atomic.c  |    5 +-
 drivers/gpu/drm/i915/intel_display.c |  121 ++++++++++++++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_drv.h     |    4 +-
 drivers/gpu/drm/i915/intel_sprite.c  |   77 ++++++++++++++++++++--
 5 files changed, 219 insertions(+), 15 deletions(-)

-- 
1.7.9.5

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

[PATCH 01/12] 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 77055b9..e9ec5e2 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -5408,6 +5408,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)
@@ -5511,6 +5512,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

[PATCH 02/12] 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 7ed8033..13f8a95 100644
--- a/drivers/gpu/drm/i915/intel_atomic.c
+++ b/drivers/gpu/drm/i915/intel_atomic.c
@@ -404,7 +404,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

[PATCH 03/12] 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.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 0a2e883..1ad7d13 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4499,9 +4499,11 @@ skl_update_scaler_users(
 		rotation = DRM_ROTATE_0;
 	}
 
-	need_scaling = intel_rotation_90_or_270(rotation) ?
-		(src_h != dst_w || src_w != dst_h):
-		(src_w != dst_w || src_h != dst_h);
+	/* scaling is required when src dst sizes doesn't match or format is NV12 */
+	need_scaling = (src_w != dst_w || src_h != dst_h ||
+		(intel_rotation_90_or_270(rotation) &&
+			(src_h != dst_w || src_w != dst_h)) ||
+		(fb && fb->pixel_format == DRM_FORMAT_NV12));
 
 	/*
 	 * if plane is being disabled or scaler is no more required or force detach
@@ -4567,6 +4569,7 @@ skl_update_scaler_users(
 		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

[PATCH 04/12] 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.

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

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index b18cfba..754172f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1388,6 +1388,7 @@ int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
 bool 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 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 3355d5a..80ae21f 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -39,7 +39,7 @@
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
 
-static bool
+bool
 format_is_yuv(uint32_t format)
 {
 	switch (format) {
@@ -47,6 +47,7 @@ format_is_yuv(uint32_t format)
 	case DRM_FORMAT_UYVY:
 	case DRM_FORMAT_VYUY:
 	case DRM_FORMAT_YVYU:
+	case DRM_FORMAT_NV12:
 		return true;
 	default:
 		return false;
-- 
1.7.9.5

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

[PATCH 05/12] drm/i915: Upscale scaler max scale for NV12.

[Chandra Konduru]

This patch updates max supported scaler limits for NV12.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1ad7d13..b7e4b3b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13001,7 +13001,8 @@ 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;
@@ -13021,11 +13022,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;
 }
@@ -13055,7 +13058,8 @@ intel_check_primary_plane(struct drm_plane *plane,
 
 	if (INTEL_INFO(dev)->gen >= 9) {
 		min_scale = 1;
-		max_scale = skl_max_scale(intel_crtc, crtc_state);
+		max_scale = skl_max_scale(intel_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 754172f..aa77af7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1139,7 +1139,8 @@ void skl_detach_scalers(struct intel_crtc *intel_crtc);
 int skl_update_scaler_users(struct intel_crtc *intel_crtc,
 	struct intel_crtc_state *crtc_state, struct intel_plane *intel_plane,
 	struct intel_plane_state *plane_state, int force_detach);
-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 80ae21f..b2491ad 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -805,7 +805,7 @@ intel_check_sprite_plane(struct drm_plane *plane,
 
 	if (INTEL_INFO(dev)->gen >= 9) {
 		min_scale = 1;
-		max_scale = skl_max_scale(intel_crtc, crtc_state);
+		max_scale = skl_max_scale(intel_crtc, crtc_state, fb->pixel_format);
 	}
 
 	drm_rect_rotate(src, fb->width << 16, fb->height << 16,
-- 
1.7.9.5

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

[PATCH 06/12] drm/i915: Add NV12 as supported format for primary plane

[Chandra Konduru]

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

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b7e4b3b..42924a6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -70,6 +70,18 @@ static const uint32_t i965_primary_formats[] = {
 	DRM_FORMAT_ABGR2101010,
 };
 
+/* Primary plane formats for gen >= 9 with NV12 */
+static const uint32_t skl_primary_formats_with_nv12[] = {
+	COMMON_PRIMARY_FORMATS, \
+	DRM_FORMAT_XBGR8888,
+	DRM_FORMAT_ABGR8888,
+	DRM_FORMAT_XRGB2101010,
+	DRM_FORMAT_ARGB2101010,
+	DRM_FORMAT_XBGR2101010,
+	DRM_FORMAT_ABGR2101010,
+	DRM_FORMAT_NV12,
+};
+
 /* Cursor formats */
 static const uint32_t intel_cursor_formats[] = {
 	DRM_FORMAT_ARGB8888,
@@ -13315,6 +13327,10 @@ static struct drm_plane *intel_primary_plane_create(struct drm_device *dev,
 	if (INTEL_INFO(dev)->gen <= 3) {
 		intel_primary_formats = i8xx_primary_formats;
 		num_formats = ARRAY_SIZE(i8xx_primary_formats);
+	} else if (INTEL_INFO(dev)->gen >= 9 &&
+		(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 = 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

[PATCH 07/12] 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 b2491ad..84755c6 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -1096,6 +1096,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)
 {
@@ -1167,8 +1180,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

[PATCH 08/12] 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.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 42924a6..41cd26f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14043,6 +14043,33 @@ 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]);
+		}
+		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

[PATCH 09/12] drm/i915: Enable NV12 primary plane via crtc set config

[Chandra Konduru]

Setup a scaler for primary plane if its FB is in NV12 format
in legacy crtc set config path.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 41cd26f..b31f0fe 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -12292,6 +12292,16 @@ intel_modeset_compute_config(struct drm_crtc *crtc,
 	if (ret)
 		return ERR_PTR(ret);
 
+	/*
+	 * FIXME: Until full atomic_crtc is available and hooked up for
+	 * legacy APIs, setup scaler for primary plane with NV12 fb coming
+	 * in those paths (e.g., crtc_setconfig).
+	 */
+	ret = intel_atomic_setup_scalers(state->dev, to_intel_crtc(crtc),
+		pipe_config);
+	if (ret)
+		return ERR_PTR(ret);
+
 	return pipe_config;
 }
 
-- 
1.7.9.5

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

[PATCH 10/12] 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)

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b31f0fe..cb3a0fc 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3028,6 +3028,9 @@ u32 skl_plane_ctl_format(uint32_t pixel_format)
 	case DRM_FORMAT_VYUY:
 		format = PLANE_CTL_FORMAT_YUV422 | PLANE_CTL_YUV422_VYUY;
 		break;
+	case DRM_FORMAT_NV12:
+		format = PLANE_CTL_FORMAT_NV12;
+		break;
 	default:
 		MISSING_CASE(pixel_format);
 	}
@@ -3102,6 +3105,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);
 
@@ -3158,11 +3163,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;
 
@@ -3170,11 +3198,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);
@@ -3183,6 +3214,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);
 	}
 
@@ -13136,6 +13168,12 @@ intel_check_primary_plane(struct drm_plane *plane,
 			intel_crtc->atomic.update_wm = true;
 	}
 
+	/* Adjust (macro)pixel boundary */
+	if (fb && format_is_yuv(fb->pixel_format)) {
+		src->x1 &= ~0x10000;
+		src->x2 &= ~0x10000;
+	}
+
 	if (INTEL_INFO(dev)->gen >= 9) {
 		ret = skl_update_scaler_users(intel_crtc, crtc_state,
 			to_intel_plane(plane), state, 0);
-- 
1.7.9.5

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

[PATCH 11/12] 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 84755c6..42cfac8 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -190,6 +190,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;
@@ -236,24 +238,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);
@@ -264,6 +290,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 12/12] drm/i915: Add 90/270 rotation for NV12 format.

[Chandra Konduru]

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

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index cb3a0fc..d108a97 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3105,7 +3105,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;
 
 	plane_state = to_intel_plane_state(plane->state);
@@ -3163,12 +3164,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;
@@ -13172,6 +13177,10 @@ intel_check_primary_plane(struct drm_plane *plane,
 	if (fb && format_is_yuv(fb->pixel_format)) {
 		src->x1 &= ~0x10000;
 		src->x2 &= ~0x10000;
+		if (intel_rotation_90_or_270(state->base.rotation)) {
+			src->y1 &= ~0x10000;
+			src->y2 &= ~0x10000;
+		}
 	}
 
 	if (INTEL_INFO(dev)->gen >= 9) {
diff --git a/drivers/gpu/drm/i915/intel_sprite.c b/drivers/gpu/drm/i915/intel_sprite.c
index 42cfac8..8b5be50 100644
--- a/drivers/gpu/drm/i915/intel_sprite.c
+++ b/drivers/gpu/drm/i915/intel_sprite.c
@@ -190,7 +190,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;
 
 	plane_ctl = PLANE_CTL_ENABLE |
@@ -239,12 +240,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;
@@ -909,6 +912,21 @@ intel_check_sprite_plane(struct drm_plane *plane,
 
 			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 (!intel_plane->can_scale)
+					crtc_h &= ~1;
+
+				if (crtc_h == 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 08/12] drm/i915: Add NV12 support to intel_framebuffer_init

[Daniel Vetter]

On Sun, May 17, 2015 at 10:11:01PM -0700, Chandra Konduru wrote:
> This patch adds NV12 as supported format to
> intel_framebuffer_init and performs various checks.
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> Testcase: igt/kms_nv12
> ---
>  drivers/gpu/drm/i915/intel_display.c |   27 +++++++++++++++++++++++++++
>  1 file changed, 27 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 42924a6..41cd26f 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14043,6 +14043,33 @@ 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;
> +		}

Nope. It's perfectly ok to have NV12 with a 0 offset for the uv plane, if
it's e.g. in a separate buffer object. Which is the part this series seems
to be completely missing - there's no code at all to look up (and store in
intel_framebuffer the 2nd i915_bo pointer) the 2nd buffer handle afaics.

You should also change your igts to use 2 separate buffers, just for test
coverage.
-Daniel

> +		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]);
> +		}
> +		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

-- 
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/12] drm/i915: Add NV12 support to intel_framebuffer_init

[Konduru, Chandra]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Tuesday, May 19, 2015 1:24 AM
> To: Konduru, Chandra
> Cc: intel-gfx@lists.freedesktop.org; Vetter, Daniel; Syrjala, Ville
> Subject: Re: [Intel-gfx] [PATCH 08/12] drm/i915: Add NV12 support to
> intel_framebuffer_init
> 
> On Sun, May 17, 2015 at 10:11:01PM -0700, Chandra Konduru wrote:
> > This patch adds NV12 as supported format to
> > intel_framebuffer_init and performs various checks.
> >
> > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > Testcase: igt/kms_nv12
> > ---
> >  drivers/gpu/drm/i915/intel_display.c |   27 +++++++++++++++++++++++++++
> >  1 file changed, 27 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_display.c
> b/drivers/gpu/drm/i915/intel_display.c
> > index 42924a6..41cd26f 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -14043,6 +14043,33 @@ 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;
> > +		}
> 
> Nope. It's perfectly ok to have NV12 with a 0 offset for the uv plane, if
> it's e.g. in a separate buffer object. Which is the part this series seems
> to be completely missing - there's no code at all to look up (and store in
> intel_framebuffer the 2nd i915_bo pointer) the 2nd buffer handle afaics.
> 
> You should also change your igts to use 2 separate buffers, just for test
> coverage.
> -Daniel

Hi Daniel,
Agree, in general that is very well ok. But as skl hw requires uv to be after 
y in gtt. This can be guaranteed by having a single bo and y and uv offsets 
into it. Above sanity checks in i915 specific fb init call are for that reason.
There are definitely ways to guarantee uv to be after y even with two 
separate bos (by uv remapping), but I see that is unnecessary 
complication and not sure value by allowing that. Or am I missing 
something here?

> 
> > +		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]);
> > +		}
> > +		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
> 
> --
> 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/12] drm/i915: Add NV12 support to intel_framebuffer_init

[Daniel Vetter]

On Tue, May 19, 2015 at 04:31:54PM +0000, Konduru, Chandra wrote:
> 
> 
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> > Sent: Tuesday, May 19, 2015 1:24 AM
> > To: Konduru, Chandra
> > Cc: intel-gfx@lists.freedesktop.org; Vetter, Daniel; Syrjala, Ville
> > Subject: Re: [Intel-gfx] [PATCH 08/12] drm/i915: Add NV12 support to
> > intel_framebuffer_init
> > 
> > On Sun, May 17, 2015 at 10:11:01PM -0700, Chandra Konduru wrote:
> > > This patch adds NV12 as supported format to
> > > intel_framebuffer_init and performs various checks.
> > >
> > > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > > Testcase: igt/kms_nv12
> > > ---
> > >  drivers/gpu/drm/i915/intel_display.c |   27 +++++++++++++++++++++++++++
> > >  1 file changed, 27 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > b/drivers/gpu/drm/i915/intel_display.c
> > > index 42924a6..41cd26f 100644
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -14043,6 +14043,33 @@ 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;
> > > +		}
> > 
> > Nope. It's perfectly ok to have NV12 with a 0 offset for the uv plane, if
> > it's e.g. in a separate buffer object. Which is the part this series seems
> > to be completely missing - there's no code at all to look up (and store in
> > intel_framebuffer the 2nd i915_bo pointer) the 2nd buffer handle afaics.
> > 
> > You should also change your igts to use 2 separate buffers, just for test
> > coverage.
> > -Daniel
> 
> Hi Daniel,
> Agree, in general that is very well ok. But as skl hw requires uv to be after 
> y in gtt. This can be guaranteed by having a single bo and y and uv offsets 
> into it. Above sanity checks in i915 specific fb init call are for that reason.
> There are definitely ways to guarantee uv to be after y even with two 
> separate bos (by uv remapping), but I see that is unnecessary 
> complication and not sure value by allowing that. Or am I missing 
> something here?

For a start you don't reject multiplane stuff where this isn't the case.
And if we indeed have the hw requirement that the gtt address for y must
be before the gtt address for uv (sounds strange to me, definitely need a
bspec reference for that) then we need to check that throughroughly:
Currently you could place Y after UV even in a single BO.

Also we need igts to make sure we catch userspace throwing invalid stuff
at us.
-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 08/12] drm/i915: Add NV12 support to intel_framebuffer_init

[Konduru, Chandra]

> -----Original Message-----
> From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> Sent: Wednesday, May 20, 2015 12:37 AM
> To: Konduru, Chandra
> Cc: Daniel Vetter; intel-gfx@lists.freedesktop.org; Vetter, Daniel; Syrjala, Ville
> Subject: Re: [Intel-gfx] [PATCH 08/12] drm/i915: Add NV12 support to
> intel_framebuffer_init
> 
> On Tue, May 19, 2015 at 04:31:54PM +0000, Konduru, Chandra wrote:
> >
> >
> > > -----Original Message-----
> > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> Vetter
> > > Sent: Tuesday, May 19, 2015 1:24 AM
> > > To: Konduru, Chandra
> > > Cc: intel-gfx@lists.freedesktop.org; Vetter, Daniel; Syrjala, Ville
> > > Subject: Re: [Intel-gfx] [PATCH 08/12] drm/i915: Add NV12 support to
> > > intel_framebuffer_init
> > >
> > > On Sun, May 17, 2015 at 10:11:01PM -0700, Chandra Konduru wrote:
> > > > This patch adds NV12 as supported format to
> > > > intel_framebuffer_init and performs various checks.
> > > >
> > > > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > > > Testcase: igt/kms_nv12
> > > > ---
> > > >  drivers/gpu/drm/i915/intel_display.c |   27
> +++++++++++++++++++++++++++
> > > >  1 file changed, 27 insertions(+)
> > > >
> > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > b/drivers/gpu/drm/i915/intel_display.c
> > > > index 42924a6..41cd26f 100644
> > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > @@ -14043,6 +14043,33 @@ 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;
> > > > +		}
> > >
> > > Nope. It's perfectly ok to have NV12 with a 0 offset for the uv plane, if
> > > it's e.g. in a separate buffer object. Which is the part this series seems
> > > to be completely missing - there's no code at all to look up (and store in
> > > intel_framebuffer the 2nd i915_bo pointer) the 2nd buffer handle afaics.
> > >
> > > You should also change your igts to use 2 separate buffers, just for test
> > > coverage.
> > > -Daniel
> >
> > Hi Daniel,
> > Agree, in general that is very well ok. But as skl hw requires uv to be after
> > y in gtt. This can be guaranteed by having a single bo and y and uv offsets
> > into it. Above sanity checks in i915 specific fb init call are for that reason.
> > There are definitely ways to guarantee uv to be after y even with two
> > separate bos (by uv remapping), but I see that is unnecessary
> > complication and not sure value by allowing that. Or am I missing
> > something here?
> 
> For a start you don't reject multiplane stuff where this isn't the case.
> And if we indeed have the hw requirement that the gtt address for y must
> be before the gtt address for uv (sounds strange to me, definitely need a
> bspec reference for that) then we need to check that throughroughly:
> Currently you could place Y after UV even in a single BO.

Hi Daniel,
NV12 programming is documented in bspec under display planes 
"Plane Planar YUV programming". There it talks about aux_dist
which is the distance between y and uv planes expecting uv to be
after y.

> 
> Also we need igts to make sure we catch userspace throwing invalid stuff
> at us.

Added a sub-test to kms_nv12 and will send out the updated igt patch.

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

Re: [PATCH 08/12] drm/i915: Add NV12 support to intel_framebuffer_init

[Daniel Vetter]

On Thu, May 21, 2015 at 12:31:49AM +0000, Konduru, Chandra wrote:
> 
> 
> > -----Original Message-----
> > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel Vetter
> > Sent: Wednesday, May 20, 2015 12:37 AM
> > To: Konduru, Chandra
> > Cc: Daniel Vetter; intel-gfx@lists.freedesktop.org; Vetter, Daniel; Syrjala, Ville
> > Subject: Re: [Intel-gfx] [PATCH 08/12] drm/i915: Add NV12 support to
> > intel_framebuffer_init
> > 
> > On Tue, May 19, 2015 at 04:31:54PM +0000, Konduru, Chandra wrote:
> > >
> > >
> > > > -----Original Message-----
> > > > From: Daniel Vetter [mailto:daniel.vetter@ffwll.ch] On Behalf Of Daniel
> > Vetter
> > > > Sent: Tuesday, May 19, 2015 1:24 AM
> > > > To: Konduru, Chandra
> > > > Cc: intel-gfx@lists.freedesktop.org; Vetter, Daniel; Syrjala, Ville
> > > > Subject: Re: [Intel-gfx] [PATCH 08/12] drm/i915: Add NV12 support to
> > > > intel_framebuffer_init
> > > >
> > > > On Sun, May 17, 2015 at 10:11:01PM -0700, Chandra Konduru wrote:
> > > > > This patch adds NV12 as supported format to
> > > > > intel_framebuffer_init and performs various checks.
> > > > >
> > > > > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > > > > Testcase: igt/kms_nv12
> > > > > ---
> > > > >  drivers/gpu/drm/i915/intel_display.c |   27
> > +++++++++++++++++++++++++++
> > > > >  1 file changed, 27 insertions(+)
> > > > >
> > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > > index 42924a6..41cd26f 100644
> > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > @@ -14043,6 +14043,33 @@ 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;
> > > > > +		}
> > > >
> > > > Nope. It's perfectly ok to have NV12 with a 0 offset for the uv plane, if
> > > > it's e.g. in a separate buffer object. Which is the part this series seems
> > > > to be completely missing - there's no code at all to look up (and store in
> > > > intel_framebuffer the 2nd i915_bo pointer) the 2nd buffer handle afaics.
> > > >
> > > > You should also change your igts to use 2 separate buffers, just for test
> > > > coverage.
> > > > -Daniel
> > >
> > > Hi Daniel,
> > > Agree, in general that is very well ok. But as skl hw requires uv to be after
> > > y in gtt. This can be guaranteed by having a single bo and y and uv offsets
> > > into it. Above sanity checks in i915 specific fb init call are for that reason.
> > > There are definitely ways to guarantee uv to be after y even with two
> > > separate bos (by uv remapping), but I see that is unnecessary
> > > complication and not sure value by allowing that. Or am I missing
> > > something here?
> > 
> > For a start you don't reject multiplane stuff where this isn't the case.
> > And if we indeed have the hw requirement that the gtt address for y must
> > be before the gtt address for uv (sounds strange to me, definitely need a
> > bspec reference for that) then we need to check that throughroughly:
> > Currently you could place Y after UV even in a single BO.
> 
> Hi Daniel,
> NV12 programming is documented in bspec under display planes 
> "Plane Planar YUV programming". There it talks about aux_dist
> which is the distance between y and uv planes expecting uv to be
> after y.

Bspec talks about wrap-around, which at least indicates that this might be
possible and the hw doesn't have a real restriction here. Art, can you
please double-check whether we could wrape-around PLANE_AUX_DIST with a
2s complement to avoid placement restrictions on the aux buffers?

Bspec doesn't say anything about this being required to be positive ...
-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 03/12] drm/i915: Stage scaler request for NV12 as src format

[Ville Syrjälä]

On Sun, May 17, 2015 at 10:10:56PM -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.
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 0a2e883..1ad7d13 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4499,9 +4499,11 @@ skl_update_scaler_users(
>  		rotation = DRM_ROTATE_0;
>  	}
>  
> -	need_scaling = intel_rotation_90_or_270(rotation) ?
> -		(src_h != dst_w || src_w != dst_h):
> -		(src_w != dst_w || src_h != dst_h);
> +	/* scaling is required when src dst sizes doesn't match or format is NV12 */
> +	need_scaling = (src_w != dst_w || src_h != dst_h ||
> +		(intel_rotation_90_or_270(rotation) &&
> +			(src_h != dst_w || src_w != dst_h)) ||

That doesn't look right. Maybe add a small helper function that has
these scaling checks so that we don't need to have them all in the same
if statement.

> +		(fb && fb->pixel_format == DRM_FORMAT_NV12));
>  
>  	/*
>  	 * if plane is being disabled or scaler is no more required or force detach
> @@ -4567,6 +4569,7 @@ skl_update_scaler_users(
>  		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

Re: [PATCH 04/12] drm/i915: Update format_is_yuv() to include NV12

[Ville Syrjälä]

On Sun, May 17, 2015 at 10:10:57PM -0700, Chandra Konduru wrote:
> This patch adds NV12 to format_is_yuv() function
> and made it available for both primary and sprite
> planes.
> 
> Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_drv.h    |    1 +
>  drivers/gpu/drm/i915/intel_sprite.c |    3 ++-
>  2 files changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index b18cfba..754172f 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1388,6 +1388,7 @@ int intel_sprite_set_colorkey(struct drm_device *dev, void *data,
>  bool 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 format_is_yuv(uint32_t format);

It could probably use an intel_ prefix if it's non static.

Or maybe even move it to the drm core somewhere?

>  
>  /* 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 3355d5a..80ae21f 100644
> --- a/drivers/gpu/drm/i915/intel_sprite.c
> +++ b/drivers/gpu/drm/i915/intel_sprite.c
> @@ -39,7 +39,7 @@
>  #include <drm/i915_drm.h>
>  #include "i915_drv.h"
>  
> -static bool
> +bool
>  format_is_yuv(uint32_t format)
>  {
>  	switch (format) {
> @@ -47,6 +47,7 @@ format_is_yuv(uint32_t format)
>  	case DRM_FORMAT_UYVY:
>  	case DRM_FORMAT_VYUY:
>  	case DRM_FORMAT_YVYU:
> +	case DRM_FORMAT_NV12:
>  		return true;
>  	default:
>  		return false;
> -- 
> 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 08/12] drm/i915: Add NV12 support to intel_framebuffer_init

[Konduru, Chandra]

> > > > > > This patch adds NV12 as supported format to
> > > > > > intel_framebuffer_init and performs various checks.
> > > > > >
> > > > > > Signed-off-by: Chandra Konduru <chandra.konduru@intel.com>
> > > > > > Testcase: igt/kms_nv12
> > > > > > ---
> > > > > >  drivers/gpu/drm/i915/intel_display.c |   27
> > > +++++++++++++++++++++++++++
> > > > > >  1 file changed, 27 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c
> > > > > b/drivers/gpu/drm/i915/intel_display.c
> > > > > > index 42924a6..41cd26f 100644
> > > > > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > > > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > > > > @@ -14043,6 +14043,33 @@ 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;
> > > > > > +		}
> > > > >
> > > > > Nope. It's perfectly ok to have NV12 with a 0 offset for the uv
> > > > > plane, if it's e.g. in a separate buffer object. Which is the
> > > > > part this series seems to be completely missing - there's no
> > > > > code at all to look up (and store in intel_framebuffer the 2nd i915_bo
> pointer) the 2nd buffer handle afaics.
> > > > >
> > > > > You should also change your igts to use 2 separate buffers, just
> > > > > for test coverage.
> > > > > -Daniel
> > > >
> > > > Hi Daniel,
> > > > Agree, in general that is very well ok. But as skl hw requires uv
> > > > to be after y in gtt. This can be guaranteed by having a single bo
> > > > and y and uv offsets into it. Above sanity checks in i915 specific fb init call
> are for that reason.
> > > > There are definitely ways to guarantee uv to be after y even with
> > > > two separate bos (by uv remapping), but I see that is unnecessary
> > > > complication and not sure value by allowing that. Or am I missing
> > > > something here?
> > >
> > > For a start you don't reject multiplane stuff where this isn't the case.
> > > And if we indeed have the hw requirement that the gtt address for y
> > > must be before the gtt address for uv (sounds strange to me,
> > > definitely need a bspec reference for that) then we need to check that
> throughroughly:
> > > Currently you could place Y after UV even in a single BO.
> >
> > Hi Daniel,
> > NV12 programming is documented in bspec under display planes "Plane
> > Planar YUV programming". There it talks about aux_dist which is the
> > distance between y and uv planes expecting uv to be after y.
> 
> Bspec talks about wrap-around, which at least indicates that this might be
> possible and the hw doesn't have a real restriction here. Art, can you please
> double-check whether we could wrape-around PLANE_AUX_DIST with a 2s
> complement to avoid placement restrictions on the aux buffers?
> 
> Bspec doesn't say anything about this being required to be positive ...
> -Daniel

Yeah, it isn't explicit, so to be sure a while ago checked with hw team about
uv plane positioning relative to y-plane. It was confirmed that aux_dist to be 
positive. Certainly Art can double confirm for you.

> --
> 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 03/12] drm/i915: Stage scaler request for NV12 as src format

[Konduru, Chandra]

> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -4499,9 +4499,11 @@ skl_update_scaler_users(
> >  		rotation = DRM_ROTATE_0;
> >  	}
> >
> > -	need_scaling = intel_rotation_90_or_270(rotation) ?
> > -		(src_h != dst_w || src_w != dst_h):
> > -		(src_w != dst_w || src_h != dst_h);
> > +	/* scaling is required when src dst sizes doesn't match or format is NV12
> */
> > +	need_scaling = (src_w != dst_w || src_h != dst_h ||
> > +		(intel_rotation_90_or_270(rotation) &&
> > +			(src_h != dst_w || src_w != dst_h)) ||
> 
> That doesn't look right. 
It is evaluating scaling needed by comparing
1) src != dst
2) format == nv12
Can you pls point what doesn't look right here?

> Maybe add a small helper function that has these scaling
> checks so that we don't need to have them all in the same if statement.

Thought about doing that but have to pass around 6 params to helper 
and do the same evaluation there which seems unnecessary.

>
> > +		(fb && fb->pixel_format == DRM_FORMAT_NV12));
> >

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

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

[Ville Syrjälä]

On Thu, May 21, 2015 at 04:24:00PM +0000, Konduru, Chandra wrote:
> > > --- a/drivers/gpu/drm/i915/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/intel_display.c
> > > @@ -4499,9 +4499,11 @@ skl_update_scaler_users(
> > >  		rotation = DRM_ROTATE_0;
> > >  	}
> > >
> > > -	need_scaling = intel_rotation_90_or_270(rotation) ?
> > > -		(src_h != dst_w || src_w != dst_h):
> > > -		(src_w != dst_w || src_h != dst_h);
> > > +	/* scaling is required when src dst sizes doesn't match or format is NV12
> > */
> > > +	need_scaling = (src_w != dst_w || src_h != dst_h ||
> > > +		(intel_rotation_90_or_270(rotation) &&
> > > +			(src_h != dst_w || src_w != dst_h)) ||
> > 
> > That doesn't look right. 
> It is evaluating scaling needed by comparing
> 1) src != dst
> 2) format == nv12
> Can you pls point what doesn't look right here?

It'll do these checks before even checking the rotation:
 src_w != dst_w || src_h != dst_h

> 
> > Maybe add a small helper function that has these scaling
> > checks so that we don't need to have them all in the same if statement.
> 
> Thought about doing that but have to pass around 6 params to helper 
> and do the same evaluation there which seems unnecessary.

Just figured it'll look a bit less convoluted if you split it up into a
few separate if statements. And doing that via a helper avoids 
polluting the main codepath with the details. I tend to like small
helper functions like that (maybe a bit too much sometimes :)

> 
> >
> > > +		(fb && fb->pixel_format == DRM_FORMAT_NV12));
> > >

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

Re: [PATCH 08/12] drm/i915: Add NV12 support to intel_framebuffer_init

[Runyan, Arthur J]

>From: Konduru, Chandra
>
>> >
>> > Hi Daniel,
>> > NV12 programming is documented in bspec under display planes "Plane
>> > Planar YUV programming". There it talks about aux_dist which is the
>> > distance between y and uv planes expecting uv to be after y.
>>
>> Bspec talks about wrap-around, which at least indicates that this might be
>> possible and the hw doesn't have a real restriction here. Art, can you please
>> double-check whether we could wrape-around PLANE_AUX_DIST with a 2s
>> complement to avoid placement restrictions on the aux buffers?
>>
>> Bspec doesn't say anything about this being required to be positive ...
>> -Daniel
>
>Yeah, it isn't explicit, so to be sure a while ago checked with hw team about
>uv plane positioning relative to y-plane. It was confirmed that aux_dist to be
>positive. Certainly Art can double confirm for you.
>

I'll check on it.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 08/12] drm/i915: Add NV12 support to intel_framebuffer_init

[Runyan, Arthur J]

>From: Konduru, Chandra
........
>> >
>> > Hi Daniel,
>> > NV12 programming is documented in bspec under display planes "Plane
>> > Planar YUV programming". There it talks about aux_dist which is the
>> > distance between y and uv planes expecting uv to be after y.
>>
>> Bspec talks about wrap-around, which at least indicates that this might be
>> possible and the hw doesn't have a real restriction here. Art, can you please
>> double-check whether we could wrape-around PLANE_AUX_DIST with a 2s
>> complement to avoid placement restrictions on the aux buffers?
>>
>> Bspec doesn't say anything about this being required to be positive ...
>> -Daniel
>
>Yeah, it isn't explicit, so to be sure a while ago checked with hw team about
>uv plane positioning relative to y-plane. It was confirmed that aux_dist to be
>positive. Certainly Art can double confirm for you.
>
Positive is correct.  We'll update the bspec to make it clear. 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx