[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 &registers,
>>>>>                             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 &registers,
>>>>>       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