[libcamera-devel,v2,16/25] libcamera: pipeline: rkisp1: Switch to FrameBuffer interface for stat and param
diff mbox series

Message ID 20191230120510.938333-17-niklas.soderlund@ragnatech.se
State Superseded
Headers show
Series
  • libcamera: Rework buffer API
Related show

Commit Message

Niklas Söderlund Dec. 30, 2019, 12:05 p.m. UTC 
The V4L2VideoDevice class can now operate using a FrameBuffer interface,
switch the RkISP1 statistics and parameters buffer to use it. We can not
yet convert the application facing buffers yet.

Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
---
 src/libcamera/pipeline/rkisp1/rkisp1.cpp | 122 ++++++++++++-----------
 1 file changed, 66 insertions(+), 56 deletions(-)

Comments

Laurent Pinchart Jan. 8, 2020, 1:45 a.m. UTC | #1 
Hi Niklas,

Thank you for the patch.

On Mon, Dec 30, 2019 at 01:05:01PM +0100, Niklas Söderlund wrote:
> The V4L2VideoDevice class can now operate using a FrameBuffer interface,
> switch the RkISP1 statistics and parameters buffer to use it. We can not
> yet convert the application facing buffers yet.

Double yet, s/yet convert/convert/
s/application facing/application-facing/

> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se>
> ---
>  src/libcamera/pipeline/rkisp1/rkisp1.cpp | 122 ++++++++++++-----------
>  1 file changed, 66 insertions(+), 56 deletions(-)
> 
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 9cd0ab3ad88b35cc..ea581aaaa85088cd 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -32,9 +32,6 @@
>  #include "v4l2_subdevice.h"
>  #include "v4l2_videodevice.h"
>  
> -#define RKISP1_PARAM_BASE 0x100
> -#define RKISP1_STAT_BASE 0x200
> -

This is nice.

>  namespace libcamera {
>  
>  LOG_DEFINE_CATEGORY(RkISP1)
> @@ -52,8 +49,8 @@ struct RkISP1FrameInfo {
>  	unsigned int frame;
>  	Request *request;
>  
> -	Buffer *paramBuffer;
> -	Buffer *statBuffer;
> +	FrameBuffer *paramBuffer;
> +	FrameBuffer *statBuffer;
>  	Buffer *videoBuffer;
>  
>  	bool paramFilled;
> @@ -71,6 +68,7 @@ public:
>  
>  	RkISP1FrameInfo *find(unsigned int frame);
>  	RkISP1FrameInfo *find(Buffer *buffer);
> +	RkISP1FrameInfo *find(FrameBuffer *buffer);
>  	RkISP1FrameInfo *find(Request *request);
>  
>  private:
> @@ -152,6 +150,7 @@ public:
>  
>  	const V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }
>  
> +

Not needed.

>  private:
>  	static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
>  
> @@ -203,8 +202,8 @@ private:
>  	int createCamera(MediaEntity *sensor);
>  	void tryCompleteRequest(Request *request);
>  	void bufferReady(Buffer *buffer);
> -	void paramReady(Buffer *buffer);
> -	void statReady(Buffer *buffer);
> +	void paramReady(FrameBuffer *buffer);
> +	void statReady(FrameBuffer *buffer);
>  
>  	MediaDevice *media_;
>  	V4L2Subdevice *dphy_;
> @@ -213,11 +212,10 @@ private:
>  	V4L2VideoDevice *param_;
>  	V4L2VideoDevice *stat_;
>  
> -	BufferPool paramPool_;
> -	BufferPool statPool_;
> -
> -	std::queue<Buffer *> paramBuffers_;
> -	std::queue<Buffer *> statBuffers_;
> +	std::vector<std::unique_ptr<FrameBuffer>> paramBuffersMemory;
> +	std::vector<std::unique_ptr<FrameBuffer>> statBuffersMemory;

Missing _ in the data member names.

How about naming these paramBuffers_ and statBuffers_, and ...

> +	std::queue<FrameBuffer *> paramBuffers_;
> +	std::queue<FrameBuffer *> statBuffers_;

... renaming those two availableParamBuffers_ and availableStatBuffers_
? (or s/available/free/)

>  
>  	Camera *activeCamera_;
>  };
> @@ -233,13 +231,13 @@ RkISP1FrameInfo *RkISP1Frames::create(unsigned int frame, Request *request, Stre
>  		LOG(RkISP1, Error) << "Parameters buffer underrun";
>  		return nullptr;
>  	}
> -	Buffer *paramBuffer = pipe_->paramBuffers_.front();
> +	FrameBuffer *paramBuffer = pipe_->paramBuffers_.front();
>  
>  	if (pipe_->statBuffers_.empty()) {
>  		LOG(RkISP1, Error) << "Statisitc buffer underrun";
>  		return nullptr;
>  	}
> -	Buffer *statBuffer = pipe_->statBuffers_.front();
> +	FrameBuffer *statBuffer = pipe_->statBuffers_.front();
>  
>  	Buffer *videoBuffer = request->findBuffer(stream);
>  	if (!videoBuffer) {
> @@ -299,9 +297,21 @@ RkISP1FrameInfo *RkISP1Frames::find(Buffer *buffer)
>  	for (auto &itInfo : frameInfo_) {
>  		RkISP1FrameInfo *info = itInfo.second;
>  
> +		if (info->videoBuffer == buffer)
> +			return info;
> +	}
> +
> +	LOG(RkISP1, Error) << "Can't locate info from buffer";
> +	return nullptr;
> +}
> +
> +RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)
> +{
> +	for (auto &itInfo : frameInfo_) {
> +		RkISP1FrameInfo *info = itInfo.second;
> +
>  		if (info->paramBuffer == buffer ||
> -		    info->statBuffer == buffer ||
> -		    info->videoBuffer == buffer)
> +		    info->statBuffer == buffer)
>  			return info;
>  	}
>  
> @@ -660,6 +670,7 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,
>  					   const std::set<Stream *> &streams)
>  {
>  	RkISP1CameraData *data = cameraData(camera);
> +	unsigned int count = 1;

My OCD would have moved this after the next line.

>  	Stream *stream = *streams.begin();
>  	int ret;
>  
> @@ -671,43 +682,41 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,
>  	if (ret)
>  		return ret;
>  
> -	paramPool_.createBuffers(stream->configuration().bufferCount + 1);

Why did we have a + 1 ?

> -	ret = param_->exportBuffers(&paramPool_);
> -	if (ret) {
> -		video_->releaseBuffers();
> -		return ret;
> -	}
> +	unsigned int maxBuffers = 0;
> +	for (const Stream *s : camera->streams())
> +		maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);
>  
> -	statPool_.createBuffers(stream->configuration().bufferCount + 1);
> -	ret = stat_->exportBuffers(&statPool_);
> -	if (ret) {
> -		param_->releaseBuffers();
> -		video_->releaseBuffers();
> -		return ret;
> -	}
> +	ret = param_->exportBuffers(maxBuffers, &paramBuffersMemory);
> +	if (ret < 0)
> +		goto err;
>  
> -	for (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {
> -		FrameBuffer::Plane plane;
> -		plane.fd = FileDescriptor(paramPool_.buffers()[i].planes()[0].fd.fd());
> -		plane.length = paramPool_.buffers()[i].planes()[0].length;
> +	ret = stat_->exportBuffers(maxBuffers, &statBuffersMemory);
> +	if (ret < 0)
> +		goto err;
>  
> -		data->ipaBuffers_.push_back({ .id = RKISP1_PARAM_BASE | i,
> -					      .planes = { plane } });
> -		paramBuffers_.push(new Buffer(i));
> +	for (std::unique_ptr<FrameBuffer> &buffer : paramBuffersMemory) {
> +		buffer->setCookie(count++);
> +		data->ipaBuffers_.push_back({ .id = buffer->cookie(),
> +					      .planes = buffer->planes() });

This duplicates the FileDescriptor instances :-S We really need to sort
this out, possibly on top of this series, but sooner than later.

> +		paramBuffers_.push(buffer.get());
>  	}
>  
> -	for (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {
> -		FrameBuffer::Plane plane;
> -		plane.fd = FileDescriptor(statPool_.buffers()[i].planes()[0].fd.fd());
> -		plane.length = statPool_.buffers()[i].planes()[0].length;
> -
> -		data->ipaBuffers_.push_back({ .id = RKISP1_STAT_BASE | i,
> -					      .planes = { plane } });
> -		statBuffers_.push(new Buffer(i));
> +	for (std::unique_ptr<FrameBuffer> &buffer : statBuffersMemory) {
> +		buffer->setCookie(count++);
> +		data->ipaBuffers_.push_back({ .id = buffer->cookie(),
> +					      .planes = buffer->planes() });
> +		statBuffers_.push(buffer.get());
>  	}
>  
>  	data->ipa_->mapBuffers(data->ipaBuffers_);
>  
> +	return 0;
> +
> +err:

s/err/error/ ?

> +	paramBuffersMemory.clear();
> +	statBuffersMemory.clear();
> +	video_->releaseBuffers();
> +
>  	return ret;
>  }
>  
> @@ -716,15 +725,14 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera,
>  {
>  	RkISP1CameraData *data = cameraData(camera);
>  
> -	while (!statBuffers_.empty()) {
> -		delete statBuffers_.front();
> +	while (!statBuffers_.empty())
>  		statBuffers_.pop();

	statBuffers_.clear();

> -	}
>  
> -	while (!paramBuffers_.empty()) {
> -		delete paramBuffers_.front();
> +	while (!paramBuffers_.empty())
>  		paramBuffers_.pop();

	paramBuffers_.clear();

> -	}
> +
> +	paramBuffersMemory.clear();
> +	statBuffersMemory.clear();
>  
>  	std::vector<unsigned int> ids;
>  	for (IPABuffer &ipabuf : data->ipaBuffers_)
> @@ -829,9 +837,11 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera,
>  	if (!info)
>  		return -ENOENT;
>  
> +	unsigned int paramid = info->paramBuffer->cookie();

s/paramid/paramId/

> +
>  	IPAOperationData op;
>  	op.operation = RKISP1_IPA_EVENT_QUEUE_REQUEST;
> -	op.data = { data->frame_, RKISP1_PARAM_BASE | info->paramBuffer->index() };
> +	op.data = { data->frame_, paramid };

Or better,

	op.data = { data->frame_, info->paramBuffer->cookie() };

>  	op.controls = { request->controls() };
>  	data->ipa_->processEvent(op);
>  
> @@ -946,8 +956,8 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
>  		return false;
>  
>  	video_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);
> -	stat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);
> -	param_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);
> +	stat_->frameBufferReady.connect(this, &PipelineHandlerRkISP1::statReady);
> +	param_->frameBufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);
>  
>  	/* Configure default links. */
>  	if (initLinks() < 0) {
> @@ -1009,7 +1019,7 @@ void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)
>  	tryCompleteRequest(request);
>  }
>  
> -void PipelineHandlerRkISP1::paramReady(Buffer *buffer)
> +void PipelineHandlerRkISP1::paramReady(FrameBuffer *buffer)
>  {
>  	ASSERT(activeCamera_);
>  	RkISP1CameraData *data = cameraData(activeCamera_);
> @@ -1020,7 +1030,7 @@ void PipelineHandlerRkISP1::paramReady(Buffer *buffer)
>  	tryCompleteRequest(info->request);
>  }
>  
> -void PipelineHandlerRkISP1::statReady(Buffer *buffer)
> +void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
>  {
>  	ASSERT(activeCamera_);
>  	RkISP1CameraData *data = cameraData(activeCamera_);
> @@ -1030,7 +1040,7 @@ void PipelineHandlerRkISP1::statReady(Buffer *buffer)
>  		return;
>  
>  	unsigned int frame = info->frame;
> -	unsigned int statid = RKISP1_STAT_BASE | info->statBuffer->index();
> +	unsigned int statid = info->statBuffer->cookie();

While at it, s/statid/statId/ or the better option proposed above ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  
>  	IPAOperationData op;
>  	op.operation = RKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER;

Patch
diff mbox series 

diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
index 9cd0ab3ad88b35cc..ea581aaaa85088cd 100644
--- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
+++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
@@ -32,9 +32,6 @@ 
 #include "v4l2_subdevice.h"
 #include "v4l2_videodevice.h"
 
-#define RKISP1_PARAM_BASE 0x100
-#define RKISP1_STAT_BASE 0x200
-
 namespace libcamera {
 
 LOG_DEFINE_CATEGORY(RkISP1)
@@ -52,8 +49,8 @@  struct RkISP1FrameInfo {
 	unsigned int frame;
 	Request *request;
 
-	Buffer *paramBuffer;
-	Buffer *statBuffer;
+	FrameBuffer *paramBuffer;
+	FrameBuffer *statBuffer;
 	Buffer *videoBuffer;
 
 	bool paramFilled;
@@ -71,6 +68,7 @@  public:
 
 	RkISP1FrameInfo *find(unsigned int frame);
 	RkISP1FrameInfo *find(Buffer *buffer);
+	RkISP1FrameInfo *find(FrameBuffer *buffer);
 	RkISP1FrameInfo *find(Request *request);
 
 private:
@@ -152,6 +150,7 @@  public:
 
 	const V4L2SubdeviceFormat &sensorFormat() { return sensorFormat_; }
 
+
 private:
 	static constexpr unsigned int RKISP1_BUFFER_COUNT = 4;
 
@@ -203,8 +202,8 @@  private:
 	int createCamera(MediaEntity *sensor);
 	void tryCompleteRequest(Request *request);
 	void bufferReady(Buffer *buffer);
-	void paramReady(Buffer *buffer);
-	void statReady(Buffer *buffer);
+	void paramReady(FrameBuffer *buffer);
+	void statReady(FrameBuffer *buffer);
 
 	MediaDevice *media_;
 	V4L2Subdevice *dphy_;
@@ -213,11 +212,10 @@  private:
 	V4L2VideoDevice *param_;
 	V4L2VideoDevice *stat_;
 
-	BufferPool paramPool_;
-	BufferPool statPool_;
-
-	std::queue<Buffer *> paramBuffers_;
-	std::queue<Buffer *> statBuffers_;
+	std::vector<std::unique_ptr<FrameBuffer>> paramBuffersMemory;
+	std::vector<std::unique_ptr<FrameBuffer>> statBuffersMemory;
+	std::queue<FrameBuffer *> paramBuffers_;
+	std::queue<FrameBuffer *> statBuffers_;
 
 	Camera *activeCamera_;
 };
@@ -233,13 +231,13 @@  RkISP1FrameInfo *RkISP1Frames::create(unsigned int frame, Request *request, Stre
 		LOG(RkISP1, Error) << "Parameters buffer underrun";
 		return nullptr;
 	}
-	Buffer *paramBuffer = pipe_->paramBuffers_.front();
+	FrameBuffer *paramBuffer = pipe_->paramBuffers_.front();
 
 	if (pipe_->statBuffers_.empty()) {
 		LOG(RkISP1, Error) << "Statisitc buffer underrun";
 		return nullptr;
 	}
-	Buffer *statBuffer = pipe_->statBuffers_.front();
+	FrameBuffer *statBuffer = pipe_->statBuffers_.front();
 
 	Buffer *videoBuffer = request->findBuffer(stream);
 	if (!videoBuffer) {
@@ -299,9 +297,21 @@  RkISP1FrameInfo *RkISP1Frames::find(Buffer *buffer)
 	for (auto &itInfo : frameInfo_) {
 		RkISP1FrameInfo *info = itInfo.second;
 
+		if (info->videoBuffer == buffer)
+			return info;
+	}
+
+	LOG(RkISP1, Error) << "Can't locate info from buffer";
+	return nullptr;
+}
+
+RkISP1FrameInfo *RkISP1Frames::find(FrameBuffer *buffer)
+{
+	for (auto &itInfo : frameInfo_) {
+		RkISP1FrameInfo *info = itInfo.second;
+
 		if (info->paramBuffer == buffer ||
-		    info->statBuffer == buffer ||
-		    info->videoBuffer == buffer)
+		    info->statBuffer == buffer)
 			return info;
 	}
 
@@ -660,6 +670,7 @@  int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,
 					   const std::set<Stream *> &streams)
 {
 	RkISP1CameraData *data = cameraData(camera);
+	unsigned int count = 1;
 	Stream *stream = *streams.begin();
 	int ret;
 
@@ -671,43 +682,41 @@  int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,
 	if (ret)
 		return ret;
 
-	paramPool_.createBuffers(stream->configuration().bufferCount + 1);
-	ret = param_->exportBuffers(&paramPool_);
-	if (ret) {
-		video_->releaseBuffers();
-		return ret;
-	}
+	unsigned int maxBuffers = 0;
+	for (const Stream *s : camera->streams())
+		maxBuffers = std::max(maxBuffers, s->configuration().bufferCount);
 
-	statPool_.createBuffers(stream->configuration().bufferCount + 1);
-	ret = stat_->exportBuffers(&statPool_);
-	if (ret) {
-		param_->releaseBuffers();
-		video_->releaseBuffers();
-		return ret;
-	}
+	ret = param_->exportBuffers(maxBuffers, &paramBuffersMemory);
+	if (ret < 0)
+		goto err;
 
-	for (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {
-		FrameBuffer::Plane plane;
-		plane.fd = FileDescriptor(paramPool_.buffers()[i].planes()[0].fd.fd());
-		plane.length = paramPool_.buffers()[i].planes()[0].length;
+	ret = stat_->exportBuffers(maxBuffers, &statBuffersMemory);
+	if (ret < 0)
+		goto err;
 
-		data->ipaBuffers_.push_back({ .id = RKISP1_PARAM_BASE | i,
-					      .planes = { plane } });
-		paramBuffers_.push(new Buffer(i));
+	for (std::unique_ptr<FrameBuffer> &buffer : paramBuffersMemory) {
+		buffer->setCookie(count++);
+		data->ipaBuffers_.push_back({ .id = buffer->cookie(),
+					      .planes = buffer->planes() });
+		paramBuffers_.push(buffer.get());
 	}
 
-	for (unsigned int i = 0; i < stream->configuration().bufferCount + 1; i++) {
-		FrameBuffer::Plane plane;
-		plane.fd = FileDescriptor(statPool_.buffers()[i].planes()[0].fd.fd());
-		plane.length = statPool_.buffers()[i].planes()[0].length;
-
-		data->ipaBuffers_.push_back({ .id = RKISP1_STAT_BASE | i,
-					      .planes = { plane } });
-		statBuffers_.push(new Buffer(i));
+	for (std::unique_ptr<FrameBuffer> &buffer : statBuffersMemory) {
+		buffer->setCookie(count++);
+		data->ipaBuffers_.push_back({ .id = buffer->cookie(),
+					      .planes = buffer->planes() });
+		statBuffers_.push(buffer.get());
 	}
 
 	data->ipa_->mapBuffers(data->ipaBuffers_);
 
+	return 0;
+
+err:
+	paramBuffersMemory.clear();
+	statBuffersMemory.clear();
+	video_->releaseBuffers();
+
 	return ret;
 }
 
@@ -716,15 +725,14 @@  int PipelineHandlerRkISP1::freeBuffers(Camera *camera,
 {
 	RkISP1CameraData *data = cameraData(camera);
 
-	while (!statBuffers_.empty()) {
-		delete statBuffers_.front();
+	while (!statBuffers_.empty())
 		statBuffers_.pop();
-	}
 
-	while (!paramBuffers_.empty()) {
-		delete paramBuffers_.front();
+	while (!paramBuffers_.empty())
 		paramBuffers_.pop();
-	}
+
+	paramBuffersMemory.clear();
+	statBuffersMemory.clear();
 
 	std::vector<unsigned int> ids;
 	for (IPABuffer &ipabuf : data->ipaBuffers_)
@@ -829,9 +837,11 @@  int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera,
 	if (!info)
 		return -ENOENT;
 
+	unsigned int paramid = info->paramBuffer->cookie();
+
 	IPAOperationData op;
 	op.operation = RKISP1_IPA_EVENT_QUEUE_REQUEST;
-	op.data = { data->frame_, RKISP1_PARAM_BASE | info->paramBuffer->index() };
+	op.data = { data->frame_, paramid };
 	op.controls = { request->controls() };
 	data->ipa_->processEvent(op);
 
@@ -946,8 +956,8 @@  bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator)
 		return false;
 
 	video_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady);
-	stat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady);
-	param_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);
+	stat_->frameBufferReady.connect(this, &PipelineHandlerRkISP1::statReady);
+	param_->frameBufferReady.connect(this, &PipelineHandlerRkISP1::paramReady);
 
 	/* Configure default links. */
 	if (initLinks() < 0) {
@@ -1009,7 +1019,7 @@  void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)
 	tryCompleteRequest(request);
 }
 
-void PipelineHandlerRkISP1::paramReady(Buffer *buffer)
+void PipelineHandlerRkISP1::paramReady(FrameBuffer *buffer)
 {
 	ASSERT(activeCamera_);
 	RkISP1CameraData *data = cameraData(activeCamera_);
@@ -1020,7 +1030,7 @@  void PipelineHandlerRkISP1::paramReady(Buffer *buffer)
 	tryCompleteRequest(info->request);
 }
 
-void PipelineHandlerRkISP1::statReady(Buffer *buffer)
+void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer)
 {
 	ASSERT(activeCamera_);
 	RkISP1CameraData *data = cameraData(activeCamera_);
@@ -1030,7 +1040,7 @@  void PipelineHandlerRkISP1::statReady(Buffer *buffer)
 		return;
 
 	unsigned int frame = info->frame;
-	unsigned int statid = RKISP1_STAT_BASE | info->statBuffer->index();
+	unsigned int statid = info->statBuffer->cookie();
 
 	IPAOperationData op;
 	op.operation = RKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER;