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

[Jessica Zhang]

On 10/31/2022 5:11 PM, Dmitry Baryshkov wrote:
> Hi,
> 
> On 01/11/2022 01:24, Jessica Zhang wrote:
>>
>>
>> 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
> 
> Ack. So you'd need to tell this to userspace.
> 
>>
>>>
>>> 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?
> 
> No. Not u64. It is a blob. Basically because when designing API you can 
> not guarantee that all fill values would fit into u64. Also see below.
> 
>>
>>>
>>> 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.
> 
> Yes. You are thinking from the DPU point of view. ARGB only. However as 
> we are adding generic API, we should not limit ourselves to it. Other 
> deivces might support other formats of fill data. For example using 
> YUY2/UYVY for filling the plane. And such YUV data is not a colour 
> anymore. It is a pixel data, just as I named it.
> 
> Another hardware might support some fill patterns. Or e.g. passing a 
> compressed texels/macrotiles. So, pixel data. Note, I've added format 
> modifiers. Maybe `u64 modifiers[4]` is an overkill, as we have just a 
> single data plane. Maybe just `u64 modifier` would be enough.

Got it, I think that's reasonable then.

> 
>>
>>>
>>> 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
> 
> Let me cite the conclusion form the IRC chat: `22:20 <robclark> 
> abhinav__: kinda.. the proposal was that userspace creates a blob 
> property with the solid fill color, and then attaches the blob-prop id 
> to the plane's FB_ID`.
> 
> It's not a pair of properties. It is a blob, because it is not that 
> limited as the pair of range properties is.
> 
> I will reread the log later, but just my 2c. Attaching the blob property 
> as the FB_ID might confuse userspace. I'd be slightly biased to being 
> more conservative here. However as the final proposal was to attach the 
> blob ID, let's do it this way.

Sounds good. Will spin up a v2 with the prop_blob implementation.

Thanks,

Jessica Zhang

> 
>>
>>>
>>> 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.
> 
> Yes, exactly. It would come in a natural way.
> 
> [Rumbling: and then it's natural to have the fill data in FB. Dull mode 
> off.]
> 
> -- 
> With best wishes
> Dmitry
>