| Message ID | 20210420130741.236848-3-kieran.bingham@ideasonboard.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thanks for the patch ! On 20/04/2021 15:07, Kieran Bingham wrote: > The FrameInfo structure associates the data sent to the IPA > and is essential for handling events. > > If it can not be found, this is a fatal error which must be fixed. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > v3: > - Make all occurrences of failing to find a frame info fatal. > --- > src/libcamera/pipeline/ipu3/frames.cpp | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp > index 03e8131c4829..a1b014eed8d7 100644 > --- a/src/libcamera/pipeline/ipu3/frames.cpp > +++ b/src/libcamera/pipeline/ipu3/frames.cpp > @@ -113,7 +113,8 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id) > if (itInfo != frameInfo_.end()) > return itInfo->second.get(); > > - LOG(IPU3, Error) << "Can't find tracking informaton for frame " << id; > + LOG(IPU3, Fatal) << "Can't find tracking informaton for frame " << id; small typo here: s/informaton/information > + > return nullptr; > } > > @@ -131,7 +132,8 @@ IPU3Frames::Info *IPU3Frames::find(FrameBuffer *buffer) > return info; > } > > - LOG(IPU3, Error) << "Can't find tracking informaton from buffer"; > + LOG(IPU3, Fatal) << "Can't find tracking informaton from buffer"; > + and here :-) > return nullptr; > } > > With those addressed: Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com>
Hi JM, On 20/04/2021 18:20, Jean-Michel Hautbois wrote: > Hi Kieran, > > Thanks for the patch ! > > On 20/04/2021 15:07, Kieran Bingham wrote: >> The FrameInfo structure associates the data sent to the IPA >> and is essential for handling events. >> >> If it can not be found, this is a fatal error which must be fixed. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >> --- >> v3: >> - Make all occurrences of failing to find a frame info fatal. >> --- >> src/libcamera/pipeline/ipu3/frames.cpp | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp >> index 03e8131c4829..a1b014eed8d7 100644 >> --- a/src/libcamera/pipeline/ipu3/frames.cpp >> +++ b/src/libcamera/pipeline/ipu3/frames.cpp >> @@ -113,7 +113,8 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id) >> if (itInfo != frameInfo_.end()) >> return itInfo->second.get(); >> >> - LOG(IPU3, Error) << "Can't find tracking informaton for frame " << id; >> + LOG(IPU3, Fatal) << "Can't find tracking informaton for frame " << id; > small typo here: s/informaton/information >> + >> return nullptr; >> } >> >> @@ -131,7 +132,8 @@ IPU3Frames::Info *IPU3Frames::find(FrameBuffer *buffer) >> return info; >> } >> >> - LOG(IPU3, Error) << "Can't find tracking informaton from buffer"; >> + LOG(IPU3, Fatal) << "Can't find tracking informaton from buffer"; >> + > and here :-) aha, good spot. Can't believe I missed that. Not actually related to the change in this patch - but worth fixing up at the same time, so I've updated the patch to include it. Thanks >> return nullptr; >> } >> >> > > With those addressed: > Reviewed-by: Jean-Michel Hautbois <jeanmichel.hautbois@ideasonboard.com> >
Hi Kieran, Thank you for the patch. On Tue, Apr 20, 2021 at 02:07:37PM +0100, Kieran Bingham wrote: > The FrameInfo structure associates the data sent to the IPA > and is essential for handling events. > > If it can not be found, this is a fatal error which must be fixed. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > v3: > - Make all occurrences of failing to find a frame info fatal. > --- > src/libcamera/pipeline/ipu3/frames.cpp | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp > index 03e8131c4829..a1b014eed8d7 100644 > --- a/src/libcamera/pipeline/ipu3/frames.cpp > +++ b/src/libcamera/pipeline/ipu3/frames.cpp > @@ -113,7 +113,8 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id) > if (itInfo != frameInfo_.end()) > return itInfo->second.get(); > > - LOG(IPU3, Error) << "Can't find tracking informaton for frame " << id; > + LOG(IPU3, Fatal) << "Can't find tracking informaton for frame " << id; > + > return nullptr; > } > > @@ -131,7 +132,8 @@ IPU3Frames::Info *IPU3Frames::find(FrameBuffer *buffer) > return info; > } > > - LOG(IPU3, Error) << "Can't find tracking informaton from buffer"; > + LOG(IPU3, Fatal) << "Can't find tracking informaton from buffer"; In addition to the informaton typo, should this read s/from buffer/for buffer/ ? Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > + > return nullptr; > } >
Hi Kieran, Thanks for the patch, On Wed, Apr 21, 2021 at 7:19 AM Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote: > > Hi Kieran, > > Thank you for the patch. > > On Tue, Apr 20, 2021 at 02:07:37PM +0100, Kieran Bingham wrote: > > The FrameInfo structure associates the data sent to the IPA > > and is essential for handling events. > > > > If it can not be found, this is a fatal error which must be fixed. > > > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > > v3: > > - Make all occurrences of failing to find a frame info fatal. > > --- > > src/libcamera/pipeline/ipu3/frames.cpp | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp > > index 03e8131c4829..a1b014eed8d7 100644 > > --- a/src/libcamera/pipeline/ipu3/frames.cpp > > +++ b/src/libcamera/pipeline/ipu3/frames.cpp > > @@ -113,7 +113,8 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id) > > if (itInfo != frameInfo_.end()) > > return itInfo->second.get(); > > > > - LOG(IPU3, Error) << "Can't find tracking informaton for frame " << id; > > + LOG(IPU3, Fatal) << "Can't find tracking informaton for frame " << id; > > + > > return nullptr; > > } > > > > @@ -131,7 +132,8 @@ IPU3Frames::Info *IPU3Frames::find(FrameBuffer *buffer) > > return info; > > } > > > > - LOG(IPU3, Error) << "Can't find tracking informaton from buffer"; > > + LOG(IPU3, Fatal) << "Can't find tracking informaton from buffer"; > > In addition to the informaton typo, should this read s/from buffer/for > buffer/ ? > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > + > > return nullptr; > > } > > > Given the fixes, Reviewed-by: Hirokazu Honda <hiroh@chromium.org> > -- > Regards, > > Laurent Pinchart > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, On 20/04/2021 23:19, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Tue, Apr 20, 2021 at 02:07:37PM +0100, Kieran Bingham wrote: >> The FrameInfo structure associates the data sent to the IPA >> and is essential for handling events. >> >> If it can not be found, this is a fatal error which must be fixed. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> v3: >> - Make all occurrences of failing to find a frame info fatal. >> --- >> src/libcamera/pipeline/ipu3/frames.cpp | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp >> index 03e8131c4829..a1b014eed8d7 100644 >> --- a/src/libcamera/pipeline/ipu3/frames.cpp >> +++ b/src/libcamera/pipeline/ipu3/frames.cpp >> @@ -113,7 +113,8 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id) >> if (itInfo != frameInfo_.end()) >> return itInfo->second.get(); >> >> - LOG(IPU3, Error) << "Can't find tracking informaton for frame " << id; >> + LOG(IPU3, Fatal) << "Can't find tracking informaton for frame " << id; >> + >> return nullptr; >> } >> >> @@ -131,7 +132,8 @@ IPU3Frames::Info *IPU3Frames::find(FrameBuffer *buffer) >> return info; >> } >> >> - LOG(IPU3, Error) << "Can't find tracking informaton from buffer"; >> + LOG(IPU3, Fatal) << "Can't find tracking informaton from buffer"; > > In addition to the informaton typo, should this read s/from buffer/for > buffer/ ? > Both make sense, and both are true, even though they are slightly different. Can't find tracking information 'for' the buffer - I can't identify which tracking state is used for this buffer Can't find tracking information 'from' buffer - Using this buffer, I was unable to find any tracking information I won't bother changing this now, as I hope in the (near?) future to be able to drop these anyway, and have the tracking information obtained from the Request, which itself would be obtained from the buffer (via Buffer->request()) Then we can turn 'searching/lookups' into simply following two pointers, by having a private cookie in the now extensible Request. With that - it won't be possible to 'not find' the entry. (Of course we will still have to guarantee that it is correct). > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > >> + >> return nullptr; >> } >> >
diff --git a/src/libcamera/pipeline/ipu3/frames.cpp b/src/libcamera/pipeline/ipu3/frames.cpp index 03e8131c4829..a1b014eed8d7 100644 --- a/src/libcamera/pipeline/ipu3/frames.cpp +++ b/src/libcamera/pipeline/ipu3/frames.cpp @@ -113,7 +113,8 @@ IPU3Frames::Info *IPU3Frames::find(unsigned int id) if (itInfo != frameInfo_.end()) return itInfo->second.get(); - LOG(IPU3, Error) << "Can't find tracking informaton for frame " << id; + LOG(IPU3, Fatal) << "Can't find tracking informaton for frame " << id; + return nullptr; } @@ -131,7 +132,8 @@ IPU3Frames::Info *IPU3Frames::find(FrameBuffer *buffer) return info; } - LOG(IPU3, Error) << "Can't find tracking informaton from buffer"; + LOG(IPU3, Fatal) << "Can't find tracking informaton from buffer"; + return nullptr; }
The FrameInfo structure associates the data sent to the IPA and is essential for handling events. If it can not be found, this is a fatal error which must be fixed. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- v3: - Make all occurrences of failing to find a frame info fatal. --- src/libcamera/pipeline/ipu3/frames.cpp | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)