[PATCH 4/4] libcamera: Make Camera::Private::isAcquired() protected

[Cheng-Hao Yang]

Hi Kieran,

On Sun, Oct 20, 2024 at 6:58 PM Kieran Bingham
<kieran.bingham at ideasonboard.com> wrote:
>
> Quoting Cheng-Hao Yang (2024-10-20 05:35:20)
> > Hi Kieran,
> >
> > On Sun, Oct 20, 2024 at 6:47 AM Kieran Bingham
> > <kieran.bingham at ideasonboard.com> wrote:
> > >
> > > Quoting Harvey Yang (2024-10-18 08:57:37)
> > > > As some pipeline handlers need to know if a CameraData is currently
> > > > acquired, this patch makes the function protected, instead of private.
> > > >
> > > > For example, the upcoming mtkisp7 needs the information to determine if
> > > > it should send camera disconnect signal.
> > >
> > > Why? If a camera is disconnected - it should always send a disconnect
> > > signal to any listener.
> > >
> > > It means the application must no longer even remember the camera
> > > existed!
> >
> > This is particularly needed for IPA disconnection:
> > Pipeline handler holds the IPA proxy, so it needs to decide if calling
> > a camera's
> > disconnected signal is proper.
>
> That's the part I didn't understand and this patch doesn't tell me.
>
> 'isAcquired' isn't something I would expect to use as a gate for when to
> send a disconnected signal or not. If the IPC is disconne
>
> If an IPA crashes or gets disconnected because the socket is broken -
> the camera is 'inoperable' right? So if we're using the equivalent calls
> of uvc as disconnected - then we should disconnect the whole camera -
> and trigger a camera removed? At least until we can correctly
> reconstruct a new camera ...
>
> Or are you trying to 'keep' the camera but report it to applications
> that it can't be used ? This patch doesn't answer anything about how
> it's expected to be used in a pipeline handler.

This is a good question.
In CrOS, there's a test that expects to receive
CAMERA3_MSG_ERROR_REQUEST when the IPA process crashes [1].
In CrOS apps, the signal itself is enough, and apps will decide to
stop the camera afterwards, IIUC.

I don't have a strong opinion if we need a mechanism in libcamera
core library to remove/keep the camera when the IPA disconnects.

We can also check the state of `isAcquired` in
`Camera::Private::notifyDisconnection()`, which is added in the first
patch.

[1]: https://chromium-review.googlesource.com/c/chromiumos/third_party/libcamera/+/5467266

>
> Perhaps - making sure something is generic - or adding the same
> functionality to the existing pipeline handlers would help demonstrate
> what you're trying to do?
>
> My concern here (this patch is trivial) is that the implementation for
> handling a disconnected IPA should be the same for /all/ pipeline
> handlers. Not something specific to an mtkisp pipeline handler that we
> haven't seen yet.

I think this is controversial: I'm not sure if an IPA is designed to be bound
to a camera or not. I don't think so in the current design. While in CrOS
mtkisp7 at least, this is how it'll be used.

>
> I see PipelineHandler::disconnect() is ultimately the path that would
> then remove the camera from the camera manager.
>
> Interestingly - that removes the whole pipeline handler, which could be
> managing multiple cameras - and presumably - here we are only trying to
> 'remove' a single camera - the pipeline handler itself has not been
> disconnected (and in theory something I presume could try to reload the
> IPA?)

Yes, that was my concern as well, so I ended up adding the first patch.

BR,
Harvey

>
>
>
> --
> Kieran
>
>
> >
> > If you think that IPA proxy disconnection is another thing, apart from
> > a camera's
> > disconnection, we can also consider adding another signal.
> >
> > BR,
> > Harvey
> >
> > >
> > > --
> > > Kieran
> > >
> > >
> > > >
> > > > Signed-off-by: Harvey Yang <chenghaoyang at chromium.org>
> > > > Co-developed-by: Yudhistira Erlandinata <yerlandinata at chromium.org>
> > > > Signed-off-by: Yudhistira Erlandinata <yerlandinata at chromium.org>
> > > > ---
> > > >  include/libcamera/internal/camera.h | 4 +++-
> > > >  src/libcamera/camera.cpp            | 3 +++
> > > >  2 files changed, 6 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/include/libcamera/internal/camera.h b/include/libcamera/internal/camera.h
> > > > index 0bef0980e..fe47a49b7 100644
> > > > --- a/include/libcamera/internal/camera.h
> > > > +++ b/include/libcamera/internal/camera.h
> > > > @@ -43,6 +43,9 @@ public:
> > > >
> > > >         const CameraControlValidator *validator() const { return validator_.get(); }
> > > >
> > > > +protected:
> > > > +       bool isAcquired() const;
> > > > +
> > > >  private:
> > > >         enum State {
> > > >                 CameraAvailable,
> > > > @@ -52,7 +55,6 @@ private:
> > > >                 CameraRunning,
> > > >         };
> > > >
> > > > -       bool isAcquired() const;
> > > >         bool isRunning() const;
> > > >         int isAccessAllowed(State state, bool allowDisconnected = false,
> > > >                             const char *from = __builtin_FUNCTION()) const;
> > > > diff --git a/src/libcamera/camera.cpp b/src/libcamera/camera.cpp
> > > > index ef5a6725f..38aa4ad22 100644
> > > > --- a/src/libcamera/camera.cpp
> > > > +++ b/src/libcamera/camera.cpp
> > > > @@ -670,6 +670,9 @@ static const char *const camera_state_names[] = {
> > > >         "Running",
> > > >  };
> > > >
> > > > +/**
> > > > + * \return True if the camera is acquired, false otherwise
> > > > + */
> > > >  bool Camera::Private::isAcquired() const
> > > >  {
> > > >         return state_.load(std::memory_order_acquire) != CameraAvailable;
> > > > --
> > > > 2.47.0.rc1.288.g06298d1525-goog
> > > >