[libcamera-devel] [PATCH v3] ipa: rpi: cam_helper: PDAF support for IMX519
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Sat Jun 24 01:48:47 CEST 2023
Hi Umang,
Thank you for the patch.
CC'ing Naush, as a review from Raspberry Pi would be nice.
On Thu, Jun 22, 2023 at 08:30:15PM +0530, Umang Jain via libcamera-devel wrote:
> From: Lee Jackson <lee.jackson at arducam.com>
>
> Adds the PDAF support for IMX519 camera module by Arducam.
>
> Tested-by: Umang Jain <umang.jain at ideasonboard.com>
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> Signed-off-by: Lee Jackson <lee.jackson at arducam.com>
> ---
> Changes in v3:
> - Collect author's S-o-B from v2.
>
> ---
> src/ipa/rpi/cam_helper/cam_helper_imx519.cpp | 50 ++++++++++++++++++++
> src/ipa/rpi/vc4/data/imx519.json | 40 ++++++++++++++++
> 2 files changed, 90 insertions(+)
>
> diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp
> index c7262aa0..6d032082 100644
> --- a/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp
> +++ b/src/ipa/rpi/cam_helper/cam_helper_imx519.cpp
> @@ -15,9 +15,13 @@
>
> #include <libcamera/base/log.h>
>
> +#include "controller/pdaf_data.h"
> +
> #include "cam_helper.h"
> #include "md_parser.h"
>
> +#define ALIGN_UP(x, a) (((x) + (a)-1) & ~(a - 1))
utils.h has a alignUp() function.
> +
> using namespace RPiController;
> using namespace libcamera;
> using libcamera::utils::Duration;
> @@ -66,8 +70,13 @@ private:
> /* Largest long exposure scale factor given as a left shift on the frame length. */
> static constexpr int longExposureShiftMax = 7;
>
> + static constexpr int pdafStatsRows = 12;
> + static constexpr int pdafStatsCols = 16;
> +
> void populateMetadata(const MdParser::RegisterMap ®isters,
> Metadata &metadata) const override;
> + static bool parsePdafData(const uint8_t *ptr, size_t len, unsigned bpp,
> + PdafRegions &pdaf);
> };
>
> CamHelperImx519::CamHelperImx519()
> @@ -90,6 +99,11 @@ void CamHelperImx519::prepare(libcamera::Span<const uint8_t> buffer, Metadata &m
> MdParser::RegisterMap registers;
> DeviceStatus deviceStatus;
>
> + LOG(IPARPI, Debug) << "Embedded buffer size: " << buffer.size();
> +
> + size_t bytesPerLine = (mode_.width * mode_.bitdepth) >> 3;
> + bytesPerLine = ALIGN_UP(bytesPerLine, 16);
> +
> if (metadata.get("device.status", deviceStatus)) {
> LOG(IPARPI, Error) << "DeviceStatus not found from DelayedControls";
> return;
> @@ -97,6 +111,14 @@ void CamHelperImx519::prepare(libcamera::Span<const uint8_t> buffer, Metadata &m
>
> parseEmbeddedData(buffer, metadata);
>
> + if (buffer.size() > 2 * bytesPerLine) {
If my understanding is correct, the value 2 here means that the sensor
generates 2 lines of register embedded data, followed by PDAF data. A
name constexpr would be nice to replace the magic value. I would also
pass a subspan of 2 lines to the parseEmbeddedData() function, as it
shouldn't look into the PDAF section.
> + PdafRegions pdaf;
> + parsePdafData(&buffer[2 * bytesPerLine],
> + buffer.size() - 2 * bytesPerLine,
> + mode_.bitdepth, pdaf);
> + metadata.set("pdaf.data", pdaf);
> + }
> +
> /*
> * The DeviceStatus struct is first populated with values obtained from
> * DelayedControls. If this reports frame length is > frameLengthMax,
> @@ -188,6 +210,34 @@ void CamHelperImx519::populateMetadata(const MdParser::RegisterMap ®isters,
> metadata.set("device.status", deviceStatus);
> }
>
> +bool CamHelperImx519::parsePdafData(const uint8_t *ptr, size_t len,
This function should take a span instead of ptr + len.
> + unsigned bpp, PdafRegions &pdaf)
> +{
> + size_t step = bpp >> 1; /* bytes per PDAF grid entry */
> +
> + if (bpp < 10 || bpp > 12 || len < 194 * step || ptr[0] != 0 || ptr[1] >= 0x40) {
Can the bpp check fail, or do we always use RAW10 or RAW12 with this
sensor ? The driver posted to the linux-media mailing list only supports
RAW10. Does the sensor support RAW8 or RAW14 (or more) ?
Where does the 194 come from ? A named constexpr, or a mathematical
expression based on named constexprs, would be good too. Same for the
first two bytes, magic values are not nice.
> + LOG(IPARPI, Error) << "PDAF data in unsupported format";
> + return false;
> + }
> +
> + pdaf.init({ pdafStatsCols, pdafStatsRows });
> +
> + ptr += 2 * step;
> + for (unsigned i = 0; i < pdafStatsRows; ++i) {
> + for (unsigned j = 0; j < pdafStatsCols; ++j) {
> + unsigned c = (ptr[0] << 3) | (ptr[1] >> 5);
uint8_t
> + int p = (((ptr[1] & 0x0F) - (ptr[1] & 0x10)) << 6) | (ptr[2] >> 2);
Lowercase for hex constants.
> + PdafData pdafData;
> + pdafData.conf = c;
> + pdafData.phase = c ? p : 0;
> + pdaf.set(libcamera::Point(j, i), { pdafData, 1, 0 });
> + ptr += step;
> + }
> + }
> +
> + return true;
> +}
Now that I've reviewed this, I realize all those commends apply to the
IMX708 helper too. Can we factor this code out instead of duplicating it
?
> +
> static CamHelper *create()
> {
> return new CamHelperImx519();
> diff --git a/src/ipa/rpi/vc4/data/imx519.json b/src/ipa/rpi/vc4/data/imx519.json
> index 1b0a7747..0733d97e 100644
> --- a/src/ipa/rpi/vc4/data/imx519.json
> +++ b/src/ipa/rpi/vc4/data/imx519.json
> @@ -350,6 +350,46 @@
> ]
> }
> },
> + {
> + "rpi.af":
> + {
> + "ranges":
> + {
> + "normal":
> + {
> + "min": 0.0,
> + "max": 12.0,
> + "default": 1.0
> + },
> + "macro":
> + {
> + "min": 3.0,
> + "max": 15.0,
> + "default": 4.0
> + }
> + },
> + "speeds":
> + {
> + "normal":
> + {
> + "step_coarse": 1.0,
> + "step_fine": 0.25,
> + "contrast_ratio": 0.75,
> + "pdaf_gain": -0.02,
> + "pdaf_squelch": 0.125,
> + "max_slew": 2.0,
> + "pdaf_frames": 20,
> + "dropout_frames": 6,
> + "step_frames": 4
> + }
> + },
> + "conf_epsilon": 8,
> + "conf_thresh": 16,
> + "conf_clip": 512,
> + "skip_frames": 5,
> + "map": [ 0.0, 0.0, 15.0, 4095 ]
> + }
> + },
> {
> "rpi.ccm":
> {
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list