Re: [PATCH 1/2] drm: add crtc background color property

[Harry Wentland]

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
>