| Message ID | 20200424215304.558317-14-jacopo@jmondi.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Jacopo, Thank you for the patch. On Fri, Apr 24, 2020 at 11:53:04PM +0200, Jacopo Mondi wrote: > Add support for camera sensor information in the libcamera IPA protocol. > > Define a new 'struct ipa_sensor_info' structure in the IPA context and > use it to perform translation between the C and the C++ API. > > Update the IPAInterface::configure() operation to accept a new > CameraSensorInfo parameter and port all users of that function to > the new interface. > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > --- > include/ipa/ipa_interface.h | 21 +++++++- > src/ipa/libipa/ipa_interface_wrapper.cpp | 19 ++++++- > src/ipa/libipa/ipa_interface_wrapper.h | 1 + > src/ipa/rkisp1/rkisp1.cpp | 13 ++++- > src/ipa/vimc/vimc.cpp | 4 +- > src/libcamera/include/ipa_context_wrapper.h | 4 +- > src/libcamera/ipa_context_wrapper.cpp | 24 +++++++-- > src/libcamera/ipa_interface.cpp | 60 +++++++++++++++++++++ > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 +++- > src/libcamera/proxy/ipa_proxy_linux.cpp | 4 +- > src/libcamera/proxy/ipa_proxy_thread.cpp | 9 ++-- > test/ipa/ipa_wrappers_test.cpp | 21 +++++++- > 12 files changed, 173 insertions(+), 16 deletions(-) > > diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h > index e65844bc7b34..a7acc09c4715 100644 > --- a/include/ipa/ipa_interface.h > +++ b/include/ipa/ipa_interface.h > @@ -18,6 +18,22 @@ struct ipa_context { > const struct ipa_context_ops *ops; > }; > > +struct ipa_sensor_info { > +#define CAMERA_SENSOR_NAME_LEN 32 > + char name[CAMERA_SENSOR_NAME_LEN]; Fixed-size strings in APIs are not nice :-S Do you think we could use a const char * ? This should also be renamed to model if you agree with the previous related suggestion. > + uint8_t bits_per_pixel; > + uint32_t active_area_width; > + uint32_t active_area_height; How about struct { uint32_t width; uint32_t height; } active_area; Same for crop and output size. > + int32_t analog_crop_left; > + int32_t analog_crop_top; > + uint32_t analog_crop_width; > + uint32_t analog_crop_height; > + uint32_t output_width; > + uint32_t output_height; > + uint32_t pixel_clock; > + uint32_t line_length; > +}; > + > struct ipa_stream { > unsigned int id; > unsigned int pixel_format; > @@ -70,6 +86,7 @@ struct ipa_context_ops { > const struct ipa_callback_ops *callbacks, > void *cb_ctx); > void (*configure)(struct ipa_context *ctx, > + const struct ipa_sensor_info *sensor_info, > const struct ipa_stream *streams, > unsigned int num_streams, > const struct ipa_control_info_map *maps, > @@ -96,6 +113,7 @@ struct ipa_context *ipaCreate(); > #include <libcamera/geometry.h> > #include <libcamera/signal.h> > > +#include "camera_sensor.h" You can forward-declare struct CameraSensorInfo instead of includeing camera_sensor.h. Same comment in many locations below. > #include "v4l2_controls.h" > > namespace libcamera { > @@ -125,7 +143,8 @@ public: > virtual int start() = 0; > virtual void stop() = 0; > > - virtual void configure(const std::map<unsigned int, IPAStream> &streamConfig, > + virtual void configure(const CameraSensorInfo &sensorInfo, > + const std::map<unsigned int, IPAStream> &streamConfig, > const std::map<unsigned int, const ControlInfoMap &> &entityControls) = 0; > > virtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0; > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp > index f50f93a0185a..049b9dd1eefc 100644 > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp > @@ -15,6 +15,7 @@ > #include <ipa/ipa_interface.h> > > #include "byte_stream_buffer.h" > +#include "camera_sensor.h" > > /** > * \file ipa_interface_wrapper.h > @@ -111,6 +112,7 @@ void IPAInterfaceWrapper::register_callbacks(struct ipa_context *_ctx, > } > > void IPAInterfaceWrapper::configure(struct ipa_context *_ctx, > + const struct ipa_sensor_info *sensor_info, > const struct ipa_stream *streams, > unsigned int num_streams, > const struct ipa_control_info_map *maps, > @@ -120,6 +122,21 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx, > > ctx->serializer_.reset(); > > + /* Translate the IPA sensor info. */ > + CameraSensorInfo sensorInfo{}; > + sensorInfo.name = sensor_info->name; > + sensorInfo.bitsPerPixel = sensor_info->bits_per_pixel; > + sensorInfo.activeAreaSize = { sensor_info->active_area_width, > + sensor_info->active_area_height, }; As this isn't an array but a braced initializer list for a class, should we remove the trailing comma ? It shouldn't grow over time. Same for the other ones below. > + sensorInfo.analogCrop = { sensor_info->analog_crop_left, > + sensor_info->analog_crop_top, > + sensor_info->analog_crop_width, > + sensor_info->analog_crop_height, }; > + sensorInfo.outputSize = { sensor_info->output_width, > + sensor_info->output_height, }; > + sensorInfo.pixelClock = sensor_info->pixel_clock; > + sensorInfo.lineLength = sensor_info->line_length; > + > /* Translate the IPA stream configurations map. */ > std::map<unsigned int, IPAStream> ipaStreams; > > @@ -145,7 +162,7 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx, > entityControls.emplace(id, infoMaps[id]); > } > > - ctx->ipa_->configure(ipaStreams, entityControls); > + ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls); > } > > void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx, > diff --git a/src/ipa/libipa/ipa_interface_wrapper.h b/src/ipa/libipa/ipa_interface_wrapper.h > index e4bc6dd4535d..febd6e66d0c4 100644 > --- a/src/ipa/libipa/ipa_interface_wrapper.h > +++ b/src/ipa/libipa/ipa_interface_wrapper.h > @@ -30,6 +30,7 @@ private: > const struct ipa_callback_ops *callbacks, > void *cb_ctx); > static void configure(struct ipa_context *ctx, > + const struct ipa_sensor_info *sensor_info, > const struct ipa_stream *streams, > unsigned int num_streams, > const struct ipa_control_info_map *maps, > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > index acbbe58c7a2e..8b081359afff 100644 > --- a/src/ipa/rkisp1/rkisp1.cpp > +++ b/src/ipa/rkisp1/rkisp1.cpp > @@ -22,6 +22,7 @@ > #include <libcamera/request.h> > #include <libipa/ipa_interface_wrapper.h> > > +#include "camera_sensor.h" > #include "log.h" > #include "utils.h" > > @@ -36,7 +37,8 @@ public: > int start() override { return 0; } > void stop() override {} > > - void configure(const std::map<unsigned int, IPAStream> &streamConfig, > + void configure(const CameraSensorInfo &info, > + const std::map<unsigned int, IPAStream> &streamConfig, > const std::map<unsigned int, const ControlInfoMap &> &entityControls) override; > void mapBuffers(const std::vector<IPABuffer> &buffers) override; > void unmapBuffers(const std::vector<unsigned int> &ids) override; > @@ -66,7 +68,14 @@ private: > uint32_t maxGain_; > }; > > -void IPARkISP1::configure(const std::map<unsigned int, IPAStream> &streamConfig, > +/** > + * \todo The RkISP1 pipeline currently provides an empty CameraSensorInfo > + * if the connected sensor does not provide enough information to properly > + * assemble one. Make sure the reported sensor information are relevant > + * befor accessing them. s/befor/before/ I think we should make sure the corresponding kernel drivers implement the features we need, and then turn this into a fatal error. Not part of this series of course. What's your opinion on that ? > + */ > +void IPARkISP1::configure(const CameraSensorInfo &info, > + const std::map<unsigned int, IPAStream> &streamConfig, > const std::map<unsigned int, const ControlInfoMap &> &entityControls) > { > if (entityControls.empty()) > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp > index d2267e11737f..b8aadd8588cb 100644 > --- a/src/ipa/vimc/vimc.cpp > +++ b/src/ipa/vimc/vimc.cpp > @@ -19,6 +19,7 @@ > > #include <libipa/ipa_interface_wrapper.h> > > +#include "camera_sensor.h" > #include "log.h" > > namespace libcamera { > @@ -36,7 +37,8 @@ public: > int start() override; > void stop() override; > > - void configure(const std::map<unsigned int, IPAStream> &streamConfig, > + void configure(const CameraSensorInfo &sensorInfo, > + const std::map<unsigned int, IPAStream> &streamConfig, > const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {} > void mapBuffers(const std::vector<IPABuffer> &buffers) override {} > void unmapBuffers(const std::vector<unsigned int> &ids) override {} > diff --git a/src/libcamera/include/ipa_context_wrapper.h b/src/libcamera/include/ipa_context_wrapper.h > index 0a48bfe732c8..e11e7b6a894d 100644 > --- a/src/libcamera/include/ipa_context_wrapper.h > +++ b/src/libcamera/include/ipa_context_wrapper.h > @@ -9,6 +9,7 @@ > > #include <ipa/ipa_interface.h> > > +#include "camera_sensor.h" > #include "control_serializer.h" > > namespace libcamera { > @@ -22,7 +23,8 @@ public: > int init() override; > int start() override; > void stop() override; > - void configure(const std::map<unsigned int, IPAStream> &streamConfig, > + void configure(const CameraSensorInfo &sensorInfo, > + const std::map<unsigned int, IPAStream> &streamConfig, > const std::map<unsigned int, const ControlInfoMap &> &entityControls) override; > > void mapBuffers(const std::vector<IPABuffer> &buffers) override; > diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp > index ab6ce396b88a..b1ff83aa8be8 100644 > --- a/src/libcamera/ipa_context_wrapper.cpp > +++ b/src/libcamera/ipa_context_wrapper.cpp > @@ -12,6 +12,7 @@ > #include <libcamera/controls.h> > > #include "byte_stream_buffer.h" > +#include "camera_sensor.h" > #include "utils.h" > > /** > @@ -104,17 +105,34 @@ void IPAContextWrapper::stop() > ctx_->ops->stop(ctx_); > } > > -void IPAContextWrapper::configure(const std::map<unsigned int, IPAStream> &streamConfig, > +void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo, > + const std::map<unsigned int, IPAStream> &streamConfig, > const std::map<unsigned int, const ControlInfoMap &> &entityControls) > { > if (intf_) > - return intf_->configure(streamConfig, entityControls); > + return intf_->configure(sensorInfo, streamConfig, entityControls); > > if (!ctx_) > return; > > serializer_.reset(); > > + /* Translate the camera sensor info. */ > + struct ipa_sensor_info sensor_info = {}; > + strncpy(sensor_info.name, sensorInfo.name.c_str(), > + sensorInfo.name.length()); > + sensor_info.bits_per_pixel = sensorInfo.bitsPerPixel; > + sensor_info.active_area_width = sensorInfo.activeAreaSize.width; > + sensor_info.active_area_height = sensorInfo.activeAreaSize.height; > + sensor_info.analog_crop_left = sensorInfo.analogCrop.x; > + sensor_info.analog_crop_top = sensorInfo.analogCrop.y; > + sensor_info.analog_crop_width = sensorInfo.analogCrop.width; > + sensor_info.analog_crop_height = sensorInfo.analogCrop.height; > + sensor_info.output_width = sensorInfo.outputSize.width; > + sensor_info.output_height = sensorInfo.outputSize.height; > + sensor_info.pixel_clock = sensorInfo.pixelClock; > + sensor_info.line_length = sensorInfo.lineLength; > + > /* Translate the IPA stream configurations map. */ > struct ipa_stream c_streams[streamConfig.size()]; > > @@ -154,7 +172,7 @@ void IPAContextWrapper::configure(const std::map<unsigned int, IPAStream> &strea > ++i; > } > > - ctx_->ops->configure(ctx_, c_streams, streamConfig.size(), > + ctx_->ops->configure(ctx_, &sensor_info, c_streams, streamConfig.size(), > c_info_maps, entityControls.size()); > } > > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp > index 890d4340e4f3..22a3567bc1d0 100644 > --- a/src/libcamera/ipa_interface.cpp > +++ b/src/libcamera/ipa_interface.cpp > @@ -92,6 +92,65 @@ > * \brief The IPA context operations > */ > > +/** > + * \struct ipa_sensor_info > + * \brief Camera sensor information for the IPA context operations > + * \sa libcamera::CameraSensorInfo > + * > + * \def CAMERA_SENSOR_NAME_LEN > + * \brief The camera sensor name maximum length > + * > + * \var ipa_sensor_info::name > + * \brief The camera sensor name > + * \todo Remove this field as soon as no IPA depends on it anymore > + * > + * \var ipa_sensor_info::bits_per_pixel > + * \brief The camera sensor image format bit depth > + * \sa libcamera::CameraSensorInfo::bitsPerPixel > + * > + * \var ipa_sensor_info::active_area_width > + * \brief The camera sensor pixel array active area width > + * \sa libcamera::CameraSensorInfo::activeAreaSize > + * > + * \var ipa_sensor_info::active_area_height > + * \brief The camera sensor pixel array active area height > + * \sa libcamera::CameraSensorInfo::activeAreaSize > + * > + * \var ipa_sensor_info::analog_crop_left > + * \brief The horizontal displacement from the active area top-left corner of > + * the cropped portion of the pixel array matrix I have a bit of a hard time parsing this late at night :-) Would the following be more readable ? * \brief The left coordinate of the analog crop rectangle, relative to * the pixel array active area Same for below. > + * \sa libcamera::CameraSensorInfo::analogCrop > + * > + * \var ipa_sensor_info::analog_crop_top > + * \brief The vertical displacement from the active area top-left corner of > + * the cropped portion of the pixel array matrix > + * \sa libcamera::CameraSensorInfo::analogCrop > + * > + * \var ipa_sensor_info::analog_crop_width > + * \brief The horizontal size of the cropped portion of the pixel array matrix > + * \sa libcamera::CameraSensorInfo::analogCrop > + * > + * \var ipa_sensor_info::analog_crop_height > + * \brief The vertical size of the cropped portion of the pixel array matrix > + * \sa libcamera::CameraSensorInfo::analogCrop > + * > + * \var ipa_sensor_info::output_width > + * \brief The horizontal size of the output image > + * \sa libcamera::CameraSensorInfo::outputSize > + * > + * \var ipa_sensor_info::output_height > + * \brief The vertical size of the output image > + * \sa libcamera::CameraSensorInfo::outputSize > + * > + * \var ipa_sensor_info::pixel_clock I forgot to mention when reviewing the CameraSensorInfo class, should this be named pixel rate instead of pixel clock, as it's effectively the number of pixels per second ? Some formats may require multiple clock cycles per pixel (or transmit multiple pixels per clock cycle), so it could make a difference. > + * \brief the pixel clock out frequency in Hz > + * \sa libcamera::CameraSensorInfo::pixelClock > + * > + * \var ipa_sensor_info::line_length > + * \brief The full line length, including blanking, in pixel units > + * \sa libcamera::CameraSensorInfo::lineLength > + */ > + > /** > * \struct ipa_stream > * \brief Stream information for the IPA context operations > @@ -447,6 +506,7 @@ namespace libcamera { > /** > * \fn IPAInterface::configure() > * \brief Configure the IPA stream and sensor settings > + * \param[in] sensorInfo Camera sensor information > * \param[in] streamConfig Configuration of all active streams > * \param[in] entityControls Controls provided by the pipeline entities > * How about adding another paragraph at the end of the documentation block to explain this in a bit more details ? * The \a sensorInfo conveys information about the camera sensor settings that * the pipeline handler has selected for the configuration. The IPA may use * that information to tune its algorithms. > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index f42264361785..169492e8313f 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -822,6 +822,13 @@ int PipelineHandlerRkISP1::start(Camera *camera) > activeCamera_ = camera; > > /* Inform IPA of stream configuration and sensor controls. */ > + CameraSensorInfo sensorInfo = {}; > + ret = data->sensor_->sensorInfo(&sensorInfo); > + if (ret) { > + LOG(RkISP1, Info) << "Camera sensor information not available"; > + sensorInfo = {}; > + } > + > std::map<unsigned int, IPAStream> streamConfig; > streamConfig[0] = { > .pixelFormat = data->stream_.configuration().pixelFormat, > @@ -831,7 +838,7 @@ int PipelineHandlerRkISP1::start(Camera *camera) > std::map<unsigned int, const ControlInfoMap &> entityControls; > entityControls.emplace(0, data->sensor_->controls()); > > - data->ipa_->configure(streamConfig, entityControls); > + data->ipa_->configure(sensorInfo, streamConfig, entityControls); > > return ret; > } > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp > index 2aa80b946704..54b1abc4727b 100644 > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp > @@ -10,6 +10,7 @@ > #include <ipa/ipa_interface.h> > #include <ipa/ipa_module_info.h> > > +#include "camera_sensor.h" > #include "ipa_module.h" > #include "ipa_proxy.h" > #include "ipc_unixsocket.h" > @@ -29,7 +30,8 @@ public: > int init() override { return 0; } > int start() override { return 0; } > void stop() override {} > - void configure(const std::map<unsigned int, IPAStream> &streamConfig, > + void configure(const CameraSensorInfo &sensorInfo, > + const std::map<unsigned int, IPAStream> &streamConfig, > const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {} > void mapBuffers(const std::vector<IPABuffer> &buffers) override {} > void unmapBuffers(const std::vector<unsigned int> &ids) override {} > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp > index 368ccca1cf60..b06734371b39 100644 > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp > @@ -10,6 +10,7 @@ > #include <ipa/ipa_interface.h> > #include <ipa/ipa_module_info.h> > > +#include "camera_sensor.h" > #include "ipa_context_wrapper.h" > #include "ipa_module.h" > #include "ipa_proxy.h" > @@ -29,7 +30,8 @@ public: > int start() override; > void stop() override; > > - void configure(const std::map<unsigned int, IPAStream> &streamConfig, > + void configure(const CameraSensorInfo &sensorInfo, > + const std::map<unsigned int, IPAStream> &streamConfig, > const std::map<unsigned int, const ControlInfoMap &> &entityControls) override; > void mapBuffers(const std::vector<IPABuffer> &buffers) override; > void unmapBuffers(const std::vector<unsigned int> &ids) override; > @@ -126,10 +128,11 @@ void IPAProxyThread::stop() > thread_.wait(); > } > > -void IPAProxyThread::configure(const std::map<unsigned int, IPAStream> &streamConfig, > +void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo, > + const std::map<unsigned int, IPAStream> &streamConfig, > const std::map<unsigned int, const ControlInfoMap &> &entityControls) > { > - ipa_->configure(streamConfig, entityControls); > + ipa_->configure(sensorInfo, streamConfig, entityControls); > } > > void IPAProxyThread::mapBuffers(const std::vector<IPABuffer> &buffers) > diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp > index fae1d56b67c7..2cd10f981ecc 100644 > --- a/test/ipa/ipa_wrappers_test.cpp > +++ b/test/ipa/ipa_wrappers_test.cpp > @@ -15,6 +15,7 @@ > #include <libcamera/controls.h> > #include <libipa/ipa_interface_wrapper.h> > > +#include "camera_sensor.h" > #include "device_enumerator.h" > #include "ipa_context_wrapper.h" > #include "media_device.h" > @@ -60,9 +61,19 @@ public: > report(Op_stop, TestPass); > } > > - void configure(const std::map<unsigned int, IPAStream> &streamConfig, > + void configure(const CameraSensorInfo &sensorInfo, > + const std::map<unsigned int, IPAStream> &streamConfig, > const std::map<unsigned int, const ControlInfoMap &> &entityControls) override > { > + /* Verify sensorInfo. */ > + if (sensorInfo.outputSize.width != 2560 || > + sensorInfo.outputSize.height != 1940) { > + cerr << "configure(): Invalid sensor info sizes: (" > + << sensorInfo.outputSize.width << "x" > + << sensorInfo.outputSize.height << ")" << endl; You can use cerr << "configure(): Invalid sensor info sizes: " << sensorInfo.outputSize.toString() << endl; > + return report(Op_configure, TestFail); > + } > + > /* Verify streamConfig. */ > if (streamConfig.size() != 2) { > cerr << "configure(): Invalid number of streams " > @@ -287,13 +298,19 @@ protected: > int ret; > > /* Test configure(). */ > + CameraSensorInfo sensorInfo = { "sensor", 8, s/ = // would save a copy-construction (but I expect the compiler to optimize it out anyway). > + { 2576, 1956 }, > + { 8, 8, 2560, 1940 }, > + { 2560, 1940 }, > + 96000000, > + 2918, }; Would CameraSensorInfo sensorInfo{ "sensor", 8, { 2576, 1956 }, { 8, 8, 2560, 1940 }, { 2560, 1940 }, 96000000, 2918, }; be more readable ? As far as I can tell, clang++ supports the C99-extension designated initializers in C++, and g++ also supports them provided the fields are expressed in the same order in the initialization than in the declaration (this differs from the gcc implementation where the order isn't constrained). Should we use this to increase readability ? CameraSensorInfo sensorInfo{ .name = "sensor", .bitsPerPixel = 8, .activeAreaSize = { 2576, 1956 }, .analogCrop = { 8, 8, 2560, 1940 }, .outputCrop = { 2560, 1940 }, .pixelClock = 96000000, .lineLength = 2918, }; > std::map<unsigned int, IPAStream> config{ > { 1, { V4L2_PIX_FMT_YUYV, { 1024, 768 } } }, > { 2, { V4L2_PIX_FMT_NV12, { 800, 600 } } }, > }; > std::map<unsigned int, const ControlInfoMap &> controlInfo; > controlInfo.emplace(42, subdev_->controls()); > - ret = INVOKE(configure, config, controlInfo); > + ret = INVOKE(configure, sensorInfo, config, controlInfo); > if (ret == TestFail) > return TestFail; >
Hi Laurent, On Sun, Apr 26, 2020 at 01:32:47AM +0300, Laurent Pinchart wrote: > Hi Jacopo, > > Thank you for the patch. > > On Fri, Apr 24, 2020 at 11:53:04PM +0200, Jacopo Mondi wrote: > > Add support for camera sensor information in the libcamera IPA protocol. > > > > Define a new 'struct ipa_sensor_info' structure in the IPA context and > > use it to perform translation between the C and the C++ API. > > > > Update the IPAInterface::configure() operation to accept a new > > CameraSensorInfo parameter and port all users of that function to > > the new interface. > > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> > > --- > > include/ipa/ipa_interface.h | 21 +++++++- > > src/ipa/libipa/ipa_interface_wrapper.cpp | 19 ++++++- > > src/ipa/libipa/ipa_interface_wrapper.h | 1 + > > src/ipa/rkisp1/rkisp1.cpp | 13 ++++- > > src/ipa/vimc/vimc.cpp | 4 +- > > src/libcamera/include/ipa_context_wrapper.h | 4 +- > > src/libcamera/ipa_context_wrapper.cpp | 24 +++++++-- > > src/libcamera/ipa_interface.cpp | 60 +++++++++++++++++++++ > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 +++- > > src/libcamera/proxy/ipa_proxy_linux.cpp | 4 +- > > src/libcamera/proxy/ipa_proxy_thread.cpp | 9 ++-- > > test/ipa/ipa_wrappers_test.cpp | 21 +++++++- > > 12 files changed, 173 insertions(+), 16 deletions(-) > > > > diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h > > index e65844bc7b34..a7acc09c4715 100644 > > --- a/include/ipa/ipa_interface.h > > +++ b/include/ipa/ipa_interface.h > > @@ -18,6 +18,22 @@ struct ipa_context { > > const struct ipa_context_ops *ops; > > }; > > > > +struct ipa_sensor_info { > > +#define CAMERA_SENSOR_NAME_LEN 32 > > + char name[CAMERA_SENSOR_NAME_LEN]; > > Fixed-size strings in APIs are not nice :-S Do you think we could use a > const char * ? > > This should also be renamed to model if you agree with the previous > related suggestion. > As clarified in the review of your recent patch on IPA configuration, I can change this, yes > > + uint8_t bits_per_pixel; > > + uint32_t active_area_width; > > + uint32_t active_area_height; > > How about > > struct { > uint32_t width; > uint32_t height; > } active_area; much nicer! > > Same for crop and output size. I've named the two structures 'analog_crop' and 'output_size' > > > + int32_t analog_crop_left; > > + int32_t analog_crop_top; > > + uint32_t analog_crop_width; > > + uint32_t analog_crop_height; > > + uint32_t output_width; > > + uint32_t output_height; > > + uint32_t pixel_clock; > > + uint32_t line_length; > > +}; > > + > > struct ipa_stream { > > unsigned int id; > > unsigned int pixel_format; > > @@ -70,6 +86,7 @@ struct ipa_context_ops { > > const struct ipa_callback_ops *callbacks, > > void *cb_ctx); > > void (*configure)(struct ipa_context *ctx, > > + const struct ipa_sensor_info *sensor_info, > > const struct ipa_stream *streams, > > unsigned int num_streams, > > const struct ipa_control_info_map *maps, > > @@ -96,6 +113,7 @@ struct ipa_context *ipaCreate(); > > #include <libcamera/geometry.h> > > #include <libcamera/signal.h> > > > > +#include "camera_sensor.h" > > You can forward-declare struct CameraSensorInfo instead of includeing > camera_sensor.h. Same comment in many locations below. > > > #include "v4l2_controls.h" > > > > namespace libcamera { > > @@ -125,7 +143,8 @@ public: > > virtual int start() = 0; > > virtual void stop() = 0; > > > > - virtual void configure(const std::map<unsigned int, IPAStream> &streamConfig, > > + virtual void configure(const CameraSensorInfo &sensorInfo, > > + const std::map<unsigned int, IPAStream> &streamConfig, > > const std::map<unsigned int, const ControlInfoMap &> &entityControls) = 0; > > > > virtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0; > > diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp > > index f50f93a0185a..049b9dd1eefc 100644 > > --- a/src/ipa/libipa/ipa_interface_wrapper.cpp > > +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp > > @@ -15,6 +15,7 @@ > > #include <ipa/ipa_interface.h> > > > > #include "byte_stream_buffer.h" > > +#include "camera_sensor.h" > > > > /** > > * \file ipa_interface_wrapper.h > > @@ -111,6 +112,7 @@ void IPAInterfaceWrapper::register_callbacks(struct ipa_context *_ctx, > > } > > > > void IPAInterfaceWrapper::configure(struct ipa_context *_ctx, > > + const struct ipa_sensor_info *sensor_info, > > const struct ipa_stream *streams, > > unsigned int num_streams, > > const struct ipa_control_info_map *maps, > > @@ -120,6 +122,21 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx, > > > > ctx->serializer_.reset(); > > > > + /* Translate the IPA sensor info. */ > > + CameraSensorInfo sensorInfo{}; > > + sensorInfo.name = sensor_info->name; > > + sensorInfo.bitsPerPixel = sensor_info->bits_per_pixel; > > + sensorInfo.activeAreaSize = { sensor_info->active_area_width, > > + sensor_info->active_area_height, }; > > As this isn't an array but a braced initializer list for a class, should > we remove the trailing comma ? It shouldn't grow over time. Same for the > other ones below. Ah yes, I usually end with a comma as it minimizes changes if it has to be extended, but this doesn't seem to be the case > > > + sensorInfo.analogCrop = { sensor_info->analog_crop_left, > > + sensor_info->analog_crop_top, > > + sensor_info->analog_crop_width, > > + sensor_info->analog_crop_height, }; > > + sensorInfo.outputSize = { sensor_info->output_width, > > + sensor_info->output_height, }; > > + sensorInfo.pixelClock = sensor_info->pixel_clock; > > + sensorInfo.lineLength = sensor_info->line_length; > > + > > /* Translate the IPA stream configurations map. */ > > std::map<unsigned int, IPAStream> ipaStreams; > > > > @@ -145,7 +162,7 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx, > > entityControls.emplace(id, infoMaps[id]); > > } > > > > - ctx->ipa_->configure(ipaStreams, entityControls); > > + ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls); > > } > > > > void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx, > > diff --git a/src/ipa/libipa/ipa_interface_wrapper.h b/src/ipa/libipa/ipa_interface_wrapper.h > > index e4bc6dd4535d..febd6e66d0c4 100644 > > --- a/src/ipa/libipa/ipa_interface_wrapper.h > > +++ b/src/ipa/libipa/ipa_interface_wrapper.h > > @@ -30,6 +30,7 @@ private: > > const struct ipa_callback_ops *callbacks, > > void *cb_ctx); > > static void configure(struct ipa_context *ctx, > > + const struct ipa_sensor_info *sensor_info, > > const struct ipa_stream *streams, > > unsigned int num_streams, > > const struct ipa_control_info_map *maps, > > diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp > > index acbbe58c7a2e..8b081359afff 100644 > > --- a/src/ipa/rkisp1/rkisp1.cpp > > +++ b/src/ipa/rkisp1/rkisp1.cpp > > @@ -22,6 +22,7 @@ > > #include <libcamera/request.h> > > #include <libipa/ipa_interface_wrapper.h> > > > > +#include "camera_sensor.h" > > #include "log.h" > > #include "utils.h" > > > > @@ -36,7 +37,8 @@ public: > > int start() override { return 0; } > > void stop() override {} > > > > - void configure(const std::map<unsigned int, IPAStream> &streamConfig, > > + void configure(const CameraSensorInfo &info, > > + const std::map<unsigned int, IPAStream> &streamConfig, > > const std::map<unsigned int, const ControlInfoMap &> &entityControls) override; > > void mapBuffers(const std::vector<IPABuffer> &buffers) override; > > void unmapBuffers(const std::vector<unsigned int> &ids) override; > > @@ -66,7 +68,14 @@ private: > > uint32_t maxGain_; > > }; > > > > -void IPARkISP1::configure(const std::map<unsigned int, IPAStream> &streamConfig, > > +/** > > + * \todo The RkISP1 pipeline currently provides an empty CameraSensorInfo > > + * if the connected sensor does not provide enough information to properly > > + * assemble one. Make sure the reported sensor information are relevant > > + * befor accessing them. > > s/befor/before/ > > I think we should make sure the corresponding kernel drivers implement > the features we need, and then turn this into a fatal error. Not part of > this series of course. What's your opinion on that ? > I would say it's up to pipeline handlers/IPA. If they actually need sensor information, they won't be able to create one without propert support for the requested features in the sensor driver. For others, say VIMC, this is fine. It has to be addressed in the pipeline handler though... > > + */ > > +void IPARkISP1::configure(const CameraSensorInfo &info, > > + const std::map<unsigned int, IPAStream> &streamConfig, > > const std::map<unsigned int, const ControlInfoMap &> &entityControls) > > { > > if (entityControls.empty()) > > diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp > > index d2267e11737f..b8aadd8588cb 100644 > > --- a/src/ipa/vimc/vimc.cpp > > +++ b/src/ipa/vimc/vimc.cpp > > @@ -19,6 +19,7 @@ > > > > #include <libipa/ipa_interface_wrapper.h> > > > > +#include "camera_sensor.h" > > #include "log.h" > > > > namespace libcamera { > > @@ -36,7 +37,8 @@ public: > > int start() override; > > void stop() override; > > > > - void configure(const std::map<unsigned int, IPAStream> &streamConfig, > > + void configure(const CameraSensorInfo &sensorInfo, > > + const std::map<unsigned int, IPAStream> &streamConfig, > > const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {} > > void mapBuffers(const std::vector<IPABuffer> &buffers) override {} > > void unmapBuffers(const std::vector<unsigned int> &ids) override {} > > diff --git a/src/libcamera/include/ipa_context_wrapper.h b/src/libcamera/include/ipa_context_wrapper.h > > index 0a48bfe732c8..e11e7b6a894d 100644 > > --- a/src/libcamera/include/ipa_context_wrapper.h > > +++ b/src/libcamera/include/ipa_context_wrapper.h > > @@ -9,6 +9,7 @@ > > > > #include <ipa/ipa_interface.h> > > > > +#include "camera_sensor.h" > > #include "control_serializer.h" > > > > namespace libcamera { > > @@ -22,7 +23,8 @@ public: > > int init() override; > > int start() override; > > void stop() override; > > - void configure(const std::map<unsigned int, IPAStream> &streamConfig, > > + void configure(const CameraSensorInfo &sensorInfo, > > + const std::map<unsigned int, IPAStream> &streamConfig, > > const std::map<unsigned int, const ControlInfoMap &> &entityControls) override; > > > > void mapBuffers(const std::vector<IPABuffer> &buffers) override; > > diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp > > index ab6ce396b88a..b1ff83aa8be8 100644 > > --- a/src/libcamera/ipa_context_wrapper.cpp > > +++ b/src/libcamera/ipa_context_wrapper.cpp > > @@ -12,6 +12,7 @@ > > #include <libcamera/controls.h> > > > > #include "byte_stream_buffer.h" > > +#include "camera_sensor.h" > > #include "utils.h" > > > > /** > > @@ -104,17 +105,34 @@ void IPAContextWrapper::stop() > > ctx_->ops->stop(ctx_); > > } > > > > -void IPAContextWrapper::configure(const std::map<unsigned int, IPAStream> &streamConfig, > > +void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo, > > + const std::map<unsigned int, IPAStream> &streamConfig, > > const std::map<unsigned int, const ControlInfoMap &> &entityControls) > > { > > if (intf_) > > - return intf_->configure(streamConfig, entityControls); > > + return intf_->configure(sensorInfo, streamConfig, entityControls); > > > > if (!ctx_) > > return; > > > > serializer_.reset(); > > > > + /* Translate the camera sensor info. */ > > + struct ipa_sensor_info sensor_info = {}; > > + strncpy(sensor_info.name, sensorInfo.name.c_str(), > > + sensorInfo.name.length()); > > + sensor_info.bits_per_pixel = sensorInfo.bitsPerPixel; > > + sensor_info.active_area_width = sensorInfo.activeAreaSize.width; > > + sensor_info.active_area_height = sensorInfo.activeAreaSize.height; > > + sensor_info.analog_crop_left = sensorInfo.analogCrop.x; > > + sensor_info.analog_crop_top = sensorInfo.analogCrop.y; > > + sensor_info.analog_crop_width = sensorInfo.analogCrop.width; > > + sensor_info.analog_crop_height = sensorInfo.analogCrop.height; > > + sensor_info.output_width = sensorInfo.outputSize.width; > > + sensor_info.output_height = sensorInfo.outputSize.height; > > + sensor_info.pixel_clock = sensorInfo.pixelClock; > > + sensor_info.line_length = sensorInfo.lineLength; > > + > > /* Translate the IPA stream configurations map. */ > > struct ipa_stream c_streams[streamConfig.size()]; > > > > @@ -154,7 +172,7 @@ void IPAContextWrapper::configure(const std::map<unsigned int, IPAStream> &strea > > ++i; > > } > > > > - ctx_->ops->configure(ctx_, c_streams, streamConfig.size(), > > + ctx_->ops->configure(ctx_, &sensor_info, c_streams, streamConfig.size(), > > c_info_maps, entityControls.size()); > > } > > > > diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp > > index 890d4340e4f3..22a3567bc1d0 100644 > > --- a/src/libcamera/ipa_interface.cpp > > +++ b/src/libcamera/ipa_interface.cpp > > @@ -92,6 +92,65 @@ > > * \brief The IPA context operations > > */ > > > > +/** > > + * \struct ipa_sensor_info > > + * \brief Camera sensor information for the IPA context operations > > + * \sa libcamera::CameraSensorInfo > > + * > > + * \def CAMERA_SENSOR_NAME_LEN > > + * \brief The camera sensor name maximum length > > + * > > + * \var ipa_sensor_info::name > > + * \brief The camera sensor name > > + * \todo Remove this field as soon as no IPA depends on it anymore > > + * > > + * \var ipa_sensor_info::bits_per_pixel > > + * \brief The camera sensor image format bit depth > > + * \sa libcamera::CameraSensorInfo::bitsPerPixel > > + * > > + * \var ipa_sensor_info::active_area_width > > + * \brief The camera sensor pixel array active area width > > + * \sa libcamera::CameraSensorInfo::activeAreaSize > > + * > > + * \var ipa_sensor_info::active_area_height > > + * \brief The camera sensor pixel array active area height > > + * \sa libcamera::CameraSensorInfo::activeAreaSize > > + * > > + * \var ipa_sensor_info::analog_crop_left > > + * \brief The horizontal displacement from the active area top-left corner of > > + * the cropped portion of the pixel array matrix > > I have a bit of a hard time parsing this late at night :-) Would the > following be more readable ? > > * \brief The left coordinate of the analog crop rectangle, relative to > * the pixel array active area > > Same for below. > > > + * \sa libcamera::CameraSensorInfo::analogCrop > > + * > > + * \var ipa_sensor_info::analog_crop_top > > + * \brief The vertical displacement from the active area top-left corner of > > + * the cropped portion of the pixel array matrix > > + * \sa libcamera::CameraSensorInfo::analogCrop > > + * > > + * \var ipa_sensor_info::analog_crop_width > > + * \brief The horizontal size of the cropped portion of the pixel array matrix > > + * \sa libcamera::CameraSensorInfo::analogCrop > > + * > > + * \var ipa_sensor_info::analog_crop_height > > + * \brief The vertical size of the cropped portion of the pixel array matrix > > + * \sa libcamera::CameraSensorInfo::analogCrop > > + * > > + * \var ipa_sensor_info::output_width > > + * \brief The horizontal size of the output image > > + * \sa libcamera::CameraSensorInfo::outputSize > > + * > > + * \var ipa_sensor_info::output_height > > + * \brief The vertical size of the output image > > + * \sa libcamera::CameraSensorInfo::outputSize > > + * > > + * \var ipa_sensor_info::pixel_clock > > I forgot to mention when reviewing the CameraSensorInfo class, should > this be named pixel rate instead of pixel clock, as it's effectively the > number of pixels per second ? Some formats may require multiple clock > cycles per pixel (or transmit multiple pixels per clock cycle), so it > could make a difference. > I thought about it, it's the rate, but also the clock in Hz so it does not actually make a difference there, but yes, as the v4l2 control represent the pixel rate, which depending on the format might be different than the pixel clock (which is acutally the sample clock) we should turn this into pixel rate. > > + * \brief the pixel clock out frequency in Hz > > + * \sa libcamera::CameraSensorInfo::pixelClock > > + * > > + * \var ipa_sensor_info::line_length > > + * \brief The full line length, including blanking, in pixel units > > + * \sa libcamera::CameraSensorInfo::lineLength > > + */ > > + > > /** > > * \struct ipa_stream > > * \brief Stream information for the IPA context operations > > @@ -447,6 +506,7 @@ namespace libcamera { > > /** > > * \fn IPAInterface::configure() > > * \brief Configure the IPA stream and sensor settings > > + * \param[in] sensorInfo Camera sensor information > > * \param[in] streamConfig Configuration of all active streams > > * \param[in] entityControls Controls provided by the pipeline entities > > * > > How about adding another paragraph at the end of the documentation block > to explain this in a bit more details ? > > * The \a sensorInfo conveys information about the camera sensor settings that > * the pipeline handler has selected for the configuration. The IPA may use > * that information to tune its algorithms. Yes, I refrained to do so as streamConfig and entityControls are protocol-specific, but yes, sensorInfo is generic, so it might fit there > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index f42264361785..169492e8313f 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -822,6 +822,13 @@ int PipelineHandlerRkISP1::start(Camera *camera) > > activeCamera_ = camera; > > > > /* Inform IPA of stream configuration and sensor controls. */ > > + CameraSensorInfo sensorInfo = {}; > > + ret = data->sensor_->sensorInfo(&sensorInfo); > > + if (ret) { > > + LOG(RkISP1, Info) << "Camera sensor information not available"; > > + sensorInfo = {}; > > + } > > + > > std::map<unsigned int, IPAStream> streamConfig; > > streamConfig[0] = { > > .pixelFormat = data->stream_.configuration().pixelFormat, > > @@ -831,7 +838,7 @@ int PipelineHandlerRkISP1::start(Camera *camera) > > std::map<unsigned int, const ControlInfoMap &> entityControls; > > entityControls.emplace(0, data->sensor_->controls()); > > > > - data->ipa_->configure(streamConfig, entityControls); > > + data->ipa_->configure(sensorInfo, streamConfig, entityControls); > > > > return ret; > > } > > diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp > > index 2aa80b946704..54b1abc4727b 100644 > > --- a/src/libcamera/proxy/ipa_proxy_linux.cpp > > +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp > > @@ -10,6 +10,7 @@ > > #include <ipa/ipa_interface.h> > > #include <ipa/ipa_module_info.h> > > > > +#include "camera_sensor.h" > > #include "ipa_module.h" > > #include "ipa_proxy.h" > > #include "ipc_unixsocket.h" > > @@ -29,7 +30,8 @@ public: > > int init() override { return 0; } > > int start() override { return 0; } > > void stop() override {} > > - void configure(const std::map<unsigned int, IPAStream> &streamConfig, > > + void configure(const CameraSensorInfo &sensorInfo, > > + const std::map<unsigned int, IPAStream> &streamConfig, > > const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {} > > void mapBuffers(const std::vector<IPABuffer> &buffers) override {} > > void unmapBuffers(const std::vector<unsigned int> &ids) override {} > > diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp > > index 368ccca1cf60..b06734371b39 100644 > > --- a/src/libcamera/proxy/ipa_proxy_thread.cpp > > +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp > > @@ -10,6 +10,7 @@ > > #include <ipa/ipa_interface.h> > > #include <ipa/ipa_module_info.h> > > > > +#include "camera_sensor.h" > > #include "ipa_context_wrapper.h" > > #include "ipa_module.h" > > #include "ipa_proxy.h" > > @@ -29,7 +30,8 @@ public: > > int start() override; > > void stop() override; > > > > - void configure(const std::map<unsigned int, IPAStream> &streamConfig, > > + void configure(const CameraSensorInfo &sensorInfo, > > + const std::map<unsigned int, IPAStream> &streamConfig, > > const std::map<unsigned int, const ControlInfoMap &> &entityControls) override; > > void mapBuffers(const std::vector<IPABuffer> &buffers) override; > > void unmapBuffers(const std::vector<unsigned int> &ids) override; > > @@ -126,10 +128,11 @@ void IPAProxyThread::stop() > > thread_.wait(); > > } > > > > -void IPAProxyThread::configure(const std::map<unsigned int, IPAStream> &streamConfig, > > +void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo, > > + const std::map<unsigned int, IPAStream> &streamConfig, > > const std::map<unsigned int, const ControlInfoMap &> &entityControls) > > { > > - ipa_->configure(streamConfig, entityControls); > > + ipa_->configure(sensorInfo, streamConfig, entityControls); > > } > > > > void IPAProxyThread::mapBuffers(const std::vector<IPABuffer> &buffers) > > diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp > > index fae1d56b67c7..2cd10f981ecc 100644 > > --- a/test/ipa/ipa_wrappers_test.cpp > > +++ b/test/ipa/ipa_wrappers_test.cpp > > @@ -15,6 +15,7 @@ > > #include <libcamera/controls.h> > > #include <libipa/ipa_interface_wrapper.h> > > > > +#include "camera_sensor.h" > > #include "device_enumerator.h" > > #include "ipa_context_wrapper.h" > > #include "media_device.h" > > @@ -60,9 +61,19 @@ public: > > report(Op_stop, TestPass); > > } > > > > - void configure(const std::map<unsigned int, IPAStream> &streamConfig, > > + void configure(const CameraSensorInfo &sensorInfo, > > + const std::map<unsigned int, IPAStream> &streamConfig, > > const std::map<unsigned int, const ControlInfoMap &> &entityControls) override > > { > > + /* Verify sensorInfo. */ > > + if (sensorInfo.outputSize.width != 2560 || > > + sensorInfo.outputSize.height != 1940) { > > + cerr << "configure(): Invalid sensor info sizes: (" > > + << sensorInfo.outputSize.width << "x" > > + << sensorInfo.outputSize.height << ")" << endl; > > You can use > > cerr << "configure(): Invalid sensor info sizes: " > << sensorInfo.outputSize.toString() << > endl; > > > + return report(Op_configure, TestFail); > > + } > > + > > /* Verify streamConfig. */ > > if (streamConfig.size() != 2) { > > cerr << "configure(): Invalid number of streams " > > @@ -287,13 +298,19 @@ protected: > > int ret; > > > > /* Test configure(). */ > > + CameraSensorInfo sensorInfo = { "sensor", 8, > > s/ = // would save a copy-construction (but I expect the compiler to > optimize it out anyway). > I would expect so, I went for = as it's just nicer to me, so I can change it back and use the list initializer. > > + { 2576, 1956 }, > > + { 8, 8, 2560, 1940 }, > > + { 2560, 1940 }, > > + 96000000, > > + 2918, }; > > Would > > CameraSensorInfo sensorInfo{ > "sensor", > 8, > { 2576, 1956 }, > { 8, 8, 2560, 1940 }, > { 2560, 1940 }, > 96000000, > 2918, > }; > > be more readable ? > > As far as I can tell, clang++ supports the C99-extension designated > initializers in C++, and g++ also supports them provided the fields are > expressed in the same order in the initialization than in the > declaration (this differs from the gcc implementation where the order > isn't constrained). Should we use this to increase readability ? I refrain from expressing my thoughts on the initializer list vs designated initializer, standard versions vs compiler variations mess that C++ brought us. > > CameraSensorInfo sensorInfo{ > .name = "sensor", > .bitsPerPixel = 8, > .activeAreaSize = { 2576, 1956 }, > .analogCrop = { 8, 8, 2560, 1940 }, > .outputCrop = { 2560, 1940 }, > .pixelClock = 96000000, > .lineLength = 2918, I'm happy to use this. It reminds me of C, the happy place my mind goes to where I have C++ nightmares. Thanks j > }; > > > std::map<unsigned int, IPAStream> config{ > > { 1, { V4L2_PIX_FMT_YUYV, { 1024, 768 } } }, > > { 2, { V4L2_PIX_FMT_NV12, { 800, 600 } } }, > > }; > > std::map<unsigned int, const ControlInfoMap &> controlInfo; > > controlInfo.emplace(42, subdev_->controls()); > > - ret = INVOKE(configure, config, controlInfo); > > + ret = INVOKE(configure, sensorInfo, config, controlInfo); > > if (ret == TestFail) > > return TestFail; > > > > -- > Regards, > > Laurent Pinchart
diff --git a/include/ipa/ipa_interface.h b/include/ipa/ipa_interface.h index e65844bc7b34..a7acc09c4715 100644 --- a/include/ipa/ipa_interface.h +++ b/include/ipa/ipa_interface.h @@ -18,6 +18,22 @@ struct ipa_context { const struct ipa_context_ops *ops; }; +struct ipa_sensor_info { +#define CAMERA_SENSOR_NAME_LEN 32 + char name[CAMERA_SENSOR_NAME_LEN]; + uint8_t bits_per_pixel; + uint32_t active_area_width; + uint32_t active_area_height; + int32_t analog_crop_left; + int32_t analog_crop_top; + uint32_t analog_crop_width; + uint32_t analog_crop_height; + uint32_t output_width; + uint32_t output_height; + uint32_t pixel_clock; + uint32_t line_length; +}; + struct ipa_stream { unsigned int id; unsigned int pixel_format; @@ -70,6 +86,7 @@ struct ipa_context_ops { const struct ipa_callback_ops *callbacks, void *cb_ctx); void (*configure)(struct ipa_context *ctx, + const struct ipa_sensor_info *sensor_info, const struct ipa_stream *streams, unsigned int num_streams, const struct ipa_control_info_map *maps, @@ -96,6 +113,7 @@ struct ipa_context *ipaCreate(); #include <libcamera/geometry.h> #include <libcamera/signal.h> +#include "camera_sensor.h" #include "v4l2_controls.h" namespace libcamera { @@ -125,7 +143,8 @@ public: virtual int start() = 0; virtual void stop() = 0; - virtual void configure(const std::map<unsigned int, IPAStream> &streamConfig, + virtual void configure(const CameraSensorInfo &sensorInfo, + const std::map<unsigned int, IPAStream> &streamConfig, const std::map<unsigned int, const ControlInfoMap &> &entityControls) = 0; virtual void mapBuffers(const std::vector<IPABuffer> &buffers) = 0; diff --git a/src/ipa/libipa/ipa_interface_wrapper.cpp b/src/ipa/libipa/ipa_interface_wrapper.cpp index f50f93a0185a..049b9dd1eefc 100644 --- a/src/ipa/libipa/ipa_interface_wrapper.cpp +++ b/src/ipa/libipa/ipa_interface_wrapper.cpp @@ -15,6 +15,7 @@ #include <ipa/ipa_interface.h> #include "byte_stream_buffer.h" +#include "camera_sensor.h" /** * \file ipa_interface_wrapper.h @@ -111,6 +112,7 @@ void IPAInterfaceWrapper::register_callbacks(struct ipa_context *_ctx, } void IPAInterfaceWrapper::configure(struct ipa_context *_ctx, + const struct ipa_sensor_info *sensor_info, const struct ipa_stream *streams, unsigned int num_streams, const struct ipa_control_info_map *maps, @@ -120,6 +122,21 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx, ctx->serializer_.reset(); + /* Translate the IPA sensor info. */ + CameraSensorInfo sensorInfo{}; + sensorInfo.name = sensor_info->name; + sensorInfo.bitsPerPixel = sensor_info->bits_per_pixel; + sensorInfo.activeAreaSize = { sensor_info->active_area_width, + sensor_info->active_area_height, }; + sensorInfo.analogCrop = { sensor_info->analog_crop_left, + sensor_info->analog_crop_top, + sensor_info->analog_crop_width, + sensor_info->analog_crop_height, }; + sensorInfo.outputSize = { sensor_info->output_width, + sensor_info->output_height, }; + sensorInfo.pixelClock = sensor_info->pixel_clock; + sensorInfo.lineLength = sensor_info->line_length; + /* Translate the IPA stream configurations map. */ std::map<unsigned int, IPAStream> ipaStreams; @@ -145,7 +162,7 @@ void IPAInterfaceWrapper::configure(struct ipa_context *_ctx, entityControls.emplace(id, infoMaps[id]); } - ctx->ipa_->configure(ipaStreams, entityControls); + ctx->ipa_->configure(sensorInfo, ipaStreams, entityControls); } void IPAInterfaceWrapper::map_buffers(struct ipa_context *_ctx, diff --git a/src/ipa/libipa/ipa_interface_wrapper.h b/src/ipa/libipa/ipa_interface_wrapper.h index e4bc6dd4535d..febd6e66d0c4 100644 --- a/src/ipa/libipa/ipa_interface_wrapper.h +++ b/src/ipa/libipa/ipa_interface_wrapper.h @@ -30,6 +30,7 @@ private: const struct ipa_callback_ops *callbacks, void *cb_ctx); static void configure(struct ipa_context *ctx, + const struct ipa_sensor_info *sensor_info, const struct ipa_stream *streams, unsigned int num_streams, const struct ipa_control_info_map *maps, diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index acbbe58c7a2e..8b081359afff 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -22,6 +22,7 @@ #include <libcamera/request.h> #include <libipa/ipa_interface_wrapper.h> +#include "camera_sensor.h" #include "log.h" #include "utils.h" @@ -36,7 +37,8 @@ public: int start() override { return 0; } void stop() override {} - void configure(const std::map<unsigned int, IPAStream> &streamConfig, + void configure(const CameraSensorInfo &info, + const std::map<unsigned int, IPAStream> &streamConfig, const std::map<unsigned int, const ControlInfoMap &> &entityControls) override; void mapBuffers(const std::vector<IPABuffer> &buffers) override; void unmapBuffers(const std::vector<unsigned int> &ids) override; @@ -66,7 +68,14 @@ private: uint32_t maxGain_; }; -void IPARkISP1::configure(const std::map<unsigned int, IPAStream> &streamConfig, +/** + * \todo The RkISP1 pipeline currently provides an empty CameraSensorInfo + * if the connected sensor does not provide enough information to properly + * assemble one. Make sure the reported sensor information are relevant + * befor accessing them. + */ +void IPARkISP1::configure(const CameraSensorInfo &info, + const std::map<unsigned int, IPAStream> &streamConfig, const std::map<unsigned int, const ControlInfoMap &> &entityControls) { if (entityControls.empty()) diff --git a/src/ipa/vimc/vimc.cpp b/src/ipa/vimc/vimc.cpp index d2267e11737f..b8aadd8588cb 100644 --- a/src/ipa/vimc/vimc.cpp +++ b/src/ipa/vimc/vimc.cpp @@ -19,6 +19,7 @@ #include <libipa/ipa_interface_wrapper.h> +#include "camera_sensor.h" #include "log.h" namespace libcamera { @@ -36,7 +37,8 @@ public: int start() override; void stop() override; - void configure(const std::map<unsigned int, IPAStream> &streamConfig, + void configure(const CameraSensorInfo &sensorInfo, + const std::map<unsigned int, IPAStream> &streamConfig, const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {} void mapBuffers(const std::vector<IPABuffer> &buffers) override {} void unmapBuffers(const std::vector<unsigned int> &ids) override {} diff --git a/src/libcamera/include/ipa_context_wrapper.h b/src/libcamera/include/ipa_context_wrapper.h index 0a48bfe732c8..e11e7b6a894d 100644 --- a/src/libcamera/include/ipa_context_wrapper.h +++ b/src/libcamera/include/ipa_context_wrapper.h @@ -9,6 +9,7 @@ #include <ipa/ipa_interface.h> +#include "camera_sensor.h" #include "control_serializer.h" namespace libcamera { @@ -22,7 +23,8 @@ public: int init() override; int start() override; void stop() override; - void configure(const std::map<unsigned int, IPAStream> &streamConfig, + void configure(const CameraSensorInfo &sensorInfo, + const std::map<unsigned int, IPAStream> &streamConfig, const std::map<unsigned int, const ControlInfoMap &> &entityControls) override; void mapBuffers(const std::vector<IPABuffer> &buffers) override; diff --git a/src/libcamera/ipa_context_wrapper.cpp b/src/libcamera/ipa_context_wrapper.cpp index ab6ce396b88a..b1ff83aa8be8 100644 --- a/src/libcamera/ipa_context_wrapper.cpp +++ b/src/libcamera/ipa_context_wrapper.cpp @@ -12,6 +12,7 @@ #include <libcamera/controls.h> #include "byte_stream_buffer.h" +#include "camera_sensor.h" #include "utils.h" /** @@ -104,17 +105,34 @@ void IPAContextWrapper::stop() ctx_->ops->stop(ctx_); } -void IPAContextWrapper::configure(const std::map<unsigned int, IPAStream> &streamConfig, +void IPAContextWrapper::configure(const CameraSensorInfo &sensorInfo, + const std::map<unsigned int, IPAStream> &streamConfig, const std::map<unsigned int, const ControlInfoMap &> &entityControls) { if (intf_) - return intf_->configure(streamConfig, entityControls); + return intf_->configure(sensorInfo, streamConfig, entityControls); if (!ctx_) return; serializer_.reset(); + /* Translate the camera sensor info. */ + struct ipa_sensor_info sensor_info = {}; + strncpy(sensor_info.name, sensorInfo.name.c_str(), + sensorInfo.name.length()); + sensor_info.bits_per_pixel = sensorInfo.bitsPerPixel; + sensor_info.active_area_width = sensorInfo.activeAreaSize.width; + sensor_info.active_area_height = sensorInfo.activeAreaSize.height; + sensor_info.analog_crop_left = sensorInfo.analogCrop.x; + sensor_info.analog_crop_top = sensorInfo.analogCrop.y; + sensor_info.analog_crop_width = sensorInfo.analogCrop.width; + sensor_info.analog_crop_height = sensorInfo.analogCrop.height; + sensor_info.output_width = sensorInfo.outputSize.width; + sensor_info.output_height = sensorInfo.outputSize.height; + sensor_info.pixel_clock = sensorInfo.pixelClock; + sensor_info.line_length = sensorInfo.lineLength; + /* Translate the IPA stream configurations map. */ struct ipa_stream c_streams[streamConfig.size()]; @@ -154,7 +172,7 @@ void IPAContextWrapper::configure(const std::map<unsigned int, IPAStream> &strea ++i; } - ctx_->ops->configure(ctx_, c_streams, streamConfig.size(), + ctx_->ops->configure(ctx_, &sensor_info, c_streams, streamConfig.size(), c_info_maps, entityControls.size()); } diff --git a/src/libcamera/ipa_interface.cpp b/src/libcamera/ipa_interface.cpp index 890d4340e4f3..22a3567bc1d0 100644 --- a/src/libcamera/ipa_interface.cpp +++ b/src/libcamera/ipa_interface.cpp @@ -92,6 +92,65 @@ * \brief The IPA context operations */ +/** + * \struct ipa_sensor_info + * \brief Camera sensor information for the IPA context operations + * \sa libcamera::CameraSensorInfo + * + * \def CAMERA_SENSOR_NAME_LEN + * \brief The camera sensor name maximum length + * + * \var ipa_sensor_info::name + * \brief The camera sensor name + * \todo Remove this field as soon as no IPA depends on it anymore + * + * \var ipa_sensor_info::bits_per_pixel + * \brief The camera sensor image format bit depth + * \sa libcamera::CameraSensorInfo::bitsPerPixel + * + * \var ipa_sensor_info::active_area_width + * \brief The camera sensor pixel array active area width + * \sa libcamera::CameraSensorInfo::activeAreaSize + * + * \var ipa_sensor_info::active_area_height + * \brief The camera sensor pixel array active area height + * \sa libcamera::CameraSensorInfo::activeAreaSize + * + * \var ipa_sensor_info::analog_crop_left + * \brief The horizontal displacement from the active area top-left corner of + * the cropped portion of the pixel array matrix + * \sa libcamera::CameraSensorInfo::analogCrop + * + * \var ipa_sensor_info::analog_crop_top + * \brief The vertical displacement from the active area top-left corner of + * the cropped portion of the pixel array matrix + * \sa libcamera::CameraSensorInfo::analogCrop + * + * \var ipa_sensor_info::analog_crop_width + * \brief The horizontal size of the cropped portion of the pixel array matrix + * \sa libcamera::CameraSensorInfo::analogCrop + * + * \var ipa_sensor_info::analog_crop_height + * \brief The vertical size of the cropped portion of the pixel array matrix + * \sa libcamera::CameraSensorInfo::analogCrop + * + * \var ipa_sensor_info::output_width + * \brief The horizontal size of the output image + * \sa libcamera::CameraSensorInfo::outputSize + * + * \var ipa_sensor_info::output_height + * \brief The vertical size of the output image + * \sa libcamera::CameraSensorInfo::outputSize + * + * \var ipa_sensor_info::pixel_clock + * \brief the pixel clock out frequency in Hz + * \sa libcamera::CameraSensorInfo::pixelClock + * + * \var ipa_sensor_info::line_length + * \brief The full line length, including blanking, in pixel units + * \sa libcamera::CameraSensorInfo::lineLength + */ + /** * \struct ipa_stream * \brief Stream information for the IPA context operations @@ -447,6 +506,7 @@ namespace libcamera { /** * \fn IPAInterface::configure() * \brief Configure the IPA stream and sensor settings + * \param[in] sensorInfo Camera sensor information * \param[in] streamConfig Configuration of all active streams * \param[in] entityControls Controls provided by the pipeline entities * diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index f42264361785..169492e8313f 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -822,6 +822,13 @@ int PipelineHandlerRkISP1::start(Camera *camera) activeCamera_ = camera; /* Inform IPA of stream configuration and sensor controls. */ + CameraSensorInfo sensorInfo = {}; + ret = data->sensor_->sensorInfo(&sensorInfo); + if (ret) { + LOG(RkISP1, Info) << "Camera sensor information not available"; + sensorInfo = {}; + } + std::map<unsigned int, IPAStream> streamConfig; streamConfig[0] = { .pixelFormat = data->stream_.configuration().pixelFormat, @@ -831,7 +838,7 @@ int PipelineHandlerRkISP1::start(Camera *camera) std::map<unsigned int, const ControlInfoMap &> entityControls; entityControls.emplace(0, data->sensor_->controls()); - data->ipa_->configure(streamConfig, entityControls); + data->ipa_->configure(sensorInfo, streamConfig, entityControls); return ret; } diff --git a/src/libcamera/proxy/ipa_proxy_linux.cpp b/src/libcamera/proxy/ipa_proxy_linux.cpp index 2aa80b946704..54b1abc4727b 100644 --- a/src/libcamera/proxy/ipa_proxy_linux.cpp +++ b/src/libcamera/proxy/ipa_proxy_linux.cpp @@ -10,6 +10,7 @@ #include <ipa/ipa_interface.h> #include <ipa/ipa_module_info.h> +#include "camera_sensor.h" #include "ipa_module.h" #include "ipa_proxy.h" #include "ipc_unixsocket.h" @@ -29,7 +30,8 @@ public: int init() override { return 0; } int start() override { return 0; } void stop() override {} - void configure(const std::map<unsigned int, IPAStream> &streamConfig, + void configure(const CameraSensorInfo &sensorInfo, + const std::map<unsigned int, IPAStream> &streamConfig, const std::map<unsigned int, const ControlInfoMap &> &entityControls) override {} void mapBuffers(const std::vector<IPABuffer> &buffers) override {} void unmapBuffers(const std::vector<unsigned int> &ids) override {} diff --git a/src/libcamera/proxy/ipa_proxy_thread.cpp b/src/libcamera/proxy/ipa_proxy_thread.cpp index 368ccca1cf60..b06734371b39 100644 --- a/src/libcamera/proxy/ipa_proxy_thread.cpp +++ b/src/libcamera/proxy/ipa_proxy_thread.cpp @@ -10,6 +10,7 @@ #include <ipa/ipa_interface.h> #include <ipa/ipa_module_info.h> +#include "camera_sensor.h" #include "ipa_context_wrapper.h" #include "ipa_module.h" #include "ipa_proxy.h" @@ -29,7 +30,8 @@ public: int start() override; void stop() override; - void configure(const std::map<unsigned int, IPAStream> &streamConfig, + void configure(const CameraSensorInfo &sensorInfo, + const std::map<unsigned int, IPAStream> &streamConfig, const std::map<unsigned int, const ControlInfoMap &> &entityControls) override; void mapBuffers(const std::vector<IPABuffer> &buffers) override; void unmapBuffers(const std::vector<unsigned int> &ids) override; @@ -126,10 +128,11 @@ void IPAProxyThread::stop() thread_.wait(); } -void IPAProxyThread::configure(const std::map<unsigned int, IPAStream> &streamConfig, +void IPAProxyThread::configure(const CameraSensorInfo &sensorInfo, + const std::map<unsigned int, IPAStream> &streamConfig, const std::map<unsigned int, const ControlInfoMap &> &entityControls) { - ipa_->configure(streamConfig, entityControls); + ipa_->configure(sensorInfo, streamConfig, entityControls); } void IPAProxyThread::mapBuffers(const std::vector<IPABuffer> &buffers) diff --git a/test/ipa/ipa_wrappers_test.cpp b/test/ipa/ipa_wrappers_test.cpp index fae1d56b67c7..2cd10f981ecc 100644 --- a/test/ipa/ipa_wrappers_test.cpp +++ b/test/ipa/ipa_wrappers_test.cpp @@ -15,6 +15,7 @@ #include <libcamera/controls.h> #include <libipa/ipa_interface_wrapper.h> +#include "camera_sensor.h" #include "device_enumerator.h" #include "ipa_context_wrapper.h" #include "media_device.h" @@ -60,9 +61,19 @@ public: report(Op_stop, TestPass); } - void configure(const std::map<unsigned int, IPAStream> &streamConfig, + void configure(const CameraSensorInfo &sensorInfo, + const std::map<unsigned int, IPAStream> &streamConfig, const std::map<unsigned int, const ControlInfoMap &> &entityControls) override { + /* Verify sensorInfo. */ + if (sensorInfo.outputSize.width != 2560 || + sensorInfo.outputSize.height != 1940) { + cerr << "configure(): Invalid sensor info sizes: (" + << sensorInfo.outputSize.width << "x" + << sensorInfo.outputSize.height << ")" << endl; + return report(Op_configure, TestFail); + } + /* Verify streamConfig. */ if (streamConfig.size() != 2) { cerr << "configure(): Invalid number of streams " @@ -287,13 +298,19 @@ protected: int ret; /* Test configure(). */ + CameraSensorInfo sensorInfo = { "sensor", 8, + { 2576, 1956 }, + { 8, 8, 2560, 1940 }, + { 2560, 1940 }, + 96000000, + 2918, }; std::map<unsigned int, IPAStream> config{ { 1, { V4L2_PIX_FMT_YUYV, { 1024, 768 } } }, { 2, { V4L2_PIX_FMT_NV12, { 800, 600 } } }, }; std::map<unsigned int, const ControlInfoMap &> controlInfo; controlInfo.emplace(42, subdev_->controls()); - ret = INVOKE(configure, config, controlInfo); + ret = INVOKE(configure, sensorInfo, config, controlInfo); if (ret == TestFail) return TestFail;
Add support for camera sensor information in the libcamera IPA protocol. Define a new 'struct ipa_sensor_info' structure in the IPA context and use it to perform translation between the C and the C++ API. Update the IPAInterface::configure() operation to accept a new CameraSensorInfo parameter and port all users of that function to the new interface. Signed-off-by: Jacopo Mondi <jacopo@jmondi.org> --- include/ipa/ipa_interface.h | 21 +++++++- src/ipa/libipa/ipa_interface_wrapper.cpp | 19 ++++++- src/ipa/libipa/ipa_interface_wrapper.h | 1 + src/ipa/rkisp1/rkisp1.cpp | 13 ++++- src/ipa/vimc/vimc.cpp | 4 +- src/libcamera/include/ipa_context_wrapper.h | 4 +- src/libcamera/ipa_context_wrapper.cpp | 24 +++++++-- src/libcamera/ipa_interface.cpp | 60 +++++++++++++++++++++ src/libcamera/pipeline/rkisp1/rkisp1.cpp | 9 +++- src/libcamera/proxy/ipa_proxy_linux.cpp | 4 +- src/libcamera/proxy/ipa_proxy_thread.cpp | 9 ++-- test/ipa/ipa_wrappers_test.cpp | 21 +++++++- 12 files changed, 173 insertions(+), 16 deletions(-)