[PATCH v3 0/8] drm/sun4i: Support more planes, zpos and plane-wide alpha

[PATCH v3 0/8] drm/sun4i: Support more planes, zpos and plane-wide alpha

[Maxime Ripard]

Hi,

This serie aims at enhancing the support for our planes in the current drm
driver on the first generation of Allwinner's display engine.

This also introduces a few generic stuff, as well as some conversion for
some other drivers.

This series basically implements three things that look orthogonal, but due
to the way the hardware works are kind of related.

The main feature is that instead of implementing 2 planes per backend, we
are now able to use the planes that are available in hardware. This was
unsupported before because of the way the composition works in the
hardware.

Indeed, the planes are first grouped into 2 pipes that are doing a basic
composition, in case of overlapping planes, it just takes whatever plane
has the highest priority (=> zpos). Then, the alpha blending is done
between the two pipes. This was simplified so far by only using two planes,
one for each pipe, which was allowing us to have an illusion of proper
alpha blending. This is further complicated by the bug/feature that the
lowest plane must not have any alpha at all, otherwise the pixel will turn
black, no matter what the value of alpha is. This basically means that we
can have a plane with alpha only in the second pipe.

However, as we have more and more blocks being worked on, 2 planes are
getting really limited and we need to support all 4 of them.

This is mostly possible by extending our atomic_check and to make sure that
we enforce those constraints, and assign the pipes automatically. This is
done by looking at the number of planes using an alpha component, and we
then end up in various scenarios:
  - 0 plane with alpha
    => we don't care for the pipes at all. All the planes are assigned to
       the first pipe
  - 1 plane with alpha
    => we assign all the planes without alpha below the plane with alpha to
       the first pipe, and then all the remaining planes to the second
       pipe. The plane with alpha will be the lowest plane on that pipe,
       which means that whatever plane is above it will have precedence,
       but the alpha component will remain and will be used on pixels that
       are not overlapping
  - 2-4 planes with alpha
    => we can't operate that way, we just reject the configuration.

In addition to the formats that embed an alpha component, we also add
support for plane-wide alpha property, and in order to tweak the
configuration the way we want to, we also add support for configurable
zpos.

Let me know what you think,
Maxime

Changes from v2:
  - Rebased on current drm-misc-next
  - Removed the patches already applied
  - Split the patch implementing the automatic pipe assignment in two

Changes from v1:
  - Document the behaviour on concurrent usage of the alpha property and an
    alpha component in the format
  - Allowed for higher alpha values
  - Moved the alpha value from a helper to the struct drm_format_info
  - Collected tags
  - Rebased on current drm-misc-next

Maxime Ripard (8):
  drm/blend: Add a generic alpha property
  drm/atmel-hclcdc: Convert to the new generic alpha property
  drm/rcar-du: Convert to the new generic alpha property
  drm/sun4i: backend: Assign the pipes automatically
  drm/sun4i: Remove the plane description structure
  drm/sun4i: backend: Make zpos configurable
  drm/sun4i: Add support for plane alpha
  drm/sun4i: backend: Remove ARGB spoofing

 Documentation/gpu/kms-properties.csv            |  2 +-
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h    | 13 +---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 89 ++----------------
 drivers/gpu/drm/drm_atomic.c                    |  4 +-
 drivers/gpu/drm/drm_atomic_helper.c             |  4 +-
 drivers/gpu/drm/drm_blend.c                     | 32 ++++++-
 drivers/gpu/drm/rcar-du/rcar_du_drv.h           |  1 +-
 drivers/gpu/drm/rcar-du/rcar_du_kms.c           |  5 +-
 drivers/gpu/drm/rcar-du/rcar_du_plane.c         | 15 +--
 drivers/gpu/drm/rcar-du/rcar_du_plane.h         |  2 +-
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c           | 42 +--------
 drivers/gpu/drm/rcar-du/rcar_du_vsp.h           |  2 +-
 drivers/gpu/drm/sun4i/sun4i_backend.c           | 64 ++++++++++---
 drivers/gpu/drm/sun4i/sun4i_backend.h           |  3 +-
 drivers/gpu/drm/sun4i/sun4i_layer.c             | 57 ++----------
 drivers/gpu/drm/sun4i/sun4i_layer.h             |  1 +-
 include/drm/drm_blend.h                         |  1 +-
 include/drm/drm_plane.h                         |  6 +-
 18 files changed, 140 insertions(+), 203 deletions(-)

base-commit: 5db520e36bca3e1e1575ab14ca7dfb23b3d68407
-- 
git-series 0.9.1

[PATCH v3 1/8] 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 <laurent.pinchart@ideasonboard.com>
Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 Documentation/gpu/kms-properties.csv |  2 +-
 drivers/gpu/drm/drm_atomic.c         |  4 ++++-
 drivers/gpu/drm/drm_atomic_helper.c  |  4 ++++-
 drivers/gpu/drm/drm_blend.c          | 32 +++++++++++++++++++++++++++++-
 include/drm/drm_blend.h              |  1 +-
 include/drm/drm_plane.h              |  6 +++++-
 6 files changed, 48 insertions(+), 1 deletion(-)

diff --git a/Documentation/gpu/kms-properties.csv b/Documentation/gpu/kms-properties.csv
index 927b65e14219..25ad3503d663 100644
--- a/Documentation/gpu/kms-properties.csv
+++ b/Documentation/gpu/kms-properties.csv
@@ -99,5 +99,5 @@ radeon,DVI-I,?coherent?,RANGE,"Min=0, Max=1",Connector,TBD
 ,,"""underscan vborder""",RANGE,"Min=0, Max=128",Connector,TBD
 ,Audio,?audio?,ENUM,"{ ""off"", ""on"", ""auto"" }",Connector,TBD
 ,FMT Dithering,?dither?,ENUM,"{ ""off"", ""on"" }",Connector,TBD
-rcar-du,Generic,"""alpha""",RANGE,"Min=0, Max=255",Plane,TBD
+,,"""alpha""",RANGE,"Min=0, Max=Driver dependant",Plane,Opacity of the plane from transparent (0) to fully opaque (MAX). If this property is set to a value different than max, and that the pixel will define an alpha component, the property will have precendance and the pixel value will be ignored. The alpha value is represented as straight alpha, ie the colors haven't been pre-adjusted for their opacity by multiplication. Therefore, the equation to get a color value for one pixel, assuming two planes A and B, will be (color_a * alpha_a + color_b * alpha_b * (MAX - alpha_a) / MAX) / (alpha_a + alpha_b * (MAX - alpha_a) / MAX)
 ,,"""colorkey""",RANGE,"Min=0, Max=0x01ffffff",Plane,TBD
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 7d9ad20040a1..3defc56a1ef2 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;
@@ -814,6 +816,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 ae3cbfe9e01c..2b88f593aab4 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3482,6 +3482,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 4c62dff14893..a5dea7cbed2c 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -104,6 +104,38 @@
  */
 
 /**
+ * drm_plane_create_alpha_property - create a new alpha property
+ * @plane: drm plane
+ * @max_alpha: maximum value of alpha
+ *
+ * This function initializes a generic, mutable, alpha property and
+ * enables support for it in the DRM core.
+ *
+ * The alpha property will be allowed to be within the bounds of 0
+ * (transparent) to @max_alpha (opaque)
+ *
+ * Returns:
+ * 0 on success, negative error code on failure.
+ */
+int drm_plane_create_alpha_property(struct drm_plane *plane, u16 max_alpha)
+{
+	struct drm_property *prop;
+
+	prop = drm_property_create_range(plane->dev, 0, "alpha", 0, max_alpha);
+	if (!prop)
+		return -ENOMEM;
+
+	drm_object_attach_property(&plane->base, prop, max_alpha);
+	plane->alpha_property = prop;
+
+	if (plane->state)
+		plane->state->alpha = max_alpha;
+
+	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..e5affba6ebde 100644
--- a/include/drm/drm_blend.h
+++ b/include/drm/drm_blend.h
@@ -36,6 +36,7 @@ static inline bool drm_rotation_90_or_270(unsigned int rotation)
 	return rotation & (DRM_MODE_ROTATE_90 | DRM_MODE_ROTATE_270);
 }
 
+int drm_plane_create_alpha_property(struct drm_plane *plane, u16 alpha);
 int drm_plane_create_rotation_property(struct drm_plane *plane,
 				       unsigned int rotation,
 				       unsigned int supported_rotations);
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
index 8185e3468a23..5a6f29524f12 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -42,6 +42,7 @@ struct drm_modeset_acquire_ctx;
  *	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)
+ * @alpha: opacity of the plane
  * @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
@@ -105,6 +106,9 @@ struct drm_plane_state {
 	uint32_t src_x, src_y;
 	uint32_t src_h, src_w;
 
+	/* Plane opacity */
+	u8 alpha;
+
 	/* Plane rotation */
 	unsigned int rotation;
 
@@ -481,6 +485,7 @@ enum drm_plane_type {
  * @funcs: helper functions
  * @properties: property tracking for this plane
  * @type: type of plane (overlay, primary, cursor)
+ * @alpha_property: alpha property for this plane
  * @zpos_property: zpos property for this plane
  * @rotation_property: rotation property for this plane
  * @helper_private: mid-layer private data
@@ -556,6 +561,7 @@ struct drm_plane {
 	 */
 	struct drm_plane_state *state;
 
+	struct drm_property *alpha_property;
 	struct drm_property *zpos_property;
 	struct drm_property *rotation_property;
 };
-- 
git-series 0.9.1

[PATCH v3 1/8] drm/blend: Add a generic alpha property

[Ville Syrjälä]

On Fri, Feb 16, 2018 at 06:39:29PM +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/034741.html
> 
> Let's create a helper in order to move that to the core.
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  Documentation/gpu/kms-properties.csv |  2 +-
>  drivers/gpu/drm/drm_atomic.c         |  4 ++++-
>  drivers/gpu/drm/drm_atomic_helper.c  |  4 ++++-
>  drivers/gpu/drm/drm_blend.c          | 32 +++++++++++++++++++++++++++++-
>  include/drm/drm_blend.h              |  1 +-
>  include/drm/drm_plane.h              |  6 +++++-
>  6 files changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/gpu/kms-properties.csv b/Documentation/gpu/kms-properties.csv
> index 927b65e14219..25ad3503d663 100644
> --- a/Documentation/gpu/kms-properties.csv
> +++ b/Documentation/gpu/kms-properties.csv
> @@ -99,5 +99,5 @@ radeon,DVI-I,?coherent?,RANGE,"Min=0, Max=1",Connector,TBD
>  ,,"""underscan vborder""",RANGE,"Min=0, Max=128",Connector,TBD
>  ,Audio,?audio?,ENUM,"{ ""off"", ""on"", ""auto"" }",Connector,TBD
>  ,FMT Dithering,?dither?,ENUM,"{ ""off"", ""on"" }",Connector,TBD
> -rcar-du,Generic,"""alpha""",RANGE,"Min=0, Max=255",Plane,TBD
> +,,"""alpha""",RANGE,"Min=0, Max=Driver dependant",Plane,Opacity of the plane from transparent (0) to fully opaque (MAX). If this property is set to a value different than max, and that the pixel will define an alpha component, the property will have precendance and the pixel value will be ignored.

Those semantics don't seem particularly good to me. I think we would want the
per-pixel alpha and global alpha both to be effecive at the same time. You can
always decide to ignore the per-pixel alpha by using a pixel format without
alpha.

Also, where's the userspace that wants this feature?

<snip>
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 8185e3468a23..5a6f29524f12 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -42,6 +42,7 @@ struct drm_modeset_acquire_ctx;
>   *	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)
> + * @alpha: opacity of the plane
>   * @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
> @@ -105,6 +106,9 @@ struct drm_plane_state {
>  	uint32_t src_x, src_y;
>  	uint32_t src_h, src_w;
>  
> +	/* Plane opacity */
> +	u8 alpha;

We may want to make that u16. The general we expect 16bpc for most color
related things, but since this is a range prop I suppose we should just
expose the actual hardware range. But making it u16 might avoid some head
scratching for the first person to have hardware with higher precision.
Either that or we should make the prop creation fail if the driver asks
for more bits than we have in the state.

Oh, and you should plug this into the state dumper as well.

> +
>  	/* Plane rotation */
>  	unsigned int rotation;
>  
> @@ -481,6 +485,7 @@ enum drm_plane_type {
>   * @funcs: helper functions
>   * @properties: property tracking for this plane
>   * @type: type of plane (overlay, primary, cursor)
> + * @alpha_property: alpha property for this plane
>   * @zpos_property: zpos property for this plane
>   * @rotation_property: rotation property for this plane
>   * @helper_private: mid-layer private data
> @@ -556,6 +561,7 @@ struct drm_plane {
>  	 */
>  	struct drm_plane_state *state;
>  
> +	struct drm_property *alpha_property;
>  	struct drm_property *zpos_property;
>  	struct drm_property *rotation_property;
>  };
> -- 
> git-series 0.9.1
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Ville Syrj?l?
Intel OTC

[PATCH v3 1/8] drm/blend: Add a generic alpha property

[Laurent Pinchart]

Hi Ville,

On Friday, 16 February 2018 20:20:41 EET Ville Syrj?l? wrote:
> On Fri, Feb 16, 2018 at 06:39:29PM +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/034741.ht
> > ml
> > 
> > Let's create a helper in order to move that to the core.
> > 
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > ---
> > 
> >  Documentation/gpu/kms-properties.csv |  2 +-
> >  drivers/gpu/drm/drm_atomic.c         |  4 ++++-
> >  drivers/gpu/drm/drm_atomic_helper.c  |  4 ++++-
> >  drivers/gpu/drm/drm_blend.c          | 32 +++++++++++++++++++++++++++++-
> >  include/drm/drm_blend.h              |  1 +-
> >  include/drm/drm_plane.h              |  6 +++++-
> >  6 files changed, 48 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/gpu/kms-properties.csv
> > b/Documentation/gpu/kms-properties.csv index 927b65e14219..25ad3503d663
> > 100644
> > --- a/Documentation/gpu/kms-properties.csv
> > +++ b/Documentation/gpu/kms-properties.csv
> > @@ -99,5 +99,5 @@ radeon,DVI-I,?coherent?,RANGE,"Min=0,
> > Max=1",Connector,TBD> 
> >  ,,"""underscan vborder""",RANGE,"Min=0, Max=128",Connector,TBD
> >  ,Audio,?audio?,ENUM,"{ ""off"", ""on"", ""auto"" }",Connector,TBD
> >  ,FMT Dithering,?dither?,ENUM,"{ ""off"", ""on"" }",Connector,TBD
> > 
> > -rcar-du,Generic,"""alpha""",RANGE,"Min=0, Max=255",Plane,TBD
> > +,,"""alpha""",RANGE,"Min=0, Max=Driver dependant",Plane,Opacity of the
> > plane from transparent (0) to fully opaque (MAX). If this property is set
> > to a value different than max, and that the pixel will define an alpha
> > component, the property will have precendance and the pixel value will be
> > ignored.
> 
> Those semantics don't seem particularly good to me. I think we would want
> the per-pixel alpha and global alpha both to be effecive at the same time.
> You can always decide to ignore the per-pixel alpha by using a pixel format
> without alpha.

That makes sense to me. However, it also brings a new question: how should a 
driver that supports either global alpha or pixel alpha but not both signal 
that to userspace, and how should it reacts when userspace selects a format 
with an alpha channel and set a global alpha value other than fully opaque ? 
To make things more complex, note that some drivers support combining global 
alpha and pixel alpha only for a subset of the formats with an alpha channel 
(for instance for ARGB 1555 formats, but not for ARGB 8888 formats).

> Also, where's the userspace that wants this feature?
> 
> <snip>
> 
> > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > index 8185e3468a23..5a6f29524f12 100644
> > --- a/include/drm/drm_plane.h
> > +++ b/include/drm/drm_plane.h
> > @@ -42,6 +42,7 @@ struct drm_modeset_acquire_ctx;
> >   *	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)
> > + * @alpha: opacity of the plane
> >   * @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
> > @@ -105,6 +106,9 @@ struct drm_plane_state {
> >  	uint32_t src_x, src_y;
> >  	uint32_t src_h, src_w;
> > 
> > +	/* Plane opacity */
> > +	u8 alpha;
> 
> We may want to make that u16. The general we expect 16bpc for most color
> related things, but since this is a range prop I suppose we should just
> expose the actual hardware range. But making it u16 might avoid some head
> scratching for the first person to have hardware with higher precision.
> Either that or we should make the prop creation fail if the driver asks
> for more bits than we have in the state.

I'm tempted to go one step further and always make the alpha property 16-bits 
wide for new users (we can't do so for existing users as it could break 
userspace), and let drivers convert that internally to the range they need. 
There could however be drawbacks I don't foresee.

> Oh, and you should plug this into the state dumper as well.
> 
> > +
> > 
> >  	/* Plane rotation */
> >  	unsigned int rotation;
> > @@ -481,6 +485,7 @@ enum drm_plane_type {
> >   * @funcs: helper functions
> >   * @properties: property tracking for this plane
> >   * @type: type of plane (overlay, primary, cursor)
> > + * @alpha_property: alpha property for this plane
> >   * @zpos_property: zpos property for this plane
> >   * @rotation_property: rotation property for this plane
> >   * @helper_private: mid-layer private data
> > @@ -556,6 +561,7 @@ struct drm_plane {
> >  	 */
> >  	
> >  	struct drm_plane_state *state;
> > +	struct drm_property *alpha_property;
> >  	struct drm_property *zpos_property;
> >  	struct drm_property *rotation_property;
> >  };

-- 
Regards,

Laurent Pinchart

[PATCH v3 1/8] drm/blend: Add a generic alpha property

[Daniel Vetter]

On Mon, Feb 19, 2018 at 9:19 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Ville,
>
> On Friday, 16 February 2018 20:20:41 EET Ville Syrj?l? wrote:
>> On Fri, Feb 16, 2018 at 06:39:29PM +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/034741.ht
>> > ml
>> >
>> > Let's create a helper in order to move that to the core.
>> >
>> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> > Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
>> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>> > ---
>> >
>> >  Documentation/gpu/kms-properties.csv |  2 +-
>> >  drivers/gpu/drm/drm_atomic.c         |  4 ++++-
>> >  drivers/gpu/drm/drm_atomic_helper.c  |  4 ++++-
>> >  drivers/gpu/drm/drm_blend.c          | 32 +++++++++++++++++++++++++++++-
>> >  include/drm/drm_blend.h              |  1 +-
>> >  include/drm/drm_plane.h              |  6 +++++-
>> >  6 files changed, 48 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/Documentation/gpu/kms-properties.csv
>> > b/Documentation/gpu/kms-properties.csv index 927b65e14219..25ad3503d663
>> > 100644
>> > --- a/Documentation/gpu/kms-properties.csv
>> > +++ b/Documentation/gpu/kms-properties.csv
>> > @@ -99,5 +99,5 @@ radeon,DVI-I,?coherent?,RANGE,"Min=0,
>> > Max=1",Connector,TBD>
>> >  ,,"""underscan vborder""",RANGE,"Min=0, Max=128",Connector,TBD
>> >  ,Audio,?audio?,ENUM,"{ ""off"", ""on"", ""auto"" }",Connector,TBD
>> >  ,FMT Dithering,?dither?,ENUM,"{ ""off"", ""on"" }",Connector,TBD
>> >
>> > -rcar-du,Generic,"""alpha""",RANGE,"Min=0, Max=255",Plane,TBD
>> > +,,"""alpha""",RANGE,"Min=0, Max=Driver dependant",Plane,Opacity of the
>> > plane from transparent (0) to fully opaque (MAX). If this property is set
>> > to a value different than max, and that the pixel will define an alpha
>> > component, the property will have precendance and the pixel value will be
>> > ignored.

Please don't document new properties in that csv file, it's an
unreadable mess. Instead follow how we document standardized
properties nowadays in full-blown sections. For plane blending we
have:

https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-properties

>> Those semantics don't seem particularly good to me. I think we would want
>> the per-pixel alpha and global alpha both to be effecive at the same time.
>> You can always decide to ignore the per-pixel alpha by using a pixel format
>> without alpha.
>
> That makes sense to me. However, it also brings a new question: how should a
> driver that supports either global alpha or pixel alpha but not both signal
> that to userspace, and how should it reacts when userspace selects a format
> with an alpha channel and set a global alpha value other than fully opaque ?
> To make things more complex, note that some drivers support combining global
> alpha and pixel alpha only for a subset of the formats with an alpha channel
> (for instance for ARGB 1555 formats, but not for ARGB 8888 formats).

atomic_check can reject unsupported configs. Userspace needs to fall
back somehow (either switch to xrgb or make alpha fully opaque or just
give up on that plane). We have a lot of such corner-cases we don't
tell userspace about explicitly at all.

>> Also, where's the userspace that wants this feature?
>>
>> <snip>
>>
>> > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>> > index 8185e3468a23..5a6f29524f12 100644
>> > --- a/include/drm/drm_plane.h
>> > +++ b/include/drm/drm_plane.h
>> > @@ -42,6 +42,7 @@ struct drm_modeset_acquire_ctx;
>> >   * 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)
>> > + * @alpha: opacity of the plane
>> >   * @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
>> > @@ -105,6 +106,9 @@ struct drm_plane_state {
>> >     uint32_t src_x, src_y;
>> >     uint32_t src_h, src_w;
>> >
>> > +   /* Plane opacity */
>> > +   u8 alpha;
>>
>> We may want to make that u16. The general we expect 16bpc for most color
>> related things, but since this is a range prop I suppose we should just
>> expose the actual hardware range. But making it u16 might avoid some head
>> scratching for the first person to have hardware with higher precision.
>> Either that or we should make the prop creation fail if the driver asks
>> for more bits than we have in the state.
>
> I'm tempted to go one step further and always make the alpha property 16-bits
> wide for new users (we can't do so for existing users as it could break
> userspace), and let drivers convert that internally to the range they need.
> There could however be drawbacks I don't foresee.

I think scaling the range to match the hw is the most sensible (yes
I'm flip-flopping around here). And once someone needs more than u8,
we can extend the internal representation easily. The external
representation in the property is an u64, that /should/ be enough for
the next few years :-)
-Daniel

>> Oh, and you should plug this into the state dumper as well.
>>
>> > +
>> >
>> >     /* Plane rotation */
>> >     unsigned int rotation;
>> > @@ -481,6 +485,7 @@ enum drm_plane_type {
>> >   * @funcs: helper functions
>> >   * @properties: property tracking for this plane
>> >   * @type: type of plane (overlay, primary, cursor)
>> > + * @alpha_property: alpha property for this plane
>> >   * @zpos_property: zpos property for this plane
>> >   * @rotation_property: rotation property for this plane
>> >   * @helper_private: mid-layer private data
>> > @@ -556,6 +561,7 @@ struct drm_plane {
>> >      */
>> >
>> >     struct drm_plane_state *state;
>> > +   struct drm_property *alpha_property;
>> >     struct drm_property *zpos_property;
>> >     struct drm_property *rotation_property;
>> >  };
>
> --
> Regards,
>
> Laurent Pinchart
>
> _______________________________________________
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH v3 1/8] drm/blend: Add a generic alpha property

[Maxime Ripard]

On Mon, Feb 19, 2018 at 10:58:40PM +0100, Daniel Vetter wrote:
> On Mon, Feb 19, 2018 at 9:19 PM, Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> > Hi Ville,
> >
> > On Friday, 16 February 2018 20:20:41 EET Ville Syrj?l? wrote:
> >> On Fri, Feb 16, 2018 at 06:39:29PM +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/034741.ht
> >> > ml
> >> >
> >> > Let's create a helper in order to move that to the core.
> >> >
> >> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> > Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
> >> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> >> > ---
> >> >
> >> >  Documentation/gpu/kms-properties.csv |  2 +-
> >> >  drivers/gpu/drm/drm_atomic.c         |  4 ++++-
> >> >  drivers/gpu/drm/drm_atomic_helper.c  |  4 ++++-
> >> >  drivers/gpu/drm/drm_blend.c          | 32 +++++++++++++++++++++++++++++-
> >> >  include/drm/drm_blend.h              |  1 +-
> >> >  include/drm/drm_plane.h              |  6 +++++-
> >> >  6 files changed, 48 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/Documentation/gpu/kms-properties.csv
> >> > b/Documentation/gpu/kms-properties.csv index 927b65e14219..25ad3503d663
> >> > 100644
> >> > --- a/Documentation/gpu/kms-properties.csv
> >> > +++ b/Documentation/gpu/kms-properties.csv
> >> > @@ -99,5 +99,5 @@ radeon,DVI-I,?coherent?,RANGE,"Min=0,
> >> > Max=1",Connector,TBD>
> >> >  ,,"""underscan vborder""",RANGE,"Min=0, Max=128",Connector,TBD
> >> >  ,Audio,?audio?,ENUM,"{ ""off"", ""on"", ""auto"" }",Connector,TBD
> >> >  ,FMT Dithering,?dither?,ENUM,"{ ""off"", ""on"" }",Connector,TBD
> >> >
> >> > -rcar-du,Generic,"""alpha""",RANGE,"Min=0, Max=255",Plane,TBD
> >> > +,,"""alpha""",RANGE,"Min=0, Max=Driver dependant",Plane,Opacity of the
> >> > plane from transparent (0) to fully opaque (MAX). If this property is set
> >> > to a value different than max, and that the pixel will define an alpha
> >> > component, the property will have precendance and the pixel value will be
> >> > ignored.
> 
> Please don't document new properties in that csv file, it's an
> unreadable mess. Instead follow how we document standardized
> properties nowadays in full-blown sections. For plane blending we
> have:
> 
> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-properties

Ack

> >> > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> >> > index 8185e3468a23..5a6f29524f12 100644
> >> > --- a/include/drm/drm_plane.h
> >> > +++ b/include/drm/drm_plane.h
> >> > @@ -42,6 +42,7 @@ struct drm_modeset_acquire_ctx;
> >> >   * 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)
> >> > + * @alpha: opacity of the plane
> >> >   * @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
> >> > @@ -105,6 +106,9 @@ struct drm_plane_state {
> >> >     uint32_t src_x, src_y;
> >> >     uint32_t src_h, src_w;
> >> >
> >> > +   /* Plane opacity */
> >> > +   u8 alpha;
> >>
> >> We may want to make that u16. The general we expect 16bpc for most color
> >> related things, but since this is a range prop I suppose we should just
> >> expose the actual hardware range. But making it u16 might avoid some head
> >> scratching for the first person to have hardware with higher precision.
> >> Either that or we should make the prop creation fail if the driver asks
> >> for more bits than we have in the state.
> >
> > I'm tempted to go one step further and always make the alpha property 16-bits
> > wide for new users (we can't do so for existing users as it could break
> > userspace), and let drivers convert that internally to the range they need.
> > There could however be drawbacks I don't foresee.
> 
> I think scaling the range to match the hw is the most sensible (yes
> I'm flip-flopping around here). And once someone needs more than u8,
> we can extend the internal representation easily. The external
> representation in the property is an u64, that /should/ be enough for
> the next few years :-)

Just to make sure we're on the same page, you want to keep the u8, and
if the hardware uses say an u16, the driver for that hardware will do
the upscaling?

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180221/ec450ac6/attachment.sig>

[PATCH v3 1/8] drm/blend: Add a generic alpha property

[Daniel Vetter]

On Wed, Feb 21, 2018 at 02:07:57PM +0100, Maxime Ripard wrote:
> On Mon, Feb 19, 2018 at 10:58:40PM +0100, Daniel Vetter wrote:
> > On Mon, Feb 19, 2018 at 9:19 PM, Laurent Pinchart
> > <laurent.pinchart@ideasonboard.com> wrote:
> > > Hi Ville,
> > >
> > > On Friday, 16 February 2018 20:20:41 EET Ville Syrj?l? wrote:
> > >> On Fri, Feb 16, 2018 at 06:39:29PM +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/034741.ht
> > >> > ml
> > >> >
> > >> > Let's create a helper in order to move that to the core.
> > >> >
> > >> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >> > Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > >> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > >> > ---
> > >> >
> > >> >  Documentation/gpu/kms-properties.csv |  2 +-
> > >> >  drivers/gpu/drm/drm_atomic.c         |  4 ++++-
> > >> >  drivers/gpu/drm/drm_atomic_helper.c  |  4 ++++-
> > >> >  drivers/gpu/drm/drm_blend.c          | 32 +++++++++++++++++++++++++++++-
> > >> >  include/drm/drm_blend.h              |  1 +-
> > >> >  include/drm/drm_plane.h              |  6 +++++-
> > >> >  6 files changed, 48 insertions(+), 1 deletion(-)
> > >> >
> > >> > diff --git a/Documentation/gpu/kms-properties.csv
> > >> > b/Documentation/gpu/kms-properties.csv index 927b65e14219..25ad3503d663
> > >> > 100644
> > >> > --- a/Documentation/gpu/kms-properties.csv
> > >> > +++ b/Documentation/gpu/kms-properties.csv
> > >> > @@ -99,5 +99,5 @@ radeon,DVI-I,?coherent?,RANGE,"Min=0,
> > >> > Max=1",Connector,TBD>
> > >> >  ,,"""underscan vborder""",RANGE,"Min=0, Max=128",Connector,TBD
> > >> >  ,Audio,?audio?,ENUM,"{ ""off"", ""on"", ""auto"" }",Connector,TBD
> > >> >  ,FMT Dithering,?dither?,ENUM,"{ ""off"", ""on"" }",Connector,TBD
> > >> >
> > >> > -rcar-du,Generic,"""alpha""",RANGE,"Min=0, Max=255",Plane,TBD
> > >> > +,,"""alpha""",RANGE,"Min=0, Max=Driver dependant",Plane,Opacity of the
> > >> > plane from transparent (0) to fully opaque (MAX). If this property is set
> > >> > to a value different than max, and that the pixel will define an alpha
> > >> > component, the property will have precendance and the pixel value will be
> > >> > ignored.
> > 
> > Please don't document new properties in that csv file, it's an
> > unreadable mess. Instead follow how we document standardized
> > properties nowadays in full-blown sections. For plane blending we
> > have:
> > 
> > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-properties
> 
> Ack
> 
> > >> > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > >> > index 8185e3468a23..5a6f29524f12 100644
> > >> > --- a/include/drm/drm_plane.h
> > >> > +++ b/include/drm/drm_plane.h
> > >> > @@ -42,6 +42,7 @@ struct drm_modeset_acquire_ctx;
> > >> >   * 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)
> > >> > + * @alpha: opacity of the plane
> > >> >   * @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
> > >> > @@ -105,6 +106,9 @@ struct drm_plane_state {
> > >> >     uint32_t src_x, src_y;
> > >> >     uint32_t src_h, src_w;
> > >> >
> > >> > +   /* Plane opacity */
> > >> > +   u8 alpha;
> > >>
> > >> We may want to make that u16. The general we expect 16bpc for most color
> > >> related things, but since this is a range prop I suppose we should just
> > >> expose the actual hardware range. But making it u16 might avoid some head
> > >> scratching for the first person to have hardware with higher precision.
> > >> Either that or we should make the prop creation fail if the driver asks
> > >> for more bits than we have in the state.
> > >
> > > I'm tempted to go one step further and always make the alpha property 16-bits
> > > wide for new users (we can't do so for existing users as it could break
> > > userspace), and let drivers convert that internally to the range they need.
> > > There could however be drawbacks I don't foresee.
> > 
> > I think scaling the range to match the hw is the most sensible (yes
> > I'm flip-flopping around here). And once someone needs more than u8,
> > we can extend the internal representation easily. The external
> > representation in the property is an u64, that /should/ be enough for
> > the next few years :-)
> 
> Just to make sure we're on the same page, you want to keep the u8, and
> if the hardware uses say an u16, the driver for that hardware will do
> the upscaling?

The idea is that we'd set the u16 limit in the property and so inform
userspace that a different range applies. But that's probably going to be
ignored.

Could we do the property itself as u16 range, and (for now, only
internally in drm in drm_plane_state) throw the lower u8 bits away? Or
just let drivers do this.

Sorry that I'm flip-flopping around on this, but we just have an ongoing
discussion about a range/size mixup in the CTM uapi, I think assuming that
all userspace will correctly scale is not realistic. So larger scale in
the uapi (but maybe not internally) from the start seems like a good idea.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH v3 1/8] drm/blend: Add a generic alpha property

[Laurent Pinchart]

Hi Daniel,

On Monday, 5 March 2018 10:58:41 EET Daniel Vetter wrote:
> On Wed, Feb 21, 2018 at 02:07:57PM +0100, Maxime Ripard wrote:
> > On Mon, Feb 19, 2018 at 10:58:40PM +0100, Daniel Vetter wrote:
> >> On Mon, Feb 19, 2018 at 9:19 PM, Laurent Pinchart wrote:
> >>> On Friday, 16 February 2018 20:20:41 EET Ville Syrj?l? wrote:
> >>>> On Fri, Feb 16, 2018 at 06:39:29PM +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/03
> >>>>> 4741.html
> >>>>> 
> >>>>> Let's create a helper in order to move that to the core.
> >>>>> 
> >>>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>>> Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
> >>>>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> >>>>> ---
> >>>>> 
> >>>>>  Documentation/gpu/kms-properties.csv |  2 +-
> >>>>>  drivers/gpu/drm/drm_atomic.c         |  4 ++++-
> >>>>>  drivers/gpu/drm/drm_atomic_helper.c  |  4 ++++-
> >>>>>  drivers/gpu/drm/drm_blend.c          | 32 +++++++++++++++++++++++-
> >>>>>  include/drm/drm_blend.h              |  1 +-
> >>>>>  include/drm/drm_plane.h              |  6 +++++-
> >>>>>  6 files changed, 48 insertions(+), 1 deletion(-)
> >>>>> 
> >>>>> diff --git a/Documentation/gpu/kms-properties.csv
> >>>>> b/Documentation/gpu/kms-properties.csv index
> >>>>> 927b65e14219..25ad3503d663
> >>>>> 100644
> >>>>> --- a/Documentation/gpu/kms-properties.csv
> >>>>> +++ b/Documentation/gpu/kms-properties.csv
> >>>>> @@ -99,5 +99,5 @@ radeon,DVI-I,?coherent?,RANGE,"Min=0,
> >>>>> Max=1",Connector,TBD>
> >>>>> 
> >>>>>  ,,"""underscan vborder""",RANGE,"Min=0, Max=128",Connector,TBD
> >>>>>  ,Audio,?audio?,ENUM,"{ ""off"", ""on"", ""auto"" }",Connector,TBD
> >>>>>  ,FMT Dithering,?dither?,ENUM,"{ ""off"", ""on"" }",Connector,TBD
> >>>>> 
> >>>>> -rcar-du,Generic,"""alpha""",RANGE,"Min=0, Max=255",Plane,TBD
> >>>>> +,,"""alpha""",RANGE,"Min=0, Max=Driver dependant",Plane,Opacity of
> >>>>> the plane from transparent (0) to fully opaque (MAX). If this
> >>>>> property is set to a value different than max, and that the pixel
> >>>>> will define an alpha component, the property will have precendance
> >>>>> and the pixel value will be ignored.
> >> 
> >> Please don't document new properties in that csv file, it's an
> >> unreadable mess. Instead follow how we document standardized
> >> properties nowadays in full-blown sections. For plane blending we
> >> have:
> >> 
> >> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-> >> properties
> > 
> > Ack
> > 
> >>>>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> >>>>> index 8185e3468a23..5a6f29524f12 100644
> >>>>> --- a/include/drm/drm_plane.h
> >>>>> +++ b/include/drm/drm_plane.h
> >>>>> @@ -42,6 +42,7 @@ struct drm_modeset_acquire_ctx;
> >>>>>   * 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)
> >>>>> + * @alpha: opacity of the plane
> >>>>>   * @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
> >>>>> @@ -105,6 +106,9 @@ struct drm_plane_state {
> >>>>>     uint32_t src_x, src_y;
> >>>>>     uint32_t src_h, src_w;
> >>>>> 
> >>>>> +   /* Plane opacity */
> >>>>> +   u8 alpha;
> >>>> 
> >>>> We may want to make that u16. The general we expect 16bpc for most
> >>>> color related things, but since this is a range prop I suppose we
> >>>> should just expose the actual hardware range. But making it u16 might
> >>>> avoid some head scratching for the first person to have hardware with
> >>>> higher precision. Either that or we should make the prop creation
> >>>> fail if the driver asks for more bits than we have in the state.
> >>> 
> >>> I'm tempted to go one step further and always make the alpha property
> >>> 16-bits wide for new users (we can't do so for existing users as it
> >>> could break userspace), and let drivers convert that internally to
> >>> the range they need. There could however be drawbacks I don't
> >>> foresee.
> >> 
> >> I think scaling the range to match the hw is the most sensible (yes
> >> I'm flip-flopping around here). And once someone needs more than u8,
> >> we can extend the internal representation easily. The external
> >> representation in the property is an u64, that /should/ be enough for
> >> the next few years :-)
> > 
> > Just to make sure we're on the same page, you want to keep the u8, and
> > if the hardware uses say an u16, the driver for that hardware will do
> > the upscaling?
> 
> The idea is that we'd set the u16 limit in the property and so inform
> userspace that a different range applies. But that's probably going to be
> ignored.
> 
> Could we do the property itself as u16 range, and (for now, only
> internally in drm in drm_plane_state) throw the lower u8 bits away? Or
> just let drivers do this.

I'm fine with this except for the drivers that currently implement the alpha 
property with a different range. The rcar-du driver for instances has the 
alpha range set to 0x00 to 0xff, so we can't change it without risk of 
breaking userspace. I don't know whether there's any userspace using the 
property, and whether that userspace has any hardcoded assumption.

> Sorry that I'm flip-flopping around on this, but we just have an ongoing
> discussion about a range/size mixup in the CTM uapi, I think assuming that
> all userspace will correctly scale is not realistic. So larger scale in
> the uapi (but maybe not internally) from the start seems like a good idea.

Can we make the range randomly chosen at every boot then ? :-) That would 
force userspace to be generic.

-- 
Regards,

Laurent Pinchart

[PATCH v3 1/8] drm/blend: Add a generic alpha property

[Daniel Vetter]

On Mon, Mar 05, 2018 at 12:08:12PM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Monday, 5 March 2018 10:58:41 EET Daniel Vetter wrote:
> > On Wed, Feb 21, 2018 at 02:07:57PM +0100, Maxime Ripard wrote:
> > > On Mon, Feb 19, 2018 at 10:58:40PM +0100, Daniel Vetter wrote:
> > >> On Mon, Feb 19, 2018 at 9:19 PM, Laurent Pinchart wrote:
> > >>> On Friday, 16 February 2018 20:20:41 EET Ville Syrj?l? wrote:
> > >>>> On Fri, Feb 16, 2018 at 06:39:29PM +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/03
> > >>>>> 4741.html
> > >>>>> 
> > >>>>> Let's create a helper in order to move that to the core.
> > >>>>> 
> > >>>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >>>>> Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > >>>>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > >>>>> ---
> > >>>>> 
> > >>>>>  Documentation/gpu/kms-properties.csv |  2 +-
> > >>>>>  drivers/gpu/drm/drm_atomic.c         |  4 ++++-
> > >>>>>  drivers/gpu/drm/drm_atomic_helper.c  |  4 ++++-
> > >>>>>  drivers/gpu/drm/drm_blend.c          | 32 +++++++++++++++++++++++-
> > >>>>>  include/drm/drm_blend.h              |  1 +-
> > >>>>>  include/drm/drm_plane.h              |  6 +++++-
> > >>>>>  6 files changed, 48 insertions(+), 1 deletion(-)
> > >>>>> 
> > >>>>> diff --git a/Documentation/gpu/kms-properties.csv
> > >>>>> b/Documentation/gpu/kms-properties.csv index
> > >>>>> 927b65e14219..25ad3503d663
> > >>>>> 100644
> > >>>>> --- a/Documentation/gpu/kms-properties.csv
> > >>>>> +++ b/Documentation/gpu/kms-properties.csv
> > >>>>> @@ -99,5 +99,5 @@ radeon,DVI-I,?coherent?,RANGE,"Min=0,
> > >>>>> Max=1",Connector,TBD>
> > >>>>> 
> > >>>>>  ,,"""underscan vborder""",RANGE,"Min=0, Max=128",Connector,TBD
> > >>>>>  ,Audio,?audio?,ENUM,"{ ""off"", ""on"", ""auto"" }",Connector,TBD
> > >>>>>  ,FMT Dithering,?dither?,ENUM,"{ ""off"", ""on"" }",Connector,TBD
> > >>>>> 
> > >>>>> -rcar-du,Generic,"""alpha""",RANGE,"Min=0, Max=255",Plane,TBD
> > >>>>> +,,"""alpha""",RANGE,"Min=0, Max=Driver dependant",Plane,Opacity of
> > >>>>> the plane from transparent (0) to fully opaque (MAX). If this
> > >>>>> property is set to a value different than max, and that the pixel
> > >>>>> will define an alpha component, the property will have precendance
> > >>>>> and the pixel value will be ignored.
> > >> 
> > >> Please don't document new properties in that csv file, it's an
> > >> unreadable mess. Instead follow how we document standardized
> > >> properties nowadays in full-blown sections. For plane blending we
> > >> have:
> > >> 
> > >> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-> >> properties
> > > 
> > > Ack
> > > 
> > >>>>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > >>>>> index 8185e3468a23..5a6f29524f12 100644
> > >>>>> --- a/include/drm/drm_plane.h
> > >>>>> +++ b/include/drm/drm_plane.h
> > >>>>> @@ -42,6 +42,7 @@ struct drm_modeset_acquire_ctx;
> > >>>>>   * 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)
> > >>>>> + * @alpha: opacity of the plane
> > >>>>>   * @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
> > >>>>> @@ -105,6 +106,9 @@ struct drm_plane_state {
> > >>>>>     uint32_t src_x, src_y;
> > >>>>>     uint32_t src_h, src_w;
> > >>>>> 
> > >>>>> +   /* Plane opacity */
> > >>>>> +   u8 alpha;
> > >>>> 
> > >>>> We may want to make that u16. The general we expect 16bpc for most
> > >>>> color related things, but since this is a range prop I suppose we
> > >>>> should just expose the actual hardware range. But making it u16 might
> > >>>> avoid some head scratching for the first person to have hardware with
> > >>>> higher precision. Either that or we should make the prop creation
> > >>>> fail if the driver asks for more bits than we have in the state.
> > >>> 
> > >>> I'm tempted to go one step further and always make the alpha property
> > >>> 16-bits wide for new users (we can't do so for existing users as it
> > >>> could break userspace), and let drivers convert that internally to
> > >>> the range they need. There could however be drawbacks I don't
> > >>> foresee.
> > >> 
> > >> I think scaling the range to match the hw is the most sensible (yes
> > >> I'm flip-flopping around here). And once someone needs more than u8,
> > >> we can extend the internal representation easily. The external
> > >> representation in the property is an u64, that /should/ be enough for
> > >> the next few years :-)
> > > 
> > > Just to make sure we're on the same page, you want to keep the u8, and
> > > if the hardware uses say an u16, the driver for that hardware will do
> > > the upscaling?
> > 
> > The idea is that we'd set the u16 limit in the property and so inform
> > userspace that a different range applies. But that's probably going to be
> > ignored.
> > 
> > Could we do the property itself as u16 range, and (for now, only
> > internally in drm in drm_plane_state) throw the lower u8 bits away? Or
> > just let drivers do this.
> 
> I'm fine with this except for the drivers that currently implement the alpha 
> property with a different range. The rcar-du driver for instances has the 
> alpha range set to 0x00 to 0xff, so we can't change it without risk of 
> breaking userspace. I don't know whether there's any userspace using the 
> property, and whether that userspace has any hardcoded assumption.

Does open source userspace for this exist that we could actually break?
:-)

> > Sorry that I'm flip-flopping around on this, but we just have an ongoing
> > discussion about a range/size mixup in the CTM uapi, I think assuming that
> > all userspace will correctly scale is not realistic. So larger scale in
> > the uapi (but maybe not internally) from the start seems like a good idea.
> 
> Can we make the range randomly chosen at every boot then ? :-) That would 
> force userspace to be generic.

Either way I think we've discussed this enough already, just pick one of
the reasonable options and we'll live with the consequences. So either
fixed 0xffff limit or (hopefully) variable limit exposed in the prop.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH v3 1/8] drm/blend: Add a generic alpha property

[Laurent Pinchart]

Hi Daniel,

On Monday, 19 February 2018 23:58:40 EET Daniel Vetter wrote:
> On Mon, Feb 19, 2018 at 9:19 PM, Laurent Pinchart wrote:
> > On Friday, 16 February 2018 20:20:41 EET Ville Syrj?l? wrote:
> >> On Fri, Feb 16, 2018 at 06:39:29PM +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/034741
> >>> .html
> >>> 
> >>> Let's create a helper in order to move that to the core.
> >>> 
> >>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>> Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
> >>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> >>> ---
> >>> 
> >>>  Documentation/gpu/kms-properties.csv |  2 +-
> >>>  drivers/gpu/drm/drm_atomic.c         |  4 ++++-
> >>>  drivers/gpu/drm/drm_atomic_helper.c  |  4 ++++-
> >>>  drivers/gpu/drm/drm_blend.c          | 32 ++++++++++++++++++++++++++++-
> >>>  include/drm/drm_blend.h              |  1 +-
> >>>  include/drm/drm_plane.h              |  6 +++++-
> >>>  6 files changed, 48 insertions(+), 1 deletion(-)
> >>> 
> >>> diff --git a/Documentation/gpu/kms-properties.csv
> >>> b/Documentation/gpu/kms-properties.csv index 927b65e14219..25ad3503d663
> >>> 100644
> >>> --- a/Documentation/gpu/kms-properties.csv
> >>> +++ b/Documentation/gpu/kms-properties.csv
> >>> @@ -99,5 +99,5 @@ radeon,DVI-I,?coherent?,RANGE,"Min=0,
> >>> Max=1",Connector,TBD>
> >>> 
> >>>  ,,"""underscan vborder""",RANGE,"Min=0, Max=128",Connector,TBD
> >>>  ,Audio,?audio?,ENUM,"{ ""off"", ""on"", ""auto"" }",Connector,TBD
> >>>  ,FMT Dithering,?dither?,ENUM,"{ ""off"", ""on"" }",Connector,TBD
> >>> 
> >>> -rcar-du,Generic,"""alpha""",RANGE,"Min=0, Max=255",Plane,TBD
> >>> +,,"""alpha""",RANGE,"Min=0, Max=Driver dependant",Plane,Opacity of the
> >>> plane from transparent (0) to fully opaque (MAX). If this property is
> >>> set to a value different than max, and that the pixel will define an
> >>> alpha component, the property will have precendance and the pixel value
> >>> will be ignored.
> 
> Please don't document new properties in that csv file, it's an
> unreadable mess. Instead follow how we document standardized
> properties nowadays in full-blown sections. For plane blending we
> have:
> 
> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-prop
> erties
> 
> >> Those semantics don't seem particularly good to me. I think we would want
> >> the per-pixel alpha and global alpha both to be effecive at the same
> >> time. You can always decide to ignore the per-pixel alpha by using a
> >> pixel format without alpha.
> > 
> > That makes sense to me. However, it also brings a new question: how should
> > a driver that supports either global alpha or pixel alpha but not both
> > signal that to userspace, and how should it reacts when userspace selects
> > a format with an alpha channel and set a global alpha value other than
> > fully opaque ? To make things more complex, note that some drivers
> > support combining global alpha and pixel alpha only for a subset of the
> > formats with an alpha channel (for instance for ARGB 1555 formats, but
> > not for ARGB 8888 formats).
> 
> atomic_check can reject unsupported configs. Userspace needs to fall
> back somehow (either switch to xrgb or make alpha fully opaque or just
> give up on that plane). We have a lot of such corner-cases we don't
> tell userspace about explicitly at all.

I'm OK with failing the commit in case in invalid configuration is requested. 
However, using a check-only commit to find out whether combining global alpha 
and pixel alpha is supported doesn't seem a good idea to me. First of all 
userspace would need to try that for all formats, making it cumbersome. Then, 
an atomic commit is a black box, we don't report the failure cause to 
userspace. It makes it hard to use it to test support for a feature as it 
could fail for an unrelated reason. Finally, we'd open the door to various 
kind of heuristics implemented differently in different userspace stacks, and 
that would increase the risk of breaking userspace. I'd rather have an 
explicitly documented way to perform such checks.

> >> Also, where's the userspace that wants this feature?
> >> 
> >> <snip>
> >> 
> >>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> >>> index 8185e3468a23..5a6f29524f12 100644
> >>> --- a/include/drm/drm_plane.h
> >>> +++ b/include/drm/drm_plane.h
> >>> @@ -42,6 +42,7 @@ struct drm_modeset_acquire_ctx;
> >>>   * 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)
> >>> + * @alpha: opacity of the plane
> >>>   * @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
> >>> @@ -105,6 +106,9 @@ struct drm_plane_state {
> >>>     uint32_t src_x, src_y;
> >>>     uint32_t src_h, src_w;
> >>> 
> >>> +   /* Plane opacity */
> >>> +   u8 alpha;
> >> 
> >> We may want to make that u16. The general we expect 16bpc for most color
> >> related things, but since this is a range prop I suppose we should just
> >> expose the actual hardware range. But making it u16 might avoid some head
> >> scratching for the first person to have hardware with higher precision.
> >> Either that or we should make the prop creation fail if the driver asks
> >> for more bits than we have in the state.
> > 
> > I'm tempted to go one step further and always make the alpha property
> > 16-bits wide for new users (we can't do so for existing users as it could
> > break userspace), and let drivers convert that internally to the range
> > they need. There could however be drawbacks I don't foresee.
> 
> I think scaling the range to match the hw is the most sensible (yes
> I'm flip-flopping around here).

So you mean keeping the proposed implementation, with a driver-specific 
maximum value ?

> And once someone needs more than u8, we can extend the internal
> representation easily. The external representation in the property is an
> u64, that /should/ be enough for the next few years :-)

The external representation is split in two parts. The value is stored in a 
64-bits field, and that is safe. The second part of the representation is the 
minimum (hardcoded to 0) and maximum (currently variable) values reported by 
the property. What we need to decide now is whether to hardcode the maximum 
value to 0xffff for all new users of the alpha property, or to always expose 
the hardware range.

> >> Oh, and you should plug this into the state dumper as well.
> >> 
> >>> +
> >>> 
> >>>     /* Plane rotation */
> >>>     unsigned int rotation;
> >>> @@ -481,6 +485,7 @@ enum drm_plane_type {
> >>>   * @funcs: helper functions
> >>>   * @properties: property tracking for this plane
> >>>   * @type: type of plane (overlay, primary, cursor)
> >>> + * @alpha_property: alpha property for this plane
> >>>   * @zpos_property: zpos property for this plane
> >>>   * @rotation_property: rotation property for this plane
> >>>   * @helper_private: mid-layer private data
> >>> @@ -556,6 +561,7 @@ struct drm_plane {
> >>>      */
> >>>     
> >>>     struct drm_plane_state *state;
> >>> +   struct drm_property *alpha_property;
> >>>     struct drm_property *zpos_property;
> >>>     struct drm_property *rotation_property;
> >>>  };

-- 
Regards,

Laurent Pinchart

[PATCH v3 1/8] drm/blend: Add a generic alpha property

[Daniel Vetter]

On Wed, Feb 21, 2018 at 10:39:02PM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Monday, 19 February 2018 23:58:40 EET Daniel Vetter wrote:
> > On Mon, Feb 19, 2018 at 9:19 PM, Laurent Pinchart wrote:
> > > On Friday, 16 February 2018 20:20:41 EET Ville Syrj?l? wrote:
> > >> On Fri, Feb 16, 2018 at 06:39:29PM +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/034741
> > >>> .html
> > >>> 
> > >>> Let's create a helper in order to move that to the core.
> > >>> 
> > >>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >>> Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
> > >>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> > >>> ---
> > >>> 
> > >>>  Documentation/gpu/kms-properties.csv |  2 +-
> > >>>  drivers/gpu/drm/drm_atomic.c         |  4 ++++-
> > >>>  drivers/gpu/drm/drm_atomic_helper.c  |  4 ++++-
> > >>>  drivers/gpu/drm/drm_blend.c          | 32 ++++++++++++++++++++++++++++-
> > >>>  include/drm/drm_blend.h              |  1 +-
> > >>>  include/drm/drm_plane.h              |  6 +++++-
> > >>>  6 files changed, 48 insertions(+), 1 deletion(-)
> > >>> 
> > >>> diff --git a/Documentation/gpu/kms-properties.csv
> > >>> b/Documentation/gpu/kms-properties.csv index 927b65e14219..25ad3503d663
> > >>> 100644
> > >>> --- a/Documentation/gpu/kms-properties.csv
> > >>> +++ b/Documentation/gpu/kms-properties.csv
> > >>> @@ -99,5 +99,5 @@ radeon,DVI-I,?coherent?,RANGE,"Min=0,
> > >>> Max=1",Connector,TBD>
> > >>> 
> > >>>  ,,"""underscan vborder""",RANGE,"Min=0, Max=128",Connector,TBD
> > >>>  ,Audio,?audio?,ENUM,"{ ""off"", ""on"", ""auto"" }",Connector,TBD
> > >>>  ,FMT Dithering,?dither?,ENUM,"{ ""off"", ""on"" }",Connector,TBD
> > >>> 
> > >>> -rcar-du,Generic,"""alpha""",RANGE,"Min=0, Max=255",Plane,TBD
> > >>> +,,"""alpha""",RANGE,"Min=0, Max=Driver dependant",Plane,Opacity of the
> > >>> plane from transparent (0) to fully opaque (MAX). If this property is
> > >>> set to a value different than max, and that the pixel will define an
> > >>> alpha component, the property will have precendance and the pixel value
> > >>> will be ignored.
> > 
> > Please don't document new properties in that csv file, it's an
> > unreadable mess. Instead follow how we document standardized
> > properties nowadays in full-blown sections. For plane blending we
> > have:
> > 
> > https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-prop
> > erties
> > 
> > >> Those semantics don't seem particularly good to me. I think we would want
> > >> the per-pixel alpha and global alpha both to be effecive at the same
> > >> time. You can always decide to ignore the per-pixel alpha by using a
> > >> pixel format without alpha.
> > > 
> > > That makes sense to me. However, it also brings a new question: how should
> > > a driver that supports either global alpha or pixel alpha but not both
> > > signal that to userspace, and how should it reacts when userspace selects
> > > a format with an alpha channel and set a global alpha value other than
> > > fully opaque ? To make things more complex, note that some drivers
> > > support combining global alpha and pixel alpha only for a subset of the
> > > formats with an alpha channel (for instance for ARGB 1555 formats, but
> > > not for ARGB 8888 formats).
> > 
> > atomic_check can reject unsupported configs. Userspace needs to fall
> > back somehow (either switch to xrgb or make alpha fully opaque or just
> > give up on that plane). We have a lot of such corner-cases we don't
> > tell userspace about explicitly at all.
> 
> I'm OK with failing the commit in case in invalid configuration is requested. 
> However, using a check-only commit to find out whether combining global alpha 
> and pixel alpha is supported doesn't seem a good idea to me. First of all 
> userspace would need to try that for all formats, making it cumbersome. Then, 
> an atomic commit is a black box, we don't report the failure cause to 
> userspace. It makes it hard to use it to test support for a feature as it 
> could fail for an unrelated reason. Finally, we'd open the door to various 
> kind of heuristics implemented differently in different userspace stacks, and 
> that would increase the risk of breaking userspace. I'd rather have an 
> explicitly documented way to perform such checks.

I am _not_ against more explicit checks. I only object against them in the
name of "future proofing" without a solid pile of different driver and
userspace implementations as demonstration vehicles that the hints are
actually useful and needed by userspace.

DRM history is full of fake-generic uapi that turned out to be useful for
exactly the 1 driver/userspace combo it was originally implemented for,
and frankly for just 1 combo hardcoding works better.

Also, these fake-generic interfaces tend to result in epic bikeshed fests
that go nowhere, I think it's better to avoid them in the first uapi round
and get the basic properties in first. Nothing prevents us from adding
hints and more useful constraints information later on.

> > >> Also, where's the userspace that wants this feature?
> > >> 
> > >> <snip>
> > >> 
> > >>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > >>> index 8185e3468a23..5a6f29524f12 100644
> > >>> --- a/include/drm/drm_plane.h
> > >>> +++ b/include/drm/drm_plane.h
> > >>> @@ -42,6 +42,7 @@ struct drm_modeset_acquire_ctx;
> > >>>   * 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)
> > >>> + * @alpha: opacity of the plane
> > >>>   * @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
> > >>> @@ -105,6 +106,9 @@ struct drm_plane_state {
> > >>>     uint32_t src_x, src_y;
> > >>>     uint32_t src_h, src_w;
> > >>> 
> > >>> +   /* Plane opacity */
> > >>> +   u8 alpha;
> > >> 
> > >> We may want to make that u16. The general we expect 16bpc for most color
> > >> related things, but since this is a range prop I suppose we should just
> > >> expose the actual hardware range. But making it u16 might avoid some head
> > >> scratching for the first person to have hardware with higher precision.
> > >> Either that or we should make the prop creation fail if the driver asks
> > >> for more bits than we have in the state.
> > > 
> > > I'm tempted to go one step further and always make the alpha property
> > > 16-bits wide for new users (we can't do so for existing users as it could
> > > break userspace), and let drivers convert that internally to the range
> > > they need. There could however be drawbacks I don't foresee.
> > 
> > I think scaling the range to match the hw is the most sensible (yes
> > I'm flip-flopping around here).
> 
> So you mean keeping the proposed implementation, with a driver-specific 
> maximum value ?
> 
> > And once someone needs more than u8, we can extend the internal
> > representation easily. The external representation in the property is an
> > u64, that /should/ be enough for the next few years :-)
> 
> The external representation is split in two parts. The value is stored in a 
> 64-bits field, and that is safe. The second part of the representation is the 
> minimum (hardcoded to 0) and maximum (currently variable) values reported by 
> the property. What we need to decide now is whether to hardcode the maximum 
> value to 0xffff for all new users of the alpha property, or to always expose 
> the hardware range.

After a bit of vacation I'm leaning towards hardcoding 0xffff for now,
because if we start out with generic userspace that's only tested on hw
with a limit of 0xff, then defacto (but maybe not in the docs, but docs
lose against reality) we'll hardcode the uapi to 0xff, not the variable hw
limit. And that would mean as soon as hw with a higher limit shows up we
need to add a alpha-but-high-res-for-real-now property.
-Daniel

> 
> > >> Oh, and you should plug this into the state dumper as well.
> > >> 
> > >>> +
> > >>> 
> > >>>     /* Plane rotation */
> > >>>     unsigned int rotation;
> > >>> @@ -481,6 +485,7 @@ enum drm_plane_type {
> > >>>   * @funcs: helper functions
> > >>>   * @properties: property tracking for this plane
> > >>>   * @type: type of plane (overlay, primary, cursor)
> > >>> + * @alpha_property: alpha property for this plane
> > >>>   * @zpos_property: zpos property for this plane
> > >>>   * @rotation_property: rotation property for this plane
> > >>>   * @helper_private: mid-layer private data
> > >>> @@ -556,6 +561,7 @@ struct drm_plane {
> > >>>      */
> > >>>     
> > >>>     struct drm_plane_state *state;
> > >>> +   struct drm_property *alpha_property;
> > >>>     struct drm_property *zpos_property;
> > >>>     struct drm_property *rotation_property;
> > >>>  };
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[PATCH v3 1/8] drm/blend: Add a generic alpha property

[Stefan Schake]

On Fri, Feb 16, 2018 at 7:20 PM, Ville Syrj?l?
<ville.syrjala@linux.intel.com> wrote:
> On Fri, Feb 16, 2018 at 06:39:29PM +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/034741.html
>>
>> Let's create a helper in order to move that to the core.
>>
>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
>> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>> ---
>>  Documentation/gpu/kms-properties.csv |  2 +-
>>  drivers/gpu/drm/drm_atomic.c         |  4 ++++-
>>  drivers/gpu/drm/drm_atomic_helper.c  |  4 ++++-
>>  drivers/gpu/drm/drm_blend.c          | 32 +++++++++++++++++++++++++++++-
>>  include/drm/drm_blend.h              |  1 +-
>>  include/drm/drm_plane.h              |  6 +++++-
>>  6 files changed, 48 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/gpu/kms-properties.csv b/Documentation/gpu/kms-properties.csv
>> index 927b65e14219..25ad3503d663 100644
>> --- a/Documentation/gpu/kms-properties.csv
>> +++ b/Documentation/gpu/kms-properties.csv
>> @@ -99,5 +99,5 @@ radeon,DVI-I,?coherent?,RANGE,"Min=0, Max=1",Connector,TBD
>>  ,,"""underscan vborder""",RANGE,"Min=0, Max=128",Connector,TBD
>>  ,Audio,?audio?,ENUM,"{ ""off"", ""on"", ""auto"" }",Connector,TBD
>>  ,FMT Dithering,?dither?,ENUM,"{ ""off"", ""on"" }",Connector,TBD
>> -rcar-du,Generic,"""alpha""",RANGE,"Min=0, Max=255",Plane,TBD
>> +,,"""alpha""",RANGE,"Min=0, Max=Driver dependant",Plane,Opacity of the plane from transparent (0) to fully opaque (MAX). If this property is set to a value different than max, and that the pixel will define an alpha component, the property will have precendance and the pixel value will be ignored.
>
> Those semantics don't seem particularly good to me. I think we would want the
> per-pixel alpha and global alpha both to be effecive at the same time. You can
> always decide to ignore the per-pixel alpha by using a pixel format without
> alpha.
>
> Also, where's the userspace that wants this feature?

drm_hwcomposer uses an 8-bit per-plane alpha property, and from what I
can tell the semantics are that both pixel and plane alpha apply
simultaneously if present.
Here is what I think was the kernel-side for this:

https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/306122

I've added Sean Paul, he might be able to give a more definitive answer.

Regards,
Stefan

[PATCH v3 1/8] drm/blend: Add a generic alpha property

[Maxime Ripard]

Hi,

On Tue, Feb 20, 2018 at 04:10:28PM +0100, Stefan Schake wrote:
> On Fri, Feb 16, 2018 at 7:20 PM, Ville Syrj?l?
> <ville.syrjala@linux.intel.com> wrote:
> > On Fri, Feb 16, 2018 at 06:39:29PM +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/034741.html
> >>
> >> Let's create a helper in order to move that to the core.
> >>
> >> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
> >> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> >> ---
> >>  Documentation/gpu/kms-properties.csv |  2 +-
> >>  drivers/gpu/drm/drm_atomic.c         |  4 ++++-
> >>  drivers/gpu/drm/drm_atomic_helper.c  |  4 ++++-
> >>  drivers/gpu/drm/drm_blend.c          | 32 +++++++++++++++++++++++++++++-
> >>  include/drm/drm_blend.h              |  1 +-
> >>  include/drm/drm_plane.h              |  6 +++++-
> >>  6 files changed, 48 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Documentation/gpu/kms-properties.csv b/Documentation/gpu/kms-properties.csv
> >> index 927b65e14219..25ad3503d663 100644
> >> --- a/Documentation/gpu/kms-properties.csv
> >> +++ b/Documentation/gpu/kms-properties.csv
> >> @@ -99,5 +99,5 @@ radeon,DVI-I,?coherent?,RANGE,"Min=0, Max=1",Connector,TBD
> >>  ,,"""underscan vborder""",RANGE,"Min=0, Max=128",Connector,TBD
> >>  ,Audio,?audio?,ENUM,"{ ""off"", ""on"", ""auto"" }",Connector,TBD
> >>  ,FMT Dithering,?dither?,ENUM,"{ ""off"", ""on"" }",Connector,TBD
> >> -rcar-du,Generic,"""alpha""",RANGE,"Min=0, Max=255",Plane,TBD
> >> +,,"""alpha""",RANGE,"Min=0, Max=Driver dependant",Plane,Opacity of the plane from transparent (0) to fully opaque (MAX). If this property is set to a value different than max, and that the pixel will define an alpha component, the property will have precendance and the pixel value will be ignored.
> >
> > Those semantics don't seem particularly good to me. I think we would want the
> > per-pixel alpha and global alpha both to be effecive at the same time. You can
> > always decide to ignore the per-pixel alpha by using a pixel format without
> > alpha.
> >
> > Also, where's the userspace that wants this feature?
> 
> drm_hwcomposer uses an 8-bit per-plane alpha property, and from what I
> can tell the semantics are that both pixel and plane alpha apply
> simultaneously if present.
> Here is what I think was the kernel-side for this:
> 
> https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/306122
> 
> I've added Sean Paul, he might be able to give a more definitive answer.

What is the behaviour of the tegra engine when it has both a
pixel-alpha and a plane-alpha?

Atmel at least will drop one (but I'm not sure which one anymore).

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180221/4e4354d6/attachment.sig>

[PATCH v3 1/8] drm/blend: Add a generic alpha property

[Stefan Schake]

Hey,

On Wed, Feb 21, 2018 at 2:04 PM, Maxime Ripard
<maxime.ripard@bootlin.com> wrote:
>
> What is the behaviour of the tegra engine when it has both a
> pixel-alpha and a plane-alpha?
>
> Atmel at least will drop one (but I'm not sure which one anymore).

Sorry, I have no more on the Tegra beyond that commit. But I did have some
more play time with drm_hwcomposer and from the rendering, it certainly
expects both to apply at the same time. To further complicate it, from
what I can tell with the VC4 HVS, you can actually configure it to
1) use only pixel alpha 2) use only plane alpha or 3) mix both pixel
and plane alpha, with 3 being what drm_hwc seems to want.

I think once writeback lands it would be a good idea to have some tests
that establish DRM plane blending standards beyond text in docs :)

Regards,
Stefan

[PATCH v3 1/8] drm/blend: Add a generic alpha property

[Laurent Pinchart]

Hi Maxime,

Thank you for the patch.

On Friday, 16 February 2018 19:39:29 EET 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/034741.html
> 
> Let's create a helper in order to move that to the core.
> 
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Boris Brezillon <boris.brezillon@bootlin.com>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> ---
>  Documentation/gpu/kms-properties.csv |  2 +-
>  drivers/gpu/drm/drm_atomic.c         |  4 ++++-
>  drivers/gpu/drm/drm_atomic_helper.c  |  4 ++++-
>  drivers/gpu/drm/drm_blend.c          | 32 +++++++++++++++++++++++++++++-
>  include/drm/drm_blend.h              |  1 +-
>  include/drm/drm_plane.h              |  6 +++++-
>  6 files changed, 48 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/gpu/kms-properties.csv
> b/Documentation/gpu/kms-properties.csv index 927b65e14219..25ad3503d663
> 100644
> --- a/Documentation/gpu/kms-properties.csv
> +++ b/Documentation/gpu/kms-properties.csv
> @@ -99,5 +99,5 @@ radeon,DVI-I,?coherent?,RANGE,"Min=0, Max=1",Connector,TBD
> ,,"""underscan vborder""",RANGE,"Min=0, Max=128",Connector,TBD
>  ,Audio,?audio?,ENUM,"{ ""off"", ""on"", ""auto"" }",Connector,TBD
>  ,FMT Dithering,?dither?,ENUM,"{ ""off"", ""on"" }",Connector,TBD
> -rcar-du,Generic,"""alpha""",RANGE,"Min=0, Max=255",Plane,TBD
> +,,"""alpha""",RANGE,"Min=0, Max=Driver dependant",Plane,Opacity of the
> plane from transparent (0) to fully opaque (MAX). If this property is set
> to a value different than max, and that the pixel will define an alpha
> component, the property will have precendance and the pixel value will be
> ignored. The alpha value is represented as straight alpha, ie the colors
> haven't been pre-adjusted for their opacity by multiplication. Therefore,
> the equation to get a color value for one pixel, assuming two planes A and
> B, will be (color_a * alpha_a + color_b * alpha_b * (MAX - alpha_a) / MAX)
> / (alpha_a + alpha_b * (MAX - alpha_a) / MAX)
> ,,"""colorkey""",RANGE,"Min=0, Max=0x01ffffff",Plane,TBD
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 7d9ad20040a1..3defc56a1ef2 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;
> @@ -814,6 +816,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 ae3cbfe9e01c..2b88f593aab4
> 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3482,6 +3482,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 4c62dff14893..a5dea7cbed2c 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -104,6 +104,38 @@
>   */
> 
>  /**
> + * drm_plane_create_alpha_property - create a new alpha property
> + * @plane: drm plane
> + * @max_alpha: maximum value of alpha
> + *
> + * This function initializes a generic, mutable, alpha property and
> + * enables support for it in the DRM core.

s/initializes/creates/

I would also mention that it attaches the property to the plane.

> + * The alpha property will be allowed to be within the bounds of 0
> + * (transparent) to @max_alpha (opaque)

s/$/./

> + *
> + * Returns:
> + * 0 on success, negative error code on failure.
> + */
> +int drm_plane_create_alpha_property(struct drm_plane *plane, u16 max_alpha)
> +{
> +	struct drm_property *prop;
> +
> +	prop = drm_property_create_range(plane->dev, 0, "alpha", 0, max_alpha);
> +	if (!prop)
> +		return -ENOMEM;
> +
> +	drm_object_attach_property(&plane->base, prop, max_alpha);
> +	plane->alpha_property = prop;
> +
> +	if (plane->state)
> +		plane->state->alpha = max_alpha;
> +
> +	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..e5affba6ebde 100644
> --- a/include/drm/drm_blend.h
> +++ b/include/drm/drm_blend.h
> @@ -36,6 +36,7 @@ static inline bool drm_rotation_90_or_270(unsigned int
> rotation) return rotation & (DRM_MODE_ROTATE_90 | DRM_MODE_ROTATE_270);
>  }
> 
> +int drm_plane_create_alpha_property(struct drm_plane *plane, u16 alpha);
>  int drm_plane_create_rotation_property(struct drm_plane *plane,
>  				       unsigned int rotation,
>  				       unsigned int supported_rotations);
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 8185e3468a23..5a6f29524f12 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -42,6 +42,7 @@ struct drm_modeset_acquire_ctx;
>   *	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)
> + * @alpha: opacity of the plane
>   * @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
> @@ -105,6 +106,9 @@ struct drm_plane_state {
>  	uint32_t src_x, src_y;
>  	uint32_t src_h, src_w;
> 
> +	/* Plane opacity */
> +	u8 alpha;
> +
>  	/* Plane rotation */
>  	unsigned int rotation;
> 
> @@ -481,6 +485,7 @@ enum drm_plane_type {
>   * @funcs: helper functions
>   * @properties: property tracking for this plane
>   * @type: type of plane (overlay, primary, cursor)
> + * @alpha_property: alpha property for this plane
>   * @zpos_property: zpos property for this plane
>   * @rotation_property: rotation property for this plane
>   * @helper_private: mid-layer private data
> @@ -556,6 +561,7 @@ struct drm_plane {
>  	 */
>  	struct drm_plane_state *state;
> 
> +	struct drm_property *alpha_property;
>  	struct drm_property *zpos_property;
>  	struct drm_property *rotation_property;
>  };

-- 
Regards,

Laurent Pinchart

[PATCH v3 2/8] drm/atmel-hclcdc: Convert to the new generic alpha property

[Maxime Ripard]

Now that we have support for per-plane alpha in the core, let's use it.

Acked-by: Boris Brezillon <boris.brezillon@bootlin.com>
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h    | 13 +---
 drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 89 ++----------------
 2 files changed, 14 insertions(+), 88 deletions(-)

diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
index ab32d5b268d2..60c937f42114 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h
@@ -299,7 +299,6 @@ struct atmel_hlcdc_layer {
 struct atmel_hlcdc_plane {
 	struct drm_plane base;
 	struct atmel_hlcdc_layer layer;
-	struct atmel_hlcdc_plane_properties *properties;
 };
 
 static inline struct atmel_hlcdc_plane *
@@ -346,18 +345,6 @@ struct atmel_hlcdc_dc_desc {
 };
 
 /**
- * Atmel HLCDC Plane properties.
- *
- * This structure stores plane property definitions.
- *
- * @alpha: alpha blending (or transparency) property
- * @rotation: rotation property
- */
-struct atmel_hlcdc_plane_properties {
-	struct drm_property *alpha;
-};
-
-/**
  * Atmel HLCDC Display Controller.
  *
  * @desc: HLCDC Display Controller description
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
index e18800ed7cd1..d1b89a6f6280 100644
--- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
+++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c
@@ -31,7 +31,6 @@
  * @src_y: y buffer position
  * @src_w: buffer width
  * @src_h: buffer height
- * @alpha: alpha blending of the plane
  * @disc_x: x discard position
  * @disc_y: y discard position
  * @disc_w: discard width
@@ -54,8 +53,6 @@ struct atmel_hlcdc_plane_state {
 	uint32_t src_w;
 	uint32_t src_h;
 
-	u8 alpha;
-
 	int disc_x;
 	int disc_y;
 	int disc_w;
@@ -385,7 +382,7 @@ atmel_hlcdc_plane_update_general_settings(struct atmel_hlcdc_plane *plane,
 			cfg |= ATMEL_HLCDC_LAYER_LAEN;
 		else
 			cfg |= ATMEL_HLCDC_LAYER_GAEN |
-			       ATMEL_HLCDC_LAYER_GA(state->alpha);
+			       ATMEL_HLCDC_LAYER_GA(state->base.alpha);
 	}
 
 	if (state->disc_h && state->disc_w)
@@ -553,7 +550,7 @@ atmel_hlcdc_plane_prepare_disc_area(struct drm_crtc_state *c_state)
 
 		if (!ovl_s->fb ||
 		    ovl_s->fb->format->has_alpha ||
-		    ovl_state->alpha != 255)
+		    ovl_s->alpha != 255)
 			continue;
 
 		/* TODO: implement a smarter hidden area detection */
@@ -829,51 +826,18 @@ static void atmel_hlcdc_plane_destroy(struct drm_plane *p)
 	drm_plane_cleanup(p);
 }
 
-static int atmel_hlcdc_plane_atomic_set_property(struct drm_plane *p,
-						 struct drm_plane_state *s,
-						 struct drm_property *property,
-						 uint64_t val)
-{
-	struct atmel_hlcdc_plane *plane = drm_plane_to_atmel_hlcdc_plane(p);
-	struct atmel_hlcdc_plane_properties *props = plane->properties;
-	struct atmel_hlcdc_plane_state *state =
-			drm_plane_state_to_atmel_hlcdc_plane_state(s);
-
-	if (property == props->alpha)
-		state->alpha = val;
-	else
-		return -EINVAL;
-
-	return 0;
-}
-
-static int atmel_hlcdc_plane_atomic_get_property(struct drm_plane *p,
-					const struct drm_plane_state *s,
-					struct drm_property *property,
-					uint64_t *val)
-{
-	struct atmel_hlcdc_plane *plane = drm_plane_to_atmel_hlcdc_plane(p);
-	struct atmel_hlcdc_plane_properties *props = plane->properties;
-	const struct atmel_hlcdc_plane_state *state =
-		container_of(s, const struct atmel_hlcdc_plane_state, base);
-
-	if (property == props->alpha)
-		*val = state->alpha;
-	else
-		return -EINVAL;
-
-	return 0;
-}
-
-static int atmel_hlcdc_plane_init_properties(struct atmel_hlcdc_plane *plane,
-				struct atmel_hlcdc_plane_properties *props)
+static int atmel_hlcdc_plane_init_properties(struct atmel_hlcdc_plane *plane)
 {
 	const struct atmel_hlcdc_layer_desc *desc = plane->layer.desc;
 
 	if (desc->type == ATMEL_HLCDC_OVERLAY_LAYER ||
-	    desc->type == ATMEL_HLCDC_CURSOR_LAYER)
-		drm_object_attach_property(&plane->base.base,
-					   props->alpha, 255);
+	    desc->type == ATMEL_HLCDC_CURSOR_LAYER) {
+		int ret;
+
+		ret = drm_plane_create_alpha_property(&plane->base, 255);
+		if (ret)
+			return ret;
+	}
 
 	if (desc->layout.xstride && desc->layout.pstride) {
 		int ret;
@@ -988,8 +952,8 @@ static void atmel_hlcdc_plane_reset(struct drm_plane *p)
 			return;
 		}
 
-		state->alpha = 255;
 		p->state = &state->base;
+		p->state->alpha = 255;
 		p->state->plane = p;
 	}
 }
@@ -1042,13 +1006,10 @@ static const struct drm_plane_funcs layer_plane_funcs = {
 	.reset = atmel_hlcdc_plane_reset,
 	.atomic_duplicate_state = atmel_hlcdc_plane_atomic_duplicate_state,
 	.atomic_destroy_state = atmel_hlcdc_plane_atomic_destroy_state,
-	.atomic_set_property = atmel_hlcdc_plane_atomic_set_property,
-	.atomic_get_property = atmel_hlcdc_plane_atomic_get_property,
 };
 
 static int atmel_hlcdc_plane_create(struct drm_device *dev,
-				    const struct atmel_hlcdc_layer_desc *desc,
-				    struct atmel_hlcdc_plane_properties *props)
+				    const struct atmel_hlcdc_layer_desc *desc)
 {
 	struct atmel_hlcdc_dc *dc = dev->dev_private;
 	struct atmel_hlcdc_plane *plane;
@@ -1060,7 +1021,6 @@ static int atmel_hlcdc_plane_create(struct drm_device *dev,
 		return -ENOMEM;
 
 	atmel_hlcdc_layer_init(&plane->layer, desc, dc->hlcdc->regmap);
-	plane->properties = props;
 
 	if (desc->type == ATMEL_HLCDC_BASE_LAYER)
 		type = DRM_PLANE_TYPE_PRIMARY;
@@ -1081,7 +1041,7 @@ static int atmel_hlcdc_plane_create(struct drm_device *dev,
 			     &atmel_hlcdc_layer_plane_helper_funcs);
 
 	/* Set default property values*/
-	ret = atmel_hlcdc_plane_init_properties(plane, props);
+	ret = atmel_hlcdc_plane_init_properties(plane);
 	if (ret)
 		return ret;
 
@@ -1090,34 +1050,13 @@ static int atmel_hlcdc_plane_create(struct drm_device *dev,
 	return 0;
 }
 
-static struct atmel_hlcdc_plane_properties *
-atmel_hlcdc_plane_create_properties(struct drm_device *dev)
-{
-	struct atmel_hlcdc_plane_properties *props;
-
-	props = devm_kzalloc(dev->dev, sizeof(*props), GFP_KERNEL);
-	if (!props)
-		return ERR_PTR(-ENOMEM);
-
-	props->alpha = drm_property_create_range(dev, 0, "alpha", 0, 255);
-	if (!props->alpha)
-		return ERR_PTR(-ENOMEM);
-
-	return props;
-}
-
 int atmel_hlcdc_create_planes(struct drm_device *dev)
 {
 	struct atmel_hlcdc_dc *dc = dev->dev_private;
-	struct atmel_hlcdc_plane_properties *props;
 	const struct atmel_hlcdc_layer_desc *descs = dc->desc->layers;
 	int nlayers = dc->desc->nlayers;
 	int i, ret;
 
-	props = atmel_hlcdc_plane_create_properties(dev);
-	if (IS_ERR(props))
-		return PTR_ERR(props);
-
 	dc->dscrpool = dmam_pool_create("atmel-hlcdc-dscr", dev->dev,
 				sizeof(struct atmel_hlcdc_dma_channel_dscr),
 				sizeof(u64), 0);
@@ -1130,7 +1069,7 @@ int atmel_hlcdc_create_planes(struct drm_device *dev)
 		    descs[i].type != ATMEL_HLCDC_CURSOR_LAYER)
 			continue;
 
-		ret = atmel_hlcdc_plane_create(dev, &descs[i], props);
+		ret = atmel_hlcdc_plane_create(dev, &descs[i]);
 		if (ret)
 			return ret;
 	}
-- 
git-series 0.9.1

[PATCH v3 3/8] drm/rcar-du: Convert to the new generic alpha property

[Maxime Ripard]

Now that we have support for per-plane alpha in the core, let's use it.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/rcar-du/rcar_du_drv.h   |  1 +-
 drivers/gpu/drm/rcar-du/rcar_du_kms.c   |  5 +---
 drivers/gpu/drm/rcar-du/rcar_du_plane.c | 15 +++------
 drivers/gpu/drm/rcar-du/rcar_du_plane.h |  2 +-
 drivers/gpu/drm/rcar-du/rcar_du_vsp.c   | 42 ++------------------------
 drivers/gpu/drm/rcar-du/rcar_du_vsp.h   |  2 +-
 6 files changed, 9 insertions(+), 58 deletions(-)

diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
index f400fde65a0c..dad87d449ef7 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h
@@ -90,7 +90,6 @@ struct rcar_du_device {
 	struct rcar_du_vsp vsps[RCAR_DU_MAX_VSPS];
 
 	struct {
-		struct drm_property *alpha;
 		struct drm_property *colorkey;
 	} props;
 
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
index 566d1a948c8f..e1b5a7b460cc 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c
@@ -417,11 +417,6 @@ static int rcar_du_encoders_init(struct rcar_du_device *rcdu)
 
 static int rcar_du_properties_init(struct rcar_du_device *rcdu)
 {
-	rcdu->props.alpha =
-		drm_property_create_range(rcdu->ddev, 0, "alpha", 0, 255);
-	if (rcdu->props.alpha == NULL)
-		return -ENOMEM;
-
 	/*
 	 * The color key is expressed as an RGB888 triplet stored in a 32-bit
 	 * integer in XRGB8888 format. Bit 24 is used as a flag to disable (0)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
index 5687a94d4cb1..136c3b464d81 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c
@@ -423,7 +423,7 @@ static void rcar_du_plane_setup_mode(struct rcar_du_group *rgrp,
 		rcar_du_plane_write(rgrp, index, PnALPHAR, PnALPHAR_ABIT_0);
 	else
 		rcar_du_plane_write(rgrp, index, PnALPHAR,
-				    PnALPHAR_ABIT_X | state->alpha);
+				    PnALPHAR_ABIT_X | state->state.alpha);
 
 	pnmr = PnMR_BM_MD | state->format->pnmr;
 
@@ -697,11 +697,11 @@ static void rcar_du_plane_reset(struct drm_plane *plane)
 
 	state->hwindex = -1;
 	state->source = RCAR_DU_PLANE_MEMORY;
-	state->alpha = 255;
 	state->colorkey = RCAR_DU_COLORKEY_NONE;
 	state->state.zpos = plane->type == DRM_PLANE_TYPE_PRIMARY ? 0 : 1;
 
 	plane->state = &state->state;
+	plane->state->alpha = 255;
 	plane->state->plane = plane;
 }
 
@@ -713,9 +713,7 @@ static int rcar_du_plane_atomic_set_property(struct drm_plane *plane,
 	struct rcar_du_plane_state *rstate = to_rcar_plane_state(state);
 	struct rcar_du_device *rcdu = to_rcar_plane(plane)->group->dev;
 
-	if (property == rcdu->props.alpha)
-		rstate->alpha = val;
-	else if (property == rcdu->props.colorkey)
+	if (property == rcdu->props.colorkey)
 		rstate->colorkey = val;
 	else
 		return -EINVAL;
@@ -731,9 +729,7 @@ static int rcar_du_plane_atomic_get_property(struct drm_plane *plane,
 		container_of(state, const struct rcar_du_plane_state, state);
 	struct rcar_du_device *rcdu = to_rcar_plane(plane)->group->dev;
 
-	if (property == rcdu->props.alpha)
-		*val = rstate->alpha;
-	else if (property == rcdu->props.colorkey)
+	if (property == rcdu->props.colorkey)
 		*val = rstate->colorkey;
 	else
 		return -EINVAL;
@@ -802,10 +798,9 @@ int rcar_du_planes_init(struct rcar_du_group *rgrp)
 			continue;
 
 		drm_object_attach_property(&plane->plane.base,
-					   rcdu->props.alpha, 255);
-		drm_object_attach_property(&plane->plane.base,
 					   rcdu->props.colorkey,
 					   RCAR_DU_COLORKEY_NONE);
+		drm_plane_create_alpha_property(&plane->plane, 255);
 		drm_plane_create_zpos_property(&plane->plane, 1, 1, 7);
 	}
 
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.h b/drivers/gpu/drm/rcar-du/rcar_du_plane.h
index 890321b4665d..5c19c69e4691 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_plane.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.h
@@ -50,7 +50,6 @@ static inline struct rcar_du_plane *to_rcar_plane(struct drm_plane *plane)
  * @state: base DRM plane state
  * @format: information about the pixel format used by the plane
  * @hwindex: 0-based hardware plane index, -1 means unused
- * @alpha: value of the plane alpha property
  * @colorkey: value of the plane colorkey property
  */
 struct rcar_du_plane_state {
@@ -60,7 +59,6 @@ struct rcar_du_plane_state {
 	int hwindex;
 	enum rcar_du_plane_source source;
 
-	unsigned int alpha;
 	unsigned int colorkey;
 };
 
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
index 2c260c33840b..bda551c01a43 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
+++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c
@@ -54,6 +54,7 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
 	};
 	struct rcar_du_plane_state state = {
 		.state = {
+			.alpha = 255,
 			.crtc = &crtc->crtc,
 			.dst.x1 = 0,
 			.dst.y1 = 0,
@@ -67,7 +68,6 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc)
 		},
 		.format = rcar_du_format_info(DRM_FORMAT_ARGB8888),
 		.source = RCAR_DU_PLANE_VSPD1,
-		.alpha = 255,
 		.colorkey = 0,
 	};
 
@@ -173,7 +173,7 @@ static void rcar_du_vsp_plane_setup(struct rcar_du_vsp_plane *plane)
 	struct vsp1_du_atomic_config cfg = {
 		.pixelformat = 0,
 		.pitch = fb->pitches[0],
-		.alpha = state->alpha,
+		.alpha = state->state.alpha,
 		.zpos = state->state.zpos,
 	};
 	unsigned int i;
@@ -335,44 +335,13 @@ static void rcar_du_vsp_plane_reset(struct drm_plane *plane)
 	if (state == NULL)
 		return;
 
-	state->alpha = 255;
+	state->state.alpha = 255;
 	state->state.zpos = plane->type == DRM_PLANE_TYPE_PRIMARY ? 0 : 1;
 
 	plane->state = &state->state;
 	plane->state->plane = plane;
 }
 
-static int rcar_du_vsp_plane_atomic_set_property(struct drm_plane *plane,
-	struct drm_plane_state *state, struct drm_property *property,
-	uint64_t val)
-{
-	struct rcar_du_vsp_plane_state *rstate = to_rcar_vsp_plane_state(state);
-	struct rcar_du_device *rcdu = to_rcar_vsp_plane(plane)->vsp->dev;
-
-	if (property == rcdu->props.alpha)
-		rstate->alpha = val;
-	else
-		return -EINVAL;
-
-	return 0;
-}
-
-static int rcar_du_vsp_plane_atomic_get_property(struct drm_plane *plane,
-	const struct drm_plane_state *state, struct drm_property *property,
-	uint64_t *val)
-{
-	const struct rcar_du_vsp_plane_state *rstate =
-		container_of(state, const struct rcar_du_vsp_plane_state, state);
-	struct rcar_du_device *rcdu = to_rcar_vsp_plane(plane)->vsp->dev;
-
-	if (property == rcdu->props.alpha)
-		*val = rstate->alpha;
-	else
-		return -EINVAL;
-
-	return 0;
-}
-
 static const struct drm_plane_funcs rcar_du_vsp_plane_funcs = {
 	.update_plane = drm_atomic_helper_update_plane,
 	.disable_plane = drm_atomic_helper_disable_plane,
@@ -380,8 +349,6 @@ static const struct drm_plane_funcs rcar_du_vsp_plane_funcs = {
 	.destroy = drm_plane_cleanup,
 	.atomic_duplicate_state = rcar_du_vsp_plane_atomic_duplicate_state,
 	.atomic_destroy_state = rcar_du_vsp_plane_atomic_destroy_state,
-	.atomic_set_property = rcar_du_vsp_plane_atomic_set_property,
-	.atomic_get_property = rcar_du_vsp_plane_atomic_get_property,
 };
 
 int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np,
@@ -438,8 +405,7 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np,
 		if (type == DRM_PLANE_TYPE_PRIMARY)
 			continue;
 
-		drm_object_attach_property(&plane->plane.base,
-					   rcdu->props.alpha, 255);
+		drm_plane_create_alpha_property(&plane->plane, 255);
 		drm_plane_create_zpos_property(&plane->plane, 1, 1,
 					       vsp->num_planes - 1);
 	}
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.h b/drivers/gpu/drm/rcar-du/rcar_du_vsp.h
index f876c512163c..8b19914761e4 100644
--- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.h
+++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.h
@@ -44,7 +44,6 @@ static inline struct rcar_du_vsp_plane *to_rcar_vsp_plane(struct drm_plane *p)
  * @state: base DRM plane state
  * @format: information about the pixel format used by the plane
  * @sg_tables: scatter-gather tables for the frame buffer memory
- * @alpha: value of the plane alpha property
  * @zpos: value of the plane zpos property
  */
 struct rcar_du_vsp_plane_state {
@@ -53,7 +52,6 @@ struct rcar_du_vsp_plane_state {
 	const struct rcar_du_format_info *format;
 	struct sg_table sg_tables[3];
 
-	unsigned int alpha;
 	unsigned int zpos;
 };
 
-- 
git-series 0.9.1

[PATCH v3 4/8] drm/sun4i: backend: Assign the pipes automatically

[Maxime Ripard]

Since we now have a way to enforce the zpos, check for the number of alpha
planes, the only missing part is to assign our pipe automatically instead
of hardcoding it.

The algorithm is quite simple, but requires two iterations over the list of
planes.

In the first one (which is the same one that we've had to check for alpha,
the frontend usage, and so on), we order the planes by their zpos.

We can then do a second iteration over that array by ascending zpos
starting with the pipe 0. When and if we encounter our alpha plane, we put
it and all the other subsequent planes in the second pipe.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/sun4i/sun4i_backend.c | 40 ++++++++++++++++++++++++++--
 drivers/gpu/drm/sun4i/sun4i_layer.c   |  6 +----
 drivers/gpu/drm/sun4i/sun4i_layer.h   |  1 +-
 3 files changed, 39 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
index 245b189fc4d8..f387420bda8d 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.c
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
@@ -275,12 +275,16 @@ int sun4i_backend_update_layer_zpos(struct sun4i_backend *backend, int layer,
 				    struct drm_plane *plane)
 {
 	struct drm_plane_state *state = plane->state;
+	struct sun4i_layer_state *p_state = state_to_sun4i_layer_state(state);
 	unsigned int priority = state->normalized_zpos;
+	unsigned int pipe = p_state->pipe;
 
-	DRM_DEBUG_DRIVER("Setting layer %d's priority to %d\n", layer, priority);
-
+	DRM_DEBUG_DRIVER("Setting layer %d's priority to %d and pipe %d\n",
+			 layer, priority, pipe);
 	regmap_update_bits(backend->engine.regs, SUN4I_BACKEND_ATTCTL_REG0(layer),
+			   SUN4I_BACKEND_ATTCTL_REG0_LAY_PIPESEL_MASK |
 			   SUN4I_BACKEND_ATTCTL_REG0_LAY_PRISEL_MASK,
+			   SUN4I_BACKEND_ATTCTL_REG0_LAY_PIPESEL(p_state->pipe) |
 			   SUN4I_BACKEND_ATTCTL_REG0_LAY_PRISEL(priority));
 
 	return 0;
@@ -325,12 +329,15 @@ static void sun4i_backend_atomic_begin(struct sunxi_engine *engine,
 static int sun4i_backend_atomic_check(struct sunxi_engine *engine,
 				      struct drm_crtc_state *crtc_state)
 {
+	struct drm_plane_state *plane_states[SUN4I_BACKEND_NUM_LAYERS] = { 0 };
 	struct drm_atomic_state *state = crtc_state->state;
 	struct drm_device *drm = state->dev;
 	struct drm_plane *plane;
 	unsigned int num_planes = 0;
 	unsigned int num_alpha_planes = 0;
 	unsigned int num_frontend_planes = 0;
+	unsigned int current_pipe = 0;
+	unsigned int i;
 
 	DRM_DEBUG_DRIVER("Starting checking our planes\n");
 
@@ -361,9 +368,19 @@ static int sun4i_backend_atomic_check(struct sunxi_engine *engine,
 		if (fb->format->has_alpha)
 			num_alpha_planes++;
 
+		DRM_DEBUG_DRIVER("Plane zpos is %d\n",
+				 plane_state->normalized_zpos);
+
+		/* Sort our planes by Zpos */
+		plane_states[plane_state->normalized_zpos] = plane_state;
+
 		num_planes++;
 	}
 
+	/* All our planes were disabled, bail out */
+	if (!num_planes)
+		return 0;
+
 	/*
 	 * The hardware is a bit unusual here.
 	 *
@@ -400,6 +417,25 @@ static int sun4i_backend_atomic_check(struct sunxi_engine *engine,
 		return -EINVAL;
 	}
 
+	/* We can't have an alpha plane at the lowest position */
+	if (plane_states[0]->fb->format->has_alpha)
+		return -EINVAL;
+
+	for (i = 1; i < num_planes; i++) {
+		struct drm_plane_state *p_state = plane_states[i];
+		struct drm_framebuffer *fb = p_state->fb;
+		struct sun4i_layer_state *s_state = state_to_sun4i_layer_state(p_state);
+
+		/*
+		 * The only alpha position is the lowest plane of the
+		 * second pipe.
+		 */
+		if (fb->format->has_alpha)
+			current_pipe++;
+
+		s_state->pipe = current_pipe;
+	}
+
 	if (num_frontend_planes > SUN4I_BACKEND_NUM_FRONTEND_LAYERS) {
 		DRM_DEBUG_DRIVER("Too many planes going through the frontend, rejecting\n");
 		return -EINVAL;
diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c
index 19be798e4fac..3e3d554713cb 100644
--- a/drivers/gpu/drm/sun4i/sun4i_layer.c
+++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
@@ -220,12 +220,6 @@ struct drm_plane **sun4i_layers_init(struct drm_device *drm,
 
 		drm_plane_create_zpos_immutable_property(&layer->plane, i);
 
-		DRM_DEBUG_DRIVER("Assigning %s plane to pipe %d\n",
-				 i ? "overlay" : "primary", plane->pipe);
-		regmap_update_bits(engine->regs, SUN4I_BACKEND_ATTCTL_REG0(i),
-				   SUN4I_BACKEND_ATTCTL_REG0_LAY_PIPESEL_MASK,
-				   SUN4I_BACKEND_ATTCTL_REG0_LAY_PIPESEL(plane->pipe));
-
 		layer->id = i;
 		planes[i] = &layer->plane;
 	};
diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.h b/drivers/gpu/drm/sun4i/sun4i_layer.h
index 75b4868ba87c..36b20265bd31 100644
--- a/drivers/gpu/drm/sun4i/sun4i_layer.h
+++ b/drivers/gpu/drm/sun4i/sun4i_layer.h
@@ -24,6 +24,7 @@ struct sun4i_layer {
 
 struct sun4i_layer_state {
 	struct drm_plane_state	state;
+	unsigned int		pipe;
 	bool			uses_frontend;
 };
 
-- 
git-series 0.9.1

[PATCH v3 4/8] drm/sun4i: backend: Assign the pipes automatically

[Chen-Yu Tsai]

On Sat, Feb 17, 2018 at 1:39 AM, Maxime Ripard
<maxime.ripard@bootlin.com> wrote:
> Since we now have a way to enforce the zpos, check for the number of alpha
> planes, the only missing part is to assign our pipe automatically instead
> of hardcoding it.
>
> The algorithm is quite simple, but requires two iterations over the list of
> planes.
>
> In the first one (which is the same one that we've had to check for alpha,
> the frontend usage, and so on), we order the planes by their zpos.
>
> We can then do a second iteration over that array by ascending zpos
> starting with the pipe 0. When and if we encounter our alpha plane, we put
> it and all the other subsequent planes in the second pipe.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

Reviewed-by: Chen-Yu Tsai <wens@csie.org>

[PATCH v3 5/8] drm/sun4i: Remove the plane description structure

[Maxime Ripard]

The plane description structure was mostly needed to differentiate the
formats usable on the primary plane (because of its lowest position), and
assign the pipes. Now that both are dynamically checked and assigned, we
can remove the static definition.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/sun4i/sun4i_layer.c | 44 +++++-------------------------
 1 file changed, 8 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c
index 3e3d554713cb..85cc7e9461b3 100644
--- a/drivers/gpu/drm/sun4i/sun4i_layer.c
+++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
@@ -19,13 +19,6 @@
 #include "sun4i_layer.h"
 #include "sunxi_engine.h"
 
-struct sun4i_plane_desc {
-	enum drm_plane_type     type;
-	u8                      pipe;
-	const uint32_t          *formats;
-	uint32_t                nformats;
-};
-
 static void sun4i_backend_layer_reset(struct drm_plane *plane)
 {
 	struct sun4i_layer *layer = plane_to_sun4i_layer(plane);
@@ -133,14 +126,7 @@ static const struct drm_plane_funcs sun4i_backend_layer_funcs = {
 	.update_plane		= drm_atomic_helper_update_plane,
 };
 
-static const uint32_t sun4i_backend_layer_formats_primary[] = {
-	DRM_FORMAT_ARGB8888,
-	DRM_FORMAT_RGB888,
-	DRM_FORMAT_RGB565,
-	DRM_FORMAT_XRGB8888,
-};
-
-static const uint32_t sun4i_backend_layer_formats_overlay[] = {
+static const uint32_t sun4i_backend_layer_formats[] = {
 	DRM_FORMAT_ARGB8888,
 	DRM_FORMAT_ARGB4444,
 	DRM_FORMAT_ARGB1555,
@@ -151,24 +137,9 @@ static const uint32_t sun4i_backend_layer_formats_overlay[] = {
 	DRM_FORMAT_XRGB8888,
 };
 
-static const struct sun4i_plane_desc sun4i_backend_planes[] = {
-	{
-		.type = DRM_PLANE_TYPE_PRIMARY,
-		.pipe = 0,
-		.formats = sun4i_backend_layer_formats_primary,
-		.nformats = ARRAY_SIZE(sun4i_backend_layer_formats_primary),
-	},
-	{
-		.type = DRM_PLANE_TYPE_OVERLAY,
-		.pipe = 1,
-		.formats = sun4i_backend_layer_formats_overlay,
-		.nformats = ARRAY_SIZE(sun4i_backend_layer_formats_overlay),
-	},
-};
-
 static struct sun4i_layer *sun4i_layer_init_one(struct drm_device *drm,
 						struct sun4i_backend *backend,
-						const struct sun4i_plane_desc *plane)
+						enum drm_plane_type type)
 {
 	struct sun4i_layer *layer;
 	int ret;
@@ -180,8 +151,9 @@ static struct sun4i_layer *sun4i_layer_init_one(struct drm_device *drm,
 	/* possible crtcs are set later */
 	ret = drm_universal_plane_init(drm, &layer->plane, 0,
 				       &sun4i_backend_layer_funcs,
-				       plane->formats, plane->nformats,
-				       NULL, plane->type, NULL);
+				       sun4i_backend_layer_formats,
+				       ARRAY_SIZE(sun4i_backend_layer_formats),
+				       NULL, type, NULL);
 	if (ret) {
 		dev_err(drm->dev, "Couldn't initialize layer\n");
 		return ERR_PTR(ret);
@@ -207,11 +179,11 @@ struct drm_plane **sun4i_layers_init(struct drm_device *drm,
 	if (!planes)
 		return ERR_PTR(-ENOMEM);
 
-	for (i = 0; i < ARRAY_SIZE(sun4i_backend_planes); i++) {
-		const struct sun4i_plane_desc *plane = &sun4i_backend_planes[i];
+	for (i = 0; i < SUN4I_BACKEND_NUM_LAYERS; i++) {
+		enum drm_plane_type type = i ? DRM_PLANE_TYPE_OVERLAY : DRM_PLANE_TYPE_PRIMARY;
 		struct sun4i_layer *layer;
 
-		layer = sun4i_layer_init_one(drm, backend, plane);
+		layer = sun4i_layer_init_one(drm, backend, type);
 		if (IS_ERR(layer)) {
 			dev_err(drm->dev, "Couldn't initialize %s plane\n",
 				i ? "overlay" : "primary");
-- 
git-series 0.9.1

[PATCH v3 5/8] drm/sun4i: Remove the plane description structure

[Chen-Yu Tsai]

On Sat, Feb 17, 2018 at 1:39 AM, Maxime Ripard
<maxime.ripard@bootlin.com> wrote:
> The plane description structure was mostly needed to differentiate the
> formats usable on the primary plane (because of its lowest position), and
> assign the pipes. Now that both are dynamically checked and assigned, we
> can remove the static definition.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

Reviewed-by: Chen-Yu Tsai <wens@csie.org>

[PATCH v3 6/8] drm/sun4i: backend: Make zpos configurable

[Maxime Ripard]

Now that we have everything in place, we can make zpos configurable now.
Change the zpos property from an immutable one to a regular.

Reviewed-by: Chen-Yu Tsai <wens@csie.org>
Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/sun4i/sun4i_layer.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c
index 85cc7e9461b3..33ad377569ec 100644
--- a/drivers/gpu/drm/sun4i/sun4i_layer.c
+++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
@@ -163,6 +163,9 @@ static struct sun4i_layer *sun4i_layer_init_one(struct drm_device *drm,
 			     &sun4i_backend_layer_helper_funcs);
 	layer->backend = backend;
 
+	drm_plane_create_zpos_property(&layer->plane, 0, 0,
+				       SUN4I_BACKEND_NUM_LAYERS - 1);
+
 	return layer;
 }
 
@@ -190,8 +193,6 @@ struct drm_plane **sun4i_layers_init(struct drm_device *drm,
 			return ERR_CAST(layer);
 		};
 
-		drm_plane_create_zpos_immutable_property(&layer->plane, i);
-
 		layer->id = i;
 		planes[i] = &layer->plane;
 	};
-- 
git-series 0.9.1

[PATCH v3 7/8] drm/sun4i: Add support for plane alpha

[Maxime Ripard]

Our backend supports a per-plane alpha property. Support it through our new
helper.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/sun4i/sun4i_backend.c | 16 +++++++++++++---
 drivers/gpu/drm/sun4i/sun4i_backend.h |  3 +++
 drivers/gpu/drm/sun4i/sun4i_layer.c   |  2 ++
 3 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
index f387420bda8d..0ec00600eace 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.c
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
@@ -191,6 +191,15 @@ int sun4i_backend_update_layer_formats(struct sun4i_backend *backend,
 	DRM_DEBUG_DRIVER("Switching display backend interlaced mode %s\n",
 			 interlaced ? "on" : "off");
 
+	val = SUN4I_BACKEND_ATTCTL_REG0_LAY_GLBALPHA(state->alpha);
+	if (state->alpha != 255)
+		val |= SUN4I_BACKEND_ATTCTL_REG0_LAY_GLBALPHA_EN;
+	regmap_update_bits(backend->engine.regs,
+			   SUN4I_BACKEND_ATTCTL_REG0(layer),
+			   SUN4I_BACKEND_ATTCTL_REG0_LAY_GLBALPHA_MASK |
+			   SUN4I_BACKEND_ATTCTL_REG0_LAY_GLBALPHA_EN,
+			   val);
+
 	ret = sun4i_backend_drm_format_to_layer(plane, fb->format->format,
 						&val);
 	if (ret) {
@@ -365,7 +374,7 @@ static int sun4i_backend_atomic_check(struct sunxi_engine *engine,
 		DRM_DEBUG_DRIVER("Plane FB format is %s\n",
 				 drm_get_format_name(fb->format->format,
 						     &format_name));
-		if (fb->format->has_alpha)
+		if (fb->format->has_alpha || (plane_state->alpha != 255))
 			num_alpha_planes++;
 
 		DRM_DEBUG_DRIVER("Plane zpos is %d\n",
@@ -418,7 +427,8 @@ static int sun4i_backend_atomic_check(struct sunxi_engine *engine,
 	}
 
 	/* We can't have an alpha plane at the lowest position */
-	if (plane_states[0]->fb->format->has_alpha)
+	if (plane_states[0]->fb->format->has_alpha ||
+	    (plane_states[0]->alpha != 255))
 		return -EINVAL;
 
 	for (i = 1; i < num_planes; i++) {
@@ -430,7 +440,7 @@ static int sun4i_backend_atomic_check(struct sunxi_engine *engine,
 		 * The only alpha position is the lowest plane of the
 		 * second pipe.
 		 */
-		if (fb->format->has_alpha)
+		if (fb->format->has_alpha || (p_state->alpha != 255))
 			current_pipe++;
 
 		s_state->pipe = current_pipe;
diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.h b/drivers/gpu/drm/sun4i/sun4i_backend.h
index 52e77591186a..03294d5dd1a2 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.h
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.h
@@ -68,11 +68,14 @@
 #define SUN4I_BACKEND_CKMIN_REG			0x884
 #define SUN4I_BACKEND_CKCFG_REG			0x888
 #define SUN4I_BACKEND_ATTCTL_REG0(l)		(0x890 + (0x4 * (l)))
+#define SUN4I_BACKEND_ATTCTL_REG0_LAY_GLBALPHA_MASK	GENMASK(31, 24)
+#define SUN4I_BACKEND_ATTCTL_REG0_LAY_GLBALPHA(x)		((x) << 24)
 #define SUN4I_BACKEND_ATTCTL_REG0_LAY_PIPESEL_MASK	BIT(15)
 #define SUN4I_BACKEND_ATTCTL_REG0_LAY_PIPESEL(x)		((x) << 15)
 #define SUN4I_BACKEND_ATTCTL_REG0_LAY_PRISEL_MASK	GENMASK(11, 10)
 #define SUN4I_BACKEND_ATTCTL_REG0_LAY_PRISEL(x)			((x) << 10)
 #define SUN4I_BACKEND_ATTCTL_REG0_LAY_VDOEN		BIT(1)
+#define SUN4I_BACKEND_ATTCTL_REG0_LAY_GLBALPHA_EN	BIT(0)
 
 #define SUN4I_BACKEND_ATTCTL_REG1(l)		(0x8a0 + (0x4 * (l)))
 #define SUN4I_BACKEND_ATTCTL_REG1_LAY_HSCAFCT		GENMASK(15, 14)
diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c
index 33ad377569ec..f491fd963534 100644
--- a/drivers/gpu/drm/sun4i/sun4i_layer.c
+++ b/drivers/gpu/drm/sun4i/sun4i_layer.c
@@ -37,6 +37,7 @@ static void sun4i_backend_layer_reset(struct drm_plane *plane)
 	if (state) {
 		plane->state = &state->state;
 		plane->state->plane = plane;
+		plane->state->alpha = 255;
 		plane->state->zpos = layer->id;
 	}
 }
@@ -163,6 +164,7 @@ static struct sun4i_layer *sun4i_layer_init_one(struct drm_device *drm,
 			     &sun4i_backend_layer_helper_funcs);
 	layer->backend = backend;
 
+	drm_plane_create_alpha_property(&layer->plane, 255);
 	drm_plane_create_zpos_property(&layer->plane, 0, 0,
 				       SUN4I_BACKEND_NUM_LAYERS - 1);
 
-- 
git-series 0.9.1

[PATCH v3 7/8] drm/sun4i: Add support for plane alpha

[Chen-Yu Tsai]

On Sat, Feb 17, 2018 at 1:39 AM, Maxime Ripard
<maxime.ripard@bootlin.com> wrote:
> Our backend supports a per-plane alpha property. Support it through our new
> helper.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

Reviewed-by: Chen-Yu Tsai <wens@csie.org>

Though, not having a graphics background, does alpha = 255 mean fully
transparent and thus can be skipped? Or no alpha at all?

Thanks

[PATCH v3 7/8] drm/sun4i: Add support for plane alpha

[Maxime Ripard]

On Thu, Feb 22, 2018 at 10:17:38PM +0800, Chen-Yu Tsai wrote:
> On Sat, Feb 17, 2018 at 1:39 AM, Maxime Ripard
> <maxime.ripard@bootlin.com> wrote:
> > Our backend supports a per-plane alpha property. Support it through our new
> > helper.
> >
> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> 
> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
> 
> Though, not having a graphics background, does alpha = 255 mean fully
> transparent and thus can be skipped? Or no alpha at all?

It means that it's fully opaque, and therefore we are in the same
situation than if we had no alpha channel at all.

Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180222/d903d0bf/attachment.sig>

[PATCH v3 7/8] drm/sun4i: Add support for plane alpha

[Chen-Yu Tsai]

On Thu, Feb 22, 2018 at 10:34 PM, Maxime Ripard
<maxime.ripard@bootlin.com> wrote:
> On Thu, Feb 22, 2018 at 10:17:38PM +0800, Chen-Yu Tsai wrote:
>> On Sat, Feb 17, 2018 at 1:39 AM, Maxime Ripard
>> <maxime.ripard@bootlin.com> wrote:
>> > Our backend supports a per-plane alpha property. Support it through our new
>> > helper.
>> >
>> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
>>
>> Reviewed-by: Chen-Yu Tsai <wens@csie.org>
>>
>> Though, not having a graphics background, does alpha = 255 mean fully
>> transparent and thus can be skipped? Or no alpha at all?
>
> It means that it's fully opaque, and therefore we are in the same
> situation than if we had no alpha channel at all.

Right. Then the code makes sense. Thanks for the explanation.

[PATCH v3 8/8] drm/sun4i: backend: Remove ARGB spoofing

[Maxime Ripard]

We've had some code for quite some time to prevent the alpha bug from
happening on the lowest primary plane. Since we now check for this in our
atomic_check, we can simply remove it.

Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
---
 drivers/gpu/drm/sun4i/sun4i_backend.c | 12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c
index 0ec00600eace..ad2d39f6ba20 100644
--- a/drivers/gpu/drm/sun4i/sun4i_backend.c
+++ b/drivers/gpu/drm/sun4i/sun4i_backend.c
@@ -92,13 +92,8 @@ void sun4i_backend_layer_enable(struct sun4i_backend *backend,
 			   SUN4I_BACKEND_MODCTL_LAY_EN(layer), val);
 }
 
-static int sun4i_backend_drm_format_to_layer(struct drm_plane *plane,
-					     u32 format, u32 *mode)
+static int sun4i_backend_drm_format_to_layer(u32 format, u32 *mode)
 {
-	if (plane && (plane->type == DRM_PLANE_TYPE_PRIMARY) &&
-	    (format == DRM_FORMAT_ARGB8888))
-		format = DRM_FORMAT_XRGB8888;
-
 	switch (format) {
 	case DRM_FORMAT_ARGB8888:
 		*mode = SUN4I_BACKEND_LAY_FBFMT_ARGB8888;
@@ -200,8 +195,7 @@ int sun4i_backend_update_layer_formats(struct sun4i_backend *backend,
 			   SUN4I_BACKEND_ATTCTL_REG0_LAY_GLBALPHA_EN,
 			   val);
 
-	ret = sun4i_backend_drm_format_to_layer(plane, fb->format->format,
-						&val);
+	ret = sun4i_backend_drm_format_to_layer(fb->format->format, &val);
 	if (ret) {
 		DRM_DEBUG_DRIVER("Invalid format\n");
 		return ret;
@@ -220,7 +214,7 @@ int sun4i_backend_update_layer_frontend(struct sun4i_backend *backend,
 	u32 val;
 	int ret;
 
-	ret = sun4i_backend_drm_format_to_layer(NULL, fmt, &val);
+	ret = sun4i_backend_drm_format_to_layer(fmt, &val);
 	if (ret) {
 		DRM_DEBUG_DRIVER("Invalid format\n");
 		return ret;
-- 
git-series 0.9.1

[PATCH v3 8/8] drm/sun4i: backend: Remove ARGB spoofing

[Chen-Yu Tsai]

On Sat, Feb 17, 2018 at 1:39 AM, Maxime Ripard
<maxime.ripard@bootlin.com> wrote:
> We've had some code for quite some time to prevent the alpha bug from
> happening on the lowest primary plane. Since we now check for this in our
> atomic_check, we can simply remove it.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>

Reviewed-by: Chen-Yu Tsai <wens@csie.org>

[PATCH v3 8/8] drm/sun4i: backend: Remove ARGB spoofing

[Maxime Ripard]

On Thu, Feb 22, 2018 at 10:15:07PM +0800, Chen-Yu Tsai wrote:
> On Sat, Feb 17, 2018 at 1:39 AM, Maxime Ripard
> <maxime.ripard@bootlin.com> wrote:
> > We've had some code for quite some time to prevent the alpha bug from
> > happening on the lowest primary plane. Since we now check for this in our
> > atomic_check, we can simply remove it.
> >
> > Signed-off-by: Maxime Ripard <maxime.ripard@bootlin.com>
> 
> Reviewed-by: Chen-Yu Tsai <wens@csie.org>

Applied the patches 4, 5, 6 and 8 with your reviewed-by.

Thanks!
Maxime

-- 
Maxime Ripard, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
https://bootlin.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20180222/f31d1447/attachment.sig>