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

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 5526 bytes --]

On Tue, 13 Jul 2021 09:54:35 -0400
Harry Wentland <harry.wentland@amd.com> 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@amd.com> 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@foss.st.com> 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@foss.st.com> 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@foss.st.com>
> >>>>>> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>
> >>>>>> ---
> >>>>>>   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...


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]