Message-ID: <2295190.xHWjP7Ltc3@dimapc>
Date:   Fri, 06 Jul 2018 17:58:50 +0300
From:   Dmitry Osipenko <digetx@...il.com>
To:     Ville Syrjälä <ville.syrjala@...ux.intel.com>
Cc:     Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Laurent Pinchart <laurent.pinchart+renesas@...asonboard.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>,
        Thomas Hellstrom <thellstrom@...are.com>,
        Russell King <linux@...linux.org.uk>,
        linux-kernel@...r.kernel.org, linux-renesas-soc@...r.kernel.org,
        Ben Skeggs <bskeggs@...hat.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        linux-tegra@...r.kernel.org, linux-media@...r.kernel.org
Subject: Re: [RFC PATCH v3 1/2] drm: Add generic colorkey properties for DRM planes

On Friday, 6 July 2018 17:10:10 MSK Ville Syrjälä wrote:
> On Fri, Jul 06, 2018 at 04:05:21PM +0300, Dmitry Osipenko wrote:
> > On 06.07.2018 15:23, Ville Syrjälä wrote:
> > > On Fri, Jul 06, 2018 at 02:11:44PM +0200, Maarten Lankhorst wrote:
> > >> Hey,
> > >> 
> > >> Op 04-06-18 om 00:00 schreef Dmitry Osipenko:
> > >>> 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 three 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 | 12 +++++
> > >>>  drivers/gpu/drm/drm_blend.c  | 99
> > >>>  ++++++++++++++++++++++++++++++++++++
> > >>>  include/drm/drm_blend.h      |  3 ++
> > >>>  include/drm/drm_plane.h      | 53 +++++++++++++++++++
> > >>>  4 files changed, 167 insertions(+)
> > >>> 
> > >>> diff --git a/drivers/gpu/drm/drm_atomic.c
> > >>> b/drivers/gpu/drm/drm_atomic.c
> > >>> index 895741e9cd7d..b322cbed319b 100644
> > >>> --- a/drivers/gpu/drm/drm_atomic.c
> > >>> +++ b/drivers/gpu/drm/drm_atomic.c
> > >>> @@ -799,6 +799,12 @@ 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->color_encoding_property) {
> > >>>  	
> > >>>  		state->color_encoding = val;
> > >>>  	
> > >>>  	} else if (property == plane->color_range_property) {
> > >>> 
> > >>> @@ -864,6 +870,12 @@ 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->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..12fed2ff65c8 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,97 @@ 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_FOREGROUND_CLIP] = "foreground-clip",
> > >>> +};
> > >>> +
> > >>> +/**
> > >>> + * 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 by these properties:
> > >>> + *
> > >>> + * colorkey.mode:
> > >>> + *	The mode is an enumerated property that controls how color keying
> > >>> + *	operates.
> > >>> + *
> > >>> + * 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.
> > >>> + *
> > >>> + *	When a single color key is desired instead of a range, userspace
> > >>> shall
> > >>> + *	set the min and max properties to the same value.
> > >>> + *
> > >>> + *	Drivers return an error from their plane atomic check if range
> > >>> can't be
> > >>> + *	handled.
> > >>> + *
> > >>> + * Returns:
> > >>> + * Zero on success, negative errno on failure.
> > >>> + */
> > >>> +int drm_plane_create_colorkey_properties(struct drm_plane *plane,
> > >>> +					 u32 supported_modes)
> > >>> +{
> > >>> +	struct drm_prop_enum_list 
modes_list[DRM_PLANE_COLORKEY_MODES_NUM];
> > >>> +	struct drm_property *mode_prop;
> > >>> +	struct drm_property *min_prop;
> > >>> +	struct drm_property *max_prop;
> > >>> +	unsigned int modes_num = 0;
> > >>> +	unsigned int i;
> > >>> +
> > >>> +	/* modes are driver-specific, build the list of supported modes 
*/
> > >>> +	for (i = 0; i < DRM_PLANE_COLORKEY_MODES_NUM; i++) {
> > >>> +		if (!(supported_modes & BIT(i)))
> > >>> +			continue;
> > >>> +
> > >>> +		modes_list[modes_num].name = plane_colorkey_mode_name[i];
> > >>> +		modes_list[modes_num].type = i;
> > >>> +		modes_num++;
> > >>> +	}
> > >>> +
> > >>> +	/* at least one mode should be supported */
> > >>> +	if (!modes_num)
> > >>> +		return -EINVAL;
> > >>> +
> > >>> +	mode_prop = drm_property_create_enum(plane->dev, 0, 
"colorkey.mode",
> > >>> +					     modes_list, modes_num);
> > >>> +	if (!mode_prop)
> > >>> +		return -ENOMEM;
> > >>> +
> > >>> +	min_prop = drm_property_create_range(plane->dev, 0, 
"colorkey.min",
> > >>> +					     0, U64_MAX);
> > >>> +	if (!min_prop)
> > >>> +		goto err_destroy_mode_prop;
> > >>> +
> > >>> +	max_prop = drm_property_create_range(plane->dev, 0, 
"colorkey.max",
> > >>> +					     0, U64_MAX);
> > >>> +	if (!max_prop)
> > >>> +		goto err_destroy_min_prop;
> > >>> +
> > >>> +	drm_object_attach_property(&plane->base, mode_prop, 0);
> > >>> +	drm_object_attach_property(&plane->base, min_prop, 0);
> > >>> +	drm_object_attach_property(&plane->base, max_prop, 0);
> > >>> +
> > >>> +	plane->colorkey.mode_property = mode_prop;
> > >>> +	plane->colorkey.min_property = min_prop;
> > >>> +	plane->colorkey.max_property = max_prop;
> > >>> +
> > >>> +	return 0;
> > >>> +
> > >>> +err_destroy_min_prop:
> > >>> +	drm_property_destroy(plane->dev, min_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..9a621e1ccc47 100644
> > >>> --- a/include/drm/drm_plane.h
> > >>> +++ b/include/drm/drm_plane.h
> > >>> @@ -32,6 +32,48 @@ struct drm_crtc;
> > >>> 
> > >>>  struct drm_printer;
> > >>>  struct drm_modeset_acquire_ctx;
> > >>> 
> > >>> +/**
> > >>> + * enum drm_plane_colorkey_mode - uapi plane colorkey mode
> > >>> enumeration
> > >>> + */
> > >>> +enum drm_plane_colorkey_mode {
> > >>> +	/**
> > >>> +	 * @DRM_PLANE_COLORKEY_MODE_DISABLED:
> > >>> +	 *
> > >>> +	 * No color matching performed in this mode.
> > >>> +	 */
> > >>> +	DRM_PLANE_COLORKEY_MODE_DISABLED,
> > >>> +
> > >>> +	/**
> > >>> +	 * @DRM_PLANE_COLORKEY_MODE_FOREGROUND_CLIP:
> > >>> +	 *
> > >>> +	 * This mode is also known as a "green screen". Plane pixels are
> > >>> +	 * transparent in areas where pixels match a given color key 
range
> > >>> +	 * and there is a bottom (background) plane, in other cases plane
> > >>> +	 * pixels are unaffected.
> > >>> +	 *
> > >>> +	 */
> > >>> +	DRM_PLANE_COLORKEY_MODE_FOREGROUND_CLIP,
> > >> 
> > >> Could we add background clip as well?
> > 
> > Sure, but I think adding a new mode should be a distinct change made on
> > top of the initial series.
> > 
> > > Also could we just name them "src" and "dst" (or some variation of
> > > those). I'm betting no one has any kind of idea what these proposed
> > > names mean without looking up the docs, whereas pretty much everyone
> > > knows immediately what src/dst colorkeying means.
> > 
> > Okay, I'll rename the mode to DRM_PLANE_COLORKEY_MODE_SRC in the next
> > revision.> 
> > >> Would be nice if we could map i915's legacy ioctl handler to the new
> > >> color key mode.> >> 
> > >>> +	/**
> > >>> +	 * @DRM_PLANE_COLORKEY_MODES_NUM:
> > >>> +	 *
> > >>> +	 * Total number of color keying modes.
> > >>> +	 */
> > >>> +	DRM_PLANE_COLORKEY_MODES_NUM,
> > >>> +};
> > >>> +
> > >>> +/**
> > >>> + * struct drm_plane_colorkey_state - plane color keying state
> > >>> + * @colorkey.mode: color keying mode
> > >>> + * @colorkey.min: color key range minimum (in ARGB16161616 format)
> > >>> + * @colorkey.max: color key range maximum (in ARGB16161616 format)
> > >>> + */
> > >>> +struct drm_plane_colorkey_state {
> > >>> +	enum drm_plane_colorkey_mode mode;
> > >>> +	u64 min;
> > >>> +	u64 max;
> > >>> +};
> > >> 
> > >> Could we have some macros to extract the components for min/max?
> > >> A, R, G, B.
> > 
> > I'll add the macros in the next revision.
> > 
> > > And where did we lose the value+mask?
> > 
> > There is no use for the mask on Tegra. I'd prefer to keep initial patches
> > simple and minimal, other modes and additional properties could be added
> > in the further patches on by as-needed basis. Mask could be added later
> > with the default value of 0xffffffffffffffff. Does it sound good for you?
> 
> IIRC my earlier idea was to have different colorkey modes for the
> min+max and value+mask modes. That way userspace might actually have
> some chance of figuring out which bits of state actually do something.
> Although for Intel hw I think the general rule is that min+max for YUV,
> value+mask for RGB, so it's still not 100% clear what to pick if the
> plane supports both.
> 
> I guess one alternative would be to have min+max only, and the driver
> would reject 'min != max' if it only uses a single value?
> 

You should pick both and reject unsupported property values based on the 
planes framebuffer format. So it will be possible to set unsupported values 
while plane is disabled because it doesn't have an associated framebuffer and 
then atomic check will fail to enable plane if property values are invalid for 
the given format.

> And maybe we should have the mask always? IIRC Intel hw generally has a
> one bit enable/disable "mask" per channel in the min+max mode (I think
> there's one exception where it has only a 1 bit mask in the value+mask
> mode as well). So we could accept 0 and 0xffff mask values in this case
> and reject everything else.

Sounds good to me. Actually there is a place for the mask on Tegra, older 
generation doesn't support matching of the alpha channel and the alpha channel 
matching is simply ignored, so colorkeying could be support for the 
framebuffer formats that have an alpha channel if the alpha channel matching 
is disabled by the mask value.

> The alternative might be to enable the
> keying for the channel if 'min <= max' and disable it if 'min > max'.
> But not sure if that's slightly too magicy.

The mask property should suit well your case, I don't think there is any need 
for the magic.

Powered by blists - more mailing lists