On Tue, 27 Jun 2023 15:10:19 -0700
Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx> 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@xxxxxxxxxxx> 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:
pgpWaWYrTlYC9.pgp
Description: OpenPGP digital signature
