[libcamera-devel] [PATCH v2 09/13] libcamera: ipu3: Do not duplicate data in IPU3Stream

[Jacopo Mondi]

On Sun, Jun 28, 2020 at 02:03:44PM +0200, Niklas Söderlund wrote:
> Hi Laurent,
>
> Thanks for your feedback.
>
> On 2020-06-28 10:06:37 +0300, Laurent Pinchart wrote:
> > Hi Niklas,
> >
> > Thank you for the patch.
> >
> > On Sun, Jun 28, 2020 at 02:15:28AM +0200, Niklas Söderlund wrote:
> > > Do not keep the duplicated ImgUDevice::ImgUOutput information in both
> > > the stream and camera data classes. Remove it from the stream and only
> > > access it from the camera data class.
> > >
> > > Which stream is which can instead be checked by comparing it to the
> > > known streams in camera data. This match how streams are checked in
> > > other parts of the code making the pipeline more coherent.
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> > > ---
> > > * Changes since v1
> > > - s/driver/pipeline/ in commit message.
> > > - Reorder if ; if else ; else statement in
> > >   PipelineHandlerIPU3::queueRequestDevice()
> > > ---
> > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 32 ++++++++++++++++------------
> > >  1 file changed, 18 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > index e817f842f1216a7f..c1520ec40fe7b57c 100644
> > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > @@ -35,13 +35,11 @@ class IPU3Stream : public Stream
> > >  {
> > >  public:
> > >  	IPU3Stream()
> > > -		: active_(false), raw_(false), device_(nullptr)
> > > +		: active_(false)
> > >  	{
> > >  	}
> > >
> > >  	bool active_;
> > > -	bool raw_;
> > > -	ImgUDevice::ImgUOutput *device_;
> > >  };
> > >
> > >  class IPU3CameraData : public CameraData
> > > @@ -276,7 +274,7 @@ CameraConfiguration::Status IPU3CameraConfiguration::validate()
> > >  		const StreamConfiguration oldCfg = cfg;
> > >  		const IPU3Stream *stream = streams_[i];
> > >
> > > -		if (stream->raw_) {
> > > +		if (stream == &data_->rawStream_) {
> > >  			cfg = cio2Configuration_;
> > >  		} else {
> > >  			bool scale = stream == &data_->vfStream_;
> > > @@ -572,13 +570,16 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,
> > >  					    std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> > >  {
> > >  	IPU3CameraData *data = cameraData(camera);
> > > -	IPU3Stream *ipu3stream = static_cast<IPU3Stream *>(stream);
> > >  	unsigned int count = stream->configuration().bufferCount;
> > >
> > > -	if (ipu3stream->raw_)
> > > +	if (stream == &data->outStream_)
> > > +		return data->imgu_->output_.dev->exportBuffers(count, buffers);
> > > +	else if (stream == &data->vfStream_)
> > > +		return data->imgu_->viewfinder_.dev->exportBuffers(count, buffers);
> > > +	else if (stream == &data->rawStream_)
> > >  		return data->cio2_.exportBuffers(count, buffers);
> > >
> > > -	return ipu3stream->device_->dev->exportBuffers(count, buffers);
> > > +	return -EINVAL;
> > >  }
> > >
> > >  /**
> > > @@ -685,11 +686,17 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
> > >  	/* Queue all buffers from the request aimed for the ImgU. */
> > >  	for (auto it : request->buffers()) {
> > >  		IPU3Stream *stream = static_cast<IPU3Stream *>(it.first);
> > > -		if (stream->raw_)
> > > -			continue;
> > > -
> > >  		FrameBuffer *buffer = it.second;
> > > -		int ret = stream->device_->dev->queueBuffer(buffer);
> > > +
> > > +		if (stream == &data->rawStream_)
> > > +			continue;
> > > +
> > > +		int ret = 0;
> > > +		if (stream == &data->outStream_)
> > > +			ret = data->imgu_->output_.dev->queueBuffer(buffer);
> > > +		else if (stream == &data->vfStream_)
> > > +			ret = data->imgu_->viewfinder_.dev->queueBuffer(buffer);
> >
> > 		else
> > 			continue;
>
> I had it like this in v1 but Jacopo preferred the v2 version. I
> preferred the v1 version just like you so I will use it as Jacopo
> expressed no strong feelings.
>

Absolutely not, was just a suggestion. Sorry for the ping-pong.

> >
> > or
> >
> > 		else
> > 			ret = 0;
> >
> > and dropping the previous &data->rawStream_ check would allow not
> > initializing the ret variable to 0.
> >
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> >
> > > +
> > >  		if (ret < 0)
> > >  			error = ret;
> > >  	}
> > > @@ -801,9 +808,6 @@ int PipelineHandlerIPU3::registerCameras()
> > >  		 * second.
> > >  		 */
> > >  		data->imgu_ = numCameras ? &imgu1_ : &imgu0_;
> > > -		data->outStream_.device_ = &data->imgu_->output_;
> > > -		data->vfStream_.device_ = &data->imgu_->viewfinder_;
> > > -		data->rawStream_.raw_ = true;
> > >
> > >  		/*
> > >  		 * Connect video devices' 'bufferReady' signals to their
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
>
> --
> Regards,
> Niklas Söderlund
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel at lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel