[PATCH v3 0/7] drm/exynos: rework layer blending

[PATCH v3 0/7] drm/exynos: rework layer blending

[Marek Szyprowski]

Hello all,

This is a continuation of the work started by Tobias Jakobi. The goal of
this patch set is to remove hardcoded blending setup from Exynos Mixer
driver. This patch also enables other Exynos DRM CRTC drivers to use the
configurable plane zpos feature, however patches for FIMD or Decon are
not yet ready.

The initial patches prepared by Tobias are available here:
http://lists.freedesktop.org/archives/dri-devel/2015-November/095080.html

Main changes in v3:
- added patches for changing zpos property and layer priority
  configuration
- simplified blending setup, removed special configuration for the bottom
  layer, removed layer cache
- significantly simplified the code
- rebased on top of exynos-drm-next branch

Best regards
Marek Szyprowski
Samsung R&D Institute Poland


Patch summary:

Marek Szyprowski (5):
  drm/exynos: rename zpos to index
  drm/exynos: make zpos property configurable
  drm/exynos: mixer: set window priority based on zpos
  drm/exynos: mixer: refactor layer setup
  drm/exynos: mixer: unify a check for video-processor window

Tobias Jakobi (2):
  drm/exynos: mixer: remove all static blending setup
  drm/exynos: mixer: also allow ARGB1555 and ARGB4444

 drivers/gpu/drm/exynos/exynos5433_drm_decon.c |  10 +--
 drivers/gpu/drm/exynos/exynos7_drm_decon.c    |  10 +--
 drivers/gpu/drm/exynos/exynos_drm_drv.h       |   8 +-
 drivers/gpu/drm/exynos/exynos_drm_fimd.c      |  10 +--
 drivers/gpu/drm/exynos/exynos_drm_plane.c     |  55 ++++++++++--
 drivers/gpu/drm/exynos/exynos_drm_plane.h     |   2 +-
 drivers/gpu/drm/exynos/exynos_drm_vidi.c      |   2 +-
 drivers/gpu/drm/exynos/exynos_mixer.c         | 123 ++++++++++++++++----------
 drivers/gpu/drm/exynos/regs-mixer.h           |   4 +
 9 files changed, 150 insertions(+), 74 deletions(-)

-- 
1.9.2

[PATCH v3 1/7] drm/exynos: rename zpos to index

[Marek Szyprowski]

This patch renames zpos entry to index, because in most places it is
used as index for selecting hardware layer/window instead of
configurable layer position. This will later enable to make the zpos
property configurable.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 10 +++++-----
 drivers/gpu/drm/exynos/exynos7_drm_decon.c    | 10 +++++-----
 drivers/gpu/drm/exynos/exynos_drm_drv.h       |  4 ++--
 drivers/gpu/drm/exynos/exynos_drm_fimd.c      | 10 +++++-----
 drivers/gpu/drm/exynos/exynos_drm_plane.c     |  4 ++--
 drivers/gpu/drm/exynos/exynos_drm_plane.h     |  2 +-
 drivers/gpu/drm/exynos/exynos_drm_vidi.c      |  2 +-
 drivers/gpu/drm/exynos/exynos_mixer.c         | 14 +++++++-------
 8 files changed, 28 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
index c7362b99ce28..88d022ad5280 100644
--- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
+++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
@@ -256,7 +256,7 @@ static void decon_atomic_begin(struct exynos_drm_crtc *crtc,
 	if (test_bit(BIT_SUSPENDED, &ctx->flags))
 		return;
 
-	decon_shadow_protect_win(ctx, plane->zpos, true);
+	decon_shadow_protect_win(ctx, plane->index, true);
 }
 
 #define BIT_VAL(x, e, s) (((x) & ((1 << ((e) - (s) + 1)) - 1)) << (s))
@@ -270,7 +270,7 @@ static void decon_update_plane(struct exynos_drm_crtc *crtc,
 				to_exynos_plane_state(plane->base.state);
 	struct decon_context *ctx = crtc->ctx;
 	struct drm_framebuffer *fb = state->base.fb;
-	unsigned int win = plane->zpos;
+	unsigned int win = plane->index;
 	unsigned int bpp = fb->bits_per_pixel >> 3;
 	unsigned int pitch = fb->pitches[0];
 	dma_addr_t dma_addr = exynos_drm_fb_dma_addr(fb, 0);
@@ -320,7 +320,7 @@ static void decon_disable_plane(struct exynos_drm_crtc *crtc,
 				struct exynos_drm_plane *plane)
 {
 	struct decon_context *ctx = crtc->ctx;
-	unsigned int win = plane->zpos;
+	unsigned int win = plane->index;
 
 	if (test_bit(BIT_SUSPENDED, &ctx->flags))
 		return;
@@ -344,7 +344,7 @@ static void decon_atomic_flush(struct exynos_drm_crtc *crtc,
 	if (test_bit(BIT_SUSPENDED, &ctx->flags))
 		return;
 
-	decon_shadow_protect_win(ctx, plane->zpos, false);
+	decon_shadow_protect_win(ctx, plane->index, false);
 
 	if (ctx->out_type == IFTYPE_I80)
 		set_bit(BIT_WIN_UPDATED, &ctx->flags);
@@ -502,7 +502,7 @@ static int decon_bind(struct device *dev, struct device *master, void *data)
 		ctx->configs[win].zpos = win;
 		ctx->configs[win].type = decon_win_types[tmp];
 
-		ret = exynos_plane_init(drm_dev, &ctx->planes[win],
+		ret = exynos_plane_init(drm_dev, &ctx->planes[win], i,
 					1 << ctx->pipe, &ctx->configs[win]);
 		if (ret)
 			return ret;
diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
index c47f9af8170b..8911f965b06c 100644
--- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
+++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
@@ -393,7 +393,7 @@ static void decon_atomic_begin(struct exynos_drm_crtc *crtc,
 	if (ctx->suspended)
 		return;
 
-	decon_shadow_protect_win(ctx, plane->zpos, true);
+	decon_shadow_protect_win(ctx, plane->index, true);
 }
 
 static void decon_update_plane(struct exynos_drm_crtc *crtc,
@@ -407,7 +407,7 @@ static void decon_update_plane(struct exynos_drm_crtc *crtc,
 	unsigned long val, alpha;
 	unsigned int last_x;
 	unsigned int last_y;
-	unsigned int win = plane->zpos;
+	unsigned int win = plane->index;
 	unsigned int bpp = fb->bits_per_pixel >> 3;
 	unsigned int pitch = fb->pitches[0];
 
@@ -498,7 +498,7 @@ static void decon_disable_plane(struct exynos_drm_crtc *crtc,
 				struct exynos_drm_plane *plane)
 {
 	struct decon_context *ctx = crtc->ctx;
-	unsigned int win = plane->zpos;
+	unsigned int win = plane->index;
 	u32 val;
 
 	if (ctx->suspended)
@@ -525,7 +525,7 @@ static void decon_atomic_flush(struct exynos_drm_crtc *crtc,
 	if (ctx->suspended)
 		return;
 
-	decon_shadow_protect_win(ctx, plane->zpos, false);
+	decon_shadow_protect_win(ctx, plane->index, false);
 }
 
 static void decon_init(struct decon_context *ctx)
@@ -657,7 +657,7 @@ static int decon_bind(struct device *dev, struct device *master, void *data)
 		ctx->configs[i].zpos = i;
 		ctx->configs[i].type = decon_win_types[i];
 
-		ret = exynos_plane_init(drm_dev, &ctx->planes[i],
+		ret = exynos_plane_init(drm_dev, &ctx->planes[i], i,
 					1 << ctx->pipe, &ctx->configs[i]);
 		if (ret)
 			return ret;
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
index 82bbd7f4b316..588b6763f9c7 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -76,7 +76,7 @@ to_exynos_plane_state(struct drm_plane_state *state)
  * Exynos drm common overlay structure.
  *
  * @base: plane object
- * @zpos: order of overlay layer(z position).
+ * @index: hardware index of the overlay layer
  *
  * this structure is common to exynos SoC and its contents would be copied
  * to hardware specific overlay info.
@@ -85,7 +85,7 @@ to_exynos_plane_state(struct drm_plane_state *state)
 struct exynos_drm_plane {
 	struct drm_plane base;
 	const struct exynos_drm_plane_config *config;
-	unsigned int zpos;
+	unsigned int index;
 	struct drm_framebuffer *pending_fb;
 };
 
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index 2e2247126581..6ae1b1e55783 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -630,7 +630,7 @@ static void fimd_atomic_begin(struct exynos_drm_crtc *crtc,
 	if (ctx->suspended)
 		return;
 
-	fimd_shadow_protect_win(ctx, plane->zpos, true);
+	fimd_shadow_protect_win(ctx, plane->index, true);
 }
 
 static void fimd_atomic_flush(struct exynos_drm_crtc *crtc,
@@ -641,7 +641,7 @@ static void fimd_atomic_flush(struct exynos_drm_crtc *crtc,
 	if (ctx->suspended)
 		return;
 
-	fimd_shadow_protect_win(ctx, plane->zpos, false);
+	fimd_shadow_protect_win(ctx, plane->index, false);
 }
 
 static void fimd_update_plane(struct exynos_drm_crtc *crtc,
@@ -654,7 +654,7 @@ static void fimd_update_plane(struct exynos_drm_crtc *crtc,
 	dma_addr_t dma_addr;
 	unsigned long val, size, offset;
 	unsigned int last_x, last_y, buf_offsize, line_size;
-	unsigned int win = plane->zpos;
+	unsigned int win = plane->index;
 	unsigned int bpp = fb->bits_per_pixel >> 3;
 	unsigned int pitch = fb->pitches[0];
 
@@ -740,7 +740,7 @@ static void fimd_disable_plane(struct exynos_drm_crtc *crtc,
 			       struct exynos_drm_plane *plane)
 {
 	struct fimd_context *ctx = crtc->ctx;
-	unsigned int win = plane->zpos;
+	unsigned int win = plane->index;
 
 	if (ctx->suspended)
 		return;
@@ -944,7 +944,7 @@ static int fimd_bind(struct device *dev, struct device *master, void *data)
 		ctx->configs[i].num_pixel_formats = ARRAY_SIZE(fimd_formats);
 		ctx->configs[i].zpos = i;
 		ctx->configs[i].type = fimd_win_types[i];
-		ret = exynos_plane_init(drm_dev, &ctx->planes[i],
+		ret = exynos_plane_init(drm_dev, &ctx->planes[i], i,
 					1 << ctx->pipe, &ctx->configs[i]);
 		if (ret)
 			return ret;
diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
index 427aeec78a28..fd6cb4cee01a 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
@@ -280,7 +280,7 @@ static void exynos_plane_attach_zpos_property(struct drm_plane *plane,
 
 int exynos_plane_init(struct drm_device *dev,
 		      struct exynos_drm_plane *exynos_plane,
-		      unsigned long possible_crtcs,
+		      unsigned int index, unsigned long possible_crtcs,
 		      const struct exynos_drm_plane_config *config)
 {
 	int err;
@@ -299,7 +299,7 @@ int exynos_plane_init(struct drm_device *dev,
 
 	drm_plane_helper_add(&exynos_plane->base, &plane_helper_funcs);
 
-	exynos_plane->zpos = config->zpos;
+	exynos_plane->index = index;
 	exynos_plane->config = config;
 
 	if (config->type == DRM_PLANE_TYPE_OVERLAY)
diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.h b/drivers/gpu/drm/exynos/exynos_drm_plane.h
index 0dd096548284..9aafad164cdf 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.h
@@ -10,6 +10,6 @@
  */
 
 int exynos_plane_init(struct drm_device *dev,
-		      struct exynos_drm_plane *exynos_plane,
+		      struct exynos_drm_plane *exynos_plane, unsigned int index,
 		      unsigned long possible_crtcs,
 		      const struct exynos_drm_plane_config *config);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
index c1d33d5cbb15..adad4dc84b81 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
@@ -461,7 +461,7 @@ static int vidi_bind(struct device *dev, struct device *master, void *data)
 		plane_config.zpos = i;
 		plane_config.type = vidi_win_types[i];
 
-		ret = exynos_plane_init(drm_dev, &ctx->planes[i],
+		ret = exynos_plane_init(drm_dev, &ctx->planes[i], i,
 					1 << ctx->pipe, &plane_config);
 		if (ret)
 			return ret;
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index dfb35e2da4db..0dceeb2b532c 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -511,7 +511,7 @@ static void vp_video_buffer(struct mixer_context *ctx,
 
 	mixer_cfg_scan(ctx, mode->vdisplay);
 	mixer_cfg_rgb_fmt(ctx, mode->vdisplay);
-	mixer_cfg_layer(ctx, plane->zpos, true);
+	mixer_cfg_layer(ctx, plane->index, true);
 	mixer_run(ctx);
 
 	mixer_vsync_set_update(ctx, true);
@@ -537,7 +537,7 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
 	struct mixer_resources *res = &ctx->mixer_res;
 	struct drm_framebuffer *fb = state->base.fb;
 	unsigned long flags;
-	unsigned int win = plane->zpos;
+	unsigned int win = plane->index;
 	unsigned int x_ratio = 0, y_ratio = 0;
 	unsigned int src_x_offset, src_y_offset, dst_x_offset, dst_y_offset;
 	dma_addr_t dma_addr;
@@ -956,12 +956,12 @@ static void mixer_update_plane(struct exynos_drm_crtc *crtc,
 {
 	struct mixer_context *mixer_ctx = crtc->ctx;
 
-	DRM_DEBUG_KMS("win: %d\n", plane->zpos);
+	DRM_DEBUG_KMS("win: %d\n", plane->index);
 
 	if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
 		return;
 
-	if (plane->zpos > 1 && mixer_ctx->vp_enabled)
+	if (plane->index > 1 && mixer_ctx->vp_enabled)
 		vp_video_buffer(mixer_ctx, plane);
 	else
 		mixer_graph_buffer(mixer_ctx, plane);
@@ -974,7 +974,7 @@ static void mixer_disable_plane(struct exynos_drm_crtc *crtc,
 	struct mixer_resources *res = &mixer_ctx->mixer_res;
 	unsigned long flags;
 
-	DRM_DEBUG_KMS("win: %d\n", plane->zpos);
+	DRM_DEBUG_KMS("win: %d\n", plane->index);
 
 	if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
 		return;
@@ -982,7 +982,7 @@ static void mixer_disable_plane(struct exynos_drm_crtc *crtc,
 	spin_lock_irqsave(&res->reg_slock, flags);
 	mixer_vsync_set_update(mixer_ctx, false);
 
-	mixer_cfg_layer(mixer_ctx, plane->zpos, false);
+	mixer_cfg_layer(mixer_ctx, plane->index, false);
 
 	mixer_vsync_set_update(mixer_ctx, true);
 	spin_unlock_irqrestore(&res->reg_slock, flags);
@@ -1160,7 +1160,7 @@ static int mixer_bind(struct device *dev, struct device *manager, void *data)
 		if (i == VP_DEFAULT_WIN && !ctx->vp_enabled)
 			continue;
 
-		ret = exynos_plane_init(drm_dev, &ctx->planes[i],
+		ret = exynos_plane_init(drm_dev, &ctx->planes[i], i,
 					1 << ctx->pipe, &plane_configs[i]);
 		if (ret)
 			return ret;
-- 
1.9.2

[PATCH v3 2/7] drm/exynos: make zpos property configurable

[Marek Szyprowski]

This patch adds all infrastructure to make zpos plane property
configurable from userspace.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_drm_drv.h   |  4 ++-
 drivers/gpu/drm/exynos/exynos_drm_plane.c | 51 ++++++++++++++++++++++++++++---
 2 files changed, 49 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
index 588b6763f9c7..f0827dbebb7d 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -64,6 +64,7 @@ struct exynos_drm_plane_state {
 	struct exynos_drm_rect src;
 	unsigned int h_ratio;
 	unsigned int v_ratio;
+	unsigned int zpos;
 };
 
 static inline struct exynos_drm_plane_state *
@@ -91,11 +92,12 @@ struct exynos_drm_plane {
 
 #define EXYNOS_DRM_PLANE_CAP_DOUBLE	(1 << 0)
 #define EXYNOS_DRM_PLANE_CAP_SCALE	(1 << 1)
+#define EXYNOS_DRM_PLANE_CAP_ZPOS	(1 << 2)
 
 /*
  * Exynos DRM plane configuration structure.
  *
- * @zpos: z-position of the plane.
+ * @zpos: initial z-position of the plane.
  * @type: type of the plane (primary, cursor or overlay).
  * @pixel_formats: supported pixel formats.
  * @num_pixel_formats: number of elements in 'pixel_formats'.
diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
index fd6cb4cee01a..a2bdab836b50 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
@@ -124,6 +124,7 @@ static void exynos_plane_mode_set(struct exynos_drm_plane_state *exynos_state)
 
 static void exynos_drm_plane_reset(struct drm_plane *plane)
 {
+	struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
 	struct exynos_drm_plane_state *exynos_state;
 
 	if (plane->state) {
@@ -136,6 +137,7 @@ static void exynos_drm_plane_reset(struct drm_plane *plane)
 
 	exynos_state = kzalloc(sizeof(*exynos_state), GFP_KERNEL);
 	if (exynos_state) {
+		exynos_state->zpos = exynos_plane->config->zpos;
 		plane->state = &exynos_state->base;
 		plane->state->plane = plane;
 	}
@@ -153,6 +155,7 @@ exynos_drm_plane_duplicate_state(struct drm_plane *plane)
 		return NULL;
 
 	__drm_atomic_helper_plane_duplicate_state(plane, &copy->base);
+	copy->zpos = exynos_state->zpos;
 	return &copy->base;
 }
 
@@ -165,13 +168,53 @@ static void exynos_drm_plane_destroy_state(struct drm_plane *plane,
 	kfree(old_exynos_state);
 }
 
+static int exynos_drm_plane_atomic_set_property(struct drm_plane *plane,
+						struct drm_plane_state *state,
+						struct drm_property *property,
+						uint64_t val)
+{
+	struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
+	struct exynos_drm_plane_state *exynos_state =
+					to_exynos_plane_state(state);
+	struct exynos_drm_private *dev_priv = plane->dev->dev_private;
+	const struct exynos_drm_plane_config *config = exynos_plane->config;
+
+	if (property == dev_priv->plane_zpos_property &&
+	    (config->capabilities & EXYNOS_DRM_PLANE_CAP_ZPOS))
+		exynos_state->zpos = val;
+	else
+		return -EINVAL;
+
+	return 0;
+}
+
+static int exynos_drm_plane_atomic_get_property(struct drm_plane *plane,
+					  const struct drm_plane_state *state,
+					  struct drm_property *property,
+					  uint64_t *val)
+{
+	const struct exynos_drm_plane_state *exynos_state =
+		container_of(state, const struct exynos_drm_plane_state, base);
+	struct exynos_drm_private *dev_priv = plane->dev->dev_private;
+
+	if (property == dev_priv->plane_zpos_property)
+		*val = exynos_state->zpos;
+	else
+		return -EINVAL;
+
+	return 0;
+}
+
 static struct drm_plane_funcs exynos_plane_funcs = {
 	.update_plane	= drm_atomic_helper_update_plane,
 	.disable_plane	= drm_atomic_helper_disable_plane,
 	.destroy	= drm_plane_cleanup,
+	.set_property	= drm_atomic_helper_plane_set_property,
 	.reset		= exynos_drm_plane_reset,
 	.atomic_duplicate_state = exynos_drm_plane_duplicate_state,
 	.atomic_destroy_state = exynos_drm_plane_destroy_state,
+	.atomic_set_property = exynos_drm_plane_atomic_set_property,
+	.atomic_get_property = exynos_drm_plane_atomic_get_property,
 };
 
 static int
@@ -267,8 +310,8 @@ static void exynos_plane_attach_zpos_property(struct drm_plane *plane,
 
 	prop = dev_priv->plane_zpos_property;
 	if (!prop) {
-		prop = drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE,
-						 "zpos", 0, MAX_PLANE - 1);
+		prop = drm_property_create_range(dev, 0, "zpos",
+						 0, MAX_PLANE - 1);
 		if (!prop)
 			return;
 
@@ -302,9 +345,7 @@ int exynos_plane_init(struct drm_device *dev,
 	exynos_plane->index = index;
 	exynos_plane->config = config;
 
-	if (config->type == DRM_PLANE_TYPE_OVERLAY)
-		exynos_plane_attach_zpos_property(&exynos_plane->base,
-						  config->zpos);
+	exynos_plane_attach_zpos_property(&exynos_plane->base, config->zpos);
 
 	return 0;
 }
-- 
1.9.2

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

[PATCH v3 3/7] drm/exynos: mixer: set window priority based on zpos

[Marek Szyprowski]

'zpos' plane property is configurable, so adjust hardware layers
priority based on the zpos value. 'zpos' value shifted by one can be
used directly as hw priority value and stored to the registers, because
mixer accepts priority values from 1 to 15 (0 means that layer is
disabled).

This patch also changes the default layer priority to match already
exposed initial zpos values. The initial configuration is now:
[top] video > gfx layer1 > gfx layer0 [bottom].

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_mixer.c | 39 +++++++++++++++++++----------------
 drivers/gpu/drm/exynos/regs-mixer.h   |  3 +++
 2 files changed, 24 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 0dceeb2b532c..c0d128bc084b 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -117,19 +117,22 @@ static const struct exynos_drm_plane_config plane_configs[MIXER_WIN_NR] = {
 		.type = DRM_PLANE_TYPE_PRIMARY,
 		.pixel_formats = mixer_formats,
 		.num_pixel_formats = ARRAY_SIZE(mixer_formats),
-		.capabilities = EXYNOS_DRM_PLANE_CAP_DOUBLE,
+		.capabilities = EXYNOS_DRM_PLANE_CAP_DOUBLE |
+				EXYNOS_DRM_PLANE_CAP_ZPOS,
 	}, {
 		.zpos = 1,
 		.type = DRM_PLANE_TYPE_CURSOR,
 		.pixel_formats = mixer_formats,
 		.num_pixel_formats = ARRAY_SIZE(mixer_formats),
-		.capabilities = EXYNOS_DRM_PLANE_CAP_DOUBLE,
+		.capabilities = EXYNOS_DRM_PLANE_CAP_DOUBLE |
+				EXYNOS_DRM_PLANE_CAP_ZPOS,
 	}, {
 		.zpos = 2,
 		.type = DRM_PLANE_TYPE_OVERLAY,
 		.pixel_formats = vp_formats,
 		.num_pixel_formats = ARRAY_SIZE(vp_formats),
-		.capabilities = EXYNOS_DRM_PLANE_CAP_SCALE,
+		.capabilities = EXYNOS_DRM_PLANE_CAP_SCALE |
+				EXYNOS_DRM_PLANE_CAP_ZPOS,
 	},
 };
 
@@ -372,7 +375,7 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height)
 }
 
 static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
-				bool enable)
+			    unsigned int priority, bool enable)
 {
 	struct mixer_resources *res = &ctx->mixer_res;
 	u32 val = enable ? ~0 : 0;
@@ -380,15 +383,24 @@ static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
 	switch (win) {
 	case 0:
 		mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE);
+		mixer_reg_writemask(res, MXR_LAYER_CFG,
+				    MXR_LAYER_CFG_GRP0_VAL(priority),
+				    MXR_LAYER_CFG_GRP0_MASK);
 		break;
 	case 1:
 		mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP1_ENABLE);
+		mixer_reg_writemask(res, MXR_LAYER_CFG,
+				    MXR_LAYER_CFG_GRP1_VAL(priority),
+				    MXR_LAYER_CFG_GRP1_MASK);
 		break;
 	case 2:
 		if (ctx->vp_enabled) {
 			vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON);
 			mixer_reg_writemask(res, MXR_CFG, val,
 				MXR_CFG_VP_ENABLE);
+			mixer_reg_writemask(res, MXR_LAYER_CFG,
+					    MXR_LAYER_CFG_VP_VAL(priority),
+					    MXR_LAYER_CFG_VP_MASK);
 
 			/* control blending of graphic layer 0 */
 			mixer_reg_writemask(res, MXR_GRAPHIC_CFG(0), val,
@@ -511,7 +523,7 @@ static void vp_video_buffer(struct mixer_context *ctx,
 
 	mixer_cfg_scan(ctx, mode->vdisplay);
 	mixer_cfg_rgb_fmt(ctx, mode->vdisplay);
-	mixer_cfg_layer(ctx, plane->index, true);
+	mixer_cfg_layer(ctx, plane->index, state->zpos + 1, true);
 	mixer_run(ctx);
 
 	mixer_vsync_set_update(ctx, true);
@@ -626,7 +638,7 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
 
 	mixer_cfg_scan(ctx, mode->vdisplay);
 	mixer_cfg_rgb_fmt(ctx, mode->vdisplay);
-	mixer_cfg_layer(ctx, win, true);
+	mixer_cfg_layer(ctx, win, state->zpos + 1, true);
 
 	/* layer update mandatory for mixer 16.0.33.0 */
 	if (ctx->mxr_ver == MXR_VER_16_0_33_0 ||
@@ -674,17 +686,8 @@ static void mixer_win_reset(struct mixer_context *ctx)
 	mixer_reg_writemask(res, MXR_STATUS, MXR_STATUS_16_BURST,
 		MXR_STATUS_BURST_MASK);
 
-	/* setting default layer priority: layer1 > layer0 > video
-	 * because typical usage scenario would be
-	 * layer1 - OSD
-	 * layer0 - framebuffer
-	 * video - video overlay
-	 */
-	val = MXR_LAYER_CFG_GRP1_VAL(3);
-	val |= MXR_LAYER_CFG_GRP0_VAL(2);
-	if (ctx->vp_enabled)
-		val |= MXR_LAYER_CFG_VP_VAL(1);
-	mixer_reg_write(res, MXR_LAYER_CFG, val);
+	/* reset default layer priority */
+	mixer_reg_write(res, MXR_LAYER_CFG, 0);
 
 	/* setting background color */
 	mixer_reg_write(res, MXR_BG_COLOR0, 0x008080);
@@ -982,7 +985,7 @@ static void mixer_disable_plane(struct exynos_drm_crtc *crtc,
 	spin_lock_irqsave(&res->reg_slock, flags);
 	mixer_vsync_set_update(mixer_ctx, false);
 
-	mixer_cfg_layer(mixer_ctx, plane->index, false);
+	mixer_cfg_layer(mixer_ctx, plane->index, 0, false);
 
 	mixer_vsync_set_update(mixer_ctx, true);
 	spin_unlock_irqrestore(&res->reg_slock, flags);
diff --git a/drivers/gpu/drm/exynos/regs-mixer.h b/drivers/gpu/drm/exynos/regs-mixer.h
index ac60260c2389..dbdbc0af3358 100644
--- a/drivers/gpu/drm/exynos/regs-mixer.h
+++ b/drivers/gpu/drm/exynos/regs-mixer.h
@@ -145,8 +145,11 @@
 
 /* bit for MXR_LAYER_CFG */
 #define MXR_LAYER_CFG_GRP1_VAL(x)	MXR_MASK_VAL(x, 11, 8)
+#define MXR_LAYER_CFG_GRP1_MASK		MXR_LAYER_CFG_GRP1_VAL(~0)
 #define MXR_LAYER_CFG_GRP0_VAL(x)	MXR_MASK_VAL(x, 7, 4)
+#define MXR_LAYER_CFG_GRP0_MASK		MXR_LAYER_CFG_GRP0_VAL(~0)
 #define MXR_LAYER_CFG_VP_VAL(x)		MXR_MASK_VAL(x, 3, 0)
+#define MXR_LAYER_CFG_VP_MASK		MXR_LAYER_CFG_VP_VAL(~0)
 
 #endif /* SAMSUNG_REGS_MIXER_H */
 
-- 
1.9.2

[PATCH v3 4/7] drm/exynos: mixer: remove all static blending setup

[Marek Szyprowski]

From: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>

Previously blending setup was static and most of it was
done in mixer_win_reset().

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/exynos/exynos_mixer.c | 23 -----------------------
 1 file changed, 23 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index c0d128bc084b..c572e271579e 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -401,11 +401,6 @@ static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
 			mixer_reg_writemask(res, MXR_LAYER_CFG,
 					    MXR_LAYER_CFG_VP_VAL(priority),
 					    MXR_LAYER_CFG_VP_MASK);
-
-			/* control blending of graphic layer 0 */
-			mixer_reg_writemask(res, MXR_GRAPHIC_CFG(0), val,
-					MXR_GRP_CFG_BLEND_PRE_MUL |
-					MXR_GRP_CFG_PIXEL_BLEND_EN);
 		}
 		break;
 	}
@@ -672,7 +667,6 @@ static void mixer_win_reset(struct mixer_context *ctx)
 {
 	struct mixer_resources *res = &ctx->mixer_res;
 	unsigned long flags;
-	u32 val; /* value stored to register */
 
 	spin_lock_irqsave(&res->reg_slock, flags);
 	mixer_vsync_set_update(ctx, false);
@@ -694,23 +688,6 @@ static void mixer_win_reset(struct mixer_context *ctx)
 	mixer_reg_write(res, MXR_BG_COLOR1, 0x008080);
 	mixer_reg_write(res, MXR_BG_COLOR2, 0x008080);
 
-	/* setting graphical layers */
-	val  = MXR_GRP_CFG_COLOR_KEY_DISABLE; /* no blank key */
-	val |= MXR_GRP_CFG_WIN_BLEND_EN;
-	val |= MXR_GRP_CFG_ALPHA_VAL(0xff); /* non-transparent alpha */
-
-	/* Don't blend layer 0 onto the mixer background */
-	mixer_reg_write(res, MXR_GRAPHIC_CFG(0), val);
-
-	/* Blend layer 1 into layer 0 */
-	val |= MXR_GRP_CFG_BLEND_PRE_MUL;
-	val |= MXR_GRP_CFG_PIXEL_BLEND_EN;
-	mixer_reg_write(res, MXR_GRAPHIC_CFG(1), val);
-
-	/* setting video layers */
-	val = MXR_GRP_CFG_ALPHA_VAL(0);
-	mixer_reg_write(res, MXR_VIDEO_CFG, val);
-
 	if (ctx->vp_enabled) {
 		/* configuration of Video Processor Registers */
 		vp_win_reset(ctx);
-- 
1.9.2

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

[PATCH v3 5/7] drm/exynos: mixer: refactor layer setup

[Marek Szyprowski]

Properly configure blending properties of given hardware layer based on
the selected pixel format. Currently only per-pixel-based alpha is possible
when respective pixel format has been selected. Configuration of global,
per-plane alpha value, color key and background color will be added later.

This patch is heavily inspired by earlier work done by Tobias Jakobi
<tjakobi@math.uni-bielefeld.de>.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_mixer.c | 43 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/exynos/regs-mixer.h   |  1 +
 2 files changed, 44 insertions(+)

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index c572e271579e..ae7b122274ac 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -165,6 +165,16 @@ static const u8 filter_cr_horiz_tap4[] = {
 	70,	59,	48,	37,	27,	19,	11,	5,
 };
 
+static inline bool is_alpha_format(unsigned int pixel_format)
+{
+	switch (pixel_format) {
+	case DRM_FORMAT_ARGB8888:
+		return true;
+	default:
+		return false;
+	}
+}
+
 static inline u32 vp_reg_read(struct mixer_resources *res, u32 reg_id)
 {
 	return readl(res->vp_regs + reg_id);
@@ -294,6 +304,37 @@ static void vp_default_filter(struct mixer_resources *res)
 		filter_cr_horiz_tap4, sizeof(filter_cr_horiz_tap4));
 }
 
+static void mixer_cfg_gfx_blend(struct mixer_context *ctx, unsigned int win,
+				bool alpha)
+{
+	struct mixer_resources *res = &ctx->mixer_res;
+	u32 val;
+
+	val  = MXR_GRP_CFG_COLOR_KEY_DISABLE; /* no blank key */
+	if (alpha) {
+		/* blending based on pixel alpha */
+		val |= MXR_GRP_CFG_BLEND_PRE_MUL;
+		val |= MXR_GRP_CFG_PIXEL_BLEND_EN;
+	}
+	mixer_reg_writemask(res, MXR_GRAPHIC_CFG(win),
+			    val, MXR_GRP_CFG_MISC_MASK);
+}
+
+static void mixer_cfg_vp_blend(struct mixer_context *ctx)
+{
+	struct mixer_resources *res = &ctx->mixer_res;
+	u32 val;
+
+	/*
+	 * No blending at the moment since the NV12/NV21 pixelformats don't
+	 * have an alpha channel. However the mixer supports a global alpha
+	 * value for a layer. Once this functionality is exposed, we can
+	 * support blending of the video layer through this.
+	 */
+	val = 0;
+	mixer_reg_write(res, MXR_VIDEO_CFG, val);
+}
+
 static void mixer_vsync_set_update(struct mixer_context *ctx, bool enable)
 {
 	struct mixer_resources *res = &ctx->mixer_res;
@@ -519,6 +560,7 @@ static void vp_video_buffer(struct mixer_context *ctx,
 	mixer_cfg_scan(ctx, mode->vdisplay);
 	mixer_cfg_rgb_fmt(ctx, mode->vdisplay);
 	mixer_cfg_layer(ctx, plane->index, state->zpos + 1, true);
+	mixer_cfg_vp_blend(ctx);
 	mixer_run(ctx);
 
 	mixer_vsync_set_update(ctx, true);
@@ -634,6 +676,7 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
 	mixer_cfg_scan(ctx, mode->vdisplay);
 	mixer_cfg_rgb_fmt(ctx, mode->vdisplay);
 	mixer_cfg_layer(ctx, win, state->zpos + 1, true);
+	mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format));
 
 	/* layer update mandatory for mixer 16.0.33.0 */
 	if (ctx->mxr_ver == MXR_VER_16_0_33_0 ||
diff --git a/drivers/gpu/drm/exynos/regs-mixer.h b/drivers/gpu/drm/exynos/regs-mixer.h
index dbdbc0af3358..7f22df5bf707 100644
--- a/drivers/gpu/drm/exynos/regs-mixer.h
+++ b/drivers/gpu/drm/exynos/regs-mixer.h
@@ -113,6 +113,7 @@
 #define MXR_GRP_CFG_BLEND_PRE_MUL	(1 << 20)
 #define MXR_GRP_CFG_WIN_BLEND_EN	(1 << 17)
 #define MXR_GRP_CFG_PIXEL_BLEND_EN	(1 << 16)
+#define MXR_GRP_CFG_MISC_MASK		((3 << 16) | (3 << 20))
 #define MXR_GRP_CFG_FORMAT_VAL(x)	MXR_MASK_VAL(x, 11, 8)
 #define MXR_GRP_CFG_FORMAT_MASK		MXR_GRP_CFG_FORMAT_VAL(~0)
 #define MXR_GRP_CFG_ALPHA_VAL(x)	MXR_MASK_VAL(x, 7, 0)
-- 
1.9.2

[PATCH v3 6/7] drm/exynos: mixer: also allow ARGB1555 and ARGB4444

[Marek Szyprowski]

From: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>

Allow the remaining alpha formats now that blending
is properly setup.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_mixer.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index ae7b122274ac..31a9a228744e 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -76,7 +76,9 @@ enum mixer_flag_bits {
 
 static const uint32_t mixer_formats[] = {
 	DRM_FORMAT_XRGB4444,
+	DRM_FORMAT_ARGB4444,
 	DRM_FORMAT_XRGB1555,
+	DRM_FORMAT_ARGB1555,
 	DRM_FORMAT_RGB565,
 	DRM_FORMAT_XRGB8888,
 	DRM_FORMAT_ARGB8888,
@@ -169,6 +171,8 @@ static inline bool is_alpha_format(unsigned int pixel_format)
 {
 	switch (pixel_format) {
 	case DRM_FORMAT_ARGB8888:
+	case DRM_FORMAT_ARGB1555:
+	case DRM_FORMAT_ARGB4444:
 		return true;
 	default:
 		return false;
@@ -595,10 +599,12 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
 
 	switch (fb->pixel_format) {
 	case DRM_FORMAT_XRGB4444:
+	case DRM_FORMAT_ARGB4444:
 		fmt = MXR_FORMAT_ARGB4444;
 		break;
 
 	case DRM_FORMAT_XRGB1555:
+	case DRM_FORMAT_ARGB1555:
 		fmt = MXR_FORMAT_ARGB1555;
 		break;
 
-- 
1.9.2

[PATCH v3 7/7] drm/exynos: mixer: unify a check for video-processor window

[Marek Szyprowski]

Always use macro instead of hard-coded '2' value in conditions related
to video processor window. Additional checks are not needed, because
video layer is registered only when video processor is available.

Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
---
 drivers/gpu/drm/exynos/exynos_mixer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 31a9a228744e..bf148dc3623c 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -438,7 +438,7 @@ static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
 				    MXR_LAYER_CFG_GRP1_VAL(priority),
 				    MXR_LAYER_CFG_GRP1_MASK);
 		break;
-	case 2:
+	case VP_DEFAULT_WIN:
 		if (ctx->vp_enabled) {
 			vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON);
 			mixer_reg_writemask(res, MXR_CFG, val,
@@ -990,7 +990,7 @@ static void mixer_update_plane(struct exynos_drm_crtc *crtc,
 	if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
 		return;
 
-	if (plane->index > 1 && mixer_ctx->vp_enabled)
+	if (plane->index == VP_DEFAULT_WIN)
 		vp_video_buffer(mixer_ctx, plane);
 	else
 		mixer_graph_buffer(mixer_ctx, plane);
-- 
1.9.2

Re: [PATCH v3 2/7] drm/exynos: make zpos property configurable

[Daniel Vetter]

On Wed, Dec 16, 2015 at 01:21:43PM +0100, Marek Szyprowski wrote:
> This patch adds all infrastructure to make zpos plane property
> configurable from userspace.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>

Imo zpos should be a generic atomic property with well-defined semantics
shared across drivers. So
- storead (in decoded form) in drm_plane_state
- common functions to register/attach zpos prop to a plane, with
  full-blown kerneldoc explaining how it should work
- generic kms igt to validate that interface

One of the big things that always comes up when we talk about zpos is how
equal zpos should be handled, and how exactly they should be sorted. I
think for that we should have a drm function which computes a normalized
zpos. Or the core check code should reject such nonsense.
-Daniel

> ---
>  drivers/gpu/drm/exynos/exynos_drm_drv.h   |  4 ++-
>  drivers/gpu/drm/exynos/exynos_drm_plane.c | 51 ++++++++++++++++++++++++++++---
>  2 files changed, 49 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> index 588b6763f9c7..f0827dbebb7d 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> @@ -64,6 +64,7 @@ struct exynos_drm_plane_state {
>  	struct exynos_drm_rect src;
>  	unsigned int h_ratio;
>  	unsigned int v_ratio;
> +	unsigned int zpos;
>  };
>  
>  static inline struct exynos_drm_plane_state *
> @@ -91,11 +92,12 @@ struct exynos_drm_plane {
>  
>  #define EXYNOS_DRM_PLANE_CAP_DOUBLE	(1 << 0)
>  #define EXYNOS_DRM_PLANE_CAP_SCALE	(1 << 1)
> +#define EXYNOS_DRM_PLANE_CAP_ZPOS	(1 << 2)
>  
>  /*
>   * Exynos DRM plane configuration structure.
>   *
> - * @zpos: z-position of the plane.
> + * @zpos: initial z-position of the plane.
>   * @type: type of the plane (primary, cursor or overlay).
>   * @pixel_formats: supported pixel formats.
>   * @num_pixel_formats: number of elements in 'pixel_formats'.
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
> index fd6cb4cee01a..a2bdab836b50 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
> @@ -124,6 +124,7 @@ static void exynos_plane_mode_set(struct exynos_drm_plane_state *exynos_state)
>  
>  static void exynos_drm_plane_reset(struct drm_plane *plane)
>  {
> +	struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
>  	struct exynos_drm_plane_state *exynos_state;
>  
>  	if (plane->state) {
> @@ -136,6 +137,7 @@ static void exynos_drm_plane_reset(struct drm_plane *plane)
>  
>  	exynos_state = kzalloc(sizeof(*exynos_state), GFP_KERNEL);
>  	if (exynos_state) {
> +		exynos_state->zpos = exynos_plane->config->zpos;
>  		plane->state = &exynos_state->base;
>  		plane->state->plane = plane;
>  	}
> @@ -153,6 +155,7 @@ exynos_drm_plane_duplicate_state(struct drm_plane *plane)
>  		return NULL;
>  
>  	__drm_atomic_helper_plane_duplicate_state(plane, &copy->base);
> +	copy->zpos = exynos_state->zpos;
>  	return &copy->base;
>  }
>  
> @@ -165,13 +168,53 @@ static void exynos_drm_plane_destroy_state(struct drm_plane *plane,
>  	kfree(old_exynos_state);
>  }
>  
> +static int exynos_drm_plane_atomic_set_property(struct drm_plane *plane,
> +						struct drm_plane_state *state,
> +						struct drm_property *property,
> +						uint64_t val)
> +{
> +	struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
> +	struct exynos_drm_plane_state *exynos_state =
> +					to_exynos_plane_state(state);
> +	struct exynos_drm_private *dev_priv = plane->dev->dev_private;
> +	const struct exynos_drm_plane_config *config = exynos_plane->config;
> +
> +	if (property == dev_priv->plane_zpos_property &&
> +	    (config->capabilities & EXYNOS_DRM_PLANE_CAP_ZPOS))
> +		exynos_state->zpos = val;
> +	else
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int exynos_drm_plane_atomic_get_property(struct drm_plane *plane,
> +					  const struct drm_plane_state *state,
> +					  struct drm_property *property,
> +					  uint64_t *val)
> +{
> +	const struct exynos_drm_plane_state *exynos_state =
> +		container_of(state, const struct exynos_drm_plane_state, base);
> +	struct exynos_drm_private *dev_priv = plane->dev->dev_private;
> +
> +	if (property == dev_priv->plane_zpos_property)
> +		*val = exynos_state->zpos;
> +	else
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  static struct drm_plane_funcs exynos_plane_funcs = {
>  	.update_plane	= drm_atomic_helper_update_plane,
>  	.disable_plane	= drm_atomic_helper_disable_plane,
>  	.destroy	= drm_plane_cleanup,
> +	.set_property	= drm_atomic_helper_plane_set_property,
>  	.reset		= exynos_drm_plane_reset,
>  	.atomic_duplicate_state = exynos_drm_plane_duplicate_state,
>  	.atomic_destroy_state = exynos_drm_plane_destroy_state,
> +	.atomic_set_property = exynos_drm_plane_atomic_set_property,
> +	.atomic_get_property = exynos_drm_plane_atomic_get_property,
>  };
>  
>  static int
> @@ -267,8 +310,8 @@ static void exynos_plane_attach_zpos_property(struct drm_plane *plane,
>  
>  	prop = dev_priv->plane_zpos_property;
>  	if (!prop) {
> -		prop = drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE,
> -						 "zpos", 0, MAX_PLANE - 1);
> +		prop = drm_property_create_range(dev, 0, "zpos",
> +						 0, MAX_PLANE - 1);
>  		if (!prop)
>  			return;
>  
> @@ -302,9 +345,7 @@ int exynos_plane_init(struct drm_device *dev,
>  	exynos_plane->index = index;
>  	exynos_plane->config = config;
>  
> -	if (config->type == DRM_PLANE_TYPE_OVERLAY)
> -		exynos_plane_attach_zpos_property(&exynos_plane->base,
> -						  config->zpos);
> +	exynos_plane_attach_zpos_property(&exynos_plane->base, config->zpos);
>  
>  	return 0;
>  }
> -- 
> 1.9.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v3 2/7] drm/exynos: make zpos property configurable

[Ville Syrjälä]

On Wed, Dec 16, 2015 at 02:28:36PM +0100, Daniel Vetter wrote:
> On Wed, Dec 16, 2015 at 01:21:43PM +0100, Marek Szyprowski wrote:
> > This patch adds all infrastructure to make zpos plane property
> > configurable from userspace.
> > 
> > Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> 
> Imo zpos should be a generic atomic property with well-defined semantics
> shared across drivers. So
> - storead (in decoded form) in drm_plane_state
> - common functions to register/attach zpos prop to a plane, with
>   full-blown kerneldoc explaining how it should work
> - generic kms igt to validate that interface
> 
> One of the big things that always comes up when we talk about zpos is how
> equal zpos should be handled, and how exactly they should be sorted. I
> think for that we should have a drm function which computes a normalized
> zpos. Or the core check code should reject such nonsense.

Yeah I think just having some core check reject the operation if two
planes end up with the same zpos. And zpos should probably just be 
in the [0-number_of_planes_on_the_crtc_currently] range? Or maybe
[0-max_number_of_planes_possible_on_the_crtc], or just
[0-total_max_number_of_planes] ? One of the latter two might be sensible
because you could then enable/disable some planes on the crtc without
necessarily touching the zpos for the other planes.

Another complication is how you think about color keying. Eg. if you use
dst keying between the primary plane and some other plane, then it may
make sense to think of the primary being above the other plane and the
colorkey just punches a hole into the primary. But if the planes
otherwise are always ordered with the primary at the botton, then I'm
not sure if exposing zpos and requiring it to be reconfigured for
colorkeying to work would just make things more confusing. But this
decisions might also depend on what happens to pixels on the other plane
that fall outside of the primary plane (assuming the primary can be
non-fullscreen). I don't remember off the top of my head how this works
on Intel hw. The other option I suppose is to define color keying as
just being slightly magic that it can effectively reorder the planes
even though zpos says something else. Not quite sure which is best.

Dst color keying is rather confusing in any case since it may work
only between certain planes (and exactly which planes may depend which
planes are active on the same crtc).

> -Daniel
> 
> > ---
> >  drivers/gpu/drm/exynos/exynos_drm_drv.h   |  4 ++-
> >  drivers/gpu/drm/exynos/exynos_drm_plane.c | 51 ++++++++++++++++++++++++++++---
> >  2 files changed, 49 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> > index 588b6763f9c7..f0827dbebb7d 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> > @@ -64,6 +64,7 @@ struct exynos_drm_plane_state {
> >  	struct exynos_drm_rect src;
> >  	unsigned int h_ratio;
> >  	unsigned int v_ratio;
> > +	unsigned int zpos;
> >  };
> >  
> >  static inline struct exynos_drm_plane_state *
> > @@ -91,11 +92,12 @@ struct exynos_drm_plane {
> >  
> >  #define EXYNOS_DRM_PLANE_CAP_DOUBLE	(1 << 0)
> >  #define EXYNOS_DRM_PLANE_CAP_SCALE	(1 << 1)
> > +#define EXYNOS_DRM_PLANE_CAP_ZPOS	(1 << 2)
> >  
> >  /*
> >   * Exynos DRM plane configuration structure.
> >   *
> > - * @zpos: z-position of the plane.
> > + * @zpos: initial z-position of the plane.
> >   * @type: type of the plane (primary, cursor or overlay).
> >   * @pixel_formats: supported pixel formats.
> >   * @num_pixel_formats: number of elements in 'pixel_formats'.
> > diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
> > index fd6cb4cee01a..a2bdab836b50 100644
> > --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
> > +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
> > @@ -124,6 +124,7 @@ static void exynos_plane_mode_set(struct exynos_drm_plane_state *exynos_state)
> >  
> >  static void exynos_drm_plane_reset(struct drm_plane *plane)
> >  {
> > +	struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
> >  	struct exynos_drm_plane_state *exynos_state;
> >  
> >  	if (plane->state) {
> > @@ -136,6 +137,7 @@ static void exynos_drm_plane_reset(struct drm_plane *plane)
> >  
> >  	exynos_state = kzalloc(sizeof(*exynos_state), GFP_KERNEL);
> >  	if (exynos_state) {
> > +		exynos_state->zpos = exynos_plane->config->zpos;
> >  		plane->state = &exynos_state->base;
> >  		plane->state->plane = plane;
> >  	}
> > @@ -153,6 +155,7 @@ exynos_drm_plane_duplicate_state(struct drm_plane *plane)
> >  		return NULL;
> >  
> >  	__drm_atomic_helper_plane_duplicate_state(plane, &copy->base);
> > +	copy->zpos = exynos_state->zpos;
> >  	return &copy->base;
> >  }
> >  
> > @@ -165,13 +168,53 @@ static void exynos_drm_plane_destroy_state(struct drm_plane *plane,
> >  	kfree(old_exynos_state);
> >  }
> >  
> > +static int exynos_drm_plane_atomic_set_property(struct drm_plane *plane,
> > +						struct drm_plane_state *state,
> > +						struct drm_property *property,
> > +						uint64_t val)
> > +{
> > +	struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
> > +	struct exynos_drm_plane_state *exynos_state =
> > +					to_exynos_plane_state(state);
> > +	struct exynos_drm_private *dev_priv = plane->dev->dev_private;
> > +	const struct exynos_drm_plane_config *config = exynos_plane->config;
> > +
> > +	if (property == dev_priv->plane_zpos_property &&
> > +	    (config->capabilities & EXYNOS_DRM_PLANE_CAP_ZPOS))
> > +		exynos_state->zpos = val;
> > +	else
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> > +static int exynos_drm_plane_atomic_get_property(struct drm_plane *plane,
> > +					  const struct drm_plane_state *state,
> > +					  struct drm_property *property,
> > +					  uint64_t *val)
> > +{
> > +	const struct exynos_drm_plane_state *exynos_state =
> > +		container_of(state, const struct exynos_drm_plane_state, base);
> > +	struct exynos_drm_private *dev_priv = plane->dev->dev_private;
> > +
> > +	if (property == dev_priv->plane_zpos_property)
> > +		*val = exynos_state->zpos;
> > +	else
> > +		return -EINVAL;
> > +
> > +	return 0;
> > +}
> > +
> >  static struct drm_plane_funcs exynos_plane_funcs = {
> >  	.update_plane	= drm_atomic_helper_update_plane,
> >  	.disable_plane	= drm_atomic_helper_disable_plane,
> >  	.destroy	= drm_plane_cleanup,
> > +	.set_property	= drm_atomic_helper_plane_set_property,
> >  	.reset		= exynos_drm_plane_reset,
> >  	.atomic_duplicate_state = exynos_drm_plane_duplicate_state,
> >  	.atomic_destroy_state = exynos_drm_plane_destroy_state,
> > +	.atomic_set_property = exynos_drm_plane_atomic_set_property,
> > +	.atomic_get_property = exynos_drm_plane_atomic_get_property,
> >  };
> >  
> >  static int
> > @@ -267,8 +310,8 @@ static void exynos_plane_attach_zpos_property(struct drm_plane *plane,
> >  
> >  	prop = dev_priv->plane_zpos_property;
> >  	if (!prop) {
> > -		prop = drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE,
> > -						 "zpos", 0, MAX_PLANE - 1);
> > +		prop = drm_property_create_range(dev, 0, "zpos",
> > +						 0, MAX_PLANE - 1);
> >  		if (!prop)
> >  			return;
> >  
> > @@ -302,9 +345,7 @@ int exynos_plane_init(struct drm_device *dev,
> >  	exynos_plane->index = index;
> >  	exynos_plane->config = config;
> >  
> > -	if (config->type == DRM_PLANE_TYPE_OVERLAY)
> > -		exynos_plane_attach_zpos_property(&exynos_plane->base,
> > -						  config->zpos);
> > +	exynos_plane_attach_zpos_property(&exynos_plane->base, config->zpos);
> >  
> >  	return 0;
> >  }
> > -- 
> > 1.9.2
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/dri-devel
> 
> -- 
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH v3 2/7] drm/exynos: make zpos property configurable

[Marek Szyprowski]

Hello,

On 2015-12-16 14:28, Daniel Vetter wrote:
> On Wed, Dec 16, 2015 at 01:21:43PM +0100, Marek Szyprowski wrote:
>> This patch adds all infrastructure to make zpos plane property
>> configurable from userspace.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> Imo zpos should be a generic atomic property with well-defined semantics
> shared across drivers. So
> - storead (in decoded form) in drm_plane_state
> - common functions to register/attach zpos prop to a plane, with
>    full-blown kerneldoc explaining how it should work
> - generic kms igt to validate that interface
>
> One of the big things that always comes up when we talk about zpos is how
> equal zpos should be handled, and how exactly they should be sorted. I
> think for that we should have a drm function which computes a normalized
> zpos. Or the core check code should reject such nonsense.

IMHO it will be enough to state that the case of equal zpos is HW specific.
This might simplify some driver logic. For example, in case of Exynos Mixer,
equal priority (zpos) means that HW predefined order will be used, so there
is no need to normalize zpos values.

If you want I can move zpos to drm core and add a function to normalize 
zpos,
although for this particular driver normalization is not needed.

It should be quite easy to convert other drivers to use the generic zpos
property. The only problem I see is how to handle omap driver, which use
'zorder' property.

What about some other typical properties related to blending:
- global plane alpha,
- colorkey,
- alpha mode (standard or pre-multiplied) for alpha-enabled modes,
- crtc background color.

Do you also want to handle them as generic or driver-specific properties?

I plan to add support for them to Exynos Mixer and I would like to avoid
doing this twice.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

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

Re: [PATCH v3 2/7] drm/exynos: make zpos property configurable

[Daniel Vetter]

On Wed, Dec 16, 2015 at 02:54:04PM +0100, Marek Szyprowski wrote:
> Hello,
> 
> On 2015-12-16 14:28, Daniel Vetter wrote:
> >On Wed, Dec 16, 2015 at 01:21:43PM +0100, Marek Szyprowski wrote:
> >>This patch adds all infrastructure to make zpos plane property
> >>configurable from userspace.
> >>
> >>Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> >Imo zpos should be a generic atomic property with well-defined semantics
> >shared across drivers. So
> >- storead (in decoded form) in drm_plane_state
> >- common functions to register/attach zpos prop to a plane, with
> >   full-blown kerneldoc explaining how it should work
> >- generic kms igt to validate that interface
> >
> >One of the big things that always comes up when we talk about zpos is how
> >equal zpos should be handled, and how exactly they should be sorted. I
> >think for that we should have a drm function which computes a normalized
> >zpos. Or the core check code should reject such nonsense.
> 
> IMHO it will be enough to state that the case of equal zpos is HW specific.
> This might simplify some driver logic. For example, in case of Exynos Mixer,
> equal priority (zpos) means that HW predefined order will be used, so there
> is no need to normalize zpos values.
> 
> If you want I can move zpos to drm core and add a function to normalize
> zpos,
> although for this particular driver normalization is not needed.
> 
> It should be quite easy to convert other drivers to use the generic zpos
> property. The only problem I see is how to handle omap driver, which use
> 'zorder' property.
> 
> What about some other typical properties related to blending:
> - global plane alpha,
> - colorkey,
> - alpha mode (standard or pre-multiplied) for alpha-enabled modes,
> - crtc background color.
> 
> Do you also want to handle them as generic or driver-specific properties?

Should all be generic really. And it's also kinda ABI, so userspace
needed, and preferrably a proper/automated testcase. i915 has
infrastructure to use display pipeline CRCs to really measure it's all
correct even, and that's the standard I'd like to see for all KMS API
extensions like this.

Since if we don't do this we'll end up in a giant mess, and it will be
impossible to write a kms compositor that's generic and uses all this.

And since this stuff is supposed to be generic, fluffy/unspecified
semantics aren't good. Especially if your hw can just somehow deal with it
all.

> I plan to add support for them to Exynos Mixer and I would like to avoid
> doing this twice.

It's a lot more work than just adding a few members to drm_plane_state.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v3 2/7] drm/exynos: make zpos property configurable

[Marek Szyprowski]

Hello,

On 2015-12-16 15:21, Daniel Vetter wrote:
> On Wed, Dec 16, 2015 at 02:54:04PM +0100, Marek Szyprowski wrote:
>> On 2015-12-16 14:28, Daniel Vetter wrote:
>>> On Wed, Dec 16, 2015 at 01:21:43PM +0100, Marek Szyprowski wrote:
>>>> This patch adds all infrastructure to make zpos plane property
>>>> configurable from userspace.
>>>>
>>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> Imo zpos should be a generic atomic property with well-defined semantics
>>> shared across drivers. So
>>> - storead (in decoded form) in drm_plane_state
>>> - common functions to register/attach zpos prop to a plane, with
>>>    full-blown kerneldoc explaining how it should work
>>> - generic kms igt to validate that interface
>>>
>>> One of the big things that always comes up when we talk about zpos is how
>>> equal zpos should be handled, and how exactly they should be sorted. I
>>> think for that we should have a drm function which computes a normalized
>>> zpos. Or the core check code should reject such nonsense.
>> IMHO it will be enough to state that the case of equal zpos is HW specific.
>> This might simplify some driver logic. For example, in case of Exynos Mixer,
>> equal priority (zpos) means that HW predefined order will be used, so there
>> is no need to normalize zpos values.
>>
>> If you want I can move zpos to drm core and add a function to normalize
>> zpos,
>> although for this particular driver normalization is not needed.
>>
>> It should be quite easy to convert other drivers to use the generic zpos
>> property. The only problem I see is how to handle omap driver, which use
>> 'zorder' property.
>>
>> What about some other typical properties related to blending:
>> - global plane alpha,
>> - colorkey,
>> - alpha mode (standard or pre-multiplied) for alpha-enabled modes,
>> - crtc background color.
>>
>> Do you also want to handle them as generic or driver-specific properties?
> Should all be generic really. And it's also kinda ABI, so userspace
> needed, and preferrably a proper/automated testcase. i915 has
> infrastructure to use display pipeline CRCs to really measure it's all
> correct even, and that's the standard I'd like to see for all KMS API
> extensions like this.

Could you tell a bit more about this pipeline CRCs? What is it?

> Since if we don't do this we'll end up in a giant mess, and it will be
> impossible to write a kms compositor that's generic and uses all this.
>
> And since this stuff is supposed to be generic, fluffy/unspecified
> semantics aren't good. Especially if your hw can just somehow deal with it
> all.
>
>> I plan to add support for them to Exynos Mixer and I would like to avoid
>> doing this twice.
> It's a lot more work than just adding a few members to drm_plane_state.

I see. Could you elaborate a bit more on what you want to have in drm 
core for
handling all the mentioned features?

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v3 2/7] drm/exynos: make zpos property configurable

[Joonyoung Shim]

+Cc: Boram Park,

Hi Marek,

On 12/16/2015 09:21 PM, Marek Szyprowski wrote:
> This patch adds all infrastructure to make zpos plane property
> configurable from userspace.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_drm_drv.h   |  4 ++-
>  drivers/gpu/drm/exynos/exynos_drm_plane.c | 51 ++++++++++++++++++++++++++++---
>  2 files changed, 49 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> index 588b6763f9c7..f0827dbebb7d 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> @@ -64,6 +64,7 @@ struct exynos_drm_plane_state {
>  	struct exynos_drm_rect src;
>  	unsigned int h_ratio;
>  	unsigned int v_ratio;
> +	unsigned int zpos;
>  };
>  
>  static inline struct exynos_drm_plane_state *
> @@ -91,11 +92,12 @@ struct exynos_drm_plane {
>  
>  #define EXYNOS_DRM_PLANE_CAP_DOUBLE	(1 << 0)
>  #define EXYNOS_DRM_PLANE_CAP_SCALE	(1 << 1)
> +#define EXYNOS_DRM_PLANE_CAP_ZPOS	(1 << 2)
>  
>  /*
>   * Exynos DRM plane configuration structure.
>   *
> - * @zpos: z-position of the plane.
> + * @zpos: initial z-position of the plane.
>   * @type: type of the plane (primary, cursor or overlay).
>   * @pixel_formats: supported pixel formats.
>   * @num_pixel_formats: number of elements in 'pixel_formats'.
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
> index fd6cb4cee01a..a2bdab836b50 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
> @@ -124,6 +124,7 @@ static void exynos_plane_mode_set(struct exynos_drm_plane_state *exynos_state)
>  
>  static void exynos_drm_plane_reset(struct drm_plane *plane)
>  {
> +	struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
>  	struct exynos_drm_plane_state *exynos_state;
>  
>  	if (plane->state) {
> @@ -136,6 +137,7 @@ static void exynos_drm_plane_reset(struct drm_plane *plane)
>  
>  	exynos_state = kzalloc(sizeof(*exynos_state), GFP_KERNEL);
>  	if (exynos_state) {
> +		exynos_state->zpos = exynos_plane->config->zpos;
>  		plane->state = &exynos_state->base;
>  		plane->state->plane = plane;
>  	}
> @@ -153,6 +155,7 @@ exynos_drm_plane_duplicate_state(struct drm_plane *plane)
>  		return NULL;
>  
>  	__drm_atomic_helper_plane_duplicate_state(plane, &copy->base);
> +	copy->zpos = exynos_state->zpos;
>  	return &copy->base;
>  }
>  
> @@ -165,13 +168,53 @@ static void exynos_drm_plane_destroy_state(struct drm_plane *plane,
>  	kfree(old_exynos_state);
>  }
>  
> +static int exynos_drm_plane_atomic_set_property(struct drm_plane *plane,
> +						struct drm_plane_state *state,
> +						struct drm_property *property,
> +						uint64_t val)
> +{
> +	struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
> +	struct exynos_drm_plane_state *exynos_state =
> +					to_exynos_plane_state(state);
> +	struct exynos_drm_private *dev_priv = plane->dev->dev_private;
> +	const struct exynos_drm_plane_config *config = exynos_plane->config;
> +
> +	if (property == dev_priv->plane_zpos_property &&
> +	    (config->capabilities & EXYNOS_DRM_PLANE_CAP_ZPOS))
> +		exynos_state->zpos = val;
> +	else
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
> +static int exynos_drm_plane_atomic_get_property(struct drm_plane *plane,
> +					  const struct drm_plane_state *state,
> +					  struct drm_property *property,
> +					  uint64_t *val)
> +{
> +	const struct exynos_drm_plane_state *exynos_state =
> +		container_of(state, const struct exynos_drm_plane_state, base);
> +	struct exynos_drm_private *dev_priv = plane->dev->dev_private;
> +
> +	if (property == dev_priv->plane_zpos_property)
> +		*val = exynos_state->zpos;
> +	else
> +		return -EINVAL;
> +
> +	return 0;
> +}
> +
>  static struct drm_plane_funcs exynos_plane_funcs = {
>  	.update_plane	= drm_atomic_helper_update_plane,
>  	.disable_plane	= drm_atomic_helper_disable_plane,
>  	.destroy	= drm_plane_cleanup,
> +	.set_property	= drm_atomic_helper_plane_set_property,
>  	.reset		= exynos_drm_plane_reset,
>  	.atomic_duplicate_state = exynos_drm_plane_duplicate_state,
>  	.atomic_destroy_state = exynos_drm_plane_destroy_state,
> +	.atomic_set_property = exynos_drm_plane_atomic_set_property,
> +	.atomic_get_property = exynos_drm_plane_atomic_get_property,
>  };
>  
>  static int
> @@ -267,8 +310,8 @@ static void exynos_plane_attach_zpos_property(struct drm_plane *plane,
>  
>  	prop = dev_priv->plane_zpos_property;
>  	if (!prop) {
> -		prop = drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE,
> -						 "zpos", 0, MAX_PLANE - 1);
> +		prop = drm_property_create_range(dev, 0, "zpos",
> +						 0, MAX_PLANE - 1);

User are using zpos property as index now, if we make zpos property
configurable, i think we need a property immutable like index or a
property like type or name that can know this plane is which plane
(e.g. video plane or graphic plane).

Another way, user may be possible to check plane index as order of
plane object id.

Thanks.

>  		if (!prop)
>  			return;
>  
> @@ -302,9 +345,7 @@ int exynos_plane_init(struct drm_device *dev,
>  	exynos_plane->index = index;
>  	exynos_plane->config = config;
>  
> -	if (config->type == DRM_PLANE_TYPE_OVERLAY)
> -		exynos_plane_attach_zpos_property(&exynos_plane->base,
> -						  config->zpos);
> +	exynos_plane_attach_zpos_property(&exynos_plane->base, config->zpos);
>  
>  	return 0;
>  }
> 

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

Re: [PATCH v3 5/7] drm/exynos: mixer: refactor layer setup

[Joonyoung Shim]

Hi Marek,

On 12/16/2015 09:21 PM, Marek Szyprowski wrote:
> Properly configure blending properties of given hardware layer based on
> the selected pixel format. Currently only per-pixel-based alpha is possible
> when respective pixel format has been selected. Configuration of global,
> per-plane alpha value, color key and background color will be added later.
> 
> This patch is heavily inspired by earlier work done by Tobias Jakobi
> <tjakobi@math.uni-bielefeld.de>.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos_mixer.c | 43 +++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/exynos/regs-mixer.h   |  1 +
>  2 files changed, 44 insertions(+)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> index c572e271579e..ae7b122274ac 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> @@ -165,6 +165,16 @@ static const u8 filter_cr_horiz_tap4[] = {
>  	70,	59,	48,	37,	27,	19,	11,	5,
>  };
>  
> +static inline bool is_alpha_format(unsigned int pixel_format)
> +{
> +	switch (pixel_format) {
> +	case DRM_FORMAT_ARGB8888:
> +		return true;
> +	default:
> +		return false;
> +	}
> +}
> +
>  static inline u32 vp_reg_read(struct mixer_resources *res, u32 reg_id)
>  {
>  	return readl(res->vp_regs + reg_id);
> @@ -294,6 +304,37 @@ static void vp_default_filter(struct mixer_resources *res)
>  		filter_cr_horiz_tap4, sizeof(filter_cr_horiz_tap4));
>  }
>  
> +static void mixer_cfg_gfx_blend(struct mixer_context *ctx, unsigned int win,
> +				bool alpha)
> +{
> +	struct mixer_resources *res = &ctx->mixer_res;
> +	u32 val;
> +
> +	val  = MXR_GRP_CFG_COLOR_KEY_DISABLE; /* no blank key */
> +	if (alpha) {
> +		/* blending based on pixel alpha */
> +		val |= MXR_GRP_CFG_BLEND_PRE_MUL;
> +		val |= MXR_GRP_CFG_PIXEL_BLEND_EN;
> +	}
> +	mixer_reg_writemask(res, MXR_GRAPHIC_CFG(win),
> +			    val, MXR_GRP_CFG_MISC_MASK);

I think the priority of plane and whether vp layer exists should be
considered for blending setting. When priority of graphic layer0 is 
lowest and vp layer is not, this will blend background layer. 
It was not permitted to blend background layer until current.

Thanks.

> +}
> +
> +static void mixer_cfg_vp_blend(struct mixer_context *ctx)
> +{
> +	struct mixer_resources *res = &ctx->mixer_res;
> +	u32 val;
> +
> +	/*
> +	 * No blending at the moment since the NV12/NV21 pixelformats don't
> +	 * have an alpha channel. However the mixer supports a global alpha
> +	 * value for a layer. Once this functionality is exposed, we can
> +	 * support blending of the video layer through this.
> +	 */
> +	val = 0;
> +	mixer_reg_write(res, MXR_VIDEO_CFG, val);
> +}
> +
>  static void mixer_vsync_set_update(struct mixer_context *ctx, bool enable)
>  {
>  	struct mixer_resources *res = &ctx->mixer_res;
> @@ -519,6 +560,7 @@ static void vp_video_buffer(struct mixer_context *ctx,
>  	mixer_cfg_scan(ctx, mode->vdisplay);
>  	mixer_cfg_rgb_fmt(ctx, mode->vdisplay);
>  	mixer_cfg_layer(ctx, plane->index, state->zpos + 1, true);
> +	mixer_cfg_vp_blend(ctx);
>  	mixer_run(ctx);
>  
>  	mixer_vsync_set_update(ctx, true);
> @@ -634,6 +676,7 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
>  	mixer_cfg_scan(ctx, mode->vdisplay);
>  	mixer_cfg_rgb_fmt(ctx, mode->vdisplay);
>  	mixer_cfg_layer(ctx, win, state->zpos + 1, true);
> +	mixer_cfg_gfx_blend(ctx, win, is_alpha_format(fb->pixel_format));
>  
>  	/* layer update mandatory for mixer 16.0.33.0 */
>  	if (ctx->mxr_ver == MXR_VER_16_0_33_0 ||
> diff --git a/drivers/gpu/drm/exynos/regs-mixer.h b/drivers/gpu/drm/exynos/regs-mixer.h
> index dbdbc0af3358..7f22df5bf707 100644
> --- a/drivers/gpu/drm/exynos/regs-mixer.h
> +++ b/drivers/gpu/drm/exynos/regs-mixer.h
> @@ -113,6 +113,7 @@
>  #define MXR_GRP_CFG_BLEND_PRE_MUL	(1 << 20)
>  #define MXR_GRP_CFG_WIN_BLEND_EN	(1 << 17)
>  #define MXR_GRP_CFG_PIXEL_BLEND_EN	(1 << 16)
> +#define MXR_GRP_CFG_MISC_MASK		((3 << 16) | (3 << 20))
>  #define MXR_GRP_CFG_FORMAT_VAL(x)	MXR_MASK_VAL(x, 11, 8)
>  #define MXR_GRP_CFG_FORMAT_MASK		MXR_GRP_CFG_FORMAT_VAL(~0)
>  #define MXR_GRP_CFG_ALPHA_VAL(x)	MXR_MASK_VAL(x, 7, 0)
> 

Re: [PATCH v3 2/7] drm/exynos: make zpos property configurable

[Marek Szyprowski]

Hello,

On 2015-12-17 03:55, Joonyoung Shim wrote:
> +Cc: Boram Park,
>
> Hi Marek,
>
> On 12/16/2015 09:21 PM, Marek Szyprowski wrote:
>> This patch adds all infrastructure to make zpos plane property
>> configurable from userspace.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>   drivers/gpu/drm/exynos/exynos_drm_drv.h   |  4 ++-
>>   drivers/gpu/drm/exynos/exynos_drm_plane.c | 51 ++++++++++++++++++++++++++++---
>>   2 files changed, 49 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
>> index 588b6763f9c7..f0827dbebb7d 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
>> @@ -64,6 +64,7 @@ struct exynos_drm_plane_state {
>>   	struct exynos_drm_rect src;
>>   	unsigned int h_ratio;
>>   	unsigned int v_ratio;
>> +	unsigned int zpos;
>>   };
>>   
>>   static inline struct exynos_drm_plane_state *
>> @@ -91,11 +92,12 @@ struct exynos_drm_plane {
>>   
>>   #define EXYNOS_DRM_PLANE_CAP_DOUBLE	(1 << 0)
>>   #define EXYNOS_DRM_PLANE_CAP_SCALE	(1 << 1)
>> +#define EXYNOS_DRM_PLANE_CAP_ZPOS	(1 << 2)
>>   
>>   /*
>>    * Exynos DRM plane configuration structure.
>>    *
>> - * @zpos: z-position of the plane.
>> + * @zpos: initial z-position of the plane.
>>    * @type: type of the plane (primary, cursor or overlay).
>>    * @pixel_formats: supported pixel formats.
>>    * @num_pixel_formats: number of elements in 'pixel_formats'.
>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>> index fd6cb4cee01a..a2bdab836b50 100644
>> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
>> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>> @@ -124,6 +124,7 @@ static void exynos_plane_mode_set(struct exynos_drm_plane_state *exynos_state)
>>   
>>   static void exynos_drm_plane_reset(struct drm_plane *plane)
>>   {
>> +	struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
>>   	struct exynos_drm_plane_state *exynos_state;
>>   
>>   	if (plane->state) {
>> @@ -136,6 +137,7 @@ static void exynos_drm_plane_reset(struct drm_plane *plane)
>>   
>>   	exynos_state = kzalloc(sizeof(*exynos_state), GFP_KERNEL);
>>   	if (exynos_state) {
>> +		exynos_state->zpos = exynos_plane->config->zpos;
>>   		plane->state = &exynos_state->base;
>>   		plane->state->plane = plane;
>>   	}
>> @@ -153,6 +155,7 @@ exynos_drm_plane_duplicate_state(struct drm_plane *plane)
>>   		return NULL;
>>   
>>   	__drm_atomic_helper_plane_duplicate_state(plane, &copy->base);
>> +	copy->zpos = exynos_state->zpos;
>>   	return &copy->base;
>>   }
>>   
>> @@ -165,13 +168,53 @@ static void exynos_drm_plane_destroy_state(struct drm_plane *plane,
>>   	kfree(old_exynos_state);
>>   }
>>   
>> +static int exynos_drm_plane_atomic_set_property(struct drm_plane *plane,
>> +						struct drm_plane_state *state,
>> +						struct drm_property *property,
>> +						uint64_t val)
>> +{
>> +	struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
>> +	struct exynos_drm_plane_state *exynos_state =
>> +					to_exynos_plane_state(state);
>> +	struct exynos_drm_private *dev_priv = plane->dev->dev_private;
>> +	const struct exynos_drm_plane_config *config = exynos_plane->config;
>> +
>> +	if (property == dev_priv->plane_zpos_property &&
>> +	    (config->capabilities & EXYNOS_DRM_PLANE_CAP_ZPOS))
>> +		exynos_state->zpos = val;
>> +	else
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>> +static int exynos_drm_plane_atomic_get_property(struct drm_plane *plane,
>> +					  const struct drm_plane_state *state,
>> +					  struct drm_property *property,
>> +					  uint64_t *val)
>> +{
>> +	const struct exynos_drm_plane_state *exynos_state =
>> +		container_of(state, const struct exynos_drm_plane_state, base);
>> +	struct exynos_drm_private *dev_priv = plane->dev->dev_private;
>> +
>> +	if (property == dev_priv->plane_zpos_property)
>> +		*val = exynos_state->zpos;
>> +	else
>> +		return -EINVAL;
>> +
>> +	return 0;
>> +}
>> +
>>   static struct drm_plane_funcs exynos_plane_funcs = {
>>   	.update_plane	= drm_atomic_helper_update_plane,
>>   	.disable_plane	= drm_atomic_helper_disable_plane,
>>   	.destroy	= drm_plane_cleanup,
>> +	.set_property	= drm_atomic_helper_plane_set_property,
>>   	.reset		= exynos_drm_plane_reset,
>>   	.atomic_duplicate_state = exynos_drm_plane_duplicate_state,
>>   	.atomic_destroy_state = exynos_drm_plane_destroy_state,
>> +	.atomic_set_property = exynos_drm_plane_atomic_set_property,
>> +	.atomic_get_property = exynos_drm_plane_atomic_get_property,
>>   };
>>   
>>   static int
>> @@ -267,8 +310,8 @@ static void exynos_plane_attach_zpos_property(struct drm_plane *plane,
>>   
>>   	prop = dev_priv->plane_zpos_property;
>>   	if (!prop) {
>> -		prop = drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE,
>> -						 "zpos", 0, MAX_PLANE - 1);
>> +		prop = drm_property_create_range(dev, 0, "zpos",
>> +						 0, MAX_PLANE - 1);
> User are using zpos property as index now, if we make zpos property
> configurable, i think we need a property immutable like index or a
> property like type or name that can know this plane is which plane
> (e.g. video plane or graphic plane).

I don't get this. zpos property is attached only to OVERLAY planes, 
PRIMARY and CURSOR don't have it, so userspace cannot rely on this 
property. In my patch the initial zpos value is same as before, so if 
application doesn't relies on zpos values, it will get the same values 
as now. Application can also check the plane type by reading 'type' 
property (PRIMARY, OVERLAY or CURSOR). However after my patch 
application will get the possibility to rearrange plane order for the 
given crtc by chaning zpos values. That's especially useful to configure 
2 typical use cases: PRIMARY plane over OVERLAY plane (PRIMARY is used 
as typical on-screen-display) or OVERLAY plane over PRIMARY (OVERLAY is 
used as a window for displaying video data).

> Another way, user may be possible to check plane index as order of
> plane object id.

Application can easily find all the planes, which belongs to given crtc 
by checking 'possible crtcs' plane parameter, I see no additional need 
for 'index' property in such case. Please note that the initial plane 
configuration in case of atomic api will be always the same for all 
application (determined by exynos_drm_plane_reset function) and it will 
match the current setup.

>
> Thanks.
>
>>   		if (!prop)
>>   			return;
>>   
>> @@ -302,9 +345,7 @@ int exynos_plane_init(struct drm_device *dev,
>>   	exynos_plane->index = index;
>>   	exynos_plane->config = config;
>>   
>> -	if (config->type == DRM_PLANE_TYPE_OVERLAY)
>> -		exynos_plane_attach_zpos_property(&exynos_plane->base,
>> -						  config->zpos);
>> +	exynos_plane_attach_zpos_property(&exynos_plane->base, config->zpos);
>>   
>>   	return 0;
>>   }
>>
>

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v3 5/7] drm/exynos: mixer: refactor layer setup

[Marek Szyprowski]

Hi Joonyoung,

On 2015-12-17 05:19, Joonyoung Shim wrote:
> Hi Marek,
>
> On 12/16/2015 09:21 PM, Marek Szyprowski wrote:
>> Properly configure blending properties of given hardware layer based on
>> the selected pixel format. Currently only per-pixel-based alpha is possible
>> when respective pixel format has been selected. Configuration of global,
>> per-plane alpha value, color key and background color will be added later.
>>
>> This patch is heavily inspired by earlier work done by Tobias Jakobi
>> <tjakobi@math.uni-bielefeld.de>.
>>
>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>> ---
>>   drivers/gpu/drm/exynos/exynos_mixer.c | 43 +++++++++++++++++++++++++++++++++++
>>   drivers/gpu/drm/exynos/regs-mixer.h   |  1 +
>>   2 files changed, 44 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
>> index c572e271579e..ae7b122274ac 100644
>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
>> @@ -165,6 +165,16 @@ static const u8 filter_cr_horiz_tap4[] = {
>>   	70,	59,	48,	37,	27,	19,	11,	5,
>>   };
>>   
>> +static inline bool is_alpha_format(unsigned int pixel_format)
>> +{
>> +	switch (pixel_format) {
>> +	case DRM_FORMAT_ARGB8888:
>> +		return true;
>> +	default:
>> +		return false;
>> +	}
>> +}
>> +
>>   static inline u32 vp_reg_read(struct mixer_resources *res, u32 reg_id)
>>   {
>>   	return readl(res->vp_regs + reg_id);
>> @@ -294,6 +304,37 @@ static void vp_default_filter(struct mixer_resources *res)
>>   		filter_cr_horiz_tap4, sizeof(filter_cr_horiz_tap4));
>>   }
>>   
>> +static void mixer_cfg_gfx_blend(struct mixer_context *ctx, unsigned int win,
>> +				bool alpha)
>> +{
>> +	struct mixer_resources *res = &ctx->mixer_res;
>> +	u32 val;
>> +
>> +	val  = MXR_GRP_CFG_COLOR_KEY_DISABLE; /* no blank key */
>> +	if (alpha) {
>> +		/* blending based on pixel alpha */
>> +		val |= MXR_GRP_CFG_BLEND_PRE_MUL;
>> +		val |= MXR_GRP_CFG_PIXEL_BLEND_EN;
>> +	}
>> +	mixer_reg_writemask(res, MXR_GRAPHIC_CFG(win),
>> +			    val, MXR_GRP_CFG_MISC_MASK);
> I think the priority of plane and whether vp layer exists should be
> considered for blending setting. When priority of graphic layer0 is
> lowest and vp layer is not, this will blend background layer.
> It was not permitted to blend background layer until current.

Currently blending is hardcoded to following configuration:
1. Order: [top] Grp1 > Grp0 > Video [bottom]
2. Per-pixel alpha blending enabled unconditionally for Grp1 layer 
(regardless
    of the selected pixel format for Grp1 layer).
3. Per-pixel alpha blending enabled for Grp0 layer when Video layer gets 
enabled
    (regardless of the selected pixel format for Grp0 layer).

It is not very intuitive and it looks hardcoded for one particular use case.
With the above patch application can configure blending for its needs.

I really see no reason for special handling of the bottom layer (like
disabling per-pixel alpha even if alpha-enabled format is selected). It is
role of application to set proper pixel format (like XRGB8888 instead of
ARGB8888) if the application is not interested in alpha blending. Per-pixel
alpha blending is enabled only for formats which really support alpha.

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v3 2/7] drm/exynos: make zpos property configurable

[Joonyoung Shim]

On 12/17/2015 10:05 PM, Marek Szyprowski wrote:
> Hello,
> 
> On 2015-12-17 03:55, Joonyoung Shim wrote:
>> +Cc: Boram Park,
>>
>> Hi Marek,
>>
>> On 12/16/2015 09:21 PM, Marek Szyprowski wrote:
>>> This patch adds all infrastructure to make zpos plane property
>>> configurable from userspace.
>>>
>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> ---
>>>   drivers/gpu/drm/exynos/exynos_drm_drv.h   |  4 ++-
>>>   drivers/gpu/drm/exynos/exynos_drm_plane.c | 51 ++++++++++++++++++++++++++++---
>>>   2 files changed, 49 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
>>> index 588b6763f9c7..f0827dbebb7d 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
>>> @@ -64,6 +64,7 @@ struct exynos_drm_plane_state {
>>>       struct exynos_drm_rect src;
>>>       unsigned int h_ratio;
>>>       unsigned int v_ratio;
>>> +    unsigned int zpos;
>>>   };
>>>     static inline struct exynos_drm_plane_state *
>>> @@ -91,11 +92,12 @@ struct exynos_drm_plane {
>>>     #define EXYNOS_DRM_PLANE_CAP_DOUBLE    (1 << 0)
>>>   #define EXYNOS_DRM_PLANE_CAP_SCALE    (1 << 1)
>>> +#define EXYNOS_DRM_PLANE_CAP_ZPOS    (1 << 2)
>>>     /*
>>>    * Exynos DRM plane configuration structure.
>>>    *
>>> - * @zpos: z-position of the plane.
>>> + * @zpos: initial z-position of the plane.
>>>    * @type: type of the plane (primary, cursor or overlay).
>>>    * @pixel_formats: supported pixel formats.
>>>    * @num_pixel_formats: number of elements in 'pixel_formats'.
>>> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>>> index fd6cb4cee01a..a2bdab836b50 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
>>> @@ -124,6 +124,7 @@ static void exynos_plane_mode_set(struct exynos_drm_plane_state *exynos_state)
>>>     static void exynos_drm_plane_reset(struct drm_plane *plane)
>>>   {
>>> +    struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
>>>       struct exynos_drm_plane_state *exynos_state;
>>>         if (plane->state) {
>>> @@ -136,6 +137,7 @@ static void exynos_drm_plane_reset(struct drm_plane *plane)
>>>         exynos_state = kzalloc(sizeof(*exynos_state), GFP_KERNEL);
>>>       if (exynos_state) {
>>> +        exynos_state->zpos = exynos_plane->config->zpos;
>>>           plane->state = &exynos_state->base;
>>>           plane->state->plane = plane;
>>>       }
>>> @@ -153,6 +155,7 @@ exynos_drm_plane_duplicate_state(struct drm_plane *plane)
>>>           return NULL;
>>>         __drm_atomic_helper_plane_duplicate_state(plane, &copy->base);
>>> +    copy->zpos = exynos_state->zpos;
>>>       return &copy->base;
>>>   }
>>>   @@ -165,13 +168,53 @@ static void exynos_drm_plane_destroy_state(struct drm_plane *plane,
>>>       kfree(old_exynos_state);
>>>   }
>>>   +static int exynos_drm_plane_atomic_set_property(struct drm_plane *plane,
>>> +                        struct drm_plane_state *state,
>>> +                        struct drm_property *property,
>>> +                        uint64_t val)
>>> +{
>>> +    struct exynos_drm_plane *exynos_plane = to_exynos_plane(plane);
>>> +    struct exynos_drm_plane_state *exynos_state =
>>> +                    to_exynos_plane_state(state);
>>> +    struct exynos_drm_private *dev_priv = plane->dev->dev_private;
>>> +    const struct exynos_drm_plane_config *config = exynos_plane->config;
>>> +
>>> +    if (property == dev_priv->plane_zpos_property &&
>>> +        (config->capabilities & EXYNOS_DRM_PLANE_CAP_ZPOS))
>>> +        exynos_state->zpos = val;
>>> +    else
>>> +        return -EINVAL;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>> +static int exynos_drm_plane_atomic_get_property(struct drm_plane *plane,
>>> +                      const struct drm_plane_state *state,
>>> +                      struct drm_property *property,
>>> +                      uint64_t *val)
>>> +{
>>> +    const struct exynos_drm_plane_state *exynos_state =
>>> +        container_of(state, const struct exynos_drm_plane_state, base);
>>> +    struct exynos_drm_private *dev_priv = plane->dev->dev_private;
>>> +
>>> +    if (property == dev_priv->plane_zpos_property)
>>> +        *val = exynos_state->zpos;
>>> +    else
>>> +        return -EINVAL;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   static struct drm_plane_funcs exynos_plane_funcs = {
>>>       .update_plane    = drm_atomic_helper_update_plane,
>>>       .disable_plane    = drm_atomic_helper_disable_plane,
>>>       .destroy    = drm_plane_cleanup,
>>> +    .set_property    = drm_atomic_helper_plane_set_property,
>>>       .reset        = exynos_drm_plane_reset,
>>>       .atomic_duplicate_state = exynos_drm_plane_duplicate_state,
>>>       .atomic_destroy_state = exynos_drm_plane_destroy_state,
>>> +    .atomic_set_property = exynos_drm_plane_atomic_set_property,
>>> +    .atomic_get_property = exynos_drm_plane_atomic_get_property,
>>>   };
>>>     static int
>>> @@ -267,8 +310,8 @@ static void exynos_plane_attach_zpos_property(struct drm_plane *plane,
>>>         prop = dev_priv->plane_zpos_property;
>>>       if (!prop) {
>>> -        prop = drm_property_create_range(dev, DRM_MODE_PROP_IMMUTABLE,
>>> -                         "zpos", 0, MAX_PLANE - 1);
>>> +        prop = drm_property_create_range(dev, 0, "zpos",
>>> +                         0, MAX_PLANE - 1);
>> User are using zpos property as index now, if we make zpos property
>> configurable, i think we need a property immutable like index or a
>> property like type or name that can know this plane is which plane
>> (e.g. video plane or graphic plane).
> 
> I don't get this. zpos property is attached only to OVERLAY planes, PRIMARY and CURSOR don't have it, so userspace cannot rely on this property. In my patch the initial zpos value is same as before, so if application doesn't relies on zpos values, it will get the same values as now. Application can also check the plane type by reading 'type' property (PRIMARY, OVERLAY or CURSOR). However after my patch application will get the possibility to rearrange plane order for the given crtc by chaning zpos values. That's especially useful to configure 2 typical use cases: PRIMARY plane over OVERLAY plane (PRIMARY is used as typical on-screen-display) or OVERLAY plane over PRIMARY (OVERLAY is used as a window for displaying video data).
> 
>> Another way, user may be possible to check plane index as order of
>> plane object id.
> 
> Application can easily find all the planes, which belongs to given crtc by checking 'possible crtcs' plane parameter, I see no additional need for 'index' property in such case. Please note that the initial plane configuration in case of atomic api will be always the same for all application (determined by exynos_drm_plane_reset function) and it will match the current setup.
> 

Thing i want to say is user needs to know which plane is for grahpic0,
grahpic1 and video if they belong to mixer but it seems to be enough
as the information that user decides to use which plane, possible crtcs,
type property, pixel formats supported and zpos.

Thanks.

Re: [PATCH v3 5/7] drm/exynos: mixer: refactor layer setup

[Joonyoung Shim]

+Cc Boram Park,

On 12/18/2015 12:54 AM, Marek Szyprowski wrote:
> Hi Joonyoung,
> 
> On 2015-12-17 05:19, Joonyoung Shim wrote:
>> Hi Marek,
>>
>> On 12/16/2015 09:21 PM, Marek Szyprowski wrote:
>>> Properly configure blending properties of given hardware layer based on
>>> the selected pixel format. Currently only per-pixel-based alpha is possible
>>> when respective pixel format has been selected. Configuration of global,
>>> per-plane alpha value, color key and background color will be added later.
>>>
>>> This patch is heavily inspired by earlier work done by Tobias Jakobi
>>> <tjakobi@math.uni-bielefeld.de>.
>>>
>>> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> ---
>>>   drivers/gpu/drm/exynos/exynos_mixer.c | 43 +++++++++++++++++++++++++++++++++++
>>>   drivers/gpu/drm/exynos/regs-mixer.h   |  1 +
>>>   2 files changed, 44 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
>>> index c572e271579e..ae7b122274ac 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
>>> @@ -165,6 +165,16 @@ static const u8 filter_cr_horiz_tap4[] = {
>>>       70,    59,    48,    37,    27,    19,    11,    5,
>>>   };
>>>   +static inline bool is_alpha_format(unsigned int pixel_format)
>>> +{
>>> +    switch (pixel_format) {
>>> +    case DRM_FORMAT_ARGB8888:
>>> +        return true;
>>> +    default:
>>> +        return false;
>>> +    }
>>> +}
>>> +
>>>   static inline u32 vp_reg_read(struct mixer_resources *res, u32 reg_id)
>>>   {
>>>       return readl(res->vp_regs + reg_id);
>>> @@ -294,6 +304,37 @@ static void vp_default_filter(struct mixer_resources *res)
>>>           filter_cr_horiz_tap4, sizeof(filter_cr_horiz_tap4));
>>>   }
>>>   +static void mixer_cfg_gfx_blend(struct mixer_context *ctx, unsigned int win,
>>> +                bool alpha)
>>> +{
>>> +    struct mixer_resources *res = &ctx->mixer_res;
>>> +    u32 val;
>>> +
>>> +    val  = MXR_GRP_CFG_COLOR_KEY_DISABLE; /* no blank key */
>>> +    if (alpha) {
>>> +        /* blending based on pixel alpha */
>>> +        val |= MXR_GRP_CFG_BLEND_PRE_MUL;
>>> +        val |= MXR_GRP_CFG_PIXEL_BLEND_EN;
>>> +    }
>>> +    mixer_reg_writemask(res, MXR_GRAPHIC_CFG(win),
>>> +                val, MXR_GRP_CFG_MISC_MASK);
>> I think the priority of plane and whether vp layer exists should be
>> considered for blending setting. When priority of graphic layer0 is
>> lowest and vp layer is not, this will blend background layer.
>> It was not permitted to blend background layer until current.
> 
> Currently blending is hardcoded to following configuration:
> 1. Order: [top] Grp1 > Grp0 > Video [bottom]
> 2. Per-pixel alpha blending enabled unconditionally for Grp1 layer (regardless
>    of the selected pixel format for Grp1 layer).
> 3. Per-pixel alpha blending enabled for Grp0 layer when Video layer gets enabled
>    (regardless of the selected pixel format for Grp0 layer).
> 
> It is not very intuitive and it looks hardcoded for one particular use case.
> With the above patch application can configure blending for its needs.
> 

Sure, i'm not to oppose this patch.

> I really see no reason for special handling of the bottom layer (like
> disabling per-pixel alpha even if alpha-enabled format is selected). It is
> role of application to set proper pixel format (like XRGB8888 instead of
> ARGB8888) if the application is not interested in alpha blending. Per-pixel
> alpha blending is enabled only for formats which really support alpha.
> 

You mean this is role of application definitely, then it's reasonable to
me.

Thanks.

Re: [PATCH v3 1/7] drm/exynos: rename zpos to index

[Inki Dae]

Hi Marek,

Seems this patch could be more cleaned up.

Each ctx object of each crtc driver has its own plane config object which includes already zpos value.
So I think we wouldn't need to keep zpos of the plane config and index of the plane object.
How about removing index from exynos plane structure and using zpos of exynos plane config structure instead?

Below patch can be applied on top of your patch,
diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
index 88d022a..009fa18 100644
--- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
+++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
@@ -256,7 +256,7 @@ static void decon_atomic_begin(struct exynos_drm_crtc *crtc,
 	if (test_bit(BIT_SUSPENDED, &ctx->flags))
 		return;
 
-	decon_shadow_protect_win(ctx, plane->index, true);
+	decon_shadow_protect_win(ctx, plane->config->index, true);
 }
 
 #define BIT_VAL(x, e, s) (((x) & ((1 << ((e) - (s) + 1)) - 1)) << (s))
@@ -270,7 +270,7 @@ static void decon_update_plane(struct exynos_drm_crtc *crtc,
 				to_exynos_plane_state(plane->base.state);
 	struct decon_context *ctx = crtc->ctx;
 	struct drm_framebuffer *fb = state->base.fb;
-	unsigned int win = plane->index;
+	unsigned int win = plane->config->index;
 	unsigned int bpp = fb->bits_per_pixel >> 3;
 	unsigned int pitch = fb->pitches[0];
 	dma_addr_t dma_addr = exynos_drm_fb_dma_addr(fb, 0);
@@ -320,7 +320,7 @@ static void decon_disable_plane(struct exynos_drm_crtc *crtc,
 				struct exynos_drm_plane *plane)
 {
 	struct decon_context *ctx = crtc->ctx;
-	unsigned int win = plane->index;
+	unsigned int win = plane->config->index;
 
 	if (test_bit(BIT_SUSPENDED, &ctx->flags))
 		return;
@@ -344,7 +344,7 @@ static void decon_atomic_flush(struct exynos_drm_crtc *crtc,
 	if (test_bit(BIT_SUSPENDED, &ctx->flags))
 		return;
 
-	decon_shadow_protect_win(ctx, plane->index, false);
+	decon_shadow_protect_win(ctx, plane->config->index, false);
 
 	if (ctx->out_type == IFTYPE_I80)
 		set_bit(BIT_WIN_UPDATED, &ctx->flags);
@@ -499,10 +499,10 @@ static int decon_bind(struct device *dev, struct device *master, void *data)
 
 		ctx->configs[win].pixel_formats = decon_formats;
 		ctx->configs[win].num_pixel_formats = ARRAY_SIZE(decon_formats);
-		ctx->configs[win].zpos = win;
+		ctx->configs[win].index = win;
 		ctx->configs[win].type = decon_win_types[tmp];
 
-		ret = exynos_plane_init(drm_dev, &ctx->planes[win], i,
+		ret = exynos_plane_init(drm_dev, &ctx->planes[win],
 					1 << ctx->pipe, &ctx->configs[win]);
 		if (ret)
 			return ret;
diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
index 8911f965..6f47b60 100644
--- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
+++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
@@ -393,7 +393,7 @@ static void decon_atomic_begin(struct exynos_drm_crtc *crtc,
 	if (ctx->suspended)
 		return;
 
-	decon_shadow_protect_win(ctx, plane->index, true);
+	decon_shadow_protect_win(ctx, plane->config->index, true);
 }
 
 static void decon_update_plane(struct exynos_drm_crtc *crtc,
@@ -407,7 +407,7 @@ static void decon_update_plane(struct exynos_drm_crtc *crtc,
 	unsigned long val, alpha;
 	unsigned int last_x;
 	unsigned int last_y;
-	unsigned int win = plane->index;
+	unsigned int win = plane->config->index;
 	unsigned int bpp = fb->bits_per_pixel >> 3;
 	unsigned int pitch = fb->pitches[0];
 
@@ -498,7 +498,7 @@ static void decon_disable_plane(struct exynos_drm_crtc *crtc,
 				struct exynos_drm_plane *plane)
 {
 	struct decon_context *ctx = crtc->ctx;
-	unsigned int win = plane->index;
+	unsigned int win = plane->config->index;
 	u32 val;
 
 	if (ctx->suspended)
@@ -525,7 +525,7 @@ static void decon_atomic_flush(struct exynos_drm_crtc *crtc,
 	if (ctx->suspended)
 		return;
 
-	decon_shadow_protect_win(ctx, plane->index, false);
+	decon_shadow_protect_win(ctx, plane->config->index, false);
 }
 
 static void decon_init(struct decon_context *ctx)
@@ -654,10 +654,10 @@ static int decon_bind(struct device *dev, struct device *master, void *data)
 	for (i = 0; i < WINDOWS_NR; i++) {
 		ctx->configs[i].pixel_formats = decon_formats;
 		ctx->configs[i].num_pixel_formats = ARRAY_SIZE(decon_formats);
-		ctx->configs[i].zpos = i;
+		ctx->configs[i].index = i;
 		ctx->configs[i].type = decon_win_types[i];
 
-		ret = exynos_plane_init(drm_dev, &ctx->planes[i], i,
+		ret = exynos_plane_init(drm_dev, &ctx->planes[i],
 					1 << ctx->pipe, &ctx->configs[i]);
 		if (ret)
 			return ret;
diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
index f0827db..c006047 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
@@ -77,7 +77,6 @@ to_exynos_plane_state(struct drm_plane_state *state)
  * Exynos drm common overlay structure.
  *
  * @base: plane object
- * @index: hardware index of the overlay layer
  *
  * this structure is common to exynos SoC and its contents would be copied
  * to hardware specific overlay info.
@@ -86,7 +85,6 @@ to_exynos_plane_state(struct drm_plane_state *state)
 struct exynos_drm_plane {
 	struct drm_plane base;
 	const struct exynos_drm_plane_config *config;
-	unsigned int index;
 	struct drm_framebuffer *pending_fb;
 };
 
@@ -97,7 +95,7 @@ struct exynos_drm_plane {
 /*
  * Exynos DRM plane configuration structure.
  *
- * @zpos: initial z-position of the plane.
+ * @index: hardware index of the overlay layer
  * @type: type of the plane (primary, cursor or overlay).
  * @pixel_formats: supported pixel formats.
  * @num_pixel_formats: number of elements in 'pixel_formats'.
@@ -105,7 +103,7 @@ struct exynos_drm_plane {
  */
 
 struct exynos_drm_plane_config {
-	unsigned int zpos;
+	unsigned int index;
 	enum drm_plane_type type;
 	const uint32_t *pixel_formats;
 	unsigned int num_pixel_formats;
diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
index 6ae1b1e..df5fbd7 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
@@ -630,7 +630,7 @@ static void fimd_atomic_begin(struct exynos_drm_crtc *crtc,
 	if (ctx->suspended)
 		return;
 
-	fimd_shadow_protect_win(ctx, plane->index, true);
+	fimd_shadow_protect_win(ctx, plane->config->index, true);
 }
 
 static void fimd_atomic_flush(struct exynos_drm_crtc *crtc,
@@ -641,7 +641,7 @@ static void fimd_atomic_flush(struct exynos_drm_crtc *crtc,
 	if (ctx->suspended)
 		return;
 
-	fimd_shadow_protect_win(ctx, plane->index, false);
+	fimd_shadow_protect_win(ctx, plane->config->index, false);
 }
 
 static void fimd_update_plane(struct exynos_drm_crtc *crtc,
@@ -654,7 +654,7 @@ static void fimd_update_plane(struct exynos_drm_crtc *crtc,
 	dma_addr_t dma_addr;
 	unsigned long val, size, offset;
 	unsigned int last_x, last_y, buf_offsize, line_size;
-	unsigned int win = plane->index;
+	unsigned int win = plane->config->index;
 	unsigned int bpp = fb->bits_per_pixel >> 3;
 	unsigned int pitch = fb->pitches[0];
 
@@ -740,7 +740,7 @@ static void fimd_disable_plane(struct exynos_drm_crtc *crtc,
 			       struct exynos_drm_plane *plane)
 {
 	struct fimd_context *ctx = crtc->ctx;
-	unsigned int win = plane->index;
+	unsigned int win = plane->config->index;
 
 	if (ctx->suspended)
 		return;
@@ -942,9 +942,9 @@ static int fimd_bind(struct device *dev, struct device *master, void *data)
 	for (i = 0; i < WINDOWS_NR; i++) {
 		ctx->configs[i].pixel_formats = fimd_formats;
 		ctx->configs[i].num_pixel_formats = ARRAY_SIZE(fimd_formats);
-		ctx->configs[i].zpos = i;
+		ctx->configs[i].index = i;
 		ctx->configs[i].type = fimd_win_types[i];
-		ret = exynos_plane_init(drm_dev, &ctx->planes[i], i,
+		ret = exynos_plane_init(drm_dev, &ctx->planes[i],
 					1 << ctx->pipe, &ctx->configs[i]);
 		if (ret)
 			return ret;
diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
index d862272..4d71aa5 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
@@ -137,7 +137,7 @@ static void exynos_drm_plane_reset(struct drm_plane *plane)
 
 	exynos_state = kzalloc(sizeof(*exynos_state), GFP_KERNEL);
 	if (exynos_state) {
-		exynos_state->zpos = exynos_plane->config->zpos;
+		exynos_state->zpos = exynos_plane->config->index;
 		plane->state = &exynos_state->base;
 		plane->state->plane = plane;
 	}
@@ -323,7 +323,7 @@ static void exynos_plane_attach_zpos_property(struct drm_plane *plane,
 
 int exynos_plane_init(struct drm_device *dev,
 		      struct exynos_drm_plane *exynos_plane,
-		      unsigned int index, unsigned long possible_crtcs,
+		      unsigned long possible_crtcs,
 		      const struct exynos_drm_plane_config *config)
 {
 	int err;
@@ -341,10 +341,9 @@ int exynos_plane_init(struct drm_device *dev,
 
 	drm_plane_helper_add(&exynos_plane->base, &plane_helper_funcs);
 
-	exynos_plane->index = index;
 	exynos_plane->config = config;
 
-	exynos_plane_attach_zpos_property(&exynos_plane->base, config->zpos);
+	exynos_plane_attach_zpos_property(&exynos_plane->base, config->index);
 
 	return 0;
 }
diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.h b/drivers/gpu/drm/exynos/exynos_drm_plane.h
index 9aafad1..0dd0965 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_plane.h
+++ b/drivers/gpu/drm/exynos/exynos_drm_plane.h
@@ -10,6 +10,6 @@
  */
 
 int exynos_plane_init(struct drm_device *dev,
-		      struct exynos_drm_plane *exynos_plane, unsigned int index,
+		      struct exynos_drm_plane *exynos_plane,
 		      unsigned long possible_crtcs,
 		      const struct exynos_drm_plane_config *config);
diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
index 10f02b9..6f91460 100644
--- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
+++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
@@ -458,10 +458,9 @@ static int vidi_bind(struct device *dev, struct device *master, void *data)
 	plane_config.num_pixel_formats = ARRAY_SIZE(formats);
 
 	for (i = 0; i < WINDOWS_NR; i++) {
-		plane_config.zpos = i;
 		plane_config.type = vidi_win_types[i];
 
-		ret = exynos_plane_init(drm_dev, &ctx->planes[i], i,
+		ret = exynos_plane_init(drm_dev, &ctx->planes[i],
 					1 << ctx->pipe, &plane_config);
 		if (ret)
 			return ret;
diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index bf148dc..6bedda7 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -115,21 +115,21 @@ struct mixer_drv_data {
 
 static const struct exynos_drm_plane_config plane_configs[MIXER_WIN_NR] = {
 	{
-		.zpos = 0,
+		.index = 0,
 		.type = DRM_PLANE_TYPE_PRIMARY,
 		.pixel_formats = mixer_formats,
 		.num_pixel_formats = ARRAY_SIZE(mixer_formats),
 		.capabilities = EXYNOS_DRM_PLANE_CAP_DOUBLE |
 				EXYNOS_DRM_PLANE_CAP_ZPOS,
 	}, {
-		.zpos = 1,
+		.index = 1,
 		.type = DRM_PLANE_TYPE_CURSOR,
 		.pixel_formats = mixer_formats,
 		.num_pixel_formats = ARRAY_SIZE(mixer_formats),
 		.capabilities = EXYNOS_DRM_PLANE_CAP_DOUBLE |
 				EXYNOS_DRM_PLANE_CAP_ZPOS,
 	}, {
-		.zpos = 2,
+		.index = 2,
 		.type = DRM_PLANE_TYPE_OVERLAY,
 		.pixel_formats = vp_formats,
 		.num_pixel_formats = ARRAY_SIZE(vp_formats),
@@ -563,7 +563,7 @@ static void vp_video_buffer(struct mixer_context *ctx,
 
 	mixer_cfg_scan(ctx, mode->vdisplay);
 	mixer_cfg_rgb_fmt(ctx, mode->vdisplay);
-	mixer_cfg_layer(ctx, plane->index, state->zpos + 1, true);
+	mixer_cfg_layer(ctx, plane->config->index, state->zpos + 1, true);
 	mixer_cfg_vp_blend(ctx);
 	mixer_run(ctx);
 
@@ -590,7 +590,7 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
 	struct mixer_resources *res = &ctx->mixer_res;
 	struct drm_framebuffer *fb = state->base.fb;
 	unsigned long flags;
-	unsigned int win = plane->index;
+	unsigned int win = plane->config->index;
 	unsigned int x_ratio = 0, y_ratio = 0;
 	unsigned int src_x_offset, src_y_offset, dst_x_offset, dst_y_offset;
 	dma_addr_t dma_addr;
@@ -985,12 +985,12 @@ static void mixer_update_plane(struct exynos_drm_crtc *crtc,
 {
 	struct mixer_context *mixer_ctx = crtc->ctx;
 
-	DRM_DEBUG_KMS("win: %d\n", plane->index);
+	DRM_DEBUG_KMS("win: %d\n", plane->config->index);
 
 	if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
 		return;
 
-	if (plane->index == VP_DEFAULT_WIN)
+	if (plane->config->index == VP_DEFAULT_WIN)
 		vp_video_buffer(mixer_ctx, plane);
 	else
 		mixer_graph_buffer(mixer_ctx, plane);
@@ -1003,7 +1003,7 @@ static void mixer_disable_plane(struct exynos_drm_crtc *crtc,
 	struct mixer_resources *res = &mixer_ctx->mixer_res;
 	unsigned long flags;
 
-	DRM_DEBUG_KMS("win: %d\n", plane->index);
+	DRM_DEBUG_KMS("win: %d\n", plane->config->index);
 
 	if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
 		return;
@@ -1011,7 +1011,7 @@ static void mixer_disable_plane(struct exynos_drm_crtc *crtc,
 	spin_lock_irqsave(&res->reg_slock, flags);
 	mixer_vsync_set_update(mixer_ctx, false);
 
-	mixer_cfg_layer(mixer_ctx, plane->index, 0, false);
+	mixer_cfg_layer(mixer_ctx, plane->config->index, 0, false);
 
 	mixer_vsync_set_update(mixer_ctx, true);
 	spin_unlock_irqrestore(&res->reg_slock, flags);
@@ -1189,7 +1189,7 @@ static int mixer_bind(struct device *dev, struct device *manager, void *data)
 		if (i == VP_DEFAULT_WIN && !ctx->vp_enabled)
 			continue;
 
-		ret = exynos_plane_init(drm_dev, &ctx->planes[i], i,
+		ret = exynos_plane_init(drm_dev, &ctx->planes[i],
 					1 << ctx->pipe, &plane_configs[i]);
 		if (ret)
 			return ret;


Thanks,
Inki Dae 


2015년 12월 16일 21:21에 Marek Szyprowski 이(가) 쓴 글:
> This patch renames zpos entry to index, because in most places it is
> used as index for selecting hardware layer/window instead of
> configurable layer position. This will later enable to make the zpos
> property configurable.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 10 +++++-----
>  drivers/gpu/drm/exynos/exynos7_drm_decon.c    | 10 +++++-----
>  drivers/gpu/drm/exynos/exynos_drm_drv.h       |  4 ++--
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c      | 10 +++++-----
>  drivers/gpu/drm/exynos/exynos_drm_plane.c     |  4 ++--
>  drivers/gpu/drm/exynos/exynos_drm_plane.h     |  2 +-
>  drivers/gpu/drm/exynos/exynos_drm_vidi.c      |  2 +-
>  drivers/gpu/drm/exynos/exynos_mixer.c         | 14 +++++++-------
>  8 files changed, 28 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
> index c7362b99ce28..88d022ad5280 100644
> --- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
> +++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
> @@ -256,7 +256,7 @@ static void decon_atomic_begin(struct exynos_drm_crtc *crtc,
>  	if (test_bit(BIT_SUSPENDED, &ctx->flags))
>  		return;
>  
> -	decon_shadow_protect_win(ctx, plane->zpos, true);
> +	decon_shadow_protect_win(ctx, plane->index, true);
>  }
>  
>  #define BIT_VAL(x, e, s) (((x) & ((1 << ((e) - (s) + 1)) - 1)) << (s))
> @@ -270,7 +270,7 @@ static void decon_update_plane(struct exynos_drm_crtc *crtc,
>  				to_exynos_plane_state(plane->base.state);
>  	struct decon_context *ctx = crtc->ctx;
>  	struct drm_framebuffer *fb = state->base.fb;
> -	unsigned int win = plane->zpos;
> +	unsigned int win = plane->index;
>  	unsigned int bpp = fb->bits_per_pixel >> 3;
>  	unsigned int pitch = fb->pitches[0];
>  	dma_addr_t dma_addr = exynos_drm_fb_dma_addr(fb, 0);
> @@ -320,7 +320,7 @@ static void decon_disable_plane(struct exynos_drm_crtc *crtc,
>  				struct exynos_drm_plane *plane)
>  {
>  	struct decon_context *ctx = crtc->ctx;
> -	unsigned int win = plane->zpos;
> +	unsigned int win = plane->index;
>  
>  	if (test_bit(BIT_SUSPENDED, &ctx->flags))
>  		return;
> @@ -344,7 +344,7 @@ static void decon_atomic_flush(struct exynos_drm_crtc *crtc,
>  	if (test_bit(BIT_SUSPENDED, &ctx->flags))
>  		return;
>  
> -	decon_shadow_protect_win(ctx, plane->zpos, false);
> +	decon_shadow_protect_win(ctx, plane->index, false);
>  
>  	if (ctx->out_type == IFTYPE_I80)
>  		set_bit(BIT_WIN_UPDATED, &ctx->flags);
> @@ -502,7 +502,7 @@ static int decon_bind(struct device *dev, struct device *master, void *data)
>  		ctx->configs[win].zpos = win;
>  		ctx->configs[win].type = decon_win_types[tmp];
>  
> -		ret = exynos_plane_init(drm_dev, &ctx->planes[win],
> +		ret = exynos_plane_init(drm_dev, &ctx->planes[win], i,
>  					1 << ctx->pipe, &ctx->configs[win]);
>  		if (ret)
>  			return ret;
> diff --git a/drivers/gpu/drm/exynos/exynos7_drm_decon.c b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> index c47f9af8170b..8911f965b06c 100644
> --- a/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> +++ b/drivers/gpu/drm/exynos/exynos7_drm_decon.c
> @@ -393,7 +393,7 @@ static void decon_atomic_begin(struct exynos_drm_crtc *crtc,
>  	if (ctx->suspended)
>  		return;
>  
> -	decon_shadow_protect_win(ctx, plane->zpos, true);
> +	decon_shadow_protect_win(ctx, plane->index, true);
>  }
>  
>  static void decon_update_plane(struct exynos_drm_crtc *crtc,
> @@ -407,7 +407,7 @@ static void decon_update_plane(struct exynos_drm_crtc *crtc,
>  	unsigned long val, alpha;
>  	unsigned int last_x;
>  	unsigned int last_y;
> -	unsigned int win = plane->zpos;
> +	unsigned int win = plane->index;
>  	unsigned int bpp = fb->bits_per_pixel >> 3;
>  	unsigned int pitch = fb->pitches[0];
>  
> @@ -498,7 +498,7 @@ static void decon_disable_plane(struct exynos_drm_crtc *crtc,
>  				struct exynos_drm_plane *plane)
>  {
>  	struct decon_context *ctx = crtc->ctx;
> -	unsigned int win = plane->zpos;
> +	unsigned int win = plane->index;
>  	u32 val;
>  
>  	if (ctx->suspended)
> @@ -525,7 +525,7 @@ static void decon_atomic_flush(struct exynos_drm_crtc *crtc,
>  	if (ctx->suspended)
>  		return;
>  
> -	decon_shadow_protect_win(ctx, plane->zpos, false);
> +	decon_shadow_protect_win(ctx, plane->index, false);
>  }
>  
>  static void decon_init(struct decon_context *ctx)
> @@ -657,7 +657,7 @@ static int decon_bind(struct device *dev, struct device *master, void *data)
>  		ctx->configs[i].zpos = i;
>  		ctx->configs[i].type = decon_win_types[i];
>  
> -		ret = exynos_plane_init(drm_dev, &ctx->planes[i],
> +		ret = exynos_plane_init(drm_dev, &ctx->planes[i], i,
>  					1 << ctx->pipe, &ctx->configs[i]);
>  		if (ret)
>  			return ret;
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_drv.h b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> index 82bbd7f4b316..588b6763f9c7 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_drv.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_drv.h
> @@ -76,7 +76,7 @@ to_exynos_plane_state(struct drm_plane_state *state)
>   * Exynos drm common overlay structure.
>   *
>   * @base: plane object
> - * @zpos: order of overlay layer(z position).
> + * @index: hardware index of the overlay layer
>   *
>   * this structure is common to exynos SoC and its contents would be copied
>   * to hardware specific overlay info.
> @@ -85,7 +85,7 @@ to_exynos_plane_state(struct drm_plane_state *state)
>  struct exynos_drm_plane {
>  	struct drm_plane base;
>  	const struct exynos_drm_plane_config *config;
> -	unsigned int zpos;
> +	unsigned int index;
>  	struct drm_framebuffer *pending_fb;
>  };
>  
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_fimd.c b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> index 2e2247126581..6ae1b1e55783 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_fimd.c
> @@ -630,7 +630,7 @@ static void fimd_atomic_begin(struct exynos_drm_crtc *crtc,
>  	if (ctx->suspended)
>  		return;
>  
> -	fimd_shadow_protect_win(ctx, plane->zpos, true);
> +	fimd_shadow_protect_win(ctx, plane->index, true);
>  }
>  
>  static void fimd_atomic_flush(struct exynos_drm_crtc *crtc,
> @@ -641,7 +641,7 @@ static void fimd_atomic_flush(struct exynos_drm_crtc *crtc,
>  	if (ctx->suspended)
>  		return;
>  
> -	fimd_shadow_protect_win(ctx, plane->zpos, false);
> +	fimd_shadow_protect_win(ctx, plane->index, false);
>  }
>  
>  static void fimd_update_plane(struct exynos_drm_crtc *crtc,
> @@ -654,7 +654,7 @@ static void fimd_update_plane(struct exynos_drm_crtc *crtc,
>  	dma_addr_t dma_addr;
>  	unsigned long val, size, offset;
>  	unsigned int last_x, last_y, buf_offsize, line_size;
> -	unsigned int win = plane->zpos;
> +	unsigned int win = plane->index;
>  	unsigned int bpp = fb->bits_per_pixel >> 3;
>  	unsigned int pitch = fb->pitches[0];
>  
> @@ -740,7 +740,7 @@ static void fimd_disable_plane(struct exynos_drm_crtc *crtc,
>  			       struct exynos_drm_plane *plane)
>  {
>  	struct fimd_context *ctx = crtc->ctx;
> -	unsigned int win = plane->zpos;
> +	unsigned int win = plane->index;
>  
>  	if (ctx->suspended)
>  		return;
> @@ -944,7 +944,7 @@ static int fimd_bind(struct device *dev, struct device *master, void *data)
>  		ctx->configs[i].num_pixel_formats = ARRAY_SIZE(fimd_formats);
>  		ctx->configs[i].zpos = i;
>  		ctx->configs[i].type = fimd_win_types[i];
> -		ret = exynos_plane_init(drm_dev, &ctx->planes[i],
> +		ret = exynos_plane_init(drm_dev, &ctx->planes[i], i,
>  					1 << ctx->pipe, &ctx->configs[i]);
>  		if (ret)
>  			return ret;
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.c b/drivers/gpu/drm/exynos/exynos_drm_plane.c
> index 427aeec78a28..fd6cb4cee01a 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.c
> @@ -280,7 +280,7 @@ static void exynos_plane_attach_zpos_property(struct drm_plane *plane,
>  
>  int exynos_plane_init(struct drm_device *dev,
>  		      struct exynos_drm_plane *exynos_plane,
> -		      unsigned long possible_crtcs,
> +		      unsigned int index, unsigned long possible_crtcs,
>  		      const struct exynos_drm_plane_config *config)
>  {
>  	int err;
> @@ -299,7 +299,7 @@ int exynos_plane_init(struct drm_device *dev,
>  
>  	drm_plane_helper_add(&exynos_plane->base, &plane_helper_funcs);
>  
> -	exynos_plane->zpos = config->zpos;
> +	exynos_plane->index = index;
>  	exynos_plane->config = config;
>  
>  	if (config->type == DRM_PLANE_TYPE_OVERLAY)
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_plane.h b/drivers/gpu/drm/exynos/exynos_drm_plane.h
> index 0dd096548284..9aafad164cdf 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_plane.h
> +++ b/drivers/gpu/drm/exynos/exynos_drm_plane.h
> @@ -10,6 +10,6 @@
>   */
>  
>  int exynos_plane_init(struct drm_device *dev,
> -		      struct exynos_drm_plane *exynos_plane,
> +		      struct exynos_drm_plane *exynos_plane, unsigned int index,
>  		      unsigned long possible_crtcs,
>  		      const struct exynos_drm_plane_config *config);
> diff --git a/drivers/gpu/drm/exynos/exynos_drm_vidi.c b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> index c1d33d5cbb15..adad4dc84b81 100644
> --- a/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> +++ b/drivers/gpu/drm/exynos/exynos_drm_vidi.c
> @@ -461,7 +461,7 @@ static int vidi_bind(struct device *dev, struct device *master, void *data)
>  		plane_config.zpos = i;
>  		plane_config.type = vidi_win_types[i];
>  
> -		ret = exynos_plane_init(drm_dev, &ctx->planes[i],
> +		ret = exynos_plane_init(drm_dev, &ctx->planes[i], i,
>  					1 << ctx->pipe, &plane_config);
>  		if (ret)
>  			return ret;
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> index dfb35e2da4db..0dceeb2b532c 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> @@ -511,7 +511,7 @@ static void vp_video_buffer(struct mixer_context *ctx,
>  
>  	mixer_cfg_scan(ctx, mode->vdisplay);
>  	mixer_cfg_rgb_fmt(ctx, mode->vdisplay);
> -	mixer_cfg_layer(ctx, plane->zpos, true);
> +	mixer_cfg_layer(ctx, plane->index, true);
>  	mixer_run(ctx);
>  
>  	mixer_vsync_set_update(ctx, true);
> @@ -537,7 +537,7 @@ static void mixer_graph_buffer(struct mixer_context *ctx,
>  	struct mixer_resources *res = &ctx->mixer_res;
>  	struct drm_framebuffer *fb = state->base.fb;
>  	unsigned long flags;
> -	unsigned int win = plane->zpos;
> +	unsigned int win = plane->index;
>  	unsigned int x_ratio = 0, y_ratio = 0;
>  	unsigned int src_x_offset, src_y_offset, dst_x_offset, dst_y_offset;
>  	dma_addr_t dma_addr;
> @@ -956,12 +956,12 @@ static void mixer_update_plane(struct exynos_drm_crtc *crtc,
>  {
>  	struct mixer_context *mixer_ctx = crtc->ctx;
>  
> -	DRM_DEBUG_KMS("win: %d\n", plane->zpos);
> +	DRM_DEBUG_KMS("win: %d\n", plane->index);
>  
>  	if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
>  		return;
>  
> -	if (plane->zpos > 1 && mixer_ctx->vp_enabled)
> +	if (plane->index > 1 && mixer_ctx->vp_enabled)
>  		vp_video_buffer(mixer_ctx, plane);
>  	else
>  		mixer_graph_buffer(mixer_ctx, plane);
> @@ -974,7 +974,7 @@ static void mixer_disable_plane(struct exynos_drm_crtc *crtc,
>  	struct mixer_resources *res = &mixer_ctx->mixer_res;
>  	unsigned long flags;
>  
> -	DRM_DEBUG_KMS("win: %d\n", plane->zpos);
> +	DRM_DEBUG_KMS("win: %d\n", plane->index);
>  
>  	if (!test_bit(MXR_BIT_POWERED, &mixer_ctx->flags))
>  		return;
> @@ -982,7 +982,7 @@ static void mixer_disable_plane(struct exynos_drm_crtc *crtc,
>  	spin_lock_irqsave(&res->reg_slock, flags);
>  	mixer_vsync_set_update(mixer_ctx, false);
>  
> -	mixer_cfg_layer(mixer_ctx, plane->zpos, false);
> +	mixer_cfg_layer(mixer_ctx, plane->index, false);
>  
>  	mixer_vsync_set_update(mixer_ctx, true);
>  	spin_unlock_irqrestore(&res->reg_slock, flags);
> @@ -1160,7 +1160,7 @@ static int mixer_bind(struct device *dev, struct device *manager, void *data)
>  		if (i == VP_DEFAULT_WIN && !ctx->vp_enabled)
>  			continue;
>  
> -		ret = exynos_plane_init(drm_dev, &ctx->planes[i],
> +		ret = exynos_plane_init(drm_dev, &ctx->planes[i], i,
>  					1 << ctx->pipe, &plane_configs[i]);
>  		if (ret)
>  			return ret;
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v3 1/7] drm/exynos: rename zpos to index

[Inki Dae]

Below just trivial issue,

2015년 12월 16일 21:21에 Marek Szyprowski 이(가) 쓴 글:
> This patch renames zpos entry to index, because in most places it is
> used as index for selecting hardware layer/window instead of
> configurable layer position. This will later enable to make the zpos
> property configurable.
> 
> Signed-off-by: Marek Szyprowski <m.szyprowski@samsung.com>
> ---
>  drivers/gpu/drm/exynos/exynos5433_drm_decon.c | 10 +++++-----
>  drivers/gpu/drm/exynos/exynos7_drm_decon.c    | 10 +++++-----
>  drivers/gpu/drm/exynos/exynos_drm_drv.h       |  4 ++--
>  drivers/gpu/drm/exynos/exynos_drm_fimd.c      | 10 +++++-----
>  drivers/gpu/drm/exynos/exynos_drm_plane.c     |  4 ++--
>  drivers/gpu/drm/exynos/exynos_drm_plane.h     |  2 +-
>  drivers/gpu/drm/exynos/exynos_drm_vidi.c      |  2 +-
>  drivers/gpu/drm/exynos/exynos_mixer.c         | 14 +++++++-------
>  8 files changed, 28 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
> index c7362b99ce28..88d022ad5280 100644
> --- a/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
> +++ b/drivers/gpu/drm/exynos/exynos5433_drm_decon.c
> @@ -256,7 +256,7 @@ static void decon_atomic_begin(struct exynos_drm_crtc *crtc,
>  	if (test_bit(BIT_SUSPENDED, &ctx->flags))
>  		return;
>  
> -	decon_shadow_protect_win(ctx, plane->zpos, true);
> +	decon_shadow_protect_win(ctx, plane->index, true);
>  }
>  
>  #define BIT_VAL(x, e, s) (((x) & ((1 << ((e) - (s) + 1)) - 1)) << (s))
> @@ -270,7 +270,7 @@ static void decon_update_plane(struct exynos_drm_crtc *crtc,
>  				to_exynos_plane_state(plane->base.state);
>  	struct decon_context *ctx = crtc->ctx;
>  	struct drm_framebuffer *fb = state->base.fb;
> -	unsigned int win = plane->zpos;
> +	unsigned int win = plane->index;
>  	unsigned int bpp = fb->bits_per_pixel >> 3;
>  	unsigned int pitch = fb->pitches[0];
>  	dma_addr_t dma_addr = exynos_drm_fb_dma_addr(fb, 0);
> @@ -320,7 +320,7 @@ static void decon_disable_plane(struct exynos_drm_crtc *crtc,
>  				struct exynos_drm_plane *plane)
>  {
>  	struct decon_context *ctx = crtc->ctx;
> -	unsigned int win = plane->zpos;
> +	unsigned int win = plane->index;
>  
>  	if (test_bit(BIT_SUSPENDED, &ctx->flags))
>  		return;
> @@ -344,7 +344,7 @@ static void decon_atomic_flush(struct exynos_drm_crtc *crtc,
>  	if (test_bit(BIT_SUSPENDED, &ctx->flags))
>  		return;
>  
> -	decon_shadow_protect_win(ctx, plane->zpos, false);
> +	decon_shadow_protect_win(ctx, plane->index, false);
>  
>  	if (ctx->out_type == IFTYPE_I80)
>  		set_bit(BIT_WIN_UPDATED, &ctx->flags);
> @@ -502,7 +502,7 @@ static int decon_bind(struct device *dev, struct device *master, void *data)
>  		ctx->configs[win].zpos = win;
>  		ctx->configs[win].type = decon_win_types[tmp];
>  
> -		ret = exynos_plane_init(drm_dev, &ctx->planes[win],
> +		ret = exynos_plane_init(drm_dev, &ctx->planes[win], i,

'i' isn't declared so you have to use win instead.
Please, post the patch set at least after build test.

Thanks,
Inki Dae 

Re: [PATCH v3 1/7] drm/exynos: rename zpos to index

[Marek Szyprowski]

Hello,

On 2015-12-24 09:15, Inki Dae wrote:
> Seems this patch could be more cleaned up.
>
> Each ctx object of each crtc driver has its own plane config object which includes already zpos value.
> So I think we wouldn't need to keep zpos of the plane config and index of the plane object.
> How about removing index from exynos plane structure and using zpos of exynos plane config structure instead?
>
> Below patch can be applied on top of your patch,

If you want I can move 'index' entry to config object, but the initial 
zpos value
should also be there. Please note that index is not always equal to the 
initial zpos
and having initial zpos configurable is also needed. There were already 
concerns if
mixer's dedicated video plane should be below or above the primary gfx 
plane in the
default configuration.

 > (...)

Best regards
-- 
Marek Szyprowski, PhD
Samsung R&D Institute Poland

Re: [PATCH v3 1/7] drm/exynos: rename zpos to index

[Inki Dae]

Hi Marek,

2015년 12월 28일 21:34에 Marek Szyprowski 이(가) 쓴 글:
> Hello,
> 
> On 2015-12-24 09:15, Inki Dae wrote:
>> Seems this patch could be more cleaned up.
>>
>> Each ctx object of each crtc driver has its own plane config object which includes already zpos value.
>> So I think we wouldn't need to keep zpos of the plane config and index of the plane object.
>> How about removing index from exynos plane structure and using zpos of exynos plane config structure instead?
>>
>> Below patch can be applied on top of your patch,
> 
> If you want I can move 'index' entry to config object, but the initial zpos value
> should also be there. Please note that index is not always equal to the initial zpos
> and having initial zpos configurable is also needed. There were already concerns if
> mixer's dedicated video plane should be below or above the primary gfx plane in the
> default configuration.

Merged all patches.

Thanks,
Inki Dae

> 
>> (...)
> 
> Best regards
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel