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

[Niklas Söderlund]

Hi Laurent,

Thanks for your feedback.

On 2020-06-27 19:36:36 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Sat, Jun 27, 2020 at 05:00:39AM +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 driver more coherent.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > ---
> >  src/libcamera/pipeline/ipu3/ipu3.cpp | 31 +++++++++++++++-------------
> >  1 file changed, 17 insertions(+), 14 deletions(-)
> > 
> > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > index ae7a01b81dacf498..1f320dc166767bab 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_;
> 
> I would have preferred to add an enum to identify the stream type and
> store it in IPU3Stream. This would allow using switch...case below. I
> understand from patch 11/13 that you want to remove the IPU3Stream
> class, and I wonder if it wouldn't be best to keep it with an enum id,
> as it could be useful for later. Up to you.

I think I would like to keep the change as it is unless someone have 
strong feelings opposing it. Keeping IPU3Stream just for the numerical 
id feels a bit bloated. But if we going forward need to again subclass 
Stream for some reason I might be persuaded. For now lets keep it simple 
so we can add complexity in later patches with added functionality more 
easily :-)

> 
> >  };
> >  
> >  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,16 @@ 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);
> > +		int ret;
> > +
> > +		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;
> > +
> >  		if (ret < 0)
> >  			error = ret;
> >  	}
> > @@ -801,9 +807,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