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

[Jessica Zhang]

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.

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.

> 
> 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.

> 
> Whether values outside of the unit range will ever be needed depends
> also on the userspace design. Userspace could choose the value range
> freely if the KMS color pipeline elements happen to support that range.
> However things like LUTs are naturally limited to unit range input, so
> using values outside of the unit range might be difficult if not
> impossible. Therefore I'm not concerned about this question much.
> 
>>> So, there are multiple ways to go about this. I can think of:
>>>
>>> - Put "RGBA32" in the name of the prop, and if we ever need a different
>>>    color format, pick a different name.
> 
> I could see problems with that if the same device supports more than
> one kind. How to turn off all but one color fill property?
> 
> What if userspace understands an old color fill property but not the
> new one, and the new one happens to be set for some reason? Userspace
> will attempt to use the old property without setting the new property
> to nil, and fail.
> 
>>> - Define a struct with an enum of possible fill kinds:
>>>    #define FILL_COLOR_RGBA32 1
>>>    #define FILL_COLOR_F32 2
>>>    struct color_fill_blob { u32 kind; u8 data[]; };
> 
> This could work.
> 
> Btw. maybe call it RGBA32323232 to make it follow the drm_fourcc naming
> convention. Some userspace libraries already use RGBA32 to mean 32 bit
> per pixel instead of per channel.
> 
> How will userspace know what kinds are supported?
> 
>>> - 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.

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