[libcamera-devel] [PATCH v4 11/16] py: merge read_event() and get_ready_requests()

[Tomi Valkeinen]

On 30/05/2022 18:53, Laurent Pinchart wrote:
> Hi Tomi,
> 
> Thank you for the patch.
> 
> On Mon, May 30, 2022 at 05:27:17PM +0300, Tomi Valkeinen wrote:
>> We always call CameraManager.read_event() and
>> CameraManager.get_ready_requests(), so to simplify the use merge the
>> read_event() into the get_ready_requests().
>>
>> This has the side effect that get_ready_requests() will now block if
>> there is no event ready. If we ever need to call get_ready_requests() in
>> a polling manner we will need a new function which behaves differently.
>>
>> However, afaics the only sensible way to manage the event loop is to use
>> select/poll on the eventfd and then call get_ready_requests() once,
>> which is the use case what the current merged function supports.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen at ideasonboard.com>
>> ---
>>   src/py/cam/cam.py            | 2 --
>>   src/py/libcamera/py_main.cpp | 7 ++-----
>>   test/py/unittests.py         | 4 ----
>>   3 files changed, 2 insertions(+), 11 deletions(-)
>>
>> diff --git a/src/py/cam/cam.py b/src/py/cam/cam.py
>> index bf8529d9..2ae89fa8 100755
>> --- a/src/py/cam/cam.py
>> +++ b/src/py/cam/cam.py
>> @@ -243,8 +243,6 @@ class CaptureState:
>>       # Called from renderer when there is a libcamera event
>>       def event_handler(self):
>>           try:
>> -            self.cm.read_event()
>> -
>>               reqs = self.cm.get_ready_requests()
>>   
>>               for req in reqs:
>> diff --git a/src/py/libcamera/py_main.cpp b/src/py/libcamera/py_main.cpp
>> index fcf009f0..505cc3dc 100644
>> --- a/src/py/libcamera/py_main.cpp
>> +++ b/src/py/libcamera/py_main.cpp
>> @@ -213,15 +213,12 @@ PYBIND11_MODULE(_libcamera, m)
>>   			return gEventfd;
>>   		})
>>   
>> -		.def("read_event", [](CameraManager &) {
>> +		.def("get_ready_requests", [](CameraManager &) {
>>   			uint8_t buf[8];
>>   
>> -			int ret = read(gEventfd, buf, 8);
>> -			if (ret != 8)
>> +			if (read(gEventfd, buf, 8) != 8)
>>   				throw std::system_error(errno, std::generic_category());
> 
> We need a memory barrier here to prevent reordering. I'm quite sure

We then need the same in handleRequestCompleted().

> there's one somewhere due to the read() call and the lock below, but I
> haven't been able to figure out the C++ rule that guarantees this. Does
> anyone know ?

https://en.cppreference.com/w/cpp/atomic/memory_order

So the mutex brings the normal acquire-release ordering. I would think a 
syscall includes a barrier, but I can't find any details right away.

  Tomi