[libcamera-devel] [PATCH 2/5] libcamera: raspberrypi: Apply transform and pass through to IPA
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Wed Jul 29 17:18:40 CEST 2020
Hi David,
Thank you for the patch.
On Wed, Jul 29, 2020 at 10:31:51AM +0100, David Plowman wrote:
> The user-supplied transform is applied to the camera, taking account
> of any pre-existing rotation. It is then also plumbed through to the
> IPA so that it will (in a further commit) be able to make use of
> it.
> ---
> include/libcamera/ipa/raspberrypi.h | 1 +
> src/ipa/raspberrypi/raspberrypi.cpp | 13 ++++++++-
> .../pipeline/raspberrypi/raspberrypi.cpp | 27 +++++++++++++++++--
> 3 files changed, 38 insertions(+), 3 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index ca62990..3905a4a 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -14,6 +14,7 @@ enum RPiConfigParameters {
> RPI_IPA_CONFIG_LS_TABLE = (1 << 0),
> RPI_IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
> RPI_IPA_CONFIG_SENSOR = (1 << 2),
> + RPI_IPA_CONFIG_TRANSFORM = (1 << 3),
I would drop this flag and always pass the transform value. You could
also store it first to avoid the next_element variable below.
> };
>
> enum RPiOperations {
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 3747208..2809521 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -21,6 +21,7 @@
> #include <libcamera/ipa/raspberrypi.h>
> #include <libcamera/request.h>
> #include <libcamera/span.h>
> +#include <libcamera/transform.h>
>
> #include <libipa/ipa_interface_wrapper.h>
>
> @@ -144,6 +145,9 @@ private:
> /* LS table allocation passed in from the pipeline handler. */
> FileDescriptor lsTableHandle_;
> void *lsTable_;
> +
> + /* This is the transform requested by the user/application driving libcamera. */
> + Transform userTransform_;
> };
>
> int IPARPi::init(const IPASettings &settings)
> @@ -282,6 +286,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> lastMode_ = mode_;
>
> /* Store the lens shading table pointer and handle if available. */
> + unsigned int next_element = 0;
> if (ipaConfig.operation & RPI_IPA_CONFIG_LS_TABLE) {
> /* Remove any previous table, if there was one. */
> if (lsTable_) {
> @@ -290,7 +295,7 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> }
>
> /* Map the LS table buffer into user space. */
> - lsTableHandle_ = FileDescriptor(ipaConfig.data[0]);
> + lsTableHandle_ = FileDescriptor(ipaConfig.data[next_element++]);
> if (lsTableHandle_.isValid()) {
> lsTable_ = mmap(nullptr, MAX_LS_GRID_SIZE, PROT_READ | PROT_WRITE,
> MAP_SHARED, lsTableHandle_.fd(), 0);
> @@ -301,6 +306,12 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
> }
> }
> }
> +
> + /* Fish out any transform set by the user/application. */
> + if (ipaConfig.operation & RPI_IPA_CONFIG_TRANSFORM) {
> + uint32_t transformType = ipaConfig.data[next_element++];
> + userTransform_ = reinterpret_cast<Transform &>(transformType);
I think it would be best to just make the constructor that takes a
Transform::Type public.
> + }
> }
>
> void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 82a0a4d..5fb427a 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -324,6 +324,9 @@ public:
> uint32_t expectedSequence_;
> bool sensorMetadata_;
>
> + /* This is the transform requested by the user/application driving libcamera. */
Could you wrap this line at 80 columns ?
> + Transform userTransform_;
> +
> /*
> * All the functions in this class are called from a single calling
> * thread. So, we do not need to have any mutex to protect access to any
> @@ -400,6 +403,9 @@ CameraConfiguration::Status RPiCameraConfiguration::validate()
> if (config_.empty())
> return Invalid;
>
> + if (userTransform.contains(Transform::transpose()))
> + return Invalid;
Shouldn't you adjust it instead ?
> +
> unsigned int rawCount = 0, outCount = 0, count = 0, maxIndex = 0;
> std::pair<int, Size> outSize[2];
> Size maxSize;
> @@ -609,6 +615,9 @@ int PipelineHandlerRPi::configure(Camera *camera, CameraConfiguration *config)
> unsigned int maxIndex = 0;
> bool rawStream = false;
>
> + /* Record the transform requested by the application. */
> + data->userTransform_ = config->userTransform;
> +
> /*
> * Look for the RAW stream (if given) size as well as the largest
> * ISP output size.
> @@ -1140,6 +1149,11 @@ int RPiCameraData::configureIPA()
> ipaConfig.data = { static_cast<unsigned int>(lsTable_.fd()) };
> }
>
> + /* We must pass the user transform to the IPA too. */
> + uint32_t transformType = reinterpret_cast<uint32_t &>(userTransform_);
> + ipaConfig.operation |= RPI_IPA_CONFIG_TRANSFORM;
> + ipaConfig.data.push_back(transformType);
> +
> CameraSensorInfo sensorInfo = {};
> int ret = sensor_->sensorInfo(&sensorInfo);
> if (ret) {
> @@ -1168,8 +1182,17 @@ int RPiCameraData::configureIPA()
> /* Configure the H/V flip controls based on the sensor rotation. */
> ControlList ctrls(unicam_[Unicam::Image].dev()->controls());
> int32_t rotation = sensor_->properties().get(properties::Rotation);
> - ctrls.set(V4L2_CID_HFLIP, static_cast<int32_t>(!!rotation));
> - ctrls.set(V4L2_CID_VFLIP, static_cast<int32_t>(!!rotation));
> +
> + /* The camera needs to compose the user transform with the rotation. */
> + Transform rotationTransform;
> + if (Transform::rotation(rotation, rotationTransform) == false)
> + return -EINVAL;
> + Transform transform = userTransform_ * rotationTransform;
> +
> + ctrls.set(V4L2_CID_HFLIP,
> + static_cast<int32_t>(transform.contains(Transform::hflip())));
> + ctrls.set(V4L2_CID_VFLIP,
> + static_cast<int32_t>(transform.contains(Transform::vflip())));
> unicam_[Unicam::Image].dev()->setControls(&ctrls);
> }
>
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list