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

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

Hi,

Em 2021-05-04 12:29, Jacopo Mondi escreveu:

> Hello,
>
> On Tue, May 04, 2021 at 03:48:16PM +0100, Kieran Bingham wrote:
> > Hi Nicolas, Jacopo,
> >
> > 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.

Thanks,
Nícolas

>
> Anyway, I won't push, whatever's best for the majority is fine with
> me.
>
> Thanks
> j
>
> > --
> > Kieran
> >
> >
> > >
> > > Thanks,
> > > Nícolas
> > >
> > >>
> > >> Thanks
> > >> j
> > >>>  	int ret = d->isAccessAllowed(Private::CameraRunning);
> > >>>  	if (ret < 0)
> > >>>  		return ret;
> > >>> --
> > >>> 2.31.1
> > >>>
> > >>> _______________________________________________
> > >>> libcamera-devel mailing list
> > >>> libcamera-devel at lists.libcamera.org
> > >>> https://lists.libcamera.org/listinfo/libcamera-devel
> > > _______________________________________________
> > > libcamera-devel mailing list
> > > libcamera-devel at lists.libcamera.org
> > > https://lists.libcamera.org/listinfo/libcamera-devel
> > >
> >
> > --
> > Regards
> > --
> > Kieran
>
> --
> To unsubscribe, send mail to kernel-unsubscribe at lists.collabora.co.uk.