drm: Add generic colorkey plane properties
Subsystems (5)
v4 cover drm: Add generic colorkey plane properties
Changes in v4:
1) In response to Ville's Syrjälä review comments:
- Added new "colorkey.plane_mask" property that specifies which planes shall participate in color key matching operation.
- Added short glossary-comment to drm_plane_create_colorkey_properties() that clarifies 'destination' / 'source' plane terms.
- Returned "colorkey.mask" property that was dropped from v3, looks like masking is quite common among different HW and hence makes more sense to have that property by default instead of having additional color keying modes.
- Changed the color keying mode name to "transparent".
2) In response to Maarten's Lankhorst review comments:
- Added a drm_colorkey_extract_component() helper which is supposed to commonize color key component value extraction code among DRM drivers.
3) In response to Russell's King review comments:
- The doc-comment to drm_plane_create_colorkey_properties() now explicitly states that "The converted value shall be *rounded up* to the nearest value". Hence userspace now knows what to expect when plane has a 1bpp format.
Please review, thanks.
v3: https://lists.freedesktop.org/archives/dri-devel/2018-June/179057.html
v2: https://lists.freedesktop.org/archives/dri-devel/2018-May/178408.html
v1: https://lists.freedesktop.org/archives/dri-devel/2017-December/160510.html
Dmitry Osipenko (1):
drm/tegra: plane: Add generic colorkey properties for older Tegra's
Laurent Pinchart (1):
drm: Add generic colorkey properties for display planes
drivers/gpu/drm/drm_atomic.c | 20 +++++ drivers/gpu/drm/drm_blend.c | 150 ++++++++++++++++++++++++++++++++ drivers/gpu/drm/tegra/dc.c | 25 ++++++ drivers/gpu/drm/tegra/dc.h | 7 ++ drivers/gpu/drm/tegra/plane.c | 156 ++++++++++++++++++++++++++++++++++ include/drm/drm_blend.h | 3 + include/drm/drm_plane.h | 91 ++++++++++++++++++++ 7 files changed, 452 insertions(+)
Signature · 1 line
2.18.0
RFC PATCH v4 1/2 drm: Add generic colorkey properties for display planes
From: Laurent Pinchart <[email protected]>
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.
--- 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(+)
drivers/gpu/drm/drm_atomic.c +20 -0
@@ -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) {
drivers/gpu/drm/drm_blend.c +150 -0
159 lines not on this page · raw message
include/drm/drm_blend.h +3 -0
@@ -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
include/drm/drm_plane.h +91 -0
120 lines not on this page · raw message
Signature · 1 line
2.18.0
Re: [RFC PATCH v4 1/2] drm: Add generic colorkey properties for display planes
Quoted 9 lines · Dmitry Osipenko
+ * 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.
...
include/drm/drm_plane.h struct drm_crtc; · line 55
Quoted 9 lines
include/drm/drm_plane.h +9
-0+/** +* @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".
Signature · 3 lines
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 13.8Mbps down 630kbps up According to speedtest.net: 13Mbps down 490kbps up
Re: [RFC PATCH v4 1/2] drm: Add generic colorkey properties for display planes
Quoted 11 lines · On Wednesday, 8 August 2018 11:16:09 MSK Russell King - ARM Linux wrote:
Quoted 9 lines · Dmitry Osipenko
+ * 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.
Quoted 8 lines
...
include/drm/drm_plane.h struct drm_crtc;
Quoted 6 lines
include/drm/drm_plane.h +6
-0+/** +* @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.
Quoted 11 lines
include/drm/drm_plane.h struct drm_crtc; · line 55
Quoted 3 lines
include/drm/drm_plane.h +3
-0+* 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.
Quoted 3 lines
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.
Quoted 4 lines
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.
Quoted 7 lines
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.
Quoted 4 lines
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?
Maarten Lankhorst
maintainerRe: [RFC PATCH v4 1/2] drm: Add generic colorkey properties for display planes
Op 08-08-18 om 16:30 schreef Dmitry Osipenko:
Quoted 30 lines · On Wednesday, 8 August 2018 11:16:09 MSK Russell King - ARM Linux wrote:
Quoted 10 lines · Dmitry Osipenko
Quoted 9 lines
+ * 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.Quoted 8 lines
...
include/drm/drm_plane.h struct drm_crtc;
Quoted 6 lines
include/drm/drm_plane.h +6
-0+/** +* @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.
Quoted 10 lines
include/drm/drm_plane.h struct drm_crtc; · line 55
Quoted 3 lines
include/drm/drm_plane.h +3
-0+* 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.
Quoted 5 lines
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?
Quoted 8 lines
Quoted 3 lines
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?
Quoted 25 lines
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.
Quoted 4 lines
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.
Quoted 7 lines
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.
Quoted 4 lines
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. :)
Quoted 11 lines
Quoted 4 lines
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_TRANSPARENTshould becomeDRM_PLANE_COLORKEY_MODE_TRANSPARENT_INVERTED?Any more opinions?
Re: [RFC PATCH v4 1/2] drm: Add generic colorkey properties for display planes
Quoted 31 lines · Maarten Lankhorst
Op 08-08-18 om 16:30 schreef Dmitry Osipenko:
Quoted 30 lines · On Wednesday, 8 August 2018 11:16:09 MSK Russell King - ARM Linux wrote:
Quoted 10 lines · Dmitry Osipenko
Quoted 9 lines
+ * 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.Quoted 8 lines
...
include/drm/drm_plane.h struct drm_crtc;
Quoted 6 lines
include/drm/drm_plane.h +6
-0+/** +* @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.
Quoted 10 lines
include/drm/drm_plane.h struct drm_crtc; · line 55
Quoted 3 lines
include/drm/drm_plane.h +3
-0+* 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.
Quoted 6 lines
Quoted 5 lines
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?
Quoted 1 line
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.
Quoted 9 lines
Quoted 8 lines
Quoted 3 lines
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_ioctlto atomic.
Okay, I can add it. Though probably better to call that mode "opaque" rather than "transparent-inverted".
Quoted 1 line
Then again, could transparent-inverted also be handled by setting transparent on the primary?
If HW allows to do that, then yes.
Quoted 26 lines
Quoted 26 lines
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.
Quoted 4 lines
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.
Quoted 7 lines
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.
Quoted 5 lines
Quoted 4 lines
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_ONLYfor 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.
Re: [RFC PATCH v4 1/2] drm: Add generic colorkey properties for display planes
In reply to Dmitry OsipenkoHi Dmitry,
Thank you for the patch.
Quoted 22 lines · On Tuesday, 7 August 2018 20:22:01 EEST Dmitry Osipenko wrote:
From: Laurent Pinchart <[email protected]>
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 <[email protected]>Signed-off-by Dmitry Osipenko <[email protected]>--- 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]
Quoted 45 lines
drivers/gpu/drm/drm_blend.c +0
-0@@ -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 withdrm_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).drivers/gpu/drm/drm_blend.c +12
-0@@ -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.
+ *
Quoted 8 lines
+ * 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 ?
Quoted 3 lines
+ * 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.
Quoted 21 lines
+ * 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
Quoted 8 lines
+ * 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]
Quoted 4 lines
include/drm/drm_plane.h +0
-0
[snip]
include/drm/drm_plane.h struct drm_crtc; · line 38
Quoted 7 lines
include/drm/drm_plane.h +3
-0@@ -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/ ?
include/drm/drm_plane.h struct drm_crtc; · line 43
Quoted 5 lines
include/drm/drm_plane.h +5
-0+enum drm_plane_colorkey_mode { +/** +* @DRM_PLANE_COLORKEY_MODE_DISABLED: +* +* No color matching performed in this mode.
Do you mean "No color keying" ?
include/drm/drm_plane.h struct drm_crtc; · line 52
Quoted 9 lines
include/drm/drm_plane.h +9
-0+*/ + 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.
include/drm/drm_plane.h struct drm_crtc; · line 63
Quoted 11 lines
include/drm/drm_plane.h +11
-0+* 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.
Quoted 1 line
include/drm/drm_plane.h +1
-0+};
[snip]
Quoted 16 lines
include/drm/drm_plane.h +11
-0@@ -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) \
include/drm/drm_plane.h static inline struct drm_plane *drm_plane_find(struct · line 870
Quoted 10 lines
include/drm/drm_plane.h +10
-0+ __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 ?
include/drm/drm_plane.h static inline struct drm_plane *drm_plane_find(struct drm_device *dev, · line 872
Quoted 2 lines
include/drm/drm_plane.h +2
-0+#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)
Quoted 1 line
include/drm/drm_plane.h +0
-0#endif
Signature · 10 lines
Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH v4 1/2] drm: Add generic colorkey properties for display planes
Quoted 89 lines · Laurent Pinchart
Hi Dmitry,
Thank you for the patch.
Quoted 22 lines · On Tuesday, 7 August 2018 20:22:01 EEST Dmitry Osipenko wrote:
From: Laurent Pinchart <[email protected]>
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 <[email protected]>Signed-off-by Dmitry Osipenko <[email protected]>--- 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]
Quoted 45 lines
drivers/gpu/drm/drm_blend.c +0
-0@@ -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 withdrm_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).drivers/gpu/drm/drm_blend.c +12
-0@@ -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.
+ *Quoted 8 lines
+ * 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.
Quoted 3 lines
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
Quoted 164 lines
Quoted 4 lines
+ * colorkey.mode:
+ * The mode is an enumerated property that controls how color keying
+ * operates.A link to the
drm_plane_colorkey_modeenum documentation would be useful.Quoted 21 lines
+ * 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 shallQuoted 8 lines
+ * 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]
Quoted 4 lines
include/drm/drm_plane.h +0
-0[snip]
include/drm/drm_plane.h struct drm_crtc; · line 38
Quoted 7 lines
include/drm/drm_plane.h +3
-0@@ -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/?include/drm/drm_plane.h struct drm_crtc; · line 43
Quoted 5 lines
include/drm/drm_plane.h +5
-0+enum drm_plane_colorkey_mode { +/** +* @DRM_PLANE_COLORKEY_MODE_DISABLED: +* +* No color matching performed in this mode. Do you mean "No color keying" ?
include/drm/drm_plane.h struct drm_crtc; · line 52
Quoted 9 lines
include/drm/drm_plane.h +9
-0+*/ + 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.
include/drm/drm_plane.h struct drm_crtc; · line 63
Quoted 11 lines
include/drm/drm_plane.h +11
-0+* 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.
Quoted 1 line
include/drm/drm_plane.h +1
-0+};[snip]
Quoted 16 lines
include/drm/drm_plane.h +11
-0@@ -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) \include/drm/drm_plane.h static inline struct drm_plane *drm_plane_find(struct · line 870
Quoted 10 lines
include/drm/drm_plane.h +10
-0+ __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 ?
include/drm/drm_plane.h static inline struct drm_plane *drm_plane_find(struct drm_device *dev, · line 872
Quoted 2 lines
include/drm/drm_plane.h +2
-0+#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)Quoted 1 line
include/drm/drm_plane.h +0
-0#endif Signature · 10 lines
Regards, Laurent Pinchart _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Signature · 7 lines
Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [RFC PATCH v4 1/2] drm: Add generic colorkey properties for display planes
Quoted 90 lines · Daniel Vetter
Quoted 89 lines · Laurent Pinchart
Hi Dmitry,
Thank you for the patch.
Quoted 22 lines · On Tuesday, 7 August 2018 20:22:01 EEST Dmitry Osipenko wrote:
From: Laurent Pinchart <[email protected]>
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 <[email protected]>Signed-off-by Dmitry Osipenko <[email protected]>--- 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]
Quoted 45 lines
drivers/gpu/drm/drm_blend.c +0
-0@@ -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 withdrm_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).drivers/gpu/drm/drm_blend.c +12
-0@@ -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.
+ *Quoted 8 lines
+ * 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.
Quoted 7 lines
Quoted 3 lines
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
Quoted 2 lines
answer how the new uapi should work in all corner-cases (failures and stuff like that).
Okay.
Re: [RFC PATCH v4 1/2] drm: Add generic colorkey properties for display planes
In reply to Laurent PinchartHello Laurent,
Sorry for the late response.
Quoted 75 lines · Laurent Pinchart
Hi Dmitry,
Quoted 22 lines · Thank you for the patch. > On Tuesday, 7 August 2018 20:22:01 EEST Dmitry…
From: Laurent Pinchart <[email protected]>
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 <[email protected]>Signed-off-by Dmitry Osipenko <[email protected]>--- 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]
Quoted 47 lines
drivers/gpu/drm/drm_blend.c +0
-0@@ -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 withdrm_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).drivers/gpu/drm/drm_blend.c +12
-0@@ -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:
+ *
Quoted 8 lines
Quoted 6 lines
+ * 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.
Quoted 1 line
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.
Quoted 5 lines
Quoted 3 lines
+ * colorkey.mode:
+ * The mode is an enumerated property that controls how color keying
+ * operates.A link to the
drm_plane_colorkey_modeenum documentation would be useful.
Okay.
Quoted 30 lines
Quoted 21 lines
+ * 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 shallQuoted 8 lines
+ * 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.
+ */
Quoted 1 line
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"?
Quoted 19 lines
[snip]
Quoted 4 lines
include/drm/drm_plane.h +0
-0[snip]
include/drm/drm_plane.h struct drm_crtc; · line 38
Quoted 7 lines
include/drm/drm_plane.h +3
-0@@ -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/?
Quoted 7 lines
include/drm/drm_plane.h struct drm_crtc; · line 43
Quoted 5 lines
include/drm/drm_plane.h +5
-0+enum drm_plane_colorkey_mode { +/** +* @DRM_PLANE_COLORKEY_MODE_DISABLED: +* +* No color matching performed in this mode. Do you mean "No color keying" ?
Yes
Quoted 11 lines
include/drm/drm_plane.h struct drm_crtc; · line 52
Quoted 9 lines
include/drm/drm_plane.h +9
-0+*/ + 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.
Quoted 5 lines
Quoted 5 lines
include/drm/drm_plane.h +5
-0+* In areas where two or more source planes overlap, the topmost +* plane takes precedence. +*/ + DRM_PLANE_COLORKEY_MODE_TRANSPARENT, +
Quoted 8 lines
include/drm/drm_plane.h struct drm_crtc; · line 63
Quoted 6 lines
include/drm/drm_plane.h +6
-0+/** +* @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.
Quoted 33 lines
Quoted 1 line
include/drm/drm_plane.h +1
-0+};[snip]
Quoted 16 lines
include/drm/drm_plane.h +11
-0@@ -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) \include/drm/drm_plane.h static inline struct drm_plane *drm_plane_find(struct · line 870
Quoted 10 lines
include/drm/drm_plane.h +10
-0+ __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 ?
Quoted 4 lines
include/drm/drm_plane.h static inline struct drm_plane *drm_plane_find(struct drm_device *dev, · line 872
Quoted 2 lines
include/drm/drm_plane.h +2
-0+#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.
Quoted 15 lines
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)Quoted 1 line
include/drm/drm_plane.h +0
-0#endif
RFC PATCH v4 2/2 drm/tegra: plane: Add generic colorkey properties for older Tegra's
In reply to Dmitry OsipenkoFor the starter a minimal color keying support is implemented, which is enough to provide userspace like Opentegra Xorg driver with ability to support color keying by the XVideo extension. Blending controls interface changed on newer Tegra's, this patch provides color keying support for Tegra20/30/114 only.
--- drivers/gpu/drm/tegra/dc.c | 25 ++++++ drivers/gpu/drm/tegra/dc.h | 7 ++ drivers/gpu/drm/tegra/plane.c | 156 ++++++++++++++++++++++++++++++++++ 3 files changed, 188 insertions(+)
drivers/gpu/drm/tegra/dc.c +25 -0
@@ -162,6 +162,7 @@ static void tegra_plane_setup_blending_legacy(struct tegra_plane *plane)u32 foreground =BLEND_WEIGHT1 (255 ) |BLEND_WEIGHT0 (255 ) |BLEND_COLOR_KEY_NONE ;u32 blendnokey =BLEND_WEIGHT1 (255 ) |BLEND_WEIGHT0 (255 ); +enum drm_plane_colorkey_mode mode;struct tegra_plane_state *state;u32 blending[2 ];unsigned int i;
@@ -171,7 +172,15 @@ static void tegra_plane_setup_blending_legacy(struct tegra_plane *plane)tegra_plane_writel (plane, foreground,DC_WIN_BLEND_1WIN ); state =to_tegra_plane_state (plane->base.state); + mode = plane->base.state->colorkey.mode; +/* setup color keying */ +if (mode ==DRM_PLANE_COLORKEY_MODE_TRANSPARENT ) { +/* color key matched areas are transparent */ + foreground = background[0 ] |BLEND_COLOR_KEY_0 ; + } + +/* setup alpha blending */ if (state->opaque) {/* * Since custom fix-weight blending isn't utilized and weight
@@ -792,6 +801,11 @@ static struct drm_plane *tegra_primary_plane_create(struct drm_device *drm,dev_err (dc->dev,"failed to create rotation property: %d\n" , err); +if (dc->soc->has_legacy_blending) +drm_plane_create_colorkey_properties (&plane->base, +BIT (DRM_PLANE_COLORKEY_MODE_DISABLED ) | +BIT (DRM_PLANE_COLORKEY_MODE_TRANSPARENT )); +return &plane->base; }
@@ -1077,6 +1091,11 @@ static struct drm_plane *tegra_dc_overlay_plane_create(struct drm_device *drm,dev_err (dc->dev,"failed to create rotation property: %d\n" , err); +if (dc->soc->has_legacy_blending) +drm_plane_create_colorkey_properties (&plane->base, +BIT (DRM_PLANE_COLORKEY_MODE_DISABLED ) | +BIT (DRM_PLANE_COLORKEY_MODE_TRANSPARENT )); +return &plane->base; }
@@ -1187,6 +1206,7 @@ tegra_crtc_atomic_duplicate_state(struct drm_crtc *crtc) copy->pclk = state->pclk; copy->div = state->div; copy->planes = state->planes; + copy->ckey = state->ckey;return ©->base; }
@@ -1917,6 +1937,11 @@ static void tegra_crtc_atomic_flush(struct drm_crtc *crtc,struct tegra_dc *dc =to_tegra_dc (crtc);u32 value; +if (dc->soc->has_legacy_blending) { +tegra_dc_writel (dc, state->ckey.min,DC_DISP_COLOR_KEY0_LOWER ); +tegra_dc_writel (dc, state->ckey.max,DC_DISP_COLOR_KEY0_UPPER ); + } + value = state->planes <<8 |GENERAL_UPDATE ;tegra_dc_writel (dc, value,DC_CMD_STATE_CONTROL ); value =tegra_dc_readl (dc,DC_CMD_STATE_CONTROL );
drivers/gpu/drm/tegra/dc.h +7 -0
@@ -18,6 +18,11 @@struct tegra_output ; +struct tegra_dc_color_key_state { +u32 min; +u32 max; +}; +struct tegra_dc_state {struct drm_crtc_state base;
@@ -26,6 +31,8 @@ struct tegra_dc_state {unsigned int div;u32 planes; + +struct tegra_dc_color_key_state ckey; };static inline struct tegra_dc_state *to_dc_state (struct drm_crtc_state *state)
drivers/gpu/drm/tegra/plane.c +156 -0
167 lines not on this page · raw message
Signature · 1 line
2.18.0