| Message ID | 20180807172202.1961-2-digetx@gmail.com |
|---|---|
| State | Deferred |
| Headers | show |
| Series | drm: Add generic colorkey plane properties | expand |
On Tue, Aug 07, 2018 at 08:22:01PM +0300, Dmitry Osipenko wrote: > + * 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. ... > + /** > + * @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. > + * In areas where two or more source planes overlap, the topmost > + * plane takes precedence. > + */ This seems confusing to me. What you seem to be saying is that the "destination" plane would be the one which is (eg0 the graphic plane, and the "source" plane would be the the plane containing (eg) the video. You seem to be saying that the colorkey matches the video and determines whether the pixels in the graphic plane are opaque or transparent. Surely that is the wrong way round - in video overlay, you want to colorkey match the contents of the graphic plane to determine which pixels from the video plane to overlay. If it's the other way around (source is the graphic, destination is the video) it makes less sense to use the "source" and "destination" terms, I can't see how you could describe a plane that is being overlaid on top of another plane as a "destination". I guess the terminology has come from a thought about using a GPU to physically do the colorkeying when combining two planes - if the GPU were to write to the "destination" plane, then this would be the wrong way around. For starters, taking the above example, the video plane may well be smaller than the graphic plane. If it's the other way around, that has other problems, like destroying the colorkey in the graphic plane when writing the video plane's contents to it. So, in summary, I don't think "destination" and "source" are particularly good terms to describe the operation, and I think you have them swapped in your description of "DRM_PLANE_COLORKEY_MODE_TRANSPARENT".
On Wednesday, 8 August 2018 11:16:09 MSK Russell King - ARM Linux wrote: > On Tue, Aug 07, 2018 at 08:22:01PM +0300, Dmitry Osipenko wrote: > > + * 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. > > ... > > > + /** > > + * @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. > > + * In areas where two or more source planes overlap, the topmost > > + * plane takes precedence. > > + */ > > This seems confusing to me. > > What you seem to be saying is that the "destination" plane would be the > one which is (eg0 the graphic plane, and the "source" plane would be the > the plane containing (eg) the video. You seem to be saying that the > colorkey matches the video and determines whether the pixels in the > graphic plane are opaque or transparent. Your example is correct. With the "plane_mask" property we can specify any plane as the "source" for color key, so it can been either a video plane or graphic plane and even both at the same time. > Surely that is the wrong way round - in video overlay, you want to > colorkey match the contents of the graphic plane to determine which > pixels from the video plane to overlay. The "transparent" mode makes the color-matched pixels to become transparent, you want the inversion effect and hence that should be called something like a "transparent-inverted" mode. Maarten Lankhorst was asking for that mode in his comment to v3, I'm leaving for somebody else to add that mode later since there is no real use for it on Tegra right now. So in your case the graphic plane will be the "source" plane (specified via the colorkey.plane_mask property), video plane will be the "destination" plane (plane to which the colorkey properties are applied) and the colorkey.mode will be "transparent-inverted". Pixels of the "source" plane are being matched and "destination" plane takes the effect of color keying operation, i.e. the color-matched pixels of graphic plane leave the video plane pixels unaffected and the unmatched pixels make the video plane pixels transparent. > If it's the other way around (source is the graphic, destination is the > video) it makes less sense to use the "source" and "destination" terms, > I can't see how you could describe a plane that is being overlaid on > top of another plane as a "destination". Tegra has a bit annoying limitations in regard to a reduced plane blending functionality when color keying is enabled. I found that the best variant to work around the limitations is to move the graphic plane on top of the video plane and to make the graphic plane to match itself. I.e. the matched pixels of graphic plane become transparent and hence poked by video plane. > I guess the terminology has come from a thought about using a GPU to > physically do the colorkeying when combining two planes - if the GPU > were to write to the "destination" plane, then this would be the wrong > way around. For starters, taking the above example, the video plane > may well be smaller than the graphic plane. If it's the other way > around, that has other problems, like destroying the colorkey in the > graphic plane when writing the video plane's contents to it. It all depends on a use-case scenario. It won't be easy for userspace to generalize the usage of color keying, at best the color keying interface could be generalized and then userspace may choose the best fitting variant based on available HW capabilities. > So, in summary, I don't think "destination" and "source" are > particularly good terms to describe the operation, and I think you have > them swapped in your description of > "DRM_PLANE_COLORKEY_MODE_TRANSPARENT". Maybe the DRM_PLANE_COLORKEY_MODE_TRANSPARENT should become DRM_PLANE_COLORKEY_MODE_TRANSPARENT_INVERTED? Any more opinions? -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@ideasonboard.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@ideasonboard.com> > Signed-off-by: Dmitry Osipenko <digetx@gmail.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 ? > + * 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
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@ideasonboard.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@ideasonboard.com> > > Signed-off-by: Dmitry Osipenko <digetx@gmail.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@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel
Op 08-08-18 om 16:30 schreef Dmitry Osipenko: > On Wednesday, 8 August 2018 11:16:09 MSK Russell King - ARM Linux wrote: >> On Tue, Aug 07, 2018 at 08:22:01PM +0300, Dmitry Osipenko wrote: >>> + * 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. >> ... >> >>> + /** >>> + * @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. >>> + * In areas where two or more source planes overlap, the topmost >>> + * plane takes precedence. >>> + */ >> This seems confusing to me. >> >> What you seem to be saying is that the "destination" plane would be the >> one which is (eg0 the graphic plane, and the "source" plane would be the >> the plane containing (eg) the video. You seem to be saying that the >> colorkey matches the video and determines whether the pixels in the >> graphic plane are opaque or transparent. > Your example is correct. > > With the "plane_mask" property we can specify any plane as the "source" for > color key, so it can been either a video plane or graphic plane and even both > at the same time. I'm not sure we should specify plane mask from userspace. Can't we make major loops? How do you want to handle those? >> Surely that is the wrong way round - in video overlay, you want to >> colorkey match the contents of the graphic plane to determine which >> pixels from the video plane to overlay. > The "transparent" mode makes the color-matched pixels to become transparent, > you want the inversion effect and hence that should be called something like a > "transparent-inverted" mode. Maarten Lankhorst was asking for that mode in his > comment to v3, I'm leaving for somebody else to add that mode later since > there is no real use for it on Tegra right now. I would like it to be described and included, so I can convert the existing intel_sprite_set_colorkey_ioctl to atomic. Then again, could transparent-inverted also be handled by setting transparent on the primary? > So in your case the graphic plane will be the "source" plane (specified via > the colorkey.plane_mask property), video plane will be the "destination" plane > (plane to which the colorkey properties are applied) and the colorkey.mode > will be "transparent-inverted". Pixels of the "source" plane are being matched > and "destination" plane takes the effect of color keying operation, i.e. the > color-matched pixels of graphic plane leave the video plane pixels unaffected > and the unmatched pixels make the video plane pixels transparent. > >> If it's the other way around (source is the graphic, destination is the >> video) it makes less sense to use the "source" and "destination" terms, >> I can't see how you could describe a plane that is being overlaid on >> top of another plane as a "destination". > Tegra has a bit annoying limitations in regard to a reduced plane blending > functionality when color keying is enabled. I found that the best variant to > work around the limitations is to move the graphic plane on top of the video > plane and to make the graphic plane to match itself. I.e. the matched pixels > of graphic plane become transparent and hence poked by video plane. > >> I guess the terminology has come from a thought about using a GPU to >> physically do the colorkeying when combining two planes - if the GPU >> were to write to the "destination" plane, then this would be the wrong >> way around. For starters, taking the above example, the video plane >> may well be smaller than the graphic plane. If it's the other way >> around, that has other problems, like destroying the colorkey in the >> graphic plane when writing the video plane's contents to it. > It all depends on a use-case scenario. It won't be easy for userspace to > generalize the usage of color keying, at best the color keying interface could > be generalized and then userspace may choose the best fitting variant based on > available HW capabilities. There's TEST_ONLY for a reason, though I guess it makes generic color keying slightly more invovled for userspace. :) >> So, in summary, I don't think "destination" and "source" are >> particularly good terms to describe the operation, and I think you have >> them swapped in your description of >> "DRM_PLANE_COLORKEY_MODE_TRANSPARENT". > Maybe the DRM_PLANE_COLORKEY_MODE_TRANSPARENT should become > DRM_PLANE_COLORKEY_MODE_TRANSPARENT_INVERTED? > > Any more opinions? > > >
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@ideasonboard.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@ideasonboard.com> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.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 >
On 8/16/18 2:42 PM, Maarten Lankhorst wrote: > Op 08-08-18 om 16:30 schreef Dmitry Osipenko: >> On Wednesday, 8 August 2018 11:16:09 MSK Russell King - ARM Linux wrote: >>> On Tue, Aug 07, 2018 at 08:22:01PM +0300, Dmitry Osipenko wrote: >>>> + * 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. >>> ... >>> >>>> + /** >>>> + * @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. >>>> + * In areas where two or more source planes overlap, the topmost >>>> + * plane takes precedence. >>>> + */ >>> This seems confusing to me. >>> >>> What you seem to be saying is that the "destination" plane would be the >>> one which is (eg0 the graphic plane, and the "source" plane would be the >>> the plane containing (eg) the video. You seem to be saying that the >>> colorkey matches the video and determines whether the pixels in the >>> graphic plane are opaque or transparent. >> Your example is correct. >> >> With the "plane_mask" property we can specify any plane as the "source" for >> color key, so it can been either a video plane or graphic plane and even both >> at the same time. > I'm not sure we should specify plane mask from userspace. It looks like a quite flexible approach. Do you have any other suggestions? > Can't we make major loops? How do you want to handle those? It's up to a specific driver to accept the mask or reject it. You could make a loop if HW allows to do that, I don't see what's the problem. >>> Surely that is the wrong way round - in video overlay, you want to >>> colorkey match the contents of the graphic plane to determine which >>> pixels from the video plane to overlay. >> The "transparent" mode makes the color-matched pixels to become transparent, >> you want the inversion effect and hence that should be called something like a >> "transparent-inverted" mode. Maarten Lankhorst was asking for that mode in his >> comment to v3, I'm leaving for somebody else to add that mode later since >> there is no real use for it on Tegra right now. > I would like it to be described and included, so I can convert the existing intel_sprite_set_colorkey_ioctl to atomic. Okay, I can add it. Though probably better to call that mode "opaque" rather than "transparent-inverted". > Then again, could transparent-inverted also be handled by setting transparent on the primary? If HW allows to do that, then yes. > >> So in your case the graphic plane will be the "source" plane (specified via >> the colorkey.plane_mask property), video plane will be the "destination" plane >> (plane to which the colorkey properties are applied) and the colorkey.mode >> will be "transparent-inverted". Pixels of the "source" plane are being matched >> and "destination" plane takes the effect of color keying operation, i.e. the >> color-matched pixels of graphic plane leave the video plane pixels unaffected >> and the unmatched pixels make the video plane pixels transparent. >> >>> If it's the other way around (source is the graphic, destination is the >>> video) it makes less sense to use the "source" and "destination" terms, >>> I can't see how you could describe a plane that is being overlaid on >>> top of another plane as a "destination". >> Tegra has a bit annoying limitations in regard to a reduced plane blending >> functionality when color keying is enabled. I found that the best variant to >> work around the limitations is to move the graphic plane on top of the video >> plane and to make the graphic plane to match itself. I.e. the matched pixels >> of graphic plane become transparent and hence poked by video plane. >> >>> I guess the terminology has come from a thought about using a GPU to >>> physically do the colorkeying when combining two planes - if the GPU >>> were to write to the "destination" plane, then this would be the wrong >>> way around. For starters, taking the above example, the video plane >>> may well be smaller than the graphic plane. If it's the other way >>> around, that has other problems, like destroying the colorkey in the >>> graphic plane when writing the video plane's contents to it. >> It all depends on a use-case scenario. It won't be easy for userspace to >> generalize the usage of color keying, at best the color keying interface could >> be generalized and then userspace may choose the best fitting variant based on >> available HW capabilities. > There's TEST_ONLY for a reason, though I guess it makes generic color keying slightly more invovled for userspace. :) It is also quite involved for kernel to present a non-standard feature as something generic, pleasing everyone in the same time.
On 8/14/18 1:38 PM, Daniel Vetter wrote: > 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@ideasonboard.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@ideasonboard.com> >>> Signed-off-by: Dmitry Osipenko <digetx@gmail.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. At best I can write couple simple tests, simply because Tegra can't do more than that. Since the reference userspace cannot > answer how the new uapi should work in all corner-cases (failures and > stuff like that). Okay.
diff --git a/drivers/gpu/drm/drm_atomic.c b/drivers/gpu/drm/drm_atomic.c index 3eb061e11e2e..e97364966f6d 100644 --- a/drivers/gpu/drm/drm_atomic.c +++ b/drivers/gpu/drm/drm_atomic.c @@ -904,6 +904,16 @@ 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.plane_mask_property) { + state->colorkey.plane_mask = val; + } else if (property == plane->colorkey.mode_property) { + state->colorkey.mode = val; + } else if (property == plane->colorkey.mask_property) { + state->colorkey.mask = 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) { @@ -972,6 +982,16 @@ 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.plane_mask_property) { + *val = state->colorkey.plane_mask; + } else if (property == plane->colorkey.mode_property) { + *val = state->colorkey.mode; + } else if (property == plane->colorkey.mask_property) { + *val = state->colorkey.mask; + } 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..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. + * + * colorkey.mode: + * The mode is an enumerated property that controls how color keying + * operates. + * + * 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. + */ +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_device *dev = plane->dev; + struct drm_property *plane_mask_prop; + struct drm_property *mode_prop; + struct drm_property *mask_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; + + plane_mask_prop = drm_property_create_range(dev, 0, + "colorkey.plane_mask", + 0, U32_MAX); + if (!plane_mask_prop) + return -ENOMEM; + + mode_prop = drm_property_create_enum(dev, 0, "colorkey.mode", + modes_list, modes_num); + if (!mode_prop) + goto err_destroy_plane_mask_prop; + + mask_prop = drm_property_create_range(dev, 0, "colorkey.mask", + 0, U64_MAX); + if (!mask_prop) + goto err_destroy_mode_prop; + + min_prop = drm_property_create_range(dev, 0, "colorkey.min", + 0, U64_MAX); + if (!min_prop) + goto err_destroy_mask_prop; + + max_prop = drm_property_create_range(dev, 0, "colorkey.max", + 0, U64_MAX); + if (!max_prop) + goto err_destroy_min_prop; + + drm_object_attach_property(&plane->base, plane_mask_prop, 0); + drm_object_attach_property(&plane->base, mode_prop, 0); + drm_object_attach_property(&plane->base, mask_prop, 0); + drm_object_attach_property(&plane->base, min_prop, 0); + drm_object_attach_property(&plane->base, max_prop, 0); + + plane->colorkey.plane_mask_property = plane_mask_prop; + plane->colorkey.mode_property = mode_prop; + plane->colorkey.mask_property = mask_prop; + plane->colorkey.min_property = min_prop; + plane->colorkey.max_property = max_prop; + + return 0; + +err_destroy_min_prop: + drm_property_destroy(dev, min_prop); +err_destroy_mask_prop: + drm_property_destroy(dev, mask_prop); +err_destroy_mode_prop: + drm_property_destroy(dev, mode_prop); +err_destroy_plane_mask_prop: + drm_property_destroy(dev, plane_mask_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 8a152dc16ea5..ab6a91e6b54e 100644 --- a/include/drm/drm_plane.h +++ b/include/drm/drm_plane.h @@ -25,6 +25,7 @@ #include <linux/list.h> #include <linux/ctype.h> +#include <linux/kernel.h> #include <drm/drm_mode_object.h> #include <drm/drm_color_mgmt.h> @@ -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 + */ +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_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. + * 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, +}; + +/** + * struct drm_plane_colorkey_state - plane color keying state + * @mode: color keying mode + * @plane_mask: source planes that participate in color key matching + * @mask: color key mask (in ARGB16161616 format) + * @min: color key range minimum (in ARGB16161616 format) + * @max: color key range maximum (in ARGB16161616 format) + */ +struct drm_plane_colorkey_state { + enum drm_plane_colorkey_mode mode; + u64 plane_mask; + u64 mask; + u64 min; + u64 max; +}; + /** * struct drm_plane_state - mutable plane state * @@ -148,6 +195,13 @@ struct drm_plane_state { */ unsigned int normalized_zpos; + /** + * @colorkey: + * Color keying of the plane. See drm_plane_create_colorkey_properties() + * for more details. + */ + struct drm_plane_colorkey_state colorkey; + /** * @color_encoding: * @@ -660,6 +714,19 @@ struct drm_plane { */ struct drm_property *rotation_property; + /** + * @colorkey: + * Optional color keying properties for this plane. See + * drm_plane_create_colorkey_properties(). + */ + struct { + struct drm_property *plane_mask_property; + struct drm_property *mode_property; + struct drm_property *mask_property; + struct drm_property *min_property; + struct drm_property *max_property; + } colorkey; + /** * @color_encoding_property: * @@ -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))) + +#define __DRM_CKEY_CLAMP(value, nbits) \ + min_t(u16, (value), (1 << (nbits)) - 1) #endif