Re: [libcamera-devel] [PATCH RFC 1/4] media: v4l2-ctrls: add lens group status controls for zoom and focus

[Michael Riesch]

Hi Sakari,

On 4/12/23 17:12, Sakari Ailus wrote:
> Hi Dave, Michael,
> 
> On Wed, Apr 12, 2023 at 02:55:56PM +0100, Dave Stevenson wrote:
>>>> If the ranges aren't updated, where should that out-of-range lens
>>>> movement leave the lens?
>>>
>>> This is up to the hardware controller, but I would guess it typically
>>> stops one step before disaster. Wherever that may be, the error
>>> condition and the current position can be read out via this new STATUS
>>> control.
>>>
>>> Does this sound good so far?
>>
>> Sounds reasonable, but I'm not the gatekeeper (that would be Sakari or
>> Laurent), and I'm just expressing my views based on the lenses I've
>> encountered.
>> All of my lenses have a single drive for focus, a single drive for
>> zoom, and where there are multiple elements they are all connected
>> mechanically. Your setup sounds far more complex and is likely to need
>> a more extensive driver, but it'd be nice to not unnecessarily
>> overcomplicate the interface.
> 
> Could we also have a driver that uses these new controls?

If you are referring to the driver for our custom lens controller, then
I have to say that it is under development and simply not ready for
release yet. Also, the decision has not yet been made whether or not
this will be an open-source driver.

A different approach could be the adaptation of the vimc-lens driver,
which currently only supports FOCUS_ABSOLUTE. But this would raise
several implementation questions and at least for me this would be a
nontrivial task.

Is it required to have a driver for this interface (in the sense that
the patches cannot be accepted otherwise)?

> The controls themselves appear reasonable to me as well. I guess there are
> changes to be made based on the discussion?

I'd summarize that whether or not the status controls are compound
controls of the type V4L2_CTRL_TYPE_LENS_STATUS is the open question.

As a potential follow-up question I recently asked myself if the struct
v4l2_ctrl_lens_status should contain trailing reserved bytes for future
extension (no idea, though, what this could be).

Alternatively, we could come up with "V4L2_CID_FOCUS_CURRENT (integer)"
for the current position and "V4L2_CID_FOCUS_STATUS (bitmask)" (and add
further controls when they are needed. Here, we lose atomicity but maybe
this can be ignored. One could assume that all relevant controls are
read out with a single ioctl which provides at least some level of
atomicity.

Any comments and/or recommendations to this open question would be much
appreciated.

Other review comments will be incorporated in the next iteration of this
series as well, but they are quite straightforward.

Best regards,
Michael