[libcamera-devel] [PATCH 03/13] libcamera: ipu3: Always import buffers for ImgU sinks

[Niklas Söderlund]

Hi Jacopo,

Thanks for your feedback.

On 2020-06-27 11:56:51 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Sat, Jun 27, 2020 at 05:00:33AM +0200, Niklas Söderlund wrote:
> > When the IPU3 pipeline was first developed buffers of sinks from the
> > ImgU that where not active still needed to have allocated buffers
> 
> I think somewhere there's a "buffer" too much, I get what you mean,
> but is slightly confused.

Me too, will fix.

> 
> > associated with them or streaming was not allowed to start. This is no
> > longer true, it's enough that the sinks have imported buffers for
> > streaming to start. As we already need to import buffers for streams
> > that are active we can align the two cases and always import buffers.
> 
> as this changed in the driver since we first developed this, or has
> this always been the case?

No idea, I recall trying something like this a year back on the old 
kernel. But I have not installed the kernel or diffed the source code 
between the kernel now and then nor the libcamera sources between now 
and the state it was around Automotive summit last year. It felt like a 
bit of busy work as we know the IPU3 kernel driver is not the best 
quality.

> 
> Also, as asked in 2/13, do we know which output has to have buffers
> mandatorily allocated ?

Only thru experimentation and reading the code. I have no official 
documentation.

> 
> >
> > With this there is no longer a reason to store the allocated
> > FrameBuffers to keep them alive and the vector tracking them can be
> > removed.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 25 ++++---------------------
> >  1 file changed, 4 insertions(+), 21 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index 405550b1302fb370..5a473e18c082cee8 100644
> > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > @@ -49,7 +49,6 @@ public:
> >  		V4L2VideoDevice *dev;
> >  		unsigned int pad;
> >  		std::string name;
> > -		std::vector<std::unique_ptr<FrameBuffer>> buffers;
> >  	};
> >
> >  	ImgUDevice()
> > @@ -1124,9 +1123,6 @@ int ImgUDevice::configureOutput(ImgUOutput *output,
> >   */
> >  int ImgUDevice::allocateBuffers(IPU3CameraData *data, unsigned int bufferCount)
> >  {
> > -	IPU3Stream *outStream = &data->outStream_;
> > -	IPU3Stream *vfStream = &data->vfStream_;
> > -
> >  	/* Share buffers between CIO2 output and ImgU input. */
> >  	int ret = input_->importBuffers(bufferCount);
> >  	if (ret) {
> > @@ -1147,28 +1143,15 @@ int ImgUDevice::allocateBuffers(IPU3CameraData *data, unsigned int bufferCount)
> >  		goto error;
> >  	}
> >
> > -	/*
> > -	 * Allocate buffers for both outputs. If an output is active, prepare
> > -	 * for buffer import, otherwise allocate internal buffers. Use the same
> > -	 * number of buffers in either case.
> > -	 */
> > -	if (outStream->active_)
> > -		ret = output_.dev->importBuffers(bufferCount);
> > -	else
> > -		ret = output_.dev->allocateBuffers(bufferCount,
> > -						   &output_.buffers);
> > +	ret = output_.dev->importBuffers(bufferCount);
> >  	if (ret < 0) {
> > -		LOG(IPU3, Error) << "Failed to allocate ImgU output buffers";
> > +		LOG(IPU3, Error) << "Failed to import ImgU output buffers";
> 
> Nit: I would leave the same error message, as the function that fails
> is called ImgUDevice::allocateBuffers(). The video device will
> complain anyway for the import error.

I would not, as this aligns to others messages in this function which 
have done the switch allocate -> import with similar changes. Regarding 
the function name be at ease I hope to refactor it away in a future 
series :-)

> 
> >  		goto error;
> >  	}
> >
> > -	if (vfStream->active_)
> > -		ret = viewfinder_.dev->importBuffers(bufferCount);
> > -	else
> > -		ret = viewfinder_.dev->allocateBuffers(bufferCount,
> > -						       &viewfinder_.buffers);
> > +	ret = viewfinder_.dev->importBuffers(bufferCount);
> >  	if (ret < 0) {
> > -		LOG(IPU3, Error) << "Failed to allocate ImgU viewfinder buffers";
> > +		LOG(IPU3, Error) << "Failed to import ImgU viewfinder buffers";
> >  		goto error;
> >  	}
> >
> > --
> > 2.27.0
> >
> > _______________________________________________
> > libcamera-devel mailing list
> > libcamera-devel at lists.libcamera.org
> > https://lists.libcamera.org/listinfo/libcamera-devel

-- 
Regards,
Niklas Söderlund