Re: Fwd: Surface Go VCM type (was: Need to pass acpi_enforce_resources=lax on the Surface Go (version1))

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hello,

On Thu, Nov 11, 2021 at 07:30:39PM +0000, Dave Stevenson wrote:
> On Thu, 11 Nov 2021 at 16:50, Hans de Goede wrote:
> > On 11/11/21 16:51, Dave Stevenson wrote:
> > > On Thu, 11 Nov 2021 at 15:23, Hans de Goede wrote:
> > >> On 11/11/21 12:18, Daniel Scally wrote:
> > >>
> > >> <snip>
> > >>
> > >>>>> One problem I'm experiencing
> > >>>>> is that the focus position I set isn't maintained; it holds for a couple
> > >>>>> of seconds and then resets to the "normal" focus...this happens when the
> > >>>>> .close() callback for the driver is called, which happens right after
> > >>>>> the control value is applied. All the other VCM drivers in the kernel
> > >>>>> power down on .close() so I did the same>
> > >>>> Right, I believe that this is fine though, we expect people to use
> > >>>> libcamera with this and once libcamera gets autofocus support, then
> > >>>> I would expect libcamera to keep the fd open the entire time while
> > >>>> streaming.
> > >>>
> > >>>
> > >>> OK - as long as that's how it works then I agree that this is fine as is
> > >>> yes.
> > >>
> > >> So I've just picked up an old project of mine, called gtk-v4l which
> > >> is a nice simply v4l2 controls applet and patches it up to also
> > >> work on v4l-subdevs:
> > >>
> > >> https://github.com/jwrdegoede/gtk-v4l/
> > >>
> > >> So now you can run:
> > >>
> > >> sudo gtk-v4l -d /dev/v4l-subdev8
> > >>
> > >> And it will give you a slider to control the focus; and as
> > >> a bonus it keeps the v4l-subdev open, so no more runtime-pm
> > >> issue :)
> > >
> > > Do the lens and sensor share a regulator / enable GPIO?
> >
> > No, if they did then there would be no runtime-pm issue,
> > because then the VCM would not get turned off after
> > a v4l2-set command (for a quick test) since then the
> > streaming from the sensor would keep the sensor and
> > thus the regulator on.
> 
> Registering with the regulator was more so that it restored the
> position on sensor power up, independent of whether the lens driver
> was opened or not.
> 
> > > I was looking at the same issue for a Sony IMX135 module with AD5398
> > > VCM driver [1].
> > > In my case they do share an enable GPIO, so using regulator-gpio we
> > > can register via regulator_register_notifier for information on when
> > > the regulator is powered up. It can then also reset to the last
> > > position should the sensor subdev enable the regulator without the
> > > lens driver being opened at all.
> >
> > That sounds like it is relying on board-depedent behavior
> > (the enable GPIO and/or regulator being shared) which we don't
> > want in the VCM drivers as those are supposed to be board
> > agnostic.
> 
> All platforms I've encountered so far have used the same GPIO to
> control both VCM and sensor, hence why I asked. The number of use
> cases where you want one without the other is incredibly low, and
> hardware guys generally don't like wasting GPIOs or having to route
> them around the PCB. It's interesting that your platform has separated
> them.
> 
> > This really is something which should be fixed in userspace
> > where the userspace consumer of the sensor should also always
> > open the vcm v4l-subdev.
> 
> Not all use cases involve libcamera, and what you're proposing is
> making life very difficult for the simple use cases.
> There may be GStreamer folk on board with libcamera, but I've heard no
> noises from FFmpeg about libcamera support. V4L2 is still the default
> API that users generally care about. Particularly with mono sensors
> the output is often directly usable without worrying about the
> complexities of ISPs, but you're effectively saying "jump through lots
> of hoops or you can't use a VCM with these sensors".

Usage of libcamera is certainly not mandatory, but let's not forget that
we're dealing with complex devices. In most cases applications will want
auto-focus, which will require a userspace camera stack. Even when using
manual focus, apart from moving the lens to the infinity position, there
isn't much that an application could do without some sort of calibration
data. Having to keep the VCM subdev open is the easy part. As long as
this is documented properly in the V4L2 API, I don't think it's a big
issue.

> If userspace has called VIDIOC_STREAMON doesn't that mean they want
> the whole entity (as configured) to be powered on and start streaming?
> Are you saying that the lens isn't part of that entity? In which case
> why does Media Controller include it (and eventually link it to the
> sensor) in the media entity?
> 
> Would you advocate making backlight control in DRM a function that
> userspace is responsible for independently of the panel pipeline?
> There are significant similarities to this situation as the panel
> isn't usable without the backlight being powered, same as the sensor
> isn't usable without the VCM being powered.

Isn't the backlight actually controlled through sysfs separately from
the display pipeline ?

> Sorry, but I just see isolating power control for the VCM from the
> sensor in this way to be a very odd design decision. It'd be
> interesting to hear other views.

Despite the above, I wouldn't oppose powering the VCM automatically when
the sensor is streaming, but I'm concerned about corner cases. For
instance, one may want to keep the VCM powered when toggling streaming
off and then back on. I wouldn't be surprised if there were other need
to have control of VCM power from userspace. I haven't studied the
question in details though.

-- 
Regards,

Laurent Pinchart