[libcamera-devel] [PATCH 10/13] libcamera: pipeline: rkisp1: Configure self path
Kieran Bingham
kieran.bingham at ideasonboard.com
Mon Sep 14 13:24:22 CEST 2020
Hi Niklas,
On 14/09/2020 12:19, 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;
>
>
> edit: ohh yuck. I see
>>
>> + selfPathVideo_ = V4L2VideoDevice::fromEntityName(media_, "rkisp1_selfpath");
>
> so it's named the 'self path' at the ISP ...
>
Re-edit: Aha, I think I mis-understood this slightly. I thought this was
dealing with multiple camera paths ... but it might be dealing with
multiple stream paths from a single camera... so it's not separate pipes
necessarily ... :-S
Perhaps it's an RkISPStream object instead though ...
> --
> 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
More information about the libcamera-devel
mailing list