libcamera: Add open() and close() in CameraLens

21649 diff mbox series

From: Han-Lin Chen <hanlinchen@chromium.org>

This allows pipeline handlers to save some power when a camera is close.

Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
---
 include/libcamera/internal/camera_lens.h |  4 ++++
 src/libcamera/camera_lens.cpp            | 17 +++++++++++++++++
 2 files changed, 21 insertions(+)

Quoting Harvey Yang (2024-10-17 08:39:47)
> From: Han-Lin Chen <hanlinchen@chromium.org>
> 
> This allows pipeline handlers to save some power when a camera is close.

This seems reasonable - but makes me wonder - is there similar
limitations with the CameraSensor class that has a very similar design?


> 
> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> ---
>  include/libcamera/internal/camera_lens.h |  4 ++++
>  src/libcamera/camera_lens.cpp            | 17 +++++++++++++++++
>  2 files changed, 21 insertions(+)
> 
> diff --git a/include/libcamera/internal/camera_lens.h b/include/libcamera/internal/camera_lens.h
> index 5a4b993bb..095056791 100644
> --- a/include/libcamera/internal/camera_lens.h
> +++ b/include/libcamera/internal/camera_lens.h
> @@ -26,6 +26,10 @@ public:
>         ~CameraLens();
>  
>         int init();

Should we be calling close() at the end of init()? Perhaps with some
sort of way to make sure if an API call is used on a now closed device
it gets opened again first ? (i.e. mostly perhaps V4L2Device::setControls)?

> +
> +       int open();
> +       void close();
> +
>         int setFocusPosition(int32_t position);
>  
>         const std::string &model() const { return model_; }
> diff --git a/src/libcamera/camera_lens.cpp b/src/libcamera/camera_lens.cpp
> index ccc2a6a65..039f5ad2a 100644
> --- a/src/libcamera/camera_lens.cpp
> +++ b/src/libcamera/camera_lens.cpp
> @@ -76,6 +76,23 @@ int CameraLens::init()
>         return 0;
>  }
>  
> +/**
> + * \brief Open the subdev
> + * \return 0 on success or a negative error code otherwise
> + */
> +int CameraLens::open()
> +{
> +       return subdev_->open();
> +}
> +
> +/**
> + * \brief Close the subdev
> + */
> +void CameraLens::close()
> +{
> +       subdev_->close();
> +}
> +
>  /**
>   * \brief This function sets the focal point of the lens to a specific position.
>   * \param[in] position The focal point of the lens
> -- 
> 2.47.0.rc1.288.g06298d1525-goog
>

CC'ing Sakari.

On Sat, Oct 19, 2024 at 11:32:02PM +0100, Kieran Bingham wrote:
> Quoting Harvey Yang (2024-10-17 08:39:47)
> > From: Han-Lin Chen <hanlinchen@chromium.org>
> > 
> > This allows pipeline handlers to save some power when a camera is close.
> 
> This seems reasonable - but makes me wonder - is there similar
> limitations with the CameraSensor class that has a very similar design?

I think it's time to bite the bullet and see how to address the issue of
power management of lens controllers in the kernel. Tying it to open()
and close() isn't necessarily a great option.

> > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> > ---
> >  include/libcamera/internal/camera_lens.h |  4 ++++
> >  src/libcamera/camera_lens.cpp            | 17 +++++++++++++++++
> >  2 files changed, 21 insertions(+)
> > 
> > diff --git a/include/libcamera/internal/camera_lens.h b/include/libcamera/internal/camera_lens.h
> > index 5a4b993bb..095056791 100644
> > --- a/include/libcamera/internal/camera_lens.h
> > +++ b/include/libcamera/internal/camera_lens.h
> > @@ -26,6 +26,10 @@ public:
> >         ~CameraLens();
> >  
> >         int init();
> 
> Should we be calling close() at the end of init()? Perhaps with some
> sort of way to make sure if an API call is used on a now closed device
> it gets opened again first ? (i.e. mostly perhaps V4L2Device::setControls)?
> 
> > +
> > +       int open();
> > +       void close();
> > +
> >         int setFocusPosition(int32_t position);
> >  
> >         const std::string &model() const { return model_; }
> > diff --git a/src/libcamera/camera_lens.cpp b/src/libcamera/camera_lens.cpp
> > index ccc2a6a65..039f5ad2a 100644
> > --- a/src/libcamera/camera_lens.cpp
> > +++ b/src/libcamera/camera_lens.cpp
> > @@ -76,6 +76,23 @@ int CameraLens::init()
> >         return 0;
> >  }
> >  
> > +/**
> > + * \brief Open the subdev
> > + * \return 0 on success or a negative error code otherwise
> > + */
> > +int CameraLens::open()
> > +{
> > +       return subdev_->open();
> > +}
> > +
> > +/**
> > + * \brief Close the subdev
> > + */
> > +void CameraLens::close()
> > +{
> > +       subdev_->close();
> > +}
> > +
> >  /**
> >   * \brief This function sets the focal point of the lens to a specific position.
> >   * \param[in] position The focal point of the lens

Hi Kieran and Laurent,

On Mon, Oct 21, 2024 at 12:08 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> CC'ing Sakari.
>
> On Sat, Oct 19, 2024 at 11:32:02PM +0100, Kieran Bingham wrote:
> > Quoting Harvey Yang (2024-10-17 08:39:47)
> > > From: Han-Lin Chen <hanlinchen@chromium.org>
> > >
> > > This allows pipeline handlers to save some power when a camera is close.
> >
> > This seems reasonable - but makes me wonder - is there similar
> > limitations with the CameraSensor class that has a very similar design?
>
> I think it's time to bite the bullet and see how to address the issue of
> power management of lens controllers in the kernel. Tying it to open()
> and close() isn't necessarily a great option.

Han-lin told me that CameraLens is a special case of
V4L2Subdevice (at least in mtkisp7), that it's powered on when opened.
Other ones like CameraSensor's `subdev_` might be powered on when
the V4L2VideoDevice that receives buffers are `streamOn`ed. Just FYI.

>
> > > Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
> > > Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
> > > Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
> > > ---
> > >  include/libcamera/internal/camera_lens.h |  4 ++++
> > >  src/libcamera/camera_lens.cpp            | 17 +++++++++++++++++
> > >  2 files changed, 21 insertions(+)
> > >
> > > diff --git a/include/libcamera/internal/camera_lens.h b/include/libcamera/internal/camera_lens.h
> > > index 5a4b993bb..095056791 100644
> > > --- a/include/libcamera/internal/camera_lens.h
> > > +++ b/include/libcamera/internal/camera_lens.h
> > > @@ -26,6 +26,10 @@ public:
> > >         ~CameraLens();
> > >
> > >         int init();
> >
> > Should we be calling close() at the end of init()? Perhaps with some
> > sort of way to make sure if an API call is used on a now closed device
> > it gets opened again first ? (i.e. mostly perhaps V4L2Device::setControls)?

Hmm, in the current API, I don't see function calls that "start" to use
CameraLens.

BR,
Harvey

> >
> > > +
> > > +       int open();
> > > +       void close();
> > > +
> > >         int setFocusPosition(int32_t position);
> > >
> > >         const std::string &model() const { return model_; }
> > > diff --git a/src/libcamera/camera_lens.cpp b/src/libcamera/camera_lens.cpp
> > > index ccc2a6a65..039f5ad2a 100644
> > > --- a/src/libcamera/camera_lens.cpp
> > > +++ b/src/libcamera/camera_lens.cpp
> > > @@ -76,6 +76,23 @@ int CameraLens::init()
> > >         return 0;
> > >  }
> > >
> > > +/**
> > > + * \brief Open the subdev
> > > + * \return 0 on success or a negative error code otherwise
> > > + */
> > > +int CameraLens::open()
> > > +{
> > > +       return subdev_->open();
> > > +}
> > > +
> > > +/**
> > > + * \brief Close the subdev
> > > + */
> > > +void CameraLens::close()
> > > +{
> > > +       subdev_->close();
> > > +}
> > > +
> > >  /**
> > >   * \brief This function sets the focal point of the lens to a specific position.
> > >   * \param[in] position The focal point of the lens
>
> --
> Regards,
>
> Laurent Pinchart

Hi,

On 23-Oct-24 11:20 AM, Cheng-Hao Yang wrote:
> Hi Kieran and Laurent,
> 
> On Mon, Oct 21, 2024 at 12:08 AM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
>>
>> CC'ing Sakari.
>>
>> On Sat, Oct 19, 2024 at 11:32:02PM +0100, Kieran Bingham wrote:
>>> Quoting Harvey Yang (2024-10-17 08:39:47)
>>>> From: Han-Lin Chen <hanlinchen@chromium.org>
>>>>
>>>> This allows pipeline handlers to save some power when a camera is close.
>>>
>>> This seems reasonable - but makes me wonder - is there similar
>>> limitations with the CameraSensor class that has a very similar design?
>>
>> I think it's time to bite the bullet and see how to address the issue of
>> power management of lens controllers in the kernel. Tying it to open()
>> and close() isn't necessarily a great option.
> 
> Han-lin told me that CameraLens is a special case of
> V4L2Subdevice (at least in mtkisp7), that it's powered on when opened.
> Other ones like CameraSensor's `subdev_` might be powered on when
> the V4L2VideoDevice that receives buffers are `streamOn`ed. Just FYI.

Right, but what Laurent is proposing is changing the kernel
so that just opening the /dev/v4l20-subdev device for the VCM
will no longer power it up.

I submitted a possible proposal for this here:

https://lore.kernel.org/linux-media/20240901211834.145186-1-hdegoede@redhat.com/

Note that Laurent was not a fan of the approach taken there, so
this needs more discussion and unfortunately I have not been able
to make the time to reply to Laurent about this yet.

I'll try to make time to reply to Laurent about this today.

Regards,

Hans




> 
>>
>>>> Signed-off-by: Han-Lin Chen <hanlinchen@chromium.org>
>>>> Co-developed-by: Harvey Yang <chenghaoyang@chromium.org>
>>>> Signed-off-by: Harvey Yang <chenghaoyang@chromium.org>
>>>> ---
>>>>  include/libcamera/internal/camera_lens.h |  4 ++++
>>>>  src/libcamera/camera_lens.cpp            | 17 +++++++++++++++++
>>>>  2 files changed, 21 insertions(+)
>>>>
>>>> diff --git a/include/libcamera/internal/camera_lens.h b/include/libcamera/internal/camera_lens.h
>>>> index 5a4b993bb..095056791 100644
>>>> --- a/include/libcamera/internal/camera_lens.h
>>>> +++ b/include/libcamera/internal/camera_lens.h
>>>> @@ -26,6 +26,10 @@ public:
>>>>         ~CameraLens();
>>>>
>>>>         int init();
>>>
>>> Should we be calling close() at the end of init()? Perhaps with some
>>> sort of way to make sure if an API call is used on a now closed device
>>> it gets opened again first ? (i.e. mostly perhaps V4L2Device::setControls)?
> 
> Hmm, in the current API, I don't see function calls that "start" to use
> CameraLens.
> 
> BR,
> Harvey
> 
>>>
>>>> +
>>>> +       int open();
>>>> +       void close();
>>>> +
>>>>         int setFocusPosition(int32_t position);
>>>>
>>>>         const std::string &model() const { return model_; }
>>>> diff --git a/src/libcamera/camera_lens.cpp b/src/libcamera/camera_lens.cpp
>>>> index ccc2a6a65..039f5ad2a 100644
>>>> --- a/src/libcamera/camera_lens.cpp
>>>> +++ b/src/libcamera/camera_lens.cpp
>>>> @@ -76,6 +76,23 @@ int CameraLens::init()
>>>>         return 0;
>>>>  }
>>>>
>>>> +/**
>>>> + * \brief Open the subdev
>>>> + * \return 0 on success or a negative error code otherwise
>>>> + */
>>>> +int CameraLens::open()
>>>> +{
>>>> +       return subdev_->open();
>>>> +}
>>>> +
>>>> +/**
>>>> + * \brief Close the subdev
>>>> + */
>>>> +void CameraLens::close()
>>>> +{
>>>> +       subdev_->close();
>>>> +}
>>>> +
>>>>  /**
>>>>   * \brief This function sets the focal point of the lens to a specific position.
>>>>   * \param[in] position The focal point of the lens
>>
>> --
>> Regards,
>>
>> Laurent Pinchart
>