Message-ID: <20180814103833.GN21634@phenom.ffwll.local>
Date:   Tue, 14 Aug 2018 12:38:33 +0200
From:   Daniel Vetter <daniel@...ll.ch>
To:     Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc:     Dmitry Osipenko <digetx@...il.com>,
        linux-renesas-soc@...r.kernel.org,
        Thomas Hellstrom <thellstrom@...are.com>,
        Laurent Pinchart <laurent.pinchart+renesas@...asonboard.com>,
        Neil Armstrong <narmstrong@...libre.com>,
        Russell King <linux@...linux.org.uk>,
        dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
        Paul Kocialkowski <paul.kocialkowski@...tlin.com>,
        linux-tegra@...r.kernel.org,
        Thierry Reding <thierry.reding@...il.com>,
        Ben Skeggs <bskeggs@...hat.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        Maxime Ripard <maxime.ripard@...e-electrons.com>,
        linux-media@...r.kernel.org
Subject: Re: [RFC PATCH v4 1/2] drm: Add generic colorkey properties for
 display planes

On Tue, Aug 14, 2018 at 12:48:08PM +0300, 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.
> 
> 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 ?

General comment: This is why we need the reference userspace. I also
think that any feature throwing up so many tricky questions should come
with a full set of igt testcases. Since the reference userspace cannot
answer how the new uapi should work in all corner-cases (failures and
stuff like that).
-Daniel

> 
> > + * 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
> 
> 
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@...ts.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Powered by blists - more mailing lists