[libcamera-devel] libcamera: ipu3: Do not over-write metadata
diff mbox series

Message ID 20210219112257.53307-1-jacopo@jmondi.org
State Accepted
Headers show
Series
  • [libcamera-devel] libcamera: ipu3: Do not over-write metadata
Related show

Commit Message

Jacopo Mondi Feb. 19, 2021, 11:22 a.m. UTC 
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@jmondi.org>
---
 src/libcamera/pipeline/ipu3/ipu3.cpp | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

--
2.30.0

Comments

Kieran Bingham Feb. 19, 2021, 1:46 p.m. UTC | #1 
Hi Jacopo,

On 19/02/2021 11:22, 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@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'm wondering if our objects let us do metadata() += controls; here, but
I only as a query if we can update control lists with anther list,
rathar than overwrite it, which is perhaps what was inteneded here.

But even still - it needs full consideration, so it's better removed
until it's actually needed and used.

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

>  		info->metadataProcessed = true;
>  		if (frameInfos_.tryComplete(info))
>  			pipe_->completeRequest(request);
> --
> 2.30.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
>
Niklas Söderlund Feb. 19, 2021, 2:28 p.m. UTC | #2 
Hi Jacopo,

Thanks for your work.

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@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.

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);
> --
> 2.30.0
> 
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel
Laurent Pinchart Feb. 21, 2021, 6:39 p.m. UTC | #3 
Hello,

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@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.

> 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);
Jacopo Mondi Feb. 22, 2021, 9:01 a.m. UTC | #4 
Hello Niklas, Laurent,

On Sun, Feb 21, 2021 at 08:39:52PM +0200, Laurent Pinchart wrote:
> Hello,
>
> 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@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.

I would also question that the pipeline handler should blindly merge
the two as over-writes might happen, but I might be mistaken.

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.

> > 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
Laurent Pinchart Feb. 22, 2021, 9:18 a.m. UTC | #5 
Hi Jacopo,

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@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
:-)

> > > 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);
Jacopo Mondi Feb. 22, 2021, 10:24 a.m. UTC | #6 
Hi Laurent,

On Mon, Feb 22, 2021 at 11:18:51AM +0200, Laurent Pinchart wrote:
> Hi Jacopo,
>
> 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@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

>
> > > > 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
Laurent Pinchart Feb. 22, 2021, 10:26 a.m. UTC | #7 
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@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@ideasonboard.com>

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

> > > > > 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);
Jacopo Mondi Feb. 22, 2021, 10:54 a.m. UTC | #8 
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@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@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

Patch
diff mbox series 

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;
 		info->metadataProcessed = true;
 		if (frameInfos_.tryComplete(info))
 			pipe_->completeRequest(request);