[libcamera-devel] [PATCH 10/13] libcamera: pipeline: rkisp1: Configure self path
Niklas Söderlund
niklas.soderlund at ragnatech.se
Mon Sep 14 13:27:33 CEST 2020
Hi Kieran,
On 2020-09-14 12:19:54 +0100, Kieran Bingham wrote:
> Hi Niklas,
>
> On 14/09/2020 12:09, Niklas Söderlund wrote:
> > Hi Jacopo,
> >
> > On 2020-08-20 11:46:52 +0200, Jacopo Mondi wrote:
> >> Hi Niklas,
> >>
> >> On Thu, Aug 13, 2020 at 02:52:43AM +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 | 197 ++++++++++++++++-------
> >>> 1 file changed, 139 insertions(+), 58 deletions(-)
> >>>
> >>> diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> >>> index 132878c13b10b555..671098e11a2e2750 100644
> >>> --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> >>> +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
> >>> @@ -133,7 +133,8 @@ public:
> >>> V4L2VideoDevice *selfPathVideo)
> >>> : CameraData(pipe), sensor_(nullptr), frame_(0),
> >>> frameInfo_(pipe), mainPathVideo_(mainPathVideo),
> >>> - selfPathVideo_(selfPathVideo)
> >>> + selfPathVideo_(selfPathVideo), mainPathActive_(false),
> >>> + selfPathActive_(false)
> >>> {
> >>> }
> >>>
> >>> @@ -145,6 +146,7 @@ public:
> >>> int loadIPA();
> >>>
> >>> Stream mainPathStream_;
> >>> + Stream selfPathStream_;
> >>
> >> Would this, and all other duplicated fields be nicer is stored as
> >> arrays and accessed with fixed indexes ? I don't mind the way it is
> >> today though
> >
> > I guess this is another bikeshedding topic. I don't see any clear
> > advantage of one over the other.
> >
> > mainPathStream_
> >
> > vs
> >
> > streams_[RkISP1::MainPAth]
> >
> > If we are to switch to an array approach I do think we should do so
> > before or after this series. I picked this solution as it was the one
> > already used in the pipeline.
>
>
> Jumping into the bikeshed mostly because it really stood out in the
> title of this and the preceding patch.
>
> The 'self' path sounds horrible.
> I assume it derives from 'selfie' ... but really it's just a secondary ...
>
>
> I would probably prefer the array route (can be refactored anytime,
> doesn't have to be here). If one path is more feature full than the
> other then perhaps naming them makes sense, but otherwise I'd just have
> them as numerical pipelines.
>
> And rather than have an array of streams_ and an array of
> V4L2VideoDevice and an array of Sensors etc... I think this can be
> moddelled as a class, and then it's just a single array of that class.
>
> (Or if there's only two, It could actually be just two instances of that
> class at that point)
>
> RkISPPipe main;
> RkISPPipe secondary;
This have been suggested to another patch in this series and I agree
it's a good idea. I think that work should go on-top (or as a series
before) tho as it will be quiet large in LoC and I don't wish to do that
work in this already large series.
>
>
> edit: ohh yuck. I see
> >
> > + selfPathVideo_ = V4L2VideoDevice::fromEntityName(media_, "rkisp1_selfpath");
>
> so it's named the 'self path' at the ISP ...
I know :-( So I think we are stuck with the man and self path names.
>
> --
> Kieran
>
>
>
> >>
> >>> CameraSensor *sensor_;
> >>> unsigned int frame_;
> >>> std::vector<IPABuffer> ipaBuffers_;
> >>> @@ -154,6 +156,9 @@ public:
> >>> V4L2VideoDevice *mainPathVideo_;
> >>> V4L2VideoDevice *selfPathVideo_;
> >>>
> >>> + bool mainPathActive_;
> >>> + bool selfPathActive_;
> >>> +
> >>> private:
> >>> void queueFrameAction(unsigned int frame,
> >>> const IPAOperationData &action);
> >>> @@ -615,7 +620,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;
> >>>
> >>> @@ -657,36 +661,54 @@ 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();
> >>> -
> >>> - V4L2DeviceFormat outputFormat = {};
> >>> - outputFormat.fourcc = mainPathVideo_->toV4L2PixelFormat(cfg.pixelFormat);
> >>> - outputFormat.size = cfg.size;
> >>> - outputFormat.planesCount = 2;
> >>> -
> >>> - 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;
> >>> + }
> >>> +
> >>> + ret = resizer->setFormat(0, &ispFormat);
> >>> + if (ret < 0)
> >>> + return ret;
> >>> +
> >>> + LOG(RkISP1, Debug) << "Resizer input pad configured with " << ispFormat.toString();
> >>> +
> >>> + ispFormat.size = cfg.size;
> >>> +
> >>> + LOG(RkISP1, Debug) << "Configuring resizer output pad with " << ispFormat.toString();
> >>> +
> >>> + ret = resizer->setFormat(1, &ispFormat);
> >>> + if (ret < 0)
> >>> + return ret;
> >>> +
> >>> + LOG(RkISP1, Debug) << "Resizer output pad configured with " << ispFormat.toString();
> >>> +
> >>> + V4L2DeviceFormat outputFormat = {};
> >>> + outputFormat.fourcc = video->toV4L2PixelFormat(cfg.pixelFormat);
> >>> + outputFormat.size = cfg.size;
> >>> + outputFormat.planesCount = 2;
> >>
> >> Doesn't the planes count depend on the format ?
> >
> > Good catch. This is a preexisting bug in the pipeline. I will add a
> > separate patch to this series to sort this out.
> >
> >>
> >>> +
> >>> + 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 = {};
> >>> @@ -701,28 +723,46 @@ int PipelineHandlerRkISP1::configure(Camera *camera, CameraConfiguration *c)
> >>> if (ret)
> >>> return ret;
> >>>
> >>> - cfg.setStream(&data->mainPathStream_);
> >>> -
> >>> return 0;
> >>> }
> >>>
> >>> int PipelineHandlerRkISP1::exportFrameBuffers(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_)
> >>> + 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,
> >>> + });
> >>> +
> >>> + if (data->mainPathActive_) {
> >>> + ret = mainPathVideo_->importBuffers(count);
> >>> + if (ret < 0)
> >>> + goto error;
> >>
> >> Is it safe to call releaseBuffers if importing fails ?
> >
> > Yes, is it not always safe to call releaseBuffers? The idea is that if
> > allocateBuffers() fails it should undo all operations. Remember we
> > depend on buffer orphaning so buffers allocated by for example the
> > FrameBufferAllocater are still OK.
> >
> >>
> >>> + }
> >>> +
> >>> + if (data->selfPathActive_) {
> >>> + ret = selfPathVideo_->importBuffers(count);
> >>> + if (ret < 0)
> >>> + goto error;
> >>> + }
> >>>
> >>> ret = param_->allocateBuffers(count, ¶mBuffers_);
> >>> if (ret < 0)
> >>> @@ -754,6 +794,7 @@ error:
> >>> paramBuffers_.clear();
> >>> statBuffers_.clear();
> >>> mainPathVideo_->releaseBuffers();
> >>> + selfPathVideo_->releaseBuffers();
> >>>
> >>> return ret;
> >>> }
> >>> @@ -787,6 +828,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;
> >>> }
> >>>
> >>> @@ -829,15 +873,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,
> >>> + };
> >>
> >> What's this for ? And why assume [0] is assigned to main path ?
> >
> > This is part of the yet undocumented IPA protocl for the RkISP1. The
> > stream configuration map is mainpath = 0 and selfpath = 1. When we get
> > the new IPC framework in place all of this should of course be
> > documented. In reality this is have currently no impact as the RkISP1
> > IPA does not examine the stream configuration at all.
> >
> >>
> >>> + }
> >>> +
> >>> + if (data->selfPathActive_) {
> >>> + ret = selfPathVideo_->streamOn();
> >>> + if (ret) {
> >>> + if (data->mainPathActive_)
> >>> + mainPathVideo_->streamOff();
> >>> +
> >>> + 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;
> >>> @@ -852,12 +928,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());
> >>>
> >>> @@ -873,10 +943,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();
> >>> + }
> >>
> >> Can we call streamOff unconditionally ? It depends on the driver
> >> implementation I guess
> >
> > We can call it unconditionally, I think even v4l2-compliance tests
> > multiple s_stream(1) and s_stream(0) calls does it not? But I agree
> > there might of course be driver specific quirks.
> >
> >>
> >> Overall the patch looks good!
> >> Thanks
> >> j
> >>
> >>> +
> >>> + if (data->mainPathActive_) {
> >>> + ret = mainPathVideo_->streamOff();
> >>> + if (ret)
> >>> + LOG(RkISP1, Warning)
> >>> + << "Failed to stop main path " << camera->id();
> >>> + }
> >>>
> >>> ret = stat_->streamOff();
> >>> if (ret)
> >>> @@ -960,6 +1039,8 @@ int PipelineHandlerRkISP1::initLinks(const Camera *camera,
> >>> std::string resizer;
> >>> if (cfg.stream() == &data->mainPathStream_)
> >>> resizer = "rkisp1_resizer_mainpath";
> >>> + else if (cfg.stream() == &data->selfPathStream_)
> >>> + resizer = "rkisp1_resizer_selfpath";
> >>> else
> >>> return -EINVAL;
> >>>
> >>> --
> >>> 2.28.0
> >>>
> >>> _______________________________________________
> >>> libcamera-devel mailing list
> >>> libcamera-devel at lists.libcamera.org
> >>> https://lists.libcamera.org/listinfo/libcamera-devel
> >
>
> --
> Regards
> --
> Kieran
--
Regards,
Niklas Söderlund
More information about the libcamera-devel
mailing list