| Message ID | 20210806101409.324645-4-umang.jain@ideasonboard.com |
|---|---|
| State | Superseded |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, Thank you for the patch. On Fri, Aug 06, 2021 at 03:44:08PM +0530, Umang Jain wrote: > Since VIMC is a virtual test driver, the support for IPA > buffers(parameter and statistics) is missing. We could create s/buffers/buffers / > our own and pass it around, but created buffers needs to be > backed by dmabuf file-descriptors ideally. > > This might prove cubersome given that we use vimc for test purposes. > Hence, pass actual frame data buffers into the IPA instead, which > would result in exercising these code paths for now. Introduce plumbing > support for mapping and un-mapping of these buffers into the IPA. I know what this is about, but if I didn't I'd have trouble following the rationale. Here's a possible improvement. VIMC is a virtual test driver that doesn't generate statistics or consume parameters buffers. Its pipeline handler thus doesn't map any buffer to the IPA. This limits the ability to test corresponding code paths in unit tests, as they use VIMC extensively. Creating fake statistics and parameters buffers would be cumbersome. Map the frame buffers to the IPA instead, to extend test coverage. > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > --- > include/libcamera/ipa/vimc.mojom | 3 +++ > src/ipa/vimc/vimc.cpp | 26 ++++++++++++++++++++++++++ > src/libcamera/pipeline/vimc/vimc.cpp | 25 ++++++++++++++++++++++++- > 3 files changed, 53 insertions(+), 1 deletion(-) > > diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom > index 99b6412b..8452461d 100644 > --- a/include/libcamera/ipa/vimc.mojom > +++ b/include/libcamera/ipa/vimc.mojom > @@ -26,6 +26,9 @@ interface IPAVimcInterface { > > start() => (int32 ret); > stop(); > + > + mapBuffers(array<libcamera.IPABuffer> buffers); > + unmapBuffers(array<uint32> ids); > }; > > interface IPAVimcEventInterface { > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp > index 54d9086a..2b00687f 100644 > --- a/src/ipa/vimc/vimc.cpp > +++ b/src/ipa/vimc/vimc.cpp > @@ -19,6 +19,8 @@ > #include <libcamera/ipa/ipa_interface.h> > #include <libcamera/ipa/ipa_module_info.h> > > +#include "libcamera/internal/framebuffer.h" > + > namespace libcamera { > > LOG_DEFINE_CATEGORY(IPAVimc) > @@ -38,11 +40,15 @@ public: > const std::map<unsigned int, IPAStream> &streamConfig, > const std::map<unsigned int, ControlInfoMap> &entityControls) override; > > + void mapBuffers(const std::vector<IPABuffer> &buffers) override; > + void unmapBuffers(const std::vector<unsigned int> &ids) override; > + > private: > void initTrace(); > void trace(enum ipa::vimc::IPATraceCode operation); > > int fd_; > + std::map<unsigned int, MappedFrameBuffer> buffers_; > }; > > IPAVimc::IPAVimc() > @@ -99,6 +105,26 @@ int IPAVimc::configure([[maybe_unused]] const IPACameraSensorInfo &sensorInfo, > return 0; > } > > +void IPAVimc::mapBuffers(const std::vector<IPABuffer> &buffers) > +{ > + for (const IPABuffer &buffer : buffers) { > + const FrameBuffer fb(buffer.planes); > + buffers_.emplace(buffer.id, > + MappedFrameBuffer(&fb, PROT_READ)); > + } > +} > + > +void IPAVimc::unmapBuffers(const std::vector<unsigned int> &ids) > +{ > + for (unsigned int id : ids) { > + auto it = buffers_.find(id); > + if (it == buffers_.end()) > + continue; > + > + buffers_.erase(it); > + } > +} > + > void IPAVimc::initTrace() > { > struct stat fifoStat; > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp > index b7dd6595..fa21128d 100644 > --- a/src/libcamera/pipeline/vimc/vimc.cpp > +++ b/src/libcamera/pipeline/vimc/vimc.cpp > @@ -45,7 +45,7 @@ class VimcCameraData : public CameraData > { > public: > VimcCameraData(PipelineHandler *pipe, MediaDevice *media) > - : CameraData(pipe), media_(media) > + : CameraData(pipe), media_(media), buffers_(nullptr) > { > } > > @@ -61,6 +61,9 @@ public: > Stream stream_; > > std::unique_ptr<ipa::vimc::IPAProxyVimc> ipa_; > + > + std::vector<std::unique_ptr<FrameBuffer>> *buffers_; > + std::vector<IPABuffer> ipaBuffers_; > }; > > class VimcCameraConfiguration : public CameraConfiguration > @@ -319,6 +322,7 @@ int PipelineHandlerVimc::exportFrameBuffers(Camera *camera, Stream *stream, > { > VimcCameraData *data = cameraData(camera); > unsigned int count = stream->configuration().bufferCount; > + data->buffers_ = buffers; > > return data->video_->exportBuffers(count, buffers); > } > @@ -332,6 +336,19 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlLis > if (ret < 0) > return ret; > > + /* > + * We don't have actual parameters/statistics buffers in VIMC. > + * Hence, map frame buffers to exercise IPA IPC code paths for now. > + */ > + ASSERT(data->buffers_ != nullptr); We have a problem here. There's nothing that guarantees that the frame buffers that have been queued have been exported by the pipeline handler. I'm afraid we won't be able to use frame buffers for this :-( It looks like we'll have to instead allocate fake stats buffers, even if it's cumbersome. There are at least two options to create dmabufs from userspace, dmabuf heaps or udmabuf. I'm not sure which one is best, but let's take into account the default permissions that distributions set on the related devices, to minimize issues. > + unsigned int ipaBufferId = 1; > + for (unsigned int i = 0; i< count; i++) { > + std::unique_ptr<FrameBuffer> &buffer = data->buffers_->at(i); > + buffer->setCookie(ipaBufferId++); > + data->ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes()); > + } > + data->ipa_->mapBuffers(data->ipaBuffers_); > + > ret = data->ipa_->start(); > if (ret) { > data->video_->releaseBuffers(); > @@ -352,7 +369,13 @@ void PipelineHandlerVimc::stop(Camera *camera) > { > VimcCameraData *data = cameraData(camera); > data->video_->streamOff(); > + > + std::vector<unsigned int> ids; > + for (IPABuffer &ipabuf : data->ipaBuffers_) > + ids.push_back(ipabuf.id); > + data->ipa_->unmapBuffers(ids); > data->ipa_->stop(); > + > data->video_->releaseBuffers(); > } >
Hi Laurent, On 8/10/21 7:35 AM, Laurent Pinchart wrote: > Hi Umang, > > Thank you for the patch. > > On Fri, Aug 06, 2021 at 03:44:08PM +0530, Umang Jain wrote: >> Since VIMC is a virtual test driver, the support for IPA >> buffers(parameter and statistics) is missing. We could create > s/buffers/buffers / > >> our own and pass it around, but created buffers needs to be >> backed by dmabuf file-descriptors ideally. >> >> This might prove cubersome given that we use vimc for test purposes. >> Hence, pass actual frame data buffers into the IPA instead, which >> would result in exercising these code paths for now. Introduce plumbing >> support for mapping and un-mapping of these buffers into the IPA. > I know what this is about, but if I didn't I'd have trouble following > the rationale. Here's a possible improvement. > > > VIMC is a virtual test driver that doesn't generate statistics or > consume parameters buffers. Its pipeline handler thus doesn't map any > buffer to the IPA. This limits the ability to test corresponding code > paths in unit tests, as they use VIMC extensively. > > Creating fake statistics and parameters buffers would be cumbersome. Map > the frame buffers to the IPA instead, to extend test coverage. > >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> --- >> include/libcamera/ipa/vimc.mojom | 3 +++ >> src/ipa/vimc/vimc.cpp | 26 ++++++++++++++++++++++++++ >> src/libcamera/pipeline/vimc/vimc.cpp | 25 ++++++++++++++++++++++++- >> 3 files changed, 53 insertions(+), 1 deletion(-) >> >> diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom >> index 99b6412b..8452461d 100644 >> --- a/include/libcamera/ipa/vimc.mojom >> +++ b/include/libcamera/ipa/vimc.mojom >> @@ -26,6 +26,9 @@ interface IPAVimcInterface { >> >> start() => (int32 ret); >> stop(); >> + >> + mapBuffers(array<libcamera.IPABuffer> buffers); >> + unmapBuffers(array<uint32> ids); >> }; >> >> interface IPAVimcEventInterface { >> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp >> index 54d9086a..2b00687f 100644 >> --- a/src/ipa/vimc/vimc.cpp >> +++ b/src/ipa/vimc/vimc.cpp >> @@ -19,6 +19,8 @@ >> #include <libcamera/ipa/ipa_interface.h> >> #include <libcamera/ipa/ipa_module_info.h> >> >> +#include "libcamera/internal/framebuffer.h" >> + >> namespace libcamera { >> >> LOG_DEFINE_CATEGORY(IPAVimc) >> @@ -38,11 +40,15 @@ public: >> const std::map<unsigned int, IPAStream> &streamConfig, >> const std::map<unsigned int, ControlInfoMap> &entityControls) override; >> >> + void mapBuffers(const std::vector<IPABuffer> &buffers) override; >> + void unmapBuffers(const std::vector<unsigned int> &ids) override; >> + >> private: >> void initTrace(); >> void trace(enum ipa::vimc::IPATraceCode operation); >> >> int fd_; >> + std::map<unsigned int, MappedFrameBuffer> buffers_; >> }; >> >> IPAVimc::IPAVimc() >> @@ -99,6 +105,26 @@ int IPAVimc::configure([[maybe_unused]] const IPACameraSensorInfo &sensorInfo, >> return 0; >> } >> >> +void IPAVimc::mapBuffers(const std::vector<IPABuffer> &buffers) >> +{ >> + for (const IPABuffer &buffer : buffers) { >> + const FrameBuffer fb(buffer.planes); >> + buffers_.emplace(buffer.id, >> + MappedFrameBuffer(&fb, PROT_READ)); >> + } >> +} >> + >> +void IPAVimc::unmapBuffers(const std::vector<unsigned int> &ids) >> +{ >> + for (unsigned int id : ids) { >> + auto it = buffers_.find(id); >> + if (it == buffers_.end()) >> + continue; >> + >> + buffers_.erase(it); >> + } >> +} >> + >> void IPAVimc::initTrace() >> { >> struct stat fifoStat; >> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp >> index b7dd6595..fa21128d 100644 >> --- a/src/libcamera/pipeline/vimc/vimc.cpp >> +++ b/src/libcamera/pipeline/vimc/vimc.cpp >> @@ -45,7 +45,7 @@ class VimcCameraData : public CameraData >> { >> public: >> VimcCameraData(PipelineHandler *pipe, MediaDevice *media) >> - : CameraData(pipe), media_(media) >> + : CameraData(pipe), media_(media), buffers_(nullptr) >> { >> } >> >> @@ -61,6 +61,9 @@ public: >> Stream stream_; >> >> std::unique_ptr<ipa::vimc::IPAProxyVimc> ipa_; >> + >> + std::vector<std::unique_ptr<FrameBuffer>> *buffers_; >> + std::vector<IPABuffer> ipaBuffers_; >> }; >> >> class VimcCameraConfiguration : public CameraConfiguration >> @@ -319,6 +322,7 @@ int PipelineHandlerVimc::exportFrameBuffers(Camera *camera, Stream *stream, >> { >> VimcCameraData *data = cameraData(camera); >> unsigned int count = stream->configuration().bufferCount; >> + data->buffers_ = buffers; >> >> return data->video_->exportBuffers(count, buffers); >> } >> @@ -332,6 +336,19 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlLis >> if (ret < 0) >> return ret; >> >> + /* >> + * We don't have actual parameters/statistics buffers in VIMC. >> + * Hence, map frame buffers to exercise IPA IPC code paths for now. >> + */ >> + ASSERT(data->buffers_ != nullptr); > We have a problem here. There's nothing that guarantees that the frame > buffers that have been queued have been exported by the pipeline > handler. I'm afraid we won't be able to use frame buffers for this :-( I don't understand. We are using buffers which are exported (see the above hunk of exportFormatBuffers()) The guarantees you have mentioned, Is it about checking data->video_->exportBuffers(count, buffers) has been successful, before using the buffers for IPA paths? > It looks like we'll have to instead allocate fake stats buffers, even if > it's cumbersome. There are at least two options to create dmabufs from > userspace, dmabuf heaps or udmabuf. I'm not sure which one is best, but > let's take into account the default permissions that distributions set > on the related devices, to minimize issues. > >> + unsigned int ipaBufferId = 1; >> + for (unsigned int i = 0; i< count; i++) { >> + std::unique_ptr<FrameBuffer> &buffer = data->buffers_->at(i); >> + buffer->setCookie(ipaBufferId++); >> + data->ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes()); >> + } >> + data->ipa_->mapBuffers(data->ipaBuffers_); >> + >> ret = data->ipa_->start(); >> if (ret) { >> data->video_->releaseBuffers(); >> @@ -352,7 +369,13 @@ void PipelineHandlerVimc::stop(Camera *camera) >> { >> VimcCameraData *data = cameraData(camera); >> data->video_->streamOff(); >> + >> + std::vector<unsigned int> ids; >> + for (IPABuffer &ipabuf : data->ipaBuffers_) >> + ids.push_back(ipabuf.id); >> + data->ipa_->unmapBuffers(ids); >> data->ipa_->stop(); >> + >> data->video_->releaseBuffers(); >> } >>
Hi Umang, On Tue, Aug 10, 2021 at 08:52:59AM +0530, Umang Jain wrote: > On 8/10/21 7:35 AM, Laurent Pinchart wrote: > > On Fri, Aug 06, 2021 at 03:44:08PM +0530, Umang Jain wrote: > >> Since VIMC is a virtual test driver, the support for IPA > >> buffers(parameter and statistics) is missing. We could create > > s/buffers/buffers / > > > >> our own and pass it around, but created buffers needs to be > >> backed by dmabuf file-descriptors ideally. > >> > >> This might prove cubersome given that we use vimc for test purposes. > >> Hence, pass actual frame data buffers into the IPA instead, which > >> would result in exercising these code paths for now. Introduce plumbing > >> support for mapping and un-mapping of these buffers into the IPA. > > > > I know what this is about, but if I didn't I'd have trouble following > > the rationale. Here's a possible improvement. > > > > VIMC is a virtual test driver that doesn't generate statistics or > > consume parameters buffers. Its pipeline handler thus doesn't map any > > buffer to the IPA. This limits the ability to test corresponding code > > paths in unit tests, as they use VIMC extensively. > > > > Creating fake statistics and parameters buffers would be cumbersome. Map > > the frame buffers to the IPA instead, to extend test coverage. > > > >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > >> --- > >> include/libcamera/ipa/vimc.mojom | 3 +++ > >> src/ipa/vimc/vimc.cpp | 26 ++++++++++++++++++++++++++ > >> src/libcamera/pipeline/vimc/vimc.cpp | 25 ++++++++++++++++++++++++- > >> 3 files changed, 53 insertions(+), 1 deletion(-) > >> > >> diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom > >> index 99b6412b..8452461d 100644 > >> --- a/include/libcamera/ipa/vimc.mojom > >> +++ b/include/libcamera/ipa/vimc.mojom > >> @@ -26,6 +26,9 @@ interface IPAVimcInterface { > >> > >> start() => (int32 ret); > >> stop(); > >> + > >> + mapBuffers(array<libcamera.IPABuffer> buffers); > >> + unmapBuffers(array<uint32> ids); > >> }; > >> > >> interface IPAVimcEventInterface { > >> diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp > >> index 54d9086a..2b00687f 100644 > >> --- a/src/ipa/vimc/vimc.cpp > >> +++ b/src/ipa/vimc/vimc.cpp > >> @@ -19,6 +19,8 @@ > >> #include <libcamera/ipa/ipa_interface.h> > >> #include <libcamera/ipa/ipa_module_info.h> > >> > >> +#include "libcamera/internal/framebuffer.h" > >> + > >> namespace libcamera { > >> > >> LOG_DEFINE_CATEGORY(IPAVimc) > >> @@ -38,11 +40,15 @@ public: > >> const std::map<unsigned int, IPAStream> &streamConfig, > >> const std::map<unsigned int, ControlInfoMap> &entityControls) override; > >> > >> + void mapBuffers(const std::vector<IPABuffer> &buffers) override; > >> + void unmapBuffers(const std::vector<unsigned int> &ids) override; > >> + > >> private: > >> void initTrace(); > >> void trace(enum ipa::vimc::IPATraceCode operation); > >> > >> int fd_; > >> + std::map<unsigned int, MappedFrameBuffer> buffers_; > >> }; > >> > >> IPAVimc::IPAVimc() > >> @@ -99,6 +105,26 @@ int IPAVimc::configure([[maybe_unused]] const IPACameraSensorInfo &sensorInfo, > >> return 0; > >> } > >> > >> +void IPAVimc::mapBuffers(const std::vector<IPABuffer> &buffers) > >> +{ > >> + for (const IPABuffer &buffer : buffers) { > >> + const FrameBuffer fb(buffer.planes); > >> + buffers_.emplace(buffer.id, > >> + MappedFrameBuffer(&fb, PROT_READ)); > >> + } > >> +} > >> + > >> +void IPAVimc::unmapBuffers(const std::vector<unsigned int> &ids) > >> +{ > >> + for (unsigned int id : ids) { > >> + auto it = buffers_.find(id); > >> + if (it == buffers_.end()) > >> + continue; > >> + > >> + buffers_.erase(it); > >> + } > >> +} > >> + > >> void IPAVimc::initTrace() > >> { > >> struct stat fifoStat; > >> diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp > >> index b7dd6595..fa21128d 100644 > >> --- a/src/libcamera/pipeline/vimc/vimc.cpp > >> +++ b/src/libcamera/pipeline/vimc/vimc.cpp > >> @@ -45,7 +45,7 @@ class VimcCameraData : public CameraData > >> { > >> public: > >> VimcCameraData(PipelineHandler *pipe, MediaDevice *media) > >> - : CameraData(pipe), media_(media) > >> + : CameraData(pipe), media_(media), buffers_(nullptr) > >> { > >> } > >> > >> @@ -61,6 +61,9 @@ public: > >> Stream stream_; > >> > >> std::unique_ptr<ipa::vimc::IPAProxyVimc> ipa_; > >> + > >> + std::vector<std::unique_ptr<FrameBuffer>> *buffers_; > >> + std::vector<IPABuffer> ipaBuffers_; > >> }; > >> > >> class VimcCameraConfiguration : public CameraConfiguration > >> @@ -319,6 +322,7 @@ int PipelineHandlerVimc::exportFrameBuffers(Camera *camera, Stream *stream, > >> { > >> VimcCameraData *data = cameraData(camera); > >> unsigned int count = stream->configuration().bufferCount; > >> + data->buffers_ = buffers; > >> > >> return data->video_->exportBuffers(count, buffers); > >> } > >> @@ -332,6 +336,19 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlLis > >> if (ret < 0) > >> return ret; > >> > >> + /* > >> + * We don't have actual parameters/statistics buffers in VIMC. > >> + * Hence, map frame buffers to exercise IPA IPC code paths for now. > >> + */ > >> + ASSERT(data->buffers_ != nullptr); > > > > We have a problem here. There's nothing that guarantees that the frame > > buffers that have been queued have been exported by the pipeline > > handler. I'm afraid we won't be able to use frame buffers for this :-( > > I don't understand. We are using buffers which are exported (see the > above hunk of exportFormatBuffers()) > > The guarantees you have mentioned, Is it about checking > data->video_->exportBuffers(count, buffers) > > has been successful, before using the buffers for IPA paths? All our unit tests and test applications use the FrameBufferAllocator, but that's not a requirement. An application may construct FrameBuffer instances itself, using dmabufs obtained elsewhere (exported by a display driver for instance). > > It looks like we'll have to instead allocate fake stats buffers, even if > > it's cumbersome. There are at least two options to create dmabufs from > > userspace, dmabuf heaps or udmabuf. I'm not sure which one is best, but > > let's take into account the default permissions that distributions set > > on the related devices, to minimize issues. > > > >> + unsigned int ipaBufferId = 1; > >> + for (unsigned int i = 0; i< count; i++) { > >> + std::unique_ptr<FrameBuffer> &buffer = data->buffers_->at(i); > >> + buffer->setCookie(ipaBufferId++); > >> + data->ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes()); > >> + } > >> + data->ipa_->mapBuffers(data->ipaBuffers_); > >> + > >> ret = data->ipa_->start(); > >> if (ret) { > >> data->video_->releaseBuffers(); > >> @@ -352,7 +369,13 @@ void PipelineHandlerVimc::stop(Camera *camera) > >> { > >> VimcCameraData *data = cameraData(camera); > >> data->video_->streamOff(); > >> + > >> + std::vector<unsigned int> ids; > >> + for (IPABuffer &ipabuf : data->ipaBuffers_) > >> + ids.push_back(ipabuf.id); > >> + data->ipa_->unmapBuffers(ids); > >> data->ipa_->stop(); > >> + > >> data->video_->releaseBuffers(); > >> } > >>
diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom index 99b6412b..8452461d 100644 --- a/include/libcamera/ipa/vimc.mojom +++ b/include/libcamera/ipa/vimc.mojom @@ -26,6 +26,9 @@ interface IPAVimcInterface { start() => (int32 ret); stop(); + + mapBuffers(array<libcamera.IPABuffer> buffers); + unmapBuffers(array<uint32> ids); }; interface IPAVimcEventInterface { diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp index 54d9086a..2b00687f 100644 --- a/src/ipa/vimc/vimc.cpp +++ b/src/ipa/vimc/vimc.cpp @@ -19,6 +19,8 @@ #include <libcamera/ipa/ipa_interface.h> #include <libcamera/ipa/ipa_module_info.h> +#include "libcamera/internal/framebuffer.h" + namespace libcamera { LOG_DEFINE_CATEGORY(IPAVimc) @@ -38,11 +40,15 @@ public: const std::map<unsigned int, IPAStream> &streamConfig, const std::map<unsigned int, ControlInfoMap> &entityControls) override; + void mapBuffers(const std::vector<IPABuffer> &buffers) override; + void unmapBuffers(const std::vector<unsigned int> &ids) override; + private: void initTrace(); void trace(enum ipa::vimc::IPATraceCode operation); int fd_; + std::map<unsigned int, MappedFrameBuffer> buffers_; }; IPAVimc::IPAVimc() @@ -99,6 +105,26 @@ int IPAVimc::configure([[maybe_unused]] const IPACameraSensorInfo &sensorInfo, return 0; } +void IPAVimc::mapBuffers(const std::vector<IPABuffer> &buffers) +{ + for (const IPABuffer &buffer : buffers) { + const FrameBuffer fb(buffer.planes); + buffers_.emplace(buffer.id, + MappedFrameBuffer(&fb, PROT_READ)); + } +} + +void IPAVimc::unmapBuffers(const std::vector<unsigned int> &ids) +{ + for (unsigned int id : ids) { + auto it = buffers_.find(id); + if (it == buffers_.end()) + continue; + + buffers_.erase(it); + } +} + void IPAVimc::initTrace() { struct stat fifoStat; diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp index b7dd6595..fa21128d 100644 --- a/src/libcamera/pipeline/vimc/vimc.cpp +++ b/src/libcamera/pipeline/vimc/vimc.cpp @@ -45,7 +45,7 @@ class VimcCameraData : public CameraData { public: VimcCameraData(PipelineHandler *pipe, MediaDevice *media) - : CameraData(pipe), media_(media) + : CameraData(pipe), media_(media), buffers_(nullptr) { } @@ -61,6 +61,9 @@ public: Stream stream_; std::unique_ptr<ipa::vimc::IPAProxyVimc> ipa_; + + std::vector<std::unique_ptr<FrameBuffer>> *buffers_; + std::vector<IPABuffer> ipaBuffers_; }; class VimcCameraConfiguration : public CameraConfiguration @@ -319,6 +322,7 @@ int PipelineHandlerVimc::exportFrameBuffers(Camera *camera, Stream *stream, { VimcCameraData *data = cameraData(camera); unsigned int count = stream->configuration().bufferCount; + data->buffers_ = buffers; return data->video_->exportBuffers(count, buffers); } @@ -332,6 +336,19 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlLis if (ret < 0) return ret; + /* + * We don't have actual parameters/statistics buffers in VIMC. + * Hence, map frame buffers to exercise IPA IPC code paths for now. + */ + ASSERT(data->buffers_ != nullptr); + unsigned int ipaBufferId = 1; + for (unsigned int i = 0; i< count; i++) { + std::unique_ptr<FrameBuffer> &buffer = data->buffers_->at(i); + buffer->setCookie(ipaBufferId++); + data->ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes()); + } + data->ipa_->mapBuffers(data->ipaBuffers_); + ret = data->ipa_->start(); if (ret) { data->video_->releaseBuffers(); @@ -352,7 +369,13 @@ void PipelineHandlerVimc::stop(Camera *camera) { VimcCameraData *data = cameraData(camera); data->video_->streamOff(); + + std::vector<unsigned int> ids; + for (IPABuffer &ipabuf : data->ipaBuffers_) + ids.push_back(ipabuf.id); + data->ipa_->unmapBuffers(ids); data->ipa_->stop(); + data->video_->releaseBuffers(); }
Since VIMC is a virtual test driver, the support for IPA buffers(parameter and statistics) is missing. We could create our own and pass it around, but created buffers needs to be backed by dmabuf file-descriptors ideally. This might prove cubersome given that we use vimc for test purposes. Hence, pass actual frame data buffers into the IPA instead, which would result in exercising these code paths for now. Introduce plumbing support for mapping and un-mapping of these buffers into the IPA. Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> --- include/libcamera/ipa/vimc.mojom | 3 +++ src/ipa/vimc/vimc.cpp | 26 ++++++++++++++++++++++++++ src/libcamera/pipeline/vimc/vimc.cpp | 25 ++++++++++++++++++++++++- 3 files changed, 53 insertions(+), 1 deletion(-)