Re: [PATCH v2 1/2] drm/blend: Add per-plane pixel blend mode property

From: Brian Starkey
Date: Wed May 30 2018 - 09:28:10 EST

Hi Lowry,

On Wed, May 30, 2018 at 07:23:53PM +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 <lowry.li@xxxxxxx>

With or without the kerneldoc tweaks I've suggested below, this patch
is:

Reviewed-by: Brian Starkey <brian.starkey@xxxxxxx>

---
drivers/gpu/drm/drm_atomic.c | 4 ++
drivers/gpu/drm/drm_atomic_helper.c | 1 +
drivers/gpu/drm/drm_blend.c | 110 ++++++++++++++++++++++++++++++++++++
include/drm/drm_blend.h | 6 ++
include/drm/drm_plane.h | 6 ++
5 files changed, 127 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 7d25c42..467f8de 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -783,6 +783,8 @@ static 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_MODE_ROTATE_MASK))
return -EINVAL;
@@ -848,6 +850,8 @@ static 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 c356545..ddbd0d2 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3484,6 +3484,7 @@ void drm_atomic_helper_plane_reset(struct drm_plane *plane)
if (plane->state) {
plane->state->plane = plane;
plane->state->rotation = DRM_MODE_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 5a81e1b..4ac45da 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -100,6 +100,41 @@
* 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.
+ *
+ * Three alpha blending equations(note that the fg.rgb or bg.rgb notation
+ * means each of the R, G or B channels for the foreground and background
+ * colors, respectively):
+ *
+ * "None": Blend formula that ignores the pixel alpha.
+ * out.rgb = plane_alpha * fg.rgb + (1 - plane_alpha) * bg.rgb
+ *
+ * "Pre-multiplied": Blend formula that assumes the pixel color values
+ * have been already pre-multiplied with the alpha
+ * channel values.
+ * out.rgb = plane_alpha * fg.rgb + (1 - (plane_alpha * fg.alpha)) * bg.rgb
+ *
+ * "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.
+ * out.rgb = plane_alpha * fg.alpha * fg.rgb +
+ * (1 - (plane_alpha * fg.alpha)) * bg.rgb
+ *
+ * fg.rgb: Each of the RGB component values from the plane's pixel
+ * fg.alpha: Alpha component value from the plane's pixel
+ * bg.rgb: Each of the RGB component values from the background
+ * plane_alpha: Plane alpha value set by the plane alpha property (if
+ * applicable).
+ *
+ * This property has no effect on formats with no pixel alpha, as fg.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" property.
+ *

I did a little fiddling to make this render nicely in the
documentation output. If you like it, you can apply the hunk below:

-- >8 --

diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
index 714b0602127d..7b5c3307d4c1 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -113,34 +113,45 @@
* how the pixels from the current plane are composited with the
* background.
*
- * Three alpha blending equations(note that the fg.rgb or bg.rgb notation
- * means each of the R, G or B channels for the foreground and background
- * colors, respectively):
- *
- * "None": Blend formula that ignores the pixel alpha.
- * out.rgb = plane_alpha * fg.rgb + (1 - plane_alpha) * bg.rgb
- *
- * "Pre-multiplied": Blend formula that assumes the pixel color values
- * have been already pre-multiplied with the alpha
- * channel values.
- * out.rgb = plane_alpha * fg.rgb + (1 - (plane_alpha * fg.alpha)) * bg.rgb
- *
- * "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.
- * out.rgb = plane_alpha * fg.alpha * fg.rgb +
- * (1 - (plane_alpha * fg.alpha)) * bg.rgb
- *
- * fg.rgb: Each of the RGB component values from the plane's pixel
- * fg.alpha: Alpha component value from the plane's pixel
- * bg.rgb: Each of the RGB component values from the background
- * plane_alpha: Plane alpha value set by the plane alpha property (if
- * applicable).
- *
- * This property has no effect on formats with no pixel alpha, as fg.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" property.
+ * Three alpha blending equations are defined:
+ *
+ * "None":
+ * Blend formula that ignores the pixel alpha::
+ *
+ * out.rgb = plane_alpha * fg.rgb +
+ * (1 - plane_alpha) * bg.rgb
+ *
+ * "Pre-multiplied":
+ * Blend formula that assumes the pixel color values
+ * have been already pre-multiplied with the alpha
+ * channel values::
+ *
+ * out.rgb = plane_alpha * fg.rgb +
+ * (1 - (plane_alpha * fg.alpha)) * bg.rgb
+ *
+ * "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::
+ *
+ * out.rgb = plane_alpha * fg.alpha * fg.rgb +
+ * (1 - (plane_alpha * fg.alpha)) * bg.rgb
+ *
+ * Using the following symbols:
+ *
+ * ``fg.rgb``:
+ * Each of the RGB component values from the plane's pixel
+ * ``fg.alpha``:
+ * Alpha component value from the plane's pixel. If the plane's
+ * pixel format has no alpha component, then this is assumed to be
+ * 1.0. In these cases, this property has no effect, as all three
+ * equations become equivalent.
+ * ``bg.rgb``:
+ * Each of the RGB component values from the background
+ * ``plane_alpha``:
+ * Plane alpha value set by the plane "alpha" property. If the
+ * plane does not expose the "alpha" property, then this is
+ * assumed to be 1.0
*
* 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

-- >8 --

* 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).
@@ -409,3 +444,78 @@ 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:
+ *
+ * "None": Blend formula that ignores the pixel alpha.
+ *
+ * "Pre-multiplied": Blend formula that assumes the pixel color values have
+ * been already pre-multiplied with the alpha channel values.
+ *
+ * "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.

Similar thing here - if you put the descriptions (after the ":") on a
new line, and indent them one more level, then it renders much more
nicely:

* "None":
* Blend formula that ignores the pixel alpha.

Thanks,
-Brian

+ *
+ * RETURNS:
+ * Zero for success or -errno
+ */
+int drm_plane_create_blend_mode_property(struct drm_plane *plane,
+ unsigned int supported_modes)
+{
+ struct drm_device *dev = plane->dev;
+ struct drm_property *prop;
+ static const struct drm_prop_enum_list props[] = {
+ { DRM_MODE_BLEND_PIXEL_NONE, "None" },
+ { DRM_MODE_BLEND_PREMULTI, "Pre-multiplied" },
+ { DRM_MODE_BLEND_COVERAGE, "Coverage" },
+ };
+ unsigned int valid_mode_mask = BIT(DRM_MODE_BLEND_PIXEL_NONE) |
+ BIT(DRM_MODE_BLEND_PREMULTI) |
+ BIT(DRM_MODE_BLEND_COVERAGE);
+ int i, j = 0;
+
+ if (WARN_ON((supported_modes & ~valid_mode_mask) ||
+ ((supported_modes & BIT(DRM_MODE_BLEND_PREMULTI)) == 0)))
+ return -EINVAL;
+
+ prop = drm_property_create(dev, DRM_MODE_PROP_ENUM,
+ "pixel blend mode",
+ hweight32(supported_modes));
+ if (!prop)
+ return -ENOMEM;
+
+ for (i = 0; i < ARRAY_SIZE(props); i++) {
+ int ret;
+
+ if (!(BIT(props[i].type) & supported_modes))
+ continue;
+
+ ret = drm_property_add_enum(prop, j++, props[i].type,
+ props[i].name);
+
+ if (ret) {
+ drm_property_destroy(dev, prop);
+
+ return ret;
+ }
+ }
+
+ drm_object_attach_property(&plane->base, prop, DRM_MODE_BLEND_PREMULTI);
+ plane->blend_mode_property = prop;
+
+ if (plane->state)
+ plane->state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
+
+ return 0;
+}
+EXPORT_SYMBOL(drm_plane_create_blend_mode_property);
diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h
index 1760602..2966c0d 100644
--- a/include/drm/drm_blend.h
+++ b/include/drm/drm_blend.h
@@ -27,6 +27,10 @@
#include <linux/ctype.h>
#include <drm/drm_mode.h>

+#define DRM_MODE_BLEND_PIXEL_NONE 0
+#define DRM_MODE_BLEND_PREMULTI 1
+#define DRM_MODE_BLEND_COVERAGE 2
+
struct drm_device;
struct drm_atomic_state;
struct drm_plane;
@@ -49,4 +53,6 @@ int drm_plane_create_zpos_immutable_property(struct drm_plane *plane,
unsigned int zpos);
int drm_atomic_normalize_zpos(struct drm_device *dev,
struct drm_atomic_state *state);
+int drm_plane_create_blend_mode_property(struct drm_plane *plane,
+ unsigned int supported_modes);
#endif
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index f7bf4a4..447ebe7 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -43,6 +43,8 @@
* plane (in 16.16)
* @src_w: width of visible portion of plane (in 16.16)
* @src_h: height of visible portion of plane (in 16.16)
+ * @pixel_blend_mode: how the plane's framebuffer alpha channel is used when
+ * blending with the background colour.
* @rotation: rotation of the plane
* @zpos: priority of the given plane on crtc (optional)
* Note that multiple active planes on the same crtc can have an identical
@@ -106,6 +108,8 @@ struct drm_plane_state {
uint32_t src_x, src_y;
uint32_t src_h, src_w;

+ uint16_t pixel_blend_mode;
+
/* Plane rotation */
unsigned int rotation;

@@ -498,6 +502,7 @@ enum drm_plane_type {
* @type: type of plane (overlay, primary, cursor)
* @zpos_property: zpos property for this plane
* @rotation_property: rotation property for this plane
+ * @blend_mode_property: blend mode property for this plane
* @helper_private: mid-layer private data
*/
struct drm_plane {
@@ -573,6 +578,7 @@ struct drm_plane {

struct drm_property *zpos_property;
struct drm_property *rotation_property;
+ struct drm_property *blend_mode_property;

/**
* @color_encoding_property:
--
1.9.1