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

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@arm.com

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

Reviewed-by: Brian Starkey brian.starkey@arm.com

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