On 2021-07-12 4:03 a.m., Pekka Paalanen wrote:
> On Fri, 9 Jul 2021 18:23:26 +0200
> Raphael Gallais-Pou <raphael.gallais-pou@xxxxxxxxxxx> wrote:
>
>> 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 ?
>
> Hi,
>
> if you mean DRM_FORMAT_ARGB16161616F then that is not what you are
> using right now. That is a floating-point format using 16-bit floats
> (half float). It has only 10 bits precision IIRC.
>
> As C compilers do not(?) have built-in support for halfs, using this
> format would be inconvenient for userspace (and the kernel?). Since
> it's just for one pixel value, I think using a format that is
> convenient to craft would be good.
>
>
>>> 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.
>
> Right now, I would suggest to just dodge the whole question: define the
> background color to be opaque. Either drop the alpha channel, or
> specify that alpha must be 1.0 for now (fail ioctl if not).
>
> Let the people who actually need alpha in the background color figure
> out all the details. They would know what they want, while we don't. We
> also can't come up with a proper userspace for non-opaque alpha to
> prove that the design works.
>
> If you specify that alpha channel exists but must be 1.0, then someone
> else could later add another property that defines how the alpha would
> be used if it was less than 1.0. The existence of that other property
> would then tell userspace that non-1.0 alpha is supported and also
> define what it does. Userspace that does not understand that will just
> keep using alpha 1.0, meaning it doesn't matter what value the other
> new property has. So this seems quite future-proof to me.
>
>>> 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 ?
>
> Yes, but with the inconvenience I mentioned.
>
> This is a genuine question though, would anyone actually need
> background color values > 1.0. I don't know of any case yet where it
> would be required. It would imply that plane blending happens in a
> color space where >1.0 values are meaningful. I'm not even sure if any
> hardware supporting that exists.
>
> Maybe it would be best to assume that only [0.0, 1.0] pixel value range
> is useful, and mention in the commit message that if someone really
> needs values outside of that, they should create another background
> color property. Then, you can pick a simple unsigned integer pixel
> format, too. (I didn't see any 16 bit-per-channel formats like that in
> drm_fourcc.h though.)
>
I don't think we should artificially limit this to [0.0, 1.0]. As you
mentioned above when talking about full vs limited, the userspace
understands what's the correct value that fits the pipeline. If that
pipeline is FP16 with > 1.0 values then it would make sense that the
background color can be > 1.0.
Harry
>
> Thanks,
> pq
>
