[PATCH 01/10] drm/imx: ipuv3-plane: make sure x/y offsets are even in case of chroma subsampling

[PATCH 01/10] drm/imx: ipuv3-plane: make sure x/y offsets are even in case of chroma subsampling

[Philipp Zabel]

Odd x/y offsets are not allowed for horizontally/vertically chroma
subsampled planar YUV formats.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v1:
 - Improve comment
---
 drivers/gpu/drm/imx/ipuv3-plane.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index 5c34299..e1ad844 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -259,6 +259,7 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
 	struct drm_framebuffer *fb = state->fb;
 	struct drm_framebuffer *old_fb = old_state->fb;
 	unsigned long eba, ubo, vbo, old_ubo, old_vbo;
+	int hsub, vsub;
 
 	/* Ok to disable */
 	if (!fb)
@@ -372,6 +373,16 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
 
 		if (old_fb && old_fb->pitches[1] != fb->pitches[1])
 			crtc_state->mode_changed = true;
+
+		/*
+		 * The x/y offsets must be even in case of horizontal/vertical
+		 * chroma subsampling.
+		 */
+		hsub = drm_format_horz_chroma_subsampling(fb->pixel_format);
+		vsub = drm_format_vert_chroma_subsampling(fb->pixel_format);
+		if (((state->src_x >> 16) & (hsub - 1)) ||
+		    ((state->src_y >> 16) & (vsub - 1)))
+			return -EINVAL;
 	}
 
 	return 0;
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 02/10] drm/imx: ipuv3-plane: disable local alpha for planes without alpha channel

[Philipp Zabel]

Without this patch, after enabling the overlay plane with an RGBA
framebuffer, switching to a framebuffer without alpha channel would
cause the plane to vanish, since the pixel local alpha is constant
zero in that case. Disable local alpha again when setting an opaque
framebuffer.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/imx/ipuv3-plane.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index e1ad844..d5864ed 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -437,6 +437,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
 			ipu_dp_set_global_alpha(ipu_plane->dp, false, 0, false);
 			break;
 		default:
+			ipu_dp_set_global_alpha(ipu_plane->dp, true, 0, true);
 			break;
 		}
 	}
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 03/10] drm/imx: ipuv3-plane: request modeset if plane offsets changed

[Philipp Zabel]

If the framebuffer pixel format is planar YUV and unchanged, but the U
or V plane offsets change, do not return an error, but request a modeset
instead.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Acked-by: Liu Ying <gnuiyl@gmail.com>
---
 drivers/gpu/drm/imx/ipuv3-plane.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index d5864ed..737f085 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -356,13 +356,11 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
 		if ((ubo > 0xfffff8) || (vbo > 0xfffff8))
 			return -EINVAL;
 
-		if (old_fb &&
-		    (old_fb->pixel_format == DRM_FORMAT_YUV420 ||
-		     old_fb->pixel_format == DRM_FORMAT_YVU420)) {
+		if (old_fb && (fb->pixel_format == old_fb->pixel_format)) {
 			old_ubo = drm_plane_state_to_ubo(old_state);
 			old_vbo = drm_plane_state_to_vbo(old_state);
 			if (ubo != old_ubo || vbo != old_vbo)
-				return -EINVAL;
+				crtc_state->mode_changed = true;
 		}
 
 		if (fb->pitches[1] != fb->pitches[2])
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 04/10] drm/imx: ipuv3-plane: merge ipu_plane_atomic_set_base into atomic_update

[Philipp Zabel]

ipu_plane_atomic_set_base is called from ipu_plane_atomic_update in two
different places, depending on whether drm_atomic_crtc_needs_modeset is
true. Also depending on the same condition, this function does two
different things.
This patch removes the indirection by merging the relevant parts into
ipu_plane_atomic_update, making the actual code flow more obvious as a
result. Also remove the duplicate planar format comment, which is
already found in ipu_plane_atomic_check.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Acked-by: Liu Ying <gnuiyl@gmail.com>
---
 drivers/gpu/drm/imx/ipuv3-plane.c | 97 ++++++++++++++-------------------------
 1 file changed, 34 insertions(+), 63 deletions(-)

diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index 737f085..32871be 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -103,62 +103,6 @@ drm_plane_state_to_vbo(struct drm_plane_state *state)
 	       (state->src_x >> 16) / 2 - eba;
 }
 
-static void ipu_plane_atomic_set_base(struct ipu_plane *ipu_plane)
-{
-	struct drm_plane *plane = &ipu_plane->base;
-	struct drm_plane_state *state = plane->state;
-	struct drm_crtc_state *crtc_state = state->crtc->state;
-	struct drm_framebuffer *fb = state->fb;
-	unsigned long eba, ubo, vbo;
-	int active;
-
-	eba = drm_plane_state_to_eba(state);
-
-	switch (fb->pixel_format) {
-	case DRM_FORMAT_YUV420:
-	case DRM_FORMAT_YVU420:
-		if (!drm_atomic_crtc_needs_modeset(crtc_state))
-			break;
-
-		/*
-		 * Multiplanar formats have to meet the following restrictions:
-		 * - The (up to) three plane addresses are EBA, EBA+UBO, EBA+VBO
-		 * - EBA, UBO and VBO are a multiple of 8
-		 * - UBO and VBO are unsigned and not larger than 0xfffff8
-		 * - Only EBA may be changed while scanout is active
-		 * - The strides of U and V planes must be identical.
-		 */
-		ubo = drm_plane_state_to_ubo(state);
-		vbo = drm_plane_state_to_vbo(state);
-
-		if (fb->pixel_format == DRM_FORMAT_YUV420)
-			ipu_cpmem_set_yuv_planar_full(ipu_plane->ipu_ch,
-						      fb->pitches[1], ubo, vbo);
-		else
-			ipu_cpmem_set_yuv_planar_full(ipu_plane->ipu_ch,
-						      fb->pitches[1], vbo, ubo);
-
-		dev_dbg(ipu_plane->base.dev->dev,
-			"phy = %lu %lu %lu, x = %d, y = %d", eba, ubo, vbo,
-			state->src_x >> 16, state->src_y >> 16);
-		break;
-	default:
-		dev_dbg(ipu_plane->base.dev->dev, "phys = %lu, x = %d, y = %d",
-			eba, state->src_x >> 16, state->src_y >> 16);
-
-		break;
-	}
-
-	if (!drm_atomic_crtc_needs_modeset(crtc_state)) {
-		active = ipu_idmac_get_current_buffer(ipu_plane->ipu_ch);
-		ipu_cpmem_set_buffer(ipu_plane->ipu_ch, !active, eba);
-		ipu_idmac_select_buffer(ipu_plane->ipu_ch, !active);
-	} else {
-		ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 0, eba);
-		ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 1, eba);
-	}
-}
-
 void ipu_plane_put_resources(struct ipu_plane *ipu_plane)
 {
 	if (!IS_ERR_OR_NULL(ipu_plane->dp))
@@ -397,15 +341,19 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
 {
 	struct ipu_plane *ipu_plane = to_ipu_plane(plane);
 	struct drm_plane_state *state = plane->state;
+	struct drm_crtc_state *crtc_state = state->crtc->state;
+	struct drm_framebuffer *fb = state->fb;
+	unsigned long eba, ubo, vbo;
 	enum ipu_color_space ics;
+	int active;
 
-	if (old_state->fb) {
-		struct drm_crtc_state *crtc_state = state->crtc->state;
+	eba = drm_plane_state_to_eba(state);
 
-		if (!drm_atomic_crtc_needs_modeset(crtc_state)) {
-			ipu_plane_atomic_set_base(ipu_plane);
-			return;
-		}
+	if (old_state->fb && !drm_atomic_crtc_needs_modeset(crtc_state)) {
+		active = ipu_idmac_get_current_buffer(ipu_plane->ipu_ch);
+		ipu_cpmem_set_buffer(ipu_plane->ipu_ch, !active, eba);
+		ipu_idmac_select_buffer(ipu_plane->ipu_ch, !active);
+		return;
 	}
 
 	switch (ipu_plane->dp_flow) {
@@ -449,7 +397,30 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
 	ipu_cpmem_set_high_priority(ipu_plane->ipu_ch);
 	ipu_idmac_set_double_buffer(ipu_plane->ipu_ch, 1);
 	ipu_cpmem_set_stride(ipu_plane->ipu_ch, state->fb->pitches[0]);
-	ipu_plane_atomic_set_base(ipu_plane);
+	switch (fb->pixel_format) {
+	case DRM_FORMAT_YUV420:
+	case DRM_FORMAT_YVU420:
+		ubo = drm_plane_state_to_ubo(state);
+		vbo = drm_plane_state_to_vbo(state);
+
+		if (fb->pixel_format == DRM_FORMAT_YUV420)
+			ipu_cpmem_set_yuv_planar_full(ipu_plane->ipu_ch,
+						      fb->pitches[1], ubo, vbo);
+		else
+			ipu_cpmem_set_yuv_planar_full(ipu_plane->ipu_ch,
+						      fb->pitches[1], vbo, ubo);
+
+		dev_dbg(ipu_plane->base.dev->dev,
+			"phy = %lu %lu %lu, x = %d, y = %d", eba, ubo, vbo,
+			state->src_x >> 16, state->src_y >> 16);
+		break;
+	default:
+		dev_dbg(ipu_plane->base.dev->dev, "phys = %lu, x = %d, y = %d",
+			eba, state->src_x >> 16, state->src_y >> 16);
+		break;
+	}
+	ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 0, eba);
+	ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 1, eba);
 	ipu_plane_enable(ipu_plane);
 }
 
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 05/10] drm/imx: ipuv3-plane: let drm_plane_state_to_ubo/vbo handle chroma subsampling other than 4:2:0

[Philipp Zabel]

To support 4:2:2 or 4:4:4 chroma subsampling, divide the x/y offsets in
drm_plane_state_to_ubo/vbo only if necessary for the given pixel format.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/imx/ipuv3-plane.c | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index 32871be..52784cb 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -64,13 +64,14 @@ drm_plane_state_to_eba(struct drm_plane_state *state)
 {
 	struct drm_framebuffer *fb = state->fb;
 	struct drm_gem_cma_object *cma_obj;
+	int x = state->src_x >> 16;
+	int y = state->src_y >> 16;
 
 	cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
 	BUG_ON(!cma_obj);
 
-	return cma_obj->paddr + fb->offsets[0] +
-	       fb->pitches[0] * (state->src_y >> 16) +
-	       (fb->bits_per_pixel >> 3) * (state->src_x >> 16);
+	return cma_obj->paddr + fb->offsets[0] + fb->pitches[0] * y +
+	       drm_format_plane_cpp(fb->pixel_format, 0) * x;
 }
 
 static inline unsigned long
@@ -79,13 +80,17 @@ drm_plane_state_to_ubo(struct drm_plane_state *state)
 	struct drm_framebuffer *fb = state->fb;
 	struct drm_gem_cma_object *cma_obj;
 	unsigned long eba = drm_plane_state_to_eba(state);
+	int x = state->src_x >> 16;
+	int y = state->src_y >> 16;
 
 	cma_obj = drm_fb_cma_get_gem_obj(fb, 1);
 	BUG_ON(!cma_obj);
 
-	return cma_obj->paddr + fb->offsets[1] +
-	       fb->pitches[1] * (state->src_y >> 16) / 2 +
-	       (state->src_x >> 16) / 2 - eba;
+	x /= drm_format_horz_chroma_subsampling(fb->pixel_format);
+	y /= drm_format_vert_chroma_subsampling(fb->pixel_format);
+
+	return cma_obj->paddr + fb->offsets[1] + fb->pitches[1] * y +
+	       drm_format_plane_cpp(fb->pixel_format, 1) * x - eba;
 }
 
 static inline unsigned long
@@ -94,13 +99,17 @@ drm_plane_state_to_vbo(struct drm_plane_state *state)
 	struct drm_framebuffer *fb = state->fb;
 	struct drm_gem_cma_object *cma_obj;
 	unsigned long eba = drm_plane_state_to_eba(state);
+	int x = state->src_x >> 16;
+	int y = state->src_y >> 16;
 
 	cma_obj = drm_fb_cma_get_gem_obj(fb, 2);
 	BUG_ON(!cma_obj);
 
-	return cma_obj->paddr + fb->offsets[2] +
-	       fb->pitches[2] * (state->src_y >> 16) / 2 +
-	       (state->src_x >> 16) / 2 - eba;
+	x /= drm_format_horz_chroma_subsampling(fb->pixel_format);
+	y /= drm_format_vert_chroma_subsampling(fb->pixel_format);
+
+	return cma_obj->paddr + fb->offsets[2] + fb->pitches[2] * y +
+	       drm_format_plane_cpp(fb->pixel_format, 2) * x - eba;
 }
 
 void ipu_plane_put_resources(struct ipu_plane *ipu_plane)
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 06/10] gpu: ipu-cpmem: remove unused ipu_cpmem_set_yuv_planar function

[Philipp Zabel]

ipu_cpmem_set_yuv_planar_full is only used directly, remove the wrapper.

Suggested-by: Liu Ying <gnuiyl@gmail.com>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/ipu-v3/ipu-cpmem.c | 36 ------------------------------------
 include/video/imx-ipu-v3.h     |  2 --
 2 files changed, 38 deletions(-)

diff --git a/drivers/gpu/ipu-v3/ipu-cpmem.c b/drivers/gpu/ipu-v3/ipu-cpmem.c
index fcb7dc8..f3ca1d6 100644
--- a/drivers/gpu/ipu-v3/ipu-cpmem.c
+++ b/drivers/gpu/ipu-v3/ipu-cpmem.c
@@ -417,42 +417,6 @@ void ipu_cpmem_set_yuv_planar_full(struct ipuv3_channel *ch,
 }
 EXPORT_SYMBOL_GPL(ipu_cpmem_set_yuv_planar_full);
 
-void ipu_cpmem_set_yuv_planar(struct ipuv3_channel *ch,
-			      u32 pixel_format, int stride, int height)
-{
-	int fourcc, u_offset, v_offset;
-	int uv_stride = 0;
-
-	fourcc = v4l2_pix_fmt_to_drm_fourcc(pixel_format);
-	switch (fourcc) {
-	case DRM_FORMAT_YUV420:
-		uv_stride = stride / 2;
-		u_offset = stride * height;
-		v_offset = u_offset + (uv_stride * height / 2);
-		break;
-	case DRM_FORMAT_YVU420:
-		uv_stride = stride / 2;
-		v_offset = stride * height;
-		u_offset = v_offset + (uv_stride * height / 2);
-		break;
-	case DRM_FORMAT_YUV422:
-		uv_stride = stride / 2;
-		u_offset = stride * height;
-		v_offset = u_offset + (uv_stride * height);
-		break;
-	case DRM_FORMAT_NV12:
-	case DRM_FORMAT_NV16:
-		uv_stride = stride;
-		u_offset = stride * height;
-		v_offset = 0;
-		break;
-	default:
-		return;
-	}
-	ipu_cpmem_set_yuv_planar_full(ch, uv_stride, u_offset, v_offset);
-}
-EXPORT_SYMBOL_GPL(ipu_cpmem_set_yuv_planar);
-
 static const struct ipu_rgb def_xrgb_32 = {
 	.red	= { .offset = 16, .length = 8, },
 	.green	= { .offset =  8, .length = 8, },
diff --git a/include/video/imx-ipu-v3.h b/include/video/imx-ipu-v3.h
index 173073e..cc8174c 100644
--- a/include/video/imx-ipu-v3.h
+++ b/include/video/imx-ipu-v3.h
@@ -247,8 +247,6 @@ void ipu_cpmem_set_yuv_planar_full(struct ipuv3_channel *ch,
 				   unsigned int uv_stride,
 				   unsigned int u_offset,
 				   unsigned int v_offset);
-void ipu_cpmem_set_yuv_planar(struct ipuv3_channel *ch,
-			      u32 pixel_format, int stride, int height);
 int ipu_cpmem_set_fmt(struct ipuv3_channel *ch, u32 drm_fourcc);
 int ipu_cpmem_set_image(struct ipuv3_channel *ch, struct ipu_image *image);
 void ipu_cpmem_dump(struct ipuv3_channel *ch);
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 07/10] gpu: ipu-v3: add YUV 4:4:4 support

[Philipp Zabel]

The IDMAC does support reading and writing DRM_FORMAT_YUV444 and
DRM_FORMAT_YVU444.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Acked-by: Liu Ying <gnuiyl@gmail.com>
---
Changes since v1:
 - Drop change to ipu_cpmem_set_yuv_planar, which has been removed
---
 drivers/gpu/ipu-v3/ipu-common.c | 2 ++
 drivers/gpu/ipu-v3/ipu-cpmem.c  | 7 +++++++
 2 files changed, 9 insertions(+)

diff --git a/drivers/gpu/ipu-v3/ipu-common.c b/drivers/gpu/ipu-v3/ipu-common.c
index b9539f7..b7d7bd6 100644
--- a/drivers/gpu/ipu-v3/ipu-common.c
+++ b/drivers/gpu/ipu-v3/ipu-common.c
@@ -88,6 +88,8 @@ enum ipu_color_space ipu_drm_fourcc_to_colorspace(u32 drm_fourcc)
 	case DRM_FORMAT_YVU420:
 	case DRM_FORMAT_YUV422:
 	case DRM_FORMAT_YVU422:
+	case DRM_FORMAT_YUV444:
+	case DRM_FORMAT_YVU444:
 	case DRM_FORMAT_NV12:
 	case DRM_FORMAT_NV21:
 	case DRM_FORMAT_NV16:
diff --git a/drivers/gpu/ipu-v3/ipu-cpmem.c b/drivers/gpu/ipu-v3/ipu-cpmem.c
index f3ca1d6..4b2b671 100644
--- a/drivers/gpu/ipu-v3/ipu-cpmem.c
+++ b/drivers/gpu/ipu-v3/ipu-cpmem.c
@@ -554,6 +554,13 @@ int ipu_cpmem_set_fmt(struct ipuv3_channel *ch, u32 drm_fourcc)
 		/* burst size */
 		ipu_ch_param_write_field(ch, IPU_FIELD_NPB, 31);
 		break;
+	case DRM_FORMAT_YUV444:
+	case DRM_FORMAT_YVU444:
+		/* pix format */
+		ipu_ch_param_write_field(ch, IPU_FIELD_PFS, 0);
+		/* burst size */
+		ipu_ch_param_write_field(ch, IPU_FIELD_NPB, 31);
+		break;
 	case DRM_FORMAT_NV12:
 		/* pix format */
 		ipu_ch_param_write_field(ch, IPU_FIELD_PFS, 4);
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 08/10] drm/imx: ipuv3-plane: add support for YUV 4:2:2 and 4:4:4, NV12, and NV16 formats

[Philipp Zabel]

Hook up support for DRM_FORMAT_YUV422, DRM_FORMAT_YVU422,
DRM_FORMAT_YUV444, DRM_FORMAT_YVU444, DRM_FORMAT_NV12,
and DRM_FORMAT_NV16.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
Changes since v1:
 - Make UBO/VBO swap for YVU formats more obvious in ipu_plane_atomic_update
---
 drivers/gpu/drm/imx/ipuv3-plane.c | 60 ++++++++++++++++++++++++++++++---------
 1 file changed, 47 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index 52784cb..6a97e396 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -50,6 +50,12 @@ static const uint32_t ipu_plane_formats[] = {
 	DRM_FORMAT_YVYU,
 	DRM_FORMAT_YUV420,
 	DRM_FORMAT_YVU420,
+	DRM_FORMAT_YUV422,
+	DRM_FORMAT_YVU422,
+	DRM_FORMAT_YUV444,
+	DRM_FORMAT_YVU444,
+	DRM_FORMAT_NV12,
+	DRM_FORMAT_NV16,
 	DRM_FORMAT_RGB565,
 };
 
@@ -292,6 +298,10 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
 	switch (fb->pixel_format) {
 	case DRM_FORMAT_YUV420:
 	case DRM_FORMAT_YVU420:
+	case DRM_FORMAT_YUV422:
+	case DRM_FORMAT_YVU422:
+	case DRM_FORMAT_YUV444:
+	case DRM_FORMAT_YVU444:
 		/*
 		 * Multiplanar formats have to meet the following restrictions:
 		 * - The (up to) three plane addresses are EBA, EBA+UBO, EBA+VBO
@@ -300,25 +310,34 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
 		 * - Only EBA may be changed while scanout is active
 		 * - The strides of U and V planes must be identical.
 		 */
-		ubo = drm_plane_state_to_ubo(state);
 		vbo = drm_plane_state_to_vbo(state);
 
-		if ((ubo & 0x7) || (vbo & 0x7))
-			return -EINVAL;
-
-		if ((ubo > 0xfffff8) || (vbo > 0xfffff8))
+		if (vbo & 0x7 || vbo > 0xfffff8)
 			return -EINVAL;
 
 		if (old_fb && (fb->pixel_format == old_fb->pixel_format)) {
-			old_ubo = drm_plane_state_to_ubo(old_state);
 			old_vbo = drm_plane_state_to_vbo(old_state);
-			if (ubo != old_ubo || vbo != old_vbo)
+			if (vbo != old_vbo)
 				crtc_state->mode_changed = true;
 		}
 
 		if (fb->pitches[1] != fb->pitches[2])
 			return -EINVAL;
 
+		/* fall-through */
+	case DRM_FORMAT_NV12:
+	case DRM_FORMAT_NV16:
+		ubo = drm_plane_state_to_ubo(state);
+
+		if (ubo & 0x7 || ubo > 0xfffff8)
+			return -EINVAL;
+
+		if (old_fb && (fb->pixel_format == old_fb->pixel_format)) {
+			old_ubo = drm_plane_state_to_ubo(old_state);
+			if (ubo != old_ubo)
+				crtc_state->mode_changed = true;
+		}
+
 		if (fb->pitches[1] < 1 || fb->pitches[1] > 16384)
 			return -EINVAL;
 
@@ -409,20 +428,35 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
 	switch (fb->pixel_format) {
 	case DRM_FORMAT_YUV420:
 	case DRM_FORMAT_YVU420:
+	case DRM_FORMAT_YUV422:
+	case DRM_FORMAT_YVU422:
+	case DRM_FORMAT_YUV444:
+	case DRM_FORMAT_YVU444:
 		ubo = drm_plane_state_to_ubo(state);
 		vbo = drm_plane_state_to_vbo(state);
+		if (fb->pixel_format == DRM_FORMAT_YVU420 ||
+		    fb->pixel_format == DRM_FORMAT_YVU422 ||
+		    fb->pixel_format == DRM_FORMAT_YVU444)
+			swap(ubo, vbo);
 
-		if (fb->pixel_format == DRM_FORMAT_YUV420)
-			ipu_cpmem_set_yuv_planar_full(ipu_plane->ipu_ch,
-						      fb->pitches[1], ubo, vbo);
-		else
-			ipu_cpmem_set_yuv_planar_full(ipu_plane->ipu_ch,
-						      fb->pitches[1], vbo, ubo);
+		ipu_cpmem_set_yuv_planar_full(ipu_plane->ipu_ch,
+					      fb->pitches[1], ubo, vbo);
 
 		dev_dbg(ipu_plane->base.dev->dev,
 			"phy = %lu %lu %lu, x = %d, y = %d", eba, ubo, vbo,
 			state->src_x >> 16, state->src_y >> 16);
 		break;
+	case DRM_FORMAT_NV12:
+	case DRM_FORMAT_NV16:
+		ubo = drm_plane_state_to_ubo(state);
+
+		ipu_cpmem_set_yuv_planar_full(ipu_plane->ipu_ch,
+					      fb->pitches[1], ubo, ubo);
+
+		dev_dbg(ipu_plane->base.dev->dev,
+			"phy = %lu %lu, x = %d, y = %d", eba, ubo,
+			state->src_x >> 16, state->src_y >> 16);
+		break;
 	default:
 		dev_dbg(ipu_plane->base.dev->dev, "phys = %lu, x = %d, y = %d",
 			eba, state->src_x >> 16, state->src_y >> 16);
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 09/10] gpu: ipu-v3: initially clear all interrupts

[Philipp Zabel]

If we want to stop resetting the IPU in the future, masking all
interrupts before registering the irq handlers will not be enough to
avoid spurious interrupts. We also have to clear them.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Acked-by: Liu Ying <gnuiyl@gmail.com>
---
 drivers/gpu/ipu-v3/ipu-common.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/ipu-v3/ipu-common.c b/drivers/gpu/ipu-v3/ipu-common.c
index b7d7bd6..97218af 100644
--- a/drivers/gpu/ipu-v3/ipu-common.c
+++ b/drivers/gpu/ipu-v3/ipu-common.c
@@ -1286,8 +1286,11 @@ static int ipu_irq_init(struct ipu_soc *ipu)
 		return ret;
 	}
 
-	for (i = 0; i < IPU_NUM_IRQS; i += 32)
+	/* Mask and clear all interrupts */
+	for (i = 0; i < IPU_NUM_IRQS; i += 32) {
 		ipu_cm_write(ipu, 0, IPU_INT_CTRL(i / 32));
+		ipu_cm_write(ipu, ~unused[i / 32], IPU_INT_STAT(i / 32));
+	}
 
 	for (i = 0; i < IPU_NUM_IRQS; i += 32) {
 		gc = irq_get_domain_generic_chip(ipu->domain, i);
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 10/10] drm/imx: ipuv3-plane: use drm_plane_helper_check_state, clipped coordinates

[Philipp Zabel]

Use drm_plane_helper_check_state to clip raw user coordinates to crtc
bounds. This checks for full plane coverage and scaling already, so
we can drop some custom checks. Use the clipped coordinates everywhere.

Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 drivers/gpu/drm/imx/ipuv3-plane.c | 78 ++++++++++++++++++---------------------
 1 file changed, 36 insertions(+), 42 deletions(-)

diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
index 6a97e396..f0ce899 100644
--- a/drivers/gpu/drm/imx/ipuv3-plane.c
+++ b/drivers/gpu/drm/imx/ipuv3-plane.c
@@ -70,8 +70,8 @@ drm_plane_state_to_eba(struct drm_plane_state *state)
 {
 	struct drm_framebuffer *fb = state->fb;
 	struct drm_gem_cma_object *cma_obj;
-	int x = state->src_x >> 16;
-	int y = state->src_y >> 16;
+	int x = state->src.x1 >> 16;
+	int y = state->src.y1 >> 16;
 
 	cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
 	BUG_ON(!cma_obj);
@@ -86,8 +86,8 @@ drm_plane_state_to_ubo(struct drm_plane_state *state)
 	struct drm_framebuffer *fb = state->fb;
 	struct drm_gem_cma_object *cma_obj;
 	unsigned long eba = drm_plane_state_to_eba(state);
-	int x = state->src_x >> 16;
-	int y = state->src_y >> 16;
+	int x = state->src.x1 >> 16;
+	int y = state->src.y1 >> 16;
 
 	cma_obj = drm_fb_cma_get_gem_obj(fb, 1);
 	BUG_ON(!cma_obj);
@@ -105,8 +105,8 @@ drm_plane_state_to_vbo(struct drm_plane_state *state)
 	struct drm_framebuffer *fb = state->fb;
 	struct drm_gem_cma_object *cma_obj;
 	unsigned long eba = drm_plane_state_to_eba(state);
-	int x = state->src_x >> 16;
-	int y = state->src_y >> 16;
+	int x = state->src.x1 >> 16;
+	int y = state->src.y1 >> 16;
 
 	cma_obj = drm_fb_cma_get_gem_obj(fb, 2);
 	BUG_ON(!cma_obj);
@@ -218,7 +218,10 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
 	struct drm_framebuffer *fb = state->fb;
 	struct drm_framebuffer *old_fb = old_state->fb;
 	unsigned long eba, ubo, vbo, old_ubo, old_vbo;
+	bool can_position = (plane->type == DRM_PLANE_TYPE_OVERLAY);
+	struct drm_rect clip;
 	int hsub, vsub;
+	int ret;
 
 	/* Ok to disable */
 	if (!fb)
@@ -232,44 +235,35 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
 	if (WARN_ON(!crtc_state))
 		return -EINVAL;
 
+	clip.x1 = 0;
+	clip.y1 = 0;
+	clip.x2 = crtc_state->adjusted_mode.hdisplay;
+	clip.y2 = crtc_state->adjusted_mode.vdisplay;
+	ret = drm_plane_helper_check_state(state, &clip,
+					   DRM_PLANE_HELPER_NO_SCALING,
+					   DRM_PLANE_HELPER_NO_SCALING,
+					   can_position, true);
+	if (ret)
+		return ret;
+
 	/* CRTC should be enabled */
 	if (!crtc_state->enable)
 		return -EINVAL;
 
-	/* no scaling */
-	if (state->src_w >> 16 != state->crtc_w ||
-	    state->src_h >> 16 != state->crtc_h)
-		return -EINVAL;
-
 	switch (plane->type) {
 	case DRM_PLANE_TYPE_PRIMARY:
-		/* full plane doesn't support partial off screen */
-		if (state->crtc_x || state->crtc_y ||
-		    state->crtc_w != crtc_state->adjusted_mode.hdisplay ||
-		    state->crtc_h != crtc_state->adjusted_mode.vdisplay)
-			return -EINVAL;
-
 		/* full plane minimum width is 13 pixels */
-		if (state->crtc_w < 13)
+		if (drm_rect_width(&state->dst) < 13)
 			return -EINVAL;
 		break;
 	case DRM_PLANE_TYPE_OVERLAY:
-		if (state->crtc_x < 0 || state->crtc_y < 0)
-			return -EINVAL;
-
-		if (state->crtc_x + state->crtc_w >
-		    crtc_state->adjusted_mode.hdisplay)
-			return -EINVAL;
-		if (state->crtc_y + state->crtc_h >
-		    crtc_state->adjusted_mode.vdisplay)
-			return -EINVAL;
 		break;
 	default:
-		dev_warn(dev, "Unsupported plane type\n");
+		dev_warn(dev, "Unsupported plane type %d\n", plane->type);
 		return -EINVAL;
 	}
 
-	if (state->crtc_h < 2)
+	if (drm_rect_height(&state->dst) < 2)
 		return -EINVAL;
 
 	/*
@@ -279,9 +273,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
 	 * callback.  The planes will be reenabled in plane's ->atomic_update
 	 * callback.
 	 */
-	if (old_fb && (state->src_w != old_state->src_w ||
-			      state->src_h != old_state->src_h ||
-			      fb->pixel_format != old_fb->pixel_format))
+	if (old_fb && (!drm_rect_equals(&state->dst, &old_state->dst) ||
+		       fb->pixel_format != old_fb->pixel_format))
 		crtc_state->mode_changed = true;
 
 	eba = drm_plane_state_to_eba(state);
@@ -350,8 +343,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
 		 */
 		hsub = drm_format_horz_chroma_subsampling(fb->pixel_format);
 		vsub = drm_format_vert_chroma_subsampling(fb->pixel_format);
-		if (((state->src_x >> 16) & (hsub - 1)) ||
-		    ((state->src_y >> 16) & (vsub - 1)))
+		if (((state->src.x1 >> 16) & (hsub - 1)) ||
+		    ((state->src.y1 >> 16) & (vsub - 1)))
 			return -EINVAL;
 	}
 
@@ -395,8 +388,8 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
 		ics = ipu_drm_fourcc_to_colorspace(state->fb->pixel_format);
 		ipu_dp_setup_channel(ipu_plane->dp, ics,
 					IPUV3_COLORSPACE_UNKNOWN);
-		ipu_dp_set_window_pos(ipu_plane->dp, state->crtc_x,
-					state->crtc_y);
+		ipu_dp_set_window_pos(ipu_plane->dp,
+				      state->dst.x1, state->dst.y1);
 		/* Enable local alpha on partial plane */
 		switch (state->fb->pixel_format) {
 		case DRM_FORMAT_ARGB1555:
@@ -416,11 +409,12 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
 		}
 	}
 
-	ipu_dmfc_config_wait4eot(ipu_plane->dmfc, state->crtc_w);
+	ipu_dmfc_config_wait4eot(ipu_plane->dmfc, drm_rect_width(&state->dst));
 
 	ipu_cpmem_zero(ipu_plane->ipu_ch);
-	ipu_cpmem_set_resolution(ipu_plane->ipu_ch, state->src_w >> 16,
-					state->src_h >> 16);
+	ipu_cpmem_set_resolution(ipu_plane->ipu_ch,
+				 drm_rect_width(&state->src) >> 16,
+				 drm_rect_height(&state->src) >> 16);
 	ipu_cpmem_set_fmt(ipu_plane->ipu_ch, state->fb->pixel_format);
 	ipu_cpmem_set_high_priority(ipu_plane->ipu_ch);
 	ipu_idmac_set_double_buffer(ipu_plane->ipu_ch, 1);
@@ -444,7 +438,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
 
 		dev_dbg(ipu_plane->base.dev->dev,
 			"phy = %lu %lu %lu, x = %d, y = %d", eba, ubo, vbo,
-			state->src_x >> 16, state->src_y >> 16);
+			state->src.x1 >> 16, state->src.y1 >> 16);
 		break;
 	case DRM_FORMAT_NV12:
 	case DRM_FORMAT_NV16:
@@ -455,11 +449,11 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
 
 		dev_dbg(ipu_plane->base.dev->dev,
 			"phy = %lu %lu, x = %d, y = %d", eba, ubo,
-			state->src_x >> 16, state->src_y >> 16);
+			state->src.x1 >> 16, state->src.y1 >> 16);
 		break;
 	default:
 		dev_dbg(ipu_plane->base.dev->dev, "phys = %lu, x = %d, y = %d",
-			eba, state->src_x >> 16, state->src_y >> 16);
+			eba, state->src.x1 >> 16, state->src.y1 >> 16);
 		break;
 	}
 	ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 0, eba);
-- 
2.9.3

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/10] drm/imx: ipuv3-plane: use drm_plane_helper_check_state, clipped coordinates

[Ville Syrjälä]

On Wed, Oct 19, 2016 at 12:21:17PM +0200, Philipp Zabel wrote:
> Use drm_plane_helper_check_state to clip raw user coordinates to crtc
> bounds. This checks for full plane coverage and scaling already, so
> we can drop some custom checks. Use the clipped coordinates everywhere.
> 
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/gpu/drm/imx/ipuv3-plane.c | 78 ++++++++++++++++++---------------------
>  1 file changed, 36 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
> index 6a97e396..f0ce899 100644
> --- a/drivers/gpu/drm/imx/ipuv3-plane.c
> +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
> @@ -70,8 +70,8 @@ drm_plane_state_to_eba(struct drm_plane_state *state)
>  {
>  	struct drm_framebuffer *fb = state->fb;
>  	struct drm_gem_cma_object *cma_obj;
> -	int x = state->src_x >> 16;
> -	int y = state->src_y >> 16;
> +	int x = state->src.x1 >> 16;
> +	int y = state->src.y1 >> 16;
>  
>  	cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
>  	BUG_ON(!cma_obj);
> @@ -86,8 +86,8 @@ drm_plane_state_to_ubo(struct drm_plane_state *state)
>  	struct drm_framebuffer *fb = state->fb;
>  	struct drm_gem_cma_object *cma_obj;
>  	unsigned long eba = drm_plane_state_to_eba(state);
> -	int x = state->src_x >> 16;
> -	int y = state->src_y >> 16;
> +	int x = state->src.x1 >> 16;
> +	int y = state->src.y1 >> 16;
>  
>  	cma_obj = drm_fb_cma_get_gem_obj(fb, 1);
>  	BUG_ON(!cma_obj);
> @@ -105,8 +105,8 @@ drm_plane_state_to_vbo(struct drm_plane_state *state)
>  	struct drm_framebuffer *fb = state->fb;
>  	struct drm_gem_cma_object *cma_obj;
>  	unsigned long eba = drm_plane_state_to_eba(state);
> -	int x = state->src_x >> 16;
> -	int y = state->src_y >> 16;
> +	int x = state->src.x1 >> 16;
> +	int y = state->src.y1 >> 16;
>  
>  	cma_obj = drm_fb_cma_get_gem_obj(fb, 2);
>  	BUG_ON(!cma_obj);
> @@ -218,7 +218,10 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
>  	struct drm_framebuffer *fb = state->fb;
>  	struct drm_framebuffer *old_fb = old_state->fb;
>  	unsigned long eba, ubo, vbo, old_ubo, old_vbo;
> +	bool can_position = (plane->type == DRM_PLANE_TYPE_OVERLAY);
> +	struct drm_rect clip;
>  	int hsub, vsub;
> +	int ret;
>  
>  	/* Ok to disable */
>  	if (!fb)
> @@ -232,44 +235,35 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
>  	if (WARN_ON(!crtc_state))
>  		return -EINVAL;
>  
> +	clip.x1 = 0;
> +	clip.y1 = 0;
> +	clip.x2 = crtc_state->adjusted_mode.hdisplay;
> +	clip.y2 = crtc_state->adjusted_mode.vdisplay;
> +	ret = drm_plane_helper_check_state(state, &clip,
> +					   DRM_PLANE_HELPER_NO_SCALING,
> +					   DRM_PLANE_HELPER_NO_SCALING,
> +					   can_position, true);
> +	if (ret)
> +		return ret;
> +
>  	/* CRTC should be enabled */
>  	if (!crtc_state->enable)
>  		return -EINVAL;
>  
> -	/* no scaling */
> -	if (state->src_w >> 16 != state->crtc_w ||
> -	    state->src_h >> 16 != state->crtc_h)
> -		return -EINVAL;
> -
>  	switch (plane->type) {
>  	case DRM_PLANE_TYPE_PRIMARY:
> -		/* full plane doesn't support partial off screen */
> -		if (state->crtc_x || state->crtc_y ||
> -		    state->crtc_w != crtc_state->adjusted_mode.hdisplay ||
> -		    state->crtc_h != crtc_state->adjusted_mode.vdisplay)
> -			return -EINVAL;
> -
>  		/* full plane minimum width is 13 pixels */
> -		if (state->crtc_w < 13)
> +		if (drm_rect_width(&state->dst) < 13)
>  			return -EINVAL;
>  		break;
>  	case DRM_PLANE_TYPE_OVERLAY:
> -		if (state->crtc_x < 0 || state->crtc_y < 0)
> -			return -EINVAL;
> -
> -		if (state->crtc_x + state->crtc_w >
> -		    crtc_state->adjusted_mode.hdisplay)
> -			return -EINVAL;
> -		if (state->crtc_y + state->crtc_h >
> -		    crtc_state->adjusted_mode.vdisplay)
> -			return -EINVAL;
>  		break;
>  	default:
> -		dev_warn(dev, "Unsupported plane type\n");
> +		dev_warn(dev, "Unsupported plane type %d\n", plane->type);
>  		return -EINVAL;
>  	}
>  
> -	if (state->crtc_h < 2)
> +	if (drm_rect_height(&state->dst) < 2)
>  		return -EINVAL;
>  
>  	/*
> @@ -279,9 +273,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
>  	 * callback.  The planes will be reenabled in plane's ->atomic_update
>  	 * callback.
>  	 */
> -	if (old_fb && (state->src_w != old_state->src_w ||
> -			      state->src_h != old_state->src_h ||
> -			      fb->pixel_format != old_fb->pixel_format))
> +	if (old_fb && (!drm_rect_equals(&state->dst, &old_state->dst) ||

I presume you don't want to force a modeset for moving a plane around.
Using drm_rect_equals() here would end up doing that.

Otherwise it all looks quite decent to me.

> +		       fb->pixel_format != old_fb->pixel_format))
>  		crtc_state->mode_changed = true;
>  
>  	eba = drm_plane_state_to_eba(state);
> @@ -350,8 +343,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
>  		 */
>  		hsub = drm_format_horz_chroma_subsampling(fb->pixel_format);
>  		vsub = drm_format_vert_chroma_subsampling(fb->pixel_format);
> -		if (((state->src_x >> 16) & (hsub - 1)) ||
> -		    ((state->src_y >> 16) & (vsub - 1)))
> +		if (((state->src.x1 >> 16) & (hsub - 1)) ||
> +		    ((state->src.y1 >> 16) & (vsub - 1)))
>  			return -EINVAL;
>  	}
>  
> @@ -395,8 +388,8 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
>  		ics = ipu_drm_fourcc_to_colorspace(state->fb->pixel_format);
>  		ipu_dp_setup_channel(ipu_plane->dp, ics,
>  					IPUV3_COLORSPACE_UNKNOWN);
> -		ipu_dp_set_window_pos(ipu_plane->dp, state->crtc_x,
> -					state->crtc_y);
> +		ipu_dp_set_window_pos(ipu_plane->dp,
> +				      state->dst.x1, state->dst.y1);
>  		/* Enable local alpha on partial plane */
>  		switch (state->fb->pixel_format) {
>  		case DRM_FORMAT_ARGB1555:
> @@ -416,11 +409,12 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
>  		}
>  	}
>  
> -	ipu_dmfc_config_wait4eot(ipu_plane->dmfc, state->crtc_w);
> +	ipu_dmfc_config_wait4eot(ipu_plane->dmfc, drm_rect_width(&state->dst));
>  
>  	ipu_cpmem_zero(ipu_plane->ipu_ch);
> -	ipu_cpmem_set_resolution(ipu_plane->ipu_ch, state->src_w >> 16,
> -					state->src_h >> 16);
> +	ipu_cpmem_set_resolution(ipu_plane->ipu_ch,
> +				 drm_rect_width(&state->src) >> 16,
> +				 drm_rect_height(&state->src) >> 16);
>  	ipu_cpmem_set_fmt(ipu_plane->ipu_ch, state->fb->pixel_format);
>  	ipu_cpmem_set_high_priority(ipu_plane->ipu_ch);
>  	ipu_idmac_set_double_buffer(ipu_plane->ipu_ch, 1);
> @@ -444,7 +438,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
>  
>  		dev_dbg(ipu_plane->base.dev->dev,
>  			"phy = %lu %lu %lu, x = %d, y = %d", eba, ubo, vbo,
> -			state->src_x >> 16, state->src_y >> 16);
> +			state->src.x1 >> 16, state->src.y1 >> 16);
>  		break;
>  	case DRM_FORMAT_NV12:
>  	case DRM_FORMAT_NV16:
> @@ -455,11 +449,11 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
>  
>  		dev_dbg(ipu_plane->base.dev->dev,
>  			"phy = %lu %lu, x = %d, y = %d", eba, ubo,
> -			state->src_x >> 16, state->src_y >> 16);
> +			state->src.x1 >> 16, state->src.y1 >> 16);
>  		break;
>  	default:
>  		dev_dbg(ipu_plane->base.dev->dev, "phys = %lu, x = %d, y = %d",
> -			eba, state->src_x >> 16, state->src_y >> 16);
> +			eba, state->src.x1 >> 16, state->src.y1 >> 16);
>  		break;
>  	}
>  	ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 0, eba);
> -- 
> 2.9.3

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/10] drm/imx: ipuv3-plane: use drm_plane_helper_check_state, clipped coordinates

[Philipp Zabel]

Am Mittwoch, den 19.10.2016, 13:31 +0300 schrieb Ville Syrjälä:
> On Wed, Oct 19, 2016 at 12:21:17PM +0200, Philipp Zabel wrote:
> > Use drm_plane_helper_check_state to clip raw user coordinates to crtc
> > bounds. This checks for full plane coverage and scaling already, so
> > we can drop some custom checks. Use the clipped coordinates everywhere.
> > 
> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  drivers/gpu/drm/imx/ipuv3-plane.c | 78 ++++++++++++++++++---------------------
> >  1 file changed, 36 insertions(+), 42 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
> > index 6a97e396..f0ce899 100644
> > --- a/drivers/gpu/drm/imx/ipuv3-plane.c
> > +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
> > @@ -70,8 +70,8 @@ drm_plane_state_to_eba(struct drm_plane_state *state)
> >  {
> >  	struct drm_framebuffer *fb = state->fb;
> >  	struct drm_gem_cma_object *cma_obj;
> > -	int x = state->src_x >> 16;
> > -	int y = state->src_y >> 16;
> > +	int x = state->src.x1 >> 16;
> > +	int y = state->src.y1 >> 16;
> >  
> >  	cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
> >  	BUG_ON(!cma_obj);
> > @@ -86,8 +86,8 @@ drm_plane_state_to_ubo(struct drm_plane_state *state)
> >  	struct drm_framebuffer *fb = state->fb;
> >  	struct drm_gem_cma_object *cma_obj;
> >  	unsigned long eba = drm_plane_state_to_eba(state);
> > -	int x = state->src_x >> 16;
> > -	int y = state->src_y >> 16;
> > +	int x = state->src.x1 >> 16;
> > +	int y = state->src.y1 >> 16;
> >  
> >  	cma_obj = drm_fb_cma_get_gem_obj(fb, 1);
> >  	BUG_ON(!cma_obj);
> > @@ -105,8 +105,8 @@ drm_plane_state_to_vbo(struct drm_plane_state *state)
> >  	struct drm_framebuffer *fb = state->fb;
> >  	struct drm_gem_cma_object *cma_obj;
> >  	unsigned long eba = drm_plane_state_to_eba(state);
> > -	int x = state->src_x >> 16;
> > -	int y = state->src_y >> 16;
> > +	int x = state->src.x1 >> 16;
> > +	int y = state->src.y1 >> 16;
> >  
> >  	cma_obj = drm_fb_cma_get_gem_obj(fb, 2);
> >  	BUG_ON(!cma_obj);
> > @@ -218,7 +218,10 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
> >  	struct drm_framebuffer *fb = state->fb;
> >  	struct drm_framebuffer *old_fb = old_state->fb;
> >  	unsigned long eba, ubo, vbo, old_ubo, old_vbo;
> > +	bool can_position = (plane->type == DRM_PLANE_TYPE_OVERLAY);
> > +	struct drm_rect clip;
> >  	int hsub, vsub;
> > +	int ret;
> >  
> >  	/* Ok to disable */
> >  	if (!fb)
> > @@ -232,44 +235,35 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
> >  	if (WARN_ON(!crtc_state))
> >  		return -EINVAL;
> >  
> > +	clip.x1 = 0;
> > +	clip.y1 = 0;
> > +	clip.x2 = crtc_state->adjusted_mode.hdisplay;
> > +	clip.y2 = crtc_state->adjusted_mode.vdisplay;
> > +	ret = drm_plane_helper_check_state(state, &clip,
> > +					   DRM_PLANE_HELPER_NO_SCALING,
> > +					   DRM_PLANE_HELPER_NO_SCALING,
> > +					   can_position, true);
> > +	if (ret)
> > +		return ret;
> > +
> >  	/* CRTC should be enabled */
> >  	if (!crtc_state->enable)
> >  		return -EINVAL;
> >  
> > -	/* no scaling */
> > -	if (state->src_w >> 16 != state->crtc_w ||
> > -	    state->src_h >> 16 != state->crtc_h)
> > -		return -EINVAL;
> > -
> >  	switch (plane->type) {
> >  	case DRM_PLANE_TYPE_PRIMARY:
> > -		/* full plane doesn't support partial off screen */
> > -		if (state->crtc_x || state->crtc_y ||
> > -		    state->crtc_w != crtc_state->adjusted_mode.hdisplay ||
> > -		    state->crtc_h != crtc_state->adjusted_mode.vdisplay)
> > -			return -EINVAL;
> > -
> >  		/* full plane minimum width is 13 pixels */
> > -		if (state->crtc_w < 13)
> > +		if (drm_rect_width(&state->dst) < 13)
> >  			return -EINVAL;
> >  		break;
> >  	case DRM_PLANE_TYPE_OVERLAY:
> > -		if (state->crtc_x < 0 || state->crtc_y < 0)
> > -			return -EINVAL;
> > -
> > -		if (state->crtc_x + state->crtc_w >
> > -		    crtc_state->adjusted_mode.hdisplay)
> > -			return -EINVAL;
> > -		if (state->crtc_y + state->crtc_h >
> > -		    crtc_state->adjusted_mode.vdisplay)
> > -			return -EINVAL;
> >  		break;
> >  	default:
> > -		dev_warn(dev, "Unsupported plane type\n");
> > +		dev_warn(dev, "Unsupported plane type %d\n", plane->type);
> >  		return -EINVAL;
> >  	}
> >  
> > -	if (state->crtc_h < 2)
> > +	if (drm_rect_height(&state->dst) < 2)
> >  		return -EINVAL;
> >  
> >  	/*
> > @@ -279,9 +273,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
> >  	 * callback.  The planes will be reenabled in plane's ->atomic_update
> >  	 * callback.
> >  	 */
> > -	if (old_fb && (state->src_w != old_state->src_w ||
> > -			      state->src_h != old_state->src_h ||
> > -			      fb->pixel_format != old_fb->pixel_format))
> > +	if (old_fb && (!drm_rect_equals(&state->dst, &old_state->dst) ||
> 
> I presume you don't want to force a modeset for moving a plane around.
> Using drm_rect_equals() here would end up doing that.
> 
> Otherwise it all looks quite decent to me.

Thanks, I'll use drm_rect_width / drm_rect_height as I should have.

regards
Philipp

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 01/10] drm/imx: ipuv3-plane: make sure x/y offsets are even in case of chroma subsampling

[Ying Liu]

On Wed, Oct 19, 2016 at 6:21 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Odd x/y offsets are not allowed for horizontally/vertically chroma
> subsampled planar YUV formats.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

Acked-by: Liu Ying <gnuiyl@gmail.com>

> ---
> Changes since v1:
>  - Improve comment
> ---
>  drivers/gpu/drm/imx/ipuv3-plane.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
> index 5c34299..e1ad844 100644
> --- a/drivers/gpu/drm/imx/ipuv3-plane.c
> +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
> @@ -259,6 +259,7 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
>         struct drm_framebuffer *fb = state->fb;
>         struct drm_framebuffer *old_fb = old_state->fb;
>         unsigned long eba, ubo, vbo, old_ubo, old_vbo;
> +       int hsub, vsub;
>
>         /* Ok to disable */
>         if (!fb)
> @@ -372,6 +373,16 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
>
>                 if (old_fb && old_fb->pitches[1] != fb->pitches[1])
>                         crtc_state->mode_changed = true;
> +
> +               /*
> +                * The x/y offsets must be even in case of horizontal/vertical
> +                * chroma subsampling.
> +                */
> +               hsub = drm_format_horz_chroma_subsampling(fb->pixel_format);
> +               vsub = drm_format_vert_chroma_subsampling(fb->pixel_format);
> +               if (((state->src_x >> 16) & (hsub - 1)) ||
> +                   ((state->src_y >> 16) & (vsub - 1)))
> +                       return -EINVAL;
>         }
>
>         return 0;
> --
> 2.9.3
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 05/10] drm/imx: ipuv3-plane: let drm_plane_state_to_ubo/vbo handle chroma subsampling other than 4:2:0

[Ying Liu]

On Wed, Oct 19, 2016 at 6:21 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> To support 4:2:2 or 4:4:4 chroma subsampling, divide the x/y offsets in
> drm_plane_state_to_ubo/vbo only if necessary for the given pixel format.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

Acked-by: Liu Ying <gnuiyl@gmail.com>

> ---
>  drivers/gpu/drm/imx/ipuv3-plane.c | 27 ++++++++++++++++++---------
>  1 file changed, 18 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
> index 32871be..52784cb 100644
> --- a/drivers/gpu/drm/imx/ipuv3-plane.c
> +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
> @@ -64,13 +64,14 @@ drm_plane_state_to_eba(struct drm_plane_state *state)
>  {
>         struct drm_framebuffer *fb = state->fb;
>         struct drm_gem_cma_object *cma_obj;
> +       int x = state->src_x >> 16;
> +       int y = state->src_y >> 16;
>
>         cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
>         BUG_ON(!cma_obj);
>
> -       return cma_obj->paddr + fb->offsets[0] +
> -              fb->pitches[0] * (state->src_y >> 16) +
> -              (fb->bits_per_pixel >> 3) * (state->src_x >> 16);
> +       return cma_obj->paddr + fb->offsets[0] + fb->pitches[0] * y +
> +              drm_format_plane_cpp(fb->pixel_format, 0) * x;
>  }
>
>  static inline unsigned long
> @@ -79,13 +80,17 @@ drm_plane_state_to_ubo(struct drm_plane_state *state)
>         struct drm_framebuffer *fb = state->fb;
>         struct drm_gem_cma_object *cma_obj;
>         unsigned long eba = drm_plane_state_to_eba(state);
> +       int x = state->src_x >> 16;
> +       int y = state->src_y >> 16;
>
>         cma_obj = drm_fb_cma_get_gem_obj(fb, 1);
>         BUG_ON(!cma_obj);
>
> -       return cma_obj->paddr + fb->offsets[1] +
> -              fb->pitches[1] * (state->src_y >> 16) / 2 +
> -              (state->src_x >> 16) / 2 - eba;
> +       x /= drm_format_horz_chroma_subsampling(fb->pixel_format);
> +       y /= drm_format_vert_chroma_subsampling(fb->pixel_format);
> +
> +       return cma_obj->paddr + fb->offsets[1] + fb->pitches[1] * y +
> +              drm_format_plane_cpp(fb->pixel_format, 1) * x - eba;
>  }
>
>  static inline unsigned long
> @@ -94,13 +99,17 @@ drm_plane_state_to_vbo(struct drm_plane_state *state)
>         struct drm_framebuffer *fb = state->fb;
>         struct drm_gem_cma_object *cma_obj;
>         unsigned long eba = drm_plane_state_to_eba(state);
> +       int x = state->src_x >> 16;
> +       int y = state->src_y >> 16;
>
>         cma_obj = drm_fb_cma_get_gem_obj(fb, 2);
>         BUG_ON(!cma_obj);
>
> -       return cma_obj->paddr + fb->offsets[2] +
> -              fb->pitches[2] * (state->src_y >> 16) / 2 +
> -              (state->src_x >> 16) / 2 - eba;
> +       x /= drm_format_horz_chroma_subsampling(fb->pixel_format);
> +       y /= drm_format_vert_chroma_subsampling(fb->pixel_format);
> +
> +       return cma_obj->paddr + fb->offsets[2] + fb->pitches[2] * y +
> +              drm_format_plane_cpp(fb->pixel_format, 2) * x - eba;
>  }
>
>  void ipu_plane_put_resources(struct ipu_plane *ipu_plane)
> --
> 2.9.3
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 06/10] gpu: ipu-cpmem: remove unused ipu_cpmem_set_yuv_planar function

[Ying Liu]

On Wed, Oct 19, 2016 at 6:21 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> ipu_cpmem_set_yuv_planar_full is only used directly, remove the wrapper.
>
> Suggested-by: Liu Ying <gnuiyl@gmail.com>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

Acked-by: Liu Ying <gnuiyl@gmail.com>

> ---
>  drivers/gpu/ipu-v3/ipu-cpmem.c | 36 ------------------------------------
>  include/video/imx-ipu-v3.h     |  2 --
>  2 files changed, 38 deletions(-)
>
> diff --git a/drivers/gpu/ipu-v3/ipu-cpmem.c b/drivers/gpu/ipu-v3/ipu-cpmem.c
> index fcb7dc8..f3ca1d6 100644
> --- a/drivers/gpu/ipu-v3/ipu-cpmem.c
> +++ b/drivers/gpu/ipu-v3/ipu-cpmem.c
> @@ -417,42 +417,6 @@ void ipu_cpmem_set_yuv_planar_full(struct ipuv3_channel *ch,
>  }
>  EXPORT_SYMBOL_GPL(ipu_cpmem_set_yuv_planar_full);
>
> -void ipu_cpmem_set_yuv_planar(struct ipuv3_channel *ch,
> -                             u32 pixel_format, int stride, int height)
> -{
> -       int fourcc, u_offset, v_offset;
> -       int uv_stride = 0;
> -
> -       fourcc = v4l2_pix_fmt_to_drm_fourcc(pixel_format);
> -       switch (fourcc) {
> -       case DRM_FORMAT_YUV420:
> -               uv_stride = stride / 2;
> -               u_offset = stride * height;
> -               v_offset = u_offset + (uv_stride * height / 2);
> -               break;
> -       case DRM_FORMAT_YVU420:
> -               uv_stride = stride / 2;
> -               v_offset = stride * height;
> -               u_offset = v_offset + (uv_stride * height / 2);
> -               break;
> -       case DRM_FORMAT_YUV422:
> -               uv_stride = stride / 2;
> -               u_offset = stride * height;
> -               v_offset = u_offset + (uv_stride * height);
> -               break;
> -       case DRM_FORMAT_NV12:
> -       case DRM_FORMAT_NV16:
> -               uv_stride = stride;
> -               u_offset = stride * height;
> -               v_offset = 0;
> -               break;
> -       default:
> -               return;
> -       }
> -       ipu_cpmem_set_yuv_planar_full(ch, uv_stride, u_offset, v_offset);
> -}
> -EXPORT_SYMBOL_GPL(ipu_cpmem_set_yuv_planar);
> -
>  static const struct ipu_rgb def_xrgb_32 = {
>         .red    = { .offset = 16, .length = 8, },
>         .green  = { .offset =  8, .length = 8, },
> diff --git a/include/video/imx-ipu-v3.h b/include/video/imx-ipu-v3.h
> index 173073e..cc8174c 100644
> --- a/include/video/imx-ipu-v3.h
> +++ b/include/video/imx-ipu-v3.h
> @@ -247,8 +247,6 @@ void ipu_cpmem_set_yuv_planar_full(struct ipuv3_channel *ch,
>                                    unsigned int uv_stride,
>                                    unsigned int u_offset,
>                                    unsigned int v_offset);
> -void ipu_cpmem_set_yuv_planar(struct ipuv3_channel *ch,
> -                             u32 pixel_format, int stride, int height);
>  int ipu_cpmem_set_fmt(struct ipuv3_channel *ch, u32 drm_fourcc);
>  int ipu_cpmem_set_image(struct ipuv3_channel *ch, struct ipu_image *image);
>  void ipu_cpmem_dump(struct ipuv3_channel *ch);
> --
> 2.9.3
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 08/10] drm/imx: ipuv3-plane: add support for YUV 4:2:2 and 4:4:4, NV12, and NV16 formats

[Ying Liu]

On Wed, Oct 19, 2016 at 6:21 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Hook up support for DRM_FORMAT_YUV422, DRM_FORMAT_YVU422,
> DRM_FORMAT_YUV444, DRM_FORMAT_YVU444, DRM_FORMAT_NV12,
> and DRM_FORMAT_NV16.
>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>

Acked-by: Liu Ying <gnuiyl@gmail.com>

> ---
> Changes since v1:
>  - Make UBO/VBO swap for YVU formats more obvious in ipu_plane_atomic_update
> ---
>  drivers/gpu/drm/imx/ipuv3-plane.c | 60 ++++++++++++++++++++++++++++++---------
>  1 file changed, 47 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
> index 52784cb..6a97e396 100644
> --- a/drivers/gpu/drm/imx/ipuv3-plane.c
> +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
> @@ -50,6 +50,12 @@ static const uint32_t ipu_plane_formats[] = {
>         DRM_FORMAT_YVYU,
>         DRM_FORMAT_YUV420,
>         DRM_FORMAT_YVU420,
> +       DRM_FORMAT_YUV422,
> +       DRM_FORMAT_YVU422,
> +       DRM_FORMAT_YUV444,
> +       DRM_FORMAT_YVU444,
> +       DRM_FORMAT_NV12,
> +       DRM_FORMAT_NV16,
>         DRM_FORMAT_RGB565,
>  };
>
> @@ -292,6 +298,10 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
>         switch (fb->pixel_format) {
>         case DRM_FORMAT_YUV420:
>         case DRM_FORMAT_YVU420:
> +       case DRM_FORMAT_YUV422:
> +       case DRM_FORMAT_YVU422:
> +       case DRM_FORMAT_YUV444:
> +       case DRM_FORMAT_YVU444:
>                 /*
>                  * Multiplanar formats have to meet the following restrictions:
>                  * - The (up to) three plane addresses are EBA, EBA+UBO, EBA+VBO
> @@ -300,25 +310,34 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
>                  * - Only EBA may be changed while scanout is active
>                  * - The strides of U and V planes must be identical.
>                  */
> -               ubo = drm_plane_state_to_ubo(state);
>                 vbo = drm_plane_state_to_vbo(state);
>
> -               if ((ubo & 0x7) || (vbo & 0x7))
> -                       return -EINVAL;
> -
> -               if ((ubo > 0xfffff8) || (vbo > 0xfffff8))
> +               if (vbo & 0x7 || vbo > 0xfffff8)
>                         return -EINVAL;
>
>                 if (old_fb && (fb->pixel_format == old_fb->pixel_format)) {
> -                       old_ubo = drm_plane_state_to_ubo(old_state);
>                         old_vbo = drm_plane_state_to_vbo(old_state);
> -                       if (ubo != old_ubo || vbo != old_vbo)
> +                       if (vbo != old_vbo)
>                                 crtc_state->mode_changed = true;
>                 }
>
>                 if (fb->pitches[1] != fb->pitches[2])
>                         return -EINVAL;
>
> +               /* fall-through */
> +       case DRM_FORMAT_NV12:
> +       case DRM_FORMAT_NV16:
> +               ubo = drm_plane_state_to_ubo(state);
> +
> +               if (ubo & 0x7 || ubo > 0xfffff8)
> +                       return -EINVAL;
> +
> +               if (old_fb && (fb->pixel_format == old_fb->pixel_format)) {
> +                       old_ubo = drm_plane_state_to_ubo(old_state);
> +                       if (ubo != old_ubo)
> +                               crtc_state->mode_changed = true;
> +               }
> +
>                 if (fb->pitches[1] < 1 || fb->pitches[1] > 16384)
>                         return -EINVAL;
>
> @@ -409,20 +428,35 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
>         switch (fb->pixel_format) {
>         case DRM_FORMAT_YUV420:
>         case DRM_FORMAT_YVU420:
> +       case DRM_FORMAT_YUV422:
> +       case DRM_FORMAT_YVU422:
> +       case DRM_FORMAT_YUV444:
> +       case DRM_FORMAT_YVU444:
>                 ubo = drm_plane_state_to_ubo(state);
>                 vbo = drm_plane_state_to_vbo(state);
> +               if (fb->pixel_format == DRM_FORMAT_YVU420 ||
> +                   fb->pixel_format == DRM_FORMAT_YVU422 ||
> +                   fb->pixel_format == DRM_FORMAT_YVU444)
> +                       swap(ubo, vbo);
>
> -               if (fb->pixel_format == DRM_FORMAT_YUV420)
> -                       ipu_cpmem_set_yuv_planar_full(ipu_plane->ipu_ch,
> -                                                     fb->pitches[1], ubo, vbo);
> -               else
> -                       ipu_cpmem_set_yuv_planar_full(ipu_plane->ipu_ch,
> -                                                     fb->pitches[1], vbo, ubo);
> +               ipu_cpmem_set_yuv_planar_full(ipu_plane->ipu_ch,
> +                                             fb->pitches[1], ubo, vbo);
>
>                 dev_dbg(ipu_plane->base.dev->dev,
>                         "phy = %lu %lu %lu, x = %d, y = %d", eba, ubo, vbo,
>                         state->src_x >> 16, state->src_y >> 16);
>                 break;
> +       case DRM_FORMAT_NV12:
> +       case DRM_FORMAT_NV16:
> +               ubo = drm_plane_state_to_ubo(state);
> +
> +               ipu_cpmem_set_yuv_planar_full(ipu_plane->ipu_ch,
> +                                             fb->pitches[1], ubo, ubo);
> +
> +               dev_dbg(ipu_plane->base.dev->dev,
> +                       "phy = %lu %lu, x = %d, y = %d", eba, ubo,
> +                       state->src_x >> 16, state->src_y >> 16);
> +               break;
>         default:
>                 dev_dbg(ipu_plane->base.dev->dev, "phys = %lu, x = %d, y = %d",
>                         eba, state->src_x >> 16, state->src_y >> 16);
> --
> 2.9.3
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/10] drm/imx: ipuv3-plane: use drm_plane_helper_check_state, clipped coordinates

[Ying Liu]

On Wed, Oct 19, 2016 at 6:21 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Use drm_plane_helper_check_state to clip raw user coordinates to crtc
> bounds. This checks for full plane coverage and scaling already, so
> we can drop some custom checks. Use the clipped coordinates everywhere.
>
> Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> ---
>  drivers/gpu/drm/imx/ipuv3-plane.c | 78 ++++++++++++++++++---------------------
>  1 file changed, 36 insertions(+), 42 deletions(-)
>
> diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
> index 6a97e396..f0ce899 100644
> --- a/drivers/gpu/drm/imx/ipuv3-plane.c
> +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
> @@ -70,8 +70,8 @@ drm_plane_state_to_eba(struct drm_plane_state *state)
>  {
>         struct drm_framebuffer *fb = state->fb;
>         struct drm_gem_cma_object *cma_obj;
> -       int x = state->src_x >> 16;
> -       int y = state->src_y >> 16;
> +       int x = state->src.x1 >> 16;
> +       int y = state->src.y1 >> 16;
>
>         cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
>         BUG_ON(!cma_obj);
> @@ -86,8 +86,8 @@ drm_plane_state_to_ubo(struct drm_plane_state *state)
>         struct drm_framebuffer *fb = state->fb;
>         struct drm_gem_cma_object *cma_obj;
>         unsigned long eba = drm_plane_state_to_eba(state);
> -       int x = state->src_x >> 16;
> -       int y = state->src_y >> 16;
> +       int x = state->src.x1 >> 16;
> +       int y = state->src.y1 >> 16;
>
>         cma_obj = drm_fb_cma_get_gem_obj(fb, 1);
>         BUG_ON(!cma_obj);
> @@ -105,8 +105,8 @@ drm_plane_state_to_vbo(struct drm_plane_state *state)
>         struct drm_framebuffer *fb = state->fb;
>         struct drm_gem_cma_object *cma_obj;
>         unsigned long eba = drm_plane_state_to_eba(state);
> -       int x = state->src_x >> 16;
> -       int y = state->src_y >> 16;
> +       int x = state->src.x1 >> 16;
> +       int y = state->src.y1 >> 16;
>
>         cma_obj = drm_fb_cma_get_gem_obj(fb, 2);
>         BUG_ON(!cma_obj);
> @@ -218,7 +218,10 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
>         struct drm_framebuffer *fb = state->fb;
>         struct drm_framebuffer *old_fb = old_state->fb;
>         unsigned long eba, ubo, vbo, old_ubo, old_vbo;
> +       bool can_position = (plane->type == DRM_PLANE_TYPE_OVERLAY);
> +       struct drm_rect clip;
>         int hsub, vsub;
> +       int ret;
>
>         /* Ok to disable */
>         if (!fb)
> @@ -232,44 +235,35 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
>         if (WARN_ON(!crtc_state))
>                 return -EINVAL;
>
> +       clip.x1 = 0;
> +       clip.y1 = 0;
> +       clip.x2 = crtc_state->adjusted_mode.hdisplay;
> +       clip.y2 = crtc_state->adjusted_mode.vdisplay;
> +       ret = drm_plane_helper_check_state(state, &clip,
> +                                          DRM_PLANE_HELPER_NO_SCALING,
> +                                          DRM_PLANE_HELPER_NO_SCALING,
> +                                          can_position, true);
> +       if (ret)
> +               return ret;

Does the clip thing potentially change the user's request by force?
For example, the user request an unreasonable big resolution.

> +
>         /* CRTC should be enabled */
>         if (!crtc_state->enable)
>                 return -EINVAL;
>
> -       /* no scaling */
> -       if (state->src_w >> 16 != state->crtc_w ||
> -           state->src_h >> 16 != state->crtc_h)
> -               return -EINVAL;
> -
>         switch (plane->type) {
>         case DRM_PLANE_TYPE_PRIMARY:
> -               /* full plane doesn't support partial off screen */
> -               if (state->crtc_x || state->crtc_y ||
> -                   state->crtc_w != crtc_state->adjusted_mode.hdisplay ||
> -                   state->crtc_h != crtc_state->adjusted_mode.vdisplay)
> -                       return -EINVAL;
> -
>                 /* full plane minimum width is 13 pixels */
> -               if (state->crtc_w < 13)
> +               if (drm_rect_width(&state->dst) < 13)
>                         return -EINVAL;
>                 break;
>         case DRM_PLANE_TYPE_OVERLAY:
> -               if (state->crtc_x < 0 || state->crtc_y < 0)
> -                       return -EINVAL;

How does drm_plane_helper_check_state() cover this check?

> -
> -               if (state->crtc_x + state->crtc_w >
> -                   crtc_state->adjusted_mode.hdisplay)
> -                       return -EINVAL;
> -               if (state->crtc_y + state->crtc_h >
> -                   crtc_state->adjusted_mode.vdisplay)
> -                       return -EINVAL;

And these?

Regards,
Liu Ying

>                 break;
>         default:
> -               dev_warn(dev, "Unsupported plane type\n");
> +               dev_warn(dev, "Unsupported plane type %d\n", plane->type);
>                 return -EINVAL;
>         }
>
> -       if (state->crtc_h < 2)
> +       if (drm_rect_height(&state->dst) < 2)
>                 return -EINVAL;
>
>         /*
> @@ -279,9 +273,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
>          * callback.  The planes will be reenabled in plane's ->atomic_update
>          * callback.
>          */
> -       if (old_fb && (state->src_w != old_state->src_w ||
> -                             state->src_h != old_state->src_h ||
> -                             fb->pixel_format != old_fb->pixel_format))
> +       if (old_fb && (!drm_rect_equals(&state->dst, &old_state->dst) ||
> +                      fb->pixel_format != old_fb->pixel_format))
>                 crtc_state->mode_changed = true;
>
>         eba = drm_plane_state_to_eba(state);
> @@ -350,8 +343,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
>                  */
>                 hsub = drm_format_horz_chroma_subsampling(fb->pixel_format);
>                 vsub = drm_format_vert_chroma_subsampling(fb->pixel_format);
> -               if (((state->src_x >> 16) & (hsub - 1)) ||
> -                   ((state->src_y >> 16) & (vsub - 1)))
> +               if (((state->src.x1 >> 16) & (hsub - 1)) ||
> +                   ((state->src.y1 >> 16) & (vsub - 1)))
>                         return -EINVAL;
>         }
>
> @@ -395,8 +388,8 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
>                 ics = ipu_drm_fourcc_to_colorspace(state->fb->pixel_format);
>                 ipu_dp_setup_channel(ipu_plane->dp, ics,
>                                         IPUV3_COLORSPACE_UNKNOWN);
> -               ipu_dp_set_window_pos(ipu_plane->dp, state->crtc_x,
> -                                       state->crtc_y);
> +               ipu_dp_set_window_pos(ipu_plane->dp,
> +                                     state->dst.x1, state->dst.y1);
>                 /* Enable local alpha on partial plane */
>                 switch (state->fb->pixel_format) {
>                 case DRM_FORMAT_ARGB1555:
> @@ -416,11 +409,12 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
>                 }
>         }
>
> -       ipu_dmfc_config_wait4eot(ipu_plane->dmfc, state->crtc_w);
> +       ipu_dmfc_config_wait4eot(ipu_plane->dmfc, drm_rect_width(&state->dst));
>
>         ipu_cpmem_zero(ipu_plane->ipu_ch);
> -       ipu_cpmem_set_resolution(ipu_plane->ipu_ch, state->src_w >> 16,
> -                                       state->src_h >> 16);
> +       ipu_cpmem_set_resolution(ipu_plane->ipu_ch,
> +                                drm_rect_width(&state->src) >> 16,
> +                                drm_rect_height(&state->src) >> 16);
>         ipu_cpmem_set_fmt(ipu_plane->ipu_ch, state->fb->pixel_format);
>         ipu_cpmem_set_high_priority(ipu_plane->ipu_ch);
>         ipu_idmac_set_double_buffer(ipu_plane->ipu_ch, 1);
> @@ -444,7 +438,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
>
>                 dev_dbg(ipu_plane->base.dev->dev,
>                         "phy = %lu %lu %lu, x = %d, y = %d", eba, ubo, vbo,
> -                       state->src_x >> 16, state->src_y >> 16);
> +                       state->src.x1 >> 16, state->src.y1 >> 16);
>                 break;
>         case DRM_FORMAT_NV12:
>         case DRM_FORMAT_NV16:
> @@ -455,11 +449,11 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
>
>                 dev_dbg(ipu_plane->base.dev->dev,
>                         "phy = %lu %lu, x = %d, y = %d", eba, ubo,
> -                       state->src_x >> 16, state->src_y >> 16);
> +                       state->src.x1 >> 16, state->src.y1 >> 16);
>                 break;
>         default:
>                 dev_dbg(ipu_plane->base.dev->dev, "phys = %lu, x = %d, y = %d",
> -                       eba, state->src_x >> 16, state->src_y >> 16);
> +                       eba, state->src.x1 >> 16, state->src.y1 >> 16);
>                 break;
>         }
>         ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 0, eba);
> --
> 2.9.3
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/10] drm/imx: ipuv3-plane: use drm_plane_helper_check_state, clipped coordinates

[Ville Syrjälä]

On Thu, Oct 20, 2016 at 03:53:46PM +0800, Ying Liu wrote:
> On Wed, Oct 19, 2016 at 6:21 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > Use drm_plane_helper_check_state to clip raw user coordinates to crtc
> > bounds. This checks for full plane coverage and scaling already, so
> > we can drop some custom checks. Use the clipped coordinates everywhere.
> >
> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > ---
> >  drivers/gpu/drm/imx/ipuv3-plane.c | 78 ++++++++++++++++++---------------------
> >  1 file changed, 36 insertions(+), 42 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
> > index 6a97e396..f0ce899 100644
> > --- a/drivers/gpu/drm/imx/ipuv3-plane.c
> > +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
> > @@ -70,8 +70,8 @@ drm_plane_state_to_eba(struct drm_plane_state *state)
> >  {
> >         struct drm_framebuffer *fb = state->fb;
> >         struct drm_gem_cma_object *cma_obj;
> > -       int x = state->src_x >> 16;
> > -       int y = state->src_y >> 16;
> > +       int x = state->src.x1 >> 16;
> > +       int y = state->src.y1 >> 16;
> >
> >         cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
> >         BUG_ON(!cma_obj);
> > @@ -86,8 +86,8 @@ drm_plane_state_to_ubo(struct drm_plane_state *state)
> >         struct drm_framebuffer *fb = state->fb;
> >         struct drm_gem_cma_object *cma_obj;
> >         unsigned long eba = drm_plane_state_to_eba(state);
> > -       int x = state->src_x >> 16;
> > -       int y = state->src_y >> 16;
> > +       int x = state->src.x1 >> 16;
> > +       int y = state->src.y1 >> 16;
> >
> >         cma_obj = drm_fb_cma_get_gem_obj(fb, 1);
> >         BUG_ON(!cma_obj);
> > @@ -105,8 +105,8 @@ drm_plane_state_to_vbo(struct drm_plane_state *state)
> >         struct drm_framebuffer *fb = state->fb;
> >         struct drm_gem_cma_object *cma_obj;
> >         unsigned long eba = drm_plane_state_to_eba(state);
> > -       int x = state->src_x >> 16;
> > -       int y = state->src_y >> 16;
> > +       int x = state->src.x1 >> 16;
> > +       int y = state->src.y1 >> 16;
> >
> >         cma_obj = drm_fb_cma_get_gem_obj(fb, 2);
> >         BUG_ON(!cma_obj);
> > @@ -218,7 +218,10 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
> >         struct drm_framebuffer *fb = state->fb;
> >         struct drm_framebuffer *old_fb = old_state->fb;
> >         unsigned long eba, ubo, vbo, old_ubo, old_vbo;
> > +       bool can_position = (plane->type == DRM_PLANE_TYPE_OVERLAY);
> > +       struct drm_rect clip;
> >         int hsub, vsub;
> > +       int ret;
> >
> >         /* Ok to disable */
> >         if (!fb)
> > @@ -232,44 +235,35 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
> >         if (WARN_ON(!crtc_state))
> >                 return -EINVAL;
> >
> > +       clip.x1 = 0;
> > +       clip.y1 = 0;
> > +       clip.x2 = crtc_state->adjusted_mode.hdisplay;
> > +       clip.y2 = crtc_state->adjusted_mode.vdisplay;
> > +       ret = drm_plane_helper_check_state(state, &clip,
> > +                                          DRM_PLANE_HELPER_NO_SCALING,
> > +                                          DRM_PLANE_HELPER_NO_SCALING,
> > +                                          can_position, true);
> > +       if (ret)
> > +               return ret;
> 
> Does the clip thing potentially change the user's request by force?
> For example, the user request an unreasonable big resolution.

The user is allowed to ask for destination coordinates extending outside
the crtc dimensions. This will chop off the parts that aren't visible,
and it will chop off the corresponding areas of the source as well.

> 
> > +
> >         /* CRTC should be enabled */
> >         if (!crtc_state->enable)
> >                 return -EINVAL;
> >
> > -       /* no scaling */
> > -       if (state->src_w >> 16 != state->crtc_w ||
> > -           state->src_h >> 16 != state->crtc_h)
> > -               return -EINVAL;
> > -
> >         switch (plane->type) {
> >         case DRM_PLANE_TYPE_PRIMARY:
> > -               /* full plane doesn't support partial off screen */
> > -               if (state->crtc_x || state->crtc_y ||
> > -                   state->crtc_w != crtc_state->adjusted_mode.hdisplay ||
> > -                   state->crtc_h != crtc_state->adjusted_mode.vdisplay)
> > -                       return -EINVAL;
> > -
> >                 /* full plane minimum width is 13 pixels */
> > -               if (state->crtc_w < 13)
> > +               if (drm_rect_width(&state->dst) < 13)
> >                         return -EINVAL;
> >                 break;
> >         case DRM_PLANE_TYPE_OVERLAY:
> > -               if (state->crtc_x < 0 || state->crtc_y < 0)
> > -                       return -EINVAL;
> 
> How does drm_plane_helper_check_state() cover this check?
> 
> > -
> > -               if (state->crtc_x + state->crtc_w >
> > -                   crtc_state->adjusted_mode.hdisplay)
> > -                       return -EINVAL;
> > -               if (state->crtc_y + state->crtc_h >
> > -                   crtc_state->adjusted_mode.vdisplay)
> > -                       return -EINVAL;
> 
> And these?
> 
> Regards,
> Liu Ying
> 
> >                 break;
> >         default:
> > -               dev_warn(dev, "Unsupported plane type\n");
> > +               dev_warn(dev, "Unsupported plane type %d\n", plane->type);
> >                 return -EINVAL;
> >         }
> >
> > -       if (state->crtc_h < 2)
> > +       if (drm_rect_height(&state->dst) < 2)
> >                 return -EINVAL;
> >
> >         /*
> > @@ -279,9 +273,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
> >          * callback.  The planes will be reenabled in plane's ->atomic_update
> >          * callback.
> >          */
> > -       if (old_fb && (state->src_w != old_state->src_w ||
> > -                             state->src_h != old_state->src_h ||
> > -                             fb->pixel_format != old_fb->pixel_format))
> > +       if (old_fb && (!drm_rect_equals(&state->dst, &old_state->dst) ||
> > +                      fb->pixel_format != old_fb->pixel_format))
> >                 crtc_state->mode_changed = true;
> >
> >         eba = drm_plane_state_to_eba(state);
> > @@ -350,8 +343,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
> >                  */
> >                 hsub = drm_format_horz_chroma_subsampling(fb->pixel_format);
> >                 vsub = drm_format_vert_chroma_subsampling(fb->pixel_format);
> > -               if (((state->src_x >> 16) & (hsub - 1)) ||
> > -                   ((state->src_y >> 16) & (vsub - 1)))
> > +               if (((state->src.x1 >> 16) & (hsub - 1)) ||
> > +                   ((state->src.y1 >> 16) & (vsub - 1)))
> >                         return -EINVAL;
> >         }
> >
> > @@ -395,8 +388,8 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
> >                 ics = ipu_drm_fourcc_to_colorspace(state->fb->pixel_format);
> >                 ipu_dp_setup_channel(ipu_plane->dp, ics,
> >                                         IPUV3_COLORSPACE_UNKNOWN);
> > -               ipu_dp_set_window_pos(ipu_plane->dp, state->crtc_x,
> > -                                       state->crtc_y);
> > +               ipu_dp_set_window_pos(ipu_plane->dp,
> > +                                     state->dst.x1, state->dst.y1);
> >                 /* Enable local alpha on partial plane */
> >                 switch (state->fb->pixel_format) {
> >                 case DRM_FORMAT_ARGB1555:
> > @@ -416,11 +409,12 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
> >                 }
> >         }
> >
> > -       ipu_dmfc_config_wait4eot(ipu_plane->dmfc, state->crtc_w);
> > +       ipu_dmfc_config_wait4eot(ipu_plane->dmfc, drm_rect_width(&state->dst));
> >
> >         ipu_cpmem_zero(ipu_plane->ipu_ch);
> > -       ipu_cpmem_set_resolution(ipu_plane->ipu_ch, state->src_w >> 16,
> > -                                       state->src_h >> 16);
> > +       ipu_cpmem_set_resolution(ipu_plane->ipu_ch,
> > +                                drm_rect_width(&state->src) >> 16,
> > +                                drm_rect_height(&state->src) >> 16);
> >         ipu_cpmem_set_fmt(ipu_plane->ipu_ch, state->fb->pixel_format);
> >         ipu_cpmem_set_high_priority(ipu_plane->ipu_ch);
> >         ipu_idmac_set_double_buffer(ipu_plane->ipu_ch, 1);
> > @@ -444,7 +438,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
> >
> >                 dev_dbg(ipu_plane->base.dev->dev,
> >                         "phy = %lu %lu %lu, x = %d, y = %d", eba, ubo, vbo,
> > -                       state->src_x >> 16, state->src_y >> 16);
> > +                       state->src.x1 >> 16, state->src.y1 >> 16);
> >                 break;
> >         case DRM_FORMAT_NV12:
> >         case DRM_FORMAT_NV16:
> > @@ -455,11 +449,11 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
> >
> >                 dev_dbg(ipu_plane->base.dev->dev,
> >                         "phy = %lu %lu, x = %d, y = %d", eba, ubo,
> > -                       state->src_x >> 16, state->src_y >> 16);
> > +                       state->src.x1 >> 16, state->src.y1 >> 16);
> >                 break;
> >         default:
> >                 dev_dbg(ipu_plane->base.dev->dev, "phys = %lu, x = %d, y = %d",
> > -                       eba, state->src_x >> 16, state->src_y >> 16);
> > +                       eba, state->src.x1 >> 16, state->src.y1 >> 16);
> >                 break;
> >         }
> >         ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 0, eba);
> > --
> > 2.9.3
> >

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/10] drm/imx: ipuv3-plane: use drm_plane_helper_check_state, clipped coordinates

[Ying Liu]

On Thu, Oct 20, 2016 at 4:41 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Thu, Oct 20, 2016 at 03:53:46PM +0800, Ying Liu wrote:
>> On Wed, Oct 19, 2016 at 6:21 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
>> > Use drm_plane_helper_check_state to clip raw user coordinates to crtc
>> > bounds. This checks for full plane coverage and scaling already, so
>> > we can drop some custom checks. Use the clipped coordinates everywhere.
>> >
>> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
>> > ---
>> >  drivers/gpu/drm/imx/ipuv3-plane.c | 78 ++++++++++++++++++---------------------
>> >  1 file changed, 36 insertions(+), 42 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
>> > index 6a97e396..f0ce899 100644
>> > --- a/drivers/gpu/drm/imx/ipuv3-plane.c
>> > +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
>> > @@ -70,8 +70,8 @@ drm_plane_state_to_eba(struct drm_plane_state *state)
>> >  {
>> >         struct drm_framebuffer *fb = state->fb;
>> >         struct drm_gem_cma_object *cma_obj;
>> > -       int x = state->src_x >> 16;
>> > -       int y = state->src_y >> 16;
>> > +       int x = state->src.x1 >> 16;
>> > +       int y = state->src.y1 >> 16;
>> >
>> >         cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
>> >         BUG_ON(!cma_obj);
>> > @@ -86,8 +86,8 @@ drm_plane_state_to_ubo(struct drm_plane_state *state)
>> >         struct drm_framebuffer *fb = state->fb;
>> >         struct drm_gem_cma_object *cma_obj;
>> >         unsigned long eba = drm_plane_state_to_eba(state);
>> > -       int x = state->src_x >> 16;
>> > -       int y = state->src_y >> 16;
>> > +       int x = state->src.x1 >> 16;
>> > +       int y = state->src.y1 >> 16;
>> >
>> >         cma_obj = drm_fb_cma_get_gem_obj(fb, 1);
>> >         BUG_ON(!cma_obj);
>> > @@ -105,8 +105,8 @@ drm_plane_state_to_vbo(struct drm_plane_state *state)
>> >         struct drm_framebuffer *fb = state->fb;
>> >         struct drm_gem_cma_object *cma_obj;
>> >         unsigned long eba = drm_plane_state_to_eba(state);
>> > -       int x = state->src_x >> 16;
>> > -       int y = state->src_y >> 16;
>> > +       int x = state->src.x1 >> 16;
>> > +       int y = state->src.y1 >> 16;
>> >
>> >         cma_obj = drm_fb_cma_get_gem_obj(fb, 2);
>> >         BUG_ON(!cma_obj);
>> > @@ -218,7 +218,10 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
>> >         struct drm_framebuffer *fb = state->fb;
>> >         struct drm_framebuffer *old_fb = old_state->fb;
>> >         unsigned long eba, ubo, vbo, old_ubo, old_vbo;
>> > +       bool can_position = (plane->type == DRM_PLANE_TYPE_OVERLAY);
>> > +       struct drm_rect clip;
>> >         int hsub, vsub;
>> > +       int ret;
>> >
>> >         /* Ok to disable */
>> >         if (!fb)
>> > @@ -232,44 +235,35 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
>> >         if (WARN_ON(!crtc_state))
>> >                 return -EINVAL;
>> >
>> > +       clip.x1 = 0;
>> > +       clip.y1 = 0;
>> > +       clip.x2 = crtc_state->adjusted_mode.hdisplay;
>> > +       clip.y2 = crtc_state->adjusted_mode.vdisplay;
>> > +       ret = drm_plane_helper_check_state(state, &clip,
>> > +                                          DRM_PLANE_HELPER_NO_SCALING,
>> > +                                          DRM_PLANE_HELPER_NO_SCALING,
>> > +                                          can_position, true);
>> > +       if (ret)
>> > +               return ret;
>>
>> Does the clip thing potentially change the user's request by force?
>> For example, the user request an unreasonable big resolution.
>
> The user is allowed to ask for destination coordinates extending outside
> the crtc dimensions. This will chop off the parts that aren't visible,
> and it will chop off the corresponding areas of the source as well.

How about returning -EINVAL in this case which stands for
an atomic check failure?

>
>>
>> > +
>> >         /* CRTC should be enabled */
>> >         if (!crtc_state->enable)
>> >                 return -EINVAL;
>> >
>> > -       /* no scaling */
>> > -       if (state->src_w >> 16 != state->crtc_w ||
>> > -           state->src_h >> 16 != state->crtc_h)
>> > -               return -EINVAL;
>> > -
>> >         switch (plane->type) {
>> >         case DRM_PLANE_TYPE_PRIMARY:
>> > -               /* full plane doesn't support partial off screen */
>> > -               if (state->crtc_x || state->crtc_y ||
>> > -                   state->crtc_w != crtc_state->adjusted_mode.hdisplay ||
>> > -                   state->crtc_h != crtc_state->adjusted_mode.vdisplay)
>> > -                       return -EINVAL;
>> > -
>> >                 /* full plane minimum width is 13 pixels */
>> > -               if (state->crtc_w < 13)
>> > +               if (drm_rect_width(&state->dst) < 13)
>> >                         return -EINVAL;
>> >                 break;
>> >         case DRM_PLANE_TYPE_OVERLAY:
>> > -               if (state->crtc_x < 0 || state->crtc_y < 0)
>> > -                       return -EINVAL;
>>
>> How does drm_plane_helper_check_state() cover this check?
>>
>> > -
>> > -               if (state->crtc_x + state->crtc_w >
>> > -                   crtc_state->adjusted_mode.hdisplay)
>> > -                       return -EINVAL;
>> > -               if (state->crtc_y + state->crtc_h >
>> > -                   crtc_state->adjusted_mode.vdisplay)
>> > -                       return -EINVAL;
>>
>> And these?
>>
>> Regards,
>> Liu Ying
>>
>> >                 break;
>> >         default:
>> > -               dev_warn(dev, "Unsupported plane type\n");
>> > +               dev_warn(dev, "Unsupported plane type %d\n", plane->type);
>> >                 return -EINVAL;
>> >         }
>> >
>> > -       if (state->crtc_h < 2)
>> > +       if (drm_rect_height(&state->dst) < 2)
>> >                 return -EINVAL;
>> >
>> >         /*
>> > @@ -279,9 +273,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
>> >          * callback.  The planes will be reenabled in plane's ->atomic_update
>> >          * callback.
>> >          */
>> > -       if (old_fb && (state->src_w != old_state->src_w ||
>> > -                             state->src_h != old_state->src_h ||
>> > -                             fb->pixel_format != old_fb->pixel_format))
>> > +       if (old_fb && (!drm_rect_equals(&state->dst, &old_state->dst) ||
>> > +                      fb->pixel_format != old_fb->pixel_format))
>> >                 crtc_state->mode_changed = true;
>> >
>> >         eba = drm_plane_state_to_eba(state);
>> > @@ -350,8 +343,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
>> >                  */
>> >                 hsub = drm_format_horz_chroma_subsampling(fb->pixel_format);
>> >                 vsub = drm_format_vert_chroma_subsampling(fb->pixel_format);
>> > -               if (((state->src_x >> 16) & (hsub - 1)) ||
>> > -                   ((state->src_y >> 16) & (vsub - 1)))
>> > +               if (((state->src.x1 >> 16) & (hsub - 1)) ||
>> > +                   ((state->src.y1 >> 16) & (vsub - 1)))
>> >                         return -EINVAL;
>> >         }
>> >
>> > @@ -395,8 +388,8 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
>> >                 ics = ipu_drm_fourcc_to_colorspace(state->fb->pixel_format);
>> >                 ipu_dp_setup_channel(ipu_plane->dp, ics,
>> >                                         IPUV3_COLORSPACE_UNKNOWN);
>> > -               ipu_dp_set_window_pos(ipu_plane->dp, state->crtc_x,
>> > -                                       state->crtc_y);
>> > +               ipu_dp_set_window_pos(ipu_plane->dp,
>> > +                                     state->dst.x1, state->dst.y1);
>> >                 /* Enable local alpha on partial plane */
>> >                 switch (state->fb->pixel_format) {
>> >                 case DRM_FORMAT_ARGB1555:
>> > @@ -416,11 +409,12 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
>> >                 }
>> >         }
>> >
>> > -       ipu_dmfc_config_wait4eot(ipu_plane->dmfc, state->crtc_w);
>> > +       ipu_dmfc_config_wait4eot(ipu_plane->dmfc, drm_rect_width(&state->dst));
>> >
>> >         ipu_cpmem_zero(ipu_plane->ipu_ch);
>> > -       ipu_cpmem_set_resolution(ipu_plane->ipu_ch, state->src_w >> 16,
>> > -                                       state->src_h >> 16);
>> > +       ipu_cpmem_set_resolution(ipu_plane->ipu_ch,
>> > +                                drm_rect_width(&state->src) >> 16,
>> > +                                drm_rect_height(&state->src) >> 16);
>> >         ipu_cpmem_set_fmt(ipu_plane->ipu_ch, state->fb->pixel_format);
>> >         ipu_cpmem_set_high_priority(ipu_plane->ipu_ch);
>> >         ipu_idmac_set_double_buffer(ipu_plane->ipu_ch, 1);
>> > @@ -444,7 +438,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
>> >
>> >                 dev_dbg(ipu_plane->base.dev->dev,
>> >                         "phy = %lu %lu %lu, x = %d, y = %d", eba, ubo, vbo,
>> > -                       state->src_x >> 16, state->src_y >> 16);
>> > +                       state->src.x1 >> 16, state->src.y1 >> 16);
>> >                 break;
>> >         case DRM_FORMAT_NV12:
>> >         case DRM_FORMAT_NV16:
>> > @@ -455,11 +449,11 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
>> >
>> >                 dev_dbg(ipu_plane->base.dev->dev,
>> >                         "phy = %lu %lu, x = %d, y = %d", eba, ubo,
>> > -                       state->src_x >> 16, state->src_y >> 16);
>> > +                       state->src.x1 >> 16, state->src.y1 >> 16);
>> >                 break;
>> >         default:
>> >                 dev_dbg(ipu_plane->base.dev->dev, "phys = %lu, x = %d, y = %d",
>> > -                       eba, state->src_x >> 16, state->src_y >> 16);
>> > +                       eba, state->src.x1 >> 16, state->src.y1 >> 16);
>> >                 break;
>> >         }
>> >         ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 0, eba);
>> > --
>> > 2.9.3
>> >
>
> --
> Ville Syrjälä
> Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/10] drm/imx: ipuv3-plane: use drm_plane_helper_check_state, clipped coordinates

[Ville Syrjälä]

On Thu, Oct 20, 2016 at 04:51:30PM +0800, Ying Liu wrote:
> On Thu, Oct 20, 2016 at 4:41 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > On Thu, Oct 20, 2016 at 03:53:46PM +0800, Ying Liu wrote:
> >> On Wed, Oct 19, 2016 at 6:21 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> >> > Use drm_plane_helper_check_state to clip raw user coordinates to crtc
> >> > bounds. This checks for full plane coverage and scaling already, so
> >> > we can drop some custom checks. Use the clipped coordinates everywhere.
> >> >
> >> > Suggested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >> > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> >> > ---
> >> >  drivers/gpu/drm/imx/ipuv3-plane.c | 78 ++++++++++++++++++---------------------
> >> >  1 file changed, 36 insertions(+), 42 deletions(-)
> >> >
> >> > diff --git a/drivers/gpu/drm/imx/ipuv3-plane.c b/drivers/gpu/drm/imx/ipuv3-plane.c
> >> > index 6a97e396..f0ce899 100644
> >> > --- a/drivers/gpu/drm/imx/ipuv3-plane.c
> >> > +++ b/drivers/gpu/drm/imx/ipuv3-plane.c
> >> > @@ -70,8 +70,8 @@ drm_plane_state_to_eba(struct drm_plane_state *state)
> >> >  {
> >> >         struct drm_framebuffer *fb = state->fb;
> >> >         struct drm_gem_cma_object *cma_obj;
> >> > -       int x = state->src_x >> 16;
> >> > -       int y = state->src_y >> 16;
> >> > +       int x = state->src.x1 >> 16;
> >> > +       int y = state->src.y1 >> 16;
> >> >
> >> >         cma_obj = drm_fb_cma_get_gem_obj(fb, 0);
> >> >         BUG_ON(!cma_obj);
> >> > @@ -86,8 +86,8 @@ drm_plane_state_to_ubo(struct drm_plane_state *state)
> >> >         struct drm_framebuffer *fb = state->fb;
> >> >         struct drm_gem_cma_object *cma_obj;
> >> >         unsigned long eba = drm_plane_state_to_eba(state);
> >> > -       int x = state->src_x >> 16;
> >> > -       int y = state->src_y >> 16;
> >> > +       int x = state->src.x1 >> 16;
> >> > +       int y = state->src.y1 >> 16;
> >> >
> >> >         cma_obj = drm_fb_cma_get_gem_obj(fb, 1);
> >> >         BUG_ON(!cma_obj);
> >> > @@ -105,8 +105,8 @@ drm_plane_state_to_vbo(struct drm_plane_state *state)
> >> >         struct drm_framebuffer *fb = state->fb;
> >> >         struct drm_gem_cma_object *cma_obj;
> >> >         unsigned long eba = drm_plane_state_to_eba(state);
> >> > -       int x = state->src_x >> 16;
> >> > -       int y = state->src_y >> 16;
> >> > +       int x = state->src.x1 >> 16;
> >> > +       int y = state->src.y1 >> 16;
> >> >
> >> >         cma_obj = drm_fb_cma_get_gem_obj(fb, 2);
> >> >         BUG_ON(!cma_obj);
> >> > @@ -218,7 +218,10 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
> >> >         struct drm_framebuffer *fb = state->fb;
> >> >         struct drm_framebuffer *old_fb = old_state->fb;
> >> >         unsigned long eba, ubo, vbo, old_ubo, old_vbo;
> >> > +       bool can_position = (plane->type == DRM_PLANE_TYPE_OVERLAY);
> >> > +       struct drm_rect clip;
> >> >         int hsub, vsub;
> >> > +       int ret;
> >> >
> >> >         /* Ok to disable */
> >> >         if (!fb)
> >> > @@ -232,44 +235,35 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
> >> >         if (WARN_ON(!crtc_state))
> >> >                 return -EINVAL;
> >> >
> >> > +       clip.x1 = 0;
> >> > +       clip.y1 = 0;
> >> > +       clip.x2 = crtc_state->adjusted_mode.hdisplay;
> >> > +       clip.y2 = crtc_state->adjusted_mode.vdisplay;
> >> > +       ret = drm_plane_helper_check_state(state, &clip,
> >> > +                                          DRM_PLANE_HELPER_NO_SCALING,
> >> > +                                          DRM_PLANE_HELPER_NO_SCALING,
> >> > +                                          can_position, true);
> >> > +       if (ret)
> >> > +               return ret;
> >>
> >> Does the clip thing potentially change the user's request by force?
> >> For example, the user request an unreasonable big resolution.
> >
> > The user is allowed to ask for destination coordinates extending outside
> > the crtc dimensions. This will chop off the parts that aren't visible,
> > and it will chop off the corresponding areas of the source as well.
> 
> How about returning -EINVAL in this case which stands for
> an atomic check failure?

That's not the expected behaviour.

> 
> >
> >>
> >> > +
> >> >         /* CRTC should be enabled */
> >> >         if (!crtc_state->enable)
> >> >                 return -EINVAL;
> >> >
> >> > -       /* no scaling */
> >> > -       if (state->src_w >> 16 != state->crtc_w ||
> >> > -           state->src_h >> 16 != state->crtc_h)
> >> > -               return -EINVAL;
> >> > -
> >> >         switch (plane->type) {
> >> >         case DRM_PLANE_TYPE_PRIMARY:
> >> > -               /* full plane doesn't support partial off screen */
> >> > -               if (state->crtc_x || state->crtc_y ||
> >> > -                   state->crtc_w != crtc_state->adjusted_mode.hdisplay ||
> >> > -                   state->crtc_h != crtc_state->adjusted_mode.vdisplay)
> >> > -                       return -EINVAL;
> >> > -
> >> >                 /* full plane minimum width is 13 pixels */
> >> > -               if (state->crtc_w < 13)
> >> > +               if (drm_rect_width(&state->dst) < 13)
> >> >                         return -EINVAL;
> >> >                 break;
> >> >         case DRM_PLANE_TYPE_OVERLAY:
> >> > -               if (state->crtc_x < 0 || state->crtc_y < 0)
> >> > -                       return -EINVAL;
> >>
> >> How does drm_plane_helper_check_state() cover this check?
> >>
> >> > -
> >> > -               if (state->crtc_x + state->crtc_w >
> >> > -                   crtc_state->adjusted_mode.hdisplay)
> >> > -                       return -EINVAL;
> >> > -               if (state->crtc_y + state->crtc_h >
> >> > -                   crtc_state->adjusted_mode.vdisplay)
> >> > -                       return -EINVAL;
> >>
> >> And these?
> >>
> >> Regards,
> >> Liu Ying
> >>
> >> >                 break;
> >> >         default:
> >> > -               dev_warn(dev, "Unsupported plane type\n");
> >> > +               dev_warn(dev, "Unsupported plane type %d\n", plane->type);
> >> >                 return -EINVAL;
> >> >         }
> >> >
> >> > -       if (state->crtc_h < 2)
> >> > +       if (drm_rect_height(&state->dst) < 2)
> >> >                 return -EINVAL;
> >> >
> >> >         /*
> >> > @@ -279,9 +273,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
> >> >          * callback.  The planes will be reenabled in plane's ->atomic_update
> >> >          * callback.
> >> >          */
> >> > -       if (old_fb && (state->src_w != old_state->src_w ||
> >> > -                             state->src_h != old_state->src_h ||
> >> > -                             fb->pixel_format != old_fb->pixel_format))
> >> > +       if (old_fb && (!drm_rect_equals(&state->dst, &old_state->dst) ||
> >> > +                      fb->pixel_format != old_fb->pixel_format))
> >> >                 crtc_state->mode_changed = true;
> >> >
> >> >         eba = drm_plane_state_to_eba(state);
> >> > @@ -350,8 +343,8 @@ static int ipu_plane_atomic_check(struct drm_plane *plane,
> >> >                  */
> >> >                 hsub = drm_format_horz_chroma_subsampling(fb->pixel_format);
> >> >                 vsub = drm_format_vert_chroma_subsampling(fb->pixel_format);
> >> > -               if (((state->src_x >> 16) & (hsub - 1)) ||
> >> > -                   ((state->src_y >> 16) & (vsub - 1)))
> >> > +               if (((state->src.x1 >> 16) & (hsub - 1)) ||
> >> > +                   ((state->src.y1 >> 16) & (vsub - 1)))
> >> >                         return -EINVAL;
> >> >         }
> >> >
> >> > @@ -395,8 +388,8 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
> >> >                 ics = ipu_drm_fourcc_to_colorspace(state->fb->pixel_format);
> >> >                 ipu_dp_setup_channel(ipu_plane->dp, ics,
> >> >                                         IPUV3_COLORSPACE_UNKNOWN);
> >> > -               ipu_dp_set_window_pos(ipu_plane->dp, state->crtc_x,
> >> > -                                       state->crtc_y);
> >> > +               ipu_dp_set_window_pos(ipu_plane->dp,
> >> > +                                     state->dst.x1, state->dst.y1);
> >> >                 /* Enable local alpha on partial plane */
> >> >                 switch (state->fb->pixel_format) {
> >> >                 case DRM_FORMAT_ARGB1555:
> >> > @@ -416,11 +409,12 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
> >> >                 }
> >> >         }
> >> >
> >> > -       ipu_dmfc_config_wait4eot(ipu_plane->dmfc, state->crtc_w);
> >> > +       ipu_dmfc_config_wait4eot(ipu_plane->dmfc, drm_rect_width(&state->dst));
> >> >
> >> >         ipu_cpmem_zero(ipu_plane->ipu_ch);
> >> > -       ipu_cpmem_set_resolution(ipu_plane->ipu_ch, state->src_w >> 16,
> >> > -                                       state->src_h >> 16);
> >> > +       ipu_cpmem_set_resolution(ipu_plane->ipu_ch,
> >> > +                                drm_rect_width(&state->src) >> 16,
> >> > +                                drm_rect_height(&state->src) >> 16);
> >> >         ipu_cpmem_set_fmt(ipu_plane->ipu_ch, state->fb->pixel_format);
> >> >         ipu_cpmem_set_high_priority(ipu_plane->ipu_ch);
> >> >         ipu_idmac_set_double_buffer(ipu_plane->ipu_ch, 1);
> >> > @@ -444,7 +438,7 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
> >> >
> >> >                 dev_dbg(ipu_plane->base.dev->dev,
> >> >                         "phy = %lu %lu %lu, x = %d, y = %d", eba, ubo, vbo,
> >> > -                       state->src_x >> 16, state->src_y >> 16);
> >> > +                       state->src.x1 >> 16, state->src.y1 >> 16);
> >> >                 break;
> >> >         case DRM_FORMAT_NV12:
> >> >         case DRM_FORMAT_NV16:
> >> > @@ -455,11 +449,11 @@ static void ipu_plane_atomic_update(struct drm_plane *plane,
> >> >
> >> >                 dev_dbg(ipu_plane->base.dev->dev,
> >> >                         "phy = %lu %lu, x = %d, y = %d", eba, ubo,
> >> > -                       state->src_x >> 16, state->src_y >> 16);
> >> > +                       state->src.x1 >> 16, state->src.y1 >> 16);
> >> >                 break;
> >> >         default:
> >> >                 dev_dbg(ipu_plane->base.dev->dev, "phys = %lu, x = %d, y = %d",
> >> > -                       eba, state->src_x >> 16, state->src_y >> 16);
> >> > +                       eba, state->src.x1 >> 16, state->src.y1 >> 16);
> >> >                 break;
> >> >         }
> >> >         ipu_cpmem_set_buffer(ipu_plane->ipu_ch, 0, eba);
> >> > --
> >> > 2.9.3
> >> >
> >
> > --
> > Ville Syrjälä
> > Intel OTC

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/10] drm/imx: ipuv3-plane: use drm_plane_helper_check_state, clipped coordinates

[Philipp Zabel]

Am Donnerstag, den 20.10.2016, 16:51 +0800 schrieb Ying Liu:
> >> Does the clip thing potentially change the user's request by force?
> >> For example, the user request an unreasonable big resolution.
> >
> > The user is allowed to ask for destination coordinates extending outside
> > the crtc dimensions. This will chop off the parts that aren't visible,
> > and it will chop off the corresponding areas of the source as well.
> 
> How about returning -EINVAL in this case which stands for
> an atomic check failure?

Say the user requests to display a 640x480+0,0 source framebuffer at
destination offset -320,0 on a 320x240 screen, unscaled. The expectation
would be to see the upper right quarter of the framebuffer on the
screen, at least if the hardware was actually able to position overlays
partially offscreen.
If we can also fulfill that expectation by clipping the source rectangle
to 320,240+320,0 and changing the destination rectangle to 320x240+0,0,
why should -EINVAL be returned?

regards
Philipp

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/10] drm/imx: ipuv3-plane: use drm_plane_helper_check_state, clipped coordinates

[Ying Liu]

On Thu, Oct 20, 2016 at 9:29 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Am Donnerstag, den 20.10.2016, 16:51 +0800 schrieb Ying Liu:
>> >> Does the clip thing potentially change the user's request by force?
>> >> For example, the user request an unreasonable big resolution.
>> >
>> > The user is allowed to ask for destination coordinates extending outside
>> > the crtc dimensions. This will chop off the parts that aren't visible,
>> > and it will chop off the corresponding areas of the source as well.
>>
>> How about returning -EINVAL in this case which stands for
>> an atomic check failure?
>
> Say the user requests to display a 640x480+0,0 source framebuffer at
> destination offset -320,0 on a 320x240 screen, unscaled. The expectation
> would be to see the upper right quarter of the framebuffer on the
> screen, at least if the hardware was actually able to position overlays
> partially offscreen.
> If we can also fulfill that expectation by clipping the source rectangle
> to 320,240+320,0 and changing the destination rectangle to 320x240+0,0,
> why should -EINVAL be returned?

Well, IIUC, there are two kinds of clipping.
1) Clipping a rectangle from a fb according to src_x/y and src_w/h.
2) Clipping done by drm_plane_helper_check_state(), which potentially
    changes src/dst->x1/2 and src/dst->y1/2(in other words, src_x/y,
    src_w/h and crtc_x/y/w/h, though not directly).

1) is fine, no problem.
I doubt 2) is wrong as the users' original request could be changed.
That's why I mentioned returning -EINVAL.

Moreover, before and after applying the patch, I think the
->atomic_check behavior consistency is broken. For example,
negative crtc_x or crtc_y for overlay are changed from unacceptable
to potentially acceptable just because 2) may change their equivalent
dst_>x/y1.

Regards,
Liu Ying

>
> regards
> Philipp
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/10] drm/imx: ipuv3-plane: use drm_plane_helper_check_state, clipped coordinates

[Philipp Zabel]

Am Freitag, den 21.10.2016, 13:45 +0800 schrieb Ying Liu:
> On Thu, Oct 20, 2016 at 9:29 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > Am Donnerstag, den 20.10.2016, 16:51 +0800 schrieb Ying Liu:
> >> >> Does the clip thing potentially change the user's request by force?
> >> >> For example, the user request an unreasonable big resolution.
> >> >
> >> > The user is allowed to ask for destination coordinates extending outside
> >> > the crtc dimensions. This will chop off the parts that aren't visible,
> >> > and it will chop off the corresponding areas of the source as well.
> >>
> >> How about returning -EINVAL in this case which stands for
> >> an atomic check failure?
> >
> > Say the user requests to display a 640x480+0,0 source framebuffer at
> > destination offset -320,0 on a 320x240 screen, unscaled. The expectation
> > would be to see the upper right quarter of the framebuffer on the
> > screen, at least if the hardware was actually able to position overlays
> > partially offscreen.
> > If we can also fulfill that expectation by clipping the source rectangle
> > to 320,240+320,0 and changing the destination rectangle to 320x240+0,0,
> > why should -EINVAL be returned?
> 
> Well, IIUC, there are two kinds of clipping.
> 1) Clipping a rectangle from a fb according to src_x/y and src_w/h.
> 2) Clipping done by drm_plane_helper_check_state(), which potentially
>     changes src/dst->x1/2 and src/dst->y1/2(in other words, src_x/y,
>     src_w/h and crtc_x/y/w/h, though not directly).
> 
> 1) is fine, no problem.
> I doubt 2) is wrong as the users' original request could be changed.
> That's why I mentioned returning -EINVAL.
> 
> Moreover, before and after applying the patch, I think the
> ->atomic_check behavior consistency is broken. For example,
> negative crtc_x or crtc_y for overlay are changed from unacceptable
> to potentially acceptable just because 2) may change their equivalent
> dst_>x/y1.

I fail to see what's wrong with 2) as long as we can keep the observable
behaviour exactly the same as if the user request was unchanged.

regards
Philipp

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/10] drm/imx: ipuv3-plane: use drm_plane_helper_check_state, clipped coordinates

[Ying Liu]

On Fri, Oct 21, 2016 at 4:18 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Am Freitag, den 21.10.2016, 13:45 +0800 schrieb Ying Liu:
>> On Thu, Oct 20, 2016 at 9:29 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
>> > Am Donnerstag, den 20.10.2016, 16:51 +0800 schrieb Ying Liu:
>> >> >> Does the clip thing potentially change the user's request by force?
>> >> >> For example, the user request an unreasonable big resolution.
>> >> >
>> >> > The user is allowed to ask for destination coordinates extending outside
>> >> > the crtc dimensions. This will chop off the parts that aren't visible,
>> >> > and it will chop off the corresponding areas of the source as well.
>> >>
>> >> How about returning -EINVAL in this case which stands for
>> >> an atomic check failure?
>> >
>> > Say the user requests to display a 640x480+0,0 source framebuffer at
>> > destination offset -320,0 on a 320x240 screen, unscaled. The expectation
>> > would be to see the upper right quarter of the framebuffer on the
>> > screen, at least if the hardware was actually able to position overlays
>> > partially offscreen.
>> > If we can also fulfill that expectation by clipping the source rectangle
>> > to 320,240+320,0 and changing the destination rectangle to 320x240+0,0,
>> > why should -EINVAL be returned?
>>
>> Well, IIUC, there are two kinds of clipping.
>> 1) Clipping a rectangle from a fb according to src_x/y and src_w/h.
>> 2) Clipping done by drm_plane_helper_check_state(), which potentially
>>     changes src/dst->x1/2 and src/dst->y1/2(in other words, src_x/y,
>>     src_w/h and crtc_x/y/w/h, though not directly).
>>
>> 1) is fine, no problem.
>> I doubt 2) is wrong as the users' original request could be changed.
>> That's why I mentioned returning -EINVAL.
>>
>> Moreover, before and after applying the patch, I think the
>> ->atomic_check behavior consistency is broken. For example,
>> negative crtc_x or crtc_y for overlay are changed from unacceptable
>> to potentially acceptable just because 2) may change their equivalent
>> dst_>x/y1.
>
> I fail to see what's wrong with 2) as long as we can keep the observable
> behaviour exactly the same as if the user request was unchanged.

It seems the behavior could change - negative crtc_x or crtc_y for
overlay make ->atomic_check return -EINVAL before(overlay hw state
machine has nothing changed), and potentially successful after(overlay
hw state machine changes).

Regards,
Liu Ying

>
> regards
> Philipp
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/10] drm/imx: ipuv3-plane: use drm_plane_helper_check_state, clipped coordinates

[Philipp Zabel]

Am Freitag, den 21.10.2016, 16:49 +0800 schrieb Ying Liu:
> On Fri, Oct 21, 2016 at 4:18 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > Am Freitag, den 21.10.2016, 13:45 +0800 schrieb Ying Liu:
> >> On Thu, Oct 20, 2016 at 9:29 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> >> > Am Donnerstag, den 20.10.2016, 16:51 +0800 schrieb Ying Liu:
> >> >> >> Does the clip thing potentially change the user's request by force?
> >> >> >> For example, the user request an unreasonable big resolution.
> >> >> >
> >> >> > The user is allowed to ask for destination coordinates extending outside
> >> >> > the crtc dimensions. This will chop off the parts that aren't visible,
> >> >> > and it will chop off the corresponding areas of the source as well.
> >> >>
> >> >> How about returning -EINVAL in this case which stands for
> >> >> an atomic check failure?
> >> >
> >> > Say the user requests to display a 640x480+0,0 source framebuffer at
> >> > destination offset -320,0 on a 320x240 screen, unscaled. The expectation
> >> > would be to see the upper right quarter of the framebuffer on the
> >> > screen, at least if the hardware was actually able to position overlays
> >> > partially offscreen.
> >> > If we can also fulfill that expectation by clipping the source rectangle
> >> > to 320,240+320,0 and changing the destination rectangle to 320x240+0,0,
> >> > why should -EINVAL be returned?
> >>
> >> Well, IIUC, there are two kinds of clipping.
> >> 1) Clipping a rectangle from a fb according to src_x/y and src_w/h.
> >> 2) Clipping done by drm_plane_helper_check_state(), which potentially
> >>     changes src/dst->x1/2 and src/dst->y1/2(in other words, src_x/y,
> >>     src_w/h and crtc_x/y/w/h, though not directly).
> >>
> >> 1) is fine, no problem.
> >> I doubt 2) is wrong as the users' original request could be changed.
> >> That's why I mentioned returning -EINVAL.
> >>
> >> Moreover, before and after applying the patch, I think the
> >> ->atomic_check behavior consistency is broken. For example,
> >> negative crtc_x or crtc_y for overlay are changed from unacceptable
> >> to potentially acceptable just because 2) may change their equivalent
> >> dst_>x/y1.
> >
> > I fail to see what's wrong with 2) as long as we can keep the observable
> > behaviour exactly the same as if the user request was unchanged.
> 
> It seems the behavior could change - negative crtc_x or crtc_y for
> overlay make ->atomic_check return -EINVAL before(overlay hw state
> machine has nothing changed), and potentially successful after(overlay
> hw state machine changes).

That in itself doesn't seem so bad. One thing we can't do though is
'position' at any negative crtc_x/y due to the fact that when clipping
the src.x1/y1 still must be even for chroma subsampled pixel formats and
the x1 still must result in scanline start addresses aligned to 8-byte
boundaries. So for 32-bit framebuffer depth negative x offsets must be
even, and for 16-bit framebuffer depth only negative x offsets that are
a multiple of 4 are possible.

regards
Philipp

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 10/10] drm/imx: ipuv3-plane: use drm_plane_helper_check_state, clipped coordinates

[Ying Liu]

On Mon, Oct 24, 2016 at 7:50 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
> Am Freitag, den 21.10.2016, 16:49 +0800 schrieb Ying Liu:
>> On Fri, Oct 21, 2016 at 4:18 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
>> > Am Freitag, den 21.10.2016, 13:45 +0800 schrieb Ying Liu:
>> >> On Thu, Oct 20, 2016 at 9:29 PM, Philipp Zabel <p.zabel@pengutronix.de> wrote:
>> >> > Am Donnerstag, den 20.10.2016, 16:51 +0800 schrieb Ying Liu:
>> >> >> >> Does the clip thing potentially change the user's request by force?
>> >> >> >> For example, the user request an unreasonable big resolution.
>> >> >> >
>> >> >> > The user is allowed to ask for destination coordinates extending outside
>> >> >> > the crtc dimensions. This will chop off the parts that aren't visible,
>> >> >> > and it will chop off the corresponding areas of the source as well.
>> >> >>
>> >> >> How about returning -EINVAL in this case which stands for
>> >> >> an atomic check failure?
>> >> >
>> >> > Say the user requests to display a 640x480+0,0 source framebuffer at
>> >> > destination offset -320,0 on a 320x240 screen, unscaled. The expectation
>> >> > would be to see the upper right quarter of the framebuffer on the
>> >> > screen, at least if the hardware was actually able to position overlays
>> >> > partially offscreen.
>> >> > If we can also fulfill that expectation by clipping the source rectangle
>> >> > to 320,240+320,0 and changing the destination rectangle to 320x240+0,0,
>> >> > why should -EINVAL be returned?
>> >>
>> >> Well, IIUC, there are two kinds of clipping.
>> >> 1) Clipping a rectangle from a fb according to src_x/y and src_w/h.
>> >> 2) Clipping done by drm_plane_helper_check_state(), which potentially
>> >>     changes src/dst->x1/2 and src/dst->y1/2(in other words, src_x/y,
>> >>     src_w/h and crtc_x/y/w/h, though not directly).
>> >>
>> >> 1) is fine, no problem.
>> >> I doubt 2) is wrong as the users' original request could be changed.
>> >> That's why I mentioned returning -EINVAL.
>> >>
>> >> Moreover, before and after applying the patch, I think the
>> >> ->atomic_check behavior consistency is broken. For example,
>> >> negative crtc_x or crtc_y for overlay are changed from unacceptable
>> >> to potentially acceptable just because 2) may change their equivalent
>> >> dst_>x/y1.
>> >
>> > I fail to see what's wrong with 2) as long as we can keep the observable
>> > behaviour exactly the same as if the user request was unchanged.
>>
>> It seems the behavior could change - negative crtc_x or crtc_y for
>> overlay make ->atomic_check return -EINVAL before(overlay hw state
>> machine has nothing changed), and potentially successful after(overlay
>> hw state machine changes).
>
> That in itself doesn't seem so bad. One thing we can't do though is
> 'position' at any negative crtc_x/y due to the fact that when clipping
> the src.x1/y1 still must be even for chroma subsampled pixel formats and
> the x1 still must result in scanline start addresses aligned to 8-byte
> boundaries. So for 32-bit framebuffer depth negative x offsets must be
> even, and for 16-bit framebuffer depth only negative x offsets that are
> a multiple of 4 are possible.

I think the alignment requirements from HW can be guaranteed by
proper ->atomic_check implementation.
The main concern about this patch is still the clipping 2) itself.
Besides the negative crtc_x/y example, another one is that
'modetest -P 24:1280x800' may run successfully on the i.MX6Q
SabreSD board with the 1024x768 LVDS primary display.
IMO, this may confuse the userspace.
Note that this test case cannot pass atomic check before
applying this patch.

Regards,
Liu Ying

>
> regards
> Philipp
>
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel