| Message ID | 20210108180113.17039-1-email@uajain.com |
|---|---|
| State | Superseded |
| Delegated to: | Umang Jain |
| Headers | show |
| Series |
|
| Related | show |
Hi Umang, On Fri, Jan 08, 2021 at 11:31:13PM +0530, Umang Jain wrote: > Do not let freeBuffers() run before ImgU and CIO2 are stopped on IPA > configuration failure path. > This is based on Niklas' in-review series, isn't it ? > Signed-off-by: Umang Jain <email@uajain.com> > Suggested-by: Jacopo Mondi <jacopo@jmondi.org> > Change-Id: Iadf0c950c24dcd3b6788275e36f2c028fbc53d7b Ups, not Change-Id please :) > --- > 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 6cd1879a..3c7f98a9 100644 > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > @@ -694,6 +694,8 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] ControlList *con > if ((result.operation != IPU3_IPA_STATUS_CONFIGURATION) || > (result.data.size() != 1) || (result.data.at(0) != 1)) { > LOG(IPU3, Warning) << "Failed to configure IPA"; > + imgu->stop(); > + cio2->stop(); I would rather move these two instruction under the error: label and remove them from the above error paths Thanks j > ret = -EINVAL; > goto error; > } > -- > 2.29.2 >
Hi Jacopo On 1/8/21 11:36 PM, Jacopo Mondi wrote: > Hi Umang, > > On Fri, Jan 08, 2021 at 11:31:13PM +0530, Umang Jain wrote: >> Do not let freeBuffers() run before ImgU and CIO2 are stopped on IPA >> configuration failure path. >> > This is based on Niklas' in-review series, isn't it ? > Right. I realized it now, for some reason, I assumed it's in master :-S >> Signed-off-by: Umang Jain <email@uajain.com> >> Suggested-by: Jacopo Mondi <jacopo@jmondi.org> >> Change-Id: Iadf0c950c24dcd3b6788275e36f2c028fbc53d7b > Ups, not Change-Id please :) > >> --- >> 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 6cd1879a..3c7f98a9 100644 >> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp >> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp >> @@ -694,6 +694,8 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] ControlList *con >> if ((result.operation != IPU3_IPA_STATUS_CONFIGURATION) || >> (result.data.size() != 1) || (result.data.at(0) != 1)) { >> LOG(IPU3, Warning) << "Failed to configure IPA"; >> + imgu->stop(); >> + cio2->stop(); > I would rather move these two instruction under the error: label and > remove them from the above error paths If we move these two instructions under the error: label : I spent some time looking into effects of different permutations on error: label, for e.g. if cio2 fails and jumps to error: - it shall imgu->stop() directly (without starting it first). I concluded on the point that stopping Imgu (cio2 for that matter) _directly_ won't be a problem. The stop() in both cases calls to ioctl(VIDIOC_STREAMOFF,..) - which has clear documentation for calling it directly (without having a call to ioctl(VIDIOC_STREAMON, ...) https://www.kernel.org/doc/html/v5.4/media/uapi/v4l/vidioc-streamon.html Does it makes sense? > > Thanks > j > >> ret = -EINVAL; >> goto error; >> } >> -- >> 2.29.2 >>
Hi Uman, On Mon, Jan 11, 2021 at 02:05:00PM +0530, Umang Jain wrote: > Hi Jacopo > > On 1/8/21 11:36 PM, Jacopo Mondi wrote: > > Hi Umang, > > > > On Fri, Jan 08, 2021 at 11:31:13PM +0530, Umang Jain wrote: > > > Do not let freeBuffers() run before ImgU and CIO2 are stopped on IPA > > > configuration failure path. > > > > > This is based on Niklas' in-review series, isn't it ? > > > Right. I realized it now, for some reason, I assumed it's in master :-S > > > Signed-off-by: Umang Jain <email@uajain.com> > > > Suggested-by: Jacopo Mondi <jacopo@jmondi.org> > > > Change-Id: Iadf0c950c24dcd3b6788275e36f2c028fbc53d7b > > Ups, not Change-Id please :) > > > > > --- > > > 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 6cd1879a..3c7f98a9 100644 > > > --- a/src/libcamera/pipeline/ipu3/ipu3.cpp > > > +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp > > > @@ -694,6 +694,8 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] ControlList *con > > > if ((result.operation != IPU3_IPA_STATUS_CONFIGURATION) || > > > (result.data.size() != 1) || (result.data.at(0) != 1)) { > > > LOG(IPU3, Warning) << "Failed to configure IPA"; > > > + imgu->stop(); > > > + cio2->stop(); > > I would rather move these two instruction under the error: label and > > remove them from the above error paths > If we move these two instructions under the error: label : > > I spent some time looking into effects of different permutations on error: > label, for e.g. if cio2 fails and jumps to error: - it shall imgu->stop() > directly (without starting it first). I concluded on the point that stopping > Imgu (cio2 for that matter) _directly_ won't be a problem. The stop() in > both cases calls to ioctl(VIDIOC_STREAMOFF,..) - which has clear > documentation for calling it directly (without having a call to > ioctl(VIDIOC_STREAMON, ...) > https://www.kernel.org/doc/html/v5.4/media/uapi/v4l/vidioc-streamon.html > > Does it makes sense? Yes, STREAMOFF should be harmeless to be called if STREAMON was not called. Otherwise, the error path could be made: ret = cio2->start(); if (ret) return ret; ret = imgu->start(); if (ret) goto error_cio2_stop; ret = .... if (ret) goto error_imgu_stop; error_imgu_stop: imgu->stop(); error_cio2_stop: cio2->stop(); freeBuffers(camera); return ret; C++ is sometimes nasty with gotos, I haven't tried compiling this bit. > > > > Thanks > > j > > > > > ret = -EINVAL; > > > goto error; > > > } > > > -- > > > 2.29.2 > > > >
Hi Umang, Jacopo, On 11/01/2021 09:25, Jacopo Mondi wrote: > Hi Uman, > > On Mon, Jan 11, 2021 at 02:05:00PM +0530, Umang Jain wrote: >> Hi Jacopo >> >> On 1/8/21 11:36 PM, Jacopo Mondi wrote: >>> Hi Umang, >>> >>> On Fri, Jan 08, 2021 at 11:31:13PM +0530, Umang Jain wrote: >>>> Do not let freeBuffers() run before ImgU and CIO2 are stopped on IPA >>>> configuration failure path. >>>> >>> This is based on Niklas' in-review series, isn't it ? >>> >> Right. I realized it now, for some reason, I assumed it's in master :-S >>>> Signed-off-by: Umang Jain <email@uajain.com> >>>> Suggested-by: Jacopo Mondi <jacopo@jmondi.org> >>>> Change-Id: Iadf0c950c24dcd3b6788275e36f2c028fbc53d7b >>> Ups, not Change-Id please :) >>> >>>> --- >>>> 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 6cd1879a..3c7f98a9 100644 >>>> --- a/src/libcamera/pipeline/ipu3/ipu3.cpp >>>> +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp >>>> @@ -694,6 +694,8 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] ControlList *con >>>> if ((result.operation != IPU3_IPA_STATUS_CONFIGURATION) || >>>> (result.data.size() != 1) || (result.data.at(0) != 1)) { >>>> LOG(IPU3, Warning) << "Failed to configure IPA"; >>>> + imgu->stop(); >>>> + cio2->stop(); >>> I would rather move these two instruction under the error: label and >>> remove them from the above error paths >> If we move these two instructions under the error: label : >> >> I spent some time looking into effects of different permutations on error: >> label, for e.g. if cio2 fails and jumps to error: - it shall imgu->stop() >> directly (without starting it first). I concluded on the point that stopping >> Imgu (cio2 for that matter) _directly_ won't be a problem. The stop() in >> both cases calls to ioctl(VIDIOC_STREAMOFF,..) - which has clear >> documentation for calling it directly (without having a call to >> ioctl(VIDIOC_STREAMON, ...) >> https://www.kernel.org/doc/html/v5.4/media/uapi/v4l/vidioc-streamon.html >> >> Does it makes sense? > > Yes, STREAMOFF should be harmeless to be called if STREAMON was not > called. > I believe that statement is what stopped me from merging: "libcamera: v4l2_videodevice: Track streaming state" https://patchwork.libcamera.org/patch/2221/ As I recall it was deemed unnecessary (though has RB tags) > Otherwise, the error path could be made: > > ret = cio2->start(); > if (ret) > return ret; > > ret = imgu->start(); > if (ret) > goto error_cio2_stop; > > ret = .... > if (ret) > goto error_imgu_stop; > > error_imgu_stop: > imgu->stop(); > error_cio2_stop: > cio2->stop(); > freeBuffers(camera); > return ret; > > C++ is sometimes nasty with gotos, I haven't tried compiling this bit. I would say that if it's safe to call stop() we can just keep it simple and call stop regardless on all devices. Otherwise if there is an issue calling stop on a stopped device from V4L2, we can apply the referenced patch, to track state in the object, and then we can call stop() regardless ;-) -- Kieran > >>> >>> Thanks >>> j >>> >>>> ret = -EINVAL; >>>> goto error; >>>> } >>>> -- >>>> 2.29.2 >>>> >> > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel >
diff --git a/src/libcamera/pipeline/ipu3/ipu3.cpp b/src/libcamera/pipeline/ipu3/ipu3.cpp index 6cd1879a..3c7f98a9 100644 --- a/src/libcamera/pipeline/ipu3/ipu3.cpp +++ b/src/libcamera/pipeline/ipu3/ipu3.cpp @@ -694,6 +694,8 @@ int PipelineHandlerIPU3::start(Camera *camera, [[maybe_unused]] ControlList *con if ((result.operation != IPU3_IPA_STATUS_CONFIGURATION) || (result.data.size() != 1) || (result.data.at(0) != 1)) { LOG(IPU3, Warning) << "Failed to configure IPA"; + imgu->stop(); + cio2->stop(); ret = -EINVAL; goto error; }
Do not let freeBuffers() run before ImgU and CIO2 are stopped on IPA configuration failure path. Signed-off-by: Umang Jain <email@uajain.com> Suggested-by: Jacopo Mondi <jacopo@jmondi.org> Change-Id: Iadf0c950c24dcd3b6788275e36f2c028fbc53d7b --- src/libcamera/pipeline/ipu3/ipu3.cpp | 2 ++ 1 file changed, 2 insertions(+)