[libcamera-devel,05/10] libcamera: ipu3: Calculate number of buffers for ImgU

3894 diff mbox series

Decouple the number of buffers to allocate for the ImgU from the number
of buffers allocated for the CIO2. Instead of blindly following the CIO2
pick the maximum number of buffers requested for any stream facing
applications.

This is potentially wasteful, as each stream could allocate just as many
buffers as requested by the application instead of the maximum from the
set. But this is not more wasteful then whats already used by the
pipeline and should be fixed on top after the decoupling of the two
processing units.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Hi Niklas,

On Tue, Jun 02, 2020 at 03:39:04AM +0200, Niklas Söderlund wrote:
> Decouple the number of buffers to allocate for the ImgU from the number
> of buffers allocated for the CIO2. Instead of blindly following the CIO2
> pick the maximum number of buffers requested for any stream facing
> applications.
>
> This is potentially wasteful, as each stream could allocate just as many
> buffers as requested by the application instead of the maximum from the
> set. But this is not more wasteful then whats already used by the
> pipeline and should be fixed on top after the decoupling of the two
> processing units.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 0e7555c716b36749..f4759715c6ae7572 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -729,14 +729,16 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
>  	IPU3CameraData *data = cameraData(camera);
>  	CIO2Device *cio2 = &data->cio2_;
>  	ImgUDevice *imgu = data->imgu_;
> -	unsigned int bufferCount;
> +	unsigned int bufferCount = 0;
>  	int ret;
>
>  	ret = cio2->allocateBuffers();
>  	if (ret < 0)
>  		return ret;
>
> -	bufferCount = ret;
> +	bufferCount = std::max<unsigned int>(data->outStream_.configuration().bufferCount, bufferCount);
> +	bufferCount = std::max<unsigned int>(data->vfStream_.configuration().bufferCount, bufferCount);
> +	bufferCount = std::max<unsigned int>(data->rawStream_.configuration().bufferCount, bufferCount);

I don't have a strong opinion on the underlying idea, I think it's
better than assuming we should report the number of buffer needed by
the CIO2 unit.

On the patch itself, could you shorten it by
        buffercount = max(outstream, vfStream);
        buffercounf = max(buffercount, rawStream)

That apart
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j
>
>  	ret = imgu->allocateBuffers(data, bufferCount);
>  	if (ret < 0) {
> --
> 2.26.2
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi Niklas,

Thank you for the patch.

On Tue, Jun 02, 2020 at 03:39:04AM +0200, Niklas Söderlund wrote:
> Decouple the number of buffers to allocate for the ImgU from the number
> of buffers allocated for the CIO2. Instead of blindly following the CIO2
> pick the maximum number of buffers requested for any stream facing
> applications.
> 
> This is potentially wasteful, as each stream could allocate just as many
> buffers as requested by the application instead of the maximum from the
> set. But this is not more wasteful then whats already used by the

s/then/than/
s/whats/what is/

> pipeline and should be fixed on top after the decoupling of the two
> processing units.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 0e7555c716b36749..f4759715c6ae7572 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -729,14 +729,16 @@ int PipelineHandlerIPU3::allocateBuffers(Camera *camera)
>  	IPU3CameraData *data = cameraData(camera);
>  	CIO2Device *cio2 = &data->cio2_;
>  	ImgUDevice *imgu = data->imgu_;
> -	unsigned int bufferCount;
> +	unsigned int bufferCount = 0;
>  	int ret;
>  
>  	ret = cio2->allocateBuffers();
>  	if (ret < 0)
>  		return ret;
>  
> -	bufferCount = ret;
> +	bufferCount = std::max<unsigned int>(data->outStream_.configuration().bufferCount, bufferCount);
> +	bufferCount = std::max<unsigned int>(data->vfStream_.configuration().bufferCount, bufferCount);
> +	bufferCount = std::max<unsigned int>(data->rawStream_.configuration().bufferCount, bufferCount);

Maybe

	bufferCount = std::max({
		data->outStream_.configuration().bufferCount,
		data->vfStream_.configuration().bufferCount,
		data->rawStream_.configuration().bufferCount
	});

?

We definitely need to handle the issue of buffer count configuration
sooner than later. I'm not opposed to this change if it helps within the
context of the whole series, but I wonder why the raw stream needs to be
included here.

>  
>  	ret = imgu->allocateBuffers(data, bufferCount);
>  	if (ret < 0) {