Message ID | 20200623020930.1781469-7-niklas.soderlund@ragnatech.se |
---|---|
State | Superseded |
Headers | show |
Series |
|
Related | show |
Hi Niklas, On Tue, Jun 23, 2020 at 04:09:26AM +0200, Niklas Söderlund wrote: > Instead of spreading the mapping between media bus codes and V4L2 FourCC > all over the CIO2 code collect it in a single map and extract the data > from it. This is done in preparation of adding PixelFormat information > to the mix. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/ipu3/cio2.cpp | 50 +++++++++++++--------------- > 1 file changed, 24 insertions(+), 26 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > index dbbcf79c7b2ed5c1..f23128d412e6b1a5 100644 > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > @@ -18,6 +18,17 @@ namespace libcamera { > > LOG_DECLARE_CATEGORY(IPU3) > > +namespace { > + > +static const std::map<uint32_t, V4L2PixelFormat> mbusCodesToInfo = { > + { MEDIA_BUS_FMT_SBGGR10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) }, > + { MEDIA_BUS_FMT_SGBRG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) }, > + { MEDIA_BUS_FMT_SGRBG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) }, > + { MEDIA_BUS_FMT_SRGGB10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) }, > +}; > + > +} /* namespace */ > + Ok, I see where this is going with the next patch, but I'm not super excited by having to iterate the map just to extract keys (and anyway, why doesn't std::map have a keys() function ??). > CIO2Device::CIO2Device() > : output_(nullptr), csi2_(nullptr), sensor_(nullptr) > { > @@ -78,10 +89,10 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > * Make sure the sensor produces at least one format compatible with > * the CIO2 requirements. > */ > - const std::set<unsigned int> cio2Codes{ MEDIA_BUS_FMT_SBGGR10_1X10, > - MEDIA_BUS_FMT_SGRBG10_1X10, > - MEDIA_BUS_FMT_SGBRG10_1X10, > - MEDIA_BUS_FMT_SRGGB10_1X10 }; > + std::set<unsigned int> cio2Codes; why a set ? > + std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(), > + std::inserter(cio2Codes, cio2Codes.begin()), > + [](const std::map<uint32_t, V4L2PixelFormat>::value_type &pair){ return pair.first; }); you could [](decltype(mbusCodeToInfo)::value_type &pair){ return pair.first; }); or since C++14 [](auto &pair){ return pair.first; }); > const std::set<unsigned int> &sensorCodes = sensor_->mbusCodes(); > if (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(), > cio2Codes.begin(), cio2Codes.end())) { > @@ -125,11 +136,12 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat) > * Apply the selected format to the sensor, the CSI-2 receiver and > * the CIO2 output device. > */ > - sensorFormat = sensor_->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10, > - MEDIA_BUS_FMT_SGBRG10_1X10, > - MEDIA_BUS_FMT_SGRBG10_1X10, > - MEDIA_BUS_FMT_SRGGB10_1X10 }, > - size); > + std::vector<unsigned int> mbusCodes; You could reserve space in the vector knowing the map size > + std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(), > + std::back_inserter(mbusCodes), > + [](const std::map<uint32_t, V4L2PixelFormat>::value_type &pair){ return pair.first; }); > + > + sensorFormat = sensor_->getFormat(mbusCodes, size); > ret = sensor_->setFormat(&sensorFormat); > if (ret) > return ret; > @@ -138,25 +150,11 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat) > if (ret) > return ret; > > - V4L2PixelFormat v4l2Format; > - switch (sensorFormat.mbus_code) { > - case MEDIA_BUS_FMT_SBGGR10_1X10: > - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10); > - break; > - case MEDIA_BUS_FMT_SGBRG10_1X10: > - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10); > - break; > - case MEDIA_BUS_FMT_SGRBG10_1X10: > - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10); > - break; > - case MEDIA_BUS_FMT_SRGGB10_1X10: > - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10); > - break; > - default: > + const auto &itInfo = mbusCodesToInfo.find(sensorFormat.mbus_code); > + if (itInfo == mbusCodesToInfo.end()) > return -EINVAL; > - } > > - outputFormat->fourcc = v4l2Format; > + outputFormat->fourcc = itInfo->second; I would be tempted to suggest you to add helper functions around that map, not sure it's worth the effort, but those transform to just retreive keys are not nice imho. That apart Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > outputFormat->size = sensorFormat.size; > outputFormat->planesCount = 1; > > -- > 2.27.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Jacopo and Niklas, On Wed, Jun 24, 2020 at 12:01:14PM +0200, Jacopo Mondi wrote: > On Tue, Jun 23, 2020 at 04:09:26AM +0200, Niklas Söderlund wrote: > > Instead of spreading the mapping between media bus codes and V4L2 FourCC > > all over the CIO2 code collect it in a single map and extract the data > > from it. This is done in preparation of adding PixelFormat information > > to the mix. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/libcamera/pipeline/ipu3/cio2.cpp | 50 +++++++++++++--------------- > > 1 file changed, 24 insertions(+), 26 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > > index dbbcf79c7b2ed5c1..f23128d412e6b1a5 100644 > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > > @@ -18,6 +18,17 @@ namespace libcamera { > > > > LOG_DECLARE_CATEGORY(IPU3) > > > > +namespace { > > + > > +static const std::map<uint32_t, V4L2PixelFormat> mbusCodesToInfo = { > > + { MEDIA_BUS_FMT_SBGGR10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) }, > > + { MEDIA_BUS_FMT_SGBRG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) }, > > + { MEDIA_BUS_FMT_SGRBG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) }, > > + { MEDIA_BUS_FMT_SRGGB10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) }, > > +}; > > + > > +} /* namespace */ > > + > > Ok, I see where this is going with the next patch, but I'm not super > excited by having to iterate the map just to extract keys (and anyway, > why doesn't std::map have a keys() function ??). Possibly because its implementation would be O(n), and STL generally tries to avoid hiding expensive operations behind apparently simple functions. Just a speculation. > > CIO2Device::CIO2Device() > > : output_(nullptr), csi2_(nullptr), sensor_(nullptr) > > { > > @@ -78,10 +89,10 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > > * Make sure the sensor produces at least one format compatible with > > * the CIO2 requirements. > > */ > > - const std::set<unsigned int> cio2Codes{ MEDIA_BUS_FMT_SBGGR10_1X10, > > - MEDIA_BUS_FMT_SGRBG10_1X10, > > - MEDIA_BUS_FMT_SGBRG10_1X10, > > - MEDIA_BUS_FMT_SRGGB10_1X10 }; > > + std::set<unsigned int> cio2Codes; > > why a set ? To keep this efficient, given that std::map is sorted by key, this could be a vector, and we could call cio2Codes.reserve(mbusCodesToInfo.size()); before the std::transform(). I think a set is indeed overkill. > > + std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(), > > + std::inserter(cio2Codes, cio2Codes.begin()), > > + [](const std::map<uint32_t, V4L2PixelFormat>::value_type &pair){ return pair.first; }); > > you could > [](decltype(mbusCodeToInfo)::value_type &pair){ return pair.first; }); > or since C++14 > [](auto &pair){ return pair.first; }); Much nicer indeed. > > const std::set<unsigned int> &sensorCodes = sensor_->mbusCodes(); > > if (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(), > > cio2Codes.begin(), cio2Codes.end())) { > > @@ -125,11 +136,12 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat) > > * Apply the selected format to the sensor, the CSI-2 receiver and > > * the CIO2 output device. > > */ > > - sensorFormat = sensor_->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10, > > - MEDIA_BUS_FMT_SGBRG10_1X10, > > - MEDIA_BUS_FMT_SGRBG10_1X10, > > - MEDIA_BUS_FMT_SRGGB10_1X10 }, > > - size); > > + std::vector<unsigned int> mbusCodes; > > You could reserve space in the vector knowing the map size Seems we agree :-) > > + std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(), > > + std::back_inserter(mbusCodes), > > + [](const std::map<uint32_t, V4L2PixelFormat>::value_type &pair){ return pair.first; }); > > + > > + sensorFormat = sensor_->getFormat(mbusCodes, size); > > ret = sensor_->setFormat(&sensorFormat); > > if (ret) > > return ret; > > @@ -138,25 +150,11 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat) > > if (ret) > > return ret; > > > > - V4L2PixelFormat v4l2Format; > > - switch (sensorFormat.mbus_code) { > > - case MEDIA_BUS_FMT_SBGGR10_1X10: > > - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10); > > - break; > > - case MEDIA_BUS_FMT_SGBRG10_1X10: > > - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10); > > - break; > > - case MEDIA_BUS_FMT_SGRBG10_1X10: > > - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10); > > - break; > > - case MEDIA_BUS_FMT_SRGGB10_1X10: > > - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10); > > - break; > > - default: > > + const auto &itInfo = mbusCodesToInfo.find(sensorFormat.mbus_code); > > + if (itInfo == mbusCodesToInfo.end()) > > return -EINVAL; > > - } > > > > - outputFormat->fourcc = v4l2Format; > > + outputFormat->fourcc = itInfo->second; > > I would be tempted to suggest you to add helper functions around that > map, not sure it's worth the effort, but those transform to just > retreive keys are not nice imho. I was about to mention that, thinking that the vector of keys could then be cached, but given that the code is only used in init paths, it may not be worth it. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > That apart > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > outputFormat->size = sensorFormat.size; > > outputFormat->planesCount = 1; > >
Hi Laurent and Jacopo, On 2020-06-25 02:57:22 +0300, Laurent Pinchart wrote: > Hi Jacopo and Niklas, > > On Wed, Jun 24, 2020 at 12:01:14PM +0200, Jacopo Mondi wrote: > > On Tue, Jun 23, 2020 at 04:09:26AM +0200, Niklas Söderlund wrote: > > > Instead of spreading the mapping between media bus codes and V4L2 FourCC > > > all over the CIO2 code collect it in a single map and extract the data > > > from it. This is done in preparation of adding PixelFormat information > > > to the mix. > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > --- > > > src/libcamera/pipeline/ipu3/cio2.cpp | 50 +++++++++++++--------------- > > > 1 file changed, 24 insertions(+), 26 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > > > index dbbcf79c7b2ed5c1..f23128d412e6b1a5 100644 > > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > > > @@ -18,6 +18,17 @@ namespace libcamera { > > > > > > LOG_DECLARE_CATEGORY(IPU3) > > > > > > +namespace { > > > + > > > +static const std::map<uint32_t, V4L2PixelFormat> mbusCodesToInfo = { > > > + { MEDIA_BUS_FMT_SBGGR10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) }, > > > + { MEDIA_BUS_FMT_SGBRG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) }, > > > + { MEDIA_BUS_FMT_SGRBG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) }, > > > + { MEDIA_BUS_FMT_SRGGB10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) }, > > > +}; > > > + > > > +} /* namespace */ > > > + > > > > Ok, I see where this is going with the next patch, but I'm not super > > excited by having to iterate the map just to extract keys (and anyway, > > why doesn't std::map have a keys() function ??). > > Possibly because its implementation would be O(n), and STL generally > tries to avoid hiding expensive operations behind apparently simple > functions. Just a speculation. > > > > CIO2Device::CIO2Device() > > > : output_(nullptr), csi2_(nullptr), sensor_(nullptr) > > > { > > > @@ -78,10 +89,10 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > > > * Make sure the sensor produces at least one format compatible with > > > * the CIO2 requirements. > > > */ > > > - const std::set<unsigned int> cio2Codes{ MEDIA_BUS_FMT_SBGGR10_1X10, > > > - MEDIA_BUS_FMT_SGRBG10_1X10, > > > - MEDIA_BUS_FMT_SGBRG10_1X10, > > > - MEDIA_BUS_FMT_SRGGB10_1X10 }; > > > + std::set<unsigned int> cio2Codes; > > > > why a set ? > > To keep this efficient, given that std::map is sorted by key, this could > be a vector, and we could call > > cio2Codes.reserve(mbusCodesToInfo.size()); We would then also need to remember to sort the vector as that is a requirement for utils::set_overlap() where this is used. I think it's much nicer to use a set from the start to make sure nothing goes wrong. I'm feeling a bit of resistance against set in this series. Is there a downside to using it I don't see? I think they are neat as a specialization of vector where we get guarantees there is only one occurrence of each value and the readout/sorting order will be the same no mater where the container where created. Am I missing something? > > before the std::transform(). I think a set is indeed overkill. > > > > + std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(), > > > + std::inserter(cio2Codes, cio2Codes.begin()), > > > + [](const std::map<uint32_t, V4L2PixelFormat>::value_type &pair){ return pair.first; }); > > > > you could > > [](decltype(mbusCodeToInfo)::value_type &pair){ return pair.first; }); > > or since C++14 > > [](auto &pair){ return pair.first; }); > > Much nicer indeed. Neat. > > > > const std::set<unsigned int> &sensorCodes = sensor_->mbusCodes(); > > > if (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(), > > > cio2Codes.begin(), cio2Codes.end())) { > > > @@ -125,11 +136,12 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat) > > > * Apply the selected format to the sensor, the CSI-2 receiver and > > > * the CIO2 output device. > > > */ > > > - sensorFormat = sensor_->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10, > > > - MEDIA_BUS_FMT_SGBRG10_1X10, > > > - MEDIA_BUS_FMT_SGRBG10_1X10, > > > - MEDIA_BUS_FMT_SRGGB10_1X10 }, > > > - size); > > > + std::vector<unsigned int> mbusCodes; > > > > You could reserve space in the vector knowing the map size > > Seems we agree :-) > > > > + std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(), > > > + std::back_inserter(mbusCodes), > > > + [](const std::map<uint32_t, V4L2PixelFormat>::value_type &pair){ return pair.first; }); > > > + > > > + sensorFormat = sensor_->getFormat(mbusCodes, size); > > > ret = sensor_->setFormat(&sensorFormat); > > > if (ret) > > > return ret; > > > @@ -138,25 +150,11 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat) > > > if (ret) > > > return ret; > > > > > > - V4L2PixelFormat v4l2Format; > > > - switch (sensorFormat.mbus_code) { > > > - case MEDIA_BUS_FMT_SBGGR10_1X10: > > > - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10); > > > - break; > > > - case MEDIA_BUS_FMT_SGBRG10_1X10: > > > - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10); > > > - break; > > > - case MEDIA_BUS_FMT_SGRBG10_1X10: > > > - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10); > > > - break; > > > - case MEDIA_BUS_FMT_SRGGB10_1X10: > > > - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10); > > > - break; > > > - default: > > > + const auto &itInfo = mbusCodesToInfo.find(sensorFormat.mbus_code); > > > + if (itInfo == mbusCodesToInfo.end()) > > > return -EINVAL; > > > - } > > > > > > - outputFormat->fourcc = v4l2Format; > > > + outputFormat->fourcc = itInfo->second; > > > > I would be tempted to suggest you to add helper functions around that > > map, not sure it's worth the effort, but those transform to just > > retreive keys are not nice imho. > > I was about to mention that, thinking that the vector of keys could then > be cached, but given that the code is only used in init paths, it may > not be worth it. I see two options for this problem, the one used in this patch or creating multiple data structures in the anonyms namespace, one for each way to lookup something. The later consumes less CPU but consumes more memory, I'm not sure which on is the best option here. I went with this option as it prevents possible differences between the multiple data structures for lookup. I'm open to switching to that option if that is the consensus. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > That apart > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > outputFormat->size = sensorFormat.size; > > > outputFormat->planesCount = 1; > > > > > -- > Regards, > > Laurent Pinchart
Hi Niklas, On Thu, Jun 25, 2020 at 02:12:59AM +0200, Niklas Söderlund wrote: > On 2020-06-25 02:57:22 +0300, Laurent Pinchart wrote: > > On Wed, Jun 24, 2020 at 12:01:14PM +0200, Jacopo Mondi wrote: > > > On Tue, Jun 23, 2020 at 04:09:26AM +0200, Niklas Söderlund wrote: > > > > Instead of spreading the mapping between media bus codes and V4L2 FourCC > > > > all over the CIO2 code collect it in a single map and extract the data > > > > from it. This is done in preparation of adding PixelFormat information > > > > to the mix. > > > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > --- > > > > src/libcamera/pipeline/ipu3/cio2.cpp | 50 +++++++++++++--------------- > > > > 1 file changed, 24 insertions(+), 26 deletions(-) > > > > > > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > > > > index dbbcf79c7b2ed5c1..f23128d412e6b1a5 100644 > > > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > > > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > > > > @@ -18,6 +18,17 @@ namespace libcamera { > > > > > > > > LOG_DECLARE_CATEGORY(IPU3) > > > > > > > > +namespace { > > > > + > > > > +static const std::map<uint32_t, V4L2PixelFormat> mbusCodesToInfo = { > > > > + { MEDIA_BUS_FMT_SBGGR10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) }, > > > > + { MEDIA_BUS_FMT_SGBRG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) }, > > > > + { MEDIA_BUS_FMT_SGRBG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) }, > > > > + { MEDIA_BUS_FMT_SRGGB10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) }, > > > > +}; > > > > + > > > > +} /* namespace */ > > > > + > > > > > > Ok, I see where this is going with the next patch, but I'm not super > > > excited by having to iterate the map just to extract keys (and anyway, > > > why doesn't std::map have a keys() function ??). > > > > Possibly because its implementation would be O(n), and STL generally > > tries to avoid hiding expensive operations behind apparently simple > > functions. Just a speculation. > > > > > > CIO2Device::CIO2Device() > > > > : output_(nullptr), csi2_(nullptr), sensor_(nullptr) > > > > { > > > > @@ -78,10 +89,10 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > > > > * Make sure the sensor produces at least one format compatible with > > > > * the CIO2 requirements. > > > > */ > > > > - const std::set<unsigned int> cio2Codes{ MEDIA_BUS_FMT_SBGGR10_1X10, > > > > - MEDIA_BUS_FMT_SGRBG10_1X10, > > > > - MEDIA_BUS_FMT_SGBRG10_1X10, > > > > - MEDIA_BUS_FMT_SRGGB10_1X10 }; > > > > + std::set<unsigned int> cio2Codes; > > > > > > why a set ? > > > > To keep this efficient, given that std::map is sorted by key, this could > > be a vector, and we could call > > > > cio2Codes.reserve(mbusCodesToInfo.size()); > > We would then also need to remember to sort the vector as that is a > requirement for utils::set_overlap() where this is used. I think it's > much nicer to use a set from the start to make sure nothing goes wrong. I agree std::set has that benefit. Note that here we wouldn't need to sort the vector explicitly. I thus don't really see a reason to use a set in this specific case. However, maybe adding /* * utils::set_overlap() requires the containers to be sorted, * this is guaranteed for cio2Codes as it gets generated from a * sorted map with a loop that preserves the order. */ could avoid surprises in the future. > I'm feeling a bit of resistance against set in this series. Is there a > downside to using it I don't see? I think they are neat as a > specialization of vector where we get guarantees there is only one > occurrence of each value and the readout/sorting order will be the same > no mater where the container where created. Am I missing something? std::set is (usually) implemented as a red-black tree. That will consume more memory than a vector, and for small sets, will likely be slower than creating a vector and sorting it (if needed). I'm not necessarily opposed to usage of std::set in non-hot paths, but as noted in another reply in this series, we would need to make that consistent then, passing a set to CameraSensor::getFormats(), and possibly more functions. I fear std::set would quickly spread through lots of places. > > before the std::transform(). I think a set is indeed overkill. > > > > > > + std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(), > > > > + std::inserter(cio2Codes, cio2Codes.begin()), btw shouldn't this be end() and not begin() ? > > > > + [](const std::map<uint32_t, V4L2PixelFormat>::value_type &pair){ return pair.first; }); > > > > > > you could > > > [](decltype(mbusCodeToInfo)::value_type &pair){ return pair.first; }); > > > or since C++14 > > > [](auto &pair){ return pair.first; }); > > > > Much nicer indeed. > > Neat. > > > > > const std::set<unsigned int> &sensorCodes = sensor_->mbusCodes(); > > > > if (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(), > > > > cio2Codes.begin(), cio2Codes.end())) { > > > > @@ -125,11 +136,12 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat) > > > > * Apply the selected format to the sensor, the CSI-2 receiver and > > > > * the CIO2 output device. > > > > */ > > > > - sensorFormat = sensor_->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10, > > > > - MEDIA_BUS_FMT_SGBRG10_1X10, > > > > - MEDIA_BUS_FMT_SGRBG10_1X10, > > > > - MEDIA_BUS_FMT_SRGGB10_1X10 }, > > > > - size); > > > > + std::vector<unsigned int> mbusCodes; > > > > > > You could reserve space in the vector knowing the map size > > > > Seems we agree :-) > > > > > > + std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(), > > > > + std::back_inserter(mbusCodes), > > > > + [](const std::map<uint32_t, V4L2PixelFormat>::value_type &pair){ return pair.first; }); > > > > + > > > > + sensorFormat = sensor_->getFormat(mbusCodes, size); > > > > ret = sensor_->setFormat(&sensorFormat); > > > > if (ret) > > > > return ret; > > > > @@ -138,25 +150,11 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat) > > > > if (ret) > > > > return ret; > > > > > > > > - V4L2PixelFormat v4l2Format; > > > > - switch (sensorFormat.mbus_code) { > > > > - case MEDIA_BUS_FMT_SBGGR10_1X10: > > > > - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10); > > > > - break; > > > > - case MEDIA_BUS_FMT_SGBRG10_1X10: > > > > - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10); > > > > - break; > > > > - case MEDIA_BUS_FMT_SGRBG10_1X10: > > > > - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10); > > > > - break; > > > > - case MEDIA_BUS_FMT_SRGGB10_1X10: > > > > - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10); > > > > - break; > > > > - default: > > > > + const auto &itInfo = mbusCodesToInfo.find(sensorFormat.mbus_code); > > > > + if (itInfo == mbusCodesToInfo.end()) > > > > return -EINVAL; > > > > - } > > > > > > > > - outputFormat->fourcc = v4l2Format; > > > > + outputFormat->fourcc = itInfo->second; > > > > > > I would be tempted to suggest you to add helper functions around that > > > map, not sure it's worth the effort, but those transform to just > > > retreive keys are not nice imho. > > > > I was about to mention that, thinking that the vector of keys could then > > be cached, but given that the code is only used in init paths, it may > > not be worth it. > > I see two options for this problem, the one used in this patch or > creating multiple data structures in the anonyms namespace, one for each > way to lookup something. The later consumes less CPU but consumes more > memory, I'm not sure which on is the best option here. I went with this > option as it prevents possible differences between the multiple data > structures for lookup. I'm open to switching to that option if that is > the consensus. If it was used in more places I'd cache the keys the first time they're used, but it's not worth it here. Same for reverse lookups, I'd create and cache a reverse map on the first call to the helper, but only if it really makes a difference. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > That apart > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > > outputFormat->size = sensorFormat.size; > > > > outputFormat->planesCount = 1;
Hello, On Thu, Jun 25, 2020 at 04:23:46AM +0300, Laurent Pinchart wrote: > Hi Niklas, > > On Thu, Jun 25, 2020 at 02:12:59AM +0200, Niklas Söderlund wrote: > > On 2020-06-25 02:57:22 +0300, Laurent Pinchart wrote: > > > On Wed, Jun 24, 2020 at 12:01:14PM +0200, Jacopo Mondi wrote: > > > > On Tue, Jun 23, 2020 at 04:09:26AM +0200, Niklas Söderlund wrote: > > > > > Instead of spreading the mapping between media bus codes and V4L2 FourCC > > > > > all over the CIO2 code collect it in a single map and extract the data > > > > > from it. This is done in preparation of adding PixelFormat information > > > > > to the mix. > > > > > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > > --- > > > > > src/libcamera/pipeline/ipu3/cio2.cpp | 50 +++++++++++++--------------- > > > > > 1 file changed, 24 insertions(+), 26 deletions(-) > > > > > > > > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > > > > > index dbbcf79c7b2ed5c1..f23128d412e6b1a5 100644 > > > > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > > > > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > > > > > @@ -18,6 +18,17 @@ namespace libcamera { > > > > > > > > > > LOG_DECLARE_CATEGORY(IPU3) > > > > > > > > > > +namespace { > > > > > + > > > > > +static const std::map<uint32_t, V4L2PixelFormat> mbusCodesToInfo = { > > > > > + { MEDIA_BUS_FMT_SBGGR10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) }, > > > > > + { MEDIA_BUS_FMT_SGBRG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) }, > > > > > + { MEDIA_BUS_FMT_SGRBG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) }, > > > > > + { MEDIA_BUS_FMT_SRGGB10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) }, > > > > > +}; > > > > > + > > > > > +} /* namespace */ > > > > > + > > > > > > > > Ok, I see where this is going with the next patch, but I'm not super > > > > excited by having to iterate the map just to extract keys (and anyway, > > > > why doesn't std::map have a keys() function ??). > > > > > > Possibly because its implementation would be O(n), and STL generally > > > tries to avoid hiding expensive operations behind apparently simple > > > functions. Just a speculation. > > > Could be! > > > > > CIO2Device::CIO2Device() > > > > > : output_(nullptr), csi2_(nullptr), sensor_(nullptr) > > > > > { > > > > > @@ -78,10 +89,10 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > > > > > * Make sure the sensor produces at least one format compatible with > > > > > * the CIO2 requirements. > > > > > */ > > > > > - const std::set<unsigned int> cio2Codes{ MEDIA_BUS_FMT_SBGGR10_1X10, > > > > > - MEDIA_BUS_FMT_SGRBG10_1X10, > > > > > - MEDIA_BUS_FMT_SGBRG10_1X10, > > > > > - MEDIA_BUS_FMT_SRGGB10_1X10 }; > > > > > + std::set<unsigned int> cio2Codes; > > > > > > > > why a set ? > > > > > > To keep this efficient, given that std::map is sorted by key, this could > > > be a vector, and we could call > > > > > > cio2Codes.reserve(mbusCodesToInfo.size()); > > > > We would then also need to remember to sort the vector as that is a > > requirement for utils::set_overlap() where this is used. I think it's > > much nicer to use a set from the start to make sure nothing goes wrong. > Sorry but, we're extracting keys from a sorted map with unique entries. The resulting vector of keys will be sorted as well, isn't it ? And the requirement of being sorted depends on what you will use the vector for anway, it's not something an "extract keys" function should care about (and again, if' I'm not mistaken, the vector of keys will be anyhow sorted and unique, being the result of iterating an std::map) > I agree std::set has that benefit. Note that here we wouldn't need to > sort the vector explicitly. I thus don't really see a reason to use a > set in this specific case. However, maybe adding > > /* > * utils::set_overlap() requires the containers to be sorted, > * this is guaranteed for cio2Codes as it gets generated from a > * sorted map with a loop that preserves the order. > */ > > could avoid surprises in the future. > > > I'm feeling a bit of resistance against set in this series. Is there a > > downside to using it I don't see? I think they are neat as a > > specialization of vector where we get guarantees there is only one > > occurrence of each value and the readout/sorting order will be the same > > no mater where the container where created. Am I missing something? > > std::set is (usually) implemented as a red-black tree. That will consume > more memory than a vector, and for small sets, will likely be slower > than creating a vector and sorting it (if needed). I'm not necessarily > opposed to usage of std::set in non-hot paths, but as noted in another > reply in this series, we would need to make that consistent then, > passing a set to CameraSensor::getFormats(), and possibly more > functions. I fear std::set would quickly spread through lots of places. > That's my fear as well. If we have utility functions returning set, callers will use set too, and indeed they're not efficient as vectors. > > > before the std::transform(). I think a set is indeed overkill. > > > > > > > > + std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(), > > > > > + std::inserter(cio2Codes, cio2Codes.begin()), > > btw shouldn't this be end() and not begin() ? > As commented on Naush's patch, can this be handled with an std::back_inserter(cio2Codes) ? > > > > > + [](const std::map<uint32_t, V4L2PixelFormat>::value_type &pair){ return pair.first; }); > > > > > > > > you could > > > > [](decltype(mbusCodeToInfo)::value_type &pair){ return pair.first; }); > > > > or since C++14 > > > > [](auto &pair){ return pair.first; }); > > > > > > Much nicer indeed. > > > > Neat. > > > > > > > const std::set<unsigned int> &sensorCodes = sensor_->mbusCodes(); > > > > > if (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(), > > > > > cio2Codes.begin(), cio2Codes.end())) { > > > > > @@ -125,11 +136,12 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat) > > > > > * Apply the selected format to the sensor, the CSI-2 receiver and > > > > > * the CIO2 output device. > > > > > */ > > > > > - sensorFormat = sensor_->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10, > > > > > - MEDIA_BUS_FMT_SGBRG10_1X10, > > > > > - MEDIA_BUS_FMT_SGRBG10_1X10, > > > > > - MEDIA_BUS_FMT_SRGGB10_1X10 }, > > > > > - size); > > > > > + std::vector<unsigned int> mbusCodes; > > > > > > > > You could reserve space in the vector knowing the map size > > > > > > Seems we agree :-) > > > > > > > > + std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(), > > > > > + std::back_inserter(mbusCodes), > > > > > + [](const std::map<uint32_t, V4L2PixelFormat>::value_type &pair){ return pair.first; }); > > > > > + > > > > > + sensorFormat = sensor_->getFormat(mbusCodes, size); > > > > > ret = sensor_->setFormat(&sensorFormat); > > > > > if (ret) > > > > > return ret; > > > > > @@ -138,25 +150,11 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat) > > > > > if (ret) > > > > > return ret; > > > > > > > > > > - V4L2PixelFormat v4l2Format; > > > > > - switch (sensorFormat.mbus_code) { > > > > > - case MEDIA_BUS_FMT_SBGGR10_1X10: > > > > > - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10); > > > > > - break; > > > > > - case MEDIA_BUS_FMT_SGBRG10_1X10: > > > > > - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10); > > > > > - break; > > > > > - case MEDIA_BUS_FMT_SGRBG10_1X10: > > > > > - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10); > > > > > - break; > > > > > - case MEDIA_BUS_FMT_SRGGB10_1X10: > > > > > - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10); > > > > > - break; > > > > > - default: > > > > > + const auto &itInfo = mbusCodesToInfo.find(sensorFormat.mbus_code); > > > > > + if (itInfo == mbusCodesToInfo.end()) > > > > > return -EINVAL; > > > > > - } > > > > > > > > > > - outputFormat->fourcc = v4l2Format; > > > > > + outputFormat->fourcc = itInfo->second; > > > > > > > > I would be tempted to suggest you to add helper functions around that > > > > map, not sure it's worth the effort, but those transform to just > > > > retreive keys are not nice imho. > > > > > > I was about to mention that, thinking that the vector of keys could then > > > be cached, but given that the code is only used in init paths, it may > > > not be worth it. > > > > I see two options for this problem, the one used in this patch or > > creating multiple data structures in the anonyms namespace, one for each > > way to lookup something. The later consumes less CPU but consumes more > > memory, I'm not sure which on is the best option here. I went with this > > option as it prevents possible differences between the multiple data > > structures for lookup. I'm open to switching to that option if that is > > the consensus. > > If it was used in more places I'd cache the keys the first time they're > used, but it's not worth it here. Same for reverse lookups, I'd create > and cache a reverse map on the first call to the helper, but only if it > really makes a difference. > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > That apart > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > > > > outputFormat->size = sensorFormat.size; > > > > > outputFormat->planesCount = 1; > > -- > Regards, > > Laurent Pinchart
Hi Jacopo, Thanks for your feedback. On 2020-06-25 09:56:05 +0200, Jacopo Mondi wrote: > Hello, > > On Thu, Jun 25, 2020 at 04:23:46AM +0300, Laurent Pinchart wrote: > > Hi Niklas, > > > > On Thu, Jun 25, 2020 at 02:12:59AM +0200, Niklas Söderlund wrote: > > > On 2020-06-25 02:57:22 +0300, Laurent Pinchart wrote: > > > > On Wed, Jun 24, 2020 at 12:01:14PM +0200, Jacopo Mondi wrote: > > > > > On Tue, Jun 23, 2020 at 04:09:26AM +0200, Niklas Söderlund wrote: > > > > > > Instead of spreading the mapping between media bus codes and V4L2 FourCC > > > > > > all over the CIO2 code collect it in a single map and extract the data > > > > > > from it. This is done in preparation of adding PixelFormat information > > > > > > to the mix. > > > > > > > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > > > > --- > > > > > > src/libcamera/pipeline/ipu3/cio2.cpp | 50 +++++++++++++--------------- > > > > > > 1 file changed, 24 insertions(+), 26 deletions(-) > > > > > > > > > > > > diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp > > > > > > index dbbcf79c7b2ed5c1..f23128d412e6b1a5 100644 > > > > > > --- a/src/libcamera/pipeline/ipu3/cio2.cpp > > > > > > +++ b/src/libcamera/pipeline/ipu3/cio2.cpp > > > > > > @@ -18,6 +18,17 @@ namespace libcamera { > > > > > > > > > > > > LOG_DECLARE_CATEGORY(IPU3) > > > > > > > > > > > > +namespace { > > > > > > + > > > > > > +static const std::map<uint32_t, V4L2PixelFormat> mbusCodesToInfo = { > > > > > > + { MEDIA_BUS_FMT_SBGGR10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) }, > > > > > > + { MEDIA_BUS_FMT_SGBRG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) }, > > > > > > + { MEDIA_BUS_FMT_SGRBG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) }, > > > > > > + { MEDIA_BUS_FMT_SRGGB10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) }, > > > > > > +}; > > > > > > + > > > > > > +} /* namespace */ > > > > > > + > > > > > > > > > > Ok, I see where this is going with the next patch, but I'm not super > > > > > excited by having to iterate the map just to extract keys (and anyway, > > > > > why doesn't std::map have a keys() function ??). > > > > > > > > Possibly because its implementation would be O(n), and STL generally > > > > tries to avoid hiding expensive operations behind apparently simple > > > > functions. Just a speculation. > > > > > > Could be! > > > > > > > CIO2Device::CIO2Device() > > > > > > : output_(nullptr), csi2_(nullptr), sensor_(nullptr) > > > > > > { > > > > > > @@ -78,10 +89,10 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) > > > > > > * Make sure the sensor produces at least one format compatible with > > > > > > * the CIO2 requirements. > > > > > > */ > > > > > > - const std::set<unsigned int> cio2Codes{ MEDIA_BUS_FMT_SBGGR10_1X10, > > > > > > - MEDIA_BUS_FMT_SGRBG10_1X10, > > > > > > - MEDIA_BUS_FMT_SGBRG10_1X10, > > > > > > - MEDIA_BUS_FMT_SRGGB10_1X10 }; > > > > > > + std::set<unsigned int> cio2Codes; > > > > > > > > > > why a set ? > > > > > > > > To keep this efficient, given that std::map is sorted by key, this could > > > > be a vector, and we could call > > > > > > > > cio2Codes.reserve(mbusCodesToInfo.size()); > > > > > > We would then also need to remember to sort the vector as that is a > > > requirement for utils::set_overlap() where this is used. I think it's > > > much nicer to use a set from the start to make sure nothing goes wrong. > > > > Sorry but, we're extracting keys from a sorted map with unique > entries. The resulting vector of keys will be sorted as well, isn't it ? > > And the requirement of being sorted depends on what you will use the > vector for anway, it's not something an "extract keys" function should > care about (and again, if' I'm not mistaken, the vector of keys will > be anyhow sorted and unique, being the result of iterating an > std::map) > > > I agree std::set has that benefit. Note that here we wouldn't need to > > sort the vector explicitly. I thus don't really see a reason to use a > > set in this specific case. However, maybe adding > > > > /* > > * utils::set_overlap() requires the containers to be sorted, > > * this is guaranteed for cio2Codes as it gets generated from a > > * sorted map with a loop that preserves the order. > > */ > > > > could avoid surprises in the future. > > > > > I'm feeling a bit of resistance against set in this series. Is there a > > > downside to using it I don't see? I think they are neat as a > > > specialization of vector where we get guarantees there is only one > > > occurrence of each value and the readout/sorting order will be the same > > > no mater where the container where created. Am I missing something? > > > > std::set is (usually) implemented as a red-black tree. That will consume > > more memory than a vector, and for small sets, will likely be slower > > than creating a vector and sorting it (if needed). I'm not necessarily > > opposed to usage of std::set in non-hot paths, but as noted in another > > reply in this series, we would need to make that consistent then, > > passing a set to CameraSensor::getFormats(), and possibly more > > functions. I fear std::set would quickly spread through lots of places. > > > > That's my fear as well. If we have utility functions returning set, > callers will use set too, and indeed they're not efficient as vectors. > > > > > before the std::transform(). I think a set is indeed overkill. > > > > > > > > > > + std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(), > > > > > > + std::inserter(cio2Codes, cio2Codes.begin()), > > > > btw shouldn't this be end() and not begin() ? > > > > As commented on Naush's patch, can this be handled with an > std::back_inserter(cio2Codes) ? If its a vector yes, if its a set no :-) Sets does not implement push_back(). Since the goal of this series is not to create a (in my view) better API for the CameraSensor I will will use vectors for this interface in the next incarnation and will thus use the back_inserter here. > > > > > > > + [](const std::map<uint32_t, V4L2PixelFormat>::value_type &pair){ return pair.first; }); > > > > > > > > > > you could > > > > > [](decltype(mbusCodeToInfo)::value_type &pair){ return pair.first; }); > > > > > or since C++14 > > > > > [](auto &pair){ return pair.first; }); > > > > > > > > Much nicer indeed. > > > > > > Neat. > > > > > > > > > const std::set<unsigned int> &sensorCodes = sensor_->mbusCodes(); > > > > > > if (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(), > > > > > > cio2Codes.begin(), cio2Codes.end())) { > > > > > > @@ -125,11 +136,12 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat) > > > > > > * Apply the selected format to the sensor, the CSI-2 receiver and > > > > > > * the CIO2 output device. > > > > > > */ > > > > > > - sensorFormat = sensor_->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10, > > > > > > - MEDIA_BUS_FMT_SGBRG10_1X10, > > > > > > - MEDIA_BUS_FMT_SGRBG10_1X10, > > > > > > - MEDIA_BUS_FMT_SRGGB10_1X10 }, > > > > > > - size); > > > > > > + std::vector<unsigned int> mbusCodes; > > > > > > > > > > You could reserve space in the vector knowing the map size > > > > > > > > Seems we agree :-) > > > > > > > > > > + std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(), > > > > > > + std::back_inserter(mbusCodes), > > > > > > + [](const std::map<uint32_t, V4L2PixelFormat>::value_type &pair){ return pair.first; }); > > > > > > + > > > > > > + sensorFormat = sensor_->getFormat(mbusCodes, size); > > > > > > ret = sensor_->setFormat(&sensorFormat); > > > > > > if (ret) > > > > > > return ret; > > > > > > @@ -138,25 +150,11 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat) > > > > > > if (ret) > > > > > > return ret; > > > > > > > > > > > > - V4L2PixelFormat v4l2Format; > > > > > > - switch (sensorFormat.mbus_code) { > > > > > > - case MEDIA_BUS_FMT_SBGGR10_1X10: > > > > > > - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10); > > > > > > - break; > > > > > > - case MEDIA_BUS_FMT_SGBRG10_1X10: > > > > > > - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10); > > > > > > - break; > > > > > > - case MEDIA_BUS_FMT_SGRBG10_1X10: > > > > > > - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10); > > > > > > - break; > > > > > > - case MEDIA_BUS_FMT_SRGGB10_1X10: > > > > > > - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10); > > > > > > - break; > > > > > > - default: > > > > > > + const auto &itInfo = mbusCodesToInfo.find(sensorFormat.mbus_code); > > > > > > + if (itInfo == mbusCodesToInfo.end()) > > > > > > return -EINVAL; > > > > > > - } > > > > > > > > > > > > - outputFormat->fourcc = v4l2Format; > > > > > > + outputFormat->fourcc = itInfo->second; > > > > > > > > > > I would be tempted to suggest you to add helper functions around that > > > > > map, not sure it's worth the effort, but those transform to just > > > > > retreive keys are not nice imho. > > > > > > > > I was about to mention that, thinking that the vector of keys could then > > > > be cached, but given that the code is only used in init paths, it may > > > > not be worth it. > > > > > > I see two options for this problem, the one used in this patch or > > > creating multiple data structures in the anonyms namespace, one for each > > > way to lookup something. The later consumes less CPU but consumes more > > > memory, I'm not sure which on is the best option here. I went with this > > > option as it prevents possible differences between the multiple data > > > structures for lookup. I'm open to switching to that option if that is > > > the consensus. > > > > If it was used in more places I'd cache the keys the first time they're > > used, but it's not worth it here. Same for reverse lookups, I'd create > > and cache a reverse map on the first call to the helper, but only if it > > really makes a difference. > > > > > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > > > > > That apart > > > > > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > > > > > > > > > > outputFormat->size = sensorFormat.size; > > > > > > outputFormat->planesCount = 1; > > > > -- > > Regards, > > > > Laurent Pinchart
diff --git a/src/libcamera/pipeline/ipu3/cio2.cpp b/src/libcamera/pipeline/ipu3/cio2.cpp index dbbcf79c7b2ed5c1..f23128d412e6b1a5 100644 --- a/src/libcamera/pipeline/ipu3/cio2.cpp +++ b/src/libcamera/pipeline/ipu3/cio2.cpp @@ -18,6 +18,17 @@ namespace libcamera { LOG_DECLARE_CATEGORY(IPU3) +namespace { + +static const std::map<uint32_t, V4L2PixelFormat> mbusCodesToInfo = { + { MEDIA_BUS_FMT_SBGGR10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10) }, + { MEDIA_BUS_FMT_SGBRG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10) }, + { MEDIA_BUS_FMT_SGRBG10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10) }, + { MEDIA_BUS_FMT_SRGGB10_1X10, V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10) }, +}; + +} /* namespace */ + CIO2Device::CIO2Device() : output_(nullptr), csi2_(nullptr), sensor_(nullptr) { @@ -78,10 +89,10 @@ int CIO2Device::init(const MediaDevice *media, unsigned int index) * Make sure the sensor produces at least one format compatible with * the CIO2 requirements. */ - const std::set<unsigned int> cio2Codes{ MEDIA_BUS_FMT_SBGGR10_1X10, - MEDIA_BUS_FMT_SGRBG10_1X10, - MEDIA_BUS_FMT_SGBRG10_1X10, - MEDIA_BUS_FMT_SRGGB10_1X10 }; + std::set<unsigned int> cio2Codes; + std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(), + std::inserter(cio2Codes, cio2Codes.begin()), + [](const std::map<uint32_t, V4L2PixelFormat>::value_type &pair){ return pair.first; }); const std::set<unsigned int> &sensorCodes = sensor_->mbusCodes(); if (!utils::set_overlap(sensorCodes.begin(), sensorCodes.end(), cio2Codes.begin(), cio2Codes.end())) { @@ -125,11 +136,12 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat) * Apply the selected format to the sensor, the CSI-2 receiver and * the CIO2 output device. */ - sensorFormat = sensor_->getFormat({ MEDIA_BUS_FMT_SBGGR10_1X10, - MEDIA_BUS_FMT_SGBRG10_1X10, - MEDIA_BUS_FMT_SGRBG10_1X10, - MEDIA_BUS_FMT_SRGGB10_1X10 }, - size); + std::vector<unsigned int> mbusCodes; + std::transform(mbusCodesToInfo.begin(), mbusCodesToInfo.end(), + std::back_inserter(mbusCodes), + [](const std::map<uint32_t, V4L2PixelFormat>::value_type &pair){ return pair.first; }); + + sensorFormat = sensor_->getFormat(mbusCodes, size); ret = sensor_->setFormat(&sensorFormat); if (ret) return ret; @@ -138,25 +150,11 @@ int CIO2Device::configure(const Size &size, V4L2DeviceFormat *outputFormat) if (ret) return ret; - V4L2PixelFormat v4l2Format; - switch (sensorFormat.mbus_code) { - case MEDIA_BUS_FMT_SBGGR10_1X10: - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SBGGR10); - break; - case MEDIA_BUS_FMT_SGBRG10_1X10: - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGBRG10); - break; - case MEDIA_BUS_FMT_SGRBG10_1X10: - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SGRBG10); - break; - case MEDIA_BUS_FMT_SRGGB10_1X10: - v4l2Format = V4L2PixelFormat(V4L2_PIX_FMT_IPU3_SRGGB10); - break; - default: + const auto &itInfo = mbusCodesToInfo.find(sensorFormat.mbus_code); + if (itInfo == mbusCodesToInfo.end()) return -EINVAL; - } - outputFormat->fourcc = v4l2Format; + outputFormat->fourcc = itInfo->second; outputFormat->size = sensorFormat.size; outputFormat->planesCount = 1;
Instead of spreading the mapping between media bus codes and V4L2 FourCC all over the CIO2 code collect it in a single map and extract the data from it. This is done in preparation of adding PixelFormat information to the mix. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/libcamera/pipeline/ipu3/cio2.cpp | 50 +++++++++++++--------------- 1 file changed, 24 insertions(+), 26 deletions(-)