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

18770 diff mbox series

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@ideasonboard.com>
---
 src/ipa/rpi/cam_helper/cam_helper_imx708.cpp | 25 +++++++++++---------
 1 file changed, 14 insertions(+), 11 deletions(-)

Hi Umang,

Thank you for this work.

On Fri, 30 Jun 2023 at 13:03, Umang Jain <umang.jain@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@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
>