[libcamera-devel] [PATCH v4 25/37] ipa: raspberrypi: Add mojom data definition file
Laurent Pinchart
laurent.pinchart at ideasonboard.com
Thu Nov 26 16:02:55 CET 2020
Hi Paul,
Thank you for the patch.
On Fri, Nov 06, 2020 at 07:36:55PM +0900, Paul Elder wrote:
> Add a mojom data definition for raspberrypi pipeline handler's IPAs.
> This is a direct translation of what the raspberrypi pipeline handler
> and IPA was using before with IPAOperationData.
s/was/were/
>
> Also move the enums from raspberrypi.h to raspberrypi.mojom
>
> Signed-off-by: Paul Elder <paul.elder at ideasonboard.com>
>
> ---
> Changes in v4:
> - rename IPARPiCallbackInterface to IPARPiEventInterface
>
> Changes in v3:
> - remove stray comment about how our compiler will generate code
> - add ipa.rpi namespace
> - not ipa.RPi, because namespace conflict
> - remove RPi prefix from struct and enum names
> - add transform parameter to struct ConfigInput
>
> Changes in v2:
> - rebased on "libcamera: pipeline: ipa: raspberrypi: Rework drop frame
> signalling"
> - add license
> - move generic documentation to core.mojom
> - move documentation from IPA interface
> - customize the RPi IPA interface to make the pipeline and IPA code
> cleaner
> ---
> include/libcamera/ipa/meson.build | 4 +-
> include/libcamera/ipa/raspberrypi.h | 18 ---
> include/libcamera/ipa/raspberrypi.mojom | 158 ++++++++++++++++++++++++
> 3 files changed, 161 insertions(+), 19 deletions(-)
> create mode 100644 include/libcamera/ipa/raspberrypi.mojom
>
> diff --git a/include/libcamera/ipa/meson.build b/include/libcamera/ipa/meson.build
> index b7caec95..d49dff7b 100644
> --- a/include/libcamera/ipa/meson.build
> +++ b/include/libcamera/ipa/meson.build
> @@ -24,7 +24,9 @@ ipa_mojom_core = custom_target(core_mojom_file.split('.')[0] + '_mojom_module',
> '--mojoms', '@INPUT@'
> ])
>
> -ipa_mojom_files = []
> +ipa_mojom_files = [
> + 'raspberrypi.mojom',
> +]
>
> ipa_mojoms = []
>
> diff --git a/include/libcamera/ipa/raspberrypi.h b/include/libcamera/ipa/raspberrypi.h
> index 2b55dbfc..df30b4a2 100644
> --- a/include/libcamera/ipa/raspberrypi.h
> +++ b/include/libcamera/ipa/raspberrypi.h
> @@ -16,24 +16,6 @@ namespace libcamera {
>
> namespace RPi {
>
> -enum ConfigParameters {
> - IPA_CONFIG_LS_TABLE = (1 << 0),
> - IPA_CONFIG_STAGGERED_WRITE = (1 << 1),
> - IPA_CONFIG_SENSOR = (1 << 2),
> - IPA_CONFIG_DROP_FRAMES = (1 << 3),
> -};
> -
> -enum Operations {
> - IPA_ACTION_V4L2_SET_STAGGERED = 1,
> - IPA_ACTION_V4L2_SET_ISP,
> - IPA_ACTION_STATS_METADATA_COMPLETE,
> - IPA_ACTION_RUN_ISP,
> - IPA_ACTION_EMBEDDED_COMPLETE,
> - IPA_EVENT_SIGNAL_STAT_READY,
> - IPA_EVENT_SIGNAL_ISP_PREPARE,
> - IPA_EVENT_QUEUE_REQUEST,
> -};
> -
> enum BufferMask {
> ID = 0x00ffff,
> STATS = 0x010000,
> diff --git a/include/libcamera/ipa/raspberrypi.mojom b/include/libcamera/ipa/raspberrypi.mojom
> new file mode 100644
> index 00000000..ec261569
> --- /dev/null
> +++ b/include/libcamera/ipa/raspberrypi.mojom
> @@ -0,0 +1,158 @@
> +/* SPDX-License-Identifier: LGPL-2.1-or-later */
> +
> +module ipa.rpi;
> +
> +import "include/libcamera/ipa/core.mojom";
> +
> +interface IPARPiInterface {
> +/**
> + * \fn init()
> + * \brief Initialise the IPAInterface
> + * \param[in] settings The IPA initialization settings
> + *
> + * This function initializes the IPA interface. It shall be called before any
> + * other function of the IPAInterface. The \a settings carry initialization
> + * parameters that are valid for the whole life time of the IPA interface.
> + */
Hmmmm... I really wonder how we should handle documentation here. This
won't be parsed by doxygen, which isn't an issue (at least for now) as
it's really an internal API. It however has the drawback of not catching
issues in the documentation at compile time. I doubt there's much we
could do here for now. Longer term extracting the comment and including
it in the generated sources could be a good way forward.
Another question is what we should document here, for the functions that
are generic (that's init(), start() and stop() for now). It's a bit
pointless to duplicate that in all mojom files.
Finally, should this be indented by one tab to align it with the
functions ?
> + init(IPASettings settings) => (int32 ret);
> +
> +/**
> + * \fn start()
> + * \brief Start the IPA
> + *
> + * This method informs the IPA module that the camera is about to be started.
> + * The IPA module shall prepare any resources it needs to operate.
> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> + start() => (int32 ret);
> +
> +/**
> + * \fn stop()
> + * \brief Stop the IPA
> + *
> + * This method informs the IPA module that the camera is stopped. The IPA module
> + * shall release resources prepared in start().
> + */
> + stop();
> +
> +/**
> + * \fn configure()
> + * \brief Configure the IPA stream and sensor settings
> + * \param[in] sensorInfo Camera sensor information
> + * \param[in] streamConfig Configuration of all active streams
> + * \param[in] entityControls Controls provided by the pipeline entities
> + * \param[in] ipaConfig Pipeline-handler-specific configuration data
> + * \param[out] result Pipeline-handler-specific configuration result
> + *
> + * This method shall be called when the camera is started to inform the IPA of
> + * the camera's streams and the sensor settings. The meaning of the numerical
> + * keys in the \a streamConfig and \a entityControls maps is defined by the IPA
> + * protocol.
The mention of IPA protocol made sense when the IPAInterface was the
same for all IPAs, but that's now outdated. *This* is the IPA protocol
for RPi :-) The documentation should be reworked accordingly. Generic
parts could be preserved in the IPA guide, to explain how to create an
IPA protocol.
> + *
> + * The \a sensorInfo conveys information about the camera sensor settings that
> + * the pipeline handler has selected for the configuration. The IPA may use
> + * that information to tune its algorithms.
> + *
> + * The \a ipaConfig and \a result parameters carry custom data passed by the
> + * pipeline handler to the IPA and back. The pipeline handler may set the \a
> + * result parameter to null if the IPA protocol doesn't need to pass a result
> + * back through the configure() function.
> + */
> + configure(CameraSensorInfo sensorInfo,
> + map<uint32, IPAStream> streamConfig,
> + map<uint32, ControlInfoMap> entityControls,
> + ConfigInput ipaConfig)
> + => (ConfigOutput results);
> +
> +/**
> + * \fn mapBuffers()
> + * \brief Map buffers shared between the pipeline handler and the IPA
> + * \param[in] buffers List of buffers to map
> + *
> + * This method informs the IPA module of memory buffers set up by the pipeline
> + * handler that the IPA needs to access. It provides dmabuf file handles for
> + * each buffer, and associates the buffers with unique numerical IDs.
> + *
> + * IPAs shall map the dmabuf file handles to their address space and keep a
> + * cache of the mappings, indexed by the buffer numerical IDs. The IDs are used
> + * in all other IPA interface methods to refer to buffers, including the
> + * unmapBuffers() method.
Same here, there's lots of generic documentation that doesn't belong to
a pipeline handler-specific file.
> + *
> + * All buffers that the pipeline handler wishes to share with an IPA shall be
> + * mapped with this method. Buffers may be mapped all at once with a single
> + * call, or mapped and unmapped dynamically at runtime, depending on the IPA
> + * protocol. Regardless of the protocol, all buffers mapped at a given time
> + * shall have unique numerical IDs.
> + *
> + * The numerical IDs have no meaning defined by the IPA interface, and IPA
> + * protocols shall not give them any specific meaning either. They should be
> + * treated as opaque handles by IPAs, with the only exception that ID zero is
> + * invalid.
> + *
> + * \sa unmapBuffers()
> + *
> + * \todo Provide a generic implementation of mapBuffers and unmapBuffers for
> + * IPAs
> + */
> + mapBuffers(array<IPABuffer> buffers);
> +
> +/**
> + * \fn unmapBuffers()
> + * \brief Unmap buffers shared by the pipeline to the IPA
> + * \param[in] ids List of buffer IDs to unmap
> + *
> + * This method removes mappings set up with mapBuffers(). Buffers may be
> + * unmapped all at once with a single call, or selectively at runtime, depending
> + * on the IPA protocol. Numerical IDs of unmapped buffers may be reused when
> + * mapping new buffers.
> + *
> + * \sa mapBuffers()
> + */
> + unmapBuffers(array<uint32> ids);
> +
> + [async] signalStatReady(uint32 bufferId);
> + [async] signalQueueRequest(ControlList controls);
> + [async] signalIspPrepare(IspPreparePayload data);
> +};
> +
> +interface IPARPiEventInterface {
> + statsMetadataComplete(uint32 bufferId, ControlList controls);
> + runIsp(uint32 bufferId);
> + embeddedComplete(uint32 bufferId);
> + setIsp(ControlList controls);
> + setStaggered(ControlList controls);
> +};
> +
> +
> +enum ConfigParameters {
> + RPI_IPA_CONFIG_LS_TABLE = 0x01,
> + RPI_IPA_CONFIG_STAGGERED_WRITE = 0x02,
> + RPI_IPA_CONFIG_SENSOR = 0x04,
> + RPI_IPA_CONFIG_DROP_FRAMES = 0x08,
As this will be qualified by the ipa::rpi:: namespace, should we drop
the RPI_IPA_ prefix ?
While at it, would it make sense to move to our usual coding style of
CamelCase for enum values ?
> +};
> +
> +struct StaggeredWritePayload {
s/Payload/Config/ ? Payload really sounds like a networking term, and
while data is serialized to a payload, that shouldn't appear in the API.
Actually, wouls SensorConfig be a better name ? The name "staggered
write" disappears with Niklas' patches that move the helper to the
libcamera core.
> + uint32 gainDelay;
> + uint32 exposureDelay;
> + uint32 sensorMetadata;
> +};
> +
> +struct IspPreparePayload {
s/IspPreparePayload/ISPConfig/ ?
> + uint32 embeddedbufferId;
> + uint32 bayerbufferId;
> +};
> +
> +struct ConfigInput {
> + uint32 op;
I think you can drop this field (and the RPI_IPA_CONFIG_LS_TABLE enum)
and rely on lsTableHandle being valid instead. It can be done on later
in the series, up to you.
> + uint32 transform;
> + FileDescriptor lsTableHandle;
> + int32 lsTableHandleStatic = -1;
> +};
> +
> +struct ConfigOutput {
> + uint32 op;
Here too I'd like to drop the op field if possible.
RPI_IPA_CONFIG_DROP_FRAMES can be dropped as it's always set. For
RPI_IPA_CONFIG_SENSOR, we can check if controls is empty.
RPI_IPA_CONFIG_STAGGERED_WRITE is the annoying one, as ideally it should
be implemented through support for nullable fields. Mojo supports this
by turning fields of struct type to pointers, but we don't, as we embed
them instead. I think it would make sense doing the same, but not yet,
as that would require too many changes to this series. In the meantime,
one simple option would be to ignore staggeredWriteResult in the
pipelien handler when !gainDelay && !exposureDelay, as that's an invalid
case.
> + StaggeredWritePayload staggeredWriteResult;
staggeredWriteResult sounds a bit cryptic. If we rename the structure to
SensorConfig, sensorConfig would be a better field name.
> + ControlList controls;
Should this be named sensorControls ?
> + int32 dropFrameCount;
> +};
Even if the mojo IDL doesn't require this, should the enums and struct
be moved before the interfaces ? I think it would improve readability.
--
Regards,
Laurent Pinchart
More information about the libcamera-devel
mailing list