Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 19 Apr 2023 12:01:25 +0300
From:   Sakari Ailus <sakari.ailus@linux.intel.com>
To:     Michael Riesch <michael.riesch@wolfvision.net>
Cc:     Dave Stevenson <dave.stevenson@raspberrypi.com>,
        Mauro Carvalho Chehab <mchehab@kernel.org>,
        Michael Riesch via B4 Relay 
        <devnull+michael.riesch.wolfvision.net@kernel.org>,
        linux-kernel@vger.kernel.org,
        Matthias Fend <Matthias.Fend@wolfvision.net>,
        libcamera-devel@lists.libcamera.org, linux-media@vger.kernel.org,
        hverkuil@xs4all.nl,
        Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Subject: Re: [libcamera-devel] [PATCH RFC 1/4] media: v4l2-ctrls: add lens
 group status controls for zoom and focus
Message-ID: <ZD+t5QYH20Y8+8MV@kekkonen.localdomain>
References: <20230406-feature-controls-lens-v1-0-543189a680de@wolfvision.net>
 <20230406-feature-controls-lens-v1-1-543189a680de@wolfvision.net>
 <CAPY8ntArOOqPQzvkJrQEyuVFfb6j8x6WODTMHOn1qHPU588mbQ@mail.gmail.com>
 <0f1baf5e-2ff6-e10b-5c3e-0a82c71d0ce6@wolfvision.net>
 <CAPY8ntAjBEFfeV6nnQs34Y22QM-irT13ALDv4ksP8AYK=jWsKg@mail.gmail.com>
 <3ab7bfc4-aaae-2e39-b420-40ad8d71dda4@wolfvision.net>
 <CAPY8ntCNuvgmF37kDvVh1kuepbLqy2hWcz9HOi8iub9trHmi2g@mail.gmail.com>
 <ZDbKU5kwcb7RGeCo@kekkonen.localdomain>
 <ccae3994-3b1b-4050-ea34-98f97cf886e0@wolfvision.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <ccae3994-3b1b-4050-ea34-98f97cf886e0@wolfvision.net>
Precedence: bulk

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.

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

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

-- 
Kind regards,

Sakari Ailus