| Message ID | 20200326160819.4088361-7-niklas.soderlund@ragnatech.se |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Niklas, Thank you for the patch. On Thu, Mar 26, 2020 at 05:08:18PM +0100, Niklas Söderlund wrote: > Call the IPA start()/stop() functions before/after the camera is > started. This makes sure the IPA functions properly once thread > management is moved into the start/stop interface. > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/pipeline/vimc/vimc.cpp | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp > index b04a9726efa5ade6..c5eea3a01b0e9446 100644 > --- a/src/libcamera/pipeline/vimc/vimc.cpp > +++ b/src/libcamera/pipeline/vimc/vimc.cpp > @@ -274,8 +274,15 @@ int PipelineHandlerVimc::start(Camera *camera) > if (ret < 0) > return ret; > > + ret = data->ipa_->start(); > + if (ret) { > + data->video_->releaseBuffers(); > + return ret; > + } > + > ret = data->video_->streamOn(); > if (ret < 0) { > + data->ipa_->stop(); > data->video_->releaseBuffers(); > return ret; > } > @@ -287,6 +294,7 @@ void PipelineHandlerVimc::stop(Camera *camera) > { > VimcCameraData *data = cameraData(camera); > data->video_->streamOff(); > + data->ipa_->stop(); Conceptually speaking I think the IPA should be stopped first (and started last), as I see it "depending" on the operation of the pipeline handler, but that makes absolutely no difference in practice here. It should however be consistent between the rkisp1 and vimc pipeline handlers. Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > data->video_->releaseBuffers(); > } >
Hi Laurent, Thanks for your feedback. On 2020-03-26 23:29:23 +0200, Laurent Pinchart wrote: > Hi Niklas, > > Thank you for the patch. > > On Thu, Mar 26, 2020 at 05:08:18PM +0100, Niklas Söderlund wrote: > > Call the IPA start()/stop() functions before/after the camera is > > started. This makes sure the IPA functions properly once thread > > management is moved into the start/stop interface. > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > --- > > src/libcamera/pipeline/vimc/vimc.cpp | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp > > index b04a9726efa5ade6..c5eea3a01b0e9446 100644 > > --- a/src/libcamera/pipeline/vimc/vimc.cpp > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp > > @@ -274,8 +274,15 @@ int PipelineHandlerVimc::start(Camera *camera) > > if (ret < 0) > > return ret; > > > > + ret = data->ipa_->start(); > > + if (ret) { > > + data->video_->releaseBuffers(); > > + return ret; > > + } > > + > > ret = data->video_->streamOn(); > > if (ret < 0) { > > + data->ipa_->stop(); > > data->video_->releaseBuffers(); > > return ret; > > } > > @@ -287,6 +294,7 @@ void PipelineHandlerVimc::stop(Camera *camera) > > { > > VimcCameraData *data = cameraData(camera); > > data->video_->streamOff(); > > + data->ipa_->stop(); > > Conceptually speaking I think the IPA should be stopped first (and > started last), as I see it "depending" on the operation of the pipeline > handler, but that makes absolutely no difference in practice here. It > should however be consistent between the rkisp1 and vimc pipeline > handlers. I wrestled a bit with this when writing the IPA. As the ipa_ is stored in the base class CameraData shall we move the ipa_-{start,stop}() calls to the camera class start/stop to force them being called uniformly for all pipelines? > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > data->video_->releaseBuffers(); > > } > > > > -- > Regards, > > Laurent Pinchart
Hi Niklas, On Thu, Mar 26, 2020 at 10:43:18PM +0100, Niklas Söderlund wrote: > On 2020-03-26 23:29:23 +0200, Laurent Pinchart wrote: > > On Thu, Mar 26, 2020 at 05:08:18PM +0100, Niklas Söderlund wrote: > > > Call the IPA start()/stop() functions before/after the camera is > > > started. This makes sure the IPA functions properly once thread > > > management is moved into the start/stop interface. > > > > > > Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > > > --- > > > src/libcamera/pipeline/vimc/vimc.cpp | 8 ++++++++ > > > 1 file changed, 8 insertions(+) > > > > > > diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp > > > index b04a9726efa5ade6..c5eea3a01b0e9446 100644 > > > --- a/src/libcamera/pipeline/vimc/vimc.cpp > > > +++ b/src/libcamera/pipeline/vimc/vimc.cpp > > > @@ -274,8 +274,15 @@ int PipelineHandlerVimc::start(Camera *camera) > > > if (ret < 0) > > > return ret; > > > > > > + ret = data->ipa_->start(); > > > + if (ret) { > > > + data->video_->releaseBuffers(); > > > + return ret; > > > + } > > > + > > > ret = data->video_->streamOn(); > > > if (ret < 0) { > > > + data->ipa_->stop(); > > > data->video_->releaseBuffers(); > > > return ret; > > > } > > > @@ -287,6 +294,7 @@ void PipelineHandlerVimc::stop(Camera *camera) > > > { > > > VimcCameraData *data = cameraData(camera); > > > data->video_->streamOff(); > > > + data->ipa_->stop(); > > > > Conceptually speaking I think the IPA should be stopped first (and > > started last), as I see it "depending" on the operation of the pipeline > > handler, but that makes absolutely no difference in practice here. It > > should however be consistent between the rkisp1 and vimc pipeline > > handlers. > > I wrestled a bit with this when writing the IPA. As the ipa_ is stored > in the base class CameraData shall we move the ipa_-{start,stop}() calls > to the camera class start/stop to force them being called uniformly for > all pipelines? I think there's room for standardization, but I don't think it belongs to the Camera class, as the IPA is for the pipeline handler to control. CameraData is a bit of a misnomer, it should be PipelineCameraData or something similar, but that's longer. I'd like, when the IPAs will stabilize a bit, to see how we could have helpers in the base PipelineHandler class, but I don't think now is the right time. > > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > > > > data->video_->releaseBuffers(); > > > } > > >
diff --git a/src/libcamera/pipeline/vimc/vimc.cpp b/src/libcamera/pipeline/vimc/vimc.cpp index b04a9726efa5ade6..c5eea3a01b0e9446 100644 --- a/src/libcamera/pipeline/vimc/vimc.cpp +++ b/src/libcamera/pipeline/vimc/vimc.cpp @@ -274,8 +274,15 @@ int PipelineHandlerVimc::start(Camera *camera) if (ret < 0) return ret; + ret = data->ipa_->start(); + if (ret) { + data->video_->releaseBuffers(); + return ret; + } + ret = data->video_->streamOn(); if (ret < 0) { + data->ipa_->stop(); data->video_->releaseBuffers(); return ret; } @@ -287,6 +294,7 @@ void PipelineHandlerVimc::stop(Camera *camera) { VimcCameraData *data = cameraData(camera); data->video_->streamOff(); + data->ipa_->stop(); data->video_->releaseBuffers(); }
Call the IPA start()/stop() functions before/after the camera is started. This makes sure the IPA functions properly once thread management is moved into the start/stop interface. Signed-off-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> --- src/libcamera/pipeline/vimc/vimc.cpp | 8 ++++++++ 1 file changed, 8 insertions(+)