[libcamera-devel] [PATCH v2 10/13] libcamera: pipeline: rkisp1: Configure self path
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Tue Sep 15 03:14:06 CEST 2020
Hi Niklas,
Thank you for the patch.
On Mon, Sep 14, 2020 at 04:21:46PM +0200, Niklas Söderlund wrote:
> Allow for both the main and self path streams to be configured. This
> change adds the self path as an internal stream to the pipeline handler.
> It is not exposed as a Camera stream so it can not yet be used.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund at ragnatech.se>
> ---
> src/libcamera/pipeline/rkisp1/rkisp1.cpp | 200 ++++++++++++++++-------
> 1 file changed, 141 insertions(+), 59 deletions(-)
>
> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> index 5aa01f87afd71183..01cd461427e3a6dc 100644
> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> @@ -135,7 +135,8 @@ public:
> V4L2VideoDevice *selfPathVideo)
> : CameraData(pipe), sensor_(nullptr), frame_(0),
> frameInfo_(pipe), mainPathVideo_(mainPathVideo),
> - selfPathVideo_(selfPathVideo)
> + selfPathVideo_(selfPathVideo), mainPathActive_(false),
> + selfPathActive_(false)
> {
> }
>
> @@ -147,6 +148,7 @@ public:
> int loadIPA();
>
> Stream mainPathStream_;
> + Stream selfPathStream_;
> CameraSensor *sensor_;
> unsigned int frame_;
> std::vector<IPABuffer> ipaBuffers_;
> @@ -156,6 +158,9 @@ public:
> V4L2VideoDevice *mainPathVideo_;
> V4L2VideoDevice *selfPathVideo_;
>
> + bool mainPathActive_;
> + bool selfPathActive_;
> +
> private:
> void queueFrameAction(unsigned int frame,
> const IPAOperationData &action);
> @@ -617,7 +622,6 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> RkISP1CameraConfiguration *config =
> static_cast<RkISP1CameraConfiguration *>(c);
> RkISP1CameraData *data = cameraData(camera);
> - StreamConfiguration &cfg = config->at(0);
> CameraSensor *sensor = data->sensor_;
> int ret;
>
> @@ -659,37 +663,55 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
>
> LOG(RkISP1, Debug) << "ISP output pad configured with " << format.toString();
>
> - ret = mainPathResizer_->setFormat(0, &format);
> - if (ret < 0)
> - return ret;
> -
> - LOG(RkISP1, Debug) << "Resizer input pad configured with " << format.toString();
> -
> - format.size = cfg.size;
> -
> - LOG(RkISP1, Debug) << "Configuring resizer output pad with " << format.toString();
> -
> - ret = mainPathResizer_->setFormat(1, &format);
> - if (ret < 0)
> - return ret;
> -
> - LOG(RkISP1, Debug) << "Resizer output pad configured with " << format.toString();
> -
> - const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
> - V4L2DeviceFormat outputFormat = {};
> - outputFormat.fourcc = mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat);
> - outputFormat.size = cfg.size;
> - outputFormat.planesCount = info.numPlanes();
> -
> - ret = mainPathVideo_->setFormat(&outputFormat);
> - if (ret)
> - return ret;
> -
> - if (outputFormat.size != cfg.size ||
> - outputFormat.fourcc != mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat)) {
> - LOG(RkISP1, Error)
> - << "Unable to configure capture in " << cfg.toString();
> - return -EINVAL;
> + data->mainPathActive_ = false;
> + data->selfPathActive_ = false;
> + for (const StreamConfiguration &cfg : *config) {
> + V4L2SubdeviceFormat ispFormat = format;
> + V4L2Subdevice *resizer;
> + V4L2VideoDevice *video;
> +
> + if (cfg.stream() == &data->mainPathStream_) {
> + resizer = mainPathResizer_;
> + video = mainPathVideo_;
> + data->mainPathActive_ = true;
> + } else {
> + resizer = selfPathResizer_;
> + video = selfPathVideo_;
> + data->selfPathActive_ = true;
> + }
Pretty please, could you address the request from 08/13 on top ? This is
getting messy. I wouldn't mind if it was addressed as part of this
series, but going right on top would be acceptable.
> +
> + ret = resizer->setFormat(0, &ispFormat);
> + if (ret < 0)
> + return ret;
> +
> + LOG(RkISP1, Debug) << "Resizer input pad configured with " << ispFormat.toString();
LOG(RkISP1, Debug)
<< "Resizer input pad configured with "
<< ispFormat.toString();
and should this print which resizer is configured ?
> +
> + ispFormat.size = cfg.size;
> +
> + LOG(RkISP1, Debug) << "Configuring resizer output pad with " << ispFormat.toString();
Ditto.
> +
> + ret = resizer->setFormat(1, &ispFormat);
> + if (ret < 0)
> + return ret;
> +
> + LOG(RkISP1, Debug) << "Resizer output pad configured with " << ispFormat.toString();
And here too.
> +
> + const PixelFormatInfo &info = PixelFormatInfo::info(cfg.pixelFormat);
> + V4L2DeviceFormat outputFormat = {};
> + outputFormat.fourcc = video->toV4L2PixelFormat(cfg.pixelFormat);
> + outputFormat.size = cfg.size;
> + outputFormat.planesCount = info.numPlanes();
> +
> + ret = video->setFormat(&outputFormat);
> + if (ret)
> + return ret;
> +
> + if (outputFormat.size != cfg.size ||
> + outputFormat.fourcc != video->toV4L2PixelFormat(cfg.pixelFormat)) {
> + LOG(RkISP1, Error)
> + << "Unable to configure capture in " << cfg.toString();
> + return -EINVAL;
> + }
> }
>
> V4L2DeviceFormat paramFormat = {};
> @@ -704,28 +726,46 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> if (ret)
> return ret;
>
> - cfg.setStream(&data->mainPathStream_);
> -
> return 0;
> }
>
> int PipelineHandlerRkISP1::exportFrameBuffers([[maybe_unused]] Camera *camera, Stream *stream,
> std::vector<std::unique_ptr<FrameBuffer>> *buffers)
> {
> + RkISP1CameraData *data = cameraData(camera);
> unsigned int count = stream->configuration().bufferCount;
> - return mainPathVideo_->exportBuffers(count, buffers);
> +
> + if (stream == &data->mainPathStream_)
> + return mainPathVideo_->exportBuffers(count, buffers);
> +
> + if (stream == &data->selfPathStream_)
else if ?
> + return selfPathVideo_->exportBuffers(count, buffers);
> +
> + return -EINVAL;
> }
>
> int PipelineHandlerRkISP1::allocateBuffers(Camera *camera)
> {
> RkISP1CameraData *data = cameraData(camera);
> - unsigned int count = data->mainPathStream_.configuration().bufferCount;
> unsigned int ipaBufferId = 1;
> int ret;
>
> - ret = mainPathVideo_->importBuffers(count);
> - if (ret < 0)
> - goto error;
> + unsigned int count = std::max({
> + data->mainPathStream_.configuration().bufferCount,
> + data->selfPathStream_.configuration().bufferCount,
> + });
Why max, instead of using the respective values below ?
> +
> + if (data->mainPathActive_) {
> + ret = mainPathVideo_->importBuffers(count);
> + if (ret < 0)
> + goto error;
> + }
> +
> + if (data->selfPathActive_) {
> + ret = selfPathVideo_->importBuffers(count);
> + if (ret < 0)
> + goto error;
> + }
>
> ret = param_->allocateBuffers(count, ¶mBuffers_);
> if (ret < 0)
> @@ -757,6 +797,7 @@ error:
> paramBuffers_.clear();
> statBuffers_.clear();
> mainPathVideo_->releaseBuffers();
> + selfPathVideo_->releaseBuffers();
>
> return ret;
> }
> @@ -790,6 +831,9 @@ int PipelineHandlerRkISP1::freeBuffers(Camera *camera)
> if (mainPathVideo_->releaseBuffers())
> LOG(RkISP1, Error) << "Failed to release main path buffers";
>
> + if (selfPathVideo_->releaseBuffers())
> + LOG(RkISP1, Error) << "Failed to release self path buffers";
> +
> return 0;
> }
>
> @@ -832,15 +876,47 @@ int PipelineHandlerRkISP1::start(Camera *camera)
> return ret;
> }
>
> - ret = mainPathVideo_->streamOn();
> - if (ret) {
> - param_->streamOff();
> - stat_->streamOff();
> - data->ipa_->stop();
> - freeBuffers(camera);
> -
> - LOG(RkISP1, Error)
> - << "Failed to start camera " << camera->id();
> + std::map<unsigned int, IPAStream> streamConfig;
> +
> + if (data->mainPathActive_) {
> + ret = mainPathVideo_->streamOn();
> + if (ret) {
> + param_->streamOff();
> + stat_->streamOff();
> + data->ipa_->stop();
> + freeBuffers(camera);
> +
> + LOG(RkISP1, Error)
> + << "Failed to start main path " << camera->id();
> + return ret;
> + }
> +
> + streamConfig[0] = {
> + .pixelFormat = data->mainPathStream_.configuration().pixelFormat,
> + .size = data->mainPathStream_.configuration().size,
> + };
> + }
> +
> + if (data->selfPathActive_) {
> + ret = selfPathVideo_->streamOn();
> + if (ret) {
> + if (data->mainPathActive_)
> + mainPathVideo_->streamOff();
> +
This should stop mainPathVideo_ if it has been activated. The error
handling is really becoming nasty :-S Any chance it could be improved on
top ? Goto's are probably not a great option though as they would jump
over variable declarations. One optio would be to add an error handler
at the beginning of the function:
class ErrorHandler {
public:
ErrorHandler()
: disabled_(false)
{
}
~ErrorHandler()
{
if (disabled_)
return;
param_->streamOff();
stat_->streamOff();
data->ipa_->stop();
freeBuffers();
}
void disable() { disabled_ = true; }
private:
bool disabled_;
};
class ErrorHandler eh;
and call
eh.disable();
at the very end, before returning (names to be bikeshedded). We could
even make it a generic class that takes a lambda function as argument,
and calls it in the destructor. Usage here would become
ErrorHandler eh([&]() {
param_->streamOff();
stat_->streamOff();
data->ipa_->stop();
freeBuffers();
});
which doesn't look too bad I think.
This would require making the V4L2VideoDevice::streamOff(),
IPAProxy::stop() and PipelineHandlerRkISP1::freeBuffer() functions safe
to be called when already off, stopped of freed.
> + param_->streamOff();
> + stat_->streamOff();
> + data->ipa_->stop();
> + freeBuffers(camera);
> +
> + LOG(RkISP1, Error)
> + << "Failed to start self path " << camera->id();
> + return ret;
> + }
> +
> + streamConfig[1] = {
> + .pixelFormat = data->selfPathStream_.configuration().pixelFormat,
> + .size = data->selfPathStream_.configuration().size,
> + };
> }
>
> activeCamera_ = camera;
> @@ -855,12 +931,6 @@ int PipelineHandlerRkISP1::start(Camera *camera)
> ret = 0;
> }
>
> - std::map<unsigned int, IPAStream> streamConfig;
> - streamConfig[0] = {
> - .pixelFormat = data->mainPathStream_.configuration().pixelFormat,
> - .size = data->mainPathStream_.configuration().size,
> - };
> -
> std::map<unsigned int, const ControlInfoMap &> entityControls;
> entityControls.emplace(0, data->sensor_->controls());
>
> @@ -876,10 +946,19 @@ void PipelineHandlerRkISP1::stop(Camera *camera)
> RkISP1CameraData *data = cameraData(camera);
> int ret;
>
> - ret = mainPathVideo_->streamOff();
> - if (ret)
> - LOG(RkISP1, Warning)
> - << "Failed to stop camera " << camera->id();
> + if (data->selfPathActive_) {
> + ret = selfPathVideo_->streamOff();
> + if (ret)
> + LOG(RkISP1, Warning)
> + << "Failed to stop self path " << camera->id();
Maybe s/path/path for/ ? Or "Failed to stop self path (camera ...)" ?
> + }
> +
> + if (data->mainPathActive_) {
> + ret = mainPathVideo_->streamOff();
> + if (ret)
> + LOG(RkISP1, Warning)
> + << "Failed to stop main path " << camera->id();
> + }
>
> ret = stat_->streamOff();
> if (ret)
> @@ -964,6 +1043,9 @@ int PipelineHandlerRkISP1::initLinks(const Camera *camera,
> if (cfg.stream() == &data->mainPathStream_)
> link = media_->link("rkisp1_isp", 2,
> "rkisp1_resizer_mainpath", 0);
> + else if (cfg.stream() == &data->selfPathStream_)
> + link = media_->link("rkisp1_isp", 2,
> + "rkisp1_resizer_selfpath", 0);
This renders my alternative proposal in a previous patch moot, but you
can then skip the nullptr initialization of link.
> else
> return -EINVAL;
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list