| Message ID | 20210813144437.138005-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 13, 2021 at 08:14:36PM +0530, Umang Jain wrote: > VIMC pipeline-handler have dmabuf-backed mock FrameBuffers which are s/pipeline-handler/pipeline handler/ > specifically targetted mimicking IPA buffers(parameter and statistics). s/buffers/buffers / > Map these mock buffers to the VIMC IPA that would enable exercising IPA > IPC code paths. This will provide leverage to our test suite to test > IPA IPC code paths, which are common to various platforms. > > 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 | 16 ++++++++++++++++ > 3 files changed, 45 insertions(+) > > diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom > index ee66353d..8cb240d3 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 0535c740..082b2db7 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/mapped_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::IPAOperationCode 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, MappedFrameBuffer::MapFlag::Read)); The point of emplace compared to insert is to avoid a copy construction. This should be written as buffers_.emplace(std::piecewise_construct, std::forward_as_tuple(buffer.id), std::forward_as_tuple(&fb, MappedFrameBuffer::MapFlag::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 edf8c58e..c44a6242 100644 > --- a/src/libcamera/pipeline/vimc/vimc.cpp > +++ b/src/libcamera/pipeline/vimc/vimc.cpp > @@ -63,6 +63,7 @@ public: > > std::unique_ptr<ipa::vimc::IPAProxyVimc> ipa_; > std::vector<std::unique_ptr<FrameBuffer>> mockIPABufs_; > + std::vector<IPABuffer> ipaBuffers_; > }; > > class VimcCameraConfiguration : public CameraConfiguration > @@ -334,6 +335,15 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlLis > if (ret < 0) > return ret; > > + /* Map the mock IPA buffers to VIMC IPA to exercise IPC code paths */ > + unsigned int ipaBufferId = 1; > + for (unsigned int i = 0; i < data->mockIPABufs_.size(); i++) { > + std::unique_ptr<FrameBuffer> &buffer = data->mockIPABufs_[i]; > + buffer->setCookie(ipaBufferId++); You can use utils::enumerate() here: for (auto [i, buffer] : utils::enumerate(data->mackIPABufs_)) { buffer->setCookie(i++); > + data->ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes()); > + } > + data->ipa_->mapBuffers(data->ipaBuffers_); The return value should be checked. > + > ret = data->ipa_->start(); > if (ret) { > data->video_->releaseBuffers(); > @@ -354,7 +364,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); You could use here for (const std::unique_ptr<FrameBuffer> &buffer : data->mockIPABufs_) ids.push_back(buffer->cookie()); and replace VimcCameraData::ipaBuffers_ with a local variable in PipelineHandlerVimc::start(). > + data->ipa_->unmapBuffers(ids); > data->ipa_->stop(); > + > data->video_->releaseBuffers(); > } >
Hi Laurent, Thanks for review, one comment below On 8/14/21 6:05 AM, Laurent Pinchart wrote: > Hi Umang, > > Thank you for the patch. > > On Fri, Aug 13, 2021 at 08:14:36PM +0530, Umang Jain wrote: >> VIMC pipeline-handler have dmabuf-backed mock FrameBuffers which are > s/pipeline-handler/pipeline handler/ > >> specifically targetted mimicking IPA buffers(parameter and statistics). > s/buffers/buffers / > >> Map these mock buffers to the VIMC IPA that would enable exercising IPA >> IPC code paths. This will provide leverage to our test suite to test >> IPA IPC code paths, which are common to various platforms. >> >> 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 | 16 ++++++++++++++++ >> 3 files changed, 45 insertions(+) >> >> diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom >> index ee66353d..8cb240d3 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 0535c740..082b2db7 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/mapped_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::IPAOperationCode 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, MappedFrameBuffer::MapFlag::Read)); > The point of emplace compared to insert is to avoid a copy construction. > This should be written as > > buffers_.emplace(std::piecewise_construct, > std::forward_as_tuple(buffer.id), > std::forward_as_tuple(&fb, MappedFrameBuffer::MapFlag::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 edf8c58e..c44a6242 100644 >> --- a/src/libcamera/pipeline/vimc/vimc.cpp >> +++ b/src/libcamera/pipeline/vimc/vimc.cpp >> @@ -63,6 +63,7 @@ public: >> >> std::unique_ptr<ipa::vimc::IPAProxyVimc> ipa_; >> std::vector<std::unique_ptr<FrameBuffer>> mockIPABufs_; >> + std::vector<IPABuffer> ipaBuffers_; >> }; >> >> class VimcCameraConfiguration : public CameraConfiguration >> @@ -334,6 +335,15 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlLis >> if (ret < 0) >> return ret; >> >> + /* Map the mock IPA buffers to VIMC IPA to exercise IPC code paths */ >> + unsigned int ipaBufferId = 1; >> + for (unsigned int i = 0; i < data->mockIPABufs_.size(); i++) { >> + std::unique_ptr<FrameBuffer> &buffer = data->mockIPABufs_[i]; >> + buffer->setCookie(ipaBufferId++); > You can use utils::enumerate() here: > > for (auto [i, buffer] : utils::enumerate(data->mackIPABufs_)) { > buffer->setCookie(i++); > >> + data->ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes()); >> + } >> + data->ipa_->mapBuffers(data->ipaBuffers_); > The return value should be checked. I didn't find mapBuffers/unmapBuffers() to return a value, in any of the mojom interfaces we have today. I think mapBuffers() should have return value indeed, can I skip it here for now, and create a patch on top which touches all the concerned IPAs as well? > >> + >> ret = data->ipa_->start(); >> if (ret) { >> data->video_->releaseBuffers(); >> @@ -354,7 +364,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); > You could use here > > for (const std::unique_ptr<FrameBuffer> &buffer : data->mockIPABufs_) > ids.push_back(buffer->cookie()); > > and replace VimcCameraData::ipaBuffers_ with a local variable in > PipelineHandlerVimc::start(). > >> + data->ipa_->unmapBuffers(ids); >> data->ipa_->stop(); >> + >> data->video_->releaseBuffers(); >> } >>
Hi Umang, On Sat, Aug 14, 2021 at 10:26:45AM +0530, Umang Jain wrote: > On 8/14/21 6:05 AM, Laurent Pinchart wrote: > > On Fri, Aug 13, 2021 at 08:14:36PM +0530, Umang Jain wrote: > >> VIMC pipeline-handler have dmabuf-backed mock FrameBuffers which are > > > > s/pipeline-handler/pipeline handler/ > > > >> specifically targetted mimicking IPA buffers(parameter and statistics). > > > > s/buffers/buffers / > > > >> Map these mock buffers to the VIMC IPA that would enable exercising IPA > >> IPC code paths. This will provide leverage to our test suite to test > >> IPA IPC code paths, which are common to various platforms. > >> > >> 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 | 16 ++++++++++++++++ > >> 3 files changed, 45 insertions(+) > >> > >> diff --git a/include/libcamera/ipa/vimc.mojom b/include/libcamera/ipa/vimc.mojom > >> index ee66353d..8cb240d3 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 0535c740..082b2db7 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/mapped_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::IPAOperationCode 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, MappedFrameBuffer::MapFlag::Read)); > > > > The point of emplace compared to insert is to avoid a copy construction. > > This should be written as > > > > buffers_.emplace(std::piecewise_construct, > > std::forward_as_tuple(buffer.id), > > std::forward_as_tuple(&fb, MappedFrameBuffer::MapFlag::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 edf8c58e..c44a6242 100644 > >> --- a/src/libcamera/pipeline/vimc/vimc.cpp > >> +++ b/src/libcamera/pipeline/vimc/vimc.cpp > >> @@ -63,6 +63,7 @@ public: > >> > >> std::unique_ptr<ipa::vimc::IPAProxyVimc> ipa_; > >> std::vector<std::unique_ptr<FrameBuffer>> mockIPABufs_; > >> + std::vector<IPABuffer> ipaBuffers_; > >> }; > >> > >> class VimcCameraConfiguration : public CameraConfiguration > >> @@ -334,6 +335,15 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlLis > >> if (ret < 0) > >> return ret; > >> > >> + /* Map the mock IPA buffers to VIMC IPA to exercise IPC code paths */ > >> + unsigned int ipaBufferId = 1; > >> + for (unsigned int i = 0; i < data->mockIPABufs_.size(); i++) { > >> + std::unique_ptr<FrameBuffer> &buffer = data->mockIPABufs_[i]; > >> + buffer->setCookie(ipaBufferId++); > > > > You can use utils::enumerate() here: > > > > for (auto [i, buffer] : utils::enumerate(data->mackIPABufs_)) { > > buffer->setCookie(i++); > > > >> + data->ipaBuffers_.emplace_back(buffer->cookie(), buffer->planes()); > >> + } > >> + data->ipa_->mapBuffers(data->ipaBuffers_); > > > > The return value should be checked. > > I didn't find mapBuffers/unmapBuffers() to return a value, in any of the > mojom interfaces we have today. > > I think mapBuffers() should have return value indeed, can I skip it here > for now, and create a patch on top which touches all the concerned IPAs > as well? Sure. > >> + > >> ret = data->ipa_->start(); > >> if (ret) { > >> data->video_->releaseBuffers(); > >> @@ -354,7 +364,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); > > > > You could use here > > > > for (const std::unique_ptr<FrameBuffer> &buffer : data->mockIPABufs_) > > ids.push_back(buffer->cookie()); > > > > and replace VimcCameraData::ipaBuffers_ with a local variable in > > PipelineHandlerVimc::start(). > > > >> + 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 ee66353d..8cb240d3 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 0535c740..082b2db7 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/mapped_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::IPAOperationCode 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, MappedFrameBuffer::MapFlag::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 edf8c58e..c44a6242 100644 --- a/src/libcamera/pipeline/vimc/vimc.cpp +++ b/src/libcamera/pipeline/vimc/vimc.cpp @@ -63,6 +63,7 @@ public: std::unique_ptr<ipa::vimc::IPAProxyVimc> ipa_; std::vector<std::unique_ptr<FrameBuffer>> mockIPABufs_; + std::vector<IPABuffer> ipaBuffers_; }; class VimcCameraConfiguration : public CameraConfiguration @@ -334,6 +335,15 @@ int PipelineHandlerVimc::start(Camera *camera, [[maybe_unused]] const ControlLis if (ret < 0) return ret; + /* Map the mock IPA buffers to VIMC IPA to exercise IPC code paths */ + unsigned int ipaBufferId = 1; + for (unsigned int i = 0; i < data->mockIPABufs_.size(); i++) { + std::unique_ptr<FrameBuffer> &buffer = data->mockIPABufs_[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(); @@ -354,7 +364,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(); }
VIMC pipeline-handler have dmabuf-backed mock FrameBuffers which are specifically targetted mimicking IPA buffers(parameter and statistics). Map these mock buffers to the VIMC IPA that would enable exercising IPA IPC code paths. This will provide leverage to our test suite to test IPA IPC code paths, which are common to various platforms. 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 | 16 ++++++++++++++++ 3 files changed, 45 insertions(+)