[libcamera-devel] [PATCH v3] ipa: rpi: cam_helper: PDAF support for IMX519
Umang Jain
umang.jain at ideasonboard.com
Thu Jun 29 23:06:59 CEST 2023
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 ?
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 ?
"One lines of PDAF data" mentioned int the driver but libcamera-helper
[2] seems to consider two lines instead ?
Naushir, is there any specific documentation regarding the embedded data
/ layout etc. ? It seems I am missing context.
[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