[libcamera-devel] [PATCH v3 22/33] libcamera: pipeline: rkisp1: Switch to FrameBuffer interface for stat and param
Niklas Söderlund
niklas.soderlund at ragnatech.se
Sun Jan 12 00:31:34 CET 2020
Hi Laurent,
Thanks for your feedback.
On 2020-01-11 02:55:24 +0200, Laurent Pinchart wrote:
> Hi Niklas,
>
> Thank you for the patch.
>
> On Fri, Jan 10, 2020 at 08:37:57PM +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
> > convert the application-facing buffers yet.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> > ---
> > * Changes since v2
> > - Rename {param,stat}BuffersMemory to {param,stat}Buffers_
> > - Rename {param,stat}Buffers_ to available{Param,Stat}Buffers_
> > - s/err:/error:/
> > - Beutification of small code blocks
> > ---
> > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 132 ++++++++++++-----------
> > 1 file changed, 72 insertions(+), 60 deletions(-)
> >
> > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> > index 607ff85539a22324..dbc5df801f30e76b 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:
> > @@ -203,8 +201,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 +211,10 @@ private:
> > V4L2VideoDevice *param_;
> > V4L2VideoDevice *stat_;
> >
> > - BufferPool paramPool_;
> > - BufferPool statPool_;
> > -
> > - std::queue<Buffer *> paramBuffers_;
> > - std::queue<Buffer *> statBuffers_;
> > + std::vector<std::unique_ptr<FrameBuffer>> paramBuffers_;
> > + std::vector<std::unique_ptr<FrameBuffer>> statBuffers_;
> > + std::queue<FrameBuffer *> availableParamBuffers_;
> > + std::queue<FrameBuffer *> availableStatBuffers_;
> >
> > Camera *activeCamera_;
> > };
> > @@ -229,17 +226,17 @@ RkISP1Frames::RkISP1Frames(PipelineHandler *pipe)
> >
> > RkISP1FrameInfo *RkISP1Frames::create(unsigned int frame, Request *request, Stream *stream)
> > {
> > - if (pipe_->paramBuffers_.empty()) {
> > + if (pipe_->availableParamBuffers_.empty()) {
> > LOG(RkISP1, Error) << "Parameters buffer underrun";
> > return nullptr;
> > }
> > - Buffer *paramBuffer = pipe_->paramBuffers_.front();
> > + FrameBuffer *paramBuffer = pipe_->availableParamBuffers_.front();
> >
> > - if (pipe_->statBuffers_.empty()) {
> > + if (pipe_->availableStatBuffers_.empty()) {
> > LOG(RkISP1, Error) << "Statisitc buffer underrun";
> > return nullptr;
> > }
> > - Buffer *statBuffer = pipe_->statBuffers_.front();
> > + FrameBuffer *statBuffer = pipe_->availableStatBuffers_.front();
> >
> > Buffer *videoBuffer = request->findBuffer(stream);
> > if (!videoBuffer) {
> > @@ -248,8 +245,8 @@ RkISP1FrameInfo *RkISP1Frames::create(unsigned int frame, Request *request, Stre
> > return nullptr;
> > }
> >
> > - pipe_->paramBuffers_.pop();
> > - pipe_->statBuffers_.pop();
> > + pipe_->availableParamBuffers_.pop();
> > + pipe_->availableStatBuffers_.pop();
> >
> > RkISP1FrameInfo *info = new RkISP1FrameInfo;
> >
> > @@ -273,8 +270,8 @@ int RkISP1Frames::destroy(unsigned int frame)
> > if (!info)
> > return -ENOENT;
> >
> > - pipe_->paramBuffers_.push(info->paramBuffer);
> > - pipe_->statBuffers_.push(info->statBuffer);
> > + pipe_->availableParamBuffers_.push(info->paramBuffer);
> > + pipe_->availableStatBuffers_.push(info->statBuffer);
> >
> > frameInfo_.erase(info->frame);
> >
> > @@ -299,9 +296,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;
> > }
> >
> > @@ -661,6 +670,7 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,
> > {
> > RkISP1CameraData *data = cameraData(camera);
> > Stream *stream = *streams.begin();
> > + unsigned int count = 1;
> > int ret;
> >
> > if (stream->memoryType() == InternalMemory)
> > @@ -671,35 +681,41 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera,
> > if (ret)
> > return ret;
> >
> > - paramPool_.createBuffers(stream->configuration().bufferCount);
> > - 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);
> > - ret = stat_->exportBuffers(&statPool_);
> > - if (ret) {
> > - param_->releaseBuffers();
> > - video_->releaseBuffers();
> > - return ret;
> > - }
> > + ret = param_->exportBuffers(maxBuffers, ¶mBuffers_);
> > + if (ret < 0)
> > + goto error;
> > +
> > + ret = stat_->exportBuffers(maxBuffers, &statBuffers_);
> > + if (ret < 0)
> > + goto error;
> >
> > - for (unsigned int i = 0; i < stream->configuration().bufferCount; i++) {
> > - data->ipaBuffers_.push_back({ .id = RKISP1_PARAM_BASE | i,
> > - .planes = paramPool_.buffers()[i].planes() });
> > - paramBuffers_.push(new Buffer(i));
> > + for (std::unique_ptr<FrameBuffer> &buffer : paramBuffers_) {
> > + buffer->setCookie(count++);
> > + data->ipaBuffers_.push_back({ .id = buffer->cookie(),
> > + .planes = buffer->planes() });
> > + availableParamBuffers_.push(buffer.get());
> > }
> >
> > - for (unsigned int i = 0; i < stream->configuration().bufferCount; i++) {
> > - data->ipaBuffers_.push_back({ .id = RKISP1_STAT_BASE | i,
> > - .planes = statPool_.buffers()[i].planes() });
> > - statBuffers_.push(new Buffer(i));
> > + for (std::unique_ptr<FrameBuffer> &buffer : statBuffers_) {
> > + buffer->setCookie(count++);
> > + data->ipaBuffers_.push_back({ .id = buffer->cookie(),
> > + .planes = buffer->planes() });
> > + availableStatBuffers_.push(buffer.get());
> > }
> >
> > data->ipa_->mapBuffers(data->ipaBuffers_);
> >
> > + return 0;
> > +
> > +error:
> > + paramBuffers_.clear();
> > + statBuffers_.clear();
> > + video_->releaseBuffers();
> > +
> > return ret;
> > }
> >
> > @@ -708,15 +724,14 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera,
> > {
> > RkISP1CameraData *data = cameraData(camera);
> >
> > - while (!statBuffers_.empty()) {
> > - delete statBuffers_.front();
> > - statBuffers_.pop();
> > - }
> > + while (!availableStatBuffers_.empty())
> > + availableStatBuffers_.pop();
> >
> > - while (!paramBuffers_.empty()) {
> > - delete paramBuffers_.front();
> > - paramBuffers_.pop();
> > - }
> > + while (!availableParamBuffers_.empty())
> > + availableParamBuffers_.pop();
>
> Can't you replace this with
>
> availableStatBuffers_.clear();
> availableParamBuffers_.clear();
>
> ?
Wish I could, but std::queue does not implement clear() :-(
>
> Please keep my Reviewed-by.
>
> > +
> > + paramBuffers_.clear();
> > + statBuffers_.clear();
> >
> > std::vector<unsigned int> ids;
> > for (IPABuffer &ipabuf : data->ipaBuffers_)
> > @@ -823,7 +838,7 @@ int PipelineHandlerRkISP1::queueRequestDevice(Camera *camera,
> >
> > IPAOperationData op;
> > op.operation = RKISP1_IPA_EVENT_QUEUE_REQUEST;
> > - op.data = { data->frame_, RKISP1_PARAM_BASE | info->paramBuffer->index() };
> > + op.data = { data->frame_, info->paramBuffer->cookie() };
> > op.controls = { request->controls() };
> > data->ipa_->processEvent(op);
> >
> > @@ -938,8 +953,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) {
> > @@ -1001,7 +1016,7 @@ void PipelineHandlerRkISP1::bufferReady(Buffer *buffer)
> > tryCompleteRequest(request);
> > }
> >
> > -void PipelineHandlerRkISP1::paramReady(Buffer *buffer)
> > +void PipelineHandlerRkISP1::paramReady(FrameBuffer *buffer)
> > {
> > ASSERT(activeCamera_);
> > RkISP1CameraData *data = cameraData(activeCamera_);
> > @@ -1012,7 +1027,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_);
> > @@ -1021,12 +1036,9 @@ void PipelineHandlerRkISP1::statReady(Buffer *buffer)
> > if (!info)
> > return;
> >
> > - unsigned int frame = info->frame;
> > - unsigned int statid = RKISP1_STAT_BASE | info->statBuffer->index();
> > -
> > IPAOperationData op;
> > op.operation = RKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER;
> > - op.data = { frame, statid };
> > + op.data = { info->frame, info->statBuffer->cookie() };
> > data->ipa_->processEvent(op);
> > }
> >
>
> --
> Regards,
>
> Laurent Pinchart
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list