[PATCH] drm/sun4i: Implement zpos for DE2

[PATCH] drm/sun4i: Implement zpos for DE2

[Jernej Skrabec]

Initial implementation of DE2 planes only supported fixed zpos.

Expand implementation with configurable zpos property.

Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
---
 drivers/gpu/drm/sun4i/sun8i_mixer.c    | 15 +++++++--
 drivers/gpu/drm/sun4i/sun8i_mixer.h    |  4 +++
 drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 45 ++++++++++++++++----------
 drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 45 ++++++++++++++++----------
 4 files changed, 72 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
index ee8febb25903..0747a9a69654 100644
--- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
@@ -260,6 +260,17 @@ const struct de2_fmt_info *sun8i_mixer_format_info(u32 format)
 	return NULL;
 }
 
+static void sun8i_mixer_atomic_begin(struct sunxi_engine *engine,
+				     struct drm_crtc_state *old_state)
+{
+	/*
+	 * Disable all pipes at the beginning. They will be enabled
+	 * again if needed in plane update callback.
+	 */
+	regmap_update_bits(engine->regs, SUN8I_MIXER_BLEND_PIPE_CTL,
+			   SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK, 0);
+}
+
 static void sun8i_mixer_commit(struct sunxi_engine *engine)
 {
 	DRM_DEBUG_DRIVER("Committing changes\n");
@@ -311,6 +322,7 @@ static struct drm_plane **sun8i_layers_init(struct drm_device *drm,
 }
 
 static const struct sunxi_engine_ops sun8i_engine_ops = {
+	.atomic_begin	= sun8i_mixer_atomic_begin,
 	.commit		= sun8i_mixer_commit,
 	.layers_init	= sun8i_layers_init,
 };
@@ -432,9 +444,6 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
 	regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_ATTR_FCOLOR(0),
 		     SUN8I_MIXER_BLEND_COLOR_BLACK);
 
-	/* Fixed zpos for now */
-	regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_ROUTE, 0x43210);
-
 	plane_cnt = mixer->cfg->vi_num + mixer->cfg->ui_num;
 	for (i = 0; i < plane_cnt; i++)
 		regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_MODE(i),
diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h b/drivers/gpu/drm/sun4i/sun8i_mixer.h
index f34e70c42adf..406c42e752d7 100644
--- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
+++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
@@ -44,6 +44,7 @@
 #define SUN8I_MIXER_BLEND_CK_MIN(x)		(0x10e0 + 0x04 * (x))
 #define SUN8I_MIXER_BLEND_OUTCTL		0x10fc
 
+#define SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK	GENMASK(12, 8)
 #define SUN8I_MIXER_BLEND_PIPE_CTL_EN(pipe)	BIT(8 + pipe)
 #define SUN8I_MIXER_BLEND_PIPE_CTL_FC_EN(pipe)	BIT(pipe)
 /* colors are always in AARRGGBB format */
@@ -51,6 +52,9 @@
 /* The following numbers are some still unknown magic numbers */
 #define SUN8I_MIXER_BLEND_MODE_DEF		0x03010301
 
+#define SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(n)	(0xf << ((n) << 2))
+#define SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(n)	((n) << 2)
+
 #define SUN8I_MIXER_BLEND_OUTCTL_INTERLACED	BIT(1)
 
 #define SUN8I_MIXER_FBFMT_ARGB8888	0
diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
index 9a540330cb79..518e1921f47e 100644
--- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
@@ -27,7 +27,7 @@
 #include "sun8i_ui_scaler.h"
 
 static void sun8i_ui_layer_enable(struct sun8i_mixer *mixer, int channel,
-				  int overlay, bool enable)
+				  int overlay, bool enable, unsigned int zpos)
 {
 	u32 val;
 
@@ -43,18 +43,24 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer *mixer, int channel,
 			   SUN8I_MIXER_CHAN_UI_LAYER_ATTR(channel, overlay),
 			   SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN, val);
 
-	if (enable)
-		val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(channel);
-	else
-		val = 0;
+	if (enable) {
+		val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);
 
-	regmap_update_bits(mixer->engine.regs,
-			   SUN8I_MIXER_BLEND_PIPE_CTL,
-			   SUN8I_MIXER_BLEND_PIPE_CTL_EN(channel), val);
+		regmap_update_bits(mixer->engine.regs,
+				   SUN8I_MIXER_BLEND_PIPE_CTL, val, val);
+
+		val = channel << SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
+
+		regmap_update_bits(mixer->engine.regs,
+				   SUN8I_MIXER_BLEND_ROUTE,
+				   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
+				   val);
+	}
 }
 
 static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int channel,
-				       int overlay, struct drm_plane *plane)
+				       int overlay, struct drm_plane *plane,
+				       unsigned int zpos)
 {
 	struct drm_plane_state *state = plane->state;
 	u32 src_w, src_h, dst_w, dst_h;
@@ -137,10 +143,10 @@ static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int channel,
 			 state->dst.x1, state->dst.y1);
 	DRM_DEBUG_DRIVER("Layer destination size W: %d H: %d\n", dst_w, dst_h);
 	regmap_write(mixer->engine.regs,
-		     SUN8I_MIXER_BLEND_ATTR_COORD(channel),
+		     SUN8I_MIXER_BLEND_ATTR_COORD(zpos),
 		     SUN8I_MIXER_COORD(state->dst.x1, state->dst.y1));
 	regmap_write(mixer->engine.regs,
-		     SUN8I_MIXER_BLEND_ATTR_INSIZE(channel),
+		     SUN8I_MIXER_BLEND_ATTR_INSIZE(zpos),
 		     outsize);
 
 	return 0;
@@ -238,28 +244,30 @@ static void sun8i_ui_layer_atomic_disable(struct drm_plane *plane,
 	struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane);
 	struct sun8i_mixer *mixer = layer->mixer;
 
-	sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay, false);
+	sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay, false, 0);
 }
 
 static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
 					 struct drm_plane_state *old_state)
 {
 	struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane);
+	unsigned int zpos = plane->state->normalized_zpos;
 	struct sun8i_mixer *mixer = layer->mixer;
 
 	if (!plane->state->visible) {
 		sun8i_ui_layer_enable(mixer, layer->channel,
-				      layer->overlay, false);
+				      layer->overlay, false, 0);
 		return;
 	}
 
 	sun8i_ui_layer_update_coord(mixer, layer->channel,
-				    layer->overlay, plane);
+				    layer->overlay, plane, zpos);
 	sun8i_ui_layer_update_formats(mixer, layer->channel,
 				      layer->overlay, plane);
 	sun8i_ui_layer_update_buffer(mixer, layer->channel,
 				     layer->overlay, plane);
-	sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay, true);
+	sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,
+			      true, zpos);
 }
 
 static struct drm_plane_helper_funcs sun8i_ui_layer_helper_funcs = {
@@ -307,6 +315,7 @@ struct sun8i_ui_layer *sun8i_ui_layer_init_one(struct drm_device *drm,
 	enum drm_plane_type type = DRM_PLANE_TYPE_OVERLAY;
 	int channel = mixer->cfg->vi_num + index;
 	struct sun8i_ui_layer *layer;
+	unsigned int plane_cnt;
 	int ret;
 
 	layer = devm_kzalloc(drm->dev, sizeof(*layer), GFP_KERNEL);
@@ -327,8 +336,10 @@ struct sun8i_ui_layer *sun8i_ui_layer_init_one(struct drm_device *drm,
 		return ERR_PTR(ret);
 	}
 
-	/* fixed zpos for now */
-	ret = drm_plane_create_zpos_immutable_property(&layer->plane, channel);
+	plane_cnt = mixer->cfg->ui_num + mixer->cfg->vi_num;
+
+	ret = drm_plane_create_zpos_property(&layer->plane, channel,
+					     0, plane_cnt - 1);
 	if (ret) {
 		dev_err(drm->dev, "Couldn't add zpos property\n");
 		return ERR_PTR(ret);
diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
index 5877f8ef5895..17e0d00cfd8a 100644
--- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
@@ -21,7 +21,7 @@
 #include "sun8i_vi_scaler.h"
 
 static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel,
-				  int overlay, bool enable)
+				  int overlay, bool enable, unsigned int zpos)
 {
 	u32 val;
 
@@ -37,18 +37,24 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel,
 			   SUN8I_MIXER_CHAN_VI_LAYER_ATTR(channel, overlay),
 			   SUN8I_MIXER_CHAN_VI_LAYER_ATTR_EN, val);
 
-	if (enable)
-		val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(channel);
-	else
-		val = 0;
+	if (enable) {
+		val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);
 
-	regmap_update_bits(mixer->engine.regs,
-			   SUN8I_MIXER_BLEND_PIPE_CTL,
-			   SUN8I_MIXER_BLEND_PIPE_CTL_EN(channel), val);
+		regmap_update_bits(mixer->engine.regs,
+				   SUN8I_MIXER_BLEND_PIPE_CTL, val, val);
+
+		val = channel << SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
+
+		regmap_update_bits(mixer->engine.regs,
+				   SUN8I_MIXER_BLEND_ROUTE,
+				   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
+				   val);
+	}
 }
 
 static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int channel,
-				       int overlay, struct drm_plane *plane)
+				       int overlay, struct drm_plane *plane,
+				       unsigned int zpos)
 {
 	struct drm_plane_state *state = plane->state;
 	const struct drm_format_info *format = state->fb->format;
@@ -130,10 +136,10 @@ static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int channel,
 			 state->dst.x1, state->dst.y1);
 	DRM_DEBUG_DRIVER("Layer destination size W: %d H: %d\n", dst_w, dst_h);
 	regmap_write(mixer->engine.regs,
-		     SUN8I_MIXER_BLEND_ATTR_COORD(channel),
+		     SUN8I_MIXER_BLEND_ATTR_COORD(zpos),
 		     SUN8I_MIXER_COORD(state->dst.x1, state->dst.y1));
 	regmap_write(mixer->engine.regs,
-		     SUN8I_MIXER_BLEND_ATTR_INSIZE(channel),
+		     SUN8I_MIXER_BLEND_ATTR_INSIZE(zpos),
 		     outsize);
 
 	return 0;
@@ -266,28 +272,30 @@ static void sun8i_vi_layer_atomic_disable(struct drm_plane *plane,
 	struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane);
 	struct sun8i_mixer *mixer = layer->mixer;
 
-	sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay, false);
+	sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay, false, 0);
 }
 
 static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
 					 struct drm_plane_state *old_state)
 {
 	struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane);
+	unsigned int zpos = plane->state->normalized_zpos;
 	struct sun8i_mixer *mixer = layer->mixer;
 
 	if (!plane->state->visible) {
 		sun8i_vi_layer_enable(mixer, layer->channel,
-				      layer->overlay, false);
+				      layer->overlay, false, 0);
 		return;
 	}
 
 	sun8i_vi_layer_update_coord(mixer, layer->channel,
-				    layer->overlay, plane);
+				    layer->overlay, plane, zpos);
 	sun8i_vi_layer_update_formats(mixer, layer->channel,
 				      layer->overlay, plane);
 	sun8i_vi_layer_update_buffer(mixer, layer->channel,
 				     layer->overlay, plane);
-	sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay, true);
+	sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,
+			      true, zpos);
 }
 
 static struct drm_plane_helper_funcs sun8i_vi_layer_helper_funcs = {
@@ -351,6 +359,7 @@ struct sun8i_vi_layer *sun8i_vi_layer_init_one(struct drm_device *drm,
 					       int index)
 {
 	struct sun8i_vi_layer *layer;
+	unsigned int plane_cnt;
 	int ret;
 
 	layer = devm_kzalloc(drm->dev, sizeof(*layer), GFP_KERNEL);
@@ -368,8 +377,10 @@ struct sun8i_vi_layer *sun8i_vi_layer_init_one(struct drm_device *drm,
 		return ERR_PTR(ret);
 	}
 
-	/* fixed zpos for now */
-	ret = drm_plane_create_zpos_immutable_property(&layer->plane, index);
+	plane_cnt = mixer->cfg->ui_num + mixer->cfg->vi_num;
+
+	ret = drm_plane_create_zpos_property(&layer->plane, index,
+					     0, plane_cnt - 1);
 	if (ret) {
 		dev_err(drm->dev, "Couldn't add zpos property\n");
 		return ERR_PTR(ret);
-- 
2.18.0

Re: [PATCH] drm/sun4i: Implement zpos for DE2

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 801 bytes --]

Hi!

On Wed, Jun 27, 2018 at 06:45:14PM +0200, Jernej Skrabec wrote:
> Initial implementation of DE2 planes only supported fixed zpos.
> 
> Expand implementation with configurable zpos property.
> 
> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>

Thanks for that work. I guess you should expand a bit on the exact
setup you're doing here.

Are the pipes working the same way on the DE2 than on DE1, ie does the
pipe blending applies before the alpha blending, and therefore you
need to make sure that there's not two planes with alpha going to the
same pipe?

Also, you seem to use the pipe and channels indifferently now, why is
that?

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] drm/sun4i: Implement zpos for DE2

[Jernej Škrabec]

Dne sreda, 27. junij 2018 ob 20:25:00 CEST je Maxime Ripard napisal(a):
> Hi!
> 
> On Wed, Jun 27, 2018 at 06:45:14PM +0200, Jernej Skrabec wrote:
> > Initial implementation of DE2 planes only supported fixed zpos.
> > 
> > Expand implementation with configurable zpos property.
> > 
> > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> 
> Thanks for that work. I guess you should expand a bit on the exact
> setup you're doing here.

OK.

> 
> Are the pipes working the same way on the DE2 than on DE1, ie does the
> pipe blending applies before the alpha blending, and therefore you
> need to make sure that there's not two planes with alpha going to the
> same pipe?

I'm not familiar with DE1 and I'm not sure what the problem is.

However, there is an issue in DE2 when alpha blending multiple planes if 
bottom-most plane doesn't cover all screen. In this case alpha blending 
produce weird result on screen. Fortunately, there is elegant solution. Black 
opaque fill color is enabled for pipe 0 (always at the bottom), which covers 
any "undefined region" and that makes alpha blending happy again.

Alternatively, blending modes between planes could be tweaked or disabled, but 
I found aforementioned solution is much simpler and you set it only once.

> 
> Also, you seem to use the pipe and channels indifferently now, why is
> that?

Why do you think so?

Channel always represents HW unit, for example, on H3, mixer0, channel 0 
always represents VI plane, channel 1, represents first UI, plane, channel 2, 
second UI plane, etc.

Pipe 0 always represent channel at the bottom, pipe 1 channel on top pipe 0, 
etc. Initial, fixed zpos implementation really had 1:1 mapping, but now it can 
be different.

Register SUN8I_MIXER_BLEND_ROUTE holds pipe <-> channel mappings.
Bits 3:0 represents pipe 0 and holds channel number. Bits 7:4 represents pipe 
1, etc.

Additionaly,  there can be holes, for example, pipe 3 and pipe 0 are enabled 
and pipe 1 and 2 are disabled.

I hope I answered your questions.

Best regards,
Jernej

Re: [linux-sunxi] Re: [PATCH] drm/sun4i: Implement zpos for DE2

[Jernej Škrabec]

Dne sreda, 27. junij 2018 ob 22:58:28 CEST je Jernej Škrabec napisal(a):
> Dne sreda, 27. junij 2018 ob 20:25:00 CEST je Maxime Ripard napisal(a):
> > Hi!
> > 
> > On Wed, Jun 27, 2018 at 06:45:14PM +0200, Jernej Skrabec wrote:
> > > Initial implementation of DE2 planes only supported fixed zpos.
> > > 
> > > Expand implementation with configurable zpos property.
> > > 
> > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > 
> > Thanks for that work. I guess you should expand a bit on the exact
> > setup you're doing here.
> 
> OK.
> 
> > Are the pipes working the same way on the DE2 than on DE1, ie does the
> > pipe blending applies before the alpha blending, and therefore you
> > need to make sure that there's not two planes with alpha going to the
> > same pipe?
> 
> I'm not familiar with DE1 and I'm not sure what the problem is.
> 
> However, there is an issue in DE2 when alpha blending multiple planes if
> bottom-most plane doesn't cover all screen. In this case alpha blending
> produce weird result on screen. Fortunately, there is elegant solution.
> Black opaque fill color is enabled for pipe 0 (always at the bottom), which
> covers any "undefined region" and that makes alpha blending happy again.
> 
> Alternatively, blending modes between planes could be tweaked or disabled,
> but I found aforementioned solution is much simpler and you set it only
> once.

I forgot to test one corner case - what happens if pipe 0 has alpha channel 
enabled. I think it still has to work, since there is background color which 
IMO is meant for such cases.

> > Also, you seem to use the pipe and channels indifferently now, why is
> > that?
> 
> Why do you think so?
> 
> Channel always represents HW unit, for example, on H3, mixer0, channel 0
> always represents VI plane, channel 1, represents first UI, plane, channel
> 2, second UI plane, etc.
> 
> Pipe 0 always represent channel at the bottom, pipe 1 channel on top pipe 0,
> etc. Initial, fixed zpos implementation really had 1:1 mapping, but now it
> can be different.
> 
> Register SUN8I_MIXER_BLEND_ROUTE holds pipe <-> channel mappings.
> Bits 3:0 represents pipe 0 and holds channel number. Bits 7:4 represents
> pipe 1, etc.
> 
> Additionaly,  there can be holes, for example, pipe 3 and pipe 0 are enabled
> and pipe 1 and 2 are disabled.
> 
> I hope I answered your questions.

Looking at DE2.0 documentation, chapter 5.10.2 (pp. 87) might give you some 
answers. Block diagram seems much simpler than that of DE1.

Best regards,
Jernej

Re: [PATCH] drm/sun4i: Implement zpos for DE2

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3691 bytes --]

On Wed, Jun 27, 2018 at 10:58:28PM +0200, Jernej Škrabec wrote:
> Dne sreda, 27. junij 2018 ob 20:25:00 CEST je Maxime Ripard napisal(a):
> > Hi!
> > 
> > On Wed, Jun 27, 2018 at 06:45:14PM +0200, Jernej Skrabec wrote:
> > > Initial implementation of DE2 planes only supported fixed zpos.
> > > 
> > > Expand implementation with configurable zpos property.
> > > 
> > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > 
> > Thanks for that work. I guess you should expand a bit on the exact
> > setup you're doing here.
> 
> OK.
> 
> > 
> > Are the pipes working the same way on the DE2 than on DE1, ie does the
> > pipe blending applies before the alpha blending, and therefore you
> > need to make sure that there's not two planes with alpha going to the
> > same pipe?
> 
> I'm not familiar with DE1 and I'm not sure what the problem is.

The alpha blending is happening after the pipe blending. So you
basically have a two-stage blending, the first one between the planes
assigned to a pipe, only taking the plane priority into account, and
using the highest priority plane's pixel in overlapping area. And
then, you have alpha blending between the two pipes.

But that means that if you have two planes with alpha assigned to the
same pipe, it's not going to work since the value and alpha of the
lowest priority plane is going to be dropped in favor of the highest
priority, instead of having transparency.

> However, there is an issue in DE2 when alpha blending multiple planes if 
> bottom-most plane doesn't cover all screen. In this case alpha blending 
> produce weird result on screen. Fortunately, there is elegant solution. Black 
> opaque fill color is enabled for pipe 0 (always at the bottom), which covers 
> any "undefined region" and that makes alpha blending happy again.
> 
> Alternatively, blending modes between planes could be tweaked or
> disabled, but I found aforementioned solution is much simpler and
> you set it only once.

Yeah, we had a similar behaviour as well, if the lowest plane has a
some alpha (!= 0xff), the pixel value is completely dropped. We worked
around this by preventing any plane with alpha at the lowest position,
but it might be a good idea to check if the background color set to
black fixes it. I remember that we were indeed seeing the background
color, but I don't think I tried setting it to black and seeing what
happens.

> > Also, you seem to use the pipe and channels indifferently now, why is
> > that?
> 
> Why do you think so?

Your driver used to use the channel id, and is now replaced by the
zpos assigned (for example in SUN8I_MIXER_BLEND_PIPE_CTL_EN)

> Channel always represents HW unit, for example, on H3, mixer0, channel 0 
> always represents VI plane, channel 1, represents first UI, plane, channel 2, 
> second UI plane, etc.
> 
> Pipe 0 always represent channel at the bottom, pipe 1 channel on top pipe 0, 
> etc. Initial, fixed zpos implementation really had 1:1 mapping, but now it can 
> be different.
> 
> Register SUN8I_MIXER_BLEND_ROUTE holds pipe <-> channel mappings.
> Bits 3:0 represents pipe 0 and holds channel number. Bits 7:4 represents pipe 
> 1, etc.
> 
> Additionaly,  there can be holes, for example, pipe 3 and pipe 0 are enabled 
> and pipe 1 and 2 are disabled.

IIRC, you have 4 layers per channel, and then a combination of VI and
UI channels to form the DE2. Where is the pipe located exactly,
between the layers and the channel output, or between the channel
output?

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] drm/sun4i: Implement zpos for DE2

[Jernej Škrabec]

Dne petek, 29. junij 2018 ob 09:17:46 CEST je Maxime Ripard napisal(a):
> On Wed, Jun 27, 2018 at 10:58:28PM +0200, Jernej Škrabec wrote:
> > Dne sreda, 27. junij 2018 ob 20:25:00 CEST je Maxime Ripard napisal(a):
> > > Hi!
> > > 
> > > On Wed, Jun 27, 2018 at 06:45:14PM +0200, Jernej Skrabec wrote:
> > > > Initial implementation of DE2 planes only supported fixed zpos.
> > > > 
> > > > Expand implementation with configurable zpos property.
> > > > 
> > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > 
> > > Thanks for that work. I guess you should expand a bit on the exact
> > > setup you're doing here.
> > 
> > OK.
> > 
> > > Are the pipes working the same way on the DE2 than on DE1, ie does the
> > > pipe blending applies before the alpha blending, and therefore you
> > > need to make sure that there's not two planes with alpha going to the
> > > same pipe?
> > 
> > I'm not familiar with DE1 and I'm not sure what the problem is.
> 
> The alpha blending is happening after the pipe blending. So you
> basically have a two-stage blending, the first one between the planes
> assigned to a pipe, only taking the plane priority into account, and
> using the highest priority plane's pixel in overlapping area. And
> then, you have alpha blending between the two pipes.
> 
> But that means that if you have two planes with alpha assigned to the
> same pipe, it's not going to work since the value and alpha of the
> lowest priority plane is going to be dropped in favor of the highest
> priority, instead of having transparency.

This sounds familiar. Each channel contains 4 overlays. Those overlays have 
fixed order, cannot be scaled and only blending supported is premultiply. This 
is the first step HW does. I guess this is the thing similar to DE1 plane 
blending.

After that, HW scaling is done on channel level (if it is enabled). Then 
channels are mapped (reordered) to pipes according to route register and at 
the end, alpha blending is done between pipes.

As you can see, overlays don't fit in DRM concept. They have relative position 
to channel zpos setting and scalling can't be done on them, with only 
premultipy supported. Because of those limitations, only one overlay is used 
in one channel. With this restriction, everything else falls pretty nicely 
into DRM concept.

> 
> > However, there is an issue in DE2 when alpha blending multiple planes if
> > bottom-most plane doesn't cover all screen. In this case alpha blending
> > produce weird result on screen. Fortunately, there is elegant solution.
> > Black opaque fill color is enabled for pipe 0 (always at the bottom),
> > which covers any "undefined region" and that makes alpha blending happy
> > again.
> > 
> > Alternatively, blending modes between planes could be tweaked or
> > disabled, but I found aforementioned solution is much simpler and
> > you set it only once.
> 
> Yeah, we had a similar behaviour as well, if the lowest plane has a
> some alpha (!= 0xff), the pixel value is completely dropped. We worked
> around this by preventing any plane with alpha at the lowest position,
> but it might be a good idea to check if the background color set to
> black fixes it. I remember that we were indeed seeing the background
> color, but I don't think I tried setting it to black and seeing what
> happens.
> 

I tested both corner cases I could think of and all seems to be fine. These 
are:
1. Having bottom-most plane only partialy covered. This caused issues with 
alpha blending. Solution is to set opaque black fill color to bottom-most 
pipe. In this case, previously undefined region doesn't have undefined pixel 
data and blending is correct.
2. Bottom-most plane has alpha values <0xff. This doesn't cause any issue at 
all. I suspect that the reason for that is background color set to black.

> > > Also, you seem to use the pipe and channels indifferently now, why is
> > > that?
> > 
> > Why do you think so?
> 
> Your driver used to use the channel id, and is now replaced by the
> zpos assigned (for example in SUN8I_MIXER_BLEND_PIPE_CTL_EN)

zpos represents pipe number, so that is correct thing to do.

I think I know what bothers you. Patch shows only part of the changed 
functions. Please take a look at final functions. sun8i_vi_layer_enable() and 
sun8i_vi_layer_update_coord() still work (mostly) based on channel id.

For example, sun8i_vi_layer_update_coord() function sets almost all of the 
registers based on channel id. Only output size after scaling is set based on 
pipe (zpos) id.

More precisely, zpos has to be used for reading/writing pipe settings in 
global mixer registers (prefixed with SUN8I_MIXER_BLEND_). Channel id has to 
be used when reading/writing channel registers (prefixed with 
SUN8I_MIXER_CHAN_UI_ or SUN8I_MIXER_CHAN_VI_).

Before the patch, channel id was actually the same as zpos id and because of 
that channel id was used for pipes too.

> 
> > Channel always represents HW unit, for example, on H3, mixer0, channel 0
> > always represents VI plane, channel 1, represents first UI, plane, channel
> > 2, second UI plane, etc.
> > 
> > Pipe 0 always represent channel at the bottom, pipe 1 channel on top pipe
> > 0, etc. Initial, fixed zpos implementation really had 1:1 mapping, but
> > now it can be different.
> > 
> > Register SUN8I_MIXER_BLEND_ROUTE holds pipe <-> channel mappings.
> > Bits 3:0 represents pipe 0 and holds channel number. Bits 7:4 represents
> > pipe 1, etc.
> > 
> > Additionaly,  there can be holes, for example, pipe 3 and pipe 0 are
> > enabled and pipe 1 and 2 are disabled.
> 
> IIRC, you have 4 layers per channel, and then a combination of VI and
> UI channels to form the DE2. Where is the pipe located exactly,
> between the layers and the channel output, or between the channel
> output?

I hope I answered that in first answer.

Best regards,
Jernej

Re: [PATCH] drm/sun4i: Implement zpos for DE2

[Paul Kocialkowski]

[-- Attachment #1: Type: text/plain, Size: 12556 bytes --]

On Wed, 2018-06-27 at 18:45 +0200, Jernej Skrabec wrote:
> Initial implementation of DE2 planes only supported fixed zpos.
> 
> Expand implementation with configurable zpos property.

Thanks for adding support for this! Tested with the Sunxi-Cedrus VPU
driver on H3 with:
https://github.com/free-electrons/cedrus-frame-test/commit/9f46aa83cae1b9fa4d4901d3d2999c8d41265532

Tested-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

> Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> ---
>  drivers/gpu/drm/sun4i/sun8i_mixer.c    | 15 +++++++--
>  drivers/gpu/drm/sun4i/sun8i_mixer.h    |  4 +++
>  drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 45 ++++++++++++++++----------
>  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 45 ++++++++++++++++----------
>  4 files changed, 72 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.c b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> index ee8febb25903..0747a9a69654 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.c
> @@ -260,6 +260,17 @@ const struct de2_fmt_info *sun8i_mixer_format_info(u32 format)
>  	return NULL;
>  }
>  
> +static void sun8i_mixer_atomic_begin(struct sunxi_engine *engine,
> +				     struct drm_crtc_state *old_state)
> +{
> +	/*
> +	 * Disable all pipes at the beginning. They will be enabled
> +	 * again if needed in plane update callback.
> +	 */
> +	regmap_update_bits(engine->regs, SUN8I_MIXER_BLEND_PIPE_CTL,
> +			   SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK, 0);
> +}
> +
>  static void sun8i_mixer_commit(struct sunxi_engine *engine)
>  {
>  	DRM_DEBUG_DRIVER("Committing changes\n");
> @@ -311,6 +322,7 @@ static struct drm_plane **sun8i_layers_init(struct drm_device *drm,
>  }
>  
>  static const struct sunxi_engine_ops sun8i_engine_ops = {
> +	.atomic_begin	= sun8i_mixer_atomic_begin,
>  	.commit		= sun8i_mixer_commit,
>  	.layers_init	= sun8i_layers_init,
>  };
> @@ -432,9 +444,6 @@ static int sun8i_mixer_bind(struct device *dev, struct device *master,
>  	regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_ATTR_FCOLOR(0),
>  		     SUN8I_MIXER_BLEND_COLOR_BLACK);
>  
> -	/* Fixed zpos for now */
> -	regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_ROUTE, 0x43210);
> -
>  	plane_cnt = mixer->cfg->vi_num + mixer->cfg->ui_num;
>  	for (i = 0; i < plane_cnt; i++)
>  		regmap_write(mixer->engine.regs, SUN8I_MIXER_BLEND_MODE(i),
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> index f34e70c42adf..406c42e752d7 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> @@ -44,6 +44,7 @@
>  #define SUN8I_MIXER_BLEND_CK_MIN(x)		(0x10e0 + 0x04 * (x))
>  #define SUN8I_MIXER_BLEND_OUTCTL		0x10fc
>  
> +#define SUN8I_MIXER_BLEND_PIPE_CTL_EN_MSK	GENMASK(12, 8)
>  #define SUN8I_MIXER_BLEND_PIPE_CTL_EN(pipe)	BIT(8 + pipe)
>  #define SUN8I_MIXER_BLEND_PIPE_CTL_FC_EN(pipe)	BIT(pipe)
>  /* colors are always in AARRGGBB format */
> @@ -51,6 +52,9 @@
>  /* The following numbers are some still unknown magic numbers */
>  #define SUN8I_MIXER_BLEND_MODE_DEF		0x03010301
>  
> +#define SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(n)	(0xf << ((n) << 2))
> +#define SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(n)	((n) << 2)
> +
>  #define SUN8I_MIXER_BLEND_OUTCTL_INTERLACED	BIT(1)
>  
>  #define SUN8I_MIXER_FBFMT_ARGB8888	0
> diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> index 9a540330cb79..518e1921f47e 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> @@ -27,7 +27,7 @@
>  #include "sun8i_ui_scaler.h"
>  
>  static void sun8i_ui_layer_enable(struct sun8i_mixer *mixer, int channel,
> -				  int overlay, bool enable)
> +				  int overlay, bool enable, unsigned int zpos)
>  {
>  	u32 val;
>  
> @@ -43,18 +43,24 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer *mixer, int channel,
>  			   SUN8I_MIXER_CHAN_UI_LAYER_ATTR(channel, overlay),
>  			   SUN8I_MIXER_CHAN_UI_LAYER_ATTR_EN, val);
>  
> -	if (enable)
> -		val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(channel);
> -	else
> -		val = 0;
> +	if (enable) {
> +		val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);
>  
> -	regmap_update_bits(mixer->engine.regs,
> -			   SUN8I_MIXER_BLEND_PIPE_CTL,
> -			   SUN8I_MIXER_BLEND_PIPE_CTL_EN(channel), val);
> +		regmap_update_bits(mixer->engine.regs,
> +				   SUN8I_MIXER_BLEND_PIPE_CTL, val, val);
> +
> +		val = channel << SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
> +
> +		regmap_update_bits(mixer->engine.regs,
> +				   SUN8I_MIXER_BLEND_ROUTE,
> +				   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
> +				   val);
> +	}
>  }
>  
>  static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int channel,
> -				       int overlay, struct drm_plane *plane)
> +				       int overlay, struct drm_plane *plane,
> +				       unsigned int zpos)
>  {
>  	struct drm_plane_state *state = plane->state;
>  	u32 src_w, src_h, dst_w, dst_h;
> @@ -137,10 +143,10 @@ static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int channel,
>  			 state->dst.x1, state->dst.y1);
>  	DRM_DEBUG_DRIVER("Layer destination size W: %d H: %d\n", dst_w, dst_h);
>  	regmap_write(mixer->engine.regs,
> -		     SUN8I_MIXER_BLEND_ATTR_COORD(channel),
> +		     SUN8I_MIXER_BLEND_ATTR_COORD(zpos),
>  		     SUN8I_MIXER_COORD(state->dst.x1, state->dst.y1));
>  	regmap_write(mixer->engine.regs,
> -		     SUN8I_MIXER_BLEND_ATTR_INSIZE(channel),
> +		     SUN8I_MIXER_BLEND_ATTR_INSIZE(zpos),
>  		     outsize);
>  
>  	return 0;
> @@ -238,28 +244,30 @@ static void sun8i_ui_layer_atomic_disable(struct drm_plane *plane,
>  	struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane);
>  	struct sun8i_mixer *mixer = layer->mixer;
>  
> -	sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay, false);
> +	sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay, false, 0);
>  }
>  
>  static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
>  					 struct drm_plane_state *old_state)
>  {
>  	struct sun8i_ui_layer *layer = plane_to_sun8i_ui_layer(plane);
> +	unsigned int zpos = plane->state->normalized_zpos;
>  	struct sun8i_mixer *mixer = layer->mixer;
>  
>  	if (!plane->state->visible) {
>  		sun8i_ui_layer_enable(mixer, layer->channel,
> -				      layer->overlay, false);
> +				      layer->overlay, false, 0);
>  		return;
>  	}
>  
>  	sun8i_ui_layer_update_coord(mixer, layer->channel,
> -				    layer->overlay, plane);
> +				    layer->overlay, plane, zpos);
>  	sun8i_ui_layer_update_formats(mixer, layer->channel,
>  				      layer->overlay, plane);
>  	sun8i_ui_layer_update_buffer(mixer, layer->channel,
>  				     layer->overlay, plane);
> -	sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay, true);
> +	sun8i_ui_layer_enable(mixer, layer->channel, layer->overlay,
> +			      true, zpos);
>  }
>  
>  static struct drm_plane_helper_funcs sun8i_ui_layer_helper_funcs = {
> @@ -307,6 +315,7 @@ struct sun8i_ui_layer *sun8i_ui_layer_init_one(struct drm_device *drm,
>  	enum drm_plane_type type = DRM_PLANE_TYPE_OVERLAY;
>  	int channel = mixer->cfg->vi_num + index;
>  	struct sun8i_ui_layer *layer;
> +	unsigned int plane_cnt;
>  	int ret;
>  
>  	layer = devm_kzalloc(drm->dev, sizeof(*layer), GFP_KERNEL);
> @@ -327,8 +336,10 @@ struct sun8i_ui_layer *sun8i_ui_layer_init_one(struct drm_device *drm,
>  		return ERR_PTR(ret);
>  	}
>  
> -	/* fixed zpos for now */
> -	ret = drm_plane_create_zpos_immutable_property(&layer->plane, channel);
> +	plane_cnt = mixer->cfg->ui_num + mixer->cfg->vi_num;
> +
> +	ret = drm_plane_create_zpos_property(&layer->plane, channel,
> +					     0, plane_cnt - 1);
>  	if (ret) {
>  		dev_err(drm->dev, "Couldn't add zpos property\n");
>  		return ERR_PTR(ret);
> diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> index 5877f8ef5895..17e0d00cfd8a 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> @@ -21,7 +21,7 @@
>  #include "sun8i_vi_scaler.h"
>  
>  static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel,
> -				  int overlay, bool enable)
> +				  int overlay, bool enable, unsigned int zpos)
>  {
>  	u32 val;
>  
> @@ -37,18 +37,24 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel,
>  			   SUN8I_MIXER_CHAN_VI_LAYER_ATTR(channel, overlay),
>  			   SUN8I_MIXER_CHAN_VI_LAYER_ATTR_EN, val);
>  
> -	if (enable)
> -		val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(channel);
> -	else
> -		val = 0;
> +	if (enable) {
> +		val = SUN8I_MIXER_BLEND_PIPE_CTL_EN(zpos);
>  
> -	regmap_update_bits(mixer->engine.regs,
> -			   SUN8I_MIXER_BLEND_PIPE_CTL,
> -			   SUN8I_MIXER_BLEND_PIPE_CTL_EN(channel), val);
> +		regmap_update_bits(mixer->engine.regs,
> +				   SUN8I_MIXER_BLEND_PIPE_CTL, val, val);
> +
> +		val = channel << SUN8I_MIXER_BLEND_ROUTE_PIPE_SHIFT(zpos);
> +
> +		regmap_update_bits(mixer->engine.regs,
> +				   SUN8I_MIXER_BLEND_ROUTE,
> +				   SUN8I_MIXER_BLEND_ROUTE_PIPE_MSK(zpos),
> +				   val);
> +	}
>  }
>  
>  static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int channel,
> -				       int overlay, struct drm_plane *plane)
> +				       int overlay, struct drm_plane *plane,
> +				       unsigned int zpos)
>  {
>  	struct drm_plane_state *state = plane->state;
>  	const struct drm_format_info *format = state->fb->format;
> @@ -130,10 +136,10 @@ static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int channel,
>  			 state->dst.x1, state->dst.y1);
>  	DRM_DEBUG_DRIVER("Layer destination size W: %d H: %d\n", dst_w, dst_h);
>  	regmap_write(mixer->engine.regs,
> -		     SUN8I_MIXER_BLEND_ATTR_COORD(channel),
> +		     SUN8I_MIXER_BLEND_ATTR_COORD(zpos),
>  		     SUN8I_MIXER_COORD(state->dst.x1, state->dst.y1));
>  	regmap_write(mixer->engine.regs,
> -		     SUN8I_MIXER_BLEND_ATTR_INSIZE(channel),
> +		     SUN8I_MIXER_BLEND_ATTR_INSIZE(zpos),
>  		     outsize);
>  
>  	return 0;
> @@ -266,28 +272,30 @@ static void sun8i_vi_layer_atomic_disable(struct drm_plane *plane,
>  	struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane);
>  	struct sun8i_mixer *mixer = layer->mixer;
>  
> -	sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay, false);
> +	sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay, false, 0);
>  }
>  
>  static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
>  					 struct drm_plane_state *old_state)
>  {
>  	struct sun8i_vi_layer *layer = plane_to_sun8i_vi_layer(plane);
> +	unsigned int zpos = plane->state->normalized_zpos;
>  	struct sun8i_mixer *mixer = layer->mixer;
>  
>  	if (!plane->state->visible) {
>  		sun8i_vi_layer_enable(mixer, layer->channel,
> -				      layer->overlay, false);
> +				      layer->overlay, false, 0);
>  		return;
>  	}
>  
>  	sun8i_vi_layer_update_coord(mixer, layer->channel,
> -				    layer->overlay, plane);
> +				    layer->overlay, plane, zpos);
>  	sun8i_vi_layer_update_formats(mixer, layer->channel,
>  				      layer->overlay, plane);
>  	sun8i_vi_layer_update_buffer(mixer, layer->channel,
>  				     layer->overlay, plane);
> -	sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay, true);
> +	sun8i_vi_layer_enable(mixer, layer->channel, layer->overlay,
> +			      true, zpos);
>  }
>  
>  static struct drm_plane_helper_funcs sun8i_vi_layer_helper_funcs = {
> @@ -351,6 +359,7 @@ struct sun8i_vi_layer *sun8i_vi_layer_init_one(struct drm_device *drm,
>  					       int index)
>  {
>  	struct sun8i_vi_layer *layer;
> +	unsigned int plane_cnt;
>  	int ret;
>  
>  	layer = devm_kzalloc(drm->dev, sizeof(*layer), GFP_KERNEL);
> @@ -368,8 +377,10 @@ struct sun8i_vi_layer *sun8i_vi_layer_init_one(struct drm_device *drm,
>  		return ERR_PTR(ret);
>  	}
>  
> -	/* fixed zpos for now */
> -	ret = drm_plane_create_zpos_immutable_property(&layer->plane, index);
> +	plane_cnt = mixer->cfg->ui_num + mixer->cfg->vi_num;
> +
> +	ret = drm_plane_create_zpos_property(&layer->plane, index,
> +					     0, plane_cnt - 1);
>  	if (ret) {
>  		dev_err(drm->dev, "Couldn't add zpos property\n");
>  		return ERR_PTR(ret);
-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] drm/sun4i: Implement zpos for DE2

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 5731 bytes --]

On Fri, Jun 29, 2018 at 08:59:03PM +0200, Jernej Škrabec wrote:
> Dne petek, 29. junij 2018 ob 09:17:46 CEST je Maxime Ripard napisal(a):
> > On Wed, Jun 27, 2018 at 10:58:28PM +0200, Jernej Škrabec wrote:
> > > Dne sreda, 27. junij 2018 ob 20:25:00 CEST je Maxime Ripard napisal(a):
> > > > Hi!
> > > > 
> > > > On Wed, Jun 27, 2018 at 06:45:14PM +0200, Jernej Skrabec wrote:
> > > > > Initial implementation of DE2 planes only supported fixed zpos.
> > > > > 
> > > > > Expand implementation with configurable zpos property.
> > > > > 
> > > > > Signed-off-by: Jernej Skrabec <jernej.skrabec@siol.net>
> > > > 
> > > > Thanks for that work. I guess you should expand a bit on the exact
> > > > setup you're doing here.
> > > 
> > > OK.
> > > 
> > > > Are the pipes working the same way on the DE2 than on DE1, ie does the
> > > > pipe blending applies before the alpha blending, and therefore you
> > > > need to make sure that there's not two planes with alpha going to the
> > > > same pipe?
> > > 
> > > I'm not familiar with DE1 and I'm not sure what the problem is.
> > 
> > The alpha blending is happening after the pipe blending. So you
> > basically have a two-stage blending, the first one between the planes
> > assigned to a pipe, only taking the plane priority into account, and
> > using the highest priority plane's pixel in overlapping area. And
> > then, you have alpha blending between the two pipes.
> > 
> > But that means that if you have two planes with alpha assigned to the
> > same pipe, it's not going to work since the value and alpha of the
> > lowest priority plane is going to be dropped in favor of the highest
> > priority, instead of having transparency.
> 
> This sounds familiar. Each channel contains 4 overlays. Those overlays have 
> fixed order, cannot be scaled and only blending supported is premultiply. This 
> is the first step HW does. I guess this is the thing similar to DE1 plane 
> blending.
> 
> After that, HW scaling is done on channel level (if it is enabled). Then 
> channels are mapped (reordered) to pipes according to route register and at 
> the end, alpha blending is done between pipes.
> 
> As you can see, overlays don't fit in DRM concept. They have relative position 
> to channel zpos setting and scalling can't be done on them, with only 
> premultipy supported. Because of those limitations, only one overlay is used 
> in one channel. With this restriction, everything else falls pretty nicely 
> into DRM concept.

I guess you could expose them as planes, but you'd need to improve the
current atomic_check to make sure that all these constraints are
met. That's definitely a topic for another patch serie though.

> > > However, there is an issue in DE2 when alpha blending multiple planes if
> > > bottom-most plane doesn't cover all screen. In this case alpha blending
> > > produce weird result on screen. Fortunately, there is elegant solution.
> > > Black opaque fill color is enabled for pipe 0 (always at the bottom),
> > > which covers any "undefined region" and that makes alpha blending happy
> > > again.
> > > 
> > > Alternatively, blending modes between planes could be tweaked or
> > > disabled, but I found aforementioned solution is much simpler and
> > > you set it only once.
> > 
> > Yeah, we had a similar behaviour as well, if the lowest plane has a
> > some alpha (!= 0xff), the pixel value is completely dropped. We worked
> > around this by preventing any plane with alpha at the lowest position,
> > but it might be a good idea to check if the background color set to
> > black fixes it. I remember that we were indeed seeing the background
> > color, but I don't think I tried setting it to black and seeing what
> > happens.
> > 
> 
> I tested both corner cases I could think of and all seems to be fine. These 
> are:
> 1. Having bottom-most plane only partialy covered. This caused issues with 
> alpha blending. Solution is to set opaque black fill color to bottom-most 
> pipe. In this case, previously undefined region doesn't have undefined pixel 
> data and blending is correct.
> 2. Bottom-most plane has alpha values <0xff. This doesn't cause any issue at 
> all. I suspect that the reason for that is background color set to black.

Ok, that's good.

> > > > Also, you seem to use the pipe and channels indifferently now, why is
> > > > that?
> > > 
> > > Why do you think so?
> > 
> > Your driver used to use the channel id, and is now replaced by the
> > zpos assigned (for example in SUN8I_MIXER_BLEND_PIPE_CTL_EN)
> 
> zpos represents pipe number, so that is correct thing to do.
> 
> I think I know what bothers you. Patch shows only part of the changed 
> functions. Please take a look at final functions. sun8i_vi_layer_enable() and 
> sun8i_vi_layer_update_coord() still work (mostly) based on channel id.
> 
> For example, sun8i_vi_layer_update_coord() function sets almost all of the 
> registers based on channel id. Only output size after scaling is set based on 
> pipe (zpos) id.
> 
> More precisely, zpos has to be used for reading/writing pipe settings in 
> global mixer registers (prefixed with SUN8I_MIXER_BLEND_). Channel id has to 
> be used when reading/writing channel registers (prefixed with 
> SUN8I_MIXER_CHAN_UI_ or SUN8I_MIXER_CHAN_VI_).
> 
> Before the patch, channel id was actually the same as zpos id and because of 
> that channel id was used for pipes too.

Ok. It's the kind of explanation that definitely belongs in the commit log :)

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]