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

[Kieran Bingham]

On 22/04/2021 04:06, Hirokazu Honda wrote:
> Hi Laurent and Kieran,
> 
> On Thu, Apr 22, 2021 at 12:00 PM Laurent Pinchart
> <laurent.pinchart at ideasonboard.com> wrote:
>>
>> Hi Kieran,
>>
>> On Wed, Apr 21, 2021 at 11:27:09AM +0100, Kieran Bingham wrote:
>>> On 21/04/2021 10:14, Laurent Pinchart wrote:
>>>> 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.
>>
>> I may be missing something, but won't data->queuedRequests_ work in that
>> case too ? It lists all pending requests from a libcamera core point of
>> view, regardless of how the pipeline handler handles them.
>>
> 
> Ah, you're right. Yes, just checking data->queuedRequests_ should work.
> Sorry for the wrong comment.

Ayeee of course. I'll do the rename, but keep is as a non-virtual member.

--
Kieran


> -Hiro
> 
>>>>> 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,
>>
>> Laurent Pinchart

-- 
Regards
--
Kieran