Re: [RFC PATCH 0/3] Support for Solid Fill Planes
On Tue, 27 Jun 2023 15:10:19 -0700 Abhinav Kumar 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 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 | > >> +=
Re: [RFC PATCH 0/3] Support for Solid Fill Planes
On 6/28/2023 12:34 AM, Pekka Paalanen wrote: On Tue, 27 Jun 2023 15:10:19 -0700 Abhinav Kumar 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 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_sourc