| Message ID | 20200512003903.20986-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Naush, And on this too :-) On Tue, May 12, 2020 at 03:39:03AM +0300, Laurent Pinchart wrote: > The StaggeredCtrl class, part of the Raspberry Pi pipeline handler, is > part of libcamera. Move it to the libcamera namespace to simplify usage > of libcamera APIs. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 2 +- > .../pipeline/raspberrypi/staggered_ctrl.cpp | 25 ++++++++++--------- > .../pipeline/raspberrypi/staggered_ctrl.h | 13 ++++++---- > 3 files changed, 22 insertions(+), 18 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 21a1d7f7cca3..41d1a522fa71 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -330,7 +330,7 @@ public: > std::vector<IPABuffer> ipaBuffers_; > > /* VCSM allocation helper. */ > - RPi::Vcsm vcsm_; > + ::RPi::Vcsm vcsm_; > void *lsTable_; > > RPi::StaggeredCtrl staggeredCtrl_; > diff --git a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp > index fbd87d3e791e..d431887ea137 100644 > --- a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp > +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp > @@ -9,20 +9,19 @@ > > #include <algorithm> > > +#include <libcamera/controls.h> > + > #include "log.h" > #include "utils.h" > +#include "v4l2_videodevice.h" > > -/* For logging... */ > -using libcamera::LogCategory; > -using libcamera::LogDebug; > -using libcamera::LogInfo; > -using libcamera::utils::hex; > +namespace libcamera { > > LOG_DEFINE_CATEGORY(RPI_S_W); > > namespace RPi { > > -void StaggeredCtrl::init(libcamera::V4L2VideoDevice *dev, > +void StaggeredCtrl::init(V4L2VideoDevice *dev, > std::initializer_list<std::pair<const uint32_t, uint8_t>> delayList) > { > std::lock_guard<std::mutex> lock(lock_); > @@ -35,7 +34,7 @@ void StaggeredCtrl::init(libcamera::V4L2VideoDevice *dev, > maxDelay_ = 0; > for (auto const &p : delay_) { > LOG(RPI_S_W, Info) << "Init ctrl " > - << hex(p.first) << " with delay " > + << utils::hex(p.first) << " with delay " > << static_cast<int>(p.second); > maxDelay_ = std::max(maxDelay_, p.second); > } > @@ -92,7 +91,7 @@ bool StaggeredCtrl::set(std::initializer_list<std::pair<const uint32_t, int32_t> > return true; > } > > -bool StaggeredCtrl::set(libcamera::ControlList &controls) > +bool StaggeredCtrl::set(ControlList &controls) > { > std::lock_guard<std::mutex> lock(lock_); > > @@ -103,7 +102,7 @@ bool StaggeredCtrl::set(libcamera::ControlList &controls) > > ctrl_[p.first][setCount_] = CtrlInfo(p.second.get<int32_t>()); > LOG(RPI_S_W, Debug) << "Setting ctrl " > - << hex(p.first) << " to " > + << utils::hex(p.first) << " to " > << ctrl_[p.first][setCount_].value > << " at index " > << setCount_; > @@ -115,7 +114,7 @@ bool StaggeredCtrl::set(libcamera::ControlList &controls) > int StaggeredCtrl::write() > { > std::lock_guard<std::mutex> lock(lock_); > - libcamera::ControlList controls(dev_->controls()); > + ControlList controls(dev_->controls()); > > for (auto &p : ctrl_) { > int delayDiff = maxDelay_ - delay_[p.first]; > @@ -126,7 +125,7 @@ int StaggeredCtrl::write() > controls.set(p.first, p.second[index].value); > p.second[index].updated = false; > LOG(RPI_S_W, Debug) << "Writing ctrl " > - << hex(p.first) << " to " > + << utils::hex(p.first) << " to " > << p.second[index].value > << " at index " > << index; > @@ -149,7 +148,7 @@ void StaggeredCtrl::get(std::unordered_map<uint32_t, int32_t> &ctrl, uint8_t off > int index = std::max<int>(0, getCount_ - maxDelay_); > ctrl[p.first] = p.second[index].value; > LOG(RPI_S_W, Debug) << "Getting ctrl " > - << hex(p.first) << " to " > + << utils::hex(p.first) << " to " > << p.second[index].value > << " at index " > << index; > @@ -171,3 +170,5 @@ void StaggeredCtrl::nextFrame() > } > > } /* namespace RPi */ > + > +} /* namespace libcamera */ > diff --git a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h > index c8f000a0b43c..eef16eaac235 100644 > --- a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h > +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h > @@ -12,9 +12,10 @@ > #include <unordered_map> > #include <utility> > > -#include <libcamera/controls.h> > +namespace libcamera { > > -#include "v4l2_videodevice.h" > +class ControlList; > +class V4L2VideoDevice; > > namespace RPi { > > @@ -31,7 +32,7 @@ public: > return init_; > } > > - void init(libcamera::V4L2VideoDevice *dev, > + void init(V4L2VideoDevice *dev, > std::initializer_list<std::pair<const uint32_t, uint8_t>> delayList); > void reset(); > > @@ -39,7 +40,7 @@ public: > > bool set(uint32_t ctrl, int32_t value); > bool set(std::initializer_list<std::pair<const uint32_t, int32_t>> ctrlList); > - bool set(libcamera::ControlList &controls); > + bool set(ControlList &controls); > > int write(); > > @@ -81,10 +82,12 @@ private: > uint32_t setCount_; > uint32_t getCount_; > uint8_t maxDelay_; > - libcamera::V4L2VideoDevice *dev_; > + V4L2VideoDevice *dev_; > std::unordered_map<uint32_t, uint8_t> delay_; > std::unordered_map<uint32_t, CircularArray> ctrl_; > std::mutex lock_; > }; > > } /* namespace RPi */ > + > +} /* namespace libcamera */
Hi Laurent, On Tue, 12 May 2020 at 01:39, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > The StaggeredCtrl class, part of the Raspberry Pi pipeline handler, is > part of libcamera. Move it to the libcamera namespace to simplify usage > of libcamera APIs. > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > .../pipeline/raspberrypi/raspberrypi.cpp | 2 +- > .../pipeline/raspberrypi/staggered_ctrl.cpp | 25 ++++++++++--------- > .../pipeline/raspberrypi/staggered_ctrl.h | 13 ++++++---- > 3 files changed, 22 insertions(+), 18 deletions(-) > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > index 21a1d7f7cca3..41d1a522fa71 100644 > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > @@ -330,7 +330,7 @@ public: > std::vector<IPABuffer> ipaBuffers_; > > /* VCSM allocation helper. */ > - RPi::Vcsm vcsm_; > + ::RPi::Vcsm vcsm_; I haven't tried it myself, but is this change of scope necessary? Naush > void *lsTable_; > > RPi::StaggeredCtrl staggeredCtrl_; > diff --git a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp > index fbd87d3e791e..d431887ea137 100644 > --- a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp > +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp > @@ -9,20 +9,19 @@ > > #include <algorithm> > > +#include <libcamera/controls.h> > + > #include "log.h" > #include "utils.h" > +#include "v4l2_videodevice.h" > > -/* For logging... */ > -using libcamera::LogCategory; > -using libcamera::LogDebug; > -using libcamera::LogInfo; > -using libcamera::utils::hex; > +namespace libcamera { > > LOG_DEFINE_CATEGORY(RPI_S_W); > > namespace RPi { > > -void StaggeredCtrl::init(libcamera::V4L2VideoDevice *dev, > +void StaggeredCtrl::init(V4L2VideoDevice *dev, > std::initializer_list<std::pair<const uint32_t, uint8_t>> delayList) > { > std::lock_guard<std::mutex> lock(lock_); > @@ -35,7 +34,7 @@ void StaggeredCtrl::init(libcamera::V4L2VideoDevice *dev, > maxDelay_ = 0; > for (auto const &p : delay_) { > LOG(RPI_S_W, Info) << "Init ctrl " > - << hex(p.first) << " with delay " > + << utils::hex(p.first) << " with delay " > << static_cast<int>(p.second); > maxDelay_ = std::max(maxDelay_, p.second); > } > @@ -92,7 +91,7 @@ bool StaggeredCtrl::set(std::initializer_list<std::pair<const uint32_t, int32_t> > return true; > } > > -bool StaggeredCtrl::set(libcamera::ControlList &controls) > +bool StaggeredCtrl::set(ControlList &controls) > { > std::lock_guard<std::mutex> lock(lock_); > > @@ -103,7 +102,7 @@ bool StaggeredCtrl::set(libcamera::ControlList &controls) > > ctrl_[p.first][setCount_] = CtrlInfo(p.second.get<int32_t>()); > LOG(RPI_S_W, Debug) << "Setting ctrl " > - << hex(p.first) << " to " > + << utils::hex(p.first) << " to " > << ctrl_[p.first][setCount_].value > << " at index " > << setCount_; > @@ -115,7 +114,7 @@ bool StaggeredCtrl::set(libcamera::ControlList &controls) > int StaggeredCtrl::write() > { > std::lock_guard<std::mutex> lock(lock_); > - libcamera::ControlList controls(dev_->controls()); > + ControlList controls(dev_->controls()); > > for (auto &p : ctrl_) { > int delayDiff = maxDelay_ - delay_[p.first]; > @@ -126,7 +125,7 @@ int StaggeredCtrl::write() > controls.set(p.first, p.second[index].value); > p.second[index].updated = false; > LOG(RPI_S_W, Debug) << "Writing ctrl " > - << hex(p.first) << " to " > + << utils::hex(p.first) << " to " > << p.second[index].value > << " at index " > << index; > @@ -149,7 +148,7 @@ void StaggeredCtrl::get(std::unordered_map<uint32_t, int32_t> &ctrl, uint8_t off > int index = std::max<int>(0, getCount_ - maxDelay_); > ctrl[p.first] = p.second[index].value; > LOG(RPI_S_W, Debug) << "Getting ctrl " > - << hex(p.first) << " to " > + << utils::hex(p.first) << " to " > << p.second[index].value > << " at index " > << index; > @@ -171,3 +170,5 @@ void StaggeredCtrl::nextFrame() > } > > } /* namespace RPi */ > + > +} /* namespace libcamera */ > diff --git a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h > index c8f000a0b43c..eef16eaac235 100644 > --- a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h > +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h > @@ -12,9 +12,10 @@ > #include <unordered_map> > #include <utility> > > -#include <libcamera/controls.h> > +namespace libcamera { > > -#include "v4l2_videodevice.h" > +class ControlList; > +class V4L2VideoDevice; > > namespace RPi { > > @@ -31,7 +32,7 @@ public: > return init_; > } > > - void init(libcamera::V4L2VideoDevice *dev, > + void init(V4L2VideoDevice *dev, > std::initializer_list<std::pair<const uint32_t, uint8_t>> delayList); > void reset(); > > @@ -39,7 +40,7 @@ public: > > bool set(uint32_t ctrl, int32_t value); > bool set(std::initializer_list<std::pair<const uint32_t, int32_t>> ctrlList); > - bool set(libcamera::ControlList &controls); > + bool set(ControlList &controls); > > int write(); > > @@ -81,10 +82,12 @@ private: > uint32_t setCount_; > uint32_t getCount_; > uint8_t maxDelay_; > - libcamera::V4L2VideoDevice *dev_; > + V4L2VideoDevice *dev_; > std::unordered_map<uint32_t, uint8_t> delay_; > std::unordered_map<uint32_t, CircularArray> ctrl_; > std::mutex lock_; > }; > > } /* namespace RPi */ > + > +} /* namespace libcamera */ > -- > Regards, > > Laurent Pinchart >
Hi Naush, On Thu, May 14, 2020 at 03:58:14PM +0100, Naushir Patuck wrote: > On Tue, 12 May 2020 at 01:39, Laurent Pinchart wrote: > > > > The StaggeredCtrl class, part of the Raspberry Pi pipeline handler, is > > part of libcamera. Move it to the libcamera namespace to simplify usage > > of libcamera APIs. > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > --- > > .../pipeline/raspberrypi/raspberrypi.cpp | 2 +- > > .../pipeline/raspberrypi/staggered_ctrl.cpp | 25 ++++++++++--------- > > .../pipeline/raspberrypi/staggered_ctrl.h | 13 ++++++---- > > 3 files changed, 22 insertions(+), 18 deletions(-) > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > index 21a1d7f7cca3..41d1a522fa71 100644 > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > @@ -330,7 +330,7 @@ public: > > std::vector<IPABuffer> ipaBuffers_; > > > > /* VCSM allocation helper. */ > > - RPi::Vcsm vcsm_; > > + ::RPi::Vcsm vcsm_; > > I haven't tried it myself, but is this change of scope necessary? I would have asked the same had I received such a patch :-) It is necessary, as the compiler complains otherwise, but I haven't investigated why. I was considering moving RPi::Vcsm to the libcamera namespace too, but given that the goal is to replace Vcsm with dmabuf, I ended up taking the easy path. > > void *lsTable_; > > > > RPi::StaggeredCtrl staggeredCtrl_; > > diff --git a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp > > index fbd87d3e791e..d431887ea137 100644 > > --- a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp > > +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp > > @@ -9,20 +9,19 @@ > > > > #include <algorithm> > > > > +#include <libcamera/controls.h> > > + > > #include "log.h" > > #include "utils.h" > > +#include "v4l2_videodevice.h" > > > > -/* For logging... */ > > -using libcamera::LogCategory; > > -using libcamera::LogDebug; > > -using libcamera::LogInfo; > > -using libcamera::utils::hex; > > +namespace libcamera { > > > > LOG_DEFINE_CATEGORY(RPI_S_W); > > > > namespace RPi { > > > > -void StaggeredCtrl::init(libcamera::V4L2VideoDevice *dev, > > +void StaggeredCtrl::init(V4L2VideoDevice *dev, > > std::initializer_list<std::pair<const uint32_t, uint8_t>> delayList) > > { > > std::lock_guard<std::mutex> lock(lock_); > > @@ -35,7 +34,7 @@ void StaggeredCtrl::init(libcamera::V4L2VideoDevice *dev, > > maxDelay_ = 0; > > for (auto const &p : delay_) { > > LOG(RPI_S_W, Info) << "Init ctrl " > > - << hex(p.first) << " with delay " > > + << utils::hex(p.first) << " with delay " > > << static_cast<int>(p.second); > > maxDelay_ = std::max(maxDelay_, p.second); > > } > > @@ -92,7 +91,7 @@ bool StaggeredCtrl::set(std::initializer_list<std::pair<const uint32_t, int32_t> > > return true; > > } > > > > -bool StaggeredCtrl::set(libcamera::ControlList &controls) > > +bool StaggeredCtrl::set(ControlList &controls) > > { > > std::lock_guard<std::mutex> lock(lock_); > > > > @@ -103,7 +102,7 @@ bool StaggeredCtrl::set(libcamera::ControlList &controls) > > > > ctrl_[p.first][setCount_] = CtrlInfo(p.second.get<int32_t>()); > > LOG(RPI_S_W, Debug) << "Setting ctrl " > > - << hex(p.first) << " to " > > + << utils::hex(p.first) << " to " > > << ctrl_[p.first][setCount_].value > > << " at index " > > << setCount_; > > @@ -115,7 +114,7 @@ bool StaggeredCtrl::set(libcamera::ControlList &controls) > > int StaggeredCtrl::write() > > { > > std::lock_guard<std::mutex> lock(lock_); > > - libcamera::ControlList controls(dev_->controls()); > > + ControlList controls(dev_->controls()); > > > > for (auto &p : ctrl_) { > > int delayDiff = maxDelay_ - delay_[p.first]; > > @@ -126,7 +125,7 @@ int StaggeredCtrl::write() > > controls.set(p.first, p.second[index].value); > > p.second[index].updated = false; > > LOG(RPI_S_W, Debug) << "Writing ctrl " > > - << hex(p.first) << " to " > > + << utils::hex(p.first) << " to " > > << p.second[index].value > > << " at index " > > << index; > > @@ -149,7 +148,7 @@ void StaggeredCtrl::get(std::unordered_map<uint32_t, int32_t> &ctrl, uint8_t off > > int index = std::max<int>(0, getCount_ - maxDelay_); > > ctrl[p.first] = p.second[index].value; > > LOG(RPI_S_W, Debug) << "Getting ctrl " > > - << hex(p.first) << " to " > > + << utils::hex(p.first) << " to " > > << p.second[index].value > > << " at index " > > << index; > > @@ -171,3 +170,5 @@ void StaggeredCtrl::nextFrame() > > } > > > > } /* namespace RPi */ > > + > > +} /* namespace libcamera */ > > diff --git a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h > > index c8f000a0b43c..eef16eaac235 100644 > > --- a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h > > +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h > > @@ -12,9 +12,10 @@ > > #include <unordered_map> > > #include <utility> > > > > -#include <libcamera/controls.h> > > +namespace libcamera { > > > > -#include "v4l2_videodevice.h" > > +class ControlList; > > +class V4L2VideoDevice; > > > > namespace RPi { > > > > @@ -31,7 +32,7 @@ public: > > return init_; > > } > > > > - void init(libcamera::V4L2VideoDevice *dev, > > + void init(V4L2VideoDevice *dev, > > std::initializer_list<std::pair<const uint32_t, uint8_t>> delayList); > > void reset(); > > > > @@ -39,7 +40,7 @@ public: > > > > bool set(uint32_t ctrl, int32_t value); > > bool set(std::initializer_list<std::pair<const uint32_t, int32_t>> ctrlList); > > - bool set(libcamera::ControlList &controls); > > + bool set(ControlList &controls); > > > > int write(); > > > > @@ -81,10 +82,12 @@ private: > > uint32_t setCount_; > > uint32_t getCount_; > > uint8_t maxDelay_; > > - libcamera::V4L2VideoDevice *dev_; > > + V4L2VideoDevice *dev_; > > std::unordered_map<uint32_t, uint8_t> delay_; > > std::unordered_map<uint32_t, CircularArray> ctrl_; > > std::mutex lock_; > > }; > > > > } /* namespace RPi */ > > + > > +} /* namespace libcamera */
Hi Naush, On Thu, May 14, 2020 at 06:01:11PM +0300, Laurent Pinchart wrote: > On Thu, May 14, 2020 at 03:58:14PM +0100, Naushir Patuck wrote: > > On Tue, 12 May 2020 at 01:39, Laurent Pinchart wrote: > > > > > > The StaggeredCtrl class, part of the Raspberry Pi pipeline handler, is > > > part of libcamera. Move it to the libcamera namespace to simplify usage > > > of libcamera APIs. > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > --- > > > .../pipeline/raspberrypi/raspberrypi.cpp | 2 +- > > > .../pipeline/raspberrypi/staggered_ctrl.cpp | 25 ++++++++++--------- > > > .../pipeline/raspberrypi/staggered_ctrl.h | 13 ++++++---- > > > 3 files changed, 22 insertions(+), 18 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > index 21a1d7f7cca3..41d1a522fa71 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > @@ -330,7 +330,7 @@ public: > > > std::vector<IPABuffer> ipaBuffers_; > > > > > > /* VCSM allocation helper. */ > > > - RPi::Vcsm vcsm_; > > > + ::RPi::Vcsm vcsm_; > > > > I haven't tried it myself, but is this change of scope necessary? > > I would have asked the same had I received such a patch :-) It is > necessary, as the compiler complains otherwise, but I haven't > investigated why. I was considering moving RPi::Vcsm to the libcamera > namespace too, but given that the goal is to replace Vcsm with dmabuf, I > ended up taking the easy path. For reference: [4/70] Compiling C++ object 'src/libcamera/4ab8042@@camera@sha/pipeline_raspberrypi_raspberrypi.cpp.o'. FAILED: src/libcamera/4ab8042@@camera@sha/pipeline_raspberrypi_raspberrypi.cpp.o clang++-8 -Isrc/libcamera/4ab8042@@camera@sha -Isrc/libcamera -I../../src/libcamera -Iinclude -I../../include -Isrc/libcamera/include -I../../src/libcamera/include -I../../include/android/hardware/libhardware/include/ -I../../include/android/metadata/ -I../../include/android/system/core/include -Iinclude/libcamera -Xclang -fcolor-diagnostics -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++14 -g -stdlib=libc++ -Wno-unused-parameter -include config.h -fPIC -pthread -MD -MQ 'src/libcamera/4ab8042@@camera@sha/pipeline_raspberrypi_raspberrypi.cpp.o' -MF 'src/libcamera/4ab8042@@camera@sha/pipeline_raspberrypi_raspberrypi.cpp.o.d' -o 'src/libcamera/4ab8042@@camera@sha/pipeline_raspberrypi_raspberrypi.cpp.o' -c ../../src/libcamera/pipeline/raspberrypi/raspberrypi.cpp ../../src/libcamera/pipeline/raspberrypi/raspberrypi.cpp:333:2: error: no type named 'Vcsm' in namespace 'libcamera::RPi'; did you mean '::RPi::Vcsm'? RPi::Vcsm vcsm_; ^~~~~~~~~ ::RPi::Vcsm ../../src/libcamera/pipeline/raspberrypi/vcsm.h:22:7: note: '::RPi::Vcsm' declared here class Vcsm ^ 1 error generated. ninja: build stopped: subcommand failed. There's a similar error with gcc 8.3.0. Is the patch otherwise OK ? > > > void *lsTable_; > > > > > > RPi::StaggeredCtrl staggeredCtrl_; > > > diff --git a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp > > > index fbd87d3e791e..d431887ea137 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp > > > +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp > > > @@ -9,20 +9,19 @@ > > > > > > #include <algorithm> > > > > > > +#include <libcamera/controls.h> > > > + > > > #include "log.h" > > > #include "utils.h" > > > +#include "v4l2_videodevice.h" > > > > > > -/* For logging... */ > > > -using libcamera::LogCategory; > > > -using libcamera::LogDebug; > > > -using libcamera::LogInfo; > > > -using libcamera::utils::hex; > > > +namespace libcamera { > > > > > > LOG_DEFINE_CATEGORY(RPI_S_W); > > > > > > namespace RPi { > > > > > > -void StaggeredCtrl::init(libcamera::V4L2VideoDevice *dev, > > > +void StaggeredCtrl::init(V4L2VideoDevice *dev, > > > std::initializer_list<std::pair<const uint32_t, uint8_t>> delayList) > > > { > > > std::lock_guard<std::mutex> lock(lock_); > > > @@ -35,7 +34,7 @@ void StaggeredCtrl::init(libcamera::V4L2VideoDevice *dev, > > > maxDelay_ = 0; > > > for (auto const &p : delay_) { > > > LOG(RPI_S_W, Info) << "Init ctrl " > > > - << hex(p.first) << " with delay " > > > + << utils::hex(p.first) << " with delay " > > > << static_cast<int>(p.second); > > > maxDelay_ = std::max(maxDelay_, p.second); > > > } > > > @@ -92,7 +91,7 @@ bool StaggeredCtrl::set(std::initializer_list<std::pair<const uint32_t, int32_t> > > > return true; > > > } > > > > > > -bool StaggeredCtrl::set(libcamera::ControlList &controls) > > > +bool StaggeredCtrl::set(ControlList &controls) > > > { > > > std::lock_guard<std::mutex> lock(lock_); > > > > > > @@ -103,7 +102,7 @@ bool StaggeredCtrl::set(libcamera::ControlList &controls) > > > > > > ctrl_[p.first][setCount_] = CtrlInfo(p.second.get<int32_t>()); > > > LOG(RPI_S_W, Debug) << "Setting ctrl " > > > - << hex(p.first) << " to " > > > + << utils::hex(p.first) << " to " > > > << ctrl_[p.first][setCount_].value > > > << " at index " > > > << setCount_; > > > @@ -115,7 +114,7 @@ bool StaggeredCtrl::set(libcamera::ControlList &controls) > > > int StaggeredCtrl::write() > > > { > > > std::lock_guard<std::mutex> lock(lock_); > > > - libcamera::ControlList controls(dev_->controls()); > > > + ControlList controls(dev_->controls()); > > > > > > for (auto &p : ctrl_) { > > > int delayDiff = maxDelay_ - delay_[p.first]; > > > @@ -126,7 +125,7 @@ int StaggeredCtrl::write() > > > controls.set(p.first, p.second[index].value); > > > p.second[index].updated = false; > > > LOG(RPI_S_W, Debug) << "Writing ctrl " > > > - << hex(p.first) << " to " > > > + << utils::hex(p.first) << " to " > > > << p.second[index].value > > > << " at index " > > > << index; > > > @@ -149,7 +148,7 @@ void StaggeredCtrl::get(std::unordered_map<uint32_t, int32_t> &ctrl, uint8_t off > > > int index = std::max<int>(0, getCount_ - maxDelay_); > > > ctrl[p.first] = p.second[index].value; > > > LOG(RPI_S_W, Debug) << "Getting ctrl " > > > - << hex(p.first) << " to " > > > + << utils::hex(p.first) << " to " > > > << p.second[index].value > > > << " at index " > > > << index; > > > @@ -171,3 +170,5 @@ void StaggeredCtrl::nextFrame() > > > } > > > > > > } /* namespace RPi */ > > > + > > > +} /* namespace libcamera */ > > > diff --git a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h > > > index c8f000a0b43c..eef16eaac235 100644 > > > --- a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h > > > +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h > > > @@ -12,9 +12,10 @@ > > > #include <unordered_map> > > > #include <utility> > > > > > > -#include <libcamera/controls.h> > > > +namespace libcamera { > > > > > > -#include "v4l2_videodevice.h" > > > +class ControlList; > > > +class V4L2VideoDevice; > > > > > > namespace RPi { > > > > > > @@ -31,7 +32,7 @@ public: > > > return init_; > > > } > > > > > > - void init(libcamera::V4L2VideoDevice *dev, > > > + void init(V4L2VideoDevice *dev, > > > std::initializer_list<std::pair<const uint32_t, uint8_t>> delayList); > > > void reset(); > > > > > > @@ -39,7 +40,7 @@ public: > > > > > > bool set(uint32_t ctrl, int32_t value); > > > bool set(std::initializer_list<std::pair<const uint32_t, int32_t>> ctrlList); > > > - bool set(libcamera::ControlList &controls); > > > + bool set(ControlList &controls); > > > > > > int write(); > > > > > > @@ -81,10 +82,12 @@ private: > > > uint32_t setCount_; > > > uint32_t getCount_; > > > uint8_t maxDelay_; > > > - libcamera::V4L2VideoDevice *dev_; > > > + V4L2VideoDevice *dev_; > > > std::unordered_map<uint32_t, uint8_t> delay_; > > > std::unordered_map<uint32_t, CircularArray> ctrl_; > > > std::mutex lock_; > > > }; > > > > > > } /* namespace RPi */ > > > + > > > +} /* namespace libcamera */
Hi Laurent, On Fri, 15 May 2020 at 02:43, Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Naush, > > On Thu, May 14, 2020 at 06:01:11PM +0300, Laurent Pinchart wrote: > > On Thu, May 14, 2020 at 03:58:14PM +0100, Naushir Patuck wrote: > > > On Tue, 12 May 2020 at 01:39, Laurent Pinchart wrote: > > > > > > > > The StaggeredCtrl class, part of the Raspberry Pi pipeline handler, is > > > > part of libcamera. Move it to the libcamera namespace to simplify usage > > > > of libcamera APIs. > > > > > > > > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > --- > > > > .../pipeline/raspberrypi/raspberrypi.cpp | 2 +- > > > > .../pipeline/raspberrypi/staggered_ctrl.cpp | 25 ++++++++++--------- > > > > .../pipeline/raspberrypi/staggered_ctrl.h | 13 ++++++---- > > > > 3 files changed, 22 insertions(+), 18 deletions(-) > > > > > > > > diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > index 21a1d7f7cca3..41d1a522fa71 100644 > > > > --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > > > > @@ -330,7 +330,7 @@ public: > > > > std::vector<IPABuffer> ipaBuffers_; > > > > > > > > /* VCSM allocation helper. */ > > > > - RPi::Vcsm vcsm_; > > > > + ::RPi::Vcsm vcsm_; > > > > > > I haven't tried it myself, but is this change of scope necessary? > > > > I would have asked the same had I received such a patch :-) It is > > necessary, as the compiler complains otherwise, but I haven't > > investigated why. I was considering moving RPi::Vcsm to the libcamera > > namespace too, but given that the goal is to replace Vcsm with dmabuf, I > > ended up taking the easy path. > > For reference: > > [4/70] Compiling C++ object 'src/libcamera/4ab8042@@camera@sha/pipeline_raspberrypi_raspberrypi.cpp.o'. > FAILED: src/libcamera/4ab8042@@camera@sha/pipeline_raspberrypi_raspberrypi.cpp.o > clang++-8 -Isrc/libcamera/4ab8042@@camera@sha -Isrc/libcamera -I../../src/libcamera -Iinclude -I../../include -Isrc/libcamera/include -I../../src/libcamera/include -I../../include/android/hardware/libhardware/include/ -I../../include/android/metadata/ -I../../include/android/system/core/include -Iinclude/libcamera -Xclang -fcolor-diagnostics -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -Wextra -Werror -std=c++14 -g -stdlib=libc++ -Wno-unused-parameter -include config.h -fPIC -pthread -MD -MQ 'src/libcamera/4ab8042@@camera@sha/pipeline_raspberrypi_raspberrypi.cpp.o' -MF 'src/libcamera/4ab8042@@camera@sha/pipeline_raspberrypi_raspberrypi.cpp.o.d' -o 'src/libcamera/4ab8042@@camera@sha/pipeline_raspberrypi_raspberrypi.cpp.o' -c ../../src/libcamera/pipeline/raspberrypi/raspberrypi.cpp > ../../src/libcamera/pipeline/raspberrypi/raspberrypi.cpp:333:2: error: no type named 'Vcsm' in namespace 'libcamera::RPi'; did you mean '::RPi::Vcsm'? > RPi::Vcsm vcsm_; > ^~~~~~~~~ > ::RPi::Vcsm > ../../src/libcamera/pipeline/raspberrypi/vcsm.h:22:7: note: '::RPi::Vcsm' declared here > class Vcsm > ^ > 1 error generated. > ninja: build stopped: subcommand failed. > > There's a similar error with gcc 8.3.0. > > Is the patch otherwise OK ? > Yes, looks to be correct - but I have no idea what change could have triggered this error now :) With the dmabuf change, we can probably enclose the class in namespace libcamera and namespace RPi to avoid this problem. Reviewed-by: Naushir Patuck <naush@raspberrypi.com> > > > > void *lsTable_; > > > > > > > > RPi::StaggeredCtrl staggeredCtrl_; > > > > diff --git a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp > > > > index fbd87d3e791e..d431887ea137 100644 > > > > --- a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp > > > > +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp > > > > @@ -9,20 +9,19 @@ > > > > > > > > #include <algorithm> > > > > > > > > +#include <libcamera/controls.h> > > > > + > > > > #include "log.h" > > > > #include "utils.h" > > > > +#include "v4l2_videodevice.h" > > > > > > > > -/* For logging... */ > > > > -using libcamera::LogCategory; > > > > -using libcamera::LogDebug; > > > > -using libcamera::LogInfo; > > > > -using libcamera::utils::hex; > > > > +namespace libcamera { > > > > > > > > LOG_DEFINE_CATEGORY(RPI_S_W); > > > > > > > > namespace RPi { > > > > > > > > -void StaggeredCtrl::init(libcamera::V4L2VideoDevice *dev, > > > > +void StaggeredCtrl::init(V4L2VideoDevice *dev, > > > > std::initializer_list<std::pair<const uint32_t, uint8_t>> delayList) > > > > { > > > > std::lock_guard<std::mutex> lock(lock_); > > > > @@ -35,7 +34,7 @@ void StaggeredCtrl::init(libcamera::V4L2VideoDevice *dev, > > > > maxDelay_ = 0; > > > > for (auto const &p : delay_) { > > > > LOG(RPI_S_W, Info) << "Init ctrl " > > > > - << hex(p.first) << " with delay " > > > > + << utils::hex(p.first) << " with delay " > > > > << static_cast<int>(p.second); > > > > maxDelay_ = std::max(maxDelay_, p.second); > > > > } > > > > @@ -92,7 +91,7 @@ bool StaggeredCtrl::set(std::initializer_list<std::pair<const uint32_t, int32_t> > > > > return true; > > > > } > > > > > > > > -bool StaggeredCtrl::set(libcamera::ControlList &controls) > > > > +bool StaggeredCtrl::set(ControlList &controls) > > > > { > > > > std::lock_guard<std::mutex> lock(lock_); > > > > > > > > @@ -103,7 +102,7 @@ bool StaggeredCtrl::set(libcamera::ControlList &controls) > > > > > > > > ctrl_[p.first][setCount_] = CtrlInfo(p.second.get<int32_t>()); > > > > LOG(RPI_S_W, Debug) << "Setting ctrl " > > > > - << hex(p.first) << " to " > > > > + << utils::hex(p.first) << " to " > > > > << ctrl_[p.first][setCount_].value > > > > << " at index " > > > > << setCount_; > > > > @@ -115,7 +114,7 @@ bool StaggeredCtrl::set(libcamera::ControlList &controls) > > > > int StaggeredCtrl::write() > > > > { > > > > std::lock_guard<std::mutex> lock(lock_); > > > > - libcamera::ControlList controls(dev_->controls()); > > > > + ControlList controls(dev_->controls()); > > > > > > > > for (auto &p : ctrl_) { > > > > int delayDiff = maxDelay_ - delay_[p.first]; > > > > @@ -126,7 +125,7 @@ int StaggeredCtrl::write() > > > > controls.set(p.first, p.second[index].value); > > > > p.second[index].updated = false; > > > > LOG(RPI_S_W, Debug) << "Writing ctrl " > > > > - << hex(p.first) << " to " > > > > + << utils::hex(p.first) << " to " > > > > << p.second[index].value > > > > << " at index " > > > > << index; > > > > @@ -149,7 +148,7 @@ void StaggeredCtrl::get(std::unordered_map<uint32_t, int32_t> &ctrl, uint8_t off > > > > int index = std::max<int>(0, getCount_ - maxDelay_); > > > > ctrl[p.first] = p.second[index].value; > > > > LOG(RPI_S_W, Debug) << "Getting ctrl " > > > > - << hex(p.first) << " to " > > > > + << utils::hex(p.first) << " to " > > > > << p.second[index].value > > > > << " at index " > > > > << index; > > > > @@ -171,3 +170,5 @@ void StaggeredCtrl::nextFrame() > > > > } > > > > > > > > } /* namespace RPi */ > > > > + > > > > +} /* namespace libcamera */ > > > > diff --git a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h > > > > index c8f000a0b43c..eef16eaac235 100644 > > > > --- a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h > > > > +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h > > > > @@ -12,9 +12,10 @@ > > > > #include <unordered_map> > > > > #include <utility> > > > > > > > > -#include <libcamera/controls.h> > > > > +namespace libcamera { > > > > > > > > -#include "v4l2_videodevice.h" > > > > +class ControlList; > > > > +class V4L2VideoDevice; > > > > > > > > namespace RPi { > > > > > > > > @@ -31,7 +32,7 @@ public: > > > > return init_; > > > > } > > > > > > > > - void init(libcamera::V4L2VideoDevice *dev, > > > > + void init(V4L2VideoDevice *dev, > > > > std::initializer_list<std::pair<const uint32_t, uint8_t>> delayList); > > > > void reset(); > > > > > > > > @@ -39,7 +40,7 @@ public: > > > > > > > > bool set(uint32_t ctrl, int32_t value); > > > > bool set(std::initializer_list<std::pair<const uint32_t, int32_t>> ctrlList); > > > > - bool set(libcamera::ControlList &controls); > > > > + bool set(ControlList &controls); > > > > > > > > int write(); > > > > > > > > @@ -81,10 +82,12 @@ private: > > > > uint32_t setCount_; > > > > uint32_t getCount_; > > > > uint8_t maxDelay_; > > > > - libcamera::V4L2VideoDevice *dev_; > > > > + V4L2VideoDevice *dev_; > > > > std::unordered_map<uint32_t, uint8_t> delay_; > > > > std::unordered_map<uint32_t, CircularArray> ctrl_; > > > > std::mutex lock_; > > > > }; > > > > > > > > } /* namespace RPi */ > > > > + > > > > +} /* namespace libcamera */ > > -- > Regards, > > Laurent Pinchart
diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 21a1d7f7cca3..41d1a522fa71 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -330,7 +330,7 @@ public: std::vector<IPABuffer> ipaBuffers_; /* VCSM allocation helper. */ - RPi::Vcsm vcsm_; + ::RPi::Vcsm vcsm_; void *lsTable_; RPi::StaggeredCtrl staggeredCtrl_; diff --git a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp index fbd87d3e791e..d431887ea137 100644 --- a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.cpp @@ -9,20 +9,19 @@ #include <algorithm> +#include <libcamera/controls.h> + #include "log.h" #include "utils.h" +#include "v4l2_videodevice.h" -/* For logging... */ -using libcamera::LogCategory; -using libcamera::LogDebug; -using libcamera::LogInfo; -using libcamera::utils::hex; +namespace libcamera { LOG_DEFINE_CATEGORY(RPI_S_W); namespace RPi { -void StaggeredCtrl::init(libcamera::V4L2VideoDevice *dev, +void StaggeredCtrl::init(V4L2VideoDevice *dev, std::initializer_list<std::pair<const uint32_t, uint8_t>> delayList) { std::lock_guard<std::mutex> lock(lock_); @@ -35,7 +34,7 @@ void StaggeredCtrl::init(libcamera::V4L2VideoDevice *dev, maxDelay_ = 0; for (auto const &p : delay_) { LOG(RPI_S_W, Info) << "Init ctrl " - << hex(p.first) << " with delay " + << utils::hex(p.first) << " with delay " << static_cast<int>(p.second); maxDelay_ = std::max(maxDelay_, p.second); } @@ -92,7 +91,7 @@ bool StaggeredCtrl::set(std::initializer_list<std::pair<const uint32_t, int32_t> return true; } -bool StaggeredCtrl::set(libcamera::ControlList &controls) +bool StaggeredCtrl::set(ControlList &controls) { std::lock_guard<std::mutex> lock(lock_); @@ -103,7 +102,7 @@ bool StaggeredCtrl::set(libcamera::ControlList &controls) ctrl_[p.first][setCount_] = CtrlInfo(p.second.get<int32_t>()); LOG(RPI_S_W, Debug) << "Setting ctrl " - << hex(p.first) << " to " + << utils::hex(p.first) << " to " << ctrl_[p.first][setCount_].value << " at index " << setCount_; @@ -115,7 +114,7 @@ bool StaggeredCtrl::set(libcamera::ControlList &controls) int StaggeredCtrl::write() { std::lock_guard<std::mutex> lock(lock_); - libcamera::ControlList controls(dev_->controls()); + ControlList controls(dev_->controls()); for (auto &p : ctrl_) { int delayDiff = maxDelay_ - delay_[p.first]; @@ -126,7 +125,7 @@ int StaggeredCtrl::write() controls.set(p.first, p.second[index].value); p.second[index].updated = false; LOG(RPI_S_W, Debug) << "Writing ctrl " - << hex(p.first) << " to " + << utils::hex(p.first) << " to " << p.second[index].value << " at index " << index; @@ -149,7 +148,7 @@ void StaggeredCtrl::get(std::unordered_map<uint32_t, int32_t> &ctrl, uint8_t off int index = std::max<int>(0, getCount_ - maxDelay_); ctrl[p.first] = p.second[index].value; LOG(RPI_S_W, Debug) << "Getting ctrl " - << hex(p.first) << " to " + << utils::hex(p.first) << " to " << p.second[index].value << " at index " << index; @@ -171,3 +170,5 @@ void StaggeredCtrl::nextFrame() } } /* namespace RPi */ + +} /* namespace libcamera */ diff --git a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h index c8f000a0b43c..eef16eaac235 100644 --- a/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h +++ b/src/libcamera/pipeline/raspberrypi/staggered_ctrl.h @@ -12,9 +12,10 @@ #include <unordered_map> #include <utility> -#include <libcamera/controls.h> +namespace libcamera { -#include "v4l2_videodevice.h" +class ControlList; +class V4L2VideoDevice; namespace RPi { @@ -31,7 +32,7 @@ public: return init_; } - void init(libcamera::V4L2VideoDevice *dev, + void init(V4L2VideoDevice *dev, std::initializer_list<std::pair<const uint32_t, uint8_t>> delayList); void reset(); @@ -39,7 +40,7 @@ public: bool set(uint32_t ctrl, int32_t value); bool set(std::initializer_list<std::pair<const uint32_t, int32_t>> ctrlList); - bool set(libcamera::ControlList &controls); + bool set(ControlList &controls); int write(); @@ -81,10 +82,12 @@ private: uint32_t setCount_; uint32_t getCount_; uint8_t maxDelay_; - libcamera::V4L2VideoDevice *dev_; + V4L2VideoDevice *dev_; std::unordered_map<uint32_t, uint8_t> delay_; std::unordered_map<uint32_t, CircularArray> ctrl_; std::mutex lock_; }; } /* namespace RPi */ + +} /* namespace libcamera */
The StaggeredCtrl class, part of the Raspberry Pi pipeline handler, is part of libcamera. Move it to the libcamera namespace to simplify usage of libcamera APIs. Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- .../pipeline/raspberrypi/raspberrypi.cpp | 2 +- .../pipeline/raspberrypi/staggered_ctrl.cpp | 25 ++++++++++--------- .../pipeline/raspberrypi/staggered_ctrl.h | 13 ++++++---- 3 files changed, 22 insertions(+), 18 deletions(-)