[PATCH] OMAP3630: DSS2: Enable Pre-Multiplied Alpha Support

[PATCH] OMAP3630: DSS2: Enable Pre-Multiplied Alpha Support

[Samreen]

From: Rajkumar N <rajkumar.nagarajan@ti.com>

Enable dss to process color formats with pre-mulitplied alpha.
With this we can have alpha values defined for each pixel
and hence can have different blending values for each pixel.
sysfs entry has been created for this and pre-multiplied alpha
support is turned off by default.

Also, the check for '(plane != OMAP_DSS_VIDEO1)'
in  _dispc_setup_plane has been replaced by the correct
dss_has_feature check

Signed-off-by: Sudeep Basavaraj <sudeep.basavaraj@ti.com>
Signed-off-by: Rajkumar N <rajkumar.nagarajan@ti.com>
Signed-off-by: Samreen <samreen@ti.com>
---
 arch/arm/plat-omap/include/plat/display.h |    1 +
 drivers/video/omap2/dss/dispc.c           |   25 +++++++++++++++++---
 drivers/video/omap2/dss/dss.h             |    3 +-
 drivers/video/omap2/dss/dss_features.c    |   20 ++++++++++++++--
 drivers/video/omap2/dss/dss_features.h    |    1 +
 drivers/video/omap2/dss/manager.c         |    5 +++-
 drivers/video/omap2/dss/overlay.c         |   35 +++++++++++++++++++++++++++++
 7 files changed, 81 insertions(+), 9 deletions(-)

diff --git a/arch/arm/plat-omap/include/plat/display.h b/arch/arm/plat-omap/include/plat/display.h
index c915a66..d433baf 100644
--- a/arch/arm/plat-omap/include/plat/display.h
+++ b/arch/arm/plat-omap/include/plat/display.h
@@ -268,6 +268,7 @@ struct omap_overlay_info {
 	u16 out_width;	/* if 0, out_width == width */
 	u16 out_height;	/* if 0, out_height == height */
 	u8 global_alpha;
+	u8 pre_mult_alpha;
 };
 
 struct omap_overlay {
diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index fa40fa5..f1d1797 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -773,6 +773,17 @@ static void _dispc_set_vid_size(enum omap_plane plane, int width, int height)
 	dispc_write_reg(vsi_reg[plane-1], val);
 }
 
+static void _dispc_set_pre_mult_alpha(enum omap_plane plane, bool enable)
+{
+	if (!dss_has_feature(FEAT_PREMUL_ALPHA))
+		return;
+
+	BUG_ON(!dss_has_feature(FEAT_GLOBAL_ALPHA_VID1) &&
+		plane == OMAP_DSS_VIDEO1);
+
+	REG_FLD_MOD(dispc_reg_att[plane], enable ? 1 : 0, 28, 28);
+}
+
 static void _dispc_setup_global_alpha(enum omap_plane plane, u8 global_alpha)
 {
 	if (!dss_has_feature(FEAT_GLOBAL_ALPHA))
@@ -1507,7 +1518,8 @@ static int _dispc_setup_plane(enum omap_plane plane,
 		bool ilace,
 		enum omap_dss_rotation_type rotation_type,
 		u8 rotation, int mirror,
-		u8 global_alpha)
+		u8 global_alpha,
+		u8 pre_mult_alpha)
 {
 	const int maxdownscale = cpu_is_omap34xx() ? 4 : 2;
 	bool five_taps = 0;
@@ -1693,8 +1705,11 @@ static int _dispc_setup_plane(enum omap_plane plane,
 
 	_dispc_set_rotation_attrs(plane, rotation, mirror, color_mode);
 
-	if (plane != OMAP_DSS_VIDEO1)
+	if ((plane != OMAP_DSS_VIDEO1) ||
+		dss_has_feature(FEAT_GLOBAL_ALPHA_VID1)) {
+		_dispc_set_pre_mult_alpha(plane, pre_mult_alpha);
 		_dispc_setup_global_alpha(plane, global_alpha);
+	}
 
 	return 0;
 }
@@ -3139,7 +3154,8 @@ int dispc_setup_plane(enum omap_plane plane,
 		       enum omap_color_mode color_mode,
 		       bool ilace,
 		       enum omap_dss_rotation_type rotation_type,
-		       u8 rotation, bool mirror, u8 global_alpha)
+		       u8 rotation, bool mirror, u8 global_alpha,
+		       u8 pre_mult_alpha)
 {
 	int r = 0;
 
@@ -3161,7 +3177,8 @@ int dispc_setup_plane(enum omap_plane plane,
 			   color_mode, ilace,
 			   rotation_type,
 			   rotation, mirror,
-			   global_alpha);
+			   global_alpha,
+			   pre_mult_alpha);
 
 	enable_clocks(0);
 
diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
index 5c7940d..2bb515c 100644
--- a/drivers/video/omap2/dss/dss.h
+++ b/drivers/video/omap2/dss/dss.h
@@ -359,7 +359,8 @@ int dispc_setup_plane(enum omap_plane plane,
 		      bool ilace,
 		      enum omap_dss_rotation_type rotation_type,
 		      u8 rotation, bool mirror,
-		      u8 global_alpha);
+		      u8 global_alpha,
+		      u8 pre_mult_alpha);
 
 bool dispc_go_busy(enum omap_channel channel);
 void dispc_go(enum omap_channel channel);
diff --git a/drivers/video/omap2/dss/dss_features.c b/drivers/video/omap2/dss/dss_features.c
index 867f68d..1abb8c5 100644
--- a/drivers/video/omap2/dss/dss_features.c
+++ b/drivers/video/omap2/dss/dss_features.c
@@ -134,7 +134,7 @@ static struct omap_dss_features omap2_dss_features = {
 };
 
 /* OMAP3 DSS Features */
-static struct omap_dss_features omap3_dss_features = {
+static struct omap_dss_features omap3430_dss_features = {
 	.reg_fields = omap3_dss_reg_fields,
 	.num_reg_fields = ARRAY_SIZE(omap3_dss_reg_fields),
 
@@ -146,6 +146,18 @@ static struct omap_dss_features omap3_dss_features = {
 	.supported_color_modes = omap3_dss_supported_color_modes,
 };
 
+static struct omap_dss_features omap3630_dss_features = {
+	.reg_fields = omap3_dss_reg_fields,
+	.num_reg_fields = ARRAY_SIZE(omap3_dss_reg_fields),
+
+	.has_feature    = FEAT_GLOBAL_ALPHA | FEAT_PREMUL_ALPHA,
+
+	.num_mgrs = 2,
+	.num_ovls = 3,
+	.supported_displays = omap3_dss_supported_displays,
+	.supported_color_modes = omap3_dss_supported_color_modes,
+};
+
 /* Functions returning values related to a DSS feature */
 int dss_feat_get_num_mgrs(void)
 {
@@ -186,6 +198,8 @@ void dss_features_init(void)
 {
 	if (cpu_is_omap24xx())
 		omap_current_dss_features = &omap2_dss_features;
-	else
-		omap_current_dss_features = &omap3_dss_features;
+	else if (cpu_is_omap3630())
+		omap_current_dss_features = &omap3630_dss_features;
+	else if (cpu_is_omap34xx())
+		omap_current_dss_features = &omap3430_dss_features;
 }
diff --git a/drivers/video/omap2/dss/dss_features.h b/drivers/video/omap2/dss/dss_features.h
index cb231ea..f287da8 100644
--- a/drivers/video/omap2/dss/dss_features.h
+++ b/drivers/video/omap2/dss/dss_features.h
@@ -27,6 +27,7 @@
 enum dss_feat_id {
 	FEAT_GLOBAL_ALPHA	= 1 << 0,
 	FEAT_GLOBAL_ALPHA_VID1	= 1 << 1,
+	FEAT_PREMUL_ALPHA	= 1 << 2,
 };
 
 /* DSS register field id */
diff --git a/drivers/video/omap2/dss/manager.c b/drivers/video/omap2/dss/manager.c
index 545e9b9..873b334 100644
--- a/drivers/video/omap2/dss/manager.c
+++ b/drivers/video/omap2/dss/manager.c
@@ -406,6 +406,7 @@ struct overlay_cache_data {
 	u16 out_width;	/* if 0, out_width == width */
 	u16 out_height;	/* if 0, out_height == height */
 	u8 global_alpha;
+	u8 pre_mult_alpha;
 
 	enum omap_channel channel;
 	bool replication;
@@ -842,7 +843,8 @@ static int configure_overlay(enum omap_plane plane)
 			c->rotation_type,
 			c->rotation,
 			c->mirror,
-			c->global_alpha);
+			c->global_alpha,
+			c->pre_mult_alpha);
 
 	if (r) {
 		/* this shouldn't happen */
@@ -1265,6 +1267,7 @@ static int omap_dss_mgr_apply(struct omap_overlay_manager *mgr)
 		oc->out_width = ovl->info.out_width;
 		oc->out_height = ovl->info.out_height;
 		oc->global_alpha = ovl->info.global_alpha;
+		oc->pre_mult_alpha = ovl->info.pre_mult_alpha;
 
 		oc->replication =
 			dss_use_replication(dssdev, ovl->info.color_mode);
diff --git a/drivers/video/omap2/dss/overlay.c b/drivers/video/omap2/dss/overlay.c
index 75642c2..f704d06 100644
--- a/drivers/video/omap2/dss/overlay.c
+++ b/drivers/video/omap2/dss/overlay.c
@@ -257,6 +257,37 @@ static ssize_t overlay_global_alpha_store(struct omap_overlay *ovl,
 	return size;
 }
 
+static ssize_t overlay_pre_multiplication_alpha_show(struct omap_overlay *ovl,
+		char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%d\n",
+			ovl->info.pre_mult_alpha);
+}
+
+static ssize_t overlay_pre_multiplication_alpha_store(struct omap_overlay *ovl,
+		const char *buf, size_t size)
+{
+	int r;
+	struct omap_overlay_info info;
+
+	ovl->get_overlay_info(ovl, &info);
+
+	/* only GFX and Video2 plane support pre alpha multiplication */
+	info.pre_mult_alpha = simple_strtoul(buf, NULL, 10);
+
+	r = ovl->set_overlay_info(ovl, &info);
+	if (r)
+		return r;
+
+	if (ovl->manager) {
+		r = ovl->manager->apply(ovl->manager);
+		if (r)
+			return r;
+	}
+
+	return size;
+}
+
 struct overlay_attribute {
 	struct attribute attr;
 	ssize_t (*show)(struct omap_overlay *, char *);
@@ -280,6 +311,9 @@ static OVERLAY_ATTR(enabled, S_IRUGO|S_IWUSR,
 		overlay_enabled_show, overlay_enabled_store);
 static OVERLAY_ATTR(global_alpha, S_IRUGO|S_IWUSR,
 		overlay_global_alpha_show, overlay_global_alpha_store);
+static OVERLAY_ATTR(pre_multiplication_alpha, S_IRUGO|S_IWUSR,
+		overlay_pre_multiplication_alpha_show,
+		overlay_pre_multiplication_alpha_store);
 
 static struct attribute *overlay_sysfs_attrs[] = {
 	&overlay_attr_name.attr,
@@ -290,6 +324,7 @@ static struct attribute *overlay_sysfs_attrs[] = {
 	&overlay_attr_output_size.attr,
 	&overlay_attr_enabled.attr,
 	&overlay_attr_global_alpha.attr,
+	&overlay_attr_pre_multiplication_alpha.attr,
 	NULL
 };
 
-- 
1.5.6.3

Re: [PATCH] OMAP3630: DSS2: Enable Pre-Multiplied Alpha Support

[Tomi Valkeinen]

Hi,

On Wed, 2010-10-27 at 12:16 +0200, ext Samreen wrote:
> From: Rajkumar N <rajkumar.nagarajan@ti.com>
> 
> Enable dss to process color formats with pre-mulitplied alpha.
> With this we can have alpha values defined for each pixel
> and hence can have different blending values for each pixel.
> sysfs entry has been created for this and pre-multiplied alpha
> support is turned off by default.
> 
> Also, the check for '(plane != OMAP_DSS_VIDEO1)'
> in  _dispc_setup_plane has been replaced by the correct
> dss_has_feature check

You're changing too many things in one patch. Please separate the
dss_feature changes and the actual implementation for the pre-multiplied
alpha.

Your naming of this feature is also bit varying: you have "pre_mult",
"PREMUL", "pre_multiplication_alpha". The last one is quite wrong, as
the feature is "premultiplied alpha".

Also a few comments inline.

> 
> Signed-off-by: Sudeep Basavaraj <sudeep.basavaraj@ti.com>
> Signed-off-by: Rajkumar N <rajkumar.nagarajan@ti.com>
> Signed-off-by: Samreen <samreen@ti.com>
> ---
>  arch/arm/plat-omap/include/plat/display.h |    1 +
>  drivers/video/omap2/dss/dispc.c           |   25 +++++++++++++++++---
>  drivers/video/omap2/dss/dss.h             |    3 +-
>  drivers/video/omap2/dss/dss_features.c    |   20 ++++++++++++++--
>  drivers/video/omap2/dss/dss_features.h    |    1 +
>  drivers/video/omap2/dss/manager.c         |    5 +++-
>  drivers/video/omap2/dss/overlay.c         |   35 +++++++++++++++++++++++++++++
>  7 files changed, 81 insertions(+), 9 deletions(-)
> 
> diff --git a/arch/arm/plat-omap/include/plat/display.h b/arch/arm/plat-omap/include/plat/display.h
> index c915a66..d433baf 100644
> --- a/arch/arm/plat-omap/include/plat/display.h
> +++ b/arch/arm/plat-omap/include/plat/display.h
> @@ -268,6 +268,7 @@ struct omap_overlay_info {
>  	u16 out_width;	/* if 0, out_width == width */
>  	u16 out_height;	/* if 0, out_height == height */
>  	u8 global_alpha;
> +	u8 pre_mult_alpha;
>  };
>  
>  struct omap_overlay {
> diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
> index fa40fa5..f1d1797 100644
> --- a/drivers/video/omap2/dss/dispc.c
> +++ b/drivers/video/omap2/dss/dispc.c
> @@ -773,6 +773,17 @@ static void _dispc_set_vid_size(enum omap_plane plane, int width, int height)
>  	dispc_write_reg(vsi_reg[plane-1], val);
>  }
>  
> +static void _dispc_set_pre_mult_alpha(enum omap_plane plane, bool enable)
> +{
> +	if (!dss_has_feature(FEAT_PREMUL_ALPHA))
> +		return;
> +
> +	BUG_ON(!dss_has_feature(FEAT_GLOBAL_ALPHA_VID1) &&
> +		plane == OMAP_DSS_VIDEO1);

What is the rationale for having the function return, if
FEAT_PREMUL_ALPHA is not supported, but BUG if plane is video1 and
GLOBAL_ALPHA_VID1 is not supported?

> +
> +	REG_FLD_MOD(dispc_reg_att[plane], enable ? 1 : 0, 28, 28);
> +}
> +
>  static void _dispc_setup_global_alpha(enum omap_plane plane, u8 global_alpha)
>  {
>  	if (!dss_has_feature(FEAT_GLOBAL_ALPHA))
> @@ -1507,7 +1518,8 @@ static int _dispc_setup_plane(enum omap_plane plane,
>  		bool ilace,
>  		enum omap_dss_rotation_type rotation_type,
>  		u8 rotation, int mirror,
> -		u8 global_alpha)
> +		u8 global_alpha,
> +		u8 pre_mult_alpha)
>  {
>  	const int maxdownscale = cpu_is_omap34xx() ? 4 : 2;
>  	bool five_taps = 0;
> @@ -1693,8 +1705,11 @@ static int _dispc_setup_plane(enum omap_plane plane,
>  
>  	_dispc_set_rotation_attrs(plane, rotation, mirror, color_mode);
>  
> -	if (plane != OMAP_DSS_VIDEO1)
> +	if ((plane != OMAP_DSS_VIDEO1) ||
> +		dss_has_feature(FEAT_GLOBAL_ALPHA_VID1)) {
> +		_dispc_set_pre_mult_alpha(plane, pre_mult_alpha);
>  		_dispc_setup_global_alpha(plane, global_alpha);
> +	}
>  
>  	return 0;
>  }
> @@ -3139,7 +3154,8 @@ int dispc_setup_plane(enum omap_plane plane,
>  		       enum omap_color_mode color_mode,
>  		       bool ilace,
>  		       enum omap_dss_rotation_type rotation_type,
> -		       u8 rotation, bool mirror, u8 global_alpha)
> +		       u8 rotation, bool mirror, u8 global_alpha,
> +		       u8 pre_mult_alpha)
>  {
>  	int r = 0;
>  
> @@ -3161,7 +3177,8 @@ int dispc_setup_plane(enum omap_plane plane,
>  			   color_mode, ilace,
>  			   rotation_type,
>  			   rotation, mirror,
> -			   global_alpha);
> +			   global_alpha,
> +			   pre_mult_alpha);
>  
>  	enable_clocks(0);
>  
> diff --git a/drivers/video/omap2/dss/dss.h b/drivers/video/omap2/dss/dss.h
> index 5c7940d..2bb515c 100644
> --- a/drivers/video/omap2/dss/dss.h
> +++ b/drivers/video/omap2/dss/dss.h
> @@ -359,7 +359,8 @@ int dispc_setup_plane(enum omap_plane plane,
>  		      bool ilace,
>  		      enum omap_dss_rotation_type rotation_type,
>  		      u8 rotation, bool mirror,
> -		      u8 global_alpha);
> +		      u8 global_alpha,
> +		      u8 pre_mult_alpha);
>  
>  bool dispc_go_busy(enum omap_channel channel);
>  void dispc_go(enum omap_channel channel);
> diff --git a/drivers/video/omap2/dss/dss_features.c b/drivers/video/omap2/dss/dss_features.c
> index 867f68d..1abb8c5 100644
> --- a/drivers/video/omap2/dss/dss_features.c
> +++ b/drivers/video/omap2/dss/dss_features.c
> @@ -134,7 +134,7 @@ static struct omap_dss_features omap2_dss_features = {
>  };
>  
>  /* OMAP3 DSS Features */
> -static struct omap_dss_features omap3_dss_features = {
> +static struct omap_dss_features omap3430_dss_features = {
>  	.reg_fields = omap3_dss_reg_fields,
>  	.num_reg_fields = ARRAY_SIZE(omap3_dss_reg_fields),
>  
> @@ -146,6 +146,18 @@ static struct omap_dss_features omap3_dss_features = {
>  	.supported_color_modes = omap3_dss_supported_color_modes,
>  };
>  
> +static struct omap_dss_features omap3630_dss_features = {
> +	.reg_fields = omap3_dss_reg_fields,
> +	.num_reg_fields = ARRAY_SIZE(omap3_dss_reg_fields),
> +
> +	.has_feature    = FEAT_GLOBAL_ALPHA | FEAT_PREMUL_ALPHA,
> +
> +	.num_mgrs = 2,
> +	.num_ovls = 3,
> +	.supported_displays = omap3_dss_supported_displays,
> +	.supported_color_modes = omap3_dss_supported_color_modes,
> +};
> +
>  /* Functions returning values related to a DSS feature */
>  int dss_feat_get_num_mgrs(void)
>  {
> @@ -186,6 +198,8 @@ void dss_features_init(void)
>  {
>  	if (cpu_is_omap24xx())
>  		omap_current_dss_features = &omap2_dss_features;
> -	else
> -		omap_current_dss_features = &omap3_dss_features;
> +	else if (cpu_is_omap3630())
> +		omap_current_dss_features = &omap3630_dss_features;
> +	else if (cpu_is_omap34xx())
> +		omap_current_dss_features = &omap3430_dss_features;
>  }
> diff --git a/drivers/video/omap2/dss/dss_features.h b/drivers/video/omap2/dss/dss_features.h
> index cb231ea..f287da8 100644
> --- a/drivers/video/omap2/dss/dss_features.h
> +++ b/drivers/video/omap2/dss/dss_features.h
> @@ -27,6 +27,7 @@
>  enum dss_feat_id {
>  	FEAT_GLOBAL_ALPHA	= 1 << 0,
>  	FEAT_GLOBAL_ALPHA_VID1	= 1 << 1,
> +	FEAT_PREMUL_ALPHA	= 1 << 2,
>  };
>  
>  /* DSS register field id */
> diff --git a/drivers/video/omap2/dss/manager.c b/drivers/video/omap2/dss/manager.c
> index 545e9b9..873b334 100644
> --- a/drivers/video/omap2/dss/manager.c
> +++ b/drivers/video/omap2/dss/manager.c
> @@ -406,6 +406,7 @@ struct overlay_cache_data {
>  	u16 out_width;	/* if 0, out_width == width */
>  	u16 out_height;	/* if 0, out_height == height */
>  	u8 global_alpha;
> +	u8 pre_mult_alpha;
>  
>  	enum omap_channel channel;
>  	bool replication;
> @@ -842,7 +843,8 @@ static int configure_overlay(enum omap_plane plane)
>  			c->rotation_type,
>  			c->rotation,
>  			c->mirror,
> -			c->global_alpha);
> +			c->global_alpha,
> +			c->pre_mult_alpha);
>  
>  	if (r) {
>  		/* this shouldn't happen */
> @@ -1265,6 +1267,7 @@ static int omap_dss_mgr_apply(struct omap_overlay_manager *mgr)
>  		oc->out_width = ovl->info.out_width;
>  		oc->out_height = ovl->info.out_height;
>  		oc->global_alpha = ovl->info.global_alpha;
> +		oc->pre_mult_alpha = ovl->info.pre_mult_alpha;
>  
>  		oc->replication =
>  			dss_use_replication(dssdev, ovl->info.color_mode);
> diff --git a/drivers/video/omap2/dss/overlay.c b/drivers/video/omap2/dss/overlay.c
> index 75642c2..f704d06 100644
> --- a/drivers/video/omap2/dss/overlay.c
> +++ b/drivers/video/omap2/dss/overlay.c
> @@ -257,6 +257,37 @@ static ssize_t overlay_global_alpha_store(struct omap_overlay *ovl,
>  	return size;
>  }
>  
> +static ssize_t overlay_pre_multiplication_alpha_show(struct omap_overlay *ovl,
> +		char *buf)
> +{
> +	return snprintf(buf, PAGE_SIZE, "%d\n",
> +			ovl->info.pre_mult_alpha);
> +}
> +
> +static ssize_t overlay_pre_multiplication_alpha_store(struct omap_overlay *ovl,
> +		const char *buf, size_t size)
> +{
> +	int r;
> +	struct omap_overlay_info info;
> +
> +	ovl->get_overlay_info(ovl, &info);
> +
> +	/* only GFX and Video2 plane support pre alpha multiplication */

Shouldn't you check what the supported features are, instead of having a
comment?

> +	info.pre_mult_alpha = simple_strtoul(buf, NULL, 10);
> +
> +	r = ovl->set_overlay_info(ovl, &info);
> +	if (r)
> +		return r;
> +
> +	if (ovl->manager) {
> +		r = ovl->manager->apply(ovl->manager);
> +		if (r)
> +			return r;
> +	}
> +
> +	return size;
> +}
> +
>  struct overlay_attribute {
>  	struct attribute attr;
>  	ssize_t (*show)(struct omap_overlay *, char *);
> @@ -280,6 +311,9 @@ static OVERLAY_ATTR(enabled, S_IRUGO|S_IWUSR,
>  		overlay_enabled_show, overlay_enabled_store);
>  static OVERLAY_ATTR(global_alpha, S_IRUGO|S_IWUSR,
>  		overlay_global_alpha_show, overlay_global_alpha_store);
> +static OVERLAY_ATTR(pre_multiplication_alpha, S_IRUGO|S_IWUSR,
> +		overlay_pre_multiplication_alpha_show,
> +		overlay_pre_multiplication_alpha_store);
>  
>  static struct attribute *overlay_sysfs_attrs[] = {
>  	&overlay_attr_name.attr,
> @@ -290,6 +324,7 @@ static struct attribute *overlay_sysfs_attrs[] = {
>  	&overlay_attr_output_size.attr,
>  	&overlay_attr_enabled.attr,
>  	&overlay_attr_global_alpha.attr,
> +	&overlay_attr_pre_multiplication_alpha.attr,
>  	NULL
>  };
>  

RE: [PATCH] OMAP3630: DSS2: Enable Pre-Multiplied Alpha Support

[Taneja, Archit]

Hi,

linux-omap-owner@vger.kernel.org wrote:
> Alpha Support
> 

[snip]

>> 
>> +static void _dispc_set_pre_mult_alpha(enum omap_plane plane, bool +enable) {
>> +	if (!dss_has_feature(FEAT_PREMUL_ALPHA))
>> +		return;
>> +
>> +	BUG_ON(!dss_has_feature(FEAT_GLOBAL_ALPHA_VID1) &&
>> +		plane == OMAP_DSS_VIDEO1);
> 
> What is the rationale for having the function return, if
> FEAT_PREMUL_ALPHA is not supported, but BUG if plane is video1 and
> GLOBAL_ALPHA_VID1 is not supported?

Premultiplied alpha is available on omap36xx and above, but on 36xx
since global alpha itself isn't supported for video1 then writing to
the pre multiplied alpha bit is incorrect.

It could have been possible to make a new feature like FEAT_PRE_MULT_VID1
but I think its redundant as FEAT_GLOBAL_ALPHA_VID1 is enough to determine
if we should set the pre multiplied alpha bit for video1 plane or not.

> 
>> +
>> +	REG_FLD_MOD(dispc_reg_att[plane], enable ? 1 : 0, 28, 28); } +
>>  static void _dispc_setup_global_alpha(enum omap_plane plane, u8
>>  	global_alpha)  { if (!dss_has_feature(FEAT_GLOBAL_ALPHA))
>> @@ -1507,7 +1518,8 @@ static int _dispc_setup_plane(enum omap_plane plane, 
>>  		bool ilace, enum omap_dss_rotation_type rotation_type,
>>  		u8 rotation, int mirror,
>> -		u8 global_alpha)
>> +		u8 global_alpha,
>> +		u8 pre_mult_alpha)
>>  {
>>  	const int maxdownscale = cpu_is_omap34xx() ? 4 : 2;  	bool five_taps = 0;
>> @@ -1693,8 +1705,11 @@ static int _dispc_setup_plane(enum omap_plane plane,
>> 
>>  	_dispc_set_rotation_attrs(plane, rotation, mirror, color_mode);
>> 
>> -	if (plane != OMAP_DSS_VIDEO1)
>> +	if ((plane != OMAP_DSS_VIDEO1) ||
>> +		dss_has_feature(FEAT_GLOBAL_ALPHA_VID1)) {
>> +		_dispc_set_pre_mult_alpha(plane, pre_mult_alpha);
>>  		_dispc_setup_global_alpha(plane, global_alpha);
>> +	}
>> 
>>  	return 0;
>>  }
>> @@ -3139,7 +3154,8 @@ int dispc_setup_plane(enum omap_plane plane,
>>  		       enum omap_color_mode color_mode,
>>  		       bool ilace,
>>  		       enum omap_dss_rotation_type rotation_type,
>> -		       u8 rotation, bool mirror, u8 global_alpha)
>> +		       u8 rotation, bool mirror, u8 global_alpha, +		       u8
>>  pre_mult_alpha) {
>>  	int r = 0;
>> 
>> @@ -3161,7 +3177,8 @@ int dispc_setup_plane(enum omap_plane plane,  			  
>>  			   color_mode, ilace, rotation_type,
>>  			   rotation, mirror,
>> -			   global_alpha);
>> +			   global_alpha,
>> +			   pre_mult_alpha);
>> 
>>  	enable_clocks(0);
>> 
>> diff --git a/drivers/video/omap2/dss/dss.h
>> b/drivers/video/omap2/dss/dss.h index 5c7940d..2bb515c 100644
>> --- a/drivers/video/omap2/dss/dss.h
>> +++ b/drivers/video/omap2/dss/dss.h
>> @@ -359,7 +359,8 @@ int dispc_setup_plane(enum omap_plane plane,  		     
>>  		      bool ilace, enum omap_dss_rotation_type rotation_type,
>>  		      u8 rotation, bool mirror,
>> -		      u8 global_alpha);
>> +		      u8 global_alpha,
>> +		      u8 pre_mult_alpha);
>> 
>>  bool dispc_go_busy(enum omap_channel channel);  void dispc_go(enum
>> omap_channel channel); diff --git
>> a/drivers/video/omap2/dss/dss_features.c
>> b/drivers/video/omap2/dss/dss_features.c
>> index 867f68d..1abb8c5 100644
>> --- a/drivers/video/omap2/dss/dss_features.c
>> +++ b/drivers/video/omap2/dss/dss_features.c
>> @@ -134,7 +134,7 @@ static struct omap_dss_features omap2_dss_features = { 
>> }; 
>> 
>>  /* OMAP3 DSS Features */
>> -static struct omap_dss_features omap3_dss_features = {
>> +static struct omap_dss_features omap3430_dss_features = {
>>  	.reg_fields = omap3_dss_reg_fields,
>>  	.num_reg_fields = ARRAY_SIZE(omap3_dss_reg_fields),
>> 
>> @@ -146,6 +146,18 @@ static struct omap_dss_features omap3_dss_features = {
>>  	.supported_color_modes = omap3_dss_supported_color_modes,  };
>> 
>> +static struct omap_dss_features omap3630_dss_features = {
>> +	.reg_fields = omap3_dss_reg_fields,
>> +	.num_reg_fields = ARRAY_SIZE(omap3_dss_reg_fields), +
>> +	.has_feature    = FEAT_GLOBAL_ALPHA | FEAT_PREMUL_ALPHA, +
>> +	.num_mgrs = 2,
>> +	.num_ovls = 3,
>> +	.supported_displays = omap3_dss_supported_displays,
>> +	.supported_color_modes = omap3_dss_supported_color_modes, }; +
>>  /* Functions returning values related to a DSS feature */  int
>> dss_feat_get_num_mgrs(void)  { @@ -186,6 +198,8 @@ void
>>  	dss_features_init(void)  { if (cpu_is_omap24xx())
>>  		omap_current_dss_features = &omap2_dss_features;
>> -	else
>> -		omap_current_dss_features = &omap3_dss_features; +	else if
>> (cpu_is_omap3630()) +		omap_current_dss_features = &omap3630_dss_features;
>> +	else if (cpu_is_omap34xx()) +		omap_current_dss_features =
>> &omap3430_dss_features;  } 
>> diff --git a/drivers/video/omap2/dss/dss_features.h
>> b/drivers/video/omap2/dss/dss_features.h
>> index cb231ea..f287da8 100644
>> --- a/drivers/video/omap2/dss/dss_features.h
>> +++ b/drivers/video/omap2/dss/dss_features.h
>> @@ -27,6 +27,7 @@
>>  enum dss_feat_id {
>>  	FEAT_GLOBAL_ALPHA	= 1 << 0,
>>  	FEAT_GLOBAL_ALPHA_VID1	= 1 << 1,
>> +	FEAT_PREMUL_ALPHA	= 1 << 2,
>>  };
>> 
>>  /* DSS register field id */
>> diff --git a/drivers/video/omap2/dss/manager.c
>> b/drivers/video/omap2/dss/manager.c
>> index 545e9b9..873b334 100644
>> --- a/drivers/video/omap2/dss/manager.c
>> +++ b/drivers/video/omap2/dss/manager.c
>> @@ -406,6 +406,7 @@ struct overlay_cache_data {
>>  	u16 out_width;	/* if 0, out_width == width */
>>  	u16 out_height;	/* if 0, out_height == height */
>>  	u8 global_alpha;
>> +	u8 pre_mult_alpha;
>> 
>>  	enum omap_channel channel;
>>  	bool replication;
>> @@ -842,7 +843,8 @@ static int configure_overlay(enum omap_plane plane) 
>>  			c->rotation_type, c->rotation,
>>  			c->mirror,
>> -			c->global_alpha);
>> +			c->global_alpha,
>> +			c->pre_mult_alpha);
>> 
>>  	if (r) {
>>  		/* this shouldn't happen */
>> @@ -1265,6 +1267,7 @@ static int omap_dss_mgr_apply(struct
>>  		omap_overlay_manager *mgr) oc->out_width = ovl->info.out_width;
>>  		oc->out_height = ovl->info.out_height;
>>  		oc->global_alpha = ovl->info.global_alpha;
>> +		oc->pre_mult_alpha = ovl->info.pre_mult_alpha;
>> 
>>  		oc->replication =
>>  			dss_use_replication(dssdev,
> ovl->info.color_mode); diff --git
>> a/drivers/video/omap2/dss/overlay.c
>> b/drivers/video/omap2/dss/overlay.c
>> index 75642c2..f704d06 100644
>> --- a/drivers/video/omap2/dss/overlay.c
>> +++ b/drivers/video/omap2/dss/overlay.c
>> @@ -257,6 +257,37 @@ static ssize_t
> overlay_global_alpha_store(struct omap_overlay *ovl,
>>  	return size;
>>  }
>> 
>> +static ssize_t
> overlay_pre_multiplication_alpha_show(struct omap_overlay *ovl,
>> +		char *buf)
>> +{
>> +	return snprintf(buf, PAGE_SIZE, "%d\n",
>> +			ovl->info.pre_mult_alpha);
>> +}
>> +
>> +static ssize_t
> overlay_pre_multiplication_alpha_store(struct omap_overlay *ovl,
>> +		const char *buf, size_t size)
>> +{
>> +	int r;
>> +	struct omap_overlay_info info;
>> +
>> +	ovl->get_overlay_info(ovl, &info);
>> +
>> +	/* only GFX and Video2 plane support pre alpha multiplication */
> 
> Shouldn't you check what the supported features are, instead
> of having a comment?
> 
>> +	info.pre_mult_alpha = simple_strtoul(buf, NULL, 10); +
>> +	r = ovl->set_overlay_info(ovl, &info);
>> +	if (r)
>> +		return r;
>> +
>> +	if (ovl->manager) {
>> +		r = ovl->manager->apply(ovl->manager);
>> +		if (r)
>> +			return r;
>> +	}
>> +
>> +	return size;
>> +}
>> +
>>  struct overlay_attribute {
>>  	struct attribute attr;
>>  	ssize_t (*show)(struct omap_overlay *, char *); @@ -280,6 +311,9 @@
>> static OVERLAY_ATTR(enabled, S_IRUGO|S_IWUSR,
>>  		overlay_enabled_show, overlay_enabled_store);  static
>> OVERLAY_ATTR(global_alpha, S_IRUGO|S_IWUSR,
>>  		overlay_global_alpha_show, overlay_global_alpha_store);
>> +static OVERLAY_ATTR(pre_multiplication_alpha, S_IRUGO|S_IWUSR,
>> +		overlay_pre_multiplication_alpha_show,
>> +		overlay_pre_multiplication_alpha_store);
>> 
>>  static struct attribute *overlay_sysfs_attrs[] = { 
>> &overlay_attr_name.attr, @@ -290,6 +324,7 @@ static struct attribute
>>  	*overlay_sysfs_attrs[] = { &overlay_attr_output_size.attr,
>>  	&overlay_attr_enabled.attr,
>>  	&overlay_attr_global_alpha.attr,
>> +	&overlay_attr_pre_multiplication_alpha.attr,
>>  	NULL
>>  };

RE: [PATCH] OMAP3630: DSS2: Enable Pre-Multiplied Alpha Support

[Tomi Valkeinen]

On Wed, 2010-11-03 at 08:57 +0100, ext Taneja, Archit wrote:
> Hi,
> 
> linux-omap-owner@vger.kernel.org wrote:
> > Alpha Support
> > 
> 
> [snip]
> 
> >> 
> >> +static void _dispc_set_pre_mult_alpha(enum omap_plane plane, bool +enable) {
> >> +	if (!dss_has_feature(FEAT_PREMUL_ALPHA))
> >> +		return;
> >> +
> >> +	BUG_ON(!dss_has_feature(FEAT_GLOBAL_ALPHA_VID1) &&
> >> +		plane == OMAP_DSS_VIDEO1);
> > 
> > What is the rationale for having the function return, if
> > FEAT_PREMUL_ALPHA is not supported, but BUG if plane is video1 and
> > GLOBAL_ALPHA_VID1 is not supported?
> 
> Premultiplied alpha is available on omap36xx and above, but on 36xx
> since global alpha itself isn't supported for video1 then writing to
> the pre multiplied alpha bit is incorrect.
> 
> It could have been possible to make a new feature like FEAT_PRE_MULT_VID1
> but I think its redundant as FEAT_GLOBAL_ALPHA_VID1 is enough to determine
> if we should set the pre multiplied alpha bit for video1 plane or not.

I was referring to the first check using return, and the second using
BUG(). If nobody is supposed to call that function when

!dss_has_feature(FEAT_GLOBAL_ALPHA_VID1) && plane == OMAP_DSS_VIDEO1)

then why is it ok to call that function when 

!dss_has_feature(FEAT_PREMUL_ALPHA)

Shouldn't they both be either returns or BUGs?

  Tomi

RE: [PATCH] OMAP3630: DSS2: Enable Pre-Multiplied Alpha Support

[Taneja, Archit]

Hi,

Tomi Valkeinen wrote:
> On Wed, 2010-11-03 at 08:57 +0100, ext Taneja, Archit wrote:
>> Hi,
>> 
>> linux-omap-owner@vger.kernel.org wrote:
>>> Alpha Support
>>> 
>> 
>> [snip]
>> 
>>>> 
>>>> +static void _dispc_set_pre_mult_alpha(enum omap_plane plane, bool
>>>> +enable) { +	if (!dss_has_feature(FEAT_PREMUL_ALPHA))
>>>> +		return;
>>>> +
>>>> +	BUG_ON(!dss_has_feature(FEAT_GLOBAL_ALPHA_VID1) &&
>>>> +		plane == OMAP_DSS_VIDEO1);
>>> 
>>> What is the rationale for having the function return, if
>>> FEAT_PREMUL_ALPHA is not supported, but BUG if plane is video1 and
>>> GLOBAL_ALPHA_VID1 is not supported?
>> 
>> Premultiplied alpha is available on omap36xx and above, but on 36xx
>> since global alpha itself isn't supported for video1 then writing to
>> the pre multiplied alpha bit is incorrect.
>> 
>> It could have been possible to make a new feature like
>> FEAT_PRE_MULT_VID1 but I think its redundant as FEAT_GLOBAL_ALPHA_VID1
>> is enough to determine if we should set the pre multiplied
> alpha bit for video1 plane or not.
> 
> I was referring to the first check using return, and the
> second using BUG(). If nobody is supposed to call that function when
> 
> !dss_has_feature(FEAT_GLOBAL_ALPHA_VID1) && plane == OMAP_DSS_VIDEO1)
> 
> then why is it ok to call that function when
> 
> !dss_has_feature(FEAT_PREMUL_ALPHA)
> 
> Shouldn't they both be either returns or BUGs?
> 

If you see the current code, _dispc_setup_global_alpha() does the same thing,
and in the call to it in _dispc_setup_plane() is:

...
...
	if (plane != OMAP_DSS_VIDEO1)
		_dispc_setup_global_alpha(plane, global_alpha);
...
...

So, I guess this BUG_ON is probably not required for both setup_global_alpha
and setup_pre_multiplied_alpha if you take care while calling these functions
from _dispc_setup_plane().

But this is the same with _dispc_set_scaling(), _dispc_set_vid_size() etc where
we have a BUG_ON(plane == OMAP_DSS_GFX) even when we take care of not calling it
for the GFX pipe.

Archit

RE: [PATCH] OMAP3630: DSS2: Enable Pre-Multiplied Alpha Support

[Nilofer, Samreen]

Hi,

Taneja, Archit wrote:
> Hi,
> 
> Tomi Valkeinen wrote:
>> On Wed, 2010-11-03 at 08:57 +0100, ext Taneja, Archit wrote:
>>> Hi,
>>> 
>>> linux-omap-owner@vger.kernel.org wrote:
>>>> Alpha Support
>>>> 
>>> 
>>> [snip]
>>> 
>>>>> 
>>>>> +static void _dispc_set_pre_mult_alpha(enum omap_plane plane, bool
>>>>> +enable) { +	if (!dss_has_feature(FEAT_PREMUL_ALPHA)) +		return;
>>>>> +
>>>>> +	BUG_ON(!dss_has_feature(FEAT_GLOBAL_ALPHA_VID1) &&
>>>>> +		plane == OMAP_DSS_VIDEO1);
>>>> 
>>>> What is the rationale for having the function return, if
>>>> FEAT_PREMUL_ALPHA is not supported, but BUG if plane is video1 and
>>>> GLOBAL_ALPHA_VID1 is not supported?
>>> 
>>> Premultiplied alpha is available on omap36xx and above, but on 36xx
>>> since global alpha itself isn't supported for video1 then writing to
>>> the pre multiplied alpha bit is incorrect.
>>> 
>>> It could have been possible to make a new feature like
>>> FEAT_PRE_MULT_VID1 but I think its redundant as
>>> FEAT_GLOBAL_ALPHA_VID1 is enough to determine if we should set the
>>> pre multiplied
>> alpha bit for video1 plane or not.
>> 
>> I was referring to the first check using return, and the second using
>> BUG(). If nobody is supposed to call that function when
>> 
>> !dss_has_feature(FEAT_GLOBAL_ALPHA_VID1) && plane == OMAP_DSS_VIDEO1)
>> 
>> then why is it ok to call that function when
>> 
>> !dss_has_feature(FEAT_PREMUL_ALPHA)
>> 
>> Shouldn't they both be either returns or BUGs?
>> 
> 
> If you see the current code, _dispc_setup_global_alpha() does
> the same thing, and in the call to it in _dispc_setup_plane() is:
> 
> ...
> ...
> 	if (plane != OMAP_DSS_VIDEO1)
> 		_dispc_setup_global_alpha(plane, global_alpha); ... ...
> 
> So, I guess this BUG_ON is probably not required for both
> setup_global_alpha and setup_pre_multiplied_alpha if you take
> care while calling these functions from _dispc_setup_plane().
> 
> But this is the same with _dispc_set_scaling(),
> _dispc_set_vid_size() etc where we have a BUG_ON(plane ==
> OMAP_DSS_GFX) even when we take care of not calling it for
> the GFX pipe.
> 
> Archit

[Samreen]
     If this clarification is aligned, should I go ahead and post the patch
with the remaining review comments

Regards,
Samreen

RE: [PATCH] OMAP3630: DSS2: Enable Pre-Multiplied Alpha Support

[Nilofer, Samreen]

Hi,

Taneja, Archit wrote:
> Hi,
> 
> Tomi Valkeinen wrote:
>> On Wed, 2010-11-03 at 08:57 +0100, ext Taneja, Archit wrote:
>>> Hi,
>>> 
>>> linux-omap-owner@vger.kernel.org wrote:
>>>> Alpha Support
>>>> 
>>> 
>>> [snip]
>>> 
>>>>> 
>>>>> +static void _dispc_set_pre_mult_alpha(enum omap_plane plane, bool
>>>>> +enable) { +	if (!dss_has_feature(FEAT_PREMUL_ALPHA)) +		return;
>>>>> +
>>>>> +	BUG_ON(!dss_has_feature(FEAT_GLOBAL_ALPHA_VID1) &&
>>>>> +		plane = OMAP_DSS_VIDEO1);
>>>> 
>>>> What is the rationale for having the function return, if
>>>> FEAT_PREMUL_ALPHA is not supported, but BUG if plane is video1 and
>>>> GLOBAL_ALPHA_VID1 is not supported?
>>> 
>>> Premultiplied alpha is available on omap36xx and above, but on 36xx
>>> since global alpha itself isn't supported for video1 then writing to
>>> the pre multiplied alpha bit is incorrect.
>>> 
>>> It could have been possible to make a new feature like
>>> FEAT_PRE_MULT_VID1 but I think its redundant as
>>> FEAT_GLOBAL_ALPHA_VID1 is enough to determine if we should set the
>>> pre multiplied
>> alpha bit for video1 plane or not.
>> 
>> I was referring to the first check using return, and the second using
>> BUG(). If nobody is supposed to call that function when
>> 
>> !dss_has_feature(FEAT_GLOBAL_ALPHA_VID1) && plane = OMAP_DSS_VIDEO1)
>> 
>> then why is it ok to call that function when
>> 
>> !dss_has_feature(FEAT_PREMUL_ALPHA)
>> 
>> Shouldn't they both be either returns or BUGs?
>> 
> 
> If you see the current code, _dispc_setup_global_alpha() does
> the same thing, and in the call to it in _dispc_setup_plane() is:
> 
> ...
> ...
> 	if (plane != OMAP_DSS_VIDEO1)
> 		_dispc_setup_global_alpha(plane, global_alpha); ... ...
> 
> So, I guess this BUG_ON is probably not required for both
> setup_global_alpha and setup_pre_multiplied_alpha if you take
> care while calling these functions from _dispc_setup_plane().
> 
> But this is the same with _dispc_set_scaling(),
> _dispc_set_vid_size() etc where we have a BUG_ON(plane =
> OMAP_DSS_GFX) even when we take care of not calling it for
> the GFX pipe.
> 
> Archit

[Samreen]
     If this clarification is aligned, should I go ahead and post the patch
with the remaining review comments

Regards,
Samreen

RE: [PATCH] OMAP3630: DSS2: Enable Pre-Multiplied Alpha Support

[Tomi Valkeinen]

On Wed, 2010-11-03 at 12:57 +0100, ext Taneja, Archit wrote:
> Hi,
> 
> Tomi Valkeinen wrote:
> > On Wed, 2010-11-03 at 08:57 +0100, ext Taneja, Archit wrote:
> >> Hi,
> >> 
> >> linux-omap-owner@vger.kernel.org wrote:
> >>> Alpha Support
> >>> 
> >> 
> >> [snip]
> >> 
> >>>> 
> >>>> +static void _dispc_set_pre_mult_alpha(enum omap_plane plane, bool
> >>>> +enable) { +	if (!dss_has_feature(FEAT_PREMUL_ALPHA))
> >>>> +		return;
> >>>> +
> >>>> +	BUG_ON(!dss_has_feature(FEAT_GLOBAL_ALPHA_VID1) &&
> >>>> +		plane == OMAP_DSS_VIDEO1);
> >>> 
> >>> What is the rationale for having the function return, if
> >>> FEAT_PREMUL_ALPHA is not supported, but BUG if plane is video1 and
> >>> GLOBAL_ALPHA_VID1 is not supported?
> >> 
> >> Premultiplied alpha is available on omap36xx and above, but on 36xx
> >> since global alpha itself isn't supported for video1 then writing to
> >> the pre multiplied alpha bit is incorrect.
> >> 
> >> It could have been possible to make a new feature like
> >> FEAT_PRE_MULT_VID1 but I think its redundant as FEAT_GLOBAL_ALPHA_VID1
> >> is enough to determine if we should set the pre multiplied
> > alpha bit for video1 plane or not.
> > 
> > I was referring to the first check using return, and the
> > second using BUG(). If nobody is supposed to call that function when
> > 
> > !dss_has_feature(FEAT_GLOBAL_ALPHA_VID1) && plane == OMAP_DSS_VIDEO1)
> > 
> > then why is it ok to call that function when
> > 
> > !dss_has_feature(FEAT_PREMUL_ALPHA)
> > 
> > Shouldn't they both be either returns or BUGs?
> > 
> 
> If you see the current code, _dispc_setup_global_alpha() does the same thing,
> and in the call to it in _dispc_setup_plane() is:
> 
> ...
> ...
> 	if (plane != OMAP_DSS_VIDEO1)
> 		_dispc_setup_global_alpha(plane, global_alpha);
> ...
> ...
> 
> So, I guess this BUG_ON is probably not required for both setup_global_alpha
> and setup_pre_multiplied_alpha if you take care while calling these functions
> from _dispc_setup_plane().

BUGs should be used exactly in those cases, where you are sure you're
always calling the function with right parameters. BUGs are never meant
to trigger, so they should be used as a safeguard against broken
changes.

> But this is the same with _dispc_set_scaling(), _dispc_set_vid_size() etc where
> we have a BUG_ON(plane == OMAP_DSS_GFX) even when we take care of not calling it
> for the GFX pipe.

Yes, I'm sure there are functions that are not perfect =).

So to summarize: if _dispc_set_pre_mult_alpha() is only called from
inside DSS, and it will never be called with a wrong plane argument,
then it should only use BUGs, not return.

Now, the question is, is it better to do the argument checking in the
caller or in the callee? In the case of DSS internal functions it's
perhaps easier if the callee does the checking. So:

_dispc_setup_plane() should always call _dispc_setup_global_alpha(), as
future OMAP versions may support global alpha on any overlay, and I
don't think it's _dispc_setup_plane's job to do that check as the checks
may be lenghty. 

_dispc_setup_global_alpha() would do the check if global_alpha is
supported on that plane, and do nothing if not (or, return an error,
which _dispc_setup_plane() would ignore, but I don't think that's
needed).

And similarly for premult alpha. And I'm not saying all these have to be
changed now, but as general thoughts about the matter.

 Tomi

Re: [PATCH] OMAP3630:DSS2:Enable Pre-Multiplied Alpha Support

[Vladimir Pantelic]

Y, Kishore wrote:

>>  >  As per TRM, this bit is valid only for ARGB formats and experts
>>  suggested that we can safely assume pre-multiplied data always in real
>>  world
>>
>>  I asked a few experts here, and they weren't so sure, and neither am I.
>>
>>  I don't see any problems making this feature configurable, but there may
>>  be problems if it's hardcoded. So, it should be configurable. I think
>>  the default should be the same as on 3430, so that they will work
>>  similarly.
>>
>>   Tomi
> I haven't considered the backward compatibility with 3430 apps.
> I will make this configurable.

And please defaulting to non-pre-multiplied. All 3430 apps will be
non-pre-multiplied as this is what the 3430 supports only.

So apps need to be actively changed to use pre-multiplied.

RE: [PATCH] OMAP3630:DSS2:Enable Pre-Multiplied Alpha Support

[Y, Kishore]

> -----Original Message-----
> From: Tomi Valkeinen [mailto:tomi.valkeinen@nokia.com]
> Sent: Monday, January 18, 2010 5:54 PM
> To: Y, Kishore
> Cc: linux-omap@vger.kernel.org; Mande, Nikhil
> Subject: RE: [PATCH] OMAP3630:DSS2:Enable Pre-Multiplied Alpha Support
> 
> On Mon, 2010-01-18 at 12:26 +0100, ext Y, Kishore wrote:
> > > -----Original Message-----
> > > From: Tomi Valkeinen [mailto:tomi.valkeinen@nokia.com]
> > > Sent: Friday, January 15, 2010 4:00 PM
> > > To: Y, Kishore
> > > Cc: linux-omap@vger.kernel.org
> > > Subject: Re: [PATCH] OMAP3630:DSS2:Enable Pre-Multiplied Alpha Support
> > >
> > > Hi,
> > >
> > > On Mon, 2009-12-21 at 16:06 +0100, ext Y, Kishore wrote:
> > > > From 2f873819a4b9eb0bd658db1e59408d8f0aeb14b6 Mon Sep 17 00:00:00
> 2001
> > > > From: Sudeep Basavaraj <sudeep.basavaraj@ti.com>
> > > > Date: Mon, 14 Dec 2009 18:54:51 +0530
> > > > Subject: [PATCH] OMAP3630:DSS2:Enable Pre-Multiplied Alpha Support
> > > >
> > > > Enables dss to process color formats with pre-mulitplied alpha
> values.
> > > > With this we can have alpha values defined for each pixel
> > > > and hence can have different blending values for each pixel.
> > >
> > > What does pre-multiplied alpha mean? The TRM didn't really open it
> up...
> > > Don't we already have per pixel alpha when using ARGB/RGBA?
> >
> > When we set pixel format to ARGB/RGBA dss alpha blender would multiply
> the value present in 'A' with 'RGB'.
> > By setting this bit display hardware assumes that the R,G,B are already
> multiplied with the alpha value and there is no need to multiply again.
> >
> > Ex:-
> > 			A	R	G	B
> > argb data		128	255	128	100
> > pre-multiplied	128	128	64	50
> >
> > So this bit, when set, would not multiply the pixel with alpha value.
> 
> Ok, after staring long enough to the TRM's alpha-picture, I got it =).
> 
> But having this bit always on on 3630 would mean that the software
> designed for 3430 would not work properly on 3630, wouldn't it?
> 
> > > This patch seems to always set the bit on, never set it off. Is that
> the
> > > purpose?
> >
> > As per TRM, this bit is valid only for ARGB formats and experts
> suggested that we can safely assume pre-multiplied data always in real
> world
> 
> I asked a few experts here, and they weren't so sure, and neither am I.
> 
> I don't see any problems making this feature configurable, but there may
> be problems if it's hardcoded. So, it should be configurable. I think
> the default should be the same as on 3430, so that they will work
> similarly.
> 
>  Tomi
I haven't considered the backward compatibility with 3430 apps. 
I will make this configurable.
kishore
> 

RE: [PATCH] OMAP3630:DSS2:Enable Pre-Multiplied Alpha Support

[Tomi Valkeinen]

On Mon, 2010-01-18 at 12:26 +0100, ext Y, Kishore wrote:
> > -----Original Message-----
> > From: Tomi Valkeinen [mailto:tomi.valkeinen@nokia.com]
> > Sent: Friday, January 15, 2010 4:00 PM
> > To: Y, Kishore
> > Cc: linux-omap@vger.kernel.org
> > Subject: Re: [PATCH] OMAP3630:DSS2:Enable Pre-Multiplied Alpha Support
> > 
> > Hi,
> > 
> > On Mon, 2009-12-21 at 16:06 +0100, ext Y, Kishore wrote:
> > > From 2f873819a4b9eb0bd658db1e59408d8f0aeb14b6 Mon Sep 17 00:00:00 2001
> > > From: Sudeep Basavaraj <sudeep.basavaraj@ti.com>
> > > Date: Mon, 14 Dec 2009 18:54:51 +0530
> > > Subject: [PATCH] OMAP3630:DSS2:Enable Pre-Multiplied Alpha Support
> > >
> > > Enables dss to process color formats with pre-mulitplied alpha values.
> > > With this we can have alpha values defined for each pixel
> > > and hence can have different blending values for each pixel.
> > 
> > What does pre-multiplied alpha mean? The TRM didn't really open it up...
> > Don't we already have per pixel alpha when using ARGB/RGBA?
> 
> When we set pixel format to ARGB/RGBA dss alpha blender would multiply the value present in 'A' with 'RGB'.
> By setting this bit display hardware assumes that the R,G,B are already multiplied with the alpha value and there is no need to multiply again.
> 
> Ex:-
> 			A	R	G	B
> argb data		128	255	128	100
> pre-multiplied	128	128	64	50
> 
> So this bit, when set, would not multiply the pixel with alpha value.

Ok, after staring long enough to the TRM's alpha-picture, I got it =).

But having this bit always on on 3630 would mean that the software
designed for 3430 would not work properly on 3630, wouldn't it?

> > This patch seems to always set the bit on, never set it off. Is that the
> > purpose?
> 
> As per TRM, this bit is valid only for ARGB formats and experts suggested that we can safely assume pre-multiplied data always in real world

I asked a few experts here, and they weren't so sure, and neither am I.

I don't see any problems making this feature configurable, but there may
be problems if it's hardcoded. So, it should be configurable. I think
the default should be the same as on 3430, so that they will work
similarly.

 Tomi

RE: [PATCH] OMAP3630:DSS2:Enable Pre-Multiplied Alpha Support

[Y, Kishore]

> -----Original Message-----
> From: Tomi Valkeinen [mailto:tomi.valkeinen@nokia.com]
> Sent: Friday, January 15, 2010 4:00 PM
> To: Y, Kishore
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCH] OMAP3630:DSS2:Enable Pre-Multiplied Alpha Support
> 
> Hi,
> 
> On Mon, 2009-12-21 at 16:06 +0100, ext Y, Kishore wrote:
> > From 2f873819a4b9eb0bd658db1e59408d8f0aeb14b6 Mon Sep 17 00:00:00 2001
> > From: Sudeep Basavaraj <sudeep.basavaraj@ti.com>
> > Date: Mon, 14 Dec 2009 18:54:51 +0530
> > Subject: [PATCH] OMAP3630:DSS2:Enable Pre-Multiplied Alpha Support
> >
> > Enables dss to process color formats with pre-mulitplied alpha values.
> > With this we can have alpha values defined for each pixel
> > and hence can have different blending values for each pixel.
> 
> What does pre-multiplied alpha mean? The TRM didn't really open it up...
> Don't we already have per pixel alpha when using ARGB/RGBA?

When we set pixel format to ARGB/RGBA dss alpha blender would multiply the value present in 'A' with 'RGB'.
By setting this bit display hardware assumes that the R,G,B are already multiplied with the alpha value and there is no need to multiply again.

Ex:-
			A	R	G	B
argb data		128	255	128	100
pre-multiplied	128	128	64	50

So this bit, when set, would not multiply the pixel with alpha value.

> 
> This patch seems to always set the bit on, never set it off. Is that the
> purpose?

As per TRM, this bit is valid only for ARGB formats and experts suggested that we can safely assume pre-multiplied data always in real world

> 
>  Tomi
> 
> 
> > Signed-off-by: Sudeep Basavaraj <sudeep.basavaraj@ti.com>
> > Signed-off-by: Kishore Y <kishore.y@ti.com>
> > ---
> >  drivers/video/omap2/dss/dispc.c |    8 ++++++++
> >  1 files changed, 8 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/video/omap2/dss/dispc.c
> b/drivers/video/omap2/dss/dispc.c
> > index 6dabf4b..5f7819b 100644
> > --- a/drivers/video/omap2/dss/dispc.c
> > +++ b/drivers/video/omap2/dss/dispc.c
> > @@ -913,6 +913,11 @@ static void _dispc_set_vid_color_conv(enum
> omap_plane plane, bool enable)
> >  	dispc_write_reg(dispc_reg_att[plane], val);
> >  }
> >
> > +static void _dispc_set_alpha_blend_attrs(enum omap_plane plane, bool
> enable)
> > +{
> > +	REG_FLD_MOD(dispc_reg_att[plane], enable ? 1 : 0, 28, 28);
> > +}
> > +
> >  void dispc_enable_replication(enum omap_plane plane, bool enable)
> >  {
> >  	int bit;
> > @@ -1689,6 +1694,9 @@ static int _dispc_setup_plane(enum omap_plane
> plane,
> >
> >  	_dispc_set_rotation_attrs(plane, rotation, mirror, color_mode);
> >
> > +	if (cpu_is_omap3630() && (plane != OMAP_DSS_VIDEO1))
> > +		_dispc_set_alpha_blend_attrs(plane, 1);
> > +
> >  	if (plane != OMAP_DSS_VIDEO1)
> >  		_dispc_setup_global_alpha(plane, global_alpha);
> >
> 

Re: [PATCH] OMAP3630:DSS2:Enable Pre-Multiplied Alpha Support

[Tomi Valkeinen]

Hi,

On Mon, 2009-12-21 at 16:06 +0100, ext Y, Kishore wrote:
> From 2f873819a4b9eb0bd658db1e59408d8f0aeb14b6 Mon Sep 17 00:00:00 2001
> From: Sudeep Basavaraj <sudeep.basavaraj@ti.com>
> Date: Mon, 14 Dec 2009 18:54:51 +0530
> Subject: [PATCH] OMAP3630:DSS2:Enable Pre-Multiplied Alpha Support
> 
> Enables dss to process color formats with pre-mulitplied alpha values.
> With this we can have alpha values defined for each pixel
> and hence can have different blending values for each pixel.

What does pre-multiplied alpha mean? The TRM didn't really open it up...
Don't we already have per pixel alpha when using ARGB/RGBA?

This patch seems to always set the bit on, never set it off. Is that the
purpose?

 Tomi


> Signed-off-by: Sudeep Basavaraj <sudeep.basavaraj@ti.com>
> Signed-off-by: Kishore Y <kishore.y@ti.com>
> ---
>  drivers/video/omap2/dss/dispc.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
> index 6dabf4b..5f7819b 100644
> --- a/drivers/video/omap2/dss/dispc.c
> +++ b/drivers/video/omap2/dss/dispc.c
> @@ -913,6 +913,11 @@ static void _dispc_set_vid_color_conv(enum omap_plane plane, bool enable)
>  	dispc_write_reg(dispc_reg_att[plane], val);
>  }
>  
> +static void _dispc_set_alpha_blend_attrs(enum omap_plane plane, bool enable)
> +{
> +	REG_FLD_MOD(dispc_reg_att[plane], enable ? 1 : 0, 28, 28);
> +}
> +
>  void dispc_enable_replication(enum omap_plane plane, bool enable)
>  {
>  	int bit;
> @@ -1689,6 +1694,9 @@ static int _dispc_setup_plane(enum omap_plane plane,
>  
>  	_dispc_set_rotation_attrs(plane, rotation, mirror, color_mode);
>  
> +	if (cpu_is_omap3630() && (plane != OMAP_DSS_VIDEO1))
> +		_dispc_set_alpha_blend_attrs(plane, 1);
> +
>  	if (plane != OMAP_DSS_VIDEO1)
>  		_dispc_setup_global_alpha(plane, global_alpha);
>  

[PATCH] OMAP3630:DSS2:Enable Pre-Multiplied Alpha Support

[Y, Kishore]

>From 2f873819a4b9eb0bd658db1e59408d8f0aeb14b6 Mon Sep 17 00:00:00 2001
From: Sudeep Basavaraj <sudeep.basavaraj@ti.com>
Date: Mon, 14 Dec 2009 18:54:51 +0530
Subject: [PATCH] OMAP3630:DSS2:Enable Pre-Multiplied Alpha Support

Enables dss to process color formats with pre-mulitplied alpha values.
With this we can have alpha values defined for each pixel
and hence can have different blending values for each pixel.

Signed-off-by: Sudeep Basavaraj <sudeep.basavaraj@ti.com>
Signed-off-by: Kishore Y <kishore.y@ti.com>
---
 drivers/video/omap2/dss/dispc.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/drivers/video/omap2/dss/dispc.c b/drivers/video/omap2/dss/dispc.c
index 6dabf4b..5f7819b 100644
--- a/drivers/video/omap2/dss/dispc.c
+++ b/drivers/video/omap2/dss/dispc.c
@@ -913,6 +913,11 @@ static void _dispc_set_vid_color_conv(enum omap_plane plane, bool enable)
 	dispc_write_reg(dispc_reg_att[plane], val);
 }
 
+static void _dispc_set_alpha_blend_attrs(enum omap_plane plane, bool enable)
+{
+	REG_FLD_MOD(dispc_reg_att[plane], enable ? 1 : 0, 28, 28);
+}
+
 void dispc_enable_replication(enum omap_plane plane, bool enable)
 {
 	int bit;
@@ -1689,6 +1694,9 @@ static int _dispc_setup_plane(enum omap_plane plane,
 
 	_dispc_set_rotation_attrs(plane, rotation, mirror, color_mode);
 
+	if (cpu_is_omap3630() && (plane != OMAP_DSS_VIDEO1))
+		_dispc_set_alpha_blend_attrs(plane, 1);
+
 	if (plane != OMAP_DSS_VIDEO1)
 		_dispc_setup_global_alpha(plane, global_alpha);
 
-- 
1.5.4.3


Regards,
Kishore Y