Hello,
In this version I've reduced color keying modes and properties to a bare minimum because considering several modes and properties at once might take quite a lot of effort due to a variety of HW capabilities. This allows us to start easy with the generic colorkey properties support.
For the starter let's implement probably the most common (and simple) color keying mode - the "green screen" (or "chroma key") mode. More advanced modes and features could be implemented later on by as needed basis.
Following Ville's Syrjälä review comments to v2, the color key value is now given in ARGB16161616 format. Drivers have to convert this 16bpc format into internal color key value representation themselves. This works well for cases where conversion is done to a non-planar integer formats, but I'm not sure how drivers are supposed to cope with cases where conversion involves churning with fixed point math / floating point representation. Comments are welcome.
v2: https://lists.freedesktop.org/archives/dri-devel/2018-May/178408.html v1: https://lists.freedesktop.org/archives/dri-devel/2017-December/160510.html
Dmitry Osipenko (1): drm/tegra: plane: Implement generic colorkey property for older Tegra's
Laurent Pinchart (1): drm: Add generic colorkey properties for DRM planes
drivers/gpu/drm/drm_atomic.c | 12 ++++ drivers/gpu/drm/drm_blend.c | 99 +++++++++++++++++++++++++++++++++ drivers/gpu/drm/tegra/dc.c | 25 +++++++++ drivers/gpu/drm/tegra/dc.h | 7 +++ drivers/gpu/drm/tegra/plane.c | 102 ++++++++++++++++++++++++++++++++++ include/drm/drm_blend.h | 3 + include/drm/drm_plane.h | 53 ++++++++++++++++++ 7 files changed, 301 insertions(+)
From: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
Color keying is the action of replacing pixels matching a given color (or range of colors) with transparent pixels in an overlay when performing blitting. Depending on the hardware capabilities, the matching pixel can either become fully transparent or gain adjustment of the pixels component values.
Color keying is found in a large number of devices whose capabilities often differ, but they still have enough common features in range to standardize color key properties. This commit adds three generic DRM plane properties related to the color keying, providing initial color keying support.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Signed-off-by: Dmitry Osipenko digetx@gmail.com --- drivers/gpu/drm/drm_atomic.c | 12 +++++ drivers/gpu/drm/drm_blend.c | 99 ++++++++++++++++++++++++++++++++++++ include/drm/drm_blend.h | 3 ++ include/drm/drm_plane.h | 53 +++++++++++++++++++ 4 files changed, 167 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 895741e9cd7d..b322cbed319b 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -799,6 +799,12 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, state->rotation = val; } else if (property == plane->zpos_property) { state->zpos = val; + } else if (property == plane->colorkey.mode_property) { + state->colorkey.mode = val; + } else if (property == plane->colorkey.min_property) { + state->colorkey.min = val; + } else if (property == plane->colorkey.max_property) { + state->colorkey.max = val; } else if (property == plane->color_encoding_property) { state->color_encoding = val; } else if (property == plane->color_range_property) { @@ -864,6 +870,12 @@ drm_atomic_plane_get_property(struct drm_plane *plane, *val = state->rotation; } else if (property == plane->zpos_property) { *val = state->zpos; + } else if (property == plane->colorkey.mode_property) { + *val = state->colorkey.mode; + } else if (property == plane->colorkey.min_property) { + *val = state->colorkey.min; + } else if (property == plane->colorkey.max_property) { + *val = state->colorkey.max; } else if (property == plane->color_encoding_property) { *val = state->color_encoding; } else if (property == plane->color_range_property) { diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c index a16a74d7e15e..12fed2ff65c8 100644 --- a/drivers/gpu/drm/drm_blend.c +++ b/drivers/gpu/drm/drm_blend.c @@ -107,6 +107,11 @@ * planes. Without this property the primary plane is always below the cursor * plane, and ordering between all other planes is undefined. * + * colorkey: + * Color keying is set up with drm_plane_create_colorkey_properties(). + * It adds support for replacing a range of colors with a transparent + * color in the plane. + * * Note that all the property extensions described here apply either to the * plane or the CRTC (e.g. for the background color, which currently is not * exposed and assumed to be black). @@ -448,3 +453,97 @@ int drm_atomic_normalize_zpos(struct drm_device *dev, return 0; } EXPORT_SYMBOL(drm_atomic_normalize_zpos); + +static const char * const plane_colorkey_mode_name[] = { + [DRM_PLANE_COLORKEY_MODE_DISABLED] = "disabled", + [DRM_PLANE_COLORKEY_MODE_FOREGROUND_CLIP] = "foreground-clip", +}; + +/** + * drm_plane_create_colorkey_properties - create colorkey properties + * @plane: drm plane + * @supported_modes: bitmask of supported color keying modes + * + * This function creates the generic color keying properties and attach them to + * the plane to enable color keying control for blending operations. + * + * Color keying is controlled by these properties: + * + * colorkey.mode: + * The mode is an enumerated property that controls how color keying + * operates. + * + * colorkey.min, colorkey.max: + * These two properties specify the colors that are treated as the color + * key. Pixel whose value is in the [min, max] range is the color key + * matching pixel. The minimum and maximum values are expressed as a + * 64-bit integer in ARGB16161616 format, where A is the alpha value and + * R, G and B correspond to the color components. Drivers shall convert + * ARGB16161616 value into appropriate format within planes atomic check. + * + * When a single color key is desired instead of a range, userspace shall + * set the min and max properties to the same value. + * + * Drivers return an error from their plane atomic check if range can't be + * handled. + * + * Returns: + * Zero on success, negative errno on failure. + */ +int drm_plane_create_colorkey_properties(struct drm_plane *plane, + u32 supported_modes) +{ + struct drm_prop_enum_list modes_list[DRM_PLANE_COLORKEY_MODES_NUM]; + struct drm_property *mode_prop; + struct drm_property *min_prop; + struct drm_property *max_prop; + unsigned int modes_num = 0; + unsigned int i; + + /* modes are driver-specific, build the list of supported modes */ + for (i = 0; i < DRM_PLANE_COLORKEY_MODES_NUM; i++) { + if (!(supported_modes & BIT(i))) + continue; + + modes_list[modes_num].name = plane_colorkey_mode_name[i]; + modes_list[modes_num].type = i; + modes_num++; + } + + /* at least one mode should be supported */ + if (!modes_num) + return -EINVAL; + + mode_prop = drm_property_create_enum(plane->dev, 0, "colorkey.mode", + modes_list, modes_num); + if (!mode_prop) + return -ENOMEM; + + min_prop = drm_property_create_range(plane->dev, 0, "colorkey.min", + 0, U64_MAX); + if (!min_prop) + goto err_destroy_mode_prop; + + max_prop = drm_property_create_range(plane->dev, 0, "colorkey.max", + 0, U64_MAX); + if (!max_prop) + goto err_destroy_min_prop; + + drm_object_attach_property(&plane->base, mode_prop, 0); + drm_object_attach_property(&plane->base, min_prop, 0); + drm_object_attach_property(&plane->base, max_prop, 0); + + plane->colorkey.mode_property = mode_prop; + plane->colorkey.min_property = min_prop; + plane->colorkey.max_property = max_prop; + + return 0; + +err_destroy_min_prop: + drm_property_destroy(plane->dev, min_prop); +err_destroy_mode_prop: + drm_property_destroy(plane->dev, mode_prop); + + return -ENOMEM; +} +EXPORT_SYMBOL(drm_plane_create_colorkey_properties); diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h index 330c561c4c11..8e80d33b643e 100644 --- a/include/drm/drm_blend.h +++ b/include/drm/drm_blend.h @@ -52,4 +52,7 @@ int drm_plane_create_zpos_immutable_property(struct drm_plane *plane, unsigned int zpos); int drm_atomic_normalize_zpos(struct drm_device *dev, struct drm_atomic_state *state); + +int drm_plane_create_colorkey_properties(struct drm_plane *plane, + u32 supported_modes); #endif diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 26fa50c2a50e..9a621e1ccc47 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -32,6 +32,48 @@ struct drm_crtc; struct drm_printer; struct drm_modeset_acquire_ctx;
+/** + * enum drm_plane_colorkey_mode - uapi plane colorkey mode enumeration + */ +enum drm_plane_colorkey_mode { + /** + * @DRM_PLANE_COLORKEY_MODE_DISABLED: + * + * No color matching performed in this mode. + */ + DRM_PLANE_COLORKEY_MODE_DISABLED, + + /** + * @DRM_PLANE_COLORKEY_MODE_FOREGROUND_CLIP: + * + * This mode is also known as a "green screen". Plane pixels are + * transparent in areas where pixels match a given color key range + * and there is a bottom (background) plane, in other cases plane + * pixels are unaffected. + * + */ + DRM_PLANE_COLORKEY_MODE_FOREGROUND_CLIP, + + /** + * @DRM_PLANE_COLORKEY_MODES_NUM: + * + * Total number of color keying modes. + */ + DRM_PLANE_COLORKEY_MODES_NUM, +}; + +/** + * struct drm_plane_colorkey_state - plane color keying state + * @colorkey.mode: color keying mode + * @colorkey.min: color key range minimum (in ARGB16161616 format) + * @colorkey.max: color key range maximum (in ARGB16161616 format) + */ +struct drm_plane_colorkey_state { + enum drm_plane_colorkey_mode mode; + u64 min; + u64 max; +}; + /** * struct drm_plane_state - mutable plane state * @plane: backpointer to the plane @@ -54,6 +96,7 @@ struct drm_modeset_acquire_ctx; * where N is the number of active planes for given crtc. Note that * the driver must set drm_mode_config.normalize_zpos or call * drm_atomic_normalize_zpos() to update this before it can be trusted. + * @colorkey: colorkey state * @src: clipped source coordinates of the plane (in 16.16) * @dst: clipped destination coordinates of the plane * @state: backpointer to global drm_atomic_state @@ -124,6 +167,9 @@ struct drm_plane_state { unsigned int zpos; unsigned int normalized_zpos;
+ /* Plane colorkey */ + struct drm_plane_colorkey_state colorkey; + /** * @color_encoding: * @@ -510,6 +556,7 @@ enum drm_plane_type { * @alpha_property: alpha property for this plane * @zpos_property: zpos property for this plane * @rotation_property: rotation property for this plane + * @colorkey: colorkey properties for this plane * @helper_private: mid-layer private data */ struct drm_plane { @@ -587,6 +634,12 @@ struct drm_plane { struct drm_property *zpos_property; struct drm_property *rotation_property;
+ struct { + struct drm_property *mode_property; + struct drm_property *min_property; + struct drm_property *max_property; + } colorkey; + /** * @color_encoding_property: *
Hey,
Op 04-06-18 om 00:00 schreef Dmitry Osipenko:
From: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
Color keying is the action of replacing pixels matching a given color (or range of colors) with transparent pixels in an overlay when performing blitting. Depending on the hardware capabilities, the matching pixel can either become fully transparent or gain adjustment of the pixels component values.
Color keying is found in a large number of devices whose capabilities often differ, but they still have enough common features in range to standardize color key properties. This commit adds three generic DRM plane properties related to the color keying, providing initial color keying support.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Signed-off-by: Dmitry Osipenko digetx@gmail.com
drivers/gpu/drm/drm_atomic.c | 12 +++++ drivers/gpu/drm/drm_blend.c | 99 ++++++++++++++++++++++++++++++++++++ include/drm/drm_blend.h | 3 ++ include/drm/drm_plane.h | 53 +++++++++++++++++++ 4 files changed, 167 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 895741e9cd7d..b322cbed319b 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -799,6 +799,12 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, state->rotation = val; } else if (property == plane->zpos_property) { state->zpos = val;
- } else if (property == plane->colorkey.mode_property) {
state->colorkey.mode = val;- } else if (property == plane->colorkey.min_property) {
state->colorkey.min = val;- } else if (property == plane->colorkey.max_property) {
state->colorkey.max = val;@@ -864,6 +870,12 @@ drm_atomic_plane_get_property(struct drm_plane *plane, *val = state->rotation; } else if (property == plane->zpos_property) { *val = state->zpos;
- } else if (property == plane->colorkey.mode_property) {
*val = state->colorkey.mode;- } else if (property == plane->colorkey.min_property) {
*val = state->colorkey.min;- } else if (property == plane->colorkey.max_property) {
*val = state->colorkey.max;diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c index a16a74d7e15e..12fed2ff65c8 100644 --- a/drivers/gpu/drm/drm_blend.c +++ b/drivers/gpu/drm/drm_blend.c @@ -107,6 +107,11 @@
- planes. Without this property the primary plane is always below the cursor
- plane, and ordering between all other planes is undefined.
- colorkey:
- Color keying is set up with drm_plane_create_colorkey_properties().
- It adds support for replacing a range of colors with a transparent
- color in the plane.
- Note that all the property extensions described here apply either to the
- plane or the CRTC (e.g. for the background color, which currently is not
- exposed and assumed to be black).
@@ -448,3 +453,97 @@ int drm_atomic_normalize_zpos(struct drm_device *dev, return 0; } EXPORT_SYMBOL(drm_atomic_normalize_zpos);
+static const char * const plane_colorkey_mode_name[] = {
- [DRM_PLANE_COLORKEY_MODE_DISABLED] = "disabled",
- [DRM_PLANE_COLORKEY_MODE_FOREGROUND_CLIP] = "foreground-clip",
+};
+/**
- drm_plane_create_colorkey_properties - create colorkey properties
- @plane: drm plane
- @supported_modes: bitmask of supported color keying modes
- This function creates the generic color keying properties and attach them to
- the plane to enable color keying control for blending operations.
- Color keying is controlled by these properties:
- colorkey.mode:
- The mode is an enumerated property that controls how color keying
- operates.
- colorkey.min, colorkey.max:
- These two properties specify the colors that are treated as the color
- key. Pixel whose value is in the [min, max] range is the color key
- matching pixel. The minimum and maximum values are expressed as a
- 64-bit integer in ARGB16161616 format, where A is the alpha value and
- R, G and B correspond to the color components. Drivers shall convert
- ARGB16161616 value into appropriate format within planes atomic check.
- When a single color key is desired instead of a range, userspace shall
- set the min and max properties to the same value.
- Drivers return an error from their plane atomic check if range can't be
- handled.
- Returns:
- Zero on success, negative errno on failure.
- */
+int drm_plane_create_colorkey_properties(struct drm_plane *plane,
u32 supported_modes)+{
- struct drm_prop_enum_list modes_list[DRM_PLANE_COLORKEY_MODES_NUM];
- struct drm_property *mode_prop;
- struct drm_property *min_prop;
- struct drm_property *max_prop;
- unsigned int modes_num = 0;
- unsigned int i;
- /* modes are driver-specific, build the list of supported modes */
- for (i = 0; i < DRM_PLANE_COLORKEY_MODES_NUM; i++) {
if (!(supported_modes & BIT(i)))continue;modes_list[modes_num].name = plane_colorkey_mode_name[i];modes_list[modes_num].type = i;modes_num++;- }
- /* at least one mode should be supported */
- if (!modes_num)
return -EINVAL;- mode_prop = drm_property_create_enum(plane->dev, 0, "colorkey.mode",
modes_list, modes_num);- if (!mode_prop)
return -ENOMEM;- min_prop = drm_property_create_range(plane->dev, 0, "colorkey.min",
0, U64_MAX);- if (!min_prop)
goto err_destroy_mode_prop;- max_prop = drm_property_create_range(plane->dev, 0, "colorkey.max",
0, U64_MAX);- if (!max_prop)
goto err_destroy_min_prop;- drm_object_attach_property(&plane->base, mode_prop, 0);
- drm_object_attach_property(&plane->base, min_prop, 0);
- drm_object_attach_property(&plane->base, max_prop, 0);
- plane->colorkey.mode_property = mode_prop;
- plane->colorkey.min_property = min_prop;
- plane->colorkey.max_property = max_prop;
- return 0;
+err_destroy_min_prop:
- drm_property_destroy(plane->dev, min_prop);
+err_destroy_mode_prop:
- drm_property_destroy(plane->dev, mode_prop);
- return -ENOMEM;
+} +EXPORT_SYMBOL(drm_plane_create_colorkey_properties); diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h index 330c561c4c11..8e80d33b643e 100644 --- a/include/drm/drm_blend.h +++ b/include/drm/drm_blend.h @@ -52,4 +52,7 @@ int drm_plane_create_zpos_immutable_property(struct drm_plane *plane, unsigned int zpos); int drm_atomic_normalize_zpos(struct drm_device *dev, struct drm_atomic_state *state);
+int drm_plane_create_colorkey_properties(struct drm_plane *plane,
u32 supported_modes);#endif diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 26fa50c2a50e..9a621e1ccc47 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -32,6 +32,48 @@ struct drm_crtc; struct drm_printer; struct drm_modeset_acquire_ctx;
+/**
- enum drm_plane_colorkey_mode - uapi plane colorkey mode enumeration
- */
+enum drm_plane_colorkey_mode {
- /**
* @DRM_PLANE_COLORKEY_MODE_DISABLED:** No color matching performed in this mode.*/- DRM_PLANE_COLORKEY_MODE_DISABLED,
- /**
* @DRM_PLANE_COLORKEY_MODE_FOREGROUND_CLIP:** This mode is also known as a "green screen". Plane pixels are* transparent in areas where pixels match a given color key range* and there is a bottom (background) plane, in other cases plane* pixels are unaffected.**/- DRM_PLANE_COLORKEY_MODE_FOREGROUND_CLIP,
Could we add background clip as well?
Would be nice if we could map i915's legacy ioctl handler to the new color key mode.
- /**
* @DRM_PLANE_COLORKEY_MODES_NUM:** Total number of color keying modes.*/- DRM_PLANE_COLORKEY_MODES_NUM,
+};
+/**
- struct drm_plane_colorkey_state - plane color keying state
- @colorkey.mode: color keying mode
- @colorkey.min: color key range minimum (in ARGB16161616 format)
- @colorkey.max: color key range maximum (in ARGB16161616 format)
- */
+struct drm_plane_colorkey_state {
- enum drm_plane_colorkey_mode mode;
- u64 min;
- u64 max;
+};
Could we have some macros to extract the components for min/max? A, R, G, B.
~Maarten
On Fri, Jul 06, 2018 at 02:11:44PM +0200, Maarten Lankhorst wrote:
Hey,
Op 04-06-18 om 00:00 schreef Dmitry Osipenko:
From: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
Color keying is the action of replacing pixels matching a given color (or range of colors) with transparent pixels in an overlay when performing blitting. Depending on the hardware capabilities, the matching pixel can either become fully transparent or gain adjustment of the pixels component values.
Color keying is found in a large number of devices whose capabilities often differ, but they still have enough common features in range to standardize color key properties. This commit adds three generic DRM plane properties related to the color keying, providing initial color keying support.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Signed-off-by: Dmitry Osipenko digetx@gmail.com
drivers/gpu/drm/drm_atomic.c | 12 +++++ drivers/gpu/drm/drm_blend.c | 99 ++++++++++++++++++++++++++++++++++++ include/drm/drm_blend.h | 3 ++ include/drm/drm_plane.h | 53 +++++++++++++++++++ 4 files changed, 167 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 895741e9cd7d..b322cbed319b 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -799,6 +799,12 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, state->rotation = val; } else if (property == plane->zpos_property) { state->zpos = val;
- } else if (property == plane->colorkey.mode_property) {
state->colorkey.mode = val;- } else if (property == plane->colorkey.min_property) {
state->colorkey.min = val;- } else if (property == plane->colorkey.max_property) {
state->colorkey.max = val;@@ -864,6 +870,12 @@ drm_atomic_plane_get_property(struct drm_plane *plane, *val = state->rotation; } else if (property == plane->zpos_property) { *val = state->zpos;
- } else if (property == plane->colorkey.mode_property) {
*val = state->colorkey.mode;- } else if (property == plane->colorkey.min_property) {
*val = state->colorkey.min;- } else if (property == plane->colorkey.max_property) {
*val = state->colorkey.max;diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c index a16a74d7e15e..12fed2ff65c8 100644 --- a/drivers/gpu/drm/drm_blend.c +++ b/drivers/gpu/drm/drm_blend.c @@ -107,6 +107,11 @@
- planes. Without this property the primary plane is always below the cursor
- plane, and ordering between all other planes is undefined.
- colorkey:
- Color keying is set up with drm_plane_create_colorkey_properties().
- It adds support for replacing a range of colors with a transparent
- color in the plane.
- Note that all the property extensions described here apply either to the
- plane or the CRTC (e.g. for the background color, which currently is not
- exposed and assumed to be black).
@@ -448,3 +453,97 @@ int drm_atomic_normalize_zpos(struct drm_device *dev, return 0; } EXPORT_SYMBOL(drm_atomic_normalize_zpos);
+static const char * const plane_colorkey_mode_name[] = {
- [DRM_PLANE_COLORKEY_MODE_DISABLED] = "disabled",
- [DRM_PLANE_COLORKEY_MODE_FOREGROUND_CLIP] = "foreground-clip",
+};
+/**
- drm_plane_create_colorkey_properties - create colorkey properties
- @plane: drm plane
- @supported_modes: bitmask of supported color keying modes
- This function creates the generic color keying properties and attach them to
- the plane to enable color keying control for blending operations.
- Color keying is controlled by these properties:
- colorkey.mode:
- The mode is an enumerated property that controls how color keying
- operates.
- colorkey.min, colorkey.max:
- These two properties specify the colors that are treated as the color
- key. Pixel whose value is in the [min, max] range is the color key
- matching pixel. The minimum and maximum values are expressed as a
- 64-bit integer in ARGB16161616 format, where A is the alpha value and
- R, G and B correspond to the color components. Drivers shall convert
- ARGB16161616 value into appropriate format within planes atomic check.
- When a single color key is desired instead of a range, userspace shall
- set the min and max properties to the same value.
- Drivers return an error from their plane atomic check if range can't be
- handled.
- Returns:
- Zero on success, negative errno on failure.
- */
+int drm_plane_create_colorkey_properties(struct drm_plane *plane,
u32 supported_modes)+{
- struct drm_prop_enum_list modes_list[DRM_PLANE_COLORKEY_MODES_NUM];
- struct drm_property *mode_prop;
- struct drm_property *min_prop;
- struct drm_property *max_prop;
- unsigned int modes_num = 0;
- unsigned int i;
- /* modes are driver-specific, build the list of supported modes */
- for (i = 0; i < DRM_PLANE_COLORKEY_MODES_NUM; i++) {
if (!(supported_modes & BIT(i)))continue;modes_list[modes_num].name = plane_colorkey_mode_name[i];modes_list[modes_num].type = i;modes_num++;- }
- /* at least one mode should be supported */
- if (!modes_num)
return -EINVAL;- mode_prop = drm_property_create_enum(plane->dev, 0, "colorkey.mode",
modes_list, modes_num);- if (!mode_prop)
return -ENOMEM;- min_prop = drm_property_create_range(plane->dev, 0, "colorkey.min",
0, U64_MAX);- if (!min_prop)
goto err_destroy_mode_prop;- max_prop = drm_property_create_range(plane->dev, 0, "colorkey.max",
0, U64_MAX);- if (!max_prop)
goto err_destroy_min_prop;- drm_object_attach_property(&plane->base, mode_prop, 0);
- drm_object_attach_property(&plane->base, min_prop, 0);
- drm_object_attach_property(&plane->base, max_prop, 0);
- plane->colorkey.mode_property = mode_prop;
- plane->colorkey.min_property = min_prop;
- plane->colorkey.max_property = max_prop;
- return 0;
+err_destroy_min_prop:
- drm_property_destroy(plane->dev, min_prop);
+err_destroy_mode_prop:
- drm_property_destroy(plane->dev, mode_prop);
- return -ENOMEM;
+} +EXPORT_SYMBOL(drm_plane_create_colorkey_properties); diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h index 330c561c4c11..8e80d33b643e 100644 --- a/include/drm/drm_blend.h +++ b/include/drm/drm_blend.h @@ -52,4 +52,7 @@ int drm_plane_create_zpos_immutable_property(struct drm_plane *plane, unsigned int zpos); int drm_atomic_normalize_zpos(struct drm_device *dev, struct drm_atomic_state *state);
+int drm_plane_create_colorkey_properties(struct drm_plane *plane,
u32 supported_modes);#endif diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 26fa50c2a50e..9a621e1ccc47 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -32,6 +32,48 @@ struct drm_crtc; struct drm_printer; struct drm_modeset_acquire_ctx;
+/**
- enum drm_plane_colorkey_mode - uapi plane colorkey mode enumeration
- */
+enum drm_plane_colorkey_mode {
- /**
* @DRM_PLANE_COLORKEY_MODE_DISABLED:** No color matching performed in this mode.*/- DRM_PLANE_COLORKEY_MODE_DISABLED,
- /**
* @DRM_PLANE_COLORKEY_MODE_FOREGROUND_CLIP:** This mode is also known as a "green screen". Plane pixels are* transparent in areas where pixels match a given color key range* and there is a bottom (background) plane, in other cases plane* pixels are unaffected.**/- DRM_PLANE_COLORKEY_MODE_FOREGROUND_CLIP,
Could we add background clip as well?
Also could we just name them "src" and "dst" (or some variation of those). I'm betting no one has any kind of idea what these proposed names mean without looking up the docs, whereas pretty much everyone knows immediately what src/dst colorkeying means.
Would be nice if we could map i915's legacy ioctl handler to the new color key mode.
- /**
* @DRM_PLANE_COLORKEY_MODES_NUM:** Total number of color keying modes.*/- DRM_PLANE_COLORKEY_MODES_NUM,
+};
+/**
- struct drm_plane_colorkey_state - plane color keying state
- @colorkey.mode: color keying mode
- @colorkey.min: color key range minimum (in ARGB16161616 format)
- @colorkey.max: color key range maximum (in ARGB16161616 format)
- */
+struct drm_plane_colorkey_state {
- enum drm_plane_colorkey_mode mode;
- u64 min;
- u64 max;
+};
Could we have some macros to extract the components for min/max? A, R, G, B.
And where did we lose the value+mask?
On 06.07.2018 15:23, Ville Syrjälä wrote:
On Fri, Jul 06, 2018 at 02:11:44PM +0200, Maarten Lankhorst wrote:
Hey,
Op 04-06-18 om 00:00 schreef Dmitry Osipenko:
From: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
Color keying is the action of replacing pixels matching a given color (or range of colors) with transparent pixels in an overlay when performing blitting. Depending on the hardware capabilities, the matching pixel can either become fully transparent or gain adjustment of the pixels component values.
Color keying is found in a large number of devices whose capabilities often differ, but they still have enough common features in range to standardize color key properties. This commit adds three generic DRM plane properties related to the color keying, providing initial color keying support.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Signed-off-by: Dmitry Osipenko digetx@gmail.com
drivers/gpu/drm/drm_atomic.c | 12 +++++ drivers/gpu/drm/drm_blend.c | 99 ++++++++++++++++++++++++++++++++++++ include/drm/drm_blend.h | 3 ++ include/drm/drm_plane.h | 53 +++++++++++++++++++ 4 files changed, 167 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 895741e9cd7d..b322cbed319b 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -799,6 +799,12 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, state->rotation = val; } else if (property == plane->zpos_property) { state->zpos = val;
- } else if (property == plane->colorkey.mode_property) {
state->colorkey.mode = val;- } else if (property == plane->colorkey.min_property) {
state->colorkey.min = val;- } else if (property == plane->colorkey.max_property) {
state->colorkey.max = val;@@ -864,6 +870,12 @@ drm_atomic_plane_get_property(struct drm_plane *plane, *val = state->rotation; } else if (property == plane->zpos_property) { *val = state->zpos;
- } else if (property == plane->colorkey.mode_property) {
*val = state->colorkey.mode;- } else if (property == plane->colorkey.min_property) {
*val = state->colorkey.min;- } else if (property == plane->colorkey.max_property) {
*val = state->colorkey.max;diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c index a16a74d7e15e..12fed2ff65c8 100644 --- a/drivers/gpu/drm/drm_blend.c +++ b/drivers/gpu/drm/drm_blend.c @@ -107,6 +107,11 @@
- planes. Without this property the primary plane is always below the cursor
- plane, and ordering between all other planes is undefined.
- colorkey:
- Color keying is set up with drm_plane_create_colorkey_properties().
- It adds support for replacing a range of colors with a transparent
- color in the plane.
- Note that all the property extensions described here apply either to the
- plane or the CRTC (e.g. for the background color, which currently is not
- exposed and assumed to be black).
@@ -448,3 +453,97 @@ int drm_atomic_normalize_zpos(struct drm_device *dev, return 0; } EXPORT_SYMBOL(drm_atomic_normalize_zpos);
+static const char * const plane_colorkey_mode_name[] = {
- [DRM_PLANE_COLORKEY_MODE_DISABLED] = "disabled",
- [DRM_PLANE_COLORKEY_MODE_FOREGROUND_CLIP] = "foreground-clip",
+};
+/**
- drm_plane_create_colorkey_properties - create colorkey properties
- @plane: drm plane
- @supported_modes: bitmask of supported color keying modes
- This function creates the generic color keying properties and attach them to
- the plane to enable color keying control for blending operations.
- Color keying is controlled by these properties:
- colorkey.mode:
- The mode is an enumerated property that controls how color keying
- operates.
- colorkey.min, colorkey.max:
- These two properties specify the colors that are treated as the color
- key. Pixel whose value is in the [min, max] range is the color key
- matching pixel. The minimum and maximum values are expressed as a
- 64-bit integer in ARGB16161616 format, where A is the alpha value and
- R, G and B correspond to the color components. Drivers shall convert
- ARGB16161616 value into appropriate format within planes atomic check.
- When a single color key is desired instead of a range, userspace shall
- set the min and max properties to the same value.
- Drivers return an error from their plane atomic check if range can't be
- handled.
- Returns:
- Zero on success, negative errno on failure.
- */
+int drm_plane_create_colorkey_properties(struct drm_plane *plane,
u32 supported_modes)+{
- struct drm_prop_enum_list modes_list[DRM_PLANE_COLORKEY_MODES_NUM];
- struct drm_property *mode_prop;
- struct drm_property *min_prop;
- struct drm_property *max_prop;
- unsigned int modes_num = 0;
- unsigned int i;
- /* modes are driver-specific, build the list of supported modes */
- for (i = 0; i < DRM_PLANE_COLORKEY_MODES_NUM; i++) {
if (!(supported_modes & BIT(i)))continue;modes_list[modes_num].name = plane_colorkey_mode_name[i];modes_list[modes_num].type = i;modes_num++;- }
- /* at least one mode should be supported */
- if (!modes_num)
return -EINVAL;- mode_prop = drm_property_create_enum(plane->dev, 0, "colorkey.mode",
modes_list, modes_num);- if (!mode_prop)
return -ENOMEM;- min_prop = drm_property_create_range(plane->dev, 0, "colorkey.min",
0, U64_MAX);- if (!min_prop)
goto err_destroy_mode_prop;- max_prop = drm_property_create_range(plane->dev, 0, "colorkey.max",
0, U64_MAX);- if (!max_prop)
goto err_destroy_min_prop;- drm_object_attach_property(&plane->base, mode_prop, 0);
- drm_object_attach_property(&plane->base, min_prop, 0);
- drm_object_attach_property(&plane->base, max_prop, 0);
- plane->colorkey.mode_property = mode_prop;
- plane->colorkey.min_property = min_prop;
- plane->colorkey.max_property = max_prop;
- return 0;
+err_destroy_min_prop:
- drm_property_destroy(plane->dev, min_prop);
+err_destroy_mode_prop:
- drm_property_destroy(plane->dev, mode_prop);
- return -ENOMEM;
+} +EXPORT_SYMBOL(drm_plane_create_colorkey_properties); diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h index 330c561c4c11..8e80d33b643e 100644 --- a/include/drm/drm_blend.h +++ b/include/drm/drm_blend.h @@ -52,4 +52,7 @@ int drm_plane_create_zpos_immutable_property(struct drm_plane *plane, unsigned int zpos); int drm_atomic_normalize_zpos(struct drm_device *dev, struct drm_atomic_state *state);
+int drm_plane_create_colorkey_properties(struct drm_plane *plane,
u32 supported_modes);#endif diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 26fa50c2a50e..9a621e1ccc47 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -32,6 +32,48 @@ struct drm_crtc; struct drm_printer; struct drm_modeset_acquire_ctx;
+/**
- enum drm_plane_colorkey_mode - uapi plane colorkey mode enumeration
- */
+enum drm_plane_colorkey_mode {
- /**
* @DRM_PLANE_COLORKEY_MODE_DISABLED:** No color matching performed in this mode.*/- DRM_PLANE_COLORKEY_MODE_DISABLED,
- /**
* @DRM_PLANE_COLORKEY_MODE_FOREGROUND_CLIP:** This mode is also known as a "green screen". Plane pixels are* transparent in areas where pixels match a given color key range* and there is a bottom (background) plane, in other cases plane* pixels are unaffected.**/- DRM_PLANE_COLORKEY_MODE_FOREGROUND_CLIP,
Could we add background clip as well?
Sure, but I think adding a new mode should be a distinct change made on top of the initial series.
Also could we just name them "src" and "dst" (or some variation of those). I'm betting no one has any kind of idea what these proposed names mean without looking up the docs, whereas pretty much everyone knows immediately what src/dst colorkeying means.
Okay, I'll rename the mode to DRM_PLANE_COLORKEY_MODE_SRC in the next revision.
Would be nice if we could map i915's legacy ioctl handler to the new color key mode.
- /**
* @DRM_PLANE_COLORKEY_MODES_NUM:** Total number of color keying modes.*/- DRM_PLANE_COLORKEY_MODES_NUM,
+};
+/**
- struct drm_plane_colorkey_state - plane color keying state
- @colorkey.mode: color keying mode
- @colorkey.min: color key range minimum (in ARGB16161616 format)
- @colorkey.max: color key range maximum (in ARGB16161616 format)
- */
+struct drm_plane_colorkey_state {
- enum drm_plane_colorkey_mode mode;
- u64 min;
- u64 max;
+};
Could we have some macros to extract the components for min/max? A, R, G, B.
I'll add the macros in the next revision.
And where did we lose the value+mask?
There is no use for the mask on Tegra. I'd prefer to keep initial patches simple and minimal, other modes and additional properties could be added in the further patches on by as-needed basis. Mask could be added later with the default value of 0xffffffffffffffff. Does it sound good for you?
On Fri, Jul 06, 2018 at 04:05:21PM +0300, Dmitry Osipenko wrote:
On 06.07.2018 15:23, Ville Syrjälä wrote:
On Fri, Jul 06, 2018 at 02:11:44PM +0200, Maarten Lankhorst wrote:
Hey,
Op 04-06-18 om 00:00 schreef Dmitry Osipenko:
From: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
Color keying is the action of replacing pixels matching a given color (or range of colors) with transparent pixels in an overlay when performing blitting. Depending on the hardware capabilities, the matching pixel can either become fully transparent or gain adjustment of the pixels component values.
Color keying is found in a large number of devices whose capabilities often differ, but they still have enough common features in range to standardize color key properties. This commit adds three generic DRM plane properties related to the color keying, providing initial color keying support.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Signed-off-by: Dmitry Osipenko digetx@gmail.com
drivers/gpu/drm/drm_atomic.c | 12 +++++ drivers/gpu/drm/drm_blend.c | 99 ++++++++++++++++++++++++++++++++++++ include/drm/drm_blend.h | 3 ++ include/drm/drm_plane.h | 53 +++++++++++++++++++ 4 files changed, 167 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 895741e9cd7d..b322cbed319b 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -799,6 +799,12 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, state->rotation = val; } else if (property == plane->zpos_property) { state->zpos = val;
- } else if (property == plane->colorkey.mode_property) {
state->colorkey.mode = val;- } else if (property == plane->colorkey.min_property) {
state->colorkey.min = val;- } else if (property == plane->colorkey.max_property) {
state->colorkey.max = val;@@ -864,6 +870,12 @@ drm_atomic_plane_get_property(struct drm_plane *plane, *val = state->rotation; } else if (property == plane->zpos_property) { *val = state->zpos;
- } else if (property == plane->colorkey.mode_property) {
*val = state->colorkey.mode;- } else if (property == plane->colorkey.min_property) {
*val = state->colorkey.min;- } else if (property == plane->colorkey.max_property) {
*val = state->colorkey.max;diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c index a16a74d7e15e..12fed2ff65c8 100644 --- a/drivers/gpu/drm/drm_blend.c +++ b/drivers/gpu/drm/drm_blend.c @@ -107,6 +107,11 @@
- planes. Without this property the primary plane is always below the cursor
- plane, and ordering between all other planes is undefined.
- colorkey:
- Color keying is set up with drm_plane_create_colorkey_properties().
- It adds support for replacing a range of colors with a transparent
- color in the plane.
- Note that all the property extensions described here apply either to the
- plane or the CRTC (e.g. for the background color, which currently is not
- exposed and assumed to be black).
@@ -448,3 +453,97 @@ int drm_atomic_normalize_zpos(struct drm_device *dev, return 0; } EXPORT_SYMBOL(drm_atomic_normalize_zpos);
+static const char * const plane_colorkey_mode_name[] = {
- [DRM_PLANE_COLORKEY_MODE_DISABLED] = "disabled",
- [DRM_PLANE_COLORKEY_MODE_FOREGROUND_CLIP] = "foreground-clip",
+};
+/**
- drm_plane_create_colorkey_properties - create colorkey properties
- @plane: drm plane
- @supported_modes: bitmask of supported color keying modes
- This function creates the generic color keying properties and attach them to
- the plane to enable color keying control for blending operations.
- Color keying is controlled by these properties:
- colorkey.mode:
- The mode is an enumerated property that controls how color keying
- operates.
- colorkey.min, colorkey.max:
- These two properties specify the colors that are treated as the color
- key. Pixel whose value is in the [min, max] range is the color key
- matching pixel. The minimum and maximum values are expressed as a
- 64-bit integer in ARGB16161616 format, where A is the alpha value and
- R, G and B correspond to the color components. Drivers shall convert
- ARGB16161616 value into appropriate format within planes atomic check.
- When a single color key is desired instead of a range, userspace shall
- set the min and max properties to the same value.
- Drivers return an error from their plane atomic check if range can't be
- handled.
- Returns:
- Zero on success, negative errno on failure.
- */
+int drm_plane_create_colorkey_properties(struct drm_plane *plane,
u32 supported_modes)+{
- struct drm_prop_enum_list modes_list[DRM_PLANE_COLORKEY_MODES_NUM];
- struct drm_property *mode_prop;
- struct drm_property *min_prop;
- struct drm_property *max_prop;
- unsigned int modes_num = 0;
- unsigned int i;
- /* modes are driver-specific, build the list of supported modes */
- for (i = 0; i < DRM_PLANE_COLORKEY_MODES_NUM; i++) {
if (!(supported_modes & BIT(i)))continue;modes_list[modes_num].name = plane_colorkey_mode_name[i];modes_list[modes_num].type = i;modes_num++;- }
- /* at least one mode should be supported */
- if (!modes_num)
return -EINVAL;- mode_prop = drm_property_create_enum(plane->dev, 0, "colorkey.mode",
modes_list, modes_num);- if (!mode_prop)
return -ENOMEM;- min_prop = drm_property_create_range(plane->dev, 0, "colorkey.min",
0, U64_MAX);- if (!min_prop)
goto err_destroy_mode_prop;- max_prop = drm_property_create_range(plane->dev, 0, "colorkey.max",
0, U64_MAX);- if (!max_prop)
goto err_destroy_min_prop;- drm_object_attach_property(&plane->base, mode_prop, 0);
- drm_object_attach_property(&plane->base, min_prop, 0);
- drm_object_attach_property(&plane->base, max_prop, 0);
- plane->colorkey.mode_property = mode_prop;
- plane->colorkey.min_property = min_prop;
- plane->colorkey.max_property = max_prop;
- return 0;
+err_destroy_min_prop:
- drm_property_destroy(plane->dev, min_prop);
+err_destroy_mode_prop:
- drm_property_destroy(plane->dev, mode_prop);
- return -ENOMEM;
+} +EXPORT_SYMBOL(drm_plane_create_colorkey_properties); diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h index 330c561c4c11..8e80d33b643e 100644 --- a/include/drm/drm_blend.h +++ b/include/drm/drm_blend.h @@ -52,4 +52,7 @@ int drm_plane_create_zpos_immutable_property(struct drm_plane *plane, unsigned int zpos); int drm_atomic_normalize_zpos(struct drm_device *dev, struct drm_atomic_state *state);
+int drm_plane_create_colorkey_properties(struct drm_plane *plane,
u32 supported_modes);#endif diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 26fa50c2a50e..9a621e1ccc47 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -32,6 +32,48 @@ struct drm_crtc; struct drm_printer; struct drm_modeset_acquire_ctx;
+/**
- enum drm_plane_colorkey_mode - uapi plane colorkey mode enumeration
- */
+enum drm_plane_colorkey_mode {
- /**
* @DRM_PLANE_COLORKEY_MODE_DISABLED:** No color matching performed in this mode.*/- DRM_PLANE_COLORKEY_MODE_DISABLED,
- /**
* @DRM_PLANE_COLORKEY_MODE_FOREGROUND_CLIP:** This mode is also known as a "green screen". Plane pixels are* transparent in areas where pixels match a given color key range* and there is a bottom (background) plane, in other cases plane* pixels are unaffected.**/- DRM_PLANE_COLORKEY_MODE_FOREGROUND_CLIP,
Could we add background clip as well?
Sure, but I think adding a new mode should be a distinct change made on top of the initial series.
Also could we just name them "src" and "dst" (or some variation of those). I'm betting no one has any kind of idea what these proposed names mean without looking up the docs, whereas pretty much everyone knows immediately what src/dst colorkeying means.
Okay, I'll rename the mode to DRM_PLANE_COLORKEY_MODE_SRC in the next revision.
Would be nice if we could map i915's legacy ioctl handler to the new color key mode.
- /**
* @DRM_PLANE_COLORKEY_MODES_NUM:** Total number of color keying modes.*/- DRM_PLANE_COLORKEY_MODES_NUM,
+};
+/**
- struct drm_plane_colorkey_state - plane color keying state
- @colorkey.mode: color keying mode
- @colorkey.min: color key range minimum (in ARGB16161616 format)
- @colorkey.max: color key range maximum (in ARGB16161616 format)
- */
+struct drm_plane_colorkey_state {
- enum drm_plane_colorkey_mode mode;
- u64 min;
- u64 max;
+};
Could we have some macros to extract the components for min/max? A, R, G, B.
I'll add the macros in the next revision.
And where did we lose the value+mask?
There is no use for the mask on Tegra. I'd prefer to keep initial patches simple and minimal, other modes and additional properties could be added in the further patches on by as-needed basis. Mask could be added later with the default value of 0xffffffffffffffff. Does it sound good for you?
IIRC my earlier idea was to have different colorkey modes for the min+max and value+mask modes. That way userspace might actually have some chance of figuring out which bits of state actually do something. Although for Intel hw I think the general rule is that min+max for YUV, value+mask for RGB, so it's still not 100% clear what to pick if the plane supports both.
I guess one alternative would be to have min+max only, and the driver would reject 'min != max' if it only uses a single value?
And maybe we should have the mask always? IIRC Intel hw generally has a one bit enable/disable "mask" per channel in the min+max mode (I think there's one exception where it has only a 1 bit mask in the value+mask mode as well). So we could accept 0 and 0xffff mask values in this case and reject everything else. The alternative might be to enable the keying for the channel if 'min <= max' and disable it if 'min > max'. But not sure if that's slightly too magicy.
Op 06-07-18 om 16:10 schreef Ville Syrjälä:
On Fri, Jul 06, 2018 at 04:05:21PM +0300, Dmitry Osipenko wrote:
On 06.07.2018 15:23, Ville Syrjälä wrote:
On Fri, Jul 06, 2018 at 02:11:44PM +0200, Maarten Lankhorst wrote:
Hey,
Op 04-06-18 om 00:00 schreef Dmitry Osipenko:
From: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
Color keying is the action of replacing pixels matching a given color (or range of colors) with transparent pixels in an overlay when performing blitting. Depending on the hardware capabilities, the matching pixel can either become fully transparent or gain adjustment of the pixels component values.
Color keying is found in a large number of devices whose capabilities often differ, but they still have enough common features in range to standardize color key properties. This commit adds three generic DRM plane properties related to the color keying, providing initial color keying support.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Signed-off-by: Dmitry Osipenko digetx@gmail.com
drivers/gpu/drm/drm_atomic.c | 12 +++++ drivers/gpu/drm/drm_blend.c | 99 ++++++++++++++++++++++++++++++++++++ include/drm/drm_blend.h | 3 ++ include/drm/drm_plane.h | 53 +++++++++++++++++++ 4 files changed, 167 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 895741e9cd7d..b322cbed319b 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -799,6 +799,12 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane, state->rotation = val; } else if (property == plane->zpos_property) { state->zpos = val;
- } else if (property == plane->colorkey.mode_property) {
state->colorkey.mode = val;- } else if (property == plane->colorkey.min_property) {
state->colorkey.min = val;- } else if (property == plane->colorkey.max_property) {
state->colorkey.max = val;@@ -864,6 +870,12 @@ drm_atomic_plane_get_property(struct drm_plane *plane, *val = state->rotation; } else if (property == plane->zpos_property) { *val = state->zpos;
- } else if (property == plane->colorkey.mode_property) {
*val = state->colorkey.mode;- } else if (property == plane->colorkey.min_property) {
*val = state->colorkey.min;- } else if (property == plane->colorkey.max_property) {
*val = state->colorkey.max;diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c index a16a74d7e15e..12fed2ff65c8 100644 --- a/drivers/gpu/drm/drm_blend.c +++ b/drivers/gpu/drm/drm_blend.c @@ -107,6 +107,11 @@
- planes. Without this property the primary plane is always below the cursor
- plane, and ordering between all other planes is undefined.
- colorkey:
- Color keying is set up with drm_plane_create_colorkey_properties().
- It adds support for replacing a range of colors with a transparent
- color in the plane.
- Note that all the property extensions described here apply either to the
- plane or the CRTC (e.g. for the background color, which currently is not
- exposed and assumed to be black).
@@ -448,3 +453,97 @@ int drm_atomic_normalize_zpos(struct drm_device *dev, return 0; } EXPORT_SYMBOL(drm_atomic_normalize_zpos);
+static const char * const plane_colorkey_mode_name[] = {
- [DRM_PLANE_COLORKEY_MODE_DISABLED] = "disabled",
- [DRM_PLANE_COLORKEY_MODE_FOREGROUND_CLIP] = "foreground-clip",
+};
+/**
- drm_plane_create_colorkey_properties - create colorkey properties
- @plane: drm plane
- @supported_modes: bitmask of supported color keying modes
- This function creates the generic color keying properties and attach them to
- the plane to enable color keying control for blending operations.
- Color keying is controlled by these properties:
- colorkey.mode:
- The mode is an enumerated property that controls how color keying
- operates.
- colorkey.min, colorkey.max:
- These two properties specify the colors that are treated as the color
- key. Pixel whose value is in the [min, max] range is the color key
- matching pixel. The minimum and maximum values are expressed as a
- 64-bit integer in ARGB16161616 format, where A is the alpha value and
- R, G and B correspond to the color components. Drivers shall convert
- ARGB16161616 value into appropriate format within planes atomic check.
- When a single color key is desired instead of a range, userspace shall
- set the min and max properties to the same value.
- Drivers return an error from their plane atomic check if range can't be
- handled.
- Returns:
- Zero on success, negative errno on failure.
- */
+int drm_plane_create_colorkey_properties(struct drm_plane *plane,
u32 supported_modes)+{
- struct drm_prop_enum_list modes_list[DRM_PLANE_COLORKEY_MODES_NUM];
- struct drm_property *mode_prop;
- struct drm_property *min_prop;
- struct drm_property *max_prop;
- unsigned int modes_num = 0;
- unsigned int i;
- /* modes are driver-specific, build the list of supported modes */
- for (i = 0; i < DRM_PLANE_COLORKEY_MODES_NUM; i++) {
if (!(supported_modes & BIT(i)))continue;modes_list[modes_num].name = plane_colorkey_mode_name[i];modes_list[modes_num].type = i;modes_num++;- }
- /* at least one mode should be supported */
- if (!modes_num)
return -EINVAL;- mode_prop = drm_property_create_enum(plane->dev, 0, "colorkey.mode",
modes_list, modes_num);- if (!mode_prop)
return -ENOMEM;- min_prop = drm_property_create_range(plane->dev, 0, "colorkey.min",
0, U64_MAX);- if (!min_prop)
goto err_destroy_mode_prop;- max_prop = drm_property_create_range(plane->dev, 0, "colorkey.max",
0, U64_MAX);- if (!max_prop)
goto err_destroy_min_prop;- drm_object_attach_property(&plane->base, mode_prop, 0);
- drm_object_attach_property(&plane->base, min_prop, 0);
- drm_object_attach_property(&plane->base, max_prop, 0);
- plane->colorkey.mode_property = mode_prop;
- plane->colorkey.min_property = min_prop;
- plane->colorkey.max_property = max_prop;
- return 0;
+err_destroy_min_prop:
- drm_property_destroy(plane->dev, min_prop);
+err_destroy_mode_prop:
- drm_property_destroy(plane->dev, mode_prop);
- return -ENOMEM;
+} +EXPORT_SYMBOL(drm_plane_create_colorkey_properties); diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h index 330c561c4c11..8e80d33b643e 100644 --- a/include/drm/drm_blend.h +++ b/include/drm/drm_blend.h @@ -52,4 +52,7 @@ int drm_plane_create_zpos_immutable_property(struct drm_plane *plane, unsigned int zpos); int drm_atomic_normalize_zpos(struct drm_device *dev, struct drm_atomic_state *state);
+int drm_plane_create_colorkey_properties(struct drm_plane *plane,
u32 supported_modes);#endif diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 26fa50c2a50e..9a621e1ccc47 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -32,6 +32,48 @@ struct drm_crtc; struct drm_printer; struct drm_modeset_acquire_ctx;
+/**
- enum drm_plane_colorkey_mode - uapi plane colorkey mode enumeration
- */
+enum drm_plane_colorkey_mode {
- /**
* @DRM_PLANE_COLORKEY_MODE_DISABLED:** No color matching performed in this mode.*/- DRM_PLANE_COLORKEY_MODE_DISABLED,
- /**
* @DRM_PLANE_COLORKEY_MODE_FOREGROUND_CLIP:** This mode is also known as a "green screen". Plane pixels are* transparent in areas where pixels match a given color key range* and there is a bottom (background) plane, in other cases plane* pixels are unaffected.**/- DRM_PLANE_COLORKEY_MODE_FOREGROUND_CLIP,
Could we add background clip as well?
Sure, but I think adding a new mode should be a distinct change made on top of the initial series.
Also could we just name them "src" and "dst" (or some variation of those). I'm betting no one has any kind of idea what these proposed names mean without looking up the docs, whereas pretty much everyone knows immediately what src/dst colorkeying means.
Okay, I'll rename the mode to DRM_PLANE_COLORKEY_MODE_SRC in the next revision.
Would be nice if we could map i915's legacy ioctl handler to the new color key mode.
- /**
* @DRM_PLANE_COLORKEY_MODES_NUM:** Total number of color keying modes.*/- DRM_PLANE_COLORKEY_MODES_NUM,
+};
+/**
- struct drm_plane_colorkey_state - plane color keying state
- @colorkey.mode: color keying mode
- @colorkey.min: color key range minimum (in ARGB16161616 format)
- @colorkey.max: color key range maximum (in ARGB16161616 format)
- */
+struct drm_plane_colorkey_state {
- enum drm_plane_colorkey_mode mode;
- u64 min;
- u64 max;
+};
Could we have some macros to extract the components for min/max? A, R, G, B.
I'll add the macros in the next revision.
And where did we lose the value+mask?
There is no use for the mask on Tegra. I'd prefer to keep initial patches simple and minimal, other modes and additional properties could be added in the further patches on by as-needed basis. Mask could be added later with the default value of 0xffffffffffffffff. Does it sound good for you?
IIRC my earlier idea was to have different colorkey modes for the min+max and value+mask modes. That way userspace might actually have some chance of figuring out which bits of state actually do something. Although for Intel hw I think the general rule is that min+max for YUV, value+mask for RGB, so it's still not 100% clear what to pick if the plane supports both.
I guess one alternative would be to have min+max only, and the driver would reject 'min != max' if it only uses a single value?
Sounds sane.
And maybe we should have the mask always? IIRC Intel hw generally has a one bit enable/disable "mask" per channel in the min+max mode (I think there's one exception where it has only a 1 bit mask in the value+mask mode as well). So we could accept 0 and 0xffff mask values in this case and reject everything else. The alternative might be to enable the keying for the channel if 'min <= max' and disable it if 'min > max'. But not sure if that's slightly too magicy.
Yeah, either a single value or 0/0xffff (don't care about this component) sounds like it could work.
On Friday, 6 July 2018 17:10:10 MSK Ville Syrjälä wrote:
On Fri, Jul 06, 2018 at 04:05:21PM +0300, Dmitry Osipenko wrote:
On 06.07.2018 15:23, Ville Syrjälä wrote:
On Fri, Jul 06, 2018 at 02:11:44PM +0200, Maarten Lankhorst wrote:
Hey,
Op 04-06-18 om 00:00 schreef Dmitry Osipenko:
From: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com
Color keying is the action of replacing pixels matching a given color (or range of colors) with transparent pixels in an overlay when performing blitting. Depending on the hardware capabilities, the matching pixel can either become fully transparent or gain adjustment of the pixels component values.
Color keying is found in a large number of devices whose capabilities often differ, but they still have enough common features in range to standardize color key properties. This commit adds three generic DRM plane properties related to the color keying, providing initial color keying support.
Signed-off-by: Laurent Pinchart laurent.pinchart+renesas@ideasonboard.com Signed-off-by: Dmitry Osipenko digetx@gmail.com
drivers/gpu/drm/drm_atomic.c | 12 +++++ drivers/gpu/drm/drm_blend.c | 99 ++++++++++++++++++++++++++++++++++++ include/drm/drm_blend.h | 3 ++ include/drm/drm_plane.h | 53 +++++++++++++++++++ 4 files changed, 167 insertions(+)
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 895741e9cd7d..b322cbed319b 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -799,6 +799,12 @@ static int drm_atomic_plane_set_property(struct drm_plane *plane,> >>> state->rotation = val;
} else if (property == plane->zpos_property) {
state->zpos = val;
} else if (property == plane->colorkey.mode_property) {
state->colorkey.mode = val;} else if (property == plane->colorkey.min_property) {
state->colorkey.min = val;} else if (property == plane->colorkey.max_property) {
state->colorkey.max = val;} else if (property == plane->color_encoding_property) {
state->color_encoding = val;
} else if (property == plane->color_range_property) {
@@ -864,6 +870,12 @@ drm_atomic_plane_get_property(struct drm_plane *plane,
*val = state->rotation;} else if (property == plane->zpos_property) {
*val = state->zpos;
} else if (property == plane->colorkey.mode_property) {
*val = state->colorkey.mode;} else if (property == plane->colorkey.min_property) {
*val = state->colorkey.min;} else if (property == plane->colorkey.max_property) {
*val = state->colorkey.max;} else if (property == plane->color_encoding_property) {
*val = state->color_encoding;
} else if (property == plane->color_range_property) {
diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c index a16a74d7e15e..12fed2ff65c8 100644 --- a/drivers/gpu/drm/drm_blend.c +++ b/drivers/gpu/drm/drm_blend.c @@ -107,6 +107,11 @@
- planes. Without this property the primary plane is always below
the cursor * plane, and ordering between all other planes is undefined.
- colorkey:
- Color keying is set up with
drm_plane_create_colorkey_properties().
- It adds support for replacing a range of colors with a
transparent
- color in the plane.
Note that all the property extensions described here apply either
to the
- plane or the CRTC (e.g. for the background color, which currently
is not
- exposed and assumed to be black).
@@ -448,3 +453,97 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
return 0;
} EXPORT_SYMBOL(drm_atomic_normalize_zpos);
+static const char * const plane_colorkey_mode_name[] = {
- [DRM_PLANE_COLORKEY_MODE_DISABLED] = "disabled",
- [DRM_PLANE_COLORKEY_MODE_FOREGROUND_CLIP] = "foreground-clip",
+};
+/**
- drm_plane_create_colorkey_properties - create colorkey properties
- @plane: drm plane
- @supported_modes: bitmask of supported color keying modes
- This function creates the generic color keying properties and
attach them to + * the plane to enable color keying control for blending operations. + *
- Color keying is controlled by these properties:
- colorkey.mode:
- The mode is an enumerated property that controls how color keying
- operates.
- colorkey.min, colorkey.max:
- These two properties specify the colors that are treated as the
color
- key. Pixel whose value is in the [min, max] range is the color
key
- matching pixel. The minimum and maximum values are expressed as a
- 64-bit integer in ARGB16161616 format, where A is the alpha value
and
- R, G and B correspond to the color components. Drivers shall
convert
- ARGB16161616 value into appropriate format within planes atomic
check.
- When a single color key is desired instead of a range, userspace
shall
- set the min and max properties to the same value.
- Drivers return an error from their plane atomic check if range
can't be
- handled.
- Returns:
- Zero on success, negative errno on failure.
- */
+int drm_plane_create_colorkey_properties(struct drm_plane *plane,
u32 supported_modes)+{
- struct drm_prop_enum_list
modes_list[DRM_PLANE_COLORKEY_MODES_NUM];
- struct drm_property *mode_prop;
- struct drm_property *min_prop;
- struct drm_property *max_prop;
- unsigned int modes_num = 0;
- unsigned int i;
- /* modes are driver-specific, build the list of supported modes
*/
- for (i = 0; i < DRM_PLANE_COLORKEY_MODES_NUM; i++) {
if (!(supported_modes & BIT(i)))continue;modes_list[modes_num].name = plane_colorkey_mode_name[i];modes_list[modes_num].type = i;modes_num++;- }
- /* at least one mode should be supported */
- if (!modes_num)
return -EINVAL;- mode_prop = drm_property_create_enum(plane->dev, 0,
"colorkey.mode",
modes_list, modes_num);- if (!mode_prop)
return -ENOMEM;- min_prop = drm_property_create_range(plane->dev, 0,
"colorkey.min",
0, U64_MAX);- if (!min_prop)
goto err_destroy_mode_prop;- max_prop = drm_property_create_range(plane->dev, 0,
"colorkey.max",
0, U64_MAX);- if (!max_prop)
goto err_destroy_min_prop;- drm_object_attach_property(&plane->base, mode_prop, 0);
- drm_object_attach_property(&plane->base, min_prop, 0);
- drm_object_attach_property(&plane->base, max_prop, 0);
- plane->colorkey.mode_property = mode_prop;
- plane->colorkey.min_property = min_prop;
- plane->colorkey.max_property = max_prop;
- return 0;
+err_destroy_min_prop:
- drm_property_destroy(plane->dev, min_prop);
+err_destroy_mode_prop:
- drm_property_destroy(plane->dev, mode_prop);
- return -ENOMEM;
+} +EXPORT_SYMBOL(drm_plane_create_colorkey_properties); diff --git a/include/drm/drm_blend.h b/include/drm/drm_blend.h index 330c561c4c11..8e80d33b643e 100644 --- a/include/drm/drm_blend.h +++ b/include/drm/drm_blend.h @@ -52,4 +52,7 @@ int drm_plane_create_zpos_immutable_property(struct drm_plane *plane,> >>> unsigned int zpos);
int drm_atomic_normalize_zpos(struct drm_device *dev,
struct drm_atomic_state *state);
+int drm_plane_create_colorkey_properties(struct drm_plane *plane,
u32 supported_modes);#endif
diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h index 26fa50c2a50e..9a621e1ccc47 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -32,6 +32,48 @@ struct drm_crtc;
struct drm_printer; struct drm_modeset_acquire_ctx;
+/**
- enum drm_plane_colorkey_mode - uapi plane colorkey mode
enumeration
- */
+enum drm_plane_colorkey_mode {
- /**
* @DRM_PLANE_COLORKEY_MODE_DISABLED:** No color matching performed in this mode.*/- DRM_PLANE_COLORKEY_MODE_DISABLED,
- /**
* @DRM_PLANE_COLORKEY_MODE_FOREGROUND_CLIP:** This mode is also known as a "green screen". Plane pixels are* transparent in areas where pixels match a given color key
range
* and there is a bottom (background) plane, in other cases plane* pixels are unaffected.**/- DRM_PLANE_COLORKEY_MODE_FOREGROUND_CLIP,
Could we add background clip as well?
Sure, but I think adding a new mode should be a distinct change made on top of the initial series.
Also could we just name them "src" and "dst" (or some variation of those). I'm betting no one has any kind of idea what these proposed names mean without looking up the docs, whereas pretty much everyone knows immediately what src/dst colorkeying means.
Okay, I'll rename the mode to DRM_PLANE_COLORKEY_MODE_SRC in the next revision.>
Would be nice if we could map i915's legacy ioctl handler to the new color key mode.> >>
- /**
* @DRM_PLANE_COLORKEY_MODES_NUM:** Total number of color keying modes.*/- DRM_PLANE_COLORKEY_MODES_NUM,
+};
+/**
- struct drm_plane_colorkey_state - plane color keying state
- @colorkey.mode: color keying mode
- @colorkey.min: color key range minimum (in ARGB16161616 format)
- @colorkey.max: color key range maximum (in ARGB16161616 format)
- */
+struct drm_plane_colorkey_state {
- enum drm_plane_colorkey_mode mode;
- u64 min;
- u64 max;
+};
Could we have some macros to extract the components for min/max? A, R, G, B.
I'll add the macros in the next revision.
And where did we lose the value+mask?
There is no use for the mask on Tegra. I'd prefer to keep initial patches simple and minimal, other modes and additional properties could be added in the further patches on by as-needed basis. Mask could be added later with the default value of 0xffffffffffffffff. Does it sound good for you?
IIRC my earlier idea was to have different colorkey modes for the min+max and value+mask modes. That way userspace might actually have some chance of figuring out which bits of state actually do something. Although for Intel hw I think the general rule is that min+max for YUV, value+mask for RGB, so it's still not 100% clear what to pick if the plane supports both.
I guess one alternative would be to have min+max only, and the driver would reject 'min != max' if it only uses a single value?
You should pick both and reject unsupported property values based on the planes framebuffer format. So it will be possible to set unsupported values while plane is disabled because it doesn't have an associated framebuffer and then atomic check will fail to enable plane if property values are invalid for the given format.
And maybe we should have the mask always? IIRC Intel hw generally has a one bit enable/disable "mask" per channel in the min+max mode (I think there's one exception where it has only a 1 bit mask in the value+mask mode as well). So we could accept 0 and 0xffff mask values in this case and reject everything else.
Sounds good to me. Actually there is a place for the mask on Tegra, older generation doesn't support matching of the alpha channel and the alpha channel matching is simply ignored, so colorkeying could be support for the framebuffer formats that have an alpha channel if the alpha channel matching is disabled by the mask value.
The alternative might be to enable the keying for the channel if 'min <= max' and disable it if 'min > max'. But not sure if that's slightly too magicy.
The mask property should suit well your case, I don't think there is any need for the magic.
On Fri, Jul 06, 2018 at 05:58:50PM +0300, Dmitry Osipenko wrote:
On Friday, 6 July 2018 17:10:10 MSK Ville Syrjälä wrote:
IIRC my earlier idea was to have different colorkey modes for the min+max and value+mask modes. That way userspace might actually have some chance of figuring out which bits of state actually do something. Although for Intel hw I think the general rule is that min+max for YUV, value+mask for RGB, so it's still not 100% clear what to pick if the plane supports both.
I guess one alternative would be to have min+max only, and the driver would reject 'min != max' if it only uses a single value?
You should pick both and reject unsupported property values based on the planes framebuffer format. So it will be possible to set unsupported values while plane is disabled because it doesn't have an associated framebuffer and then atomic check will fail to enable plane if property values are invalid for the given format.
The colorkey which is attached to a plane 'A' is not applied to plane 'A', so the format of plane 'A' is not relevant. The colorkey is applied to some other plane which will be below this plane in terms of the plane blending operation.
What if you have several planes below plane 'A' with differing framebuffer formats - maybe an ARGB8888 plane and a ARGB1555 plane - do you decide to limit the colorkey to 8bits per channel, or to ARGB1555 format?
The answer is, of course, hardware dependent - generic code can't know the details of the colorkey implementation, which could be one of:
lower plane data -> expand to 8bpc -> match ARGB8888 colorkey lower plane data -> match ARGB8888 reduced to plane compatible colorkey
which will give different results depending on the format of the lower plane data.
On Fri, Jul 06, 2018 at 04:40:27PM +0100, Russell King - ARM Linux wrote:
On Fri, Jul 06, 2018 at 05:58:50PM +0300, Dmitry Osipenko wrote:
On Friday, 6 July 2018 17:10:10 MSK Ville Syrjälä wrote:
IIRC my earlier idea was to have different colorkey modes for the min+max and value+mask modes. That way userspace might actually have some chance of figuring out which bits of state actually do something. Although for Intel hw I think the general rule is that min+max for YUV, value+mask for RGB, so it's still not 100% clear what to pick if the plane supports both.
I guess one alternative would be to have min+max only, and the driver would reject 'min != max' if it only uses a single value?
You should pick both and reject unsupported property values based on the planes framebuffer format. So it will be possible to set unsupported values while plane is disabled because it doesn't have an associated framebuffer and then atomic check will fail to enable plane if property values are invalid for the given format.
The colorkey which is attached to a plane 'A' is not applied to plane 'A', so the format of plane 'A' is not relevant. The colorkey is applied to some other plane which will be below this plane in terms of the plane blending operation.
What if you have several planes below plane 'A' with differing framebuffer formats - maybe an ARGB8888 plane and a ARGB1555 plane - do you decide to limit the colorkey to 8bits per channel, or to ARGB1555 format?
The answer is, of course, hardware dependent - generic code can't know the details of the colorkey implementation, which could be one of:
lower plane data -> expand to 8bpc -> match ARGB8888 colorkey lower plane data -> match ARGB8888 reduced to plane compatible colorkey
which will give different results depending on the format of the lower plane data.
Yeah. This is one of the other issues I highlighted previously. On some Intel hw you enable the dst colorkey on the lower plane where you paint the colorkey, on others you enable it on the plane above that gets shown/hidden based on the key match on the lower plane. I think on most of our hardware dst keying only ever happens between two planes, even if more planes are enabled on the crtc. But there is at least one exception where you can have two overlay planes that get keyed against the same primary plane.
I was questioning which one is the better model for the uapi. Either way you still have the uncertainty of which planes actually participate in the keying process. One way around that would be to expose some kind of plane mask which would indicate the set of planes taking part in the keying process. I think for that to work we would have to move to a model where you enable the dst key on the lower plane where you paint the color key, otherwise having multiple bits set in the mask wouldn't make sense. That would perhaps also make it more clear what the format of the color key will be.
So as an example that exception Intel hw case would have: overlay 2: key_mode={src,none} overlay 1: key_mode={src,none} primary: key_mode={dst,none}, dst_key_plane_mask=0x6
to indicate that both overlay planes participate in the keying process.
Whereas most other Intel hw would have: overlay 2: key_mode={src,none} overlay 1: key_mode={src,none} primary: key_mode={dst,none}, dst_key_plane_mask=0x2
to indicate that only the overlay immediately above the primary will participate.
On Friday, 6 July 2018 18:40:27 MSK Russell King - ARM Linux wrote:
On Fri, Jul 06, 2018 at 05:58:50PM +0300, Dmitry Osipenko wrote:
On Friday, 6 July 2018 17:10:10 MSK Ville Syrjälä wrote:
IIRC my earlier idea was to have different colorkey modes for the min+max and value+mask modes. That way userspace might actually have some chance of figuring out which bits of state actually do something. Although for Intel hw I think the general rule is that min+max for YUV, value+mask for RGB, so it's still not 100% clear what to pick if the plane supports both.
I guess one alternative would be to have min+max only, and the driver would reject 'min != max' if it only uses a single value?
You should pick both and reject unsupported property values based on the planes framebuffer format. So it will be possible to set unsupported values while plane is disabled because it doesn't have an associated framebuffer and then atomic check will fail to enable plane if property values are invalid for the given format.
The colorkey which is attached to a plane 'A' is not applied to plane 'A', so the format of plane 'A' is not relevant. The colorkey is applied to some other plane which will be below this plane in terms of the plane blending operation.
What if you have several planes below plane 'A' with differing framebuffer formats - maybe an ARGB8888 plane and a ARGB1555 plane - do you decide to limit the colorkey to 8bits per channel, or to ARGB1555 format?
The answer is, of course, hardware dependent - generic code can't know the details of the colorkey implementation, which could be one of:
lower plane data -> expand to 8bpc -> match ARGB8888 colorkey lower plane data -> match ARGB8888 reduced to plane compatible colorkey
which will give different results depending on the format of the lower plane data.
All unsupportable cases should be rejected in the atomic check. If your HW can't handle the case where multiple bottom planes have a different format, then in the planes atomic check you'll have to walk up all the bottom planes and verify their formats.
I'm not sure whether it's worth to check the planes intersection and fail only if top plane intersects with multiple bottom planes having different formats. Perhaps that could be a driver-specific implementation detail, userspace should take into account that atomic commit may fail on changing planes position.
On Fri, Jul 06, 2018 at 07:33:14PM +0300, Dmitry Osipenko wrote:
On Friday, 6 July 2018 18:40:27 MSK Russell King - ARM Linux wrote:
On Fri, Jul 06, 2018 at 05:58:50PM +0300, Dmitry Osipenko wrote:
On Friday, 6 July 2018 17:10:10 MSK Ville Syrjälä wrote:
IIRC my earlier idea was to have different colorkey modes for the min+max and value+mask modes. That way userspace might actually have some chance of figuring out which bits of state actually do something. Although for Intel hw I think the general rule is that min+max for YUV, value+mask for RGB, so it's still not 100% clear what to pick if the plane supports both.
I guess one alternative would be to have min+max only, and the driver would reject 'min != max' if it only uses a single value?
You should pick both and reject unsupported property values based on the planes framebuffer format. So it will be possible to set unsupported values while plane is disabled because it doesn't have an associated framebuffer and then atomic check will fail to enable plane if property values are invalid for the given format.
The colorkey which is attached to a plane 'A' is not applied to plane 'A', so the format of plane 'A' is not relevant. The colorkey is applied to some other plane which will be below this plane in terms of the plane blending operation.
What if you have several planes below plane 'A' with differing framebuffer formats - maybe an ARGB8888 plane and a ARGB1555 plane - do you decide to limit the colorkey to 8bits per channel, or to ARGB1555 format?
The answer is, of course, hardware dependent - generic code can't know the details of the colorkey implementation, which could be one of:
lower plane data -> expand to 8bpc -> match ARGB8888 colorkey lower plane data -> match ARGB8888 reduced to plane compatible colorkey
which will give different results depending on the format of the lower plane data.
All unsupportable cases should be rejected in the atomic check. If your HW can't handle the case where multiple bottom planes have a different format, then in the planes atomic check you'll have to walk up all the bottom planes and verify their formats.
That is *not* what I'm trying to point out.
You are claiming that we should check the validity of the colorkey format in relation to the lower planes, and it sounds like you're suggesting it in generic code. I'm trying to get you to think a bit more about what you're suggesting by considering a theoretical (or maybe not so theoretical) case.
We do have hardware out there which can have multiple planes that are merged together - I seem to remember that Tegra? hardware has that ability, but it isn't implemented in the driver yet.
So, I'm asking how you forsee the validity check working in the presence of different formats for multiple lower planes.
I'm not talking about whether the hardware supports it or not - I'm assuming that the hardware _does_ support multiple lower planes with differing formats.
From what I understand, to take the simple case of one lower plane,
you are proposing:
- if the lower plane is ARGB1555, then specifying a colorkey with an alpha of anything except 0 or 0xffff would be invalid and should be rejected.
- if a lower plane is ARGB8888, then specifying a colorkey which is anything except 0...0xffff in 0x101 (65535 / 255) steps would be invalid and should be rejected.
Now consider the case I mentioned above. What if there are two lower planes, one with ARGB1555 and the other with ARGB8888. Does this mean that (eg) the alpha colorkey component should be rejected if:
- the alpha in the colorkey is not 0 or 0xffff, or - it's anything except 0...0xffff in 0x101 steps?
My assertion is that this is only a decision that can be made by the driver and not by generic code, because it is hardware dependent.
I am _not_ disagreeing with the general principle of validating that the requested state is possible with the hardware.
On Friday, 6 July 2018 20:01:36 MSK Russell King - ARM Linux wrote:
On Fri, Jul 06, 2018 at 07:33:14PM +0300, Dmitry Osipenko wrote:
On Friday, 6 July 2018 18:40:27 MSK Russell King - ARM Linux wrote:
On Fri, Jul 06, 2018 at 05:58:50PM +0300, Dmitry Osipenko wrote:
On Friday, 6 July 2018 17:10:10 MSK Ville Syrjälä wrote:
IIRC my earlier idea was to have different colorkey modes for the min+max and value+mask modes. That way userspace might actually have some chance of figuring out which bits of state actually do something. Although for Intel hw I think the general rule is that min+max for YUV, value+mask for RGB, so it's still not 100% clear what to pick if the plane supports both.
I guess one alternative would be to have min+max only, and the driver would reject 'min != max' if it only uses a single value?
You should pick both and reject unsupported property values based on the planes framebuffer format. So it will be possible to set unsupported values while plane is disabled because it doesn't have an associated framebuffer and then atomic check will fail to enable plane if property values are invalid for the given format.
The colorkey which is attached to a plane 'A' is not applied to plane 'A', so the format of plane 'A' is not relevant. The colorkey is applied to some other plane which will be below this plane in terms of the plane blending operation.
What if you have several planes below plane 'A' with differing framebuffer formats - maybe an ARGB8888 plane and a ARGB1555 plane - do you decide to limit the colorkey to 8bits per channel, or to ARGB1555 format?
The answer is, of course, hardware dependent - generic code can't know the details of the colorkey implementation, which could be one
of: lower plane data -> expand to 8bpc -> match ARGB8888 colorkey lower plane data -> match ARGB8888 reduced to plane compatible colorkey
which will give different results depending on the format of the lower plane data.
All unsupportable cases should be rejected in the atomic check. If your HW can't handle the case where multiple bottom planes have a different format, then in the planes atomic check you'll have to walk up all the bottom planes and verify their formats.
That is *not* what I'm trying to point out.
You are claiming that we should check the validity of the colorkey format in relation to the lower planes, and it sounds like you're suggesting it in generic code. I'm trying to get you to think a bit more about what you're suggesting by considering a theoretical (or maybe not so theoretical) case.
We do have hardware out there which can have multiple planes that are merged together - I seem to remember that Tegra? hardware has that ability, but it isn't implemented in the driver yet.
I'm not sure what you're meaning by planes "merging", could you please elaborate?
So, I'm asking how you forsee the validity check working in the presence of different formats for multiple lower planes.
I'm not talking about whether the hardware supports it or not - I'm assuming that the hardware _does_ support multiple lower planes with differing formats.
From what I understand, to take the simple case of one lower plane, you are proposing:
if the lower plane is ARGB1555, then specifying a colorkey with an alpha of anything except 0 or 0xffff would be invalid and should be rejected.
if a lower plane is ARGB8888, then specifying a colorkey which is anything except 0...0xffff in 0x101 (65535 / 255) steps would be invalid and should be rejected.
Now consider the case I mentioned above. What if there are two lower planes, one with ARGB1555 and the other with ARGB8888. Does this mean that (eg) the alpha colorkey component should be rejected if:
- the alpha in the colorkey is not 0 or 0xffff, or
- it's anything except 0...0xffff in 0x101 steps?
My assertion is that this is only a decision that can be made by the driver and not by generic code, because it is hardware dependent.
Definitely the conversion rule must be defined explicitly, otherwise colorkey property values can't be considered generic. Thank you for pointing at it. I think rounding to a closest value should be the generic conversion rule.
I'll document the conversion rule in the next revision. Please let me know if you see any problems with the rounding to a closest value.
The final decision will be made by the driver, but driver and userspace will have to take into account the defined generic conversion rule.
I am _not_ disagreeing with the general principle of validating that the requested state is possible with the hardware.
Thank you for the clarification.
For the starter a minimal color keying support is implemented, which is enough to provide userspace like Opentegra Xorg driver with ability to support color keying by the XVideo extension.
Signed-off-by: Dmitry Osipenko digetx@gmail.com --- drivers/gpu/drm/tegra/dc.c | 25 +++++++++ drivers/gpu/drm/tegra/dc.h | 7 +++ drivers/gpu/drm/tegra/plane.c | 102 ++++++++++++++++++++++++++++++++++ 3 files changed, 134 insertions(+)
diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c index c3afe7b2237e..685a0fedb01d 100644 --- a/drivers/gpu/drm/tegra/dc.c +++ b/drivers/gpu/drm/tegra/dc.c @@ -162,6 +162,7 @@ static void tegra_plane_setup_blending_legacy(struct tegra_plane *plane) u32 foreground = BLEND_WEIGHT1(255) | BLEND_WEIGHT0(255) | BLEND_COLOR_KEY_NONE; u32 blendnokey = BLEND_WEIGHT1(255) | BLEND_WEIGHT0(255); + enum drm_plane_colorkey_mode mode; struct tegra_plane_state *state; u32 blending[2]; unsigned int i; @@ -171,7 +172,15 @@ static void tegra_plane_setup_blending_legacy(struct tegra_plane *plane) tegra_plane_writel(plane, foreground, DC_WIN_BLEND_1WIN);
state = to_tegra_plane_state(plane->base.state); + mode = plane->base.state->colorkey.mode;
+ /* setup color keying */ + if (mode == DRM_PLANE_COLORKEY_MODE_FOREGROUND_CLIP) { + /* color key matched areas are transparent */ + foreground = background[0] | BLEND_COLOR_KEY_0; + } + + /* setup alpha blending */ if (state->opaque) { /* * Since custom fix-weight blending isn't utilized and weight @@ -792,6 +801,11 @@ static struct drm_plane *tegra_primary_plane_create(struct drm_device *drm, dev_err(dc->dev, "failed to create rotation property: %d\n", err);
+ if (dc->soc->has_legacy_blending) + drm_plane_create_colorkey_properties(&plane->base, + BIT(DRM_PLANE_COLORKEY_MODE_DISABLED) | + BIT(DRM_PLANE_COLORKEY_MODE_FOREGROUND_CLIP)); + return &plane->base; }
@@ -1077,6 +1091,11 @@ static struct drm_plane *tegra_dc_overlay_plane_create(struct drm_device *drm, dev_err(dc->dev, "failed to create rotation property: %d\n", err);
+ if (dc->soc->has_legacy_blending) + drm_plane_create_colorkey_properties(&plane->base, + BIT(DRM_PLANE_COLORKEY_MODE_DISABLED) | + BIT(DRM_PLANE_COLORKEY_MODE_FOREGROUND_CLIP)); + return &plane->base; }
@@ -1187,6 +1206,7 @@ tegra_crtc_atomic_duplicate_state(struct drm_crtc *crtc) copy->pclk = state->pclk; copy->div = state->div; copy->planes = state->planes; + copy->ckey = state->ckey;
return ©->base; } @@ -1917,6 +1937,11 @@ static void tegra_crtc_atomic_flush(struct drm_crtc *crtc, struct tegra_dc *dc = to_tegra_dc(crtc); u32 value;
+ if (dc->soc->has_legacy_blending) { + tegra_dc_writel(dc, state->ckey.min, DC_DISP_COLOR_KEY0_LOWER); + tegra_dc_writel(dc, state->ckey.max, DC_DISP_COLOR_KEY0_UPPER); + } + value = state->planes << 8 | GENERAL_UPDATE; tegra_dc_writel(dc, value, DC_CMD_STATE_CONTROL); value = tegra_dc_readl(dc, DC_CMD_STATE_CONTROL); diff --git a/drivers/gpu/drm/tegra/dc.h b/drivers/gpu/drm/tegra/dc.h index e96f582ca692..14ed31f0ff37 100644 --- a/drivers/gpu/drm/tegra/dc.h +++ b/drivers/gpu/drm/tegra/dc.h @@ -18,6 +18,11 @@
struct tegra_output;
+struct tegra_dc_color_key_state { + u32 min; + u32 max; +}; + struct tegra_dc_state { struct drm_crtc_state base;
@@ -26,6 +31,8 @@ struct tegra_dc_state { unsigned int div;
u32 planes; + + struct tegra_dc_color_key_state ckey; };
static inline struct tegra_dc_state *to_dc_state(struct drm_crtc_state *state) diff --git a/drivers/gpu/drm/tegra/plane.c b/drivers/gpu/drm/tegra/plane.c index d068e8aa3553..b8f05fd6ffcc 100644 --- a/drivers/gpu/drm/tegra/plane.c +++ b/drivers/gpu/drm/tegra/plane.c @@ -465,6 +465,104 @@ static int tegra_plane_setup_transparency(struct tegra_plane *tegra, return 0; }
+static u32 tegra_plane_convert_colorkey_format(u64 value) +{ + /* convert ARGB16161616 to ARGB8888 */ + u16 a = (value >> 48) & 0xFFFF; + u16 r = (value >> 32) & 0xFFFF; + u16 g = (value >> 16) & 0xFFFF; + u16 b = (value >> 0) & 0xFFFF; + + a = min_t(u16, 0xff, a / 256); + r = min_t(u16, 0xff, r / 256); + g = min_t(u16, 0xff, g / 256); + b = min_t(u16, 0xff, b / 256); + + return (a << 24) | (r << 16) | (g << 8) | b; +} + +static bool tegra_plane_format_invalid_for_colorkey( + struct drm_plane_state *state) +{ + /* + * Older Tegra's do not support alpha channel matching. It should + * be possible to support certain cases where planes format has alpha + * channel, but for simplicity these cases currently are unsupported. + */ + if (state->fb && __drm_format_has_alpha(state->fb->format->format)) + return true; + + return false; +} + +static int tegra_plane_setup_colorkey(struct tegra_plane *tegra, + struct tegra_plane_state *tegra_state) +{ + enum drm_plane_colorkey_mode mode; + struct drm_plane_state *old_plane; + struct drm_crtc_state *crtc_state; + struct tegra_dc_state *dc_state; + struct drm_plane_state *state; + u64 min, max; + + mode = tegra_state->base.colorkey.mode; + min = tegra_state->base.colorkey.min; + max = tegra_state->base.colorkey.max; + + /* no need to proceed if color keying is disabled */ + if (mode == DRM_PLANE_COLORKEY_MODE_DISABLED) + return 0; + + state = &tegra_state->base; + old_plane = drm_atomic_get_old_plane_state(state->state, &tegra->base); + + /* + * Currently color keying implemented for the middle plane only + * to simplify things. + */ + if (state->normalized_zpos != 1) { + /* foreground-clip has no effect when applied to bottom plane */ + if (state->normalized_zpos == 0) + return 0; + + return -EINVAL; + } + + /* + * There is no need to proceed, adding CRTC and other planes to + * the atomic update, if color keying state is unchanged. + */ + if (old_plane && + old_plane->colorkey.mode == mode && + old_plane->colorkey.min == min && + old_plane->colorkey.max == max) + return 0; + + /* convert color key values to HW format */ + min = tegra_plane_convert_colorkey_format(min); + max = tegra_plane_convert_colorkey_format(max); + + /* validate planes format */ + if (tegra_plane_format_invalid_for_colorkey(state)) + return -EINVAL; + + /* + * Tegra's HW stores color key values within CRTC, hence adjust + * planes CRTC atomic state. + */ + crtc_state = drm_atomic_get_crtc_state(state->state, state->crtc); + if (IS_ERR(crtc_state)) + return PTR_ERR(crtc_state); + + dc_state = to_dc_state(crtc_state); + + /* update CRTC's color key state */ + dc_state->ckey.min = min; + dc_state->ckey.max = max; + + return 0; +} + int tegra_plane_setup_legacy_state(struct tegra_plane *tegra, struct tegra_plane_state *state) { @@ -478,5 +576,9 @@ int tegra_plane_setup_legacy_state(struct tegra_plane *tegra, if (err < 0) return err;
+ err = tegra_plane_setup_colorkey(tegra, state); + if (err < 0) + return err; + return 0; }
dri-devel@lists.freedesktop.org
-
Dmitry Osipenko -
Maarten Lankhorst -
Russell King - ARM Linux -
Ville Syrjälä