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

[Kieran Bingham]

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.

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



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