On Wed, 1 Feb 2023 18:06:41 -0800 Jessica Zhang <quic_jesszhan@xxxxxxxxxxx> wrote: > On 1/31/2023 4:49 AM, Pekka Paalanen wrote: > > On Tue, 31 Jan 2023 11:21:18 +0000 > > Simon Ser <contact@xxxxxxxxxxx> wrote: > > > >> On Tuesday, January 31st, 2023 at 12:13, Pekka Paalanen <ppaalanen@xxxxxxxxx> wrote: > >> > >>> On Tue, 31 Jan 2023 10:06:39 +0000 > >>> Simon Ser <contact@xxxxxxxxxxx> wrote: > >>> > >>>> On Tuesday, January 31st, 2023 at 10:25, Pekka Paalanen <ppaalanen@xxxxxxxxx> 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:
pgpDGuBW0jvSo.pgp
Description: OpenPGP digital signature
