[libcamera-devel] [PATCH 06/11] libcamera: ipu3: cio2: Generate start of frame event
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Dec 8 02:58:23 CET 2020
Hi Niklas,
Thank you for the patch.
On Mon, Nov 09, 2020 at 12:13:02PM +0100, Jacopo Mondi wrote:
> On Thu, Nov 05, 2020 at 01:15:41AM +0100, Niklas Söderlund wrote:
> > Propagate the frameStart event whenever the CSI-2 receiver in the CIO2
> > pipeline generates one.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> > src/libcamera/pipeline/ipu3/cio2.cpp | 18 +++++++++++++++---
> > src/libcamera/pipeline/ipu3/cio2.h | 3 ++-
> > 2 files changed, 17 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> > index 1c7b9676f52abdb6..1eca674da339b8e0 100644
> > --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> > @@ -247,15 +247,27 @@ int CIO2Device::start()
> > availableBuffers_.push(buffer.get());
> >
> > ret = output_->streamOn();
> > - if (ret)
> > + if (ret) {
> > freeBuffers();
> > + return ret;
> > + }
>
> I wonder if we shouldn't just call stop() everywhere, as it seems to
> me calling streamoff() and setFrameStartEnabled(false) is a no-op if
> the subdevice is not started and the signal not enabled. Minor detail
> though.
That would be a bit cleaner indeed, if possible.
> > - return ret;
> > + ret = csi2_->setFrameStartEnabled(true);
> > + if (ret) {
> > + stop();
> > + return ret;
> > + }
> > +
> > + return 0;
> > }
> >
> > int CIO2Device::stop()
> > {
> > - int ret = output_->streamOff();
> > + int ret;
> > +
> > + csi2_->setFrameStartEnabled(false);
> > +
> > + ret = output_->streamOff();
> >
> > freeBuffers();
> >
> > diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
> > index e8b491a0c104a3e2..67fede4e3b4e9fb6 100644
> > --- a/src/libcamera/pipeline/ipu3/cio2.h
> > +++ b/src/libcamera/pipeline/ipu3/cio2.h
> > @@ -13,6 +13,7 @@
> >
> > #include <libcamera/signal.h>
> >
> > +#include "libcamera/internal/v4l2_subdevice.h"
> > #include "libcamera/internal/v4l2_videodevice.h"
> >
> > namespace libcamera {
> > @@ -24,7 +25,6 @@ class PixelFormat;
> > class Request;
> > class Size;
> > class SizeRange;
> > -class V4L2Subdevice;
> > struct StreamConfiguration;
> >
> > class CIO2Device
> > @@ -56,6 +56,7 @@ public:
> > void tryReturnBuffer(FrameBuffer *buffer);
> > Signal<FrameBuffer *> &bufferReady() { return output_->bufferReady; }
> >
> > + Signal<uint32_t> &frameStart() { return csi2_->frameStart; }
>
> Please move this one line up and keep one empty line before private:
>
> Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > private:
> > void freeBuffers();
> >
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list