[RFC,v2,1/2] drm: Add generic colorkey properties

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 nine generic DRM plane
properties related to the color keying to cover various HW capabilities.

This patch is based on the initial work done by Laurent Pinchart, most of
credits for this patch goes to him.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/gpu/drm/drm_atomic.c |  36 ++++++
 drivers/gpu/drm/drm_blend.c  | 229 +++++++++++++++++++++++++++++++++++
 include/drm/drm_blend.h      |   3 +
 include/drm/drm_plane.h      |  77 ++++++++++++
 4 files changed, 345 insertions(+)

Hi Dimitri,

Thank you for the patch.

I'll review this in details, but as this patch is based on the "[PATCH/RFC 
1/4] drm: Add colorkey properties" patch I've submitted, please retain the 
authorship, both in the Signed-off-by line, and in the patch author in git.

On Saturday, 26 May 2018 18:56:22 EEST Dmitry Osipenko wrote:
> 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 nine generic DRM plane
> properties related to the color keying to cover various HW capabilities.
> 
> This patch is based on the initial work done by Laurent Pinchart, most of
> credits for this patch goes to him.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/gpu/drm/drm_atomic.c |  36 ++++++
>  drivers/gpu/drm/drm_blend.c  | 229 +++++++++++++++++++++++++++++++++++
>  include/drm/drm_blend.h      |   3 +
>  include/drm/drm_plane.h      |  77 ++++++++++++
>  4 files changed, 345 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 895741e9cd7d..5b808cb68654 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -799,6 +799,24 @@ 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->colorkey.format_property) {
> +		state->colorkey.format = val;
> +	} else if (property == plane->colorkey.mask_property) {
> +		state->colorkey.mask = val;
> +	} else if (property == plane->colorkey.inverted_match_property) {
> +		state->colorkey.inverted_match = val;
> +	} else if (property == plane->colorkey.replacement_mask_property) {
> +		state->colorkey.replacement_mask = val;
> +	} else if (property == plane->colorkey.replacement_value_property) {
> +		state->colorkey.replacement_value = val;
> +	} else if (property == plane->colorkey.replacement_format_property) {
> +		state->colorkey.replacement_format = val;
>  	} else if (property == plane->color_encoding_property) {
>  		state->color_encoding = val;
>  	} else if (property == plane->color_range_property) {
> @@ -864,6 +882,24 @@ 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->colorkey.format_property) {
> +		*val = state->colorkey.format;
> +	} else if (property == plane->colorkey.mask_property) {
> +		*val = state->colorkey.mask;
> +	} else if (property == plane->colorkey.inverted_match_property) {
> +		*val = state->colorkey.inverted_match;
> +	} else if (property == plane->colorkey.replacement_mask_property) {
> +		*val = state->colorkey.replacement_mask;
> +	} else if (property == plane->colorkey.replacement_value_property) {
> +		*val = state->colorkey.replacement_value;
> +	} else if (property == plane->colorkey.replacement_format_property) {
> +		*val = state->colorkey.replacement_format;
>  	} 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..05e5632ce375 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,227 @@ 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_SRC] = "src-match-src-replace",
> +	[DRM_PLANE_COLORKEY_MODE_DST] = "dst-match-src-replace",
> +};
> +
> +/**
> + * 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 through nine properties:
> + *
> + * colorkey.mode:
> + *	The mode is an enumerated property that controls how color keying
> + *	operates. The "disabled" mode that disables color keying and is
> + *	very likely to exist if color keying is supported, it should be the
> + *	default mode.
> + *
> + * 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 AXYZ16161616 format, where A is the alpha value and
> + *	X, Y and Z correspond to the color components of the colorkey.format.
> + *	In most cases XYZ will be either RGB or YUV.
> + *
> + *	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.
> + *
> + * colorkey.format:
> + *	This property specifies the pixel format for the colorkey.min / max
> + *	properties. The format is given in a form of DRM fourcc code.
> + *
> + *	Drivers return an error from their plane atomic check if pixel format
> + *	is unsupported.
> + *
> + * 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
> + *	form of DRM fourcc code corresponding to the colorkey.format property.
> + *
> + *	For example: userspace shall set the colorkey.mask to 0x0000ff00
> + *	to match only the green component if colorkey.format is set to
> + *	DRM_FORMAT_XRGB8888.
> + *
> + *	Drivers return an error from their plane atomic check if mask value
> + *	can't be handled.
> + *
> + * colorkey.inverted-match:
> + *	This property specifies whether the matching min-max range should
> + *	be inverted, i.e. pixels outside of the given color range become
> + *	the color key match.
> + *
> + *	Drivers return an error from their plane atomic check if inversion
> + *	mode can't be handled.
> + *
> + * colorkey.replacement-value:
> + *	This property specifies the color value that replaces pixels matching
> + *	the color key. The value is expressed in AXYZ16161616 format, where A
> + *	is the alpha value and X, Y and Z correspond to the color components
> + *	of the colorkey.replacement-format.
> + *
> + *	Drivers return an error from their plane atomic check if replacement
> + *	value can't be handled.
> + *
> + * colorkey.replacement-format:
> + *	This property specifies the pixel format for the
> + *	colorkey.replacement-value property. The format is given in a form of
> + *	DRM fourcc code.
> + *
> + *	Drivers return an error from their plane atomic check if replacement
> + *	pixel format is unsupported.
> + *
> + * colorkey.replacement-mask:
> + *	This property specifies the pixel components mask that defines
> + *	what components of the colorkey.replacement-value will participate in
> + *	replacement of the pixels color. Unmasked pixel components are not
> + *	participating in the replacement. The mask value is given in a form of
> + *	DRM fourcc code corresponding to the colorkey.replacement-format
> + *	property.
> + *
> + *	For example: userspace shall set the colorkey.replacement-mask to
> + *	0x0000ff00 to replace only the green component if
> + *	colorkey.replacement-format is set to DRM_FORMAT_XRGB8888.
> + *
> + *	Userspace shall set colorkey.replacement-mask to 0 to disable the color
> + *	replacement. In this case matching pixels become transparent.
> + *
> + *	Drivers return an error from their plane atomic check if replacement
> + *	mask value 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 *replacement_format_prop;
> +	struct drm_property *replacement_value_prop;
> +	struct drm_property *replacement_mask_prop;
> +	struct drm_property *inverted_match_prop;
> +	struct drm_property *format_prop;
> +	struct drm_property *mask_prop;
> +	struct drm_property *mode_prop;
> +	struct drm_property *min_prop;
> +	struct drm_property *max_prop;
> +	unsigned int modes_num = 0;
> +	unsigned int i;
> +
> +	/* at least two modes should be supported */
> +	if (!supported_modes)
> +		return -EINVAL;
> +
> +	/* 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++;
> +	}
> +
> +	mode_prop = drm_property_create_enum(plane->dev, 0, "colorkey.mode",
> +					     modes_list, modes_num);
> +	if (!mode_prop)
> +		return -ENOMEM;
> +
> +	mask_prop = drm_property_create_range(plane->dev, 0, "colorkey.mask",
> +					      0, U64_MAX);
> +	if (!mask_prop)
> +		goto err_destroy_mode_prop;
> +
> +	min_prop = drm_property_create_range(plane->dev, 0, "colorkey.min",
> +					     0, U64_MAX);
> +	if (!min_prop)
> +		goto err_destroy_mask_prop;
> +
> +	max_prop = drm_property_create_range(plane->dev, 0, "colorkey.max",
> +					     0, U64_MAX);
> +	if (!max_prop)
> +		goto err_destroy_min_prop;
> +
> +	format_prop = drm_property_create_range(plane->dev, 0,
> +					"colorkey.format",
> +					0, U32_MAX);
> +	if (!format_prop)
> +		goto err_destroy_max_prop;
> +
> +	inverted_match_prop = drm_property_create_bool(plane->dev, 0,
> +					"colorkey.inverted-match");
> +	if (!inverted_match_prop)
> +		goto err_destroy_format_prop;
> +
> +	replacement_mask_prop = drm_property_create_range(plane->dev, 0,
> +					"colorkey.replacement-mask",
> +					0, U64_MAX);
> +	if (!replacement_mask_prop)
> +		goto err_destroy_inverted_match_prop;
> +
> +	replacement_value_prop = drm_property_create_range(plane->dev, 0,
> +					"colorkey.replacement-value",
> +					0, U64_MAX);
> +	if (!replacement_value_prop)
> +		goto err_destroy_replacement_mask_prop;
> +
> +	replacement_format_prop = drm_property_create_range(plane->dev, 0,
> +					"colorkey.replacement-format",
> +					0, U64_MAX);
> +	if (!replacement_format_prop)
> +		goto err_destroy_replacement_value_prop;
> +
> +	drm_object_attach_property(&plane->base, min_prop, 0);
> +	drm_object_attach_property(&plane->base, max_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, format_prop, 0);
> +	drm_object_attach_property(&plane->base, inverted_match_prop, 0);
> +	drm_object_attach_property(&plane->base, replacement_mask_prop, 0);
> +	drm_object_attach_property(&plane->base, replacement_value_prop, 0);
> +	drm_object_attach_property(&plane->base, replacement_format_prop, 0);
> +
> +	plane->colorkey.min_property = min_prop;
> +	plane->colorkey.max_property = max_prop;
> +	plane->colorkey.mode_property = mode_prop;
> +	plane->colorkey.mask_property = mask_prop;
> +	plane->colorkey.format_property = format_prop;
> +	plane->colorkey.inverted_match_property = inverted_match_prop;
> +	plane->colorkey.replacement_mask_property = replacement_mask_prop;
> +	plane->colorkey.replacement_value_property = replacement_value_prop;
> +	plane->colorkey.replacement_format_property = replacement_format_prop;
> +
> +	return 0;
> +
> +err_destroy_replacement_value_prop:
> +	drm_property_destroy(plane->dev, replacement_value_prop);
> +err_destroy_replacement_mask_prop:
> +	drm_property_destroy(plane->dev, replacement_mask_prop);
> +err_destroy_inverted_match_prop:
> +	drm_property_destroy(plane->dev, inverted_match_prop);
> +err_destroy_format_prop:
> +	drm_property_destroy(plane->dev, format_prop);
> +err_destroy_max_prop:
> +	drm_property_destroy(plane->dev, max_prop);
> +err_destroy_min_prop:
> +	drm_property_destroy(plane->dev, min_prop);
> +err_destroy_mask_prop:
> +	drm_property_destroy(plane->dev, mask_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..ff7f5ebe2b79 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -32,6 +32,42 @@ 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. This is the default
> +	 * common mode.
> +	 */
> +	DRM_PLANE_COLORKEY_MODE_DISABLED,
> +
> +	/**
> +	 * @DRM_PLANE_COLORKEY_MODE_SRC:
> +	 *
> +	 * In this mode color matching is performed with the pixels of
> +	 * the given plane and the matched pixels are fully (or partially)
> +	 * replaced with the replacement color or become completely
> +	 * transparent.
> +	 */
> +	DRM_PLANE_COLORKEY_MODE_SRC,
> +
> +	/**
> +	 * @DRM_PLANE_COLORKEY_MODE_DST:
> +	 *
> +	 * In this mode color matching is performed with the pixels of the
> +	 * planes z-positioned under the given plane and the pixels of the
> +	 * hovering plane that are xy-positioned as the underlying
> +	 * color-matched pixels are fully (or partially) replaced with the
> +	 * replacement color or become completely transparent.
> +	 */
> +	DRM_PLANE_COLORKEY_MODE_DST,
> +
> +	DRM_PLANE_COLORKEY_MODES_NUM,
> +};
> +
>  /**
>   * struct drm_plane_state - mutable plane state
>   * @plane: backpointer to the plane
> @@ -54,6 +90,21 @@ 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.mode: color key mode
> + * @colorkey.min: color key range minimum. The value is stored in
> AXYZ16161616 + *	format, where A is the alpha value and X, Y and Z
> correspond to the + *	color components of the plane's pixel format (usually
> RGB or YUV) + * @colorkey.max: color key range maximum (in AXYZ16161616
> format) + * @colorkey.mask: color key mask value (in AXYZ16161616 format)
> + * @colorkey.format: color key min/max/mask values pixel format (in
> + * 	DRM_FORMAT_AXYZ16161616 form)
> + * @colorkey.inverted_match: color key min-max matching range is inverted
> + * @colorkey.replacement_mask: color key replacement mask value (in
> + * 	AXYZ16161616 format)
> + * @colorkey.replacement_value: color key replacement value (in
> + * 	AXYZ16161616 format)
> + * @colorkey.replacement_format: color key replacement value / mask
> + *	pixel format (in DRM_FORMAT_AXYZ16161616 form)
>   * @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 +175,19 @@ struct drm_plane_state {
>  	unsigned int zpos;
>  	unsigned int normalized_zpos;
> 
> +	/* Plane colorkey */
> +	struct {
> +		enum drm_plane_colorkey_mode mode;
> +		u64 min;
> +		u64 max;
> +		u64 mask;
> +		u32 format;
> +		bool inverted_match;
> +		u64 replacement_mask;
> +		u64 replacement_value;
> +		u32 replacement_format;
> +	} colorkey;
> +
>  	/**
>  	 * @color_encoding:
>  	 *
> @@ -510,6 +574,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 +652,18 @@ struct drm_plane {
>  	struct drm_property *zpos_property;
>  	struct drm_property *rotation_property;
> 
> +	struct {
> +		struct drm_property *min_property;
> +		struct drm_property *max_property;
> +		struct drm_property *mode_property;
> +		struct drm_property *mask_property;
> +		struct drm_property *format_property;
> +		struct drm_property *inverted_match_property;
> +		struct drm_property *replacement_mask_property;
> +		struct drm_property *replacement_value_property;
> +		struct drm_property *replacement_format_property;
> +	} colorkey;
> +
>  	/**
>  	 * @color_encoding_property:
>  	 *

On Saturday, 26 May 2018 19:16:54 EEST Laurent Pinchart wrote:
> Hi Dimitri,

And sorry for the spelling mistake :-/

> Thank you for the patch.
> 
> I'll review this in details, but as this patch is based on the "[PATCH/RFC
> 1/4] drm: Add colorkey properties" patch I've submitted, please retain the
> authorship, both in the Signed-off-by line, and in the patch author in git.
> 
> On Saturday, 26 May 2018 18:56:22 EEST Dmitry Osipenko wrote:
> > 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 nine generic DRM plane
> > properties related to the color keying to cover various HW capabilities.
> > 
> > This patch is based on the initial work done by Laurent Pinchart, most of
> > credits for this patch goes to him.
> > 
> > Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> > ---
> > 
> >  drivers/gpu/drm/drm_atomic.c |  36 ++++++
> >  drivers/gpu/drm/drm_blend.c  | 229 +++++++++++++++++++++++++++++++++++
> >  include/drm/drm_blend.h      |   3 +
> >  include/drm/drm_plane.h      |  77 ++++++++++++
> >  4 files changed, 345 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> > index 895741e9cd7d..5b808cb68654 100644
> > --- a/drivers/gpu/drm/drm_atomic.c
> > +++ b/drivers/gpu/drm/drm_atomic.c
> > @@ -799,6 +799,24 @@ 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->colorkey.format_property) {
> > +		state->colorkey.format = val;
> > +	} else if (property == plane->colorkey.mask_property) {
> > +		state->colorkey.mask = val;
> > +	} else if (property == plane->colorkey.inverted_match_property) {
> > +		state->colorkey.inverted_match = val;
> > +	} else if (property == plane->colorkey.replacement_mask_property) {
> > +		state->colorkey.replacement_mask = val;
> > +	} else if (property == plane->colorkey.replacement_value_property) {
> > +		state->colorkey.replacement_value = val;
> > +	} else if (property == plane->colorkey.replacement_format_property) {
> > +		state->colorkey.replacement_format = val;
> > 
> >  	} else if (property == plane->color_encoding_property) {
> >  	
> >  		state->color_encoding = val;
> >  	
> >  	} else if (property == plane->color_range_property) {
> > 
> > @@ -864,6 +882,24 @@ 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->colorkey.format_property) {
> > +		*val = state->colorkey.format;
> > +	} else if (property == plane->colorkey.mask_property) {
> > +		*val = state->colorkey.mask;
> > +	} else if (property == plane->colorkey.inverted_match_property) {
> > +		*val = state->colorkey.inverted_match;
> > +	} else if (property == plane->colorkey.replacement_mask_property) {
> > +		*val = state->colorkey.replacement_mask;
> > +	} else if (property == plane->colorkey.replacement_value_property) {
> > +		*val = state->colorkey.replacement_value;
> > +	} else if (property == plane->colorkey.replacement_format_property) {
> > +		*val = state->colorkey.replacement_format;
> > 
> >  	} 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..05e5632ce375 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,227 @@ 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_SRC] = "src-match-src-replace",
> > +	[DRM_PLANE_COLORKEY_MODE_DST] = "dst-match-src-replace",
> > +};
> > +
> > +/**
> > + * 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 through nine properties:
> > + *
> > + * colorkey.mode:
> > + *	The mode is an enumerated property that controls how color keying
> > + *	operates. The "disabled" mode that disables color keying and is
> > + *	very likely to exist if color keying is supported, it should be the
> > + *	default mode.
> > + *
> > + * 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 AXYZ16161616 format, where A is the alpha value 
and
> > + *	X, Y and Z correspond to the color components of the 
colorkey.format.
> > + *	In most cases XYZ will be either RGB or YUV.
> > + *
> > + *	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.
> > + *
> > + * colorkey.format:
> > + *	This property specifies the pixel format for the colorkey.min / max
> > + *	properties. The format is given in a form of DRM fourcc code.
> > + *
> > + *	Drivers return an error from their plane atomic check if pixel 
format
> > + *	is unsupported.
> > + *
> > + * 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
> > + *	form of DRM fourcc code corresponding to the colorkey.format 
property.
> > + *
> > + *	For example: userspace shall set the colorkey.mask to 0x0000ff00
> > + *	to match only the green component if colorkey.format is set to
> > + *	DRM_FORMAT_XRGB8888.
> > + *
> > + *	Drivers return an error from their plane atomic check if mask value
> > + *	can't be handled.
> > + *
> > + * colorkey.inverted-match:
> > + *	This property specifies whether the matching min-max range should
> > + *	be inverted, i.e. pixels outside of the given color range become
> > + *	the color key match.
> > + *
> > + *	Drivers return an error from their plane atomic check if inversion
> > + *	mode can't be handled.
> > + *
> > + * colorkey.replacement-value:
> > + *	This property specifies the color value that replaces pixels 
matching
> > + *	the color key. The value is expressed in AXYZ16161616 format, where 
A
> > + *	is the alpha value and X, Y and Z correspond to the color components
> > + *	of the colorkey.replacement-format.
> > + *
> > + *	Drivers return an error from their plane atomic check if replacement
> > + *	value can't be handled.
> > + *
> > + * colorkey.replacement-format:
> > + *	This property specifies the pixel format for the
> > + *	colorkey.replacement-value property. The format is given in a form 
of
> > + *	DRM fourcc code.
> > + *
> > + *	Drivers return an error from their plane atomic check if replacement
> > + *	pixel format is unsupported.
> > + *
> > + * colorkey.replacement-mask:
> > + *	This property specifies the pixel components mask that defines
> > + *	what components of the colorkey.replacement-value will participate 
in
> > + *	replacement of the pixels color. Unmasked pixel components are not
> > + *	participating in the replacement. The mask value is given in a form 
of
> > + *	DRM fourcc code corresponding to the colorkey.replacement-format
> > + *	property.
> > + *
> > + *	For example: userspace shall set the colorkey.replacement-mask to
> > + *	0x0000ff00 to replace only the green component if
> > + *	colorkey.replacement-format is set to DRM_FORMAT_XRGB8888.
> > + *
> > + *	Userspace shall set colorkey.replacement-mask to 0 to disable the
> > color
> > + *	replacement. In this case matching pixels become transparent.
> > + *
> > + *	Drivers return an error from their plane atomic check if replacement
> > + *	mask value 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 *replacement_format_prop;
> > +	struct drm_property *replacement_value_prop;
> > +	struct drm_property *replacement_mask_prop;
> > +	struct drm_property *inverted_match_prop;
> > +	struct drm_property *format_prop;
> > +	struct drm_property *mask_prop;
> > +	struct drm_property *mode_prop;
> > +	struct drm_property *min_prop;
> > +	struct drm_property *max_prop;
> > +	unsigned int modes_num = 0;
> > +	unsigned int i;
> > +
> > +	/* at least two modes should be supported */
> > +	if (!supported_modes)
> > +		return -EINVAL;
> > +
> > +	/* 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++;
> > +	}
> > +
> > +	mode_prop = drm_property_create_enum(plane->dev, 0, "colorkey.mode",
> > +					     modes_list, modes_num);
> > +	if (!mode_prop)
> > +		return -ENOMEM;
> > +
> > +	mask_prop = drm_property_create_range(plane->dev, 0, "colorkey.mask",
> > +					      0, U64_MAX);
> > +	if (!mask_prop)
> > +		goto err_destroy_mode_prop;
> > +
> > +	min_prop = drm_property_create_range(plane->dev, 0, "colorkey.min",
> > +					     0, U64_MAX);
> > +	if (!min_prop)
> > +		goto err_destroy_mask_prop;
> > +
> > +	max_prop = drm_property_create_range(plane->dev, 0, "colorkey.max",
> > +					     0, U64_MAX);
> > +	if (!max_prop)
> > +		goto err_destroy_min_prop;
> > +
> > +	format_prop = drm_property_create_range(plane->dev, 0,
> > +					"colorkey.format",
> > +					0, U32_MAX);
> > +	if (!format_prop)
> > +		goto err_destroy_max_prop;
> > +
> > +	inverted_match_prop = drm_property_create_bool(plane->dev, 0,
> > +					"colorkey.inverted-match");
> > +	if (!inverted_match_prop)
> > +		goto err_destroy_format_prop;
> > +
> > +	replacement_mask_prop = drm_property_create_range(plane->dev, 0,
> > +					"colorkey.replacement-mask",
> > +					0, U64_MAX);
> > +	if (!replacement_mask_prop)
> > +		goto err_destroy_inverted_match_prop;
> > +
> > +	replacement_value_prop = drm_property_create_range(plane->dev, 0,
> > +					"colorkey.replacement-value",
> > +					0, U64_MAX);
> > +	if (!replacement_value_prop)
> > +		goto err_destroy_replacement_mask_prop;
> > +
> > +	replacement_format_prop = drm_property_create_range(plane->dev, 0,
> > +					"colorkey.replacement-format",
> > +					0, U64_MAX);
> > +	if (!replacement_format_prop)
> > +		goto err_destroy_replacement_value_prop;
> > +
> > +	drm_object_attach_property(&plane->base, min_prop, 0);
> > +	drm_object_attach_property(&plane->base, max_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, format_prop, 0);
> > +	drm_object_attach_property(&plane->base, inverted_match_prop, 0);
> > +	drm_object_attach_property(&plane->base, replacement_mask_prop, 0);
> > +	drm_object_attach_property(&plane->base, replacement_value_prop, 0);
> > +	drm_object_attach_property(&plane->base, replacement_format_prop, 0);
> > +
> > +	plane->colorkey.min_property = min_prop;
> > +	plane->colorkey.max_property = max_prop;
> > +	plane->colorkey.mode_property = mode_prop;
> > +	plane->colorkey.mask_property = mask_prop;
> > +	plane->colorkey.format_property = format_prop;
> > +	plane->colorkey.inverted_match_property = inverted_match_prop;
> > +	plane->colorkey.replacement_mask_property = replacement_mask_prop;
> > +	plane->colorkey.replacement_value_property = replacement_value_prop;
> > +	plane->colorkey.replacement_format_property = replacement_format_prop;
> > +
> > +	return 0;
> > +
> > +err_destroy_replacement_value_prop:
> > +	drm_property_destroy(plane->dev, replacement_value_prop);
> > +err_destroy_replacement_mask_prop:
> > +	drm_property_destroy(plane->dev, replacement_mask_prop);
> > +err_destroy_inverted_match_prop:
> > +	drm_property_destroy(plane->dev, inverted_match_prop);
> > +err_destroy_format_prop:
> > +	drm_property_destroy(plane->dev, format_prop);
> > +err_destroy_max_prop:
> > +	drm_property_destroy(plane->dev, max_prop);
> > +err_destroy_min_prop:
> > +	drm_property_destroy(plane->dev, min_prop);
> > +err_destroy_mask_prop:
> > +	drm_property_destroy(plane->dev, mask_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..ff7f5ebe2b79 100644
> > --- a/include/drm/drm_plane.h
> > +++ b/include/drm/drm_plane.h
> > @@ -32,6 +32,42 @@ 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. This is the default
> > +	 * common mode.
> > +	 */
> > +	DRM_PLANE_COLORKEY_MODE_DISABLED,
> > +
> > +	/**
> > +	 * @DRM_PLANE_COLORKEY_MODE_SRC:
> > +	 *
> > +	 * In this mode color matching is performed with the pixels of
> > +	 * the given plane and the matched pixels are fully (or partially)
> > +	 * replaced with the replacement color or become completely
> > +	 * transparent.
> > +	 */
> > +	DRM_PLANE_COLORKEY_MODE_SRC,
> > +
> > +	/**
> > +	 * @DRM_PLANE_COLORKEY_MODE_DST:
> > +	 *
> > +	 * In this mode color matching is performed with the pixels of the
> > +	 * planes z-positioned under the given plane and the pixels of the
> > +	 * hovering plane that are xy-positioned as the underlying
> > +	 * color-matched pixels are fully (or partially) replaced with the
> > +	 * replacement color or become completely transparent.
> > +	 */
> > +	DRM_PLANE_COLORKEY_MODE_DST,
> > +
> > +	DRM_PLANE_COLORKEY_MODES_NUM,
> > +};
> > +
> > 
> >  /**
> >  
> >   * struct drm_plane_state - mutable plane state
> >   * @plane: backpointer to the plane
> > 
> > @@ -54,6 +90,21 @@ 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.mode: color key mode
> > + * @colorkey.min: color key range minimum. The value is stored in
> > AXYZ16161616 + *	format, where A is the alpha value and X, Y and Z
> > correspond to the + *	color components of the plane's pixel format
> > (usually
> > RGB or YUV) + * @colorkey.max: color key range maximum (in AXYZ16161616
> > format) + * @colorkey.mask: color key mask value (in AXYZ16161616 format)
> > + * @colorkey.format: color key min/max/mask values pixel format (in
> > + * 	DRM_FORMAT_AXYZ16161616 form)
> > + * @colorkey.inverted_match: color key min-max matching range is inverted
> > + * @colorkey.replacement_mask: color key replacement mask value (in
> > + * 	AXYZ16161616 format)
> > + * @colorkey.replacement_value: color key replacement value (in
> > + * 	AXYZ16161616 format)
> > + * @colorkey.replacement_format: color key replacement value / mask
> > + *	pixel format (in DRM_FORMAT_AXYZ16161616 form)
> > 
> >   * @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 +175,19 @@ struct drm_plane_state {
> > 
> >  	unsigned int zpos;
> >  	unsigned int normalized_zpos;
> > 
> > +	/* Plane colorkey */
> > +	struct {
> > +		enum drm_plane_colorkey_mode mode;
> > +		u64 min;
> > +		u64 max;
> > +		u64 mask;
> > +		u32 format;
> > +		bool inverted_match;
> > +		u64 replacement_mask;
> > +		u64 replacement_value;
> > +		u32 replacement_format;
> > +	} colorkey;
> > +
> > 
> >  	/**
> >  	
> >  	 * @color_encoding:
> >  	 *
> > 
> > @@ -510,6 +574,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 +652,18 @@ struct drm_plane {
> > 
> >  	struct drm_property *zpos_property;
> >  	struct drm_property *rotation_property;
> > 
> > +	struct {
> > +		struct drm_property *min_property;
> > +		struct drm_property *max_property;
> > +		struct drm_property *mode_property;
> > +		struct drm_property *mask_property;
> > +		struct drm_property *format_property;
> > +		struct drm_property *inverted_match_property;
> > +		struct drm_property *replacement_mask_property;
> > +		struct drm_property *replacement_value_property;
> > +		struct drm_property *replacement_format_property;
> > +	} colorkey;
> > +
> > 
> >  	/**
> >  	
> >  	 * @color_encoding_property:
> >  	 *

On 26.05.2018 19:16, Laurent Pinchart wrote:
> Hi Dimitri,
> 
> Thank you for the patch.
> 
> I'll review this in details, but as this patch is based on the "[PATCH/RFC 
> 1/4] drm: Add colorkey properties" patch I've submitted, please retain the 
> authorship, both in the Signed-off-by line, and in the patch author in git.
Okay. /I think/ I've seen requests to do the other way around for the picked up
and re-worked patches, though I don't mind at all to keep your authorship. I'll
change the authorship in the next iteration. Waiting for you review comments,
thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 26.05.2018 19:18, Laurent Pinchart wrote:
> On Saturday, 26 May 2018 19:16:54 EEST Laurent Pinchart wrote:
>> Hi Dimitri,
> 
> And sorry for the spelling mistake :-/

That's also a kinda correct spelling. No worries ;)
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Sat, May 26, 2018 at 06:56:22PM +0300, Dmitry Osipenko wrote:
> 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 nine generic DRM plane
> properties related to the color keying to cover various HW capabilities.
> 
> This patch is based on the initial work done by Laurent Pinchart, most of
> credits for this patch goes to him.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/gpu/drm/drm_atomic.c |  36 ++++++
>  drivers/gpu/drm/drm_blend.c  | 229 +++++++++++++++++++++++++++++++++++
>  include/drm/drm_blend.h      |   3 +
>  include/drm/drm_plane.h      |  77 ++++++++++++
>  4 files changed, 345 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 895741e9cd7d..5b808cb68654 100644
> --- a/drivers/gpu/drm/drm_atomic.c
> +++ b/drivers/gpu/drm/drm_atomic.c
> @@ -799,6 +799,24 @@ 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->colorkey.format_property) {
> +		state->colorkey.format = val;
> +	} else if (property == plane->colorkey.mask_property) {
> +		state->colorkey.mask = val;
> +	} else if (property == plane->colorkey.inverted_match_property) {
> +		state->colorkey.inverted_match = val;
> +	} else if (property == plane->colorkey.replacement_mask_property) {
> +		state->colorkey.replacement_mask = val;
> +	} else if (property == plane->colorkey.replacement_value_property) {
> +		state->colorkey.replacement_value = val;
> +	} else if (property == plane->colorkey.replacement_format_property) {
> +		state->colorkey.replacement_format = val;
>  	} else if (property == plane->color_encoding_property) {
>  		state->color_encoding = val;
>  	} else if (property == plane->color_range_property) {
> @@ -864,6 +882,24 @@ 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->colorkey.format_property) {
> +		*val = state->colorkey.format;
> +	} else if (property == plane->colorkey.mask_property) {
> +		*val = state->colorkey.mask;
> +	} else if (property == plane->colorkey.inverted_match_property) {
> +		*val = state->colorkey.inverted_match;
> +	} else if (property == plane->colorkey.replacement_mask_property) {
> +		*val = state->colorkey.replacement_mask;
> +	} else if (property == plane->colorkey.replacement_value_property) {
> +		*val = state->colorkey.replacement_value;
> +	} else if (property == plane->colorkey.replacement_format_property) {
> +		*val = state->colorkey.replacement_format;
>  	} 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..05e5632ce375 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,227 @@ 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_SRC] = "src-match-src-replace",
> +	[DRM_PLANE_COLORKEY_MODE_DST] = "dst-match-src-replace",

This list seems way more limited than I was expecting, at least when
compared to all the different props you're proposing to add.

> +};
> +
> +/**
> + * 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 through nine properties:
> + *
> + * colorkey.mode:
> + *	The mode is an enumerated property that controls how color keying
> + *	operates. The "disabled" mode that disables color keying and is
> + *	very likely to exist if color keying is supported, it should be the
> + *	default mode.
> + *
> + * 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 AXYZ16161616 format, where A is the alpha value and
> + *	X, Y and Z correspond to the color components of the colorkey.format.
> + *	In most cases XYZ will be either RGB or YUV.
> + *
> + *	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.
> + *
> + * colorkey.format:
> + *	This property specifies the pixel format for the colorkey.min / max
> + *	properties. The format is given in a form of DRM fourcc code.

Umm. Why we do even need this? This seems incompatible with your earlier
"min/max are specified in 16bpc format" statement.

> + *
> + *	Drivers return an error from their plane atomic check if pixel format
> + *	is unsupported.
> + *
> + * 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
> + *	form of DRM fourcc code corresponding to the colorkey.format property.
> + *
> + *	For example: userspace shall set the colorkey.mask to 0x0000ff00
> + *	to match only the green component if colorkey.format is set to
> + *	DRM_FORMAT_XRGB8888.
> + *
> + *	Drivers return an error from their plane atomic check if mask value
> + *	can't be handled.
> + *
> + * colorkey.inverted-match:
> + *	This property specifies whether the matching min-max range should
> + *	be inverted, i.e. pixels outside of the given color range become
> + *	the color key match.
> + *
> + *	Drivers return an error from their plane atomic check if inversion
> + *	mode can't be handled.

Hmm. I'm trying to figure out what this means for the src vs. dst
colorkey modes. Those pretty much already have an inverted meaning when
compared to each other. So I can't figure out from these docs whether 
you're supposed to use this when you want a normal dst ckey or normal
src key semantics.

> + *
> + * colorkey.replacement-value:
> + *	This property specifies the color value that replaces pixels matching
> + *	the color key. The value is expressed in AXYZ16161616 format, where A
> + *	is the alpha value and X, Y and Z correspond to the color components
> + *	of the colorkey.replacement-format.
> + *
> + *	Drivers return an error from their plane atomic check if replacement
> + *	value can't be handled.
> + *
> + * colorkey.replacement-format:
> + *	This property specifies the pixel format for the
> + *	colorkey.replacement-value property. The format is given in a form of
> + *	DRM fourcc code.

Again this seems at odds with the 16bpc replacement-value.

> + *
> + *	Drivers return an error from their plane atomic check if replacement
> + *	pixel format is unsupported.
> + *
> + * colorkey.replacement-mask:
> + *	This property specifies the pixel components mask that defines
> + *	what components of the colorkey.replacement-value will participate in
> + *	replacement of the pixels color. Unmasked pixel components are not
> + *	participating in the replacement.

Does that mean that the data for the unmasked bits will be coming
from the source?

> The mask value is given in a form of
> + *	DRM fourcc code corresponding to the colorkey.replacement-format
> + *	property.
> + *
> + *	For example: userspace shall set the colorkey.replacement-mask to
> + *	0x0000ff00 to replace only the green component if
> + *	colorkey.replacement-format is set to DRM_FORMAT_XRGB8888.
> + *
> + *	Userspace shall set colorkey.replacement-mask to 0 to disable the color
> + *	replacement. In this case matching pixels become transparent.
> + *
> + *	Drivers return an error from their plane atomic check if replacement
> + *	mask value 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 *replacement_format_prop;
> +	struct drm_property *replacement_value_prop;
> +	struct drm_property *replacement_mask_prop;
> +	struct drm_property *inverted_match_prop;
> +	struct drm_property *format_prop;
> +	struct drm_property *mask_prop;
> +	struct drm_property *mode_prop;
> +	struct drm_property *min_prop;
> +	struct drm_property *max_prop;
> +	unsigned int modes_num = 0;
> +	unsigned int i;
> +
> +	/* at least two modes should be supported */
> +	if (!supported_modes)
> +		return -EINVAL;
> +
> +	/* 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++;
> +	}
> +
> +	mode_prop = drm_property_create_enum(plane->dev, 0, "colorkey.mode",
> +					     modes_list, modes_num);
> +	if (!mode_prop)
> +		return -ENOMEM;
> +
> +	mask_prop = drm_property_create_range(plane->dev, 0, "colorkey.mask",
> +					      0, U64_MAX);
> +	if (!mask_prop)
> +		goto err_destroy_mode_prop;
> +
> +	min_prop = drm_property_create_range(plane->dev, 0, "colorkey.min",
> +					     0, U64_MAX);
> +	if (!min_prop)
> +		goto err_destroy_mask_prop;
> +
> +	max_prop = drm_property_create_range(plane->dev, 0, "colorkey.max",
> +					     0, U64_MAX);
> +	if (!max_prop)
> +		goto err_destroy_min_prop;
> +
> +	format_prop = drm_property_create_range(plane->dev, 0,
> +					"colorkey.format",
> +					0, U32_MAX);
> +	if (!format_prop)
> +		goto err_destroy_max_prop;
> +
> +	inverted_match_prop = drm_property_create_bool(plane->dev, 0,
> +					"colorkey.inverted-match");
> +	if (!inverted_match_prop)
> +		goto err_destroy_format_prop;
> +
> +	replacement_mask_prop = drm_property_create_range(plane->dev, 0,
> +					"colorkey.replacement-mask",
> +					0, U64_MAX);
> +	if (!replacement_mask_prop)
> +		goto err_destroy_inverted_match_prop;
> +
> +	replacement_value_prop = drm_property_create_range(plane->dev, 0,
> +					"colorkey.replacement-value",
> +					0, U64_MAX);
> +	if (!replacement_value_prop)
> +		goto err_destroy_replacement_mask_prop;
> +
> +	replacement_format_prop = drm_property_create_range(plane->dev, 0,
> +					"colorkey.replacement-format",
> +					0, U64_MAX);
> +	if (!replacement_format_prop)
> +		goto err_destroy_replacement_value_prop;

I don't think we want to add all these props for every driver/hardware.
IMO the set of props we expose should depend on the supported set of
colorkeying modes.

> +
> +	drm_object_attach_property(&plane->base, min_prop, 0);
> +	drm_object_attach_property(&plane->base, max_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, format_prop, 0);
> +	drm_object_attach_property(&plane->base, inverted_match_prop, 0);
> +	drm_object_attach_property(&plane->base, replacement_mask_prop, 0);
> +	drm_object_attach_property(&plane->base, replacement_value_prop, 0);
> +	drm_object_attach_property(&plane->base, replacement_format_prop, 0);
> +
> +	plane->colorkey.min_property = min_prop;
> +	plane->colorkey.max_property = max_prop;
> +	plane->colorkey.mode_property = mode_prop;
> +	plane->colorkey.mask_property = mask_prop;
> +	plane->colorkey.format_property = format_prop;
> +	plane->colorkey.inverted_match_property = inverted_match_prop;
> +	plane->colorkey.replacement_mask_property = replacement_mask_prop;
> +	plane->colorkey.replacement_value_property = replacement_value_prop;
> +	plane->colorkey.replacement_format_property = replacement_format_prop;
> +
> +	return 0;
> +
> +err_destroy_replacement_value_prop:
> +	drm_property_destroy(plane->dev, replacement_value_prop);
> +err_destroy_replacement_mask_prop:
> +	drm_property_destroy(plane->dev, replacement_mask_prop);
> +err_destroy_inverted_match_prop:
> +	drm_property_destroy(plane->dev, inverted_match_prop);
> +err_destroy_format_prop:
> +	drm_property_destroy(plane->dev, format_prop);
> +err_destroy_max_prop:
> +	drm_property_destroy(plane->dev, max_prop);
> +err_destroy_min_prop:
> +	drm_property_destroy(plane->dev, min_prop);
> +err_destroy_mask_prop:
> +	drm_property_destroy(plane->dev, mask_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..ff7f5ebe2b79 100644
> --- a/include/drm/drm_plane.h
> +++ b/include/drm/drm_plane.h
> @@ -32,6 +32,42 @@ 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. This is the default
> +	 * common mode.
> +	 */
> +	DRM_PLANE_COLORKEY_MODE_DISABLED,
> +
> +	/**
> +	 * @DRM_PLANE_COLORKEY_MODE_SRC:
> +	 *
> +	 * In this mode color matching is performed with the pixels of
> +	 * the given plane and the matched pixels are fully (or partially)
> +	 * replaced with the replacement color or become completely
> +	 * transparent.
> +	 */
> +	DRM_PLANE_COLORKEY_MODE_SRC,
> +
> +	/**
> +	 * @DRM_PLANE_COLORKEY_MODE_DST:
> +	 *
> +	 * In this mode color matching is performed with the pixels of the
> +	 * planes z-positioned under the given plane and the pixels of the
> +	 * hovering plane that are xy-positioned as the underlying
> +	 * color-matched pixels are fully (or partially) replaced with the
> +	 * replacement color or become completely transparent.
> +	 */
> +	DRM_PLANE_COLORKEY_MODE_DST,
> +
> +	DRM_PLANE_COLORKEY_MODES_NUM,
> +};
> +
>  /**
>   * struct drm_plane_state - mutable plane state
>   * @plane: backpointer to the plane
> @@ -54,6 +90,21 @@ 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.mode: color key mode
> + * @colorkey.min: color key range minimum. The value is stored in AXYZ16161616
> + *	format, where A is the alpha value and X, Y and Z correspond to the
> + *	color components of the plane's pixel format (usually RGB or YUV)
> + * @colorkey.max: color key range maximum (in AXYZ16161616 format)
> + * @colorkey.mask: color key mask value (in AXYZ16161616 format)
> + * @colorkey.format: color key min/max/mask values pixel format (in
> + * 	DRM_FORMAT_AXYZ16161616 form)
> + * @colorkey.inverted_match: color key min-max matching range is inverted
> + * @colorkey.replacement_mask: color key replacement mask value (in
> + * 	AXYZ16161616 format)
> + * @colorkey.replacement_value: color key replacement value (in
> + * 	AXYZ16161616 format)
> + * @colorkey.replacement_format: color key replacement value / mask
> + *	pixel format (in DRM_FORMAT_AXYZ16161616 form)
>   * @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 +175,19 @@ struct drm_plane_state {
>  	unsigned int zpos;
>  	unsigned int normalized_zpos;
>  
> +	/* Plane colorkey */
> +	struct {
> +		enum drm_plane_colorkey_mode mode;
> +		u64 min;
> +		u64 max;
> +		u64 mask;
> +		u32 format;
> +		bool inverted_match;
> +		u64 replacement_mask;
> +		u64 replacement_value;
> +		u32 replacement_format;
> +	} colorkey;
> +
>  	/**
>  	 * @color_encoding:
>  	 *
> @@ -510,6 +574,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 +652,18 @@ struct drm_plane {
>  	struct drm_property *zpos_property;
>  	struct drm_property *rotation_property;
>  
> +	struct {
> +		struct drm_property *min_property;
> +		struct drm_property *max_property;
> +		struct drm_property *mode_property;
> +		struct drm_property *mask_property;
> +		struct drm_property *format_property;
> +		struct drm_property *inverted_match_property;
> +		struct drm_property *replacement_mask_property;
> +		struct drm_property *replacement_value_property;
> +		struct drm_property *replacement_format_property;
> +	} colorkey;
> +
>  	/**
>  	 * @color_encoding_property:
>  	 *
> -- 
> 2.17.0

On 28.05.2018 16:15, Ville Syrjälä wrote:
> On Sat, May 26, 2018 at 06:56:22PM +0300, Dmitry Osipenko wrote:
>> 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 nine generic DRM plane
>> properties related to the color keying to cover various HW capabilities.
>>
>> This patch is based on the initial work done by Laurent Pinchart, most of
>> credits for this patch goes to him.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/gpu/drm/drm_atomic.c |  36 ++++++
>>  drivers/gpu/drm/drm_blend.c  | 229 +++++++++++++++++++++++++++++++++++
>>  include/drm/drm_blend.h      |   3 +
>>  include/drm/drm_plane.h      |  77 ++++++++++++
>>  4 files changed, 345 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 895741e9cd7d..5b808cb68654 100644
>> --- a/drivers/gpu/drm/drm_atomic.c
>> +++ b/drivers/gpu/drm/drm_atomic.c
>> @@ -799,6 +799,24 @@ 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->colorkey.format_property) {
>> +		state->colorkey.format = val;
>> +	} else if (property == plane->colorkey.mask_property) {
>> +		state->colorkey.mask = val;
>> +	} else if (property == plane->colorkey.inverted_match_property) {
>> +		state->colorkey.inverted_match = val;
>> +	} else if (property == plane->colorkey.replacement_mask_property) {
>> +		state->colorkey.replacement_mask = val;
>> +	} else if (property == plane->colorkey.replacement_value_property) {
>> +		state->colorkey.replacement_value = val;
>> +	} else if (property == plane->colorkey.replacement_format_property) {
>> +		state->colorkey.replacement_format = val;
>>  	} else if (property == plane->color_encoding_property) {
>>  		state->color_encoding = val;
>>  	} else if (property == plane->color_range_property) {
>> @@ -864,6 +882,24 @@ 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->colorkey.format_property) {
>> +		*val = state->colorkey.format;
>> +	} else if (property == plane->colorkey.mask_property) {
>> +		*val = state->colorkey.mask;
>> +	} else if (property == plane->colorkey.inverted_match_property) {
>> +		*val = state->colorkey.inverted_match;
>> +	} else if (property == plane->colorkey.replacement_mask_property) {
>> +		*val = state->colorkey.replacement_mask;
>> +	} else if (property == plane->colorkey.replacement_value_property) {
>> +		*val = state->colorkey.replacement_value;
>> +	} else if (property == plane->colorkey.replacement_format_property) {
>> +		*val = state->colorkey.replacement_format;
>>  	} 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..05e5632ce375 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,227 @@ 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_SRC] = "src-match-src-replace",
>> +	[DRM_PLANE_COLORKEY_MODE_DST] = "dst-match-src-replace",
> 
> This list seems way more limited than I was expecting, at least when
> compared to all the different props you're proposing to add.
> 

What else are you expecting to see in the list?

>> +};
>> +
>> +/**
>> + * 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 through nine properties:
>> + *
>> + * colorkey.mode:
>> + *	The mode is an enumerated property that controls how color keying
>> + *	operates. The "disabled" mode that disables color keying and is
>> + *	very likely to exist if color keying is supported, it should be the
>> + *	default mode.
>> + *
>> + * 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 AXYZ16161616 format, where A is the alpha value and
>> + *	X, Y and Z correspond to the color components of the colorkey.format.
>> + *	In most cases XYZ will be either RGB or YUV.
>> + *
>> + *	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.
>> + *
>> + * colorkey.format:
>> + *	This property specifies the pixel format for the colorkey.min / max
>> + *	properties. The format is given in a form of DRM fourcc code.
> 
> Umm. Why we do even need this? This seems incompatible with your earlier
> "min/max are specified in 16bpc format" statement.
> 

AXYZ16161616 is just an abstraction of a pixel format, at least that's how I'm
interpreting the AXYZ16161616.

Previously Laurent specified that min/max values should be given in the format
of the planes framebuffer. This doesn't work for a case of setting property for
a disabled plane because disabled plane doesn't have a framebuffer. This also
doesn't work for Tegra that can take only XRGB8888 format for a color key, AFAIK
HW internally converts all pixel formats to ARGB8888 and RGB part participates
in the color matching (later Tegra's support alpha channel matching as well).
Hence I added the format property that explicitly tells in what format the color
key integer value is given.

I'm now thinking that format property should be exposed to userspace in a form
of a ENUM-list, because otherwise userspace doesn't know what color key formats
are supported by the DRM driver.

And probably somehow userspace should be informed about that colorkey format
should match the framebuffers format if that's what driver expects. Maybe a
read-only property?

>> + *
>> + *	Drivers return an error from their plane atomic check if pixel format
>> + *	is unsupported.
>> + *
>> + * 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
>> + *	form of DRM fourcc code corresponding to the colorkey.format property.
>> + *
>> + *	For example: userspace shall set the colorkey.mask to 0x0000ff00
>> + *	to match only the green component if colorkey.format is set to
>> + *	DRM_FORMAT_XRGB8888.
>> + *
>> + *	Drivers return an error from their plane atomic check if mask value
>> + *	can't be handled.
>> + *
>> + * colorkey.inverted-match:
>> + *	This property specifies whether the matching min-max range should
>> + *	be inverted, i.e. pixels outside of the given color range become
>> + *	the color key match.
>> + *
>> + *	Drivers return an error from their plane atomic check if inversion
>> + *	mode can't be handled.
> 
> Hmm. I'm trying to figure out what this means for the src vs. dst
> colorkey modes. Those pretty much already have an inverted meaning when
> compared to each other. So I can't figure out from these docs whether 
> you're supposed to use this when you want a normal dst ckey or normal
> src key semantics.
> 

I also couldn't understand what initial semantic to src/dst was given by Laurent.

In a case of this patch:

The src/dst-match specifies the "source" plane for which the color matching is
performed. Src means the plane to which the colorkey properties are applied. Dst
means planes other than plane X to which the colorkey properties are applied, in
particular the planes that are Z-positioned under the plane X. Hope that's more
clear now.

The src-replace part specifies that pixels of the plane to which the colorkey
properties are applied will be replaced.

I'll try to re-work the doc if the above sounds good.

The inverted-match property controls whether given color-match range shall be
inverted, like for example: if given color key is a red color, then all colors
expect the red will be matched as a color key.

Actually maybe inversion could be expressed using the mask solely. Like we could
add a helper that converts "value+mask" into "value=(value & ~mask),
inversion=true" if mask has form of 0x11000111, though this could be not
applicable to all possible pixel formats.. not sure.

>> + *
>> + * colorkey.replacement-value:
>> + *	This property specifies the color value that replaces pixels matching
>> + *	the color key. The value is expressed in AXYZ16161616 format, where A
>> + *	is the alpha value and X, Y and Z correspond to the color components
>> + *	of the colorkey.replacement-format.
>> + *
>> + *	Drivers return an error from their plane atomic check if replacement
>> + *	value can't be handled.
>> + *
>> + * colorkey.replacement-format:
>> + *	This property specifies the pixel format for the
>> + *	colorkey.replacement-value property. The format is given in a form of
>> + *	DRM fourcc code.
> 
> Again this seems at odds with the 16bpc replacement-value.
> 
>> + *
>> + *	Drivers return an error from their plane atomic check if replacement
>> + *	pixel format is unsupported.
>> + *
>> + * colorkey.replacement-mask:
>> + *	This property specifies the pixel components mask that defines
>> + *	what components of the colorkey.replacement-value will participate in
>> + *	replacement of the pixels color. Unmasked pixel components are not
>> + *	participating in the replacement.
> 
> Does that mean that the data for the unmasked bits will be coming
> from the source?
> 

Yeah, I see that "source" is vaguely defined.

Yes, the unmasked bits are coming from the source. The "source" is defined by
the mode.

src-match- -->SRC<-- -replace
dst-match- -->SRC<-- -replace

Src means the plane to which the colorkey properties are applied, as I stated above.

>> The mask value is given in a form of
>> + *	DRM fourcc code corresponding to the colorkey.replacement-format
>> + *	property.
>> + *
>> + *	For example: userspace shall set the colorkey.replacement-mask to
>> + *	0x0000ff00 to replace only the green component if
>> + *	colorkey.replacement-format is set to DRM_FORMAT_XRGB8888.
>> + *
>> + *	Userspace shall set colorkey.replacement-mask to 0 to disable the color
>> + *	replacement. In this case matching pixels become transparent.
>> + *
>> + *	Drivers return an error from their plane atomic check if replacement
>> + *	mask value 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 *replacement_format_prop;
>> +	struct drm_property *replacement_value_prop;
>> +	struct drm_property *replacement_mask_prop;
>> +	struct drm_property *inverted_match_prop;
>> +	struct drm_property *format_prop;
>> +	struct drm_property *mask_prop;
>> +	struct drm_property *mode_prop;
>> +	struct drm_property *min_prop;
>> +	struct drm_property *max_prop;
>> +	unsigned int modes_num = 0;
>> +	unsigned int i;
>> +
>> +	/* at least two modes should be supported */
>> +	if (!supported_modes)
>> +		return -EINVAL;
>> +
>> +	/* 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++;
>> +	}
>> +
>> +	mode_prop = drm_property_create_enum(plane->dev, 0, "colorkey.mode",
>> +					     modes_list, modes_num);
>> +	if (!mode_prop)
>> +		return -ENOMEM;
>> +
>> +	mask_prop = drm_property_create_range(plane->dev, 0, "colorkey.mask",
>> +					      0, U64_MAX);
>> +	if (!mask_prop)
>> +		goto err_destroy_mode_prop;
>> +
>> +	min_prop = drm_property_create_range(plane->dev, 0, "colorkey.min",
>> +					     0, U64_MAX);
>> +	if (!min_prop)
>> +		goto err_destroy_mask_prop;
>> +
>> +	max_prop = drm_property_create_range(plane->dev, 0, "colorkey.max",
>> +					     0, U64_MAX);
>> +	if (!max_prop)
>> +		goto err_destroy_min_prop;
>> +
>> +	format_prop = drm_property_create_range(plane->dev, 0,
>> +					"colorkey.format",
>> +					0, U32_MAX);
>> +	if (!format_prop)
>> +		goto err_destroy_max_prop;
>> +
>> +	inverted_match_prop = drm_property_create_bool(plane->dev, 0,
>> +					"colorkey.inverted-match");
>> +	if (!inverted_match_prop)
>> +		goto err_destroy_format_prop;
>> +
>> +	replacement_mask_prop = drm_property_create_range(plane->dev, 0,
>> +					"colorkey.replacement-mask",
>> +					0, U64_MAX);
>> +	if (!replacement_mask_prop)
>> +		goto err_destroy_inverted_match_prop;
>> +
>> +	replacement_value_prop = drm_property_create_range(plane->dev, 0,
>> +					"colorkey.replacement-value",
>> +					0, U64_MAX);
>> +	if (!replacement_value_prop)
>> +		goto err_destroy_replacement_mask_prop;
>> +
>> +	replacement_format_prop = drm_property_create_range(plane->dev, 0,
>> +					"colorkey.replacement-format",
>> +					0, U64_MAX);
>> +	if (!replacement_format_prop)
>> +		goto err_destroy_replacement_value_prop;
> 
> I don't think we want to add all these props for every driver/hardware.
> IMO the set of props we expose should depend on the supported set of
> colorkeying modes.
> 

Probably, I don't mind. This should be documented then, I can address that in
the next iteration.

/I think/ all of the currently-defined properties are relevant to the defined
color keying modes. Later, if the list of modes will be extended with new modes,
the creation of properties could be conditionalized.

Though maybe "color components replacement" and "replacement with a complete
transparency" could be factored out into a specific color keying modes.

I'm open for suggestions.

[snip]

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 29.05.2018 02:48, Dmitry Osipenko wrote:
> inversion=true" if mask has form of 0x11000111, though this could be not

For clarity: I meant s/0x11000111/0xFF000FFF/.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, May 29, 2018 at 02:48:22AM +0300, Dmitry Osipenko wrote:
> On 28.05.2018 16:15, Ville Syrjälä wrote:
> > On Sat, May 26, 2018 at 06:56:22PM +0300, Dmitry Osipenko wrote:
> >> 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 nine generic DRM plane
> >> properties related to the color keying to cover various HW capabilities.
> >>
> >> This patch is based on the initial work done by Laurent Pinchart, most of
> >> credits for this patch goes to him.
> >>
> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >> ---
> >>  drivers/gpu/drm/drm_atomic.c |  36 ++++++
> >>  drivers/gpu/drm/drm_blend.c  | 229 +++++++++++++++++++++++++++++++++++
> >>  include/drm/drm_blend.h      |   3 +
> >>  include/drm/drm_plane.h      |  77 ++++++++++++
> >>  4 files changed, 345 insertions(+)
> >>
> >> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> >> index 895741e9cd7d..5b808cb68654 100644
> >> --- a/drivers/gpu/drm/drm_atomic.c
> >> +++ b/drivers/gpu/drm/drm_atomic.c
> >> @@ -799,6 +799,24 @@ 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->colorkey.format_property) {
> >> +		state->colorkey.format = val;
> >> +	} else if (property == plane->colorkey.mask_property) {
> >> +		state->colorkey.mask = val;
> >> +	} else if (property == plane->colorkey.inverted_match_property) {
> >> +		state->colorkey.inverted_match = val;
> >> +	} else if (property == plane->colorkey.replacement_mask_property) {
> >> +		state->colorkey.replacement_mask = val;
> >> +	} else if (property == plane->colorkey.replacement_value_property) {
> >> +		state->colorkey.replacement_value = val;
> >> +	} else if (property == plane->colorkey.replacement_format_property) {
> >> +		state->colorkey.replacement_format = val;
> >>  	} else if (property == plane->color_encoding_property) {
> >>  		state->color_encoding = val;
> >>  	} else if (property == plane->color_range_property) {
> >> @@ -864,6 +882,24 @@ 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->colorkey.format_property) {
> >> +		*val = state->colorkey.format;
> >> +	} else if (property == plane->colorkey.mask_property) {
> >> +		*val = state->colorkey.mask;
> >> +	} else if (property == plane->colorkey.inverted_match_property) {
> >> +		*val = state->colorkey.inverted_match;
> >> +	} else if (property == plane->colorkey.replacement_mask_property) {
> >> +		*val = state->colorkey.replacement_mask;
> >> +	} else if (property == plane->colorkey.replacement_value_property) {
> >> +		*val = state->colorkey.replacement_value;
> >> +	} else if (property == plane->colorkey.replacement_format_property) {
> >> +		*val = state->colorkey.replacement_format;
> >>  	} 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..05e5632ce375 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,227 @@ 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_SRC] = "src-match-src-replace",
> >> +	[DRM_PLANE_COLORKEY_MODE_DST] = "dst-match-src-replace",
> > 
> > This list seems way more limited than I was expecting, at least when
> > compared to all the different props you're proposing to add.
> > 
> 
> What else are you expecting to see in the list?

Prerry much all kinds of modes for the funky replacement values
and whanot.

> 
> >> +};
> >> +
> >> +/**
> >> + * 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 through nine properties:
> >> + *
> >> + * colorkey.mode:
> >> + *	The mode is an enumerated property that controls how color keying
> >> + *	operates. The "disabled" mode that disables color keying and is
> >> + *	very likely to exist if color keying is supported, it should be the
> >> + *	default mode.
> >> + *
> >> + * 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 AXYZ16161616 format, where A is the alpha value and
> >> + *	X, Y and Z correspond to the color components of the colorkey.format.
> >> + *	In most cases XYZ will be either RGB or YUV.
> >> + *
> >> + *	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.
> >> + *
> >> + * colorkey.format:
> >> + *	This property specifies the pixel format for the colorkey.min / max
> >> + *	properties. The format is given in a form of DRM fourcc code.
> > 
> > Umm. Why we do even need this? This seems incompatible with your earlier
> > "min/max are specified in 16bpc format" statement.
> > 
> 
> AXYZ16161616 is just an abstraction of a pixel format, at least that's how I'm
> interpreting the AXYZ16161616.
> 
> Previously Laurent specified that min/max values should be given in the format
> of the planes framebuffer. This doesn't work for a case of setting property for
> a disabled plane because disabled plane doesn't have a framebuffer. This also
> doesn't work for Tegra that can take only XRGB8888 format for a color key, AFAIK
> HW internally converts all pixel formats to ARGB8888 and RGB part participates
> in the color matching (later Tegra's support alpha channel matching as well).
> Hence I added the format property that explicitly tells in what format the color
> key integer value is given.
> 
> I'm now thinking that format property should be exposed to userspace in a form
> of a ENUM-list, because otherwise userspace doesn't know what color key formats
> are supported by the DRM driver.

I'm not sure that'll help particularly much. Generally you can't 
choose the format anyway, and it may just depends on the fb format
or some other related factors. So not really sure how much benefit
such an enum would have.

And for hw where eg. the colorkey is always RGB even for YUV data
(or vice versa) we could just do the YUV<->RGB conversion in the
driver for the colorkey value provided by the user. The driver is
in the best position to know the exact YUV<->RGB conversion formula
used by the hardware anyway.

> 
> And probably somehow userspace should be informed about that colorkey format
> should match the framebuffers format if that's what driver expects. Maybe a
> read-only property?
> 
> >> + *
> >> + *	Drivers return an error from their plane atomic check if pixel format
> >> + *	is unsupported.
> >> + *
> >> + * 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
> >> + *	form of DRM fourcc code corresponding to the colorkey.format property.
> >> + *
> >> + *	For example: userspace shall set the colorkey.mask to 0x0000ff00
> >> + *	to match only the green component if colorkey.format is set to
> >> + *	DRM_FORMAT_XRGB8888.
> >> + *
> >> + *	Drivers return an error from their plane atomic check if mask value
> >> + *	can't be handled.
> >> + *
> >> + * colorkey.inverted-match:
> >> + *	This property specifies whether the matching min-max range should
> >> + *	be inverted, i.e. pixels outside of the given color range become
> >> + *	the color key match.
> >> + *
> >> + *	Drivers return an error from their plane atomic check if inversion
> >> + *	mode can't be handled.
> > 
> > Hmm. I'm trying to figure out what this means for the src vs. dst
> > colorkey modes. Those pretty much already have an inverted meaning when
> > compared to each other. So I can't figure out from these docs whether 
> > you're supposed to use this when you want a normal dst ckey or normal
> > src key semantics.
> > 
> 
> I also couldn't understand what initial semantic to src/dst was given by Laurent.
> 
> In a case of this patch:
> 
> The src/dst-match specifies the "source" plane for which the color matching is
> performed. Src means the plane to which the colorkey properties are applied. Dst
> means planes other than plane X to which the colorkey properties are applied, in
> particular the planes that are Z-positioned under the plane X. Hope that's more
> clear now.
> 
> The src-replace part specifies that pixels of the plane to which the colorkey
> properties are applied will be replaced.
> 
> I'll try to re-work the doc if the above sounds good.
> 
> The inverted-match property controls whether given color-match range shall be
> inverted, like for example: if given color key is a red color, then all colors
> expect the red will be matched as a color key.
> 
> Actually maybe inversion could be expressed using the mask solely. Like we could
> add a helper that converts "value+mask" into "value=(value & ~mask),
> inversion=true" if mask has form of 0x11000111, though this could be not
> applicable to all possible pixel formats.. not sure.
> 
> >> + *
> >> + * colorkey.replacement-value:
> >> + *	This property specifies the color value that replaces pixels matching
> >> + *	the color key. The value is expressed in AXYZ16161616 format, where A
> >> + *	is the alpha value and X, Y and Z correspond to the color components
> >> + *	of the colorkey.replacement-format.
> >> + *
> >> + *	Drivers return an error from their plane atomic check if replacement
> >> + *	value can't be handled.
> >> + *
> >> + * colorkey.replacement-format:
> >> + *	This property specifies the pixel format for the
> >> + *	colorkey.replacement-value property. The format is given in a form of
> >> + *	DRM fourcc code.
> > 
> > Again this seems at odds with the 16bpc replacement-value.
> > 
> >> + *
> >> + *	Drivers return an error from their plane atomic check if replacement
> >> + *	pixel format is unsupported.
> >> + *
> >> + * colorkey.replacement-mask:
> >> + *	This property specifies the pixel components mask that defines
> >> + *	what components of the colorkey.replacement-value will participate in
> >> + *	replacement of the pixels color. Unmasked pixel components are not
> >> + *	participating in the replacement.
> > 
> > Does that mean that the data for the unmasked bits will be coming
> > from the source?
> > 
> 
> Yeah, I see that "source" is vaguely defined.
> 
> Yes, the unmasked bits are coming from the source. The "source" is defined by
> the mode.
> 
> src-match- -->SRC<-- -replace
> dst-match- -->SRC<-- -replace
> 
> Src means the plane to which the colorkey properties are applied, as I stated above.
> 
> >> The mask value is given in a form of
> >> + *	DRM fourcc code corresponding to the colorkey.replacement-format
> >> + *	property.
> >> + *
> >> + *	For example: userspace shall set the colorkey.replacement-mask to
> >> + *	0x0000ff00 to replace only the green component if
> >> + *	colorkey.replacement-format is set to DRM_FORMAT_XRGB8888.
> >> + *
> >> + *	Userspace shall set colorkey.replacement-mask to 0 to disable the color
> >> + *	replacement. In this case matching pixels become transparent.
> >> + *
> >> + *	Drivers return an error from their plane atomic check if replacement
> >> + *	mask value 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 *replacement_format_prop;
> >> +	struct drm_property *replacement_value_prop;
> >> +	struct drm_property *replacement_mask_prop;
> >> +	struct drm_property *inverted_match_prop;
> >> +	struct drm_property *format_prop;
> >> +	struct drm_property *mask_prop;
> >> +	struct drm_property *mode_prop;
> >> +	struct drm_property *min_prop;
> >> +	struct drm_property *max_prop;
> >> +	unsigned int modes_num = 0;
> >> +	unsigned int i;
> >> +
> >> +	/* at least two modes should be supported */
> >> +	if (!supported_modes)
> >> +		return -EINVAL;
> >> +
> >> +	/* 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++;
> >> +	}
> >> +
> >> +	mode_prop = drm_property_create_enum(plane->dev, 0, "colorkey.mode",
> >> +					     modes_list, modes_num);
> >> +	if (!mode_prop)
> >> +		return -ENOMEM;
> >> +
> >> +	mask_prop = drm_property_create_range(plane->dev, 0, "colorkey.mask",
> >> +					      0, U64_MAX);
> >> +	if (!mask_prop)
> >> +		goto err_destroy_mode_prop;
> >> +
> >> +	min_prop = drm_property_create_range(plane->dev, 0, "colorkey.min",
> >> +					     0, U64_MAX);
> >> +	if (!min_prop)
> >> +		goto err_destroy_mask_prop;
> >> +
> >> +	max_prop = drm_property_create_range(plane->dev, 0, "colorkey.max",
> >> +					     0, U64_MAX);
> >> +	if (!max_prop)
> >> +		goto err_destroy_min_prop;
> >> +
> >> +	format_prop = drm_property_create_range(plane->dev, 0,
> >> +					"colorkey.format",
> >> +					0, U32_MAX);
> >> +	if (!format_prop)
> >> +		goto err_destroy_max_prop;
> >> +
> >> +	inverted_match_prop = drm_property_create_bool(plane->dev, 0,
> >> +					"colorkey.inverted-match");
> >> +	if (!inverted_match_prop)
> >> +		goto err_destroy_format_prop;
> >> +
> >> +	replacement_mask_prop = drm_property_create_range(plane->dev, 0,
> >> +					"colorkey.replacement-mask",
> >> +					0, U64_MAX);
> >> +	if (!replacement_mask_prop)
> >> +		goto err_destroy_inverted_match_prop;
> >> +
> >> +	replacement_value_prop = drm_property_create_range(plane->dev, 0,
> >> +					"colorkey.replacement-value",
> >> +					0, U64_MAX);
> >> +	if (!replacement_value_prop)
> >> +		goto err_destroy_replacement_mask_prop;
> >> +
> >> +	replacement_format_prop = drm_property_create_range(plane->dev, 0,
> >> +					"colorkey.replacement-format",
> >> +					0, U64_MAX);
> >> +	if (!replacement_format_prop)
> >> +		goto err_destroy_replacement_value_prop;
> > 
> > I don't think we want to add all these props for every driver/hardware.
> > IMO the set of props we expose should depend on the supported set of
> > colorkeying modes.
> > 
> 
> Probably, I don't mind. This should be documented then, I can address that in
> the next iteration.
> 
> /I think/ all of the currently-defined properties are relevant to the defined
> color keying modes.

i915 will want just value/mask or min/max, depending on whether we're
dealing with rgb or yuv. And I've seen a lot of hardware with just the
value/mask approach over the years.

> Later, if the list of modes will be extended with new modes,
> the creation of properties could be conditionalized.
> 
> Though maybe "color components replacement" and "replacement with a complete
> transparency" could be factored out into a specific color keying modes.

Yes. I've never seen those in any hardware. In fact I'm wondering where
is the userspace for all these complex modes? Ie. should we even bother
with them at this time?

On Sat, May 26, 2018 at 06:56:22PM +0300, Dmitry Osipenko wrote:
> 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 nine generic DRM plane
> properties related to the color keying to cover various HW capabilities.
> 
> This patch is based on the initial work done by Laurent Pinchart, most of
> credits for this patch goes to him.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/gpu/drm/drm_atomic.c |  36 ++++++
>  drivers/gpu/drm/drm_blend.c  | 229 +++++++++++++++++++++++++++++++++++
>  include/drm/drm_blend.h      |   3 +
>  include/drm/drm_plane.h      |  77 ++++++++++++
>  4 files changed, 345 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
> index 895741e9cd7d..5b808cb68654 100644
> @@ -124,6 +175,19 @@ struct drm_plane_state {
>  	unsigned int zpos;
>  	unsigned int normalized_zpos;
>  
> +	/* Plane colorkey */
> +	struct {
> +		enum drm_plane_colorkey_mode mode;
> +		u64 min;
> +		u64 max;
> +		u64 mask;
> +		u32 format;
> +		bool inverted_match;
> +		u64 replacement_mask;
> +		u64 replacement_value;
> +		u32 replacement_format;
> +	} colorkey;

After a quick stab at implementing this for i915 I came to the
conclusion that I'd like this struct to have a name so that I can pass
it around/consult it easily without having to mess about with the entire
plane state.

One extra question that came up is how are we going to define the
destination color key? Is it to be a) enabled on the plane that has the
colorkey painted on it, or is it to be b) enabled on a plane that sits
above the plane that is going to be covering up the colorkey? Modern
Intel hardware defines it the a) way, whereas older hw used the b)
method. Thinking about it I do think the a) method seems nicer because
it removes the ambiguity as to which plane's pixels are going to be
compared again the colorkey. So kinda matches the src colorkey case
better.

Oh and this also brings up the question as to which other plane(s) are
taking part in the colorkey process. Looks like on modern Intel hw it's
supposed to be just the plane immediately above the plane with
dst keying enabled. On some really old hw there were some more
complicated rules as to which pair of planes are involved. On middle
aged hw the situation is simpler a there are only ever two
(non-cursor) planes on the same crtc. The cursor doesn't seem to
participate in the colorkeing ever. Not sure there's a sane way to
fully abstract this all.

I should probably poke at the hardware a bit more to figure out how
this really works when there are more than two active planes on the
crtc. <day later> I did poke at one particular class of hw which is
a bit of a mix of old and middle aged hw, and there it seems I can
also do dst colorkeying the a) way. And in this case there are three
planes taking part in the dst colorkey match (the primary that
has the colorkey painted on it, and two overlay planes that cover
up the matched pixels). So for that case definitely the a) approach
in the uapi would make more sense as trying to specify conflicting
dst colorkey settings for each of the overlay planes wouldn't make
any sense. I'll need to poke at the modern hw a bit more still...

On 29.05.2018 10:17, Ville Syrjälä wrote:
> On Sat, May 26, 2018 at 06:56:22PM +0300, Dmitry Osipenko wrote:
>> 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 nine generic DRM plane
>> properties related to the color keying to cover various HW capabilities.
>>
>> This patch is based on the initial work done by Laurent Pinchart, most of
>> credits for this patch goes to him.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/gpu/drm/drm_atomic.c |  36 ++++++
>>  drivers/gpu/drm/drm_blend.c  | 229 +++++++++++++++++++++++++++++++++++
>>  include/drm/drm_blend.h      |   3 +
>>  include/drm/drm_plane.h      |  77 ++++++++++++
>>  4 files changed, 345 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c
>> index 895741e9cd7d..5b808cb68654 100644
>> @@ -124,6 +175,19 @@ struct drm_plane_state {
>>  	unsigned int zpos;
>>  	unsigned int normalized_zpos;
>>  
>> +	/* Plane colorkey */
>> +	struct {
>> +		enum drm_plane_colorkey_mode mode;
>> +		u64 min;
>> +		u64 max;
>> +		u64 mask;
>> +		u32 format;
>> +		bool inverted_match;
>> +		u64 replacement_mask;
>> +		u64 replacement_value;
>> +		u32 replacement_format;
>> +	} colorkey;
> 
> After a quick stab at implementing this for i915 I came to the
> conclusion that I'd like this struct to have a name so that I can pass
> it around/consult it easily without having to mess about with the entire
> plane state.
> 
> One extra question that came up is how are we going to define the
> destination color key? Is it to be a) enabled on the plane that has the
> colorkey painted on it, or is it to be b) enabled on a plane that sits
> above the plane that is going to be covering up the colorkey? Modern
> Intel hardware defines it the a) way, whereas older hw used the b)
> method. Thinking about it I do think the a) method seems nicer because
> it removes the ambiguity as to which plane's pixels are going to be
> compared again the colorkey. So kinda matches the src colorkey case
> better.
> 
> Oh and this also brings up the question as to which other plane(s) are
> taking part in the colorkey process. Looks like on modern Intel hw it's
> supposed to be just the plane immediately above the plane with
> dst keying enabled. On some really old hw there were some more
> complicated rules as to which pair of planes are involved. On middle
> aged hw the situation is simpler a there are only ever two
> (non-cursor) planes on the same crtc. The cursor doesn't seem to
> participate in the colorkeing ever. Not sure there's a sane way to
> fully abstract this all.
> 
> I should probably poke at the hardware a bit more to figure out how
> this really works when there are more than two active planes on the
> crtc. <day later> I did poke at one particular class of hw which is
> a bit of a mix of old and middle aged hw, and there it seems I can
> also do dst colorkeying the a) way. And in this case there are three
> planes taking part in the dst colorkey match (the primary that
> has the colorkey painted on it, and two overlay planes that cover
> up the matched pixels). So for that case definitely the a) approach
> in the uapi would make more sense as trying to specify conflicting
> dst colorkey settings for each of the overlay planes wouldn't make
> any sense. I'll need to poke at the modern hw a bit more still...
> 

Color keying mode must explicitly define the expected behavior. It is up to a
driver to enable HW color keying for an appropriate plane, or whatever needs to
be done to provide the behavior.

After some more considering, I've reduced the color keying properties and modes
to a bare-and-practical minimum, also taking into account your comments. I'll
send out a new version of the patch later today, let's continue discussion there.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Tue, May 29, 2018 at 10:11:03AM +0300, Ville Syrjälä wrote:
> On Tue, May 29, 2018 at 02:48:22AM +0300, Dmitry Osipenko wrote:
> > Though maybe "color components replacement" and "replacement with a complete
> > transparency" could be factored out into a specific color keying modes.
> 
> Yes. I've never seen those in any hardware. In fact I'm wondering where
> is the userspace for all these complex modes? Ie. should we even bother
> with them at this time?

Such hardware does exist - here's what Armada 510 supports (and is
supported via armada-drm):

Color Key Mode
0 = Disable: Disable color key function.
1 = EnableY: Video Y color key is enabled.
2 = EnableU: Video U color key is enabled.
3 = EnableRGB: Graphic RGB color key is enabled.
4 = EnableV: Video V color key is enabled.
5 = EnableR: Graphic R color key is enabled.
6 = EnableG: Graphic G color key is enabled.
7 = EnableB: Graphic B color key is enabled.

The description of how the colour keying works isn't particularly good,
which is rather unfortunate, but basically, there's three 32-bit
registers named LCD_SPU_COLORKEY_Y, LCD_SPU_COLORKEY_U and
LCD_SPU_COLORKEY_V.

When a graphic mode is selected, then:
 LCD_SPU_COLORKEY_Y is the R or B component depending on the red/blue swap
 LCD_SPU_COLORKEY_U is the G component
 LCD_SPU_COLORKEY_V is the B or R component according to the swap

31:24 is the high bound for the component (inclusive)
23:16 is the low bound for the component (inclusive)
15:8  is the replacement value for the component
 7:0  is the alpha value - seems to only be for LCD_SPU_COLORKEY_Y and
	ignored in the other registers when in RGB mode, but I've not
	fully tested this.  I suspect it's used with the single-channel
	colour keying modes.

The colour key stage provides an alpha value to the next stage - which
is alpha blending between the graphic (primary) plane and video
(overlay) plane.  Zero gives overlay only, 255 gives graphic only.

So, it's possible to use an 0x0101fe (RGB) colour key, and have it
appear as "black" on the screen if you disable the overlay plane,
rather than the unsightly bright blue.

One point to make though is about what userspace expects _today_ from
overlay.  VLC, for example, expects overlay to be colour keyed, so it
can display its full-screen controller when the user moves the mouse.
I don't believe it explicitly sets up colour keying, but just expects
it to be there and functional.  It _is_ rather necessary when you
consider that overlay via the Xvideo extension is supposed to be
"drawn" into the specified drawable, which may be a mapped window
partially obscured by other mapped windows.

Maybe if the kernel DRM components doesn't want to do that, it'd be
something that an Xorg DDX would have to default-enable to ensure
that existing applications and expected Xorg behaviour doesn't break.

On Tue, Jun 19, 2018 at 06:40:12PM +0100, Russell King - ARM Linux wrote:
> On Tue, May 29, 2018 at 10:11:03AM +0300, Ville Syrjälä wrote:
> > On Tue, May 29, 2018 at 02:48:22AM +0300, Dmitry Osipenko wrote:
> > > Though maybe "color components replacement" and "replacement with a complete
> > > transparency" could be factored out into a specific color keying modes.
> > 
> > Yes. I've never seen those in any hardware. In fact I'm wondering where
> > is the userspace for all these complex modes? Ie. should we even bother
> > with them at this time?
> 
> Such hardware does exist - here's what Armada 510 supports (and is
> supported via armada-drm):
> 
> Color Key Mode
> 0 = Disable: Disable color key function.
> 1 = EnableY: Video Y color key is enabled.
> 2 = EnableU: Video U color key is enabled.
> 3 = EnableRGB: Graphic RGB color key is enabled.
> 4 = EnableV: Video V color key is enabled.
> 5 = EnableR: Graphic R color key is enabled.
> 6 = EnableG: Graphic G color key is enabled.
> 7 = EnableB: Graphic B color key is enabled.
> 
> The description of how the colour keying works isn't particularly good,
> which is rather unfortunate, but basically, there's three 32-bit
> registers named LCD_SPU_COLORKEY_Y, LCD_SPU_COLORKEY_U and
> LCD_SPU_COLORKEY_V.
> 
> When a graphic mode is selected, then:
>  LCD_SPU_COLORKEY_Y is the R or B component depending on the red/blue swap
>  LCD_SPU_COLORKEY_U is the G component
>  LCD_SPU_COLORKEY_V is the B or R component according to the swap
> 
> 31:24 is the high bound for the component (inclusive)
> 23:16 is the low bound for the component (inclusive)
> 15:8  is the replacement value for the component
>  7:0  is the alpha value - seems to only be for LCD_SPU_COLORKEY_Y and
> 	ignored in the other registers when in RGB mode, but I've not
> 	fully tested this.  I suspect it's used with the single-channel
> 	colour keying modes.
> 
> The colour key stage provides an alpha value to the next stage - which
> is alpha blending between the graphic (primary) plane and video
> (overlay) plane.  Zero gives overlay only, 255 gives graphic only.
> 
> So, it's possible to use an 0x0101fe (RGB) colour key, and have it
> appear as "black" on the screen if you disable the overlay plane,
> rather than the unsightly bright blue.
> 
> One point to make though is about what userspace expects _today_ from
> overlay.  VLC, for example, expects overlay to be colour keyed, so it
> can display its full-screen controller when the user moves the mouse.
> I don't believe it explicitly sets up colour keying, but just expects
> it to be there and functional.  It _is_ rather necessary when you
> consider that overlay via the Xvideo extension is supposed to be
> "drawn" into the specified drawable, which may be a mapped window
> partially obscured by other mapped windows.
> 
> Maybe if the kernel DRM components doesn't want to do that, it'd be
> something that an Xorg DDX would have to default-enable to ensure
> that existing applications and expected Xorg behaviour doesn't break.

Yes -modesetting (or whichever other driver) would need to set up the
properties correctly for Xvideo color keying. Since default assumption for
all other (generic) compositors is that planes won't do any color keying
in the boot-up state.
-Daniel

On Wed, Jun 20, 2018 at 11:17:50AM +0200, Daniel Vetter wrote:
> Yes -modesetting (or whichever other driver) would need to set up the
> properties correctly for Xvideo color keying. Since default assumption for
> all other (generic) compositors is that planes won't do any color keying
> in the boot-up state.

Thanks, is that documented anywhere?

On Wed, Jun 20, 2018 at 11:33 AM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Wed, Jun 20, 2018 at 11:17:50AM +0200, Daniel Vetter wrote:
>> Yes -modesetting (or whichever other driver) would need to set up the
>> properties correctly for Xvideo color keying. Since default assumption for
>> all other (generic) compositors is that planes won't do any color keying
>> in the boot-up state.
>
> Thanks, is that documented anywhere?

With the standardization of properties I'm also trying to get these
expectations better documented, e.g.

https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-properties

But I think we're not doing a good job yet documenting default
expectations. But the above would be a good place to get that fixed -
this patch here should do that, at least for color keying.
-Daniel

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

On Tuesday, 19 June 2018 20:40:12 MSK Russell King - ARM Linux wrote:
> On Tue, May 29, 2018 at 10:11:03AM +0300, Ville Syrjälä wrote:
> > On Tue, May 29, 2018 at 02:48:22AM +0300, Dmitry Osipenko wrote:
> > > Though maybe "color components replacement" and "replacement with a
> > > complete transparency" could be factored out into a specific color
> > > keying modes.> 
> > Yes. I've never seen those in any hardware. In fact I'm wondering where
> > is the userspace for all these complex modes? Ie. should we even bother
> > with them at this time?
> 
> Such hardware does exist - here's what Armada 510 supports (and is
> supported via armada-drm):

It shouldn't be a problem to extend the list of common color keying modes with 
a mode that is supported only by a particular HW. The only requirement for 
adding a new mode is that this mode must be utilized by opensource userspace 
software.


--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wednesday, 20 June 2018 13:10:07 MSK Daniel Vetter wrote:
> On Wed, Jun 20, 2018 at 11:33 AM, Russell King - ARM Linux
> 
> <linux@armlinux.org.uk> wrote:
> > On Wed, Jun 20, 2018 at 11:17:50AM +0200, Daniel Vetter wrote:
> >> Yes -modesetting (or whichever other driver) would need to set up the
> >> properties correctly for Xvideo color keying. Since default assumption
> >> for
> >> all other (generic) compositors is that planes won't do any color keying
> >> in the boot-up state.
> > 
> > Thanks, is that documented anywhere?
> 
> With the standardization of properties I'm also trying to get these
> expectations better documented, e.g.
> 
> https://dri.freedesktop.org/docs/drm/gpu/drm-kms.html#plane-composition-prop
> erties
> 
> But I think we're not doing a good job yet documenting default
> expectations. But the above would be a good place to get that fixed -
> this patch here should do that, at least for color keying.
> -Daniel

There is a comment in this patch that says:

+ * colorkey.mode:
+ *     The mode is an enumerated property that controls how color keying
+ *     operates. The "disabled" mode that disables color keying and is
+ *     very likely to exist if color keying is supported, it should be the
+ *     default mode.

So the default-disabled state is kinda documented. Though that comment is 
omitted in the v3, I'll correct that in the next revision.

For now one question that keeps this patchset in RFC state is about how to 
expose the color key value properties to userspace, whether having drivers to 
perform RGB->YUV conversion is a viable way (see the v3 of the patchset).


--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html