All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property
@ 2022-06-05 15:47 ` Roman Stratiienko
  0 siblings, 0 replies; 19+ messages in thread
From: Roman Stratiienko @ 2022-06-05 15:47 UTC (permalink / raw)
  To: mripard, wens, jernej.skrabec, airlied, daniel, samuel,
	dri-devel, linux-arm-kernel, linux-sunxi, linux-kernel, megi
  Cc: Roman Stratiienko

Allwinner DE2 and DE3 hardware support 3 pixel blend modes:
"None", "Pre-multiplied", "Coverage"

Add the blend mode property and route it to the appropriate registers.

Note:
"force_premulti" parameter was added to handle multi-overlay channel
cases in future changes. It must be set to true for cases when more
than 1 overlay layer is used within a channel and at least one of the
overlay layers within a group uses premultiplied blending mode.

Test:
Manually tested all the modes using kmsxx python wrapper with and
without 'force_premulti' flag enabled.

Signed-off-by: Roman Stratiienko <roman.o.stratiienko@globallogic.com>
---
 drivers/gpu/drm/sun4i/sun8i_mixer.h    |  2 ++
 drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 48 ++++++++++++++++++++-----
 drivers/gpu/drm/sun4i/sun8i_ui_layer.h |  5 +++
 drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 49 ++++++++++++++++++++++----
 drivers/gpu/drm/sun4i/sun8i_vi_layer.h |  5 +++
 5 files changed, 94 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h b/drivers/gpu/drm/sun4i/sun8i_mixer.h
index ebfc276b2464..5c05907e26fb 100644
--- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
+++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
@@ -65,6 +65,8 @@
 #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_PREMULTIPLY_EN(pipe)	BIT(pipe)
+
 #define SUN8I_MIXER_BLEND_OUTCTL_INTERLACED	BIT(1)
 
 #define SUN50I_MIXER_BLEND_CSC_CTL_EN(ch)	BIT(ch)
diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
index 6ccbbca3176d..29c0d9cca19a 100644
--- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
@@ -58,24 +58,46 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer *mixer, int channel,
 }
 
 static void sun8i_ui_layer_update_alpha(struct sun8i_mixer *mixer, int channel,
-					int overlay, struct drm_plane *plane)
+					int overlay, struct drm_plane *plane,
+					unsigned int zpos, bool force_premulti)
 {
-	u32 mask, val, ch_base;
+	u32 mask, val, ch_base, bld_base;
+	bool in_premulti, out_premulti;
 
+	bld_base = sun8i_blender_base(mixer);
 	ch_base = sun8i_channel_base(mixer, channel);
 
+	in_premulti = plane->state->pixel_blend_mode == DRM_MODE_BLEND_PREMULTI;
+	out_premulti = force_premulti || in_premulti;
+
 	mask = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_MASK |
-		SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MASK;
+	       SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MASK |
+	       SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_MASK;
 
 	val = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA(plane->state->alpha >> 8);
 
-	val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE) ?
-		SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_PIXEL :
-		SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED;
+	if (plane->state->pixel_blend_mode == DRM_MODE_BLEND_PIXEL_NONE) {
+		val |= SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_LAYER;
+	} else {
+		val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE) ?
+			       SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_PIXEL :
+			       SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED;
+
+		if (in_premulti)
+			val |= SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_PREMULTI;
+	}
+
+	if (!in_premulti && out_premulti)
+		val |= SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COV2PREM;
 
 	regmap_update_bits(mixer->engine.regs,
 			   SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base, overlay),
 			   mask, val);
+
+	regmap_update_bits(
+		mixer->engine.regs, SUN8I_MIXER_BLEND_PREMULTIPLY(bld_base),
+		SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos),
+		out_premulti ? SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos) : 0);
 }
 
 static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int channel,
@@ -274,7 +296,7 @@ static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
 	sun8i_ui_layer_update_coord(mixer, layer->channel,
 				    layer->overlay, plane, zpos);
 	sun8i_ui_layer_update_alpha(mixer, layer->channel,
-				    layer->overlay, plane);
+				    layer->overlay, plane, zpos, false);
 	sun8i_ui_layer_update_formats(mixer, layer->channel,
 				      layer->overlay, plane);
 	sun8i_ui_layer_update_buffer(mixer, layer->channel,
@@ -332,8 +354,8 @@ 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;
+	unsigned int plane_cnt, blend_modes;
 	struct sun8i_ui_layer *layer;
-	unsigned int plane_cnt;
 	int ret;
 
 	layer = devm_kzalloc(drm->dev, sizeof(*layer), GFP_KERNEL);
@@ -362,6 +384,16 @@ struct sun8i_ui_layer *sun8i_ui_layer_init_one(struct drm_device *drm,
 		return ERR_PTR(ret);
 	}
 
+	blend_modes = BIT(DRM_MODE_BLEND_PREMULTI) |
+		      BIT(DRM_MODE_BLEND_COVERAGE) |
+		      BIT(DRM_MODE_BLEND_PIXEL_NONE);
+
+	ret = drm_plane_create_blend_mode_property(&layer->plane, blend_modes);
+	if (ret) {
+		dev_err(drm->dev, "Couldn't add blend mode property\n");
+		return ERR_PTR(ret);
+	}
+
 	ret = drm_plane_create_zpos_property(&layer->plane, channel,
 					     0, plane_cnt - 1);
 	if (ret) {
diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.h b/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
index 43c48cf7bc51..bd3c30e8058d 100644
--- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
+++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
@@ -46,6 +46,11 @@
 #define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_LAYER		((1) << 1)
 #define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED	((2) << 1)
 
+#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_MASK	GENMASK(17, 16)
+#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COVERAGE	((0) << 16)
+#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COV2PREM	((1) << 16)
+#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_PREMULTI	((2) << 16)
+
 struct sun8i_mixer;
 
 struct sun8i_ui_layer {
diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
index 662ba1018cc4..6581fc7d9668 100644
--- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
@@ -52,21 +52,38 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel,
 }
 
 static void sun8i_vi_layer_update_alpha(struct sun8i_mixer *mixer, int channel,
-					int overlay, struct drm_plane *plane)
+					int overlay, struct drm_plane *plane,
+					unsigned int zpos, bool force_premulti)
 {
-	u32 mask, val, ch_base;
+	u32 mask, val, ch_base, bld_base;
+	bool in_premulti = false, out_premulti = false;
 
 	ch_base = sun8i_channel_base(mixer, channel);
+	bld_base = sun8i_blender_base(mixer);
 
 	if (mixer->cfg->is_de3) {
+		in_premulti = plane->state->pixel_blend_mode == DRM_MODE_BLEND_PREMULTI;
+		out_premulti = force_premulti || in_premulti;
+
 		mask = SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK |
-		       SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_MASK;
+		       SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_MASK |
+		       SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_MASK;
 		val = SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA
 			(plane->state->alpha >> 8);
 
-		val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE) ?
-			SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL :
-			SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED;
+		if (plane->state->pixel_blend_mode == DRM_MODE_BLEND_PIXEL_NONE) {
+			val |= SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_LAYER;
+		} else {
+			val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE) ?
+				       SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL :
+				       SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED;
+
+			if (in_premulti)
+				val |= SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_PREMULTI;
+		}
+
+		if (!in_premulti && out_premulti)
+			val |= SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COV2PREM;
 
 		regmap_update_bits(mixer->engine.regs,
 				   SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base,
@@ -79,6 +96,11 @@ static void sun8i_vi_layer_update_alpha(struct sun8i_mixer *mixer, int channel,
 				   SUN8I_MIXER_FCC_GLOBAL_ALPHA
 					(plane->state->alpha >> 8));
 	}
+
+	regmap_update_bits(
+		mixer->engine.regs, SUN8I_MIXER_BLEND_PREMULTIPLY(bld_base),
+		SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos),
+		out_premulti ? SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos) : 0);
 }
 
 static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int channel,
@@ -408,7 +430,7 @@ static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
 	sun8i_vi_layer_update_coord(mixer, layer->channel,
 				    layer->overlay, plane, zpos);
 	sun8i_vi_layer_update_alpha(mixer, layer->channel,
-				    layer->overlay, plane);
+				    layer->overlay, plane, zpos, false);
 	sun8i_vi_layer_update_formats(mixer, layer->channel,
 				      layer->overlay, plane);
 	sun8i_vi_layer_update_buffer(mixer, layer->channel,
@@ -563,6 +585,19 @@ struct sun8i_vi_layer *sun8i_vi_layer_init_one(struct drm_device *drm,
 		}
 	}
 
+	if (mixer->cfg->is_de3) {
+		unsigned int blend_modes = BIT(DRM_MODE_BLEND_PREMULTI) |
+					   BIT(DRM_MODE_BLEND_COVERAGE) |
+					   BIT(DRM_MODE_BLEND_PIXEL_NONE);
+
+		ret = drm_plane_create_blend_mode_property(&layer->plane,
+							   blend_modes);
+		if (ret) {
+			dev_err(drm->dev, "Couldn't add blend mode property\n");
+			return ERR_PTR(ret);
+		}
+	}
+
 	ret = drm_plane_create_zpos_property(&layer->plane, index,
 					     0, plane_cnt - 1);
 	if (ret) {
diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
index 9939a4cc7a52..ccf91f09f1fe 100644
--- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
+++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
@@ -44,6 +44,11 @@
 #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK	GENMASK(31, 24)
 #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA(x)	((x) << 24)
 
+#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_MASK	GENMASK(17, 16)
+#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COVERAGE	((0) << 16)
+#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COV2PREM	((1) << 16)
+#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_PREMULTI	((2) << 16)
+
 #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL	((0) << 1)
 #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_LAYER	((1) << 1)
 #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED	((2) << 1)
-- 
2.30.2


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property
@ 2022-06-05 15:47 ` Roman Stratiienko
  0 siblings, 0 replies; 19+ messages in thread
From: Roman Stratiienko @ 2022-06-05 15:47 UTC (permalink / raw)
  To: mripard, wens, jernej.skrabec, airlied, daniel, samuel,
	dri-devel, linux-arm-kernel, linux-sunxi, linux-kernel, megi
  Cc: Roman Stratiienko

Allwinner DE2 and DE3 hardware support 3 pixel blend modes:
"None", "Pre-multiplied", "Coverage"

Add the blend mode property and route it to the appropriate registers.

Note:
"force_premulti" parameter was added to handle multi-overlay channel
cases in future changes. It must be set to true for cases when more
than 1 overlay layer is used within a channel and at least one of the
overlay layers within a group uses premultiplied blending mode.

Test:
Manually tested all the modes using kmsxx python wrapper with and
without 'force_premulti' flag enabled.

Signed-off-by: Roman Stratiienko <roman.o.stratiienko@globallogic.com>
---
 drivers/gpu/drm/sun4i/sun8i_mixer.h    |  2 ++
 drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 48 ++++++++++++++++++++-----
 drivers/gpu/drm/sun4i/sun8i_ui_layer.h |  5 +++
 drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 49 ++++++++++++++++++++++----
 drivers/gpu/drm/sun4i/sun8i_vi_layer.h |  5 +++
 5 files changed, 94 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h b/drivers/gpu/drm/sun4i/sun8i_mixer.h
index ebfc276b2464..5c05907e26fb 100644
--- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
+++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
@@ -65,6 +65,8 @@
 #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_PREMULTIPLY_EN(pipe)	BIT(pipe)
+
 #define SUN8I_MIXER_BLEND_OUTCTL_INTERLACED	BIT(1)
 
 #define SUN50I_MIXER_BLEND_CSC_CTL_EN(ch)	BIT(ch)
diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
index 6ccbbca3176d..29c0d9cca19a 100644
--- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
@@ -58,24 +58,46 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer *mixer, int channel,
 }
 
 static void sun8i_ui_layer_update_alpha(struct sun8i_mixer *mixer, int channel,
-					int overlay, struct drm_plane *plane)
+					int overlay, struct drm_plane *plane,
+					unsigned int zpos, bool force_premulti)
 {
-	u32 mask, val, ch_base;
+	u32 mask, val, ch_base, bld_base;
+	bool in_premulti, out_premulti;
 
+	bld_base = sun8i_blender_base(mixer);
 	ch_base = sun8i_channel_base(mixer, channel);
 
+	in_premulti = plane->state->pixel_blend_mode == DRM_MODE_BLEND_PREMULTI;
+	out_premulti = force_premulti || in_premulti;
+
 	mask = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_MASK |
-		SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MASK;
+	       SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MASK |
+	       SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_MASK;
 
 	val = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA(plane->state->alpha >> 8);
 
-	val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE) ?
-		SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_PIXEL :
-		SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED;
+	if (plane->state->pixel_blend_mode == DRM_MODE_BLEND_PIXEL_NONE) {
+		val |= SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_LAYER;
+	} else {
+		val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE) ?
+			       SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_PIXEL :
+			       SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED;
+
+		if (in_premulti)
+			val |= SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_PREMULTI;
+	}
+
+	if (!in_premulti && out_premulti)
+		val |= SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COV2PREM;
 
 	regmap_update_bits(mixer->engine.regs,
 			   SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base, overlay),
 			   mask, val);
+
+	regmap_update_bits(
+		mixer->engine.regs, SUN8I_MIXER_BLEND_PREMULTIPLY(bld_base),
+		SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos),
+		out_premulti ? SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos) : 0);
 }
 
 static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int channel,
@@ -274,7 +296,7 @@ static void sun8i_ui_layer_atomic_update(struct drm_plane *plane,
 	sun8i_ui_layer_update_coord(mixer, layer->channel,
 				    layer->overlay, plane, zpos);
 	sun8i_ui_layer_update_alpha(mixer, layer->channel,
-				    layer->overlay, plane);
+				    layer->overlay, plane, zpos, false);
 	sun8i_ui_layer_update_formats(mixer, layer->channel,
 				      layer->overlay, plane);
 	sun8i_ui_layer_update_buffer(mixer, layer->channel,
@@ -332,8 +354,8 @@ 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;
+	unsigned int plane_cnt, blend_modes;
 	struct sun8i_ui_layer *layer;
-	unsigned int plane_cnt;
 	int ret;
 
 	layer = devm_kzalloc(drm->dev, sizeof(*layer), GFP_KERNEL);
@@ -362,6 +384,16 @@ struct sun8i_ui_layer *sun8i_ui_layer_init_one(struct drm_device *drm,
 		return ERR_PTR(ret);
 	}
 
+	blend_modes = BIT(DRM_MODE_BLEND_PREMULTI) |
+		      BIT(DRM_MODE_BLEND_COVERAGE) |
+		      BIT(DRM_MODE_BLEND_PIXEL_NONE);
+
+	ret = drm_plane_create_blend_mode_property(&layer->plane, blend_modes);
+	if (ret) {
+		dev_err(drm->dev, "Couldn't add blend mode property\n");
+		return ERR_PTR(ret);
+	}
+
 	ret = drm_plane_create_zpos_property(&layer->plane, channel,
 					     0, plane_cnt - 1);
 	if (ret) {
diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.h b/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
index 43c48cf7bc51..bd3c30e8058d 100644
--- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
+++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
@@ -46,6 +46,11 @@
 #define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_LAYER		((1) << 1)
 #define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED	((2) << 1)
 
+#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_MASK	GENMASK(17, 16)
+#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COVERAGE	((0) << 16)
+#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COV2PREM	((1) << 16)
+#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_PREMULTI	((2) << 16)
+
 struct sun8i_mixer;
 
 struct sun8i_ui_layer {
diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
index 662ba1018cc4..6581fc7d9668 100644
--- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
+++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
@@ -52,21 +52,38 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer *mixer, int channel,
 }
 
 static void sun8i_vi_layer_update_alpha(struct sun8i_mixer *mixer, int channel,
-					int overlay, struct drm_plane *plane)
+					int overlay, struct drm_plane *plane,
+					unsigned int zpos, bool force_premulti)
 {
-	u32 mask, val, ch_base;
+	u32 mask, val, ch_base, bld_base;
+	bool in_premulti = false, out_premulti = false;
 
 	ch_base = sun8i_channel_base(mixer, channel);
+	bld_base = sun8i_blender_base(mixer);
 
 	if (mixer->cfg->is_de3) {
+		in_premulti = plane->state->pixel_blend_mode == DRM_MODE_BLEND_PREMULTI;
+		out_premulti = force_premulti || in_premulti;
+
 		mask = SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK |
-		       SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_MASK;
+		       SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_MASK |
+		       SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_MASK;
 		val = SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA
 			(plane->state->alpha >> 8);
 
-		val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE) ?
-			SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL :
-			SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED;
+		if (plane->state->pixel_blend_mode == DRM_MODE_BLEND_PIXEL_NONE) {
+			val |= SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_LAYER;
+		} else {
+			val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE) ?
+				       SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL :
+				       SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED;
+
+			if (in_premulti)
+				val |= SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_PREMULTI;
+		}
+
+		if (!in_premulti && out_premulti)
+			val |= SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COV2PREM;
 
 		regmap_update_bits(mixer->engine.regs,
 				   SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base,
@@ -79,6 +96,11 @@ static void sun8i_vi_layer_update_alpha(struct sun8i_mixer *mixer, int channel,
 				   SUN8I_MIXER_FCC_GLOBAL_ALPHA
 					(plane->state->alpha >> 8));
 	}
+
+	regmap_update_bits(
+		mixer->engine.regs, SUN8I_MIXER_BLEND_PREMULTIPLY(bld_base),
+		SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos),
+		out_premulti ? SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos) : 0);
 }
 
 static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int channel,
@@ -408,7 +430,7 @@ static void sun8i_vi_layer_atomic_update(struct drm_plane *plane,
 	sun8i_vi_layer_update_coord(mixer, layer->channel,
 				    layer->overlay, plane, zpos);
 	sun8i_vi_layer_update_alpha(mixer, layer->channel,
-				    layer->overlay, plane);
+				    layer->overlay, plane, zpos, false);
 	sun8i_vi_layer_update_formats(mixer, layer->channel,
 				      layer->overlay, plane);
 	sun8i_vi_layer_update_buffer(mixer, layer->channel,
@@ -563,6 +585,19 @@ struct sun8i_vi_layer *sun8i_vi_layer_init_one(struct drm_device *drm,
 		}
 	}
 
+	if (mixer->cfg->is_de3) {
+		unsigned int blend_modes = BIT(DRM_MODE_BLEND_PREMULTI) |
+					   BIT(DRM_MODE_BLEND_COVERAGE) |
+					   BIT(DRM_MODE_BLEND_PIXEL_NONE);
+
+		ret = drm_plane_create_blend_mode_property(&layer->plane,
+							   blend_modes);
+		if (ret) {
+			dev_err(drm->dev, "Couldn't add blend mode property\n");
+			return ERR_PTR(ret);
+		}
+	}
+
 	ret = drm_plane_create_zpos_property(&layer->plane, index,
 					     0, plane_cnt - 1);
 	if (ret) {
diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
index 9939a4cc7a52..ccf91f09f1fe 100644
--- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
+++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
@@ -44,6 +44,11 @@
 #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK	GENMASK(31, 24)
 #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA(x)	((x) << 24)
 
+#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_MASK	GENMASK(17, 16)
+#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COVERAGE	((0) << 16)
+#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COV2PREM	((1) << 16)
+#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_PREMULTI	((2) << 16)
+
 #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL	((0) << 1)
 #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_LAYER	((1) << 1)
 #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED	((2) << 1)
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property
  2022-06-05 15:47 ` Roman Stratiienko
@ 2022-06-05 20:36   ` Jernej Škrabec
  -1 siblings, 0 replies; 19+ messages in thread
From: Jernej Škrabec @ 2022-06-05 20:36 UTC (permalink / raw)
  To: mripard, wens, airlied, daniel, samuel, dri-devel,
	linux-arm-kernel, linux-sunxi, linux-kernel, megi,
	Roman Stratiienko
  Cc: Roman Stratiienko

Dne nedelja, 05. junij 2022 ob 17:47:31 CEST je Roman Stratiienko napisal(a):
> Allwinner DE2 and DE3 hardware support 3 pixel blend modes:
> "None", "Pre-multiplied", "Coverage"
> 
> Add the blend mode property and route it to the appropriate registers.
> 
> Note:
> "force_premulti" parameter was added to handle multi-overlay channel
> cases in future changes. It must be set to true for cases when more
> than 1 overlay layer is used within a channel and at least one of the
> overlay layers within a group uses premultiplied blending mode.

Please remove this parameter. It's nothing special, so it can be easily added 
once it's actually needed. For now, it only complicates code.

> 
> Test:
> Manually tested all the modes using kmsxx python wrapper with and
> without 'force_premulti' flag enabled.
> 
> Signed-off-by: Roman Stratiienko <roman.o.stratiienko@globallogic.com>
> ---
>  drivers/gpu/drm/sun4i/sun8i_mixer.h    |  2 ++
>  drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 48 ++++++++++++++++++++-----
>  drivers/gpu/drm/sun4i/sun8i_ui_layer.h |  5 +++
>  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 49 ++++++++++++++++++++++----
>  drivers/gpu/drm/sun4i/sun8i_vi_layer.h |  5 +++
>  5 files changed, 94 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> b/drivers/gpu/drm/sun4i/sun8i_mixer.h index ebfc276b2464..5c05907e26fb
> 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> @@ -65,6 +65,8 @@
>  #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_PREMULTIPLY_EN(pipe)	BIT(pipe)
> +
>  #define SUN8I_MIXER_BLEND_OUTCTL_INTERLACED	BIT(1)
> 
>  #define SUN50I_MIXER_BLEND_CSC_CTL_EN(ch)	BIT(ch)
> diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c index 6ccbbca3176d..29c0d9cca19a
> 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> @@ -58,24 +58,46 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer
> *mixer, int channel, }
> 
>  static void sun8i_ui_layer_update_alpha(struct sun8i_mixer *mixer, int
> channel, -					int overlay, struct 
drm_plane *plane)
> +					int overlay, struct 
drm_plane *plane,
> +					unsigned int zpos, 
bool force_premulti)
>  {
> -	u32 mask, val, ch_base;
> +	u32 mask, val, ch_base, bld_base;
> +	bool in_premulti, out_premulti;
> 
> +	bld_base = sun8i_blender_base(mixer);
>  	ch_base = sun8i_channel_base(mixer, channel);
> 
> +	in_premulti = plane->state->pixel_blend_mode == 
DRM_MODE_BLEND_PREMULTI;
> +	out_premulti = force_premulti || in_premulti;
> +
>  	mask = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_MASK |
> -		SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MASK;
> +	       SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MASK |
> +	       SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_MASK;
> 
>  	val = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA(plane->state->alpha >> 
8);
> 
> -	val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE) ?
> -		SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> -		SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> +	if (plane->state->pixel_blend_mode == DRM_MODE_BLEND_PIXEL_NONE) {
> +		val |= SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_LAYER;
> +	} else {
> +		val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE) 
?
> +			       
SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> +			       
SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> +
> +		if (in_premulti)
> +			val |= 
SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_PREMULTI;
> +	}
> +
> +	if (!in_premulti && out_premulti)
> +		val |= SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COV2PREM;
> 
>  	regmap_update_bits(mixer->engine.regs,
>  			   SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base, 
overlay),
>  			   mask, val);
> +
> +	regmap_update_bits(
> +		mixer->engine.regs, 
SUN8I_MIXER_BLEND_PREMULTIPLY(bld_base),
> +		SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos),
> +		out_premulti ? SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos) : 
0);
>  }
> 
>  static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int
> channel, @@ -274,7 +296,7 @@ static void
> sun8i_ui_layer_atomic_update(struct drm_plane *plane,
> sun8i_ui_layer_update_coord(mixer, layer->channel,
>  				    layer->overlay, plane, zpos);
>  	sun8i_ui_layer_update_alpha(mixer, layer->channel,
> -				    layer->overlay, plane);
> +				    layer->overlay, plane, zpos, 
false);
>  	sun8i_ui_layer_update_formats(mixer, layer->channel,
>  				      layer->overlay, plane);
>  	sun8i_ui_layer_update_buffer(mixer, layer->channel,
> @@ -332,8 +354,8 @@ 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;
> +	unsigned int plane_cnt, blend_modes;
>  	struct sun8i_ui_layer *layer;
> -	unsigned int plane_cnt;
>  	int ret;
> 
>  	layer = devm_kzalloc(drm->dev, sizeof(*layer), GFP_KERNEL);
> @@ -362,6 +384,16 @@ struct sun8i_ui_layer *sun8i_ui_layer_init_one(struct
> drm_device *drm, return ERR_PTR(ret);
>  	}
> 
> +	blend_modes = BIT(DRM_MODE_BLEND_PREMULTI) |
> +		      BIT(DRM_MODE_BLEND_COVERAGE) |
> +		      BIT(DRM_MODE_BLEND_PIXEL_NONE);
> +
> +	ret = drm_plane_create_blend_mode_property(&layer->plane, 
blend_modes);
> +	if (ret) {
> +		dev_err(drm->dev, "Couldn't add blend mode 
property\n");
> +		return ERR_PTR(ret);
> +	}
> +
>  	ret = drm_plane_create_zpos_property(&layer->plane, channel,
>  					     0, plane_cnt - 
1);
>  	if (ret) {
> diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
> b/drivers/gpu/drm/sun4i/sun8i_ui_layer.h index 43c48cf7bc51..bd3c30e8058d
> 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
> +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
> @@ -46,6 +46,11 @@
>  #define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_LAYER		((1) << 1)
>  #define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED	((2) << 1)
> 
> +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_MASK	GENMASK(17, 16)
> +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COVERAGE	((0) << 16)
> +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COV2PREM	((1) << 16)
> +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_PREMULTI	((2) << 16)
> +
>  struct sun8i_mixer;
> 
>  struct sun8i_ui_layer {
> diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index 662ba1018cc4..6581fc7d9668
> 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> @@ -52,21 +52,38 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer
> *mixer, int channel, }
> 
>  static void sun8i_vi_layer_update_alpha(struct sun8i_mixer *mixer, int
> channel, -					int overlay, struct 
drm_plane *plane)
> +					int overlay, struct 
drm_plane *plane,
> +					unsigned int zpos, 
bool force_premulti)
>  {
> -	u32 mask, val, ch_base;
> +	u32 mask, val, ch_base, bld_base;
> +	bool in_premulti = false, out_premulti = false;
> 
>  	ch_base = sun8i_channel_base(mixer, channel);
> +	bld_base = sun8i_blender_base(mixer);
> 
>  	if (mixer->cfg->is_de3) {
> +		in_premulti = plane->state->pixel_blend_mode == 
DRM_MODE_BLEND_PREMULTI;
> +		out_premulti = force_premulti || in_premulti;
> +
>  		mask = SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK |
> -		       SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_MASK;
> +		       SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_MASK 
|
> +		       SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_MASK;
>  		val = SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA
>  			(plane->state->alpha >> 8);
> 
> -		val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE) 
?
> -			
SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> -			
SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> +		if (plane->state->pixel_blend_mode == 
DRM_MODE_BLEND_PIXEL_NONE) {
> +			val |= 
SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_LAYER;
> +		} else {
> +			val |= (plane->state->alpha == 
DRM_BLEND_ALPHA_OPAQUE) ?
> +				       
SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> +				       
SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> +
> +			if (in_premulti)
> +				val |= 
SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_PREMULTI;
> +		}
> +
> +		if (!in_premulti && out_premulti)
> +			val |= 
SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COV2PREM;
> 
>  		regmap_update_bits(mixer->engine.regs,
>  				   
SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base,
> @@ -79,6 +96,11 @@ static void sun8i_vi_layer_update_alpha(struct
> sun8i_mixer *mixer, int channel, SUN8I_MIXER_FCC_GLOBAL_ALPHA
>  					(plane->state->alpha 
>> 8));
>  	}
> +
> +	regmap_update_bits(
> +		mixer->engine.regs, 
SUN8I_MIXER_BLEND_PREMULTIPLY(bld_base),
> +		SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos),
> +		out_premulti ? SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos) : 
0);

Shouldn't be above block inside DE3 if block?

Looks good otherwise.

Best regards,
Jernej

>  }
> 
>  static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int
> channel, @@ -408,7 +430,7 @@ static void
> sun8i_vi_layer_atomic_update(struct drm_plane *plane,
> sun8i_vi_layer_update_coord(mixer, layer->channel,
>  				    layer->overlay, plane, zpos);
>  	sun8i_vi_layer_update_alpha(mixer, layer->channel,
> -				    layer->overlay, plane);
> +				    layer->overlay, plane, zpos, 
false);
>  	sun8i_vi_layer_update_formats(mixer, layer->channel,
>  				      layer->overlay, plane);
>  	sun8i_vi_layer_update_buffer(mixer, layer->channel,
> @@ -563,6 +585,19 @@ struct sun8i_vi_layer *sun8i_vi_layer_init_one(struct
> drm_device *drm, }
>  	}
> 
> +	if (mixer->cfg->is_de3) {
> +		unsigned int blend_modes = BIT(DRM_MODE_BLEND_PREMULTI) 
|
> +					   
BIT(DRM_MODE_BLEND_COVERAGE) |
> +					   
BIT(DRM_MODE_BLEND_PIXEL_NONE);
> +
> +		ret = drm_plane_create_blend_mode_property(&layer-
>plane,
> +							   
blend_modes);
> +		if (ret) {
> +			dev_err(drm->dev, "Couldn't add blend mode 
property\n");
> +			return ERR_PTR(ret);
> +		}
> +	}
> +
>  	ret = drm_plane_create_zpos_property(&layer->plane, index,
>  					     0, plane_cnt - 
1);
>  	if (ret) {
> diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h index 9939a4cc7a52..ccf91f09f1fe
> 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> @@ -44,6 +44,11 @@
>  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK	GENMASK(31, 24)
>  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA(x)	((x) << 24)
> 
> +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_MASK	GENMASK(17, 16)
> +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COVERAGE	((0) << 16)
> +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COV2PREM	((1) << 16)
> +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_PREMULTI	((2) << 16)
> +
>  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL	((0) << 
1)
>  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_LAYER	((1) << 
1)
>  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED	((2) << 1)





^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property
@ 2022-06-05 20:36   ` Jernej Škrabec
  0 siblings, 0 replies; 19+ messages in thread
From: Jernej Škrabec @ 2022-06-05 20:36 UTC (permalink / raw)
  To: mripard, wens, airlied, daniel, samuel, dri-devel,
	linux-arm-kernel, linux-sunxi, linux-kernel, megi,
	Roman Stratiienko
  Cc: Roman Stratiienko

Dne nedelja, 05. junij 2022 ob 17:47:31 CEST je Roman Stratiienko napisal(a):
> Allwinner DE2 and DE3 hardware support 3 pixel blend modes:
> "None", "Pre-multiplied", "Coverage"
> 
> Add the blend mode property and route it to the appropriate registers.
> 
> Note:
> "force_premulti" parameter was added to handle multi-overlay channel
> cases in future changes. It must be set to true for cases when more
> than 1 overlay layer is used within a channel and at least one of the
> overlay layers within a group uses premultiplied blending mode.

Please remove this parameter. It's nothing special, so it can be easily added 
once it's actually needed. For now, it only complicates code.

> 
> Test:
> Manually tested all the modes using kmsxx python wrapper with and
> without 'force_premulti' flag enabled.
> 
> Signed-off-by: Roman Stratiienko <roman.o.stratiienko@globallogic.com>
> ---
>  drivers/gpu/drm/sun4i/sun8i_mixer.h    |  2 ++
>  drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 48 ++++++++++++++++++++-----
>  drivers/gpu/drm/sun4i/sun8i_ui_layer.h |  5 +++
>  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 49 ++++++++++++++++++++++----
>  drivers/gpu/drm/sun4i/sun8i_vi_layer.h |  5 +++
>  5 files changed, 94 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> b/drivers/gpu/drm/sun4i/sun8i_mixer.h index ebfc276b2464..5c05907e26fb
> 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> @@ -65,6 +65,8 @@
>  #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_PREMULTIPLY_EN(pipe)	BIT(pipe)
> +
>  #define SUN8I_MIXER_BLEND_OUTCTL_INTERLACED	BIT(1)
> 
>  #define SUN50I_MIXER_BLEND_CSC_CTL_EN(ch)	BIT(ch)
> diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c index 6ccbbca3176d..29c0d9cca19a
> 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> @@ -58,24 +58,46 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer
> *mixer, int channel, }
> 
>  static void sun8i_ui_layer_update_alpha(struct sun8i_mixer *mixer, int
> channel, -					int overlay, struct 
drm_plane *plane)
> +					int overlay, struct 
drm_plane *plane,
> +					unsigned int zpos, 
bool force_premulti)
>  {
> -	u32 mask, val, ch_base;
> +	u32 mask, val, ch_base, bld_base;
> +	bool in_premulti, out_premulti;
> 
> +	bld_base = sun8i_blender_base(mixer);
>  	ch_base = sun8i_channel_base(mixer, channel);
> 
> +	in_premulti = plane->state->pixel_blend_mode == 
DRM_MODE_BLEND_PREMULTI;
> +	out_premulti = force_premulti || in_premulti;
> +
>  	mask = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_MASK |
> -		SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MASK;
> +	       SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MASK |
> +	       SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_MASK;
> 
>  	val = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA(plane->state->alpha >> 
8);
> 
> -	val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE) ?
> -		SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> -		SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> +	if (plane->state->pixel_blend_mode == DRM_MODE_BLEND_PIXEL_NONE) {
> +		val |= SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_LAYER;
> +	} else {
> +		val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE) 
?
> +			       
SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> +			       
SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> +
> +		if (in_premulti)
> +			val |= 
SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_PREMULTI;
> +	}
> +
> +	if (!in_premulti && out_premulti)
> +		val |= SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COV2PREM;
> 
>  	regmap_update_bits(mixer->engine.regs,
>  			   SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base, 
overlay),
>  			   mask, val);
> +
> +	regmap_update_bits(
> +		mixer->engine.regs, 
SUN8I_MIXER_BLEND_PREMULTIPLY(bld_base),
> +		SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos),
> +		out_premulti ? SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos) : 
0);
>  }
> 
>  static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int
> channel, @@ -274,7 +296,7 @@ static void
> sun8i_ui_layer_atomic_update(struct drm_plane *plane,
> sun8i_ui_layer_update_coord(mixer, layer->channel,
>  				    layer->overlay, plane, zpos);
>  	sun8i_ui_layer_update_alpha(mixer, layer->channel,
> -				    layer->overlay, plane);
> +				    layer->overlay, plane, zpos, 
false);
>  	sun8i_ui_layer_update_formats(mixer, layer->channel,
>  				      layer->overlay, plane);
>  	sun8i_ui_layer_update_buffer(mixer, layer->channel,
> @@ -332,8 +354,8 @@ 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;
> +	unsigned int plane_cnt, blend_modes;
>  	struct sun8i_ui_layer *layer;
> -	unsigned int plane_cnt;
>  	int ret;
> 
>  	layer = devm_kzalloc(drm->dev, sizeof(*layer), GFP_KERNEL);
> @@ -362,6 +384,16 @@ struct sun8i_ui_layer *sun8i_ui_layer_init_one(struct
> drm_device *drm, return ERR_PTR(ret);
>  	}
> 
> +	blend_modes = BIT(DRM_MODE_BLEND_PREMULTI) |
> +		      BIT(DRM_MODE_BLEND_COVERAGE) |
> +		      BIT(DRM_MODE_BLEND_PIXEL_NONE);
> +
> +	ret = drm_plane_create_blend_mode_property(&layer->plane, 
blend_modes);
> +	if (ret) {
> +		dev_err(drm->dev, "Couldn't add blend mode 
property\n");
> +		return ERR_PTR(ret);
> +	}
> +
>  	ret = drm_plane_create_zpos_property(&layer->plane, channel,
>  					     0, plane_cnt - 
1);
>  	if (ret) {
> diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
> b/drivers/gpu/drm/sun4i/sun8i_ui_layer.h index 43c48cf7bc51..bd3c30e8058d
> 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
> +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
> @@ -46,6 +46,11 @@
>  #define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_LAYER		((1) << 1)
>  #define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED	((2) << 1)
> 
> +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_MASK	GENMASK(17, 16)
> +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COVERAGE	((0) << 16)
> +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COV2PREM	((1) << 16)
> +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_PREMULTI	((2) << 16)
> +
>  struct sun8i_mixer;
> 
>  struct sun8i_ui_layer {
> diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index 662ba1018cc4..6581fc7d9668
> 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> @@ -52,21 +52,38 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer
> *mixer, int channel, }
> 
>  static void sun8i_vi_layer_update_alpha(struct sun8i_mixer *mixer, int
> channel, -					int overlay, struct 
drm_plane *plane)
> +					int overlay, struct 
drm_plane *plane,
> +					unsigned int zpos, 
bool force_premulti)
>  {
> -	u32 mask, val, ch_base;
> +	u32 mask, val, ch_base, bld_base;
> +	bool in_premulti = false, out_premulti = false;
> 
>  	ch_base = sun8i_channel_base(mixer, channel);
> +	bld_base = sun8i_blender_base(mixer);
> 
>  	if (mixer->cfg->is_de3) {
> +		in_premulti = plane->state->pixel_blend_mode == 
DRM_MODE_BLEND_PREMULTI;
> +		out_premulti = force_premulti || in_premulti;
> +
>  		mask = SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK |
> -		       SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_MASK;
> +		       SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_MASK 
|
> +		       SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_MASK;
>  		val = SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA
>  			(plane->state->alpha >> 8);
> 
> -		val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE) 
?
> -			
SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> -			
SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> +		if (plane->state->pixel_blend_mode == 
DRM_MODE_BLEND_PIXEL_NONE) {
> +			val |= 
SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_LAYER;
> +		} else {
> +			val |= (plane->state->alpha == 
DRM_BLEND_ALPHA_OPAQUE) ?
> +				       
SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> +				       
SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> +
> +			if (in_premulti)
> +				val |= 
SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_PREMULTI;
> +		}
> +
> +		if (!in_premulti && out_premulti)
> +			val |= 
SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COV2PREM;
> 
>  		regmap_update_bits(mixer->engine.regs,
>  				   
SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base,
> @@ -79,6 +96,11 @@ static void sun8i_vi_layer_update_alpha(struct
> sun8i_mixer *mixer, int channel, SUN8I_MIXER_FCC_GLOBAL_ALPHA
>  					(plane->state->alpha 
>> 8));
>  	}
> +
> +	regmap_update_bits(
> +		mixer->engine.regs, 
SUN8I_MIXER_BLEND_PREMULTIPLY(bld_base),
> +		SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos),
> +		out_premulti ? SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos) : 
0);

Shouldn't be above block inside DE3 if block?

Looks good otherwise.

Best regards,
Jernej

>  }
> 
>  static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int
> channel, @@ -408,7 +430,7 @@ static void
> sun8i_vi_layer_atomic_update(struct drm_plane *plane,
> sun8i_vi_layer_update_coord(mixer, layer->channel,
>  				    layer->overlay, plane, zpos);
>  	sun8i_vi_layer_update_alpha(mixer, layer->channel,
> -				    layer->overlay, plane);
> +				    layer->overlay, plane, zpos, 
false);
>  	sun8i_vi_layer_update_formats(mixer, layer->channel,
>  				      layer->overlay, plane);
>  	sun8i_vi_layer_update_buffer(mixer, layer->channel,
> @@ -563,6 +585,19 @@ struct sun8i_vi_layer *sun8i_vi_layer_init_one(struct
> drm_device *drm, }
>  	}
> 
> +	if (mixer->cfg->is_de3) {
> +		unsigned int blend_modes = BIT(DRM_MODE_BLEND_PREMULTI) 
|
> +					   
BIT(DRM_MODE_BLEND_COVERAGE) |
> +					   
BIT(DRM_MODE_BLEND_PIXEL_NONE);
> +
> +		ret = drm_plane_create_blend_mode_property(&layer-
>plane,
> +							   
blend_modes);
> +		if (ret) {
> +			dev_err(drm->dev, "Couldn't add blend mode 
property\n");
> +			return ERR_PTR(ret);
> +		}
> +	}
> +
>  	ret = drm_plane_create_zpos_property(&layer->plane, index,
>  					     0, plane_cnt - 
1);
>  	if (ret) {
> diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h index 9939a4cc7a52..ccf91f09f1fe
> 100644
> --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> @@ -44,6 +44,11 @@
>  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK	GENMASK(31, 24)
>  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA(x)	((x) << 24)
> 
> +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_MASK	GENMASK(17, 16)
> +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COVERAGE	((0) << 16)
> +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COV2PREM	((1) << 16)
> +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_PREMULTI	((2) << 16)
> +
>  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL	((0) << 
1)
>  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_LAYER	((1) << 
1)
>  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED	((2) << 1)





_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property
  2022-06-05 20:36   ` Jernej Škrabec
  (?)
@ 2022-06-06  8:17     ` Roman Stratiienko
  -1 siblings, 0 replies; 19+ messages in thread
From: Roman Stratiienko @ 2022-06-06  8:17 UTC (permalink / raw)
  To: Jernej Škrabec
  Cc: mripard, wens, airlied, Daniel Vetter, Samuel Holland, dri-devel,
	linux-arm-kernel, linux-sunxi, linux-kernel, megi,
	Roman Stratiienko

Hello Jernej,

Thank you for having a look.

вс, 5 июн. 2022 г. в 23:37, Jernej Škrabec <jernej.skrabec@gmail.com>:
>
> Dne nedelja, 05. junij 2022 ob 17:47:31 CEST je Roman Stratiienko napisal(a):
> > Allwinner DE2 and DE3 hardware support 3 pixel blend modes:
> > "None", "Pre-multiplied", "Coverage"
> >
> > Add the blend mode property and route it to the appropriate registers.
> >
> > Note:
> > "force_premulti" parameter was added to handle multi-overlay channel
> > cases in future changes. It must be set to true for cases when more
> > than 1 overlay layer is used within a channel and at least one of the
> > overlay layers within a group uses premultiplied blending mode.
>
> Please remove this parameter. It's nothing special, so it can be easily added
> once it's actually needed. For now, it only complicates code.

I would prefer keeping it if you do not have any strong opinion against it.

I am working now on exposing all overlays, so it will be needed soon anyway.

Also it helps to better understand the COV2PREMULT mode which has not
the best description in the datasheet. Only after testing this
register using devmem I became confident on its purpose.

>
> >
> > Test:
> > Manually tested all the modes using kmsxx python wrapper with and
> > without 'force_premulti' flag enabled.
> >
> > Signed-off-by: Roman Stratiienko <roman.o.stratiienko@globallogic.com>
> > ---
> >  drivers/gpu/drm/sun4i/sun8i_mixer.h    |  2 ++
> >  drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 48 ++++++++++++++++++++-----
> >  drivers/gpu/drm/sun4i/sun8i_ui_layer.h |  5 +++
> >  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 49 ++++++++++++++++++++++----
> >  drivers/gpu/drm/sun4i/sun8i_vi_layer.h |  5 +++
> >  5 files changed, 94 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > b/drivers/gpu/drm/sun4i/sun8i_mixer.h index ebfc276b2464..5c05907e26fb
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > @@ -65,6 +65,8 @@
> >  #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_PREMULTIPLY_EN(pipe)       BIT(pipe)
> > +
> >  #define SUN8I_MIXER_BLEND_OUTCTL_INTERLACED  BIT(1)
> >
> >  #define SUN50I_MIXER_BLEND_CSC_CTL_EN(ch)    BIT(ch)
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c index 6ccbbca3176d..29c0d9cca19a
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > @@ -58,24 +58,46 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer
> > *mixer, int channel, }
> >
> >  static void sun8i_ui_layer_update_alpha(struct sun8i_mixer *mixer, int
> > channel, -                                    int overlay, struct
> drm_plane *plane)
> > +                                     int overlay, struct
> drm_plane *plane,
> > +                                     unsigned int zpos,
> bool force_premulti)
> >  {
> > -     u32 mask, val, ch_base;
> > +     u32 mask, val, ch_base, bld_base;
> > +     bool in_premulti, out_premulti;
> >
> > +     bld_base = sun8i_blender_base(mixer);
> >       ch_base = sun8i_channel_base(mixer, channel);
> >
> > +     in_premulti = plane->state->pixel_blend_mode ==
> DRM_MODE_BLEND_PREMULTI;
> > +     out_premulti = force_premulti || in_premulti;
> > +
> >       mask = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_MASK |
> > -             SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MASK;
> > +            SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MASK |
> > +            SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_MASK;
> >
> >       val = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA(plane->state->alpha >>
> 8);
> >
> > -     val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE) ?
> > -             SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> > -             SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> > +     if (plane->state->pixel_blend_mode == DRM_MODE_BLEND_PIXEL_NONE) {
> > +             val |= SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_LAYER;
> > +     } else {
> > +             val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE)
> ?
> > +
> SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> > +
> SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> > +
> > +             if (in_premulti)
> > +                     val |=
> SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_PREMULTI;
> > +     }
> > +
> > +     if (!in_premulti && out_premulti)
> > +             val |= SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COV2PREM;
> >
> >       regmap_update_bits(mixer->engine.regs,
> >                          SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base,
> overlay),
> >                          mask, val);
> > +
> > +     regmap_update_bits(
> > +             mixer->engine.regs,
> SUN8I_MIXER_BLEND_PREMULTIPLY(bld_base),
> > +             SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos),
> > +             out_premulti ? SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos) :
> 0);
> >  }
> >
> >  static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int
> > channel, @@ -274,7 +296,7 @@ static void
> > sun8i_ui_layer_atomic_update(struct drm_plane *plane,
> > sun8i_ui_layer_update_coord(mixer, layer->channel,
> >                                   layer->overlay, plane, zpos);
> >       sun8i_ui_layer_update_alpha(mixer, layer->channel,
> > -                                 layer->overlay, plane);
> > +                                 layer->overlay, plane, zpos,
> false);
> >       sun8i_ui_layer_update_formats(mixer, layer->channel,
> >                                     layer->overlay, plane);
> >       sun8i_ui_layer_update_buffer(mixer, layer->channel,
> > @@ -332,8 +354,8 @@ 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;
> > +     unsigned int plane_cnt, blend_modes;
> >       struct sun8i_ui_layer *layer;
> > -     unsigned int plane_cnt;
> >       int ret;
> >
> >       layer = devm_kzalloc(drm->dev, sizeof(*layer), GFP_KERNEL);
> > @@ -362,6 +384,16 @@ struct sun8i_ui_layer *sun8i_ui_layer_init_one(struct
> > drm_device *drm, return ERR_PTR(ret);
> >       }
> >
> > +     blend_modes = BIT(DRM_MODE_BLEND_PREMULTI) |
> > +                   BIT(DRM_MODE_BLEND_COVERAGE) |
> > +                   BIT(DRM_MODE_BLEND_PIXEL_NONE);
> > +
> > +     ret = drm_plane_create_blend_mode_property(&layer->plane,
> blend_modes);
> > +     if (ret) {
> > +             dev_err(drm->dev, "Couldn't add blend mode
> property\n");
> > +             return ERR_PTR(ret);
> > +     }
> > +
> >       ret = drm_plane_create_zpos_property(&layer->plane, channel,
> >                                            0, plane_cnt -
> 1);
> >       if (ret) {
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
> > b/drivers/gpu/drm/sun4i/sun8i_ui_layer.h index 43c48cf7bc51..bd3c30e8058d
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
> > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
> > @@ -46,6 +46,11 @@
> >  #define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_LAYER              ((1) << 1)
> >  #define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED   ((2) << 1)
> >
> > +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_MASK    GENMASK(17, 16)
> > +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COVERAGE        ((0) << 16)
> > +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COV2PREM        ((1) << 16)
> > +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_PREMULTI        ((2) << 16)
> > +
> >  struct sun8i_mixer;
> >
> >  struct sun8i_ui_layer {
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index 662ba1018cc4..6581fc7d9668
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > @@ -52,21 +52,38 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer
> > *mixer, int channel, }
> >
> >  static void sun8i_vi_layer_update_alpha(struct sun8i_mixer *mixer, int
> > channel, -                                    int overlay, struct
> drm_plane *plane)
> > +                                     int overlay, struct
> drm_plane *plane,
> > +                                     unsigned int zpos,
> bool force_premulti)
> >  {
> > -     u32 mask, val, ch_base;
> > +     u32 mask, val, ch_base, bld_base;
> > +     bool in_premulti = false, out_premulti = false;
> >
> >       ch_base = sun8i_channel_base(mixer, channel);
> > +     bld_base = sun8i_blender_base(mixer);
> >
> >       if (mixer->cfg->is_de3) {
> > +             in_premulti = plane->state->pixel_blend_mode ==
> DRM_MODE_BLEND_PREMULTI;
> > +             out_premulti = force_premulti || in_premulti;
> > +
> >               mask = SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK |
> > -                    SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_MASK;
> > +                    SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_MASK
> |
> > +                    SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_MASK;
> >               val = SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA
> >                       (plane->state->alpha >> 8);
> >
> > -             val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE)
> ?
> > -
> SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> > -
> SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> > +             if (plane->state->pixel_blend_mode ==
> DRM_MODE_BLEND_PIXEL_NONE) {
> > +                     val |=
> SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_LAYER;
> > +             } else {
> > +                     val |= (plane->state->alpha ==
> DRM_BLEND_ALPHA_OPAQUE) ?
> > +
> SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> > +
> SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> > +
> > +                     if (in_premulti)
> > +                             val |=
> SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_PREMULTI;
> > +             }
> > +
> > +             if (!in_premulti && out_premulti)
> > +                     val |=
> SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COV2PREM;
> >
> >               regmap_update_bits(mixer->engine.regs,
> >
> SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base,
> > @@ -79,6 +96,11 @@ static void sun8i_vi_layer_update_alpha(struct
> > sun8i_mixer *mixer, int channel, SUN8I_MIXER_FCC_GLOBAL_ALPHA
> >                                       (plane->state->alpha
> >> 8));
> >       }
> > +
> > +     regmap_update_bits(
> > +             mixer->engine.regs,
> SUN8I_MIXER_BLEND_PREMULTIPLY(bld_base),
> > +             SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos),
> > +             out_premulti ? SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos) :
> 0);
>
> Shouldn't be above block inside DE3 if block?

No, we have to update the value to non-premulti in case it was set to
premulti by another layer.


Regards,
Roman.

>
> Looks good otherwise.
>
> Best regards,
> Jernej
>
> >  }
> >
> >  static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int
> > channel, @@ -408,7 +430,7 @@ static void
> > sun8i_vi_layer_atomic_update(struct drm_plane *plane,
> > sun8i_vi_layer_update_coord(mixer, layer->channel,
> >                                   layer->overlay, plane, zpos);
> >       sun8i_vi_layer_update_alpha(mixer, layer->channel,
> > -                                 layer->overlay, plane);
> > +                                 layer->overlay, plane, zpos,
> false);
> >       sun8i_vi_layer_update_formats(mixer, layer->channel,
> >                                     layer->overlay, plane);
> >       sun8i_vi_layer_update_buffer(mixer, layer->channel,
> > @@ -563,6 +585,19 @@ struct sun8i_vi_layer *sun8i_vi_layer_init_one(struct
> > drm_device *drm, }
> >       }
> >
> > +     if (mixer->cfg->is_de3) {
> > +             unsigned int blend_modes = BIT(DRM_MODE_BLEND_PREMULTI)
> |
> > +
> BIT(DRM_MODE_BLEND_COVERAGE) |
> > +
> BIT(DRM_MODE_BLEND_PIXEL_NONE);
> > +
> > +             ret = drm_plane_create_blend_mode_property(&layer-
> >plane,
> > +
> blend_modes);
> > +             if (ret) {
> > +                     dev_err(drm->dev, "Couldn't add blend mode
> property\n");
> > +                     return ERR_PTR(ret);
> > +             }
> > +     }
> > +
> >       ret = drm_plane_create_zpos_property(&layer->plane, index,
> >                                            0, plane_cnt -
> 1);
> >       if (ret) {
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h index 9939a4cc7a52..ccf91f09f1fe
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > @@ -44,6 +44,11 @@
> >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK   GENMASK(31, 24)
> >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA(x)     ((x) << 24)
> >
> > +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_MASK   GENMASK(17, 16)
> > +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COVERAGE       ((0) << 16)
> > +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COV2PREM       ((1) << 16)
> > +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_PREMULTI       ((2) << 16)
> > +
> >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL     ((0) <<
> 1)
> >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_LAYER     ((1) <<
> 1)
> >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED  ((2) << 1)
>
>
>
>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property
@ 2022-06-06  8:17     ` Roman Stratiienko
  0 siblings, 0 replies; 19+ messages in thread
From: Roman Stratiienko @ 2022-06-06  8:17 UTC (permalink / raw)
  To: Jernej Škrabec
  Cc: Samuel Holland, airlied, Roman Stratiienko, linux-kernel,
	dri-devel, wens, megi, linux-sunxi, linux-arm-kernel

Hello Jernej,

Thank you for having a look.

вс, 5 июн. 2022 г. в 23:37, Jernej Škrabec <jernej.skrabec@gmail.com>:
>
> Dne nedelja, 05. junij 2022 ob 17:47:31 CEST je Roman Stratiienko napisal(a):
> > Allwinner DE2 and DE3 hardware support 3 pixel blend modes:
> > "None", "Pre-multiplied", "Coverage"
> >
> > Add the blend mode property and route it to the appropriate registers.
> >
> > Note:
> > "force_premulti" parameter was added to handle multi-overlay channel
> > cases in future changes. It must be set to true for cases when more
> > than 1 overlay layer is used within a channel and at least one of the
> > overlay layers within a group uses premultiplied blending mode.
>
> Please remove this parameter. It's nothing special, so it can be easily added
> once it's actually needed. For now, it only complicates code.

I would prefer keeping it if you do not have any strong opinion against it.

I am working now on exposing all overlays, so it will be needed soon anyway.

Also it helps to better understand the COV2PREMULT mode which has not
the best description in the datasheet. Only after testing this
register using devmem I became confident on its purpose.

>
> >
> > Test:
> > Manually tested all the modes using kmsxx python wrapper with and
> > without 'force_premulti' flag enabled.
> >
> > Signed-off-by: Roman Stratiienko <roman.o.stratiienko@globallogic.com>
> > ---
> >  drivers/gpu/drm/sun4i/sun8i_mixer.h    |  2 ++
> >  drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 48 ++++++++++++++++++++-----
> >  drivers/gpu/drm/sun4i/sun8i_ui_layer.h |  5 +++
> >  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 49 ++++++++++++++++++++++----
> >  drivers/gpu/drm/sun4i/sun8i_vi_layer.h |  5 +++
> >  5 files changed, 94 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > b/drivers/gpu/drm/sun4i/sun8i_mixer.h index ebfc276b2464..5c05907e26fb
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > @@ -65,6 +65,8 @@
> >  #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_PREMULTIPLY_EN(pipe)       BIT(pipe)
> > +
> >  #define SUN8I_MIXER_BLEND_OUTCTL_INTERLACED  BIT(1)
> >
> >  #define SUN50I_MIXER_BLEND_CSC_CTL_EN(ch)    BIT(ch)
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c index 6ccbbca3176d..29c0d9cca19a
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > @@ -58,24 +58,46 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer
> > *mixer, int channel, }
> >
> >  static void sun8i_ui_layer_update_alpha(struct sun8i_mixer *mixer, int
> > channel, -                                    int overlay, struct
> drm_plane *plane)
> > +                                     int overlay, struct
> drm_plane *plane,
> > +                                     unsigned int zpos,
> bool force_premulti)
> >  {
> > -     u32 mask, val, ch_base;
> > +     u32 mask, val, ch_base, bld_base;
> > +     bool in_premulti, out_premulti;
> >
> > +     bld_base = sun8i_blender_base(mixer);
> >       ch_base = sun8i_channel_base(mixer, channel);
> >
> > +     in_premulti = plane->state->pixel_blend_mode ==
> DRM_MODE_BLEND_PREMULTI;
> > +     out_premulti = force_premulti || in_premulti;
> > +
> >       mask = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_MASK |
> > -             SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MASK;
> > +            SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MASK |
> > +            SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_MASK;
> >
> >       val = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA(plane->state->alpha >>
> 8);
> >
> > -     val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE) ?
> > -             SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> > -             SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> > +     if (plane->state->pixel_blend_mode == DRM_MODE_BLEND_PIXEL_NONE) {
> > +             val |= SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_LAYER;
> > +     } else {
> > +             val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE)
> ?
> > +
> SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> > +
> SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> > +
> > +             if (in_premulti)
> > +                     val |=
> SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_PREMULTI;
> > +     }
> > +
> > +     if (!in_premulti && out_premulti)
> > +             val |= SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COV2PREM;
> >
> >       regmap_update_bits(mixer->engine.regs,
> >                          SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base,
> overlay),
> >                          mask, val);
> > +
> > +     regmap_update_bits(
> > +             mixer->engine.regs,
> SUN8I_MIXER_BLEND_PREMULTIPLY(bld_base),
> > +             SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos),
> > +             out_premulti ? SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos) :
> 0);
> >  }
> >
> >  static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int
> > channel, @@ -274,7 +296,7 @@ static void
> > sun8i_ui_layer_atomic_update(struct drm_plane *plane,
> > sun8i_ui_layer_update_coord(mixer, layer->channel,
> >                                   layer->overlay, plane, zpos);
> >       sun8i_ui_layer_update_alpha(mixer, layer->channel,
> > -                                 layer->overlay, plane);
> > +                                 layer->overlay, plane, zpos,
> false);
> >       sun8i_ui_layer_update_formats(mixer, layer->channel,
> >                                     layer->overlay, plane);
> >       sun8i_ui_layer_update_buffer(mixer, layer->channel,
> > @@ -332,8 +354,8 @@ 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;
> > +     unsigned int plane_cnt, blend_modes;
> >       struct sun8i_ui_layer *layer;
> > -     unsigned int plane_cnt;
> >       int ret;
> >
> >       layer = devm_kzalloc(drm->dev, sizeof(*layer), GFP_KERNEL);
> > @@ -362,6 +384,16 @@ struct sun8i_ui_layer *sun8i_ui_layer_init_one(struct
> > drm_device *drm, return ERR_PTR(ret);
> >       }
> >
> > +     blend_modes = BIT(DRM_MODE_BLEND_PREMULTI) |
> > +                   BIT(DRM_MODE_BLEND_COVERAGE) |
> > +                   BIT(DRM_MODE_BLEND_PIXEL_NONE);
> > +
> > +     ret = drm_plane_create_blend_mode_property(&layer->plane,
> blend_modes);
> > +     if (ret) {
> > +             dev_err(drm->dev, "Couldn't add blend mode
> property\n");
> > +             return ERR_PTR(ret);
> > +     }
> > +
> >       ret = drm_plane_create_zpos_property(&layer->plane, channel,
> >                                            0, plane_cnt -
> 1);
> >       if (ret) {
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
> > b/drivers/gpu/drm/sun4i/sun8i_ui_layer.h index 43c48cf7bc51..bd3c30e8058d
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
> > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
> > @@ -46,6 +46,11 @@
> >  #define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_LAYER              ((1) << 1)
> >  #define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED   ((2) << 1)
> >
> > +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_MASK    GENMASK(17, 16)
> > +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COVERAGE        ((0) << 16)
> > +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COV2PREM        ((1) << 16)
> > +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_PREMULTI        ((2) << 16)
> > +
> >  struct sun8i_mixer;
> >
> >  struct sun8i_ui_layer {
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index 662ba1018cc4..6581fc7d9668
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > @@ -52,21 +52,38 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer
> > *mixer, int channel, }
> >
> >  static void sun8i_vi_layer_update_alpha(struct sun8i_mixer *mixer, int
> > channel, -                                    int overlay, struct
> drm_plane *plane)
> > +                                     int overlay, struct
> drm_plane *plane,
> > +                                     unsigned int zpos,
> bool force_premulti)
> >  {
> > -     u32 mask, val, ch_base;
> > +     u32 mask, val, ch_base, bld_base;
> > +     bool in_premulti = false, out_premulti = false;
> >
> >       ch_base = sun8i_channel_base(mixer, channel);
> > +     bld_base = sun8i_blender_base(mixer);
> >
> >       if (mixer->cfg->is_de3) {
> > +             in_premulti = plane->state->pixel_blend_mode ==
> DRM_MODE_BLEND_PREMULTI;
> > +             out_premulti = force_premulti || in_premulti;
> > +
> >               mask = SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK |
> > -                    SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_MASK;
> > +                    SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_MASK
> |
> > +                    SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_MASK;
> >               val = SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA
> >                       (plane->state->alpha >> 8);
> >
> > -             val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE)
> ?
> > -
> SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> > -
> SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> > +             if (plane->state->pixel_blend_mode ==
> DRM_MODE_BLEND_PIXEL_NONE) {
> > +                     val |=
> SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_LAYER;
> > +             } else {
> > +                     val |= (plane->state->alpha ==
> DRM_BLEND_ALPHA_OPAQUE) ?
> > +
> SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> > +
> SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> > +
> > +                     if (in_premulti)
> > +                             val |=
> SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_PREMULTI;
> > +             }
> > +
> > +             if (!in_premulti && out_premulti)
> > +                     val |=
> SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COV2PREM;
> >
> >               regmap_update_bits(mixer->engine.regs,
> >
> SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base,
> > @@ -79,6 +96,11 @@ static void sun8i_vi_layer_update_alpha(struct
> > sun8i_mixer *mixer, int channel, SUN8I_MIXER_FCC_GLOBAL_ALPHA
> >                                       (plane->state->alpha
> >> 8));
> >       }
> > +
> > +     regmap_update_bits(
> > +             mixer->engine.regs,
> SUN8I_MIXER_BLEND_PREMULTIPLY(bld_base),
> > +             SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos),
> > +             out_premulti ? SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos) :
> 0);
>
> Shouldn't be above block inside DE3 if block?

No, we have to update the value to non-premulti in case it was set to
premulti by another layer.


Regards,
Roman.

>
> Looks good otherwise.
>
> Best regards,
> Jernej
>
> >  }
> >
> >  static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int
> > channel, @@ -408,7 +430,7 @@ static void
> > sun8i_vi_layer_atomic_update(struct drm_plane *plane,
> > sun8i_vi_layer_update_coord(mixer, layer->channel,
> >                                   layer->overlay, plane, zpos);
> >       sun8i_vi_layer_update_alpha(mixer, layer->channel,
> > -                                 layer->overlay, plane);
> > +                                 layer->overlay, plane, zpos,
> false);
> >       sun8i_vi_layer_update_formats(mixer, layer->channel,
> >                                     layer->overlay, plane);
> >       sun8i_vi_layer_update_buffer(mixer, layer->channel,
> > @@ -563,6 +585,19 @@ struct sun8i_vi_layer *sun8i_vi_layer_init_one(struct
> > drm_device *drm, }
> >       }
> >
> > +     if (mixer->cfg->is_de3) {
> > +             unsigned int blend_modes = BIT(DRM_MODE_BLEND_PREMULTI)
> |
> > +
> BIT(DRM_MODE_BLEND_COVERAGE) |
> > +
> BIT(DRM_MODE_BLEND_PIXEL_NONE);
> > +
> > +             ret = drm_plane_create_blend_mode_property(&layer-
> >plane,
> > +
> blend_modes);
> > +             if (ret) {
> > +                     dev_err(drm->dev, "Couldn't add blend mode
> property\n");
> > +                     return ERR_PTR(ret);
> > +             }
> > +     }
> > +
> >       ret = drm_plane_create_zpos_property(&layer->plane, index,
> >                                            0, plane_cnt -
> 1);
> >       if (ret) {
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h index 9939a4cc7a52..ccf91f09f1fe
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > @@ -44,6 +44,11 @@
> >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK   GENMASK(31, 24)
> >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA(x)     ((x) << 24)
> >
> > +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_MASK   GENMASK(17, 16)
> > +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COVERAGE       ((0) << 16)
> > +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COV2PREM       ((1) << 16)
> > +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_PREMULTI       ((2) << 16)
> > +
> >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL     ((0) <<
> 1)
> >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_LAYER     ((1) <<
> 1)
> >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED  ((2) << 1)
>
>
>
>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property
@ 2022-06-06  8:17     ` Roman Stratiienko
  0 siblings, 0 replies; 19+ messages in thread
From: Roman Stratiienko @ 2022-06-06  8:17 UTC (permalink / raw)
  To: Jernej Škrabec
  Cc: mripard, wens, airlied, Daniel Vetter, Samuel Holland, dri-devel,
	linux-arm-kernel, linux-sunxi, linux-kernel, megi,
	Roman Stratiienko

Hello Jernej,

Thank you for having a look.

вс, 5 июн. 2022 г. в 23:37, Jernej Škrabec <jernej.skrabec@gmail.com>:
>
> Dne nedelja, 05. junij 2022 ob 17:47:31 CEST je Roman Stratiienko napisal(a):
> > Allwinner DE2 and DE3 hardware support 3 pixel blend modes:
> > "None", "Pre-multiplied", "Coverage"
> >
> > Add the blend mode property and route it to the appropriate registers.
> >
> > Note:
> > "force_premulti" parameter was added to handle multi-overlay channel
> > cases in future changes. It must be set to true for cases when more
> > than 1 overlay layer is used within a channel and at least one of the
> > overlay layers within a group uses premultiplied blending mode.
>
> Please remove this parameter. It's nothing special, so it can be easily added
> once it's actually needed. For now, it only complicates code.

I would prefer keeping it if you do not have any strong opinion against it.

I am working now on exposing all overlays, so it will be needed soon anyway.

Also it helps to better understand the COV2PREMULT mode which has not
the best description in the datasheet. Only after testing this
register using devmem I became confident on its purpose.

>
> >
> > Test:
> > Manually tested all the modes using kmsxx python wrapper with and
> > without 'force_premulti' flag enabled.
> >
> > Signed-off-by: Roman Stratiienko <roman.o.stratiienko@globallogic.com>
> > ---
> >  drivers/gpu/drm/sun4i/sun8i_mixer.h    |  2 ++
> >  drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 48 ++++++++++++++++++++-----
> >  drivers/gpu/drm/sun4i/sun8i_ui_layer.h |  5 +++
> >  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 49 ++++++++++++++++++++++----
> >  drivers/gpu/drm/sun4i/sun8i_vi_layer.h |  5 +++
> >  5 files changed, 94 insertions(+), 15 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > b/drivers/gpu/drm/sun4i/sun8i_mixer.h index ebfc276b2464..5c05907e26fb
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > @@ -65,6 +65,8 @@
> >  #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_PREMULTIPLY_EN(pipe)       BIT(pipe)
> > +
> >  #define SUN8I_MIXER_BLEND_OUTCTL_INTERLACED  BIT(1)
> >
> >  #define SUN50I_MIXER_BLEND_CSC_CTL_EN(ch)    BIT(ch)
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c index 6ccbbca3176d..29c0d9cca19a
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > @@ -58,24 +58,46 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer
> > *mixer, int channel, }
> >
> >  static void sun8i_ui_layer_update_alpha(struct sun8i_mixer *mixer, int
> > channel, -                                    int overlay, struct
> drm_plane *plane)
> > +                                     int overlay, struct
> drm_plane *plane,
> > +                                     unsigned int zpos,
> bool force_premulti)
> >  {
> > -     u32 mask, val, ch_base;
> > +     u32 mask, val, ch_base, bld_base;
> > +     bool in_premulti, out_premulti;
> >
> > +     bld_base = sun8i_blender_base(mixer);
> >       ch_base = sun8i_channel_base(mixer, channel);
> >
> > +     in_premulti = plane->state->pixel_blend_mode ==
> DRM_MODE_BLEND_PREMULTI;
> > +     out_premulti = force_premulti || in_premulti;
> > +
> >       mask = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_MASK |
> > -             SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MASK;
> > +            SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MASK |
> > +            SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_MASK;
> >
> >       val = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA(plane->state->alpha >>
> 8);
> >
> > -     val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE) ?
> > -             SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> > -             SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> > +     if (plane->state->pixel_blend_mode == DRM_MODE_BLEND_PIXEL_NONE) {
> > +             val |= SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_LAYER;
> > +     } else {
> > +             val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE)
> ?
> > +
> SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> > +
> SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> > +
> > +             if (in_premulti)
> > +                     val |=
> SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_PREMULTI;
> > +     }
> > +
> > +     if (!in_premulti && out_premulti)
> > +             val |= SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COV2PREM;
> >
> >       regmap_update_bits(mixer->engine.regs,
> >                          SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base,
> overlay),
> >                          mask, val);
> > +
> > +     regmap_update_bits(
> > +             mixer->engine.regs,
> SUN8I_MIXER_BLEND_PREMULTIPLY(bld_base),
> > +             SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos),
> > +             out_premulti ? SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos) :
> 0);
> >  }
> >
> >  static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int
> > channel, @@ -274,7 +296,7 @@ static void
> > sun8i_ui_layer_atomic_update(struct drm_plane *plane,
> > sun8i_ui_layer_update_coord(mixer, layer->channel,
> >                                   layer->overlay, plane, zpos);
> >       sun8i_ui_layer_update_alpha(mixer, layer->channel,
> > -                                 layer->overlay, plane);
> > +                                 layer->overlay, plane, zpos,
> false);
> >       sun8i_ui_layer_update_formats(mixer, layer->channel,
> >                                     layer->overlay, plane);
> >       sun8i_ui_layer_update_buffer(mixer, layer->channel,
> > @@ -332,8 +354,8 @@ 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;
> > +     unsigned int plane_cnt, blend_modes;
> >       struct sun8i_ui_layer *layer;
> > -     unsigned int plane_cnt;
> >       int ret;
> >
> >       layer = devm_kzalloc(drm->dev, sizeof(*layer), GFP_KERNEL);
> > @@ -362,6 +384,16 @@ struct sun8i_ui_layer *sun8i_ui_layer_init_one(struct
> > drm_device *drm, return ERR_PTR(ret);
> >       }
> >
> > +     blend_modes = BIT(DRM_MODE_BLEND_PREMULTI) |
> > +                   BIT(DRM_MODE_BLEND_COVERAGE) |
> > +                   BIT(DRM_MODE_BLEND_PIXEL_NONE);
> > +
> > +     ret = drm_plane_create_blend_mode_property(&layer->plane,
> blend_modes);
> > +     if (ret) {
> > +             dev_err(drm->dev, "Couldn't add blend mode
> property\n");
> > +             return ERR_PTR(ret);
> > +     }
> > +
> >       ret = drm_plane_create_zpos_property(&layer->plane, channel,
> >                                            0, plane_cnt -
> 1);
> >       if (ret) {
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
> > b/drivers/gpu/drm/sun4i/sun8i_ui_layer.h index 43c48cf7bc51..bd3c30e8058d
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
> > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
> > @@ -46,6 +46,11 @@
> >  #define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_LAYER              ((1) << 1)
> >  #define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED   ((2) << 1)
> >
> > +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_MASK    GENMASK(17, 16)
> > +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COVERAGE        ((0) << 16)
> > +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COV2PREM        ((1) << 16)
> > +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_PREMULTI        ((2) << 16)
> > +
> >  struct sun8i_mixer;
> >
> >  struct sun8i_ui_layer {
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index 662ba1018cc4..6581fc7d9668
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > @@ -52,21 +52,38 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer
> > *mixer, int channel, }
> >
> >  static void sun8i_vi_layer_update_alpha(struct sun8i_mixer *mixer, int
> > channel, -                                    int overlay, struct
> drm_plane *plane)
> > +                                     int overlay, struct
> drm_plane *plane,
> > +                                     unsigned int zpos,
> bool force_premulti)
> >  {
> > -     u32 mask, val, ch_base;
> > +     u32 mask, val, ch_base, bld_base;
> > +     bool in_premulti = false, out_premulti = false;
> >
> >       ch_base = sun8i_channel_base(mixer, channel);
> > +     bld_base = sun8i_blender_base(mixer);
> >
> >       if (mixer->cfg->is_de3) {
> > +             in_premulti = plane->state->pixel_blend_mode ==
> DRM_MODE_BLEND_PREMULTI;
> > +             out_premulti = force_premulti || in_premulti;
> > +
> >               mask = SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK |
> > -                    SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_MASK;
> > +                    SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_MASK
> |
> > +                    SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_MASK;
> >               val = SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA
> >                       (plane->state->alpha >> 8);
> >
> > -             val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE)
> ?
> > -
> SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> > -
> SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> > +             if (plane->state->pixel_blend_mode ==
> DRM_MODE_BLEND_PIXEL_NONE) {
> > +                     val |=
> SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_LAYER;
> > +             } else {
> > +                     val |= (plane->state->alpha ==
> DRM_BLEND_ALPHA_OPAQUE) ?
> > +
> SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> > +
> SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> > +
> > +                     if (in_premulti)
> > +                             val |=
> SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_PREMULTI;
> > +             }
> > +
> > +             if (!in_premulti && out_premulti)
> > +                     val |=
> SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COV2PREM;
> >
> >               regmap_update_bits(mixer->engine.regs,
> >
> SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base,
> > @@ -79,6 +96,11 @@ static void sun8i_vi_layer_update_alpha(struct
> > sun8i_mixer *mixer, int channel, SUN8I_MIXER_FCC_GLOBAL_ALPHA
> >                                       (plane->state->alpha
> >> 8));
> >       }
> > +
> > +     regmap_update_bits(
> > +             mixer->engine.regs,
> SUN8I_MIXER_BLEND_PREMULTIPLY(bld_base),
> > +             SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos),
> > +             out_premulti ? SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos) :
> 0);
>
> Shouldn't be above block inside DE3 if block?

No, we have to update the value to non-premulti in case it was set to
premulti by another layer.


Regards,
Roman.

>
> Looks good otherwise.
>
> Best regards,
> Jernej
>
> >  }
> >
> >  static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int
> > channel, @@ -408,7 +430,7 @@ static void
> > sun8i_vi_layer_atomic_update(struct drm_plane *plane,
> > sun8i_vi_layer_update_coord(mixer, layer->channel,
> >                                   layer->overlay, plane, zpos);
> >       sun8i_vi_layer_update_alpha(mixer, layer->channel,
> > -                                 layer->overlay, plane);
> > +                                 layer->overlay, plane, zpos,
> false);
> >       sun8i_vi_layer_update_formats(mixer, layer->channel,
> >                                     layer->overlay, plane);
> >       sun8i_vi_layer_update_buffer(mixer, layer->channel,
> > @@ -563,6 +585,19 @@ struct sun8i_vi_layer *sun8i_vi_layer_init_one(struct
> > drm_device *drm, }
> >       }
> >
> > +     if (mixer->cfg->is_de3) {
> > +             unsigned int blend_modes = BIT(DRM_MODE_BLEND_PREMULTI)
> |
> > +
> BIT(DRM_MODE_BLEND_COVERAGE) |
> > +
> BIT(DRM_MODE_BLEND_PIXEL_NONE);
> > +
> > +             ret = drm_plane_create_blend_mode_property(&layer-
> >plane,
> > +
> blend_modes);
> > +             if (ret) {
> > +                     dev_err(drm->dev, "Couldn't add blend mode
> property\n");
> > +                     return ERR_PTR(ret);
> > +             }
> > +     }
> > +
> >       ret = drm_plane_create_zpos_property(&layer->plane, index,
> >                                            0, plane_cnt -
> 1);
> >       if (ret) {
> > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h index 9939a4cc7a52..ccf91f09f1fe
> > 100644
> > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > @@ -44,6 +44,11 @@
> >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK   GENMASK(31, 24)
> >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA(x)     ((x) << 24)
> >
> > +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_MASK   GENMASK(17, 16)
> > +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COVERAGE       ((0) << 16)
> > +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COV2PREM       ((1) << 16)
> > +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_PREMULTI       ((2) << 16)
> > +
> >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL     ((0) <<
> 1)
> >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_LAYER     ((1) <<
> 1)
> >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED  ((2) << 1)
>
>
>
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property
  2022-06-06  8:17     ` Roman Stratiienko
  (?)
@ 2022-06-06  9:20       ` Maxime Ripard
  -1 siblings, 0 replies; 19+ messages in thread
From: Maxime Ripard @ 2022-06-06  9:20 UTC (permalink / raw)
  To: Roman Stratiienko
  Cc: Jernej Škrabec, wens, airlied, Daniel Vetter,
	Samuel Holland, dri-devel, linux-arm-kernel, linux-sunxi,
	linux-kernel, megi, Roman Stratiienko

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

On Mon, Jun 06, 2022 at 11:17:20AM +0300, Roman Stratiienko wrote:
> Hello Jernej,
> 
> Thank you for having a look.
> 
> вс, 5 июн. 2022 г. в 23:37, Jernej Škrabec <jernej.skrabec@gmail.com>:
> >
> > Dne nedelja, 05. junij 2022 ob 17:47:31 CEST je Roman Stratiienko napisal(a):
> > > Allwinner DE2 and DE3 hardware support 3 pixel blend modes:
> > > "None", "Pre-multiplied", "Coverage"
> > >
> > > Add the blend mode property and route it to the appropriate registers.
> > >
> > > Note:
> > > "force_premulti" parameter was added to handle multi-overlay channel
> > > cases in future changes. It must be set to true for cases when more
> > > than 1 overlay layer is used within a channel and at least one of the
> > > overlay layers within a group uses premultiplied blending mode.
> >
> > Please remove this parameter. It's nothing special, so it can be easily added
> > once it's actually needed. For now, it only complicates code.
> 
> I would prefer keeping it if you do not have any strong opinion against it.
> 
> I am working now on exposing all overlays, so it will be needed soon anyway.

KMS assumes pre-multiplied alpha anyway, so we shouldn't need a
parameter to force it: we're always going to force it.

> Also it helps to better understand the COV2PREMULT mode which has not
> the best description in the datasheet. Only after testing this
> register using devmem I became confident on its purpose.

Sounds like a good job for a comment in the source code.

Maxime

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property
@ 2022-06-06  9:20       ` Maxime Ripard
  0 siblings, 0 replies; 19+ messages in thread
From: Maxime Ripard @ 2022-06-06  9:20 UTC (permalink / raw)
  To: Roman Stratiienko
  Cc: Samuel Holland, airlied, Roman Stratiienko, linux-kernel,
	dri-devel, wens, megi, Jernej Škrabec, linux-sunxi,
	linux-arm-kernel

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

On Mon, Jun 06, 2022 at 11:17:20AM +0300, Roman Stratiienko wrote:
> Hello Jernej,
> 
> Thank you for having a look.
> 
> вс, 5 июн. 2022 г. в 23:37, Jernej Škrabec <jernej.skrabec@gmail.com>:
> >
> > Dne nedelja, 05. junij 2022 ob 17:47:31 CEST je Roman Stratiienko napisal(a):
> > > Allwinner DE2 and DE3 hardware support 3 pixel blend modes:
> > > "None", "Pre-multiplied", "Coverage"
> > >
> > > Add the blend mode property and route it to the appropriate registers.
> > >
> > > Note:
> > > "force_premulti" parameter was added to handle multi-overlay channel
> > > cases in future changes. It must be set to true for cases when more
> > > than 1 overlay layer is used within a channel and at least one of the
> > > overlay layers within a group uses premultiplied blending mode.
> >
> > Please remove this parameter. It's nothing special, so it can be easily added
> > once it's actually needed. For now, it only complicates code.
> 
> I would prefer keeping it if you do not have any strong opinion against it.
> 
> I am working now on exposing all overlays, so it will be needed soon anyway.

KMS assumes pre-multiplied alpha anyway, so we shouldn't need a
parameter to force it: we're always going to force it.

> Also it helps to better understand the COV2PREMULT mode which has not
> the best description in the datasheet. Only after testing this
> register using devmem I became confident on its purpose.

Sounds like a good job for a comment in the source code.

Maxime

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property
@ 2022-06-06  9:20       ` Maxime Ripard
  0 siblings, 0 replies; 19+ messages in thread
From: Maxime Ripard @ 2022-06-06  9:20 UTC (permalink / raw)
  To: Roman Stratiienko
  Cc: Jernej Škrabec, wens, airlied, Daniel Vetter,
	Samuel Holland, dri-devel, linux-arm-kernel, linux-sunxi,
	linux-kernel, megi, Roman Stratiienko


[-- Attachment #1.1: Type: text/plain, Size: 1526 bytes --]

On Mon, Jun 06, 2022 at 11:17:20AM +0300, Roman Stratiienko wrote:
> Hello Jernej,
> 
> Thank you for having a look.
> 
> вс, 5 июн. 2022 г. в 23:37, Jernej Škrabec <jernej.skrabec@gmail.com>:
> >
> > Dne nedelja, 05. junij 2022 ob 17:47:31 CEST je Roman Stratiienko napisal(a):
> > > Allwinner DE2 and DE3 hardware support 3 pixel blend modes:
> > > "None", "Pre-multiplied", "Coverage"
> > >
> > > Add the blend mode property and route it to the appropriate registers.
> > >
> > > Note:
> > > "force_premulti" parameter was added to handle multi-overlay channel
> > > cases in future changes. It must be set to true for cases when more
> > > than 1 overlay layer is used within a channel and at least one of the
> > > overlay layers within a group uses premultiplied blending mode.
> >
> > Please remove this parameter. It's nothing special, so it can be easily added
> > once it's actually needed. For now, it only complicates code.
> 
> I would prefer keeping it if you do not have any strong opinion against it.
> 
> I am working now on exposing all overlays, so it will be needed soon anyway.

KMS assumes pre-multiplied alpha anyway, so we shouldn't need a
parameter to force it: we're always going to force it.

> Also it helps to better understand the COV2PREMULT mode which has not
> the best description in the datasheet. Only after testing this
> register using devmem I became confident on its purpose.

Sounds like a good job for a comment in the source code.

Maxime

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property
  2022-06-06  9:20       ` Maxime Ripard
  (?)
@ 2022-06-06  9:22         ` Maxime Ripard
  -1 siblings, 0 replies; 19+ messages in thread
From: Maxime Ripard @ 2022-06-06  9:22 UTC (permalink / raw)
  To: Roman Stratiienko
  Cc: Jernej Škrabec, wens, airlied, Daniel Vetter,
	Samuel Holland, dri-devel, linux-arm-kernel, linux-sunxi,
	linux-kernel, megi, Roman Stratiienko

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

On Mon, Jun 06, 2022 at 11:20:06AM +0200, Maxime Ripard wrote:
> On Mon, Jun 06, 2022 at 11:17:20AM +0300, Roman Stratiienko wrote:
> > Hello Jernej,
> > 
> > Thank you for having a look.
> > 
> > вс, 5 июн. 2022 г. в 23:37, Jernej Škrabec <jernej.skrabec@gmail.com>:
> > >
> > > Dne nedelja, 05. junij 2022 ob 17:47:31 CEST je Roman Stratiienko napisal(a):
> > > > Allwinner DE2 and DE3 hardware support 3 pixel blend modes:
> > > > "None", "Pre-multiplied", "Coverage"
> > > >
> > > > Add the blend mode property and route it to the appropriate registers.
> > > >
> > > > Note:
> > > > "force_premulti" parameter was added to handle multi-overlay channel
> > > > cases in future changes. It must be set to true for cases when more
> > > > than 1 overlay layer is used within a channel and at least one of the
> > > > overlay layers within a group uses premultiplied blending mode.
> > >
> > > Please remove this parameter. It's nothing special, so it can be easily added
> > > once it's actually needed. For now, it only complicates code.
> > 
> > I would prefer keeping it if you do not have any strong opinion against it.
> > 
> > I am working now on exposing all overlays, so it will be needed soon anyway.
> 
> KMS assumes pre-multiplied alpha anyway, so we shouldn't need a
> parameter to force it: we're always going to force it.

My bad, I got confused with your other patch.

Still, I agree with Jernej, if it's not needed it only complicates the
code for no particular reason. If you need it at some point in the
future, add it then. Otherwise, it's hard to reason about, since we
don't know what are the expectations that those future patches will
bring.

Maxime

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property
@ 2022-06-06  9:22         ` Maxime Ripard
  0 siblings, 0 replies; 19+ messages in thread
From: Maxime Ripard @ 2022-06-06  9:22 UTC (permalink / raw)
  To: Roman Stratiienko
  Cc: Samuel Holland, airlied, Roman Stratiienko, linux-kernel,
	dri-devel, wens, megi, Jernej Škrabec, linux-sunxi,
	linux-arm-kernel

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

On Mon, Jun 06, 2022 at 11:20:06AM +0200, Maxime Ripard wrote:
> On Mon, Jun 06, 2022 at 11:17:20AM +0300, Roman Stratiienko wrote:
> > Hello Jernej,
> > 
> > Thank you for having a look.
> > 
> > вс, 5 июн. 2022 г. в 23:37, Jernej Škrabec <jernej.skrabec@gmail.com>:
> > >
> > > Dne nedelja, 05. junij 2022 ob 17:47:31 CEST je Roman Stratiienko napisal(a):
> > > > Allwinner DE2 and DE3 hardware support 3 pixel blend modes:
> > > > "None", "Pre-multiplied", "Coverage"
> > > >
> > > > Add the blend mode property and route it to the appropriate registers.
> > > >
> > > > Note:
> > > > "force_premulti" parameter was added to handle multi-overlay channel
> > > > cases in future changes. It must be set to true for cases when more
> > > > than 1 overlay layer is used within a channel and at least one of the
> > > > overlay layers within a group uses premultiplied blending mode.
> > >
> > > Please remove this parameter. It's nothing special, so it can be easily added
> > > once it's actually needed. For now, it only complicates code.
> > 
> > I would prefer keeping it if you do not have any strong opinion against it.
> > 
> > I am working now on exposing all overlays, so it will be needed soon anyway.
> 
> KMS assumes pre-multiplied alpha anyway, so we shouldn't need a
> parameter to force it: we're always going to force it.

My bad, I got confused with your other patch.

Still, I agree with Jernej, if it's not needed it only complicates the
code for no particular reason. If you need it at some point in the
future, add it then. Otherwise, it's hard to reason about, since we
don't know what are the expectations that those future patches will
bring.

Maxime

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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property
@ 2022-06-06  9:22         ` Maxime Ripard
  0 siblings, 0 replies; 19+ messages in thread
From: Maxime Ripard @ 2022-06-06  9:22 UTC (permalink / raw)
  To: Roman Stratiienko
  Cc: Jernej Škrabec, wens, airlied, Daniel Vetter,
	Samuel Holland, dri-devel, linux-arm-kernel, linux-sunxi,
	linux-kernel, megi, Roman Stratiienko


[-- Attachment #1.1: Type: text/plain, Size: 1722 bytes --]

On Mon, Jun 06, 2022 at 11:20:06AM +0200, Maxime Ripard wrote:
> On Mon, Jun 06, 2022 at 11:17:20AM +0300, Roman Stratiienko wrote:
> > Hello Jernej,
> > 
> > Thank you for having a look.
> > 
> > вс, 5 июн. 2022 г. в 23:37, Jernej Škrabec <jernej.skrabec@gmail.com>:
> > >
> > > Dne nedelja, 05. junij 2022 ob 17:47:31 CEST je Roman Stratiienko napisal(a):
> > > > Allwinner DE2 and DE3 hardware support 3 pixel blend modes:
> > > > "None", "Pre-multiplied", "Coverage"
> > > >
> > > > Add the blend mode property and route it to the appropriate registers.
> > > >
> > > > Note:
> > > > "force_premulti" parameter was added to handle multi-overlay channel
> > > > cases in future changes. It must be set to true for cases when more
> > > > than 1 overlay layer is used within a channel and at least one of the
> > > > overlay layers within a group uses premultiplied blending mode.
> > >
> > > Please remove this parameter. It's nothing special, so it can be easily added
> > > once it's actually needed. For now, it only complicates code.
> > 
> > I would prefer keeping it if you do not have any strong opinion against it.
> > 
> > I am working now on exposing all overlays, so it will be needed soon anyway.
> 
> KMS assumes pre-multiplied alpha anyway, so we shouldn't need a
> parameter to force it: we're always going to force it.

My bad, I got confused with your other patch.

Still, I agree with Jernej, if it's not needed it only complicates the
code for no particular reason. If you need it at some point in the
future, add it then. Otherwise, it's hard to reason about, since we
don't know what are the expectations that those future patches will
bring.

Maxime

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

[-- Attachment #2: Type: text/plain, Size: 176 bytes --]

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property
  2022-06-06  9:22         ` Maxime Ripard
  (?)
@ 2022-06-06 11:21           ` Roman Stratiienko
  -1 siblings, 0 replies; 19+ messages in thread
From: Roman Stratiienko @ 2022-06-06 11:21 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: Jernej Škrabec, wens, airlied, Daniel Vetter,
	Samuel Holland, dri-devel, linux-arm-kernel, linux-sunxi,
	linux-kernel, megi, Roman Stratiienko

Hi Maxime,

пн, 6 июн. 2022 г. в 12:22, Maxime Ripard <maxime@cerno.tech>:
>
> On Mon, Jun 06, 2022 at 11:20:06AM +0200, Maxime Ripard wrote:
> > On Mon, Jun 06, 2022 at 11:17:20AM +0300, Roman Stratiienko wrote:
> > > Hello Jernej,
> > >
> > > Thank you for having a look.
> > >
> > > вс, 5 июн. 2022 г. в 23:37, Jernej Škrabec <jernej.skrabec@gmail.com>:
> > > >
> > > > Dne nedelja, 05. junij 2022 ob 17:47:31 CEST je Roman Stratiienko napisal(a):
> > > > > Allwinner DE2 and DE3 hardware support 3 pixel blend modes:
> > > > > "None", "Pre-multiplied", "Coverage"
> > > > >
> > > > > Add the blend mode property and route it to the appropriate registers.
> > > > >
> > > > > Note:
> > > > > "force_premulti" parameter was added to handle multi-overlay channel
> > > > > cases in future changes. It must be set to true for cases when more
> > > > > than 1 overlay layer is used within a channel and at least one of the
> > > > > overlay layers within a group uses premultiplied blending mode.
> > > >
> > > > Please remove this parameter. It's nothing special, so it can be easily added
> > > > once it's actually needed. For now, it only complicates code.
> > >
> > > I would prefer keeping it if you do not have any strong opinion against it.
> > >
> > > I am working now on exposing all overlays, so it will be needed soon anyway.
> >
> > KMS assumes pre-multiplied alpha anyway, so we shouldn't need a
> > parameter to force it: we're always going to force it.
>
> My bad, I got confused with your other patch.
>
> Still, I agree with Jernej, if it's not needed it only complicates the
> code for no particular reason. If you need it at some point in the
> future, add it then. Otherwise, it's hard to reason about, since we
> don't know what are the expectations that those future patches will
> bring.

Well, existing code already has some sort of support for the sub-overlays:
For example 'int overlay, ' parameter is always 0, but it still passed through
the call stack. Therefore this patch is just aligned with already existing
traditions to keep sub-overlays in mind.

>
> Maxime

Regards,
Roman

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property
@ 2022-06-06 11:21           ` Roman Stratiienko
  0 siblings, 0 replies; 19+ messages in thread
From: Roman Stratiienko @ 2022-06-06 11:21 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: Samuel Holland, airlied, Roman Stratiienko, linux-kernel,
	dri-devel, wens, megi, Jernej Škrabec, linux-sunxi,
	linux-arm-kernel

Hi Maxime,

пн, 6 июн. 2022 г. в 12:22, Maxime Ripard <maxime@cerno.tech>:
>
> On Mon, Jun 06, 2022 at 11:20:06AM +0200, Maxime Ripard wrote:
> > On Mon, Jun 06, 2022 at 11:17:20AM +0300, Roman Stratiienko wrote:
> > > Hello Jernej,
> > >
> > > Thank you for having a look.
> > >
> > > вс, 5 июн. 2022 г. в 23:37, Jernej Škrabec <jernej.skrabec@gmail.com>:
> > > >
> > > > Dne nedelja, 05. junij 2022 ob 17:47:31 CEST je Roman Stratiienko napisal(a):
> > > > > Allwinner DE2 and DE3 hardware support 3 pixel blend modes:
> > > > > "None", "Pre-multiplied", "Coverage"
> > > > >
> > > > > Add the blend mode property and route it to the appropriate registers.
> > > > >
> > > > > Note:
> > > > > "force_premulti" parameter was added to handle multi-overlay channel
> > > > > cases in future changes. It must be set to true for cases when more
> > > > > than 1 overlay layer is used within a channel and at least one of the
> > > > > overlay layers within a group uses premultiplied blending mode.
> > > >
> > > > Please remove this parameter. It's nothing special, so it can be easily added
> > > > once it's actually needed. For now, it only complicates code.
> > >
> > > I would prefer keeping it if you do not have any strong opinion against it.
> > >
> > > I am working now on exposing all overlays, so it will be needed soon anyway.
> >
> > KMS assumes pre-multiplied alpha anyway, so we shouldn't need a
> > parameter to force it: we're always going to force it.
>
> My bad, I got confused with your other patch.
>
> Still, I agree with Jernej, if it's not needed it only complicates the
> code for no particular reason. If you need it at some point in the
> future, add it then. Otherwise, it's hard to reason about, since we
> don't know what are the expectations that those future patches will
> bring.

Well, existing code already has some sort of support for the sub-overlays:
For example 'int overlay, ' parameter is always 0, but it still passed through
the call stack. Therefore this patch is just aligned with already existing
traditions to keep sub-overlays in mind.

>
> Maxime

Regards,
Roman

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property
@ 2022-06-06 11:21           ` Roman Stratiienko
  0 siblings, 0 replies; 19+ messages in thread
From: Roman Stratiienko @ 2022-06-06 11:21 UTC (permalink / raw)
  To: Maxime Ripard
  Cc: Jernej Škrabec, wens, airlied, Daniel Vetter,
	Samuel Holland, dri-devel, linux-arm-kernel, linux-sunxi,
	linux-kernel, megi, Roman Stratiienko

Hi Maxime,

пн, 6 июн. 2022 г. в 12:22, Maxime Ripard <maxime@cerno.tech>:
>
> On Mon, Jun 06, 2022 at 11:20:06AM +0200, Maxime Ripard wrote:
> > On Mon, Jun 06, 2022 at 11:17:20AM +0300, Roman Stratiienko wrote:
> > > Hello Jernej,
> > >
> > > Thank you for having a look.
> > >
> > > вс, 5 июн. 2022 г. в 23:37, Jernej Škrabec <jernej.skrabec@gmail.com>:
> > > >
> > > > Dne nedelja, 05. junij 2022 ob 17:47:31 CEST je Roman Stratiienko napisal(a):
> > > > > Allwinner DE2 and DE3 hardware support 3 pixel blend modes:
> > > > > "None", "Pre-multiplied", "Coverage"
> > > > >
> > > > > Add the blend mode property and route it to the appropriate registers.
> > > > >
> > > > > Note:
> > > > > "force_premulti" parameter was added to handle multi-overlay channel
> > > > > cases in future changes. It must be set to true for cases when more
> > > > > than 1 overlay layer is used within a channel and at least one of the
> > > > > overlay layers within a group uses premultiplied blending mode.
> > > >
> > > > Please remove this parameter. It's nothing special, so it can be easily added
> > > > once it's actually needed. For now, it only complicates code.
> > >
> > > I would prefer keeping it if you do not have any strong opinion against it.
> > >
> > > I am working now on exposing all overlays, so it will be needed soon anyway.
> >
> > KMS assumes pre-multiplied alpha anyway, so we shouldn't need a
> > parameter to force it: we're always going to force it.
>
> My bad, I got confused with your other patch.
>
> Still, I agree with Jernej, if it's not needed it only complicates the
> code for no particular reason. If you need it at some point in the
> future, add it then. Otherwise, it's hard to reason about, since we
> don't know what are the expectations that those future patches will
> bring.

Well, existing code already has some sort of support for the sub-overlays:
For example 'int overlay, ' parameter is always 0, but it still passed through
the call stack. Therefore this patch is just aligned with already existing
traditions to keep sub-overlays in mind.

>
> Maxime

Regards,
Roman

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Re: [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property
  2022-06-06  8:17     ` Roman Stratiienko
  (?)
@ 2022-06-06 21:27       ` Jernej Škrabec
  -1 siblings, 0 replies; 19+ messages in thread
From: Jernej Škrabec @ 2022-06-06 21:27 UTC (permalink / raw)
  To: Roman Stratiienko
  Cc: mripard, wens, airlied, Daniel Vetter, Samuel Holland, dri-devel,
	linux-arm-kernel, linux-sunxi, linux-kernel, megi,
	Roman Stratiienko

Dne ponedeljek, 06. junij 2022 ob 10:17:20 CEST je Roman Stratiienko 
napisal(a):
> Hello Jernej,
> 
> Thank you for having a look.
> 
> вс, 5 июн. 2022 г. в 23:37, Jernej Škrabec <jernej.skrabec@gmail.com>:
> >
> > Dne nedelja, 05. junij 2022 ob 17:47:31 CEST je Roman Stratiienko 
napisal(a):
> > > Allwinner DE2 and DE3 hardware support 3 pixel blend modes:
> > > "None", "Pre-multiplied", "Coverage"
> > >
> > > Add the blend mode property and route it to the appropriate registers.
> > >
> > > Note:
> > > "force_premulti" parameter was added to handle multi-overlay channel
> > > cases in future changes. It must be set to true for cases when more
> > > than 1 overlay layer is used within a channel and at least one of the
> > > overlay layers within a group uses premultiplied blending mode.
> >
> > Please remove this parameter. It's nothing special, so it can be easily 
added
> > once it's actually needed. For now, it only complicates code.
> 
> I would prefer keeping it if you do not have any strong opinion against it.

Actually I do. Patch will be smaller and easier to follow if there is no extra 
variables with fixed values in it.

> 
> I am working now on exposing all overlays, so it will be needed soon anyway.

Well, it will just be one patch more there, if at all.

Regards,
Jernej

> 
> Also it helps to better understand the COV2PREMULT mode which has not
> the best description in the datasheet. Only after testing this
> register using devmem I became confident on its purpose.
> 
> >
> > >
> > > Test:
> > > Manually tested all the modes using kmsxx python wrapper with and
> > > without 'force_premulti' flag enabled.
> > >
> > > Signed-off-by: Roman Stratiienko <roman.o.stratiienko@globallogic.com>
> > > ---
> > >  drivers/gpu/drm/sun4i/sun8i_mixer.h    |  2 ++
> > >  drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 48 ++++++++++++++++++++-----
> > >  drivers/gpu/drm/sun4i/sun8i_ui_layer.h |  5 +++
> > >  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 49 ++++++++++++++++++++++----
> > >  drivers/gpu/drm/sun4i/sun8i_vi_layer.h |  5 +++
> > >  5 files changed, 94 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > b/drivers/gpu/drm/sun4i/sun8i_mixer.h index ebfc276b2464..5c05907e26fb
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > @@ -65,6 +65,8 @@
> > >  #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_PREMULTIPLY_EN(pipe)       BIT(pipe)
> > > +
> > >  #define SUN8I_MIXER_BLEND_OUTCTL_INTERLACED  BIT(1)
> > >
> > >  #define SUN50I_MIXER_BLEND_CSC_CTL_EN(ch)    BIT(ch)
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c index 6ccbbca3176d..
29c0d9cca19a
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > @@ -58,24 +58,46 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer
> > > *mixer, int channel, }
> > >
> > >  static void sun8i_ui_layer_update_alpha(struct sun8i_mixer *mixer, int
> > > channel, -                                    int overlay, struct
> > drm_plane *plane)
> > > +                                     int overlay, struct
> > drm_plane *plane,
> > > +                                     unsigned int zpos,
> > bool force_premulti)
> > >  {
> > > -     u32 mask, val, ch_base;
> > > +     u32 mask, val, ch_base, bld_base;
> > > +     bool in_premulti, out_premulti;
> > >
> > > +     bld_base = sun8i_blender_base(mixer);
> > >       ch_base = sun8i_channel_base(mixer, channel);
> > >
> > > +     in_premulti = plane->state->pixel_blend_mode ==
> > DRM_MODE_BLEND_PREMULTI;
> > > +     out_premulti = force_premulti || in_premulti;
> > > +
> > >       mask = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_MASK |
> > > -             SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MASK;
> > > +            SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MASK |
> > > +            SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_MASK;
> > >
> > >       val = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA(plane->state->alpha >>
> > 8);
> > >
> > > -     val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE) ?
> > > -             SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> > > -             SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> > > +     if (plane->state->pixel_blend_mode == DRM_MODE_BLEND_PIXEL_NONE) {
> > > +             val |= SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_LAYER;
> > > +     } else {
> > > +             val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE)
> > ?
> > > +
> > SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> > > +
> > SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> > > +
> > > +             if (in_premulti)
> > > +                     val |=
> > SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_PREMULTI;
> > > +     }
> > > +
> > > +     if (!in_premulti && out_premulti)
> > > +             val |= SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COV2PREM;
> > >
> > >       regmap_update_bits(mixer->engine.regs,
> > >                          SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base,
> > overlay),
> > >                          mask, val);
> > > +
> > > +     regmap_update_bits(
> > > +             mixer->engine.regs,
> > SUN8I_MIXER_BLEND_PREMULTIPLY(bld_base),
> > > +             SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos),
> > > +             out_premulti ? SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos) :
> > 0);
> > >  }
> > >
> > >  static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int
> > > channel, @@ -274,7 +296,7 @@ static void
> > > sun8i_ui_layer_atomic_update(struct drm_plane *plane,
> > > sun8i_ui_layer_update_coord(mixer, layer->channel,
> > >                                   layer->overlay, plane, zpos);
> > >       sun8i_ui_layer_update_alpha(mixer, layer->channel,
> > > -                                 layer->overlay, plane);
> > > +                                 layer->overlay, plane, zpos,
> > false);
> > >       sun8i_ui_layer_update_formats(mixer, layer->channel,
> > >                                     layer->overlay, plane);
> > >       sun8i_ui_layer_update_buffer(mixer, layer->channel,
> > > @@ -332,8 +354,8 @@ 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;
> > > +     unsigned int plane_cnt, blend_modes;
> > >       struct sun8i_ui_layer *layer;
> > > -     unsigned int plane_cnt;
> > >       int ret;
> > >
> > >       layer = devm_kzalloc(drm->dev, sizeof(*layer), GFP_KERNEL);
> > > @@ -362,6 +384,16 @@ struct sun8i_ui_layer 
*sun8i_ui_layer_init_one(struct
> > > drm_device *drm, return ERR_PTR(ret);
> > >       }
> > >
> > > +     blend_modes = BIT(DRM_MODE_BLEND_PREMULTI) |
> > > +                   BIT(DRM_MODE_BLEND_COVERAGE) |
> > > +                   BIT(DRM_MODE_BLEND_PIXEL_NONE);
> > > +
> > > +     ret = drm_plane_create_blend_mode_property(&layer->plane,
> > blend_modes);
> > > +     if (ret) {
> > > +             dev_err(drm->dev, "Couldn't add blend mode
> > property\n");
> > > +             return ERR_PTR(ret);
> > > +     }
> > > +
> > >       ret = drm_plane_create_zpos_property(&layer->plane, channel,
> > >                                            0, plane_cnt -
> > 1);
> > >       if (ret) {
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
> > > b/drivers/gpu/drm/sun4i/sun8i_ui_layer.h index 
43c48cf7bc51..bd3c30e8058d
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
> > > @@ -46,6 +46,11 @@
> > >  #define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_LAYER              
((1) << 1)
> > >  #define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED   ((2) << 1)
> > >
> > > +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_MASK    GENMASK(17, 16)
> > > +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COVERAGE        ((0) << 16)
> > > +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COV2PREM        ((1) << 16)
> > > +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_PREMULTI        ((2) << 16)
> > > +
> > >  struct sun8i_mixer;
> > >
> > >  struct sun8i_ui_layer {
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index 
662ba1018cc4..6581fc7d9668
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > @@ -52,21 +52,38 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer
> > > *mixer, int channel, }
> > >
> > >  static void sun8i_vi_layer_update_alpha(struct sun8i_mixer *mixer, int
> > > channel, -                                    int overlay, struct
> > drm_plane *plane)
> > > +                                     int overlay, struct
> > drm_plane *plane,
> > > +                                     unsigned int zpos,
> > bool force_premulti)
> > >  {
> > > -     u32 mask, val, ch_base;
> > > +     u32 mask, val, ch_base, bld_base;
> > > +     bool in_premulti = false, out_premulti = false;
> > >
> > >       ch_base = sun8i_channel_base(mixer, channel);
> > > +     bld_base = sun8i_blender_base(mixer);
> > >
> > >       if (mixer->cfg->is_de3) {
> > > +             in_premulti = plane->state->pixel_blend_mode ==
> > DRM_MODE_BLEND_PREMULTI;
> > > +             out_premulti = force_premulti || in_premulti;
> > > +
> > >               mask = SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK |
> > > -                    SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_MASK;
> > > +                    SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_MASK
> > |
> > > +                    SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_MASK;
> > >               val = SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA
> > >                       (plane->state->alpha >> 8);
> > >
> > > -             val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE)
> > ?
> > > -
> > SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> > > -
> > SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> > > +             if (plane->state->pixel_blend_mode ==
> > DRM_MODE_BLEND_PIXEL_NONE) {
> > > +                     val |=
> > SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_LAYER;
> > > +             } else {
> > > +                     val |= (plane->state->alpha ==
> > DRM_BLEND_ALPHA_OPAQUE) ?
> > > +
> > SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> > > +
> > SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> > > +
> > > +                     if (in_premulti)
> > > +                             val |=
> > SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_PREMULTI;
> > > +             }
> > > +
> > > +             if (!in_premulti && out_premulti)
> > > +                     val |=
> > SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COV2PREM;
> > >
> > >               regmap_update_bits(mixer->engine.regs,
> > >
> > SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base,
> > > @@ -79,6 +96,11 @@ static void sun8i_vi_layer_update_alpha(struct
> > > sun8i_mixer *mixer, int channel, SUN8I_MIXER_FCC_GLOBAL_ALPHA
> > >                                       (plane->state->alpha
> > >> 8));
> > >       }
> > > +
> > > +     regmap_update_bits(
> > > +             mixer->engine.regs,
> > SUN8I_MIXER_BLEND_PREMULTIPLY(bld_base),
> > > +             SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos),
> > > +             out_premulti ? SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos) :
> > 0);
> >
> > Shouldn't be above block inside DE3 if block?
> 
> No, we have to update the value to non-premulti in case it was set to
> premulti by another layer.
> 
> 
> Regards,
> Roman.
> 
> >
> > Looks good otherwise.
> >
> > Best regards,
> > Jernej
> >
> > >  }
> > >
> > >  static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int
> > > channel, @@ -408,7 +430,7 @@ static void
> > > sun8i_vi_layer_atomic_update(struct drm_plane *plane,
> > > sun8i_vi_layer_update_coord(mixer, layer->channel,
> > >                                   layer->overlay, plane, zpos);
> > >       sun8i_vi_layer_update_alpha(mixer, layer->channel,
> > > -                                 layer->overlay, plane);
> > > +                                 layer->overlay, plane, zpos,
> > false);
> > >       sun8i_vi_layer_update_formats(mixer, layer->channel,
> > >                                     layer->overlay, plane);
> > >       sun8i_vi_layer_update_buffer(mixer, layer->channel,
> > > @@ -563,6 +585,19 @@ struct sun8i_vi_layer 
*sun8i_vi_layer_init_one(struct
> > > drm_device *drm, }
> > >       }
> > >
> > > +     if (mixer->cfg->is_de3) {
> > > +             unsigned int blend_modes = BIT(DRM_MODE_BLEND_PREMULTI)
> > |
> > > +
> > BIT(DRM_MODE_BLEND_COVERAGE) |
> > > +
> > BIT(DRM_MODE_BLEND_PIXEL_NONE);
> > > +
> > > +             ret = drm_plane_create_blend_mode_property(&layer-
> > >plane,
> > > +
> > blend_modes);
> > > +             if (ret) {
> > > +                     dev_err(drm->dev, "Couldn't add blend mode
> > property\n");
> > > +                     return ERR_PTR(ret);
> > > +             }
> > > +     }
> > > +
> > >       ret = drm_plane_create_zpos_property(&layer->plane, index,
> > >                                            0, plane_cnt -
> > 1);
> > >       if (ret) {
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h index 
9939a4cc7a52..ccf91f09f1fe
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > > @@ -44,6 +44,11 @@
> > >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK   GENMASK(31, 24)
> > >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA(x)     ((x) << 24)
> > >
> > > +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_MASK   GENMASK(17, 16)
> > > +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COVERAGE       ((0) << 16)
> > > +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COV2PREM       ((1) << 16)
> > > +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_PREMULTI       ((2) << 16)
> > > +
> > >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL     ((0) <<
> > 1)
> > >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_LAYER     ((1) <<
> > 1)
> > >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED  ((2) << 1)
> >
> >
> >
> >
> 



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Re: [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property
@ 2022-06-06 21:27       ` Jernej Škrabec
  0 siblings, 0 replies; 19+ messages in thread
From: Jernej Škrabec @ 2022-06-06 21:27 UTC (permalink / raw)
  To: Roman Stratiienko
  Cc: Samuel Holland, airlied, Roman Stratiienko, linux-kernel,
	dri-devel, wens, megi, linux-sunxi, linux-arm-kernel

Dne ponedeljek, 06. junij 2022 ob 10:17:20 CEST je Roman Stratiienko 
napisal(a):
> Hello Jernej,
> 
> Thank you for having a look.
> 
> вс, 5 июн. 2022 г. в 23:37, Jernej Škrabec <jernej.skrabec@gmail.com>:
> >
> > Dne nedelja, 05. junij 2022 ob 17:47:31 CEST je Roman Stratiienko 
napisal(a):
> > > Allwinner DE2 and DE3 hardware support 3 pixel blend modes:
> > > "None", "Pre-multiplied", "Coverage"
> > >
> > > Add the blend mode property and route it to the appropriate registers.
> > >
> > > Note:
> > > "force_premulti" parameter was added to handle multi-overlay channel
> > > cases in future changes. It must be set to true for cases when more
> > > than 1 overlay layer is used within a channel and at least one of the
> > > overlay layers within a group uses premultiplied blending mode.
> >
> > Please remove this parameter. It's nothing special, so it can be easily 
added
> > once it's actually needed. For now, it only complicates code.
> 
> I would prefer keeping it if you do not have any strong opinion against it.

Actually I do. Patch will be smaller and easier to follow if there is no extra 
variables with fixed values in it.

> 
> I am working now on exposing all overlays, so it will be needed soon anyway.

Well, it will just be one patch more there, if at all.

Regards,
Jernej

> 
> Also it helps to better understand the COV2PREMULT mode which has not
> the best description in the datasheet. Only after testing this
> register using devmem I became confident on its purpose.
> 
> >
> > >
> > > Test:
> > > Manually tested all the modes using kmsxx python wrapper with and
> > > without 'force_premulti' flag enabled.
> > >
> > > Signed-off-by: Roman Stratiienko <roman.o.stratiienko@globallogic.com>
> > > ---
> > >  drivers/gpu/drm/sun4i/sun8i_mixer.h    |  2 ++
> > >  drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 48 ++++++++++++++++++++-----
> > >  drivers/gpu/drm/sun4i/sun8i_ui_layer.h |  5 +++
> > >  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 49 ++++++++++++++++++++++----
> > >  drivers/gpu/drm/sun4i/sun8i_vi_layer.h |  5 +++
> > >  5 files changed, 94 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > b/drivers/gpu/drm/sun4i/sun8i_mixer.h index ebfc276b2464..5c05907e26fb
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > @@ -65,6 +65,8 @@
> > >  #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_PREMULTIPLY_EN(pipe)       BIT(pipe)
> > > +
> > >  #define SUN8I_MIXER_BLEND_OUTCTL_INTERLACED  BIT(1)
> > >
> > >  #define SUN50I_MIXER_BLEND_CSC_CTL_EN(ch)    BIT(ch)
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c index 6ccbbca3176d..
29c0d9cca19a
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > @@ -58,24 +58,46 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer
> > > *mixer, int channel, }
> > >
> > >  static void sun8i_ui_layer_update_alpha(struct sun8i_mixer *mixer, int
> > > channel, -                                    int overlay, struct
> > drm_plane *plane)
> > > +                                     int overlay, struct
> > drm_plane *plane,
> > > +                                     unsigned int zpos,
> > bool force_premulti)
> > >  {
> > > -     u32 mask, val, ch_base;
> > > +     u32 mask, val, ch_base, bld_base;
> > > +     bool in_premulti, out_premulti;
> > >
> > > +     bld_base = sun8i_blender_base(mixer);
> > >       ch_base = sun8i_channel_base(mixer, channel);
> > >
> > > +     in_premulti = plane->state->pixel_blend_mode ==
> > DRM_MODE_BLEND_PREMULTI;
> > > +     out_premulti = force_premulti || in_premulti;
> > > +
> > >       mask = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_MASK |
> > > -             SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MASK;
> > > +            SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MASK |
> > > +            SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_MASK;
> > >
> > >       val = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA(plane->state->alpha >>
> > 8);
> > >
> > > -     val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE) ?
> > > -             SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> > > -             SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> > > +     if (plane->state->pixel_blend_mode == DRM_MODE_BLEND_PIXEL_NONE) {
> > > +             val |= SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_LAYER;
> > > +     } else {
> > > +             val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE)
> > ?
> > > +
> > SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> > > +
> > SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> > > +
> > > +             if (in_premulti)
> > > +                     val |=
> > SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_PREMULTI;
> > > +     }
> > > +
> > > +     if (!in_premulti && out_premulti)
> > > +             val |= SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COV2PREM;
> > >
> > >       regmap_update_bits(mixer->engine.regs,
> > >                          SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base,
> > overlay),
> > >                          mask, val);
> > > +
> > > +     regmap_update_bits(
> > > +             mixer->engine.regs,
> > SUN8I_MIXER_BLEND_PREMULTIPLY(bld_base),
> > > +             SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos),
> > > +             out_premulti ? SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos) :
> > 0);
> > >  }
> > >
> > >  static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int
> > > channel, @@ -274,7 +296,7 @@ static void
> > > sun8i_ui_layer_atomic_update(struct drm_plane *plane,
> > > sun8i_ui_layer_update_coord(mixer, layer->channel,
> > >                                   layer->overlay, plane, zpos);
> > >       sun8i_ui_layer_update_alpha(mixer, layer->channel,
> > > -                                 layer->overlay, plane);
> > > +                                 layer->overlay, plane, zpos,
> > false);
> > >       sun8i_ui_layer_update_formats(mixer, layer->channel,
> > >                                     layer->overlay, plane);
> > >       sun8i_ui_layer_update_buffer(mixer, layer->channel,
> > > @@ -332,8 +354,8 @@ 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;
> > > +     unsigned int plane_cnt, blend_modes;
> > >       struct sun8i_ui_layer *layer;
> > > -     unsigned int plane_cnt;
> > >       int ret;
> > >
> > >       layer = devm_kzalloc(drm->dev, sizeof(*layer), GFP_KERNEL);
> > > @@ -362,6 +384,16 @@ struct sun8i_ui_layer 
*sun8i_ui_layer_init_one(struct
> > > drm_device *drm, return ERR_PTR(ret);
> > >       }
> > >
> > > +     blend_modes = BIT(DRM_MODE_BLEND_PREMULTI) |
> > > +                   BIT(DRM_MODE_BLEND_COVERAGE) |
> > > +                   BIT(DRM_MODE_BLEND_PIXEL_NONE);
> > > +
> > > +     ret = drm_plane_create_blend_mode_property(&layer->plane,
> > blend_modes);
> > > +     if (ret) {
> > > +             dev_err(drm->dev, "Couldn't add blend mode
> > property\n");
> > > +             return ERR_PTR(ret);
> > > +     }
> > > +
> > >       ret = drm_plane_create_zpos_property(&layer->plane, channel,
> > >                                            0, plane_cnt -
> > 1);
> > >       if (ret) {
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
> > > b/drivers/gpu/drm/sun4i/sun8i_ui_layer.h index 
43c48cf7bc51..bd3c30e8058d
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
> > > @@ -46,6 +46,11 @@
> > >  #define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_LAYER              
((1) << 1)
> > >  #define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED   ((2) << 1)
> > >
> > > +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_MASK    GENMASK(17, 16)
> > > +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COVERAGE        ((0) << 16)
> > > +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COV2PREM        ((1) << 16)
> > > +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_PREMULTI        ((2) << 16)
> > > +
> > >  struct sun8i_mixer;
> > >
> > >  struct sun8i_ui_layer {
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index 
662ba1018cc4..6581fc7d9668
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > @@ -52,21 +52,38 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer
> > > *mixer, int channel, }
> > >
> > >  static void sun8i_vi_layer_update_alpha(struct sun8i_mixer *mixer, int
> > > channel, -                                    int overlay, struct
> > drm_plane *plane)
> > > +                                     int overlay, struct
> > drm_plane *plane,
> > > +                                     unsigned int zpos,
> > bool force_premulti)
> > >  {
> > > -     u32 mask, val, ch_base;
> > > +     u32 mask, val, ch_base, bld_base;
> > > +     bool in_premulti = false, out_premulti = false;
> > >
> > >       ch_base = sun8i_channel_base(mixer, channel);
> > > +     bld_base = sun8i_blender_base(mixer);
> > >
> > >       if (mixer->cfg->is_de3) {
> > > +             in_premulti = plane->state->pixel_blend_mode ==
> > DRM_MODE_BLEND_PREMULTI;
> > > +             out_premulti = force_premulti || in_premulti;
> > > +
> > >               mask = SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK |
> > > -                    SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_MASK;
> > > +                    SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_MASK
> > |
> > > +                    SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_MASK;
> > >               val = SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA
> > >                       (plane->state->alpha >> 8);
> > >
> > > -             val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE)
> > ?
> > > -
> > SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> > > -
> > SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> > > +             if (plane->state->pixel_blend_mode ==
> > DRM_MODE_BLEND_PIXEL_NONE) {
> > > +                     val |=
> > SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_LAYER;
> > > +             } else {
> > > +                     val |= (plane->state->alpha ==
> > DRM_BLEND_ALPHA_OPAQUE) ?
> > > +
> > SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> > > +
> > SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> > > +
> > > +                     if (in_premulti)
> > > +                             val |=
> > SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_PREMULTI;
> > > +             }
> > > +
> > > +             if (!in_premulti && out_premulti)
> > > +                     val |=
> > SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COV2PREM;
> > >
> > >               regmap_update_bits(mixer->engine.regs,
> > >
> > SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base,
> > > @@ -79,6 +96,11 @@ static void sun8i_vi_layer_update_alpha(struct
> > > sun8i_mixer *mixer, int channel, SUN8I_MIXER_FCC_GLOBAL_ALPHA
> > >                                       (plane->state->alpha
> > >> 8));
> > >       }
> > > +
> > > +     regmap_update_bits(
> > > +             mixer->engine.regs,
> > SUN8I_MIXER_BLEND_PREMULTIPLY(bld_base),
> > > +             SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos),
> > > +             out_premulti ? SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos) :
> > 0);
> >
> > Shouldn't be above block inside DE3 if block?
> 
> No, we have to update the value to non-premulti in case it was set to
> premulti by another layer.
> 
> 
> Regards,
> Roman.
> 
> >
> > Looks good otherwise.
> >
> > Best regards,
> > Jernej
> >
> > >  }
> > >
> > >  static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int
> > > channel, @@ -408,7 +430,7 @@ static void
> > > sun8i_vi_layer_atomic_update(struct drm_plane *plane,
> > > sun8i_vi_layer_update_coord(mixer, layer->channel,
> > >                                   layer->overlay, plane, zpos);
> > >       sun8i_vi_layer_update_alpha(mixer, layer->channel,
> > > -                                 layer->overlay, plane);
> > > +                                 layer->overlay, plane, zpos,
> > false);
> > >       sun8i_vi_layer_update_formats(mixer, layer->channel,
> > >                                     layer->overlay, plane);
> > >       sun8i_vi_layer_update_buffer(mixer, layer->channel,
> > > @@ -563,6 +585,19 @@ struct sun8i_vi_layer 
*sun8i_vi_layer_init_one(struct
> > > drm_device *drm, }
> > >       }
> > >
> > > +     if (mixer->cfg->is_de3) {
> > > +             unsigned int blend_modes = BIT(DRM_MODE_BLEND_PREMULTI)
> > |
> > > +
> > BIT(DRM_MODE_BLEND_COVERAGE) |
> > > +
> > BIT(DRM_MODE_BLEND_PIXEL_NONE);
> > > +
> > > +             ret = drm_plane_create_blend_mode_property(&layer-
> > >plane,
> > > +
> > blend_modes);
> > > +             if (ret) {
> > > +                     dev_err(drm->dev, "Couldn't add blend mode
> > property\n");
> > > +                     return ERR_PTR(ret);
> > > +             }
> > > +     }
> > > +
> > >       ret = drm_plane_create_zpos_property(&layer->plane, index,
> > >                                            0, plane_cnt -
> > 1);
> > >       if (ret) {
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h index 
9939a4cc7a52..ccf91f09f1fe
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > > @@ -44,6 +44,11 @@
> > >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK   GENMASK(31, 24)
> > >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA(x)     ((x) << 24)
> > >
> > > +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_MASK   GENMASK(17, 16)
> > > +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COVERAGE       ((0) << 16)
> > > +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COV2PREM       ((1) << 16)
> > > +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_PREMULTI       ((2) << 16)
> > > +
> > >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL     ((0) <<
> > 1)
> > >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_LAYER     ((1) <<
> > 1)
> > >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED  ((2) << 1)
> >
> >
> >
> >
> 



^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: Re: [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property
@ 2022-06-06 21:27       ` Jernej Škrabec
  0 siblings, 0 replies; 19+ messages in thread
From: Jernej Škrabec @ 2022-06-06 21:27 UTC (permalink / raw)
  To: Roman Stratiienko
  Cc: mripard, wens, airlied, Daniel Vetter, Samuel Holland, dri-devel,
	linux-arm-kernel, linux-sunxi, linux-kernel, megi,
	Roman Stratiienko

Dne ponedeljek, 06. junij 2022 ob 10:17:20 CEST je Roman Stratiienko 
napisal(a):
> Hello Jernej,
> 
> Thank you for having a look.
> 
> вс, 5 июн. 2022 г. в 23:37, Jernej Škrabec <jernej.skrabec@gmail.com>:
> >
> > Dne nedelja, 05. junij 2022 ob 17:47:31 CEST je Roman Stratiienko 
napisal(a):
> > > Allwinner DE2 and DE3 hardware support 3 pixel blend modes:
> > > "None", "Pre-multiplied", "Coverage"
> > >
> > > Add the blend mode property and route it to the appropriate registers.
> > >
> > > Note:
> > > "force_premulti" parameter was added to handle multi-overlay channel
> > > cases in future changes. It must be set to true for cases when more
> > > than 1 overlay layer is used within a channel and at least one of the
> > > overlay layers within a group uses premultiplied blending mode.
> >
> > Please remove this parameter. It's nothing special, so it can be easily 
added
> > once it's actually needed. For now, it only complicates code.
> 
> I would prefer keeping it if you do not have any strong opinion against it.

Actually I do. Patch will be smaller and easier to follow if there is no extra 
variables with fixed values in it.

> 
> I am working now on exposing all overlays, so it will be needed soon anyway.

Well, it will just be one patch more there, if at all.

Regards,
Jernej

> 
> Also it helps to better understand the COV2PREMULT mode which has not
> the best description in the datasheet. Only after testing this
> register using devmem I became confident on its purpose.
> 
> >
> > >
> > > Test:
> > > Manually tested all the modes using kmsxx python wrapper with and
> > > without 'force_premulti' flag enabled.
> > >
> > > Signed-off-by: Roman Stratiienko <roman.o.stratiienko@globallogic.com>
> > > ---
> > >  drivers/gpu/drm/sun4i/sun8i_mixer.h    |  2 ++
> > >  drivers/gpu/drm/sun4i/sun8i_ui_layer.c | 48 ++++++++++++++++++++-----
> > >  drivers/gpu/drm/sun4i/sun8i_ui_layer.h |  5 +++
> > >  drivers/gpu/drm/sun4i/sun8i_vi_layer.c | 49 ++++++++++++++++++++++----
> > >  drivers/gpu/drm/sun4i/sun8i_vi_layer.h |  5 +++
> > >  5 files changed, 94 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > b/drivers/gpu/drm/sun4i/sun8i_mixer.h index ebfc276b2464..5c05907e26fb
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_mixer.h
> > > @@ -65,6 +65,8 @@
> > >  #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_PREMULTIPLY_EN(pipe)       BIT(pipe)
> > > +
> > >  #define SUN8I_MIXER_BLEND_OUTCTL_INTERLACED  BIT(1)
> > >
> > >  #define SUN50I_MIXER_BLEND_CSC_CTL_EN(ch)    BIT(ch)
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c index 6ccbbca3176d..
29c0d9cca19a
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.c
> > > @@ -58,24 +58,46 @@ static void sun8i_ui_layer_enable(struct sun8i_mixer
> > > *mixer, int channel, }
> > >
> > >  static void sun8i_ui_layer_update_alpha(struct sun8i_mixer *mixer, int
> > > channel, -                                    int overlay, struct
> > drm_plane *plane)
> > > +                                     int overlay, struct
> > drm_plane *plane,
> > > +                                     unsigned int zpos,
> > bool force_premulti)
> > >  {
> > > -     u32 mask, val, ch_base;
> > > +     u32 mask, val, ch_base, bld_base;
> > > +     bool in_premulti, out_premulti;
> > >
> > > +     bld_base = sun8i_blender_base(mixer);
> > >       ch_base = sun8i_channel_base(mixer, channel);
> > >
> > > +     in_premulti = plane->state->pixel_blend_mode ==
> > DRM_MODE_BLEND_PREMULTI;
> > > +     out_premulti = force_premulti || in_premulti;
> > > +
> > >       mask = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_MASK |
> > > -             SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MASK;
> > > +            SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MASK |
> > > +            SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_MASK;
> > >
> > >       val = SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA(plane->state->alpha >>
> > 8);
> > >
> > > -     val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE) ?
> > > -             SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> > > -             SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> > > +     if (plane->state->pixel_blend_mode == DRM_MODE_BLEND_PIXEL_NONE) {
> > > +             val |= SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_LAYER;
> > > +     } else {
> > > +             val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE)
> > ?
> > > +
> > SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> > > +
> > SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> > > +
> > > +             if (in_premulti)
> > > +                     val |=
> > SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_PREMULTI;
> > > +     }
> > > +
> > > +     if (!in_premulti && out_premulti)
> > > +             val |= SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COV2PREM;
> > >
> > >       regmap_update_bits(mixer->engine.regs,
> > >                          SUN8I_MIXER_CHAN_UI_LAYER_ATTR(ch_base,
> > overlay),
> > >                          mask, val);
> > > +
> > > +     regmap_update_bits(
> > > +             mixer->engine.regs,
> > SUN8I_MIXER_BLEND_PREMULTIPLY(bld_base),
> > > +             SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos),
> > > +             out_premulti ? SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos) :
> > 0);
> > >  }
> > >
> > >  static int sun8i_ui_layer_update_coord(struct sun8i_mixer *mixer, int
> > > channel, @@ -274,7 +296,7 @@ static void
> > > sun8i_ui_layer_atomic_update(struct drm_plane *plane,
> > > sun8i_ui_layer_update_coord(mixer, layer->channel,
> > >                                   layer->overlay, plane, zpos);
> > >       sun8i_ui_layer_update_alpha(mixer, layer->channel,
> > > -                                 layer->overlay, plane);
> > > +                                 layer->overlay, plane, zpos,
> > false);
> > >       sun8i_ui_layer_update_formats(mixer, layer->channel,
> > >                                     layer->overlay, plane);
> > >       sun8i_ui_layer_update_buffer(mixer, layer->channel,
> > > @@ -332,8 +354,8 @@ 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;
> > > +     unsigned int plane_cnt, blend_modes;
> > >       struct sun8i_ui_layer *layer;
> > > -     unsigned int plane_cnt;
> > >       int ret;
> > >
> > >       layer = devm_kzalloc(drm->dev, sizeof(*layer), GFP_KERNEL);
> > > @@ -362,6 +384,16 @@ struct sun8i_ui_layer 
*sun8i_ui_layer_init_one(struct
> > > drm_device *drm, return ERR_PTR(ret);
> > >       }
> > >
> > > +     blend_modes = BIT(DRM_MODE_BLEND_PREMULTI) |
> > > +                   BIT(DRM_MODE_BLEND_COVERAGE) |
> > > +                   BIT(DRM_MODE_BLEND_PIXEL_NONE);
> > > +
> > > +     ret = drm_plane_create_blend_mode_property(&layer->plane,
> > blend_modes);
> > > +     if (ret) {
> > > +             dev_err(drm->dev, "Couldn't add blend mode
> > property\n");
> > > +             return ERR_PTR(ret);
> > > +     }
> > > +
> > >       ret = drm_plane_create_zpos_property(&layer->plane, channel,
> > >                                            0, plane_cnt -
> > 1);
> > >       if (ret) {
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
> > > b/drivers/gpu/drm/sun4i/sun8i_ui_layer.h index 
43c48cf7bc51..bd3c30e8058d
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_ui_layer.h
> > > @@ -46,6 +46,11 @@
> > >  #define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_LAYER              
((1) << 1)
> > >  #define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_ALPHA_MODE_COMBINED   ((2) << 1)
> > >
> > > +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_MASK    GENMASK(17, 16)
> > > +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COVERAGE        ((0) << 16)
> > > +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_COV2PREM        ((1) << 16)
> > > +#define SUN8I_MIXER_CHAN_UI_LAYER_ATTR_BLEND_PREMULTI        ((2) << 16)
> > > +
> > >  struct sun8i_mixer;
> > >
> > >  struct sun8i_ui_layer {
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c index 
662ba1018cc4..6581fc7d9668
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.c
> > > @@ -52,21 +52,38 @@ static void sun8i_vi_layer_enable(struct sun8i_mixer
> > > *mixer, int channel, }
> > >
> > >  static void sun8i_vi_layer_update_alpha(struct sun8i_mixer *mixer, int
> > > channel, -                                    int overlay, struct
> > drm_plane *plane)
> > > +                                     int overlay, struct
> > drm_plane *plane,
> > > +                                     unsigned int zpos,
> > bool force_premulti)
> > >  {
> > > -     u32 mask, val, ch_base;
> > > +     u32 mask, val, ch_base, bld_base;
> > > +     bool in_premulti = false, out_premulti = false;
> > >
> > >       ch_base = sun8i_channel_base(mixer, channel);
> > > +     bld_base = sun8i_blender_base(mixer);
> > >
> > >       if (mixer->cfg->is_de3) {
> > > +             in_premulti = plane->state->pixel_blend_mode ==
> > DRM_MODE_BLEND_PREMULTI;
> > > +             out_premulti = force_premulti || in_premulti;
> > > +
> > >               mask = SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK |
> > > -                    SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_MASK;
> > > +                    SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_MASK
> > |
> > > +                    SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_MASK;
> > >               val = SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA
> > >                       (plane->state->alpha >> 8);
> > >
> > > -             val |= (plane->state->alpha == DRM_BLEND_ALPHA_OPAQUE)
> > ?
> > > -
> > SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> > > -
> > SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> > > +             if (plane->state->pixel_blend_mode ==
> > DRM_MODE_BLEND_PIXEL_NONE) {
> > > +                     val |=
> > SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_LAYER;
> > > +             } else {
> > > +                     val |= (plane->state->alpha ==
> > DRM_BLEND_ALPHA_OPAQUE) ?
> > > +
> > SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL :
> > > +
> > SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED;
> > > +
> > > +                     if (in_premulti)
> > > +                             val |=
> > SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_PREMULTI;
> > > +             }
> > > +
> > > +             if (!in_premulti && out_premulti)
> > > +                     val |=
> > SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COV2PREM;
> > >
> > >               regmap_update_bits(mixer->engine.regs,
> > >
> > SUN8I_MIXER_CHAN_VI_LAYER_ATTR(ch_base,
> > > @@ -79,6 +96,11 @@ static void sun8i_vi_layer_update_alpha(struct
> > > sun8i_mixer *mixer, int channel, SUN8I_MIXER_FCC_GLOBAL_ALPHA
> > >                                       (plane->state->alpha
> > >> 8));
> > >       }
> > > +
> > > +     regmap_update_bits(
> > > +             mixer->engine.regs,
> > SUN8I_MIXER_BLEND_PREMULTIPLY(bld_base),
> > > +             SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos),
> > > +             out_premulti ? SUN8I_MIXER_BLEND_PREMULTIPLY_EN(zpos) :
> > 0);
> >
> > Shouldn't be above block inside DE3 if block?
> 
> No, we have to update the value to non-premulti in case it was set to
> premulti by another layer.
> 
> 
> Regards,
> Roman.
> 
> >
> > Looks good otherwise.
> >
> > Best regards,
> > Jernej
> >
> > >  }
> > >
> > >  static int sun8i_vi_layer_update_coord(struct sun8i_mixer *mixer, int
> > > channel, @@ -408,7 +430,7 @@ static void
> > > sun8i_vi_layer_atomic_update(struct drm_plane *plane,
> > > sun8i_vi_layer_update_coord(mixer, layer->channel,
> > >                                   layer->overlay, plane, zpos);
> > >       sun8i_vi_layer_update_alpha(mixer, layer->channel,
> > > -                                 layer->overlay, plane);
> > > +                                 layer->overlay, plane, zpos,
> > false);
> > >       sun8i_vi_layer_update_formats(mixer, layer->channel,
> > >                                     layer->overlay, plane);
> > >       sun8i_vi_layer_update_buffer(mixer, layer->channel,
> > > @@ -563,6 +585,19 @@ struct sun8i_vi_layer 
*sun8i_vi_layer_init_one(struct
> > > drm_device *drm, }
> > >       }
> > >
> > > +     if (mixer->cfg->is_de3) {
> > > +             unsigned int blend_modes = BIT(DRM_MODE_BLEND_PREMULTI)
> > |
> > > +
> > BIT(DRM_MODE_BLEND_COVERAGE) |
> > > +
> > BIT(DRM_MODE_BLEND_PIXEL_NONE);
> > > +
> > > +             ret = drm_plane_create_blend_mode_property(&layer-
> > >plane,
> > > +
> > blend_modes);
> > > +             if (ret) {
> > > +                     dev_err(drm->dev, "Couldn't add blend mode
> > property\n");
> > > +                     return ERR_PTR(ret);
> > > +             }
> > > +     }
> > > +
> > >       ret = drm_plane_create_zpos_property(&layer->plane, index,
> > >                                            0, plane_cnt -
> > 1);
> > >       if (ret) {
> > > diff --git a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > > b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h index 
9939a4cc7a52..ccf91f09f1fe
> > > 100644
> > > --- a/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > > +++ b/drivers/gpu/drm/sun4i/sun8i_vi_layer.h
> > > @@ -44,6 +44,11 @@
> > >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MASK   GENMASK(31, 24)
> > >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA(x)     ((x) << 24)
> > >
> > > +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_MASK   GENMASK(17, 16)
> > > +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COVERAGE       ((0) << 16)
> > > +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_COV2PREM       ((1) << 16)
> > > +#define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_BLEND_PREMULTI       ((2) << 16)
> > > +
> > >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_PIXEL     ((0) <<
> > 1)
> > >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_LAYER     ((1) <<
> > 1)
> > >  #define SUN50I_MIXER_CHAN_VI_LAYER_ATTR_ALPHA_MODE_COMBINED  ((2) << 1)
> >
> >
> >
> >
> 



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2022-06-06 21:28 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-06-05 15:47 [PATCH] drm/sun4i: sun8i: Add support for pixel blend mode property Roman Stratiienko
2022-06-05 15:47 ` Roman Stratiienko
2022-06-05 20:36 ` Jernej Škrabec
2022-06-05 20:36   ` Jernej Škrabec
2022-06-06  8:17   ` Roman Stratiienko
2022-06-06  8:17     ` Roman Stratiienko
2022-06-06  8:17     ` Roman Stratiienko
2022-06-06  9:20     ` Maxime Ripard
2022-06-06  9:20       ` Maxime Ripard
2022-06-06  9:20       ` Maxime Ripard
2022-06-06  9:22       ` Maxime Ripard
2022-06-06  9:22         ` Maxime Ripard
2022-06-06  9:22         ` Maxime Ripard
2022-06-06 11:21         ` Roman Stratiienko
2022-06-06 11:21           ` Roman Stratiienko
2022-06-06 11:21           ` Roman Stratiienko
2022-06-06 21:27     ` Jernej Škrabec
2022-06-06 21:27       ` Jernej Škrabec
2022-06-06 21:27       ` Jernej Škrabec

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.