Re: [RFC 1/1] drm/i915: Added support for setting plane alpha through drm property

Re: [RFC 1/1] drm/i915: Added support for setting plane alpha through drm property

[Ville Syrjälä]

On Wed, Feb 19, 2014 at 03:38:08PM +0530, sagar.a.kamble@intel.com wrote:
> From: Sagar Kamble <sagar.a.kamble@intel.com>
> 
> With this patch two properties are added. One for CRTC+Sprite planes
> and another for Cursor planes. Through these client will be able to
> change the pixel format of the planes w.r.t Alpha channel.
> Number of drm properties are limited so should we restrain from adding this
> as drm property or should we expose this as IOCTL itself.

We need to stop adding properties on a whim and design and document them
properly. So what I'd like to see is someone going over the current
properties and collecting them up in some (even bares bones) documentation
in Documentation/Docbook. The current way will lead to a huge mess when
userspace actually starts to depend on properties. So far properties
have been mostly some optional extra junk on the side people that may
want to frob, but that's going to change as we add more of them,
especially with plane and crtc properties, which actually affect how
the scene gets composed together.

And I think we need to put a hold on adding the plane properties to the
crtc since the plan is to convert everything to drm_plane. With the
current rate we're going to have a ton of properties on the crtc that no
one will use. Adding properties to the sprite planes seems OK in the
meantime.

As far as alpha blending is concerned I've had the following ideas:
- we need a plane property for constant alpha. Some drivers might
  already have this, so might be good to check. Although I'm fairly
  sure what's there won't be entirely future proof. I was thinking that
  we should standardize of using 16bits for color components in
  properties. That way you can still stick a full ARGB value into
  one property, and we should be good for a few more years until
  someone has the idea to move beyond 16bits per channel. And it's
  more or less hardware agnostic. Obviously if the hardware won't
  use the full precision, you get to throw away the low bits, but I
  don't think there's any other good way to go.
- we need another property to indicate whether the source pixels
  are premultiplied or not. Or maybe it's easier for people to think
  in terms of what operations the hardware will do, in which case
  we should make the property indicate whether the hardware will
  do the premultiplication during blending or not. I'm not sure
  which approach feels more natural to people.
- And finally we need to figure out how to blend it all together.
  It might make sense to model this after glBlendFunc(), so it would
  be an enum property, or maybe two if we want separate properties
  for source and destination factors.

Obviously the final result will depend on additional things like the
z-order, which is going to be another property. I think this one might
already exists in some form in other drivers. So we should definitely
look at what's there and try to do the same if possible. Which again
underlines the need to collect up the current properties into some
central documentation.

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [RFC 1/1] drm/i915: Added support for setting plane alpha through drm property

[Daniel Vetter]

On Tue, Feb 25, 2014 at 08:18:30PM +0200, Ville Syrjälä wrote:
> On Wed, Feb 19, 2014 at 03:38:08PM +0530, sagar.a.kamble@intel.com wrote:
> > From: Sagar Kamble <sagar.a.kamble@intel.com>
> > 
> > With this patch two properties are added. One for CRTC+Sprite planes
> > and another for Cursor planes. Through these client will be able to
> > change the pixel format of the planes w.r.t Alpha channel.
> > Number of drm properties are limited so should we restrain from adding this
> > as drm property or should we expose this as IOCTL itself.
> 
> We need to stop adding properties on a whim and design and document them
> properly. So what I'd like to see is someone going over the current
> properties and collecting them up in some (even bares bones) documentation
> in Documentation/Docbook. The current way will lead to a huge mess when
> userspace actually starts to depend on properties. So far properties
> have been mostly some optional extra junk on the side people that may
> want to frob, but that's going to change as we add more of them,
> especially with plane and crtc properties, which actually affect how
> the scene gets composed together.
> 
> And I think we need to put a hold on adding the plane properties to the
> crtc since the plan is to convert everything to drm_plane. With the
> current rate we're going to have a ton of properties on the crtc that no
> one will use. Adding properties to the sprite planes seems OK in the
> meantime.
> 
> As far as alpha blending is concerned I've had the following ideas:
> - we need a plane property for constant alpha. Some drivers might
>   already have this, so might be good to check. Although I'm fairly
>   sure what's there won't be entirely future proof. I was thinking that
>   we should standardize of using 16bits for color components in
>   properties. That way you can still stick a full ARGB value into
>   one property, and we should be good for a few more years until
>   someone has the idea to move beyond 16bits per channel. And it's
>   more or less hardware agnostic. Obviously if the hardware won't
>   use the full precision, you get to throw away the low bits, but I
>   don't think there's any other good way to go.
> - we need another property to indicate whether the source pixels
>   are premultiplied or not. Or maybe it's easier for people to think
>   in terms of what operations the hardware will do, in which case
>   we should make the property indicate whether the hardware will
>   do the premultiplication during blending or not. I'm not sure
>   which approach feels more natural to people.
> - And finally we need to figure out how to blend it all together.
>   It might make sense to model this after glBlendFunc(), so it would
>   be an enum property, or maybe two if we want separate properties
>   for source and destination factors.
> 
> Obviously the final result will depend on additional things like the
> z-order, which is going to be another property. I think this one might
> already exists in some form in other drivers. So we should definitely
> look at what's there and try to do the same if possible. Which again
> underlines the need to collect up the current properties into some
> central documentation.

Concurred on stealing the blending model from GL. It seems what everyon
else is aiming for at least at both the hw and sw level ... One issue with
that is handling color keys, since those aren't supported by glBlendFunc.
But I guess we could just add those as additional modes, since the usual
blend funcs already require a constant blend color, which could be reused
as the color key.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [RFC 1/1] drm/i915: Added support for setting plane alpha through drm property

[Ville Syrjälä]

On Tue, Mar 04, 2014 at 10:42:38AM +0100, Daniel Vetter wrote:
> On Tue, Feb 25, 2014 at 08:18:30PM +0200, Ville Syrjälä wrote:
> > On Wed, Feb 19, 2014 at 03:38:08PM +0530, sagar.a.kamble@intel.com wrote:
> > > From: Sagar Kamble <sagar.a.kamble@intel.com>
> > > 
> > > With this patch two properties are added. One for CRTC+Sprite planes
> > > and another for Cursor planes. Through these client will be able to
> > > change the pixel format of the planes w.r.t Alpha channel.
> > > Number of drm properties are limited so should we restrain from adding this
> > > as drm property or should we expose this as IOCTL itself.
> > 
> > We need to stop adding properties on a whim and design and document them
> > properly. So what I'd like to see is someone going over the current
> > properties and collecting them up in some (even bares bones) documentation
> > in Documentation/Docbook. The current way will lead to a huge mess when
> > userspace actually starts to depend on properties. So far properties
> > have been mostly some optional extra junk on the side people that may
> > want to frob, but that's going to change as we add more of them,
> > especially with plane and crtc properties, which actually affect how
> > the scene gets composed together.
> > 
> > And I think we need to put a hold on adding the plane properties to the
> > crtc since the plan is to convert everything to drm_plane. With the
> > current rate we're going to have a ton of properties on the crtc that no
> > one will use. Adding properties to the sprite planes seems OK in the
> > meantime.
> > 
> > As far as alpha blending is concerned I've had the following ideas:
> > - we need a plane property for constant alpha. Some drivers might
> >   already have this, so might be good to check. Although I'm fairly
> >   sure what's there won't be entirely future proof. I was thinking that
> >   we should standardize of using 16bits for color components in
> >   properties. That way you can still stick a full ARGB value into
> >   one property, and we should be good for a few more years until
> >   someone has the idea to move beyond 16bits per channel. And it's
> >   more or less hardware agnostic. Obviously if the hardware won't
> >   use the full precision, you get to throw away the low bits, but I
> >   don't think there's any other good way to go.
> > - we need another property to indicate whether the source pixels
> >   are premultiplied or not. Or maybe it's easier for people to think
> >   in terms of what operations the hardware will do, in which case
> >   we should make the property indicate whether the hardware will
> >   do the premultiplication during blending or not. I'm not sure
> >   which approach feels more natural to people.
> > - And finally we need to figure out how to blend it all together.
> >   It might make sense to model this after glBlendFunc(), so it would
> >   be an enum property, or maybe two if we want separate properties
> >   for source and destination factors.
> > 
> > Obviously the final result will depend on additional things like the
> > z-order, which is going to be another property. I think this one might
> > already exists in some form in other drivers. So we should definitely
> > look at what's there and try to do the same if possible. Which again
> > underlines the need to collect up the current properties into some
> > central documentation.
> 
> Concurred on stealing the blending model from GL. It seems what everyon
> else is aiming for at least at both the hw and sw level ... One issue with
> that is handling color keys, since those aren't supported by glBlendFunc.
> But I guess we could just add those as additional modes, since the usual
> blend funcs already require a constant blend color, which could be reused
> as the color key.

I've been thinking that color key stuff could just be implemented as
separate properties.

Color keying anyway may require min+max values or value+mask, so a single
value is not enough. But we might want to model the keying function in
a similar fashion to the blendfunc. Some old ATI hardware had a very
flexible color keying functinality where you would specify the graphics
and video keying functions (true,false,eq,neq) and then you had to
choose how to combine the result from those (and,or). If the combined
result was true it would pick the video data, and for false it'd pick
the graphics data. I've not seen that on any other hardware since, but
it's certainly flexible enough to represent the typical src/dst keying
modes.

-- 
Ville Syrjälä
Intel OTC

Re: [Intel-gfx] [RFC 1/1] drm/i915: Added support for setting plane alpha through drm property

[Sagar Arun Kamble]

On Tue, 2014-03-04 at 14:06 +0200, Ville Syrjälä wrote:
> On Tue, Mar 04, 2014 at 10:42:38AM +0100, Daniel Vetter wrote:
> > On Tue, Feb 25, 2014 at 08:18:30PM +0200, Ville Syrjälä wrote:
> > > On Wed, Feb 19, 2014 at 03:38:08PM +0530, sagar.a.kamble@intel.com wrote:
> > > > From: Sagar Kamble <sagar.a.kamble@intel.com>
> > > > 
> > > > With this patch two properties are added. One for CRTC+Sprite planes
> > > > and another for Cursor planes. Through these client will be able to
> > > > change the pixel format of the planes w.r.t Alpha channel.
> > > > Number of drm properties are limited so should we restrain from adding this
> > > > as drm property or should we expose this as IOCTL itself.
> > > 
> > > We need to stop adding properties on a whim and design and document them
> > > properly. So what I'd like to see is someone going over the current
> > > properties and collecting them up in some (even bares bones) documentation
> > > in Documentation/Docbook. The current way will lead to a huge mess when
> > > userspace actually starts to depend on properties. So far properties
> > > have been mostly some optional extra junk on the side people that may
> > > want to frob, but that's going to change as we add more of them,
> > > especially with plane and crtc properties, which actually affect how
> > > the scene gets composed together.
> > > 
> > > And I think we need to put a hold on adding the plane properties to the
> > > crtc since the plan is to convert everything to drm_plane. With the
> > > current rate we're going to have a ton of properties on the crtc that no
> > > one will use. Adding properties to the sprite planes seems OK in the
> > > meantime.
> > > 
> > > As far as alpha blending is concerned I've had the following ideas:
> > > - we need a plane property for constant alpha. Some drivers might
> > >   already have this, so might be good to check. Although I'm fairly
> > >   sure what's there won't be entirely future proof. I was thinking that
> > >   we should standardize of using 16bits for color components in
> > >   properties. That way you can still stick a full ARGB value into
> > >   one property, and we should be good for a few more years until
> > >   someone has the idea to move beyond 16bits per channel. And it's
> > >   more or less hardware agnostic. Obviously if the hardware won't
> > >   use the full precision, you get to throw away the low bits, but I
> > >   don't think there's any other good way to go.
> > > - we need another property to indicate whether the source pixels
> > >   are premultiplied or not. Or maybe it's easier for people to think
> > >   in terms of what operations the hardware will do, in which case
> > >   we should make the property indicate whether the hardware will
> > >   do the premultiplication during blending or not. I'm not sure
> > >   which approach feels more natural to people.
> > > - And finally we need to figure out how to blend it all together.
> > >   It might make sense to model this after glBlendFunc(), so it would
> > >   be an enum property, or maybe two if we want separate properties
> > >   for source and destination factors.
> > > 
> > > Obviously the final result will depend on additional things like the
> > > z-order, which is going to be another property. I think this one might
> > > already exists in some form in other drivers. So we should definitely
> > > look at what's there and try to do the same if possible. Which again
> > > underlines the need to collect up the current properties into some
> > > central documentation.
> > 
> > Concurred on stealing the blending model from GL. It seems what everyon
> > else is aiming for at least at both the hw and sw level ... One issue with
> > that is handling color keys, since those aren't supported by glBlendFunc.
> > But I guess we could just add those as additional modes, since the usual
> > blend funcs already require a constant blend color, which could be reused
> > as the color key.
> 
> I've been thinking that color key stuff could just be implemented as
> separate properties.
> 
> Color keying anyway may require min+max values or value+mask, so a single
> value is not enough. But we might want to model the keying function in
> a similar fashion to the blendfunc. Some old ATI hardware had a very
> flexible color keying functinality where you would specify the graphics
> and video keying functions (true,false,eq,neq) and then you had to
> choose how to combine the result from those (and,or). If the combined
> result was true it would pick the video data, and for false it'd pick
> the graphics data. I've not seen that on any other hardware since, but
> it's certainly flexible enough to represent the typical src/dst keying
> modes.
We can only model GL_CONSTANT_ALPHA given we have sprite control
registers for that. How do we model other pixel arithmetic related to
color blending?
How do we model source and destination pixel arithmetic? We can only set
alpha/pre-multiplied alpha for individual planes.

> 


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

Re: [RFC 1/1] drm/i915: Added support for setting plane alpha through drm property

[Daniel Vetter]

On Thu, Mar 6, 2014 at 11:28 AM, Sagar Arun Kamble
<sagar.a.kamble@intel.com> wrote:
> We can only model GL_CONSTANT_ALPHA given we have sprite control
> registers for that. How do we model other pixel arithmetic related to
> color blending?
> How do we model source and destination pixel arithmetic? We can only set
> alpha/pre-multiplied alpha for individual planes.

We reject it if userspace attempts it ;-) But we kinda want a model
which also works for the next few hw platforms and also on other
drivers, so going with this sounds like the right approach to me.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH 1/4] drm: Added plane alpha and color blending property

[sagar.a.kamble]

From: Sagar Kamble <sagar.a.kamble@intel.com>

This patch creates a generic blending enum property.
Drivers may support subset of these values.

Cc: airlied@linux.ie
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/drm_crtc.c | 33 +++++++++++++++++++++++++++++++++
 include/drm/drm_crtc.h     | 25 +++++++++++++++++++++++++
 2 files changed, 58 insertions(+)

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 4e43fc2..15281a3 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -4147,3 +4147,36 @@ void drm_mode_config_cleanup(struct drm_device *dev)
 	idr_destroy(&dev->mode_config.crtc_idr);
 }
 EXPORT_SYMBOL(drm_mode_config_cleanup);
+
+struct drm_property *drm_mode_create_blend_property(struct drm_device *dev,
+				unsigned int supported_factors)
+{
+	static const struct drm_prop_enum_list props[] = {
+		{ DRM_BLEND_NONE,   			"none" },
+		{ DRM_BLEND_ZERO,  			"zero" },
+		{ DRM_BLEND_ONE, 			"one" },
+		{ DRM_BLEND_SRC_COLOR, 			"src-color" },
+		{ DRM_BLEND_ONE_MINUS_SRC_COLOR,  	"one-minus-src-color" },
+		{ DRM_BLEND_DST_COLOR, 			"dst-color" },
+		{ DRM_BLEND_ONE_MINUS_DST_COLOR,  	"one-minus-dst-color" },
+		{ DRM_BLEND_SRC_ALPHA, 			"src-alpha" },
+		{ DRM_BLEND_ONE_MINUS_SRC_ALPHA, 	"one-minus-src-alpha" },
+		{ DRM_BLEND_DST_ALPHA, 			"dst-alpha" },
+		{ DRM_BLEND_ONE_MINUS_DST_ALPHA, 	"one-minus-dst-alpha" },
+		{ DRM_BLEND_CONSTANT_COLOR, 		"constant-color" },
+		{ DRM_BLEND_ONE_MINUS_CONSTANT_COLOR, 	"one-minus-constant-color" },
+		{ DRM_BLEND_CONSTANT_ALPHA, 		"constant-alpha" },
+		{ DRM_BLEND_ONE_MINUS_CONSTANT_ALPHA, 	"one-minus-constant-alpha" },
+		{ DRM_BLEND_SRC_ALPHA_SATURATE, 	"alpha-saturate" },
+		{ DRM_BLEND_SRC1_COLOR, 		"src1-color" },
+		{ DRM_BLEND_ONE_MINUS_SRC1_COLOR, 	"one-minus-src1-color" },
+		{ DRM_BLEND_SRC1_ALPHA, 		"src1-alpha" },
+		{ DRM_BLEND_ONE_MINUS_SRC1_ALPHA, 	"one-minus-src1-alpha" },
+		{ DRM_BLEND_PREMULTIPLIED_ALPHA, 	"pre-multiplied-alpha" }
+	};
+
+	return drm_property_create_bitmask(dev, 0, "blend",
+					   props, ARRAY_SIZE(props),
+					   supported_factors);
+}
+EXPORT_SYMBOL(drm_mode_create_blend_property);
diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
index 784a568..6c5847f 100644
--- a/include/drm/drm_crtc.h
+++ b/include/drm/drm_crtc.h
@@ -65,6 +65,29 @@ struct drm_object_properties {
 	uint64_t values[DRM_OBJECT_MAX_PROPERTY];
 };
 
+/* Blending property bits */
+#define DRM_BLEND_NONE				0
+#define DRM_BLEND_ZERO				1
+#define DRM_BLEND_ONE				2
+#define DRM_BLEND_SRC_COLOR			3
+#define DRM_BLEND_ONE_MINUS_SRC_COLOR		4
+#define DRM_BLEND_DST_COLOR			5
+#define DRM_BLEND_ONE_MINUS_DST_COLOR		6
+#define DRM_BLEND_SRC_ALPHA			7
+#define DRM_BLEND_ONE_MINUS_SRC_ALPHA		8
+#define DRM_BLEND_DST_ALPHA			9
+#define DRM_BLEND_ONE_MINUS_DST_ALPHA		10
+#define DRM_BLEND_CONSTANT_COLOR		11
+#define DRM_BLEND_ONE_MINUS_CONSTANT_COLOR	12
+#define DRM_BLEND_CONSTANT_ALPHA		13
+#define DRM_BLEND_ONE_MINUS_CONSTANT_ALPHA	14
+#define DRM_BLEND_SRC_ALPHA_SATURATE		15
+#define DRM_BLEND_SRC1_COLOR			16
+#define DRM_BLEND_ONE_MINUS_SRC1_COLOR		17
+#define DRM_BLEND_SRC1_ALPHA			18
+#define DRM_BLEND_ONE_MINUS_SRC1_ALPHA		19
+#define DRM_BLEND_PREMULTIPLIED_ALPHA		20
+
 /*
  * Note on terminology:  here, for brevity and convenience, we refer to connector
  * control chips as 'CRTCs'.  They can control any type of connector, VGA, LVDS,
@@ -1179,6 +1202,8 @@ extern int drm_format_plane_cpp(uint32_t format, int plane);
 extern int drm_format_horz_chroma_subsampling(uint32_t format);
 extern int drm_format_vert_chroma_subsampling(uint32_t format);
 extern const char *drm_get_format_name(uint32_t format);
+extern struct drm_property *drm_mode_create_blend_property(struct drm_device *dev,
+				unsigned int supported_factors);
 
 /* Helpers */
 static inline struct drm_crtc *drm_crtc_find(struct drm_device *dev,
-- 
1.8.5

Re: [PATCH 1/4] drm: Added plane alpha and color blending property

[Damien Lespiau]

On Sat, Mar 08, 2014 at 01:51:16PM +0530, sagar.a.kamble@intel.com wrote:
> From: Sagar Kamble <sagar.a.kamble@intel.com>
> 
> This patch creates a generic blending enum property.
> Drivers may support subset of these values.
> 
> Cc: airlied@linux.ie
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 33 +++++++++++++++++++++++++++++++++
>  include/drm/drm_crtc.h     | 25 +++++++++++++++++++++++++
>  2 files changed, 58 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 4e43fc2..15281a3 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -4147,3 +4147,36 @@ void drm_mode_config_cleanup(struct drm_device *dev)
>  	idr_destroy(&dev->mode_config.crtc_idr);
>  }
>  EXPORT_SYMBOL(drm_mode_config_cleanup);
> +
> +struct drm_property *drm_mode_create_blend_property(struct drm_device *dev,
> +				unsigned int supported_factors)
> +{
> +	static const struct drm_prop_enum_list props[] = {
> +		{ DRM_BLEND_NONE,   			"none" },
> +		{ DRM_BLEND_ZERO,  			"zero" },
> +		{ DRM_BLEND_ONE, 			"one" },
> +		{ DRM_BLEND_SRC_COLOR, 			"src-color" },
> +		{ DRM_BLEND_ONE_MINUS_SRC_COLOR,  	"one-minus-src-color" },
> +		{ DRM_BLEND_DST_COLOR, 			"dst-color" },
> +		{ DRM_BLEND_ONE_MINUS_DST_COLOR,  	"one-minus-dst-color" },
> +		{ DRM_BLEND_SRC_ALPHA, 			"src-alpha" },
> +		{ DRM_BLEND_ONE_MINUS_SRC_ALPHA, 	"one-minus-src-alpha" },
> +		{ DRM_BLEND_DST_ALPHA, 			"dst-alpha" },
> +		{ DRM_BLEND_ONE_MINUS_DST_ALPHA, 	"one-minus-dst-alpha" },
> +		{ DRM_BLEND_CONSTANT_COLOR, 		"constant-color" },
> +		{ DRM_BLEND_ONE_MINUS_CONSTANT_COLOR, 	"one-minus-constant-color" },
> +		{ DRM_BLEND_CONSTANT_ALPHA, 		"constant-alpha" },
> +		{ DRM_BLEND_ONE_MINUS_CONSTANT_ALPHA, 	"one-minus-constant-alpha" },
> +		{ DRM_BLEND_SRC_ALPHA_SATURATE, 	"alpha-saturate" },
> +		{ DRM_BLEND_SRC1_COLOR, 		"src1-color" },
> +		{ DRM_BLEND_ONE_MINUS_SRC1_COLOR, 	"one-minus-src1-color" },
> +		{ DRM_BLEND_SRC1_ALPHA, 		"src1-alpha" },
> +		{ DRM_BLEND_ONE_MINUS_SRC1_ALPHA, 	"one-minus-src1-alpha" },
> +		{ DRM_BLEND_PREMULTIPLIED_ALPHA, 	"pre-multiplied-alpha" }

Again, whether the fbs are premultipled or not is orthogonal to how we
want to blend the plane. I still think it'd be better to add
premultiplied fb DRM formats.

-- 
Damien

Re: [Intel-gfx] [PATCH 1/4] drm: Added plane alpha and color blending property

[Damien Lespiau]

On Sat, Mar 08, 2014 at 01:51:16PM +0530, sagar.a.kamble@intel.com wrote:
> From: Sagar Kamble <sagar.a.kamble@intel.com>
> 
> This patch creates a generic blending enum property.
> Drivers may support subset of these values.
> 
> Cc: airlied@linux.ie
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
> ---
>  drivers/gpu/drm/drm_crtc.c | 33 +++++++++++++++++++++++++++++++++
>  include/drm/drm_crtc.h     | 25 +++++++++++++++++++++++++
>  2 files changed, 58 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index 4e43fc2..15281a3 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -4147,3 +4147,36 @@ void drm_mode_config_cleanup(struct drm_device *dev)
>  	idr_destroy(&dev->mode_config.crtc_idr);
>  }
>  EXPORT_SYMBOL(drm_mode_config_cleanup);
> +
> +struct drm_property *drm_mode_create_blend_property(struct drm_device *dev,
> +				unsigned int supported_factors)
> +{
> +	static const struct drm_prop_enum_list props[] = {
> +		{ DRM_BLEND_NONE,   			"none" },
> +		{ DRM_BLEND_ZERO,  			"zero" },
> +		{ DRM_BLEND_ONE, 			"one" },
> +		{ DRM_BLEND_SRC_COLOR, 			"src-color" },
> +		{ DRM_BLEND_ONE_MINUS_SRC_COLOR,  	"one-minus-src-color" },
> +		{ DRM_BLEND_DST_COLOR, 			"dst-color" },
> +		{ DRM_BLEND_ONE_MINUS_DST_COLOR,  	"one-minus-dst-color" },
> +		{ DRM_BLEND_SRC_ALPHA, 			"src-alpha" },
> +		{ DRM_BLEND_ONE_MINUS_SRC_ALPHA, 	"one-minus-src-alpha" },
> +		{ DRM_BLEND_DST_ALPHA, 			"dst-alpha" },
> +		{ DRM_BLEND_ONE_MINUS_DST_ALPHA, 	"one-minus-dst-alpha" },
> +		{ DRM_BLEND_CONSTANT_COLOR, 		"constant-color" },
> +		{ DRM_BLEND_ONE_MINUS_CONSTANT_COLOR, 	"one-minus-constant-color" },
> +		{ DRM_BLEND_CONSTANT_ALPHA, 		"constant-alpha" },
> +		{ DRM_BLEND_ONE_MINUS_CONSTANT_ALPHA, 	"one-minus-constant-alpha" },
> +		{ DRM_BLEND_SRC_ALPHA_SATURATE, 	"alpha-saturate" },
> +		{ DRM_BLEND_SRC1_COLOR, 		"src1-color" },
> +		{ DRM_BLEND_ONE_MINUS_SRC1_COLOR, 	"one-minus-src1-color" },
> +		{ DRM_BLEND_SRC1_ALPHA, 		"src1-alpha" },
> +		{ DRM_BLEND_ONE_MINUS_SRC1_ALPHA, 	"one-minus-src1-alpha" },
> +		{ DRM_BLEND_PREMULTIPLIED_ALPHA, 	"pre-multiplied-alpha" }
> +	};

Another few notes:

- You seem to assume that there's a need for DRM_BLEND_NONE, what would it
do? The property should default to DRM_BLEND_SRC_COLOR ie "Take the
color from this plane for the blending".

- There's no need to expose SRC1 variants, this was done in GL to expose a
second color the fragment shader can output, not applicable here.

-- 
Damien

[PATCH 4/4] Documentation: drm: describing plane alpha and color blending property

[sagar.a.kamble]

From: Sagar Kamble <sagar.a.kamble@intel.com>

Cc: Rob Landley <rob@landley.net>
Cc: Dave Airlie <airlied@redhat.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
Cc: David Herrmann <dh.herrmann@gmail.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
Cc: Sagar Kamble <sagar.a.kamble@intel.com>
Cc: "Purushothaman, Vijay A" <vijay.a.purushothaman@intel.com>
Cc: linux-doc@vger.kernel.org
Cc: dri-devel@lists.freedesktop.org
Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
---
 Documentation/DocBook/drm.tmpl | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
index 5650d13..ba260e8 100644
--- a/Documentation/DocBook/drm.tmpl
+++ b/Documentation/DocBook/drm.tmpl
@@ -2334,7 +2334,7 @@ void intel_crt_init(struct drm_device *dev)
 </tr>
 <tr>
 <td rowspan="19" valign="top" >DRM</td>
-<td rowspan="2" valign="top" >Generic</td>
+<td rowspan="3" valign="top" >Generic</td>
 <td valign="top" >“EDID”</td>
 <td valign="top" >BLOB | IMMUTABLE</td>
 <td valign="top" >0</td>
@@ -2349,6 +2349,17 @@ void intel_crt_init(struct drm_device *dev)
 <td valign="top" >Contains DPMS operation mode value.</td>
 </tr>
 <tr>
+<td valign="top" >“blend”</td>
+<td valign="top" >BITMASK</td>
+<td valign="top" >{ {0, "none"}, {1, "zero"}, {2, "one"}, {3, "src-color"}, {4, "one-minus-src-color"}
+, {5, "dst-color"}, {6, "one-minus-dst-color"}, {7, "src-alpha"}, {8, "one-minus-src-alpha"}, {9, "dst-alpha"}
+, {10, "one-minus-dst-alpha"}, {11, "constant-color"}, {12, "one-minus-constant-color"}, {13, "constant-alpha"}
+, {14, "one-minus-constant-alpha"}, {15, "alpha-saturate"}, {16, "src1-color"}, {17, "one-minus-src1-color"}
+, {18, "src1-alpha"}, {19, "one-minus-src1-alpha"}, {20, "pre-multiplied-alpha"} }</td>
+<td valign="top" >Plane</td>
+<td valign="top" >Contains plane alpha/color blending operation value.</td>
+</tr>
+<tr>
 <td rowspan="2" valign="top" >DVI-I</td>
 <td valign="top" >“subconnector”</td>
 <td valign="top" >ENUM</td>
-- 
1.8.5

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

Re: [PATCH 4/4] Documentation: drm: describing plane alpha and color blending property

[Laurent Pinchart]

Hi Sagar,

Thank you for the patch.

On Saturday 08 March 2014 13:51:19 sagar.a.kamble@intel.com wrote:
> From: Sagar Kamble <sagar.a.kamble@intel.com>
> 
> Cc: Rob Landley <rob@landley.net>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Laurent Pinchart <laurent.pinchart+renesas@ideasonboard.com>
> Cc: David Herrmann <dh.herrmann@gmail.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: "Ville Syrjälä" <ville.syrjala@linux.intel.com>
> Cc: Sagar Kamble <sagar.a.kamble@intel.com>
> Cc: "Purushothaman, Vijay A" <vijay.a.purushothaman@intel.com>
> Cc: linux-doc@vger.kernel.org
> Cc: dri-devel@lists.freedesktop.org
> Signed-off-by: Sagar Kamble <sagar.a.kamble@intel.com>
> ---
>  Documentation/DocBook/drm.tmpl | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/DocBook/drm.tmpl b/Documentation/DocBook/drm.tmpl
> index 5650d13..ba260e8 100644
> --- a/Documentation/DocBook/drm.tmpl
> +++ b/Documentation/DocBook/drm.tmpl
> @@ -2334,7 +2334,7 @@ void intel_crt_init(struct drm_device *dev)
>  </tr>
>  <tr>
>  <td rowspan="19" valign="top" >DRM</td>
> -<td rowspan="2" valign="top" >Generic</td>
> +<td rowspan="3" valign="top" >Generic</td>
>  <td valign="top" >“EDID”</td>
>  <td valign="top" >BLOB | IMMUTABLE</td>
>  <td valign="top" >0</td>
> @@ -2349,6 +2349,17 @@ void intel_crt_init(struct drm_device *dev)
>  <td valign="top" >Contains DPMS operation mode value.</td>
>  </tr>
>  <tr>
> +<td valign="top" >“blend”</td>
> +<td valign="top" >BITMASK</td>
> +<td valign="top" >{ {0, "none"}, {1, "zero"}, {2, "one"}, {3, "src-color"},
> {4, "one-minus-src-color"} +, {5, "dst-color"}, {6, "one-minus-dst-color"},
> {7, "src-alpha"}, {8, "one-minus-src-alpha"}, {9, "dst-alpha"} +, {10,
> "one-minus-dst-alpha"}, {11, "constant-color"}, {12,
> "one-minus-constant-color"}, {13, "constant-alpha"} +, {14,
> "one-minus-constant-alpha"}, {15, "alpha-saturate"}, {16, "src1-color"},
> {17, "one-minus-src1-color"} +, {18, "src1-alpha"}, {19,
> "one-minus-src1-alpha"}, {20, "pre-multiplied-alpha"} }</td> +<td
> valign="top" >Plane</td>
> +<td valign="top" >Contains plane alpha/color blending operation value.</td>

I believe this calls for a description of each property value. From patches 
1/4 and 4/4 it's not entirely clear to me what all values mean.

> +</tr>
> +<tr>
>  <td rowspan="2" valign="top" >DVI-I</td>
>  <td valign="top" >“subconnector”</td>
>  <td valign="top" >ENUM</td>

-- 
Regards,

Laurent Pinchart

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx