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

[Hirokazu Honda]

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 ?
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>

Do you think to make this a virtual function?
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?

-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