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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, May 30, 2018 at 02:27:55PM +0100, Brian Starkey wrote:
> Hi Lowry,
> 
> On Wed, May 30, 2018 at 07:23:53PM +0800, Lowry Li wrote:
> >Pixel blend modes represent the alpha blending equation
> >selection, describing how the pixels from the current
> >plane are composited with the background.
> >
> >Add a pixel_blend_mode to drm_plane_state and a
> >blend_mode_property to drm_plane, and related support
> >functions.
> >
> >Defines three blend modes in drm_blend.h.
> >
> >Signed-off-by: Lowry Li <lowry.li@xxxxxxx>
> 
> With or without the kerneldoc tweaks I've suggested below, this patch
> is:
> 
> Reviewed-by: Brian Starkey <brian.starkey@xxxxxxx>
> 
Hi Brian,

Thanks for the patch and comments. Will update a new patchset with this.

Regards,
Lowry
> >---
> >drivers/gpu/drm/drm_atomic.c        |   4 ++
> >drivers/gpu/drm/drm_atomic_helper.c |   1 +
> >drivers/gpu/drm/drm_blend.c         | 110 ++++++++++++++++++++++++++++++++++++
> >include/drm/drm_blend.h             |   6 ++
> >include/drm/drm_plane.h             |   6 ++
> >5 files changed, 127 insertions(+)
> >
> >diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >index 7d25c42..467f8de 100644
> >--- a/drivers/gpu/drm/drm_atomic.c
> >+++ b/drivers/gpu/drm/drm_atomic.c
> >@@ -783,6 +783,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
> >		state->src_w = val;
> >	} else if (property == config->prop_src_h) {
> >		state->src_h = val;
> >+	} else if (property == plane->blend_mode_property) {
> >+		state->pixel_blend_mode = val;
> >	} else if (property == plane->rotation_property) {
> >		if (!is_power_of_2(val & DRM_MODE_ROTATE_MASK))
> >			return -EINVAL;
> >@@ -848,6 +850,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
> >		*val = state->src_w;
> >	} else if (property == config->prop_src_h) {
> >		*val = state->src_h;
> >+	} else if (property == plane->blend_mode_property) {
> >+		*val = state->pixel_blend_mode;
> >	} else if (property == plane->rotation_property) {
> >		*val = state->rotation;
> >	} else if (property == plane->zpos_property) {
> >diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c
> >index c356545..ddbd0d2 100644
> >--- a/drivers/gpu/drm/drm_atomic_helper.c
> >+++ b/drivers/gpu/drm/drm_atomic_helper.c
> >@@ -3484,6 +3484,7 @@ void drm_atomic_helper_plane_reset(struct drm_plane *plane)
> >	if (plane->state) {
> >		plane->state->plane = plane;
> >		plane->state->rotation = DRM_MODE_ROTATE_0;
> >+		plane->state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
> >	}
> >}
> >EXPORT_SYMBOL(drm_atomic_helper_plane_reset);
> >diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> >index 5a81e1b..4ac45da 100644
> >--- a/drivers/gpu/drm/drm_blend.c
> >+++ b/drivers/gpu/drm/drm_blend.c
> >@@ -100,6 +100,41 @@
> > *	planes. Without this property the primary plane is always below the cursor
> > *	plane, and ordering between all other planes is undefined.
> > *
> >+ * pixel blend mode:
> >+ *	Pixel blend mode is set up with drm_plane_create_blend_mode_property().
> >+ *	It adds a blend mode for alpha blending equation selection, describing
> >+ *	how the pixels from the current plane are composited with the
> >+ *	background.
> >+ *
> >+ *	Three alpha blending equations(note that the fg.rgb or bg.rgb notation
> >+ *	means each of the R, G or B channels for the foreground and background
> >+ *	colors, respectively):
> >+ *
> >+ *	"None": Blend formula that ignores the pixel alpha.
> >+ *	out.rgb = plane_alpha * fg.rgb + (1 - plane_alpha) * bg.rgb
> >+ *
> >+ *	"Pre-multiplied": Blend formula that assumes the pixel color values
> >+ *	have been already pre-multiplied with the alpha
> >+ *	channel values.
> >+ *	out.rgb = plane_alpha * fg.rgb + (1 - (plane_alpha * fg.alpha)) * bg.rgb
> >+ *
> >+ *	"Coverage": Blend formula that assumes the pixel color values have not
> >+ *	been pre-multiplied and will do so when blending them to the background
> >+ *	color values.
> >+ *	out.rgb = plane_alpha * fg.alpha * fg.rgb +
> >+ *		  (1 - (plane_alpha * fg.alpha)) * bg.rgb
> >+ *
> >+ *	fg.rgb: Each of the RGB component values from the plane's pixel
> >+ *	fg.alpha: Alpha component value from the plane's pixel
> >+ *	bg.rgb: Each of the RGB component values from the background
> >+ *	plane_alpha: Plane alpha value set by the plane alpha property (if
> >+ *		     applicable).
> >+ *
> >+ *	This property has no effect on formats with no pixel alpha, as fg.alpha
> >+ *	is assumed to be 1.0. If the plane does not expose the "alpha" property,
> >+ *	then plane_alpha is assumed to be 1.0, otherwise, it is the value of the
> >+ *	"alpha" property.
> >+ *
> 
> I did a little fiddling to make this render nicely in the
> documentation output. If you like it, you can apply the hunk below:
> 
> -- >8 --
> 
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index 714b0602127d..7b5c3307d4c1 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -113,34 +113,45 @@
>   *	how the pixels from the current plane are composited with the
>   *	background.
>   *
> - *	Three alpha blending equations(note that the fg.rgb or bg.rgb notation
> - *	means each of the R, G or B channels for the foreground and background
> - *	colors, respectively):
> - *
> - *	"None": Blend formula that ignores the pixel alpha.
> - *	out.rgb = plane_alpha * fg.rgb + (1 - plane_alpha) * bg.rgb
> - *
> - *	"Pre-multiplied": Blend formula that assumes the pixel color values
> - *	have been already pre-multiplied with the alpha
> - *	channel values.
> - *	out.rgb = plane_alpha * fg.rgb + (1 - (plane_alpha * fg.alpha)) * bg.rgb
> - *
> - *	"Coverage": Blend formula that assumes the pixel color values have not
> - *	been pre-multiplied and will do so when blending them to the background
> - *	color values.
> - *	out.rgb = plane_alpha * fg.alpha * fg.rgb +
> - *		  (1 - (plane_alpha * fg.alpha)) * bg.rgb
> - *
> - *	fg.rgb: Each of the RGB component values from the plane's pixel
> - *	fg.alpha: Alpha component value from the plane's pixel
> - *	bg.rgb: Each of the RGB component values from the background
> - *	plane_alpha: Plane alpha value set by the plane alpha property (if
> - *		     applicable).
> - *
> - *	This property has no effect on formats with no pixel alpha, as fg.alpha
> - *	is assumed to be 1.0. If the plane does not expose the "alpha" property,
> - *	then plane_alpha is assumed to be 1.0, otherwise, it is the value of the
> - *	"alpha" property.
> + *	Three alpha blending equations are defined:
> + *
> + *	"None":
> + *		Blend formula that ignores the pixel alpha::
> + *
> + *			out.rgb = plane_alpha * fg.rgb +
> + *				(1 - plane_alpha) * bg.rgb
> + *
> + *	"Pre-multiplied":
> + *		Blend formula that assumes the pixel color values
> + *		have been already pre-multiplied with the alpha
> + *		channel values::
> + *
> + *			out.rgb = plane_alpha * fg.rgb +
> + *				(1 - (plane_alpha * fg.alpha)) * bg.rgb
> + *
> + *	"Coverage":
> + *		Blend formula that assumes the pixel color values have not
> + *		been pre-multiplied and will do so when blending them to the
> + *		background color values::
> + *
> + *			out.rgb = plane_alpha * fg.alpha * fg.rgb +
> + *				(1 - (plane_alpha * fg.alpha)) * bg.rgb
> + *
> + *	Using the following symbols:
> + *
> + *	``fg.rgb``:
> + *		Each of the RGB component values from the plane's pixel
> + *	``fg.alpha``:
> + *		Alpha component value from the plane's pixel. If the plane's
> + *		pixel format has no alpha component, then this is assumed to be
> + *		1.0. In these cases, this property has no effect, as all three
> + *		equations become equivalent.
> + *	``bg.rgb``:
> + *		Each of the RGB component values from the background
> + *	``plane_alpha``:
> + *		Plane alpha value set by the plane "alpha" property. If the
> + *		plane does not expose the "alpha" property, then this is
> + *		assumed to be 1.0
>   *
>   * Note that all the property extensions described here apply either to the
>   * plane or the CRTC (e.g. for the background color, which currently is not
> 
> -- >8 --
> 
> > * Note that all the property extensions described here apply either to the
> > * plane or the CRTC (e.g. for the background color, which currently is not
> > * exposed and assumed to be black).
> >@@ -409,3 +444,78 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
> >	return 0;
> >}
> >EXPORT_SYMBOL(drm_atomic_normalize_zpos);
> >+
> >+/**
> >+ * drm_plane_create_blend_mode_property - create a new blend mode property
> >+ * @plane: drm plane
> >+ * @supported_modes: bitmask of supported modes, must include
> >+ *		     BIT(DRM_MODE_BLEND_PREMULTI)
> >+ *
> >+ * This creates a new property describing the blend mode.
> >+ *
> >+ * The property exposed to userspace is an enumeration property (see
> >+ * drm_property_create_enum()) called "pixel blend mode" and has the
> >+ * following enumeration values:
> >+ *
> >+ * "None": Blend formula that ignores the pixel alpha.
> >+ *
> >+ * "Pre-multiplied": Blend formula that assumes the pixel color values have
> >+ *		     been already pre-multiplied with the alpha channel values.
> >+ *
> >+ * "Coverage": Blend formula that assumes the pixel color values have not been
> >+ *	       pre-multiplied and will do so when blending them to the
> >+ *	       background color values.
> 
> Similar thing here - if you put the descriptions (after the ":") on a
> new line, and indent them one more level, then it renders much more
> nicely:
> 
> * "None":
> *	Blend formula that ignores the pixel alpha.
> 
> Thanks,
> -Brian
> 
> >+ *
> >+ * RETURNS:
> >+ * Zero for success or -errno
> >+ */
> >+int drm_plane_create_blend_mode_property(struct drm_plane *plane,
> >+					 unsigned int supported_modes)
> >+{
> >+	struct drm_device *dev = plane->dev;
> >+	struct drm_property *prop;
> >+	static const struct drm_prop_enum_list props[] = {
> >+		{ DRM_MODE_BLEND_PIXEL_NONE, "None" },
> >+		{ DRM_MODE_BLEND_PREMULTI, "Pre-multiplied" },
> >+		{ DRM_MODE_BLEND_COVERAGE, "Coverage" },
> >+	};
> >+	unsigned int valid_mode_mask = BIT(DRM_MODE_BLEND_PIXEL_NONE) |
> >+				       BIT(DRM_MODE_BLEND_PREMULTI)   |
> >+				       BIT(DRM_MODE_BLEND_COVERAGE);
> >+	int i, j = 0;
> >+
> >+	if (WARN_ON((supported_modes & ~valid_mode_mask) ||
> >+		    ((supported_modes & BIT(DRM_MODE_BLEND_PREMULTI)) == 0)))
> >+		return -EINVAL;
> >+
> >+	prop = drm_property_create(dev, DRM_MODE_PROP_ENUM,
> >+				   "pixel blend mode",
> >+				   hweight32(supported_modes));
> >+	if (!prop)
> >+		return -ENOMEM;
> >+
> >+	for (i = 0; i < ARRAY_SIZE(props); i++) {
> >+		int ret;
> >+
> >+		if (!(BIT(props[i].type) & supported_modes))
> >+			continue;
> >+
> >+		ret = drm_property_add_enum(prop, j++, props[i].type,
> >+					    props[i].name);
> >+
> >+		if (ret) {
> >+			drm_property_destroy(dev, prop);
> >+
> >+			return ret;
> >+		}
> >+	}
> >+
> >+	drm_object_attach_property(&plane->base, prop, DRM_MODE_BLEND_PREMULTI);
> >+	plane->blend_mode_property = prop;
> >+
> >+	if (plane->state)
> >+		plane->state->pixel_blend_mode = DRM_MODE_BLEND_PREMULTI;
> >+
> >+	return 0;
> >+}
> >+EXPORT_SYMBOL(drm_plane_create_blend_mode_property);
> >diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h
> >index 1760602..2966c0d 100644
> >--- a/include/drm/drm_blend.h
> >+++ b/include/drm/drm_blend.h
> >@@ -27,6 +27,10 @@
> >#include <linux/ctype.h>
> >#include <drm/drm_mode.h>
> >
> >+#define DRM_MODE_BLEND_PIXEL_NONE	0
> >+#define DRM_MODE_BLEND_PREMULTI		1
> >+#define DRM_MODE_BLEND_COVERAGE		2
> >+
> >struct drm_device;
> >struct drm_atomic_state;
> >struct drm_plane;
> >@@ -49,4 +53,6 @@ int drm_plane_create_zpos_immutable_property(struct drm_plane *plane,
> >					     unsigned int zpos);
> >int drm_atomic_normalize_zpos(struct drm_device *dev,
> >			      struct drm_atomic_state *state);
> >+int drm_plane_create_blend_mode_property(struct drm_plane *plane,
> >+					 unsigned int supported_modes);
> >#endif
> >diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> >index f7bf4a4..447ebe7 100644
> >--- a/include/drm/drm_plane.h
> >+++ b/include/drm/drm_plane.h
> >@@ -43,6 +43,8 @@
> > *	plane (in 16.16)
> > * @src_w: width of visible portion of plane (in 16.16)
> > * @src_h: height of visible portion of plane (in 16.16)
> >+ * @pixel_blend_mode: how the plane's framebuffer alpha channel is used when
> >+ *	blending with the background colour.
> > * @rotation: rotation of the plane
> > * @zpos: priority of the given plane on crtc (optional)
> > *	Note that multiple active planes on the same crtc can have an identical
> >@@ -106,6 +108,8 @@ struct drm_plane_state {
> >	uint32_t src_x, src_y;
> >	uint32_t src_h, src_w;
> >
> >+	uint16_t pixel_blend_mode;
> >+
> >	/* Plane rotation */
> >	unsigned int rotation;
> >
> >@@ -498,6 +502,7 @@ enum drm_plane_type {
> > * @type: type of plane (overlay, primary, cursor)
> > * @zpos_property: zpos property for this plane
> > * @rotation_property: rotation property for this plane
> >+ * @blend_mode_property: blend mode property for this plane
> > * @helper_private: mid-layer private data
> > */
> >struct drm_plane {
> >@@ -573,6 +578,7 @@ struct drm_plane {
> >
> >	struct drm_property *zpos_property;
> >	struct drm_property *rotation_property;
> >+	struct drm_property *blend_mode_property;
> >
> >	/**
> >	 * @color_encoding_property:
> >-- 
> >1.9.1
> >

-- 
Regards,
Lowry
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux