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

[Emil Velikov]

On 1 June 2018 at 13:41, Lowry Li <lowry.li@xxxxxxx> 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>
> ---
>  drivers/gpu/drm/drm_atomic.c        |   4 ++
>  drivers/gpu/drm/drm_atomic_helper.c |   1 +
>  drivers/gpu/drm/drm_blend.c         | 126 ++++++++++++++++++++++++++++++++++++
>  include/drm/drm_blend.h             |   6 ++
>  include/drm/drm_plane.h             |   5 ++
>  5 files changed, 142 insertions(+)
>
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 07fef42..1d18389 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -785,6 +785,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>                 state->src_h = val;
>         } else if (property == plane->alpha_property) {
>                 state->alpha = 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;
> @@ -852,6 +854,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,
>                 *val = state->src_h;
>         } else if (property == plane->alpha_property) {
>                 *val = state->alpha;
> +       } 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 130da51..7f5d463 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3515,6 +3515,7 @@ void drm_atomic_helper_plane_reset(struct drm_plane *plane)
>                 /* Reset the alpha value to fully opaque if it matters */
>                 if (plane->alpha_property)
>                         plane->state->alpha = plane->alpha_property->values[1];
> +               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 a16a74d..ac6f19c 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -107,6 +107,52 @@
>   *     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 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
>   * exposed and assumed to be black).
> @@ -448,3 +494,83 @@ 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). Current DRM assumption is
> + *                  that alpha is premultiplied, and old userspace can break if
> + *                  the property defaults to coverage.
> + *

Thanks for the explanation Lowry.
Sigh, old userspace :-\

One pedantic suggestion s/defaults to coverage/defaults to anything else/
Since defaulting to "none" or any future blend mode is also a no-go.

I wouldn't bother resending solely for ^^. Perhaps squash locally
before merging?

With that, the patch is
Reviewed-by: Emil Velikov <emil.velikov@xxxxxxxxxxxxx>

-Emil