diff mbox

[06/19] drm/blend: Add a generic alpha property

Message ID 5c765fc730d75cb362dc37bcdb3b3aeacc7bdb30.1515494838.git-series.maxime.ripard@free-electrons.com (mailing list archive)
State New, archived
Headers show

Commit Message

Maxime Ripard Jan. 9, 2018, 10:56 a.m. UTC
Some drivers duplicate the logic to create a property to store a per-plane
alpha.

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

Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 Documentation/gpu/kms-properties.csv |  2 +-
 drivers/gpu/drm/drm_atomic.c         |  4 ++++-
 drivers/gpu/drm/drm_atomic_helper.c  |  1 +-
 drivers/gpu/drm/drm_blend.c          | 32 +++++++++++++++++++++++++++++-
 include/drm/drm_blend.h              |  1 +-
 include/drm/drm_plane.h              |  6 +++++-
 6 files changed, 45 insertions(+), 1 deletion(-)

Comments

Boris BREZILLON Jan. 9, 2018, 12:31 p.m. UTC | #1
On Tue,  9 Jan 2018 11:56:25 +0100
Maxime Ripard <maxime.ripard@free-electrons.com> wrote:

> Some drivers duplicate the logic to create a property to store a per-plane
> alpha.
> 
> Let's create a helper in order to move that to the core.
> 
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>

Reviewed-by: Boris Brezillon <boris.brezillon@free-electrons.com>

> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  Documentation/gpu/kms-properties.csv |  2 +-
>  drivers/gpu/drm/drm_atomic.c         |  4 ++++-
>  drivers/gpu/drm/drm_atomic_helper.c  |  1 +-
>  drivers/gpu/drm/drm_blend.c          | 32 +++++++++++++++++++++++++++++-
>  include/drm/drm_blend.h              |  1 +-
>  include/drm/drm_plane.h              |  6 +++++-
>  6 files changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/gpu/kms-properties.csv b/Documentation/gpu/kms-properties.csv
> index 927b65e14219..a3c3969c1992 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=255",Plane,Opacity of the plane from transparent (0) to opaque (255)
>  ,,"""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 c2da5585e201..ade18cf62c89 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -749,6 +749,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;
> @@ -810,6 +812,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 71d712f1b56a..018993df4c18 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3372,6 +3372,7 @@ void drm_atomic_helper_plane_reset(struct drm_plane *plane)
>  
>  	if (plane->state) {
>  		plane->state->plane = plane;
> +		plane->state->alpha = 255;
>  		plane->state->rotation = DRM_MODE_ROTATE_0;
>  	}
>  }
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index 2e5e089dd912..8eea2a8af458 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
> + * @alpha: initial value of alpha, from 0 (transparent) to 255 (opaque)
> + *
> + * This function initializes a generic, mutable, alpha property and
> + * enables support for it in the DRM core.
> + *
> + * Drivers can then attach this property to their plane to enable
> + * support for configurable plane alpha.
> + *
> + * Returns:
> + * 0 on success, negative error code on failure.
> + */
> +int drm_plane_create_alpha_property(struct drm_plane *plane, u8 alpha)
> +{
> +	struct drm_property *prop;
> +
> +	prop = drm_property_create_range(plane->dev, 0, "alpha", 0, 255);
> +	if (!prop)
> +		return -ENOMEM;
> +
> +	drm_object_attach_property(&plane->base, prop, alpha);
> +	plane->alpha_property = prop;
> +
> +	if (plane->state)
> +		plane->state->alpha = 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..5979a8fce453 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, u8 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 571615079230..a5e26064b132 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
> @@ -546,6 +551,7 @@ struct drm_plane {
>  	 */
>  	struct drm_plane_state *state;
>  
> +	struct drm_property *alpha_property;
>  	struct drm_property *zpos_property;
>  	struct drm_property *rotation_property;
>  };
Daniel Vetter Jan. 9, 2018, 12:32 p.m. UTC | #2
On Tue, Jan 09, 2018 at 11:56:25AM +0100, Maxime Ripard wrote:
> Some drivers duplicate the logic to create a property to store a per-plane
> alpha.
> 
> Let's create a helper in order to move that to the core.
> 
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

Do we have userspace for this? Is encoding a fixed 0-255 range really the
best idea?

I know other drivers have skimped on the rules here a bit ... But at least
internally (i.e. within the drm_plane_state) we probably should restrict
ourselves to u8. And this needs real docs (i.e. the full blend equation
drivers are supposed to implement).
-Daniel

> ---
>  Documentation/gpu/kms-properties.csv |  2 +-
>  drivers/gpu/drm/drm_atomic.c         |  4 ++++-
>  drivers/gpu/drm/drm_atomic_helper.c  |  1 +-
>  drivers/gpu/drm/drm_blend.c          | 32 +++++++++++++++++++++++++++++-
>  include/drm/drm_blend.h              |  1 +-
>  include/drm/drm_plane.h              |  6 +++++-
>  6 files changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/gpu/kms-properties.csv b/Documentation/gpu/kms-properties.csv
> index 927b65e14219..a3c3969c1992 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=255",Plane,Opacity of the plane from transparent (0) to opaque (255)
>  ,,"""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 c2da5585e201..ade18cf62c89 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -749,6 +749,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;
> @@ -810,6 +812,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 71d712f1b56a..018993df4c18 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3372,6 +3372,7 @@ void drm_atomic_helper_plane_reset(struct drm_plane *plane)
>  
>  	if (plane->state) {
>  		plane->state->plane = plane;
> +		plane->state->alpha = 255;
>  		plane->state->rotation = DRM_MODE_ROTATE_0;
>  	}
>  }
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index 2e5e089dd912..8eea2a8af458 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
> + * @alpha: initial value of alpha, from 0 (transparent) to 255 (opaque)
> + *
> + * This function initializes a generic, mutable, alpha property and
> + * enables support for it in the DRM core.
> + *
> + * Drivers can then attach this property to their plane to enable
> + * support for configurable plane alpha.
> + *
> + * Returns:
> + * 0 on success, negative error code on failure.
> + */
> +int drm_plane_create_alpha_property(struct drm_plane *plane, u8 alpha)
> +{
> +	struct drm_property *prop;
> +
> +	prop = drm_property_create_range(plane->dev, 0, "alpha", 0, 255);
> +	if (!prop)
> +		return -ENOMEM;
> +
> +	drm_object_attach_property(&plane->base, prop, alpha);
> +	plane->alpha_property = prop;
> +
> +	if (plane->state)
> +		plane->state->alpha = 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..5979a8fce453 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, u8 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 571615079230..a5e26064b132 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
> @@ -546,6 +551,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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
Laurent Pinchart Jan. 9, 2018, 12:34 p.m. UTC | #3
Hi Maxime,

Thank you for the patch.

On Tuesday, 9 January 2018 12:56:25 EET Maxime Ripard wrote:
> Some drivers duplicate the logic to create a property to store a per-plane
> alpha.
> 
> Let's create a helper in order to move that to the core.
> 
> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  Documentation/gpu/kms-properties.csv |  2 +-
>  drivers/gpu/drm/drm_atomic.c         |  4 ++++-
>  drivers/gpu/drm/drm_atomic_helper.c  |  1 +-
>  drivers/gpu/drm/drm_blend.c          | 32 +++++++++++++++++++++++++++++-
>  include/drm/drm_blend.h              |  1 +-
>  include/drm/drm_plane.h              |  6 +++++-
>  6 files changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/gpu/kms-properties.csv
> b/Documentation/gpu/kms-properties.csv index 927b65e14219..a3c3969c1992
> 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=255",Plane,Opacity of the plane from
> transparent (0) to opaque (255) ,,"""colorkey""",RANGE,"Min=0,
> Max=0x01ffffff",Plane,TBD

I think more documentation is needed. You should explain how the property 
operates and which formats it is applicable to. For instance you need to 
clarify what happens for format that contain an alpha component.

> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index c2da5585e201..ade18cf62c89 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -749,6 +749,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;
> @@ -810,6 +812,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 71d712f1b56a..018993df4c18
> 100644
> --- a/drivers/gpu/drm/drm_atomic_helper.c
> +++ b/drivers/gpu/drm/drm_atomic_helper.c
> @@ -3372,6 +3372,7 @@ void drm_atomic_helper_plane_reset(struct drm_plane
> *plane)
> 
>  	if (plane->state) {
>  		plane->state->plane = plane;
> +		plane->state->alpha = 255;

If you keep the ability to select an initial value other than fully opaque 
(see my comment below about that) you should reset to that value instead of 
hardcoding 255.

>  		plane->state->rotation = DRM_MODE_ROTATE_0;
>  	}
>  }
> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index 2e5e089dd912..8eea2a8af458 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
> + * @alpha: initial value of alpha, from 0 (transparent) to 255 (opaque)

Do you have a use case for initializing the alpha value to something else than 
fully opaque ?

> + * This function initializes a generic, mutable, alpha property and
> + * enables support for it in the DRM core.
> + *
> + * Drivers can then attach this property to their plane to enable
> + * support for configurable plane alpha.

The function attaches the property to the plane, is the documentation outdated 
?

> + * Returns:
> + * 0 on success, negative error code on failure.
> + */
> +int drm_plane_create_alpha_property(struct drm_plane *plane, u8 alpha)
> +{
> +	struct drm_property *prop;
> +
> +	prop = drm_property_create_range(plane->dev, 0, "alpha", 0, 255);

Do you think the 0-255 range will fit all use cases ?

> +	if (!prop)
> +		return -ENOMEM;
> +
> +	drm_object_attach_property(&plane->base, prop, alpha);
> +	plane->alpha_property = prop;
> +
> +	if (plane->state)
> +		plane->state->alpha = 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..5979a8fce453 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, u8 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 571615079230..a5e26064b132 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
> @@ -546,6 +551,7 @@ struct drm_plane {
>  	 */
>  	struct drm_plane_state *state;
> 
> +	struct drm_property *alpha_property;
>  	struct drm_property *zpos_property;
>  	struct drm_property *rotation_property;
>  };
Maxime Ripard Jan. 9, 2018, 1:53 p.m. UTC | #4
On Tue, Jan 09, 2018 at 01:32:41PM +0100, Daniel Vetter wrote:
> On Tue, Jan 09, 2018 at 11:56:25AM +0100, Maxime Ripard wrote:
> > Some drivers duplicate the logic to create a property to store a per-plane
> > alpha.
> > 
> > Let's create a helper in order to move that to the core.
> > 
> > Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> 
> Do we have userspace for this?

Wayland seems to be on its way to implement this, with ChromeOS using
it:
https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741.html

and more specifically:
https://chromium.googlesource.com/chromium/src/+/master/third_party/wayland-protocols/unstable/alpha-compositing/alpha-compositing-unstable-v1.xml#118

> Is encoding a fixed 0-255 range really the best idea?

I don't really know, is there hardware or formats where there is more
than 255? Or did you mean less than that?

> I know other drivers have skimped on the rules here a bit ... But at least
> internally (i.e. within the drm_plane_state) we probably should restrict
> ourselves to u8. And this needs real docs (i.e. the full blend equation
> drivers are supposed to implement).

You mean straight vs premultiplied? Maybe we should implement this as
an additional property in read only depending on how the hardware
behaves?

Thanks!
Maxime
Maxime Ripard Jan. 9, 2018, 1:59 p.m. UTC | #5
Hi Laurent,

On Tue, Jan 09, 2018 at 02:34:47PM +0200, Laurent Pinchart wrote:
> > Some drivers duplicate the logic to create a property to store a per-plane
> > alpha.
> > 
> > Let's create a helper in order to move that to the core.
> > 
> > Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > ---
> >  Documentation/gpu/kms-properties.csv |  2 +-
> >  drivers/gpu/drm/drm_atomic.c         |  4 ++++-
> >  drivers/gpu/drm/drm_atomic_helper.c  |  1 +-
> >  drivers/gpu/drm/drm_blend.c          | 32 +++++++++++++++++++++++++++++-
> >  include/drm/drm_blend.h              |  1 +-
> >  include/drm/drm_plane.h              |  6 +++++-
> >  6 files changed, 45 insertions(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/gpu/kms-properties.csv
> > b/Documentation/gpu/kms-properties.csv index 927b65e14219..a3c3969c1992
> > 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=255",Plane,Opacity of the plane from
> > transparent (0) to opaque (255) ,,"""colorkey""",RANGE,"Min=0,
> > Max=0x01ffffff",Plane,TBD
> 
> I think more documentation is needed. You should explain how the property 
> operates and which formats it is applicable to. For instance you need to 
> clarify what happens for format that contain an alpha component.

That's a very good point, and I'm not quite sure what should happen in
the first place :)

Should we forbid that configuration?

> >  /**
> > + * drm_plane_create_alpha_property - create a new alpha property
> > + * @plane: drm plane
> > + * @alpha: initial value of alpha, from 0 (transparent) to 255 (opaque)
> 
> Do you have a use case for initializing the alpha value to something else than 
> fully opaque ?

I don't, but thought it might be useful. Maybe it isn't, in which case
I'm totally fine with it getting away.

> > + * This function initializes a generic, mutable, alpha property and
> > + * enables support for it in the DRM core.
> > + *
> > + * Drivers can then attach this property to their plane to enable
> > + * support for configurable plane alpha.
> 
> The function attaches the property to the plane, is the
> documentation outdated ?

Yes, I'll fix it.

> > + * Returns:
> > + * 0 on success, negative error code on failure.
> > + */
> > +int drm_plane_create_alpha_property(struct drm_plane *plane, u8 alpha)
> > +{
> > +	struct drm_property *prop;
> > +
> > +	prop = drm_property_create_range(plane->dev, 0, "alpha", 0, 255);
> 
> Do you think the 0-255 range will fit all use cases ?

This is kind of the same discussion we're having with Daniel, but are
there hardware or formats with an alpha component wider than 255?

Thanks!
Maxime
Daniel Vetter Jan. 9, 2018, 2:28 p.m. UTC | #6
On Tue, Jan 09, 2018 at 02:53:22PM +0100, Maxime Ripard wrote:
> On Tue, Jan 09, 2018 at 01:32:41PM +0100, Daniel Vetter wrote:
> > On Tue, Jan 09, 2018 at 11:56:25AM +0100, Maxime Ripard wrote:
> > > Some drivers duplicate the logic to create a property to store a per-plane
> > > alpha.
> > > 
> > > Let's create a helper in order to move that to the core.
> > > 
> > > Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > 
> > Do we have userspace for this?
> 
> Wayland seems to be on its way to implement this, with ChromeOS using
> it:
> https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741.html
> 
> and more specifically:
> https://chromium.googlesource.com/chromium/src/+/master/third_party/wayland-protocols/unstable/alpha-compositing/alpha-compositing-unstable-v1.xml#118

Yay, would be good to include these links in the patch description. Really
happy we're having a real standard now used by multiple people.

> > Is encoding a fixed 0-255 range really the best idea?
> 
> I don't really know, is there hardware or formats where there is more
> than 255? Or did you mean less than that?

30bit I'd assume wants more alpha. In the past we've done some fixed-point
stuff (e.g. for LUT), using the 0.0-1.0 float range. Using that for the
blend equation docs is also what I recommend (and that we map from 0-255
to 0.0-1.0 logically). Ofc the hw might not do any of that ... I think
0.16 fixed point, stored in a u16 is probably best. That's what we're
doing for gamma tables already, and that way drivers can simply throw away
the lower bits.

> > I know other drivers have skimped on the rules here a bit ... But at least
> > internally (i.e. within the drm_plane_state) we probably should restrict
> > ourselves to u8. And this needs real docs (i.e. the full blend equation
> > drivers are supposed to implement).
> 
> You mean straight vs premultiplied? Maybe we should implement this as
> an additional property in read only depending on how the hardware
> behaves?

No need for an additional property right now, but definitely document
whether you mean straight or pre-multiplied. Just writing down the blend
equation is probably best.
-Daniel
Maxime Ripard Jan. 11, 2018, 3:58 p.m. UTC | #7
On Tue, Jan 09, 2018 at 03:28:34PM +0100, Daniel Vetter wrote:
> On Tue, Jan 09, 2018 at 02:53:22PM +0100, Maxime Ripard wrote:
> > On Tue, Jan 09, 2018 at 01:32:41PM +0100, Daniel Vetter wrote:
> > > On Tue, Jan 09, 2018 at 11:56:25AM +0100, Maxime Ripard wrote:
> > > > Some drivers duplicate the logic to create a property to store a per-plane
> > > > alpha.
> > > > 
> > > > Let's create a helper in order to move that to the core.
> > > > 
> > > > Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > 
> > > Do we have userspace for this?
> > 
> > Wayland seems to be on its way to implement this, with ChromeOS using
> > it:
> > https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741.html
> > 
> > and more specifically:
> > https://chromium.googlesource.com/chromium/src/+/master/third_party/wayland-protocols/unstable/alpha-compositing/alpha-compositing-unstable-v1.xml#118
> 
> Yay, would be good to include these links in the patch description. Really
> happy we're having a real standard now used by multiple people.

I will.

> > > Is encoding a fixed 0-255 range really the best idea?
> > 
> > I don't really know, is there hardware or formats where there is more
> > than 255? Or did you mean less than that?
> 
> 30bit I'd assume wants more alpha. In the past we've done some fixed-point
> stuff (e.g. for LUT), using the 0.0-1.0 float range. Using that for the
> blend equation docs is also what I recommend (and that we map from 0-255
> to 0.0-1.0 logically). Ofc the hw might not do any of that ... I think
> 0.16 fixed point, stored in a u16 is probably best. That's what we're
> doing for gamma tables already, and that way drivers can simply throw away
> the lower bits.

But that would also break the two users of that property that won't be
able to move to the generic property (with the same name) without
breaking userspace. The point of that patch was to allow some code
consolidation, and that would mean failing to do so here :/

> > > I know other drivers have skimped on the rules here a bit ... But at least
> > > internally (i.e. within the drm_plane_state) we probably should restrict
> > > ourselves to u8. And this needs real docs (i.e. the full blend equation
> > > drivers are supposed to implement).
> > 
> > You mean straight vs premultiplied? Maybe we should implement this as
> > an additional property in read only depending on how the hardware
> > behaves?
> 
> No need for an additional property right now, but definitely document
> whether you mean straight or pre-multiplied. Just writing down the blend
> equation is probably best.

Ack.

Thanks!
Maxime
Daniel Vetter Jan. 11, 2018, 4:36 p.m. UTC | #8
On Thu, Jan 11, 2018 at 4:58 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> On Tue, Jan 09, 2018 at 03:28:34PM +0100, Daniel Vetter wrote:
>> On Tue, Jan 09, 2018 at 02:53:22PM +0100, Maxime Ripard wrote:
>> > On Tue, Jan 09, 2018 at 01:32:41PM +0100, Daniel Vetter wrote:
>> > > On Tue, Jan 09, 2018 at 11:56:25AM +0100, Maxime Ripard wrote:
>> > > > Some drivers duplicate the logic to create a property to store a per-plane
>> > > > alpha.
>> > > >
>> > > > Let's create a helper in order to move that to the core.
>> > > >
>> > > > Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
>> > > > Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>> > > > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>> > >
>> > > Do we have userspace for this?
>> >
>> > Wayland seems to be on its way to implement this, with ChromeOS using
>> > it:
>> > https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741.html
>> >
>> > and more specifically:
>> > https://chromium.googlesource.com/chromium/src/+/master/third_party/wayland-protocols/unstable/alpha-compositing/alpha-compositing-unstable-v1.xml#118
>>
>> Yay, would be good to include these links in the patch description. Really
>> happy we're having a real standard now used by multiple people.
>
> I will.
>
>> > > Is encoding a fixed 0-255 range really the best idea?
>> >
>> > I don't really know, is there hardware or formats where there is more
>> > than 255? Or did you mean less than that?
>>
>> 30bit I'd assume wants more alpha. In the past we've done some fixed-point
>> stuff (e.g. for LUT), using the 0.0-1.0 float range. Using that for the
>> blend equation docs is also what I recommend (and that we map from 0-255
>> to 0.0-1.0 logically). Ofc the hw might not do any of that ... I think
>> 0.16 fixed point, stored in a u16 is probably best. That's what we're
>> doing for gamma tables already, and that way drivers can simply throw away
>> the lower bits.
>
> But that would also break the two users of that property that won't be
> able to move to the generic property (with the same name) without
> breaking userspace. The point of that patch was to allow some code
> consolidation, and that would mean failing to do so here :/

Let me try to clarify:
- We'll keep the exact existing property semantics with the 0-255
range for the userspace visible property.

- But internally, in the decode value that we store into
drm_plane_state, we'll do the slightly more future proof thing with a
few more bits.

That gives us the option of exposing those bits in the future without
having to change all the drivers again (which we have to do for this
series here already anyway, since the decoded value moves into
drm_plane_state from driver subclasses).

Definitely not asking to break userspace here :-)

Cheers, Daniel

>> > > I know other drivers have skimped on the rules here a bit ... But at least
>> > > internally (i.e. within the drm_plane_state) we probably should restrict
>> > > ourselves to u8. And this needs real docs (i.e. the full blend equation
>> > > drivers are supposed to implement).
>> >
>> > You mean straight vs premultiplied? Maybe we should implement this as
>> > an additional property in read only depending on how the hardware
>> > behaves?
>>
>> No need for an additional property right now, but definitely document
>> whether you mean straight or pre-multiplied. Just writing down the blend
>> equation is probably best.
>
> Ack.
>
> Thanks!
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com
>
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
Laurent Pinchart Jan. 11, 2018, 6:34 p.m. UTC | #9
Hi Daniel,

On Thursday, 11 January 2018 18:36:10 EET Daniel Vetter wrote:
> On Thu, Jan 11, 2018 at 4:58 PM, Maxime Ripard wrote:
> > On Tue, Jan 09, 2018 at 03:28:34PM +0100, Daniel Vetter wrote:
> >> On Tue, Jan 09, 2018 at 02:53:22PM +0100, Maxime Ripard wrote:
> >>> On Tue, Jan 09, 2018 at 01:32:41PM +0100, Daniel Vetter wrote:
> >>>> On Tue, Jan 09, 2018 at 11:56:25AM +0100, Maxime Ripard wrote:
> >>>>> Some drivers duplicate the logic to create a property to store a
> >>>>> per-plane alpha.
> >>>>> 
> >>>>> Let's create a helper in order to move that to the core.
> >>>>> 
> >>>>> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> >>>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >>>>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> >>>> 
> >>>> Do we have userspace for this?
> >>> 
> >>> Wayland seems to be on its way to implement this, with ChromeOS using
> >>> it:
> >>> https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741
> >>> .html
> >>> 
> >>> and more specifically:
> >>> https://chromium.googlesource.com/chromium/src/+/master/third_party/way
> >>> land-protocols/unstable/alpha-compositing/alpha-compositing-unstable-v1
> >>> .xml#118
> >> 
> >> Yay, would be good to include these links in the patch description.
> >> Really happy we're having a real standard now used by multiple people.
> > 
> > I will.
> > 
> >> > > Is encoding a fixed 0-255 range really the best idea?
> >> > 
> >> > I don't really know, is there hardware or formats where there is more
> >> > than 255? Or did you mean less than that?
> >> 
> >> 30bit I'd assume wants more alpha. In the past we've done some
> >> fixed-point stuff (e.g. for LUT), using the 0.0-1.0 float range. Using
> >> that for the blend equation docs is also what I recommend (and that we
> >> map from 0-255 to 0.0-1.0 logically). Ofc the hw might not do any of that
> >> ... I think 0.16 fixed point, stored in a u16 is probably best. That's
> >> what we're doing for gamma tables already, and that way drivers can
> >> simply throw away the lower bits.
> > 
> > But that would also break the two users of that property that won't be
> > able to move to the generic property (with the same name) without
> > breaking userspace. The point of that patch was to allow some code
> > consolidation, and that would mean failing to do so here :/
> 
> Let me try to clarify:
> - We'll keep the exact existing property semantics with the 0-255
> range for the userspace visible property.
> 
> - But internally, in the decode value that we store into
> drm_plane_state, we'll do the slightly more future proof thing with a
> few more bits.
> 
> That gives us the option of exposing those bits in the future without
> having to change all the drivers again (which we have to do for this
> series here already anyway, since the decoded value moves into
> drm_plane_state from driver subclasses).
> 
> Definitely not asking to break userspace here :-)

As the property range is available to userspace, we could allow drivers to 
expose a driver-dependent number of bits for the alpha value without breaking 
anything. We can even require new drivers to expose 16 bits regardless of the 
number of bits that the hardware can handle, or we could keep that driver-
specific.

Please note, however, that U0.16 can only represent [0.0, 0.999984741...] 
while we need [0.0, 1.0]. As all the hardware I know map the full range of 
values ([0, 255] for 8-bit for instance) to [0.0, 1.0] I wouldn't pretend that 
the 16-bit value exposed to userspace is U0.16.

> >>>> I know other drivers have skimped on the rules here a bit ... But at
> >>>> least internally (i.e. within the drm_plane_state) we probably should
> >>>> restrict ourselves to u8. And this needs real docs (i.e. the full blend
> >>>> equation drivers are supposed to implement).
> >>> 
> >>> You mean straight vs premultiplied? Maybe we should implement this as
> >>> an additional property in read only depending on how the hardware
> >>> behaves?
> >> 
> >> No need for an additional property right now, but definitely document
> >> whether you mean straight or pre-multiplied. Just writing down the blend
> >> equation is probably best.
Maxime Ripard Jan. 17, 2018, 9:20 a.m. UTC | #10
On Thu, Jan 11, 2018 at 08:34:58PM +0200, Laurent Pinchart wrote:
> Hi Daniel,
> 
> On Thursday, 11 January 2018 18:36:10 EET Daniel Vetter wrote:
> > On Thu, Jan 11, 2018 at 4:58 PM, Maxime Ripard wrote:
> > > On Tue, Jan 09, 2018 at 03:28:34PM +0100, Daniel Vetter wrote:
> > >> On Tue, Jan 09, 2018 at 02:53:22PM +0100, Maxime Ripard wrote:
> > >>> On Tue, Jan 09, 2018 at 01:32:41PM +0100, Daniel Vetter wrote:
> > >>>> On Tue, Jan 09, 2018 at 11:56:25AM +0100, Maxime Ripard wrote:
> > >>>>> Some drivers duplicate the logic to create a property to store a
> > >>>>> per-plane alpha.
> > >>>>> 
> > >>>>> Let's create a helper in order to move that to the core.
> > >>>>> 
> > >>>>> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> > >>>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > >>>>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > >>>> 
> > >>>> Do we have userspace for this?
> > >>> 
> > >>> Wayland seems to be on its way to implement this, with ChromeOS using
> > >>> it:
> > >>> https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741
> > >>> .html
> > >>> 
> > >>> and more specifically:
> > >>> https://chromium.googlesource.com/chromium/src/+/master/third_party/way
> > >>> land-protocols/unstable/alpha-compositing/alpha-compositing-unstable-v1
> > >>> .xml#118
> > >> 
> > >> Yay, would be good to include these links in the patch description.
> > >> Really happy we're having a real standard now used by multiple people.
> > > 
> > > I will.
> > > 
> > >> > > Is encoding a fixed 0-255 range really the best idea?
> > >> > 
> > >> > I don't really know, is there hardware or formats where there is more
> > >> > than 255? Or did you mean less than that?
> > >> 
> > >> 30bit I'd assume wants more alpha. In the past we've done some
> > >> fixed-point stuff (e.g. for LUT), using the 0.0-1.0 float range. Using
> > >> that for the blend equation docs is also what I recommend (and that we
> > >> map from 0-255 to 0.0-1.0 logically). Ofc the hw might not do any of that
> > >> ... I think 0.16 fixed point, stored in a u16 is probably best. That's
> > >> what we're doing for gamma tables already, and that way drivers can
> > >> simply throw away the lower bits.
> > > 
> > > But that would also break the two users of that property that won't be
> > > able to move to the generic property (with the same name) without
> > > breaking userspace. The point of that patch was to allow some code
> > > consolidation, and that would mean failing to do so here :/
> > 
> > Let me try to clarify:
> > - We'll keep the exact existing property semantics with the 0-255
> > range for the userspace visible property.
> > 
> > - But internally, in the decode value that we store into
> > drm_plane_state, we'll do the slightly more future proof thing with a
> > few more bits.
> > 
> > That gives us the option of exposing those bits in the future without
> > having to change all the drivers again (which we have to do for this
> > series here already anyway, since the decoded value moves into
> > drm_plane_state from driver subclasses).
> > 
> > Definitely not asking to break userspace here :-)
> 
> As the property range is available to userspace, we could allow drivers to 
> expose a driver-dependent number of bits for the alpha value without breaking 
> anything. We can even require new drivers to expose 16 bits regardless of the 
> number of bits that the hardware can handle, or we could keep that driver-
> specific.
> 
> Please note, however, that U0.16 can only represent [0.0, 0.999984741...] 
> while we need [0.0, 1.0]. As all the hardware I know map the full range of 
> values ([0, 255] for 8-bit for instance) to [0.0, 1.0] I wouldn't pretend that 
> the 16-bit value exposed to userspace is U0.16.

So this would involve not changing too much the current patch, but
instead extending the type from an u8 to an u16. Would that work for
you Daniel?

Thanks!
Maxime
Daniel Vetter Jan. 17, 2018, 9:30 a.m. UTC | #11
On Wed, Jan 17, 2018 at 10:20:24AM +0100, Maxime Ripard wrote:
> On Thu, Jan 11, 2018 at 08:34:58PM +0200, Laurent Pinchart wrote:
> > Hi Daniel,
> > 
> > On Thursday, 11 January 2018 18:36:10 EET Daniel Vetter wrote:
> > > On Thu, Jan 11, 2018 at 4:58 PM, Maxime Ripard wrote:
> > > > On Tue, Jan 09, 2018 at 03:28:34PM +0100, Daniel Vetter wrote:
> > > >> On Tue, Jan 09, 2018 at 02:53:22PM +0100, Maxime Ripard wrote:
> > > >>> On Tue, Jan 09, 2018 at 01:32:41PM +0100, Daniel Vetter wrote:
> > > >>>> On Tue, Jan 09, 2018 at 11:56:25AM +0100, Maxime Ripard wrote:
> > > >>>>> Some drivers duplicate the logic to create a property to store a
> > > >>>>> per-plane alpha.
> > > >>>>> 
> > > >>>>> Let's create a helper in order to move that to the core.
> > > >>>>> 
> > > >>>>> Cc: Boris Brezillon <boris.brezillon@free-electrons.com>
> > > >>>>> Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > > >>>>> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> > > >>>> 
> > > >>>> Do we have userspace for this?
> > > >>> 
> > > >>> Wayland seems to be on its way to implement this, with ChromeOS using
> > > >>> it:
> > > >>> https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741
> > > >>> .html
> > > >>> 
> > > >>> and more specifically:
> > > >>> https://chromium.googlesource.com/chromium/src/+/master/third_party/way
> > > >>> land-protocols/unstable/alpha-compositing/alpha-compositing-unstable-v1
> > > >>> .xml#118
> > > >> 
> > > >> Yay, would be good to include these links in the patch description.
> > > >> Really happy we're having a real standard now used by multiple people.
> > > > 
> > > > I will.
> > > > 
> > > >> > > Is encoding a fixed 0-255 range really the best idea?
> > > >> > 
> > > >> > I don't really know, is there hardware or formats where there is more
> > > >> > than 255? Or did you mean less than that?
> > > >> 
> > > >> 30bit I'd assume wants more alpha. In the past we've done some
> > > >> fixed-point stuff (e.g. for LUT), using the 0.0-1.0 float range. Using
> > > >> that for the blend equation docs is also what I recommend (and that we
> > > >> map from 0-255 to 0.0-1.0 logically). Ofc the hw might not do any of that
> > > >> ... I think 0.16 fixed point, stored in a u16 is probably best. That's
> > > >> what we're doing for gamma tables already, and that way drivers can
> > > >> simply throw away the lower bits.
> > > > 
> > > > But that would also break the two users of that property that won't be
> > > > able to move to the generic property (with the same name) without
> > > > breaking userspace. The point of that patch was to allow some code
> > > > consolidation, and that would mean failing to do so here :/
> > > 
> > > Let me try to clarify:
> > > - We'll keep the exact existing property semantics with the 0-255
> > > range for the userspace visible property.
> > > 
> > > - But internally, in the decode value that we store into
> > > drm_plane_state, we'll do the slightly more future proof thing with a
> > > few more bits.
> > > 
> > > That gives us the option of exposing those bits in the future without
> > > having to change all the drivers again (which we have to do for this
> > > series here already anyway, since the decoded value moves into
> > > drm_plane_state from driver subclasses).
> > > 
> > > Definitely not asking to break userspace here :-)
> > 
> > As the property range is available to userspace, we could allow drivers to 
> > expose a driver-dependent number of bits for the alpha value without breaking 
> > anything. We can even require new drivers to expose 16 bits regardless of the 
> > number of bits that the hardware can handle, or we could keep that driver-
> > specific.
> > 
> > Please note, however, that U0.16 can only represent [0.0, 0.999984741...] 
> > while we need [0.0, 1.0]. As all the hardware I know map the full range of 
> > values ([0, 255] for 8-bit for instance) to [0.0, 1.0] I wouldn't pretend that 
> > the 16-bit value exposed to userspace is U0.16.
> 
> So this would involve not changing too much the current patch, but
> instead extending the type from an u8 to an u16. Would that work for
> you Daniel?

Yeah, Laurent's clarification is what I've meant ... And hopefully it's
enough future-proofing that we don't need to redo this all again.
-Daniel
diff mbox

Patch

diff --git a/Documentation/gpu/kms-properties.csv b/Documentation/gpu/kms-properties.csv
index 927b65e14219..a3c3969c1992 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=255",Plane,Opacity of the plane from transparent (0) to opaque (255)
 ,,"""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 c2da5585e201..ade18cf62c89 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -749,6 +749,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;
@@ -810,6 +812,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 71d712f1b56a..018993df4c18 100644
--- a/drivers/gpu/drm/drm_atomic_helper.c
+++ b/drivers/gpu/drm/drm_atomic_helper.c
@@ -3372,6 +3372,7 @@  void drm_atomic_helper_plane_reset(struct drm_plane *plane)
 
 	if (plane->state) {
 		plane->state->plane = plane;
+		plane->state->alpha = 255;
 		plane->state->rotation = DRM_MODE_ROTATE_0;
 	}
 }
diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
index 2e5e089dd912..8eea2a8af458 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
+ * @alpha: initial value of alpha, from 0 (transparent) to 255 (opaque)
+ *
+ * This function initializes a generic, mutable, alpha property and
+ * enables support for it in the DRM core.
+ *
+ * Drivers can then attach this property to their plane to enable
+ * support for configurable plane alpha.
+ *
+ * Returns:
+ * 0 on success, negative error code on failure.
+ */
+int drm_plane_create_alpha_property(struct drm_plane *plane, u8 alpha)
+{
+	struct drm_property *prop;
+
+	prop = drm_property_create_range(plane->dev, 0, "alpha", 0, 255);
+	if (!prop)
+		return -ENOMEM;
+
+	drm_object_attach_property(&plane->base, prop, alpha);
+	plane->alpha_property = prop;
+
+	if (plane->state)
+		plane->state->alpha = 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..5979a8fce453 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, u8 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 571615079230..a5e26064b132 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
@@ -546,6 +551,7 @@  struct drm_plane {
 	 */
 	struct drm_plane_state *state;
 
+	struct drm_property *alpha_property;
 	struct drm_property *zpos_property;
 	struct drm_property *rotation_property;
 };