| Message ID | 20200325102355.3969245-1-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Delegated to: | Niklas Söderlund |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for the patch. On Wed, Mar 25, 2020 at 11:23:55AM +0100, Niklas Söderlund wrote: > If the parameter buffer is not filled in by the IPA and therefor not s/therefor/therefore/ > used, mark it as already dequeued. If not a request without a parameter s/If not/Otherwise,/ (or "If not,") > buffer will never complete. I'm not sure to follow you there, parameters buffers are not provided in requests. Is this about the IPA not providing a parameters buffer in time ? Doesn't that introduce a bad race condition if we don't provide a parameters buffer for every request ? > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > index 2f909cef7c75ba0f..6f4eafa1523a4a39 100644 > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > @@ -351,12 +351,15 @@ protected: > if (!info) > LOG(RkISP1, Fatal) << "Frame not known"; > > - if (info->paramFilled) > + if (info->paramFilled) { > pipe_->param_->queueBuffer(info->paramBuffer); > - else > + } else { > + /* No need to dequeue param if it's never queued. */ > + info->paramDequeued = true; > LOG(RkISP1, Error) > << "Parameters not ready on time for frame " > << frame() << ", ignore parameters."; How often does this occur ? > + } I think the logic should be simplified. Do we really have a need to dequeue the parameters buffer to complete the request ? It seems we should be able to just dequeue them when they're signalled, and put them in a list of free parameters buffer for later use. > > pipe_->stat_->queueBuffer(info->statBuffer); > pipe_->video_->queueBuffer(info->videoBuffer);
Hi Laurent, Thanks for your feedback. On 2020-03-25 12:41:54 +0200, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Wed, Mar 25, 2020 at 11:23:55AM +0100, Niklas Söderlund wrote: > > If the parameter buffer is not filled in by the IPA and therefor not > > s/therefor/therefore/ > > > used, mark it as already dequeued. If not a request without a parameter > > s/If not/Otherwise,/ (or "If not,") > > > buffer will never complete. > > I'm not sure to follow you there, parameters buffers are not provided in > requests. Is this about the IPA not providing a parameters buffer in > time ? Doesn't that introduce a bad race condition if we don't provide a > parameters buffer for every request ? For RkISP1 when a request is queued to the pipeline it contains no buffers for stats or params. So the first thing that happens at this point is to internally associate buffers for this and inform the IPA that for frame # please use this param buffer. Then the timeline is used to schedule a point in time when this request needs to be queued to hardware. When that time comes and the IPA have not filled in the param buffer it should not be queued to hardware as we don't know what parameters are in it. This is no problem for the hardware as it don't need a param buffer queued to operate. But as we have already associated a param buffer with the request we need mark that we shall not wait for it to dequeue as we never queue it to hardware. > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 +++++-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > index 2f909cef7c75ba0f..6f4eafa1523a4a39 100644 > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > @@ -351,12 +351,15 @@ protected: > > if (!info) > > LOG(RkISP1, Fatal) << "Frame not known"; > > > > - if (info->paramFilled) > > + if (info->paramFilled) { > > pipe_->param_->queueBuffer(info->paramBuffer); > > - else > > + } else { > > + /* No need to dequeue param if it's never queued. */ > > + info->paramDequeued = true; > > LOG(RkISP1, Error) > > << "Parameters not ready on time for frame " > > << frame() << ", ignore parameters."; > > How often does this occur ? Without the ipa thread series I have never seen it. With the ipa thread series I don't see it for cam runs, but for qcam I see them at least every other frame. If I add the opengl rendering series ontop (fps ~3 -> 30) I see them for the first ~10 frames and they are gone. > > > + } > > I think the logic should be simplified. Do we really have a need to > dequeue the parameters buffer to complete the request ? It seems we > should be able to just dequeue them when they're signalled, and put them > in a list of free parameters buffer for later use. I think the current design is good. It makes sure all resources associated with a request when it enters the pipeline are available until the request is marked as completed and "exits" the pipeline handler. I however think we should at some point create a helper class to track requests and additional buffers added to it inside a pipeline handler, as we have a similar need in IPU3. > > > > > pipe_->stat_->queueBuffer(info->statBuffer); > > pipe_->video_->queueBuffer(info->videoBuffer); > > -- > Regards, > > Laurent Pinchart
Hi Niklas, On Wed, Mar 25, 2020 at 01:03:57PM +0100, Niklas Söderlund wrote: > On 2020-03-25 12:41:54 +0200, Laurent Pinchart wrote: > > On Wed, Mar 25, 2020 at 11:23:55AM +0100, Niklas Söderlund wrote: > > > If the parameter buffer is not filled in by the IPA and therefor not > > > > s/therefor/therefore/ > > > > > used, mark it as already dequeued. If not a request without a parameter > > > > s/If not/Otherwise,/ (or "If not,") > > > > > buffer will never complete. > > > > I'm not sure to follow you there, parameters buffers are not provided in > > requests. Is this about the IPA not providing a parameters buffer in > > time ? Doesn't that introduce a bad race condition if we don't provide a > > parameters buffer for every request ? > > For RkISP1 when a request is queued to the pipeline it contains no > buffers for stats or params. So the first thing that happens at this > point is to internally associate buffers for this and inform the IPA > that for frame # please use this param buffer. > > Then the timeline is used to schedule a point in time when this request > needs to be queued to hardware. When that time comes and the IPA have > not filled in the param buffer it should not be queued to hardware as we > don't know what parameters are in it. This is no problem for the > hardware as it don't need a param buffer queued to operate. But as we > have already associated a param buffer with the request we need mark > that we shall not wait for it to dequeue as we never queue it to > hardware. What happens if - request N is queued to the driver with no params buffer - request N+1 is queued to the driver with a param buffer - the driver executes request N Won't the driver take the params buffer of request N+1 for request N ? > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > --- > > > src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 +++++-- > > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > > > diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > index 2f909cef7c75ba0f..6f4eafa1523a4a39 100644 > > > --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp > > > @@ -351,12 +351,15 @@ protected: > > > if (!info) > > > LOG(RkISP1, Fatal) << "Frame not known"; > > > > > > - if (info->paramFilled) > > > + if (info->paramFilled) { > > > pipe_->param_->queueBuffer(info->paramBuffer); > > > - else > > > + } else { > > > + /* No need to dequeue param if it's never queued. */ > > > + info->paramDequeued = true; > > > LOG(RkISP1, Error) > > > << "Parameters not ready on time for frame " > > > << frame() << ", ignore parameters."; > > > > How often does this occur ? > > Without the ipa thread series I have never seen it. > > With the ipa thread series I don't see it for cam runs, but for qcam I > see them at least every other frame. > > If I add the opengl rendering series ontop (fps ~3 -> 30) I see them for > the first ~10 frames and they are gone. That's still pretty bad though, it should be a very uncommon case. > > > > > + } > > > > I think the logic should be simplified. Do we really have a need to > > dequeue the parameters buffer to complete the request ? It seems we > > should be able to just dequeue them when they're signalled, and put them > > in a list of free parameters buffer for later use. > > I think the current design is good. It makes sure all resources > associated with a request when it enters the pipeline are available > until the request is marked as completed and "exits" the pipeline > handler. Acquiring a params buffer at the time the request is queued sounds good to me, but I don't see a reason to delay request completion until the params buffer is dequeued, as it's not needed. This will just delay request completion for no real reason. > I however think we should at some point create a helper class to track > requests and additional buffers added to it inside a pipeline handler, > as we have a similar need in IPU3. > > > > > > > pipe_->stat_->queueBuffer(info->statBuffer); > > > pipe_->video_->queueBuffer(info->videoBuffer);
diff --git a/src/libcamera/pipeline/rkisp1/rkisp1.cpp b/src/libcamera/pipeline/rkisp1/rkisp1.cpp index 2f909cef7c75ba0f..6f4eafa1523a4a39 100644 --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp @@ -351,12 +351,15 @@ protected: if (!info) LOG(RkISP1, Fatal) << "Frame not known"; - if (info->paramFilled) + if (info->paramFilled) { pipe_->param_->queueBuffer(info->paramBuffer); - else + } else { + /* No need to dequeue param if it's never queued. */ + info->paramDequeued = true; LOG(RkISP1, Error) << "Parameters not ready on time for frame " << frame() << ", ignore parameters."; + } pipe_->stat_->queueBuffer(info->statBuffer); pipe_->video_->queueBuffer(info->videoBuffer);
If the parameter buffer is not filled in by the IPA and therefor not used, mark it as already dequeued. If not a request without a parameter buffer will never complete. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/libcamera/pipeline/rkisp1/rkisp1.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)