[Freedreno] [RFC PATCH 1/3] drm: Introduce color fill properties for drm plane

[Pekka Paalanen]

On Wed, 23 Nov 2022 15:27:04 -0800
Jessica Zhang <quic_jesszhan at quicinc.com> wrote:

> On 11/9/2022 1:18 AM, Pekka Paalanen wrote:
> > On Tue, 8 Nov 2022 23:01:47 +0100
> > Sebastian Wick <sebastian.wick at redhat.com> wrote:
> >   
> >> On Tue, Nov 8, 2022 at 7:51 PM Simon Ser <contact at emersion.fr> wrote:  
> >>>
> >>> cc'ing Pekka and wayland-devel for userspace devs feedback on the new uAPI.  
> > 
> > Hi all,
> > 
> > thanks! Comments below.  
> 
> Thanks for the feedback!
> 
> >   
> >>>
> >>> On Saturday, October 29th, 2022 at 14:08, Dmitry Baryshkov <dmitry.baryshkov at linaro.org> wrote:
> >>>     
> >>>> On 29/10/2022 01:59, Jessica Zhang wrote:  
> >>>>> Add support for COLOR_FILL and COLOR_FILL_FORMAT properties for
> >>>>> drm_plane. In addition, add support for setting and getting the values
> >>>>> of these properties.
> >>>>>
> >>>>> COLOR_FILL represents the color fill of a plane while COLOR_FILL_FORMAT
> >>>>> represents the format of the color fill. Userspace can set enable solid
> >>>>> fill on a plane by assigning COLOR_FILL to a uint64_t value, assigning
> >>>>> the COLOR_FILL_FORMAT property to a uint32_t value, and setting the
> >>>>> framebuffer to NULL.
> >>>>>
> >>>>> Signed-off-by: Jessica Zhang <quic_jesszhan at quicinc.com>  
> >>>>
> >>>> Planes report supported formats using the drm_mode_getplane(). You'd
> >>>> also need to tell userspace, which formats are supported for color fill.
> >>>> I don't think one supports e.g. YV12.
> >>>>
> >>>> A bit of generic comment for the discussion (this is an RFC anyway).
> >>>> Using color_fill/color_fill_format properties sounds simple, but this
> >>>> might be not generic enough. Limiting color_fill to 32 bits would
> >>>> prevent anybody from using floating point formats (e.g.
> >>>> DRM_FORMAT_XRGB16161616F, 64-bit value). Yes, this can be solved with
> >>>> e.g. using 64-bit for the color_fill value, but then this doesn't sound
> >>>> extensible too much.
> >>>>
> >>>> So, a question for other hardware maintainers. Do we have hardware that
> >>>> supports such 'color filled' planes? Do we want to support format
> >>>> modifiers for filling color/data? Because what I have in mind is closer
> >>>> to the blob structure, which can then be used for filling the plane:
> >>>>
> >>>> struct color_fill_blob {
> >>>>       u32 pixel_format;
> >>>>       u64 modifiers4];
> >>>>       u32 pixel_data_size; // fixme: is this necessary?
> >>>>       u8 pixel_data[];
> >>>> };
> >>>>
> >>>> And then... This sounds a lot like a custom framebuffer.
> >>>>
> >>>> So, maybe what should we do instead is to add new DRM_MODE_FB_COLOR_FILL
> >>>> flag to the framebuffers, which would e.g. mean that the FB gets stamped
> >>>> all over the plane. This would also save us from changing if (!fb)
> >>>> checks all over the drm core.
> >>>>
> >>>> Another approach might be using a format modifier instead of the FB flag.
> >>>>
> >>>> What do you think?  
> >>>
> >>> First off, we only need to represent the value of a single pixel here. So I'm
> >>> not quite following why we need format modifiers. Format modifiers describe how
> >>> pixels are laid out in memory. Since there's a single pixel described, this
> >>> is non-sensical to me, the format modifier is always LINEAR.  
> > 
> > Agreed.
> >   
> >>>
> >>> Then, I can understand why putting the pixel_format in there is tempting to
> >>> guarantee future extensibility, but it also adds complexity. For instance, how
> >>> does user-space figure out which formats can be used for COLOR_FILL? Can
> >>> user-space use any format supported by the plane? What does it mean for
> >>> multi-planar formats? Do we really want the kernel to have conversion logic for
> >>> all existing formats? Do we need to also add a new read-only blob prop to
> >>> indicate supported COLOR_FILL formats?  
> 
> FWIW the formats supported by solid_fill wouldn't necessarily be all the 
> formats supported by the plane (ex. for msm/dpu, solid_fill only 
> supports all RGB color variants, though planes can normally support YUV 
> formats too).
> 
> That being said, I'm ok with having the solid_fill take in only 
> RGBA32323232 format based on the comments below.
> 
> > 
> > Right. This does not seem to require pixel formats at all.
> > 
> > The point of pixel formats is to be able to feed large amounts of data
> > as-is into hardware and avoid the CPU ever touching it. You do that
> > with DRM FBs pointing to suitably allocated hardware buffers. But here
> > we have exactly one pixel, which I imagine will always be read by the
> > CPU so the driver will convert it into a hardware-specific format and
> > program it; probably the driver will not create an internal DRM FB for
> > it. >
> > The above might also be a reason to not model this as a special-case
> > DRM FB in UAPI. Or, at least you need a whole new ioctl to create such
> > DRM FB to avoid the need to allocate e.g. a dumb buffer or a
> > GPU-specific buffer. >
> > What one does need is what Sebastian brought up: does it support alpha
> > or not?  
> Hmm, the drm_plane struct already supports an alpha property so it seems 
> a bit redundant to also have a separate alpha value in the solid fill color.

Hi Jessica,

that's a good point! - Assuming that if hardware supports fill with
alpha, it supports plane-alpha with real FBs as well.

> That being said, we could have it so that setting the alpha for the 
> solid_fill property will also change the value of the plane's alpha 
> property too.

No! Definitely not. That would be confusing.

One must not have properties that change the value of other
non-immutable properties. It would become a real mess to handle in
userspace and for backward compatibility. Just like the kernel must not
spontaneously change the value of a non-immutable property. (Some
mistakes exist already, and I think they cause userspace to need
exceptional code for them.)


> > 
> > Userspace would also be interested in the supported precision of the
> > values, but the hardware pixel component order is irrelevant because the
> > driver will always convert the one pixel with CPU anyway.
> > 
> > YUV vs. RGB is a another question. The KMS color pipeline is defined in
> > terms of RGBA as far as I know, and alpha-blending YUV values makes no
> > sense. So will there ever be any need to set an YUV fill? I have a hard
> > time imagining it.
> > 
> > If you do set an YUV fill, the KMS color pipeline most likely needs to
> > convert it to RGBA for blending, and then you need the plane properties
> > COLOR_ENCODING and COLOR_RANGE.
> > 
> > But why bother when userspace can convert that one pixel to RGBA itself
> > anyway?  
> 
> Noted, I think this is reasonable.
> 
> >   
> >>> We've recently-ish standardized a new Wayland protocol [1] which has the same
> >>> purpose as this new kernel uAPI. The conclusion there was that using 32-bit
> >>> values for each channel (R, G, B, A) would be enough for almost all use-cases.
> >>> The driver can convert these high-precision values to what the hardware expects.
> >>> The only concern was about sending values outside of the [0.0, 1.0] range,
> >>> which may have HDR use-cases.  
> > 
> > This is what I would suggest, yes. This representation has enough
> > precision to be future-proof, and the driver will be converting the
> > value anyway.
> > 
> > The question about values outside of the unit range is a good one, too.
> > With Wayland, we can simply add another request to set a value in
> > floating-point if that turns up necessary.
> > 
> > Whether that will ever be necessary is connected to how the DRM KMS
> > abstract color pipeline is modelled, and that you must define from the
> > beginning:
> > 
> > If DRM KMS gets color processing related plane properties like CTM,
> > GAMMA or DEGAMMA (they already exist for CRTC, and these have been
> > proposed for planes quite some time ago), does the fill color go
> > through all these operations, or will the fill color skip all these
> > operations and go straight to plane blending?  
> 
> The fill color would still go through color processing operations, 
> though FWIW the MSM driver doesn't support GAMMA/DEGAMMA.

That's ok. The important bit is to define what must happen *if* such
plane properties are exposed by a driver. If they are not exposed, no
problem.

Btw. I could easily expect disagreement between different hardware
here, so I think this part will need many eyes to review.

If hardware is hard-wired to feed the fill color straight to blending,
then if fill color UAPI is defined to go through per-plane color
processing, the driver needs to apply that color processing on the CPU
before programming the hardware.

If hardware allows processing the fill color through per-plane color
processing, but fill color UAPI is defined to feed straight to blending,
then the driver can simply ignore the per-plane color properties and
program pass-through to the hardware.

For userspace, I think the choice makes little difference. Userspace
can compensate for the choice the same way a driver would, except
userspace can perhaps use more precise calculation methods. OTOH, if
fill color is intended to match the color on a real FB on another
plane, not going through the exact same computations might cause error.
Whether that error is significant depends on the use case and is
impossible to say here and now.

The important bit is to make that choice and document it.

...

> >>> - Define a struct with a version and RGBA values:
> >>>    struct color_fill_blob { u32 version; u32 rgba[4]; };
> >>>    If we need to add more formats later, or new metadata:
> >>>    struct color_fill_blob2 { u32 version; /* new fields */ };
> >>>    where version must be set to 2.  
> > 
> > This could work.  
> 
> Leaning towards this option.

Yes, it seems the best to me too. Just rgb[3] rather than rgba[4],
given the discussion about the plane alpha property. Or even u32 r;
u32 g; u32 b; to avoid having to think about the index in code.


Thanks,
pq

> 
> Thanks,
> 
> Jessica Zhang
> 
> >   
> >>> - Define a struct with a "pixel_format" prop, but force user-space to use a
> >>>    fixed format for now. Later, if we need another format, add a new prop to
> >>>    advertise supported formats.
> >>> - More complicated solutions, e.g. advertise the list of supported formats from
> >>>    the start.  
> > 
> > Feels more complicated than necessary.
> > 
> > Anyway, the idea of creating a blob and then setting that into some KMS
> > plane property sounds a very good mechanism.
> >   
> >>>
> >>> [1]: https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/104
> >>>     
> >>
> >> Agreeing with most of what you said here. However, what's the idea
> >> behind a format anyway? The 4 values provided here are fed directly
> >> into the color pipeline which seems to define the color channels it's
> >> working on as RGBA (or doesn't define anything at all). The only
> >> reason I can think of is that hardware might support only ingesting
> >> values either in a format with high bit depth color channels and no
> >> alpha or a format with low bit depth color but with alpha, so choosing
> >> between the formats provides a real trade-off. Is that actually
> >> something hardware might be restricted to or do they all just support
> >> ingesting the color data with enough precision on every channel? >  
> > Right.
> > 
> > 
> > Thanks,
> > pq  

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <https://lists.freedesktop.org/archives/freedreno/attachments/20221124/d69bb7ce/attachment.sig>