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

[Pekka Paalanen <ppaalanen@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 7443 bytes --]

On Tue, 27 Jun 2023 15:10:19 -0700
Abhinav Kumar <quic_abhinavk@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@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.

Btw. there may be some confusion about "legacy userspace" which usually
refers to pre-atomic userspace, and old atomic userspace that does not
understand the new properties. That makes no difference in the table
here though, the legacy ioctls should just smash pixel_source.


Thanks,
pq

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]