| Message ID | 20230622150015.45888-1-umang.jain@ideasonboard.com |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series |
|
| Related | show |
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@arducam.com> > > Adds the PDAF support for IMX519 camera module by Arducam. > > Tested-by: Umang Jain <umang.jain@ideasonboard.com> > Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > Signed-off-by: Lee Jackson <lee.jackson@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. > + 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": > {
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@arducam.com> >> >> Adds the PDAF support for IMX519 camera module by Arducam. >> >> Tested-by: Umang Jain <umang.jain@ideasonboard.com> >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >> Signed-off-by: Lee Jackson <lee.jackson@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": >> {
Hi Umang, On Thu, 29 Jun 2023 at 22:07, Umang Jain <umang.jain@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@arducam.com> > >> > >> Adds the PDAF support for IMX519 camera module by Arducam. > >> > >> Tested-by: Umang Jain <umang.jain@ideasonboard.com> > >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > >> Signed-off-by: Lee Jackson <lee.jackson@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": > >> { >
Hi Umang On Fri, 30 Jun 2023 at 08:53, Naushir Patuck via libcamera-devel <libcamera-devel@lists.libcamera.org> wrote: > > Hi Umang, > > On Thu, 29 Jun 2023 at 22:07, Umang Jain <umang.jain@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@arducam.com> > > >> > > >> Adds the PDAF support for IMX519 camera module by Arducam. > > >> > > >> Tested-by: Umang Jain <umang.jain@ideasonboard.com> > > >> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > > >> Signed-off-by: Lee Jackson <lee.jackson@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": > > >> { > >
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@lists.libcamera.org> wrote: >> Hi Umang, >> >> On Thu, 29 Jun 2023 at 22:07, Umang Jain <umang.jain@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@arducam.com> >>>>> >>>>> Adds the PDAF support for IMX519 camera module by Arducam. >>>>> >>>>> Tested-by: Umang Jain <umang.jain@ideasonboard.com> >>>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> >>>>> Signed-off-by: Lee Jackson <lee.jackson@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": >>>>> {
Hi Umang On Fri, 30 Jun 2023 at 12:39, Umang Jain <umang.jain@ideasonboard.com> wrote: > > 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@lists.libcamera.org> wrote: > >> Hi Umang, > >> > >> On Thu, 29 Jun 2023 at 22:07, Umang Jain <umang.jain@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@arducam.com> > >>>>> > >>>>> Adds the PDAF support for IMX519 camera module by Arducam. > >>>>> > >>>>> Tested-by: Umang Jain <umang.jain@ideasonboard.com> > >>>>> Signed-off-by: Umang Jain <umang.jain@ideasonboard.com> > >>>>> Signed-off-by: Lee Jackson <lee.jackson@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 ? There's no requirement for alternate data types to have specific lengths per frame. Taking the Toshiba TC358743 HDMI to CSI2 bridge chip as an example, it can pass HDMI audio data and InfoFrames over CSI2 using alternate data types. That's using the same virtual channel as the images, but audio is largely independent of any framing, let alone image parameters. How would you handle that in the receiver if you forced a stride type parameter on it? > > 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 IMX219 datasheet is public (although not officially) at [1]. Fig 20 "Frame Structure for Serial signal output" on page 47 gives you an example of the typical framing definition you'll get for a sensor. The Packet Header on each line dictates what the line contains. The embedded data lines will use data type 0x12 in their header, whilst the image data will be 0x2a for raw8, or 0x2b for raw10 (specified in the MIPI CSI-2 spec). The generic case is presented in the CSI-2 spec section 11.1.2 "Embedded Information", and Figure 57 "Frame Structure with Embedded Data at the Beginning and End of the Frame". For IMX708 (and presumably the others) you'll find that it adds a number of lines afterwards in a specific order for all the extra metadata. Registers allow enabling/disabling of each, and setting the data type field to be used. The MIPI CCS spec [2] contains the packing of the registers into the embedded buffer - section 7.15 "Embedded Data Line Formats" in v1.1 (2 Dec 2019). Most sensors seem to follow that, even if they aren't compliant to CCS with regard to configuration. Dave [1] https://github.com/rellimmot/Sony-IMX219-Raspberry-Pi-V2-CMOS/blob/master/RASPBERRY%20PI%20CAMERA%20V2%20DATASHEET%20IMX219PQH5_7.0.0_Datasheet_XXX.PDF [2] https://www.mipi.org/camera-command-set-v1-1-1-download Sorry, the CSI-2 is restricted to MIPI Alliance members only, but there are copies floating about on the web. I'm not going to directly link to any. Very little changes between versions, so v1.01 April 2009 is sufficient. > > 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": > >>>>> { >
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)) + 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) { + 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, + 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) { + 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); + int p = (((ptr[1] & 0x0F) - (ptr[1] & 0x10)) << 6) | (ptr[2] >> 2); + PdafData pdafData; + pdafData.conf = c; + pdafData.phase = c ? p : 0; + pdaf.set(libcamera::Point(j, i), { pdafData, 1, 0 }); + ptr += step; + } + } + + return true; +} + 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": {