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

[Laurent Pinchart]

Hi Dmitry,

Thank you for the patch.

On Tuesday, 7 August 2018 20:22:01 EEST Dmitry Osipenko wrote:
> From: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> 
> Color keying is the action of replacing pixels matching a given color
> (or range of colors) with transparent pixels in an overlay when
> performing blitting. Depending on the hardware capabilities, the
> matching pixel can either become fully transparent or gain adjustment
> of the pixels component values.
> 
> Color keying is found in a large number of devices whose capabilities
> often differ, but they still have enough common features in range to
> standardize color key properties. This commit adds new generic DRM plane
> properties related to the color keying, providing initial color keying
> support.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@xxxxxxxxxxxxxxxx>
> Signed-off-by: Dmitry Osipenko <digetx@xxxxxxxxx>
> ---
>  drivers/gpu/drm/drm_atomic.c |  20 +++++
>  drivers/gpu/drm/drm_blend.c  | 150 +++++++++++++++++++++++++++++++++++
>  include/drm/drm_blend.h      |   3 +
>  include/drm/drm_plane.h      |  91 +++++++++++++++++++++
>  4 files changed, 264 insertions(+)

[snip]

> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
> index a16a74d7e15e..13c61dd0d9b7 100644
> --- a/drivers/gpu/drm/drm_blend.c
> +++ b/drivers/gpu/drm/drm_blend.c
> @@ -107,6 +107,11 @@
>   *	planes. Without this property the primary plane is always below the
> cursor *	plane, and ordering between all other planes is undefined.
>   *
> + * colorkey:
> + *	Color keying is set up with drm_plane_create_colorkey_properties().
> + *	It adds support for actions like replacing a range of colors with a
> + *	transparent color in the plane. Color keying is disabled by default.
> + *
>   * Note that all the property extensions described here apply either to the
> * plane or the CRTC (e.g. for the background color, which currently is not
> * exposed and assumed to be black).
> @@ -448,3 +453,148 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
>  	return 0;
>  }
>  EXPORT_SYMBOL(drm_atomic_normalize_zpos);
> +
> +static const char * const plane_colorkey_mode_name[] = {
> +	[DRM_PLANE_COLORKEY_MODE_DISABLED] = "disabled",
> +	[DRM_PLANE_COLORKEY_MODE_TRANSPARENT] = "transparent",
> +};
> +
> +/**
> + * drm_plane_create_colorkey_properties - create colorkey properties
> + * @plane: drm plane
> + * @supported_modes: bitmask of supported color keying modes
> + *
> + * This function creates the generic color keying properties and attaches
> them
> + * to the @plane to enable color keying control for blending operations.
> + *
> + * Glossary:
> + *
> + * Destination plane:
> + *	Plane to which color keying properties are applied, this planes takes
> + *	the effect of color keying operation. The effect is determined by a
> + *	given color keying mode.
> + *
> + * Source plane:
> + *	Pixels of this plane are the source for color key matching operation.
> + *
> + * Color keying is controlled by these properties:
> + *
> + * colorkey.plane_mask:
> + *	The mask property specifies which planes participate in color key
> + *	matching process, these planes are the color key sources.
> + *
> + *	Drivers return an error from their plane atomic check if plane can't be
> + *	handled.

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

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

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

A link to the drm_plane_colorkey_mode enum documentation would be useful.

> + * colorkey.mask:
> + *	This property specifies the pixel components mask. Unmasked pixel
> + *	components are not participating in the matching. This mask value is
> + *	applied to colorkey.min / max values. The mask value is given in a
> + *	64-bit integer in ARGB16161616 format, where A is the alpha value and
> + *	R, G and B correspond to the color components. Drivers shall convert
> + *	ARGB16161616 value into appropriate format within planes atomic check.
> + *
> + *	Drivers return an error from their plane atomic check if mask can't be
> + *	handled.
> + *
> + * colorkey.min, colorkey.max:
> + *	These two properties specify the colors that are treated as the color
> + *	key. Pixel whose value is in the [min, max] range is the color key
> + *	matching pixel. The minimum and maximum values are expressed as a
> + *	64-bit integer in ARGB16161616 format, where A is the alpha value and
> + *	R, G and B correspond to the color components. Drivers shall convert
> + *	ARGB16161616 value into appropriate format within planes atomic check.
> + *	The converted value shall be *rounded up* to the nearest value.
> + *
> + *	When a single color key is desired instead of a range, userspace shall
> + *	set the min and max properties to the same value.
> + *
> + *	Drivers return an error from their plane atomic check if range can't be
> + *	handled.
> + *
> + * Returns:
> + * Zero on success, negative errno on failure.
> + */

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

[snip]

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

[snip]

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

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

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

Do you mean "No color keying" ?

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

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

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

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

> +};

[snip]

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

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

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

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

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

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

>  #endif

-- 
Regards,

Laurent Pinchart