[libcamera-devel] [PATCH v4 1/6] libcamera: camera: Assert pipelines complete all requests

[Kieran Bingham]

Hi Hiro,

On 21/04/2021 05:46, Hirokazu Honda wrote:
> Hi Kieran, thanks for the patch.
> 
> On Wed, Apr 21, 2021 at 7:22 AM Laurent Pinchart
> <laurent.pinchart at ideasonboard.com> wrote:
>>
>> Hi Kieran,
>>
>> Thank you for the patch.
>>
>> On Tue, Apr 20, 2021 at 02:07:36PM +0100, Kieran Bingham wrote:
>>> When the camera manager calls stop on a pipeline, it is expected that
>>> the pipeline handler guarantees all requests are returned back to the
>>> application before the camera has stopped.
>>>
>>> Ensure that this guarantee is met by providing an accessor on the
>>> pipeline handler to validate that all pending requests are removed.
>>>
>>> Signed-off-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
>>> ---
>>>  include/libcamera/internal/pipeline_handler.h |  1 +
>>>  src/libcamera/camera.cpp                      |  2 ++
>>>  src/libcamera/pipeline_handler.cpp            | 15 +++++++++++++++
>>>  3 files changed, 18 insertions(+)
>>>
>>> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
>>> index c6454db6b2c4..27d45d8ffc20 100644
>>> --- a/include/libcamera/internal/pipeline_handler.h
>>> +++ b/include/libcamera/internal/pipeline_handler.h
>>> @@ -80,6 +80,7 @@ public:
>>>
>>>       virtual int start(Camera *camera, const ControlList *controls) = 0;
>>>       virtual void stop(Camera *camera) = 0;
>>> +     bool active(const Camera *camera) const;
>>>
>>>       void queueRequest(Request *request);
>>>
>>> diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
>>> index 763f3b9926fd..c3fc3dd91bd6 100644
>>> --- a/src/libcamera/camera.cpp
>>> +++ b/src/libcamera/camera.cpp
>>> @@ -1084,6 +1084,8 @@ int Camera::stop()
>>>       d->pipe_->invokeMethod(&PipelineHandler::stop, ConnectionTypeBlocking,
>>>                              this);
>>>
>>> +     ASSERT(!d->pipe_->active(this));
>>> +
>>>       d->setState(Private::CameraConfigured);
>>>
>>>       return 0;
>>> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
>>> index 3b3150bdbbf7..8629e3578f02 100644
>>> --- a/src/libcamera/pipeline_handler.cpp
>>> +++ b/src/libcamera/pipeline_handler.cpp
>>> @@ -368,6 +368,21 @@ const ControlList &PipelineHandler::properties(const Camera *camera) const
>>>   * \context This function is called from the CameraManager thread.
>>>   */
>>>
>>> +/**
>>> + * \brief Determine if the camera has any requests pending
>>> + * \param[in] camera The camera to check
>>> + *
>>> + * This method determines if there are any requests queued to the pipeline
>>> + * awaiting processing.
>>> + *
>>> + * \return True if there are pending requests, or false otherwise
>>> + */
>>> +bool PipelineHandler::active(const Camera *camera) const
>>
>> How about naming the function hasPendingRequests() to make it more
>> explicit ?

Yes, I can rename it.


>>
>> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>>
> 
> Do you think to make this a virtual function?

Yes, it might be sensible to be virtual so that the Pipeline handler can
override it if there are more specific things to add.


> Since I introduce a pending requests queue in IPU3CameraData, I would
> like to IPU3PipelineHandler checks data->queuedRequests_ and
> data->pendingRequests_.empty(), or this should be a virtual function
> of CameraData.
> How do you think?

At the moment, I would think it's better that the Pipeline handler ...
handles it. The PH has access to the pipeline specific camera data anyway.

I'll make this

virtual bool hasPendingRequests(const Camera *camera) const;

in the next version.

--
Kieran

> 
> -Hiro
> 
>>> +{
>>> +     const CameraData *data = cameraData(camera);
>>> +     return !data->queuedRequests_.empty();
>>> +}
>>> +
>>>  /**
>>>   * \fn PipelineHandler::queueRequest()
>>>   * \brief Queue a request
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
>> _______________________________________________
>> libcamera-devel mailing list
>> libcamera-devel at lists.libcamera.org
>> https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards
--
Kieran