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

[Harry Wentland]

On 2021-07-15 5:34 a.m., Pekka Paalanen wrote:
> On Wed, 14 Jul 2021 12:13:58 -0400
> Harry Wentland <harry.wentland@xxxxxxx> wrote:
> 
>> On 2021-07-14 3:35 a.m., Pekka Paalanen wrote:
>>> On Tue, 13 Jul 2021 09:54:35 -0400
>>> Harry Wentland <harry.wentland@xxxxxxx> wrote:
>>>   
>>>> On 2021-07-13 3:52 a.m., Pekka Paalanen wrote:  
>>>>> On Mon, 12 Jul 2021 12:15:59 -0400
>>>>> Harry Wentland <harry.wentland@xxxxxxx> wrote:
>>>>>     
>>>>>> 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(-)    
>>>>>
>>>>> ...
>>>>>     
>>>>>>>>> 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.    
>>>>>
>>>>> Ok. The standard FP32 format then for ease of use and guaranteed enough
>>>>> range and precision for far into the future?
>>>>>     
>>>>
>>>> I don't have a strong preference for FP16 vs FP32. My understanding is
>>>> that FP16 is enough to represent linearly encoded data in a way that
>>>> looks smooth to humans.
>>>>
>>>> scRGB uses FP16 with linear encoding in a range of [-0.5, 7.4999].
>>>>  
>>>>> Or do you want to keep it in 64 bits total, so the UABI can pack
>>>>> everything into a u64 instead of needing to create a blob?
>>>>>
>>>>> I don't mind as long as it's clearly documented what it is and how it
>>>>> works, and it carries enough precision.
>>>>>
>>>>> But FP16 with its 10 bits of precision might be too little for integer
>>>>> 12-16 bpc pipelines and sinks?  
>>>
>>> The 10 bits worries me still.
>>>
>>> If you have a pipeline that works in [0.0, 1.0] range only, then FP16
>>> limits precision to 10 bits (in the upper half of the range?).
>>>   
>>>>>
>>>>> If the values can go beyond [0.0, 1.0] range, then does the blending
>>>>> hardware and the degamma/ctm/gamma coming afterwards cope with them, or
>>>>> do they get clamped anyway?
>>>>>     
>>>>
>>>> That probably depends on the HW and how it's configured. AMD HW can handle
>>>> values above and below [0.0, 1.0].  
>>>
>>> Right, so how would userspace know what will happen?
>>>
>>> Or do we need to specify that while values outside that unit range are
>>> expressable, it is hardware-specific on how they will behave, so
>>> generic userspace should not attempt to use values outside of the unit
>>> range?
>>>
>>> I guess this caveat should be documented for everything, not just for
>>> background color? LUT inputs and outputs, CTM input and output ranges,
>>> FB formats...
>>>   
>>
>> I'm not sure we should artificially limit the interface at this point, or
>> document hypotheticals. At this point I don't even know whether going beyond
>> [0.0, 1.0] would be a challenge for any HW that supports floating point
>> formats.
> 
> Exactly, we don't know. Yet we have to document how background color
> works. If background color can express values outside of [0.0, 1.0],
> the documentation must say something about it.
> 
> If there is no way to know, then documentation must say you don't know
> (or that it is hardware-specific, which to generic userspace is the
> same thing).
> 
> If userspace does not know what happens, then obviously it will avoid
> using values it does not know what happens with.
> 
> For example, let's say that blending can produce values outside of
> [0.0, 1.0]. The next step in the pipeline is DEGAMMA, which is a 1D
> LUT. How do you sample a 1D LUT with input values beyond [0.0, 1.0]? Do
> you clamp them to the unit range? Does the clamping still happen even
> when the LUT is in pass-through mode?
> 
> And in general, how big or how negative values will actually go through
> the pipeline?
> 
> Of course the background color property should not document everything
> above, but it must say something, like "The handling of values outside
> of [0.0, 1.0] depends on the capabilities of the hardware blending
> engine." That makes the handling unknown to generic userspace, but
> userspace drivers could make use of it.
> 
> The important bit is to understand that the background color values may
> sometimes (when?) not reach the sink unmodified even if userspace has
> configured the KMS pipeline to not modify them.
> 
> I would expect that values in [0.0, 1.0] have no problem passing
> through the KMS pipeline unharmed, and there are obvious expectations
> about how a LUT or a CTM processes them. But as soon as values outside
> of that range are possible, a whole slew of questions arises. The
> documentation must not be silent, it must set expectations like "it's
> hardware specific" if that's what it is.
> 

Agreed. I think ultimately we don't know because we haven't gotten to
use-cases like that.

I'm fine with documentation stating "The handling of values outside
of [0.0, 1.0] depends on the capabilities of the hardware blending
engine." or "Handling of values outside [0.0, 1.0] is currently
undefined."

Harry

> 
> Thanks,
> pq
>