[libcamera-devel,v4,06/12] libcamera: ipu3: Queue requests for multiple streams

961 diff mbox series

Add support for queueing requests for multiple streams in the IPU3
pipeline handler class. The output video node should be queued with
buffers even if not part of the requested streams.

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

Hi Jacopo,

Thanks for your work.

On 2019-04-09 21:25:42 +0200, Jacopo Mondi wrote:
> Add support for queueing requests for multiple streams in the IPU3
> pipeline handler class. The output video node should be queued with
> buffers even if not part of the requested streams.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 46 +++++++++++++++++++++-------
>  1 file changed, 35 insertions(+), 11 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index f5d768f9f87f..bb8d4ce644ca 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -146,6 +146,7 @@ public:
>  	std::string name_;
>  	ImgUDevice::ImgUOutput *device_;
>  	const StreamConfiguration *cfg_;
> +	unsigned int bufferCount;

I do not understand the full usage of bufferCount, I think see bellow.

>  };
>  
>  class PipelineHandlerIPU3 : public PipelineHandler
> @@ -473,6 +474,9 @@ int PipelineHandlerIPU3::start(Camera *camera)
>  	ImgUDevice *imgu = data->imgu_;
>  	int ret;
>  
> +	if (!data->outStream_.active_)
> +		data->outStream_.bufferCount = 0;
> +
>  	/*
>  	 * Start the ImgU video devices, buffers will be queued to the
>  	 * ImgU output and viewfinder when requests will be queued.
> @@ -513,20 +517,40 @@ void PipelineHandlerIPU3::stop(Camera *camera)
>  int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)
>  {
>  	IPU3CameraData *data = cameraData(camera);
> -	V4L2Device *output = data->imgu_->output_.dev;
> -	IPU3Stream *stream = &data->outStream_;
> +	IPU3Stream *output = &data->outStream_;
> +	bool outputRequested = false;
>  
> -	/* Queue a buffer to the ImgU output for capture. */
> -	Buffer *buffer = request->findBuffer(stream);
> -	if (!buffer) {
> -		LOG(IPU3, Error)
> -			<< "Attempt to queue request with invalid stream";
> -		return -ENOENT;
> +	for (auto const &bufferMap : request->bufferMap()) {
> +		IPU3Stream *stream = static_cast<IPU3Stream *>(bufferMap.first);
> +		Buffer *buffer = bufferMap.second;
> +
> +		int ret = stream->device_->dev->queueBuffer(buffer);
> +		if (ret)
> +			continue;
> +
> +		if (stream == output)
> +			outputRequested = true;
>  	}
>  
> -	int ret = output->queueBuffer(buffer);
> -	if (ret < 0)
> -		return ret;
> +	/*
> +	 * The output node should be queued with buffers even if not part
> +	 * of the request, otherwise the ImgU stalls.
> +	 *
> +	 * Use buffers from the stream's pool or the internal pool depending
> +	 * if the output stream has been configured or not.
> +	 */
> +	if (!outputRequested) {
> +		BufferPool *pool = output->active_ ? &output->bufferPool()
> +						   : output->device_->pool;
> +		Buffer *buffer = &pool->buffers()[output->bufferCount];
> +		unsigned int numBuffers = pool->count();
> +
> +		int ret = output->device_->dev->queueBuffer(buffer);
> +		if (ret)
> +			return ret;
> +
> +		output->bufferCount = (output->bufferCount + 1) % numBuffers;

This looks worthy of a comment to me, I can't really tell what's going 
on. You cyclically pick buffers from the pool's buffers. I'm the first 
one to admit I'm no expert in the Buffer part of libcamera so I have to 
ask, is there any grantee the buffer picked is free? I'm thinking about 
under-run situations. Maybe it's just my inexperience that is showing 
and this is correct.

> +	}
>  
>  	PipelineHandler::queueRequest(camera, request);
>  
> -- 
> 2.21.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

HI Niklas,

On Sun, Apr 14, 2019 at 10:05:16PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2019-04-09 21:25:42 +0200, Jacopo Mondi wrote:
> > Add support for queueing requests for multiple streams in the IPU3
> > pipeline handler class. The output video node should be queued with
> > buffers even if not part of the requested streams.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 46 +++++++++++++++++++++-------
> >  1 file changed, 35 insertions(+), 11 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index f5d768f9f87f..bb8d4ce644ca 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -146,6 +146,7 @@ public:
> >  	std::string name_;
> >  	ImgUDevice::ImgUOutput *device_;
> >  	const StreamConfiguration *cfg_;
> > +	unsigned int bufferCount;
>
> I do not understand the full usage of bufferCount, I think see bellow.
>
> >  };
> >
> >  class PipelineHandlerIPU3 : public PipelineHandler
> > @@ -473,6 +474,9 @@ int PipelineHandlerIPU3::start(Camera *camera)
> >  	ImgUDevice *imgu = data->imgu_;
> >  	int ret;
> >
> > +	if (!data->outStream_.active_)
> > +		data->outStream_.bufferCount = 0;
> > +
> >  	/*
> >  	 * Start the ImgU video devices, buffers will be queued to the
> >  	 * ImgU output and viewfinder when requests will be queued.
> > @@ -513,20 +517,40 @@ void PipelineHandlerIPU3::stop(Camera *camera)
> >  int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)
> >  {
> >  	IPU3CameraData *data = cameraData(camera);
> > -	V4L2Device *output = data->imgu_->output_.dev;
> > -	IPU3Stream *stream = &data->outStream_;
> > +	IPU3Stream *output = &data->outStream_;
> > +	bool outputRequested = false;
> >
> > -	/* Queue a buffer to the ImgU output for capture. */
> > -	Buffer *buffer = request->findBuffer(stream);
> > -	if (!buffer) {
> > -		LOG(IPU3, Error)
> > -			<< "Attempt to queue request with invalid stream";
> > -		return -ENOENT;
> > +	for (auto const &bufferMap : request->bufferMap()) {
> > +		IPU3Stream *stream = static_cast<IPU3Stream *>(bufferMap.first);
> > +		Buffer *buffer = bufferMap.second;
> > +
> > +		int ret = stream->device_->dev->queueBuffer(buffer);
> > +		if (ret)
> > +			continue;
> > +
> > +		if (stream == output)
> > +			outputRequested = true;
> >  	}
> >
> > -	int ret = output->queueBuffer(buffer);
> > -	if (ret < 0)
> > -		return ret;
> > +	/*
> > +	 * The output node should be queued with buffers even if not part
> > +	 * of the request, otherwise the ImgU stalls.
> > +	 *
> > +	 * Use buffers from the stream's pool or the internal pool depending
> > +	 * if the output stream has been configured or not.
> > +	 */
> > +	if (!outputRequested) {
> > +		BufferPool *pool = output->active_ ? &output->bufferPool()
> > +						   : output->device_->pool;
> > +		Buffer *buffer = &pool->buffers()[output->bufferCount];
> > +		unsigned int numBuffers = pool->count();
> > +
> > +		int ret = output->device_->dev->queueBuffer(buffer);
> > +		if (ret)
> > +			return ret;
> > +
> > +		output->bufferCount = (output->bufferCount + 1) % numBuffers;
>
> This looks worthy of a comment to me, I can't really tell what's going
> on. You cyclically pick buffers from the pool's buffers. I'm the first
> one to admit I'm no expert in the Buffer part of libcamera so I have to
> ask, is there any grantee the buffer picked is free? I'm thinking about
> under-run situations. Maybe it's just my inexperience that is showing
> and this is correct.

No, it's not correct, but it opens up  an opportunity for further
discussions.

The comment at the beginning of the if() switch I think it's clear,
and bufferCount is a counter that it is used to pick buffers from the
internal pool. It's not nice and will have to be changed.

The fact is that queuing and dequeuing frames from 'output' is mandatory to
have ImgU operations triggered and you can't get any frame out from
viewfinder if output has no buffer queued.

Now, there are three possible configurations:
1) output is not part of the configured streams
2) output is configured and in all capture requests
3) output is configured but not part of all the capture requests

1) is easy: we use the internal buffer pool in this case
   - reserve enough buffers in the output (as many as the ones
     reserved for the viewfinder)
   - export them to an internal pool
   - for every request queue a new buffer from the internal pool to
     the 'output' capture device
   - when a buffer completes from viewfinder the request is completed, the
     buffer from output had completed already and will be discarded as
     it it not connected to any request
   - if applications queue a number of request > than the buffer pool
     size all at once (before enough request completes) we could get
     into a buffer underrun

2) It's easy as well, as we act the same but buffers are mapped to the
   Stream's pool. All request will include buffers from the 'output'
   node. In the same way, if applications queue 100 requests before
   completing any, than what happens I guess depends on the IO mode
   implemented by the pipeline handler: the request queuing could
   block on QBUF or if working in non-blocking mode it would return an
   error. I think this should be standardized and we need to specify
   how pipeline handlers should behave in such a case (or is it already?)

3) It's not easy, as 'output' is configured, but not all requests
   include it in the list of streams to capture from. In that case
   (output is not included) we need to queue buffers to it to get
   anything out from viewfinder and stat. And that's where my ugly
   bufferCount gets into play: it picks buffers from the internal pool
   and use them for to trigger ImgU processing.

   Now we have two problems:
   a) memory mapping and exporting
      As 'output' is configured, its buffers have been exported to the
      stream pool, not to the internal one. Sincerely, I'm not sure
      why it works at the moment, as the internal pool's buffers
      should be invalid. And I'm not sure how to handle this neither,
      one possible solutions could be:
      a.1) Export always to the internal pool without allocating, as
           allocation happens when exporting to the Stream's pool,
           but this mean we have exported the same buffers to two
           different pools and we alternate them depending on the
           content of the request... not sure it is desirable.
   b) buffer underrun
      This is of less concern to me, as it reduce to the same as 1)
      as if we have viewfinder complete, it means that output
      completed as well, so it's safe to assume that if we got a frame
      from viewfinder, the same has completed on output too.

   The bufferCount mechanism is an hack, and should be replaced with a
   dynamic queue of available buffers probably, but that does not
   solve how to handle allocation and queueing in case 3)

   How do you think it should be handled?

>
> > +	}
> >
> >  	PipelineHandler::queueRequest(camera, request);
> >
> > --
> > 2.21.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel@lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund

Hi Jacopo,

On Mon, Apr 15, 2019 at 11:25:37AM +0200, Jacopo Mondi wrote:
> On Sun, Apr 14, 2019 at 10:05:16PM +0200, Niklas Söderlund wrote:
> > On 2019-04-09 21:25:42 +0200, Jacopo Mondi wrote:
> >> Add support for queueing requests for multiple streams in the IPU3
> >> pipeline handler class. The output video node should be queued with
> >> buffers even if not part of the requested streams.
> >>
> >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >> ---
> >>  src/libcamera/pipeline/ipu3/ipu3.cpp | 46 +++++++++++++++++++++-------
> >>  1 file changed, 35 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> index f5d768f9f87f..bb8d4ce644ca 100644
> >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> >> @@ -146,6 +146,7 @@ public:
> >>  	std::string name_;
> >>  	ImgUDevice::ImgUOutput *device_;
> >>  	const StreamConfiguration *cfg_;
> >> +	unsigned int bufferCount;
> >
> > I do not understand the full usage of bufferCount, I think see bellow.
> >
> >>  };
> >>
> >>  class PipelineHandlerIPU3 : public PipelineHandler
> >> @@ -473,6 +474,9 @@ int PipelineHandlerIPU3::start(Camera *camera)
> >>  	ImgUDevice *imgu = data->imgu_;
> >>  	int ret;
> >>
> >> +	if (!data->outStream_.active_)
> >> +		data->outStream_.bufferCount = 0;
> >> +
> >>  	/*
> >>  	 * Start the ImgU video devices, buffers will be queued to the
> >>  	 * ImgU output and viewfinder when requests will be queued.
> >> @@ -513,20 +517,40 @@ void PipelineHandlerIPU3::stop(Camera *camera)
> >>  int PipelineHandlerIPU3::queueRequest(Camera *camera, Request *request)
> >>  {
> >>  	IPU3CameraData *data = cameraData(camera);
> >> -	V4L2Device *output = data->imgu_->output_.dev;
> >> -	IPU3Stream *stream = &data->outStream_;
> >> +	IPU3Stream *output = &data->outStream_;
> >> +	bool outputRequested = false;
> >>
> >> -	/* Queue a buffer to the ImgU output for capture. */
> >> -	Buffer *buffer = request->findBuffer(stream);
> >> -	if (!buffer) {
> >> -		LOG(IPU3, Error)
> >> -			<< "Attempt to queue request with invalid stream";
> >> -		return -ENOENT;
> >> +	for (auto const &bufferMap : request->bufferMap()) {

bufferMap is not a map. You can name this

	for (auto it : request->bufferMap()) {

> >> +		IPU3Stream *stream = static_cast<IPU3Stream *>(bufferMap.first);
> >> +		Buffer *buffer = bufferMap.second;
> >> +
> >> +		int ret = stream->device_->dev->queueBuffer(buffer);
> >> +		if (ret)
> >> +			continue;
> >> +
> >> +		if (stream == output)
> >> +			outputRequested = true;
> >>  	}
> >>
> >> -	int ret = output->queueBuffer(buffer);
> >> -	if (ret < 0)
> >> -		return ret;
> >> +	/*
> >> +	 * The output node should be queued with buffers even if not part
> >> +	 * of the request, otherwise the ImgU stalls.
> >> +	 *
> >> +	 * Use buffers from the stream's pool or the internal pool depending
> >> +	 * if the output stream has been configured or not.
> >> +	 */
> >> +	if (!outputRequested) {
> >> +		BufferPool *pool = output->active_ ? &output->bufferPool()
> >> +						   : output->device_->pool;
> >> +		Buffer *buffer = &pool->buffers()[output->bufferCount];
> >> +		unsigned int numBuffers = pool->count();
> >> +
> >> +		int ret = output->device_->dev->queueBuffer(buffer);
> >> +		if (ret)
> >> +			return ret;
> >> +
> >> +		output->bufferCount = (output->bufferCount + 1) % numBuffers;
> >
> > This looks worthy of a comment to me, I can't really tell what's going
> > on. You cyclically pick buffers from the pool's buffers. I'm the first
> > one to admit I'm no expert in the Buffer part of libcamera so I have to
> > ask, is there any grantee the buffer picked is free? I'm thinking about
> > under-run situations. Maybe it's just my inexperience that is showing
> > and this is correct.
> 
> No, it's not correct, but it opens up  an opportunity for further
> discussions.
> 
> The comment at the beginning of the if() switch I think it's clear,
> and bufferCount is a counter that it is used to pick buffers from the
> internal pool. It's not nice and will have to be changed.
> 
> The fact is that queuing and dequeuing frames from 'output' is mandatory to
> have ImgU operations triggered and you can't get any frame out from
> viewfinder if output has no buffer queued.
> 
> Now, there are three possible configurations:
> 1) output is not part of the configured streams
> 2) output is configured and in all capture requests
> 3) output is configured but not part of all the capture requests
> 
> 1) is easy: we use the internal buffer pool in this case
>    - reserve enough buffers in the output (as many as the ones
>      reserved for the viewfinder)
>    - export them to an internal pool
>    - for every request queue a new buffer from the internal pool to
>      the 'output' capture device
>    - when a buffer completes from viewfinder the request is completed, the
>      buffer from output had completed already and will be discarded as
>      it it not connected to any request
>    - if applications queue a number of request > than the buffer pool
>      size all at once (before enough request completes) we could get
>      into a buffer underrun
> 
> 2) It's easy as well, as we act the same but buffers are mapped to the
>    Stream's pool. All request will include buffers from the 'output'
>    node. In the same way, if applications queue 100 requests before
>    completing any, than what happens I guess depends on the IO mode
>    implemented by the pipeline handler: the request queuing could
>    block on QBUF or if working in non-blocking mode it would return an
>    error. I think this should be standardized and we need to specify
>    how pipeline handlers should behave in such a case (or is it already?)
> 
> 3) It's not easy, as 'output' is configured, but not all requests
>    include it in the list of streams to capture from. In that case
>    (output is not included) we need to queue buffers to it to get
>    anything out from viewfinder and stat. And that's where my ugly
>    bufferCount gets into play: it picks buffers from the internal pool
>    and use them for to trigger ImgU processing.
> 
>    Now we have two problems:
>    a) memory mapping and exporting
>       As 'output' is configured, its buffers have been exported to the
>       stream pool, not to the internal one. Sincerely, I'm not sure
>       why it works at the moment, as the internal pool's buffers
>       should be invalid. And I'm not sure how to handle this neither,
>       one possible solutions could be:
>       a.1) Export always to the internal pool without allocating, as
>            allocation happens when exporting to the Stream's pool,
>            but this mean we have exported the same buffers to two
>            different pools and we alternate them depending on the
>            content of the request... not sure it is desirable.
>    b) buffer underrun
>       This is of less concern to me, as it reduce to the same as 1)
>       as if we have viewfinder complete, it means that output
>       completed as well, so it's safe to assume that if we got a frame
>       from viewfinder, the same has completed on output too.
> 
>    The bufferCount mechanism is an hack, and should be replaced with a
>    dynamic queue of available buffers probably, but that does not
>    solve how to handle allocation and queueing in case 3)
> 
>    How do you think it should be handled?

I think you should allocate extra buffers for the output video node, and
use them as scratch buffers in case a request doesn't contain any output
buffer. Possibly easier said than done though, I haven't checked. A
single scratch buffer will unfortunately not work as it would prevent
queuing multiple requests, so you have to basically double the number of
buffers.

This should really be fixed on the driver side.

> >> +	}
> >>
> >>  	PipelineHandler::queueRequest(camera, request);
> >>