[libcamera-devel,v2,09/13] libcamera: ipu3: Do not duplicate data in IPU3Stream
diff mbox series

Message ID 20200628001532.2685967-10-niklas.soderlund@ragnatech.se
State Accepted
Headers show
Series
  • libcamera: ipu3: Refactoring of ImgU
Related show

Commit Message

Niklas Söderlund June 28, 2020, 12:15 a.m. UTC 
Do not keep the duplicated ImgUDevice::ImgUOutput information in both
the stream and camera data classes. Remove it from the stream and only
access it from the camera data class.

Which stream is which can instead be checked by comparing it to the
known streams in camera data. This match how streams are checked in
other parts of the code making the pipeline more coherent.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
---
* Changes since v1
- s/driver/pipeline/ in commit message.
- Reorder if ; if else ; else statement in
  PipelineHandlerIPU3::queueRequestDevice()
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 32 ++++++++++++++++------------
 1 file changed, 18 insertions(+), 14 deletions(-)

Comments

Laurent Pinchart June 28, 2020, 7:06 a.m. UTC | #1 
Hi Niklas,

Thank you for the patch.

On Sun, Jun 28, 2020 at 02:15:28AM +0200, Niklas Söderlund wrote:
> Do not keep the duplicated ImgUDevice::ImgUOutput information in both
> the stream and camera data classes. Remove it from the stream and only
> access it from the camera data class.
> 
> Which stream is which can instead be checked by comparing it to the
> known streams in camera data. This match how streams are checked in
> other parts of the code making the pipeline more coherent.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
> * Changes since v1
> - s/driver/pipeline/ in commit message.
> - Reorder if ; if else ; else statement in
>   PipelineHandlerIPU3::queueRequestDevice()
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 32 ++++++++++++++++------------
>  1 file changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index e817f842f1216a7f..c1520ec40fe7b57c 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -35,13 +35,11 @@ class IPU3Stream : public Stream
>  {
>  public:
>  	IPU3Stream()
> -		: active_(false), raw_(false), device_(nullptr)
> +		: active_(false)
>  	{
>  	}
>  
>  	bool active_;
> -	bool raw_;
> -	ImgUDevice::ImgUOutput *device_;
>  };
>  
>  class IPU3CameraData : public CameraData
> @@ -276,7 +274,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
>  		const StreamConfiguration oldCfg = cfg;
>  		const IPU3Stream *stream = streams_[i];
>  
> -		if (stream->raw_) {
> +		if (stream == &data_->rawStream_) {
>  			cfg = cio2Configuration_;
>  		} else {
>  			bool scale = stream == &data_->vfStream_;
> @@ -572,13 +570,16 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,
>  					    std::vector<std::unique_ptr<FrameBuffer>> *buffers)
>  {
>  	IPU3CameraData *data = cameraData(camera);
> -	IPU3Stream *ipu3stream = static_cast<IPU3Stream *>(stream);
>  	unsigned int count = stream->configuration().bufferCount;
>  
> -	if (ipu3stream->raw_)
> +	if (stream == &data->outStream_)
> +		return data->imgu_->output_.dev->exportBuffers(count, buffers);
> +	else if (stream == &data->vfStream_)
> +		return data->imgu_->viewfinder_.dev->exportBuffers(count, buffers);
> +	else if (stream == &data->rawStream_)
>  		return data->cio2_.exportBuffers(count, buffers);
>  
> -	return ipu3stream->device_->dev->exportBuffers(count, buffers);
> +	return -EINVAL;
>  }
>  
>  /**
> @@ -685,11 +686,17 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
>  	/* Queue all buffers from the request aimed for the ImgU. */
>  	for (auto it : request->buffers()) {
>  		IPU3Stream *stream = static_cast<IPU3Stream *>(it.first);
> -		if (stream->raw_)
> -			continue;
> -
>  		FrameBuffer *buffer = it.second;
> -		int ret = stream->device_->dev->queueBuffer(buffer);
> +
> +		if (stream == &data->rawStream_)
> +			continue;
> +
> +		int ret = 0;
> +		if (stream == &data->outStream_)
> +			ret = data->imgu_->output_.dev->queueBuffer(buffer);
> +		else if (stream == &data->vfStream_)
> +			ret = data->imgu_->viewfinder_.dev->queueBuffer(buffer);

		else
			continue;

or

		else
			ret = 0;

and dropping the previous &data->rawStream_ check would allow not
initializing the ret variable to 0.

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

> +
>  		if (ret < 0)
>  			error = ret;
>  	}
> @@ -801,9 +808,6 @@ int PipelineHandlerIPU3::registerCameras()
>  		 * second.
>  		 */
>  		data->imgu_ = numCameras ? &imgu1_ : &imgu0_;
> -		data->outStream_.device_ = &data->imgu_->output_;
> -		data->vfStream_.device_ = &data->imgu_->viewfinder_;
> -		data->rawStream_.raw_ = true;
>  
>  		/*
>  		 * Connect video devices' 'bufferReady' signals to their
Niklas Söderlund June 28, 2020, 12:03 p.m. UTC | #2 
Hi Laurent,

Thanks for your feedback.

On 2020-06-28 10:06:37 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Sun, Jun 28, 2020 at 02:15:28AM +0200, Niklas Söderlund wrote:
> > Do not keep the duplicated ImgUDevice::ImgUOutput information in both
> > the stream and camera data classes. Remove it from the stream and only
> > access it from the camera data class.
> > 
> > Which stream is which can instead be checked by comparing it to the
> > known streams in camera data. This match how streams are checked in
> > other parts of the code making the pipeline more coherent.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> > * Changes since v1
> > - s/driver/pipeline/ in commit message.
> > - Reorder if ; if else ; else statement in
> >   PipelineHandlerIPU3::queueRequestDevice()
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 32 ++++++++++++++++------------
> >  1 file changed, 18 insertions(+), 14 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index e817f842f1216a7f..c1520ec40fe7b57c 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -35,13 +35,11 @@ class IPU3Stream : public Stream
> >  {
> >  public:
> >  	IPU3Stream()
> > -		: active_(false), raw_(false), device_(nullptr)
> > +		: active_(false)
> >  	{
> >  	}
> >  
> >  	bool active_;
> > -	bool raw_;
> > -	ImgUDevice::ImgUOutput *device_;
> >  };
> >  
> >  class IPU3CameraData : public CameraData
> > @@ -276,7 +274,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> >  		const StreamConfiguration oldCfg = cfg;
> >  		const IPU3Stream *stream = streams_[i];
> >  
> > -		if (stream->raw_) {
> > +		if (stream == &data_->rawStream_) {
> >  			cfg = cio2Configuration_;
> >  		} else {
> >  			bool scale = stream == &data_->vfStream_;
> > @@ -572,13 +570,16 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,
> >  					    std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> >  {
> >  	IPU3CameraData *data = cameraData(camera);
> > -	IPU3Stream *ipu3stream = static_cast<IPU3Stream *>(stream);
> >  	unsigned int count = stream->configuration().bufferCount;
> >  
> > -	if (ipu3stream->raw_)
> > +	if (stream == &data->outStream_)
> > +		return data->imgu_->output_.dev->exportBuffers(count, buffers);
> > +	else if (stream == &data->vfStream_)
> > +		return data->imgu_->viewfinder_.dev->exportBuffers(count, buffers);
> > +	else if (stream == &data->rawStream_)
> >  		return data->cio2_.exportBuffers(count, buffers);
> >  
> > -	return ipu3stream->device_->dev->exportBuffers(count, buffers);
> > +	return -EINVAL;
> >  }
> >  
> >  /**
> > @@ -685,11 +686,17 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
> >  	/* Queue all buffers from the request aimed for the ImgU. */
> >  	for (auto it : request->buffers()) {
> >  		IPU3Stream *stream = static_cast<IPU3Stream *>(it.first);
> > -		if (stream->raw_)
> > -			continue;
> > -
> >  		FrameBuffer *buffer = it.second;
> > -		int ret = stream->device_->dev->queueBuffer(buffer);
> > +
> > +		if (stream == &data->rawStream_)
> > +			continue;
> > +
> > +		int ret = 0;
> > +		if (stream == &data->outStream_)
> > +			ret = data->imgu_->output_.dev->queueBuffer(buffer);
> > +		else if (stream == &data->vfStream_)
> > +			ret = data->imgu_->viewfinder_.dev->queueBuffer(buffer);
> 
> 		else
> 			continue;

I had it like this in v1 but Jacopo preferred the v2 version. I 
preferred the v1 version just like you so I will use it as Jacopo 
expressed no strong feelings.

> 
> or
> 
> 		else
> 			ret = 0;
> 
> and dropping the previous &data->rawStream_ check would allow not
> initializing the ret variable to 0.
> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > +
> >  		if (ret < 0)
> >  			error = ret;
> >  	}
> > @@ -801,9 +808,6 @@ int PipelineHandlerIPU3::registerCameras()
> >  		 * second.
> >  		 */
> >  		data->imgu_ = numCameras ? &imgu1_ : &imgu0_;
> > -		data->outStream_.device_ = &data->imgu_->output_;
> > -		data->vfStream_.device_ = &data->imgu_->viewfinder_;
> > -		data->rawStream_.raw_ = true;
> >  
> >  		/*
> >  		 * Connect video devices' 'bufferReady' signals to their
> 
> -- 
> Regards,
> 
> Laurent Pinchart
Jacopo Mondi June 28, 2020, 12:18 p.m. UTC | #3 
On Sun, Jun 28, 2020 at 02:03:44PM +0200, Niklas Söderlund wrote:
> Hi Laurent,
>
> Thanks for your feedback.
>
> On 2020-06-28 10:06:37 +0300, Laurent Pinchart wrote:
> > Hi Niklas,
> >
> > Thank you for the patch.
> >
> > On Sun, Jun 28, 2020 at 02:15:28AM +0200, Niklas Söderlund wrote:
> > > Do not keep the duplicated ImgUDevice::ImgUOutput information in both
> > > the stream and camera data classes. Remove it from the stream and only
> > > access it from the camera data class.
> > >
> > > Which stream is which can instead be checked by comparing it to the
> > > known streams in camera data. This match how streams are checked in
> > > other parts of the code making the pipeline more coherent.
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > > * Changes since v1
> > > - s/driver/pipeline/ in commit message.
> > > - Reorder if ; if else ; else statement in
> > >   PipelineHandlerIPU3::queueRequestDevice()
> > > ---
> > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 32 ++++++++++++++++------------
> > >  1 file changed, 18 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index e817f842f1216a7f..c1520ec40fe7b57c 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -35,13 +35,11 @@ class IPU3Stream : public Stream
> > >  {
> > >  public:
> > >  	IPU3Stream()
> > > -		: active_(false), raw_(false), device_(nullptr)
> > > +		: active_(false)
> > >  	{
> > >  	}
> > >
> > >  	bool active_;
> > > -	bool raw_;
> > > -	ImgUDevice::ImgUOutput *device_;
> > >  };
> > >
> > >  class IPU3CameraData : public CameraData
> > > @@ -276,7 +274,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> > >  		const StreamConfiguration oldCfg = cfg;
> > >  		const IPU3Stream *stream = streams_[i];
> > >
> > > -		if (stream->raw_) {
> > > +		if (stream == &data_->rawStream_) {
> > >  			cfg = cio2Configuration_;
> > >  		} else {
> > >  			bool scale = stream == &data_->vfStream_;
> > > @@ -572,13 +570,16 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,
> > >  					    std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > >  {
> > >  	IPU3CameraData *data = cameraData(camera);
> > > -	IPU3Stream *ipu3stream = static_cast<IPU3Stream *>(stream);
> > >  	unsigned int count = stream->configuration().bufferCount;
> > >
> > > -	if (ipu3stream->raw_)
> > > +	if (stream == &data->outStream_)
> > > +		return data->imgu_->output_.dev->exportBuffers(count, buffers);
> > > +	else if (stream == &data->vfStream_)
> > > +		return data->imgu_->viewfinder_.dev->exportBuffers(count, buffers);
> > > +	else if (stream == &data->rawStream_)
> > >  		return data->cio2_.exportBuffers(count, buffers);
> > >
> > > -	return ipu3stream->device_->dev->exportBuffers(count, buffers);
> > > +	return -EINVAL;
> > >  }
> > >
> > >  /**
> > > @@ -685,11 +686,17 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
> > >  	/* Queue all buffers from the request aimed for the ImgU. */
> > >  	for (auto it : request->buffers()) {
> > >  		IPU3Stream *stream = static_cast<IPU3Stream *>(it.first);
> > > -		if (stream->raw_)
> > > -			continue;
> > > -
> > >  		FrameBuffer *buffer = it.second;
> > > -		int ret = stream->device_->dev->queueBuffer(buffer);
> > > +
> > > +		if (stream == &data->rawStream_)
> > > +			continue;
> > > +
> > > +		int ret = 0;
> > > +		if (stream == &data->outStream_)
> > > +			ret = data->imgu_->output_.dev->queueBuffer(buffer);
> > > +		else if (stream == &data->vfStream_)
> > > +			ret = data->imgu_->viewfinder_.dev->queueBuffer(buffer);
> >
> > 		else
> > 			continue;
>
> I had it like this in v1 but Jacopo preferred the v2 version. I
> preferred the v1 version just like you so I will use it as Jacopo
> expressed no strong feelings.
>

Absolutely not, was just a suggestion. Sorry for the ping-pong.

> >
> > or
> >
> > 		else
> > 			ret = 0;
> >
> > and dropping the previous &data->rawStream_ check would allow not
> > initializing the ret variable to 0.
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >
> > > +
> > >  		if (ret < 0)
> > >  			error = ret;
> > >  	}
> > > @@ -801,9 +808,6 @@ int PipelineHandlerIPU3::registerCameras()
> > >  		 * second.
> > >  		 */
> > >  		data->imgu_ = numCameras ? &imgu1_ : &imgu0_;
> > > -		data->outStream_.device_ = &data->imgu_->output_;
> > > -		data->vfStream_.device_ = &data->imgu_->viewfinder_;
> > > -		data->rawStream_.raw_ = true;
> > >
> > >  		/*
> > >  		 * Connect video devices' 'bufferReady' signals to their
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
>
> --
> Regards,
> Niklas Söderlund
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index e817f842f1216a7f..c1520ec40fe7b57c 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -35,13 +35,11 @@  class IPU3Stream : public Stream
 {
 public:
 	IPU3Stream()
-		: active_(false), raw_(false), device_(nullptr)
+		: active_(false)
 	{
 	}
 
 	bool active_;
-	bool raw_;
-	ImgUDevice::ImgUOutput *device_;
 };
 
 class IPU3CameraData : public CameraData
@@ -276,7 +274,7 @@  CameraConfiguration::Status IPU3CameraConfiguration::validate()
 		const StreamConfiguration oldCfg = cfg;
 		const IPU3Stream *stream = streams_[i];
 
-		if (stream->raw_) {
+		if (stream == &data_->rawStream_) {
 			cfg = cio2Configuration_;
 		} else {
 			bool scale = stream == &data_->vfStream_;
@@ -572,13 +570,16 @@  int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,
 					    std::vector<std::unique_ptr<FrameBuffer>> *buffers)
 {
 	IPU3CameraData *data = cameraData(camera);
-	IPU3Stream *ipu3stream = static_cast<IPU3Stream *>(stream);
 	unsigned int count = stream->configuration().bufferCount;
 
-	if (ipu3stream->raw_)
+	if (stream == &data->outStream_)
+		return data->imgu_->output_.dev->exportBuffers(count, buffers);
+	else if (stream == &data->vfStream_)
+		return data->imgu_->viewfinder_.dev->exportBuffers(count, buffers);
+	else if (stream == &data->rawStream_)
 		return data->cio2_.exportBuffers(count, buffers);
 
-	return ipu3stream->device_->dev->exportBuffers(count, buffers);
+	return -EINVAL;
 }
 
 /**
@@ -685,11 +686,17 @@  int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
 	/* Queue all buffers from the request aimed for the ImgU. */
 	for (auto it : request->buffers()) {
 		IPU3Stream *stream = static_cast<IPU3Stream *>(it.first);
-		if (stream->raw_)
-			continue;
-
 		FrameBuffer *buffer = it.second;
-		int ret = stream->device_->dev->queueBuffer(buffer);
+
+		if (stream == &data->rawStream_)
+			continue;
+
+		int ret = 0;
+		if (stream == &data->outStream_)
+			ret = data->imgu_->output_.dev->queueBuffer(buffer);
+		else if (stream == &data->vfStream_)
+			ret = data->imgu_->viewfinder_.dev->queueBuffer(buffer);
+
 		if (ret < 0)
 			error = ret;
 	}
@@ -801,9 +808,6 @@  int PipelineHandlerIPU3::registerCameras()
 		 * second.
 		 */
 		data->imgu_ = numCameras ? &imgu1_ : &imgu0_;
-		data->outStream_.device_ = &data->imgu_->output_;
-		data->vfStream_.device_ = &data->imgu_->viewfinder_;
-		data->rawStream_.raw_ = true;
 
 		/*
 		 * Connect video devices' 'bufferReady' signals to their