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

[Simon Ser]

cc'ing Pekka and wayland-devel for userspace devs feedback on the new uAPI.

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.

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?

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.

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.
- 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[]; };
- 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.
- 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.

[1]: https://gitlab.freedesktop.org/wayland/wayland-protocols/-/merge_requests/104