| Message ID | 5c765fc730d75cb362dc37bcdb3b3aeacc7bdb30.1515494838.git-series.maxime.ripard@free-electrons.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; > };
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
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; > };
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
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
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
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
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 >
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.
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
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 --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; };
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(-)