| Message ID | 20200813005246.3265807-5-niklas.soderlund@ragnatech.se |
|---|---|
| State | Accepted |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, On Thu, Aug 13, 2020 at 02:52:37AM +0200, Niklas Söderlund wrote: > The buffer ready handlers are designed for a single application facing > stream from the main path. To prepare for multiple application facing > streams from main and/or self path the handlers need to be prepared. > > The data keeping tasks of frame number and advancing the timeline can be > moved from the application facing buffer ready handler to the statistics > handler. For each request processed there will always be a statistic > buffer and as the ISP is inline and is the source of both main, self and > statistic paths there is no lose in precision from this. > > The application facing handler no longer needs a special case for Why isn't this required anymore ? > cancelled frames and can be made simpler. With this change the handlers > are ready to deal with any combinations of application facing streams. It's alla bit terse.. Don't repeat 'application facing' all the times and it might read better. We don't have internal streams for this pipeline after all, am I wrong ? > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 +++++------------ > 1 file changed, 5 insertions(+), 12 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 38382a6894dac22a..a1cda12d244f2d97 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -1074,20 +1074,8 @@ void PipelineHandlerRkISP1::tryCompleteRequest(Request *request) > void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer) > { > ASSERT(activeCamera_); > - RkISP1CameraData *data = cameraData(activeCamera_); > Request *request = buffer->request(); > > - if (buffer->metadata().status == FrameMetadata::FrameCancelled) { > - completeBuffer(activeCamera_, request, buffer); > - completeRequest(activeCamera_, request); > - return; > - } > - > - data->timeline_.bufferReady(buffer); > - > - if (data->frame_ <= buffer->metadata().sequence) > - data->frame_ = buffer->metadata().sequence + 1; > - > completeBuffer(activeCamera_, request, buffer); > tryCompleteRequest(request); > } > @@ -1118,6 +1106,11 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer) > if (!info) > return; > > + data->timeline_.bufferReady(buffer); > + > + if (data->frame_ <= buffer->metadata().sequence) > + data->frame_ = buffer->metadata().sequence + 1; > + So we're moving advancing the timeline and sequence number from buffer complete to stat complete, as we are sure there's one single stat for each frame processed by the ISP, right ? Seems fair to me, but there might be implications I cannot see at the moment (ie. is stat mandatory to be queued with buffers ? How does this play with RAW capture, asking as I don't know where we do get RAW frames on RkISP1) Seems legit to me Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> Thanks j > IPAOperationData op; > op.operation = RKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER; > op.data = { info->frame, info->statBuffer->cookie() }; > -- > 2.28.0 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Niklas, Thank you for the patch. On Thu, Aug 13, 2020 at 02:52:37AM +0200, Niklas Söderlund wrote: > The buffer ready handlers are designed for a single application facing > stream from the main path. To prepare for multiple application facing > streams from main and/or self path the handlers need to be prepared. > > The data keeping tasks of frame number and advancing the timeline can be > moved from the application facing buffer ready handler to the statistics > handler. For each request processed there will always be a statistic > buffer and as the ISP is inline and is the source of both main, self and > statistic paths there is no lose in precision from this. > > The application facing handler no longer needs a special case for > cancelled frames and can be made simpler. With this change the handlers > are ready to deal with any combinations of application facing streams. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 +++++------------ > 1 file changed, 5 insertions(+), 12 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 38382a6894dac22a..a1cda12d244f2d97 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -1074,20 +1074,8 @@ void PipelineHandlerRkISP1::tryCompleteRequest(Request *request) > void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer) > { > ASSERT(activeCamera_); > - RkISP1CameraData *data = cameraData(activeCamera_); > Request *request = buffer->request(); > > - if (buffer->metadata().status == FrameMetadata::FrameCancelled) { > - completeBuffer(activeCamera_, request, buffer); > - completeRequest(activeCamera_, request); > - return; > - } > - > - data->timeline_.bufferReady(buffer); > - > - if (data->frame_ <= buffer->metadata().sequence) > - data->frame_ = buffer->metadata().sequence + 1; > - > completeBuffer(activeCamera_, request, buffer); > tryCompleteRequest(request); > } > @@ -1118,6 +1106,11 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer) > if (!info) > return; > > + data->timeline_.bufferReady(buffer); > + > + if (data->frame_ <= buffer->metadata().sequence) > + data->frame_ = buffer->metadata().sequence + 1; > + This should work for now, but may cause issues later, as we should fill the request with metadata before completing it. That will however require more rework, so, for now, Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > IPAOperationData op; > op.operation = RKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER; > op.data = { info->frame, info->statBuffer->cookie() };
Hi Jacopo, On 2020-08-20 10:30:41 +0200, Jacopo Mondi wrote: > Hi Niklas, > > On Thu, Aug 13, 2020 at 02:52:37AM +0200, Niklas Söderlund wrote: > > The buffer ready handlers are designed for a single application facing > > stream from the main path. To prepare for multiple application facing > > streams from main and/or self path the handlers need to be prepared. > > > > The data keeping tasks of frame number and advancing the timeline can be > > moved from the application facing buffer ready handler to the statistics > > handler. For each request processed there will always be a statistic > > buffer and as the ISP is inline and is the source of both main, self and > > statistic paths there is no lose in precision from this. > > > > The application facing handler no longer needs a special case for > > Why isn't this required anymore ? We only needed it to not interfere with the timeline if the buffer was cancelled. As the actions taken for cancelled buffers are the same as taken for the buffer after the timeline was modified this was poorly structured in the first place (my bad). > > > cancelled frames and can be made simpler. With this change the handlers > > are ready to deal with any combinations of application facing streams. > > It's alla bit terse.. Don't repeat 'application facing' all the times > and it might read better. > We don't have internal streams for this pipeline after all, am I wrong ? We have internal "streams" for parameters and statistic for the RkISP1 pipeline. > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 +++++------------ > > 1 file changed, 5 insertions(+), 12 deletions(-) > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index 38382a6894dac22a..a1cda12d244f2d97 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -1074,20 +1074,8 @@ void PipelineHandlerRkISP1::tryCompleteRequest(Request *request) > > void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer) > > { > > ASSERT(activeCamera_); > > - RkISP1CameraData *data = cameraData(activeCamera_); > > Request *request = buffer->request(); > > > > - if (buffer->metadata().status == FrameMetadata::FrameCancelled) { > > - completeBuffer(activeCamera_, request, buffer); > > - completeRequest(activeCamera_, request); > > - return; > > - } > > - > > - data->timeline_.bufferReady(buffer); > > - > > - if (data->frame_ <= buffer->metadata().sequence) > > - data->frame_ = buffer->metadata().sequence + 1; > > - > > completeBuffer(activeCamera_, request, buffer); > > tryCompleteRequest(request); > > } > > @@ -1118,6 +1106,11 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer) > > if (!info) > > return; > > > > + data->timeline_.bufferReady(buffer); > > + > > + if (data->frame_ <= buffer->metadata().sequence) > > + data->frame_ = buffer->metadata().sequence + 1; > > + > > So we're moving advancing the timeline and sequence number from buffer > complete to stat complete, as we are sure there's one single stat for > each frame processed by the ISP, right ? > > Seems fair to me, but there might be implications I cannot see at the > moment (ie. is stat mandatory to be queued with buffers ? How does > this play with RAW capture, asking as I don't know where we do get RAW > frames on RkISP1) RAW is not (yet) supported by this pipeline so it won't effect it at all. I have placed with adding RAW support as part of this series but ran into a can of worms unrelated to this series so I will do so on top. But in short the statistic buffers can still be queued for RAW so I have no concern this effect will effect that work. > > Seems legit to me > Reviewed-by: Jacopo Mondi <jacopo@jmondi.org> > > Thanks > j > > > IPAOperationData op; > > op.operation = RKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER; > > op.data = { info->frame, info->statBuffer->cookie() }; > > -- > > 2.28.0 > > > > _______________________________________________ > > libcamera-devel mailing list > > libcamera-devel@lists.libcamera.org > > https://lists.libcamera.org/listinfo/libcamera-devel
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 38382a6894dac22a..a1cda12d244f2d97 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -1074,20 +1074,8 @@ void PipelineHandlerRkISP1::tryCompleteRequest(Request *request) void PipelineHandlerRkISP1::bufferReady(FrameBuffer *buffer) { ASSERT(activeCamera_); - RkISP1CameraData *data = cameraData(activeCamera_); Request *request = buffer->request(); - if (buffer->metadata().status == FrameMetadata::FrameCancelled) { - completeBuffer(activeCamera_, request, buffer); - completeRequest(activeCamera_, request); - return; - } - - data->timeline_.bufferReady(buffer); - - if (data->frame_ <= buffer->metadata().sequence) - data->frame_ = buffer->metadata().sequence + 1; - completeBuffer(activeCamera_, request, buffer); tryCompleteRequest(request); } @@ -1118,6 +1106,11 @@ void PipelineHandlerRkISP1::statReady(FrameBuffer *buffer) if (!info) return; + data->timeline_.bufferReady(buffer); + + if (data->frame_ <= buffer->metadata().sequence) + data->frame_ = buffer->metadata().sequence + 1; + IPAOperationData op; op.operation = RKISP1_IPA_EVENT_SIGNAL_STAT_BUFFER; op.data = { info->frame, info->statBuffer->cookie() };
The buffer ready handlers are designed for a single application facing stream from the main path. To prepare for multiple application facing streams from main and/or self path the handlers need to be prepared. The data keeping tasks of frame number and advancing the timeline can be moved from the application facing buffer ready handler to the statistics handler. For each request processed there will always be a statistic buffer and as the ISP is inline and is the source of both main, self and statistic paths there is no lose in precision from this. The application facing handler no longer needs a special case for cancelled frames and can be made simpler. With this change the handlers are ready to deal with any combinations of application facing streams. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 17 +++++------------ 1 file changed, 5 insertions(+), 12 deletions(-)