| Message ID | 20210312061131.854849-4-kieran.bingham@ideasonboard.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series |
|
| Related | show |
Hi Kieran, Thank you for the patch. On Fri, Mar 12, 2021 at 06:11:26AM +0000, Kieran Bingham wrote: > Reset the request sequence counter back to zero when a camera has > stopped, giving each camera run an independent sequence stream. > > Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > --- > RFC: This again would be perhaps better handled by the base class to > ensure all pipeline handlers handle this in a consistent manner. Yes, if we want to do this, I think it should go in the base class. I'm not sure we should reset the sequence number though, as not doing so may help identifying requests from a previous capture session that leak to a new capture session. > src/libcamera/pipeline/ipu3/ipu3.cpp | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > index a61e2b51ef9e..3d3bd4a43c23 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -780,6 +780,8 @@ void PipelineHandlerIPU3::stop(Camera *camera) > LOG(IPU3, Fatal) << "There are still requests to complete."; > > freeBuffers(camera); > + > + data->requestSequence_ = 0; > } > > int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
Hi Laurent, On 14/03/2021 01:43, Laurent Pinchart wrote: > Hi Kieran, > > Thank you for the patch. > > On Fri, Mar 12, 2021 at 06:11:26AM +0000, Kieran Bingham wrote: >> Reset the request sequence counter back to zero when a camera has >> stopped, giving each camera run an independent sequence stream. >> >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >> --- >> RFC: This again would be perhaps better handled by the base class to >> ensure all pipeline handlers handle this in a consistent manner. > > Yes, if we want to do this, I think it should go in the base class. I'm > not sure we should reset the sequence number though, as not doing so may > help identifying requests from a previous capture session that leak to a > new capture session. I hope that lives in the world of things that now shouldn't be able to happen - but given the exercise that caused this series was entirely about requests from previous streams being used late ... But wouldn't it be more obvious that such an event has happened if the stream starts running and there is a request involved with a high sequence value compared to the expectation of starting from zero? If we expect streams to start at some arbitrary high value each time, it might be harder to notice (without requiring a direct comparison against the 'last known' sequence value). > >> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp >> index a61e2b51ef9e..3d3bd4a43c23 100644 >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp >> @@ -780,6 +780,8 @@ void PipelineHandlerIPU3::stop(Camera *camera) >> LOG(IPU3, Fatal) << "There are still requests to complete."; >> >> freeBuffers(camera); >> + >> + data->requestSequence_ = 0; >> } >> >> int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request) >
Hi Kieran, On Mon, Mar 15, 2021 at 12:13:15PM +0000, Kieran Bingham wrote: > On 14/03/2021 01:43, Laurent Pinchart wrote: > > On Fri, Mar 12, 2021 at 06:11:26AM +0000, Kieran Bingham wrote: > >> Reset the request sequence counter back to zero when a camera has > >> stopped, giving each camera run an independent sequence stream. > >> > >> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> > >> --- > >> RFC: This again would be perhaps better handled by the base class to > >> ensure all pipeline handlers handle this in a consistent manner. > > > > Yes, if we want to do this, I think it should go in the base class. I'm > > not sure we should reset the sequence number though, as not doing so may > > help identifying requests from a previous capture session that leak to a > > new capture session. > > I hope that lives in the world of things that now shouldn't be able to > happen - but given the exercise that caused this series was entirely > about requests from previous streams being used late ... > > But wouldn't it be more obvious that such an event has happened if the > stream starts running and there is a request involved with a high > sequence value compared to the expectation of starting from zero? > > If we expect streams to start at some arbitrary high value each time, it > might be harder to notice (without requiring a direct comparison against > the 'last known' sequence value). You're right, but there's a potential case of the first capture session being very short (let's say just one request in the worst case), resetting the counter would not allow us to distinguish two different "request 0". That's a pathological case though. > >> src/libcamera/pipeline/ipu3/ipu3.cpp | 2 ++ > >> 1 file changed, 2 insertions(+) > >> > >> diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> index a61e2b51ef9e..3d3bd4a43c23 100644 > >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > >> @@ -780,6 +780,8 @@ void PipelineHandlerIPU3::stop(Camera *camera) > >> LOG(IPU3, Fatal) << "There are still requests to complete."; > >> > >> freeBuffers(camera); > >> + > >> + data->requestSequence_ = 0; > >> } > >> > >> int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index a61e2b51ef9e..3d3bd4a43c23 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -780,6 +780,8 @@ void PipelineHandlerIPU3::stop(Camera *camera) LOG(IPU3, Fatal) << "There are still requests to complete."; freeBuffers(camera); + + data->requestSequence_ = 0; } int PipelineHandlerIPU3::queueRequestDevice(Camera *camera, Request *request)
Reset the request sequence counter back to zero when a camera has stopped, giving each camera run an independent sequence stream. Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com> --- RFC: This again would be perhaps better handled by the base class to ensure all pipeline handlers handle this in a consistent manner. src/libcamera/pipeline/ipu3/ipu3.cpp | 2 ++ 1 file changed, 2 insertions(+)