[RFC PATCH v4 0/2] drm: Add generic colorkey plane properties

[RFC PATCH v4 0/2] drm: Add generic colorkey plane properties

[Dmitry Osipenko]

Changes in v4:

1) In response to Ville's Syrjälä review comments:

	- Added new "colorkey.plane_mask" property that specifies which
	  planes shall participate in color key matching operation.

	- Added short glossary-comment to drm_plane_create_colorkey_properties()
	  that clarifies 'destination' / 'source' plane terms.

	- Returned "colorkey.mask" property that was dropped from v3, looks
	  like masking is quite common among different HW and hence makes more
	  sense to have that property by default instead of having additional
	  color keying modes.

	- Changed the color keying mode name to "transparent".

2) In response to Maarten's Lankhorst review comments:

	- Added a drm_colorkey_extract_component() helper which is supposed to
	  commonize color key component value extraction code among DRM drivers.

3) In response to Russell's King review comments:

	- The doc-comment to drm_plane_create_colorkey_properties() now
	  explicitly states that "The converted value shall be *rounded up* to
	  the nearest value". Hence userspace now knows what to expect when
	  plane has a 1bpp format.

Please review, thanks.


v3: https://lists.freedesktop.org/archives/dri-devel/2018-June/179057.html
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: Add generic colorkey properties for older Tegra's

Laurent Pinchart (1):
  drm: Add generic colorkey properties for display planes

 drivers/gpu/drm/drm_atomic.c  |  20 +++++
 drivers/gpu/drm/drm_blend.c   | 150 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/tegra/dc.c    |  25 ++++++
 drivers/gpu/drm/tegra/dc.h    |   7 ++
 drivers/gpu/drm/tegra/plane.c | 156 ++++++++++++++++++++++++++++++++++
 include/drm/drm_blend.h       |   3 +
 include/drm/drm_plane.h       |  91 ++++++++++++++++++++
 7 files changed, 452 insertions(+)

-- 
2.18.0

[RFC PATCH v4 1/2] drm: Add generic colorkey properties for display planes

[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 new 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 |  20 +++++
 drivers/gpu/drm/drm_blend.c  | 150 +++++++++++++++++++++++++++++++++++
 include/drm/drm_blend.h      |   3 +
 include/drm/drm_plane.h      |  91 +++++++++++++++++++++
 4 files changed, 264 insertions(+)

diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
index 3eb061e11e2e..e97364966f6d 100644
--- a/drivers/gpu/drm/drm_atomic.c
+++ b/drivers/gpu/drm/drm_atomic.c
@@ -904,6 +904,16 @@ 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.plane_mask_property) {
+		state->colorkey.plane_mask = val;
+	} else if (property == plane->colorkey.mode_property) {
+		state->colorkey.mode = val;
+	} else if (property == plane->colorkey.mask_property) {
+		state->colorkey.mask = 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) {
@@ -972,6 +982,16 @@ 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.plane_mask_property) {
+		*val = state->colorkey.plane_mask;
+	} else if (property == plane->colorkey.mode_property) {
+		*val = state->colorkey.mode;
+	} else if (property == plane->colorkey.mask_property) {
+		*val = state->colorkey.mask;
+	} 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..13c61dd0d9b7 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 actions like replacing a range of colors with a
+ *	transparent color in the plane. Color keying is disabled by default.
+ *
  * 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,148 @@ 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_TRANSPARENT] = "transparent",
+};
+
+/**
+ * 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 attaches them
+ * to the @plane to enable color keying control for blending operations.
+ *
+ * Glossary:
+ *
+ * Destination plane:
+ *	Plane to which color keying properties are applied, this planes takes
+ *	the effect of color keying operation. The effect is determined by a
+ *	given color keying mode.
+ *
+ * Source plane:
+ *	Pixels of this plane are the source for color key matching operation.
+ *
+ * Color keying is controlled by these properties:
+ *
+ * colorkey.plane_mask:
+ *	The mask property specifies which planes participate in color key
+ *	matching process, these planes are the color key sources.
+ *
+ *	Drivers return an error from their plane atomic check if plane can't be
+ *	handled.
+ *
+ * colorkey.mode:
+ *	The mode is an enumerated property that controls how color keying
+ *	operates.
+ *
+ * colorkey.mask:
+ *	This property specifies the pixel components mask. Unmasked pixel
+ *	components are not participating in the matching. This mask value is
+ *	applied to colorkey.min / max values. The mask value is given in 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.
+ *
+ *	Drivers return an error from their plane atomic check if mask can't be
+ *	handled.
+ *
+ * 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.
+ *	The converted value shall be *rounded up* to the nearest value.
+ *
+ *	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_device *dev = plane->dev;
+	struct drm_property *plane_mask_prop;
+	struct drm_property *mode_prop;
+	struct drm_property *mask_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;
+
+	plane_mask_prop = drm_property_create_range(dev, 0,
+						    "colorkey.plane_mask",
+						    0, U32_MAX);
+	if (!plane_mask_prop)
+		return -ENOMEM;
+
+	mode_prop = drm_property_create_enum(dev, 0, "colorkey.mode",
+					     modes_list, modes_num);
+	if (!mode_prop)
+		goto err_destroy_plane_mask_prop;
+
+	mask_prop = drm_property_create_range(dev, 0, "colorkey.mask",
+					      0, U64_MAX);
+	if (!mask_prop)
+		goto err_destroy_mode_prop;
+
+	min_prop = drm_property_create_range(dev, 0, "colorkey.min",
+					     0, U64_MAX);
+	if (!min_prop)
+		goto err_destroy_mask_prop;
+
+	max_prop = drm_property_create_range(dev, 0, "colorkey.max",
+					     0, U64_MAX);
+	if (!max_prop)
+		goto err_destroy_min_prop;
+
+	drm_object_attach_property(&plane->base, plane_mask_prop, 0);
+	drm_object_attach_property(&plane->base, mode_prop, 0);
+	drm_object_attach_property(&plane->base, mask_prop, 0);
+	drm_object_attach_property(&plane->base, min_prop, 0);
+	drm_object_attach_property(&plane->base, max_prop, 0);
+
+	plane->colorkey.plane_mask_property = plane_mask_prop;
+	plane->colorkey.mode_property = mode_prop;
+	plane->colorkey.mask_property = mask_prop;
+	plane->colorkey.min_property = min_prop;
+	plane->colorkey.max_property = max_prop;
+
+	return 0;
+
+err_destroy_min_prop:
+	drm_property_destroy(dev, min_prop);
+err_destroy_mask_prop:
+	drm_property_destroy(dev, mask_prop);
+err_destroy_mode_prop:
+	drm_property_destroy(dev, mode_prop);
+err_destroy_plane_mask_prop:
+	drm_property_destroy(dev, plane_mask_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 8a152dc16ea5..ab6a91e6b54e 100644
--- a/include/drm/drm_plane.h
+++ b/include/drm/drm_plane.h
@@ -25,6 +25,7 @@
 
 #include <linux/list.h>
 #include <linux/ctype.h>
+#include <linux/kernel.h>
 #include <drm/drm_mode_object.h>
 #include <drm/drm_color_mgmt.h>
 
@@ -32,6 +33,52 @@ 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_TRANSPARENT:
+	 *
+	 * Destination plane pixels are completely transparent in areas
+	 * where pixels of a source plane are matching a given color key
+	 * range, in other cases pixels of a destination plane are unaffected.
+	 * In areas where two or more source planes overlap, the topmost
+	 * plane takes precedence.
+	 */
+	DRM_PLANE_COLORKEY_MODE_TRANSPARENT,
+
+	/**
+	 * @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
+ * @mode: color keying mode
+ * @plane_mask: source planes that participate in color key matching
+ * @mask: color key mask (in ARGB16161616 format)
+ * @min: color key range minimum (in ARGB16161616 format)
+ * @max: color key range maximum (in ARGB16161616 format)
+ */
+struct drm_plane_colorkey_state {
+	enum drm_plane_colorkey_mode mode;
+	u64 plane_mask;
+	u64 mask;
+	u64 min;
+	u64 max;
+};
+
 /**
  * struct drm_plane_state - mutable plane state
  *
@@ -148,6 +195,13 @@ struct drm_plane_state {
 	 */
 	unsigned int normalized_zpos;
 
+	/**
+	 * @colorkey:
+	 * Color keying of the plane. See drm_plane_create_colorkey_properties()
+	 * for more details.
+	 */
+	struct drm_plane_colorkey_state colorkey;
+
 	/**
 	 * @color_encoding:
 	 *
@@ -660,6 +714,19 @@ struct drm_plane {
 	 */
 	struct drm_property *rotation_property;
 
+	/**
+	 * @colorkey:
+	 * Optional color keying properties for this plane. See
+	 * drm_plane_create_colorkey_properties().
+	 */
+	struct {
+		struct drm_property *plane_mask_property;
+		struct drm_property *mode_property;
+		struct drm_property *mask_property;
+		struct drm_property *min_property;
+		struct drm_property *max_property;
+	} colorkey;
+
 	/**
 	 * @color_encoding_property:
 	 *
@@ -779,5 +846,29 @@ static inline struct drm_plane *drm_plane_find(struct drm_device *dev,
 #define drm_for_each_plane(plane, dev) \
 	list_for_each_entry(plane, &(dev)->mode_config.plane_list, head)
 
+/**
+ * drm_colorkey_extract_component - get color key component value
+ * @ckey64: 64bit color key value
+ * @comp_name: name of 16bit color component to extract
+ * @nbits: size in bits of extracted component value
+ *
+ * Extract 16bit color component of @ckey64 given by @comp_name (alpha, red,
+ * green or blue) and convert it to an unsigned integer that has bit-width
+ * of @nbits (result is rounded-up).
+ */
+#define drm_colorkey_extract_component(ckey64, comp_name, nbits) \
+	__DRM_CKEY_CLAMP(__DRM_CKEY_CONV(ckey64, comp_name, nbits), nbits)
+
+#define __drm_ckey_alpha_shift	48
+#define __drm_ckey_red_shift	32
+#define __drm_ckey_green_shift	16
+#define __drm_ckey_blue_shift	0
+
+#define __DRM_CKEY_CONV(ckey64, comp_name, nbits) \
+	DIV_ROUND_UP((u16)((ckey64) >> __drm_ckey_ ## comp_name ## _shift), \
+		     1 << (16 - (nbits)))
+
+#define __DRM_CKEY_CLAMP(value, nbits) \
+	min_t(u16, (value), (1 << (nbits)) - 1)
 
 #endif
-- 
2.18.0

Re: [RFC PATCH v4 1/2] drm: Add generic colorkey properties for display planes

[Russell King - ARM Linux]

On Tue, Aug 07, 2018 at 08:22:01PM +0300, Dmitry Osipenko wrote:
> + * Glossary:
> + *
> + * Destination plane:
> + *	Plane to which color keying properties are applied, this planes takes
> + *	the effect of color keying operation. The effect is determined by a
> + *	given color keying mode.
> + *
> + * Source plane:
> + *	Pixels of this plane are the source for color key matching operation.
...
> +	/**
> +	 * @DRM_PLANE_COLORKEY_MODE_TRANSPARENT:
> +	 *
> +	 * Destination plane pixels are completely transparent in areas
> +	 * where pixels of a source plane are matching a given color key
> +	 * range, in other cases pixels of a destination plane are unaffected.
> +	 * In areas where two or more source planes overlap, the topmost
> +	 * plane takes precedence.
> +	 */

This seems confusing to me.

What you seem to be saying is that the "destination" plane would be the
one which is (eg0 the graphic plane, and the "source" plane would be the
the plane containing (eg) the video.  You seem to be saying that the
colorkey matches the video and determines whether the pixels in the
graphic plane are opaque or transparent.

Surely that is the wrong way round - in video overlay, you want to
colorkey match the contents of the graphic plane to determine which
pixels from the video plane to overlay.

If it's the other way around (source is the graphic, destination is the
video) it makes less sense to use the "source" and "destination" terms,
I can't see how you could describe a plane that is being overlaid on
top of another plane as a "destination".

I guess the terminology has come from a thought about using a GPU to
physically do the colorkeying when combining two planes - if the GPU
were to write to the "destination" plane, then this would be the wrong
way around.  For starters, taking the above example, the video plane
may well be smaller than the graphic plane.  If it's the other way
around, that has other problems, like destroying the colorkey in the
graphic plane when writing the video plane's contents to it.

So, in summary, I don't think "destination" and "source" are
particularly good terms to describe the operation, and I think you have
them swapped in your description of
"DRM_PLANE_COLORKEY_MODE_TRANSPARENT".

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up
According to speedtest.net: 13Mbps down 490kbps up

Re: [RFC PATCH v4 1/2] drm: Add generic colorkey properties for display planes

[Dmitry Osipenko]

On Wednesday, 8 August 2018 11:16:09 MSK Russell King - ARM Linux wrote:
> On Tue, Aug 07, 2018 at 08:22:01PM +0300, Dmitry Osipenko wrote:
> > + * Glossary:
> > + *
> > + * Destination plane:
> > + *	Plane to which color keying properties are applied, this planes takes
> > + *	the effect of color keying operation. The effect is determined by a
> > + *	given color keying mode.
> > + *
> > + * Source plane:
> > + *	Pixels of this plane are the source for color key matching operation.
> 
> ...
> 
> > +	/**
> > +	 * @DRM_PLANE_COLORKEY_MODE_TRANSPARENT:
> > +	 *
> > +	 * Destination plane pixels are completely transparent in areas
> > +	 * where pixels of a source plane are matching a given color key
> > +	 * range, in other cases pixels of a destination plane are 
unaffected.
> > +	 * In areas where two or more source planes overlap, the topmost
> > +	 * plane takes precedence.
> > +	 */
> 
> This seems confusing to me.
> 
> What you seem to be saying is that the "destination" plane would be the
> one which is (eg0 the graphic plane, and the "source" plane would be the
> the plane containing (eg) the video.  You seem to be saying that the
> colorkey matches the video and determines whether the pixels in the
> graphic plane are opaque or transparent.

Your example is correct.

With the "plane_mask" property we can specify any plane as the "source" for 
color key, so it can been either a video plane or graphic plane and even both 
at the same time.

> Surely that is the wrong way round - in video overlay, you want to
> colorkey match the contents of the graphic plane to determine which
> pixels from the video plane to overlay.

The "transparent" mode makes the color-matched pixels to become transparent, 
you want the inversion effect and hence that should be called something like a 
"transparent-inverted" mode. Maarten Lankhorst was asking for that mode in his 
comment to v3, I'm leaving for somebody else to add that mode later since 
there is no real use for it on Tegra right now.

So in your case the graphic plane will be the "source" plane (specified via 
the colorkey.plane_mask property), video plane will be the "destination" plane 
(plane to which the colorkey properties are applied) and the colorkey.mode 
will be "transparent-inverted". Pixels of the "source" plane are being matched 
and "destination" plane takes the effect of color keying operation, i.e. the 
color-matched pixels of graphic plane leave the video plane pixels unaffected 
and the unmatched pixels make the video plane pixels transparent.

> If it's the other way around (source is the graphic, destination is the
> video) it makes less sense to use the "source" and "destination" terms,
> I can't see how you could describe a plane that is being overlaid on
> top of another plane as a "destination".

Tegra has a bit annoying limitations in regard to a reduced plane blending 
functionality when color keying is enabled. I found that the best variant to 
work around the limitations is to move the graphic plane on top of the video 
plane and to make the graphic plane to match itself. I.e. the matched pixels 
of graphic plane become transparent and hence poked by video plane.

> I guess the terminology has come from a thought about using a GPU to
> physically do the colorkeying when combining two planes - if the GPU
> were to write to the "destination" plane, then this would be the wrong
> way around.  For starters, taking the above example, the video plane
> may well be smaller than the graphic plane.  If it's the other way
> around, that has other problems, like destroying the colorkey in the
> graphic plane when writing the video plane's contents to it.

It all depends on a use-case scenario. It won't be easy for userspace to 
generalize the usage of color keying, at best the color keying interface could 
be generalized and then userspace may choose the best fitting variant based on 
available HW capabilities.

> So, in summary, I don't think "destination" and "source" are
> particularly good terms to describe the operation, and I think you have
> them swapped in your description of
> "DRM_PLANE_COLORKEY_MODE_TRANSPARENT".

Maybe the DRM_PLANE_COLORKEY_MODE_TRANSPARENT should become 
DRM_PLANE_COLORKEY_MODE_TRANSPARENT_INVERTED? 

Any more opinions?

Re: [RFC PATCH v4 1/2] drm: Add generic colorkey properties for display planes

[Maarten Lankhorst]

Op 08-08-18 om 16:30 schreef Dmitry Osipenko:
> On Wednesday, 8 August 2018 11:16:09 MSK Russell King - ARM Linux wrote:
>> On Tue, Aug 07, 2018 at 08:22:01PM +0300, Dmitry Osipenko wrote:
>>> + * Glossary:
>>> + *
>>> + * Destination plane:
>>> + *	Plane to which color keying properties are applied, this planes takes
>>> + *	the effect of color keying operation. The effect is determined by a
>>> + *	given color keying mode.
>>> + *
>>> + * Source plane:
>>> + *	Pixels of this plane are the source for color key matching operation.
>> ...
>>
>>> +	/**
>>> +	 * @DRM_PLANE_COLORKEY_MODE_TRANSPARENT:
>>> +	 *
>>> +	 * Destination plane pixels are completely transparent in areas
>>> +	 * where pixels of a source plane are matching a given color key
>>> +	 * range, in other cases pixels of a destination plane are 
> unaffected.
>>> +	 * In areas where two or more source planes overlap, the topmost
>>> +	 * plane takes precedence.
>>> +	 */
>> This seems confusing to me.
>>
>> What you seem to be saying is that the "destination" plane would be the
>> one which is (eg0 the graphic plane, and the "source" plane would be the
>> the plane containing (eg) the video.  You seem to be saying that the
>> colorkey matches the video and determines whether the pixels in the
>> graphic plane are opaque or transparent.
> Your example is correct.
>
> With the "plane_mask" property we can specify any plane as the "source" for 
> color key, so it can been either a video plane or graphic plane and even both 
> at the same time.
I'm not sure we should specify plane mask from userspace.

Can't we make major loops? How do you want to handle those?
>> Surely that is the wrong way round - in video overlay, you want to
>> colorkey match the contents of the graphic plane to determine which
>> pixels from the video plane to overlay.
> The "transparent" mode makes the color-matched pixels to become transparent, 
> you want the inversion effect and hence that should be called something like a 
> "transparent-inverted" mode. Maarten Lankhorst was asking for that mode in his 
> comment to v3, I'm leaving for somebody else to add that mode later since 
> there is no real use for it on Tegra right now.
I would like it to be described and included, so I can convert the existing intel_sprite_set_colorkey_ioctl to atomic.

Then again, could transparent-inverted also be handled by setting transparent on the primary?


> So in your case the graphic plane will be the "source" plane (specified via 
> the colorkey.plane_mask property), video plane will be the "destination" plane 
> (plane to which the colorkey properties are applied) and the colorkey.mode 
> will be "transparent-inverted". Pixels of the "source" plane are being matched 
> and "destination" plane takes the effect of color keying operation, i.e. the 
> color-matched pixels of graphic plane leave the video plane pixels unaffected 
> and the unmatched pixels make the video plane pixels transparent.
>
>> If it's the other way around (source is the graphic, destination is the
>> video) it makes less sense to use the "source" and "destination" terms,
>> I can't see how you could describe a plane that is being overlaid on
>> top of another plane as a "destination".
> Tegra has a bit annoying limitations in regard to a reduced plane blending 
> functionality when color keying is enabled. I found that the best variant to 
> work around the limitations is to move the graphic plane on top of the video 
> plane and to make the graphic plane to match itself. I.e. the matched pixels 
> of graphic plane become transparent and hence poked by video plane.
>
>> I guess the terminology has come from a thought about using a GPU to
>> physically do the colorkeying when combining two planes - if the GPU
>> were to write to the "destination" plane, then this would be the wrong
>> way around.  For starters, taking the above example, the video plane
>> may well be smaller than the graphic plane.  If it's the other way
>> around, that has other problems, like destroying the colorkey in the
>> graphic plane when writing the video plane's contents to it.
> It all depends on a use-case scenario. It won't be easy for userspace to 
> generalize the usage of color keying, at best the color keying interface could 
> be generalized and then userspace may choose the best fitting variant based on 
> available HW capabilities.
There's TEST_ONLY for a reason, though I guess it makes generic color keying slightly more invovled for userspace. :)
>> So, in summary, I don't think "destination" and "source" are
>> particularly good terms to describe the operation, and I think you have
>> them swapped in your description of
>> "DRM_PLANE_COLORKEY_MODE_TRANSPARENT".
> Maybe the DRM_PLANE_COLORKEY_MODE_TRANSPARENT should become 
> DRM_PLANE_COLORKEY_MODE_TRANSPARENT_INVERTED? 
>
> Any more opinions?
>
>
>

Re: [RFC PATCH v4 1/2] drm: Add generic colorkey properties for display planes

[Dmitry Osipenko]

On 8/16/18 2:42 PM, Maarten Lankhorst wrote:
> Op 08-08-18 om 16:30 schreef Dmitry Osipenko:
>> On Wednesday, 8 August 2018 11:16:09 MSK Russell King - ARM Linux wrote:
>>> On Tue, Aug 07, 2018 at 08:22:01PM +0300, Dmitry Osipenko wrote:
>>>> + * Glossary:
>>>> + *
>>>> + * Destination plane:
>>>> + *	Plane to which color keying properties are applied, this planes takes
>>>> + *	the effect of color keying operation. The effect is determined by a
>>>> + *	given color keying mode.
>>>> + *
>>>> + * Source plane:
>>>> + *	Pixels of this plane are the source for color key matching operation.
>>> ...
>>>
>>>> +	/**
>>>> +	 * @DRM_PLANE_COLORKEY_MODE_TRANSPARENT:
>>>> +	 *
>>>> +	 * Destination plane pixels are completely transparent in areas
>>>> +	 * where pixels of a source plane are matching a given color key
>>>> +	 * range, in other cases pixels of a destination plane are
>> unaffected.
>>>> +	 * In areas where two or more source planes overlap, the topmost
>>>> +	 * plane takes precedence.
>>>> +	 */
>>> This seems confusing to me.
>>>
>>> What you seem to be saying is that the "destination" plane would be the
>>> one which is (eg0 the graphic plane, and the "source" plane would be the
>>> the plane containing (eg) the video.  You seem to be saying that the
>>> colorkey matches the video and determines whether the pixels in the
>>> graphic plane are opaque or transparent.
>> Your example is correct.
>>
>> With the "plane_mask" property we can specify any plane as the "source" for
>> color key, so it can been either a video plane or graphic plane and even both
>> at the same time.
> I'm not sure we should specify plane mask from userspace.

It looks like a quite flexible approach. Do you have any other suggestions?

> Can't we make major loops? How do you want to handle those?

It's up to a specific driver to accept the mask or reject it. You could make a 
loop if HW allows to do that, I don't see what's the problem.

>>> Surely that is the wrong way round - in video overlay, you want to
>>> colorkey match the contents of the graphic plane to determine which
>>> pixels from the video plane to overlay.
>> The "transparent" mode makes the color-matched pixels to become transparent,
>> you want the inversion effect and hence that should be called something like a
>> "transparent-inverted" mode. Maarten Lankhorst was asking for that mode in his
>> comment to v3, I'm leaving for somebody else to add that mode later since
>> there is no real use for it on Tegra right now.
> I would like it to be described and included, so I can convert the existing intel_sprite_set_colorkey_ioctl to atomic.

Okay, I can add it. Though probably better to call that mode "opaque" rather 
than "transparent-inverted".

> Then again, could transparent-inverted also be handled by setting transparent on the primary?

If HW allows to do that, then yes.

> 
>> So in your case the graphic plane will be the "source" plane (specified via
>> the colorkey.plane_mask property), video plane will be the "destination" plane
>> (plane to which the colorkey properties are applied) and the colorkey.mode
>> will be "transparent-inverted". Pixels of the "source" plane are being matched
>> and "destination" plane takes the effect of color keying operation, i.e. the
>> color-matched pixels of graphic plane leave the video plane pixels unaffected
>> and the unmatched pixels make the video plane pixels transparent.
>>
>>> If it's the other way around (source is the graphic, destination is the
>>> video) it makes less sense to use the "source" and "destination" terms,
>>> I can't see how you could describe a plane that is being overlaid on
>>> top of another plane as a "destination".
>> Tegra has a bit annoying limitations in regard to a reduced plane blending
>> functionality when color keying is enabled. I found that the best variant to
>> work around the limitations is to move the graphic plane on top of the video
>> plane and to make the graphic plane to match itself. I.e. the matched pixels
>> of graphic plane become transparent and hence poked by video plane.
>>
>>> I guess the terminology has come from a thought about using a GPU to
>>> physically do the colorkeying when combining two planes - if the GPU
>>> were to write to the "destination" plane, then this would be the wrong
>>> way around.  For starters, taking the above example, the video plane
>>> may well be smaller than the graphic plane.  If it's the other way
>>> around, that has other problems, like destroying the colorkey in the
>>> graphic plane when writing the video plane's contents to it.
>> It all depends on a use-case scenario. It won't be easy for userspace to
>> generalize the usage of color keying, at best the color keying interface could
>> be generalized and then userspace may choose the best fitting variant based on
>> available HW capabilities.
> There's TEST_ONLY for a reason, though I guess it makes generic color keying slightly more invovled for userspace. :)

It is also quite involved for kernel to present a non-standard feature as 
something generic, pleasing everyone in the same time.

Re: [RFC PATCH v4 1/2] drm: Add generic colorkey properties for display planes

[Laurent Pinchart]

Hi Dmitry,

Thank you for the patch.

On Tuesday, 7 August 2018 20:22:01 EEST Dmitry Osipenko wrote:
> 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 new 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 |  20 +++++
>  drivers/gpu/drm/drm_blend.c  | 150 +++++++++++++++++++++++++++++++++++
>  include/drm/drm_blend.h      |   3 +
>  include/drm/drm_plane.h      |  91 +++++++++++++++++++++
>  4 files changed, 264 insertions(+)

[snip]

> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index a16a74d7e15e..13c61dd0d9b7 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 actions like replacing a range of colors with a
> + *	transparent color in the plane. Color keying is disabled by default.
> + *
>   * 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,148 @@ 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_TRANSPARENT] = "transparent",
> +};
> +
> +/**
> + * 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 attaches
> them
> + * to the @plane to enable color keying control for blending operations.
> + *
> + * Glossary:
> + *
> + * Destination plane:
> + *	Plane to which color keying properties are applied, this planes takes
> + *	the effect of color keying operation. The effect is determined by a
> + *	given color keying mode.
> + *
> + * Source plane:
> + *	Pixels of this plane are the source for color key matching operation.
> + *
> + * Color keying is controlled by these properties:
> + *
> + * colorkey.plane_mask:
> + *	The mask property specifies which planes participate in color key
> + *	matching process, these planes are the color key sources.
> + *
> + *	Drivers return an error from their plane atomic check if plane can't be
> + *	handled.

This seems fragile to me. We don't document how userspace determines which 
planes need to be specified here, and we don't document what happens if a 
plane underneath the destination plane is not specified in the mask. More 
precise documentation is needed if we want to use such a property.

It also seems quite complex. Is an explicit plane mask really the best option 
? What's the reason why planes couldn't be handled ? How do drivers determine 
that ?

> + * colorkey.mode:
> + *	The mode is an enumerated property that controls how color keying
> + *	operates.

A link to the drm_plane_colorkey_mode enum documentation would be useful.

> + * colorkey.mask:
> + *	This property specifies the pixel components mask. Unmasked pixel
> + *	components are not participating in the matching. This mask value is
> + *	applied to colorkey.min / max values. The mask value is given in 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.
> + *
> + *	Drivers return an error from their plane atomic check if mask can't be
> + *	handled.
> + *
> + * 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.
> + *	The converted value shall be *rounded up* to the nearest value.
> + *
> + *	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.
> + */

While you're defining the concept of source and destination planes, it's not 
clear from the documentation how all this maps to the usual source and 
destination color keying concepts. I think that should be documented as well 
or users will be confused. Examples could help in this area.

[snip]

> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> index 8a152dc16ea5..ab6a91e6b54e 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h

[snip]

> @@ -32,6 +33,52 @@ struct drm_crtc;
>  struct drm_printer;
>  struct drm_modeset_acquire_ctx;
> 
> +/**
> + * enum drm_plane_colorkey_mode - uapi plane colorkey mode enumeration
> + */

If it's uAPI, should it be moved to include/uapi/drm/ ?

> +enum drm_plane_colorkey_mode {
> +	/**
> +	 * @DRM_PLANE_COLORKEY_MODE_DISABLED:
> +	 *
> +	 * No color matching performed in this mode.

Do you mean "No color keying" ?

> +	 */
> +	DRM_PLANE_COLORKEY_MODE_DISABLED,
> +
> +	/**
> +	 * @DRM_PLANE_COLORKEY_MODE_TRANSPARENT:
> +	 *
> +	 * Destination plane pixels are completely transparent in areas
> +	 * where pixels of a source plane are matching a given color key
> +	 * range, in other cases pixels of a destination plane are unaffected.

How do we handle hardware that performs configurable color replacement instead 
of a fixed fully transparency ? That was included in my original proposal and 
available in R-Car hardware.

> +	 * In areas where two or more source planes overlap, the topmost
> +	 * plane takes precedence.
> +	 */
> +	DRM_PLANE_COLORKEY_MODE_TRANSPARENT,
> +
> +	/**
> +	 * @DRM_PLANE_COLORKEY_MODES_NUM:
> +	 *
> +	 * Total number of color keying modes.
> +	 */
> +	DRM_PLANE_COLORKEY_MODES_NUM,

This one, however, shouldn't be part of the uAPI as it will change when we 
will add new modes.

> +};

[snip]

> @@ -779,5 +846,29 @@ static inline struct drm_plane *drm_plane_find(struct
> drm_device *dev, #define drm_for_each_plane(plane, dev) \
>  	list_for_each_entry(plane, &(dev)->mode_config.plane_list, head)
> 
> +/**
> + * drm_colorkey_extract_component - get color key component value
> + * @ckey64: 64bit color key value
> + * @comp_name: name of 16bit color component to extract
> + * @nbits: size in bits of extracted component value
> + *
> + * Extract 16bit color component of @ckey64 given by @comp_name (alpha,
> red,
> + * green or blue) and convert it to an unsigned integer that has bit-width
> + * of @nbits (result is rounded-up).
> + */
> +#define drm_colorkey_extract_component(ckey64, comp_name, nbits) \
> +	__DRM_CKEY_CLAMP(__DRM_CKEY_CONV(ckey64, comp_name, nbits), nbits)
> +
> +#define __drm_ckey_alpha_shift	48
> +#define __drm_ckey_red_shift	32
> +#define __drm_ckey_green_shift	16
> +#define __drm_ckey_blue_shift	0
> +
> +#define __DRM_CKEY_CONV(ckey64, comp_name, nbits) \
> +	DIV_ROUND_UP((u16)((ckey64) >> __drm_ckey_ ## comp_name ## _shift), \
> +		     1 << (16 - (nbits)))

As the divisor is a power of two, could we use masking instead of a division ? 
Or do you expect the compiler to optimize it properly ?

> +#define __DRM_CKEY_CLAMP(value, nbits) \
> +	min_t(u16, (value), (1 << (nbits)) - 1)

Would the following be simpler to read and a bit more efficient as it avoids 
the division ?

static inline u16 __drm_colorkey_extract_component(u64 ckey64, 
                                                   unsigned int shift, 
                                                   unsigned int nbits)
{       
        u16 mask = (1 << (16 - nbits)) - 1;
        
        return ((u16)(ckey >> shift) + mask) >> (16 - nbits);
}

#define drm_colorkey_extract_component(ckey64, comp_name, nbits) \
        __drm_colorkey_extract_component(ckey64, __drm_ckey_ ## comp_name ## 
_shift, nbits)

>  #endif

-- 
Regards,

Laurent Pinchart

Re: [RFC PATCH v4 1/2] drm: Add generic colorkey properties for display planes

[Daniel Vetter]

On Tue, Aug 14, 2018 at 12:48:08PM +0300, Laurent Pinchart wrote:
> Hi Dmitry,
> 
> Thank you for the patch.
> 
> On Tuesday, 7 August 2018 20:22:01 EEST Dmitry Osipenko wrote:
> > 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 new 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 |  20 +++++
> >  drivers/gpu/drm/drm_blend.c  | 150 +++++++++++++++++++++++++++++++++++
> >  include/drm/drm_blend.h      |   3 +
> >  include/drm/drm_plane.h      |  91 +++++++++++++++++++++
> >  4 files changed, 264 insertions(+)
> 
> [snip]
> 
> > diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> > index a16a74d7e15e..13c61dd0d9b7 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 actions like replacing a range of colors with a
> > + *	transparent color in the plane. Color keying is disabled by default.
> > + *
> >   * 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,148 @@ 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_TRANSPARENT] = "transparent",
> > +};
> > +
> > +/**
> > + * 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 attaches
> > them
> > + * to the @plane to enable color keying control for blending operations.
> > + *
> > + * Glossary:
> > + *
> > + * Destination plane:
> > + *	Plane to which color keying properties are applied, this planes takes
> > + *	the effect of color keying operation. The effect is determined by a
> > + *	given color keying mode.
> > + *
> > + * Source plane:
> > + *	Pixels of this plane are the source for color key matching operation.
> > + *
> > + * Color keying is controlled by these properties:
> > + *
> > + * colorkey.plane_mask:
> > + *	The mask property specifies which planes participate in color key
> > + *	matching process, these planes are the color key sources.
> > + *
> > + *	Drivers return an error from their plane atomic check if plane can't be
> > + *	handled.
> 
> This seems fragile to me. We don't document how userspace determines which 
> planes need to be specified here, and we don't document what happens if a 
> plane underneath the destination plane is not specified in the mask. More 
> precise documentation is needed if we want to use such a property.
> 
> It also seems quite complex. Is an explicit plane mask really the best option 
> ? What's the reason why planes couldn't be handled ? How do drivers determine 
> that ?

General comment: This is why we need the reference userspace. I also
think that any feature throwing up so many tricky questions should come
with a full set of igt testcases. Since the reference userspace cannot
answer how the new uapi should work in all corner-cases (failures and
stuff like that).
-Daniel

> 
> > + * colorkey.mode:
> > + *	The mode is an enumerated property that controls how color keying
> > + *	operates.
> 
> A link to the drm_plane_colorkey_mode enum documentation would be useful.
> 
> > + * colorkey.mask:
> > + *	This property specifies the pixel components mask. Unmasked pixel
> > + *	components are not participating in the matching. This mask value is
> > + *	applied to colorkey.min / max values. The mask value is given in 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.
> > + *
> > + *	Drivers return an error from their plane atomic check if mask can't be
> > + *	handled.
> > + *
> > + * 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.
> > + *	The converted value shall be *rounded up* to the nearest value.
> > + *
> > + *	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.
> > + */
> 
> While you're defining the concept of source and destination planes, it's not 
> clear from the documentation how all this maps to the usual source and 
> destination color keying concepts. I think that should be documented as well 
> or users will be confused. Examples could help in this area.
> 
> [snip]
> 
> > diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
> > index 8a152dc16ea5..ab6a91e6b54e 100644
> > --- a/include/drm/drm_plane.h
> > +++ b/include/drm/drm_plane.h
> 
> [snip]
> 
> > @@ -32,6 +33,52 @@ struct drm_crtc;
> >  struct drm_printer;
> >  struct drm_modeset_acquire_ctx;
> > 
> > +/**
> > + * enum drm_plane_colorkey_mode - uapi plane colorkey mode enumeration
> > + */
> 
> If it's uAPI, should it be moved to include/uapi/drm/ ?
> 
> > +enum drm_plane_colorkey_mode {
> > +	/**
> > +	 * @DRM_PLANE_COLORKEY_MODE_DISABLED:
> > +	 *
> > +	 * No color matching performed in this mode.
> 
> Do you mean "No color keying" ?
> 
> > +	 */
> > +	DRM_PLANE_COLORKEY_MODE_DISABLED,
> > +
> > +	/**
> > +	 * @DRM_PLANE_COLORKEY_MODE_TRANSPARENT:
> > +	 *
> > +	 * Destination plane pixels are completely transparent in areas
> > +	 * where pixels of a source plane are matching a given color key
> > +	 * range, in other cases pixels of a destination plane are unaffected.
> 
> How do we handle hardware that performs configurable color replacement instead 
> of a fixed fully transparency ? That was included in my original proposal and 
> available in R-Car hardware.
> 
> > +	 * In areas where two or more source planes overlap, the topmost
> > +	 * plane takes precedence.
> > +	 */
> > +	DRM_PLANE_COLORKEY_MODE_TRANSPARENT,
> > +
> > +	/**
> > +	 * @DRM_PLANE_COLORKEY_MODES_NUM:
> > +	 *
> > +	 * Total number of color keying modes.
> > +	 */
> > +	DRM_PLANE_COLORKEY_MODES_NUM,
> 
> This one, however, shouldn't be part of the uAPI as it will change when we 
> will add new modes.
> 
> > +};
> 
> [snip]
> 
> > @@ -779,5 +846,29 @@ static inline struct drm_plane *drm_plane_find(struct
> > drm_device *dev, #define drm_for_each_plane(plane, dev) \
> >  	list_for_each_entry(plane, &(dev)->mode_config.plane_list, head)
> > 
> > +/**
> > + * drm_colorkey_extract_component - get color key component value
> > + * @ckey64: 64bit color key value
> > + * @comp_name: name of 16bit color component to extract
> > + * @nbits: size in bits of extracted component value
> > + *
> > + * Extract 16bit color component of @ckey64 given by @comp_name (alpha,
> > red,
> > + * green or blue) and convert it to an unsigned integer that has bit-width
> > + * of @nbits (result is rounded-up).
> > + */
> > +#define drm_colorkey_extract_component(ckey64, comp_name, nbits) \
> > +	__DRM_CKEY_CLAMP(__DRM_CKEY_CONV(ckey64, comp_name, nbits), nbits)
> > +
> > +#define __drm_ckey_alpha_shift	48
> > +#define __drm_ckey_red_shift	32
> > +#define __drm_ckey_green_shift	16
> > +#define __drm_ckey_blue_shift	0
> > +
> > +#define __DRM_CKEY_CONV(ckey64, comp_name, nbits) \
> > +	DIV_ROUND_UP((u16)((ckey64) >> __drm_ckey_ ## comp_name ## _shift), \
> > +		     1 << (16 - (nbits)))
> 
> As the divisor is a power of two, could we use masking instead of a division ? 
> Or do you expect the compiler to optimize it properly ?
> 
> > +#define __DRM_CKEY_CLAMP(value, nbits) \
> > +	min_t(u16, (value), (1 << (nbits)) - 1)
> 
> Would the following be simpler to read and a bit more efficient as it avoids 
> the division ?
> 
> static inline u16 __drm_colorkey_extract_component(u64 ckey64, 
>                                                    unsigned int shift, 
>                                                    unsigned int nbits)
> {       
>         u16 mask = (1 << (16 - nbits)) - 1;
>         
>         return ((u16)(ckey >> shift) + mask) >> (16 - nbits);
> }
> 
> #define drm_colorkey_extract_component(ckey64, comp_name, nbits) \
>         __drm_colorkey_extract_component(ckey64, __drm_ckey_ ## comp_name ## 
> _shift, nbits)
> 
> >  #endif
> 
> -- 
> Regards,
> 
> Laurent Pinchart
> 
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

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

Re: [RFC PATCH v4 1/2] drm: Add generic colorkey properties for display planes

[Dmitry Osipenko]

On 8/14/18 1:38 PM, Daniel Vetter wrote:
> On Tue, Aug 14, 2018 at 12:48:08PM +0300, Laurent Pinchart wrote:
>> Hi Dmitry,
>>
>> Thank you for the patch.
>>
>> On Tuesday, 7 August 2018 20:22:01 EEST Dmitry Osipenko wrote:
>>> 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 new 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 |  20 +++++
>>>   drivers/gpu/drm/drm_blend.c  | 150 +++++++++++++++++++++++++++++++++++
>>>   include/drm/drm_blend.h      |   3 +
>>>   include/drm/drm_plane.h      |  91 +++++++++++++++++++++
>>>   4 files changed, 264 insertions(+)
>>
>> [snip]
>>
>>> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
>>> index a16a74d7e15e..13c61dd0d9b7 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 actions like replacing a range of colors with a
>>> + *	transparent color in the plane. Color keying is disabled by default.
>>> + *
>>>    * 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,148 @@ 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_TRANSPARENT] = "transparent",
>>> +};
>>> +
>>> +/**
>>> + * 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 attaches
>>> them
>>> + * to the @plane to enable color keying control for blending operations.
>>> + *
>>> + * Glossary:
>>> + *
>>> + * Destination plane:
>>> + *	Plane to which color keying properties are applied, this planes takes
>>> + *	the effect of color keying operation. The effect is determined by a
>>> + *	given color keying mode.
>>> + *
>>> + * Source plane:
>>> + *	Pixels of this plane are the source for color key matching operation.
>>> + *
>>> + * Color keying is controlled by these properties:
>>> + *
>>> + * colorkey.plane_mask:
>>> + *	The mask property specifies which planes participate in color key
>>> + *	matching process, these planes are the color key sources.
>>> + *
>>> + *	Drivers return an error from their plane atomic check if plane can't be
>>> + *	handled.
>>
>> This seems fragile to me. We don't document how userspace determines which
>> planes need to be specified here, and we don't document what happens if a
>> plane underneath the destination plane is not specified in the mask. More
>> precise documentation is needed if we want to use such a property.
>>
>> It also seems quite complex. Is an explicit plane mask really the best option
>> ? What's the reason why planes couldn't be handled ? How do drivers determine
>> that ?
> 
> General comment: This is why we need the reference userspace. I also
> think that any feature throwing up so many tricky questions should come
> with a full set of igt testcases.

At best I can write couple simple tests, simply because Tegra can't do more than 
that.

  Since the reference userspace cannot
> answer how the new uapi should work in all corner-cases (failures and
> stuff like that).

Okay.

Re: [RFC PATCH v4 1/2] drm: Add generic colorkey properties for display planes

[Dmitry Osipenko]

Hello Laurent,

Sorry for the late response.

On 8/14/18 12:48 PM, Laurent Pinchart wrote:
> Hi Dmitry,
> 
> Thank you for the patch. > On Tuesday, 7 August 2018 20:22:01 EEST Dmitry Osipenko wrote:
>> 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 new 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 |  20 +++++
>>  drivers/gpu/drm/drm_blend.c  | 150 +++++++++++++++++++++++++++++++++++
>>  include/drm/drm_blend.h      |   3 +
>>  include/drm/drm_plane.h      |  91 +++++++++++++++++++++
>>  4 files changed, 264 insertions(+)
> 
> [snip]
> 
>> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
>> index a16a74d7e15e..13c61dd0d9b7 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 actions like replacing a range of colors with a
>> + *	transparent color in the plane. Color keying is disabled by default.
>> + *
>>   * 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,148 @@ 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_TRANSPARENT] = "transparent",
>> +};
>> +
>> +/**
>> + * 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 attaches
>> them
>> + * to the @plane to enable color keying control for blending operations.
>> + *
>> + * Glossary:
>> + *
>> + * Destination plane:
>> + *	Plane to which color keying properties are applied, this planes takes
>> + *	the effect of color keying operation. The effect is determined by a
>> + *	given color keying mode.
>> + *
>> + * Source plane:
>> + *	Pixels of this plane are the source for color key matching operation.
>> + *
>> + * Color keying is controlled by these properties:
>> + *
>> + * colorkey.plane_mask:
>> + *	The mask property specifies which planes participate in color key
>> + *	matching process, these planes are the color key sources.
>> + *
>> + *	Drivers return an error from their plane atomic check if plane can't be
>> + *	handled.
> 
> This seems fragile to me. We don't document how userspace determines which 
> planes need to be specified here, and we don't document what happens if a 
> plane underneath the destination plane is not specified in the mask. More 
> precise documentation is needed if we want to use such a property.

I'll add couple more words.

> It also seems quite complex. Is an explicit plane mask really the best option 
> ? What's the reason why planes couldn't be handled ? How do drivers determine 
> that ?

The reasons for that property:

1) HW limitations. IIUC, some of the Intel HW has a limitation such that only a 
subset of planes could participate in the color keying. That should be also the 
case for Tegra and others.

Drivers know all available HW capabilities, hence they know exactly what could 
be handled.

2) Flexibility. This gives userspace more variants of how color keying could be 
performed.

Whether it's the best option is questionable, I don't have better ideas for now.

>> + * colorkey.mode:
>> + *	The mode is an enumerated property that controls how color keying
>> + *	operates.
> 
> A link to the drm_plane_colorkey_mode enum documentation would be useful.

Okay.

>> + * colorkey.mask:
>> + *	This property specifies the pixel components mask. Unmasked pixel
>> + *	components are not participating in the matching. This mask value is
>> + *	applied to colorkey.min / max values. The mask value is given in 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.
>> + *
>> + *	Drivers return an error from their plane atomic check if mask can't be
>> + *	handled.
>> + *
>> + * 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.
>> + *	The converted value shall be *rounded up* to the nearest value.
>> + *
>> + *	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.
>> + */
> 
> While you're defining the concept of source and destination planes, it's not 
> clear from the documentation how all this maps to the usual source and 
> destination color keying concepts. I think that should be documented as well 
> or users will be confused. Examples could help in this area.

Could you please clarify what are the "usual source and destination color keying 
concepts"?

> [snip]
> 
>> diff --git a/include/drm/drm_plane.h b/include/drm/drm_plane.h
>> index 8a152dc16ea5..ab6a91e6b54e 100644
>> --- a/include/drm/drm_plane.h
>> +++ b/include/drm/drm_plane.h
> 
> [snip]
> 
>> @@ -32,6 +33,52 @@ struct drm_crtc;
>>  struct drm_printer;
>>  struct drm_modeset_acquire_ctx;
>>
>> +/**
>> + * enum drm_plane_colorkey_mode - uapi plane colorkey mode enumeration
>> + */
> 
> If it's uAPI, should it be moved to include/uapi/drm/ ?
> 
>> +enum drm_plane_colorkey_mode {
>> +	/**
>> +	 * @DRM_PLANE_COLORKEY_MODE_DISABLED:
>> +	 *
>> +	 * No color matching performed in this mode.
> 
> Do you mean "No color keying" ?

Yes

>> +	 */
>> +	DRM_PLANE_COLORKEY_MODE_DISABLED,
>> +
>> +	/**
>> +	 * @DRM_PLANE_COLORKEY_MODE_TRANSPARENT:
>> +	 *
>> +	 * Destination plane pixels are completely transparent in areas
>> +	 * where pixels of a source plane are matching a given color key
>> +	 * range, in other cases pixels of a destination plane are unaffected.
> 
> How do we handle hardware that performs configurable color replacement instead 
> of a fixed fully transparency ? That was included in my original proposal and 
> available in R-Car hardware.

I thought that maybe it will be better to have it as another color keying mode. 
Though probably "replacement" mode should be also good for the complete 
transparency, will get back and revisit it.

>> +	 * In areas where two or more source planes overlap, the topmost
>> +	 * plane takes precedence.
>> +	 */
>> +	DRM_PLANE_COLORKEY_MODE_TRANSPARENT,
>> +
>> +	/**
>> +	 * @DRM_PLANE_COLORKEY_MODES_NUM:
>> +	 *
>> +	 * Total number of color keying modes.
>> +	 */
>> +	DRM_PLANE_COLORKEY_MODES_NUM,
> 
> This one, however, shouldn't be part of the uAPI as it will change when we 
> will add new modes.

But that won't break ABI and so shouldn't cause any problem, isn't it? Anyway, I 
don't mind to change that.

>> +};
> 
> [snip]
> 
>> @@ -779,5 +846,29 @@ static inline struct drm_plane *drm_plane_find(struct
>> drm_device *dev, #define drm_for_each_plane(plane, dev) \
>>  	list_for_each_entry(plane, &(dev)->mode_config.plane_list, head)
>>
>> +/**
>> + * drm_colorkey_extract_component - get color key component value
>> + * @ckey64: 64bit color key value
>> + * @comp_name: name of 16bit color component to extract
>> + * @nbits: size in bits of extracted component value
>> + *
>> + * Extract 16bit color component of @ckey64 given by @comp_name (alpha,
>> red,
>> + * green or blue) and convert it to an unsigned integer that has bit-width
>> + * of @nbits (result is rounded-up).
>> + */
>> +#define drm_colorkey_extract_component(ckey64, comp_name, nbits) \
>> +	__DRM_CKEY_CLAMP(__DRM_CKEY_CONV(ckey64, comp_name, nbits), nbits)
>> +
>> +#define __drm_ckey_alpha_shift	48
>> +#define __drm_ckey_red_shift	32
>> +#define __drm_ckey_green_shift	16
>> +#define __drm_ckey_blue_shift	0
>> +
>> +#define __DRM_CKEY_CONV(ckey64, comp_name, nbits) \
>> +	DIV_ROUND_UP((u16)((ckey64) >> __drm_ckey_ ## comp_name ## _shift), \
>> +		     1 << (16 - (nbits)))
> 
> As the divisor is a power of two, could we use masking instead of a division ? 
> Or do you expect the compiler to optimize it properly ?
> 
>> +#define __DRM_CKEY_CLAMP(value, nbits) \
>> +	min_t(u16, (value), (1 << (nbits)) - 1)
> 
> Would the following be simpler to read and a bit more efficient as it avoids 
> the division ?

Yes, thank you. Only the final result needs to be clamped in the end.

> static inline u16 __drm_colorkey_extract_component(u64 ckey64, 
>                                                    unsigned int shift, 
>                                                    unsigned int nbits)
> {       
>         u16 mask = (1 << (16 - nbits)) - 1;
>         
>         return ((u16)(ckey >> shift) + mask) >> (16 - nbits);
> }
> 
> #define drm_colorkey_extract_component(ckey64, comp_name, nbits) \
>         __drm_colorkey_extract_component(ckey64, __drm_ckey_ ## comp_name ## 
> _shift, nbits)
> 
>>  #endif
> 

[RFC PATCH v4 2/2] drm/tegra: plane: Add generic colorkey properties for older Tegra's

[Dmitry Osipenko]

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. Blending controls interface
changed on newer Tegra's, this patch provides color keying support for
Tegra20/30/114 only.

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 | 156 ++++++++++++++++++++++++++++++++++
 3 files changed, 188 insertions(+)

diff --git a/drivers/gpu/drm/tegra/dc.c b/drivers/gpu/drm/tegra/dc.c
index 965088afcfad..7e90df036c48 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_TRANSPARENT) {
+		/* 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_TRANSPARENT));
+
 	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_TRANSPARENT));
+
 	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 &copy->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..0823deef8d03 100644
--- a/drivers/gpu/drm/tegra/plane.c
+++ b/drivers/gpu/drm/tegra/plane.c
@@ -465,6 +465,158 @@ static int tegra_plane_setup_transparency(struct tegra_plane *tegra,
 	return 0;
 }
 
+static u32 tegra_plane_colorkey_to_hw_format(u64 drm_ckey64)
+{
+	/* convert ARGB16161616 to ARGB8888 */
+	u8 a = drm_colorkey_extract_component(drm_ckey64, alpha, 8);
+	u8 r = drm_colorkey_extract_component(drm_ckey64, red, 8);
+	u8 g = drm_colorkey_extract_component(drm_ckey64, green, 8);
+	u8 b = drm_colorkey_extract_component(drm_ckey64, blue, 8);
+
+	return (a << 24) | (r << 16) | (g << 8) | b;
+}
+
+static bool tegra_plane_format_valid_for_colorkey(struct drm_plane_state *state)
+{
+	struct tegra_plane_state *tegra_state = to_tegra_plane_state(state);
+
+	/*
+	 * Tegra20 does not support alpha channel matching. Newer Tegra's
+	 * support the alpha matching, but it is not implemented yet.
+	 *
+	 * Formats other than XRGB8888 haven't been tested much, hence they
+	 * are not supported for now.
+	 */
+	switch (tegra_state->format) {
+	case WIN_COLOR_DEPTH_R8G8B8X8:
+	case WIN_COLOR_DEPTH_B8G8R8X8:
+		break;
+
+	default:
+		return false;
+	};
+
+	return true;
+}
+
+static int tegra_plane_setup_colorkey(struct tegra_plane *tegra,
+				      struct tegra_plane_state *tegra_state)
+{
+	enum drm_plane_colorkey_mode mode;
+	struct drm_crtc_state *crtc_state;
+	struct tegra_dc_state *dc_state;
+	struct drm_plane_state *state;
+	struct drm_plane_state *old;
+	struct drm_plane_state *new;
+	struct drm_plane *plane;
+	unsigned int normalized_zpos;
+	u32 min_hw, max_hw, mask_hw;
+	u32 plane_mask;
+	u64 min, max;
+	u64 mask;
+
+	normalized_zpos = tegra_state->base.normalized_zpos;
+	plane_mask = tegra_state->base.colorkey.plane_mask;
+	mode = tegra_state->base.colorkey.mode;
+	mask = tegra_state->base.colorkey.mask;
+	min = tegra_state->base.colorkey.min;
+	max = tegra_state->base.colorkey.max;
+
+	/* convert color key values to HW format */
+	mask_hw = tegra_plane_colorkey_to_hw_format(mask);
+	min_hw = tegra_plane_colorkey_to_hw_format(min);
+	max_hw = tegra_plane_colorkey_to_hw_format(max);
+
+	state = &tegra_state->base;
+	old = drm_atomic_get_old_plane_state(state->state, &tegra->base);
+
+	/* no need to proceed if color keying state is unchanged */
+	if (old->colorkey.plane_mask == plane_mask &&
+	    old->colorkey.mask == mask &&
+	    old->colorkey.mode == mode &&
+	    old->colorkey.min == min &&
+	    old->colorkey.max == max &&
+	    old->crtc)
+	{
+		if (mode == DRM_PLANE_COLORKEY_MODE_DISABLED)
+			return 0;
+
+		crtc_state = drm_atomic_get_crtc_state(state->state,
+						       state->crtc);
+		if (IS_ERR(crtc_state))
+			return PTR_ERR(crtc_state);
+
+		if (!crtc_state->zpos_changed) {
+			dc_state = to_dc_state(crtc_state);
+
+			if (dc_state->ckey.min == min_hw &&
+			    dc_state->ckey.max == max_hw)
+				return 0;
+		}
+	}
+
+	/*
+	 * Currently color keying is implemented for the middle plane
+	 * only (source and destination) to simplify things, validate planes
+	 * position and mask.
+	 */
+	if (state->fb && mode != DRM_PLANE_COLORKEY_MODE_DISABLED) {
+		/*
+		 * Tegra does not support color key masking, note that alpha
+		 * channel mask is ignored because only opaque formats are
+		 * currently supported.
+		 */
+		if ((mask_hw & 0xffffff) != 0xffffff)
+			return -EINVAL;
+
+		drm_for_each_plane_mask(plane, tegra->base.dev, plane_mask) {
+			struct tegra_plane *p = to_tegra_plane(plane);
+
+			/* HW can't access planes on a different CRTC */
+			if (p->dc != tegra->dc)
+				return -EINVAL;
+
+			new = drm_atomic_get_plane_state(state->state, plane);
+			if (IS_ERR(new))
+				return PTR_ERR(new);
+
+			/* don't care about disabled plane */
+			if (!new->fb)
+				continue;
+
+			if (!tegra_plane_format_valid_for_colorkey(new))
+				return -EINVAL;
+
+			/* middle plane sourcing itself */
+			if (new->normalized_zpos == 1 &&
+			    normalized_zpos == 1)
+				continue;
+
+			return -EINVAL;
+		}
+	}
+
+	/* only middle plane affects the color key state, see comment above */
+	if (normalized_zpos != 1)
+		return 0;
+
+	/*
+	 * Tegra's HW has color key values stored 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_hw;
+	dc_state->ckey.max = max_hw;
+
+	return 0;
+}
+
 int tegra_plane_setup_legacy_state(struct tegra_plane *tegra,
 				   struct tegra_plane_state *state)
 {
@@ -478,5 +630,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;
 }
-- 
2.18.0