Re: [PATCH 1/2] drm: add crtc background color property
From: Raphael Gallais-Pou
Date: Fri Jul 09 2021 - 12:23:52 EST
On 7/9/21 10:04 AM, Pekka Paalanen wrote:
On Wed, 7 Jul 2021 08:48:47 +0000
Raphael GALLAIS-POU - foss <raphael.gallais-pou@xxxxxxxxxxx> wrote:
Some display controllers can be programmed to present non-black colors
for pixels not covered by any plane (or pixels covered by the
transparent regions of higher planes). Compositors that want a UI with
a solid color background can potentially save memory bandwidth by
setting the CRTC background property and using smaller planes to display
the rest of the content.
To avoid confusion between different ways of encoding RGB data, we
define a standard 64-bit format that should be used for this property's
value. Helper functions and macros are provided to generate and dissect
values in this standard format with varying component precision values.
Signed-off-by: Raphael Gallais-Pou <raphael.gallais-pou@xxxxxxxxxxx>
Signed-off-by: Matt Roper <matthew.d.roper@xxxxxxxxx>
---
drivers/gpu/drm/drm_atomic_state_helper.c | 1 +
drivers/gpu/drm/drm_atomic_uapi.c | 4 +++
drivers/gpu/drm/drm_blend.c | 34 +++++++++++++++++++++--
drivers/gpu/drm/drm_mode_config.c | 6 ++++
include/drm/drm_blend.h | 1 +
include/drm/drm_crtc.h | 12 ++++++++
include/drm/drm_mode_config.h | 5 ++++
include/uapi/drm/drm_mode.h | 28 +++++++++++++++++++
8 files changed, 89 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_atomic_state_helper.c b/drivers/gpu/drm/drm_atomic_state_helper.c
index ddcf5c2c8e6a..1b95a4ecdb2b 100644
--- a/drivers/gpu/drm/drm_atomic_state_helper.c
+++ b/drivers/gpu/drm/drm_atomic_state_helper.c
@@ -72,6 +72,7 @@ __drm_atomic_helper_crtc_state_reset(struct drm_crtc_state *crtc_state,
struct drm_crtc *crtc)
{
crtc_state->crtc = crtc;
+ crtc_state->bgcolor = drm_argb(16, 0xffff, 0, 0, 0);
}
EXPORT_SYMBOL(__drm_atomic_helper_crtc_state_reset);
diff --git a/drivers/gpu/drm/drm_atomic_uapi.c b/drivers/gpu/drm/drm_atomic_uapi.c
index 438e9585b225..fea68f8f17f8 100644
--- a/drivers/gpu/drm/drm_atomic_uapi.c
+++ b/drivers/gpu/drm/drm_atomic_uapi.c
@@ -483,6 +483,8 @@ static int drm_atomic_crtc_set_property(struct drm_crtc *crtc,
set_out_fence_for_crtc(state->state, crtc, fence_ptr);
} else if (property == crtc->scaling_filter_property) {
state->scaling_filter = val;
+ } else if (property == config->bgcolor_property) {
+ state->bgcolor = val;
} else if (crtc->funcs->atomic_set_property) {
return crtc->funcs->atomic_set_property(crtc, state, property, val);
} else {
@@ -520,6 +522,8 @@ drm_atomic_crtc_get_property(struct drm_crtc *crtc,
*val = 0;
else if (property == crtc->scaling_filter_property)
*val = state->scaling_filter;
+ else if (property == config->bgcolor_property)
+ *val = state->bgcolor;
else if (crtc->funcs->atomic_get_property)
return crtc->funcs->atomic_get_property(crtc, state, property, val);
else
diff --git a/drivers/gpu/drm/drm_blend.c b/drivers/gpu/drm/drm_blend.c
index ec37cbfabb50..6692d6a6db22 100644
--- a/drivers/gpu/drm/drm_blend.c
+++ b/drivers/gpu/drm/drm_blend.c
@@ -186,8 +186,7 @@
* assumed to be 1.0
*
* 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).
+ * plane or the CRTC.
*
* SCALING_FILTER:
* Indicates scaling filter to be used for plane scaler
@@ -201,6 +200,21 @@
*
* Drivers can set up this property for a plane by calling
* drm_plane_create_scaling_filter_property
+ *
Hi,
Hi Pekka,
Many thanks for your feedback, your comments are taken into account for
a v2.
I assume the below block is the UAPI documentation of this new property
and that it would appear with the other UAPI docs.
+ * BACKGROUND_COLOR:
+ * Defines the ARGB color of a full-screen layer that exists below all
+ * planes. This color will be used for pixels not covered by any plane
+ * and may also be blended with plane contents as allowed by a plane's
+ * alpha values. The background color defaults to black, and is assumed
+ * to be black for drivers that do not expose this property.
All good up to here.
Although
+ * background color isn't a plane, it is assumed that the color provided
+ * here undergoes the same pipe-level degamma/CSC/gamma transformations
+ * that planes undergo.
This sounds to me like it refers to the per-plane degamma/csc/gamma
which are new properties in development. I believe you do not mean
that, but you mean the CRTC degamma/csc/gamma and everything else which
apply *after* the blending of planes. So the wording here would need
clarification.
Yes, I was not sure about this, but it is effectively the general CRTC
color correction which is applicable to the background color.
Note that the color value provided here includes
+ * an alpha channel...non-opaque background color values are allowed,
+ * but are generally only honored in special cases (e.g., when a memory
+ * writeback connector is in use).
This could be read as: if you use a non-opaque color value, it will
usually be completely ignored (and the background will be e.g. black
instead). Is that your intention?
I think a more useful definition would be that the alpha is used in
blending as always, but because we do not yet have physically
transparent monitors, the final alpha value may not reach the video
sink or the video sink may ignore it.
In our case, the hardware does not support alpha channel (as you can see
the DRM_ARGB_TO_LTDC_RGB24 macro in the second patch).
For chip vendors who does support this feature, the video sink would get
this transparency parameter. In the case where it is not, alpha channel
would be ignored.
+ *
+ * This property is setup with drm_crtc_add_bgcolor_property().
You forgot to document the value format of this property. The ARGB color
format needs to be defined at least to the same detail as all pixel
formats in drm_fourcc.h are. If there is a suitable DRM_FORMAT_*
definition already, simply saying the color is in that format would be
enough.
Will do ! :)
I was thinking about the FourCC AR4H format. Have you something else in
mind ?
Another thing to document is whether this color value is alpha
pre-multiplied or not. Planes can have the property "pixel blend mode",
but because the background color is not on a plane, that property
cannot apply here.
The difference it makes is that if background color is both non-opaque
and pre-multiplied, then the question arises what pixel values will
actually be transmitted to video sink for the background. Will the
alpha pre-multiplication be undone?
Btw. note that "pixel blend mode" property does not document the use of
background alpha at all. So if the background color can have non-opaque
alpha, then you need to document the behavior in "pixel blend mode". It
also does not document what alpha value will result from blending, for
blending the next plane.
Those are questions that did not crossed my mind at all.
What would you suggest ? Instinctively I would say that in the case
where there is a non-opaque background color,
alpha pre-multiplication would not be taken into account, although it is
maybe not the best solution.
As I am not quite sure, I will lookup for this.
The question about full vs. limited range seems unnecessary to me, as
the background color will be used as-is in the blending stage, so
userspace can just program the correct value that fits the pipeline it
is setting up.
One more question is, as HDR exists, could we need background colors
with component values greater than 1.0?
AR4H color format should cover that case, isn't it ?
Regards,
Raphaël
Scanout of FP16 formats exists.
*/
Thanks,
pq