[libcamera-devel] [PATCH] libcamera: ipu3: Do not over-write metadata

[Jacopo Mondi]

Hi Laurent,

On Mon, Feb 22, 2021 at 12:26:09PM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Feb 22, 2021 at 11:24:41AM +0100, Jacopo Mondi wrote:
> > On Mon, Feb 22, 2021 at 11:18:51AM +0200, Laurent Pinchart wrote:
> > > On Mon, Feb 22, 2021 at 10:01:08AM +0100, Jacopo Mondi wrote:
> > > > On Sun, Feb 21, 2021 at 08:39:52PM +0200, Laurent Pinchart wrote:
> > > > > On Fri, Feb 19, 2021 at 03:28:34PM +0100, Niklas Söderlund wrote:
> > > > > > On 2021-02-19 12:22:57 +0100, Jacopo Mondi wrote:
> > > > > > > When a Request is completed upon receiving the IPA produced metadata,
> > > > > > > the metadata associated with the Request are over-written, deleting
> > > > > > > the information set, in example, at ImgU output buffer completion time.
> > > > > > >
> > > > > > > If any additional Request metadata should be registered by inspecting
> > > > > > > the IPA produced metadata it has to be done without deleting the already
> > > > > > > registered entries.
> > > > > > >
> > > > > > > Fix this by replacing the metadata over-write with a todo entry.
> > > > > > >
> > > > > > > This change fixes CTS which was broken due to missing metadata in
> > > > > > > the completed requests.
> > > > > > >
> > > > > > > Fixes: 9708f49fecf2 ("libcamera: ipu3: Share parameter and statistic buffers with IPA")
> > > > > > > Signed-off-by: Jacopo Mondi <jacopo at jmondi.org>
> > > > > > > ---
> > > > > > >  src/libcamera/pipeline/ipu3/ipu3.cpp | 5 ++++-
> > > > > > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > > > > > >
> > > > > > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > > > > index 2aed826a892a..9e867ab2e98a 100644
> > > > > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp
> > > > > > > @@ -1101,8 +1101,11 @@ void IPU3CameraData::queueFrameAction(unsigned int id,
> > > > > > >  		if (!info)
> > > > > > >  			break;
> > > > > > >
> > > > > > > +		/*
> > > > > > > +		 * \todo Parse the value of the controls returned by the IPA
> > > > > > > +		 * in action.controls to register additional request metadata.
> > > > > > > +		 */
> > > > > > >  		Request *request = info->request;
> > > > > > > -		request->metadata() = action.controls;
> > > > > >
> > > > > > I understand the intent is to keep metadata set by the pipeline but this
> > > > > > change simply moves the problem. With this change any metadata reported
> > > > > > by the IPA is discarded. I thought the idea was that all metadata would
> > > > > > be generated by the IPA? If not I think we need to merge the two here.
> > > > >
> > > > > Merging the two seems like the right solution. The ControlList class
> > > > > doesn't support this at the moment, but it shouldn't be difficult to
> > > > > extend it. A merge() function, along the lines of
> > > > > https://en.cppreference.com/w/cpp/container/unordered_map/merge, should
> > > > > do.
> > > >
> > > > At the moment the IPA does not report any metadata if I'm not
> > > > mistaken, and this change breaks 80+ CTS tests as it overwrites the actual
> > > > metadata with an empty list.
> > >
> > > Correct, so I'm fine with this patch, but we'll need to immediately
> > > improve it on top :-)
> > >
> > > > I would also question that the pipeline handler should blindly merge
> > > > the two as over-writes might happen, but I might be mistaken.
> > >
> > > In theory that's possible, but given that the pipeline handler and IPA
> > > are supposed to be developed in sync, that's something the developer
> > > should be able to control. They could either make sure not conflict
> > > occurs, or implement a more elaborate merge method. I would expect the
> > > former to be the common case, hence the proposal for
> > > ControlList::merge().
> > >
> > > > Anyway, I'm fine having more CTS tests broken waiting for the IPA to
> > > > actually set something there if that's the desired direction. I would
> > > > prefer the other way around as this hinders CTS progresses but I would
> > > > go with what the group prefers.
> > >
> > > No, we can merge this patch first, that's not an issue. The fact that
> > > Niklas and I have pointed out issues doesn't mean they all have to be
> > > fixed as a prerequisite, they can also go on top where it makes sense
> > > :-)
> >
> > Great, but I need one more tag to merge I think
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>
>

Thanks

> Would you be able to implement merge() on top of this ?
>

I currently don't have a use case for this as for CTS only the
pipeline set metadata are required.

I think this will most likely be in the development path of the IPA
component for IPU3.

> > > > > > If we wish to take this in the direction of this patch I think the IPA
> > > > > > interface and IPA should also be updated to not report any metadata.
> > > > > >
> > > > > > >  		info->metadataProcessed = true;
> > > > > > >  		if (frameInfos_.tryComplete(info))
> > > > > > >  			pipe_->completeRequest(request);
>
> --
> Regards,
>
> Laurent Pinchart