API Thread Safety

[arcnmx]

From writing a rust wrapper around the c api I've realised that there's no documentation on the thread-safety (or lack thereof) for any of the API calls. A few specific scenarios I'm wondering about, and whether they're safe and/or guaranteed:

• is it safe to enumerate/create display handles on one thread, then spawn another thread and run operations there? Can you move handles to other threads between operations without issue?
  • I think this mostly is a question of whether there's any use of hidden background threads or TLS when API operations aren't running?
• is it safe to concurrently run operations on different threads with different/distinct displays?
  • the CLI has a --async flag that indicates this could maybe be okay?
• is it safe to concurrently run operations on the same handle from different threads?
  • this one seems useless because you'd have to wait for one operation to complete before another can start, but it's useful to know whether there's any inherent synchronization in the implementation or not.

[rockowitz]

Hello! First off, I'm impressed to see what you're doing. Do you have in mind to write a UI in Rust? You're correct in surmising that ddcutil was created without much attention to thread safety, which is slowly being retrofitted. There's some experimental code in the current development version for threaded operation and queued requests. However, as I've gained experience building a C++ GUI using Qt, handling threading and queued requests in the UI seems more practical. Let's set aside the cases of monitor communication using the old AMD fglrx API or USB. In practice, those rarely occur. In the normal case, each ddcutil device handle translates to a single open /dev/i2c device. So it's possible to communicate with multiple monitors simultaneously on separate threads. As you note however, you need to wait for a request to complete before initiating another. I've not tested how the I2C bus and monitor would respond to a new request before the existing one completes, but I'm pretty certain it would be garbage. What we don't want: - Using multiple display handles to access the same display. The API needs to be extended to prevent this. - Making multiple overlapping requests (from different threads) to the same display handle. The Qt based GUI currently does the following: - calls ddca_get_display_info_list() to enumerate the displays - for each display, creates a thread which: - opens the display - maintains a queue of requests - calls the ddcutil api for each request, and updates the GUI accordingly Because DDC operations are slow (given the mandated sleeps), I expect to perform some request queue merging, e.g. if there are multiple setvcp requests in the queue for the same feature then earlier ones will be discarded. We'll see if this optimization is really needed. To your specific question, I see no reason why creating display handles on one thread and using them on another should not work. The problem would be using the same handle from multiple threads simultaneously. The API is still a work in progress. My experiences using it for the Qt GUI, and the experimental Python API. have resulted in changes If there are changes that make the Rust wrapper simpler and/or more functional, please let me know. Finally, do let me know when you're ready to give your work wider publicity and I'll reference it on the ddcutil web site. Regards, Sanford

…

On 2/25/2018 3:58 PM, arcnmx wrote: From writing a rust wrapper around the c api <https://github.com/arcnmx/ddcutil-rs> I've realised that there's no documentation on the thread-safety (or lack thereof) for any of the API calls. A few specific scenarios I'm wondering about, and whether they're safe and/or guaranteed: * is it safe to enumerate/create display handles on one thread, then spawn another thread and run operations there? Can you move handles to other threads between operations without issue? o I think this mostly is a question of whether there's any use of hidden background threads or TLS when API operations aren't running? * is it safe to concurrently run operations on different threads with different/distinct displays? o the CLI has a |--async| flag that indicates this could maybe be okay? * is it safe to concurrently run operations on the same handle from different threads? o this one seems useless because you'd have to wait for one operation to complete before another can start, but it's useful to know whether there's any inherent synchronization in the implementation or not. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#49>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ANhsbjo3DF67E25xQukeaCPRD4x0qGpFks5tYfOqgaJpZM4SShgU>.

[arcnmx]

Thanks for clearing that up! My use case is a pretty niche application for sharing a monitor with a QEMU VM - an X program that manages input redirection and switching the monitor back and forth when interaction with a VM is desired (the code in that repo using ddcutil has not been committed yet). I'm aiming to keep the bindings at feature parity with the C API, and it is pretty feature-complete so far: a messy short example that replicates "ddcutil getvcp ALL" covers most of the capability parsing and screen control.

As for how I'm handling threading, most of my application uses nonblocking IO and futures-rs. libddcutil API calls are just done on a secondary thread and message passing is used to indicate completion/failure/etc. back to the application. Regarding feedback on the API:

• A nonblocking API that integrates into an event loop using epoll or similar would be ideal, but that seems complicated to design. A thread per display mostly just blocking on syscalls isn't a huge waste of resources anyway, as DDC events are rather infrequent, so I think the current blocking API is perfectly fine.
• An async variant of the API such as the experimental ddca_start_get_any_vcp_value that just spawns threads internally is not useful for a Rust API, as it's better for the threading and synchronization to be managed in Rust anyway. This is probably the same conclusion you've come to in using Qt.
• I believe something like ddca_free_any_vcp_value() is missing, as currently you have to check the param type and if it's a table, there's an extra free() to be done.
• Perhaps a struct versioning scheme could be used to enable backwards compatibility once the API stabilizes. There are tons of breaking changes being made, and a way to mitigate breakage in the future after the library hits a "1.0" release may be helpful. This is probably not a priority right now.

What we don't want:

• Using multiple display handles to access the same display. The API
  needs to be extended to prevent this.
• Making multiple overlapping requests (from different threads) to the
  same display handle.

In practice, you probably can't prevent the application (or another application) from just opening the I2C device anyway and interfering, so I'm not sure there's much advantage to this. Managing global state to prevent this may end up complicating thread-safety as well, and adding locks could be unneeded overhead for single-threaded applications. A KISS API that simply forbids this and requires downstream library users to manage their own synchronization if they wish to share handles across threads is preferable, because the Rust type system can prevent handles from being used across threads without needing runtime locks or checks.

[rockowitz]

Thanks for your comments. I've added code to ddca_open_display() that prevents more than one thread from opening the same monitor simultaneously.  What makes this less than trivial is determining "the same monitor".  At first glance, this would be just the IO path, e.g. /dev/i2c-3. However, it is conceivable that multiple video cables could connect one or more video cards on the computer to multiple inputs on the monitor.   Or that there could be both an I2C and a USB connection to the monitor (e.g. Apple Cinema Display, some EIZOs).  So you have to look at the identifier fields in the EDID to fully check if monitors are the same. There are still some gaps: - During the monitor detection phase (which occurs at a more primitive level than ddca_open_display()),  the EDID is not yet available.  This complicates the --async option. - If multiple applications have multiple copies of the ddcutil library, then there is no cross-checking.  The only real solution is for the ddcutil library to become a service daemon. - And of course, if multiple computers are trying to communicate with a monitor simultaneously, there's nothing to be done. Regards, Sanford

…

On 02/25/2018 09:58 PM, arcnmx wrote: Thanks for clearing that up! My use case is a pretty niche application for sharing a monitor with a QEMU VM <https://github.com/arcnmx/screenstub> - an X program that manages input redirection and switching the monitor back and forth when interaction with a VM is desired (the code in that repo using ddcutil has not been committed yet). I'm aiming to keep the bindings at feature parity with the C API, and it is pretty feature-complete so far: a messy short example that replicates "ddcutil getvcp ALL" <https://github.com/arcnmx/ddcutil-rs/blob/master/examples/features.rs> covers most of the capability parsing and screen control. As for how I'm handling threading, most of my application uses nonblocking IO and futures-rs <https://crates.io/crates/futures>. libddcutil API calls are just done on a secondary thread and message passing is used to indicate completion/failure/etc. back to the application. Regarding feedback on the API: * A nonblocking API that integrates into an event loop using epoll or similar would be ideal, but that seems complicated to design. A thread per display mostly just blocking on syscalls isn't a huge waste of resources anyway, as DDC events are rather infrequent, so I think the current blocking API is perfectly fine. * An async variant of the API such as the experimental |ddca_start_get_any_vcp_value| that just spawns threads internally is not useful for a Rust API, as it's better for the threading and synchronization to be managed in Rust anyway. This is probably the same conclusion you've come to in using Qt. * I believe something like |ddca_free_any_vcp_value()| is missing, as currently you have to check the param type and if it's a table, there's an extra free() to be done. * Perhaps a struct versioning scheme could be used to enable backwards compatibility once the API stabilizes. There are tons of breaking changes being made, and a way to mitigate breakage in the future after the library hits a "1.0" release may be helpful. This is probably not a priority right now. What we don't want: * Using multiple display handles to access the same display. The API needs to be extended to prevent this. * Making multiple overlapping requests (from different threads) to the same display handle. In practice, you probably can't prevent the application (or another application) from just opening the I2C device anyway and interfering, so I'm not sure there's much advantage to this. Managing global state to prevent this may end up complicating thread-safety as well, and adding locks could be unneeded overhead for single-threaded applications. A KISS API that simply forbids this and requires downstream library users to manage their own synchronization if they wish to share handles across threads is preferable, because the Rust type system can prevent handles from being used across threads without needing runtime locks or checks. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#49 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ANhsbr5lSktkNFPKXzBpvDeAfj0vKpvIks5tYh3agaJpZM4SShgU>.

[arcnmx]

So you have to look at the identifier fields in the EDID to fully check if monitors are the same

Hm, I object to that change and this sounds like trouble, I don't think this is something ddcutil should handle. Sounds like a lot of complexity and too many edge cases for no benefit:

1. What if you have two identical monitor models that do not contain a unique serial number or other identifying information? Would this prevent you from controlling both because ddcutil thinks they're the same monitor?
2. It seems like it should be valid to have two DDC connections to the same monitor on different inputs. This is a totally expected and normal use case when using a TV or monitor with multiple devices, and my own monitors block off DDC temporarily on all inputs except the currently active one. My own use-case for ddcutil actually relies on this behaviour and expects there to be two video connections to the same monitor, and keeps an active display handle open to control it and switch between them.

If multiple applications have multiple copies of the ddcutil library, then there is no cross-checking. The only real solution is for the ddcutil library to become a service daemon.

Yeah, this is again something you cannot feasibly guard against, because someone could be using ddccontrol or something else instead. It is not ddcutil's responsibility to guard against every possible wrong use of DDC, and as long as those two applications cooperate and serialize their requests to the underlying monitor they're not even doing anything wrong by holding multiple handles open.

This seems like limiting flexibility of ddcutil while trying to guard against something that shouldn't be a problem. Documenting that "you should not make multiple requests to the same display without expecting to encounter I2C communication errors" (whether that's through the same handle or multiple handles to the same display) was really the most I was looking to clarify here - anything else sounds like complex and fallible application logic. Using an atomic in the display info pointer to prevent multiple handles from being opened is probably the most ddcutil should do, but it doesn't even sound like that would be necessary or effective.

[rockowitz]

I think your points are generally well taken, and I'll give them further thought.  Most saliently, if the documentation has to explain lots of edge cases, the design is suspect.   Comments interspersed.

On 03/04/2018 02:02 PM, arcnmx wrote: So you have to look at the identifier fields in the EDID to fully check if monitors are the same Hm, I object to that change and this sounds like trouble, I don't think this is something ddcutil should handle. Sounds like a lot of complexity and too many edge cases for no benefit: 1. What if you have two identical monitor models that do not contain a unique serial number or other identifying information? Would this prevent you from controlling both because ddcutil thinks they're the same monitor?

Yes, that's an issue.  As a practical matter, I've not yet seen a manufacturer's EDID that lacks unique identifiers.   It is possible that a user could flash the firmware in multiple monitors with a custom EDID having identical identifiers.  ddcutil reads the EDID directly from the monitor over I2C, so is not affected by any mechanism that replaces the EDID higher in the stack.

1. It seems like it should be valid to have two DDC connections to the same monitor on different inputs. This is a totally expected and normal use case when using a TV or monitor with multiple devices, and my own monitors block off DDC temporarily on all inputs except the currently active one.

Whether monitors recognize DDC communication on all inputs, or just the currently selected input, is monitor dependent.

1. My own use-case for ddcutil actually relies on this behaviour and expects there to be two video connections to the same monitor, and keeps an active display handle open to control it and switch between them.

To clarify, are you doing this from a single instance of ddcutil, or multiple instances? If you're doing this from a single instance of ddcutil the change would affect you. Note that if you're keeping multiple handles open performance reasons it's pointless.   The simplest way to see this is to run the command line version of ddcutil with the --stats option.  Execution time is completely dominated by sleeps mandated by the DDC protocol.  After that comes the execution time of the I2C reads and writes.  open and close of the /dev/i2c devices is negligible, as is the the time spent in the ddcutil code itself outside of system calls.

…

If multiple applications have multiple copies of the ddcutil library, then there is no cross-checking. The only real solution is for the ddcutil library to become a service daemon. Yeah, this is again something you cannot feasibly guard against, because someone could be using ddccontrol or something else instead. It is not ddcutil's responsibility to guard against every possible wrong use of DDC, and as long as those two applications cooperate and serialize their requests to the underlying monitor they're not even doing anything wrong by holding multiple handles open. This seems like limiting flexibility of ddcutil while trying to guard against something that shouldn't be a problem. Documenting that "you should not make multiple requests to the same display without expecting to encounter I2C communication errors" (whether that's through the same handle or multiple handles to the same display) was really the most I was looking to clarify here - anything else sounds like complex and fallible application logic. Using an atomic in the display info pointer to prevent multiple handles from being opened is probably the most ddcutil should do, but it doesn't even sound like that would be necessary or effective. — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#49 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ANhsboh_Fo1D7ySir7_6hhUQwzGZSAimks5tbDpigaJpZM4SShgU>.