[RFC PATCH 0/3] Support for Solid Fill Planes
Jessica Zhang
quic_jesszhan at quicinc.com
Thu Jun 29 18:52:38 UTC 2023
On 6/27/2023 3:10 PM, Abhinav Kumar wrote:
>
>
> On 6/27/2023 2:59 PM, Dmitry Baryshkov wrote:
>> On 28/06/2023 00:27, Jessica Zhang wrote:
>>>
>>>
>>> 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 | |
>>
>> Probably these two cells were swapped.
>>
>
> Ack, yes they were swapped.
>
>>> +-----------------+-------------------------+-------------------------+
>>> | NULL commit | pixel_source = FB | pixel_source = FB |
>>> | | FB_ID = NULL | FB_ID = NULL |
>>> +-----------------+-------------------------+-------------------------+
>>
>> | or:
>> | pixel_source = COLOR
>> | solid_fill = NULL
Hi Dmitry and Abhinav,
Sorry, forgot to respond to this comment.
Yep, if pixel_source is set to COLOR, then a NULL solid_fill blob will
count as a NULL commit.
>>>
>>> Is there anything I'm missing or needs to be clarified?
>>>
>>
>> LGTM otherwise
>>
>
> LGTM too.
Thanks for the feedback!
BR,
Jessica Zhang
>
>>> 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
>>>>
>>
More information about the dri-devel
mailing list