[libcamera-devel] Fwd: Surface Go VCM type (was: Need to pass acpi_enforce_resources=lax on the Surface Go (version1))

[Daniel Scally]

Hello

On 11/11/2021 10:35, Hans de Goede wrote:
> Hi,
>
> On 11/10/21 01:01, Daniel Scally wrote:
>> Hi Hans
>>
>> On 09/11/2021 16:35, Daniel Scally wrote:
>>>>>> That's not working correctly for me at the moment, but I think this is a
>>>>>> surmountable problem rather than the wrong approach, so I'm just working
>>>>>> through the differences to try and get the matching working.
>>>>> OK, I eventually got this working - the dw9719 registers as
>>>>> /dev/v4l-subdev7 for me now ... long story short is the attached patch
>>>>> was needed to make the references work, as the internals of v4l2 aren't
>>>>> checking for fwnode->secondary. Prior to your latest series as well, an
>>>>> additional problem was that once the VCMs fwnode was linked to the
>>>>> sensor's the .complete() callback for ipu3-cio2 would never call
>>>>> (because it needs ALL the devices for the linked fwnodes to be bound to
>>>>> do that)...which meant the VCMs never got instantiated, because that was
>>>>> where that function was called. With your new set separating those
>>>>> processes it works well, so yes I like that new approach very much :D
>>>>>
>>>>>
>>>>> In the end we don't have to add a call creating the subdev's - it turns
>>>>> out that v4l2 knows it's part of ipu3-cio2's v4l2-device so it registers
>>>>> the nodes for the vcm when .complete() is called for that driver. I
>>>>> still think we should add a bit creating the link to expose to userspace
>>>>> in match_notify() though.
>>>>>
>>>>>
>>>>> Trying to list controls for the dw9719 with v4l2-ctl -d /dev/v4l-subdev7
>>>>> -L fails with an IOCTL error, so I have some remedial work on the driver
>>>>> which I'll do tonight; I'd expect to be able to control focus with
>>>>> v4l2-ctl -d /dev/v4l-subdev7 -c absolute_focus=n once this is sorted.
>>>> That is great, thank you so much. I wanted to look into this myself
>>>> today but I got distracted by other stuff.
>>> No problem; I'll link you the patches for the updated versions of
>>> everything once I've sorted the IOCTL error tonight.
>>
>> OK, this is running now. With the attached patches on top of your v5
>> series and the 4-patch series from earlier today, the dw9719 registers
>> as a v4l2 subdev and I can control it with v4l2-ctl -d /dev/v4l-subdev7
>> -c focus_absolute=1200 (or whatever value).
> Great, thank you! I've given this a quick test and indeed everything
> works :)
>
> I did notice a typo in a comment in the dw9719.c file which I added
> myself, can you squash in this fix pleas? :


No problem, will do

> Also I think that the 
>
> "device property: Check fwnode->secondary when finding properties"
>
> That patch looks good to me, so please add my:
>
> Reviewed-by: Hans de Goede <hdegoede at redhat.com>
>
> Can you submit this upstream please?


Thanks; I'll post it later yes (and thanks for your R-b too Andy)

> I will prepare a new version of my:
>
> "[PATCH v5 00/11] Add support for X86/ACPI camera sensor/PMIC setup with clk and regulator platform data"
>
> series, addressing the few remaining comments and adding the regulator
> data + instantiating support for the VCM.
>
>> 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.


> What is necessary is some way for libcamera to:
>
> 1. See if there is a VCM which belongs to the sensor; and
> 2. If there is a VCM figure out which v4l2-subdev it is.
>
> Also see this email thread, where Hans Verkuil came to the
> conclusion that this info is currently missing from the MC
> representation (link is to the conclusion):
>
> https://lists.libcamera.org/pipermail/libcamera-devel/2021-October/026144.html


Yeah I read through that thread too, and had a brief chat with Laurent
about it. My plan was to add a new type of link called an "ancillary
link" between two entities, and automatically create those in
match_notify() based on the function field of the matching entities, and
expose them as part of the media graph. I've started working on that but
not progressed far enough to share anything. Libcamera would need
updating with support for that too though.

>
> Regards,
>
> Hans
>