Message-ID: <45fea085-54c3-157e-6459-adaaf1edebf5@wolfvision.net>
Date:   Wed, 19 Apr 2023 13:24:58 +0200
From:   Michael Riesch <michael.riesch@...fvision.net>
To:     Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc:     Dave Stevenson <dave.stevenson@...pberrypi.com>,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Michael Riesch via B4 Relay 
        <devnull+michael.riesch.wolfvision.net@...nel.org>,
        linux-kernel@...r.kernel.org,
        Matthias Fend <Matthias.Fend@...fvision.net>,
        libcamera-devel@...ts.libcamera.org, linux-media@...r.kernel.org,
        hverkuil@...all.nl,
        Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        Alexander Brotzge <Alexander.Brotzge@...fvision.net>,
        Dieter Mathis <Dieter.Mathis@...fvision.net>
Subject: Re: [libcamera-devel] [PATCH RFC 1/4] media: v4l2-ctrls: add lens
 group status controls for zoom and focus

Hi Sakari,

On 4/19/23 11:01, Sakari Ailus wrote:
> Hi Michael,
> 
> On Mon, Apr 17, 2023 at 02:38:20PM +0200, Michael Riesch wrote:
>> 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)?
> 
> That has been traditionally required, and a virtual driver isn't usually
> considered enough. There are at least two reasons for this. The first one
> being that if the driver isn't reviewable and targetting upstream it may be
> difficult to figure out whether the interface changes are the right ones
> for that driver. This is perhaps a lesser concern here. Secondly, there is
> also unwillingness to add interface elements that might never be supported
> by the kernel itself --- this is effectively just dead code.
> 
> Also cc Hans and Laurent.

I understand your concerns. Cc: Alexander and Dieter

We aim to be an open-source friendly company. If you are OK with us
submitting a driver that targets very custom hardware that is only
available in integrated form in our products (and not, for instance,
available for sale as a standalone device), then we are prepared to
submit the driver sources for consideration for inclusion in mainline
Linux. Would this be acceptable?

As I already stated above, it will take us some time to prepare
everything in a form that is suitable for submission. Now should I
submit the next iteration(s) of the series at hand as RFC or as regular
patch series?

>>> 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.
> 
> There might be something that could be done in the control framework to
> address this. But it's not something that can be expected to happen soon.
> 
> I'd perhaps keep them separate, not to make it a compound control just for
> the access reason. But I certainly don't have a strong opinion about it.

After some further considerations, and following Dave's and your
comments, I'll keep them separate.

Discussion to be continued with v2.

Best regards,
Michael

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

Powered by blists - more mailing lists