| Message ID | 20190828011710.32128-14-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, The final piece of the puzzle :-D On 28/08/2019 02:17, Niklas Söderlund wrote: > Add the plumbing to the pipeline handler to interact with an IPA module. > To support this parameter and statistic buffers needs to be associated > with every request queued. The parameters buffer needs to be passed to > the IPA before any buffer in the request is queued to hardware and the > statistics buffer needs to be passed to the IPA for inspection as soon > as it's ready. > > This change makes the usage of an IPA module mandatory for the rkisp1 > pipeline. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 263 ++++++++++++++++++++++- > 1 file changed, 252 insertions(+), 11 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index de4ab523d0e4fe36..80d4832c49ebe78c 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -9,7 +9,7 @@ > #include <array> > #include <iomanip> > #include <memory> > -#include <vector> > +#include <queue> > > #include <linux/media-bus-format.h> > > @@ -34,7 +34,7 @@ class RkISP1CameraData : public CameraData > { > public: > RkISP1CameraData(PipelineHandler *pipe) > - : CameraData(pipe), sensor_(nullptr) > + : CameraData(pipe, 1, 1), sensor_(nullptr) > { > } > > @@ -43,8 +43,21 @@ public: > delete sensor_; > } > > + int initCameraData() override; > + > Stream stream_; > CameraSensor *sensor_; > + > +private: > + void updateSensor(V4L2ControlList controls); > + void queueRequestHardware(const void *cookie); > +}; > + > +class RkISP1RequestData : public RequestData > +{ > +public: > + Buffer *stat; > + Buffer *param; > }; > > class RkISP1CameraConfiguration : public CameraConfiguration > @@ -99,18 +112,69 @@ private: > PipelineHandler::cameraData(camera)); > } > > + friend RkISP1CameraData; Is this necessary? Isn't almost everything in a CameraData class public? If this is required - should members in CameraData be moved to protected/private? > + > int initLinks(); > int createCamera(MediaEntity *sensor); > + void tryCompleteRequest(Request *request); > void bufferReady(Buffer *buffer); > + void statReady(Buffer *buffer); > + void paramReady(Buffer *buffer); > > MediaDevice *media_; > V4L2Subdevice *dphy_; > V4L2Subdevice *isp_; > V4L2VideoDevice *video_; > + V4L2VideoDevice *stat_; > + V4L2VideoDevice *param_; > + > + BufferPool statPool_; > + BufferPool paramPool_; > + > + std::queue<Buffer *> statBuffers_; > + std::queue<Buffer *> paramBuffers_; Wouldn't these be associated as buffers in a Request, and thus live on that queue? > Camera *activeCamera_; > }; > > +int RkISP1CameraData::initCameraData() > +{ > + ipa_->updateSensor.connect(this, > + &RkISP1CameraData::updateSensor); > + ipa_->queueRequest.connect(this, > + &RkISP1CameraData::queueRequestHardware); > + return 0; > +} > + > +void RkISP1CameraData::updateSensor(V4L2ControlList controls) > +{ > + sensor_->setControls(&controls); > +} > + > +void RkISP1CameraData::queueRequestHardware(const void *cookie) > +{ > + /* Translate cookie to request. */ > + Request *request = reinterpret_cast<Request *>(const_cast<void *>(cookie)); C++ casts are so ... pleasant ... :S do you need the (const_cast<void *>.. ? Isn't cookie already a const void *? > + PipelineHandlerRkISP1 *pipe = > + static_cast<PipelineHandlerRkISP1 *>(pipe_); > + RkISP1RequestData *reqData = > + static_cast<RkISP1RequestData *>(request->data); > + Buffer *buffer = request->findBuffer(&stream_); > + int ret; > + > + ret = pipe->param_->queueBuffer(reqData->param); > + if (ret < 0) > + LOG(RkISP1, Error) << "Failed to queue paramaeters"; s/paramaeters/parameters/ > + > + ret = pipe->stat_->queueBuffer(reqData->stat); > + if (ret < 0) > + LOG(RkISP1, Error) << "Failed to queue statistics"; I wonder if we should keep a counter somewhere of all 'failures' which could not be returned... I guess it depends on whether we would somewhat successfully continue or not without the statistics buffer queued. > + > + ret = pipe->video_->queueBuffer(buffer); > + if (ret < 0) > + LOG(RkISP1, Error) << "Failed to queue video"; > +} > + > RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera, > RkISP1CameraData *data) > : CameraConfiguration() > @@ -202,12 +266,14 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager) > : PipelineHandler(manager), dphy_(nullptr), isp_(nullptr), > - video_(nullptr) > + video_(nullptr), stat_(nullptr), param_(nullptr) > { > } > > PipelineHandlerRkISP1::~PipelineHandlerRkISP1() > { > + delete param_; > + delete stat_; > delete video_; > delete isp_; > delete dphy_; > @@ -317,6 +383,20 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > if (ret) > return ret; > > + V4L2DeviceFormat statFormat = {}; > + statFormat.fourcc = V4L2_META_FMT_RK_ISP1_STAT_3A; > + > + ret = stat_->setFormat(&statFormat); > + if (ret) > + return ret; It's fine to have it, I'm sure - but is this required? Or will a device which only supports one fourcc, not already be configured to use that fourcc? Probably good to be explicit even if it's not required though. > + V4L2DeviceFormat paramFormat = {}; > + paramFormat.fourcc = V4L2_META_FMT_RK_ISP1_PARAMS; > + > + ret = param_->setFormat(¶mFormat); > + if (ret) > + return ret; > + > if (outputFormat.size != cfg.size || > outputFormat.fourcc != cfg.pixelFormat) { > LOG(RkISP1, Error) > @@ -333,30 +413,92 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera, > const std::set<Stream *> &streams) > { > Stream *stream = *streams.begin(); > + int ret; > > if (stream->memoryType() == InternalMemory) > - return video_->exportBuffers(&stream->bufferPool()); > + ret = video_->exportBuffers(&stream->bufferPool()); > else > - return video_->importBuffers(&stream->bufferPool()); > + ret = video_->importBuffers(&stream->bufferPool()); > + > + if (ret) > + return ret; > + > + statPool_.createBuffers(stream->configuration().bufferCount); > + ret = stat_->exportBuffers(&statPool_); > + if (ret) { > + video_->releaseBuffers(); Can we let these be handled by the destructor? or just call freeBuffers()? > + return ret; > + } > + > + paramPool_.createBuffers(stream->configuration().bufferCount); > + ret = param_->exportBuffers(¶mPool_); > + if (ret) { > + stat_->releaseBuffers(); > + video_->releaseBuffers(); And these? > + return ret; > + } > + > + for (unsigned int i = 0; i < stream->configuration().bufferCount; i++) { > + statBuffers_.push(new Buffer(i)); > + paramBuffers_.push(new Buffer(i)); > + } > + > + return ret; > } > > int PipelineHandlerRkISP1::freeBuffers(Camera *camera, > const std::set<Stream *> &streams) > { > + while (!paramBuffers_.empty()) > + paramBuffers_.pop(); > + > + while (!statBuffers_.empty()) > + statBuffers_.pop(); Does the queue not have a .clear() operator? No. However stackoverflow [0] informs me that you can just assign an empty queue to empty the queue... paramBuffers_ = {}; statBuffers_ = {}; I don't know if that's better or not :D [0] https://stackoverflow.com/questions/3874624/why-stdqueue-doesnt-support-clear-function/3874743#3874743 I wonder if we'll end up needing to use a deque anyway, so that we can 'peek' into the queue to determine if we need to take any actions early based on achieving per-frame-controls > + > + if (param_->releaseBuffers()) > + LOG(RkISP1, Error) << "Failed to release parameters buffers"; > + > + if (stat_->releaseBuffers()) > + LOG(RkISP1, Error) << "Failed to release stat buffers"; > + > if (video_->releaseBuffers()) > - LOG(RkISP1, Error) << "Failed to release buffers"; > + LOG(RkISP1, Error) << "Failed to release video buffers"; > > return 0; > } > > int PipelineHandlerRkISP1::start(Camera *camera) > { > + RkISP1CameraData *data = cameraData(camera); > int ret; > > + ret = data->ipa_->initSensor(data->sensor_->controls()); > + if (ret) > + return ret; > + > + ret = param_->streamOn(); > + if (ret) { > + LOG(RkISP1, Error) > + << "Failed to start parameters " << camera->name(); > + return ret; > + } > + > + ret = stat_->streamOn(); > + if (ret) { > + param_->streamOff(); > + LOG(RkISP1, Error) > + << "Failed to start statisticis " << camera->name(); s/statisticis/statistics/ > + return ret; > + } > + > ret = video_->streamOn(); > - if (ret) > + if (ret) { > + param_->streamOff(); > + stat_->streamOff(); > + > LOG(RkISP1, Error) > << "Failed to start camera " << camera->name(); > + } > > activeCamera_ = camera; > > @@ -372,6 +514,16 @@ void PipelineHandlerRkISP1::stop(Camera *camera) > LOG(RkISP1, Warning) > << "Failed to stop camera " << camera->name(); > > + ret = stat_->streamOff(); > + if (ret) > + LOG(RkISP1, Warning) > + << "Failed to stop statisticis " << camera->name(); s/statisticis/statistics/ > + > + ret = param_->streamOff(); > + if (ret) > + LOG(RkISP1, Warning) > + << "Failed to stop parameters " << camera->name(); > + > activeCamera_ = nullptr; > } > > @@ -380,6 +532,16 @@ int PipelineHandlerRkISP1::queueRequest(Camera *camera, Request *request) > RkISP1CameraData *data = cameraData(camera); > Stream *stream = &data->stream_; > > + if (paramBuffers_.empty()) { > + LOG(RkISP1, Error) << "Parameters buffer underrun"; > + return -ENOENT; > + } > + > + if (statBuffers_.empty()) { > + LOG(RkISP1, Error) << "Statisitc buffer underrun"; s/Statisitc/Statistic/ > + return -ENOENT; > + } > + > Buffer *buffer = request->findBuffer(stream); > if (!buffer) { > LOG(RkISP1, Error) > @@ -387,12 +549,24 @@ int PipelineHandlerRkISP1::queueRequest(Camera *camera, Request *request) > return -ENOENT; > } > > - int ret = video_->queueBuffer(buffer); > - if (ret < 0) > - return ret; > + RkISP1RequestData *reqData = new RkISP1RequestData(); > + request->data = reqData; > + reqData->param = paramBuffers_.front(); > + reqData->stat = statBuffers_.front(); > + > + prepareInternalBuffer(reqData->param, request, > + ¶mPool_.buffers()[reqData->param->index()]); > + prepareInternalBuffer(reqData->stat, request, > + &statPool_.buffers()[reqData->stat->index()]); > + > + paramBuffers_.pop(); > + statBuffers_.pop(); > > PipelineHandler::queueRequest(camera, request); > > + data->ipa_->processRequest(request, request->controls(), > + *reqData->param); > + > return 0; > } > > @@ -435,6 +609,10 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor) > std::unique_ptr<RkISP1CameraData> data = > utils::make_unique<RkISP1CameraData>(this); > > + data->controlInfo_.emplace(std::piecewise_construct, > + std::forward_as_tuple(AeEnable), > + std::forward_as_tuple(AeEnable, false, true)); > + > data->sensor_ = new CameraSensor(sensor); > ret = data->sensor_->init(); > if (ret) > @@ -478,7 +656,17 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator) > if (video_->open() < 0) > return false; > > + stat_ = V4L2VideoDevice::fromEntityName(media_, "rkisp1-statistics"); > + if (stat_->open() < 0) > + return false; > + > + param_ = V4L2VideoDevice::fromEntityName(media_, "rkisp1-input-params"); > + if (param_->open() < 0) > + return false; Ah - good, I see these entities are already in the DeviceMatch. > + > video_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady); > + stat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady); > + param_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady); > > /* Configure default links. */ > if (initLinks() < 0) { > @@ -504,13 +692,66 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator) > * Buffer Handling > */ > > +void PipelineHandlerRkISP1::tryCompleteRequest(Request *request) > +{ > + RkISP1RequestData *reqData = > + static_cast<RkISP1RequestData *>(request->data); > + > + if (reqData->param) > + return; > + > + if (reqData->stat) > + return; > + > + if (request->hasPendingBuffers()) > + return; Hrm... for some reason I assumed these buffers would be tied into the hasPendingBuffers() calls ... but maybe that would require creating new streams. And these are 'internal' streams ... > + > + delete reqData; > + request->data = nullptr; > + > + completeRequest(activeCamera_, request); > +} > + > void PipelineHandlerRkISP1::bufferReady(Buffer *buffer) > { > ASSERT(activeCamera_); > Request *request = buffer->request(); > > completeBuffer(activeCamera_, request, buffer); > - completeRequest(activeCamera_, request); > + tryCompleteRequest(request); I feel like somehow the param and stats buffers should be more directly associated with the Request to facilitate the existing completeRequest() mechanism, which is what I thought it was for. Is there anything preventing that? > +} > + > +void PipelineHandlerRkISP1::statReady(Buffer *buffer) > +{ > + ASSERT(activeCamera_); > + RkISP1CameraData *data = cameraData(activeCamera_); > + Request *request = buffer->request(); > + RkISP1RequestData *reqData = > + static_cast<RkISP1RequestData *>(request->data); > + > + data->ipa_->updateStatistics(request, *buffer); > + > + /* TODO: Fetch libcamera status controls from IPA */ status controls? Do we perhaps need to support two interactions with the IPA per request? One at queue time, and one at complete? > + > + reqData->stat = nullptr; > + > + statBuffers_.push(buffer); > + > + tryCompleteRequest(request); > +} > + > +void PipelineHandlerRkISP1::paramReady(Buffer *buffer) > +{ > + ASSERT(activeCamera_); > + Request *request = buffer->request(); > + RkISP1RequestData *reqData = > + static_cast<RkISP1RequestData *>(request->data); > + > + reqData->param = nullptr; > + > + paramBuffers_.push(buffer); > + > + tryCompleteRequest(request); > } > > REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1); >
Hi Kieran, On 2019-08-29 17:28:38 +0100, Kieran Bingham wrote: > Hi Niklas, > > The final piece of the puzzle :-D Thanks for your final piece of feedback ;-) > > On 28/08/2019 02:17, Niklas Söderlund wrote: > > Add the plumbing to the pipeline handler to interact with an IPA module. > > To support this parameter and statistic buffers needs to be associated > > with every request queued. The parameters buffer needs to be passed to > > the IPA before any buffer in the request is queued to hardware and the > > statistics buffer needs to be passed to the IPA for inspection as soon > > as it's ready. > > > > This change makes the usage of an IPA module mandatory for the rkisp1 > > pipeline. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 263 ++++++++++++++++++++++- > > 1 file changed, 252 insertions(+), 11 deletions(-) > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index de4ab523d0e4fe36..80d4832c49ebe78c 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -9,7 +9,7 @@ > > #include <array> > > #include <iomanip> > > #include <memory> > > -#include <vector> > > +#include <queue> > > > > #include <linux/media-bus-format.h> > > > > @@ -34,7 +34,7 @@ class RkISP1CameraData : public CameraData > > { > > public: > > RkISP1CameraData(PipelineHandler *pipe) > > - : CameraData(pipe), sensor_(nullptr) > > + : CameraData(pipe, 1, 1), sensor_(nullptr) > > { > > } > > > > @@ -43,8 +43,21 @@ public: > > delete sensor_; > > } > > > > + int initCameraData() override; > > + > > Stream stream_; > > CameraSensor *sensor_; > > + > > +private: > > + void updateSensor(V4L2ControlList controls); > > + void queueRequestHardware(const void *cookie); > > +}; > > + > > +class RkISP1RequestData : public RequestData > > +{ > > +public: > > + Buffer *stat; > > + Buffer *param; > > }; > > > > class RkISP1CameraConfiguration : public CameraConfiguration > > @@ -99,18 +112,69 @@ private: > > PipelineHandler::cameraData(camera)); > > } > > > > + friend RkISP1CameraData; > > Is this necessary? Isn't almost everything in a CameraData class public? > If this is required - should members in CameraData be moved to > protected/private? It's the other way around, this allows RkISP1CameraData to access the param_, stat_ and video_ members of RkISP1CameraConfiguration. But the question still holds, maybe there is little point in keeping them private? > > > + > > int initLinks(); > > int createCamera(MediaEntity *sensor); > > + void tryCompleteRequest(Request *request); > > void bufferReady(Buffer *buffer); > > + void statReady(Buffer *buffer); > > + void paramReady(Buffer *buffer); > > > > MediaDevice *media_; > > V4L2Subdevice *dphy_; > > V4L2Subdevice *isp_; > > V4L2VideoDevice *video_; > > + V4L2VideoDevice *stat_; > > + V4L2VideoDevice *param_; > > + > > + BufferPool statPool_; > > + BufferPool paramPool_; > > + > > + std::queue<Buffer *> statBuffers_; > > + std::queue<Buffer *> paramBuffers_; > > Wouldn't these be associated as buffers in a Request, and thus live on > that queue? The life cycle is that the Buffer object's are create at camera start and put in these queues. Once a request is "queued" to the pipeline a buffer is take of these queues and associated with the request. Before the request is complete (and returned to the application) the internal buffers are removed from the request and put back at the end of this queue. > > > > Camera *activeCamera_; > > }; > > > > +int RkISP1CameraData::initCameraData() > > +{ > > + ipa_->updateSensor.connect(this, > > + &RkISP1CameraData::updateSensor); > > + ipa_->queueRequest.connect(this, > > + &RkISP1CameraData::queueRequestHardware); > > + return 0; > > +} > > + > > +void RkISP1CameraData::updateSensor(V4L2ControlList controls) > > +{ > > + sensor_->setControls(&controls); > > +} > > + > > +void RkISP1CameraData::queueRequestHardware(const void *cookie) > > +{ > > + /* Translate cookie to request. */ > > + Request *request = reinterpret_cast<Request *>(const_cast<void *>(cookie)); > > C++ casts are so ... pleasant ... :S > > do you need the (const_cast<void *>.. ? Isn't cookie already a const void *? It is, but I need it to lose to const, as i can't reinterpret_cast() from a const object o a none const object. Agree this looks dumb and there might be a more clever way of doing it, but I can't find it :-( > > > > + PipelineHandlerRkISP1 *pipe = > > + static_cast<PipelineHandlerRkISP1 *>(pipe_); > > + RkISP1RequestData *reqData = > > + static_cast<RkISP1RequestData *>(request->data); > > + Buffer *buffer = request->findBuffer(&stream_); > > + int ret; > > + > > + ret = pipe->param_->queueBuffer(reqData->param); > > + if (ret < 0) > > + LOG(RkISP1, Error) << "Failed to queue paramaeters"; > > s/paramaeters/parameters/ > > > > + > > + ret = pipe->stat_->queueBuffer(reqData->stat); > > + if (ret < 0) > > + LOG(RkISP1, Error) << "Failed to queue statistics"; > > I wonder if we should keep a counter somewhere of all 'failures' which > could not be returned... > > I guess it depends on whether we would somewhat successfully continue or > not without the statistics buffer queued. I don't think we would retain a good image quality if this fails, maybe this should be made a Fatal error? It really should not happen. > > > + > > + ret = pipe->video_->queueBuffer(buffer); > > + if (ret < 0) > > + LOG(RkISP1, Error) << "Failed to queue video"; > > +} > > + > > RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera, > > RkISP1CameraData *data) > > : CameraConfiguration() > > @@ -202,12 +266,14 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() > > > > PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager) > > : PipelineHandler(manager), dphy_(nullptr), isp_(nullptr), > > - video_(nullptr) > > + video_(nullptr), stat_(nullptr), param_(nullptr) > > { > > } > > > > PipelineHandlerRkISP1::~PipelineHandlerRkISP1() > > { > > + delete param_; > > + delete stat_; > > delete video_; > > delete isp_; > > delete dphy_; > > @@ -317,6 +383,20 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) > > if (ret) > > return ret; > > > > + V4L2DeviceFormat statFormat = {}; > > + statFormat.fourcc = V4L2_META_FMT_RK_ISP1_STAT_3A; > > + > > + ret = stat_->setFormat(&statFormat); > > + if (ret) > > + return ret; > > It's fine to have it, I'm sure - but is this required? > > Or will a device which only supports one fourcc, not already be > configured to use that fourcc? It should. > > Probably good to be explicit even if it's not required though. I'm open to not check return value here but should we not then change the return type of setFormat() to void? > > > > + V4L2DeviceFormat paramFormat = {}; > > + paramFormat.fourcc = V4L2_META_FMT_RK_ISP1_PARAMS; > > + > > + ret = param_->setFormat(¶mFormat); > > + if (ret) > > + return ret; > > + > > if (outputFormat.size != cfg.size || > > outputFormat.fourcc != cfg.pixelFormat) { > > LOG(RkISP1, Error) > > @@ -333,30 +413,92 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera, > > const std::set<Stream *> &streams) > > { > > Stream *stream = *streams.begin(); > > + int ret; > > > > if (stream->memoryType() == InternalMemory) > > - return video_->exportBuffers(&stream->bufferPool()); > > + ret = video_->exportBuffers(&stream->bufferPool()); > > else > > - return video_->importBuffers(&stream->bufferPool()); > > + ret = video_->importBuffers(&stream->bufferPool()); > > + > > + if (ret) > > + return ret; > > + > > + statPool_.createBuffers(stream->configuration().bufferCount); > > + ret = stat_->exportBuffers(&statPool_); > > + if (ret) { > > + video_->releaseBuffers(); > > Can we let these be handled by the destructor? > or just call freeBuffers()? > > > + return ret; > > + } > > + > > + paramPool_.createBuffers(stream->configuration().bufferCount); > > + ret = param_->exportBuffers(¶mPool_); > > + if (ret) { > > + stat_->releaseBuffers(); > > + video_->releaseBuffers(); > > And these? The destructor is only called once the camera is destroyed right? This will undo what happens at stream on, potentially allowing a second stream on to succeed. > > > + return ret; > > + } > > + > > + for (unsigned int i = 0; i < stream->configuration().bufferCount; i++) { > > + statBuffers_.push(new Buffer(i)); > > + paramBuffers_.push(new Buffer(i)); > > + } > > + > > + return ret; > > } > > > > int PipelineHandlerRkISP1::freeBuffers(Camera *camera, > > const std::set<Stream *> &streams) > > { > > + while (!paramBuffers_.empty()) > > + paramBuffers_.pop(); > > + > > + while (!statBuffers_.empty()) > > + statBuffers_.pop(); > > Does the queue not have a .clear() operator? > No. > > However stackoverflow [0] informs me that you can just assign an empty > queue to empty the queue... > > paramBuffers_ = {}; > statBuffers_ = {}; > > I don't know if that's better or not :D > > [0] > https://stackoverflow.com/questions/3874624/why-stdqueue-doesnt-support-clear-function/3874743#3874743 > > I wonder if we'll end up needing to use a deque anyway, so that we can > 'peek' into the queue to determine if we need to take any actions early > based on achieving per-frame-controls I like this explicit popping better then assigning an empty queue. But I think this is just a matter of taste. > > > > + > > + if (param_->releaseBuffers()) > > + LOG(RkISP1, Error) << "Failed to release parameters buffers"; > > + > > + if (stat_->releaseBuffers()) > > + LOG(RkISP1, Error) << "Failed to release stat buffers"; > > + > > if (video_->releaseBuffers()) > > - LOG(RkISP1, Error) << "Failed to release buffers"; > > + LOG(RkISP1, Error) << "Failed to release video buffers"; > > > > return 0; > > } > > > > int PipelineHandlerRkISP1::start(Camera *camera) > > { > > + RkISP1CameraData *data = cameraData(camera); > > int ret; > > > > + ret = data->ipa_->initSensor(data->sensor_->controls()); > > + if (ret) > > + return ret; > > + > > + ret = param_->streamOn(); > > + if (ret) { > > + LOG(RkISP1, Error) > > + << "Failed to start parameters " << camera->name(); > > + return ret; > > + } > > + > > + ret = stat_->streamOn(); > > + if (ret) { > > + param_->streamOff(); > > + LOG(RkISP1, Error) > > + << "Failed to start statisticis " << camera->name(); > > s/statisticis/statistics/ > > > + return ret; > > + } > > + > > ret = video_->streamOn(); > > - if (ret) > > + if (ret) { > > + param_->streamOff(); > > + stat_->streamOff(); > > + > > LOG(RkISP1, Error) > > << "Failed to start camera " << camera->name(); > > + } > > > > activeCamera_ = camera; > > > > @@ -372,6 +514,16 @@ void PipelineHandlerRkISP1::stop(Camera *camera) > > LOG(RkISP1, Warning) > > << "Failed to stop camera " << camera->name(); > > > > + ret = stat_->streamOff(); > > + if (ret) > > + LOG(RkISP1, Warning) > > + << "Failed to stop statisticis " << camera->name(); > > > s/statisticis/statistics/ > > > + > > + ret = param_->streamOff(); > > + if (ret) > > + LOG(RkISP1, Warning) > > + << "Failed to stop parameters " << camera->name(); > > + > > activeCamera_ = nullptr; > > } > > > > @@ -380,6 +532,16 @@ int PipelineHandlerRkISP1::queueRequest(Camera *camera, Request *request) > > RkISP1CameraData *data = cameraData(camera); > > Stream *stream = &data->stream_; > > > > + if (paramBuffers_.empty()) { > > + LOG(RkISP1, Error) << "Parameters buffer underrun"; > > + return -ENOENT; > > + } > > + > > + if (statBuffers_.empty()) { > > + LOG(RkISP1, Error) << "Statisitc buffer underrun"; > > s/Statisitc/Statistic/ > > > + return -ENOENT; > > + } > > + > > Buffer *buffer = request->findBuffer(stream); > > if (!buffer) { > > LOG(RkISP1, Error) > > @@ -387,12 +549,24 @@ int PipelineHandlerRkISP1::queueRequest(Camera *camera, Request *request) > > return -ENOENT; > > } > > > > - int ret = video_->queueBuffer(buffer); > > - if (ret < 0) > > - return ret; > > + RkISP1RequestData *reqData = new RkISP1RequestData(); > > + request->data = reqData; > > + reqData->param = paramBuffers_.front(); > > + reqData->stat = statBuffers_.front(); > > + > > + prepareInternalBuffer(reqData->param, request, > > + ¶mPool_.buffers()[reqData->param->index()]); > > + prepareInternalBuffer(reqData->stat, request, > > + &statPool_.buffers()[reqData->stat->index()]); > > + > > + paramBuffers_.pop(); > > + statBuffers_.pop(); > > > > PipelineHandler::queueRequest(camera, request); > > > > + data->ipa_->processRequest(request, request->controls(), > > + *reqData->param); > > + > > return 0; > > } > > > > @@ -435,6 +609,10 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor) > > std::unique_ptr<RkISP1CameraData> data = > > utils::make_unique<RkISP1CameraData>(this); > > > > + data->controlInfo_.emplace(std::piecewise_construct, > > + std::forward_as_tuple(AeEnable), > > + std::forward_as_tuple(AeEnable, false, true)); > > + > > data->sensor_ = new CameraSensor(sensor); > > ret = data->sensor_->init(); > > if (ret) > > @@ -478,7 +656,17 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator) > > if (video_->open() < 0) > > return false; > > > > + stat_ = V4L2VideoDevice::fromEntityName(media_, "rkisp1-statistics"); > > + if (stat_->open() < 0) > > + return false; > > + > > + param_ = V4L2VideoDevice::fromEntityName(media_, "rkisp1-input-params"); > > + if (param_->open() < 0) > > + return false; > > Ah - good, I see these entities are already in the DeviceMatch. > > > > + > > video_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady); > > + stat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady); > > + param_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady); > > > > /* Configure default links. */ > > if (initLinks() < 0) { > > @@ -504,13 +692,66 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator) > > * Buffer Handling > > */ > > > > +void PipelineHandlerRkISP1::tryCompleteRequest(Request *request) > > +{ > > + RkISP1RequestData *reqData = > > + static_cast<RkISP1RequestData *>(request->data); > > + > > + if (reqData->param) > > + return; > > + > > + if (reqData->stat) > > + return; > > + > > + if (request->hasPendingBuffers()) > > + return; > > Hrm... for some reason I assumed these buffers would be tied into the > hasPendingBuffers() calls ... but maybe that would require creating new > streams. And these are 'internal' streams ... I do not view them as streams, but you are right maybe we should. But I don't think adding them to hasPendingBuffers() logic is the right move, but I might be wrong. > > > + > > + delete reqData; > > + request->data = nullptr; > > + > > + completeRequest(activeCamera_, request); > > +} > > + > > void PipelineHandlerRkISP1::bufferReady(Buffer *buffer) > > { > > ASSERT(activeCamera_); > > Request *request = buffer->request(); > > > > completeBuffer(activeCamera_, request, buffer); > > - completeRequest(activeCamera_, request); > > + tryCompleteRequest(request); > > I feel like somehow the param and stats buffers should be more directly > associated with the Request to facilitate the existing completeRequest() > mechanism, which is what I thought it was for. > > Is there anything preventing that? I think tryCompleteRequest() to include more things than just buffers (completion of meta data, status control bellow) for example. I might be wrong but for now lets keep it in the pipeline handler and generalize later once we have more IPA implementations. > > > > +} > > + > > +void PipelineHandlerRkISP1::statReady(Buffer *buffer) > > +{ > > + ASSERT(activeCamera_); > > + RkISP1CameraData *data = cameraData(activeCamera_); > > + Request *request = buffer->request(); > > + RkISP1RequestData *reqData = > > + static_cast<RkISP1RequestData *>(request->data); > > + > > + data->ipa_->updateStatistics(request, *buffer); > > + > > + /* TODO: Fetch libcamera status controls from IPA */ > > status controls? > > Do we perhaps need to support two interactions with the IPA per request? > One at queue time, and one at complete? Yes, I'm working on this on-top of this series. I think I'm going to go with another signal to carry meta data out of the IPA after the ISP have finished processing, but not sure yet. > > > > + > > + reqData->stat = nullptr; > > + > > + statBuffers_.push(buffer); > > + > > + tryCompleteRequest(request); > > +} > > + > > +void PipelineHandlerRkISP1::paramReady(Buffer *buffer) > > +{ > > + ASSERT(activeCamera_); > > + Request *request = buffer->request(); > > + RkISP1RequestData *reqData = > > + static_cast<RkISP1RequestData *>(request->data); > > + > > + reqData->param = nullptr; > > + > > + paramBuffers_.push(buffer); > > + > > + tryCompleteRequest(request); > > } > > > > REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1); > > > > -- > Regards > -- > Kieran
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index de4ab523d0e4fe36..80d4832c49ebe78c 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -9,7 +9,7 @@ #include <array> #include <iomanip> #include <memory> -#include <vector> +#include <queue> #include <linux/media-bus-format.h> @@ -34,7 +34,7 @@ class RkISP1CameraData : public CameraData { public: RkISP1CameraData(PipelineHandler *pipe) - : CameraData(pipe), sensor_(nullptr) + : CameraData(pipe, 1, 1), sensor_(nullptr) { } @@ -43,8 +43,21 @@ public: delete sensor_; } + int initCameraData() override; + Stream stream_; CameraSensor *sensor_; + +private: + void updateSensor(V4L2ControlList controls); + void queueRequestHardware(const void *cookie); +}; + +class RkISP1RequestData : public RequestData +{ +public: + Buffer *stat; + Buffer *param; }; class RkISP1CameraConfiguration : public CameraConfiguration @@ -99,18 +112,69 @@ private: PipelineHandler::cameraData(camera)); } + friend RkISP1CameraData; + int initLinks(); int createCamera(MediaEntity *sensor); + void tryCompleteRequest(Request *request); void bufferReady(Buffer *buffer); + void statReady(Buffer *buffer); + void paramReady(Buffer *buffer); MediaDevice *media_; V4L2Subdevice *dphy_; V4L2Subdevice *isp_; V4L2VideoDevice *video_; + V4L2VideoDevice *stat_; + V4L2VideoDevice *param_; + + BufferPool statPool_; + BufferPool paramPool_; + + std::queue<Buffer *> statBuffers_; + std::queue<Buffer *> paramBuffers_; Camera *activeCamera_; }; +int RkISP1CameraData::initCameraData() +{ + ipa_->updateSensor.connect(this, + &RkISP1CameraData::updateSensor); + ipa_->queueRequest.connect(this, + &RkISP1CameraData::queueRequestHardware); + return 0; +} + +void RkISP1CameraData::updateSensor(V4L2ControlList controls) +{ + sensor_->setControls(&controls); +} + +void RkISP1CameraData::queueRequestHardware(const void *cookie) +{ + /* Translate cookie to request. */ + Request *request = reinterpret_cast<Request *>(const_cast<void *>(cookie)); + PipelineHandlerRkISP1 *pipe = + static_cast<PipelineHandlerRkISP1 *>(pipe_); + RkISP1RequestData *reqData = + static_cast<RkISP1RequestData *>(request->data); + Buffer *buffer = request->findBuffer(&stream_); + int ret; + + ret = pipe->param_->queueBuffer(reqData->param); + if (ret < 0) + LOG(RkISP1, Error) << "Failed to queue paramaeters"; + + ret = pipe->stat_->queueBuffer(reqData->stat); + if (ret < 0) + LOG(RkISP1, Error) << "Failed to queue statistics"; + + ret = pipe->video_->queueBuffer(buffer); + if (ret < 0) + LOG(RkISP1, Error) << "Failed to queue video"; +} + RkISP1CameraConfiguration::RkISP1CameraConfiguration(Camera *camera, RkISP1CameraData *data) : CameraConfiguration() @@ -202,12 +266,14 @@ CameraConfiguration::Status RkISP1CameraConfiguration::validate() PipelineHandlerRkISP1::PipelineHandlerRkISP1(CameraManager *manager) : PipelineHandler(manager), dphy_(nullptr), isp_(nullptr), - video_(nullptr) + video_(nullptr), stat_(nullptr), param_(nullptr) { } PipelineHandlerRkISP1::~PipelineHandlerRkISP1() { + delete param_; + delete stat_; delete video_; delete isp_; delete dphy_; @@ -317,6 +383,20 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c) if (ret) return ret; + V4L2DeviceFormat statFormat = {}; + statFormat.fourcc = V4L2_META_FMT_RK_ISP1_STAT_3A; + + ret = stat_->setFormat(&statFormat); + if (ret) + return ret; + + V4L2DeviceFormat paramFormat = {}; + paramFormat.fourcc = V4L2_META_FMT_RK_ISP1_PARAMS; + + ret = param_->setFormat(¶mFormat); + if (ret) + return ret; + if (outputFormat.size != cfg.size || outputFormat.fourcc != cfg.pixelFormat) { LOG(RkISP1, Error) @@ -333,30 +413,92 @@ int PipelineHandlerRkISP1::allocateBuffers(Camera *camera, const std::set<Stream *> &streams) { Stream *stream = *streams.begin(); + int ret; if (stream->memoryType() == InternalMemory) - return video_->exportBuffers(&stream->bufferPool()); + ret = video_->exportBuffers(&stream->bufferPool()); else - return video_->importBuffers(&stream->bufferPool()); + ret = video_->importBuffers(&stream->bufferPool()); + + if (ret) + return ret; + + statPool_.createBuffers(stream->configuration().bufferCount); + ret = stat_->exportBuffers(&statPool_); + if (ret) { + video_->releaseBuffers(); + return ret; + } + + paramPool_.createBuffers(stream->configuration().bufferCount); + ret = param_->exportBuffers(¶mPool_); + if (ret) { + stat_->releaseBuffers(); + video_->releaseBuffers(); + return ret; + } + + for (unsigned int i = 0; i < stream->configuration().bufferCount; i++) { + statBuffers_.push(new Buffer(i)); + paramBuffers_.push(new Buffer(i)); + } + + return ret; } int PipelineHandlerRkISP1::freeBuffers(Camera *camera, const std::set<Stream *> &streams) { + while (!paramBuffers_.empty()) + paramBuffers_.pop(); + + while (!statBuffers_.empty()) + statBuffers_.pop(); + + if (param_->releaseBuffers()) + LOG(RkISP1, Error) << "Failed to release parameters buffers"; + + if (stat_->releaseBuffers()) + LOG(RkISP1, Error) << "Failed to release stat buffers"; + if (video_->releaseBuffers()) - LOG(RkISP1, Error) << "Failed to release buffers"; + LOG(RkISP1, Error) << "Failed to release video buffers"; return 0; } int PipelineHandlerRkISP1::start(Camera *camera) { + RkISP1CameraData *data = cameraData(camera); int ret; + ret = data->ipa_->initSensor(data->sensor_->controls()); + if (ret) + return ret; + + ret = param_->streamOn(); + if (ret) { + LOG(RkISP1, Error) + << "Failed to start parameters " << camera->name(); + return ret; + } + + ret = stat_->streamOn(); + if (ret) { + param_->streamOff(); + LOG(RkISP1, Error) + << "Failed to start statisticis " << camera->name(); + return ret; + } + ret = video_->streamOn(); - if (ret) + if (ret) { + param_->streamOff(); + stat_->streamOff(); + LOG(RkISP1, Error) << "Failed to start camera " << camera->name(); + } activeCamera_ = camera; @@ -372,6 +514,16 @@ void PipelineHandlerRkISP1::stop(Camera *camera) LOG(RkISP1, Warning) << "Failed to stop camera " << camera->name(); + ret = stat_->streamOff(); + if (ret) + LOG(RkISP1, Warning) + << "Failed to stop statisticis " << camera->name(); + + ret = param_->streamOff(); + if (ret) + LOG(RkISP1, Warning) + << "Failed to stop parameters " << camera->name(); + activeCamera_ = nullptr; } @@ -380,6 +532,16 @@ int PipelineHandlerRkISP1::queueRequest(Camera *camera, Request *request) RkISP1CameraData *data = cameraData(camera); Stream *stream = &data->stream_; + if (paramBuffers_.empty()) { + LOG(RkISP1, Error) << "Parameters buffer underrun"; + return -ENOENT; + } + + if (statBuffers_.empty()) { + LOG(RkISP1, Error) << "Statisitc buffer underrun"; + return -ENOENT; + } + Buffer *buffer = request->findBuffer(stream); if (!buffer) { LOG(RkISP1, Error) @@ -387,12 +549,24 @@ int PipelineHandlerRkISP1::queueRequest(Camera *camera, Request *request) return -ENOENT; } - int ret = video_->queueBuffer(buffer); - if (ret < 0) - return ret; + RkISP1RequestData *reqData = new RkISP1RequestData(); + request->data = reqData; + reqData->param = paramBuffers_.front(); + reqData->stat = statBuffers_.front(); + + prepareInternalBuffer(reqData->param, request, + ¶mPool_.buffers()[reqData->param->index()]); + prepareInternalBuffer(reqData->stat, request, + &statPool_.buffers()[reqData->stat->index()]); + + paramBuffers_.pop(); + statBuffers_.pop(); PipelineHandler::queueRequest(camera, request); + data->ipa_->processRequest(request, request->controls(), + *reqData->param); + return 0; } @@ -435,6 +609,10 @@ int PipelineHandlerRkISP1::createCamera(MediaEntity *sensor) std::unique_ptr<RkISP1CameraData> data = utils::make_unique<RkISP1CameraData>(this); + data->controlInfo_.emplace(std::piecewise_construct, + std::forward_as_tuple(AeEnable), + std::forward_as_tuple(AeEnable, false, true)); + data->sensor_ = new CameraSensor(sensor); ret = data->sensor_->init(); if (ret) @@ -478,7 +656,17 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator) if (video_->open() < 0) return false; + stat_ = V4L2VideoDevice::fromEntityName(media_, "rkisp1-statistics"); + if (stat_->open() < 0) + return false; + + param_ = V4L2VideoDevice::fromEntityName(media_, "rkisp1-input-params"); + if (param_->open() < 0) + return false; + video_->bufferReady.connect(this, &PipelineHandlerRkISP1::bufferReady); + stat_->bufferReady.connect(this, &PipelineHandlerRkISP1::statReady); + param_->bufferReady.connect(this, &PipelineHandlerRkISP1::paramReady); /* Configure default links. */ if (initLinks() < 0) { @@ -504,13 +692,66 @@ bool PipelineHandlerRkISP1::match(DeviceEnumerator *enumerator) * Buffer Handling */ +void PipelineHandlerRkISP1::tryCompleteRequest(Request *request) +{ + RkISP1RequestData *reqData = + static_cast<RkISP1RequestData *>(request->data); + + if (reqData->param) + return; + + if (reqData->stat) + return; + + if (request->hasPendingBuffers()) + return; + + delete reqData; + request->data = nullptr; + + completeRequest(activeCamera_, request); +} + void PipelineHandlerRkISP1::bufferReady(Buffer *buffer) { ASSERT(activeCamera_); Request *request = buffer->request(); completeBuffer(activeCamera_, request, buffer); - completeRequest(activeCamera_, request); + tryCompleteRequest(request); +} + +void PipelineHandlerRkISP1::statReady(Buffer *buffer) +{ + ASSERT(activeCamera_); + RkISP1CameraData *data = cameraData(activeCamera_); + Request *request = buffer->request(); + RkISP1RequestData *reqData = + static_cast<RkISP1RequestData *>(request->data); + + data->ipa_->updateStatistics(request, *buffer); + + /* TODO: Fetch libcamera status controls from IPA */ + + reqData->stat = nullptr; + + statBuffers_.push(buffer); + + tryCompleteRequest(request); +} + +void PipelineHandlerRkISP1::paramReady(Buffer *buffer) +{ + ASSERT(activeCamera_); + Request *request = buffer->request(); + RkISP1RequestData *reqData = + static_cast<RkISP1RequestData *>(request->data); + + reqData->param = nullptr; + + paramBuffers_.push(buffer); + + tryCompleteRequest(request); } REGISTER_PIPELINE_HANDLER(PipelineHandlerRkISP1);
Add the plumbing to the pipeline handler to interact with an IPA module. To support this parameter and statistic buffers needs to be associated with every request queued. The parameters buffer needs to be passed to the IPA before any buffer in the request is queued to hardware and the statistics buffer needs to be passed to the IPA for inspection as soon as it's ready. This change makes the usage of an IPA module mandatory for the rkisp1 pipeline. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 263 ++++++++++++++++++++++- 1 file changed, 252 insertions(+), 11 deletions(-)