[libcamera-devel] [PATCH v2 08/10] libcamera: ipu3: cio2: Make the V4L2 devices private

[Laurent Pinchart]

Hi Niklas,

Thank you for the patch.

On Sat, Jun 06, 2020 at 05:04:34PM +0200, Niklas Söderlund wrote:
> In order to make the CIO2 easier to extend with new features make the
> V4L2 deices (sensor, CIO2 and video device) private members. This

s/deices/devices/

> requires a few helper functions to be added to allow for the IPU3 driver
> to still be able to interact with all parts of the CIO2. These helper
> functions will later be extended to add new features to the IPU3
> pipeline.

I understand it may feel cleaner, but is it really, given that the
CIO2Device class is closely related to the rest of the IPU3 pipeline
handler ?

> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> * Changes since v1
> - Drop sensor access helpers and replace with a sensor() call to get
>   hold of the CameraSensor pointer directly.
> ---
>  src/libcamera/pipeline/ipu3/cio2.cpp | 20 +++++++++++++++++++-
>  src/libcamera/pipeline/ipu3/cio2.h   | 16 +++++++++++++---
>  src/libcamera/pipeline/ipu3/ipu3.cpp | 16 +++++++---------
>  3 files changed, 39 insertions(+), 13 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp
> index 0d961ae8f5a0682b..2399be8de974ff92 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.cpp
> +++ b/src/libcamera/pipeline/ipu3/cio2.cpp
> @@ -32,7 +32,7 @@ const std::map<uint32_t, PixelFormat> sensorMbusToPixel = {
>  } /* namespace */
>  
>  CIO2Device::CIO2Device()
> -	: output_(nullptr), csi2_(nullptr), sensor_(nullptr)
> +	: sensor_(nullptr), csi2_(nullptr), output_(nullptr)
>  {
>  }
>  
> @@ -123,6 +123,8 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index)
>  	if (ret)
>  		return ret;
>  
> +	output_->bufferReady.connect(this, &CIO2Device::cio2BufferReady);
> +
>  	return 0;
>  }
>  
> @@ -248,6 +250,12 @@ int CIO2Device::allocateBuffers()
>  	return ret;
>  }
>  
> +int CIO2Device::exportBuffers(unsigned int count,
> +			      std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> +{
> +	return output_->exportBuffers(count, buffers);
> +}
> +
>  void CIO2Device::freeBuffers()
>  {
>  	/* The default std::queue constructor is explicit with gcc 5 and 6. */
> @@ -288,4 +296,14 @@ int CIO2Device::stop()
>  	return output_->streamOff();
>  }
>  
> +int CIO2Device::queueBuffer(FrameBuffer *buffer)
> +{
> +	return output_->queueBuffer(buffer);
> +}
> +
> +void CIO2Device::cio2BufferReady(FrameBuffer *buffer)
> +{
> +	bufferReady.emit(buffer);
> +}
> +
>  } /* namespace libcamera */
> diff --git a/src/libcamera/pipeline/ipu3/cio2.h b/src/libcamera/pipeline/ipu3/cio2.h
> index b7eb69a4e104f400..8c3d9dd24188ef1c 100644
> --- a/src/libcamera/pipeline/ipu3/cio2.h
> +++ b/src/libcamera/pipeline/ipu3/cio2.h
> @@ -13,6 +13,7 @@
>  
>  #include <libcamera/geometry.h>
>  #include <libcamera/pixel_format.h>
> +#include <libcamera/signal.h>
>  
>  namespace libcamera {
>  
> @@ -39,6 +40,8 @@ public:
>  						  const Size desiredSize = {}) const;
>  
>  	int allocateBuffers();
> +	int exportBuffers(unsigned int count,
> +			  std::vector<std::unique_ptr<FrameBuffer>> *buffers);
>  	void freeBuffers();
>  
>  	FrameBuffer *getBuffer();
> @@ -47,11 +50,18 @@ public:
>  	int start();
>  	int stop();
>  
> -	V4L2VideoDevice *output_;
> -	V4L2Subdevice *csi2_;
> -	CameraSensor *sensor_;
> +	CameraSensor *sensor() { return sensor_; }
> +
> +	int queueBuffer(FrameBuffer *buffer);
> +	Signal<FrameBuffer *> bufferReady;

Proxying the signal emitted by the output video device is a bit costly,
and is executed in a hot path :-( Is there a way we could improve that ?

>  
>  private:
> +	void cio2BufferReady(FrameBuffer *buffer);
> +
> +	CameraSensor *sensor_;
> +	V4L2Subdevice *csi2_;
> +	V4L2VideoDevice *output_;
> +
>  	std::vector<std::unique_ptr<FrameBuffer>> buffers_;
>  	std::queue<FrameBuffer *> availableBuffers_;
>  };
> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> index 85d4e64396e77222..55958a6c5e64dbf1 100644
> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> @@ -447,7 +447,7 @@ CameraConfiguration *PipelineHandlerIPU3::generateConfiguration(Camera *camera,
>  			 * available sensor resolution and to the IPU3
>  			 * alignment constraints.
>  			 */
> -			const Size &res = data->cio2_.sensor_->resolution();
> +			const Size &res = data->cio2_.sensor()->resolution();
>  			unsigned int width = std::min(1280U, res.width);
>  			unsigned int height = std::min(720U, res.height);
>  			cfg.size = { width & ~7, height & ~3 };
> @@ -627,13 +627,11 @@ int PipelineHandlerIPU3::exportFrameBuffers(Camera *camera, Stream *stream,
>  	IPU3CameraData *data = cameraData(camera);
>  	IPU3Stream *ipu3stream = static_cast<IPU3Stream *>(stream);
>  	unsigned int count = stream->configuration().bufferCount;
> -	V4L2VideoDevice *video;
>  
>  	if (ipu3stream->raw_)
> -		video = data->cio2_.output_;
> -	else
> -		video = ipu3stream->device_->dev;
> +		return data->cio2_.exportBuffers(count, buffers);
>  
> +	V4L2VideoDevice *video = ipu3stream->device_->dev;
>  	return video->exportBuffers(count, buffers);

This could be

 	return ipu3stream->device_->dev->exportBuffers(count, buffers);

>  }
>  
> @@ -744,7 +742,7 @@ int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
>  		return -EINVAL;
>  
>  	buffer->setRequest(request);
> -	data->cio2_.output_->queueBuffer(buffer);
> +	data->cio2_.queueBuffer(buffer);
>  
>  	for (auto it : request->buffers()) {
>  		IPU3Stream *stream = static_cast<IPU3Stream *>(it.first);
> @@ -857,7 +855,7 @@ int PipelineHandlerIPU3::registerCameras()
>  			continue;
>  
>  		/* Initialize the camera properties. */
> -		data->properties_ = cio2->sensor_->properties();
> +		data->properties_ = cio2->sensor()->properties();
>  
>  		/**
>  		 * \todo Dynamically assign ImgU and output devices to each
> @@ -881,7 +879,7 @@ int PipelineHandlerIPU3::registerCameras()
>  		 * associated ImgU input where they get processed and
>  		 * returned through the ImgU main and secondary outputs.
>  		 */
> -		data->cio2_.output_->bufferReady.connect(data.get(),
> +		data->cio2_.bufferReady.connect(data.get(),
>  					&IPU3CameraData::cio2BufferReady);
>  		data->imgu_->input_->bufferReady.connect(data.get(),
>  					&IPU3CameraData::imguInputBufferReady);
> @@ -891,7 +889,7 @@ int PipelineHandlerIPU3::registerCameras()
>  					&IPU3CameraData::imguOutputBufferReady);
>  
>  		/* Create and register the Camera instance. */
> -		std::string cameraName = cio2->sensor_->entity()->name();
> +		std::string cameraName = cio2->sensor()->entity()->name();
>  		std::shared_ptr<Camera> camera = Camera::create(this,
>  								cameraName,
>  								streams);

-- 
Regards,

Laurent Pinchart