Re: [RFC PATCH] drm: Add per-plane pixel blend mode property

[Maarten Lankhorst]

Op 08-05-18 om 12:34 schreef Lowry Li:
> Pixel blend modes represent the alpha blending equation
> selection, describing how the pixels from the current
> plane are composited with the background.
>
> Add a pixel_blend_mode to drm_plane_state and a
> blend_mode_property to drm_plane, and related support
> functions.
>
> Defines three blend modes in drm_blend.h.
>
> Signed-off-by: Lowry Li 
> ---
>  drivers/gpu/drm/drm_atomic.c|  4 ++
>  drivers/gpu/drm/drm_atomic_helper.c |  1 +
>  drivers/gpu/drm/drm_blend.c | 95 
> +
>  include/drm/drm_blend.h |  6 +++
>  include/drm/drm_plane.h |  7 +++
>  5 files changed, 113 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index a567310..0bb6de1 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -764,6 +764,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>   state->src_w = val;
>   } else if (property == config->prop_src_h) {
>   state->src_h = val;
> + } else if (property == plane->blend_mode_property) {
> + state->pixel_blend_mode = val;
>   } else if (property == plane->rotation_property) {
>   if (!is_power_of_2(val & DRM_ROTATE_MASK))
>   return -EINVAL;
> @@ -826,6 +828,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>   *val = state->src_w;
>   } else if (property == config->prop_src_h) {
>   *val = state->src_h;
> + } else if (property == plane->blend_mode_property) {
> + *val = state->pixel_blend_mode;
>   } else if (property == plane->rotation_property) {
>   *val = state->rotation;
>   } else if (property == plane->zpos_property) {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 01d936b..e4377fd 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3133,6 +3133,7 @@ void drm_atomic_helper_plane_reset(struct drm_plane 
> *plane)
>   if (plane->state) {
>   plane->state->plane = plane;
>   plane->state->rotation = DRM_ROTATE_0;
> + plane->state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
>   }
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_plane_reset);
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index 665aafc..bb938de 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -98,6 +98,12 @@
>   *   planes. Without this property the primary plane is always below the 
> cursor
>   *   plane, and ordering between all other planes is undefined.
>   *
> + * pixel blend mode:
> + *   Pixel blend mode is set up with drm_plane_create_blend_mode_property().
> + *   It adds a blend mode for alpha blending equation selection, describing
> + *   how the pixels from the current plane are composited with the
> + *   background.
> + *
>   * Note that all the property extensions described here apply either to the
>   * plane or the CRTC (e.g. for the background color, which currently is not
>   * exposed and assumed to be black).
> @@ -405,3 +411,92 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
>   return 0;
>  }
>  EXPORT_SYMBOL(drm_atomic_normalize_zpos);
> +
> +/**
> + * drm_plane_create_blend_mode_property - create a new blend mode property
> + * @plane: drm plane
> + * @supported_modes: bitmask of supported modes, must include
> + *BIT(DRM_MODE_BLEND_PREMULTI)
> + *
> + * This creates a new property describing the blend mode.
> + *
> + * The property exposed to userspace is an enumeration property (see
> + * drm_property_create_enum()) called "pixel blend mode" and has the
> + * following enumeration values:
> + *
> + * DRM_MODE_BLEND_PIXEL_NONE: Blend formula that ignores the pixel alpha.
> + *   "None"
> + *   out.rgb = plane.alpha * pixel.rgb + (1 - plane.alpha) * bg.rgb
> + *
> + * DRM_MODE_BLEND_PREMULTI: Blend formula that assumes the pixel color values
> + *   have been already pre-multiplied with the alpha
> + *   channel values.
> + *   "Pre-multiplied"
> + *   out.rgb = plane.alpha * pixel.rgb +
> + * (1 - (plane.alpha * pixel.alpha)) * bg.rgb
> + *
> + * DRM_MODE_BLEND_COVERAGE: Blend formula that assumes the pixel color values
> + *   have not been pre-multiplied and will do so when
> + *   blending them to the background color values.
> + *   "Coverage"
> + *   out.rgb = plane.alpha * pixel.alpha * pixel.rgb +
> + * (1 - (plane.alpha * pixel.alpha)) * bg.rgb
> + *
> + * This property has no effect on formats with no pixel alpha, as pixel.alpha
> + * is assumed to be 1.0. If the plane does not expose the "alpha" property, 
> then
> + * plane.alpha is assumed to be 1.0, otherwise, it is the value of 

Re: [RFC PATCH] drm: Add per-plane pixel blend mode property

[Maarten Lankhorst]

Op 08-05-18 om 12:34 schreef Lowry Li:
> Pixel blend modes represent the alpha blending equation
> selection, describing how the pixels from the current
> plane are composited with the background.
>
> Add a pixel_blend_mode to drm_plane_state and a
> blend_mode_property to drm_plane, and related support
> functions.
>
> Defines three blend modes in drm_blend.h.
>
> Signed-off-by: Lowry Li 
> ---
>  drivers/gpu/drm/drm_atomic.c|  4 ++
>  drivers/gpu/drm/drm_atomic_helper.c |  1 +
>  drivers/gpu/drm/drm_blend.c | 95 
> +
>  include/drm/drm_blend.h |  6 +++
>  include/drm/drm_plane.h |  7 +++
>  5 files changed, 113 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index a567310..0bb6de1 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -764,6 +764,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>   state->src_w = val;
>   } else if (property == config->prop_src_h) {
>   state->src_h = val;
> + } else if (property == plane->blend_mode_property) {
> + state->pixel_blend_mode = val;
>   } else if (property == plane->rotation_property) {
>   if (!is_power_of_2(val & DRM_ROTATE_MASK))
>   return -EINVAL;
> @@ -826,6 +828,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>   *val = state->src_w;
>   } else if (property == config->prop_src_h) {
>   *val = state->src_h;
> + } else if (property == plane->blend_mode_property) {
> + *val = state->pixel_blend_mode;
>   } else if (property == plane->rotation_property) {
>   *val = state->rotation;
>   } else if (property == plane->zpos_property) {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 01d936b..e4377fd 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3133,6 +3133,7 @@ void drm_atomic_helper_plane_reset(struct drm_plane 
> *plane)
>   if (plane->state) {
>   plane->state->plane = plane;
>   plane->state->rotation = DRM_ROTATE_0;
> + plane->state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
>   }
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_plane_reset);
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index 665aafc..bb938de 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -98,6 +98,12 @@
>   *   planes. Without this property the primary plane is always below the 
> cursor
>   *   plane, and ordering between all other planes is undefined.
>   *
> + * pixel blend mode:
> + *   Pixel blend mode is set up with drm_plane_create_blend_mode_property().
> + *   It adds a blend mode for alpha blending equation selection, describing
> + *   how the pixels from the current plane are composited with the
> + *   background.
> + *
>   * Note that all the property extensions described here apply either to the
>   * plane or the CRTC (e.g. for the background color, which currently is not
>   * exposed and assumed to be black).
> @@ -405,3 +411,92 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
>   return 0;
>  }
>  EXPORT_SYMBOL(drm_atomic_normalize_zpos);
> +
> +/**
> + * drm_plane_create_blend_mode_property - create a new blend mode property
> + * @plane: drm plane
> + * @supported_modes: bitmask of supported modes, must include
> + *BIT(DRM_MODE_BLEND_PREMULTI)
> + *
> + * This creates a new property describing the blend mode.
> + *
> + * The property exposed to userspace is an enumeration property (see
> + * drm_property_create_enum()) called "pixel blend mode" and has the
> + * following enumeration values:
> + *
> + * DRM_MODE_BLEND_PIXEL_NONE: Blend formula that ignores the pixel alpha.
> + *   "None"
> + *   out.rgb = plane.alpha * pixel.rgb + (1 - plane.alpha) * bg.rgb
> + *
> + * DRM_MODE_BLEND_PREMULTI: Blend formula that assumes the pixel color values
> + *   have been already pre-multiplied with the alpha
> + *   channel values.
> + *   "Pre-multiplied"
> + *   out.rgb = plane.alpha * pixel.rgb +
> + * (1 - (plane.alpha * pixel.alpha)) * bg.rgb
> + *
> + * DRM_MODE_BLEND_COVERAGE: Blend formula that assumes the pixel color values
> + *   have not been pre-multiplied and will do so when
> + *   blending them to the background color values.
> + *   "Coverage"
> + *   out.rgb = plane.alpha * pixel.alpha * pixel.rgb +
> + * (1 - (plane.alpha * pixel.alpha)) * bg.rgb
> + *
> + * This property has no effect on formats with no pixel alpha, as pixel.alpha
> + * is assumed to be 1.0. If the plane does not expose the "alpha" property, 
> then
> + * plane.alpha is assumed to be 1.0, otherwise, it is the value of the 
> "alpha"
> + 

Re: [RFC PATCH] drm: Add per-plane pixel blend mode property

[Ville Syrjälä]

On Tue, May 08, 2018 at 06:34:36PM +0800, Lowry Li wrote:
> Pixel blend modes represent the alpha blending equation
> selection, describing how the pixels from the current
> plane are composited with the background.
> 
> Add a pixel_blend_mode to drm_plane_state and a
> blend_mode_property to drm_plane, and related support
> functions.
> 
> Defines three blend modes in drm_blend.h.
> 
> Signed-off-by: Lowry Li 
> ---
>  drivers/gpu/drm/drm_atomic.c|  4 ++
>  drivers/gpu/drm/drm_atomic_helper.c |  1 +
>  drivers/gpu/drm/drm_blend.c | 95 
> +
>  include/drm/drm_blend.h |  6 +++
>  include/drm/drm_plane.h |  7 +++
>  5 files changed, 113 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index a567310..0bb6de1 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -764,6 +764,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>   state->src_w = val;
>   } else if (property == config->prop_src_h) {
>   state->src_h = val;
> + } else if (property == plane->blend_mode_property) {
> + state->pixel_blend_mode = val;
>   } else if (property == plane->rotation_property) {
>   if (!is_power_of_2(val & DRM_ROTATE_MASK))
>   return -EINVAL;
> @@ -826,6 +828,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>   *val = state->src_w;
>   } else if (property == config->prop_src_h) {
>   *val = state->src_h;
> + } else if (property == plane->blend_mode_property) {
> + *val = state->pixel_blend_mode;
>   } else if (property == plane->rotation_property) {
>   *val = state->rotation;
>   } else if (property == plane->zpos_property) {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 01d936b..e4377fd 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3133,6 +3133,7 @@ void drm_atomic_helper_plane_reset(struct drm_plane 
> *plane)
>   if (plane->state) {
>   plane->state->plane = plane;
>   plane->state->rotation = DRM_ROTATE_0;
> + plane->state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
>   }
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_plane_reset);
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index 665aafc..bb938de 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -98,6 +98,12 @@
>   *   planes. Without this property the primary plane is always below the 
> cursor
>   *   plane, and ordering between all other planes is undefined.
>   *
> + * pixel blend mode:
> + *   Pixel blend mode is set up with drm_plane_create_blend_mode_property().
> + *   It adds a blend mode for alpha blending equation selection, describing
> + *   how the pixels from the current plane are composited with the
> + *   background.
> + *
>   * Note that all the property extensions described here apply either to the
>   * plane or the CRTC (e.g. for the background color, which currently is not
>   * exposed and assumed to be black).
> @@ -405,3 +411,92 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
>   return 0;
>  }
>  EXPORT_SYMBOL(drm_atomic_normalize_zpos);
> +
> +/**
> + * drm_plane_create_blend_mode_property - create a new blend mode property
> + * @plane: drm plane
> + * @supported_modes: bitmask of supported modes, must include
> + *BIT(DRM_MODE_BLEND_PREMULTI)
> + *
> + * This creates a new property describing the blend mode.
> + *
> + * The property exposed to userspace is an enumeration property (see
> + * drm_property_create_enum()) called "pixel blend mode" and has the
> + * following enumeration values:
> + *
> + * DRM_MODE_BLEND_PIXEL_NONE: Blend formula that ignores the pixel alpha.
> + *   "None"
> + *   out.rgb = plane.alpha * pixel.rgb + (1 - plane.alpha) * bg.rgb
> + *
> + * DRM_MODE_BLEND_PREMULTI: Blend formula that assumes the pixel color values
> + *   have been already pre-multiplied with the alpha
> + *   channel values.
> + *   "Pre-multiplied"
> + *   out.rgb = plane.alpha * pixel.rgb +
> + * (1 - (plane.alpha * pixel.alpha)) * bg.rgb
> + *
> + * DRM_MODE_BLEND_COVERAGE: Blend formula that assumes the pixel color values
> + *   have not been pre-multiplied and will do so when
> + *   blending them to the background color values.
> + *   "Coverage"
> + *   out.rgb = plane.alpha * pixel.alpha * pixel.rgb +
> + * (1 - (plane.alpha * pixel.alpha)) * bg.rgb

I'm not a huge fan of these enum value names. "coverage" makes me think
of "alpha to coverage".

Years ago there was quite a bit of dicscussion on this and the concensus
at the at time was to try and model this after the 

Re: [RFC PATCH] drm: Add per-plane pixel blend mode property

[Ville Syrjälä]

On Tue, May 08, 2018 at 06:34:36PM +0800, Lowry Li wrote:
> Pixel blend modes represent the alpha blending equation
> selection, describing how the pixels from the current
> plane are composited with the background.
> 
> Add a pixel_blend_mode to drm_plane_state and a
> blend_mode_property to drm_plane, and related support
> functions.
> 
> Defines three blend modes in drm_blend.h.
> 
> Signed-off-by: Lowry Li 
> ---
>  drivers/gpu/drm/drm_atomic.c|  4 ++
>  drivers/gpu/drm/drm_atomic_helper.c |  1 +
>  drivers/gpu/drm/drm_blend.c | 95 
> +
>  include/drm/drm_blend.h |  6 +++
>  include/drm/drm_plane.h |  7 +++
>  5 files changed, 113 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index a567310..0bb6de1 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -764,6 +764,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>   state->src_w = val;
>   } else if (property == config->prop_src_h) {
>   state->src_h = val;
> + } else if (property == plane->blend_mode_property) {
> + state->pixel_blend_mode = val;
>   } else if (property == plane->rotation_property) {
>   if (!is_power_of_2(val & DRM_ROTATE_MASK))
>   return -EINVAL;
> @@ -826,6 +828,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>   *val = state->src_w;
>   } else if (property == config->prop_src_h) {
>   *val = state->src_h;
> + } else if (property == plane->blend_mode_property) {
> + *val = state->pixel_blend_mode;
>   } else if (property == plane->rotation_property) {
>   *val = state->rotation;
>   } else if (property == plane->zpos_property) {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 01d936b..e4377fd 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3133,6 +3133,7 @@ void drm_atomic_helper_plane_reset(struct drm_plane 
> *plane)
>   if (plane->state) {
>   plane->state->plane = plane;
>   plane->state->rotation = DRM_ROTATE_0;
> + plane->state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
>   }
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_plane_reset);
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index 665aafc..bb938de 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -98,6 +98,12 @@
>   *   planes. Without this property the primary plane is always below the 
> cursor
>   *   plane, and ordering between all other planes is undefined.
>   *
> + * pixel blend mode:
> + *   Pixel blend mode is set up with drm_plane_create_blend_mode_property().
> + *   It adds a blend mode for alpha blending equation selection, describing
> + *   how the pixels from the current plane are composited with the
> + *   background.
> + *
>   * Note that all the property extensions described here apply either to the
>   * plane or the CRTC (e.g. for the background color, which currently is not
>   * exposed and assumed to be black).
> @@ -405,3 +411,92 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
>   return 0;
>  }
>  EXPORT_SYMBOL(drm_atomic_normalize_zpos);
> +
> +/**
> + * drm_plane_create_blend_mode_property - create a new blend mode property
> + * @plane: drm plane
> + * @supported_modes: bitmask of supported modes, must include
> + *BIT(DRM_MODE_BLEND_PREMULTI)
> + *
> + * This creates a new property describing the blend mode.
> + *
> + * The property exposed to userspace is an enumeration property (see
> + * drm_property_create_enum()) called "pixel blend mode" and has the
> + * following enumeration values:
> + *
> + * DRM_MODE_BLEND_PIXEL_NONE: Blend formula that ignores the pixel alpha.
> + *   "None"
> + *   out.rgb = plane.alpha * pixel.rgb + (1 - plane.alpha) * bg.rgb
> + *
> + * DRM_MODE_BLEND_PREMULTI: Blend formula that assumes the pixel color values
> + *   have been already pre-multiplied with the alpha
> + *   channel values.
> + *   "Pre-multiplied"
> + *   out.rgb = plane.alpha * pixel.rgb +
> + * (1 - (plane.alpha * pixel.alpha)) * bg.rgb
> + *
> + * DRM_MODE_BLEND_COVERAGE: Blend formula that assumes the pixel color values
> + *   have not been pre-multiplied and will do so when
> + *   blending them to the background color values.
> + *   "Coverage"
> + *   out.rgb = plane.alpha * pixel.alpha * pixel.rgb +
> + * (1 - (plane.alpha * pixel.alpha)) * bg.rgb

I'm not a huge fan of these enum value names. "coverage" makes me think
of "alpha to coverage".

Years ago there was quite a bit of dicscussion on this and the concensus
at the at time was to try and model this after the GL blend func. If 

Re: [RFC PATCH] drm: Add per-plane pixel blend mode property

[Daniel Vetter]

On Tue, May 08, 2018 at 06:34:36PM +0800, Lowry Li wrote:
> Pixel blend modes represent the alpha blending equation
> selection, describing how the pixels from the current
> plane are composited with the background.
> 
> Add a pixel_blend_mode to drm_plane_state and a
> blend_mode_property to drm_plane, and related support
> functions.
> 
> Defines three blend modes in drm_blend.h.
> 
> Signed-off-by: Lowry Li 

Some suggestions for the kerneldoc below. lgtm otherwise (but needs the
driver implementation + userspace support before we can merge).
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic.c|  4 ++
>  drivers/gpu/drm/drm_atomic_helper.c |  1 +
>  drivers/gpu/drm/drm_blend.c | 95 
> +
>  include/drm/drm_blend.h |  6 +++
>  include/drm/drm_plane.h |  7 +++
>  5 files changed, 113 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index a567310..0bb6de1 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -764,6 +764,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>   state->src_w = val;
>   } else if (property == config->prop_src_h) {
>   state->src_h = val;
> + } else if (property == plane->blend_mode_property) {
> + state->pixel_blend_mode = val;
>   } else if (property == plane->rotation_property) {
>   if (!is_power_of_2(val & DRM_ROTATE_MASK))
>   return -EINVAL;
> @@ -826,6 +828,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>   *val = state->src_w;
>   } else if (property == config->prop_src_h) {
>   *val = state->src_h;
> + } else if (property == plane->blend_mode_property) {
> + *val = state->pixel_blend_mode;
>   } else if (property == plane->rotation_property) {
>   *val = state->rotation;
>   } else if (property == plane->zpos_property) {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 01d936b..e4377fd 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3133,6 +3133,7 @@ void drm_atomic_helper_plane_reset(struct drm_plane 
> *plane)
>   if (plane->state) {
>   plane->state->plane = plane;
>   plane->state->rotation = DRM_ROTATE_0;
> + plane->state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
>   }
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_plane_reset);
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index 665aafc..bb938de 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -98,6 +98,12 @@
>   *   planes. Without this property the primary plane is always below the 
> cursor
>   *   plane, and ordering between all other planes is undefined.
>   *
> + * pixel blend mode:
> + *   Pixel blend mode is set up with drm_plane_create_blend_mode_property().
> + *   It adds a blend mode for alpha blending equation selection, describing
> + *   how the pixels from the current plane are composited with the
> + *   background.

I think a link here to drm_plane_create_blend_mode_property() for the
blending equations would be really good.

> + *
>   * Note that all the property extensions described here apply either to the
>   * plane or the CRTC (e.g. for the background color, which currently is not
>   * exposed and assumed to be black).
> @@ -405,3 +411,92 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
>   return 0;
>  }
>  EXPORT_SYMBOL(drm_atomic_normalize_zpos);
> +
> +/**
> + * drm_plane_create_blend_mode_property - create a new blend mode property
> + * @plane: drm plane
> + * @supported_modes: bitmask of supported modes, must include
> + *BIT(DRM_MODE_BLEND_PREMULTI)
> + *
> + * This creates a new property describing the blend mode.
> + *
> + * The property exposed to userspace is an enumeration property (see
> + * drm_property_create_enum()) called "pixel blend mode" and has the
> + * following enumeration values:
> + *
> + * DRM_MODE_BLEND_PIXEL_NONE: Blend formula that ignores the pixel alpha.

Please don't enumerate the #defines, but instead the string values (i.e.
"None" here). Enum properties are supposed to be identified with their
string values (but in practice we probably can't ever change the
corresponding numeric values).

> + *   "None"
> + *   out.rgb = plane.alpha * pixel.rgb + (1 - plane.alpha) * bg.rgb

Would be good to define what the different variables here mean.

Also I think putting the blending equation into the DOC comment would be
good, so that when we do further blending properties we can have one
overall big picture for how this works. Instead of everything spread
around. Also maybe use fg.* instead of pixel.* and plane_alpha instead of
plane.alpha (to make it clear it's just one value).

> + *
> + * 

Re: [RFC PATCH] drm: Add per-plane pixel blend mode property

[Daniel Vetter]

On Tue, May 08, 2018 at 06:34:36PM +0800, Lowry Li wrote:
> Pixel blend modes represent the alpha blending equation
> selection, describing how the pixels from the current
> plane are composited with the background.
> 
> Add a pixel_blend_mode to drm_plane_state and a
> blend_mode_property to drm_plane, and related support
> functions.
> 
> Defines three blend modes in drm_blend.h.
> 
> Signed-off-by: Lowry Li 

Some suggestions for the kerneldoc below. lgtm otherwise (but needs the
driver implementation + userspace support before we can merge).
-Daniel

> ---
>  drivers/gpu/drm/drm_atomic.c|  4 ++
>  drivers/gpu/drm/drm_atomic_helper.c |  1 +
>  drivers/gpu/drm/drm_blend.c | 95 
> +
>  include/drm/drm_blend.h |  6 +++
>  include/drm/drm_plane.h |  7 +++
>  5 files changed, 113 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index a567310..0bb6de1 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -764,6 +764,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>   state->src_w = val;
>   } else if (property == config->prop_src_h) {
>   state->src_h = val;
> + } else if (property == plane->blend_mode_property) {
> + state->pixel_blend_mode = val;
>   } else if (property == plane->rotation_property) {
>   if (!is_power_of_2(val & DRM_ROTATE_MASK))
>   return -EINVAL;
> @@ -826,6 +828,8 @@ int drm_atomic_plane_set_property(struct drm_plane *plane,
>   *val = state->src_w;
>   } else if (property == config->prop_src_h) {
>   *val = state->src_h;
> + } else if (property == plane->blend_mode_property) {
> + *val = state->pixel_blend_mode;
>   } else if (property == plane->rotation_property) {
>   *val = state->rotation;
>   } else if (property == plane->zpos_property) {
> diff --git a/drivers/gpu/drm/drm_atomic_helper.c 
> b/drivers/gpu/drm/drm_atomic_helper.c
> index 01d936b..e4377fd 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3133,6 +3133,7 @@ void drm_atomic_helper_plane_reset(struct drm_plane 
> *plane)
>   if (plane->state) {
>   plane->state->plane = plane;
>   plane->state->rotation = DRM_ROTATE_0;
> + plane->state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
>   }
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_plane_reset);
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index 665aafc..bb938de 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -98,6 +98,12 @@
>   *   planes. Without this property the primary plane is always below the 
> cursor
>   *   plane, and ordering between all other planes is undefined.
>   *
> + * pixel blend mode:
> + *   Pixel blend mode is set up with drm_plane_create_blend_mode_property().
> + *   It adds a blend mode for alpha blending equation selection, describing
> + *   how the pixels from the current plane are composited with the
> + *   background.

I think a link here to drm_plane_create_blend_mode_property() for the
blending equations would be really good.

> + *
>   * Note that all the property extensions described here apply either to the
>   * plane or the CRTC (e.g. for the background color, which currently is not
>   * exposed and assumed to be black).
> @@ -405,3 +411,92 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
>   return 0;
>  }
>  EXPORT_SYMBOL(drm_atomic_normalize_zpos);
> +
> +/**
> + * drm_plane_create_blend_mode_property - create a new blend mode property
> + * @plane: drm plane
> + * @supported_modes: bitmask of supported modes, must include
> + *BIT(DRM_MODE_BLEND_PREMULTI)
> + *
> + * This creates a new property describing the blend mode.
> + *
> + * The property exposed to userspace is an enumeration property (see
> + * drm_property_create_enum()) called "pixel blend mode" and has the
> + * following enumeration values:
> + *
> + * DRM_MODE_BLEND_PIXEL_NONE: Blend formula that ignores the pixel alpha.

Please don't enumerate the #defines, but instead the string values (i.e.
"None" here). Enum properties are supposed to be identified with their
string values (but in practice we probably can't ever change the
corresponding numeric values).

> + *   "None"
> + *   out.rgb = plane.alpha * pixel.rgb + (1 - plane.alpha) * bg.rgb

Would be good to define what the different variables here mean.

Also I think putting the blending equation into the DOC comment would be
good, so that when we do further blending properties we can have one
overall big picture for how this works. Instead of everything spread
around. Also maybe use fg.* instead of pixel.* and plane_alpha instead of
plane.alpha (to make it clear it's just one value).

> + *
> + * DRM_MODE_BLEND_PREMULTI: