[libcamera-devel] [PATCH] libcamera: pipeline: ipu3: Stop ImgU and CIO2 on IPA error path

[Jacopo Mondi]

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 at uajain.com>
> > > Suggested-by: Jacopo Mondi <jacopo at 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
> > >
>