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

[Pekka Paalanen]

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.


Thanks,
pq

Attachment: pgpgxTx0Cnbnw.pgp
Description: OpenPGP digital signature