[RFC PATCH 0/3] Support for Solid Fill Planes

[Jessica Zhang]

On 6/27/2023 12:58 AM, Pekka Paalanen wrote:
> On Mon, 26 Jun 2023 16:02:50 -0700
> Jessica Zhang <quic_jesszhan at quicinc.com> wrote:
> 
>> On 11/7/2022 11:37 AM, Ville Syrjälä wrote:
>>> On Fri, Oct 28, 2022 at 03:59:49PM -0700, Jessica Zhang wrote:
>>>> Introduce and add support for COLOR_FILL and COLOR_FILL_FORMAT
>>>> properties. When the color fill value is set, and the framebuffer is set
>>>> to NULL, memory fetch will be disabled.
>>>
>>> Thinking a bit more universally I wonder if there should be
>>> some kind of enum property:
>>>
>>> enum plane_pixel_source {
>>> 	FB,
>>> 	COLOR,
>>> 	LIVE_FOO,
>>> 	LIVE_BAR,
>>> 	...
>>> }
>>
>> Reviving this thread as this was the initial comment suggesting to
>> implement pixel_source as an enum.
>>
>> I think the issue with having pixel_source as an enum is how to decide
>> what counts as a NULL commit.
>>
>> Currently, setting the FB to NULL will disable the plane. So I'm
>> guessing we will extend that logic to "if there's no pixel_source set
>> for the plane, then it will be a NULL commit and disable the plane".
>>
>> In that case, the question then becomes when to set the pixel_source to
>> NONE. Because if we do that when setting a NULL FB (or NULL solid_fill
>> blob), it then forces userspace to set one property before the other.
> 
> Right, that won't work.
> 
> There is no ordering between each property being set inside a single
> atomic commit. They can all be applied to kernel-internal state
> theoretically simultaneously, or any arbitrary random order, and the
> end result must always be the same. Hence, setting one property cannot
> change the state of another mutable property. I believe that doing
> otherwise would make userspace fragile and hard to get right.
> 
> I guess there might be an exception to that rule when the same property
> is set multiple times in a single atomic commit; the last setting in
> the array prevails. That's universal and not a special-case between two
> specific properties.
> 
>> Because of that, I'm thinking of having pixel_source be represented by a
>> bitmask instead. That way, we will simply unset the corresponding
>> pixel_source bit when passing in a NULL FB/solid_fill blob. Then, in
>> order to detect whether a commit is NULL or has a valid pixel source, we
>> can just check if pixel_source == 0.
> 
> Sounds fine to me at first hand, but isn't there the enum property that
> says if the kernel must look at solid_fill blob *or* FB_ID?
> 
> If enum prop says "use solid_fill prop", the why would changes to FB_ID
> do anything? Is it for backwards-compatibility with KMS clients that do
> not know about the enum prop?
> 
> It seems like that kind of backwards-compatiblity will cause problems
> in trying to reason about the atomic state, as explained above, leading
> to very delicate and fragile conditions where things work intuitively.
> Hence, I'm not sure backwards-compatibility is wanted. This won't be
> the first or the last KMS property where an unexpected value left over
> will make old atomic KMS clients silently malfunction up to showing no
> recognisable picture at all. *If* that problem needs solving, there
> have been ideas floating around about resetting everything to nice
> values so that userspace can ignore what it does not understand. So far
> there has been no real interest in solving that problem in the kernel
> though.
> 
> Legacy non-atomic UAPI wrappers can do whatever they want, and program
> any (new) properties they want in order to implement the legacy
> expectations, so that does not seem to be a problem.

Hi Pekka and Dmitry,

After reading through both of your comments, I think I have a better 
understanding of the pixel_source implementation now.

So to summarize, we want to expose another property called 
"pixel_source" to userspace that will default to FB (as to not break 
legacy userspace).

If userspace wants to use solid fill planes, it will set both the 
solid_fill *and* pixel_source properties to a valid blob and COLOR 
respectively. If it wants to use FB, it will set FB_ID and pixel_source 
to a valid FB and FB.

Here's a table illustrating what I've described above:

+-----------------+-------------------------+-------------------------+
| Use Case        | Legacy Userspace        | solid_fill-aware        |
|                 |                         | Userspace               |
+=================+=========================+=========================+
| Valid FB        | pixel_source = FB       | pixel_source = FB       |
|                 | FB_ID = valid FB        | FB_ID = valid FB        |
+-----------------+-------------------------+-------------------------+
| Valid           | pixel_source = COLOR    | N/A                     |
| solid_fill blob | solid_fill = valid blob |                         |
+-----------------+-------------------------+-------------------------+
| NULL commit     | pixel_source = FB       | pixel_source = FB       |
|                 | FB_ID = NULL            | FB_ID = NULL            |
+-----------------+-------------------------+-------------------------+

Is there anything I'm missing or needs to be clarified?

Thanks,

Jessica Zhang

> 
> 
> Thanks,
> pq
> 
> 
>>
>> Would be interested in any feedback on this.
>>
>> Thanks,
>>
>> Jessica Zhang
>>
>>>    
>>>> In addition, loosen the NULL FB checks within the atomic commit callstack
>>>> to allow a NULL FB when color_fill is nonzero and add FB checks in
>>>> methods where the FB was previously assumed to be non-NULL.
>>>>
>>>> Finally, have the DPU driver use drm_plane_state.color_fill and
>>>> drm_plane_state.color_fill_format instead of dpu_plane_state.color_fill,
>>>> and add extra checks in the DPU atomic commit callstack to account for a
>>>> NULL FB in cases where color_fill is set.
>>>>
>>>> Some drivers support hardware that have optimizations for solid fill
>>>> planes. This series aims to expose these capabilities to userspace as
>>>> some compositors have a solid fill flag (ex. SOLID_COLOR in the Android
>>>> hardware composer HAL) that can be set by apps like the Android Gears
>>>> app.
>>>>
>>>> Userspace can set the color_fill value by setting COLOR_FILL_FORMAT to a
>>>> DRM format, setting COLOR_FILL to a color fill value, and setting the
>>>> framebuffer to NULL.
>>>
>>> Is there some real reason for the format property? Ie. why not
>>> just do what was the plan for the crttc background color and
>>> specify the color in full 16bpc format and just pick as many
>>> msbs from that as the hw can use?
>>>    
>>>>
>>>> Jessica Zhang (3):
>>>>     drm: Introduce color fill properties for drm plane
>>>>     drm: Adjust atomic checks for solid fill color
>>>>     drm/msm/dpu: Use color_fill property for DPU planes
>>>>
>>>>    drivers/gpu/drm/drm_atomic.c              | 68 ++++++++++++-----------
>>>>    drivers/gpu/drm/drm_atomic_helper.c       | 34 +++++++-----
>>>>    drivers/gpu/drm/drm_atomic_uapi.c         |  8 +++
>>>>    drivers/gpu/drm/drm_blend.c               | 38 +++++++++++++
>>>>    drivers/gpu/drm/drm_plane.c               |  8 +--
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c  |  7 ++-
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_plane.c | 66 ++++++++++++++--------
>>>>    include/drm/drm_atomic_helper.h           |  5 +-
>>>>    include/drm/drm_blend.h                   |  2 +
>>>>    include/drm/drm_plane.h                   | 28 ++++++++++
>>>>    10 files changed, 188 insertions(+), 76 deletions(-)
>>>>
>>>> -- 
>>>> 2.38.0
>>>
>>> -- 
>>> Ville Syrjälä
>>> Intel
>