Re: [Intel-gfx] [PATCHv2 1/5] drm: Introduce the blend-func property

[Ville Syrjälä]

On Fri, Apr 29, 2016 at 02:59:13PM +0530, Vandita Kulkarni wrote:
> From: Damien Lespiau 
> 
> We'd like to be able to program the blending modes of display planes.
> Ville suggested to use something similar to the GL blend states, which
> does seem like a good idea.
> 
> For now, we only consider blend factors, but room is left for
> extensions: blend equation, separate rgb/alpha blend factors, blend
> color.
> 
> V2: Added the belnd func property support in get property.
> 
> Suggested-by: Ville Syrjälä 
> Signed-off-by: Damien Lespiau 
> Signed-off-by: Vandita Kulkarni 
> ---
>  Documentation/DocBook/gpu.tmpl | 11 +--
>  drivers/gpu/drm/drm_atomic.c   | 14 ++
>  drivers/gpu/drm/drm_crtc.c |  5 +
>  include/drm/drm_crtc.h | 20 
>  4 files changed, 48 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
> index 1464fb2..f673989 100644
> --- a/Documentation/DocBook/gpu.tmpl
> +++ b/Documentation/DocBook/gpu.tmpl
> @@ -1816,7 +1816,7 @@ void intel_crt_init(struct drm_device *dev)
>   Description/Restrictions
>   
>   
> - DRM
> + DRM
>   Generic
>   “rotation”
>   BITMASK
> @@ -1868,7 +1868,7 @@ void intel_crt_init(struct drm_device *dev)
>   CRTC that connector is attached to (atomic)
>   
>   
> - Plane
> + Plane
>   “type”
>   ENUM | IMMUTABLE
>   { "Overlay", "Primary", "Cursor" }
> @@ -1946,6 +1946,13 @@ void intel_crt_init(struct drm_device *dev)
>   CRTC that plane is attached to (atomic)
>   
>   
> + “blend_func”
> + None
> + DRM_BLEND_FUNC()
> + Plane
> + Source and destination blending factors
> + 
> + 
>   DVI-I
>   “subconnector”
>   ENUM
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 8ee1db8..c2ead2d 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -701,6 +701,18 @@ int drm_atomic_plane_set_property(struct drm_plane 
> *plane,
>   state->src_h = val;
>   } else if (property == config->rotation_property) {
>   state->rotation = val;
> + } else if (property == config->prop_blend_func) {
> + enum drm_blend_factor src_factor, dst_factor;
> +
> + src_factor = DRM_BLEND_FUNC_SRC_FACTOR(val);
> + dst_factor = DRM_BLEND_FUNC_DST_FACTOR(val);
> +
> + if (src_factor != dst_factor &&
> + (src_factor == DRM_BLEND_FACTOR_AUTO ||
> + dst_factor == DRM_BLEND_FACTOR_AUTO))
> + return -EINVAL;
> +
> + state->blend_mode.func = val & GENMASK(31, 0);
>   } else if (plane->funcs->atomic_set_property) {
>   return plane->funcs->atomic_set_property(plane, state,
>   property, val);
> @@ -757,6 +769,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>   *val = state->src_h;
>   } else if (property == config->rotation_property) {
>   *val = state->rotation;
> + } else if (property == config->prop_blend_func) {
> + *val = state->blend_mode.func;
>   } else if (plane->funcs->atomic_get_property) {
>   return plane->funcs->atomic_get_property(plane, state, 
> property, val);
>   } else {
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index f7fe9e1..2cac5e1 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1587,6 +1587,11 @@ static int drm_mode_create_standard_properties(struct 
> drm_device *dev)
>   return -ENOMEM;
>   dev->mode_config.gamma_lut_size_property = prop;
>  
> + prop = drm_property_create_range(dev, 0, "blend_func", 0, U32_MAX);
> + if (!prop)
> + return -ENOMEM;
> + dev->mode_config.prop_blend_func = prop;
> +
>   return 0;
>  }
>  
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 6d46842..269f660 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -89,6 +89,23 @@ static inline uint64_t I642U64(int64_t val)
>  #define DRM_REFLECT_X4
>  #define DRM_REFLECT_Y5
>  
> +enum drm_blend_factor {
> + DRM_BLEND_FACTOR_AUTO,
> + DRM_BLEND_FACTOR_ZERO,
> + DRM_BLEND_FACTOR_ONE,
> + DRM_BLEND_FACTOR_SRC_ALPHA,
> + DRM_BLEND_FACTOR_ONE_MINUS_SRC_ALPHA,

Based on what I remember of the earlier discussion I don't think these
are quite enough. We could want some way to deal with the constant alpha
as well. Hmm. I think I need to refresh my brain a bit on this before I
can say anything really.

Also IIRC I was thinking that the premult vs. non-premult would be a
separate thing outside the blend equation. That's how we have it some
hw at least AFAIK, so be shoving it all 

Re: [Intel-gfx] [PATCHv2 1/5] drm: Introduce the blend-func property

[Jani Nikula]

On Tue, 26 Apr 2016, Daniel Vetter  wrote:
> New props need to be cc'ed to dri-devel. And we need to coordinate with
> other folks doing the same and have one unified set of blending ops.

The more general answer is, check the maintainers for the files you're
changing, in the MAINTAINERS file. scripts/get_maintainer.pl will help
you with that.

$ scripts/get_maintainer.pl 0001-drm-Introduce-the-blend-func-property.patch
Jonathan Corbet  (maintainer:DOCUMENTATION)
David Airlie  (maintainer:DRM DRIVERS)
Daniel Vetter  
(commit_signer:45/42=100%,authored:19/42=45%)
Lukas Wunner  (commit_signer:8/42=19%,authored:7/42=17%)
Thierry Reding  (commit_signer:7/42=17%)
Darren Hart  (commit_signer:3/42=7%)
Jani Nikula  (commit_signer:3/42=7%,authored:3/42=7%)
linux-...@vger.kernel.org (open list:DOCUMENTATION)
linux-ker...@vger.kernel.org (open list)
dri-de...@lists.freedesktop.org (open list:DRM DRIVERS)

You can often ignore the "commit_signer" stats (and sometimes people get
annyoed for being randomly Cc'd for things they did long ago), and here
you need to take into account the following patches and include the
drm/i915 list and maintainers. get_maintainer.pl is not the silver
bullet, but it will give you an idea, and in this case a clear
indication intel-gfx is not enough.

BR,
Jani.

-- 
Jani Nikula, Intel Open Source Technology Center
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCHv2 1/5] drm: Introduce the blend-func property

[Daniel Vetter]

On Tue, Apr 26, 2016 at 03:03:00PM +0530, Vandita Kulkarni wrote:
> From: Damien Lespiau 
> 
> We'd like to be able to program the blending modes of display planes.
> Ville suggested to use something similar to the GL blend states, which
> does seem like a good idea.
> 
> For now, we only consider blend factors, but room is left for
> extensions: blend equation, separate rgb/alpha blend factors, blend
> color.
> 
> V2: Added the belnd func property support in get property.
> 
> Suggested-by: Ville Syrjälä 
> Signed-off-by: Damien Lespiau 
> Signed-off-by: Vandita Kulkarni 

Also, new ABI = where is the open source userspace for this?
-Daniel

> ---
>  Documentation/DocBook/gpu.tmpl | 11 +--
>  drivers/gpu/drm/drm_atomic.c   | 14 ++
>  drivers/gpu/drm/drm_crtc.c |  5 +
>  include/drm/drm_crtc.h | 20 
>  4 files changed, 48 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
> index 1464fb2..f673989 100644
> --- a/Documentation/DocBook/gpu.tmpl
> +++ b/Documentation/DocBook/gpu.tmpl
> @@ -1816,7 +1816,7 @@ void intel_crt_init(struct drm_device *dev)
>   Description/Restrictions
>   
>   
> - DRM
> + DRM
>   Generic
>   “rotation”
>   BITMASK
> @@ -1868,7 +1868,7 @@ void intel_crt_init(struct drm_device *dev)
>   CRTC that connector is attached to (atomic)
>   
>   
> - Plane
> + Plane
>   “type”
>   ENUM | IMMUTABLE
>   { "Overlay", "Primary", "Cursor" }
> @@ -1946,6 +1946,13 @@ void intel_crt_init(struct drm_device *dev)
>   CRTC that plane is attached to (atomic)
>   
>   
> + “blend_func”
> + None
> + DRM_BLEND_FUNC()
> + Plane
> + Source and destination blending factors
> + 
> + 
>   DVI-I
>   “subconnector”
>   ENUM
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 8ee1db8..c2ead2d 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -701,6 +701,18 @@ int drm_atomic_plane_set_property(struct drm_plane 
> *plane,
>   state->src_h = val;
>   } else if (property == config->rotation_property) {
>   state->rotation = val;
> + } else if (property == config->prop_blend_func) {
> + enum drm_blend_factor src_factor, dst_factor;
> +
> + src_factor = DRM_BLEND_FUNC_SRC_FACTOR(val);
> + dst_factor = DRM_BLEND_FUNC_DST_FACTOR(val);
> +
> + if (src_factor != dst_factor &&
> + (src_factor == DRM_BLEND_FACTOR_AUTO ||
> + dst_factor == DRM_BLEND_FACTOR_AUTO))
> + return -EINVAL;
> +
> + state->blend_mode.func = val & GENMASK(31, 0);
>   } else if (plane->funcs->atomic_set_property) {
>   return plane->funcs->atomic_set_property(plane, state,
>   property, val);
> @@ -757,6 +769,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>   *val = state->src_h;
>   } else if (property == config->rotation_property) {
>   *val = state->rotation;
> + } else if (property == config->prop_blend_func) {
> + *val = state->blend_mode.func;
>   } else if (plane->funcs->atomic_get_property) {
>   return plane->funcs->atomic_get_property(plane, state, 
> property, val);
>   } else {
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index f7fe9e1..2cac5e1 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1587,6 +1587,11 @@ static int drm_mode_create_standard_properties(struct 
> drm_device *dev)
>   return -ENOMEM;
>   dev->mode_config.gamma_lut_size_property = prop;
>  
> + prop = drm_property_create_range(dev, 0, "blend_func", 0, U32_MAX);
> + if (!prop)
> + return -ENOMEM;
> + dev->mode_config.prop_blend_func = prop;
> +
>   return 0;
>  }
>  
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 6d46842..269f660 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -89,6 +89,23 @@ static inline uint64_t I642U64(int64_t val)
>  #define DRM_REFLECT_X4
>  #define DRM_REFLECT_Y5
>  
> +enum drm_blend_factor {
> + DRM_BLEND_FACTOR_AUTO,
> + DRM_BLEND_FACTOR_ZERO,
> + DRM_BLEND_FACTOR_ONE,
> + DRM_BLEND_FACTOR_SRC_ALPHA,
> + DRM_BLEND_FACTOR_ONE_MINUS_SRC_ALPHA,
> +};
> +
> +#define DRM_BLEND_FUNC(src_factor, dst_factor)   \
> + (DRM_BLEND_FACTOR_##src_factor << 16 | DRM_BLEND_FACTOR_##dst_factor)
> +#define DRM_BLEND_FUNC_SRC_FACTOR(val)   (((val) >> 16) & 0x)
> +#define DRM_BLEND_FUNC_DST_FACTOR(val)   ((val) & 0x)
> +
> +struct drm_blend_mode {
> + uint64_t func;
> +};

Re: [Intel-gfx] [PATCHv2 1/5] drm: Introduce the blend-func property

[Daniel Vetter]

On Tue, Apr 26, 2016 at 03:03:00PM +0530, Vandita Kulkarni wrote:
> From: Damien Lespiau 
> 
> We'd like to be able to program the blending modes of display planes.
> Ville suggested to use something similar to the GL blend states, which
> does seem like a good idea.
> 
> For now, we only consider blend factors, but room is left for
> extensions: blend equation, separate rgb/alpha blend factors, blend
> color.
> 
> V2: Added the belnd func property support in get property.
> 
> Suggested-by: Ville Syrjälä 
> Signed-off-by: Damien Lespiau 
> Signed-off-by: Vandita Kulkarni 

New props need to be cc'ed to dri-devel. And we need to coordinate with
other folks doing the same and have one unified set of blending ops.
-Daniel

> ---
>  Documentation/DocBook/gpu.tmpl | 11 +--
>  drivers/gpu/drm/drm_atomic.c   | 14 ++
>  drivers/gpu/drm/drm_crtc.c |  5 +
>  include/drm/drm_crtc.h | 20 
>  4 files changed, 48 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/DocBook/gpu.tmpl b/Documentation/DocBook/gpu.tmpl
> index 1464fb2..f673989 100644
> --- a/Documentation/DocBook/gpu.tmpl
> +++ b/Documentation/DocBook/gpu.tmpl
> @@ -1816,7 +1816,7 @@ void intel_crt_init(struct drm_device *dev)
>   Description/Restrictions
>   
>   
> - DRM
> + DRM
>   Generic
>   “rotation”
>   BITMASK
> @@ -1868,7 +1868,7 @@ void intel_crt_init(struct drm_device *dev)
>   CRTC that connector is attached to (atomic)
>   
>   
> - Plane
> + Plane
>   “type”
>   ENUM | IMMUTABLE
>   { "Overlay", "Primary", "Cursor" }
> @@ -1946,6 +1946,13 @@ void intel_crt_init(struct drm_device *dev)
>   CRTC that plane is attached to (atomic)
>   
>   
> + “blend_func”
> + None
> + DRM_BLEND_FUNC()
> + Plane
> + Source and destination blending factors
> + 
> + 
>   DVI-I
>   “subconnector”
>   ENUM
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 8ee1db8..c2ead2d 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -701,6 +701,18 @@ int drm_atomic_plane_set_property(struct drm_plane 
> *plane,
>   state->src_h = val;
>   } else if (property == config->rotation_property) {
>   state->rotation = val;
> + } else if (property == config->prop_blend_func) {
> + enum drm_blend_factor src_factor, dst_factor;
> +
> + src_factor = DRM_BLEND_FUNC_SRC_FACTOR(val);
> + dst_factor = DRM_BLEND_FUNC_DST_FACTOR(val);
> +
> + if (src_factor != dst_factor &&
> + (src_factor == DRM_BLEND_FACTOR_AUTO ||
> + dst_factor == DRM_BLEND_FACTOR_AUTO))
> + return -EINVAL;
> +
> + state->blend_mode.func = val & GENMASK(31, 0);
>   } else if (plane->funcs->atomic_set_property) {
>   return plane->funcs->atomic_set_property(plane, state,
>   property, val);
> @@ -757,6 +769,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
>   *val = state->src_h;
>   } else if (property == config->rotation_property) {
>   *val = state->rotation;
> + } else if (property == config->prop_blend_func) {
> + *val = state->blend_mode.func;
>   } else if (plane->funcs->atomic_get_property) {
>   return plane->funcs->atomic_get_property(plane, state, 
> property, val);
>   } else {
> diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
> index f7fe9e1..2cac5e1 100644
> --- a/drivers/gpu/drm/drm_crtc.c
> +++ b/drivers/gpu/drm/drm_crtc.c
> @@ -1587,6 +1587,11 @@ static int drm_mode_create_standard_properties(struct 
> drm_device *dev)
>   return -ENOMEM;
>   dev->mode_config.gamma_lut_size_property = prop;
>  
> + prop = drm_property_create_range(dev, 0, "blend_func", 0, U32_MAX);
> + if (!prop)
> + return -ENOMEM;
> + dev->mode_config.prop_blend_func = prop;
> +
>   return 0;
>  }
>  
> diff --git a/include/drm/drm_crtc.h b/include/drm/drm_crtc.h
> index 6d46842..269f660 100644
> --- a/include/drm/drm_crtc.h
> +++ b/include/drm/drm_crtc.h
> @@ -89,6 +89,23 @@ static inline uint64_t I642U64(int64_t val)
>  #define DRM_REFLECT_X4
>  #define DRM_REFLECT_Y5
>  
> +enum drm_blend_factor {
> + DRM_BLEND_FACTOR_AUTO,
> + DRM_BLEND_FACTOR_ZERO,
> + DRM_BLEND_FACTOR_ONE,
> + DRM_BLEND_FACTOR_SRC_ALPHA,
> + DRM_BLEND_FACTOR_ONE_MINUS_SRC_ALPHA,
> +};
> +
> +#define DRM_BLEND_FUNC(src_factor, dst_factor)   \
> + (DRM_BLEND_FACTOR_##src_factor << 16 | DRM_BLEND_FACTOR_##dst_factor)
> +#define DRM_BLEND_FUNC_SRC_FACTOR(val)   (((val) >> 16) & 0x)
> +#define DRM_BLEND_FUNC_DST_FACTOR(val)