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

[Laurent Pinchart]

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.

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