Re: [PATCH RFC v6 00/10] Support for Solid Fill Planes
On 2023-08-28 20:05, Jessica Zhang wrote: > Some drivers support hardware that have optimizations for solid fill > planes. This series aims to expose these capabilities to userspace as > some compositors have a solid fill flag (ex. SOLID_COLOR in the Android > hardware composer HAL) that can be set by apps like the Android Gears > app. > > In order to expose this capability to userspace, this series will: > > - Introduce solid_fill and pixel_source properties to allow userspace to > toggle between FB and solid fill sources > - Loosen NULL FB checks within the DRM atomic commit callstack to allow > for NULL FB when solid fill is enabled. > - Add NULL FB checks in methods where FB was previously assumed to be > non-NULL > - Have MSM DPU driver use drm_plane_state.solid_fill instead of > dpu_plane_state.color_fill > > Note: The solid fill planes feature depends on both the solid_fill *and* > pixel_source properties. > > To use this feature, userspace can set the solid_fill property to a blob > containing the appropriate version number and solid fill color (in > RGB323232 format) and and setting the pixel_source property to > DRM_PLANE_PIXEL_SOURCE_COLOR. This will disable memory fetch and the > resulting plane will display the color specified by the solid_fill blob. > > Currently, there's only one version of the solid_fill blob property. > However if other drivers want to support a similar feature, but require > more than just the solid fill color, they can extend this feature by > creating additional versions of the drm_solid_fill struct. > > This 2 property approach was chosen because passing in a special 1x1 FB > with the necessary color information would have unecessary overhead that > does not reflect the behavior of the solid fill feature. In addition, > assigning the solid fill blob to FB_ID would require loosening some core > drm_property checks that might cause unwanted side effects elsewhere. > I didn't have a detailed review of this patchset but at a high-level this change makes sense to me. Feel free to add my Acked-by: Harry Wentland to patches 1-5. Harry > --- > Changes in v6: > - Have _dpu_plane_color_fill() take in a single ABGR color instead > of having separate alpha and BGR color parameters (Dmitry) > - Drop plane->state->pixel_source != DRM_PLANE_PIXEL_SOURCE_FB check > in SetPlane ioctl (Dmitry) > - Add DRM_PLANE_PIXEL_SOURCE_NONE as a default pixel source (Sebastian) > - Dropped versioning from solid fill property blob (Dmitry) > - Use DRM_ENUM_NAME_FN (Dmitry) > - Use drm_atomic_replace_property_blob_from_id() (Dmitry) > - drm_atomic_check_fb -> drm_atomic_plane_check_fb (Dmitry) > - Group redundant NULL FB checks (Dmitry) > - Squashed drm_plane_needs_disable() implementation with > DRM_PLANE_PIXEL_SOURCE_NONE declaration (Sebastian) > - Add comment to support RGBA solid fill color in the future (Dmitry) > - Link to v5: > https://lore.kernel.org/r/20230728-solid-fill-v5-0-053dbefa9...@quicinc.com > > Changes in v5: > - Added support for PIXEL_SOURCE_NONE (Sebastian) > - Added WARN_ON() in drm_plane_has_visible_data() if pixel_source isn't > set (Dmitry) > - Added debugfs support for both properties (Dmitry) > - Corrected u32 to u8 conversion (Pekka) > - Moved drm_solid_fill_info struct and related documentation to > include/uapi (Pekka) > - Changed drm_solid_fill_info.version to __u32 for data alignment (Pekka) > - Added more detailed UAPI and kernel documentation (Pekka) > - Reordered patch series so that the pixel_source property is introduced > before solid_fill (Dmitry) > - Fixed inconsistent ABGR/RGBA format declaration (Pekka) > - Reset pixel_source to FB in drm_mode_setplane() (Dmitry) > - Rename supported_sources to extra_sources (Dmitry) > - Only destroy old solid_fill blob state if new state is valid (Pekka) > - Link to v4: > https://lore.kernel.org/r/20230404-solid-fill-v4-0-f4ec0caa7...@quicinc.com > > Changes in v4: > - Rebased onto latest kernel > - Reworded cover letter for clarity (Dmitry) > - Reworded commit messages for clarity > - Split existing changes into smaller commits > - Added pixel_source enum property (Dmitry, Pekka, Ville) > - Updated drm-kms comment docs with pixel_source and solid_fill > properties (Dmitry) > - Inlined drm_atomic_convert_solid_fill_info() (Dmitry) > - Passed in plane state alpha value to _dpu_plane_color_fill_pipe() > - Link to v3: > https://lore.kernel.org/r/20230104234036.636-1-quic_jessz...@quicinc.com > > Changes in v3: > - Fixed some logic errors in atomic checks (Dmitry) > - Introduced drm_plane_has_visible_data() and drm_atomic_check_fb() helper > methods (Dmitry) > - Fixed typo in drm_solid_fill struct documentation > - Created drm_plane_has_visible_data() helper and corrected CRTC and FB > NULL-check logic (Dmitry) > - Merged `if (fb)` blocks in drm_atomic_plane_check() and abstracted > them into helper method (Dmitry) > - Inverted `if (solid_fill_enabled) else if
Re: race between (say) wl_display_sync and wl_callback_add_listener?
Hi >On Tue, Sep 26, 2023 at 10:08:55AM +0100, John Cox wrote: >> Hi >> >> Many thanks for your comprehensive answer >> >> >On Mon, Sep 25, 2023 at 05:10:30PM +0100, John Cox wrote: >> >> Hi >> > >> >Hi, >> > >> >> Assuming I have a separate poll/read_events/dispatch_queue thread to my >> >> main thread. If I go wl_display_sync/wl_callback_add_listener on the >> >> main thread is there a race condition between those two calls where the >> >> other thread can read & dispatch the callback before the listener is >> >> added or is there some magic s.t. adding the listener will always work >> >> as expected? >> > >> >This is indeed a racy interaction, in more ways than one: >> > >> >1. There is an inherent data race setting and using the >> > listener/user_data concurrently from different threads. >> > Neither adding a listener/user_data (see wl_proxy_add_listener()), >> > nor the actual dispatching operation in libwayland (which uses the >> > listener, see wayland-client.c:dispatch_event()) is protected by >> > the internal "display lock". >> >> So are all interactions from another thread than the dispatch thread >> unsafe e.g. buffer creation, setup & assignment to a surface or is it >> just the obviously racy subset? > >From a low-level libwayland API perspective, creating objects or making >requests from another thread is generally fine. It's this particular >case of requests that create object and cause events to be emitted >immediately for them that's problematic. > >Many requests that fall in this category are in fact global binds (e.g., >wl_shm, wl_seat, wl_output), which are typically handled in the context of the >wl_registry.global handler, thus in the dispatch thread, in which case no >special consideration is required. > >There are also some requests that at first glance seem problematic, >but in fact they are not (if things are done properly, and assuming >correct compositor behavior). For example: > > struct wl_callback *wl_surface_frame(struct wl_surface *surface); > >seems suspiciously similar to wl_display_sync(). However, this request >takes effect (i.e., the callback will receive events) only after the next >wl_surface_commit(), so the following is safe: > >cb = wl_surface_frame(surface); >/* As long as the listener is set before the commit we are good. */ >wl_callback_add_listener(cb); >wl_surface_commit(surface); > >Of course, on top of all these there are also the typical higher-level >multithreading synchronization considerations. > > Good - I think my current expectations align with what you and pq have said. So thank you once again for taking the time to help me John Cox
Re: Questions about object ID lifetimes
On Tue, 26 Sep 2023 11:53:07 +0300 Pekka Paalanen wrote: > On Mon, 25 Sep 2023 12:05:30 -0400 > jleivent wrote: > > > How do I get CI/CD capability turned on? I tried building the unit > > tests locally, but get errors that suggest those tests need to be > > run in CI. Issue 540 says I need to apply for the guest role - how > > do I do that? > > I don't recall libwayland having anything that needs to be > specifically run in a CI environment, and if it does, it should > automatically skip outside of CI environment. Weston does this. > > What errors did you get? How did you run them? > > 'meson test' is the command. I get: $ meson test ERROR: No such build data file as '/home/jil/gits/wayland-idfix/meson-private/build.dat'. I used this to build and install it: $ meson build/ --prefix=/home/jil/gits/wayland-idfix/install/ $ ninja -C build/ install Since that didn't create the needed meson-private/build.dat, I thought that might get put in by the CI somehow. > > I think applying for the guest role means that you can file an issue > on the upstream project asking for the permission. At minimum, a > maintainer needs to know your gitlab handle. I'll do that.
Re: race between (say) wl_display_sync and wl_callback_add_listener?
On Tue, Sep 26, 2023 at 10:08:55AM +0100, John Cox wrote: > Hi > > Many thanks for your comprehensive answer > > >On Mon, Sep 25, 2023 at 05:10:30PM +0100, John Cox wrote: > >> Hi > > > >Hi, > > > >> Assuming I have a separate poll/read_events/dispatch_queue thread to my > >> main thread. If I go wl_display_sync/wl_callback_add_listener on the > >> main thread is there a race condition between those two calls where the > >> other thread can read & dispatch the callback before the listener is > >> added or is there some magic s.t. adding the listener will always work > >> as expected? > > > >This is indeed a racy interaction, in more ways than one: > > > >1. There is an inherent data race setting and using the > > listener/user_data concurrently from different threads. > > Neither adding a listener/user_data (see wl_proxy_add_listener()), > > nor the actual dispatching operation in libwayland (which uses the > > listener, see wayland-client.c:dispatch_event()) is protected by > > the internal "display lock". > > So are all interactions from another thread than the dispatch thread > unsafe e.g. buffer creation, setup & assignment to a surface or is it > just the obviously racy subset? >From a low-level libwayland API perspective, creating objects or making requests from another thread is generally fine. It's this particular case of requests that create object and cause events to be emitted immediately for them that's problematic. Many requests that fall in this category are in fact global binds (e.g., wl_shm, wl_seat, wl_output), which are typically handled in the context of the wl_registry.global handler, thus in the dispatch thread, in which case no special consideration is required. There are also some requests that at first glance seem problematic, but in fact they are not (if things are done properly, and assuming correct compositor behavior). For example: struct wl_callback *wl_surface_frame(struct wl_surface *surface); seems suspiciously similar to wl_display_sync(). However, this request takes effect (i.e., the callback will receive events) only after the next wl_surface_commit(), so the following is safe: cb = wl_surface_frame(surface); /* As long as the listener is set before the commit we are good. */ wl_callback_add_listener(cb); wl_surface_commit(surface); Of course, on top of all these there are also the typical higher-level multithreading synchronization considerations. > >Finally, you might also want to consider a different design more in line > >with the libwayland API expectations, if possible. > > Not really possible - there are some things I need done (buffer reclaim > mostly) async as soon as possible and I don't have control over the main > loop. Buffer reclaiming (if by that you mean handling wl_buffer.release and making the buffer available for drawing again) is a good case for a separate queue, since wl_buffers are "independent" objects, as long as you can dispatch on demand: when you need a new buffer and don't have any you can dispatch the queue to check if the compositor has released one. > There may be a document that sets out everything you've said above and > gives the exact expectations but I failed to find it. In general the > individual call documentation is great but how interactions between > calls are managed is harder to find. I started from an (incorrect) > assumption that everything was fully async and could be called from any > thread (my natural progamming style) and so there must be magic to > enable that and have only slowly been corrected by reality. I wouldn't mind an official Wayland API/technical FAQ myself :) Thanks, Alexandros
Re: race between (say) wl_display_sync and wl_callback_add_listener?
On Mon, 25 Sep 2023 17:10:30 +0100 John Cox wrote: > Hi > > Assuming I have a separate poll/read_events/dispatch_queue thread to my > main thread. If I go wl_display_sync/wl_callback_add_listener on the > main thread is there a race condition between those two calls where the > other thread can read & dispatch the callback before the listener is > added or is there some magic s.t. adding the listener will always work > as expected? > > I assume it must be safe if dispatch & create/add_listener are in the > same thread. > > The same question applies to adding listeners after binding (say) the > shm extension where the bind provokes a set of events giving formats. > > Is there a safe way of doing this? (mutex around the dispatch and > create/add_listener pairs would seem plausible even if really not what I > want to do) Hi, yeah, this requires some careful handling, because the race is real otherwise. The following applies to the libwayland implementation. The first thing is that you should have a separate wl_event_queue for each "execution context". A thread is definitely an execution context, but not the only possible case. Then, you dispatch that event queue only from the correct execution context. This will serialize your own actions with the dispatch, so when you send a wl_display.sync, even if the compositor already responds with wl_callback.done, it won't get dispatched until you explicitly call dispatch in that thread. wl_display itself counts as one wl_event_queue, the default event queue. wl_event_queues are also a way to filter events without dispatching everything up to the event you wait for. This is the other case of execution context. The disadvantage is that you lose the event ordering between two queues. The second thing is more subtle. wl_display is always on the default event queue. A new protocol object inherits the event queue of the object it is created from. That is, wl_display.sync creates a wl_callback, and the wl_callback starts in the default event queue. This is a problem if you send wl_display.sync from another thread. Your main thread might dispatch the 'done' reply before you can set up the listener or change the queue assignment. This problem is fixed by using wl_proxy_create_wrapper(). See its documentation. Proxy wrappers are used whenever you need to have the new protocol object in a different event queue than the object creating it. You can run the poll/read_events/dispatch_queue loop in any and all execution contexts you like as long as at least one thread is always quick to read the Wayland socket. The reader will direct events to all queues. Thanks, pq pgpMhYVsnH4gl.pgp Description: OpenPGP digital signature
Re: race between (say) wl_display_sync and wl_callback_add_listener?
Hi Many thanks for your comprehensive answer >On Mon, Sep 25, 2023 at 05:10:30PM +0100, John Cox wrote: >> Hi > >Hi, > >> Assuming I have a separate poll/read_events/dispatch_queue thread to my >> main thread. If I go wl_display_sync/wl_callback_add_listener on the >> main thread is there a race condition between those two calls where the >> other thread can read & dispatch the callback before the listener is >> added or is there some magic s.t. adding the listener will always work >> as expected? > >This is indeed a racy interaction, in more ways than one: > >1. There is an inherent data race setting and using the > listener/user_data concurrently from different threads. > Neither adding a listener/user_data (see wl_proxy_add_listener()), > nor the actual dispatching operation in libwayland (which uses the > listener, see wayland-client.c:dispatch_event()) is protected by > the internal "display lock". So are all interactions from another thread than the dispatch thread unsafe e.g. buffer creation, setup & assignment to a surface or is it just the obviously racy subset? >2. Even ignoring the above, if the callback done event is received and > dispatched before the listener is actually set, the callback will > not get called. That was the race I could see >> I assume it must be safe if dispatch & create/add_listener are in the >> same thread. > >Yes, a single thread guarantees that the listener is properly set before >any relevant event is dispatched (assuming you don't dispatch events >before setting the listener!). > >> The same question applies to adding listeners after binding (say) the >> shm extension where the bind provokes a set of events giving formats. > >Yes, any object creation that may cause the compositor to send events as >an immediate response has this problem. > >> Is there a safe way of doing this? (mutex around the dispatch and >> create/add_listener pairs would seem plausible even if really not what I >> want to do) > >Here are some ideas: > >Use a separate queue in the main thread for this event. Special care is >needed to ensure the event is actually dispatched on that queue. See >wl_proxy_create_wrapper(), and wl_display_roundtrip_queue() for a >function that uses a wrapper. Note that using a separate queue may >cause the event(s) to be handled out of order relative to the events >in the main queue. This may or may not be a problem depending on the >specific scenario. Yup - understood >Using a mutex as you describe is also a possible solution. The typical >caveats apply: holding a mutex when calling out to potentially arbitrary >code (i.e., dispatch handlers) may increase deadlock risk etc. Thats why I don't want to do that >Another alternative would be to introduce a mechanism to pause and >resume the dispatch thread (e.g., wake up the poll with a pipe/eventfd >and then wait on some condition variable to resume), which would be used >like: > >pause_dispatch_thread(); >... sync and add listener ... >resume_dispatch_thread(); > >You could also introduce a mechanism to schedule your own function >calls in the dispatch thread: > >schedule_in_dispatch_thread(sync_and_add_listener, data); Yup - thats what I'm now doing for the obvious cases >Finally, you might also want to consider a different design more in line >with the libwayland API expectations, if possible. Not really possible - there are some things I need done (buffer reclaim mostly) async as soon as possible and I don't have control over the main loop. There may be a document that sets out everything you've said above and gives the exact expectations but I failed to find it. In general the individual call documentation is great but how interactions between calls are managed is harder to find. I started from an (incorrect) assumption that everything was fully async and could be called from any thread (my natural progamming style) and so there must be magic to enable that and have only slowly been corrected by reality. Again many thanks John Cox >HTH, >Alexandros
Re: CI/CD privileges for wayland-idfix fork
On Sat, 23 Sep 2023 09:49:20 -0400 jleivent wrote: > On Sat, 23 Sep 2023 09:40:20 -0400 > jleivent wrote: > > > Could I have CI/CD privileges for > > https://gitlab.freedesktop.org/jonleivent/wayland-idfix > > > > Thanks > > Jon > > Also: > With respect to the caching scheme described in .gitlab-ci.yaml, should > I change my FDO_DISTRIBUTION_TAG to stay out of the way? Anything else > I need to do before CI is turned on? You shouldn't need to do anything than check your project is public and has container registry enabled, apart from getting permission to run CI. Your project that you forked from upstream has its own container registry, and if your CI runs need to create new containers, they are pushed there and not upstream. You wouldn't have permissions to change anything in the upstream project anyway. Thanks, pq pgpelNdiymLAl.pgp Description: OpenPGP digital signature
Re: Questions about object ID lifetimes
On Mon, 25 Sep 2023 12:05:30 -0400 jleivent wrote: > How do I get CI/CD capability turned on? I tried building the unit > tests locally, but get errors that suggest those tests need to be run > in CI. Issue 540 says I need to apply for the guest role - how do I do > that? I don't recall libwayland having anything that needs to be specifically run in a CI environment, and if it does, it should automatically skip outside of CI environment. Weston does this. What errors did you get? How did you run them? 'meson test' is the command. I think applying for the guest role means that you can file an issue on the upstream project asking for the permission. At minimum, a maintainer needs to know your gitlab handle. All this permission hassle is just to avoid people that want to steal CPU time from CI runners for unrelated or unwanted purposes (like building a complete Android OS image from scratch or cryptomining). Thanks, pq pgprhB_7TXsgg.pgp Description: OpenPGP digital signature
Re: race between (say) wl_display_sync and wl_callback_add_listener?
On Mon, Sep 25, 2023 at 05:10:30PM +0100, John Cox wrote: > Hi Hi, > Assuming I have a separate poll/read_events/dispatch_queue thread to my > main thread. If I go wl_display_sync/wl_callback_add_listener on the > main thread is there a race condition between those two calls where the > other thread can read & dispatch the callback before the listener is > added or is there some magic s.t. adding the listener will always work > as expected? This is indeed a racy interaction, in more ways than one: 1. There is an inherent data race setting and using the listener/user_data concurrently from different threads. Neither adding a listener/user_data (see wl_proxy_add_listener()), nor the actual dispatching operation in libwayland (which uses the listener, see wayland-client.c:dispatch_event()) is protected by the internal "display lock". 2. Even ignoring the above, if the callback done event is received and dispatched before the listener is actually set, the callback will not get called. > I assume it must be safe if dispatch & create/add_listener are in the > same thread. Yes, a single thread guarantees that the listener is properly set before any relevant event is dispatched (assuming you don't dispatch events before setting the listener!). > The same question applies to adding listeners after binding (say) the > shm extension where the bind provokes a set of events giving formats. Yes, any object creation that may cause the compositor to send events as an immediate response has this problem. > Is there a safe way of doing this? (mutex around the dispatch and > create/add_listener pairs would seem plausible even if really not what I > want to do) Here are some ideas: Use a separate queue in the main thread for this event. Special care is needed to ensure the event is actually dispatched on that queue. See wl_proxy_create_wrapper(), and wl_display_roundtrip_queue() for a function that uses a wrapper. Note that using a separate queue may cause the event(s) to be handled out of order relative to the events in the main queue. This may or may not be a problem depending on the specific scenario. Using a mutex as you describe is also a possible solution. The typical caveats apply: holding a mutex when calling out to potentially arbitrary code (i.e., dispatch handlers) may increase deadlock risk etc. Another alternative would be to introduce a mechanism to pause and resume the dispatch thread (e.g., wake up the poll with a pipe/eventfd and then wait on some condition variable to resume), which would be used like: pause_dispatch_thread(); ... sync and add listener ... resume_dispatch_thread(); You could also introduce a mechanism to schedule your own function calls in the dispatch thread: schedule_in_dispatch_thread(sync_and_add_listener, data); Finally, you might also want to consider a different design more in line with the libwayland API expectations, if possible. HTH, Alexandros
