Hi,

On 11/9/21 13:09, Daniel Scally wrote:

Hi Hans

On 09/11/2021 00:43, Daniel Scally wrote:

quoted

Hi Hans

On 08/11/2021 13:12, Hans de Goede wrote:

quoted

Hi,

On 11/2/21 00:43, Daniel Scally wrote:

quoted

Hi Hans

<snip>
 

quoted

quoted

2. I need some help with all the fwnode link stuff (I'm not very familiar
with this). There seems to be a chicken and egg problem here though,
because the v4l2subdev for the VCM does not register because of async stuff
and if we add it to the "graph" then my idea to enumerate the VCMs
from the SSDB on the complete() callback won't work. But we can do this
on a per sensor basis instead from the cio2_notifier_bound() callback
instead I guess ?

I think on top of your work in the cio2-bridge for patch 3 you can do this:


1. Create another software node against the cio2_sensor struct, with the
name coming from the vcm_types array

2. Assign that software node to board_info.swnode in
cio2_bridge_instantiate_vcm_i2c_client()

3. Add another entry to dev_properties for the sensor, that is named
"lens-focus" and contains a reference to the software_node created in #2
just like the references to the sensor/cio2 nodes.


This way when the sensor driver calls
v4l2_async_register_subdev_sensor() it should create a notifier that
looks for that VCM client to bind. I think then rather than putting
anything in the .bound() / .complete() callbacks, we should modify core
to do _something_ when async matching some subdevs. The something would
depend on the kind of devices that match, for example with the sensor
driver and the ipu3-cio2 driver, there's an entity whos function is
MEDIA_ENT_F_VID_IF_BRIDGE matching to an entity whos function is
MEDIA_ENT_F_CAM_SENSOR, and it seems to me that every scenario like that
is going to result in media pad links being created. Similarly for our
sensor that's a device with entity function MEDIA_ENT_F_LENS matching to
MEDIA_ENT_F_CAM_SENSOR, and I think that in those cases we can create
either an interface link or a new kind of link (maybe
"MEDIA_LNK_FL_ANCILLARY_LINK" or something...) between the two to show
that they form a single logical unit, which we can then report to libcamera.


Hope that makes sense...

Ok, so I gave this a try, see the attached patches, but the v4l2-subdev for
the VCM still does not show up.

This is exactly where I got to over the weekend too

quoted

I think that instead I need to build a full link between the sensor
and the VCM similar to the cio2 <-> sensor link. Both ends of that link
have:

<base-swnode attached to the device>
|
--<port-swnode named (SWNODE_GRAPH_PORT_NAME_FMT, X), where X is 0 on the
  |                           sensor side and the link nr on the cio2 side
  |
  --<end-point-swnode named (SWNODE_GRAPH_ENDPOINT_NAME_FMT, 0)

And then the 2 endpoints contain a swref property pointing to the
other endpoint swnode.

I think we need a similar setup adding a swnode child named
(SWNODE_GRAPH_PORT_NAME_FMT, 1), to the nodes[SWNODE_SENSOR_HID] node.

Note 1, since 0 is the "port" to the cio2, this new port child then
gets an endpoint "0" child itself, likewise we add a "port 0" child
to the vcm swnode, with a "endpoint 0" child below that and then have
the 2 endpoints contain swref properties pointing to each other.

I think that this will properly make the VCm part of the graph and
will make its v4l2-subdev get instantiated when the graph is
complete.  Before I spend a bunch of time on implementing this,
let me ask this:

Does this sound reasonable / like I'm heading in the right direction?

I don't think that we need to add the software nodes as
ports/endpoints...as far as I can tell it ought to work like this:


1. The sensor calls v4l2_async_register_subdev_sensor() which...

    a) creates a notifier

    b) looks for reference properties of the device's fwnode called
"lens-focus" and calls v4l2_async_notifier_add_fwnode_subdev() against
the reference, which tells the notifier it's connected to this other
fwnode and to expect it to bind.

2. When new subdevs are registered they get tested for a match against
the notifier registered in 1a that matches to their fwnode using
match_fwnode() [1]. This should work, on the grounds that we registered
the device using the board_info.swnode and registered a lens-focus
property that points to that software_node

3. When a match is found, the notifier's .bound() function is called.
When all the asds that the notifier expects are bound the notifier's
.complete() callback is called.


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.

Mainly getting Windows to work (including cameras) after a fresh
Windows install on A Dell Latitude 7285 which I just got as a second
device to test IPU3 stuff on :)

Talking about this Dell Latitude 7285, I haven't had a chance to
look into this at all. But chances are I will need to do some
I2C-register dumps under Windows, last time you mentioned you
had some small tool for this ? It is ok if it is a bit hackish,
it will still be very useful to have :)  And I believe I will
also need to override the DSDT under Windows for this, right?
I should be able to cope with that too.

Regards,

Hans

quoted

For the devnodes, the ipu3-cio2 driver itself creates the devnodes for
the subdevices that bind to it (like the sensor) as part of its
.complete() callback [2] by calling v4l2_device_register_subdev_nodes(),
as far as I can tell there's nothing in v4l2 core that handles that
automatically so I think that that lack is what's preventing the
devnodes from showing up. I think we should tackle the problem of the
missing devnodes by mimicking the effects of that function somewhere
within core, probably v4l2_async_match_notify() (which calls the
notifier's .bound() callback). I think the creation of the links to
expose to userspace that this is a logical unit should probably happen
in the same place, using the entity.function field of the subdev and the
asd to decide exactly what kind of link to create.


[1]
https://elixir.bootlin.com/linux/latest/source/drivers/media/v4l2-core/v4l2-async.c#L69

[2]
https://elixir.bootlin.com/linux/latest/source/drivers/media/pci/intel/ipu3/ipu3-cio2-main.c#L1449

quoted

Regards,

Hans



p.s.

I have found a new solution for the probe-ordering problem which
is patch 2 of the attached patches, I personally I'm happy with
this solution. I hope you like it too.