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

[Pekka Paalanen]

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?

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?

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?


Thanks,
pq

Attachment: pgpsDyQrDWB6A.pgp
Description: OpenPGP digital signature