[libcamera-devel] [PATCH 05/10] libcamera: request: Add support for fences

[Umang Jain]

Hi Jacopo,

On 11/10/21 2:58 PM, Jacopo Mondi wrote:
> Hi Umang,
>
> On Wed, Nov 10, 2021 at 02:51:44PM +0530, Umang Jain wrote:
>> Hi Jacopo,
>>
>> On 10/28/21 4:45 PM, Jacopo Mondi wrote:
>>> Prepare the Request::Private class to handle fences by providing a
>>> storage vector and interface functions to handle the number of pending
>>> and expired fences.
>>>
>>> The intended user of the interface is the PipelineHandler class
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
>>> ---
>>>    include/libcamera/internal/request.h | 21 +++++++
>>>    src/libcamera/request.cpp            | 89 +++++++++++++++++++++++++++-
>>>    2 files changed, 109 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/include/libcamera/internal/request.h b/include/libcamera/internal/request.h
>>> index df0cc014067e..2be4874756de 100644
>>> --- a/include/libcamera/internal/request.h
>>> +++ b/include/libcamera/internal/request.h
>>> @@ -7,8 +7,12 @@
>>>    #ifndef __LIBCAMERA_INTERNAL_REQUEST_H__
>>>    #define __LIBCAMERA_INTERNAL_REQUEST_H__
>>> +#include <vector>
>>> +
>>>    #include <libcamera/request.h>
>>> +#include <libcamera/internal/fence.h>
>>> +
>>>    namespace libcamera {
>>>    class Camera;
>>> @@ -24,9 +28,26 @@ public:
>>>    	Camera *camera() const { return camera_; }
>>> +	unsigned int pendingFences() const { return pendingFences_; }
>>> +	unsigned int expiredFences() const { return expiredFences_; }
>>> +
>>> +	void reuse();
>>> +
>>> +	std::vector<Fence> &fences() { return fences_; }
>>> +	void addFence(Fence &&fence);
>>> +	void clearFences();
>>> +
>>> +	void fenceExpired();
>>> +	void fenceCompleted();
>>> +
>>>    private:
>>>    	Camera *camera_;
>>>    	bool cancelled_;
>>> +
>>> +	unsigned int pendingFences_ = 0;
>>> +	unsigned int expiredFences_ = 0;
>>> +
>>> +	std::vector<Fence> fences_;
>>>    };
>>>    } /* namespace libcamera */
>>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp
>>> index 33fee1ac05ba..e88eee1fac36 100644
>>> --- a/src/libcamera/request.cpp
>>> +++ b/src/libcamera/request.cpp
>>> @@ -63,6 +63,92 @@ Request::Private::~Private()
>>>     * request hasn't been queued
>>>     */
>>> +/**
>>> + * \fn Request::Private::pendingFences()
>>> + * \brief Retrieve the number of pending fences
>>> + *
>>> + * A Fence is pending if has not yet been signalled or it has not expired yet.
>>> + *
>>> + * \return The number of pending fences
>>> + */
>>> +
>>> +/**
>>> + * \fn Request::Private::expiredFences()
>>> + * \brief Retrieve the number of expired fences
>>> + * \return The number of expired fences
>>> + */
>>> +
>>> +/**
>>> + * \brief Reset the request for reuse
>>> + */
>>> +void Request::Private::reuse()
>>> +{
>>> +	cancelled_ = false;
>>> +
>>> +	fences_.clear();
>>> +	pendingFences_ = 0;
>>> +	expiredFences_ = 0;
>>> +}
>>> +
>>> +/**
>>> + * \fn Request::Private::fences()
>>> + * \brief Retrieve a reference to the vector of pending fences
>>> + * \return A reference to the vector of pending fences
>>> + */
>>> +
>>> +/**
>>> + * \brief Move a Fence into the Request
>>> + *
>>> + * Move a Fence into the Request and increase the pending fences
>>> + * counter. The Fence is now stored into the Request and the original
>>> + * one passed in as parameter is reset.
>>> + *
>>> + * Once the Fence completes, either because it is signalled or because
>>> + * it has expired, the caller shall notify the Request about this by
>>> + * calling fenceCompleted() or fenceExpired();
>>> + */
>>> +void Request::Private::addFence(Fence &&fence)
>>> +{
>>> +	fences_.push_back(std::move(fence));
>> Drive-by comment, please let me know if I am wrong:
>>
>> As per my understanding and your clarification, the fence is moved to a new
>> Fence and pushed into fences_. I believe this new Fence will be disabled
>> (by-default behaviour as stated in Fence class). So I don't think any of
>> fenceCompleted() or fenceExpired() getting called, which seems problematic.
>>
> No, that's by design :)
>
> Fences will be activated only when the request is queued to the camera.


Ok, I see in the below patch that you enable one-by-one.

I think I got confused by your reply and the statement point from the 
cover letter :

```

- Fences are attacched to a FrameBuffer and their value is valid until the
   Request is queued to the Camera

```

>
>>> +	pendingFences_++;
>>> +}
>>> +
>>> +/**
>>> + * \brief Release all Fences stored in the request
>>> + *
>>> + * Clear the vector of fences in the Request by releasing all of them.
>>> + * All Fences are closed and their file descriptors reset to 0.
>>> + */
>>> +void Request::Private::clearFences()
>>> +{
>>> +	ASSERT(!pendingFences_);
>>> +	fences_.clear();
>>> +}
>>> +
>>> +/**
>>> + * \brief Notify that a Fence has been signalled
>>> + *
>>> + * Notify to the Request that a Fence has completed. This function decrease the
>>> + * number of pending Fences in the request.
>>> + */
>>> +void Request::Private::fenceCompleted()
>>> +{
>>> +	pendingFences_--;
>>> +}
>>> +
>>> +/**
>>> + * \brief Notify that a Fence has expired
>>> + *
>>> + * Notify to the Request that a Fence has expired. This function decrease the
>>> + * number of pending Fences in the request and increase the number of expired
>>> + * ones.
>>> + */
>>> +void Request::Private::fenceExpired()
>>> +{
>>> +	expiredFences_++;
>>> +	pendingFences_--;
>>> +}
>>> +
>>>    /**
>>>     * \enum Request::Status
>>>     * Request completion status
>>> @@ -158,10 +244,11 @@ void Request::reuse(ReuseFlag flags)
>>>    	sequence_ = 0;
>>>    	status_ = RequestPending;
>>> -	_d()->cancelled_ = false;
>>>    	controls_->clear();
>>>    	metadata_->clear();
>>> +
>>> +	_d()->reuse();
>>>    }
>>>    /**