[PATCH v2 0/5] drm/exynos: rework layer blending setup

[PATCH v2 0/5] drm/exynos: rework layer blending setup

[Tobias Jakobi]

Hello,

this is a rework of the layer blending setup in the Exynos DRM mixer. There are currently two problems with the setup:

(1) It's static and doesn't take the alpha state of the layers into consideration.
(2) It's spread out through the mixer code.

This rework pushes all the configuration details into a layer_cfg array, which specifies the priority of each layer, and adds dynamic configuration based on layer enable and alpha state.

Two default configs are currently specified, one for mixer versions with a video processor (VP) and one for mixers without VP. The VP gives us one additional layer, the video layer, which natively supports the NV12/NV21 pixelformat.

The blending setup roughly works like this:
1) Find the bottom-most enabled layer. Disable all blending for this layer. This is done because we currently don't expose modification of the mixer background to userspace. Once this is done we can add more flexibility here.
2) Find the next enabled layer in our layer stack. If the layer has a framebuffer with an alpha-pixelformat attached, enable blending for this layer. If not, disable blending.
3) Iterate (2) until all enabled layers are processed.

The series has been tested on a Hardkernel Odroid-X2 (Exynos4412, which has a VP).

You can check the series with libdrm's modetest by setting up two planes, one with a NV12/NV21 format and another one with a ARGB8888 format (or another format with an alpha channel).

With best wishes,
Tobias

v2: General code clean-up, indentation fixes and other small misc changes.
    Layer state caching was added, to reduce the amount of register programming.

Tobias Jakobi (5):
  drm/exynos: mixer: refactor layer setup
  drm/exynos: mixer: introduce mixer_layer_blending()
  drm/exynos: mixer: remove all static blending setup
  drm/exynos: mixer: do blending setup in mixer_cfg_layer()
  drm/exynos: mixer: also allow ARGB1555 and ARGB4444

 drivers/gpu/drm/exynos/exynos_mixer.c | 236 +++++++++++++++++++++++++++++-----
 drivers/gpu/drm/exynos/regs-mixer.h   |   1 +
 2 files changed, 203 insertions(+), 34 deletions(-)

-- 
2.4.9

[PATCH v2 1/5] drm/exynos: mixer: refactor layer setup

[Tobias Jakobi]

First step in allowing a more generic way to setup complex
blending for the different layers.

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

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 7498c6e..2c1cea3 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -63,6 +63,11 @@ struct mixer_resources {
 	struct clk		*mout_mixer;
 };
 
+struct layer_cfg {
+	unsigned int index;
+	unsigned int priority;
+};
+
 enum mixer_version_id {
 	MXR_VER_0_0_0_16,
 	MXR_VER_16_0_33_0,
@@ -92,6 +97,8 @@ struct mixer_context {
 	struct drm_device	*drm_dev;
 	struct exynos_drm_crtc	*crtc;
 	struct exynos_drm_plane	planes[MIXER_WIN_NR];
+	const struct layer_cfg	*layer_cfg;
+	unsigned int		num_layer;
 	int			pipe;
 	unsigned long		flags;
 	bool			interlace;
@@ -110,6 +117,34 @@ struct mixer_drv_data {
 	bool					has_sclk;
 };
 
+/*
+ * The default layer priorities for non-VP (video processor)
+ * and VP mixer configurations.
+ *
+ * A higher priority means that the layer is at the top of
+ * the layer stack.
+ * Configurations have to be specified with its entries
+ * sorted with increasing priority.
+ *
+ * The default config assumes the following usage scenario:
+ * layer1: OSD [top]
+ * layer0: main framebuffer
+ * video layer: video overlay [bottom]
+ * Note that the video layer is only usable when the
+ * VP is available.
+ */
+
+static const struct layer_cfg nonvp_default_cfg[] = {
+	{ .index = 0, .priority = 1 },		/* layer0 */
+	{ .index = 1, .priority = 2 },		/* layer1 */
+};
+
+static const struct layer_cfg vp_default_cfg[] = {
+	{ .index = 2, .priority = 1 },		/* video layer */
+	{ .index = 0, .priority = 2 },		/* layer0 */
+	{ .index = 1, .priority = 3 },		/* layer1 */
+};
+
 static const u8 filter_y_horiz_tap8[] = {
 	0,	-1,	-1,	-1,	-1,	-1,	-1,	-1,
 	-1,	-1,	-1,	-1,	-1,	0,	0,	0,
@@ -268,6 +303,34 @@ static void vp_default_filter(struct mixer_resources *res)
 		filter_cr_horiz_tap4, sizeof(filter_cr_horiz_tap4));
 }
 
+static void mixer_layer_priority(struct mixer_context *ctx)
+{
+	u32 val = 0;
+	unsigned int i, priority;
+
+	for (i = 0; i < ctx->num_layer; ++i) {
+		priority = ctx->layer_cfg[i].priority;
+		BUG_ON(priority > 15);
+
+		switch (ctx->layer_cfg[i].index) {
+		case 0:
+			val |= MXR_LAYER_CFG_GRP0_VAL(priority);
+			break;
+		case 1:
+			val |= MXR_LAYER_CFG_GRP1_VAL(priority);
+			break;
+		case 2:
+			val |= MXR_LAYER_CFG_VP_VAL(priority);
+			break;
+		default:
+			BUG_ON(true);
+			break;
+		}
+	}
+
+	mixer_reg_write(&ctx->mixer_res, MXR_LAYER_CFG, val);
+}
+
 static void mixer_vsync_set_update(struct mixer_context *ctx, bool enable)
 {
 	struct mixer_resources *res = &ctx->mixer_res;
@@ -673,17 +736,7 @@ 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);
+	mixer_layer_priority(ctx);
 
 	/* setting background color */
 	mixer_reg_write(res, MXR_BG_COLOR0, 0x008080);
@@ -1209,6 +1262,15 @@ static int mixer_probe(struct platform_device *pdev)
 	ctx->vp_enabled = drv->is_vp_enabled;
 	ctx->has_sclk = drv->has_sclk;
 	ctx->mxr_ver = drv->version;
+
+	if (drv->is_vp_enabled) {
+		ctx->layer_cfg = vp_default_cfg;
+		ctx->num_layer = ARRAY_SIZE(vp_default_cfg);
+	} else {
+		ctx->layer_cfg = nonvp_default_cfg;
+		ctx->num_layer = ARRAY_SIZE(nonvp_default_cfg);
+	}
+
 	init_waitqueue_head(&ctx->wait_vsync_queue);
 	atomic_set(&ctx->wait_vsync_event, 0);
 
-- 
2.4.9

[PATCH v2 2/5] drm/exynos: mixer: introduce mixer_layer_blending()

[Tobias Jakobi]

This analyses the current layer configuration (which layers
are enabled, which have alpha-pixelformat, etc.) and setups
blending accordingly.

We currently disable all kinds of blending for the bottom-most
layer, since configuration of the mixer background is not
yet exposed.
Also blending is only enabled when the layer has a pixelformat
with alpha attached.

Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
---
 drivers/gpu/drm/exynos/exynos_mixer.c | 88 +++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/exynos/regs-mixer.h   |  1 +
 2 files changed, 89 insertions(+)

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 2c1cea3..ec77aad 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -174,6 +174,21 @@ static const u8 filter_cr_horiz_tap4[] = {
 	70,	59,	48,	37,	27,	19,	11,	5,
 };
 
+static inline bool is_alpha_format(const struct mixer_context* ctx, unsigned int win)
+{
+	const struct drm_plane_state *state = ctx->planes[win].base.state;
+	const struct drm_framebuffer *fb = state->fb;
+
+	switch (fb->pixel_format) {
+	case DRM_FORMAT_ARGB8888:
+	case DRM_FORMAT_ARGB1555:
+	case DRM_FORMAT_ARGB4444:
+		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);
@@ -331,6 +346,79 @@ static void mixer_layer_priority(struct mixer_context *ctx)
 	mixer_reg_write(&ctx->mixer_res, MXR_LAYER_CFG, val);
 }
 
+/* Configure blending for bottom-most layer. */
+static void mixer_bottom_layer(struct mixer_context *ctx,
+		const struct layer_cfg *cfg)
+{
+	u32 val;
+	struct mixer_resources *res = &ctx->mixer_res;
+
+	if (cfg->index == 2) {
+		val = 0; /* use defaults for video layer */
+		mixer_reg_write(res, MXR_VIDEO_CFG, val);
+	} else {
+		val = MXR_GRP_CFG_COLOR_KEY_DISABLE; /* no blank key */
+
+		/* Don't blend bottom-most layer onto the mixer background. */
+		mixer_reg_writemask(res, MXR_GRAPHIC_CFG(cfg->index),
+				    val, MXR_GRP_CFG_MISC_MASK);
+	}
+}
+
+static void mixer_general_layer(struct mixer_context *ctx,
+		const struct layer_cfg *cfg)
+{
+	u32 val;
+	struct mixer_resources *res = &ctx->mixer_res;
+
+	if (is_alpha_format(ctx, cfg->index)) {
+		val  = MXR_GRP_CFG_COLOR_KEY_DISABLE; /* no blank key */
+		val |= MXR_GRP_CFG_BLEND_PRE_MUL;
+		val |= MXR_GRP_CFG_PIXEL_BLEND_EN; /* blending based on pixel alpha */
+
+		/* The video layer never has an alpha pixelformat. */
+		mixer_reg_writemask(res, MXR_GRAPHIC_CFG(cfg->index),
+				    val, MXR_GRP_CFG_MISC_MASK);
+	} else {
+		if (cfg->index == 2) {
+			/*
+			 * 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);
+		} else {
+			val = MXR_GRP_CFG_COLOR_KEY_DISABLE;
+			mixer_reg_writemask(res, MXR_GRAPHIC_CFG(cfg->index),
+					    val, MXR_GRP_CFG_MISC_MASK);
+		}
+	}
+}
+
+static void mixer_layer_blending(struct mixer_context *ctx, unsigned int enable_state)
+{
+	unsigned int i, index;
+	bool bottom_layer = false;
+
+	for (i = 0; i < ctx->num_layer; ++i) {
+		index = ctx->layer_cfg[i].index;
+
+		/* Skip layer if it's not enabled. */
+		if (!(enable_state & (1 << index)))
+			continue;
+
+		/* Bottom layer needs special handling. */
+		if (bottom_layer) {
+			mixer_general_layer(ctx, &ctx->layer_cfg[i]);
+		} else {
+			mixer_bottom_layer(ctx, &ctx->layer_cfg[i]);
+			bottom_layer = true;
+		}
+	}
+}
+
 static void mixer_vsync_set_update(struct mixer_context *ctx, bool enable)
 {
 	struct mixer_resources *res = &ctx->mixer_res;
diff --git a/drivers/gpu/drm/exynos/regs-mixer.h b/drivers/gpu/drm/exynos/regs-mixer.h
index ac60260..118872e 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)
-- 
2.4.9

[PATCH v2 3/5] drm/exynos: mixer: remove all static blending setup

[Tobias Jakobi]

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 ec77aad..ec9659e 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -517,11 +517,6 @@ static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
 			vp_reg_writemask(res, VP_ENABLE, val, VP_ENABLE_ON);
 			mixer_reg_writemask(res, MXR_CFG, val,
 				MXR_CFG_VP_ENABLE);
-
-			/* 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;
 	}
@@ -810,7 +805,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);
@@ -831,23 +825,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);
-- 
2.4.9

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

[PATCH v2 4/5] drm/exynos: mixer: do blending setup in mixer_cfg_layer()

[Tobias Jakobi]

This updates the blending setup when the layer configuration
changes (triggered by mixer_win_{commit,disable}).

To avoid unnecesary reconfigurations we cache the layer
state in the mixer context.

Extra care has to be taken for the layer that is currently
being enabled/disabled.

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

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index ec9659e..1c24fb5 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -99,6 +99,7 @@ struct mixer_context {
 	struct exynos_drm_plane	planes[MIXER_WIN_NR];
 	const struct layer_cfg	*layer_cfg;
 	unsigned int		num_layer;
+	u32			layer_state;
 	int			pipe;
 	unsigned long		flags;
 	bool			interlace;
@@ -189,6 +190,27 @@ static inline bool is_alpha_format(const struct mixer_context* ctx, unsigned int
 	}
 }
 
+static inline u32 get_layer_state(const struct mixer_context *ctx,
+	unsigned int win, bool enable)
+{
+	u32 enable_state, alpha_state;
+
+	enable_state = ctx->layer_state & 0xffff;
+	alpha_state = ctx->layer_state >> 16;
+
+	if (enable)
+		enable_state |= (1 << win);
+	else
+		enable_state &= ~(1 << win);
+
+	if (enable && is_alpha_format(ctx, win))
+		alpha_state |= (1 << win);
+	else
+		alpha_state &= ~(1 << win);
+
+	return ((alpha_state << 16) | enable_state);
+}
+
 static inline u32 vp_reg_read(struct mixer_resources *res, u32 reg_id)
 {
 	return readl(res->vp_regs + reg_id);
@@ -370,8 +392,9 @@ static void mixer_general_layer(struct mixer_context *ctx,
 {
 	u32 val;
 	struct mixer_resources *res = &ctx->mixer_res;
+	const u32 alpha_state = ctx->layer_state >> 16;
 
-	if (is_alpha_format(ctx, cfg->index)) {
+	if (alpha_state & (1 << cfg->index)) {
 		val  = MXR_GRP_CFG_COLOR_KEY_DISABLE; /* no blank key */
 		val |= MXR_GRP_CFG_BLEND_PRE_MUL;
 		val |= MXR_GRP_CFG_PIXEL_BLEND_EN; /* blending based on pixel alpha */
@@ -397,10 +420,11 @@ static void mixer_general_layer(struct mixer_context *ctx,
 	}
 }
 
-static void mixer_layer_blending(struct mixer_context *ctx, unsigned int enable_state)
+static void mixer_layer_blending(struct mixer_context *ctx)
 {
 	unsigned int i, index;
 	bool bottom_layer = false;
+	const u32 enable_state = ctx->layer_state & 0xffff;
 
 	for (i = 0; i < ctx->num_layer; ++i) {
 		index = ctx->layer_cfg[i].index;
@@ -503,8 +527,19 @@ static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
 				bool enable)
 {
 	struct mixer_resources *res = &ctx->mixer_res;
+	u32 new_layer_state;
 	u32 val = enable ? ~0 : 0;
 
+	new_layer_state = get_layer_state(ctx, win, enable);
+	if (new_layer_state == ctx->layer_state)
+		return;
+
+	/*
+	 * Update the layer state so that mixer_layer_blending()
+	 * below can use it.
+	 */
+	ctx->layer_state = new_layer_state;
+
 	switch (win) {
 	case 0:
 		mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE);
@@ -520,6 +555,8 @@ static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
 		}
 		break;
 	}
+
+	mixer_layer_blending(ctx);
 }
 
 static void mixer_run(struct mixer_context *ctx)
-- 
2.4.9

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

[PATCH v2 5/5] drm/exynos: mixer: also allow ARGB1555 and ARGB4444

[Tobias Jakobi]

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

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

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 1c24fb5..245be7b 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -80,7 +80,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,
@@ -728,10 +730,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;
 
-- 
2.4.9

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

Re: [PATCH v2 1/5] drm/exynos: mixer: refactor layer setup

[Inki Dae]

Hi Tobias,

2015년 11월 23일 01:09에 Tobias Jakobi 이(가) 쓴 글:
> First step in allowing a more generic way to setup complex
> blending for the different layers.
> 
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
>  drivers/gpu/drm/exynos/exynos_mixer.c | 84 ++++++++++++++++++++++++++++++-----
>  1 file changed, 73 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> index 7498c6e..2c1cea3 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> @@ -63,6 +63,11 @@ struct mixer_resources {
>  	struct clk		*mout_mixer;
>  };
>  
> +struct layer_cfg {
> +	unsigned int index;
> +	unsigned int priority;
> +};
> +
>  enum mixer_version_id {
>  	MXR_VER_0_0_0_16,
>  	MXR_VER_16_0_33_0,
> @@ -92,6 +97,8 @@ struct mixer_context {
>  	struct drm_device	*drm_dev;
>  	struct exynos_drm_crtc	*crtc;
>  	struct exynos_drm_plane	planes[MIXER_WIN_NR];
> +	const struct layer_cfg	*layer_cfg;
> +	unsigned int		num_layer;
>  	int			pipe;
>  	unsigned long		flags;
>  	bool			interlace;
> @@ -110,6 +117,34 @@ struct mixer_drv_data {
>  	bool					has_sclk;
>  };
>  
> +/*
> + * The default layer priorities for non-VP (video processor)
> + * and VP mixer configurations.
> + *
> + * A higher priority means that the layer is at the top of
> + * the layer stack.
> + * Configurations have to be specified with its entries
> + * sorted with increasing priority.
> + *
> + * The default config assumes the following usage scenario:
> + * layer1: OSD [top]
> + * layer0: main framebuffer
> + * video layer: video overlay [bottom]
> + * Note that the video layer is only usable when the
> + * VP is available.
> + */
> +
> +static const struct layer_cfg nonvp_default_cfg[] = {
> +	{ .index = 0, .priority = 1 },		/* layer0 */
> +	{ .index = 1, .priority = 2 },		/* layer1 */
> +};
> +
> +static const struct layer_cfg vp_default_cfg[] = {
> +	{ .index = 2, .priority = 1 },		/* video layer */
> +	{ .index = 0, .priority = 2 },		/* layer0 */
> +	{ .index = 1, .priority = 3 },		/* layer1 */
> +};
> +
>  static const u8 filter_y_horiz_tap8[] = {
>  	0,	-1,	-1,	-1,	-1,	-1,	-1,	-1,
>  	-1,	-1,	-1,	-1,	-1,	0,	0,	0,
> @@ -268,6 +303,34 @@ static void vp_default_filter(struct mixer_resources *res)
>  		filter_cr_horiz_tap4, sizeof(filter_cr_horiz_tap4));
>  }
>  
> +static void mixer_layer_priority(struct mixer_context *ctx)
> +{
> +	u32 val = 0;
> +	unsigned int i, priority;
> +
> +	for (i = 0; i < ctx->num_layer; ++i) {
> +		priority = ctx->layer_cfg[i].priority;
> +		BUG_ON(priority > 15);

What doesn constant, 15 mean? You need to clarify the meaning
and please, use a macro instead.
And it'd better to just return in this case so that the layer
configuration can be set with a default value.
I think it'd be enough to print out warning message.

Thanks,
Inki Dae

> +
> +		switch (ctx->layer_cfg[i].index) {
> +		case 0:
> +			val |= MXR_LAYER_CFG_GRP0_VAL(priority);
> +			break;
> +		case 1:
> +			val |= MXR_LAYER_CFG_GRP1_VAL(priority);
> +			break;
> +		case 2:
> +			val |= MXR_LAYER_CFG_VP_VAL(priority);
> +			break;
> +		default:
> +			BUG_ON(true);
> +			break;
> +		}
> +	}
> +
> +	mixer_reg_write(&ctx->mixer_res, MXR_LAYER_CFG, val);
> +}
> +
>  static void mixer_vsync_set_update(struct mixer_context *ctx, bool enable)
>  {
>  	struct mixer_resources *res = &ctx->mixer_res;
> @@ -673,17 +736,7 @@ 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);
> +	mixer_layer_priority(ctx);
>  
>  	/* setting background color */
>  	mixer_reg_write(res, MXR_BG_COLOR0, 0x008080);
> @@ -1209,6 +1262,15 @@ static int mixer_probe(struct platform_device *pdev)
>  	ctx->vp_enabled = drv->is_vp_enabled;
>  	ctx->has_sclk = drv->has_sclk;
>  	ctx->mxr_ver = drv->version;
> +
> +	if (drv->is_vp_enabled) {
> +		ctx->layer_cfg = vp_default_cfg;
> +		ctx->num_layer = ARRAY_SIZE(vp_default_cfg);
> +	} else {
> +		ctx->layer_cfg = nonvp_default_cfg;
> +		ctx->num_layer = ARRAY_SIZE(nonvp_default_cfg);
> +	}
> +
>  	init_waitqueue_head(&ctx->wait_vsync_queue);
>  	atomic_set(&ctx->wait_vsync_event, 0);
>  
> 

Re: [PATCH v2 4/5] drm/exynos: mixer: do blending setup in mixer_cfg_layer()

[Inki Dae]

2015년 11월 23일 01:09에 Tobias Jakobi 이(가) 쓴 글:
> This updates the blending setup when the layer configuration
> changes (triggered by mixer_win_{commit,disable}).
> 
> To avoid unnecesary reconfigurations we cache the layer
> state in the mixer context.
> 
> Extra care has to be taken for the layer that is currently
> being enabled/disabled.
> 
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
>  drivers/gpu/drm/exynos/exynos_mixer.c | 41 +++++++++++++++++++++++++++++++++--
>  1 file changed, 39 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> index ec9659e..1c24fb5 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> @@ -99,6 +99,7 @@ struct mixer_context {
>  	struct exynos_drm_plane	planes[MIXER_WIN_NR];
>  	const struct layer_cfg	*layer_cfg;
>  	unsigned int		num_layer;
> +	u32			layer_state;
>  	int			pipe;
>  	unsigned long		flags;
>  	bool			interlace;
> @@ -189,6 +190,27 @@ static inline bool is_alpha_format(const struct mixer_context* ctx, unsigned int
>  	}
>  }
>  
> +static inline u32 get_layer_state(const struct mixer_context *ctx,
> +	unsigned int win, bool enable)
> +{
> +	u32 enable_state, alpha_state;
> +
> +	enable_state = ctx->layer_state & 0xffff;
> +	alpha_state = ctx->layer_state >> 16;
> +
> +	if (enable)
> +		enable_state |= (1 << win);
> +	else
> +		enable_state &= ~(1 << win);
> +
> +	if (enable && is_alpha_format(ctx, win))
> +		alpha_state |= (1 << win);
> +	else
> +		alpha_state &= ~(1 << win);
> +
> +	return ((alpha_state << 16) | enable_state);
> +}
> +
>  static inline u32 vp_reg_read(struct mixer_resources *res, u32 reg_id)
>  {
>  	return readl(res->vp_regs + reg_id);
> @@ -370,8 +392,9 @@ static void mixer_general_layer(struct mixer_context *ctx,
>  {
>  	u32 val;
>  	struct mixer_resources *res = &ctx->mixer_res;
> +	const u32 alpha_state = ctx->layer_state >> 16;
>  
> -	if (is_alpha_format(ctx, cfg->index)) {
> +	if (alpha_state & (1 << cfg->index)) {
>  		val  = MXR_GRP_CFG_COLOR_KEY_DISABLE; /* no blank key */
>  		val |= MXR_GRP_CFG_BLEND_PRE_MUL;
>  		val |= MXR_GRP_CFG_PIXEL_BLEND_EN; /* blending based on pixel alpha */
> @@ -397,10 +420,11 @@ static void mixer_general_layer(struct mixer_context *ctx,
>  	}
>  }
>  
> -static void mixer_layer_blending(struct mixer_context *ctx, unsigned int enable_state)
> +static void mixer_layer_blending(struct mixer_context *ctx)
>  {
>  	unsigned int i, index;
>  	bool bottom_layer = false;
> +	const u32 enable_state = ctx->layer_state & 0xffff;
>  
>  	for (i = 0; i < ctx->num_layer; ++i) {
>  		index = ctx->layer_cfg[i].index;
> @@ -503,8 +527,19 @@ static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
>  				bool enable)
>  {
>  	struct mixer_resources *res = &ctx->mixer_res;
> +	u32 new_layer_state;
>  	u32 val = enable ? ~0 : 0;
>  
> +	new_layer_state = get_layer_state(ctx, win, enable);
> +	if (new_layer_state == ctx->layer_state)
> +		return;
> +
> +	/*
> +	 * Update the layer state so that mixer_layer_blending()
> +	 * below can use it.
> +	 */
> +	ctx->layer_state = new_layer_state;

It may be trivial but I think it'd be better to move above line to most bottom of this function.

> +
>  	switch (win) {
>  	case 0:
>  		mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE);
> @@ -520,6 +555,8 @@ static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
>  		}
>  		break;
>  	}
> +
> +	mixer_layer_blending(ctx);

Here.

Thanks,
Inki Dae

>  }
>  
>  static void mixer_run(struct mixer_context *ctx)
> 

Re: [PATCH v2 2/5] drm/exynos: mixer: introduce mixer_layer_blending()

[Inki Dae]

2015년 11월 23일 01:09에 Tobias Jakobi 이(가) 쓴 글:
> This analyses the current layer configuration (which layers
> are enabled, which have alpha-pixelformat, etc.) and setups
> blending accordingly.
> 
> We currently disable all kinds of blending for the bottom-most
> layer, since configuration of the mixer background is not
> yet exposed.
> Also blending is only enabled when the layer has a pixelformat
> with alpha attached.
> 
> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
> ---
>  drivers/gpu/drm/exynos/exynos_mixer.c | 88 +++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/exynos/regs-mixer.h   |  1 +
>  2 files changed, 89 insertions(+)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> index 2c1cea3..ec77aad 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> @@ -174,6 +174,21 @@ static const u8 filter_cr_horiz_tap4[] = {
>  	70,	59,	48,	37,	27,	19,	11,	5,
>  };
>  
> +static inline bool is_alpha_format(const struct mixer_context* ctx, unsigned int win)
> +{
> +	const struct drm_plane_state *state = ctx->planes[win].base.state;
> +	const struct drm_framebuffer *fb = state->fb;
> +
> +	switch (fb->pixel_format) {
> +	case DRM_FORMAT_ARGB8888:
> +	case DRM_FORMAT_ARGB1555:
> +	case DRM_FORMAT_ARGB4444:
> +		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);
> @@ -331,6 +346,79 @@ static void mixer_layer_priority(struct mixer_context *ctx)
>  	mixer_reg_write(&ctx->mixer_res, MXR_LAYER_CFG, val);
>  }
>  
> +/* Configure blending for bottom-most layer. */
> +static void mixer_bottom_layer(struct mixer_context *ctx,
> +		const struct layer_cfg *cfg)
> +{
> +	u32 val;
> +	struct mixer_resources *res = &ctx->mixer_res;
> +
> +	if (cfg->index == 2) {
> +		val = 0; /* use defaults for video layer */
> +		mixer_reg_write(res, MXR_VIDEO_CFG, val);
> +	} else {
> +		val = MXR_GRP_CFG_COLOR_KEY_DISABLE; /* no blank key */
> +
> +		/* Don't blend bottom-most layer onto the mixer background. */
> +		mixer_reg_writemask(res, MXR_GRAPHIC_CFG(cfg->index),
> +				    val, MXR_GRP_CFG_MISC_MASK);
> +	}
> +}
> +
> +static void mixer_general_layer(struct mixer_context *ctx,
> +		const struct layer_cfg *cfg)
> +{
> +	u32 val;
> +	struct mixer_resources *res = &ctx->mixer_res;
> +
> +	if (is_alpha_format(ctx, cfg->index)) {
> +		val  = MXR_GRP_CFG_COLOR_KEY_DISABLE; /* no blank key */
> +		val |= MXR_GRP_CFG_BLEND_PRE_MUL;
> +		val |= MXR_GRP_CFG_PIXEL_BLEND_EN; /* blending based on pixel alpha */
> +
> +		/* The video layer never has an alpha pixelformat. */

Looks like above comment isn't related to graphics layer.

> +		mixer_reg_writemask(res, MXR_GRAPHIC_CFG(cfg->index),
> +				    val, MXR_GRP_CFG_MISC_MASK);
> +	} else {
> +		if (cfg->index == 2) {
> +			/*
> +			 * 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);

Above 'if statement' wouldn't be called because cfg->index has always a value less than 2
so it should be removed.

> +		} else {
> +			val = MXR_GRP_CFG_COLOR_KEY_DISABLE;
> +			mixer_reg_writemask(res, MXR_GRAPHIC_CFG(cfg->index),
> +					    val, MXR_GRP_CFG_MISC_MASK);
> +		}
> +	}
> +}
> +
> +static void mixer_layer_blending(struct mixer_context *ctx, unsigned int enable_state)
> +{
> +	unsigned int i, index;
> +	bool bottom_layer = false;
> +
> +	for (i = 0; i < ctx->num_layer; ++i) {
> +		index = ctx->layer_cfg[i].index;
> +
> +		/* Skip layer if it's not enabled. */
> +		if (!(enable_state & (1 << index)))
> +			continue;
> +
> +		/* Bottom layer needs special handling. */
> +		if (bottom_layer) {
> +			mixer_general_layer(ctx, &ctx->layer_cfg[i]);
> +		} else {
> +			mixer_bottom_layer(ctx, &ctx->layer_cfg[i]);
> +			bottom_layer = true;
> +		}

I think above codes could be more cleanned up like below if I understood correctly,

		if (cfg->index == 2) {
			val = 0;
			mixer_reg_write(res, MXR_VIDEO_CFG, val);
		} else {
			mixer_general_layer(ctx, &ctx->layer_cfg[i]);
		}


It'd be better to use a macro - i.e., VIDEO_LAYER - instead of 2.
And how about changing the function name, mixer_general_layer to mixer_grp_layer_set_blending?
Or how about just adding all codes of the function?

Thanks,
Inki Dae

> +	}
> +}
> +
>  static void mixer_vsync_set_update(struct mixer_context *ctx, bool enable)
>  {
>  	struct mixer_resources *res = &ctx->mixer_res;
> diff --git a/drivers/gpu/drm/exynos/regs-mixer.h b/drivers/gpu/drm/exynos/regs-mixer.h
> index ac60260..118872e 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)
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/5] drm/exynos: mixer: refactor layer setup

[Tobias Jakobi]

Hey Inki,


Inki Dae wrote:
> Hi Tobias,
> 
> 2015년 11월 23일 01:09에 Tobias Jakobi 이(가) 쓴 글:
>> First step in allowing a more generic way to setup complex
>> blending for the different layers.
>>
>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> ---
>>  drivers/gpu/drm/exynos/exynos_mixer.c | 84 ++++++++++++++++++++++++++++++-----
>>  1 file changed, 73 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
>> index 7498c6e..2c1cea3 100644
>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
>> @@ -63,6 +63,11 @@ struct mixer_resources {
>>  	struct clk		*mout_mixer;
>>  };
>>  
>> +struct layer_cfg {
>> +	unsigned int index;
>> +	unsigned int priority;
>> +};
>> +
>>  enum mixer_version_id {
>>  	MXR_VER_0_0_0_16,
>>  	MXR_VER_16_0_33_0,
>> @@ -92,6 +97,8 @@ struct mixer_context {
>>  	struct drm_device	*drm_dev;
>>  	struct exynos_drm_crtc	*crtc;
>>  	struct exynos_drm_plane	planes[MIXER_WIN_NR];
>> +	const struct layer_cfg	*layer_cfg;
>> +	unsigned int		num_layer;
>>  	int			pipe;
>>  	unsigned long		flags;
>>  	bool			interlace;
>> @@ -110,6 +117,34 @@ struct mixer_drv_data {
>>  	bool					has_sclk;
>>  };
>>  
>> +/*
>> + * The default layer priorities for non-VP (video processor)
>> + * and VP mixer configurations.
>> + *
>> + * A higher priority means that the layer is at the top of
>> + * the layer stack.
>> + * Configurations have to be specified with its entries
>> + * sorted with increasing priority.
>> + *
>> + * The default config assumes the following usage scenario:
>> + * layer1: OSD [top]
>> + * layer0: main framebuffer
>> + * video layer: video overlay [bottom]
>> + * Note that the video layer is only usable when the
>> + * VP is available.
>> + */
>> +
>> +static const struct layer_cfg nonvp_default_cfg[] = {
>> +	{ .index = 0, .priority = 1 },		/* layer0 */
>> +	{ .index = 1, .priority = 2 },		/* layer1 */
>> +};
>> +
>> +static const struct layer_cfg vp_default_cfg[] = {
>> +	{ .index = 2, .priority = 1 },		/* video layer */
>> +	{ .index = 0, .priority = 2 },		/* layer0 */
>> +	{ .index = 1, .priority = 3 },		/* layer1 */
>> +};
>> +
>>  static const u8 filter_y_horiz_tap8[] = {
>>  	0,	-1,	-1,	-1,	-1,	-1,	-1,	-1,
>>  	-1,	-1,	-1,	-1,	-1,	0,	0,	0,
>> @@ -268,6 +303,34 @@ static void vp_default_filter(struct mixer_resources *res)
>>  		filter_cr_horiz_tap4, sizeof(filter_cr_horiz_tap4));
>>  }
>>  
>> +static void mixer_layer_priority(struct mixer_context *ctx)
>> +{
>> +	u32 val = 0;
>> +	unsigned int i, priority;
>> +
>> +	for (i = 0; i < ctx->num_layer; ++i) {
>> +		priority = ctx->layer_cfg[i].priority;
>> +		BUG_ON(priority > 15);
> 
> What doesn constant, 15 mean? You need to clarify the meaning
> and please, use a macro instead.
If I read the specs correctly the layer priority is encoded with just
4-bits in the corresponding register. I'll add a define to make this
more clear.


> And it'd better to just return in this case so that the layer
> configuration can be set with a default value.
The point is that these (currently) are the default values that
mixer_layer_priority() sets.
The BUG_ON() is there to make sure that nobody changes
{vp,nonvp}_default_cfg[] later with illegal values.


With best wishes,
Tobias


> I think it'd be enough to print out warning message.
> 
> Thanks,
> Inki Dae
> 
>> +
>> +		switch (ctx->layer_cfg[i].index) {
>> +		case 0:
>> +			val |= MXR_LAYER_CFG_GRP0_VAL(priority);
>> +			break;
>> +		case 1:
>> +			val |= MXR_LAYER_CFG_GRP1_VAL(priority);
>> +			break;
>> +		case 2:
>> +			val |= MXR_LAYER_CFG_VP_VAL(priority);
>> +			break;
>> +		default:
>> +			BUG_ON(true);
>> +			break;
>> +		}
>> +	}
>> +
>> +	mixer_reg_write(&ctx->mixer_res, MXR_LAYER_CFG, val);
>> +}
>> +
>>  static void mixer_vsync_set_update(struct mixer_context *ctx, bool enable)
>>  {
>>  	struct mixer_resources *res = &ctx->mixer_res;
>> @@ -673,17 +736,7 @@ 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);
>> +	mixer_layer_priority(ctx);
>>  
>>  	/* setting background color */
>>  	mixer_reg_write(res, MXR_BG_COLOR0, 0x008080);
>> @@ -1209,6 +1262,15 @@ static int mixer_probe(struct platform_device *pdev)
>>  	ctx->vp_enabled = drv->is_vp_enabled;
>>  	ctx->has_sclk = drv->has_sclk;
>>  	ctx->mxr_ver = drv->version;
>> +
>> +	if (drv->is_vp_enabled) {
>> +		ctx->layer_cfg = vp_default_cfg;
>> +		ctx->num_layer = ARRAY_SIZE(vp_default_cfg);
>> +	} else {
>> +		ctx->layer_cfg = nonvp_default_cfg;
>> +		ctx->num_layer = ARRAY_SIZE(nonvp_default_cfg);
>> +	}
>> +
>>  	init_waitqueue_head(&ctx->wait_vsync_queue);
>>  	atomic_set(&ctx->wait_vsync_event, 0);
>>  
>>

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

Re: [PATCH v2 2/5] drm/exynos: mixer: introduce mixer_layer_blending()

[Tobias Jakobi]

Hey Inki,


Inki Dae wrote:
> 
> 
> 2015년 11월 23일 01:09에 Tobias Jakobi 이(가) 쓴 글:
>> This analyses the current layer configuration (which layers
>> are enabled, which have alpha-pixelformat, etc.) and setups
>> blending accordingly.
>>
>> We currently disable all kinds of blending for the bottom-most
>> layer, since configuration of the mixer background is not
>> yet exposed.
>> Also blending is only enabled when the layer has a pixelformat
>> with alpha attached.
>>
>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> ---
>>  drivers/gpu/drm/exynos/exynos_mixer.c | 88 +++++++++++++++++++++++++++++++++++
>>  drivers/gpu/drm/exynos/regs-mixer.h   |  1 +
>>  2 files changed, 89 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
>> index 2c1cea3..ec77aad 100644
>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
>> @@ -174,6 +174,21 @@ static const u8 filter_cr_horiz_tap4[] = {
>>  	70,	59,	48,	37,	27,	19,	11,	5,
>>  };
>>  
>> +static inline bool is_alpha_format(const struct mixer_context* ctx, unsigned int win)
>> +{
>> +	const struct drm_plane_state *state = ctx->planes[win].base.state;
>> +	const struct drm_framebuffer *fb = state->fb;
>> +
>> +	switch (fb->pixel_format) {
>> +	case DRM_FORMAT_ARGB8888:
>> +	case DRM_FORMAT_ARGB1555:
>> +	case DRM_FORMAT_ARGB4444:
>> +		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);
>> @@ -331,6 +346,79 @@ static void mixer_layer_priority(struct mixer_context *ctx)
>>  	mixer_reg_write(&ctx->mixer_res, MXR_LAYER_CFG, val);
>>  }
>>  
>> +/* Configure blending for bottom-most layer. */
>> +static void mixer_bottom_layer(struct mixer_context *ctx,
>> +		const struct layer_cfg *cfg)
>> +{
>> +	u32 val;
>> +	struct mixer_resources *res = &ctx->mixer_res;
>> +
>> +	if (cfg->index == 2) {
>> +		val = 0; /* use defaults for video layer */
>> +		mixer_reg_write(res, MXR_VIDEO_CFG, val);
>> +	} else {
>> +		val = MXR_GRP_CFG_COLOR_KEY_DISABLE; /* no blank key */
>> +
>> +		/* Don't blend bottom-most layer onto the mixer background. */
>> +		mixer_reg_writemask(res, MXR_GRAPHIC_CFG(cfg->index),
>> +				    val, MXR_GRP_CFG_MISC_MASK);
>> +	}
>> +}
>> +
>> +static void mixer_general_layer(struct mixer_context *ctx,
>> +		const struct layer_cfg *cfg)
>> +{
>> +	u32 val;
>> +	struct mixer_resources *res = &ctx->mixer_res;
>> +
>> +	if (is_alpha_format(ctx, cfg->index)) {
>> +		val  = MXR_GRP_CFG_COLOR_KEY_DISABLE; /* no blank key */
>> +		val |= MXR_GRP_CFG_BLEND_PRE_MUL;
>> +		val |= MXR_GRP_CFG_PIXEL_BLEND_EN; /* blending based on pixel alpha */
>> +
>> +		/* The video layer never has an alpha pixelformat. */
> 
> Looks like above comment isn't related to graphics layer.
Why do you think so? Because it certainly is.


>> +		mixer_reg_writemask(res, MXR_GRAPHIC_CFG(cfg->index),
>> +				    val, MXR_GRP_CFG_MISC_MASK);
>> +	} else {
>> +		if (cfg->index == 2) {
>> +			/*
>> +			 * 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);
> 
> Above 'if statement' wouldn't be called because cfg->index has always a value less than 2
> so it should be removed.
Can you explain why you think that index is always < 2 here?


>> +		} else {
>> +			val = MXR_GRP_CFG_COLOR_KEY_DISABLE;
>> +			mixer_reg_writemask(res, MXR_GRAPHIC_CFG(cfg->index),
>> +					    val, MXR_GRP_CFG_MISC_MASK);
>> +		}
>> +	}
>> +}
>> +
>> +static void mixer_layer_blending(struct mixer_context *ctx, unsigned int enable_state)
>> +{
>> +	unsigned int i, index;
>> +	bool bottom_layer = false;
>> +
>> +	for (i = 0; i < ctx->num_layer; ++i) {
>> +		index = ctx->layer_cfg[i].index;
>> +
>> +		/* Skip layer if it's not enabled. */
>> +		if (!(enable_state & (1 << index)))
>> +			continue;
>> +
>> +		/* Bottom layer needs special handling. */
>> +		if (bottom_layer) {
>> +			mixer_general_layer(ctx, &ctx->layer_cfg[i]);
>> +		} else {
>> +			mixer_bottom_layer(ctx, &ctx->layer_cfg[i]);
>> +			bottom_layer = true;
>> +		}
> 
> I think above codes could be more cleanned up like below if I understood correctly,
And then we end up with the same static setup again, this is not what I
try to achieve here. The final goal is to let userspace provide layer
priorities, so all these calls should work with arbitrary priorities.


With best wishes,
Tobias


> 		if (cfg->index == 2) {
> 			val = 0;
> 			mixer_reg_write(res, MXR_VIDEO_CFG, val);
> 		} else {
> 			mixer_general_layer(ctx, &ctx->layer_cfg[i]);
> 		}
> 
> 
> It'd be better to use a macro - i.e., VIDEO_LAYER - instead of 2.
> And how about changing the function name, mixer_general_layer to mixer_grp_layer_set_blending?
> Or how about just adding all codes of the function?
> 
> Thanks,
> Inki Dae
> 
>> +	}
>> +}
>> +
>>  static void mixer_vsync_set_update(struct mixer_context *ctx, bool enable)
>>  {
>>  	struct mixer_resources *res = &ctx->mixer_res;
>> diff --git a/drivers/gpu/drm/exynos/regs-mixer.h b/drivers/gpu/drm/exynos/regs-mixer.h
>> index ac60260..118872e 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)
>>

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

Re: [PATCH v2 4/5] drm/exynos: mixer: do blending setup in mixer_cfg_layer()

[Tobias Jakobi]

Hey Inki,


Inki Dae wrote:
> 
> 
> 2015년 11월 23일 01:09에 Tobias Jakobi 이(가) 쓴 글:
>> This updates the blending setup when the layer configuration
>> changes (triggered by mixer_win_{commit,disable}).
>>
>> To avoid unnecesary reconfigurations we cache the layer
>> state in the mixer context.
>>
>> Extra care has to be taken for the layer that is currently
>> being enabled/disabled.
>>
>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>> ---
>>  drivers/gpu/drm/exynos/exynos_mixer.c | 41 +++++++++++++++++++++++++++++++++--
>>  1 file changed, 39 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
>> index ec9659e..1c24fb5 100644
>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
>> @@ -99,6 +99,7 @@ struct mixer_context {
>>  	struct exynos_drm_plane	planes[MIXER_WIN_NR];
>>  	const struct layer_cfg	*layer_cfg;
>>  	unsigned int		num_layer;
>> +	u32			layer_state;
>>  	int			pipe;
>>  	unsigned long		flags;
>>  	bool			interlace;
>> @@ -189,6 +190,27 @@ static inline bool is_alpha_format(const struct mixer_context* ctx, unsigned int
>>  	}
>>  }
>>  
>> +static inline u32 get_layer_state(const struct mixer_context *ctx,
>> +	unsigned int win, bool enable)
>> +{
>> +	u32 enable_state, alpha_state;
>> +
>> +	enable_state = ctx->layer_state & 0xffff;
>> +	alpha_state = ctx->layer_state >> 16;
>> +
>> +	if (enable)
>> +		enable_state |= (1 << win);
>> +	else
>> +		enable_state &= ~(1 << win);
>> +
>> +	if (enable && is_alpha_format(ctx, win))
>> +		alpha_state |= (1 << win);
>> +	else
>> +		alpha_state &= ~(1 << win);
>> +
>> +	return ((alpha_state << 16) | enable_state);
>> +}
>> +
>>  static inline u32 vp_reg_read(struct mixer_resources *res, u32 reg_id)
>>  {
>>  	return readl(res->vp_regs + reg_id);
>> @@ -370,8 +392,9 @@ static void mixer_general_layer(struct mixer_context *ctx,
>>  {
>>  	u32 val;
>>  	struct mixer_resources *res = &ctx->mixer_res;
>> +	const u32 alpha_state = ctx->layer_state >> 16;
>>  
>> -	if (is_alpha_format(ctx, cfg->index)) {
>> +	if (alpha_state & (1 << cfg->index)) {
>>  		val  = MXR_GRP_CFG_COLOR_KEY_DISABLE; /* no blank key */
>>  		val |= MXR_GRP_CFG_BLEND_PRE_MUL;
>>  		val |= MXR_GRP_CFG_PIXEL_BLEND_EN; /* blending based on pixel alpha */
>> @@ -397,10 +420,11 @@ static void mixer_general_layer(struct mixer_context *ctx,
>>  	}
>>  }
>>  
>> -static void mixer_layer_blending(struct mixer_context *ctx, unsigned int enable_state)
>> +static void mixer_layer_blending(struct mixer_context *ctx)
>>  {
>>  	unsigned int i, index;
>>  	bool bottom_layer = false;
>> +	const u32 enable_state = ctx->layer_state & 0xffff;
>>  
>>  	for (i = 0; i < ctx->num_layer; ++i) {
>>  		index = ctx->layer_cfg[i].index;
>> @@ -503,8 +527,19 @@ static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
>>  				bool enable)
>>  {
>>  	struct mixer_resources *res = &ctx->mixer_res;
>> +	u32 new_layer_state;
>>  	u32 val = enable ? ~0 : 0;
>>  
>> +	new_layer_state = get_layer_state(ctx, win, enable);
>> +	if (new_layer_state == ctx->layer_state)
>> +		return;
>> +
>> +	/*
>> +	 * Update the layer state so that mixer_layer_blending()
>> +	 * below can use it.
>> +	 */
>> +	ctx->layer_state = new_layer_state;
> 
> It may be trivial but I think it'd be better to move above line to most bottom of this function.
Sure, I can do that, but I would rather know what's the rationale here.


With best wishes,
Tobias

>> +
>>  	switch (win) {
>>  	case 0:
>>  		mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE);
>> @@ -520,6 +555,8 @@ static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
>>  		}
>>  		break;
>>  	}
>> +
>> +	mixer_layer_blending(ctx);
> 
> Here.
> 
> Thanks,
> Inki Dae
> 
>>  }
>>  
>>  static void mixer_run(struct mixer_context *ctx)
>>

Re: [PATCH v2 4/5] drm/exynos: mixer: do blending setup in mixer_cfg_layer()

[Inki Dae]

2015-11-24 2:44 GMT+09:00 Tobias Jakobi <tjakobi@math.uni-bielefeld.de>:
> Hey Inki,
>
>
> Inki Dae wrote:
>>
>>
>> 2015년 11월 23일 01:09에 Tobias Jakobi 이(가) 쓴 글:
>>> This updates the blending setup when the layer configuration
>>> changes (triggered by mixer_win_{commit,disable}).
>>>
>>> To avoid unnecesary reconfigurations we cache the layer
>>> state in the mixer context.
>>>
>>> Extra care has to be taken for the layer that is currently
>>> being enabled/disabled.
>>>
>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>>> ---
>>>  drivers/gpu/drm/exynos/exynos_mixer.c | 41 +++++++++++++++++++++++++++++++++--
>>>  1 file changed, 39 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
>>> index ec9659e..1c24fb5 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
>>> @@ -99,6 +99,7 @@ struct mixer_context {
>>>      struct exynos_drm_plane planes[MIXER_WIN_NR];
>>>      const struct layer_cfg  *layer_cfg;
>>>      unsigned int            num_layer;
>>> +    u32                     layer_state;
>>>      int                     pipe;
>>>      unsigned long           flags;
>>>      bool                    interlace;
>>> @@ -189,6 +190,27 @@ static inline bool is_alpha_format(const struct mixer_context* ctx, unsigned int
>>>      }
>>>  }
>>>
>>> +static inline u32 get_layer_state(const struct mixer_context *ctx,
>>> +    unsigned int win, bool enable)
>>> +{
>>> +    u32 enable_state, alpha_state;
>>> +
>>> +    enable_state = ctx->layer_state & 0xffff;
>>> +    alpha_state = ctx->layer_state >> 16;
>>> +
>>> +    if (enable)
>>> +            enable_state |= (1 << win);
>>> +    else
>>> +            enable_state &= ~(1 << win);
>>> +
>>> +    if (enable && is_alpha_format(ctx, win))
>>> +            alpha_state |= (1 << win);
>>> +    else
>>> +            alpha_state &= ~(1 << win);
>>> +
>>> +    return ((alpha_state << 16) | enable_state);
>>> +}
>>> +
>>>  static inline u32 vp_reg_read(struct mixer_resources *res, u32 reg_id)
>>>  {
>>>      return readl(res->vp_regs + reg_id);
>>> @@ -370,8 +392,9 @@ static void mixer_general_layer(struct mixer_context *ctx,
>>>  {
>>>      u32 val;
>>>      struct mixer_resources *res = &ctx->mixer_res;
>>> +    const u32 alpha_state = ctx->layer_state >> 16;
>>>
>>> -    if (is_alpha_format(ctx, cfg->index)) {
>>> +    if (alpha_state & (1 << cfg->index)) {
>>>              val  = MXR_GRP_CFG_COLOR_KEY_DISABLE; /* no blank key */
>>>              val |= MXR_GRP_CFG_BLEND_PRE_MUL;
>>>              val |= MXR_GRP_CFG_PIXEL_BLEND_EN; /* blending based on pixel alpha */
>>> @@ -397,10 +420,11 @@ static void mixer_general_layer(struct mixer_context *ctx,
>>>      }
>>>  }
>>>
>>> -static void mixer_layer_blending(struct mixer_context *ctx, unsigned int enable_state)
>>> +static void mixer_layer_blending(struct mixer_context *ctx)
>>>  {
>>>      unsigned int i, index;
>>>      bool bottom_layer = false;
>>> +    const u32 enable_state = ctx->layer_state & 0xffff;
>>>
>>>      for (i = 0; i < ctx->num_layer; ++i) {
>>>              index = ctx->layer_cfg[i].index;
>>> @@ -503,8 +527,19 @@ static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
>>>                              bool enable)
>>>  {
>>>      struct mixer_resources *res = &ctx->mixer_res;
>>> +    u32 new_layer_state;
>>>      u32 val = enable ? ~0 : 0;
>>>
>>> +    new_layer_state = get_layer_state(ctx, win, enable);
>>> +    if (new_layer_state == ctx->layer_state)
>>> +            return;
>>> +
>>> +    /*
>>> +     * Update the layer state so that mixer_layer_blending()
>>> +     * below can use it.
>>> +     */
>>> +    ctx->layer_state = new_layer_state;
>>
>> It may be trivial but I think it'd be better to move above line to most bottom of this function.
> Sure, I can do that, but I would rather know what's the rationale here.

Very simple. Above line sets layer_state to new_layer_state logically
but physically not.
So it'd be reasonable to update layer state after physically setting
the mixer layer.

Thanks,
Inki Dae

>
>
> With best wishes,
> Tobias
>
>>> +
>>>      switch (win) {
>>>      case 0:
>>>              mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE);
>>> @@ -520,6 +555,8 @@ static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
>>>              }
>>>              break;
>>>      }
>>> +
>>> +    mixer_layer_blending(ctx);
>>
>> Here.
>>
>> Thanks,
>> Inki Dae
>>
>>>  }
>>>
>>>  static void mixer_run(struct mixer_context *ctx)
>>>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/5] drm/exynos: mixer: introduce mixer_layer_blending()

[Inki Dae]

2015-11-24 2:44 GMT+09:00 Tobias Jakobi <tjakobi@math.uni-bielefeld.de>:
> Hey Inki,
>
>
> Inki Dae wrote:
>>
>>
>> 2015년 11월 23일 01:09에 Tobias Jakobi 이(가) 쓴 글:
>>> This analyses the current layer configuration (which layers
>>> are enabled, which have alpha-pixelformat, etc.) and setups
>>> blending accordingly.
>>>
>>> We currently disable all kinds of blending for the bottom-most
>>> layer, since configuration of the mixer background is not
>>> yet exposed.
>>> Also blending is only enabled when the layer has a pixelformat
>>> with alpha attached.
>>>
>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>>> ---
>>>  drivers/gpu/drm/exynos/exynos_mixer.c | 88 +++++++++++++++++++++++++++++++++++
>>>  drivers/gpu/drm/exynos/regs-mixer.h   |  1 +
>>>  2 files changed, 89 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
>>> index 2c1cea3..ec77aad 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
>>> @@ -174,6 +174,21 @@ static const u8 filter_cr_horiz_tap4[] = {
>>>      70,     59,     48,     37,     27,     19,     11,     5,
>>>  };
>>>
>>> +static inline bool is_alpha_format(const struct mixer_context* ctx, unsigned int win)
>>> +{
>>> +    const struct drm_plane_state *state = ctx->planes[win].base.state;
>>> +    const struct drm_framebuffer *fb = state->fb;
>>> +
>>> +    switch (fb->pixel_format) {
>>> +    case DRM_FORMAT_ARGB8888:
>>> +    case DRM_FORMAT_ARGB1555:
>>> +    case DRM_FORMAT_ARGB4444:
>>> +            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);
>>> @@ -331,6 +346,79 @@ static void mixer_layer_priority(struct mixer_context *ctx)
>>>      mixer_reg_write(&ctx->mixer_res, MXR_LAYER_CFG, val);
>>>  }
>>>
>>> +/* Configure blending for bottom-most layer. */
>>> +static void mixer_bottom_layer(struct mixer_context *ctx,
>>> +            const struct layer_cfg *cfg)
>>> +{
>>> +    u32 val;
>>> +    struct mixer_resources *res = &ctx->mixer_res;
>>> +
>>> +    if (cfg->index == 2) {
>>> +            val = 0; /* use defaults for video layer */
>>> +            mixer_reg_write(res, MXR_VIDEO_CFG, val);
>>> +    } else {
>>> +            val = MXR_GRP_CFG_COLOR_KEY_DISABLE; /* no blank key */
>>> +
>>> +            /* Don't blend bottom-most layer onto the mixer background. */
>>> +            mixer_reg_writemask(res, MXR_GRAPHIC_CFG(cfg->index),
>>> +                                val, MXR_GRP_CFG_MISC_MASK);
>>> +    }
>>> +}
>>> +
>>> +static void mixer_general_layer(struct mixer_context *ctx,
>>> +            const struct layer_cfg *cfg)
>>> +{
>>> +    u32 val;
>>> +    struct mixer_resources *res = &ctx->mixer_res;
>>> +
>>> +    if (is_alpha_format(ctx, cfg->index)) {
>>> +            val  = MXR_GRP_CFG_COLOR_KEY_DISABLE; /* no blank key */
>>> +            val |= MXR_GRP_CFG_BLEND_PRE_MUL;
>>> +            val |= MXR_GRP_CFG_PIXEL_BLEND_EN; /* blending based on pixel alpha */
>>> +
>>> +            /* The video layer never has an alpha pixelformat. */
>>
>> Looks like above comment isn't related to graphics layer.
> Why do you think so? Because it certainly is.

Below line configures graphics layer not video layer. Why did you
comment it about video layer?

>
>
>>> +            mixer_reg_writemask(res, MXR_GRAPHIC_CFG(cfg->index),
>>> +                                val, MXR_GRP_CFG_MISC_MASK);
>>> +    } else {
>>> +            if (cfg->index == 2) {
>>> +                    /*
>>> +                     * 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);
>>
>> Above 'if statement' wouldn't be called because cfg->index has always a value less than 2
>> so it should be removed.
> Can you explain why you think that index is always < 2 here?

Please, see mixer_layer_blending function. In this function, a local
variable, a bottom_layer is declared and
this has false as a default. At first loop, mixer_general_layer
function won't be called
but at second loop like below,

for (i = 0; i < ctx->num_layer; ++i) {
        index = ctx->layer_cfg[i].index;

        /* Skip layer if it's not enabled. */
        if (!(enable_state & (1 << index)))
                continue;

        /* Bottom layer needs special handling. */
        if (bottom_layer) {
                mixer_general_layer(ctx, &ctx->layer_cfg[i]);    <-
and second call.
        } else {
                mixer_bottom_layer(ctx, &ctx->layer_cfg[i]);     <-- first call.
                bottom_layer = true;
}

And also, see the below declarations,

static const struct layer_cfg nonvp_default_cfg[] = {
       { .index = 0, .priority = 1 },          /* layer0 */
       { .index = 1, .priority = 2 },          /* layer1 */
};

static const struct layer_cfg vp_default_cfg[] = {
       { .index = 2, .priority = 1 },          /* video layer */
       { .index = 0, .priority = 2 },          /* layer0 */
       { .index = 1, .priority = 3 },          /* layer1 */
};

As you can see, the first position of above array has 'index = 2' so
'if (cfg->index == 2' statement of mixer_general_layer function would be true
only when mixer_general_layer function is called at first loop in
mixer_layer_blending function.

By any chance, did you consider that vp default configuration can be changed?
If so, what Exynos SoC could have such configuration?
Or how the index value could be changed in runtime?
Even you declared the above default configurations with const.

Thanks,
Inki Dae

>
>
>>> +            } else {
>>> +                    val = MXR_GRP_CFG_COLOR_KEY_DISABLE;
>>> +                    mixer_reg_writemask(res, MXR_GRAPHIC_CFG(cfg->index),
>>> +                                        val, MXR_GRP_CFG_MISC_MASK);
>>> +            }
>>> +    }
>>> +}
>>> +
>>> +static void mixer_layer_blending(struct mixer_context *ctx, unsigned int enable_state)
>>> +{
>>> +    unsigned int i, index;
>>> +    bool bottom_layer = false;
>>> +
>>> +    for (i = 0; i < ctx->num_layer; ++i) {
>>> +            index = ctx->layer_cfg[i].index;
>>> +
>>> +            /* Skip layer if it's not enabled. */
>>> +            if (!(enable_state & (1 << index)))
>>> +                    continue;
>>> +
>>> +            /* Bottom layer needs special handling. */
>>> +            if (bottom_layer) {
>>> +                    mixer_general_layer(ctx, &ctx->layer_cfg[i]);
>>> +            } else {
>>> +                    mixer_bottom_layer(ctx, &ctx->layer_cfg[i]);
>>> +                    bottom_layer = true;
>>> +            }
>>
>> I think above codes could be more cleanned up like below if I understood correctly,
> And then we end up with the same static setup again, this is not what I
> try to achieve here. The final goal is to let userspace provide layer
> priorities, so all these calls should work with arbitrary priorities.
>
>
> With best wishes,
> Tobias
>
>
>>               if (cfg->index == 2) {
>>                       val = 0;
>>                       mixer_reg_write(res, MXR_VIDEO_CFG, val);
>>               } else {
>>                       mixer_general_layer(ctx, &ctx->layer_cfg[i]);
>>               }
>>
>>
>> It'd be better to use a macro - i.e., VIDEO_LAYER - instead of 2.
>> And how about changing the function name, mixer_general_layer to mixer_grp_layer_set_blending?
>> Or how about just adding all codes of the function?
>>
>> Thanks,
>> Inki Dae
>>
>>> +    }
>>> +}
>>> +
>>>  static void mixer_vsync_set_update(struct mixer_context *ctx, bool enable)
>>>  {
>>>      struct mixer_resources *res = &ctx->mixer_res;
>>> diff --git a/drivers/gpu/drm/exynos/regs-mixer.h b/drivers/gpu/drm/exynos/regs-mixer.h
>>> index ac60260..118872e 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)
>>>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 1/5] drm/exynos: mixer: refactor layer setup

[Inki Dae]

2015-11-24 2:44 GMT+09:00 Tobias Jakobi <tjakobi@math.uni-bielefeld.de>:
> Hey Inki,
>
>
> Inki Dae wrote:
>> Hi Tobias,
>>
>> 2015년 11월 23일 01:09에 Tobias Jakobi 이(가) 쓴 글:
>>> First step in allowing a more generic way to setup complex
>>> blending for the different layers.
>>>
>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>>> ---
>>>  drivers/gpu/drm/exynos/exynos_mixer.c | 84 ++++++++++++++++++++++++++++++-----
>>>  1 file changed, 73 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
>>> index 7498c6e..2c1cea3 100644
>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
>>> @@ -63,6 +63,11 @@ struct mixer_resources {
>>>      struct clk              *mout_mixer;
>>>  };
>>>
>>> +struct layer_cfg {
>>> +    unsigned int index;
>>> +    unsigned int priority;
>>> +};
>>> +
>>>  enum mixer_version_id {
>>>      MXR_VER_0_0_0_16,
>>>      MXR_VER_16_0_33_0,
>>> @@ -92,6 +97,8 @@ struct mixer_context {
>>>      struct drm_device       *drm_dev;
>>>      struct exynos_drm_crtc  *crtc;
>>>      struct exynos_drm_plane planes[MIXER_WIN_NR];
>>> +    const struct layer_cfg  *layer_cfg;
>>> +    unsigned int            num_layer;
>>>      int                     pipe;
>>>      unsigned long           flags;
>>>      bool                    interlace;
>>> @@ -110,6 +117,34 @@ struct mixer_drv_data {
>>>      bool                                    has_sclk;
>>>  };
>>>
>>> +/*
>>> + * The default layer priorities for non-VP (video processor)
>>> + * and VP mixer configurations.
>>> + *
>>> + * A higher priority means that the layer is at the top of
>>> + * the layer stack.
>>> + * Configurations have to be specified with its entries
>>> + * sorted with increasing priority.
>>> + *
>>> + * The default config assumes the following usage scenario:
>>> + * layer1: OSD [top]
>>> + * layer0: main framebuffer
>>> + * video layer: video overlay [bottom]
>>> + * Note that the video layer is only usable when the
>>> + * VP is available.
>>> + */
>>> +
>>> +static const struct layer_cfg nonvp_default_cfg[] = {
>>> +    { .index = 0, .priority = 1 },          /* layer0 */
>>> +    { .index = 1, .priority = 2 },          /* layer1 */
>>> +};
>>> +
>>> +static const struct layer_cfg vp_default_cfg[] = {
>>> +    { .index = 2, .priority = 1 },          /* video layer */
>>> +    { .index = 0, .priority = 2 },          /* layer0 */
>>> +    { .index = 1, .priority = 3 },          /* layer1 */
>>> +};
>>> +
>>>  static const u8 filter_y_horiz_tap8[] = {
>>>      0,      -1,     -1,     -1,     -1,     -1,     -1,     -1,
>>>      -1,     -1,     -1,     -1,     -1,     0,      0,      0,
>>> @@ -268,6 +303,34 @@ static void vp_default_filter(struct mixer_resources *res)
>>>              filter_cr_horiz_tap4, sizeof(filter_cr_horiz_tap4));
>>>  }
>>>
>>> +static void mixer_layer_priority(struct mixer_context *ctx)
>>> +{
>>> +    u32 val = 0;
>>> +    unsigned int i, priority;
>>> +
>>> +    for (i = 0; i < ctx->num_layer; ++i) {
>>> +            priority = ctx->layer_cfg[i].priority;
>>> +            BUG_ON(priority > 15);
>>
>> What doesn constant, 15 mean? You need to clarify the meaning
>> and please, use a macro instead.
> If I read the specs correctly the layer priority is encoded with just
> 4-bits in the corresponding register. I'll add a define to make this
> more clear.

Please use a macro instead of 15.

>
>
>> And it'd better to just return in this case so that the layer
>> configuration can be set with a default value.
> The point is that these (currently) are the default values that
> mixer_layer_priority() sets.
> The BUG_ON() is there to make sure that nobody changes
> {vp,nonvp}_default_cfg[] later with illegal values.

Please, know that BUG_ON is very strong macro so kernel will be halt.
By any chance, if someone changed the definitions, then it'd be better
to return error
so that other devices could work correctly.

Thanks,
Inki Dae

>
>
> With best wishes,
> Tobias
>
>
>> I think it'd be enough to print out warning message.
>>
>> Thanks,
>> Inki Dae
>>
>>> +
>>> +            switch (ctx->layer_cfg[i].index) {
>>> +            case 0:
>>> +                    val |= MXR_LAYER_CFG_GRP0_VAL(priority);
>>> +                    break;
>>> +            case 1:
>>> +                    val |= MXR_LAYER_CFG_GRP1_VAL(priority);
>>> +                    break;
>>> +            case 2:
>>> +                    val |= MXR_LAYER_CFG_VP_VAL(priority);
>>> +                    break;
>>> +            default:
>>> +                    BUG_ON(true);
>>> +                    break;
>>> +            }
>>> +    }
>>> +
>>> +    mixer_reg_write(&ctx->mixer_res, MXR_LAYER_CFG, val);
>>> +}
>>> +
>>>  static void mixer_vsync_set_update(struct mixer_context *ctx, bool enable)
>>>  {
>>>      struct mixer_resources *res = &ctx->mixer_res;
>>> @@ -673,17 +736,7 @@ 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);
>>> +    mixer_layer_priority(ctx);
>>>
>>>      /* setting background color */
>>>      mixer_reg_write(res, MXR_BG_COLOR0, 0x008080);
>>> @@ -1209,6 +1262,15 @@ static int mixer_probe(struct platform_device *pdev)
>>>      ctx->vp_enabled = drv->is_vp_enabled;
>>>      ctx->has_sclk = drv->has_sclk;
>>>      ctx->mxr_ver = drv->version;
>>> +
>>> +    if (drv->is_vp_enabled) {
>>> +            ctx->layer_cfg = vp_default_cfg;
>>> +            ctx->num_layer = ARRAY_SIZE(vp_default_cfg);
>>> +    } else {
>>> +            ctx->layer_cfg = nonvp_default_cfg;
>>> +            ctx->num_layer = ARRAY_SIZE(nonvp_default_cfg);
>>> +    }
>>> +
>>>      init_waitqueue_head(&ctx->wait_vsync_queue);
>>>      atomic_set(&ctx->wait_vsync_event, 0);
>>>
>>>
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 4/5] drm/exynos: mixer: do blending setup in mixer_cfg_layer()

[Tobias Jakobi]

Hello Inki,

my main system which I also used for development was stolen on Tuesday,
so I won't be working on this series anytime soon. If anyone wants to
pick it up, please go ahead.

- Tobias


Inki Dae wrote:
> 2015-11-24 2:44 GMT+09:00 Tobias Jakobi <tjakobi@math.uni-bielefeld.de>:
>> Hey Inki,
>>
>>
>> Inki Dae wrote:
>>>
>>> 2015년 11월 23일 01:09에 Tobias Jakobi 이(가) 쓴 글:
>>>> This updates the blending setup when the layer configuration
>>>> changes (triggered by mixer_win_{commit,disable}).
>>>>
>>>> To avoid unnecesary reconfigurations we cache the layer
>>>> state in the mixer context.
>>>>
>>>> Extra care has to be taken for the layer that is currently
>>>> being enabled/disabled.
>>>>
>>>> Signed-off-by: Tobias Jakobi <tjakobi@math.uni-bielefeld.de>
>>>> ---
>>>>  drivers/gpu/drm/exynos/exynos_mixer.c | 41 +++++++++++++++++++++++++++++++++--
>>>>  1 file changed, 39 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
>>>> index ec9659e..1c24fb5 100644
>>>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
>>>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
>>>> @@ -99,6 +99,7 @@ struct mixer_context {
>>>>      struct exynos_drm_plane planes[MIXER_WIN_NR];
>>>>      const struct layer_cfg  *layer_cfg;
>>>>      unsigned int            num_layer;
>>>> +    u32                     layer_state;
>>>>      int                     pipe;
>>>>      unsigned long           flags;
>>>>      bool                    interlace;
>>>> @@ -189,6 +190,27 @@ static inline bool is_alpha_format(const struct mixer_context* ctx, unsigned int
>>>>      }
>>>>  }
>>>>
>>>> +static inline u32 get_layer_state(const struct mixer_context *ctx,
>>>> +    unsigned int win, bool enable)
>>>> +{
>>>> +    u32 enable_state, alpha_state;
>>>> +
>>>> +    enable_state =tx->layer_state & 0xffff;
>>>> +    alpha_state =tx->layer_state >> 16;
>>>> +
>>>> +    if (enable)
>>>> +            enable_state |=1 << win);
>>>> +    else
>>>> +            enable_state &=(1 << win);
>>>> +
>>>> +    if (enable && is_alpha_format(ctx, win))
>>>> +            alpha_state |=1 << win);
>>>> +    else
>>>> +            alpha_state &=(1 << win);
>>>> +
>>>> +    return ((alpha_state << 16) | enable_state);
>>>> +}
>>>> +
>>>>  static inline u32 vp_reg_read(struct mixer_resources *res, u32 reg_id)
>>>>  {
>>>>      return readl(res->vp_regs + reg_id);
>>>> @@ -370,8 +392,9 @@ static void mixer_general_layer(struct mixer_context *ctx,
>>>>  {
>>>>      u32 val;
>>>>      struct mixer_resources *res =ctx->mixer_res;
>>>> +    const u32 alpha_state =tx->layer_state >> 16;
>>>>
>>>> -    if (is_alpha_format(ctx, cfg->index)) {
>>>> +    if (alpha_state & (1 << cfg->index)) {
>>>>              val  =XR_GRP_CFG_COLOR_KEY_DISABLE; /* no blank key */
>>>>              val |=XR_GRP_CFG_BLEND_PRE_MUL;
>>>>              val |=XR_GRP_CFG_PIXEL_BLEND_EN; /* blending based on pixel alpha */
>>>> @@ -397,10 +420,11 @@ static void mixer_general_layer(struct mixer_context *ctx,
>>>>      }
>>>>  }
>>>>
>>>> -static void mixer_layer_blending(struct mixer_context *ctx, unsigned int enable_state)
>>>> +static void mixer_layer_blending(struct mixer_context *ctx)
>>>>  {
>>>>      unsigned int i, index;
>>>>      bool bottom_layer =alse;
>>>> +    const u32 enable_state =tx->layer_state & 0xffff;
>>>>
>>>>      for (i =; i < ctx->num_layer; ++i) {
>>>>              index =tx->layer_cfg[i].index;
>>>> @@ -503,8 +527,19 @@ static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
>>>>                              bool enable)
>>>>  {
>>>>      struct mixer_resources *res =ctx->mixer_res;
>>>> +    u32 new_layer_state;
>>>>      u32 val =nable ? ~0 : 0;
>>>>
>>>> +    new_layer_state =et_layer_state(ctx, win, enable);
>>>> +    if (new_layer_state =ctx->layer_state)
>>>> +            return;
>>>> +
>>>> +    /*
>>>> +     * Update the layer state so that mixer_layer_blending()
>>>> +     * below can use it.
>>>> +     */
>>>> +    ctx->layer_state =ew_layer_state;
>>> It may be trivial but I think it'd be better to move above line to most bottom of this function.
>> Sure, I can do that, but I would rather know what's the rationale here.
> Very simple. Above line sets layer_state to new_layer_state logically
> but physically not.
> So it'd be reasonable to update layer state after physically setting
> the mixer layer.
>
> Thanks,
> Inki Dae
>
>>
>> With best wishes,
>> Tobias
>>
>>>> +
>>>>      switch (win) {
>>>>      case 0:
>>>>              mixer_reg_writemask(res, MXR_CFG, val, MXR_CFG_GRP0_ENABLE);
>>>> @@ -520,6 +555,8 @@ static void mixer_cfg_layer(struct mixer_context *ctx, unsigned int win,
>>>>              }
>>>>              break;
>>>>      }
>>>> +
>>>> +    mixer_layer_blending(ctx);
>>> Here.
>>>
>>> Thanks,
>>> Inki Dae
>>>
>>>>  }
>>>>
>>>>  static void mixer_run(struct mixer_context *ctx)
>>>>
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/dri-devel
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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