* [RFC PATCH] drm: Add per-plane pixel blend mode property
@ 2018-05-08 10:34 Lowry Li
2018-05-09 8:08 ` Daniel Vetter
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Lowry Li @ 2018-05-08 10:34 UTC (permalink / raw)
To: liviu.dudau
Cc: daniel.vetter, jani.nikula, seanpaul, airlied, dri-devel,
linux-kernel, brian.starkey, nd
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@arm.com>
---
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"
+ * property.
+ *
+ * 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 13221cf..6bf95a4 100644
--- a/include/drm/drm_blend.h
+++ b/include/drm/drm_blend.h
@@ -26,6 +26,10 @@
#include <linux/list.h>
#include <linux/ctype.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;
@@ -65,4 +69,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 20867b4..f9cbfee 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -41,6 +41,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
@@ -104,6 +106,9 @@ struct drm_plane_state {
uint32_t src_x, src_y;
uint32_t src_h, src_w;
+ /* Plane pixel blend mode */
+ uint16_t pixel_blend_mode;
+
/* Plane rotation */
unsigned int rotation;
@@ -459,6 +464,7 @@ enum drm_plane_type {
* @state: current atomic state for this plane
* @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 {
@@ -506,6 +512,7 @@ struct drm_plane {
struct drm_property *zpos_property;
struct drm_property *rotation_property;
+ struct drm_property *blend_mode_property;
};
#define obj_to_plane(x) container_of(x, struct drm_plane, base)
--
1.9.1
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] drm: Add per-plane pixel blend mode property
2018-05-08 10:34 [RFC PATCH] drm: Add per-plane pixel blend mode property Lowry Li
@ 2018-05-09 8:08 ` Daniel Vetter
2018-05-09 10:48 ` Ville Syrjälä
2018-05-17 8:05 ` Maarten Lankhorst
2 siblings, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2018-05-09 8:08 UTC (permalink / raw)
To: Lowry Li; +Cc: liviu.dudau, airlied, linux-kernel, dri-devel, daniel.vetter, nd
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 <lowry.li@arm.com>
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: 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"
> + * property.
> + *
> + * 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 13221cf..6bf95a4 100644
> --- a/include/drm/drm_blend.h
> +++ b/include/drm/drm_blend.h
> @@ -26,6 +26,10 @@
> #include <linux/list.h>
> #include <linux/ctype.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;
>
> @@ -65,4 +69,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 20867b4..f9cbfee 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -41,6 +41,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
> @@ -104,6 +106,9 @@ struct drm_plane_state {
> uint32_t src_x, src_y;
> uint32_t src_h, src_w;
>
> + /* Plane pixel blend mode */
Please use in-line struct member comments when you feel like you need a
comment here too. Yes we should probably change the other comments too.
Also would be good to again point at
drm_plane_create_zpos_immutable_property() and related stuff.
> + uint16_t pixel_blend_mode;
> +
> /* Plane rotation */
> unsigned int rotation;
>
> @@ -459,6 +464,7 @@ enum drm_plane_type {
> * @state: current atomic state for this plane
> * @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 {
> @@ -506,6 +512,7 @@ struct drm_plane {
>
> struct drm_property *zpos_property;
> struct drm_property *rotation_property;
> + struct drm_property *blend_mode_property;
> };
>
> #define obj_to_plane(x) container_of(x, struct drm_plane, base)
> --
> 1.9.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] drm: Add per-plane pixel blend mode property
@ 2018-05-09 8:08 ` Daniel Vetter
0 siblings, 0 replies; 10+ messages in thread
From: Daniel Vetter @ 2018-05-09 8:08 UTC (permalink / raw)
To: Lowry Li; +Cc: airlied, liviu.dudau, linux-kernel, dri-devel, daniel.vetter, nd
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 <lowry.li@arm.com>
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: 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"
> + * property.
> + *
> + * 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 13221cf..6bf95a4 100644
> --- a/include/drm/drm_blend.h
> +++ b/include/drm/drm_blend.h
> @@ -26,6 +26,10 @@
> #include <linux/list.h>
> #include <linux/ctype.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;
>
> @@ -65,4 +69,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 20867b4..f9cbfee 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -41,6 +41,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
> @@ -104,6 +106,9 @@ struct drm_plane_state {
> uint32_t src_x, src_y;
> uint32_t src_h, src_w;
>
> + /* Plane pixel blend mode */
Please use in-line struct member comments when you feel like you need a
comment here too. Yes we should probably change the other comments too.
Also would be good to again point at
drm_plane_create_zpos_immutable_property() and related stuff.
> + uint16_t pixel_blend_mode;
> +
> /* Plane rotation */
> unsigned int rotation;
>
> @@ -459,6 +464,7 @@ enum drm_plane_type {
> * @state: current atomic state for this plane
> * @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 {
> @@ -506,6 +512,7 @@ struct drm_plane {
>
> struct drm_property *zpos_property;
> struct drm_property *rotation_property;
> + struct drm_property *blend_mode_property;
> };
>
> #define obj_to_plane(x) container_of(x, struct drm_plane, base)
> --
> 1.9.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] drm: Add per-plane pixel blend mode property
2018-05-08 10:34 [RFC PATCH] drm: Add per-plane pixel blend mode property Lowry Li
@ 2018-05-09 10:48 ` Ville Syrjälä
2018-05-09 10:48 ` Ville Syrjälä
2018-05-17 8:05 ` Maarten Lankhorst
2 siblings, 0 replies; 10+ messages in thread
From: Ville Syrjälä @ 2018-05-09 10:48 UTC (permalink / raw)
To: Lowry Li; +Cc: liviu.dudau, airlied, linux-kernel, dri-devel, daniel.vetter, nd
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 <lowry.li@arm.com>
> ---
> 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 we
could do that then it would be much easier to see what's going to happen
just by reading the string.
But actually I'm not sure we can fit any sensible GL blend func like
names in the limited length of the enum value strings. Would be nice
to try at least.
> + *
> + * 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"
> + * property.
> + *
> + * 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 13221cf..6bf95a4 100644
> --- a/include/drm/drm_blend.h
> +++ b/include/drm/drm_blend.h
> @@ -26,6 +26,10 @@
> #include <linux/list.h>
> #include <linux/ctype.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;
>
> @@ -65,4 +69,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 20867b4..f9cbfee 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -41,6 +41,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
> @@ -104,6 +106,9 @@ struct drm_plane_state {
> uint32_t src_x, src_y;
> uint32_t src_h, src_w;
>
> + /* Plane pixel blend mode */
> + uint16_t pixel_blend_mode;
> +
> /* Plane rotation */
> unsigned int rotation;
>
> @@ -459,6 +464,7 @@ enum drm_plane_type {
> * @state: current atomic state for this plane
> * @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 {
> @@ -506,6 +512,7 @@ struct drm_plane {
>
> struct drm_property *zpos_property;
> struct drm_property *rotation_property;
> + struct drm_property *blend_mode_property;
> };
>
> #define obj_to_plane(x) container_of(x, struct drm_plane, base)
> --
> 1.9.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] drm: Add per-plane pixel blend mode property
@ 2018-05-09 10:48 ` Ville Syrjälä
0 siblings, 0 replies; 10+ messages in thread
From: Ville Syrjälä @ 2018-05-09 10:48 UTC (permalink / raw)
To: Lowry Li; +Cc: airlied, liviu.dudau, linux-kernel, dri-devel, daniel.vetter, nd
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 <lowry.li@arm.com>
> ---
> 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 we
could do that then it would be much easier to see what's going to happen
just by reading the string.
But actually I'm not sure we can fit any sensible GL blend func like
names in the limited length of the enum value strings. Would be nice
to try at least.
> + *
> + * 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"
> + * property.
> + *
> + * 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 13221cf..6bf95a4 100644
> --- a/include/drm/drm_blend.h
> +++ b/include/drm/drm_blend.h
> @@ -26,6 +26,10 @@
> #include <linux/list.h>
> #include <linux/ctype.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;
>
> @@ -65,4 +69,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 20867b4..f9cbfee 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -41,6 +41,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
> @@ -104,6 +106,9 @@ struct drm_plane_state {
> uint32_t src_x, src_y;
> uint32_t src_h, src_w;
>
> + /* Plane pixel blend mode */
> + uint16_t pixel_blend_mode;
> +
> /* Plane rotation */
> unsigned int rotation;
>
> @@ -459,6 +464,7 @@ enum drm_plane_type {
> * @state: current atomic state for this plane
> * @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 {
> @@ -506,6 +512,7 @@ struct drm_plane {
>
> struct drm_property *zpos_property;
> struct drm_property *rotation_property;
> + struct drm_property *blend_mode_property;
> };
>
> #define obj_to_plane(x) container_of(x, struct drm_plane, base)
> --
> 1.9.1
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
--
Ville Syrjälä
Intel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] drm: Add per-plane pixel blend mode property
2018-05-08 10:34 [RFC PATCH] drm: Add per-plane pixel blend mode property Lowry Li
@ 2018-05-17 8:05 ` Maarten Lankhorst
2018-05-09 10:48 ` Ville Syrjälä
2018-05-17 8:05 ` Maarten Lankhorst
2 siblings, 0 replies; 10+ messages in thread
From: Maarten Lankhorst @ 2018-05-17 8:05 UTC (permalink / raw)
To: Lowry Li, liviu.dudau; +Cc: airlied, linux-kernel, dri-devel, daniel.vetter, nd
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 <lowry.li@arm.com>
> ---
> 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"
> + * property.
> + *
> + * 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));
Just a small nitpick: all other crtc properties appear to use upper case
and underscores instead of spaces and lowercase for the property name.
For the value names I would imagine it's fine, but probably best to be
consistent for the name.
:)
All in all with the review comments from the others addressed, I think this looks good.
~Maarten
> + 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 13221cf..6bf95a4 100644
> --- a/include/drm/drm_blend.h
> +++ b/include/drm/drm_blend.h
> @@ -26,6 +26,10 @@
> #include <linux/list.h>
> #include <linux/ctype.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;
Seems in general when such values are defined, they're put inside uapi/drm/drm_mode.h :)
> @@ -65,4 +69,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 20867b4..f9cbfee 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -41,6 +41,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
> @@ -104,6 +106,9 @@ struct drm_plane_state {
> uint32_t src_x, src_y;
> uint32_t src_h, src_w;
>
> + /* Plane pixel blend mode */
> + uint16_t pixel_blend_mode;
> +
> /* Plane rotation */
> unsigned int rotation;
>
> @@ -459,6 +464,7 @@ enum drm_plane_type {
> * @state: current atomic state for this plane
> * @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 {
> @@ -506,6 +512,7 @@ struct drm_plane {
>
> struct drm_property *zpos_property;
> struct drm_property *rotation_property;
> + struct drm_property *blend_mode_property;
> };
>
> #define obj_to_plane(x) container_of(x, struct drm_plane, base)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] drm: Add per-plane pixel blend mode property
@ 2018-05-17 8:05 ` Maarten Lankhorst
0 siblings, 0 replies; 10+ messages in thread
From: Maarten Lankhorst @ 2018-05-17 8:05 UTC (permalink / raw)
To: Lowry Li, liviu.dudau; +Cc: airlied, daniel.vetter, nd, linux-kernel, dri-devel
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 <lowry.li@arm.com>
> ---
> 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"
> + * property.
> + *
> + * 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));
Just a small nitpick: all other crtc properties appear to use upper case
and underscores instead of spaces and lowercase for the property name.
For the value names I would imagine it's fine, but probably best to be
consistent for the name.
:)
All in all with the review comments from the others addressed, I think this looks good.
~Maarten
> + 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 13221cf..6bf95a4 100644
> --- a/include/drm/drm_blend.h
> +++ b/include/drm/drm_blend.h
> @@ -26,6 +26,10 @@
> #include <linux/list.h>
> #include <linux/ctype.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;
Seems in general when such values are defined, they're put inside uapi/drm/drm_mode.h :)
> @@ -65,4 +69,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 20867b4..f9cbfee 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -41,6 +41,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
> @@ -104,6 +106,9 @@ struct drm_plane_state {
> uint32_t src_x, src_y;
> uint32_t src_h, src_w;
>
> + /* Plane pixel blend mode */
> + uint16_t pixel_blend_mode;
> +
> /* Plane rotation */
> unsigned int rotation;
>
> @@ -459,6 +464,7 @@ enum drm_plane_type {
> * @state: current atomic state for this plane
> * @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 {
> @@ -506,6 +512,7 @@ struct drm_plane {
>
> struct drm_property *zpos_property;
> struct drm_property *rotation_property;
> + struct drm_property *blend_mode_property;
> };
>
> #define obj_to_plane(x) container_of(x, struct drm_plane, base)
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] drm: Add per-plane pixel blend mode property
2018-05-09 8:08 ` Daniel Vetter
(?)
@ 2018-05-22 11:07 ` Lowry Li
-1 siblings, 0 replies; 10+ messages in thread
From: Lowry Li @ 2018-05-22 11:07 UTC (permalink / raw)
To: liviu.dudau, airlied, linux-kernel, dri-devel, daniel.vetter, nd
On Wed, May 09, 2018 at 10:08:16AM +0200, Daniel Vetter wrote:
> 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 <lowry.li@arm.com>
>
> Some suggestions for the kerneldoc below. lgtm otherwise (but needs the
> driver implementation + userspace support before we can merge).
> -Daniel
Thanks for the review comments.
OK. Driver implementation and userspace support will be committed in
other patchsets.
> > ---
> > 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.
About the details of blending equations, as your comments below, we
will move them here. And drm_plane_create_blend_mode_property()
already has a link which can take the reader to the function easily.
> > + *
> > * 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).
Ok, will delete the "DRM_MODE_BLEND_PIXEL_NONE" bits.
> > + * "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).
Will move the blending equation to the DOC comment.
Ok, will use fg.* instead of pixel.* and plane_alpha instead of
plane.alpha.
> > + *
> > + * 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"
> > + * property.
> > + *
> > + * 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 13221cf..6bf95a4 100644
> > --- a/include/drm/drm_blend.h
> > +++ b/include/drm/drm_blend.h
> > @@ -26,6 +26,10 @@
> > #include <linux/list.h>
> > #include <linux/ctype.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;
> >
> > @@ -65,4 +69,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 20867b4..f9cbfee 100644
> > --- a/include/drm/drm_plane.h
> > +++ b/include/drm/drm_plane.h
> > @@ -41,6 +41,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
> > @@ -104,6 +106,9 @@ struct drm_plane_state {
> > uint32_t src_x, src_y;
> > uint32_t src_h, src_w;
> >
> > + /* Plane pixel blend mode */
>
> Please use in-line struct member comments when you feel like you need a
> comment here too. Yes we should probably change the other comments too.
> Also would be good to again point at
> drm_plane_create_zpos_immutable_property() and related stuff.
We've given the detail description of it on the comments of
drm_plane_state. So we can remove the comment here.
> > + uint16_t pixel_blend_mode;
> > +
> > /* Plane rotation */
> > unsigned int rotation;
> >
> > @@ -459,6 +464,7 @@ enum drm_plane_type {
> > * @state: current atomic state for this plane
> > * @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 {
> > @@ -506,6 +512,7 @@ struct drm_plane {
> >
> > struct drm_property *zpos_property;
> > struct drm_property *rotation_property;
> > + struct drm_property *blend_mode_property;
> > };
> >
> > #define obj_to_plane(x) container_of(x, struct drm_plane, base)
> > --
> > 1.9.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch
--
Regards,
Lowry
IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] drm: Add per-plane pixel blend mode property
2018-05-09 10:48 ` Ville Syrjälä
(?)
@ 2018-05-22 11:13 ` Lowry Li
-1 siblings, 0 replies; 10+ messages in thread
From: Lowry Li @ 2018-05-22 11:13 UTC (permalink / raw)
To: Ville Syrjälä
Cc: airlied, liviu.dudau, linux-kernel, dri-devel, daniel.vetter, nd
On Wed, May 09, 2018 at 01:48:06PM +0300, Ville Syrjälä wrote:
> 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 <lowry.li@arm.com>
> > ---
> > 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 we
> could do that then it would be much easier to see what's going to happen
> just by reading the string.
>
> But actually I'm not sure we can fit any sensible GL blend func like
> names in the limited length of the enum value strings. Would be nice
> to try at least.
Thanks a lot for the review comments. About the names, any suggestion
about this? We are just copying the android.
> > + *
> > + * 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"
> > + * property.
> > + *
> > + * 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 13221cf..6bf95a4 100644
> > --- a/include/drm/drm_blend.h
> > +++ b/include/drm/drm_blend.h
> > @@ -26,6 +26,10 @@
> > #include <linux/list.h>
> > #include <linux/ctype.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;
> >
> > @@ -65,4 +69,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 20867b4..f9cbfee 100644
> > --- a/include/drm/drm_plane.h
> > +++ b/include/drm/drm_plane.h
> > @@ -41,6 +41,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
> > @@ -104,6 +106,9 @@ struct drm_plane_state {
> > uint32_t src_x, src_y;
> > uint32_t src_h, src_w;
> >
> > + /* Plane pixel blend mode */
> > + uint16_t pixel_blend_mode;
> > +
> > /* Plane rotation */
> > unsigned int rotation;
> >
> > @@ -459,6 +464,7 @@ enum drm_plane_type {
> > * @state: current atomic state for this plane
> > * @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 {
> > @@ -506,6 +512,7 @@ struct drm_plane {
> >
> > struct drm_property *zpos_property;
> > struct drm_property *rotation_property;
> > + struct drm_property *blend_mode_property;
> > };
> >
> > #define obj_to_plane(x) container_of(x, struct drm_plane, base)
> > --
> > 1.9.1
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Ville Syrjälä
> Intel
--
Regards,
Lowry
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [RFC PATCH] drm: Add per-plane pixel blend mode property
2018-05-17 8:05 ` Maarten Lankhorst
(?)
@ 2018-05-22 11:25 ` Lowry Li
-1 siblings, 0 replies; 10+ messages in thread
From: Lowry Li @ 2018-05-22 11:25 UTC (permalink / raw)
To: Maarten Lankhorst
Cc: airlied, liviu.dudau, linux-kernel, dri-devel, daniel.vetter, nd
On Thu, May 17, 2018 at 10:05:35AM +0200, Maarten Lankhorst wrote:
> 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 <lowry.li@arm.com>
> > ---
> > 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"
> > + * property.
> > + *
> > + * 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));
>
> Just a small nitpick: all other crtc properties appear to use upper case
> and underscores instead of spaces and lowercase for the property name.
> For the value names I would imagine it's fine, but probably best to be
> consistent for the name.
>
> :)
>
> All in all with the review comments from the others addressed, I think this looks good.
>
> ~Maarten
Thanks a lot for your review. We asked on IRC about lower case or
upper case, the verdict was whatever is OK. But the plane "alpha"
property here is lowercase. Considering this, we make ours to be lower
case too.
Please also refer to the talking record as below on IRC.
https://people.freedesktop.org/~cbrill/dri-log/?channel=dri-devel&highlight_names=&date=2018-04-25&show_html=true
Please read from around 13:27. :)
> > + 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 13221cf..6bf95a4 100644
> > --- a/include/drm/drm_blend.h
> > +++ b/include/drm/drm_blend.h
> > @@ -26,6 +26,10 @@
> > #include <linux/list.h>
> > #include <linux/ctype.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;
> Seems in general when such values are defined, they're put inside uapi/drm/drm_mode.h :)
The reason is that userspace programs should be relying on the enum
names (not the values). so we want them not in uapi but drm_mode.h.
> > @@ -65,4 +69,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 20867b4..f9cbfee 100644
> > --- a/include/drm/drm_plane.h
> > +++ b/include/drm/drm_plane.h
> > @@ -41,6 +41,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
> > @@ -104,6 +106,9 @@ struct drm_plane_state {
> > uint32_t src_x, src_y;
> > uint32_t src_h, src_w;
> >
> > + /* Plane pixel blend mode */
> > + uint16_t pixel_blend_mode;
> > +
> > /* Plane rotation */
> > unsigned int rotation;
> >
> > @@ -459,6 +464,7 @@ enum drm_plane_type {
> > * @state: current atomic state for this plane
> > * @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 {
> > @@ -506,6 +512,7 @@ struct drm_plane {
> >
> > struct drm_property *zpos_property;
> > struct drm_property *rotation_property;
> > + struct drm_property *blend_mode_property;
> > };
> >
> > #define obj_to_plane(x) container_of(x, struct drm_plane, base)
>
--
Regards,
Lowry
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2018-05-22 11:26 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-05-08 10:34 [RFC PATCH] drm: Add per-plane pixel blend mode property Lowry Li
2018-05-09 8:08 ` Daniel Vetter
2018-05-09 8:08 ` Daniel Vetter
2018-05-22 11:07 ` Lowry Li
2018-05-09 10:48 ` Ville Syrjälä
2018-05-09 10:48 ` Ville Syrjälä
2018-05-22 11:13 ` Lowry Li
2018-05-17 8:05 ` Maarten Lankhorst
2018-05-17 8:05 ` Maarten Lankhorst
2018-05-22 11:25 ` Lowry Li
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.