[libcamera-devel,06/11] libcamera: ipu3: cio2: Generate start of frame event
diff mbox series

Message ID 20201105001546.1690179-7-niklas.soderlund@ragnatech.se
State Superseded
Delegated to: Niklas Söderlund
Headers show
Series
  • libcamera: ipu3: Attach to an skeleton IPA
Related show

Commit Message

Niklas Söderlund Nov. 5, 2020, 12:15 a.m. UTC 
Propagate the frameStart event whenever the CSI-2 receiver in the CIO2
pipeline generates one.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/libcamera/pipeline/ipu3/cio2.cpp | 18 +++++++++++++++---
 src/libcamera/pipeline/ipu3/cio2.h   |  3 ++-
 2 files changed, 17 insertions(+), 4 deletions(-)

Comments

Kieran Bingham Nov. 5, 2020, 11:39 a.m. UTC | #1 
Hi Niklas,

On 05/11/2020 00:15, Niklas Söderlund wrote:
> Propagate the frameStart event whenever the CSI-2 receiver in the CIO2
> pipeline generates one.
> 

I kinda like how these signals are quite flexible.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> Signed-off-by: Niklas Söderlund <niklas.soderlund@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;
> +	}
>  
> -	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; }
>  private:
>  	void freeBuffers();
>  
>
Jacopo Mondi Nov. 9, 2020, 11:13 a.m. UTC | #2 
Hi Niklas,

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

>
> -	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@jmondi.org>

Thanks
   j
>  private:
>  	void freeBuffers();
>
> --
> 2.29.2
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Dec. 8, 2020, 1:58 a.m. UTC | #3 
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@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@jmondi.org>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> >  private:
> >  	void freeBuffers();
> >

Patch
diff mbox series 

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;
+	}
 
-	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; }
 private:
 	void freeBuffers();