[libcamera-devel,RFC,3/3] libcamera: pipeline: rkisp1: Make fixed amount of internal buffer allocation more explicit

11885 diff mbox series

Now that bufferCount can be increased and there is a lc-compliance test
in place to check if the pipeline can handle more requests than it has
internal buffers, we need to be able to test it.

Make the internal buffer allocation always constant, so that by
increasing bufferCount we can simulate this scenario. Scaling the
internal buffers amount with bufferCount only hides the issue, that
should be handled by queueing the overflowing requests.

Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp    | 9 ++-------
 src/libcamera/pipeline/rkisp1/rkisp1_path.h | 4 ++--
 2 files changed, 4 insertions(+), 9 deletions(-)

Hi Nicolas,

On Fri, Apr 09, 2021 at 06:38:15PM -0300, Nícolas F. R. A. Prado wrote:
> Now that bufferCount can be increased and there is a lc-compliance test
> in place to check if the pipeline can handle more requests than it has
> internal buffers, we need to be able to test it.
>
> Make the internal buffer allocation always constant, so that by
> increasing bufferCount we can simulate this scenario. Scaling the
> internal buffers amount with bufferCount only hides the issue, that
> should be handled by queueing the overflowing requests.
>

Not a 100% sure I got it: is this patch meant to trigger an error just
for the purpose of testing ?

Thanks
  j

> Signed-off-by: Nícolas F. R. A. Prado <nfraprado@collabora.com>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp    | 9 ++-------
>  src/libcamera/pipeline/rkisp1/rkisp1_path.h | 4 ++--
>  2 files changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index c7b93b2804c7..0dc474f343fc 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -685,16 +685,11 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
>  	unsigned int ipaBufferId = 1;
>  	int ret;
>
> -	unsigned int maxCount = std::max({
> -		data->mainPathStream_.configuration().bufferCount,
> -		data->selfPathStream_.configuration().bufferCount,
> -	});
> -
> -	ret = param_->allocateBuffers(maxCount, &paramBuffers_);
> +	ret = param_->allocateBuffers(RKISP1_MIN_BUFFER_COUNT, &paramBuffers_);
>  	if (ret < 0)
>  		goto error;
>
> -	ret = stat_->allocateBuffers(maxCount, &statBuffers_);
> +	ret = stat_->allocateBuffers(RKISP1_MIN_BUFFER_COUNT, &statBuffers_);
>  	if (ret < 0)
>  		goto error;
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1_path.h b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> index 13291da89dc5..4ab4c0516ebc 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1_path.h
> @@ -21,6 +21,8 @@
>
>  namespace libcamera {
>
> +static constexpr unsigned int RKISP1_MIN_BUFFER_COUNT = 4;
> +
>  class MediaDevice;
>  class V4L2Subdevice;
>  struct StreamConfiguration;
> @@ -56,8 +58,6 @@ public:
>  	Signal<FrameBuffer *> &bufferReady() { return video_->bufferReady; }
>
>  private:
> -	static constexpr unsigned int RKISP1_MIN_BUFFER_COUNT = 4;
> -
>  	const char *name_;
>  	bool running_;
>
> --
> 2.31.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Em 2021-04-10 05:31, Jacopo Mondi escreveu:

> Hi Nicolas,
>
> On Fri, Apr 09, 2021 at 06:38:15PM -0300, Nícolas F. R. A. Prado wrote:
> > Now that bufferCount can be increased and there is a lc-compliance test
> > in place to check if the pipeline can handle more requests than it has
> > internal buffers, we need to be able to test it.
> >
> > Make the internal buffer allocation always constant, so that by
> > increasing bufferCount we can simulate this scenario. Scaling the
> > internal buffers amount with bufferCount only hides the issue, that
> > should be handled by queueing the overflowing requests.
> >
>
> Not a 100% sure I got it: is this patch meant to trigger an error just
> for the purpose of testing ?

More or less, yes. I think actually that this is something that would make sense
to merge only after other changes, but I sent it as a discussion starter.

Before this patch, a buffer overrun can happen on the internal buffer only if
the user allocates the buffers without using the FrameBufferAllocator. After the
patch, that can also happen using the FrameBufferAllocator if the bufferCount
value configured is above RKISP1_MIN_BUFFER_COUNT. So the way this is, it
actually increases the scenarios of failure, although in a way that is tested by
the new lc-compliance test.

From my discussion with Laurent on IRC, my understanding is that the issue
should be fixed by queuing the requests until there are free internal buffers
rather than allocating more of them. Additionally, it seems the idea in the
future is to remove bufferCount. With that in mind it makes sense to me to have
a fixed amount of internal buffers and only rely on queuing the overflowing
requests internally. But that also probably means that this change should be
made after the internal request queueing is implemented.

I think a change like this would make a lot more sense right now on the IPU3
pipeline since that already has a patch for solving the issue in the works [1],
but I made it for the rkisp1 since it's what I have and can test on.

Thanks,
Nícolas

[1] https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019108.html