[PATCH] libcamera: Add open() and close() in CameraLens

[Hans de Goede]

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 at 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 at 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 at chromium.org>
>>>> Co-developed-by: Harvey Yang <chenghaoyang at chromium.org>
>>>> Signed-off-by: Harvey Yang <chenghaoyang at 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
>