Message-ID: <c6c27f5d-bce1-95e0-61d0-1c14eaf04a83@gmail.com>
Date:   Thu, 20 Sep 2018 18:10:08 +0300
From:   Dmitry Osipenko <digetx@...il.com>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     Ville Syrjälä <ville.syrjala@...ux.intel.com>,
        Thierry Reding <thierry.reding@...il.com>,
        Neil Armstrong <narmstrong@...libre.com>,
        Maxime Ripard <maxime.ripard@...e-electrons.com>,
        dri-devel@...ts.freedesktop.org,
        Paul Kocialkowski <paul.kocialkowski@...tlin.com>,
        Russell King <linux@...linux.org.uk>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        linux-media@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
        Ben Skeggs <bskeggs@...hat.com>,
        Sinclair Yeh <syeh@...are.com>,
        Thomas Hellstrom <thellstrom@...are.com>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH v4 1/2] drm: Add generic colorkey properties for
 display planes

Hello Laurent,

Sorry for the late response.

On 8/14/18 12:48 PM, Laurent Pinchart wrote:
> Hi Dmitry,
> 
> Thank you for the patch. > On Tuesday, 7 August 2018 20:22:01 EEST Dmitry Osipenko wrote:
>> From: Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>
>>
>> Color keying is the action of replacing pixels matching a given color
>> (or range of colors) with transparent pixels in an overlay when
>> performing blitting. Depending on the hardware capabilities, the
>> matching pixel can either become fully transparent or gain adjustment
>> of the pixels component values.
>>
>> Color keying is found in a large number of devices whose capabilities
>> often differ, but they still have enough common features in range to
>> standardize color key properties. This commit adds new generic DRM plane
>> properties related to the color keying, providing initial color keying
>> support.
>>
>> Signed-off-by: Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>
>> Signed-off-by: Dmitry Osipenko <digetx@...il.com>
>> ---
>>  drivers/gpu/drm/drm_atomic.c |  20 +++++
>>  drivers/gpu/drm/drm_blend.c  | 150 +++++++++++++++++++++++++++++++++++
>>  include/drm/drm_blend.h      |   3 +
>>  include/drm/drm_plane.h      |  91 +++++++++++++++++++++
>>  4 files changed, 264 insertions(+)
> 
> [snip]
> 
>> diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
>> index a16a74d7e15e..13c61dd0d9b7 100644
>> --- a/drivers/gpu/drm/drm_blend.c
>> +++ b/drivers/gpu/drm/drm_blend.c
>> @@ -107,6 +107,11 @@
>>   *	planes. Without this property the primary plane is always below the
>> cursor *	plane, and ordering between all other planes is undefined.
>>   *
>> + * colorkey:
>> + *	Color keying is set up with drm_plane_create_colorkey_properties().
>> + *	It adds support for actions like replacing a range of colors with a
>> + *	transparent color in the plane. Color keying is disabled by default.
>> + *
>>   * Note that all the property extensions described here apply either to the
>> * plane or the CRTC (e.g. for the background color, which currently is not
>> * exposed and assumed to be black).
>> @@ -448,3 +453,148 @@ int drm_atomic_normalize_zpos(struct drm_device *dev,
>>  	return 0;
>>  }
>>  EXPORT_SYMBOL(drm_atomic_normalize_zpos);
>> +
>> +static const char * const plane_colorkey_mode_name[] = {
>> +	[DRM_PLANE_COLORKEY_MODE_DISABLED] = "disabled",
>> +	[DRM_PLANE_COLORKEY_MODE_TRANSPARENT] = "transparent",
>> +};
>> +
>> +/**
>> + * drm_plane_create_colorkey_properties - create colorkey properties
>> + * @plane: drm plane
>> + * @supported_modes: bitmask of supported color keying modes
>> + *
>> + * This function creates the generic color keying properties and attaches
>> them
>> + * to the @plane to enable color keying control for blending operations.
>> + *
>> + * Glossary:
>> + *
>> + * Destination plane:
>> + *	Plane to which color keying properties are applied, this planes takes
>> + *	the effect of color keying operation. The effect is determined by a
>> + *	given color keying mode.
>> + *
>> + * Source plane:
>> + *	Pixels of this plane are the source for color key matching operation.
>> + *
>> + * Color keying is controlled by these properties:
>> + *
>> + * colorkey.plane_mask:
>> + *	The mask property specifies which planes participate in color key
>> + *	matching process, these planes are the color key sources.
>> + *
>> + *	Drivers return an error from their plane atomic check if plane can't be
>> + *	handled.
> 
> This seems fragile to me. We don't document how userspace determines which 
> planes need to be specified here, and we don't document what happens if a 
> plane underneath the destination plane is not specified in the mask. More 
> precise documentation is needed if we want to use such a property.

I'll add couple more words.

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

The reasons for that property:

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

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

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

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

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

Okay.

>> + * colorkey.mask:
>> + *	This property specifies the pixel components mask. Unmasked pixel
>> + *	components are not participating in the matching. This mask value is
>> + *	applied to colorkey.min / max values. The mask value is given in a
>> + *	64-bit integer in ARGB16161616 format, where A is the alpha value and
>> + *	R, G and B correspond to the color components. Drivers shall convert
>> + *	ARGB16161616 value into appropriate format within planes atomic check.
>> + *
>> + *	Drivers return an error from their plane atomic check if mask can't be
>> + *	handled.
>> + *
>> + * colorkey.min, colorkey.max:
>> + *	These two properties specify the colors that are treated as the color
>> + *	key. Pixel whose value is in the [min, max] range is the color key
>> + *	matching pixel. The minimum and maximum values are expressed as a
>> + *	64-bit integer in ARGB16161616 format, where A is the alpha value and
>> + *	R, G and B correspond to the color components. Drivers shall convert
>> + *	ARGB16161616 value into appropriate format within planes atomic check.
>> + *	The converted value shall be *rounded up* to the nearest value.
>> + *
>> + *	When a single color key is desired instead of a range, userspace shall
>> + *	set the min and max properties to the same value.
>> + *
>> + *	Drivers return an error from their plane atomic check if range can't be
>> + *	handled.
>> + *
>> + * Returns:
>> + * Zero on success, negative errno on failure.
>> + */
> 
> While you're defining the concept of source and destination planes, it's not 
> clear from the documentation how all this maps to the usual source and 
> destination color keying concepts. I think that should be documented as well 
> or users will be confused. Examples could help in this area.

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

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

Yes

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

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

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

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

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

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

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

Powered by blists - more mailing lists