[libcamera-devel] [PATCH v3] ipa: rpi: cam_helper: PDAF support for IMX519
Umang Jain
umang.jain at ideasonboard.com
Fri Jun 30 13:39:37 CEST 2023
Hi Dave, Naushir,
On 6/30/23 12:55 PM, Dave Stevenson wrote:
> 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
Can I ask why it is like that? Embedded data with same stride / bytes
per line as the image area, makes sense to me. Using no stride probably
a try to optimise something ?
> 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.
Ah, I see.
> 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.
That's is something i've locally (will be pushed shortly as RFC, have a
look).
>
> 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.
Ack. Till then let the patches iterate as RFC and have discussions
ongoing as well.
>
> 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