[libcamera-devel] [PATCH v3] ipa: rpi: cam_helper: PDAF support for IMX519
Naushir Patuck
naush at raspberrypi.com
Fri Jun 30 09:52:49 CEST 2023
Hi Umang,
On Thu, 29 Jun 2023 at 22:07, Umang Jain <umang.jain at ideasonboard.com> wrote:
>
> Hello,
>
> On 6/24/23 1:48 AM, Laurent Pinchart wrote:
> > 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.
>
> I am refactoring this as mentioned in the review but having few questions?
>
> Is embedded data contains PDAF data as well ? OR embedded data is
> separate and PDAF data is separate sections ?
PDAF data comes after embedded data - typically with a different packet id.
However, because Unicam only has 2x DMA channels, embedded data, PDAF data and
HDR histogram data all get routed into a single metadata buffer.
>
> For e.g. I was looking at IMX708 driver [1], and what it states is
> confusing to me as well:
>
> /*
> * Metadata buffer holds a variety of data, all sent with the same
> VC/DT (0x12).
> * It comprises two scanlines (of up to 5760 bytes each, for 4608 pixels)
> * of embedded data, one line of PDAF data, and two lines of AE-HIST data
> * (AE histograms are valid for HDR mode and empty in non-HDR modes).
> */
> #define IMX708_EMBEDDED_LINE_WIDTH (5 * 5760)
> #define IMX708_NUM_EMBEDDED_LINES 1
>
> "Two scanlines of embedded data" but macro IMX708_NUM_EMBEDDED_LINES = 1
> ? Is scanlines different ?
You should probably ignore this value. As you know we don't currently have
official V4L2 support for embedded data or generic sensor metadata. For our
downstream workaround this define is only used to size up the metadata buffer
(since formats have to have a width and height in V4L2). It conveys no
information on the actual embedded data structure.
>
> "One lines of PDAF data" mentioned int the driver but libcamera-helper
> [2] seems to consider two lines instead ?
>
The actual number of embedded data scan lines is 2.
> Naushir, is there any specific documentation regarding the embedded data
> / layout etc. ? It seems I am missing context.
The datasheet for the imx708 is under strict NDA with Sony so unfortunately I
cannot share any more than what is already in the parsing code here.
I should also qualify that this is specific to the IMX708. I don't know
anything about structure of the IMX519 metadata, but given the parsing code
seems to give some sensible results, it may very well be the same.
Regards,
Naush
>
> [1]:
> https://github.com/raspberrypi/linux/blob/rpi-6.4.y/drivers/media/i2c/imx708.c
> [2]:
> https://git.libcamera.org/libcamera/libcamera.git/tree/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp#n127
> >
> >> + 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":
> >> {
>
More information about the libcamera-devel
mailing list