| Message ID | 20241018075942.1150378-5-chenghaoyang@chromium.org |
|---|---|
| State | New |
| Headers | show |
| Series |
|
| Related | show |
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! -- Kieran > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org> > Co-developed-by: Yudhistira Erlandinata <yerlandinata@chromium.org> > Signed-off-by: Yudhistira Erlandinata <yerlandinata@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 >
Hi Kieran, On Sun, Oct 20, 2024 at 6:47 AM Kieran Bingham <kieran.bingham@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. 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@chromium.org> > > Co-developed-by: Yudhistira Erlandinata <yerlandinata@chromium.org> > > Signed-off-by: Yudhistira Erlandinata <yerlandinata@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 > >
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@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@chromium.org> > > > Co-developed-by: Yudhistira Erlandinata <yerlandinata@chromium.org> > > > Signed-off-by: Yudhistira Erlandinata <yerlandinata@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 > > >
Hi Kieran, On Sun, Oct 20, 2024 at 6:58 PM Kieran Bingham <kieran.bingham@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@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@chromium.org> > > > > Co-developed-by: Yudhistira Erlandinata <yerlandinata@chromium.org> > > > > Signed-off-by: Yudhistira Erlandinata <yerlandinata@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 > > > >
Hi Kieran, On Mon, Oct 21, 2024 at 3:31 PM Cheng-Hao Yang <chenghaoyang@chromium.org> wrote: > > Hi Kieran, > > On Sun, Oct 20, 2024 at 6:58 PM Kieran Bingham > <kieran.bingham@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@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@chromium.org> > > > > > Co-developed-by: Yudhistira Erlandinata <yerlandinata@chromium.org> > > > > > Signed-off-by: Yudhistira Erlandinata <yerlandinata@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 > > > > >
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;