[libcamera-devel] [PATCH v4 10/12] libcamera: ipu3: Connect viewfinder's BufferReady signal

[Jacopo Mondi]

Hi Niklas,

On Sun, Apr 14, 2019 at 10:32:29PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your work.
>
> On 2019-04-09 21:25:46 +0200, Jacopo Mondi wrote:
> > Connect the viewfinder buffer ready signal to the IPU3CameraData slot
> > that complets the buffer first, and if not waiting for other buffers
> > completes the request as well.
> >
> > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 43 ++++++++++++++++++++++++----
> >  1 file changed, 37 insertions(+), 6 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index bb8d4ce644ca..75ffdc56d157 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -182,7 +182,7 @@ private:
> >  		{
> >  		}
> >
> > -		void imguOutputBufferReady(Buffer *buffer);
> > +		void imguCaptureBufferReady(Buffer *buffer);
> >  		void imguInputBufferReady(Buffer *buffer);
> >  		void cio2BufferReady(Buffer *buffer);
> >
> > @@ -722,7 +722,9 @@ int PipelineHandlerIPU3::registerCameras()
> >  		data->imgu_->input_->bufferReady.connect(data.get(),
> >  					&IPU3CameraData::imguInputBufferReady);
> >  		data->imgu_->output_.dev->bufferReady.connect(data.get(),
> > -					&IPU3CameraData::imguOutputBufferReady);
> > +					&IPU3CameraData::imguCaptureBufferReady);
> > +		data->imgu_->viewfinder_.dev->bufferReady.connect(data.get(),
> > +					&IPU3CameraData::imguCaptureBufferReady);
> >
> >
> >  		/* Initialize and register the Camera and its streams. */
> > @@ -769,12 +771,41 @@ void PipelineHandlerIPU3::IPU3CameraData::imguInputBufferReady(Buffer *buffer)
> >   *
> >   * Buffers completed from the ImgU output are directed to the application.
> >   */
> > -void PipelineHandlerIPU3::IPU3CameraData::imguOutputBufferReady(Buffer *buffer)
> > +void PipelineHandlerIPU3::IPU3CameraData::imguCaptureBufferReady(Buffer *buffer)
> >  {
> > -	Request *request = queuedRequests_.front();
> > +	Request *request = buffer->request();

When commenting patch #9 you said you don't like this.
I agree on the part that makes the 'request' and 'setRequest' method
public and we might need to play with 'friend' to limit the
accessibility of those method, but you also said you don't like how it
is used. How would you like to see this being used ?


> > +	if (!request)
> > +		/*
> > +		 * Completed buffers not part of a request are ignored
> > +		 * (they most probably come from the output stream
> > +		 * internal pool)
> > +		 */
> > +		return;
> > +
> > +	if (!pipe_->completeBuffer(camera_, request, buffer))
> > +		/* Request not completed yet, return here. */
> > +		return;
> > +
> > +	/*
> > +	 * Complete requests in queuing order: if some other request is
> > +	 * pending, post-pone completion.
> > +	 */
> > +	Request *front = queuedRequests_.front();
> > +	if (front != request)
> > +		return;
>
> I'm not sure, but i think this needs to be moved to the framework
> somehow. It will be slight variations of how this will be implemented in
> different pipeline handlers if we leave it to each pipeline handler to
> implement ordered completion of requests.
>

I agree, but not right now. We agreed when discussing v3 to do this
for IPU3 and see how it looks like. The second 'real' pipeline handler
we'll have we'll try to move this one layer up.

> Best case it should happen transparent to the pipeline handlers, but
> that might be a tall order. How about implementing a
> PipelineHandler::completeReady() helper which implementations can call
> which would take care of dequeuing requests in a ordered fashion? Or if
> you have a nice idea for how to skip the helper and move to something
> transparent that would be even better.
>
> >
> > -	pipe_->completeBuffer(camera_, request, buffer);
> > -	pipe_->completeRequest(camera_, request);
> > +	/*
> > +	 * Complete the current request, and all the other pending ones,
> > +	 * in queuing order.
> > +	 */
> > +	while (1) {
> > +		if (front->empty())
> > +			pipe_->completeRequest(camera_, front);
> > +		else
> > +			break;
> > +
> > +		front = queuedRequests_.front();
> > +	}
> >  }
> >
> >  /**
> > --
> > 2.21.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel
>
> --
> Regards,
> Niklas Söderlund
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.libcamera.org/pipermail/libcamera-devel/attachments/20190415/6813df90/attachment.sig>