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

[Kieran Bingham]

Hi Laurent,

On 21/04/2021 10:14, Laurent Pinchart wrote:
> Hi Kieran,
> 
> On Wed, Apr 21, 2021 at 09:50:56AM +0100, Kieran Bingham wrote:
>> On 21/04/2021 05:46, Hirokazu Honda wrote:
>>> On Wed, Apr 21, 2021 at 7:22 AM Laurent Pinchart wrote:
>>>> 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.
> 
> What use cases do you foresee for this ? With a function called
> hasPendingRequests(), the semantics is clear, and the base
> PipelineHandler class should have all the information it needs. If we
> want to also take into account more information, the function name may
> not be a good match anymore. I'd like to make a decision based on use
> cases. If we don't have any yet, I'd keep this function non-virtual, and
> possibly change it later once use cases arise.

Have you read this below?

> 
>>> 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?


This is already an extended use case. Hiro is suggesting he will / has
added a queuedRequests in the IPU3CameraData (not CameraData).

To validate that hasPendingRequests() is true, that queue should also be
checked.

As it is IPU3 specific, it needs to be overridden in the IPU3 pipeline
handler.

Of course, if this were to be a generic thing, where we track two lists,
pending, and queued in the base CameraData(), then the
hasPendingRequests() can check both there too.


--
Kieran


>>
>> 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.
>>
>>>>> +{
>>>>> +     const CameraData *data = cameraData(camera);
>>>>> +     return !data->queuedRequests_.empty();
>>>>> +}
>>>>> +
>>>>>  /**
>>>>>   * \fn PipelineHandler::queueRequest()
>>>>>   * \brief Queue a request
> 

-- 
Regards
--
Kieran