cio2 ipu3 module to automatically connect sensors via swnodes

cio2 ipu3 module to automatically connect sensors via swnodes

[Daniel Scally]

Hello


Following on from this thread:
https://www.spinics.net/lists/linux-driver-devel/msg135122.html -
apologies, can't see a way to reply to it directly.


Myself and others [1] have been trying to get cameras working on
Microsoft Surface and similar platforms, currently I'm working on
expanding Jordan's module connecting cameras to the cio2
infrastructure (which works - we can use it to take images), aiming to
discover connection information from SSDB in the DSDT, as well as
connect as many supported sensors as are found on the device. I'm just
struggling with a problem I've encountered using the swnode patch that
Heikki linked in that thread; the module's working ok when I only
attempt to connect a single one of my sensors (by preventing the
driver for the other sensor from loading, in which case this new
module ignores the sensor), but when I attempt to connect both cameras
at the same time I get a kernel oops as part of
software_node_get_next_child. The module is creating software_nodes
like this...

/sys/kernel/software_node/INT343E/port0/endpoint0
/sys/kernel/software_node/INT343E/port1/endpoint0
/sys/kernel/software_node/OVTI2680/port0/endpoint0
/sys/kernel/software_node/OVTI5648/port0/endpoint0

And that's the structure that I expect to see, but it seems like the
call to list_next_entry in that function is returning something that
isn't quite a valid swnode. Printing the address of c->fwnode after
that point returns "3", which isn't a valid address of course, and
that's causing the oops when it's either returned (in the version of
the function without the patches) or when the program flow tries to
call the "get" op against that fwnode (in the patched version). I've
been trying to track it down for a while now without success, so I
posted the problem on SO[2] and it was suggested that I mail these
addressees for advice - hope that that is ok.


My copy of Jordan's module is parked in my git repo [3] for now, and
requires a batch of patches from Jordan's repo [4] (one made by Heikki
to fill in the missing swnode graph pieces, and some further tweaks) -
I've been applying those against 5.8.0-rc7. Any other criticism more
than welcome - I'm new to both c and kernel programming so I'm happy
to take all the advice people have the time to give.


On a more general note; Kieran from the libcamera project suggested we
ought to be talking to you guys anyway to get some guidance on design,
and some more expert eye on the things we don't really understand. In
particular; we haven't been able to figure out how sensors that are
intended to work with the cio2 ipu3 stuff have their clock/regulators
supplied - in fact I can strip all the "usual" clock/regulator
functionality out of my camera's driver and it still functions fine,
which seems a bit weird. So far all we're doing as "power management"
of the camera's is turning on the GPIO pins that DSDT tables assign to
the tps68470 PMICs the cameras are theoretically hooked up to...but
given the drivers continue to work without using the PMICs regulator
and clk drivers (which we found in the intel-lts tree on Github),
we're a bit confused exactly how these are connected. Given the
potential for actual hardware damage if we mess something up there
it'd be great if anyone can shed some light on those elements.


Regards

Dan


[1] https://github.com/linux-surface/linux-surface/issues/91

[2] https://stackoverflow.com/questions/63742967/what-is-causing-this-kernel-oops-when-parsing-firmware?

[3] https://github.com/djrscally/miix-510-cameras/blob/master/surface_camera/surface_camera.c

[4] https://github.com/jhand2/surface-camera/tree/master/patches

Re: cio2 ipu3 module to automatically connect sensors via swnodes

[Andy Shevchenko]

+Cc: Surface community people.

On Sat, Sep 05, 2020 at 09:19:51AM +0100, Daniel Scally wrote:
> 
> Following on from this thread:
> https://www.spinics.net/lists/linux-driver-devel/msg135122.html -
> apologies, can't see a way to reply to it directly.

Use lore [5] and its feature of downloading mailbox (or just seeing Message-Id
which is enough for good MUA to attach reply properly to the thread).

[5]: https://lore.kernel.org/linux-media/12fbe3f5c6a16c5f3447adbc09fe27ceb2b16823.1589625807.git.mchehab+huawei@kernel.org/

> Myself and others [1] have been trying to get cameras working on
> Microsoft Surface and similar platforms, currently I'm working on
> expanding Jordan's module connecting cameras to the cio2
> infrastructure (which works - we can use it to take images), aiming to
> discover connection information from SSDB in the DSDT, as well as
> connect as many supported sensors as are found on the device. I'm just
> struggling with a problem I've encountered using the swnode patch that
> Heikki linked in that thread; the module's working ok when I only
> attempt to connect a single one of my sensors (by preventing the
> driver for the other sensor from loading, in which case this new
> module ignores the sensor), but when I attempt to connect both cameras
> at the same time I get a kernel oops as part of
> software_node_get_next_child. The module is creating software_nodes
> like this...
> 
> /sys/kernel/software_node/INT343E/port0/endpoint0
> /sys/kernel/software_node/INT343E/port1/endpoint0
> /sys/kernel/software_node/OVTI2680/port0/endpoint0
> /sys/kernel/software_node/OVTI5648/port0/endpoint0
> 
> And that's the structure that I expect to see, but it seems like the
> call to list_next_entry in that function is returning something that
> isn't quite a valid swnode. Printing the address of c->fwnode after
> that point returns "3", which isn't a valid address of course, and
> that's causing the oops when it's either returned (in the version of
> the function without the patches) or when the program flow tries to
> call the "get" op against that fwnode (in the patched version). I've
> been trying to track it down for a while now without success, so I
> posted the problem on SO[2] and it was suggested that I mail these
> addressees for advice - hope that that is ok.
> 
> 
> My copy of Jordan's module is parked in my git repo [3] for now, and
> requires a batch of patches from Jordan's repo [4] (one made by Heikki
> to fill in the missing swnode graph pieces, and some further tweaks) -
> I've been applying those against 5.8.0-rc7. Any other criticism more
> than welcome - I'm new to both c and kernel programming so I'm happy
> to take all the advice people have the time to give.
> 
> 
> On a more general note; Kieran from the libcamera project suggested we
> ought to be talking to you guys anyway to get some guidance on design,
> and some more expert eye on the things we don't really understand. In
> particular; we haven't been able to figure out how sensors that are
> intended to work with the cio2 ipu3 stuff have their clock/regulators
> supplied - in fact I can strip all the "usual" clock/regulator
> functionality out of my camera's driver and it still functions fine,
> which seems a bit weird. So far all we're doing as "power management"
> of the camera's is turning on the GPIO pins that DSDT tables assign to
> the tps68470 PMICs the cameras are theoretically hooked up to...but
> given the drivers continue to work without using the PMICs regulator
> and clk drivers (which we found in the intel-lts tree on Github),
> we're a bit confused exactly how these are connected. Given the
> potential for actual hardware damage if we mess something up there
> it'd be great if anyone can shed some light on those elements.

> [1] https://github.com/linux-surface/linux-surface/issues/91
> [2] https://stackoverflow.com/questions/63742967/what-is-causing-this-kernel-oops-when-parsing-firmware?
> [3] https://github.com/djrscally/miix-510-cameras/blob/master/surface_camera/surface_camera.c
> [4] https://github.com/jhand2/surface-camera/tree/master/patches

-- 
With Best Regards,
Andy Shevchenko

Re: cio2 ipu3 module to automatically connect sensors via swnodes

[Kieran Bingham]

Hi Daniel,

On 07/09/2020 10:44, Andy Shevchenko wrote:
> +Cc: Surface community people.
> 
> On Sat, Sep 05, 2020 at 09:19:51AM +0100, Daniel Scally wrote:
>>
>> Following on from this thread:
>> https://www.spinics.net/lists/linux-driver-devel/msg135122.html -
>> apologies, can't see a way to reply to it directly.
> 
> Use lore [5] and its feature of downloading mailbox (or just seeing Message-Id
> which is enough for good MUA to attach reply properly to the thread).
> 
> [5]: https://lore.kernel.org/linux-media/12fbe3f5c6a16c5f3447adbc09fe27ceb2b16823.1589625807.git.mchehab+huawei@kernel.org/
> 

I also like to use the NNTP interface on lore:

nntp://nntp.lore.kernel.org

That lets you read/reply to many of the kernel lists without actually
having to subscribe. And of course gets the whole archive too.


>> Myself and others [1] have been trying to get cameras working on
>> Microsoft Surface and similar platforms, currently I'm working on
>> expanding Jordan's module connecting cameras to the cio2
>> infrastructure (which works - we can use it to take images), aiming to
>> discover connection information from SSDB in the DSDT, as well as
>> connect as many supported sensors as are found on the device. I'm just
>> struggling with a problem I've encountered using the swnode patch that
>> Heikki linked in that thread; the module's working ok when I only
>> attempt to connect a single one of my sensors (by preventing the
>> driver for the other sensor from loading, in which case this new
>> module ignores the sensor), but when I attempt to connect both cameras
>> at the same time I get a kernel oops as part of
>> software_node_get_next_child. The module is creating software_nodes
>> like this...
>>
>> /sys/kernel/software_node/INT343E/port0/endpoint0
>> /sys/kernel/software_node/INT343E/port1/endpoint0
>> /sys/kernel/software_node/OVTI2680/port0/endpoint0
>> /sys/kernel/software_node/OVTI5648/port0/endpoint0
>>
>> And that's the structure that I expect to see, but it seems like the
>> call to list_next_entry in that function is returning something that
>> isn't quite a valid swnode. Printing the address of c->fwnode after
>> that point returns "3", which isn't a valid address of course, and
>> that's causing the oops when it's either returned (in the version of
>> the function without the patches) or when the program flow tries to
>> call the "get" op against that fwnode (in the patched version). I've
>> been trying to track it down for a while now without success, so I
>> posted the problem on SO[2] and it was suggested that I mail these
>> addressees for advice - hope that that is ok.
>>
>>
>> My copy of Jordan's module is parked in my git repo [3] for now, and
>> requires a batch of patches from Jordan's repo [4] (one made by Heikki
>> to fill in the missing swnode graph pieces, and some further tweaks) -
>> I've been applying those against 5.8.0-rc7. Any other criticism more
>> than welcome - I'm new to both c and kernel programming so I'm happy
>> to take all the advice people have the time to give.
>>
>>
>> On a more general note; Kieran from the libcamera project suggested we
>> ought to be talking to you guys anyway to get some guidance on design,
>> and some more expert eye on the things we don't really understand. In
>> particular; we haven't been able to figure out how sensors that are
>> intended to work with the cio2 ipu3 stuff have their clock/regulators
>> supplied - in fact I can strip all the "usual" clock/regulator
>> functionality out of my camera's driver and it still functions fine,
>> which seems a bit weird. So far all we're doing as "power management"
>> of the camera's is turning on the GPIO pins that DSDT tables assign to
>> the tps68470 PMICs the cameras are theoretically hooked up to...but
>> given the drivers continue to work without using the PMICs regulator
>> and clk drivers (which we found in the intel-lts tree on Github),
>> we're a bit confused exactly how these are connected. Given the
>> potential for actual hardware damage if we mess something up there
>> it'd be great if anyone can shed some light on those elements.
> 
>> [1] https://github.com/linux-surface/linux-surface/issues/91
>> [2] https://stackoverflow.com/questions/63742967/what-is-causing-this-kernel-oops-when-parsing-firmware?
>> [3] https://github.com/djrscally/miix-510-cameras/blob/master/surface_camera/surface_camera.c
>> [4] https://github.com/jhand2/surface-camera/tree/master/patches
> 

-- 
Regards
--
Kieran

Re: cio2 ipu3 module to automatically connect sensors via swnodes

[Dan Scally]

Hello

On 07/09/2020 11:49, Kieran Bingham wrote:
> Hi Daniel,
> 
> On 07/09/2020 10:44, Andy Shevchenko wrote:
>> +Cc: Surface community people.
>>
>> On Sat, Sep 05, 2020 at 09:19:51AM +0100, Daniel Scally wrote:
>>>
>>> Following on from this thread:
>>> https://www.spinics.net/lists/linux-driver-devel/msg135122.html -
>>> apologies, can't see a way to reply to it directly.
>>
>> Use lore [5] and its feature of downloading mailbox (or just seeing Message-Id
>> which is enough for good MUA to attach reply properly to the thread).
>>
>> [5]: https://lore.kernel.org/linux-media/12fbe3f5c6a16c5f3447adbc09fe27ceb2b16823.1589625807.git.mchehab+huawei@kernel.org/
>>
> 
> I also like to use the NNTP interface on lore:
> 
> nntp://nntp.lore.kernel.org
> 
> That lets you read/reply to many of the kernel lists without actually
> having to subscribe. And of course gets the whole archive too.

Ah, TIL. Thank you both - let me re-send as a reply to the original
thread so that the context is more easily accessible.

>>> Myself and others [1] have been trying to get cameras working on
>>> Microsoft Surface and similar platforms, currently I'm working on
>>> expanding Jordan's module connecting cameras to the cio2
>>> infrastructure (which works - we can use it to take images), aiming to
>>> discover connection information from SSDB in the DSDT, as well as
>>> connect as many supported sensors as are found on the device. I'm just
>>> struggling with a problem I've encountered using the swnode patch that
>>> Heikki linked in that thread; the module's working ok when I only
>>> attempt to connect a single one of my sensors (by preventing the
>>> driver for the other sensor from loading, in which case this new
>>> module ignores the sensor), but when I attempt to connect both cameras
>>> at the same time I get a kernel oops as part of
>>> software_node_get_next_child. The module is creating software_nodes
>>> like this...
>>>
>>> /sys/kernel/software_node/INT343E/port0/endpoint0
>>> /sys/kernel/software_node/INT343E/port1/endpoint0
>>> /sys/kernel/software_node/OVTI2680/port0/endpoint0
>>> /sys/kernel/software_node/OVTI5648/port0/endpoint0
>>>
>>> And that's the structure that I expect to see, but it seems like the
>>> call to list_next_entry in that function is returning something that
>>> isn't quite a valid swnode. Printing the address of c->fwnode after
>>> that point returns "3", which isn't a valid address of course, and
>>> that's causing the oops when it's either returned (in the version of
>>> the function without the patches) or when the program flow tries to
>>> call the "get" op against that fwnode (in the patched version). I've
>>> been trying to track it down for a while now without success, so I
>>> posted the problem on SO[2] and it was suggested that I mail these
>>> addressees for advice - hope that that is ok.
>>>
>>>
>>> My copy of Jordan's module is parked in my git repo [3] for now, and
>>> requires a batch of patches from Jordan's repo [4] (one made by Heikki
>>> to fill in the missing swnode graph pieces, and some further tweaks) -
>>> I've been applying those against 5.8.0-rc7. Any other criticism more
>>> than welcome - I'm new to both c and kernel programming so I'm happy
>>> to take all the advice people have the time to give.
>>>
>>>
>>> On a more general note; Kieran from the libcamera project suggested we
>>> ought to be talking to you guys anyway to get some guidance on design,
>>> and some more expert eye on the things we don't really understand. In
>>> particular; we haven't been able to figure out how sensors that are
>>> intended to work with the cio2 ipu3 stuff have their clock/regulators
>>> supplied - in fact I can strip all the "usual" clock/regulator
>>> functionality out of my camera's driver and it still functions fine,
>>> which seems a bit weird. So far all we're doing as "power management"
>>> of the camera's is turning on the GPIO pins that DSDT tables assign to
>>> the tps68470 PMICs the cameras are theoretically hooked up to...but
>>> given the drivers continue to work without using the PMICs regulator
>>> and clk drivers (which we found in the intel-lts tree on Github),
>>> we're a bit confused exactly how these are connected. Given the
>>> potential for actual hardware damage if we mess something up there
>>> it'd be great if anyone can shed some light on those elements.
>>
>>> [1] https://github.com/linux-surface/linux-surface/issues/91
>>> [2] https://stackoverflow.com/questions/63742967/what-is-causing-this-kernel-oops-when-parsing-firmware?
>>> [3] https://github.com/djrscally/miix-510-cameras/blob/master/surface_camera/surface_camera.c
>>> [4] https://github.com/jhand2/surface-camera/tree/master/patches
>>
> 

Re: cio2 ipu3 module to automatically connect sensors via swnodes

[Sakari Ailus]

Hi Daniel,

On Sat, Sep 05, 2020 at 09:19:51AM +0100, Daniel Scally wrote:
> Hello
> 
> 
> Following on from this thread:
> https://www.spinics.net/lists/linux-driver-devel/msg135122.html -
> apologies, can't see a way to reply to it directly.
> 
> 
> Myself and others [1] have been trying to get cameras working on
> Microsoft Surface and similar platforms, currently I'm working on
> expanding Jordan's module connecting cameras to the cio2
> infrastructure (which works - we can use it to take images), aiming to
> discover connection information from SSDB in the DSDT, as well as
> connect as many supported sensors as are found on the device. I'm just
> struggling with a problem I've encountered using the swnode patch that
> Heikki linked in that thread; the module's working ok when I only
> attempt to connect a single one of my sensors (by preventing the
> driver for the other sensor from loading, in which case this new
> module ignores the sensor), but when I attempt to connect both cameras
> at the same time I get a kernel oops as part of
> software_node_get_next_child. The module is creating software_nodes
> like this...
> 
> /sys/kernel/software_node/INT343E/port0/endpoint0
> /sys/kernel/software_node/INT343E/port1/endpoint0
> /sys/kernel/software_node/OVTI2680/port0/endpoint0
> /sys/kernel/software_node/OVTI5648/port0/endpoint0
> 
> And that's the structure that I expect to see, but it seems like the
> call to list_next_entry in that function is returning something that
> isn't quite a valid swnode. Printing the address of c->fwnode after
> that point returns "3", which isn't a valid address of course, and
> that's causing the oops when it's either returned (in the version of
> the function without the patches) or when the program flow tries to
> call the "get" op against that fwnode (in the patched version). I've
> been trying to track it down for a while now without success, so I
> posted the problem on SO[2] and it was suggested that I mail these
> addressees for advice - hope that that is ok.
> 
> 
> My copy of Jordan's module is parked in my git repo [3] for now, and
> requires a batch of patches from Jordan's repo [4] (one made by Heikki
> to fill in the missing swnode graph pieces, and some further tweaks) -
> I've been applying those against 5.8.0-rc7. Any other criticism more
> than welcome - I'm new to both c and kernel programming so I'm happy
> to take all the advice people have the time to give.
> 
> 
> On a more general note; Kieran from the libcamera project suggested we
> ought to be talking to you guys anyway to get some guidance on design,
> and some more expert eye on the things we don't really understand. In
> particular; we haven't been able to figure out how sensors that are
> intended to work with the cio2 ipu3 stuff have their clock/regulators
> supplied - in fact I can strip all the "usual" clock/regulator
> functionality out of my camera's driver and it still functions fine,
> which seems a bit weird. So far all we're doing as "power management"
> of the camera's is turning on the GPIO pins that DSDT tables assign to
> the tps68470 PMICs the cameras are theoretically hooked up to...but
> given the drivers continue to work without using the PMICs regulator
> and clk drivers (which we found in the intel-lts tree on Github),
> we're a bit confused exactly how these are connected. Given the
> potential for actual hardware damage if we mess something up there
> it'd be great if anyone can shed some light on those elements.

On ACPI systems regulators and clocks as well as GPIOs to some extent are
controlled by AML code in the DSDT and SSDT. There are different ways this
can be implemented though. It may be that the PMIC in this case is
controlled entirely from the AML code without the need for a driver.

This might be the case here. It should be possible to figure this out from
the DSDT and SSDT tables.

If you do not change how the regulators in the PMIC are controlled I'd
think it's very, very unlikely you'd be able to fry the sensors.

The GPIOs there I'd expect to be reset GPIOs, one for each sensor.
Interesting that they are not handled by ACPI in this case. FWIW, the
tps68470 driver is present also in the upstream kernel.

-- 
Kind regards,

Sakari Ailus

Re: cio2 ipu3 module to automatically connect sensors via swnodes

[Dan Scally]

Hi Sakari - thanks for the reply

On 08/09/2020 09:03, Sakari Ailus wrote:
> On ACPI systems regulators and clocks as well as GPIOs to some extent are
> controlled by AML code in the DSDT and SSDT. There are different ways this
> can be implemented though. It may be that the PMIC in this case is
> controlled entirely from the AML code without the need for a driver.
>
> This might be the case here. It should be possible to figure this out from
> the DSDT and SSDT tables.

Ah - that's interesting, thanks. I'll delve into the SSDT and DSDT 
tables and see if I can spot that happening. Presumably it is the case 
though, as like I say it seems to be working fine without any 
intervention by our sensor drivers.

> If you do not change how the regulators in the PMIC are controlled I'd
> think it's very, very unlikely you'd be able to fry the sensors.
Very reassuring!
> The GPIOs there I'd expect to be reset GPIOs, one for each sensor.
> Interesting that they are not handled by ACPI in this case. FWIW, the
> tps68470 driver is present also in the upstream kernel.
Yeah we found the tps68470 gpio driver (actually andriy pointed it out I 
think) - it seems that the pins _provided_ by that driver don't actually 
have any affect when toggled though, only the ones allocated to the PMIC 
in its _CRM seem to turn the sensors on/off when toggled (at least, 
switching those off is the only thing that stops the sensor from 
appearing in i2cdetect). The pins from the PMIC's _CRM seem to just be 
system GPIO pins, controllable with `gpioset gpiochip0` for example. For 
the most part we've been controlling them in the sensor drivers by 
evaluating the sensor's _DEP entry in ACPI to get to the PMIC's 
acpi_device. That does seem a little hackish though, and it's definitely 
pretty ugly.

Re: cio2 ipu3 module to automatically connect sensors via swnodes

[Andy Shevchenko]

On Tue, Sep 08, 2020 at 10:40:09AM +0100, Dan Scally wrote:
> On 08/09/2020 09:03, Sakari Ailus wrote:
> > On ACPI systems regulators and clocks as well as GPIOs to some extent are
> > controlled by AML code in the DSDT and SSDT. There are different ways this
> > can be implemented though. It may be that the PMIC in this case is
> > controlled entirely from the AML code without the need for a driver.
> > 
> > This might be the case here. It should be possible to figure this out from
> > the DSDT and SSDT tables.
> 
> Ah - that's interesting, thanks. I'll delve into the SSDT and DSDT tables
> and see if I can spot that happening. Presumably it is the case though, as
> like I say it seems to be working fine without any intervention by our
> sensor drivers.
> 
> > If you do not change how the regulators in the PMIC are controlled I'd
> > think it's very, very unlikely you'd be able to fry the sensors.
> Very reassuring!
> > The GPIOs there I'd expect to be reset GPIOs, one for each sensor.
> > Interesting that they are not handled by ACPI in this case. FWIW, the
> > tps68470 driver is present also in the upstream kernel.

There are 3 GPIOs (per PMIC, AFAIR) that are controlled via AML. But I dunno
what is going on in those methods and what the purpose is.

> Yeah we found the tps68470 gpio driver (actually andriy pointed it out I
> think) - it seems that the pins _provided_ by that driver don't actually
> have any affect when toggled though, only the ones allocated to the PMIC in
> its _CRM seem to turn the sensors on/off when toggled (at least, switching
> those off is the only thing that stops the sensor from appearing in
> i2cdetect). The pins from the PMIC's _CRM seem to just be system GPIO pins,
> controllable with `gpioset gpiochip0` for example. For the most part we've
> been controlling them in the sensor drivers by evaluating the sensor's _DEP
> entry in ACPI to get to the PMIC's acpi_device. That does seem a little
> hackish though, and it's definitely pretty ugly.

-- 
With Best Regards,
Andy Shevchenko

Re: cio2 ipu3 module to automatically connect sensors via swnodes

[Sakari Ailus]

Hi Dan,

On Tue, Sep 08, 2020 at 10:40:09AM +0100, Dan Scally wrote:
> Hi Sakari - thanks for the reply
> 
> On 08/09/2020 09:03, Sakari Ailus wrote:
> > On ACPI systems regulators and clocks as well as GPIOs to some extent are
> > controlled by AML code in the DSDT and SSDT. There are different ways this
> > can be implemented though. It may be that the PMIC in this case is
> > controlled entirely from the AML code without the need for a driver.
> > 
> > This might be the case here. It should be possible to figure this out from
> > the DSDT and SSDT tables.
> 
> Ah - that's interesting, thanks. I'll delve into the SSDT and DSDT tables
> and see if I can spot that happening. Presumably it is the case though, as
> like I say it seems to be working fine without any intervention by our
> sensor drivers.
> 
> > If you do not change how the regulators in the PMIC are controlled I'd
> > think it's very, very unlikely you'd be able to fry the sensors.
> Very reassuring!
> > The GPIOs there I'd expect to be reset GPIOs, one for each sensor.
> > Interesting that they are not handled by ACPI in this case. FWIW, the
> > tps68470 driver is present also in the upstream kernel.
> Yeah we found the tps68470 gpio driver (actually andriy pointed it out I
> think) - it seems that the pins _provided_ by that driver don't actually
> have any affect when toggled though, only the ones allocated to the PMIC in
> its _CRM seem to turn the sensors on/off when toggled (at least, switching
> those off is the only thing that stops the sensor from appearing in
> i2cdetect). The pins from the PMIC's _CRM seem to just be system GPIO pins,
> controllable with `gpioset gpiochip0` for example. For the most part we've
> been controlling them in the sensor drivers by evaluating the sensor's _DEP
> entry in ACPI to get to the PMIC's acpi_device. That does seem a little
> hackish though, and it's definitely pretty ugly.

In the end there should be no driver changes required to the sensor or
other peripheral drivers. But I guess at this point it'd be important to
get things in working order, before they can be cleaned up.

Would you be able to pass me the ACPI tables from this machine btw.?

-- 
Kind regards,

Sakari Ailus

Re: cio2 ipu3 module to automatically connect sensors via swnodes

[Heikki Krogerus]

Hi,

On Sat, Sep 05, 2020 at 09:19:51AM +0100, Daniel Scally wrote:
> Hello
> 
> Following on from this thread:
> https://www.spinics.net/lists/linux-driver-devel/msg135122.html -
> apologies, can't see a way to reply to it directly.
> 
> Myself and others [1] have been trying to get cameras working on
> Microsoft Surface and similar platforms, currently I'm working on
> expanding Jordan's module connecting cameras to the cio2
> infrastructure (which works - we can use it to take images), aiming to
> discover connection information from SSDB in the DSDT, as well as
> connect as many supported sensors as are found on the device. I'm just
> struggling with a problem I've encountered using the swnode patch that
> Heikki linked in that thread; the module's working ok when I only
> attempt to connect a single one of my sensors (by preventing the
> driver for the other sensor from loading, in which case this new
> module ignores the sensor), but when I attempt to connect both cameras
> at the same time I get a kernel oops as part of
> software_node_get_next_child. The module is creating software_nodes
> like this...
> 
> /sys/kernel/software_node/INT343E/port0/endpoint0
> /sys/kernel/software_node/INT343E/port1/endpoint0
> /sys/kernel/software_node/OVTI2680/port0/endpoint0
> /sys/kernel/software_node/OVTI5648/port0/endpoint0
> 
> And that's the structure that I expect to see, but it seems like the
> call to list_next_entry in that function is returning something that
> isn't quite a valid swnode. Printing the address of c->fwnode after
> that point returns "3", which isn't a valid address of course, and
> that's causing the oops when it's either returned (in the version of
> the function without the patches) or when the program flow tries to
> call the "get" op against that fwnode (in the patched version). I've
> been trying to track it down for a while now without success, so I
> posted the problem on SO[2] and it was suggested that I mail these
> addressees for advice - hope that that is ok.
> 
> My copy of Jordan's module is parked in my git repo [3] for now, and
> requires a batch of patches from Jordan's repo [4] (one made by Heikki
> to fill in the missing swnode graph pieces, and some further tweaks) -
> I've been applying those against 5.8.0-rc7. Any other criticism more
> than welcome - I'm new to both c and kernel programming so I'm happy
> to take all the advice people have the time to give.

I'm almost certain that my graph patch is broken. My tests did not
cover nearly as much that is needed to properly test the patch. I
think at least the refcount handling is totally broken in
software_node_graph_get_next_endpoint(), so that function at least
needs to be rewritten.

Unfortunately I do not have time to work on that patch right now.

thanks,

-- 
heikki

Re: cio2 ipu3 module to automatically connect sensors via swnodes

[Dan Scally]

Hi Heikki

On 09/09/2020 14:40, Heikki Krogerus wrote:
> Hi,
>
> On Sat, Sep 05, 2020 at 09:19:51AM +0100, Daniel Scally wrote:
>> Hello
>>
>> Following on from this thread:
>> https://www.spinics.net/lists/linux-driver-devel/msg135122.html -
>> apologies, can't see a way to reply to it directly.
>>
>> Myself and others [1] have been trying to get cameras working on
>> Microsoft Surface and similar platforms, currently I'm working on
>> expanding Jordan's module connecting cameras to the cio2
>> infrastructure (which works - we can use it to take images), aiming to
>> discover connection information from SSDB in the DSDT, as well as
>> connect as many supported sensors as are found on the device. I'm just
>> struggling with a problem I've encountered using the swnode patch that
>> Heikki linked in that thread; the module's working ok when I only
>> attempt to connect a single one of my sensors (by preventing the
>> driver for the other sensor from loading, in which case this new
>> module ignores the sensor), but when I attempt to connect both cameras
>> at the same time I get a kernel oops as part of
>> software_node_get_next_child. The module is creating software_nodes
>> like this...
>>
>> /sys/kernel/software_node/INT343E/port0/endpoint0
>> /sys/kernel/software_node/INT343E/port1/endpoint0
>> /sys/kernel/software_node/OVTI2680/port0/endpoint0
>> /sys/kernel/software_node/OVTI5648/port0/endpoint0
>>
>> And that's the structure that I expect to see, but it seems like the
>> call to list_next_entry in that function is returning something that
>> isn't quite a valid swnode. Printing the address of c->fwnode after
>> that point returns "3", which isn't a valid address of course, and
>> that's causing the oops when it's either returned (in the version of
>> the function without the patches) or when the program flow tries to
>> call the "get" op against that fwnode (in the patched version). I've
>> been trying to track it down for a while now without success, so I
>> posted the problem on SO[2] and it was suggested that I mail these
>> addressees for advice - hope that that is ok.
>>
>> My copy of Jordan's module is parked in my git repo [3] for now, and
>> requires a batch of patches from Jordan's repo [4] (one made by Heikki
>> to fill in the missing swnode graph pieces, and some further tweaks) -
>> I've been applying those against 5.8.0-rc7. Any other criticism more
>> than welcome - I'm new to both c and kernel programming so I'm happy
>> to take all the advice people have the time to give.
> I'm almost certain that my graph patch is broken. My tests did not
> cover nearly as much that is needed to properly test the patch. I
> think at least the refcount handling is totally broken in
> software_node_graph_get_next_endpoint(), so that function at least
> needs to be rewritten.
>
> Unfortunately I do not have time to work on that patch right now.
>
> thanks,

Alright no problem; I shall persevere with what we have and see if I can 
figure out why the linking isn't working. I think I actually found the 
problem with refcount handling in software_node_graph_get_endpoint(); 
see 
https://lore.kernel.org/linux-media/2d4f1abb-c617-476a-1005-0ed91906a5f5@gmail.com/T/#mf0dcf7dd78ae0cd40af998bc25a5a775c7e3f93d 
- that function seems to behave as expected after a small tweak (at 
least as I expect it to...it doesn't get and hold 4x references to the 
port fwnode_handle now), but there might be one more refcount handling 
issue somewhere (I see one more reference held than I expect to see at 
the moment...but it could be from my code too of course).


If I get to the bottom of the problems I'll let you know.


Regards

Dan

Re: cio2 ipu3 module to automatically connect sensors via swnodes

[Dan Scally]

Hello Heikki

On 09/09/2020 14:40, Heikki Krogerus wrote:
> I'm almost certain that my graph patch is broken. My tests did not
> cover nearly as much that is needed to properly test the patch. I
> think at least the refcount handling is totally broken in
> software_node_graph_get_next_endpoint(), so that function at least
> needs to be rewritten.
>
> Unfortunately I do not have time to work on that patch right now.
>
> thanks,

I managed to get to the bottom of the remaining issue (which was the 
cause of the problem I originally posted here about - that's now all 
resolved).  In addition to the refcount handling problems, 
software_node_graph_get_next_endpoint() was occasionally passing an 
invalid combination of parameters to software_node_get_next_child(); 
sometimes it would pass an existing endpoint as old in combination with 
a port which was not the endpoint's parent. Applying this on top of your 
patch seems to stop both of those errors:

---
  drivers/base/swnode.c | 13 +++++++++++++
  1 file changed, 13 insertions(+)

diff --git a/drivers/base/swnode.c b/drivers/base/swnode.c
index 5cf9f1eef96f..80255e0b7739 100644
--- a/drivers/base/swnode.c
+++ b/drivers/base/swnode.c
@@ -563,6 +563,7 @@ software_node_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
  {
  	struct swnode *swnode = to_swnode(fwnode);
  	struct fwnode_handle *old = endpoint;
+	struct fwnode_handle *parent_of_old;
  	struct fwnode_handle *parent;
  	struct fwnode_handle *port;
  
@@ -581,10 +582,22 @@ software_node_graph_get_next_endpoint(const struct fwnode_handle *fwnode,
  	}
  
  	for (; port; port = swnode_graph_find_next_port(parent, port)) {
+
+		if (old) {
+			parent_of_old = software_node_get_parent(old);
+
+			if (parent_of_old != port)
+				old = NULL;
+
+			fwnode_handle_put(parent_of_old);
+		}
+
  		endpoint = software_node_get_next_child(port, old);
  		fwnode_handle_put(old);
  		if (endpoint)
  			break;
+		else
+			fwnode_handle_put(port);
  	}
  
  	fwnode_handle_put(port);
-- 
2.25.1

Following that change, everything seems to be working ok. The module linking sensors to the cio2 infrastructure can correctly link multiple sensors now, and the reference count issues that prevented that module from unloading are resolved too.

Getting those patches and the bridge module upstream would be a good step to getting working cameras on ACPI platforms using ipu3. I'd like to take that on if you haven't the time; would you be ok with me applying my changes on top of your original patch, and submitting the combined result as a v2? And then I'll tackle any changes that come back - might take me a little while but I should be able to manage it.

Thanks
Dan

Re: cio2 ipu3 module to automatically connect sensors via swnodes

[Andy Shevchenko]

On Sat, Sep 12, 2020 at 08:45:10AM +0100, Dan Scally wrote:
> Hello Heikki
> 
> On 09/09/2020 14:40, Heikki Krogerus wrote:
> > I'm almost certain that my graph patch is broken. My tests did not
> > cover nearly as much that is needed to properly test the patch. I
> > think at least the refcount handling is totally broken in
> > software_node_graph_get_next_endpoint(), so that function at least
> > needs to be rewritten.
> > 
> > Unfortunately I do not have time to work on that patch right now.
> > 
> > thanks,
> 
> I managed to get to the bottom of the remaining issue (which was the cause
> of the problem I originally posted here about - that's now all resolved). 
> In addition to the refcount handling problems,
> software_node_graph_get_next_endpoint() was occasionally passing an invalid
> combination of parameters to software_node_get_next_child(); sometimes it
> would pass an existing endpoint as old in combination with a port which was
> not the endpoint's parent. Applying this on top of your patch seems to stop
> both of those errors:

Can you send a formal patch where you will be a co-developer to Heikki?
Though I don't remember if Heikki signed off it.

-- 
With Best Regards,
Andy Shevchenko

Re: cio2 ipu3 module to automatically connect sensors via swnodes

[Dan Scally]

On 14/09/2020 15:58, Andy Shevchenko wrote:
> On Sat, Sep 12, 2020 at 08:45:10AM +0100, Dan Scally wrote:
>> Hello Heikki
>>
>> On 09/09/2020 14:40, Heikki Krogerus wrote:
>>> I'm almost certain that my graph patch is broken. My tests did not
>>> cover nearly as much that is needed to properly test the patch. I
>>> think at least the refcount handling is totally broken in
>>> software_node_graph_get_next_endpoint(), so that function at least
>>> needs to be rewritten.
>>>
>>> Unfortunately I do not have time to work on that patch right now.
>>>
>>> thanks,
>>
>> I managed to get to the bottom of the remaining issue (which was the cause
>> of the problem I originally posted here about - that's now all resolved).
>> In addition to the refcount handling problems,
>> software_node_graph_get_next_endpoint() was occasionally passing an invalid
>> combination of parameters to software_node_get_next_child(); sometimes it
>> would pass an existing endpoint as old in combination with a port which was
>> not the endpoint's parent. Applying this on top of your patch seems to stop
>> both of those errors:
> 
> Can you send a formal patch where you will be a co-developer to Heikki?
> Though I don't remember if Heikki signed off it.
> 

The original has his Signed-off-by on it yes, but it wasn't raised as a 
formal patch. I'll raise one and send it yes; probably tomorrow.

Thanks,
Dan