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

[Hirokazu Honda]

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