[libcamera-devel] [RFC PATCH 1/5] rpi: cam_helper_imx708: Use Span<> to pass PDAF data

[Naushir Patuck]

Hi Umang,

Thank you for this work.

On Fri, 30 Jun 2023 at 13:03, Umang Jain <umang.jain at ideasonboard.com> wrote:
>
> Instead of passing raw buffer pointer and length, construct a Span<>
> and pass it in parsePdafData().
>
> While at it, introduce a constexpr which denotes the scanline of the
> PDAF data in the embedded data. Use that constexpr to compute the
> offset of PDAF buffer in the embedded data.
>
> Np functional changes intended in this patch.
>
> Signed-off-by: Umang Jain <umang.jain at ideasonboard.com>
> ---
>  src/ipa/rpi/cam_helper/cam_helper_imx708.cpp | 25 +++++++++++---------
>  1 file changed, 14 insertions(+), 11 deletions(-)
>
> diff --git a/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp b/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp
> index 641ba18f..b24ee643 100644
> --- a/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp
> +++ b/src/ipa/rpi/cam_helper/cam_helper_imx708.cpp
> @@ -42,6 +42,9 @@ constexpr std::initializer_list<uint32_t> registerList =
>         { expHiReg, expLoReg, gainHiReg, gainLoReg, lineLengthHiReg,
>           lineLengthLoReg, frameLengthHiReg, frameLengthLoReg, temperatureReg };
>
> +/* PDAF data is expect to occupy the third scanline of embedded data. */
> +constexpr uint32_t pdafLineOffsetImx708 = 2;

Could this live in the private member area of the helper class?  We have other
const values listed there.  Also, I would remove the "Imx708" suffix.

Regards,
Naush

> +
>  class CamHelperImx708 : public CamHelper
>  {
>  public:
> @@ -75,7 +78,7 @@ private:
>         void populateMetadata(const MdParser::RegisterMap &registers,
>                               Metadata &metadata) const override;
>
> -       static bool parsePdafData(const uint8_t *ptr, size_t len, unsigned bpp,
> +       static bool parsePdafData(libcamera::Span<const uint8_t> &pdafData, unsigned bpp,
>                                   PdafRegions &pdaf);
>
>         bool parseAEHist(const uint8_t *ptr, size_t len, unsigned bpp);
> @@ -116,17 +119,16 @@ void CamHelperImx708::prepare(libcamera::Span<const uint8_t> buffer, Metadata &m
>
>         parseEmbeddedData(buffer, metadata);
>
> -       /*
> -        * Parse PDAF data, which we expect to occupy the third scanline
> -        * of embedded data. As PDAF is quite sensor-specific, it's parsed here.
> -        */
> +       /* Parse sensor-specific PDAF data. */
>         size_t bytesPerLine = (mode_.width * mode_.bitdepth) >> 3;
> +       size_t pdafDataOffset = pdafLineOffsetImx708 * bytesPerLine;
>
> -       if (buffer.size() > 2 * bytesPerLine) {
> +       if (buffer.size() > pdafDataOffset) {
>                 PdafRegions pdaf;
> -               if (parsePdafData(&buffer[2 * bytesPerLine],
> -                                 buffer.size() - 2 * bytesPerLine,
> -                                 mode_.bitdepth, pdaf))
> +               libcamera::Span<const uint8_t> pdafData{ &buffer[pdafDataOffset],
> +                                                        buffer.size() - pdafDataOffset };
> +
> +               if (parsePdafData(pdafData, mode_.bitdepth, pdaf))
>                         metadata.set("pdaf.regions", pdaf);
>         }
>
> @@ -241,9 +243,10 @@ void CamHelperImx708::populateMetadata(const MdParser::RegisterMap &registers,
>         metadata.set("device.status", deviceStatus);
>  }
>
> -bool CamHelperImx708::parsePdafData(const uint8_t *ptr, size_t len,
> -                                   unsigned bpp, PdafRegions &pdaf)
> +bool CamHelperImx708::parsePdafData(Span<const uint8_t> &data, unsigned bpp, PdafRegions &pdaf)
>  {
> +       const uint8_t *ptr = data.data();
> +       unsigned int len = data.size();
>         size_t step = bpp >> 1; /* bytes per PDAF grid entry */
>
>         if (bpp < 10 || bpp > 12 || len < 194 * step || ptr[0] != 0 || ptr[1] >= 0x40) {
> --
> 2.39.1
>