Re: [PATCH v4 1/5] drm/blend: Add a generic alpha property

[Eric Anholt]

Paul Kocialkowski  writes:

> [ Unknown signature status ]
> Hi,
>
> On Fri, 2018-03-30 at 13:37 -0700, Eric Anholt wrote:
>> Maxime Ripard  writes:
>> 
>> > Some drivers duplicate the logic to create a property to store a
>> > per-plane
>> > alpha.
>> > 
>> > This is especially useful if we ever want to support extra protocols
>> > for
>> > Wayland like:
>> > https://lists.freedesktop.org/archives/wayland-devel/2017-August/034
>> > 741.html
>> > 
>> > Let's create a helper in order to move that to the core.
>> > diff --git a/drivers/gpu/drm/drm_blend.c
>> > b/drivers/gpu/drm/drm_blend.c
>> > index 5a81e1b4c076..05eda2d57c77 100644
>> > --- a/drivers/gpu/drm/drm_blend.c
>> > +++ b/drivers/gpu/drm/drm_blend.c
>> > @@ -88,6 +88,12 @@
>> >   * On top of this basic transformation additional properties can be
>> > exposed by
>> >   * the driver:
>> >   *
>> > + * alpha:
>> > + *Alpha is setup with drm_plane_create_alpha_property().
>> > It controls the
>> 
>> s/setup/set up/
>> 
>> > + *plane-wide opacity, from transparent (0) to opaque
>> > (0x). It can be
>> > + *combined with pixel alpha.
>> > + *The alpha value is represented as premultiplied alpha.
>> 
>> I don't think this premultiplied comment makes any sense.  What are
>> you saying it's premultiplied with?  Maybe you mean that the output
>> pixels will have both their color and alpha channels multiplied by
>> this alpha?
>> I'd just drop it.
>
> I disagree here: since there are multiple ways to blend the pixel alpha
> value and the plane-wide alpha value, I think it's important to clearly
> specify which blending equation DRM expects here. Otherwise, the plane-
> wide alpha value just doesn't have a specified meaning and driver
> implementations can't figure whether the hardware uses the same equation
> or not and whether adaptation to this coefficient is needed.
>
> The equations for premultiplied alpha blending are at:
> https://en.wikipedia.org/wiki/Alpha_compositing#Alpha_blending
>
> What do you think?

"Premultiplied alpha" only has a meaning for an RGBA value, where RGB
have been already multiplied by their own A.  In this case, you're
saying that an alpha-only value is "premultiplied", but with what?  What
would an alternative to it being "premultiplied" do instead?  That's
what I'm confused about.


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 1/5] drm/blend: Add a generic alpha property

[Laurent Pinchart]

Hi Paul,

On Wednesday, 4 April 2018 12:15:04 EEST Paul Kocialkowski wrote:
> On Fri, 2018-03-30 at 13:37 -0700, Eric Anholt wrote:
> > Maxime Ripard  writes:
> >> Some drivers duplicate the logic to create a property to store a
> >> per-plane alpha.
> >> 
> >> This is especially useful if we ever want to support extra protocols
> >> for Wayland like:
> >> https://lists.freedesktop.org/archives/wayland-devel/2017-August/034
> >> 741.html
> >> 
> >> Let's create a helper in order to move that to the core.
> >> diff --git a/drivers/gpu/drm/drm_blend.c
> >> b/drivers/gpu/drm/drm_blend.c
> >> index 5a81e1b4c076..05eda2d57c77 100644
> >> --- a/drivers/gpu/drm/drm_blend.c
> >> +++ b/drivers/gpu/drm/drm_blend.c
> >> @@ -88,6 +88,12 @@
> >>   * On top of this basic transformation additional properties can be
> >> exposed by
> >>   * the driver:
> >>   *
> >> + * alpha:
> >> + *Alpha is setup with drm_plane_create_alpha_property().
> >> It controls the
> > 
> > s/setup/set up/
> > 
> >> + *plane-wide opacity, from transparent (0) to opaque
> >> (0x). It can be
> >> + *combined with pixel alpha.
> >> + *The alpha value is represented as premultiplied alpha.
> > 
> > I don't think this premultiplied comment makes any sense.  What are
> > you saying it's premultiplied with?  Maybe you mean that the output
> > pixels will have both their color and alpha channels multiplied by
> > this alpha? I'd just drop it.
> 
> I disagree here: since there are multiple ways to blend the pixel alpha
> value and the plane-wide alpha value, I think it's important to clearly
> specify which blending equation DRM expects here. Otherwise, the plane-
> wide alpha value just doesn't have a specified meaning and driver
> implementations can't figure whether the hardware uses the same equation
> or not and whether adaptation to this coefficient is needed.
> 
> The equations for premultiplied alpha blending are at:
> https://en.wikipedia.org/wiki/Alpha_compositing#Alpha_blending
> 
> What do you think?

I think it's important to be clear in the documentation, but I also don't 
think "premultiplied alpha" makes sense in this context. It's not the alpha 
value that is premultiplied, it is the RGB data. As correctly explained in the 
link you have provided, to compute the output RGB values the src and dst RGB 
values have to be multiplied by the src and dst alpha respectively. To 
simplify the hardware computation, RGB values can be premultiplied in a the 
framebuffer. This is useful when you deal with textures that have an alpha 
component (you can premultiply them offline first) or with multi-pass alpha 
blending where you can skip the division by outA in the intermediate steps. 
However, for plane global alpha, I fail to see a use case that would feed the 
device with a XRGB framebuffer premultiplied with a fixed global alpha value.

To summarize this, I'm fine specifying how alpha premultiplication is handled 
in relationship with the generic alpha property, but I think Maxime's patch 
was wrong, and the RGB values should not be premultiplied.

> > Reviewed-by: Eric Anholt 
> > 
> > We should probably do this property for vc4 at some point, too.

-- 
Regards,

Laurent Pinchart



___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 1/5] drm/blend: Add a generic alpha property

[Paul Kocialkowski]

Hi,

On Fri, 2018-03-30 at 13:37 -0700, Eric Anholt wrote:
> Maxime Ripard  writes:
> 
> > Some drivers duplicate the logic to create a property to store a
> > per-plane
> > alpha.
> > 
> > This is especially useful if we ever want to support extra protocols
> > for
> > Wayland like:
> > https://lists.freedesktop.org/archives/wayland-devel/2017-August/034
> > 741.html
> > 
> > Let's create a helper in order to move that to the core.
> > diff --git a/drivers/gpu/drm/drm_blend.c
> > b/drivers/gpu/drm/drm_blend.c
> > index 5a81e1b4c076..05eda2d57c77 100644
> > --- a/drivers/gpu/drm/drm_blend.c
> > +++ b/drivers/gpu/drm/drm_blend.c
> > @@ -88,6 +88,12 @@
> >   * On top of this basic transformation additional properties can be
> > exposed by
> >   * the driver:
> >   *
> > + * alpha:
> > + * Alpha is setup with drm_plane_create_alpha_property().
> > It controls the
> 
> s/setup/set up/
> 
> > + * plane-wide opacity, from transparent (0) to opaque
> > (0x). It can be
> > + * combined with pixel alpha.
> > + * The alpha value is represented as premultiplied alpha.
> 
> I don't think this premultiplied comment makes any sense.  What are
> you saying it's premultiplied with?  Maybe you mean that the output
> pixels will have both their color and alpha channels multiplied by
> this alpha?
> I'd just drop it.

I disagree here: since there are multiple ways to blend the pixel alpha
value and the plane-wide alpha value, I think it's important to clearly
specify which blending equation DRM expects here. Otherwise, the plane-
wide alpha value just doesn't have a specified meaning and driver
implementations can't figure whether the hardware uses the same equation
or not and whether adaptation to this coefficient is needed.

The equations for premultiplied alpha blending are at:
https://en.wikipedia.org/wiki/Alpha_compositing#Alpha_blending

What do you think?

> Reviewed-by: Eric Anholt 
> 
> We should probably do this property for vc4 at some point, too.

-- 
Paul Kocialkowski, Bootlin (formerly Free Electrons)
Embedded Linux and kernel engineering
https://bootlin.com

signature.asc
Description: This is a digitally signed message part
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v4 1/5] drm/blend: Add a generic alpha property

[Paul Kocialkowski]

Hi,

On Tue, 2018-03-13 at 21:54 +0100, Maxime Ripard wrote:
> Some drivers duplicate the logic to create a property to store a per-
> plane alpha.
> 
> This is especially useful if we ever want to support extra protocols
> for
> Wayland like:
> https://lists.freedesktop.org/archives/wayland-devel/2017-August/03474
> 1.html
> 
> Let's create a helper in order to move that to the core.

See one comment below about the alpha blending equation.
Otherwise, this is:

Reviewed-by: Paul Kocialkowski 

> Cc: Laurent Pinchart 
> Reviewed-by: Boris Brezillon 
> Signed-off-by: Maxime Ripard 
> ---
>  drivers/gpu/drm/drm_atomic.c|  4 +++-
>  drivers/gpu/drm/drm_atomic_helper.c |  4 +++-
>  drivers/gpu/drm/drm_blend.c | 38
> ++-
>  include/drm/drm_blend.h |  3 ++-
>  include/drm/drm_plane.h |  6 +-
>  5 files changed, 55 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c
> b/drivers/gpu/drm/drm_atomic.c
> index 34b7d420e555..39204c88d2c5 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -753,6 +753,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->alpha_property) {
> + state->alpha = val;
>   } else if (property == plane->rotation_property) {
>   if (!is_power_of_2(val & DRM_MODE_ROTATE_MASK))
>   return -EINVAL;
> @@ -818,6 +820,8 @@ drm_atomic_plane_get_property(struct drm_plane
> *plane,
>   *val = state->src_w;
>   } else if (property == config->prop_src_h) {
>   *val = state->src_h;
> + } else if (property == plane->alpha_property) {
> + *val = state->alpha;
>   } 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 00c78c1c9681..ac4c3f18a0b1 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3484,6 +3484,10 @@ void drm_atomic_helper_plane_reset(struct
> drm_plane *plane)
>   if (plane->state) {
>   plane->state->plane = plane;
>   plane->state->rotation = DRM_MODE_ROTATE_0;
> +
> + /* Reset the alpha value to fully opaque if it
> matters */
> + if (plane->alpha_property)
> + plane->state->alpha = plane->alpha_property-
> >values[1];
>   }
>  }
>  EXPORT_SYMBOL(drm_atomic_helper_plane_reset);
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index 5a81e1b4c076..05eda2d57c77 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -88,6 +88,12 @@
>   * On top of this basic transformation additional properties can be
> exposed by
>   * the driver:
>   *
> + * alpha:
> + *   Alpha is setup with drm_plane_create_alpha_property(). It
> controls the
> + *   plane-wide opacity, from transparent (0) to opaque
> (0x). It can be
> + *   combined with pixel alpha.
> + *   The alpha value is represented as premultiplied alpha.

As Eric highlighted already, it's hard to grasp what "premultiplied
alpha" is about. From what I can see, the Wayland proposal supports the
following blending equations:
* none: no alpha
* opaque: alpha between 1 (opaque) and 0 (transparent)
* premultiplied: alpha between 1 and 1 - (pixel alpha)
* straight: alpha between (pixel alpha) and 1 - (pixel alpha)

The comment seems to imply that the value should always be specified to
DRM as premultiplied. I think it would be good to explain what this
entails in the comment and in other relevant places (e.g. DRM
documentation).

Also, I'm not sure this is the best fit for all the hardware out there,
so to deal with the possible variety of cases, we could:
* Have a way for the driver to expose what blending equations are
supported (maybe another DRM property?)
* Keep the DRM property implicitly (although with clear documentation
about it) tied to one specific equation (e.g. premultiplied) and have
the drivers to the adaptation on the coefficient if needed to fit what
the hardware needs.

> + *
>   * rotation:
>   *   Rotation is set up with
> drm_plane_create_rotation_property(). It adds a
>   *   rotation and reflection step between the source and
> destination rectangles.
> @@ -106,6 +112,38 @@
>   */
>  
>  /**
> + * drm_plane_create_alpha_property - create a new alpha property
> + * @plane: drm plane
> + *
> + * This function creates a generic, mutable, alpha property and
> enables support
> + * for it in the DRM core. It is attached to @plane.
> + *
> + * The alpha property 

Re: [PATCH v4 1/5] drm/blend: Add a generic alpha property

[Eric Anholt]

Maxime Ripard  writes:

> Some drivers duplicate the logic to create a property to store a per-plane
> alpha.
>
> This is especially useful if we ever want to support extra protocols for
> Wayland like:
> https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741.html
>
> Let's create a helper in order to move that to the core.

> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index 5a81e1b4c076..05eda2d57c77 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -88,6 +88,12 @@
>   * On top of this basic transformation additional properties can be exposed 
> by
>   * the driver:
>   *
> + * alpha:
> + *   Alpha is setup with drm_plane_create_alpha_property(). It controls the

s/setup/set up/

> + *   plane-wide opacity, from transparent (0) to opaque (0x). It can be
> + *   combined with pixel alpha.
> + *   The alpha value is represented as premultiplied alpha.

I don't think this premultiplied comment makes any sense.  What are you
saying it's premultiplied with?  Maybe you mean that the output pixels
will have both their color and alpha channels multiplied by this alpha?
I'd just drop it.

Reviewed-by: Eric Anholt 

We should probably do this property for vc4 at some point, too.


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v4 1/5] drm/blend: Add a generic alpha property

[Maxime Ripard]

Some drivers duplicate the logic to create a property to store a per-plane
alpha.

This is especially useful if we ever want to support extra protocols for
Wayland like:
https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741.html

Let's create a helper in order to move that to the core.

Cc: Laurent Pinchart 
Reviewed-by: Boris Brezillon 
Signed-off-by: Maxime Ripard 
---
 drivers/gpu/drm/drm_atomic.c|  4 +++-
 drivers/gpu/drm/drm_atomic_helper.c |  4 +++-
 drivers/gpu/drm/drm_blend.c | 38 ++-
 include/drm/drm_blend.h |  3 ++-
 include/drm/drm_plane.h |  6 +-
 5 files changed, 55 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 34b7d420e555..39204c88d2c5 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -753,6 +753,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->alpha_property) {
+   state->alpha = val;
} else if (property == plane->rotation_property) {
if (!is_power_of_2(val & DRM_MODE_ROTATE_MASK))
return -EINVAL;
@@ -818,6 +820,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
*val = state->src_w;
} else if (property == config->prop_src_h) {
*val = state->src_h;
+   } else if (property == plane->alpha_property) {
+   *val = state->alpha;
} 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 00c78c1c9681..ac4c3f18a0b1 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3484,6 +3484,10 @@ void drm_atomic_helper_plane_reset(struct drm_plane 
*plane)
if (plane->state) {
plane->state->plane = plane;
plane->state->rotation = DRM_MODE_ROTATE_0;
+
+   /* Reset the alpha value to fully opaque if it matters */
+   if (plane->alpha_property)
+   plane->state->alpha = plane->alpha_property->values[1];
}
 }
 EXPORT_SYMBOL(drm_atomic_helper_plane_reset);
diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
index 5a81e1b4c076..05eda2d57c77 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -88,6 +88,12 @@
  * On top of this basic transformation additional properties can be exposed by
  * the driver:
  *
+ * alpha:
+ * Alpha is setup with drm_plane_create_alpha_property(). It controls the
+ * plane-wide opacity, from transparent (0) to opaque (0x). It can be
+ * combined with pixel alpha.
+ * The alpha value is represented as premultiplied alpha.
+ *
  * rotation:
  * Rotation is set up with drm_plane_create_rotation_property(). It adds a
  * rotation and reflection step between the source and destination 
rectangles.
@@ -106,6 +112,38 @@
  */
 
 /**
+ * drm_plane_create_alpha_property - create a new alpha property
+ * @plane: drm plane
+ *
+ * This function creates a generic, mutable, alpha property and enables support
+ * for it in the DRM core. It is attached to @plane.
+ *
+ * The alpha property will be allowed to be within the bounds of 0
+ * (transparent) to 0x (opaque).
+ *
+ * Returns:
+ * 0 on success, negative error code on failure.
+ */
+int drm_plane_create_alpha_property(struct drm_plane *plane)
+{
+   struct drm_property *prop;
+
+   prop = drm_property_create_range(plane->dev, 0, "alpha",
+0, DRM_BLEND_ALPHA_OPAQUE);
+   if (!prop)
+   return -ENOMEM;
+
+   drm_object_attach_property(>base, prop, DRM_BLEND_ALPHA_OPAQUE);
+   plane->alpha_property = prop;
+
+   if (plane->state)
+   plane->state->alpha = DRM_BLEND_ALPHA_OPAQUE;
+
+   return 0;
+}
+EXPORT_SYMBOL(drm_plane_create_alpha_property);
+
+/**
  * drm_plane_create_rotation_property - create a new rotation property
  * @plane: drm plane
  * @rotation: initial value of the rotation property
diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h
index 17606026590b..330c561c4c11 100644
--- a/include/drm/drm_blend.h
+++ b/include/drm/drm_blend.h
@@ -36,6 +36,9 @@ static inline bool drm_rotation_90_or_270(unsigned int 
rotation)
return rotation & (DRM_MODE_ROTATE_90 | DRM_MODE_ROTATE_270);
 }
 
+#define DRM_BLEND_ALPHA_OPAQUE 0x
+
+int drm_plane_create_alpha_property(struct drm_plane *plane);
 int drm_plane_create_rotation_property(struct drm_plane *plane,