Re: [PATCH 5/6] mx31moboard: camera support

Re: [PATCH 5/6] mx31moboard: camera support

[Valentin Longchamp]

Sascha Hauer wrote:
> On Mon, Oct 19, 2009 at 06:41:13PM +0200, Valentin Longchamp wrote:
>> Hi Guennadi,
>>
>> Guennadi Liakhovetski wrote:
>>> Hi
>>>
>>> On Thu, 15 Oct 2009, Valentin Longchamp wrote:
>>>
>>>> We have two mt9t031 cameras that have a muxed bus on the robot.
>>>> We can control which one we are using with gpio outputs. This
>>>> currently is not optimal
>>> So, what prevents you from registering two platform devices for your 
>>> two cameras? Is there a problem with that?
>> The lack of time until now to do it properly. I sent those patches as  
>> initial RFC (and by the way thanks for your comment).
>>
>> I would like to have one video interface only and that I can switch  
>> between the two physical camera using a quite simple system call. Would  
>> that be compatible with registering the two platform devices ?
> 
> Wouldn't it be better to have /dev/video[01] for two cameras? How do
> keep the registers synchron between both cameras otherwise?
> 

Well, from my experimentations, most initializations are done when you 
open the device. So if you close the device, switch camera and open it 
again, the registers are initialized with the need values. Of course 
there is a problem is you switch camera while the device is open.

It could be ok with /dev/video[01], but I would need something that 
would prevent one device to be opened when the other already is open (a 
mutex, but where ?).

Besides, I have read a slide from Dongsoo Kim 
(http://www.celinuxforum.org/CelfPubWiki/ELC2009Presentations?action=AttachFile&do=get&target=Framework_for_digital_camera_in_linux-in_detail.ppt 
slides 41-47) and the cleanest solution would be to have the two chips 
enumerated with VIDIOC_ENUMINPUT as proposed. What would then be the 
v4l2 call to switch from one device to each other ? How to "link" it 
with the kernel code that make the real hardware switching ?

Thanks for your inputs.

Val

-- 
Valentin Longchamp, PhD Student, EPFL-STI-LSRO1
valentin.longchamp@epfl.ch, Phone: +41216937827
http://people.epfl.ch/valentin.longchamp
MEA3485, Station 9, CH-1015 Lausanne

Re: [PATCH 5/6] mx31moboard: camera support

[Guennadi Liakhovetski]

On Wed, 21 Oct 2009, Valentin Longchamp wrote:

> Sascha Hauer wrote:
> > On Mon, Oct 19, 2009 at 06:41:13PM +0200, Valentin Longchamp wrote:
> > > Hi Guennadi,
> > > 
> > > Guennadi Liakhovetski wrote:
> > > > Hi
> > > > 
> > > > On Thu, 15 Oct 2009, Valentin Longchamp wrote:
> > > > 
> > > > > We have two mt9t031 cameras that have a muxed bus on the robot.
> > > > > We can control which one we are using with gpio outputs. This
> > > > > currently is not optimal
> > > > So, what prevents you from registering two platform devices for your two
> > > > cameras? Is there a problem with that?
> > > The lack of time until now to do it properly. I sent those patches as
> > > initial RFC (and by the way thanks for your comment).
> > > 
> > > I would like to have one video interface only and that I can switch
> > > between the two physical camera using a quite simple system call. Would
> > > that be compatible with registering the two platform devices ?
> > 
> > Wouldn't it be better to have /dev/video[01] for two cameras? How do
> > keep the registers synchron between both cameras otherwise?
> > 
> 
> Well, from my experimentations, most initializations are done when you open
> the device. So if you close the device, switch camera and open it again, the
> registers are initialized with the need values. Of course there is a problem
> is you switch camera while the device is open.
> 
> It could be ok with /dev/video[01], but I would need something that would
> prevent one device to be opened when the other already is open (a mutex, but
> where ?).
> 
> Besides, I have read a slide from Dongsoo Kim
> (http://www.celinuxforum.org/CelfPubWiki/ELC2009Presentations?action=AttachFile&do=get&target=Framework_for_digital_camera_in_linux-in_detail.ppt
> slides 41-47) and the cleanest solution would be to have the two chips
> enumerated with VIDIOC_ENUMINPUT as proposed. What would then be the v4l2 call
> to switch from one device to each other ? How to "link" it with the kernel
> code that make the real hardware switching ?

Ok, I don't have a definite answer to this, so, just my thoughts:

1. soc-camera currently registers one struct v4l2_device device per _host_ 
immediately upon its registration, and one struct video_device per 
_client_ platform device.

2. we currently have 1 or 2 boards in the mainline with two video client 
devices on one interface: arch/sh/boards/mach-migor/ and (unsure about) 
arch/sh/boards/board-ap325rxa.c. At least the first of them exports two 
platform devices and thus gets /dev/video[01]. Accesses are synchronised 
with a mutex (I don't actually like that, I'd prefer to get a -EBUSY back 
instead of hanging in D in open()), and a successful acquisition of the 
mutex switches the respective camera on. See code for details. So, this 
approach is supported and it works. In this case we have one v4l2_device 
and two video_device instances, don't know whether this matches how this 
is supposed to be done, but it works so far:-)

3. to support switching inputs, significant modifications to soc_camera.c 
would be required. I read Nate's argument before, that as long as clients 
can only be accessed one at a time, this should be presented by multiple 
inputs rather than multiple device nodes. Somebody else from the V4L folk 
has also confirmed this opinion. In principle I don't feel strongly either 
way. But currently soc-camera uses a one i2c client to one device node 
model, and I'm somewhat reluctant to change this before we're done with 
the v4l2-subdev conversion.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 5/6] mx31moboard: camera support

[Valentin Longchamp]

Guennadi Liakhovetski wrote:
> On Wed, 21 Oct 2009, Valentin Longchamp wrote:
> 
>> Sascha Hauer wrote:
>>> On Mon, Oct 19, 2009 at 06:41:13PM +0200, Valentin Longchamp wrote:
>>>> Hi Guennadi,
>>>>
>>>> Guennadi Liakhovetski wrote:
>>>>> Hi
>>>>>
>>>>> On Thu, 15 Oct 2009, Valentin Longchamp wrote:
>>>>>
>>>>>> We have two mt9t031 cameras that have a muxed bus on the robot.
>>>>>> We can control which one we are using with gpio outputs. This
>>>>>> currently is not optimal
>>>>> So, what prevents you from registering two platform devices for your two
>>>>> cameras? Is there a problem with that?
>>>> The lack of time until now to do it properly. I sent those patches as
>>>> initial RFC (and by the way thanks for your comment).
>>>>
>>>> I would like to have one video interface only and that I can switch
>>>> between the two physical camera using a quite simple system call. Would
>>>> that be compatible with registering the two platform devices ?
>>> Wouldn't it be better to have /dev/video[01] for two cameras? How do
>>> keep the registers synchron between both cameras otherwise?
>>>
>> Well, from my experimentations, most initializations are done when you open
>> the device. So if you close the device, switch camera and open it again, the
>> registers are initialized with the need values. Of course there is a problem
>> is you switch camera while the device is open.
>>
>> It could be ok with /dev/video[01], but I would need something that would
>> prevent one device to be opened when the other already is open (a mutex, but
>> where ?).
>>
>> Besides, I have read a slide from Dongsoo Kim
>> (http://www.celinuxforum.org/CelfPubWiki/ELC2009Presentations?action=AttachFile&do=get&target=Framework_for_digital_camera_in_linux-in_detail.ppt
>> slides 41-47) and the cleanest solution would be to have the two chips
>> enumerated with VIDIOC_ENUMINPUT as proposed. What would then be the v4l2 call
>> to switch from one device to each other ? How to "link" it with the kernel
>> code that make the real hardware switching ?
> 
> Ok, I don't have a definite answer to this, so, just my thoughts:
> 
> 1. soc-camera currently registers one struct v4l2_device device per _host_ 
> immediately upon its registration, and one struct video_device per 
> _client_ platform device.

Ok understood.

> 
> 2. we currently have 1 or 2 boards in the mainline with two video client 
> devices on one interface: arch/sh/boards/mach-migor/ and (unsure about) 
> arch/sh/boards/board-ap325rxa.c. At least the first of them exports two 
> platform devices and thus gets /dev/video[01]. Accesses are synchronised 
> with a mutex (I don't actually like that, I'd prefer to get a -EBUSY back 
> instead of hanging in D in open()), and a successful acquisition of the 
> mutex switches the respective camera on. See code for details. So, this 
> approach is supported and it works. In this case we have one v4l2_device 
> and two video_device instances, don't know whether this matches how this 
> is supposed to be done, but it works so far:-)

I am going to stick to this approach since it works now. This would 
allow me to have code that could go now into mainline.

> 
> 3. to support switching inputs, significant modifications to soc_camera.c 
> would be required. I read Nate's argument before, that as long as clients 
> can only be accessed one at a time, this should be presented by multiple 
> inputs rather than multiple device nodes. Somebody else from the V4L folk 
> has also confirmed this opinion. In principle I don't feel strongly either 
> way. But currently soc-camera uses a one i2c client to one device node 
> model, and I'm somewhat reluctant to change this before we're done with 
> the v4l2-subdev conversion.
> 

Sure, one step at a time. So for now the switching is not possible with 
soc_camera.

My problem is that both cameras have the same I2C address since they are 
the same.

Would I need to declare 2 i2c_device with the same address (I'm not sure 
it would even work ...) used by two _client_ platform_devices or would I 
have to have the two platform devices pointing to the same i2c_device ?

Thanks

Val

-- 
Valentin Longchamp, PhD Student, EPFL-STI-LSRO1
valentin.longchamp@epfl.ch, Phone: +41216937827
http://people.epfl.ch/valentin.longchamp
MEA3485, Station 9, CH-1015 Lausanne

Re: [PATCH 5/6] mx31moboard: camera support

[Valentin Longchamp]

Hi Guennadi,

Valentin Longchamp wrote:
> Guennadi Liakhovetski wrote:
> 
>> 3. to support switching inputs, significant modifications to soc_camera.c 
>> would be required. I read Nate's argument before, that as long as clients 
>> can only be accessed one at a time, this should be presented by multiple 
>> inputs rather than multiple device nodes. Somebody else from the V4L folk 
>> has also confirmed this opinion. In principle I don't feel strongly either 
>> way. But currently soc-camera uses a one i2c client to one device node 
>> model, and I'm somewhat reluctant to change this before we're done with 
>> the v4l2-subdev conversion.
>>
> 
> Sure, one step at a time. So for now the switching is not possible with 
> soc_camera.
> 
> My problem is that both cameras have the same I2C address since they are 
> the same.
> 
> Would I need to declare 2 i2c_device with the same address (I'm not sure 
> it would even work ...) used by two _client_ platform_devices or would I 
> have to have the two platform devices pointing to the same i2c_device ?
> 

I've finally had time to test all this. My current problem with 
registering the two cameras is that they both have the same i2c address, 
and soc_camera calls v4l2_i2c_new_subdev_board where in my case the same 
address on the same i2c tries to be registered and of course fails.

We would need a way in soc_camera not to register a new i2c client for 
device but use an existing one (but that's what you don't want to change 
for now as you state it in your above last sentence). I just want to 
point this out once more so that you know there is interest about this 
for the next soc_camera works.

So my current solution for mainline inclusion is to register only one 
camera device node without taking care of the cam mux for now.

Val

-- 
Valentin Longchamp, PhD Student, EPFL-STI-LSRO1
valentin.longchamp@epfl.ch, Phone: +41216937827
http://people.epfl.ch/valentin.longchamp
MEA3485, Station 9, CH-1015 Lausanne

Re: [PATCH 5/6] mx31moboard: camera support

[Guennadi Liakhovetski]

On Tue, 3 Nov 2009, Valentin Longchamp wrote:

> Hi Guennadi,
> 
> Valentin Longchamp wrote:
> > Guennadi Liakhovetski wrote:
> > 
> > > 3. to support switching inputs, significant modifications to soc_camera.c
> > > would be required. I read Nate's argument before, that as long as clients
> > > can only be accessed one at a time, this should be presented by multiple
> > > inputs rather than multiple device nodes. Somebody else from the V4L folk
> > > has also confirmed this opinion. In principle I don't feel strongly either
> > > way. But currently soc-camera uses a one i2c client to one device node
> > > model, and I'm somewhat reluctant to change this before we're done with
> > > the v4l2-subdev conversion.
> > > 
> > 
> > Sure, one step at a time. So for now the switching is not possible with
> > soc_camera.
> > 
> > My problem is that both cameras have the same I2C address since they are the
> > same.
> > 
> > Would I need to declare 2 i2c_device with the same address (I'm not sure it
> > would even work ...) used by two _client_ platform_devices or would I have
> > to have the two platform devices pointing to the same i2c_device ?
> > 
> 
> I've finally had time to test all this. My current problem with registering
> the two cameras is that they both have the same i2c address, and soc_camera
> calls v4l2_i2c_new_subdev_board where in my case the same address on the same
> i2c tries to be registered and of course fails.
> 
> We would need a way in soc_camera not to register a new i2c client for device
> but use an existing one (but that's what you don't want to change for now as
> you state it in your above last sentence). I just want to point this out once
> more so that you know there is interest about this for the next soc_camera
> works.

These are two separate issues: inability to work with two devices with the 
same i2c address, and arguably suboptimal choice of the way to switch 
between multiple mutually-exclusive clients (sensors) on a single 
interface.

For multiple chips with the same adderess, in principle you could register 
one or more video devices yet before registering respective i2c devices. 
And then on the selected switching operation (either opening of one of the 
/dev/video* nodes, or selecting an input) you register the i2c device, 
probe it, etc. This would work, but looks seriously overengineered to me. 
And it would indeed require pretty fundamental changes to the soc-camera 
core.

Otherwise we could push this switching down into the driver / platform. We 
could just export only one camera from the platform code, implement a 
S_INPUT method in soc-camera, that would be delivered to the sensor 
driver, it would save context of the current sensor, call the platform 
hook to switch to another camera, and restore its configuration. In this 
case the soc-camera core and the host driver would not see two sensors, 
but just one, all the switching would be done internally in the sensor 
driver / platform callback.

If we also decide to use S_INPUT to switch between different sensors on an 
interface, we would have to make a distinction between two cases in the 
core - whether the input we're switching to belongs to the "same" sensor 
or to another one.

> So my current solution for mainline inclusion is to register only one camera
> device node without taking care of the cam mux for now.

Yes, please, send me an updated version of the patch. I think, you haven't 
done that yet, right?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 5/6] mx31moboard: camera support

[Valentin Longchamp]

Guennadi Liakhovetski wrote:
> On Tue, 3 Nov 2009, Valentin Longchamp wrote:
> 
>> Hi Guennadi,
>>
>> Valentin Longchamp wrote:
>>> Guennadi Liakhovetski wrote:
>>>
>>>> 3. to support switching inputs, significant modifications to soc_camera.c
>>>> would be required. I read Nate's argument before, that as long as clients
>>>> can only be accessed one at a time, this should be presented by multiple
>>>> inputs rather than multiple device nodes. Somebody else from the V4L folk
>>>> has also confirmed this opinion. In principle I don't feel strongly either
>>>> way. But currently soc-camera uses a one i2c client to one device node
>>>> model, and I'm somewhat reluctant to change this before we're done with
>>>> the v4l2-subdev conversion.
>>>>
>>> Sure, one step at a time. So for now the switching is not possible with
>>> soc_camera.
>>>
>>> My problem is that both cameras have the same I2C address since they are the
>>> same.
>>>
>>> Would I need to declare 2 i2c_device with the same address (I'm not sure it
>>> would even work ...) used by two _client_ platform_devices or would I have
>>> to have the two platform devices pointing to the same i2c_device ?
>>>
>> I've finally had time to test all this. My current problem with registering
>> the two cameras is that they both have the same i2c address, and soc_camera
>> calls v4l2_i2c_new_subdev_board where in my case the same address on the same
>> i2c tries to be registered and of course fails.
>>
>> We would need a way in soc_camera not to register a new i2c client for device
>> but use an existing one (but that's what you don't want to change for now as
>> you state it in your above last sentence). I just want to point this out once
>> more so that you know there is interest about this for the next soc_camera
>> works.
> 
> These are two separate issues: inability to work with two devices with the 
> same i2c address, and arguably suboptimal choice of the way to switch 
> between multiple mutually-exclusive clients (sensors) on a single 
> interface.
> 
> For multiple chips with the same adderess, in principle you could register 
> one or more video devices yet before registering respective i2c devices. 
> And then on the selected switching operation (either opening of one of the 
> /dev/video* nodes, or selecting an input) you register the i2c device, 
> probe it, etc. This would work, but looks seriously overengineered to me. 
> And it would indeed require pretty fundamental changes to the soc-camera 
> core.

Yeah I had noticed that this was possible by not calling 
i2c_register_device (or some like that) is soc_camera.c and give the i2c 
device directly to the soc_camera client device init method, but since 
this requires changes in the soc_camera core code that you are currently 
heavily modifying, I did not find it usefull.

> 
> Otherwise we could push this switching down into the driver / platform. We 
> could just export only one camera from the platform code, implement a 
> S_INPUT method in soc-camera, that would be delivered to the sensor 
> driver, it would save context of the current sensor, call the platform 
> hook to switch to another camera, and restore its configuration. In this 
> case the soc-camera core and the host driver would not see two sensors, 
> but just one, all the switching would be done internally in the sensor 
> driver / platform callback.
> 
> If we also decide to use S_INPUT to switch between different sensors on an 
> interface, we would have to make a distinction between two cases in the 
> core - whether the input we're switching to belongs to the "same" sensor 
> or to another one.

Leaving the the camera switch to platform code looks very important to me.

Having only one camera exported looks fine to me, especially since I 
have both cameras the same (but I don't think it would be possible with 
two different sensors ?). But I don't know v4l2 API well enough to see 
when it would be used to switch to an input on the same physical sensor.

> 
>> So my current solution for mainline inclusion is to register only one camera
>> device node without taking care of the cam mux for now.
> 
> Yes, please, send me an updated version of the patch. I think, you haven't 
> done that yet, right?

I have the updated version, I have however forgotten to add you in the 
recipient list, have a look on the arm-mailing-list: 
http://article.gmane.org/gmane.linux.ports.arm.kernel/68123

Thanks for all your comments

Val

-- 
Valentin Longchamp, PhD Student, EPFL-STI-LSRO1
valentin.longchamp@epfl.ch, Phone: +41216937827
http://people.epfl.ch/valentin.longchamp
MEA3485, Station 9, CH-1015 Lausanne