[libcamera-devel] [PATCH v1 6/9] ipa: raspberrypi: Pass lens shading table through configure() function

[Naushir Patuck]

Hi Laurent,

Thank you for your patch.

On Mon, 29 Jun 2020 at 00:19, Laurent Pinchart
<laurent.pinchart at ideasonboard.com> wrote:
>
> The IPAInterface now accepts custom configuration data. Use it to pass
> the lens shading table instead of using a custom IPA event. This will
> allow starting the IPA when starting the camera, instead of pre-starting
> it early in order to process the lens shading table allocation event.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
> ---
>  include/libcamera/ipa/raspberrypi.h                |  5 ++++-
>  src/ipa/raspberrypi/raspberrypi.cpp                | 12 ++++++------
>  src/libcamera/pipeline/raspberrypi/raspberrypi.cpp | 10 ++++------
>  3 files changed, 14 insertions(+), 13 deletions(-)
>
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index a18ce9a884b6..c335d0259549 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -10,6 +10,10 @@
>  #include <libcamera/control_ids.h>
>  #include <libcamera/controls.h>
>
> +enum RPiConfigParameters {
> +       RPI_IPA_CONFIG_LSTABLE = (1 << 0),
> +};

Minor thing, but could you rename this to RPI_IPA_CONFIG_LS_TABLE to
be consistent with other labels?

Otherwise all looks good.

Reviewed-by: Naushir Patuck <naush at raspberrypi.com>



> +
>  enum RPiOperations {
>         RPI_IPA_ACTION_V4L2_SET_STAGGERED = 1,
>         RPI_IPA_ACTION_V4L2_SET_ISP,
> @@ -21,7 +25,6 @@ enum RPiOperations {
>         RPI_IPA_EVENT_SIGNAL_STAT_READY,
>         RPI_IPA_EVENT_SIGNAL_ISP_PREPARE,
>         RPI_IPA_EVENT_QUEUE_REQUEST,
> -       RPI_IPA_EVENT_LS_TABLE_ALLOCATION,
>  };
>
>  enum RPiIpaMask {
> diff --git a/src/ipa/raspberrypi/raspberrypi.cpp b/src/ipa/raspberrypi/raspberrypi.cpp
> index 860be22ddb5d..c9f7dea375a5 100644
> --- a/src/ipa/raspberrypi/raspberrypi.cpp
> +++ b/src/ipa/raspberrypi/raspberrypi.cpp
> @@ -277,6 +277,12 @@ void IPARPi::configure(const CameraSensorInfo &sensorInfo,
>                 applyAGC(&agcStatus);
>
>         lastMode_ = mode_;
> +
> +       /* Store the lens shading table pointer and handle if available. */
> +       if (ipaConfig.operation & RPI_IPA_CONFIG_LSTABLE) {
> +               lsTable_ = reinterpret_cast<void *>(ipaConfig.data[0]);
> +               lsTableHandle_ = ipaConfig.data[1];
> +       }
>  }
>
>  void IPARPi::mapBuffers(const std::vector<IPABuffer> &buffers)
> @@ -358,12 +364,6 @@ void IPARPi::processEvent(const IPAOperationData &event)
>                 break;
>         }
>
> -       case RPI_IPA_EVENT_LS_TABLE_ALLOCATION: {
> -               lsTable_ = reinterpret_cast<void *>(event.data[0]);
> -               lsTableHandle_ = event.data[1];
> -               break;
> -       }
> -
>         default:
>                 LOG(IPARPI, Error) << "Unknown event " << event.operation;
>                 break;
> diff --git a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> index 0f9237a7f346..903796790f44 100644
> --- a/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> +++ b/src/libcamera/pipeline/raspberrypi/raspberrypi.cpp
> @@ -1109,6 +1109,7 @@ int RPiCameraData::configureIPA()
>  {
>         std::map<unsigned int, IPAStream> streamConfig;
>         std::map<unsigned int, const ControlInfoMap &> entityControls;
> +       IPAOperationData ipaConfig = {};
>
>         /* Get the device format to pass to the IPA. */
>         V4L2DeviceFormat sensorFormat;
> @@ -1138,11 +1139,9 @@ int RPiCameraData::configureIPA()
>                  * The vcsm allocation will always be in the memory region
>                  * < 32-bits to allow Videocore to access the memory.
>                  */
> -               IPAOperationData op;
> -               op.operation = RPI_IPA_EVENT_LS_TABLE_ALLOCATION;
> -               op.data = { static_cast<uint32_t>(ptr & 0xffffffff),
> -                           vcsm_.getVCHandle(lsTable_) };
> -               ipa_->processEvent(op);
> +               ipaConfig.operation = RPI_IPA_CONFIG_LSTABLE;
> +               ipaConfig.data = { static_cast<uint32_t>(ptr & 0xffffffff),
> +                                  vcsm_.getVCHandle(lsTable_) };
>         }
>
>         CameraSensorInfo sensorInfo = {};
> @@ -1153,7 +1152,6 @@ int RPiCameraData::configureIPA()
>         }
>
>         /* Ready the IPA - it must know about the sensor resolution. */
> -       IPAOperationData ipaConfig;
>         ipa_->configure(sensorInfo, streamConfig, entityControls, ipaConfig,
>                         nullptr);
>
> --
> Regards,
>
> Laurent Pinchart
>