[Freedreno] [RFC PATCH 0/3] Support for Solid Fill Planes
Jessica Zhang
quic_jesszhan at quicinc.com
Thu Jun 29 18:53:00 UTC 2023
On 6/29/2023 12:29 AM, Pekka Paalanen wrote:
> On Wed, 28 Jun 2023 09:40:21 -0700
> Jessica Zhang <quic_jesszhan at quicinc.com> wrote:
>
>> On 6/28/2023 12:34 AM, Pekka Paalanen wrote:
>>> On Tue, 27 Jun 2023 15:10:19 -0700
>>> Abhinav Kumar <quic_abhinavk at quicinc.com> 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
>>>>>>
>>>>>> Is there anything I'm missing or needs to be clarified?
>>>>>>
>>>>>
>>>>> LGTM otherwise
>>>>>
>>>>
>>>> LGTM too.
>>>
>>> Hi,
>>>
>>> yeah, that sounds fine to me, if everyone thinks that adding a new
>>> property for pixel_source is a good idea. I just assumed it was already
>>> agreed, and based my comments on that.
>>>
>>> I don't really remember much of the discussion about a special FB that
>>> is actually a solid fill vs. this two new properties design, so I
>>> cannot currently give an opinion on that, or any other design.
>>
>> Hi Pekka,
>>
>> It was discussed in the v3 of this series, but we came to the conclusion
>> that allocating an FB for solid_fill was unnecessary since solid fill
>> does not require memory fetch.
>
> Hi,
>
> it just occurred to me that the pixel_source property could be replaced
> with the rule that if a solid_fill blob id is 0, then use FD_IB. Or
> vice versa. But if someone then adds a third way of setting content on
> a plane, it would result in a chain of "if this is 0, then use the next
> one" and only if all are 0, there is no content.
Hi Pekka,
Agreed. I would prefer having a pixel_source property as it will be more
extendable than having arbitrary checks.
>
> I'm not sure if that's better or worse. Both designs seem to have the
> same backwards compatibility issues, and the exact same impact to
> legacy SetCrtc ioctl. Maybe pixel_source property is easier to document
> and understand though when there is no "if this does not exist or is 0
> then ..." chain.
FWIW, the solid_fill feature will require both the solid_fill and
pixel_source properties. I'll make a note of this in the cover letter.
>
> So, pixel_source is fine by me.
Awesome, thanks for the feedback! Will push a v4 with these changes.
Thanks,
Jessica Zhang
>
>
> Thanks,
> pq
More information about the wayland-devel
mailing list