[libcamera-devel,06/10] libcamera: ipu3: Implement buffer release
diff mbox series

Message ID 20190228200410.3022-7-jacopo@jmondi.org
State Superseded
Headers show
Series
  • libcamera: ipu3: ImgU support
Related show

Commit Message

Jacopo Mondi Feb. 28, 2019, 8:04 p.m. UTC 
Release buffers on all video devices in the pipeline.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 40 +++++++++++++++++++++++++---
 1 file changed, 36 insertions(+), 4 deletions(-)

Comments

Laurent Pinchart March 2, 2019, 10:54 p.m. UTC | #1 
Hi Jacopo,

Thank you for the patch.

On Thu, Feb 28, 2019 at 09:04:06PM +0100, Jacopo Mondi wrote:
> Release buffers on all video devices in the pipeline.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 40 +++++++++++++++++++++++++---
>  1 file changed, 36 insertions(+), 4 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index c7b7973952a0..60a48859b398 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -145,6 +145,8 @@ private:
>  	int initCio2(unsigned int index, Cio2Device *cio2);
>  	void registerCameras();
>  
> +	int releaseBuffers(V4L2Device *dev);
> +
>  	ImguDevice imgu0_;
>  	ImguDevice imgu1_;
>  
> @@ -371,13 +373,32 @@ error_reserve_memory:
>  int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream)
>  {
>  	IPU3CameraData *data = cameraData(camera);
> +	V4L2Device *viewfinder = data->imgu->viewfinder;
> +	V4L2Device *output = data->imgu->output;
> +	V4L2Device *input = data->imgu->input;
>  	V4L2Device *cio2 = data->cio2.output;
> +	V4L2Device *stat = data->imgu->stat;
> +	int ret;
>  
> -	int ret = cio2->releaseBuffers();
> -	if (ret) {
> -		LOG(IPU3, Error) << "Failed to release memory";
> +	ret = releaseBuffers(viewfinder);
> +	if (ret)
> +		return ret;

Let's not bail out early. If we fail to release buffers for one video
node, we can still try the other ones.

> +
> +	ret = releaseBuffers(stat);
> +	if (ret)
> +		return ret;
> +
> +	ret = releaseBuffers(output);
> +	if (ret)
> +		return ret;
> +
> +	ret = releaseBuffers(cio2);
> +	if (ret)
> +		return ret;
> +
> +	ret = releaseBuffers(input);
> +	if (ret)
>  		return ret;
> -	}
>  
>  	return 0;
>  }
> @@ -877,6 +898,17 @@ void PipelineHandlerIPU3::registerCameras()
>  	}
>  }
>  
> +int PipelineHandlerIPU3::releaseBuffers(V4L2Device *dev)
> +{
> +	int ret = dev->releaseBuffers();

Interestingly enough this function never fails. Should we turn it into a
void function, or keep the return type for later use ?

> +	if (ret) {
> +		LOG(IPU3, Error) << "Failed to release memory";
> +		return ret;
> +	}

I'd print this in V4L2Device::releaseBuffers(), not here. That way you
can remove the releaseBuffers() method in this class.

> +
> +	return 0;
> +}
> +
>  REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);
>  
>  } /* namespace libcamera */
Jacopo Mondi March 9, 2019, 8:59 p.m. UTC | #2 
Hi Laurent

On Sun, Mar 03, 2019 at 12:54:05AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Thu, Feb 28, 2019 at 09:04:06PM +0100, Jacopo Mondi wrote:
> > Release buffers on all video devices in the pipeline.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 40 +++++++++++++++++++++++++---
> >  1 file changed, 36 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index c7b7973952a0..60a48859b398 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -145,6 +145,8 @@ private:
> >  	int initCio2(unsigned int index, Cio2Device *cio2);
> >  	void registerCameras();
> >
> > +	int releaseBuffers(V4L2Device *dev);
> > +
> >  	ImguDevice imgu0_;
> >  	ImguDevice imgu1_;
> >
> > @@ -371,13 +373,32 @@ error_reserve_memory:
> >  int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream)
> >  {
> >  	IPU3CameraData *data = cameraData(camera);
> > +	V4L2Device *viewfinder = data->imgu->viewfinder;
> > +	V4L2Device *output = data->imgu->output;
> > +	V4L2Device *input = data->imgu->input;
> >  	V4L2Device *cio2 = data->cio2.output;
> > +	V4L2Device *stat = data->imgu->stat;
> > +	int ret;
> >
> > -	int ret = cio2->releaseBuffers();
> > -	if (ret) {
> > -		LOG(IPU3, Error) << "Failed to release memory";
> > +	ret = releaseBuffers(viewfinder);
> > +	if (ret)
> > +		return ret;
>
> Let's not bail out early. If we fail to release buffers for one video
> node, we can still try the other ones.
>
> > +
> > +	ret = releaseBuffers(stat);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = releaseBuffers(output);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = releaseBuffers(cio2);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = releaseBuffers(input);
> > +	if (ret)
> >  		return ret;
> > -	}
> >
> >  	return 0;
> >  }
> > @@ -877,6 +898,17 @@ void PipelineHandlerIPU3::registerCameras()
> >  	}
> >  }
> >
> > +int PipelineHandlerIPU3::releaseBuffers(V4L2Device *dev)
> > +{
> > +	int ret = dev->releaseBuffers();
>
> Interestingly enough this function never fails. Should we turn it into a
> void function, or keep the return type for later use ?
>
> > +	if (ret) {
> > +		LOG(IPU3, Error) << "Failed to release memory";
> > +		return ret;
> > +	}
>
> I'd print this in V4L2Device::releaseBuffers(), not here. That way you
> can remove the releaseBuffers() method in this class.
>

I'll drop this method, it doesn't even make the code nicer...

> > +
> > +	return 0;
> > +}
> > +
> >  REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);
> >
> >  } /* namespace libcamera */
>
> --
> Regards,
>
> Laurent Pinchart

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
index c7b7973952a0..60a48859b398 100644
--- a/src/libcamera/pipeline/ipu3/ipu3.cpp
+++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
@@ -145,6 +145,8 @@  private:
 	int initCio2(unsigned int index, Cio2Device *cio2);
 	void registerCameras();
 
+	int releaseBuffers(V4L2Device *dev);
+
 	ImguDevice imgu0_;
 	ImguDevice imgu1_;
 
@@ -371,13 +373,32 @@  error_reserve_memory:
 int PipelineHandlerIPU3::freeBuffers(Camera *camera, Stream *stream)
 {
 	IPU3CameraData *data = cameraData(camera);
+	V4L2Device *viewfinder = data->imgu->viewfinder;
+	V4L2Device *output = data->imgu->output;
+	V4L2Device *input = data->imgu->input;
 	V4L2Device *cio2 = data->cio2.output;
+	V4L2Device *stat = data->imgu->stat;
+	int ret;
 
-	int ret = cio2->releaseBuffers();
-	if (ret) {
-		LOG(IPU3, Error) << "Failed to release memory";
+	ret = releaseBuffers(viewfinder);
+	if (ret)
+		return ret;
+
+	ret = releaseBuffers(stat);
+	if (ret)
+		return ret;
+
+	ret = releaseBuffers(output);
+	if (ret)
+		return ret;
+
+	ret = releaseBuffers(cio2);
+	if (ret)
+		return ret;
+
+	ret = releaseBuffers(input);
+	if (ret)
 		return ret;
-	}
 
 	return 0;
 }
@@ -877,6 +898,17 @@  void PipelineHandlerIPU3::registerCameras()
 	}
 }
 
+int PipelineHandlerIPU3::releaseBuffers(V4L2Device *dev)
+{
+	int ret = dev->releaseBuffers();
+	if (ret) {
+		LOG(IPU3, Error) << "Failed to release memory";
+		return ret;
+	}
+
+	return 0;
+}
+
 REGISTER_PIPELINE_HANDLER(PipelineHandlerIPU3);
 
 } /* namespace libcamera */