From patchwork Mon Feb 8 22:54:29 2021 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Laurent Pinchart X-Patchwork-Id: 11199 X-Patchwork-Delegate: laurent.pinchart@ideasonboard.com Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 084BABD160 for ; Mon, 8 Feb 2021 22:55:01 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id CABB96160E; Mon, 8 Feb 2021 23:55:00 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="pFErNuh2"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [213.167.242.64]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id A4BF360D37 for ; Mon, 8 Feb 2021 23:54:57 +0100 (CET) Received: from pendragon.lan (62-78-145-57.bb.dnainternet.fi [62.78.145.57]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id 404903D7 for ; Mon, 8 Feb 2021 23:54:57 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1612824897; bh=VzCdV7ikEmrOe0+22ZFpHP/iKAuQTiPigwAc3Ql+BFQ=; h=From:To:Subject:Date:In-Reply-To:References:From; b=pFErNuh2F/79r0TP2u2bresdfGrDdsEn2p9FWi+ucEWwjlgvnjg5ymFRIQYHB3Plu ec1rvzEM9PjxFx3O6cfPYfkSpZPd+9Ydis6zaSOz3AlpH20w6i6q1EXgJ7aE/Hq0lP pC7JV/XK9R3EvXhA/W80cCc0cYmH0t/s5VhqlFOM= From: Laurent Pinchart To: libcamera-devel@lists.libcamera.org Date: Tue, 9 Feb 2021 00:54:29 +0200 Message-Id: <20210208225429.31627-3-laurent.pinchart@ideasonboard.com> X-Mailer: git-send-email 2.28.0 In-Reply-To: <20210208225429.31627-1-laurent.pinchart@ideasonboard.com> References: <20210208225429.31627-1-laurent.pinchart@ideasonboard.com> MIME-Version: 1.0 Subject: [libcamera-devel] [PATCH/RFC 2/2] libcamera: Use V4L2 Control instances X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" Use the V4L2 Control instances instead of the numerical V4L2_CID_* identifiers where possible. This simplify usage of the ControlList API in several places, and allows printing control names in the CameraSensor class and the RaspberryPi IPA. There are still locations where the numerical identifiers are used: - In the IPU3 and RaspberryPi code, for custom controls not defined in linux/v4l2-controls.h, which can be fixed by adding additional device-specific Control instances. - In the DelayedControls and V4L2Device controls APIs, which we could move to ControlId if desired. - In the uvcvideo and vimc pipeline handlers, when translating between V4L2 and libcamera controls, for switch/case or for code that operate on different controls in a generic way. This would be more difficult to replace. Further improvements to the controls API may be possible on top of this, such as dropping the ID-based ControlList::get() and ControlList::set() functions (at possibly replacing them with a version that takes a ControlId pointer). Another possibly interesting improvement would be to add a lookup API to the ControlInfoMap class that would take a Control reference, and return a type-aware wrapper around ControlInfo to avoid having to deal manually with ControlValue. Signed-off-by: Laurent Pinchart Acked-by: Kieran Bingham Reviewed-by: Kieran Bingham --- src/ipa/ipu3/ipu3.cpp | 10 +++--- src/ipa/raspberrypi/raspberrypi.cpp | 25 ++++++------- src/ipa/rkisp1/rkisp1.cpp | 10 +++--- src/libcamera/camera_sensor.cpp | 36 +++++++++---------- src/libcamera/pipeline/ipu3/ipu3.cpp | 2 +- .../pipeline/raspberrypi/raspberrypi.cpp | 16 ++++----- src/libcamera/pipeline/uvcvideo/uvcvideo.cpp | 29 +++++++-------- src/libcamera/pipeline/vimc/vimc.cpp | 10 +++--- 8 files changed, 68 insertions(+), 70 deletions(-) diff --git a/src/ipa/ipu3/ipu3.cpp b/src/ipa/ipu3/ipu3.cpp index b11b03efa6ce..5ee3fd302e01 100644 --- a/src/ipa/ipu3/ipu3.cpp +++ b/src/ipa/ipu3/ipu3.cpp @@ -11,7 +11,6 @@ #include #include -#include #include #include @@ -23,6 +22,7 @@ #include "libcamera/internal/buffer.h" #include "libcamera/internal/log.h" +#include "libcamera/internal/v4l2_controls.h" namespace libcamera { @@ -80,13 +80,13 @@ void IPAIPU3::configure([[maybe_unused]] const CameraSensorInfo &info, ctrls_ = entityControls.at(0); - const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE); + const auto itExp = ctrls_.find(&v4l2::EXPOSURE); if (itExp == ctrls_.end()) { LOG(IPAIPU3, Error) << "Can't find exposure control"; return; } - const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN); + const auto itGain = ctrls_.find(&v4l2::ANALOGUE_GAIN); if (itGain == ctrls_.end()) { LOG(IPAIPU3, Error) << "Can't find gain control"; return; @@ -214,8 +214,8 @@ void IPAIPU3::setControls(unsigned int frame) op.operation = IPU3_IPA_ACTION_SET_SENSOR_CONTROLS; ControlList ctrls(ctrls_); - ctrls.set(V4L2_CID_EXPOSURE, static_cast(exposure_)); - ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain_)); + ctrls.set(v4l2::EXPOSURE, exposure_); + ctrls.set(v4l2::ANALOGUE_GAIN, gain_); op.controls.push_back(ctrls); queueFrameAction.emit(frame, op); diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp index ff14cfc4b706..897f3f0b91f1 100644 --- a/src/ipa/raspberrypi/raspberrypi.cpp +++ b/src/ipa/raspberrypi/raspberrypi.cpp @@ -542,16 +542,16 @@ void IPARPi::reportMetadata() bool IPARPi::validateSensorControls() { - static const uint32_t ctrls[] = { - V4L2_CID_ANALOGUE_GAIN, - V4L2_CID_EXPOSURE, - V4L2_CID_VBLANK, + static const ControlId *const ctrls[] = { + &v4l2::ANALOGUE_GAIN, + &v4l2::EXPOSURE, + &v4l2::VBLANK, }; for (auto c : ctrls) { if (sensorCtrls_.find(c) == sensorCtrls_.end()) { LOG(IPARPI, Error) << "Unable to find sensor control " - << utils::hex(c); + << c->name(); return false; } } @@ -1038,10 +1038,8 @@ void IPARPi::applyAWB(const struct AwbStatus *awbStatus, ControlList &ctrls) LOG(IPARPI, Debug) << "Applying WB R: " << awbStatus->gain_r << " B: " << awbStatus->gain_b; - ctrls.set(V4L2_CID_RED_BALANCE, - static_cast(awbStatus->gain_r * 1000)); - ctrls.set(V4L2_CID_BLUE_BALANCE, - static_cast(awbStatus->gain_b * 1000)); + ctrls.set(v4l2::RED_BALANCE, awbStatus->gain_r * 1000); + ctrls.set(v4l2::BLUE_BALANCE, awbStatus->gain_b * 1000); } void IPARPi::applyFrameDurations(double minFrameDuration, double maxFrameDuration) @@ -1100,15 +1098,14 @@ void IPARPi::applyAGC(const struct AgcStatus *agcStatus, ControlList &ctrls) * exposure time without us knowing. The next time though this function should * clip exposure correctly. */ - ctrls.set(V4L2_CID_VBLANK, vblanking); - ctrls.set(V4L2_CID_EXPOSURE, exposureLines); - ctrls.set(V4L2_CID_ANALOGUE_GAIN, gainCode); + ctrls.set(v4l2::VBLANK, vblanking); + ctrls.set(v4l2::EXPOSURE, exposureLines); + ctrls.set(v4l2::ANALOGUE_GAIN, gainCode); } void IPARPi::applyDG(const struct AgcStatus *dgStatus, ControlList &ctrls) { - ctrls.set(V4L2_CID_DIGITAL_GAIN, - static_cast(dgStatus->digital_gain * 1000)); + ctrls.set(v4l2::DIGITAL_GAIN, dgStatus->digital_gain * 1000); } void IPARPi::applyCCM(const struct CcmStatus *ccmStatus, ControlList &ctrls) diff --git a/src/ipa/rkisp1/rkisp1.cpp b/src/ipa/rkisp1/rkisp1.cpp index 39783abd731b..ce47ba8cb38a 100644 --- a/src/ipa/rkisp1/rkisp1.cpp +++ b/src/ipa/rkisp1/rkisp1.cpp @@ -13,7 +13,6 @@ #include #include -#include #include #include @@ -25,6 +24,7 @@ #include #include "libcamera/internal/log.h" +#include "libcamera/internal/v4l2_controls.h" namespace libcamera { @@ -91,13 +91,13 @@ void IPARkISP1::configure([[maybe_unused]] const CameraSensorInfo &info, ctrls_ = entityControls.at(0); - const auto itExp = ctrls_.find(V4L2_CID_EXPOSURE); + const auto itExp = ctrls_.find(&v4l2::EXPOSURE); if (itExp == ctrls_.end()) { LOG(IPARkISP1, Error) << "Can't find exposure control"; return; } - const auto itGain = ctrls_.find(V4L2_CID_ANALOGUE_GAIN); + const auto itGain = ctrls_.find(&v4l2::ANALOGUE_GAIN); if (itGain == ctrls_.end()) { LOG(IPARkISP1, Error) << "Can't find gain control"; return; @@ -261,8 +261,8 @@ void IPARkISP1::setControls(unsigned int frame) op.operation = RKISP1_IPA_ACTION_V4L2_SET; ControlList ctrls(ctrls_); - ctrls.set(V4L2_CID_EXPOSURE, static_cast(exposure_)); - ctrls.set(V4L2_CID_ANALOGUE_GAIN, static_cast(gain_)); + ctrls.set(v4l2::EXPOSURE, exposure_); + ctrls.set(v4l2::ANALOGUE_GAIN, gain_); op.controls.push_back(ctrls); queueFrameAction.emit(frame, op); diff --git a/src/libcamera/camera_sensor.cpp b/src/libcamera/camera_sensor.cpp index c9e8d49b7935..eb96c4841708 100644 --- a/src/libcamera/camera_sensor.cpp +++ b/src/libcamera/camera_sensor.cpp @@ -280,16 +280,16 @@ int CameraSensor::validateSensorDriver() * Optional controls are used to register optional sensor properties. If * not present, some values will be defaulted. */ - static constexpr uint32_t optionalControls[] = { - V4L2_CID_CAMERA_ORIENTATION, - V4L2_CID_CAMERA_SENSOR_ROTATION, + static const ControlId *const optionalControls[] = { + &v4l2::CAMERA_ORIENTATION, + &v4l2::CAMERA_SENSOR_ROTATION, }; - const ControlIdMap &controls = subdev_->controls().idmap(); - for (uint32_t ctrl : optionalControls) { + const ControlInfoMap &controls = subdev_->controls(); + for (const ControlId *ctrl : optionalControls) { if (!controls.count(ctrl)) LOG(CameraSensor, Debug) - << "Optional V4L2 control " << utils::hex(ctrl) + << "Optional V4L2 control " << ctrl->name() << " not supported"; } @@ -346,18 +346,18 @@ int CameraSensor::validateSensorDriver() * For raw sensors, make sure the sensor driver supports the controls * required by the CameraSensor class. */ - static constexpr uint32_t mandatoryControls[] = { - V4L2_CID_EXPOSURE, - V4L2_CID_HBLANK, - V4L2_CID_PIXEL_RATE, - V4L2_CID_VBLANK, + static const ControlId *const mandatoryControls[] = { + &v4l2::EXPOSURE, + &v4l2::HBLANK, + &v4l2::PIXEL_RATE, + &v4l2::VBLANK, }; err = 0; - for (uint32_t ctrl : mandatoryControls) { + for (const ControlId *ctrl : mandatoryControls) { if (!controls.count(ctrl)) { LOG(CameraSensor, Error) - << "Mandatory V4L2 control " << utils::hex(ctrl) + << "Mandatory V4L2 control " << ctrl->name() << " not available"; err = -EINVAL; } @@ -425,7 +425,7 @@ int CameraSensor::initProperties() const ControlInfoMap &controls = subdev_->controls(); int32_t propertyValue; - const auto &orientation = controls.find(V4L2_CID_CAMERA_ORIENTATION); + const auto &orientation = controls.find(&v4l2::CAMERA_ORIENTATION); if (orientation != controls.end()) { int32_t v4l2Orientation = orientation->second.def().get(); @@ -450,7 +450,7 @@ int CameraSensor::initProperties() } properties_.set(properties::Location, propertyValue); - const auto &rotationControl = controls.find(V4L2_CID_CAMERA_SENSOR_ROTATION); + const auto &rotationControl = controls.find(&v4l2::CAMERA_SENSOR_ROTATION); if (rotationControl != controls.end()) { propertyValue = rotationControl->second.def().get(); properties_.set(properties::Rotation, propertyValue); @@ -775,11 +775,11 @@ int CameraSensor::sensorInfo(CameraSensorInfo *info) const return -EINVAL; } - int32_t hblank = ctrls.get(V4L2_CID_HBLANK).get(); + int32_t hblank = ctrls.get(v4l2::HBLANK); info->lineLength = info->outputSize.width + hblank; - info->pixelRate = ctrls.get(V4L2_CID_PIXEL_RATE).get(); + info->pixelRate = ctrls.get(v4l2::PIXEL_RATE); - const ControlInfo vblank = ctrls.infoMap()->at(V4L2_CID_VBLANK); + const ControlInfo vblank = ctrls.infoMap()->at(&v4l2::VBLANK); info->minFrameLength = info->outputSize.height + vblank.min().get(); info->maxFrameLength = info->outputSize.height + vblank.max().get(); diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 9bc3df3352fd..d3fc51518598 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -845,7 +845,7 @@ int PipelineHandlerIPU3::initControls(IPU3CameraData *data) double lineDuration = sensorInfo.lineLength / (sensorInfo.pixelRate / 1e6); const ControlInfoMap &sensorControls = sensor->controls(); - const ControlInfo &v4l2Exposure = sensorControls.find(V4L2_CID_EXPOSURE)->second; + const ControlInfo &v4l2Exposure = sensorControls.find(&v4l2::EXPOSURE)->second; int32_t minExposure = v4l2Exposure.min().get() * lineDuration; int32_t maxExposure = v4l2Exposure.max().get() * lineDuration; int32_t defExposure = v4l2Exposure.def().get() * lineDuration; diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp index 0804a4393c19..90f846530346 100644 --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp @@ -992,8 +992,8 @@ bool PipelineHandlerRPi::match(DeviceEnumerator *enumerator) data->flipsAlterBayerOrder_ = hflipCtrl->flags & V4L2_CTRL_FLAG_MODIFY_LAYOUT; ControlList ctrls(dev->controls()); - ctrls.set(V4L2_CID_HFLIP, 0); - ctrls.set(V4L2_CID_VFLIP, 0); + ctrls.set(v4l2::HFLIP, 0); + ctrls.set(v4l2::VFLIP, 0); dev->setControls(&ctrls); } @@ -1246,10 +1246,10 @@ int RPiCameraData::configureIPA(const CameraConfiguration *config) */ if (supportsFlips_) { ControlList ctrls(unicam_[Unicam::Image].dev()->controls()); - ctrls.set(V4L2_CID_HFLIP, - static_cast(!!(rpiConfig->combinedTransform_ & Transform::HFlip))); - ctrls.set(V4L2_CID_VFLIP, - static_cast(!!(rpiConfig->combinedTransform_ & Transform::VFlip))); + ctrls.set(v4l2::HFLIP, + !!(rpiConfig->combinedTransform_ & Transform::HFlip)); + ctrls.set(v4l2::VFLIP, + !!(rpiConfig->combinedTransform_ & Transform::VFlip)); unicam_[Unicam::Image].dev()->setControls(&ctrls); } @@ -1383,8 +1383,8 @@ void RPiCameraData::unicamBufferDequeue(FrameBuffer *buffer) auto it = mappedEmbeddedBuffers_.find(bufferId); if (it != mappedEmbeddedBuffers_.end()) { uint32_t *mem = reinterpret_cast(it->second.maps()[0].data()); - mem[0] = ctrl.get(V4L2_CID_EXPOSURE).get(); - mem[1] = ctrl.get(V4L2_CID_ANALOGUE_GAIN).get(); + mem[0] = ctrl.get(v4l2::EXPOSURE); + mem[1] = ctrl.get(v4l2::ANALOGUE_GAIN); } else { LOG(RPI, Warning) << "Failed to find embedded buffer " << bufferId; diff --git a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp index 08a594175091..a80b51f174a9 100644 --- a/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp +++ b/src/libcamera/pipeline/uvcvideo/uvcvideo.cpp @@ -260,20 +260,20 @@ void PipelineHandlerUVC::stop(Camera *camera) int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id, const ControlValue &value) { - uint32_t cid; + const ControlId *cid; if (id == controls::Brightness) - cid = V4L2_CID_BRIGHTNESS; + cid = &v4l2::BRIGHTNESS; else if (id == controls::Contrast) - cid = V4L2_CID_CONTRAST; + cid = &v4l2::CONTRAST; else if (id == controls::Saturation) - cid = V4L2_CID_SATURATION; + cid = &v4l2::SATURATION; else if (id == controls::AeEnable) - cid = V4L2_CID_EXPOSURE_AUTO; + cid = &v4l2::EXPOSURE_AUTO; else if (id == controls::ExposureTime) - cid = V4L2_CID_EXPOSURE_ABSOLUTE; + cid = &v4l2::EXPOSURE_ABSOLUTE; else if (id == controls::AnalogueGain) - cid = V4L2_CID_GAIN; + cid = &v4l2::GAIN; else return -EINVAL; @@ -286,18 +286,18 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id, * See UVCCameraData::addControl() for explanations of the different * value mappings. */ - switch (cid) { + switch (cid->id()) { case V4L2_CID_BRIGHTNESS: { float scale = std::max(max - def, def - min); float fvalue = value.get() * scale + def; - controls->set(cid, static_cast(lroundf(fvalue))); + controls->set(v4l2::BRIGHTNESS, lroundf(fvalue)); break; } case V4L2_CID_SATURATION: { float scale = def - min; float fvalue = value.get() * scale + min; - controls->set(cid, static_cast(lroundf(fvalue))); + controls->set(v4l2::SATURATION, lroundf(fvalue)); break; } @@ -305,12 +305,13 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id, int32_t ivalue = value.get() ? V4L2_EXPOSURE_APERTURE_PRIORITY : V4L2_EXPOSURE_MANUAL; - controls->set(V4L2_CID_EXPOSURE_AUTO, ivalue); + controls->set(v4l2::EXPOSURE_AUTO, ivalue); break; } case V4L2_CID_EXPOSURE_ABSOLUTE: - controls->set(cid, value.get() / 100); + controls->set(v4l2::EXPOSURE_ABSOLUTE, + value.get() / 100); break; case V4L2_CID_CONTRAST: @@ -324,13 +325,13 @@ int PipelineHandlerUVC::processControl(ControlList *controls, unsigned int id, } float fvalue = (value.get() - p) / m; - controls->set(cid, static_cast(lroundf(fvalue))); + controls->set(cid->id(), static_cast(lroundf(fvalue))); break; } default: { int32_t ivalue = value.get(); - controls->set(cid, ivalue); + controls->set(cid->id(), ivalue); break; } } diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp index 36325ffbbd7d..24b39b7dd879 100644 --- a/src/libcamera/pipeline/vimc/vimc.cpp +++ b/src/libcamera/pipeline/vimc/vimc.cpp @@ -342,24 +342,24 @@ int PipelineHandlerVimc::processControls(VimcCameraData *data, Request *request) for (auto it : request->controls()) { unsigned int id = it.first; + const ControlId *cid; unsigned int offset; - uint32_t cid; if (id == controls::Brightness) { - cid = V4L2_CID_BRIGHTNESS; + cid = &v4l2::BRIGHTNESS; offset = 128; } else if (id == controls::Contrast) { - cid = V4L2_CID_CONTRAST; + cid = &v4l2::CONTRAST; offset = 0; } else if (id == controls::Saturation) { - cid = V4L2_CID_SATURATION; + cid = &v4l2::SATURATION; offset = 0; } else { continue; } int32_t value = lroundf(it.second.get() * 128 + offset); - controls.set(cid, std::clamp(value, 0, 255)); + controls.set(cid->id(), std::clamp(value, 0, 255)); } for (const auto &ctrl : controls)