[libcamera-devel] [PATCH v3] ipa: rpi: cam_helper: PDAF support for IMX519
Dave Stevenson
dave.stevenson at raspberrypi.com
Fri Jun 30 12:55:54 CEST 2023
Hi Umang
On Fri, 30 Jun 2023 at 08:53, Naushir Patuck via libcamera-devel
<libcamera-devel at lists.libcamera.org> wrote:
>
> 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 IMX708 the CSI2 data type for additional metadata lines (PDAF, AE,
histograms, etc) is configurable, but our downstream driver sets them
to use 0x12 to be the same as the embedded data. As Naush says, it
makes no difference with Unicam as that only has 2 data paths (image,
and "everything else"), but keeps it a little cleaner.
> >
> > 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.
The "everything else" channel in Unicam has no stride / bytesperline
configuration, therefore it will write the data exactly as it comes
in. For most sensors the "line length" of the metadata will be the
same as that for the image mode, therefore the parser needs to know
the image format too.
The order that the sensor produces the lines of metadata is stated in
the datasheet, so the parser can know what to expect and in what
order.
If a CSI2 receiver supports demultiplexing more data streams, and the
driver is amended to use different data id values, then the PDAF data
could be in a separate buffer to embedded data etc. So if you're
really trying to make it generic then you want to be passing in a
pointer to the start of the metadata.
I would expect this to all change when Sakari lands the "Generic line
based metadata support" series that was discussed in Prague earlier
this week.
> >
> > "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.
Note that Arducam have just made a PR adding PDAF to the kernel driver
for their 64MPix camera (IMX682). They don't appear to have added PDAF
support to their libcamera fork[2], but I would expect it to happen
relatively soon. Perhaps it's worth waiting for that just to ensure
you're covering it too.
Dave
[1] https://github.com/raspberrypi/linux/pull/5523
[2] https://github.com/arducam/libcamera
> 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