Hi,
This serie aims at enhancing the support for our planes in the current drm driver on the first generation of Allwinner's display engine.
This also introduces a few generic stuff, as well as some conversion for some other drivers.
This series basically implements three things that look orthogonal, but due to the way the hardware works are kind of related.
The main feature is that instead of implementing 2 planes per backend, we are now able to use the planes that are available in hardware. This was unsupported before because of the way the composition works in the hardware.
Indeed, the planes are first grouped into 2 pipes that are doing a basic composition, in case of overlapping planes, it just takes whatever plane has the highest priority (=> zpos). Then, the alpha blending is done between the two pipes. This was simplified so far by only using two planes, one for each pipe, which was allowing us to have an illusion of proper alpha blending. This is further complicated by the bug/feature that the lowest plane must not have any alpha at all, otherwise the pixel will turn black, no matter what the value of alpha is. This basically means that we can have a plane with alpha only in the second pipe.
However, as we have more and more blocks being worked on, 2 planes are getting really limited and we need to support all 4 of them.
This is mostly possible by extending our atomic_check and to make sure that we enforce those constraints, and assign the pipes automatically. This is done by looking at the number of planes using an alpha component, and we then end up in various scenarios: - 0 plane with alpha => we don't care for the pipes at all. All the planes are assigned to the first pipe - 1 plane with alpha => we assign all the planes without alpha below the plane with alpha to the first pipe, and then all the remaining planes to the second pipe. The plane with alpha will be the lowest plane on that pipe, which means that whatever plane is above it will have precedence, but the alpha component will remain and will be used on pixels that are not overlapping - 2-4 planes with alpha => we can't operate that way, we just reject the configuration.
In addition to the formats that embed an alpha component, we also add support for plane-wide alpha property, and in order to tweak the configuration the way we want to, we also add support for configurable zpos.
Let me know what you think, Maxime
Changes from v2: - Rebased on current drm-misc-next - Removed the patches already applied - Split the patch implementing the automatic pipe assignment in two
Changes from v1: - Document the behaviour on concurrent usage of the alpha property and an alpha component in the format - Allowed for higher alpha values - Moved the alpha value from a helper to the struct drm_format_info - Collected tags - Rebased on current drm-misc-next
Maxime Ripard (8): drm/blend: Add a generic alpha property drm/atmel-hclcdc: Convert to the new generic alpha property drm/rcar-du: Convert to the new generic alpha property drm/sun4i: backend: Assign the pipes automatically drm/sun4i: Remove the plane description structure drm/sun4i: backend: Make zpos configurable drm/sun4i: Add support for plane alpha drm/sun4i: backend: Remove ARGB spoofing
Documentation/gpu/kms-properties.csv | 2 +- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h | 13 +--- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 89 ++---------------- drivers/gpu/drm/drm_atomic.c | 4 +- drivers/gpu/drm/drm_atomic_helper.c | 4 +- drivers/gpu/drm/drm_blend.c | 32 ++++++- drivers/gpu/drm/rcar-du/rcar_du_drv.h | 1 +- drivers/gpu/drm/rcar-du/rcar_du_kms.c | 5 +- drivers/gpu/drm/rcar-du/rcar_du_plane.c | 15 +-- drivers/gpu/drm/rcar-du/rcar_du_plane.h | 2 +- drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 42 +-------- drivers/gpu/drm/rcar-du/rcar_du_vsp.h | 2 +- drivers/gpu/drm/sun4i/sun4i_backend.c | 64 ++++++++++--- drivers/gpu/drm/sun4i/sun4i_backend.h | 3 +- drivers/gpu/drm/sun4i/sun4i_layer.c | 57 ++---------- drivers/gpu/drm/sun4i/sun4i_layer.h | 1 +- include/drm/drm_blend.h | 1 +- include/drm/drm_plane.h | 6 +- 18 files changed, 140 insertions(+), 203 deletions(-)
base-commit: 5db520e36bca3e1e1575ab14ca7dfb23b3d68407
Some drivers duplicate the logic to create a property to store a per-plane alpha.
This is especially useful if we ever want to support extra protocols for Wayland like: https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741.html
Let's create a helper in order to move that to the core.
Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Reviewed-by: Boris Brezillon boris.brezillon@bootlin.com Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com --- Documentation/gpu/kms-properties.csv | 2 +- drivers/gpu/drm/drm_atomic.c | 4 ++++- drivers/gpu/drm/drm_atomic_helper.c | 4 ++++- drivers/gpu/drm/drm_blend.c | 32 +++++++++++++++++++++++++++++- include/drm/drm_blend.h | 1 +- include/drm/drm_plane.h | 6 +++++- 6 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/Documentation/gpu/kms-properties.csv b/Documentation/gpu/kms-properties.csv index 927b65e14219..25ad3503d663 100644 --- a/Documentation/gpu/kms-properties.csv +++ b/Documentation/gpu/kms-properties.csv @@ -99,5 +99,5 @@ radeon,DVI-I,“coherent”,RANGE,"Min=0, Max=1",Connector,TBD ,,"""underscan vborder""",RANGE,"Min=0, Max=128",Connector,TBD ,Audio,“audio”,ENUM,"{ ""off"", ""on"", ""auto"" }",Connector,TBD ,FMT Dithering,“dither”,ENUM,"{ ""off"", ""on"" }",Connector,TBD -rcar-du,Generic,"""alpha""",RANGE,"Min=0, Max=255",Plane,TBD +,,"""alpha""",RANGE,"Min=0, Max=Driver dependant",Plane,Opacity of the plane from transparent (0) to fully opaque (MAX). If this property is set to a value different than max, and that the pixel will define an alpha component, the property will have precendance and the pixel value will be ignored. The alpha value is represented as straight alpha, ie the colors haven't been pre-adjusted for their opacity by multiplication. Therefore, the equation to get a color value for one pixel, assuming two planes A and B, will be (color_a * alpha_a + color_b * alpha_b * (MAX - alpha_a) / MAX) / (alpha_a + alpha_b * (MAX - alpha_a) / MAX) ,,"""colorkey""",RANGE,"Min=0, Max=0x01ffffff",Plane,TBD diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 7d9ad20040a1..3defc56a1ef2 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -753,6 +753,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, state->src_w = val; } else if (property == config->prop_src_h) { state->src_h = val; + } else if (property == plane->alpha_property) { + state->alpha = val; } else if (property == plane->rotation_property) { if (!is_power_of_2(val & DRM_MODE_ROTATE_MASK)) return -EINVAL; @@ -814,6 +816,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane, *val = state->src_w; } else if (property == config->prop_src_h) { *val = state->src_h; + } else if (property == plane->alpha_property) { + *val = state->alpha; } else if (property == plane->rotation_property) { *val = state->rotation; } else if (property == plane->zpos_property) { diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ae3cbfe9e01c..2b88f593aab4 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3482,6 +3482,10 @@ void drm_atomic_helper_plane_reset(struct drm_plane *plane) if (plane->state) { plane->state->plane = plane; plane->state->rotation = DRM_MODE_ROTATE_0; + + /* Reset the alpha value to fully opaque if it matters */ + if (plane->alpha_property) + plane->state->alpha = plane->alpha_property->values[1]; } } EXPORT_SYMBOL(drm_atomic_helper_plane_reset); diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c index 4c62dff14893..a5dea7cbed2c 100644 --- a/drivers/gpu/drm/drm_blend.c +++ b/drivers/gpu/drm/drm_blend.c @@ -104,6 +104,38 @@ */
/** + * drm_plane_create_alpha_property - create a new alpha property + * @plane: drm plane + * @max_alpha: maximum value of alpha + * + * This function initializes a generic, mutable, alpha property and + * enables support for it in the DRM core. + * + * The alpha property will be allowed to be within the bounds of 0 + * (transparent) to @max_alpha (opaque) + * + * Returns: + * 0 on success, negative error code on failure. + */ +int drm_plane_create_alpha_property(struct drm_plane *plane, u16 max_alpha) +{ + struct drm_property *prop; + + prop = drm_property_create_range(plane->dev, 0, "alpha", 0, max_alpha); + if (!prop) + return -ENOMEM; + + drm_object_attach_property(&plane->base, prop, max_alpha); + plane->alpha_property = prop; + + if (plane->state) + plane->state->alpha = max_alpha; + + return 0; +} +EXPORT_SYMBOL(drm_plane_create_alpha_property); + +/** * drm_plane_create_rotation_property - create a new rotation property * @plane: drm plane * @rotation: initial value of the rotation property diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h index 17606026590b..e5affba6ebde 100644 --- a/include/drm/drm_blend.h +++ b/include/drm/drm_blend.h @@ -36,6 +36,7 @@ static inline bool drm_rotation_90_or_270(unsigned int rotation) return rotation & (DRM_MODE_ROTATE_90 | DRM_MODE_ROTATE_270); }
+int drm_plane_create_alpha_property(struct drm_plane *plane, u16 alpha); int drm_plane_create_rotation_property(struct drm_plane *plane, unsigned int rotation, unsigned int supported_rotations); diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 8185e3468a23..5a6f29524f12 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -42,6 +42,7 @@ struct drm_modeset_acquire_ctx; * plane (in 16.16) * @src_w: width of visible portion of plane (in 16.16) * @src_h: height of visible portion of plane (in 16.16) + * @alpha: opacity of the plane * @rotation: rotation of the plane * @zpos: priority of the given plane on crtc (optional) * Note that multiple active planes on the same crtc can have an identical @@ -105,6 +106,9 @@ struct drm_plane_state { uint32_t src_x, src_y; uint32_t src_h, src_w;
+ /* Plane opacity */ + u8 alpha; + /* Plane rotation */ unsigned int rotation;
@@ -481,6 +485,7 @@ enum drm_plane_type { * @funcs: helper functions * @properties: property tracking for this plane * @type: type of plane (overlay, primary, cursor) + * @alpha_property: alpha property for this plane * @zpos_property: zpos property for this plane * @rotation_property: rotation property for this plane * @helper_private: mid-layer private data @@ -556,6 +561,7 @@ struct drm_plane { */ struct drm_plane_state *state;
+ struct drm_property *alpha_property; struct drm_property *zpos_property; struct drm_property *rotation_property; };
On Fri, Feb 16, 2018 at 06:39:29PM +0100, Maxime Ripard wrote:
Some drivers duplicate the logic to create a property to store a per-plane alpha.
This is especially useful if we ever want to support extra protocols for Wayland like: https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741.html
Let's create a helper in order to move that to the core.
Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Reviewed-by: Boris Brezillon boris.brezillon@bootlin.com Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com
Documentation/gpu/kms-properties.csv | 2 +- drivers/gpu/drm/drm_atomic.c | 4 ++++- drivers/gpu/drm/drm_atomic_helper.c | 4 ++++- drivers/gpu/drm/drm_blend.c | 32 +++++++++++++++++++++++++++++- include/drm/drm_blend.h | 1 +- include/drm/drm_plane.h | 6 +++++- 6 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/Documentation/gpu/kms-properties.csv b/Documentation/gpu/kms-properties.csv index 927b65e14219..25ad3503d663 100644 --- a/Documentation/gpu/kms-properties.csv +++ b/Documentation/gpu/kms-properties.csv @@ -99,5 +99,5 @@ radeon,DVI-I,“coherent”,RANGE,"Min=0, Max=1",Connector,TBD ,,"""underscan vborder""",RANGE,"Min=0, Max=128",Connector,TBD ,Audio,“audio”,ENUM,"{ ""off"", ""on"", ""auto"" }",Connector,TBD ,FMT Dithering,“dither”,ENUM,"{ ""off"", ""on"" }",Connector,TBD -rcar-du,Generic,"""alpha""",RANGE,"Min=0, Max=255",Plane,TBD +,,"""alpha""",RANGE,"Min=0, Max=Driver dependant",Plane,Opacity of the plane from transparent (0) to fully opaque (MAX). If this property is set to a value different than max, and that the pixel will define an alpha component, the property will have precendance and the pixel value will be ignored.
Those semantics don't seem particularly good to me. I think we would want the per-pixel alpha and global alpha both to be effecive at the same time. You can always decide to ignore the per-pixel alpha by using a pixel format without alpha.
Also, where's the userspace that wants this feature?
<snip>
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 8185e3468a23..5a6f29524f12 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -42,6 +42,7 @@ struct drm_modeset_acquire_ctx;
- plane (in 16.16)
- @src_w: width of visible portion of plane (in 16.16)
- @src_h: height of visible portion of plane (in 16.16)
- @alpha: opacity of the plane
- @rotation: rotation of the plane
- @zpos: priority of the given plane on crtc (optional)
- Note that multiple active planes on the same crtc can have an identical
@@ -105,6 +106,9 @@ struct drm_plane_state { uint32_t src_x, src_y; uint32_t src_h, src_w;
- /* Plane opacity */
- u8 alpha;
We may want to make that u16. The general we expect 16bpc for most color related things, but since this is a range prop I suppose we should just expose the actual hardware range. But making it u16 might avoid some head scratching for the first person to have hardware with higher precision. Either that or we should make the prop creation fail if the driver asks for more bits than we have in the state.
Oh, and you should plug this into the state dumper as well.
- /* Plane rotation */ unsigned int rotation;
@@ -481,6 +485,7 @@ enum drm_plane_type {
- @funcs: helper functions
- @properties: property tracking for this plane
- @type: type of plane (overlay, primary, cursor)
- @alpha_property: alpha property for this plane
- @zpos_property: zpos property for this plane
- @rotation_property: rotation property for this plane
- @helper_private: mid-layer private data
@@ -556,6 +561,7 @@ struct drm_plane { */ struct drm_plane_state *state;
- struct drm_property *alpha_property; struct drm_property *zpos_property; struct drm_property *rotation_property;
};
git-series 0.9.1 _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Hi Ville,
On Friday, 16 February 2018 20:20:41 EET Ville Syrjälä wrote:
On Fri, Feb 16, 2018 at 06:39:29PM +0100, Maxime Ripard wrote:
Some drivers duplicate the logic to create a property to store a per-plane alpha.
This is especially useful if we ever want to support extra protocols for Wayland like: https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741.ht ml
Let's create a helper in order to move that to the core.
Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Reviewed-by: Boris Brezillon boris.brezillon@bootlin.com Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com
Documentation/gpu/kms-properties.csv | 2 +- drivers/gpu/drm/drm_atomic.c | 4 ++++- drivers/gpu/drm/drm_atomic_helper.c | 4 ++++- drivers/gpu/drm/drm_blend.c | 32 +++++++++++++++++++++++++++++- include/drm/drm_blend.h | 1 +- include/drm/drm_plane.h | 6 +++++- 6 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/Documentation/gpu/kms-properties.csv b/Documentation/gpu/kms-properties.csv index 927b65e14219..25ad3503d663 100644 --- a/Documentation/gpu/kms-properties.csv +++ b/Documentation/gpu/kms-properties.csv @@ -99,5 +99,5 @@ radeon,DVI-I,“coherent”,RANGE,"Min=0, Max=1",Connector,TBD> ,,"""underscan vborder""",RANGE,"Min=0, Max=128",Connector,TBD ,Audio,“audio”,ENUM,"{ ""off"", ""on"", ""auto"" }",Connector,TBD ,FMT Dithering,“dither”,ENUM,"{ ""off"", ""on"" }",Connector,TBD
-rcar-du,Generic,"""alpha""",RANGE,"Min=0, Max=255",Plane,TBD +,,"""alpha""",RANGE,"Min=0, Max=Driver dependant",Plane,Opacity of the plane from transparent (0) to fully opaque (MAX). If this property is set to a value different than max, and that the pixel will define an alpha component, the property will have precendance and the pixel value will be ignored.
Those semantics don't seem particularly good to me. I think we would want the per-pixel alpha and global alpha both to be effecive at the same time. You can always decide to ignore the per-pixel alpha by using a pixel format without alpha.
That makes sense to me. However, it also brings a new question: how should a driver that supports either global alpha or pixel alpha but not both signal that to userspace, and how should it reacts when userspace selects a format with an alpha channel and set a global alpha value other than fully opaque ? To make things more complex, note that some drivers support combining global alpha and pixel alpha only for a subset of the formats with an alpha channel (for instance for ARGB 1555 formats, but not for ARGB 8888 formats).
Also, where's the userspace that wants this feature?
<snip>
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 8185e3468a23..5a6f29524f12 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -42,6 +42,7 @@ struct drm_modeset_acquire_ctx;
- plane (in 16.16)
- @src_w: width of visible portion of plane (in 16.16)
- @src_h: height of visible portion of plane (in 16.16)
identical
- @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
@@ -105,6 +106,9 @@ struct drm_plane_state { uint32_t src_x, src_y; uint32_t src_h, src_w;
- /* Plane opacity */
- u8 alpha;
We may want to make that u16. The general we expect 16bpc for most color related things, but since this is a range prop I suppose we should just expose the actual hardware range. But making it u16 might avoid some head scratching for the first person to have hardware with higher precision. Either that or we should make the prop creation fail if the driver asks for more bits than we have in the state.
I'm tempted to go one step further and always make the alpha property 16-bits wide for new users (we can't do so for existing users as it could break userspace), and let drivers convert that internally to the range they need. There could however be drawbacks I don't foresee.
Oh, and you should plug this into the state dumper as well.
/* Plane rotation */ unsigned int rotation;
@@ -481,6 +485,7 @@ enum drm_plane_type {
- @funcs: helper functions
- @properties: property tracking for this plane
- @type: type of plane (overlay, primary, cursor)
- @alpha_property: alpha property for this plane
- @zpos_property: zpos property for this plane
- @rotation_property: rotation property for this plane
- @helper_private: mid-layer private data
@@ -556,6 +561,7 @@ struct drm_plane { */
struct drm_plane_state *state;
- struct drm_property *alpha_property; struct drm_property *zpos_property; struct drm_property *rotation_property;
};
On Mon, Feb 19, 2018 at 9:19 PM, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Ville,
On Friday, 16 February 2018 20:20:41 EET Ville Syrjälä wrote:
On Fri, Feb 16, 2018 at 06:39:29PM +0100, Maxime Ripard wrote:
Some drivers duplicate the logic to create a property to store a per-plane alpha.
This is especially useful if we ever want to support extra protocols for Wayland like: https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741.ht ml
Let's create a helper in order to move that to the core.
Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Reviewed-by: Boris Brezillon boris.brezillon@bootlin.com Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com
Documentation/gpu/kms-properties.csv | 2 +- drivers/gpu/drm/drm_atomic.c | 4 ++++- drivers/gpu/drm/drm_atomic_helper.c | 4 ++++- drivers/gpu/drm/drm_blend.c | 32 +++++++++++++++++++++++++++++- include/drm/drm_blend.h | 1 +- include/drm/drm_plane.h | 6 +++++- 6 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/Documentation/gpu/kms-properties.csv b/Documentation/gpu/kms-properties.csv index 927b65e14219..25ad3503d663 100644 --- a/Documentation/gpu/kms-properties.csv +++ b/Documentation/gpu/kms-properties.csv @@ -99,5 +99,5 @@ radeon,DVI-I,“coherent”,RANGE,"Min=0, Max=1",Connector,TBD> ,,"""underscan vborder""",RANGE,"Min=0, Max=128",Connector,TBD ,Audio,“audio”,ENUM,"{ ""off"", ""on"", ""auto"" }",Connector,TBD ,FMT Dithering,“dither”,ENUM,"{ ""off"", ""on"" }",Connector,TBD
-rcar-du,Generic,"""alpha""",RANGE,"Min=0, Max=255",Plane,TBD +,,"""alpha""",RANGE,"Min=0, Max=Driver dependant",Plane,Opacity of the plane from transparent (0) to fully opaque (MAX). If this property is set to a value different than max, and that the pixel will define an alpha component, the property will have precendance and the pixel value will be ignored.
Please don't document new properties in that csv file, it's an unreadable mess. Instead follow how we document standardized properties nowadays in full-blown sections. For plane blending we have:
https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-prop...
Those semantics don't seem particularly good to me. I think we would want the per-pixel alpha and global alpha both to be effecive at the same time. You can always decide to ignore the per-pixel alpha by using a pixel format without alpha.
That makes sense to me. However, it also brings a new question: how should a driver that supports either global alpha or pixel alpha but not both signal that to userspace, and how should it reacts when userspace selects a format with an alpha channel and set a global alpha value other than fully opaque ? To make things more complex, note that some drivers support combining global alpha and pixel alpha only for a subset of the formats with an alpha channel (for instance for ARGB 1555 formats, but not for ARGB 8888 formats).
atomic_check can reject unsupported configs. Userspace needs to fall back somehow (either switch to xrgb or make alpha fully opaque or just give up on that plane). We have a lot of such corner-cases we don't tell userspace about explicitly at all.
Also, where's the userspace that wants this feature?
<snip>
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 8185e3468a23..5a6f29524f12 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -42,6 +42,7 @@ struct drm_modeset_acquire_ctx;
- plane (in 16.16)
- @src_w: width of visible portion of plane (in 16.16)
- @src_h: height of visible portion of plane (in 16.16)
identical
- @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
@@ -105,6 +106,9 @@ struct drm_plane_state { uint32_t src_x, src_y; uint32_t src_h, src_w;
- /* Plane opacity */
- u8 alpha;
We may want to make that u16. The general we expect 16bpc for most color related things, but since this is a range prop I suppose we should just expose the actual hardware range. But making it u16 might avoid some head scratching for the first person to have hardware with higher precision. Either that or we should make the prop creation fail if the driver asks for more bits than we have in the state.
I'm tempted to go one step further and always make the alpha property 16-bits wide for new users (we can't do so for existing users as it could break userspace), and let drivers convert that internally to the range they need. There could however be drawbacks I don't foresee.
I think scaling the range to match the hw is the most sensible (yes I'm flip-flopping around here). And once someone needs more than u8, we can extend the internal representation easily. The external representation in the property is an u64, that /should/ be enough for the next few years :-) -Daniel
Oh, and you should plug this into the state dumper as well.
/* Plane rotation */ unsigned int rotation;
@@ -481,6 +485,7 @@ enum drm_plane_type {
- @funcs: helper functions
- @properties: property tracking for this plane
- @type: type of plane (overlay, primary, cursor)
- @alpha_property: alpha property for this plane
- @zpos_property: zpos property for this plane
- @rotation_property: rotation property for this plane
- @helper_private: mid-layer private data
@@ -556,6 +561,7 @@ struct drm_plane { */
struct drm_plane_state *state;
- struct drm_property *alpha_property; struct drm_property *zpos_property; struct drm_property *rotation_property;
};
-- Regards,
Laurent Pinchart
dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
On Mon, Feb 19, 2018 at 10:58:40PM +0100, Daniel Vetter wrote:
On Mon, Feb 19, 2018 at 9:19 PM, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Ville,
On Friday, 16 February 2018 20:20:41 EET Ville Syrjälä wrote:
On Fri, Feb 16, 2018 at 06:39:29PM +0100, Maxime Ripard wrote:
Some drivers duplicate the logic to create a property to store a per-plane alpha.
This is especially useful if we ever want to support extra protocols for Wayland like: https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741.ht ml
Let's create a helper in order to move that to the core.
Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Reviewed-by: Boris Brezillon boris.brezillon@bootlin.com Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com
Documentation/gpu/kms-properties.csv | 2 +- drivers/gpu/drm/drm_atomic.c | 4 ++++- drivers/gpu/drm/drm_atomic_helper.c | 4 ++++- drivers/gpu/drm/drm_blend.c | 32 +++++++++++++++++++++++++++++- include/drm/drm_blend.h | 1 +- include/drm/drm_plane.h | 6 +++++- 6 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/Documentation/gpu/kms-properties.csv b/Documentation/gpu/kms-properties.csv index 927b65e14219..25ad3503d663 100644 --- a/Documentation/gpu/kms-properties.csv +++ b/Documentation/gpu/kms-properties.csv @@ -99,5 +99,5 @@ radeon,DVI-I,“coherent”,RANGE,"Min=0, Max=1",Connector,TBD> ,,"""underscan vborder""",RANGE,"Min=0, Max=128",Connector,TBD ,Audio,“audio”,ENUM,"{ ""off"", ""on"", ""auto"" }",Connector,TBD ,FMT Dithering,“dither”,ENUM,"{ ""off"", ""on"" }",Connector,TBD
-rcar-du,Generic,"""alpha""",RANGE,"Min=0, Max=255",Plane,TBD +,,"""alpha""",RANGE,"Min=0, Max=Driver dependant",Plane,Opacity of the plane from transparent (0) to fully opaque (MAX). If this property is set to a value different than max, and that the pixel will define an alpha component, the property will have precendance and the pixel value will be ignored.
Please don't document new properties in that csv file, it's an unreadable mess. Instead follow how we document standardized properties nowadays in full-blown sections. For plane blending we have:
https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-prop...
Ack
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 8185e3468a23..5a6f29524f12 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -42,6 +42,7 @@ struct drm_modeset_acquire_ctx;
- plane (in 16.16)
- @src_w: width of visible portion of plane (in 16.16)
- @src_h: height of visible portion of plane (in 16.16)
identical
- @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
@@ -105,6 +106,9 @@ struct drm_plane_state { uint32_t src_x, src_y; uint32_t src_h, src_w;
- /* Plane opacity */
- u8 alpha;
We may want to make that u16. The general we expect 16bpc for most color related things, but since this is a range prop I suppose we should just expose the actual hardware range. But making it u16 might avoid some head scratching for the first person to have hardware with higher precision. Either that or we should make the prop creation fail if the driver asks for more bits than we have in the state.
I'm tempted to go one step further and always make the alpha property 16-bits wide for new users (we can't do so for existing users as it could break userspace), and let drivers convert that internally to the range they need. There could however be drawbacks I don't foresee.
I think scaling the range to match the hw is the most sensible (yes I'm flip-flopping around here). And once someone needs more than u8, we can extend the internal representation easily. The external representation in the property is an u64, that /should/ be enough for the next few years :-)
Just to make sure we're on the same page, you want to keep the u8, and if the hardware uses say an u16, the driver for that hardware will do the upscaling?
Thanks! Maxime
On Wed, Feb 21, 2018 at 02:07:57PM +0100, Maxime Ripard wrote:
On Mon, Feb 19, 2018 at 10:58:40PM +0100, Daniel Vetter wrote:
On Mon, Feb 19, 2018 at 9:19 PM, Laurent Pinchart laurent.pinchart@ideasonboard.com wrote:
Hi Ville,
On Friday, 16 February 2018 20:20:41 EET Ville Syrjälä wrote:
On Fri, Feb 16, 2018 at 06:39:29PM +0100, Maxime Ripard wrote:
Some drivers duplicate the logic to create a property to store a per-plane alpha.
This is especially useful if we ever want to support extra protocols for Wayland like: https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741.ht ml
Let's create a helper in order to move that to the core.
Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Reviewed-by: Boris Brezillon boris.brezillon@bootlin.com Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com
Documentation/gpu/kms-properties.csv | 2 +- drivers/gpu/drm/drm_atomic.c | 4 ++++- drivers/gpu/drm/drm_atomic_helper.c | 4 ++++- drivers/gpu/drm/drm_blend.c | 32 +++++++++++++++++++++++++++++- include/drm/drm_blend.h | 1 +- include/drm/drm_plane.h | 6 +++++- 6 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/Documentation/gpu/kms-properties.csv b/Documentation/gpu/kms-properties.csv index 927b65e14219..25ad3503d663 100644 --- a/Documentation/gpu/kms-properties.csv +++ b/Documentation/gpu/kms-properties.csv @@ -99,5 +99,5 @@ radeon,DVI-I,“coherent”,RANGE,"Min=0, Max=1",Connector,TBD> ,,"""underscan vborder""",RANGE,"Min=0, Max=128",Connector,TBD ,Audio,“audio”,ENUM,"{ ""off"", ""on"", ""auto"" }",Connector,TBD ,FMT Dithering,“dither”,ENUM,"{ ""off"", ""on"" }",Connector,TBD
-rcar-du,Generic,"""alpha""",RANGE,"Min=0, Max=255",Plane,TBD +,,"""alpha""",RANGE,"Min=0, Max=Driver dependant",Plane,Opacity of the plane from transparent (0) to fully opaque (MAX). If this property is set to a value different than max, and that the pixel will define an alpha component, the property will have precendance and the pixel value will be ignored.
Please don't document new properties in that csv file, it's an unreadable mess. Instead follow how we document standardized properties nowadays in full-blown sections. For plane blending we have:
https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-prop...
Ack
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 8185e3468a23..5a6f29524f12 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -42,6 +42,7 @@ struct drm_modeset_acquire_ctx;
- plane (in 16.16)
- @src_w: width of visible portion of plane (in 16.16)
- @src_h: height of visible portion of plane (in 16.16)
identical
- @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
@@ -105,6 +106,9 @@ struct drm_plane_state { uint32_t src_x, src_y; uint32_t src_h, src_w;
- /* Plane opacity */
- u8 alpha;
We may want to make that u16. The general we expect 16bpc for most color related things, but since this is a range prop I suppose we should just expose the actual hardware range. But making it u16 might avoid some head scratching for the first person to have hardware with higher precision. Either that or we should make the prop creation fail if the driver asks for more bits than we have in the state.
I'm tempted to go one step further and always make the alpha property 16-bits wide for new users (we can't do so for existing users as it could break userspace), and let drivers convert that internally to the range they need. There could however be drawbacks I don't foresee.
I think scaling the range to match the hw is the most sensible (yes I'm flip-flopping around here). And once someone needs more than u8, we can extend the internal representation easily. The external representation in the property is an u64, that /should/ be enough for the next few years :-)
Just to make sure we're on the same page, you want to keep the u8, and if the hardware uses say an u16, the driver for that hardware will do the upscaling?
The idea is that we'd set the u16 limit in the property and so inform userspace that a different range applies. But that's probably going to be ignored.
Could we do the property itself as u16 range, and (for now, only internally in drm in drm_plane_state) throw the lower u8 bits away? Or just let drivers do this.
Sorry that I'm flip-flopping around on this, but we just have an ongoing discussion about a range/size mixup in the CTM uapi, I think assuming that all userspace will correctly scale is not realistic. So larger scale in the uapi (but maybe not internally) from the start seems like a good idea. -Daniel
Hi Daniel,
On Monday, 5 March 2018 10:58:41 EET Daniel Vetter wrote:
On Wed, Feb 21, 2018 at 02:07:57PM +0100, Maxime Ripard wrote:
On Mon, Feb 19, 2018 at 10:58:40PM +0100, Daniel Vetter wrote:
On Mon, Feb 19, 2018 at 9:19 PM, Laurent Pinchart wrote:
On Friday, 16 February 2018 20:20:41 EET Ville Syrjälä wrote:
On Fri, Feb 16, 2018 at 06:39:29PM +0100, Maxime Ripard wrote:
Some drivers duplicate the logic to create a property to store a per-plane alpha.
This is especially useful if we ever want to support extra protocols for Wayland like: https://lists.freedesktop.org/archives/wayland-devel/2017-August/03 4741.html
Let's create a helper in order to move that to the core.
Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Reviewed-by: Boris Brezillon boris.brezillon@bootlin.com Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com
Documentation/gpu/kms-properties.csv | 2 +- drivers/gpu/drm/drm_atomic.c | 4 ++++- drivers/gpu/drm/drm_atomic_helper.c | 4 ++++- drivers/gpu/drm/drm_blend.c | 32 +++++++++++++++++++++++- include/drm/drm_blend.h | 1 +- include/drm/drm_plane.h | 6 +++++- 6 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/Documentation/gpu/kms-properties.csv b/Documentation/gpu/kms-properties.csv index 927b65e14219..25ad3503d663 100644 --- a/Documentation/gpu/kms-properties.csv +++ b/Documentation/gpu/kms-properties.csv @@ -99,5 +99,5 @@ radeon,DVI-I,“coherent”,RANGE,"Min=0, Max=1",Connector,TBD>
,,"""underscan vborder""",RANGE,"Min=0, Max=128",Connector,TBD ,Audio,“audio”,ENUM,"{ ""off"", ""on"", ""auto"" }",Connector,TBD ,FMT Dithering,“dither”,ENUM,"{ ""off"", ""on"" }",Connector,TBD
-rcar-du,Generic,"""alpha""",RANGE,"Min=0, Max=255",Plane,TBD +,,"""alpha""",RANGE,"Min=0, Max=Driver dependant",Plane,Opacity of the plane from transparent (0) to fully opaque (MAX). If this property is set to a value different than max, and that the pixel will define an alpha component, the property will have precendance and the pixel value will be ignored.
Please don't document new properties in that csv file, it's an unreadable mess. Instead follow how we document standardized properties nowadays in full-blown sections. For plane blending we have:
https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-%3E >> properties
Ack
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 8185e3468a23..5a6f29524f12 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -42,6 +42,7 @@ struct drm_modeset_acquire_ctx;
- plane (in 16.16)
- @src_w: width of visible portion of plane (in 16.16)
- @src_h: height of visible portion of plane (in 16.16)
identical
- @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
@@ -105,6 +106,9 @@ struct drm_plane_state { uint32_t src_x, src_y; uint32_t src_h, src_w;
- /* Plane opacity */
- u8 alpha;
We may want to make that u16. The general we expect 16bpc for most color related things, but since this is a range prop I suppose we should just expose the actual hardware range. But making it u16 might avoid some head scratching for the first person to have hardware with higher precision. Either that or we should make the prop creation fail if the driver asks for more bits than we have in the state.
I'm tempted to go one step further and always make the alpha property 16-bits wide for new users (we can't do so for existing users as it could break userspace), and let drivers convert that internally to the range they need. There could however be drawbacks I don't foresee.
I think scaling the range to match the hw is the most sensible (yes I'm flip-flopping around here). And once someone needs more than u8, we can extend the internal representation easily. The external representation in the property is an u64, that /should/ be enough for the next few years :-)
Just to make sure we're on the same page, you want to keep the u8, and if the hardware uses say an u16, the driver for that hardware will do the upscaling?
The idea is that we'd set the u16 limit in the property and so inform userspace that a different range applies. But that's probably going to be ignored.
Could we do the property itself as u16 range, and (for now, only internally in drm in drm_plane_state) throw the lower u8 bits away? Or just let drivers do this.
I'm fine with this except for the drivers that currently implement the alpha property with a different range. The rcar-du driver for instances has the alpha range set to 0x00 to 0xff, so we can't change it without risk of breaking userspace. I don't know whether there's any userspace using the property, and whether that userspace has any hardcoded assumption.
Sorry that I'm flip-flopping around on this, but we just have an ongoing discussion about a range/size mixup in the CTM uapi, I think assuming that all userspace will correctly scale is not realistic. So larger scale in the uapi (but maybe not internally) from the start seems like a good idea.
Can we make the range randomly chosen at every boot then ? :-) That would force userspace to be generic.
On Mon, Mar 05, 2018 at 12:08:12PM +0200, Laurent Pinchart wrote:
Hi Daniel,
On Monday, 5 March 2018 10:58:41 EET Daniel Vetter wrote:
On Wed, Feb 21, 2018 at 02:07:57PM +0100, Maxime Ripard wrote:
On Mon, Feb 19, 2018 at 10:58:40PM +0100, Daniel Vetter wrote:
On Mon, Feb 19, 2018 at 9:19 PM, Laurent Pinchart wrote:
On Friday, 16 February 2018 20:20:41 EET Ville Syrjälä wrote:
On Fri, Feb 16, 2018 at 06:39:29PM +0100, Maxime Ripard wrote: > Some drivers duplicate the logic to create a property to store a > per-plane alpha. > > This is especially useful if we ever want to support extra > protocols for Wayland like: > https://lists.freedesktop.org/archives/wayland-devel/2017-August/03 > 4741.html > > Let's create a helper in order to move that to the core. > > Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com > Reviewed-by: Boris Brezillon boris.brezillon@bootlin.com > Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com > --- > > Documentation/gpu/kms-properties.csv | 2 +- > drivers/gpu/drm/drm_atomic.c | 4 ++++- > drivers/gpu/drm/drm_atomic_helper.c | 4 ++++- > drivers/gpu/drm/drm_blend.c | 32 +++++++++++++++++++++++- > include/drm/drm_blend.h | 1 +- > include/drm/drm_plane.h | 6 +++++- > 6 files changed, 48 insertions(+), 1 deletion(-) > > diff --git a/Documentation/gpu/kms-properties.csv > b/Documentation/gpu/kms-properties.csv index > 927b65e14219..25ad3503d663 > 100644 > --- a/Documentation/gpu/kms-properties.csv > +++ b/Documentation/gpu/kms-properties.csv > @@ -99,5 +99,5 @@ radeon,DVI-I,“coherent”,RANGE,"Min=0, > Max=1",Connector,TBD> > > ,,"""underscan vborder""",RANGE,"Min=0, Max=128",Connector,TBD > ,Audio,“audio”,ENUM,"{ ""off"", ""on"", ""auto"" }",Connector,TBD > ,FMT Dithering,“dither”,ENUM,"{ ""off"", ""on"" }",Connector,TBD > > -rcar-du,Generic,"""alpha""",RANGE,"Min=0, Max=255",Plane,TBD > +,,"""alpha""",RANGE,"Min=0, Max=Driver dependant",Plane,Opacity of > the plane from transparent (0) to fully opaque (MAX). If this > property is set to a value different than max, and that the pixel > will define an alpha component, the property will have precendance > and the pixel value will be ignored.
Please don't document new properties in that csv file, it's an unreadable mess. Instead follow how we document standardized properties nowadays in full-blown sections. For plane blending we have:
https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-%3E >> properties
Ack
> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h > index 8185e3468a23..5a6f29524f12 100644 > --- a/include/drm/drm_plane.h > +++ b/include/drm/drm_plane.h > @@ -42,6 +42,7 @@ struct drm_modeset_acquire_ctx; > * plane (in 16.16) > * @src_w: width of visible portion of plane (in 16.16) > * @src_h: height of visible portion of plane (in 16.16) > + * @alpha: opacity of the plane > * @rotation: rotation of the plane > * @zpos: priority of the given plane on crtc (optional) > * Note that multiple active planes on the same crtc can have an > identical > @@ -105,6 +106,9 @@ struct drm_plane_state { > uint32_t src_x, src_y; > uint32_t src_h, src_w; > > + /* Plane opacity */ > + u8 alpha;
We may want to make that u16. The general we expect 16bpc for most color related things, but since this is a range prop I suppose we should just expose the actual hardware range. But making it u16 might avoid some head scratching for the first person to have hardware with higher precision. Either that or we should make the prop creation fail if the driver asks for more bits than we have in the state.
I'm tempted to go one step further and always make the alpha property 16-bits wide for new users (we can't do so for existing users as it could break userspace), and let drivers convert that internally to the range they need. There could however be drawbacks I don't foresee.
I think scaling the range to match the hw is the most sensible (yes I'm flip-flopping around here). And once someone needs more than u8, we can extend the internal representation easily. The external representation in the property is an u64, that /should/ be enough for the next few years :-)
Just to make sure we're on the same page, you want to keep the u8, and if the hardware uses say an u16, the driver for that hardware will do the upscaling?
The idea is that we'd set the u16 limit in the property and so inform userspace that a different range applies. But that's probably going to be ignored.
Could we do the property itself as u16 range, and (for now, only internally in drm in drm_plane_state) throw the lower u8 bits away? Or just let drivers do this.
I'm fine with this except for the drivers that currently implement the alpha property with a different range. The rcar-du driver for instances has the alpha range set to 0x00 to 0xff, so we can't change it without risk of breaking userspace. I don't know whether there's any userspace using the property, and whether that userspace has any hardcoded assumption.
Does open source userspace for this exist that we could actually break? :-)
Sorry that I'm flip-flopping around on this, but we just have an ongoing discussion about a range/size mixup in the CTM uapi, I think assuming that all userspace will correctly scale is not realistic. So larger scale in the uapi (but maybe not internally) from the start seems like a good idea.
Can we make the range randomly chosen at every boot then ? :-) That would force userspace to be generic.
Either way I think we've discussed this enough already, just pick one of the reasonable options and we'll live with the consequences. So either fixed 0xffff limit or (hopefully) variable limit exposed in the prop. -Daniel
Hi Daniel,
On Monday, 19 February 2018 23:58:40 EET Daniel Vetter wrote:
On Mon, Feb 19, 2018 at 9:19 PM, Laurent Pinchart wrote:
On Friday, 16 February 2018 20:20:41 EET Ville Syrjälä wrote:
On Fri, Feb 16, 2018 at 06:39:29PM +0100, Maxime Ripard wrote: Some drivers duplicate the logic to create a property to store a
per-plane alpha.
This is especially useful if we ever want to support extra protocols for Wayland like: https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741 .html
Let's create a helper in order to move that to the core.
Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Reviewed-by: Boris Brezillon boris.brezillon@bootlin.com Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com
Documentation/gpu/kms-properties.csv | 2 +- drivers/gpu/drm/drm_atomic.c | 4 ++++- drivers/gpu/drm/drm_atomic_helper.c | 4 ++++- drivers/gpu/drm/drm_blend.c | 32 ++++++++++++++++++++++++++++- include/drm/drm_blend.h | 1 +- include/drm/drm_plane.h | 6 +++++- 6 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/Documentation/gpu/kms-properties.csv b/Documentation/gpu/kms-properties.csv index 927b65e14219..25ad3503d663 100644 --- a/Documentation/gpu/kms-properties.csv +++ b/Documentation/gpu/kms-properties.csv @@ -99,5 +99,5 @@ radeon,DVI-I,“coherent”,RANGE,"Min=0, Max=1",Connector,TBD>
,,"""underscan vborder""",RANGE,"Min=0, Max=128",Connector,TBD ,Audio,“audio”,ENUM,"{ ""off"", ""on"", ""auto"" }",Connector,TBD ,FMT Dithering,“dither”,ENUM,"{ ""off"", ""on"" }",Connector,TBD
-rcar-du,Generic,"""alpha""",RANGE,"Min=0, Max=255",Plane,TBD +,,"""alpha""",RANGE,"Min=0, Max=Driver dependant",Plane,Opacity of the plane from transparent (0) to fully opaque (MAX). If this property is set to a value different than max, and that the pixel will define an alpha component, the property will have precendance and the pixel value will be ignored.
Please don't document new properties in that csv file, it's an unreadable mess. Instead follow how we document standardized properties nowadays in full-blown sections. For plane blending we have:
https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-prop erties
Those semantics don't seem particularly good to me. I think we would want the per-pixel alpha and global alpha both to be effecive at the same time. You can always decide to ignore the per-pixel alpha by using a pixel format without alpha.
That makes sense to me. However, it also brings a new question: how should a driver that supports either global alpha or pixel alpha but not both signal that to userspace, and how should it reacts when userspace selects a format with an alpha channel and set a global alpha value other than fully opaque ? To make things more complex, note that some drivers support combining global alpha and pixel alpha only for a subset of the formats with an alpha channel (for instance for ARGB 1555 formats, but not for ARGB 8888 formats).
atomic_check can reject unsupported configs. Userspace needs to fall back somehow (either switch to xrgb or make alpha fully opaque or just give up on that plane). We have a lot of such corner-cases we don't tell userspace about explicitly at all.
I'm OK with failing the commit in case in invalid configuration is requested. However, using a check-only commit to find out whether combining global alpha and pixel alpha is supported doesn't seem a good idea to me. First of all userspace would need to try that for all formats, making it cumbersome. Then, an atomic commit is a black box, we don't report the failure cause to userspace. It makes it hard to use it to test support for a feature as it could fail for an unrelated reason. Finally, we'd open the door to various kind of heuristics implemented differently in different userspace stacks, and that would increase the risk of breaking userspace. I'd rather have an explicitly documented way to perform such checks.
Also, where's the userspace that wants this feature?
<snip>
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 8185e3468a23..5a6f29524f12 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -42,6 +42,7 @@ struct drm_modeset_acquire_ctx;
- plane (in 16.16)
- @src_w: width of visible portion of plane (in 16.16)
- @src_h: height of visible portion of plane (in 16.16)
identical
- @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
@@ -105,6 +106,9 @@ struct drm_plane_state { uint32_t src_x, src_y; uint32_t src_h, src_w;
- /* Plane opacity */
- u8 alpha;
We may want to make that u16. The general we expect 16bpc for most color related things, but since this is a range prop I suppose we should just expose the actual hardware range. But making it u16 might avoid some head scratching for the first person to have hardware with higher precision. Either that or we should make the prop creation fail if the driver asks for more bits than we have in the state.
I'm tempted to go one step further and always make the alpha property 16-bits wide for new users (we can't do so for existing users as it could break userspace), and let drivers convert that internally to the range they need. There could however be drawbacks I don't foresee.
I think scaling the range to match the hw is the most sensible (yes I'm flip-flopping around here).
So you mean keeping the proposed implementation, with a driver-specific maximum value ?
And once someone needs more than u8, we can extend the internal representation easily. The external representation in the property is an u64, that /should/ be enough for the next few years :-)
The external representation is split in two parts. The value is stored in a 64-bits field, and that is safe. The second part of the representation is the minimum (hardcoded to 0) and maximum (currently variable) values reported by the property. What we need to decide now is whether to hardcode the maximum value to 0xffff for all new users of the alpha property, or to always expose the hardware range.
Oh, and you should plug this into the state dumper as well.
/* Plane rotation */ unsigned int rotation;
@@ -481,6 +485,7 @@ enum drm_plane_type {
- @funcs: helper functions
- @properties: property tracking for this plane
- @type: type of plane (overlay, primary, cursor)
- @alpha_property: alpha property for this plane
- @zpos_property: zpos property for this plane
- @rotation_property: rotation property for this plane
- @helper_private: mid-layer private data
@@ -556,6 +561,7 @@ struct drm_plane { */
struct drm_plane_state *state;
- struct drm_property *alpha_property; struct drm_property *zpos_property; struct drm_property *rotation_property;
};
On Wed, Feb 21, 2018 at 10:39:02PM +0200, Laurent Pinchart wrote:
Hi Daniel,
On Monday, 19 February 2018 23:58:40 EET Daniel Vetter wrote:
On Mon, Feb 19, 2018 at 9:19 PM, Laurent Pinchart wrote:
On Friday, 16 February 2018 20:20:41 EET Ville Syrjälä wrote:
On Fri, Feb 16, 2018 at 06:39:29PM +0100, Maxime Ripard wrote: Some drivers duplicate the logic to create a property to store a
per-plane alpha.
This is especially useful if we ever want to support extra protocols for Wayland like: https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741 .html
Let's create a helper in order to move that to the core.
Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Reviewed-by: Boris Brezillon boris.brezillon@bootlin.com Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com
Documentation/gpu/kms-properties.csv | 2 +- drivers/gpu/drm/drm_atomic.c | 4 ++++- drivers/gpu/drm/drm_atomic_helper.c | 4 ++++- drivers/gpu/drm/drm_blend.c | 32 ++++++++++++++++++++++++++++- include/drm/drm_blend.h | 1 +- include/drm/drm_plane.h | 6 +++++- 6 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/Documentation/gpu/kms-properties.csv b/Documentation/gpu/kms-properties.csv index 927b65e14219..25ad3503d663 100644 --- a/Documentation/gpu/kms-properties.csv +++ b/Documentation/gpu/kms-properties.csv @@ -99,5 +99,5 @@ radeon,DVI-I,“coherent”,RANGE,"Min=0, Max=1",Connector,TBD>
,,"""underscan vborder""",RANGE,"Min=0, Max=128",Connector,TBD ,Audio,“audio”,ENUM,"{ ""off"", ""on"", ""auto"" }",Connector,TBD ,FMT Dithering,“dither”,ENUM,"{ ""off"", ""on"" }",Connector,TBD
-rcar-du,Generic,"""alpha""",RANGE,"Min=0, Max=255",Plane,TBD +,,"""alpha""",RANGE,"Min=0, Max=Driver dependant",Plane,Opacity of the plane from transparent (0) to fully opaque (MAX). If this property is set to a value different than max, and that the pixel will define an alpha component, the property will have precendance and the pixel value will be ignored.
Please don't document new properties in that csv file, it's an unreadable mess. Instead follow how we document standardized properties nowadays in full-blown sections. For plane blending we have:
https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-prop erties
Those semantics don't seem particularly good to me. I think we would want the per-pixel alpha and global alpha both to be effecive at the same time. You can always decide to ignore the per-pixel alpha by using a pixel format without alpha.
That makes sense to me. However, it also brings a new question: how should a driver that supports either global alpha or pixel alpha but not both signal that to userspace, and how should it reacts when userspace selects a format with an alpha channel and set a global alpha value other than fully opaque ? To make things more complex, note that some drivers support combining global alpha and pixel alpha only for a subset of the formats with an alpha channel (for instance for ARGB 1555 formats, but not for ARGB 8888 formats).
atomic_check can reject unsupported configs. Userspace needs to fall back somehow (either switch to xrgb or make alpha fully opaque or just give up on that plane). We have a lot of such corner-cases we don't tell userspace about explicitly at all.
I'm OK with failing the commit in case in invalid configuration is requested. However, using a check-only commit to find out whether combining global alpha and pixel alpha is supported doesn't seem a good idea to me. First of all userspace would need to try that for all formats, making it cumbersome. Then, an atomic commit is a black box, we don't report the failure cause to userspace. It makes it hard to use it to test support for a feature as it could fail for an unrelated reason. Finally, we'd open the door to various kind of heuristics implemented differently in different userspace stacks, and that would increase the risk of breaking userspace. I'd rather have an explicitly documented way to perform such checks.
I am _not_ against more explicit checks. I only object against them in the name of "future proofing" without a solid pile of different driver and userspace implementations as demonstration vehicles that the hints are actually useful and needed by userspace.
DRM history is full of fake-generic uapi that turned out to be useful for exactly the 1 driver/userspace combo it was originally implemented for, and frankly for just 1 combo hardcoding works better.
Also, these fake-generic interfaces tend to result in epic bikeshed fests that go nowhere, I think it's better to avoid them in the first uapi round and get the basic properties in first. Nothing prevents us from adding hints and more useful constraints information later on.
Also, where's the userspace that wants this feature?
<snip>
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 8185e3468a23..5a6f29524f12 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -42,6 +42,7 @@ struct drm_modeset_acquire_ctx;
- plane (in 16.16)
- @src_w: width of visible portion of plane (in 16.16)
- @src_h: height of visible portion of plane (in 16.16)
identical
- @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
@@ -105,6 +106,9 @@ struct drm_plane_state { uint32_t src_x, src_y; uint32_t src_h, src_w;
- /* Plane opacity */
- u8 alpha;
We may want to make that u16. The general we expect 16bpc for most color related things, but since this is a range prop I suppose we should just expose the actual hardware range. But making it u16 might avoid some head scratching for the first person to have hardware with higher precision. Either that or we should make the prop creation fail if the driver asks for more bits than we have in the state.
I'm tempted to go one step further and always make the alpha property 16-bits wide for new users (we can't do so for existing users as it could break userspace), and let drivers convert that internally to the range they need. There could however be drawbacks I don't foresee.
I think scaling the range to match the hw is the most sensible (yes I'm flip-flopping around here).
So you mean keeping the proposed implementation, with a driver-specific maximum value ?
And once someone needs more than u8, we can extend the internal representation easily. The external representation in the property is an u64, that /should/ be enough for the next few years :-)
The external representation is split in two parts. The value is stored in a 64-bits field, and that is safe. The second part of the representation is the minimum (hardcoded to 0) and maximum (currently variable) values reported by the property. What we need to decide now is whether to hardcode the maximum value to 0xffff for all new users of the alpha property, or to always expose the hardware range.
After a bit of vacation I'm leaning towards hardcoding 0xffff for now, because if we start out with generic userspace that's only tested on hw with a limit of 0xff, then defacto (but maybe not in the docs, but docs lose against reality) we'll hardcode the uapi to 0xff, not the variable hw limit. And that would mean as soon as hw with a higher limit shows up we need to add a alpha-but-high-res-for-real-now property. -Daniel
Oh, and you should plug this into the state dumper as well.
/* Plane rotation */ unsigned int rotation;
@@ -481,6 +485,7 @@ enum drm_plane_type {
- @funcs: helper functions
- @properties: property tracking for this plane
- @type: type of plane (overlay, primary, cursor)
- @alpha_property: alpha property for this plane
- @zpos_property: zpos property for this plane
- @rotation_property: rotation property for this plane
- @helper_private: mid-layer private data
@@ -556,6 +561,7 @@ struct drm_plane { */
struct drm_plane_state *state;
- struct drm_property *alpha_property; struct drm_property *zpos_property; struct drm_property *rotation_property;
};
-- Regards,
Laurent Pinchart
On Fri, Feb 16, 2018 at 7:20 PM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Fri, Feb 16, 2018 at 06:39:29PM +0100, Maxime Ripard wrote:
Some drivers duplicate the logic to create a property to store a per-plane alpha.
This is especially useful if we ever want to support extra protocols for Wayland like: https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741.html
Let's create a helper in order to move that to the core.
Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Reviewed-by: Boris Brezillon boris.brezillon@bootlin.com Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com
Documentation/gpu/kms-properties.csv | 2 +- drivers/gpu/drm/drm_atomic.c | 4 ++++- drivers/gpu/drm/drm_atomic_helper.c | 4 ++++- drivers/gpu/drm/drm_blend.c | 32 +++++++++++++++++++++++++++++- include/drm/drm_blend.h | 1 +- include/drm/drm_plane.h | 6 +++++- 6 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/Documentation/gpu/kms-properties.csv b/Documentation/gpu/kms-properties.csv index 927b65e14219..25ad3503d663 100644 --- a/Documentation/gpu/kms-properties.csv +++ b/Documentation/gpu/kms-properties.csv @@ -99,5 +99,5 @@ radeon,DVI-I,“coherent”,RANGE,"Min=0, Max=1",Connector,TBD ,,"""underscan vborder""",RANGE,"Min=0, Max=128",Connector,TBD ,Audio,“audio”,ENUM,"{ ""off"", ""on"", ""auto"" }",Connector,TBD ,FMT Dithering,“dither”,ENUM,"{ ""off"", ""on"" }",Connector,TBD -rcar-du,Generic,"""alpha""",RANGE,"Min=0, Max=255",Plane,TBD +,,"""alpha""",RANGE,"Min=0, Max=Driver dependant",Plane,Opacity of the plane from transparent (0) to fully opaque (MAX). If this property is set to a value different than max, and that the pixel will define an alpha component, the property will have precendance and the pixel value will be ignored.
Those semantics don't seem particularly good to me. I think we would want the per-pixel alpha and global alpha both to be effecive at the same time. You can always decide to ignore the per-pixel alpha by using a pixel format without alpha.
Also, where's the userspace that wants this feature?
drm_hwcomposer uses an 8-bit per-plane alpha property, and from what I can tell the semantics are that both pixel and plane alpha apply simultaneously if present. Here is what I think was the kernel-side for this:
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3...
I've added Sean Paul, he might be able to give a more definitive answer.
Regards, Stefan
Hi,
On Tue, Feb 20, 2018 at 04:10:28PM +0100, Stefan Schake wrote:
On Fri, Feb 16, 2018 at 7:20 PM, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Fri, Feb 16, 2018 at 06:39:29PM +0100, Maxime Ripard wrote:
Some drivers duplicate the logic to create a property to store a per-plane alpha.
This is especially useful if we ever want to support extra protocols for Wayland like: https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741.html
Let's create a helper in order to move that to the core.
Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Reviewed-by: Boris Brezillon boris.brezillon@bootlin.com Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com
Documentation/gpu/kms-properties.csv | 2 +- drivers/gpu/drm/drm_atomic.c | 4 ++++- drivers/gpu/drm/drm_atomic_helper.c | 4 ++++- drivers/gpu/drm/drm_blend.c | 32 +++++++++++++++++++++++++++++- include/drm/drm_blend.h | 1 +- include/drm/drm_plane.h | 6 +++++- 6 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/Documentation/gpu/kms-properties.csv b/Documentation/gpu/kms-properties.csv index 927b65e14219..25ad3503d663 100644 --- a/Documentation/gpu/kms-properties.csv +++ b/Documentation/gpu/kms-properties.csv @@ -99,5 +99,5 @@ radeon,DVI-I,“coherent”,RANGE,"Min=0, Max=1",Connector,TBD ,,"""underscan vborder""",RANGE,"Min=0, Max=128",Connector,TBD ,Audio,“audio”,ENUM,"{ ""off"", ""on"", ""auto"" }",Connector,TBD ,FMT Dithering,“dither”,ENUM,"{ ""off"", ""on"" }",Connector,TBD -rcar-du,Generic,"""alpha""",RANGE,"Min=0, Max=255",Plane,TBD +,,"""alpha""",RANGE,"Min=0, Max=Driver dependant",Plane,Opacity of the plane from transparent (0) to fully opaque (MAX). If this property is set to a value different than max, and that the pixel will define an alpha component, the property will have precendance and the pixel value will be ignored.
Those semantics don't seem particularly good to me. I think we would want the per-pixel alpha and global alpha both to be effecive at the same time. You can always decide to ignore the per-pixel alpha by using a pixel format without alpha.
Also, where's the userspace that wants this feature?
drm_hwcomposer uses an 8-bit per-plane alpha property, and from what I can tell the semantics are that both pixel and plane alpha apply simultaneously if present. Here is what I think was the kernel-side for this:
https://chromium-review.googlesource.com/c/chromiumos/third_party/kernel/+/3...
I've added Sean Paul, he might be able to give a more definitive answer.
What is the behaviour of the tegra engine when it has both a pixel-alpha and a plane-alpha?
Atmel at least will drop one (but I'm not sure which one anymore).
Thanks! Maxime
Hey,
On Wed, Feb 21, 2018 at 2:04 PM, Maxime Ripard maxime.ripard@bootlin.com wrote:
What is the behaviour of the tegra engine when it has both a pixel-alpha and a plane-alpha?
Atmel at least will drop one (but I'm not sure which one anymore).
Sorry, I have no more on the Tegra beyond that commit. But I did have some more play time with drm_hwcomposer and from the rendering, it certainly expects both to apply at the same time. To further complicate it, from what I can tell with the VC4 HVS, you can actually configure it to 1) use only pixel alpha 2) use only plane alpha or 3) mix both pixel and plane alpha, with 3 being what drm_hwc seems to want.
I think once writeback lands it would be a good idea to have some tests that establish DRM plane blending standards beyond text in docs :)
Regards, Stefan
Hi Maxime,
Thank you for the patch.
On Friday, 16 February 2018 19:39:29 EET Maxime Ripard wrote:
Some drivers duplicate the logic to create a property to store a per-plane alpha.
This is especially useful if we ever want to support extra protocols for Wayland like: https://lists.freedesktop.org/archives/wayland-devel/2017-August/034741.html
Let's create a helper in order to move that to the core.
Cc: Laurent Pinchart laurent.pinchart@ideasonboard.com Reviewed-by: Boris Brezillon boris.brezillon@bootlin.com Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com
Documentation/gpu/kms-properties.csv | 2 +- drivers/gpu/drm/drm_atomic.c | 4 ++++- drivers/gpu/drm/drm_atomic_helper.c | 4 ++++- drivers/gpu/drm/drm_blend.c | 32 +++++++++++++++++++++++++++++- include/drm/drm_blend.h | 1 +- include/drm/drm_plane.h | 6 +++++- 6 files changed, 48 insertions(+), 1 deletion(-)
diff --git a/Documentation/gpu/kms-properties.csv b/Documentation/gpu/kms-properties.csv index 927b65e14219..25ad3503d663 100644 --- a/Documentation/gpu/kms-properties.csv +++ b/Documentation/gpu/kms-properties.csv @@ -99,5 +99,5 @@ radeon,DVI-I,“coherent”,RANGE,"Min=0, Max=1",Connector,TBD ,,"""underscan vborder""",RANGE,"Min=0, Max=128",Connector,TBD ,Audio,“audio”,ENUM,"{ ""off"", ""on"", ""auto"" }",Connector,TBD ,FMT Dithering,“dither”,ENUM,"{ ""off"", ""on"" }",Connector,TBD -rcar-du,Generic,"""alpha""",RANGE,"Min=0, Max=255",Plane,TBD +,,"""alpha""",RANGE,"Min=0, Max=Driver dependant",Plane,Opacity of the plane from transparent (0) to fully opaque (MAX). If this property is set to a value different than max, and that the pixel will define an alpha component, the property will have precendance and the pixel value will be ignored. The alpha value is represented as straight alpha, ie the colors haven't been pre-adjusted for their opacity by multiplication. Therefore, the equation to get a color value for one pixel, assuming two planes A and B, will be (color_a * alpha_a + color_b * alpha_b * (MAX - alpha_a) / MAX) / (alpha_a + alpha_b * (MAX - alpha_a) / MAX) ,,"""colorkey""",RANGE,"Min=0, Max=0x01ffffff",Plane,TBD diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 7d9ad20040a1..3defc56a1ef2 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -753,6 +753,8 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, state->src_w = val; } else if (property == config->prop_src_h) { state->src_h = val;
- } else if (property == plane->alpha_property) {
state->alpha = val;@@ -814,6 +816,8 @@ drm_atomic_plane_get_property(struct drm_plane *plane, *val = state->src_w; } else if (property == config->prop_src_h) { *val = state->src_h;
- } else if (property == plane->alpha_property) {
*val = state->alpha;diff --git a/drivers/gpu/drm/drm_atomic_helper.c b/drivers/gpu/drm/drm_atomic_helper.c index ae3cbfe9e01c..2b88f593aab4 100644 --- a/drivers/gpu/drm/drm_atomic_helper.c +++ b/drivers/gpu/drm/drm_atomic_helper.c @@ -3482,6 +3482,10 @@ void drm_atomic_helper_plane_reset(struct drm_plane *plane) if (plane->state) { plane->state->plane = plane; plane->state->rotation = DRM_MODE_ROTATE_0;
/* Reset the alpha value to fully opaque if it matters */if (plane->alpha_property)plane->state->alpha = plane->alpha_property->values[1];} EXPORT_SYMBOL(drm_atomic_helper_plane_reset); diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c index 4c62dff14893..a5dea7cbed2c 100644 --- a/drivers/gpu/drm/drm_blend.c +++ b/drivers/gpu/drm/drm_blend.c @@ -104,6 +104,38 @@ */
/**
- drm_plane_create_alpha_property - create a new alpha property
- @plane: drm plane
- @max_alpha: maximum value of alpha
- This function initializes a generic, mutable, alpha property and
- enables support for it in the DRM core.
s/initializes/creates/
I would also mention that it attaches the property to the plane.
- The alpha property will be allowed to be within the bounds of 0
- (transparent) to @max_alpha (opaque)
s/$/./
- Returns:
- 0 on success, negative error code on failure.
- */
+int drm_plane_create_alpha_property(struct drm_plane *plane, u16 max_alpha) +{
- struct drm_property *prop;
- prop = drm_property_create_range(plane->dev, 0, "alpha", 0, max_alpha);
- if (!prop)
return -ENOMEM;- drm_object_attach_property(&plane->base, prop, max_alpha);
- plane->alpha_property = prop;
- if (plane->state)
plane->state->alpha = max_alpha;- return 0;
+} +EXPORT_SYMBOL(drm_plane_create_alpha_property);
+/**
- drm_plane_create_rotation_property - create a new rotation property
- @plane: drm plane
- @rotation: initial value of the rotation property
diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h index 17606026590b..e5affba6ebde 100644 --- a/include/drm/drm_blend.h +++ b/include/drm/drm_blend.h @@ -36,6 +36,7 @@ static inline bool drm_rotation_90_or_270(unsigned int rotation) return rotation & (DRM_MODE_ROTATE_90 | DRM_MODE_ROTATE_270); }
+int drm_plane_create_alpha_property(struct drm_plane *plane, u16 alpha); int drm_plane_create_rotation_property(struct drm_plane *plane, unsigned int rotation, unsigned int supported_rotations); diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 8185e3468a23..5a6f29524f12 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -42,6 +42,7 @@ struct drm_modeset_acquire_ctx;
- plane (in 16.16)
- @src_w: width of visible portion of plane (in 16.16)
- @src_h: height of visible portion of plane (in 16.16)
- @alpha: opacity of the plane
- @rotation: rotation of the plane
- @zpos: priority of the given plane on crtc (optional)
- Note that multiple active planes on the same crtc can have an identical
@@ -105,6 +106,9 @@ struct drm_plane_state { uint32_t src_x, src_y; uint32_t src_h, src_w;
- /* Plane opacity */
- u8 alpha;
- /* Plane rotation */ unsigned int rotation;
@@ -481,6 +485,7 @@ enum drm_plane_type {
- @funcs: helper functions
- @properties: property tracking for this plane
- @type: type of plane (overlay, primary, cursor)
- @alpha_property: alpha property for this plane
- @zpos_property: zpos property for this plane
- @rotation_property: rotation property for this plane
- @helper_private: mid-layer private data
@@ -556,6 +561,7 @@ struct drm_plane { */ struct drm_plane_state *state;
- struct drm_property *alpha_property; struct drm_property *zpos_property; struct drm_property *rotation_property;
};
Now that we have support for per-plane alpha in the core, let's use it.
Acked-by: Boris Brezillon boris.brezillon@bootlin.com Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com --- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h | 13 +--- drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c | 89 ++---------------- 2 files changed, 14 insertions(+), 88 deletions(-)
diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h index ab32d5b268d2..60c937f42114 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_dc.h @@ -299,7 +299,6 @@ struct atmel_hlcdc_layer { struct atmel_hlcdc_plane { struct drm_plane base; struct atmel_hlcdc_layer layer; - struct atmel_hlcdc_plane_properties *properties; };
static inline struct atmel_hlcdc_plane * @@ -346,18 +345,6 @@ struct atmel_hlcdc_dc_desc { };
/** - * Atmel HLCDC Plane properties. - * - * This structure stores plane property definitions. - * - * @alpha: alpha blending (or transparency) property - * @rotation: rotation property - */ -struct atmel_hlcdc_plane_properties { - struct drm_property *alpha; -}; - -/** * Atmel HLCDC Display Controller. * * @desc: HLCDC Display Controller description diff --git a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c index e18800ed7cd1..d1b89a6f6280 100644 --- a/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c +++ b/drivers/gpu/drm/atmel-hlcdc/atmel_hlcdc_plane.c @@ -31,7 +31,6 @@ * @src_y: y buffer position * @src_w: buffer width * @src_h: buffer height - * @alpha: alpha blending of the plane * @disc_x: x discard position * @disc_y: y discard position * @disc_w: discard width @@ -54,8 +53,6 @@ struct atmel_hlcdc_plane_state { uint32_t src_w; uint32_t src_h;
- u8 alpha; - int disc_x; int disc_y; int disc_w; @@ -385,7 +382,7 @@ atmel_hlcdc_plane_update_general_settings(struct atmel_hlcdc_plane *plane, cfg |= ATMEL_HLCDC_LAYER_LAEN; else cfg |= ATMEL_HLCDC_LAYER_GAEN | - ATMEL_HLCDC_LAYER_GA(state->alpha); + ATMEL_HLCDC_LAYER_GA(state->base.alpha); }
if (state->disc_h && state->disc_w) @@ -553,7 +550,7 @@ atmel_hlcdc_plane_prepare_disc_area(struct drm_crtc_state *c_state)
if (!ovl_s->fb || ovl_s->fb->format->has_alpha || - ovl_state->alpha != 255) + ovl_s->alpha != 255) continue;
/* TODO: implement a smarter hidden area detection */ @@ -829,51 +826,18 @@ static void atmel_hlcdc_plane_destroy(struct drm_plane *p) drm_plane_cleanup(p); }
-static int atmel_hlcdc_plane_atomic_set_property(struct drm_plane *p, - struct drm_plane_state *s, - struct drm_property *property, - uint64_t val) -{ - struct atmel_hlcdc_plane *plane = drm_plane_to_atmel_hlcdc_plane(p); - struct atmel_hlcdc_plane_properties *props = plane->properties; - struct atmel_hlcdc_plane_state *state = - drm_plane_state_to_atmel_hlcdc_plane_state(s); - - if (property == props->alpha) - state->alpha = val; - else - return -EINVAL; - - return 0; -} - -static int atmel_hlcdc_plane_atomic_get_property(struct drm_plane *p, - const struct drm_plane_state *s, - struct drm_property *property, - uint64_t *val) -{ - struct atmel_hlcdc_plane *plane = drm_plane_to_atmel_hlcdc_plane(p); - struct atmel_hlcdc_plane_properties *props = plane->properties; - const struct atmel_hlcdc_plane_state *state = - container_of(s, const struct atmel_hlcdc_plane_state, base); - - if (property == props->alpha) - *val = state->alpha; - else - return -EINVAL; - - return 0; -} - -static int atmel_hlcdc_plane_init_properties(struct atmel_hlcdc_plane *plane, - struct atmel_hlcdc_plane_properties *props) +static int atmel_hlcdc_plane_init_properties(struct atmel_hlcdc_plane *plane) { const struct atmel_hlcdc_layer_desc *desc = plane->layer.desc;
if (desc->type == ATMEL_HLCDC_OVERLAY_LAYER || - desc->type == ATMEL_HLCDC_CURSOR_LAYER) - drm_object_attach_property(&plane->base.base, - props->alpha, 255); + desc->type == ATMEL_HLCDC_CURSOR_LAYER) { + int ret; + + ret = drm_plane_create_alpha_property(&plane->base, 255); + if (ret) + return ret; + }
if (desc->layout.xstride && desc->layout.pstride) { int ret; @@ -988,8 +952,8 @@ static void atmel_hlcdc_plane_reset(struct drm_plane *p) return; }
- state->alpha = 255; p->state = &state->base; + p->state->alpha = 255; p->state->plane = p; } } @@ -1042,13 +1006,10 @@ static const struct drm_plane_funcs layer_plane_funcs = { .reset = atmel_hlcdc_plane_reset, .atomic_duplicate_state = atmel_hlcdc_plane_atomic_duplicate_state, .atomic_destroy_state = atmel_hlcdc_plane_atomic_destroy_state, - .atomic_set_property = atmel_hlcdc_plane_atomic_set_property, - .atomic_get_property = atmel_hlcdc_plane_atomic_get_property, };
static int atmel_hlcdc_plane_create(struct drm_device *dev, - const struct atmel_hlcdc_layer_desc *desc, - struct atmel_hlcdc_plane_properties *props) + const struct atmel_hlcdc_layer_desc *desc) { struct atmel_hlcdc_dc *dc = dev->dev_private; struct atmel_hlcdc_plane *plane; @@ -1060,7 +1021,6 @@ static int atmel_hlcdc_plane_create(struct drm_device *dev, return -ENOMEM;
atmel_hlcdc_layer_init(&plane->layer, desc, dc->hlcdc->regmap); - plane->properties = props;
if (desc->type == ATMEL_HLCDC_BASE_LAYER) type = DRM_PLANE_TYPE_PRIMARY; @@ -1081,7 +1041,7 @@ static int atmel_hlcdc_plane_create(struct drm_device *dev, &atmel_hlcdc_layer_plane_helper_funcs);
/* Set default property values*/ - ret = atmel_hlcdc_plane_init_properties(plane, props); + ret = atmel_hlcdc_plane_init_properties(plane); if (ret) return ret;
@@ -1090,34 +1050,13 @@ static int atmel_hlcdc_plane_create(struct drm_device *dev, return 0; }
-static struct atmel_hlcdc_plane_properties * -atmel_hlcdc_plane_create_properties(struct drm_device *dev) -{ - struct atmel_hlcdc_plane_properties *props; - - props = devm_kzalloc(dev->dev, sizeof(*props), GFP_KERNEL); - if (!props) - return ERR_PTR(-ENOMEM); - - props->alpha = drm_property_create_range(dev, 0, "alpha", 0, 255); - if (!props->alpha) - return ERR_PTR(-ENOMEM); - - return props; -} - int atmel_hlcdc_create_planes(struct drm_device *dev) { struct atmel_hlcdc_dc *dc = dev->dev_private; - struct atmel_hlcdc_plane_properties *props; const struct atmel_hlcdc_layer_desc *descs = dc->desc->layers; int nlayers = dc->desc->nlayers; int i, ret;
- props = atmel_hlcdc_plane_create_properties(dev); - if (IS_ERR(props)) - return PTR_ERR(props); - dc->dscrpool = dmam_pool_create("atmel-hlcdc-dscr", dev->dev, sizeof(struct atmel_hlcdc_dma_channel_dscr), sizeof(u64), 0); @@ -1130,7 +1069,7 @@ int atmel_hlcdc_create_planes(struct drm_device *dev) descs[i].type != ATMEL_HLCDC_CURSOR_LAYER) continue;
- ret = atmel_hlcdc_plane_create(dev, &descs[i], props); + ret = atmel_hlcdc_plane_create(dev, &descs[i]); if (ret) return ret; }
Now that we have support for per-plane alpha in the core, let's use it.
Reviewed-by: Laurent Pinchart laurent.pinchart@ideasonboard.com Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com --- drivers/gpu/drm/rcar-du/rcar_du_drv.h | 1 +- drivers/gpu/drm/rcar-du/rcar_du_kms.c | 5 +--- drivers/gpu/drm/rcar-du/rcar_du_plane.c | 15 +++------ drivers/gpu/drm/rcar-du/rcar_du_plane.h | 2 +- drivers/gpu/drm/rcar-du/rcar_du_vsp.c | 42 ++------------------------ drivers/gpu/drm/rcar-du/rcar_du_vsp.h | 2 +- 6 files changed, 9 insertions(+), 58 deletions(-)
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_drv.h b/drivers/gpu/drm/rcar-du/rcar_du_drv.h index f400fde65a0c..dad87d449ef7 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_drv.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_drv.h @@ -90,7 +90,6 @@ struct rcar_du_device { struct rcar_du_vsp vsps[RCAR_DU_MAX_VSPS];
struct { - struct drm_property *alpha; struct drm_property *colorkey; } props;
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_kms.c b/drivers/gpu/drm/rcar-du/rcar_du_kms.c index 566d1a948c8f..e1b5a7b460cc 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_kms.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_kms.c @@ -417,11 +417,6 @@ static int rcar_du_encoders_init(struct rcar_du_device *rcdu)
static int rcar_du_properties_init(struct rcar_du_device *rcdu) { - rcdu->props.alpha = - drm_property_create_range(rcdu->ddev, 0, "alpha", 0, 255); - if (rcdu->props.alpha == NULL) - return -ENOMEM; - /* * The color key is expressed as an RGB888 triplet stored in a 32-bit * integer in XRGB8888 format. Bit 24 is used as a flag to disable (0) diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.c b/drivers/gpu/drm/rcar-du/rcar_du_plane.c index 5687a94d4cb1..136c3b464d81 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.c @@ -423,7 +423,7 @@ static void rcar_du_plane_setup_mode(struct rcar_du_group *rgrp, rcar_du_plane_write(rgrp, index, PnALPHAR, PnALPHAR_ABIT_0); else rcar_du_plane_write(rgrp, index, PnALPHAR, - PnALPHAR_ABIT_X | state->alpha); + PnALPHAR_ABIT_X | state->state.alpha);
pnmr = PnMR_BM_MD | state->format->pnmr;
@@ -697,11 +697,11 @@ static void rcar_du_plane_reset(struct drm_plane *plane)
state->hwindex = -1; state->source = RCAR_DU_PLANE_MEMORY; - state->alpha = 255; state->colorkey = RCAR_DU_COLORKEY_NONE; state->state.zpos = plane->type == DRM_PLANE_TYPE_PRIMARY ? 0 : 1;
plane->state = &state->state; + plane->state->alpha = 255; plane->state->plane = plane; }
@@ -713,9 +713,7 @@ static int rcar_du_plane_atomic_set_property(struct drm_plane *plane, struct rcar_du_plane_state *rstate = to_rcar_plane_state(state); struct rcar_du_device *rcdu = to_rcar_plane(plane)->group->dev;
- if (property == rcdu->props.alpha) - rstate->alpha = val; - else if (property == rcdu->props.colorkey) + if (property == rcdu->props.colorkey) rstate->colorkey = val; else return -EINVAL; @@ -731,9 +729,7 @@ static int rcar_du_plane_atomic_get_property(struct drm_plane *plane, container_of(state, const struct rcar_du_plane_state, state); struct rcar_du_device *rcdu = to_rcar_plane(plane)->group->dev;
- if (property == rcdu->props.alpha) - *val = rstate->alpha; - else if (property == rcdu->props.colorkey) + if (property == rcdu->props.colorkey) *val = rstate->colorkey; else return -EINVAL; @@ -802,10 +798,9 @@ int rcar_du_planes_init(struct rcar_du_group *rgrp) continue;
drm_object_attach_property(&plane->plane.base, - rcdu->props.alpha, 255); - drm_object_attach_property(&plane->plane.base, rcdu->props.colorkey, RCAR_DU_COLORKEY_NONE); + drm_plane_create_alpha_property(&plane->plane, 255); drm_plane_create_zpos_property(&plane->plane, 1, 1, 7); }
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_plane.h b/drivers/gpu/drm/rcar-du/rcar_du_plane.h index 890321b4665d..5c19c69e4691 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_plane.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_plane.h @@ -50,7 +50,6 @@ static inline struct rcar_du_plane *to_rcar_plane(struct drm_plane *plane) * @state: base DRM plane state * @format: information about the pixel format used by the plane * @hwindex: 0-based hardware plane index, -1 means unused - * @alpha: value of the plane alpha property * @colorkey: value of the plane colorkey property */ struct rcar_du_plane_state { @@ -60,7 +59,6 @@ struct rcar_du_plane_state { int hwindex; enum rcar_du_plane_source source;
- unsigned int alpha; unsigned int colorkey; };
diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c index 2c260c33840b..bda551c01a43 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.c +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.c @@ -54,6 +54,7 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc) }; struct rcar_du_plane_state state = { .state = { + .alpha = 255, .crtc = &crtc->crtc, .dst.x1 = 0, .dst.y1 = 0, @@ -67,7 +68,6 @@ void rcar_du_vsp_enable(struct rcar_du_crtc *crtc) }, .format = rcar_du_format_info(DRM_FORMAT_ARGB8888), .source = RCAR_DU_PLANE_VSPD1, - .alpha = 255, .colorkey = 0, };
@@ -173,7 +173,7 @@ static void rcar_du_vsp_plane_setup(struct rcar_du_vsp_plane *plane) struct vsp1_du_atomic_config cfg = { .pixelformat = 0, .pitch = fb->pitches[0], - .alpha = state->alpha, + .alpha = state->state.alpha, .zpos = state->state.zpos, }; unsigned int i; @@ -335,44 +335,13 @@ static void rcar_du_vsp_plane_reset(struct drm_plane *plane) if (state == NULL) return;
- state->alpha = 255; + state->state.alpha = 255; state->state.zpos = plane->type == DRM_PLANE_TYPE_PRIMARY ? 0 : 1;
plane->state = &state->state; plane->state->plane = plane; }
-static int rcar_du_vsp_plane_atomic_set_property(struct drm_plane *plane, - struct drm_plane_state *state, struct drm_property *property, - uint64_t val) -{ - struct rcar_du_vsp_plane_state *rstate = to_rcar_vsp_plane_state(state); - struct rcar_du_device *rcdu = to_rcar_vsp_plane(plane)->vsp->dev; - - if (property == rcdu->props.alpha) - rstate->alpha = val; - else - return -EINVAL; - - return 0; -} - -static int rcar_du_vsp_plane_atomic_get_property(struct drm_plane *plane, - const struct drm_plane_state *state, struct drm_property *property, - uint64_t *val) -{ - const struct rcar_du_vsp_plane_state *rstate = - container_of(state, const struct rcar_du_vsp_plane_state, state); - struct rcar_du_device *rcdu = to_rcar_vsp_plane(plane)->vsp->dev; - - if (property == rcdu->props.alpha) - *val = rstate->alpha; - else - return -EINVAL; - - return 0; -} - static const struct drm_plane_funcs rcar_du_vsp_plane_funcs = { .update_plane = drm_atomic_helper_update_plane, .disable_plane = drm_atomic_helper_disable_plane, @@ -380,8 +349,6 @@ static const struct drm_plane_funcs rcar_du_vsp_plane_funcs = { .destroy = drm_plane_cleanup, .atomic_duplicate_state = rcar_du_vsp_plane_atomic_duplicate_state, .atomic_destroy_state = rcar_du_vsp_plane_atomic_destroy_state, - .atomic_set_property = rcar_du_vsp_plane_atomic_set_property, - .atomic_get_property = rcar_du_vsp_plane_atomic_get_property, };
int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np, @@ -438,8 +405,7 @@ int rcar_du_vsp_init(struct rcar_du_vsp *vsp, struct device_node *np, if (type == DRM_PLANE_TYPE_PRIMARY) continue;
- drm_object_attach_property(&plane->plane.base, - rcdu->props.alpha, 255); + drm_plane_create_alpha_property(&plane->plane, 255); drm_plane_create_zpos_property(&plane->plane, 1, 1, vsp->num_planes - 1); } diff --git a/drivers/gpu/drm/rcar-du/rcar_du_vsp.h b/drivers/gpu/drm/rcar-du/rcar_du_vsp.h index f876c512163c..8b19914761e4 100644 --- a/drivers/gpu/drm/rcar-du/rcar_du_vsp.h +++ b/drivers/gpu/drm/rcar-du/rcar_du_vsp.h @@ -44,7 +44,6 @@ static inline struct rcar_du_vsp_plane *to_rcar_vsp_plane(struct drm_plane *p) * @state: base DRM plane state * @format: information about the pixel format used by the plane * @sg_tables: scatter-gather tables for the frame buffer memory - * @alpha: value of the plane alpha property * @zpos: value of the plane zpos property */ struct rcar_du_vsp_plane_state { @@ -53,7 +52,6 @@ struct rcar_du_vsp_plane_state { const struct rcar_du_format_info *format; struct sg_table sg_tables[3];
- unsigned int alpha; unsigned int zpos; };
Since we now have a way to enforce the zpos, check for the number of alpha planes, the only missing part is to assign our pipe automatically instead of hardcoding it.
The algorithm is quite simple, but requires two iterations over the list of planes.
In the first one (which is the same one that we've had to check for alpha, the frontend usage, and so on), we order the planes by their zpos.
We can then do a second iteration over that array by ascending zpos starting with the pipe 0. When and if we encounter our alpha plane, we put it and all the other subsequent planes in the second pipe.
Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com --- drivers/gpu/drm/sun4i/sun4i_backend.c | 40 ++++++++++++++++++++++++++-- drivers/gpu/drm/sun4i/sun4i_layer.c | 6 +---- drivers/gpu/drm/sun4i/sun4i_layer.h | 1 +- 3 files changed, 39 insertions(+), 8 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c index 245b189fc4d8..f387420bda8d 100644 --- a/drivers/gpu/drm/sun4i/sun4i_backend.c +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c @@ -275,12 +275,16 @@ int sun4i_backend_update_layer_zpos(struct sun4i_backend *backend, int layer, struct drm_plane *plane) { struct drm_plane_state *state = plane->state; + struct sun4i_layer_state *p_state = state_to_sun4i_layer_state(state); unsigned int priority = state->normalized_zpos; + unsigned int pipe = p_state->pipe;
- DRM_DEBUG_DRIVER("Setting layer %d's priority to %d\n", layer, priority); - + DRM_DEBUG_DRIVER("Setting layer %d's priority to %d and pipe %d\n", + layer, priority, pipe); regmap_update_bits(backend->engine.regs, SUN4I_BACKEND_ATTCTL_REG0(layer), + SUN4I_BACKEND_ATTCTL_REG0_LAY_PIPESEL_MASK | SUN4I_BACKEND_ATTCTL_REG0_LAY_PRISEL_MASK, + SUN4I_BACKEND_ATTCTL_REG0_LAY_PIPESEL(p_state->pipe) | SUN4I_BACKEND_ATTCTL_REG0_LAY_PRISEL(priority));
return 0; @@ -325,12 +329,15 @@ static void sun4i_backend_atomic_begin(struct sunxi_engine *engine, static int sun4i_backend_atomic_check(struct sunxi_engine *engine, struct drm_crtc_state *crtc_state) { + struct drm_plane_state *plane_states[SUN4I_BACKEND_NUM_LAYERS] = { 0 }; struct drm_atomic_state *state = crtc_state->state; struct drm_device *drm = state->dev; struct drm_plane *plane; unsigned int num_planes = 0; unsigned int num_alpha_planes = 0; unsigned int num_frontend_planes = 0; + unsigned int current_pipe = 0; + unsigned int i;
DRM_DEBUG_DRIVER("Starting checking our planes\n");
@@ -361,9 +368,19 @@ static int sun4i_backend_atomic_check(struct sunxi_engine *engine, if (fb->format->has_alpha) num_alpha_planes++;
+ DRM_DEBUG_DRIVER("Plane zpos is %d\n", + plane_state->normalized_zpos); + + /* Sort our planes by Zpos */ + plane_states[plane_state->normalized_zpos] = plane_state; + num_planes++; }
+ /* All our planes were disabled, bail out */ + if (!num_planes) + return 0; + /* * The hardware is a bit unusual here. * @@ -400,6 +417,25 @@ static int sun4i_backend_atomic_check(struct sunxi_engine *engine, return -EINVAL; }
+ /* We can't have an alpha plane at the lowest position */ + if (plane_states[0]->fb->format->has_alpha) + return -EINVAL; + + for (i = 1; i < num_planes; i++) { + struct drm_plane_state *p_state = plane_states[i]; + struct drm_framebuffer *fb = p_state->fb; + struct sun4i_layer_state *s_state = state_to_sun4i_layer_state(p_state); + + /* + * The only alpha position is the lowest plane of the + * second pipe. + */ + if (fb->format->has_alpha) + current_pipe++; + + s_state->pipe = current_pipe; + } + if (num_frontend_planes > SUN4I_BACKEND_NUM_FRONTEND_LAYERS) { DRM_DEBUG_DRIVER("Too many planes going through the frontend, rejecting\n"); return -EINVAL; diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c index 19be798e4fac..3e3d554713cb 100644 --- a/drivers/gpu/drm/sun4i/sun4i_layer.c +++ b/drivers/gpu/drm/sun4i/sun4i_layer.c @@ -220,12 +220,6 @@ struct drm_plane **sun4i_layers_init(struct drm_device *drm,
drm_plane_create_zpos_immutable_property(&layer->plane, i);
- DRM_DEBUG_DRIVER("Assigning %s plane to pipe %d\n", - i ? "overlay" : "primary", plane->pipe); - regmap_update_bits(engine->regs, SUN4I_BACKEND_ATTCTL_REG0(i), - SUN4I_BACKEND_ATTCTL_REG0_LAY_PIPESEL_MASK, - SUN4I_BACKEND_ATTCTL_REG0_LAY_PIPESEL(plane->pipe)); - layer->id = i; planes[i] = &layer->plane; }; diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.h b/drivers/gpu/drm/sun4i/sun4i_layer.h index 75b4868ba87c..36b20265bd31 100644 --- a/drivers/gpu/drm/sun4i/sun4i_layer.h +++ b/drivers/gpu/drm/sun4i/sun4i_layer.h @@ -24,6 +24,7 @@ struct sun4i_layer {
struct sun4i_layer_state { struct drm_plane_state state; + unsigned int pipe; bool uses_frontend; };
On Sat, Feb 17, 2018 at 1:39 AM, Maxime Ripard maxime.ripard@bootlin.com wrote:
Since we now have a way to enforce the zpos, check for the number of alpha planes, the only missing part is to assign our pipe automatically instead of hardcoding it.
The algorithm is quite simple, but requires two iterations over the list of planes.
In the first one (which is the same one that we've had to check for alpha, the frontend usage, and so on), we order the planes by their zpos.
We can then do a second iteration over that array by ascending zpos starting with the pipe 0. When and if we encounter our alpha plane, we put it and all the other subsequent planes in the second pipe.
Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com
Reviewed-by: Chen-Yu Tsai wens@csie.org
The plane description structure was mostly needed to differentiate the formats usable on the primary plane (because of its lowest position), and assign the pipes. Now that both are dynamically checked and assigned, we can remove the static definition.
Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com --- drivers/gpu/drm/sun4i/sun4i_layer.c | 44 +++++------------------------- 1 file changed, 8 insertions(+), 36 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c index 3e3d554713cb..85cc7e9461b3 100644 --- a/drivers/gpu/drm/sun4i/sun4i_layer.c +++ b/drivers/gpu/drm/sun4i/sun4i_layer.c @@ -19,13 +19,6 @@ #include "sun4i_layer.h" #include "sunxi_engine.h"
-struct sun4i_plane_desc { - enum drm_plane_type type; - u8 pipe; - const uint32_t *formats; - uint32_t nformats; -}; - static void sun4i_backend_layer_reset(struct drm_plane *plane) { struct sun4i_layer *layer = plane_to_sun4i_layer(plane); @@ -133,14 +126,7 @@ static const struct drm_plane_funcs sun4i_backend_layer_funcs = { .update_plane = drm_atomic_helper_update_plane, };
-static const uint32_t sun4i_backend_layer_formats_primary[] = { - DRM_FORMAT_ARGB8888, - DRM_FORMAT_RGB888, - DRM_FORMAT_RGB565, - DRM_FORMAT_XRGB8888, -}; - -static const uint32_t sun4i_backend_layer_formats_overlay[] = { +static const uint32_t sun4i_backend_layer_formats[] = { DRM_FORMAT_ARGB8888, DRM_FORMAT_ARGB4444, DRM_FORMAT_ARGB1555, @@ -151,24 +137,9 @@ static const uint32_t sun4i_backend_layer_formats_overlay[] = { DRM_FORMAT_XRGB8888, };
-static const struct sun4i_plane_desc sun4i_backend_planes[] = { - { - .type = DRM_PLANE_TYPE_PRIMARY, - .pipe = 0, - .formats = sun4i_backend_layer_formats_primary, - .nformats = ARRAY_SIZE(sun4i_backend_layer_formats_primary), - }, - { - .type = DRM_PLANE_TYPE_OVERLAY, - .pipe = 1, - .formats = sun4i_backend_layer_formats_overlay, - .nformats = ARRAY_SIZE(sun4i_backend_layer_formats_overlay), - }, -}; - static struct sun4i_layer *sun4i_layer_init_one(struct drm_device *drm, struct sun4i_backend *backend, - const struct sun4i_plane_desc *plane) + enum drm_plane_type type) { struct sun4i_layer *layer; int ret; @@ -180,8 +151,9 @@ static struct sun4i_layer *sun4i_layer_init_one(struct drm_device *drm, /* possible crtcs are set later */ ret = drm_universal_plane_init(drm, &layer->plane, 0, &sun4i_backend_layer_funcs, - plane->formats, plane->nformats, - NULL, plane->type, NULL); + sun4i_backend_layer_formats, + ARRAY_SIZE(sun4i_backend_layer_formats), + NULL, type, NULL); if (ret) { dev_err(drm->dev, "Couldn't initialize layer\n"); return ERR_PTR(ret); @@ -207,11 +179,11 @@ struct drm_plane **sun4i_layers_init(struct drm_device *drm, if (!planes) return ERR_PTR(-ENOMEM);
- for (i = 0; i < ARRAY_SIZE(sun4i_backend_planes); i++) { - const struct sun4i_plane_desc *plane = &sun4i_backend_planes[i]; + for (i = 0; i < SUN4I_BACKEND_NUM_LAYERS; i++) { + enum drm_plane_type type = i ? DRM_PLANE_TYPE_OVERLAY : DRM_PLANE_TYPE_PRIMARY; struct sun4i_layer *layer;
- layer = sun4i_layer_init_one(drm, backend, plane); + layer = sun4i_layer_init_one(drm, backend, type); if (IS_ERR(layer)) { dev_err(drm->dev, "Couldn't initialize %s plane\n", i ? "overlay" : "primary");
On Sat, Feb 17, 2018 at 1:39 AM, Maxime Ripard maxime.ripard@bootlin.com wrote:
The plane description structure was mostly needed to differentiate the formats usable on the primary plane (because of its lowest position), and assign the pipes. Now that both are dynamically checked and assigned, we can remove the static definition.
Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com
Reviewed-by: Chen-Yu Tsai wens@csie.org
Now that we have everything in place, we can make zpos configurable now. Change the zpos property from an immutable one to a regular.
Reviewed-by: Chen-Yu Tsai wens@csie.org Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com --- drivers/gpu/drm/sun4i/sun4i_layer.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c index 85cc7e9461b3..33ad377569ec 100644 --- a/drivers/gpu/drm/sun4i/sun4i_layer.c +++ b/drivers/gpu/drm/sun4i/sun4i_layer.c @@ -163,6 +163,9 @@ static struct sun4i_layer *sun4i_layer_init_one(struct drm_device *drm, &sun4i_backend_layer_helper_funcs); layer->backend = backend;
+ drm_plane_create_zpos_property(&layer->plane, 0, 0, + SUN4I_BACKEND_NUM_LAYERS - 1); + return layer; }
@@ -190,8 +193,6 @@ struct drm_plane **sun4i_layers_init(struct drm_device *drm, return ERR_CAST(layer); };
- drm_plane_create_zpos_immutable_property(&layer->plane, i); - layer->id = i; planes[i] = &layer->plane; };
Our backend supports a per-plane alpha property. Support it through our new helper.
Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com --- drivers/gpu/drm/sun4i/sun4i_backend.c | 16 +++++++++++++--- drivers/gpu/drm/sun4i/sun4i_backend.h | 3 +++ drivers/gpu/drm/sun4i/sun4i_layer.c | 2 ++ 3 files changed, 18 insertions(+), 3 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c index f387420bda8d..0ec00600eace 100644 --- a/drivers/gpu/drm/sun4i/sun4i_backend.c +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c @@ -191,6 +191,15 @@ int sun4i_backend_update_layer_formats(struct sun4i_backend *backend, DRM_DEBUG_DRIVER("Switching display backend interlaced mode %s\n", interlaced ? "on" : "off");
+ val = SUN4I_BACKEND_ATTCTL_REG0_LAY_GLBALPHA(state->alpha); + if (state->alpha != 255) + val |= SUN4I_BACKEND_ATTCTL_REG0_LAY_GLBALPHA_EN; + regmap_update_bits(backend->engine.regs, + SUN4I_BACKEND_ATTCTL_REG0(layer), + SUN4I_BACKEND_ATTCTL_REG0_LAY_GLBALPHA_MASK | + SUN4I_BACKEND_ATTCTL_REG0_LAY_GLBALPHA_EN, + val); + ret = sun4i_backend_drm_format_to_layer(plane, fb->format->format, &val); if (ret) { @@ -365,7 +374,7 @@ static int sun4i_backend_atomic_check(struct sunxi_engine *engine, DRM_DEBUG_DRIVER("Plane FB format is %s\n", drm_get_format_name(fb->format->format, &format_name)); - if (fb->format->has_alpha) + if (fb->format->has_alpha || (plane_state->alpha != 255)) num_alpha_planes++;
DRM_DEBUG_DRIVER("Plane zpos is %d\n", @@ -418,7 +427,8 @@ static int sun4i_backend_atomic_check(struct sunxi_engine *engine, }
/* We can't have an alpha plane at the lowest position */ - if (plane_states[0]->fb->format->has_alpha) + if (plane_states[0]->fb->format->has_alpha || + (plane_states[0]->alpha != 255)) return -EINVAL;
for (i = 1; i < num_planes; i++) { @@ -430,7 +440,7 @@ static int sun4i_backend_atomic_check(struct sunxi_engine *engine, * The only alpha position is the lowest plane of the * second pipe. */ - if (fb->format->has_alpha) + if (fb->format->has_alpha || (p_state->alpha != 255)) current_pipe++;
s_state->pipe = current_pipe; diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.h b/drivers/gpu/drm/sun4i/sun4i_backend.h index 52e77591186a..03294d5dd1a2 100644 --- a/drivers/gpu/drm/sun4i/sun4i_backend.h +++ b/drivers/gpu/drm/sun4i/sun4i_backend.h @@ -68,11 +68,14 @@ #define SUN4I_BACKEND_CKMIN_REG 0x884 #define SUN4I_BACKEND_CKCFG_REG 0x888 #define SUN4I_BACKEND_ATTCTL_REG0(l) (0x890 + (0x4 * (l))) +#define SUN4I_BACKEND_ATTCTL_REG0_LAY_GLBALPHA_MASK GENMASK(31, 24) +#define SUN4I_BACKEND_ATTCTL_REG0_LAY_GLBALPHA(x) ((x) << 24) #define SUN4I_BACKEND_ATTCTL_REG0_LAY_PIPESEL_MASK BIT(15) #define SUN4I_BACKEND_ATTCTL_REG0_LAY_PIPESEL(x) ((x) << 15) #define SUN4I_BACKEND_ATTCTL_REG0_LAY_PRISEL_MASK GENMASK(11, 10) #define SUN4I_BACKEND_ATTCTL_REG0_LAY_PRISEL(x) ((x) << 10) #define SUN4I_BACKEND_ATTCTL_REG0_LAY_VDOEN BIT(1) +#define SUN4I_BACKEND_ATTCTL_REG0_LAY_GLBALPHA_EN BIT(0)
#define SUN4I_BACKEND_ATTCTL_REG1(l) (0x8a0 + (0x4 * (l))) #define SUN4I_BACKEND_ATTCTL_REG1_LAY_HSCAFCT GENMASK(15, 14) diff --git a/drivers/gpu/drm/sun4i/sun4i_layer.c b/drivers/gpu/drm/sun4i/sun4i_layer.c index 33ad377569ec..f491fd963534 100644 --- a/drivers/gpu/drm/sun4i/sun4i_layer.c +++ b/drivers/gpu/drm/sun4i/sun4i_layer.c @@ -37,6 +37,7 @@ static void sun4i_backend_layer_reset(struct drm_plane *plane) if (state) { plane->state = &state->state; plane->state->plane = plane; + plane->state->alpha = 255; plane->state->zpos = layer->id; } } @@ -163,6 +164,7 @@ static struct sun4i_layer *sun4i_layer_init_one(struct drm_device *drm, &sun4i_backend_layer_helper_funcs); layer->backend = backend;
+ drm_plane_create_alpha_property(&layer->plane, 255); drm_plane_create_zpos_property(&layer->plane, 0, 0, SUN4I_BACKEND_NUM_LAYERS - 1);
On Sat, Feb 17, 2018 at 1:39 AM, Maxime Ripard maxime.ripard@bootlin.com wrote:
Our backend supports a per-plane alpha property. Support it through our new helper.
Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com
Reviewed-by: Chen-Yu Tsai wens@csie.org
Though, not having a graphics background, does alpha = 255 mean fully transparent and thus can be skipped? Or no alpha at all?
Thanks
On Thu, Feb 22, 2018 at 10:17:38PM +0800, Chen-Yu Tsai wrote:
On Sat, Feb 17, 2018 at 1:39 AM, Maxime Ripard maxime.ripard@bootlin.com wrote:
Our backend supports a per-plane alpha property. Support it through our new helper.
Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com
Reviewed-by: Chen-Yu Tsai wens@csie.org
Though, not having a graphics background, does alpha = 255 mean fully transparent and thus can be skipped? Or no alpha at all?
It means that it's fully opaque, and therefore we are in the same situation than if we had no alpha channel at all.
Maxime
On Thu, Feb 22, 2018 at 10:34 PM, Maxime Ripard maxime.ripard@bootlin.com wrote:
On Thu, Feb 22, 2018 at 10:17:38PM +0800, Chen-Yu Tsai wrote:
On Sat, Feb 17, 2018 at 1:39 AM, Maxime Ripard maxime.ripard@bootlin.com wrote:
Our backend supports a per-plane alpha property. Support it through our new helper.
Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com
Reviewed-by: Chen-Yu Tsai wens@csie.org
Though, not having a graphics background, does alpha = 255 mean fully transparent and thus can be skipped? Or no alpha at all?
It means that it's fully opaque, and therefore we are in the same situation than if we had no alpha channel at all.
Right. Then the code makes sense. Thanks for the explanation.
We've had some code for quite some time to prevent the alpha bug from happening on the lowest primary plane. Since we now check for this in our atomic_check, we can simply remove it.
Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com --- drivers/gpu/drm/sun4i/sun4i_backend.c | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/sun4i/sun4i_backend.c b/drivers/gpu/drm/sun4i/sun4i_backend.c index 0ec00600eace..ad2d39f6ba20 100644 --- a/drivers/gpu/drm/sun4i/sun4i_backend.c +++ b/drivers/gpu/drm/sun4i/sun4i_backend.c @@ -92,13 +92,8 @@ void sun4i_backend_layer_enable(struct sun4i_backend *backend, SUN4I_BACKEND_MODCTL_LAY_EN(layer), val); }
-static int sun4i_backend_drm_format_to_layer(struct drm_plane *plane, - u32 format, u32 *mode) +static int sun4i_backend_drm_format_to_layer(u32 format, u32 *mode) { - if (plane && (plane->type == DRM_PLANE_TYPE_PRIMARY) && - (format == DRM_FORMAT_ARGB8888)) - format = DRM_FORMAT_XRGB8888; - switch (format) { case DRM_FORMAT_ARGB8888: *mode = SUN4I_BACKEND_LAY_FBFMT_ARGB8888; @@ -200,8 +195,7 @@ int sun4i_backend_update_layer_formats(struct sun4i_backend *backend, SUN4I_BACKEND_ATTCTL_REG0_LAY_GLBALPHA_EN, val);
- ret = sun4i_backend_drm_format_to_layer(plane, fb->format->format, - &val); + ret = sun4i_backend_drm_format_to_layer(fb->format->format, &val); if (ret) { DRM_DEBUG_DRIVER("Invalid format\n"); return ret; @@ -220,7 +214,7 @@ int sun4i_backend_update_layer_frontend(struct sun4i_backend *backend, u32 val; int ret;
- ret = sun4i_backend_drm_format_to_layer(NULL, fmt, &val); + ret = sun4i_backend_drm_format_to_layer(fmt, &val); if (ret) { DRM_DEBUG_DRIVER("Invalid format\n"); return ret;
On Sat, Feb 17, 2018 at 1:39 AM, Maxime Ripard maxime.ripard@bootlin.com wrote:
We've had some code for quite some time to prevent the alpha bug from happening on the lowest primary plane. Since we now check for this in our atomic_check, we can simply remove it.
Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com
Reviewed-by: Chen-Yu Tsai wens@csie.org
On Thu, Feb 22, 2018 at 10:15:07PM +0800, Chen-Yu Tsai wrote:
On Sat, Feb 17, 2018 at 1:39 AM, Maxime Ripard maxime.ripard@bootlin.com wrote:
We've had some code for quite some time to prevent the alpha bug from happening on the lowest primary plane. Since we now check for this in our atomic_check, we can simply remove it.
Signed-off-by: Maxime Ripard maxime.ripard@bootlin.com
Reviewed-by: Chen-Yu Tsai wens@csie.org
Applied the patches 4, 5, 6 and 8 with your reviewed-by.
Thanks! Maxime
dri-devel@lists.freedesktop.org
-
Chen-Yu Tsai -
Daniel Vetter -
Laurent Pinchart -
Maxime Ripard -
Stefan Schake -
Ville Syrjälä