[libcamera-devel] [PATCH 13/13] libcamera: pipeline: rkisp1: Attach to an IPA
Kieran Bingham
kieran.bingham at ideasonboard.com
Thu Aug 29 18:28:38 CEST 2019
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 at 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);
>
--
Regards
--
Kieran
More information about the libcamera-devel
mailing list