Update va.h for multi-threaded usages #347
Conversation
va/va.h
Outdated
| * VAAPI objects, it it the application's responsibility to synchronize these | ||
| * operations in order to generate the expected results. | ||
| * | ||
| * It is worth noting that VAContext is not meant to be shared among different |
va/va.h
Outdated
| * All VAAPI functions implemented in libva should be thread-safe. However, | ||
| * VAAPI and libva do not impose thread-safety requirements on the driver or | ||
| * the backend implementation, and it is the responsibility of the driver or the | ||
| * backend implementation to ensure thread-saftey of VAAPI functions. In other |
|
I am confused about the purpose of this patch. Otherwise, gstreamer-vaapi's implementation can be regarded as wrong because it doesn't do anything for VA-API call serialization. |
va/va.h
Outdated
| @@ -129,6 +129,98 @@ extern "C" { | |||
| * - \ref api_fei | |||
| * - \ref api_fei_h264 | |||
| * - \ref api_fei_hevc | |||
| * | |||
| * \section threading Multithreading Guide | |||
| * All VAAPI functions implemented in libva should be thread-safe, and the | |||
There was a problem hiding this comment.
Please be careful with the wording. I'd recommend sticking to what https://www.ietf.org/rfc/rfc2119.txt suggests, and in particular the use of MUST and SHOULD. That said, this paragraph is misleading, because it starts saying "A is guaranteed" then "B should be guaranteed" but then says that if "B is not verified" then "A is not verified either".
Recommended writing:
All VAAPI functions implemented in libva are thread-safe insofar as the driver and backend
implementation are thread-safe. Note that libva cannot impose thread-safety guarantees
on neither the driver nor the backend, so it is ultimately the responsability of the latter to
ensure said thread-safety.
Please fix "thread-saftey" with "thread-safety" (ideally also introduce a ref-link
to e.g. https://en.wikipedia.org/wiki/Thread_safety).
There was a problem hiding this comment.
Thanks for the suggested changes and it's been incorporated into the latest version
va/va.h
Outdated
| * VAAPI objects, it is the application's responsibility to synchronize these | ||
| * operations in order to generate the expected results. This is especially true | ||
| * when a VAContext object is shared among different threads. Due to the fact that | ||
| * the internal states of the VAContext object can be modified through API calls |
There was a problem hiding this comment.
I'm confused here. A couple of lines above you say:
and VAAPI internal states will not be corrupted
but here I read
the internal states of the VAContext object can be modified
and, presumably, corrupted. Could you please elaborate on what do you
mean with "internal states" in l.145-146 and why VAContext is not one
of those?
There was a problem hiding this comment.
Changed "states" to "data structures" which may be a better term. Also removed the block on VAContext as it can just be treated as other VAAPI objects under multi-threaded usage, i.e. app needs to handle the synchronization with operations using VAContext to ensure the correct results.
|
@jonathanbian should we make it as a mandatory requirement for the va-backend driver to keep thread safety ? Otherwise the applications still have to think whether a particular driver is providing thread-safety or not. |
|
@sreerenjb my thinking is that as there is no formal certification/conformation process for the backend driver, making it a mandatory requirement won't mean much and the application still needs to verify whether the backend driver is thread-safe or not. |
|
@jonathanbian the point of making this mandatory is that the applications don't have to take care of locks themselves rather they can point out the place where driver is misbehaving. Once it is pointed out, it is up to the driver devs to fix the issue. Otherwise each middleware implementation has to write their own locking which is going to bring another level of bugs. I rather prefer to have the proper locking in one place for all middleware implementations which is the backend driver. |
|
Yes, without making it mandatory on VAAPI level as spec conformance requirement, this PR does not make sense. |
|
@sreerenjb @dvrogozh Updated with more strict requirements on the backend |
must of the VA-API calls (at least in gstvaapidisplay and gstvaapisurface) are mutexed: https://gitlab.freedesktop.org/gstreamer/gstreamer-vaapi/blob/master/gst-libs/gst/vaapi/gstvaapidisplay.c#L923 |
|
Not sure if it is the right place to ask. The calling sequence described is quit common usage on libva, that DECODE, ENCODE and VPP work on different threads. Want to double confirm if Intel-Media-Driver va-backend is
Is the thread-safety or context-safety guaranteed by Intel-Media-Driver's design? |
|
@daijh sorry for missing your comments, yes, context-safety guaranteed by https://github.com/intel/media-driver design |
Added description for multi-threaded usages of the API
jonathanbian
force-pushed
the
multithreading
branch
from
cf33af3
to
5a86e32
Compare
All threads in GPU process acquire a common lock whenever calling VA-API functions, because VA-API didn't ensure thread safety. The statement of thread safety has been added since libva 2.10.0 [1]. However, the thread-safety of libva depends on backend implementation. This CL introduces a feature switch |kGlobalVaapiLock| to control whether VaapiWrapper should run without a global lock should be used in VaapiWrapper when the backend implementation is thread-safe and |enforce_sequence_affinity_| is true. Current default value of the switch is ENABLED, this CL doesn't change the lock behavior in VA-API. Instead of removing the locks at once, we are going to experiment disabling the lock via a finch experiment with the switch. The benefits are: - It will reduce the latency as avoiding cpu lock - It will reduce the gpu power as converging jobs for gpu parallel execution - It will optimize the throughput for gpu which has multiple hw decoder/encoder instances Intel iHD driver is thread safe, allowing parallel VA-API calls. [1] intel/libva#347 Bug: 1123429, b:182008564 Co-Authored-By: hiroh@chromium.org Change-Id: I7da8a787d7d48b394921056da64eaa84ebec1377 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3139873 Reviewed-by: Xiaohan Wang <xhwang@chromium.org> Reviewed-by: Andres Calderon Jaramillo <andrescj@chromium.org> Commit-Queue: Andres Calderon Jaramillo <andrescj@chromium.org> Cr-Commit-Position: refs/heads/main@{#951415}
Added description for multi-threaded usages of the API