[libcamera-devel] [PATCH v4 25/37] ipa: raspberrypi: Add mojom data definition file
paul.elder at ideasonboard.com
paul.elder at ideasonboard.com
Sat Dec 5 10:20:10 CET 2020
Hi Laurent,
Thank you for the review.
On Thu, Nov 26, 2020 at 05:02:55PM +0200, Laurent Pinchart wrote:
> 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.
Yeah :/
> 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.
I'll move the generic ones to core.mojom for now. That's where I
intended them to be to begin with. Maybe we can get doxygen to extract
docs from there?
> Finally, should this be indented by one tab to align it with the
> functions ?
Yeah that's better.
> > + 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.
Ah yes.
> > + *
> > + * 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 ?
Here, yeah, we can do that.
> While at it, would it make sense to move to our usual coding style of
> CamelCase for enum values ?
Yeah.
> > +};
> > +
> > +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.
SensorConfig it is.
> > + uint32 gainDelay;
> > + uint32 exposureDelay;
> > + uint32 sensorMetadata;
> > +};
> > +
> > +struct IspPreparePayload {
>
> s/IspPreparePayload/ISPConfig/ ?
Yep.
> > + 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.
I'll change around the ops and the ops processing in v6.
> > + StaggeredWritePayload staggeredWriteResult;
>
> staggeredWriteResult sounds a bit cryptic. If we rename the structure to
> SensorConfig, sensorConfig would be a better field name.
Yeah.
> > + ControlList controls;
>
> Should this be named sensorControls ?
I thought controls was just fine...
> > + 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.
Yeah.
Thanks,
Paul
More information about the libcamera-devel
mailing list