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

[Jessica Zhang]

On 10/29/2022 5:08 AM, Dmitry Baryshkov 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.

Hey Dmitry,

Good point. Forgot to add this in the patch [3/3] commit message, but 
FWIW MSM DPU devices only support the RGB format variants [1].

[1] 
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c#L697

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

Hm... I can definitely change color_fill to use u64 instead. As for 
making it more extensible, do you have any suggestions?

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

Just a question about this blob struct -- what is the purpose of pixel_data?

At least for devices using the DPU driver, the only data needed to 
enable solid fill is color_fill and color_fill_format. I'd also be 
interested in knowing if there are other drivers support a similar 
feature and what is needed for them.

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

JFYI we did originally consider using a custom 1x1 FB to for color fill 
[1], but decided to go with a plane property instead. IIRC the 
conclusion was that having color fill as a plane property is a cleaner 
solution.

As for creating a new blob struct to hold all color fill info, I'm open 
to implementing that over having 2 separate properties.

[1] https://oftc.irclog.whitequark.org/dri-devel/2022-09-23#31409842

> 
> Another approach might be using a format modifier instead of the FB flag.
I like the idea of having a format modifier denoting if the driver 
supports color_fill for that specific format. This would allow userspace 
to know which formats are supported by solid fill planes.

Thanks,

Jessica Zhang

> 
> What do you think?
> 
> -- 
> With best wishes
> Dmitry
>