| Message ID | 20210312054727.852622-9-kieran.bingham@ideasonboard.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran On Fri, Mar 12, 2021 at 05:47:27AM +0000, 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. As this is a pipeline development issue, Fatal seems appropriate to have it fixed during development. Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index c5facaea16dd..a61e2b51ef9e 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -1168,8 +1168,10 @@ void IPU3CameraData::queueFrameAction(unsigned int id, > } > case ipa::ipu3::ActionParamFilled: { > IPU3Frames::Info *info = frameInfos_.find(id); > - if (!info) > + if (!info) { > + LOG(IPU3, Fatal) << "No frameInfo for ActionParamFilled on ID: " << id; > break; > + } > > /* Queue all buffers from the request aimed for the ImgU. */ > for (auto it : info->request->buffers()) { > @@ -1190,8 +1192,10 @@ void IPU3CameraData::queueFrameAction(unsigned int id, > } > case ipa::ipu3::ActionMetadataReady: { > IPU3Frames::Info *info = frameInfos_.find(id); > - if (!info) > + if (!info) { > + LOG(IPU3, Fatal) << "No frameInfo for ActionMetadataReady on ID: " << id; > break; > + } > > /* > * \todo Parse the value of the controls returned by the IPA > -- > 2.25.1 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Kieran, Thank you for the patch. On Fri, Mar 12, 2021 at 05:47:27AM +0000, 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> > --- > src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index c5facaea16dd..a61e2b51ef9e 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -1168,8 +1168,10 @@ void IPU3CameraData::queueFrameAction(unsigned int id, > } > case ipa::ipu3::ActionParamFilled: { > IPU3Frames::Info *info = frameInfos_.find(id); > - if (!info) > + if (!info) { > + LOG(IPU3, Fatal) << "No frameInfo for ActionParamFilled on ID: " << id; s/ID:/ID/ ? Let's shorten lines a bit. LOG(IPU3, Fatal) << "No frameInfo for ActionParamFilled on ID " << id; Same below. This being said, couldn't we more simply turn the existing Error message in IPU3Frames::find(unsigned int id) into a Fatal message ? I also wonder if the same shouldn't be done for IPU3Frames::find(FrameBuffer *buffer). > break; > + } > > /* Queue all buffers from the request aimed for the ImgU. */ > for (auto it : info->request->buffers()) { > @@ -1190,8 +1192,10 @@ void IPU3CameraData::queueFrameAction(unsigned int id, > } > case ipa::ipu3::ActionMetadataReady: { > IPU3Frames::Info *info = frameInfos_.find(id); > - if (!info) > + if (!info) { > + LOG(IPU3, Fatal) << "No frameInfo for ActionMetadataReady on ID: " << id; > break; > + } > > /* > * \todo Parse the value of the controls returned by the IPA
Hi Laurent, On 13/03/2021 23:13, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Fri, Mar 12, 2021 at 05:47:27AM +0000, 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> >> --- >> src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++++-- >> 1 file changed, 6 insertions(+), 2 deletions(-) >> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp >> index c5facaea16dd..a61e2b51ef9e 100644 >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp >> @@ -1168,8 +1168,10 @@ void IPU3CameraData::queueFrameAction(unsigned int id, >> } >> case ipa::ipu3::ActionParamFilled: { >> IPU3Frames::Info *info = frameInfos_.find(id); >> - if (!info) >> + if (!info) { >> + LOG(IPU3, Fatal) << "No frameInfo for ActionParamFilled on ID: " << id; > > s/ID:/ID/ ? > > Let's shorten lines a bit. > > LOG(IPU3, Fatal) > << "No frameInfo for ActionParamFilled on ID " > << id; > > Same below. > > This being said, couldn't we more simply turn the existing Error message > in IPU3Frames::find(unsigned int id) into a Fatal message ? I think I hadn't done that because I thought that there were legitimate reasons for not being able to find a FrameInfo for a buffer. But I don't believe there is. FrameInfo is not removed until the request is completed, so any failure to find it is fatal. Having the internal structure associated with a Request::Private internal pointer would therefore save lookups and be a (nearish) future improvement. I'll make this fully fatal and retest everything. > I also wonder if the same shouldn't be done for > IPU3Frames::find(FrameBuffer *buffer). > >> break; >> + } >> >> /* Queue all buffers from the request aimed for the ImgU. */ >> for (auto it : info->request->buffers()) { >> @@ -1190,8 +1192,10 @@ void IPU3CameraData::queueFrameAction(unsigned int id, >> } >> case ipa::ipu3::ActionMetadataReady: { >> IPU3Frames::Info *info = frameInfos_.find(id); >> - if (!info) >> + if (!info) { >> + LOG(IPU3, Fatal) << "No frameInfo for ActionMetadataReady on ID: " << id; >> break; >> + } >> >> /* >> * \todo Parse the value of the controls returned by the IPA >
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index c5facaea16dd..a61e2b51ef9e 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -1168,8 +1168,10 @@ void IPU3CameraData::queueFrameAction(unsigned int id, } case ipa::ipu3::ActionParamFilled: { IPU3Frames::Info *info = frameInfos_.find(id); - if (!info) + if (!info) { + LOG(IPU3, Fatal) << "No frameInfo for ActionParamFilled on ID: " << id; break; + } /* Queue all buffers from the request aimed for the ImgU. */ for (auto it : info->request->buffers()) { @@ -1190,8 +1192,10 @@ void IPU3CameraData::queueFrameAction(unsigned int id, } case ipa::ipu3::ActionMetadataReady: { IPU3Frames::Info *info = frameInfos_.find(id); - if (!info) + if (!info) { + LOG(IPU3, Fatal) << "No frameInfo for ActionMetadataReady on ID: " << id; break; + } /* * \todo Parse the value of the controls returned by the IPA
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> --- src/libcamera/pipeline/ipu3/ipu3.cpp | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)