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

[Cheng-Hao Yang]

Hi Kieran,

On Mon, Oct 21, 2024 at 3:31 PM Cheng-Hao Yang
<chenghaoyang at chromium.org> wrote:
>
> 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.

Sorry, CrOS doesn't expect the camera to be removed from the
pipeline handler (calling CameraManager::Private::removeCamera).
CrOS expects the camera to work as expected when restarting the IPA
process.

I meant that we can either do nothing after the signal, or stop the
camera (calling stop() / release()).

BR,
Harvey

>
> 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
> > > > >