[RFC] drm/exynos: rework layer blending setup

[RFC] drm/exynos: rework layer blending setup

[Tobias Jakobi]

Hello,

here's the rework of the layer blending setup that I discussed with Joonyoung in the past days. There's still 
some TODOs in the code, but more or less it does what it's supposed to do. What still bothers me a bit is that I 
currently call blending reconfig in mixer_cfg_layer(). It would be nice if this could be reduced to one call per 
"frame" (so with the last win_{commit,disable} call). Maybe atomic provides such an infrastructure?

With best wishes,
Tobias

[RFC 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 | 74 +++++++++++++++++++++++++++++------
 1 file changed, 63 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 4155f43..a06b8e2 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -63,6 +63,12 @@ struct mixer_resources {
 	struct clk		*mout_mixer;
 };
 
+struct layer_config {
+	unsigned int index;
+	unsigned int priority;
+	u32 cfg;
+};
+
 enum mixer_version_id {
 	MXR_VER_0_0_0_16,
 	MXR_VER_16_0_33_0,
@@ -75,6 +81,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_config *layer_config;
+	unsigned int num_layer;
 	int			pipe;
 	bool			interlace;
 	bool			powered;
@@ -95,6 +103,40 @@ struct mixer_drv_data {
 	bool					has_sclk;
 };
 
+/*
+ * The default layer priorities. A higher priority means that
+ * the layer is at the top of layer stack.
+ * The current configuration 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
+ * video processor is available.
+ */
+
+static const struct layer_config default_layer_config[] = {
+	{
+		.index = 0, .priority = 1, /* layer0 */
+		.cfg = MXR_LAYER_CFG_GRP0_VAL(1)
+	}, {
+		.index = 1, .priority = 2, /* layer1 */
+		.cfg = MXR_LAYER_CFG_GRP1_VAL(2)
+	}
+};
+
+static const struct layer_config vp_layer_config[] = {
+	{
+		.index = 2, .priority = 1, /* video layer */
+		.cfg = MXR_LAYER_CFG_VP_VAL(1)
+	}, {
+		.index = 0, .priority = 2, /* layer0 */
+		.cfg = MXR_LAYER_CFG_GRP0_VAL(2)
+	}, {
+		.index = 1, .priority = 3, /* layer1 */
+		.cfg = MXR_LAYER_CFG_GRP1_VAL(3)
+	}
+};
+
 static const u8 filter_y_horiz_tap8[] = {
 	0,	-1,	-1,	-1,	-1,	-1,	-1,	-1,
 	-1,	-1,	-1,	-1,	-1,	0,	0,	0,
@@ -253,6 +295,17 @@ 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;
+
+	for (i = 0; i < ctx->num_layer; ++i)
+		val |= ctx->layer_config[i].cfg;
+
+	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;
@@ -655,17 +708,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);
@@ -1274,6 +1317,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_config = vp_layer_config;
+		ctx->num_layer = ARRAY_SIZE(vp_layer_config);
+	} else {
+		ctx->layer_config = default_layer_config;
+		ctx->num_layer = ARRAY_SIZE(default_layer_config);
+	}
+
 	init_waitqueue_head(&ctx->wait_vsync_queue);
 	atomic_set(&ctx->wait_vsync_event, 0);
 
-- 
2.0.5

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

[RFC 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 | 101 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/exynos/regs-mixer.h   |   1 +
 2 files changed, 102 insertions(+)

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index a06b8e2..ff1168d 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -166,6 +166,18 @@ 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)
+{
+	switch (ctx->planes[win].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);
@@ -306,6 +318,95 @@ 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_config *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_config *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;
+
+		/* 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) {
+			// TODO
+		} 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)
+{
+	const struct layer_config *cfg;
+	unsigned int i = 0;
+	unsigned int index;
+
+	/* Find bottom-most enabled layer. */
+	cfg = NULL;
+	while (i < ctx->num_layer) {
+		index = ctx->layer_config[i].index;
+		++i;
+
+		if (enable_state & (1 << index)) {
+			cfg = &ctx->layer_config[i-1];
+			break;
+		}
+	}
+
+	/* No enabled layers found, nothing to do. */
+	if (!cfg)
+		return;
+
+	mixer_bottom_layer(ctx, cfg);
+
+	while (1) {
+		/* Find the next layer. */
+		cfg = NULL;
+		while (i < ctx->num_layer) {
+			index = ctx->layer_config[i].index;
+			++i;
+
+			if (enable_state & (1 << index)) {
+				cfg = &ctx->layer_config[i-1];
+				break;
+			}
+		}
+
+		/* No more enabled layers found. */
+		if (!cfg)
+			return;
+
+		mixer_general_layer(ctx, cfg);
+	}
+}
+
 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.0.5

[RFC 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 | 22 ----------------------
 1 file changed, 22 deletions(-)

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index ff1168d..3eec9ce 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -504,11 +504,6 @@ static void mixer_cfg_layer(struct mixer_context *ctx, int win, bool enable)
 			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;
 	}
@@ -816,23 +811,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.0.5

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

[RFC 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}).

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 | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 3eec9ce..809f840 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -178,6 +178,18 @@ static inline bool is_alpha_format(const struct mixer_context* ctx, unsigned int
 	}
 }
 
+static inline unsigned int layer_bitmask(const struct mixer_context* ctx)
+{
+	unsigned int i, mask = 0;
+
+	for (i = 0; i < MIXER_WIN_NR; ++i) {
+		if (ctx->planes[i].enabled)
+			mask |= (1 << i);
+	}
+
+	return mask;
+}
+
 static inline u32 vp_reg_read(struct mixer_resources *res, u32 reg_id)
 {
 	return readl(res->vp_regs + reg_id);
@@ -490,6 +502,7 @@ static void mixer_cfg_rgb_fmt(struct mixer_context *ctx, unsigned int height)
 static void mixer_cfg_layer(struct mixer_context *ctx, int win, bool enable)
 {
 	struct mixer_resources *res = &ctx->mixer_res;
+	unsigned int enable_state;
 	u32 val = enable ? ~0 : 0;
 
 	switch (win) {
@@ -507,6 +520,16 @@ static void mixer_cfg_layer(struct mixer_context *ctx, int win, bool enable)
 		}
 		break;
 	}
+
+	/* Determine the current enabled/disabled state of the layers. */
+	enable_state = layer_bitmask(ctx);
+	if (enable)
+		enable_state |= (1 << win);
+	else
+		enable_state &= ~(1 << win);
+
+	/* Layer configuration has changed, update blending setup. */
+	mixer_layer_blending(ctx, enable_state);
 }
 
 static void mixer_run(struct mixer_context *ctx)
-- 
2.0.5

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

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

[Tobias Jakobi]

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

diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
index 809f840..5a435aa 100644
--- a/drivers/gpu/drm/exynos/exynos_mixer.c
+++ b/drivers/gpu/drm/exynos/exynos_mixer.c
@@ -701,10 +701,12 @@ static void mixer_graph_buffer(struct mixer_context *ctx, int win)
 
 	switch (plane->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.0.5

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

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

[Gustavo Padovan]

Hi Tobias,

2015-04-30 Tobias Jakobi <tjakobi@math.uni-bielefeld.de>:

> 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 | 74 +++++++++++++++++++++++++++++------
>  1 file changed, 63 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> index 4155f43..a06b8e2 100644
> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> @@ -63,6 +63,12 @@ struct mixer_resources {
>  	struct clk		*mout_mixer;
>  };
>  
> +struct layer_config {
> +	unsigned int index;
> +	unsigned int priority;
> +	u32 cfg;
> +};

I don't see why you are creating this struct, index and priority are
never used in this patch series.

> +
>  enum mixer_version_id {
>  	MXR_VER_0_0_0_16,
>  	MXR_VER_16_0_33_0,
> @@ -75,6 +81,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_config *layer_config;
> +	unsigned int num_layer;
>  	int			pipe;
>  	bool			interlace;
>  	bool			powered;
> @@ -95,6 +103,40 @@ struct mixer_drv_data {
>  	bool					has_sclk;
>  };
>  
> +/*
> + * The default layer priorities. A higher priority means that
> + * the layer is at the top of layer stack.
> + * The current configuration 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
> + * video processor is available.
> + */
> +
> +static const struct layer_config default_layer_config[] = {
> +	{
> +		.index = 0, .priority = 1, /* layer0 */
> +		.cfg = MXR_LAYER_CFG_GRP0_VAL(1)
> +	}, {
> +		.index = 1, .priority = 2, /* layer1 */
> +		.cfg = MXR_LAYER_CFG_GRP1_VAL(2)
> +	}
> +};
> +
> +static const struct layer_config vp_layer_config[] = {
> +	{
> +		.index = 2, .priority = 1, /* video layer */
> +		.cfg = MXR_LAYER_CFG_VP_VAL(1)
> +	}, {
> +		.index = 0, .priority = 2, /* layer0 */
> +		.cfg = MXR_LAYER_CFG_GRP0_VAL(2)
> +	}, {
> +		.index = 1, .priority = 3, /* layer1 */
> +		.cfg = MXR_LAYER_CFG_GRP1_VAL(3)
> +	}
> +};
> +
>  static const u8 filter_y_horiz_tap8[] = {
>  	0,	-1,	-1,	-1,	-1,	-1,	-1,	-1,
>  	-1,	-1,	-1,	-1,	-1,	0,	0,	0,
> @@ -253,6 +295,17 @@ 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;
> +
> +	for (i = 0; i < ctx->num_layer; ++i)
> +		val |= ctx->layer_config[i].cfg;
> +
> +	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;
> @@ -655,17 +708,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);

I would move this exaclty piece of code into mixer_layer_priority().

> +	mixer_layer_priority(ctx);
>  
>  	/* setting background color */
>  	mixer_reg_write(res, MXR_BG_COLOR0, 0x008080);
> @@ -1274,6 +1317,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_config = vp_layer_config;
> +		ctx->num_layer = ARRAY_SIZE(vp_layer_config);
> +	} else {
> +		ctx->layer_config = default_layer_config;
> +		ctx->num_layer = ARRAY_SIZE(default_layer_config);
> +	}

Then this piece of code is useless.

	Gustavo

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

[Tobias Jakobi]

Hello Gustavo!


Gustavo Padovan wrote:
> Hi Tobias,
> 
> 2015-04-30 Tobias Jakobi <tjakobi@math.uni-bielefeld.de>:
> 
>> 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 | 74 +++++++++++++++++++++++++++++------
>>  1 file changed, 63 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
>> index 4155f43..a06b8e2 100644
>> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
>> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
>> @@ -63,6 +63,12 @@ struct mixer_resources {
>>  	struct clk		*mout_mixer;
>>  };
>>  
>> +struct layer_config {
>> +	unsigned int index;
>> +	unsigned int priority;
>> +	u32 cfg;
>> +};
> 
> I don't see why you are creating this struct, index and priority are
> never used in this patch series.
Good catch about 'priority'. But 'index' is used, see the second patch.



> 
>> +
>>  enum mixer_version_id {
>>  	MXR_VER_0_0_0_16,
>>  	MXR_VER_16_0_33_0,
>> @@ -75,6 +81,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_config *layer_config;
>> +	unsigned int num_layer;
>>  	int			pipe;
>>  	bool			interlace;
>>  	bool			powered;
>> @@ -95,6 +103,40 @@ struct mixer_drv_data {
>>  	bool					has_sclk;
>>  };
>>  
>> +/*
>> + * The default layer priorities. A higher priority means that
>> + * the layer is at the top of layer stack.
>> + * The current configuration 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
>> + * video processor is available.
>> + */
>> +
>> +static const struct layer_config default_layer_config[] = {
>> +	{
>> +		.index = 0, .priority = 1, /* layer0 */
>> +		.cfg = MXR_LAYER_CFG_GRP0_VAL(1)
>> +	}, {
>> +		.index = 1, .priority = 2, /* layer1 */
>> +		.cfg = MXR_LAYER_CFG_GRP1_VAL(2)
>> +	}
>> +};
>> +
>> +static const struct layer_config vp_layer_config[] = {
>> +	{
>> +		.index = 2, .priority = 1, /* video layer */
>> +		.cfg = MXR_LAYER_CFG_VP_VAL(1)
>> +	}, {
>> +		.index = 0, .priority = 2, /* layer0 */
>> +		.cfg = MXR_LAYER_CFG_GRP0_VAL(2)
>> +	}, {
>> +		.index = 1, .priority = 3, /* layer1 */
>> +		.cfg = MXR_LAYER_CFG_GRP1_VAL(3)
>> +	}
>> +};
>> +
>>  static const u8 filter_y_horiz_tap8[] = {
>>  	0,	-1,	-1,	-1,	-1,	-1,	-1,	-1,
>>  	-1,	-1,	-1,	-1,	-1,	0,	0,	0,
>> @@ -253,6 +295,17 @@ 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;
>> +
>> +	for (i = 0; i < ctx->num_layer; ++i)
>> +		val |= ctx->layer_config[i].cfg;
>> +
>> +	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;
>> @@ -655,17 +708,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);
> 
> I would move this exaclty piece of code into mixer_layer_priority().
Then we end up with the same static/hardcoded setup as before. That's
something I want to move away from. The entire information about layer
ordering should be stored in 'layer_config'.



>> +	mixer_layer_priority(ctx);
>>  
>>  	/* setting background color */
>>  	mixer_reg_write(res, MXR_BG_COLOR0, 0x008080);
>> @@ -1274,6 +1317,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_config = vp_layer_config;
>> +		ctx->num_layer = ARRAY_SIZE(vp_layer_config);
>> +	} else {
>> +		ctx->layer_config = default_layer_config;
>> +		ctx->num_layer = ARRAY_SIZE(default_layer_config);
>> +	}
> 
> Then this piece of code is useless.
No, since the second patch depends on it.



With best wishes,
Tobias


> 
> 	Gustavo
> --
> 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

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

[Gustavo Padovan]

2015-04-30 Tobias Jakobi <liquid.acid@gmx.net>:

> Hello Gustavo!
> 
> 
> Gustavo Padovan wrote:
> > Hi Tobias,
> > 
> > 2015-04-30 Tobias Jakobi <tjakobi@math.uni-bielefeld.de>:
> > 
> >> 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 | 74 +++++++++++++++++++++++++++++------
> >>  1 file changed, 63 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/exynos/exynos_mixer.c b/drivers/gpu/drm/exynos/exynos_mixer.c
> >> index 4155f43..a06b8e2 100644
> >> --- a/drivers/gpu/drm/exynos/exynos_mixer.c
> >> +++ b/drivers/gpu/drm/exynos/exynos_mixer.c
> >> @@ -63,6 +63,12 @@ struct mixer_resources {
> >>  	struct clk		*mout_mixer;
> >>  };
> >>  
> >> +struct layer_config {
> >> +	unsigned int index;
> >> +	unsigned int priority;
> >> +	u32 cfg;
> >> +};
> > 
> > I don't see why you are creating this struct, index and priority are
> > never used in this patch series.
> Good catch about 'priority'. But 'index' is used, see the second patch.
> 
> 
> 
> > 
> >> +
> >>  enum mixer_version_id {
> >>  	MXR_VER_0_0_0_16,
> >>  	MXR_VER_16_0_33_0,
> >> @@ -75,6 +81,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_config *layer_config;
> >> +	unsigned int num_layer;
> >>  	int			pipe;
> >>  	bool			interlace;
> >>  	bool			powered;
> >> @@ -95,6 +103,40 @@ struct mixer_drv_data {
> >>  	bool					has_sclk;
> >>  };
> >>  
> >> +/*
> >> + * The default layer priorities. A higher priority means that
> >> + * the layer is at the top of layer stack.
> >> + * The current configuration 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
> >> + * video processor is available.
> >> + */
> >> +
> >> +static const struct layer_config default_layer_config[] = {
> >> +	{
> >> +		.index = 0, .priority = 1, /* layer0 */
> >> +		.cfg = MXR_LAYER_CFG_GRP0_VAL(1)
> >> +	}, {
> >> +		.index = 1, .priority = 2, /* layer1 */
> >> +		.cfg = MXR_LAYER_CFG_GRP1_VAL(2)
> >> +	}
> >> +};
> >> +
> >> +static const struct layer_config vp_layer_config[] = {
> >> +	{
> >> +		.index = 2, .priority = 1, /* video layer */
> >> +		.cfg = MXR_LAYER_CFG_VP_VAL(1)
> >> +	}, {
> >> +		.index = 0, .priority = 2, /* layer0 */
> >> +		.cfg = MXR_LAYER_CFG_GRP0_VAL(2)
> >> +	}, {
> >> +		.index = 1, .priority = 3, /* layer1 */
> >> +		.cfg = MXR_LAYER_CFG_GRP1_VAL(3)
> >> +	}
> >> +};
> >> +
> >>  static const u8 filter_y_horiz_tap8[] = {
> >>  	0,	-1,	-1,	-1,	-1,	-1,	-1,	-1,
> >>  	-1,	-1,	-1,	-1,	-1,	0,	0,	0,
> >> @@ -253,6 +295,17 @@ 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;
> >> +
> >> +	for (i = 0; i < ctx->num_layer; ++i)
> >> +		val |= ctx->layer_config[i].cfg;
> >> +
> >> +	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;
> >> @@ -655,17 +708,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);
> > 
> > I would move this exaclty piece of code into mixer_layer_priority().
> Then we end up with the same static/hardcoded setup as before. That's
> something I want to move away from. The entire information about layer
> ordering should be stored in 'layer_config'.
> 
> 
> 
> >> +	mixer_layer_priority(ctx);
> >>  
> >>  	/* setting background color */
> >>  	mixer_reg_write(res, MXR_BG_COLOR0, 0x008080);
> >> @@ -1274,6 +1317,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_config = vp_layer_config;
> >> +		ctx->num_layer = ARRAY_SIZE(vp_layer_config);
> >> +	} else {
> >> +		ctx->layer_config = default_layer_config;
> >> +		ctx->num_layer = ARRAY_SIZE(default_layer_config);
> >> +	}
> > 
> > Then this piece of code is useless.
> No, since the second patch depends on it.

Right, I did a second look on patch 2 and realized that index is still
used and this code is actually needed here.

	Gustavo

Re: [RFC] drm/exynos: rework layer blending setup

[Inki Dae]

Hi Tobias,

To make patch files, you could use below command,
#git format-patch --cover-letter from..to

With this command, a cover file will be created and you could describe
what this patch series mean in the cover file.

Thanks,
Inki Dae

On 2015년 04월 30일 23:56, Tobias Jakobi wrote:
> Hello,
> 
> here's the rework of the layer blending setup that I discussed with Joonyoung in the past days. There's still 
> some TODOs in the code, but more or less it does what it's supposed to do. What still bothers me a bit is that I 
> currently call blending reconfig in mixer_cfg_layer(). It would be nice if this could be reduced to one call per 
> "frame" (so with the last win_{commit,disable} call). Maybe atomic provides such an infrastructure?
> 
> With best wishes,
> Tobias
> 
> 

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

Re: [RFC] drm/exynos: rework layer blending setup

[Tobias Jakobi]

Hello Inki!

Inki Dae wrote:
> Hi Tobias,
> 
> To make patch files, you could use below command,
> #git format-patch --cover-letter from..to
Thanks, I'm going to add the cover letter to the next revision.


> With this command, a cover file will be created and you could describe
> what this patch series mean in the cover file.
I left out a detailed description since the series isn't really finished
yet and I more or less wanted to hear Joonyoung' thoughts, who I
discussed stuff with.

I still have to change some details and remove the TODOs. Once that's
done I'm going to write a full description.


With best wishes,
Tobias

> 
> Thanks,
> Inki Dae
> 
> On 2015년 04월 30일 23:56, Tobias Jakobi wrote:
>> Hello,
>>
>> here's the rework of the layer blending setup that I discussed with Joonyoung in the past days. There's still 
>> some TODOs in the code, but more or less it does what it's supposed to do. What still bothers me a bit is that I 
>> currently call blending reconfig in mixer_cfg_layer(). It would be nice if this could be reduced to one call per 
>> "frame" (so with the last win_{commit,disable} call). Maybe atomic provides such an infrastructure?
>>
>> With best wishes,
>> Tobias
>>
>>
> 

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