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

[Pekka Paalanen <ppaalanen@gmail.com>]

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

On Wed, 1 Feb 2023 18:06:41 -0800
Jessica Zhang <quic_jesszhan@quicinc.com> wrote:

> On 1/31/2023 4:49 AM, Pekka Paalanen wrote:
> > On Tue, 31 Jan 2023 11:21:18 +0000
> > Simon Ser <contact@emersion.fr> wrote:
> >   
> >> On Tuesday, January 31st, 2023 at 12:13, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> >>  
> >>> On Tue, 31 Jan 2023 10:06:39 +0000
> >>> Simon Ser <contact@emersion.fr> wrote:
> >>>      
> >>>> On Tuesday, January 31st, 2023 at 10:25, Pekka Paalanen <ppaalanen@gmail.com> wrote:
> >>>>      
> >>>>> indeed, what about simply using a 1x1 framebuffer for real? Why was that
> >>>>> approach rejected?  
> >>>>
> >>>> Ideally we don't want to allocate any GPU memory for the solid-fill
> >>>> stuff. And if we special-case 1x1 FB creation to not be backed by real
> >>>> GPU memory then we hit several situations where user-space expects a
> >>>> real FB but there isn't: for instance, GETFB2 converts from FB object
> >>>> ID to GEM handles. Even if we make GETFB2 fail and accept that this
> >>>> breaks user-space, then there is no way for user-space to recover the
> >>>> FB color for flicker-free transitions and such.
> >>>>
> >>>> This is all purely from a uAPI PoV, completely ignoring the potential
> >>>> issues with the internal kernel abstractions which might not be suitable
> >>>> for this either.  
> >>>
> >>> I mean a real 1x1 buffer: a dumb buffer.
> >>>
> >>> It would be absolutely compatible with anything existing, because it is
> >>> a real FB. As a dumb buffer it would be trivial to write into and read
> >>> out. As 1x1 it would be tiny (one page?). Even if something needs to
> >>> raw-access uncached memory over 33 MHz PCI bus or whatever the worst
> >>> case is, it's just one pixel, so it's fast enough, right? And it only
> >>> needs to be read once when set, like USB display drivers do. The driver
> >>> does not need to manually apply any color operations, because none are
> >>> supported in this special case.
> >>>
> >>> One can put all these limitations and even pixel format in the plane
> >>> property that tells userspace that a 1x1 FB works here.
> >>>
> >>> To recap, the other alternatives under discussion I see right now are:
> >>>
> >>> - this proposal of dedicated fill color property
> >>> - stuffing something new into FB_ID property
> >>>
> >>> There is also the question of other kinds of plane content sources like
> >>> live camera feeds where userspace won't be shovelling each frame
> >>> individually like we do now.
> >>>
> >>> 1x1 dumb buffer is not as small and lean as a dedicated fill color
> >>> property, but the UAPI design questions seem to be much less. What's
> >>> the best trade-off and for whom?  
> >>
> >> By "real memory" yes I mean the 1 page.
> >>
> >> Using a real buffer also brings back other discussions, e.g. the one about
> >> which pixel formats to accept.  
> > 
> > Yeah, which is why I wrote: "One can put all these limitations and even
> > pixel format in the plane property". It doesn't even need to be a
> > variable in the UAPI, it can be hardcoded in the UAPI doc.
> > 
> > Please, do not understand this as me strongly advocating for the real FB
> > approach! I just don't want that option to be misunderstood.
> > 
> > I don't really care which design is chosen, but I do care about
> > documenting why other designs were rejected. If the rejection reasons
> > were false, they should be revised, even if the decision does not
> > change.  
> 
> Hi Pekka/Daniel,
> 
> Looks like the general sentiment is to keep solid fill as a separate 
> property, so I will stick with that implementation for v4.
> 
> I can document the reason why we chose this approach over 1x1 FB in the 
> cover letter, but to summarize here:
> 
> Allocating an FB for solid_fill brings in unnecessary overhead (ex. 
> having to allocate memory for the FB). In addition, since memory fetch 
> is disabled when solid fill is enabled, having a separate property that 
> doesn't do any memory allocation for solid fill better reflects the 
> behavior of this feature within driver.
> 
> We also wanted to avoid having FB_ID accept a property blob as it would 
> involve loosening some drm_property checks, which could cause issues 
> with other property ioctls.
> 

That's fine by me, thanks!

> Also, re: other plane sources -- FWIW, I have tried implementing a 
> source enum as Ville suggested, but ultimately dropped the change as it 
> would require userspace to set properties in a specific order (i.e. to 
> enable solid_fill, userspace would have to first set FB_ID to NULL then 
> set SOLID_FILL).
> 
> I'm not sure how much of a can of worms that would be for userspace, but 
> if you're fine with having that as a requirement the I can re-add the code.

There is no ordering between properties set in a single atomic commit,
they all apply at the same time. Therefore the kernel code needs to
consider the whole new state set as a single entity.

If userspace splits changing those two properties into different atomic
commits, that's a userspace bug. It would not work with atomic
properties already today, where you need to set half a dozen properties
to update one KMS plane.

The only complication I can see is the legacy KMS UAPI, non-atomic.
They will change FB_ID, but they cannot touch the solid fill property.
I guess that needs to be special-cased somehow.


Thanks,
pq

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