[libcamera-devel] [PATCH v2 16/25] libcamera: pipeline: rkisp1: Switch to FrameBuffer interface for stat and param
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jan 8 02:45:39 CET 2020
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 at 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(¶mPool_);
> - 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, ¶mBuffersMemory);
> + 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 at ideasonboard.com>
>
> IPAOperationData op;
> op.operation = RKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER;
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list