[libcamera-devel] [RFC PATCH v2 1/3] libcamera: camera: Make stop() idempotent

[Nícolas F. R. A. Prado]

Hi Jacopo,

Em 2021-05-08 04:24, Jacopo Mondi escreveu:

> Hi Laurent,
>
> On Sat, May 08, 2021 at 03:03:54AM +0300, Laurent Pinchart wrote:
> > Hello,
> >
> > On Tue, May 04, 2021 at 04:15:17PM -0300, Nícolas F. R. A. Prado wrote:
> > > Em 2021-05-04 12:29, Jacopo Mondi escreveu:
> > > > On Tue, May 04, 2021 at 03:48:16PM +0100, Kieran Bingham wrote:
> > > > > On 04/05/2021 15:31, Nícolas F. R. A. Prado wrote:
> > > > > > Hi Jacopo,
> > > > > >
> > > > > > Em 2021-05-04 04:41, Jacopo Mondi escreveu:
> > > > > >
> > > >
> > > > [snip]
> > > >
> > > > > >> Personally, I would be in favour of calling stop() only when it's
> > > > > >> appropriate instead of shortcutting the camera state machine. What is
> > > > > >> the use case that makes it hard to call stop() only when actually
> > > > > >> required ?
> > > > > >
> > > > > > The use case is always calling stop() in the destructor of the capture in
> > > > > > lc-compliance, which makes the cleanup path simpler as we can safely fail an
> > > > > > assert and still have the camera stopped.
> > > > > >
> > > > > > That said, it wouldn't be hard to call stop() only when required. We could track
> > > > > > whether the camera was started internally in lc-compliance. But it seemed like
> > > > > > this would be a common pattern and handling it by making Camera::stop() itself
> > > > > > be idempotent would be beneficial.
> > > > > >
> > > > > > I'll wait for others to comment on this as well.
> > > > >
> > > > > I think being able to clean up easily on shutdowns is beneficial.
> > > > >
> > > > > I would consider this use case to be that of being able to call
> > > > >
> > > > > 	p = NULL;
> > > > > 	...
> > > > > 	kfree(p);
> > > > >
> > > > > rather than
> > > > > 	if (p)
> > > > > 		kfree(p);
> > > > >
> > > > > It's a convenience feature, so that extra state doesn't have to be
> > > > > repeatedly maintained elsewhere.
> > > >
> > > > Might be a personal opinion, but I would prefer, as a general idea,
> > > > a stricter API. I concur there's not much value in the error message
> > > > when stop() is called from the wrong state, but sometime it helped me
> > > > finding a wrong patter when working on the camera HAL. I guess it
> > > > might be beneficial for other use cases too...
> > >
> > > I think adding a log at level DEBUG or INFO stating that "stop() called for
> > > non-running camera" would help debug those cases while still allowing the
> > > convenience of calling it in any state.
> >
> > Sorry about the conflicting directions :-S
> >
> > There are two options. Calling Camera::stop() when stopped is either
> > valid, in which case the function shouldn't log any message with a level
> > higher than debug, or it's invalid, in which case we should log an
> > error. In the latter case, the caller needs to keep track of the camera
> > state, which is a bit of a hassle. Taking Niklas' example, it would be
> > similar to requiring the caller to have a bool is_p_null variable, as
> > the Camera class doesn't expose its state, so a caller can do
> >
> > 	if (cam->state() == CameraRunning)
> > 		cam->stop();
> >
> > We could expose the camera state, but that increases the surface of the
> > public API, for little gain I believe.
> >
> > Jacopo, what do you think ?
> >
>
> Either we change the camera state machine and allow Stop->Stop by
> adding a state
> Stopping -> Stopping [label = "stop()"];
>
> and change
> isAccessAllowed(Private::CameraStopped,
> Private::CameraRunning);
>
> not to throw any error

But this wouldn't really solve the problem. The Stopping state only happens
while requests are being finished, and the intention here is to make it possible
to call stop() in any state to make cleanup easier.

If we were to add it as part of the state machine, it would mean making each
state (apart from Running) point to itself with label "stop()", as it wouldn't
make sense to transition Acquired to Stopping, for example.

But if we don't want to transition to the Stopping state from a state like
Acquired, it means that we want to return early on stop() for the cases where
the camera isn't Running. This implies not making use of isAccessAllowed().
Because if we did use it, there would have to be two isAccessAllowed() calls in
stop(), like I did in v1 [1]. But as Kieran noted there, this isn't a case of
having access, since all states are accounted for.

Regarding the state diagram, maybe it is okay to omit the fact that "stop()" can
be called from states other than Running (ie keep it the way it is), given that
those are no-ops and aren't there to change the operation but to make it more
convenient? (It will be documented in the stop() function, just not on the state
diagram)

Finally, having an error message in isAccessAllowed() sounds important to warn
about invalid state transitions. It doesn't seem right to me to remove that
message just because stop() is a special case.

These are my thoughts :).

Thanks,
Nícolas

[1] https://lists.libcamera.org/pipermail/libcamera-devel/2021-April/019735.html

>
> Or we mandate applications to track the state themeselves.
>
> Exposing the state has a little gain, as application will
> unconditionally repeat the pattern
>
> if (cam->state() == CameraRunning)
> cam->stop();
>
> hence they could similarly be allowed to just call stop() without
> bothering about the state check.
>
> I would like application to be in charge of tracking the state and
> know what they're doing precisely, but I'm afraid they will just
> call stop() and ignore the error, and I'm not 100% sure it is fair to
> mandate the state tracking burden to apps.
>
> So yes, allowing stop->stop seems fair, but it should be made part of
> the Camera state machine and not worked around by not checking if the
> access is actually allowed or not imho.
>
> Thanks
> j
>
> > > > Anyway, I won't push, whatever's best for the majority is fine with
> > > > me.
> > > >
> > > > > >>>  	int ret = d->isAccessAllowed(Private::CameraRunning);
> > > > > >>>  	if (ret < 0)
> > > > > >>>  		return ret;
> >
> > --
> > Regards,
> >
> > Laurent Pinchart
>
> --
> To unsubscribe, send mail to kernel-unsubscribe at lists.collabora.co.uk.