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

[Ville Syrjälä]

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@xxxxxxxxx>
> ---
>  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...

-- 
Ville Syrjälä
Intel