[Letux-kernel] omap3 isp -- media-ctl

[H. Nikolaus Schaller]

Hello Javier,

> Am 08.09.2016 um 13:53 schrieb Javier Martinez Canillas <javier at osg.samsung.com>:
> 
> Hello Nikolaus,
> 
> On 09/08/2016 05:58 AM, H. Nikolaus Schaller wrote:
>> Hi Javier,
>> 
>>> Am 08.09.2016 um 00:20 schrieb Javier Martinez Canillas <javier at osg.samsung.com>:
>>> 
>>> On 09/07/2016 06:04 PM, Javier Martinez Canillas wrote:
>>> 
>>> [snip]
>>> 
>>>>> 
>>>>>> Does the omap3isp driver
>>>>>> probes? and does probe the driver for your attached camera sensor?
>>>>> 
>>>>> Yes, both. Here an excerpt of the boot log and lsmod
>>>>> 
>>>> 
>>>> Yes, both are probed but your camera driver (drivers/media/i2c/ov9650.c) is
>>>> missing a call to v4l2_async_register_subdev() in its probe function to be
>>>> registered async with the bridge driver (omap3isp). You need something like
>>>> 
>>>> commit c7d97499cc8a ("[media] tvp5150: add support for asynchronous probing")
>>>> 
>>> 
>>> In fact, I went and wrote the patch since is pretty trivial. Could you please
>>> test it and if that's the fixes your problem then I can post it to the list?
>> 
>> Yes!!! It makes the significant difference (I would have searched some more weeks
>> for it without your help :):
>> 
> 
> Great, I'm glad that it helped.
> 
>> root at letux:~# ls -l /dev/media*
>> crw------- 1 root root 245, 0 Jan  1  2000 /dev/media0
>> root at letux:~# ls -l /dev/video*
>> crw-rw---- 1 root video 81, 0 Jan  1  2000 /dev/video0
>> crw-rw---- 1 root video 81, 1 Jan  1  2000 /dev/video1
>> crw-rw---- 1 root video 81, 2 Jan  1  2000 /dev/video2
>> crw-rw---- 1 root video 81, 3 Jan  1  2000 /dev/video3
>> crw-rw---- 1 root video 81, 4 Jan  1  2000 /dev/video4
>> crw-rw---- 1 root video 81, 5 Jan  1  2000 /dev/video5
>> crw-rw---- 1 root video 81, 6 Jan  1  2000 /dev/video6
>> root at letux:~# 
>> 
>> I still get a "green" image in mplayer but that could be bugs or missing pieces
>> in our camera module setup, which I wasn't able to debug so far.
>> 
> 
> That's an unrelated issue indeed.
> 
>> We use an ov9655 chip and the original platform data based driver is for ov9650
>> and ov9652 only. And these sensors have a slightly different register set which
>> we have to account for.
>> 
> 
> Yes, I noticed that you had a define for OV9655 that wasn't present in the upstream
> driver.
> 
>> So we currently disable I2C writing to the camera so that the module comes up
>> in default settings.
>> 
>> 
>> Regarding your patch I could also submit it together with the full set of driver
>> patches (we have added DT support) if you agree. But I don't see any issues if you
>> submit it separately.
>> 
> 
> Since adding async probe is orthogonal to the ov9655 support, I think that's
> better to post it separately. And in any case mention in your patch series
> that you need a previous posted patch to make things to work (but I expect
> this patch to be merged very quick since is trivial and not controversial).
> 
>> The latest patch set (including it) is here:
>> 
>> http://git.goldelico.com/?p=gta04-kernel.git;a=shortlog;h=refs/heads/work/hns/video/ov9655
>> 
> 
> Great, I can't comment on the contents since I'm not familiar with the ov965x
> family. But I noticed that you are missing the [media] prefix in the subjects.
> 
>> This also includes one patch for the isp driver which silences a bad of_node_put() warning.
>> This could also be submitted separately. Should I?
>> 
> 
> I think so, this is a bug fix for omap3isp so I think should be post it ASAP.
> 
>> 
>> For the DT style topic we currently have this style to have an endpoint
>> reference at some separate location in the DT source file.
>> 
>> The reason is that we merge several patch sets into a single one and multiple of
>> them touch the DTS. This construction reduces the risk of difficult to solve
>> git merge conflicts and patch set dependencies because changes to &parallel_ep
>> are not within the code line range of i2c devices. So git merge can more easily
>> identify hunks with inexact line number references.
>> 
> 
> Got it. Makes sense.
> 
>> So indeed it is more difficult to read, but easier to maintain and debug
>> (during development). But if it becomes stable I think it can be inlined
>> as you suggest.
>> 
> 
> Ok, I don't even know if it would be an issue for DT reviewers so maybe
> it can be left at it is.

Quite some months since you helped to get this part working. We were then
distracted by other topics to make the camera driver really work but now I
want to start over...

The status is:
* the camera driver gets probed successfully
* the chip responds
* all device files are created (this was the patch discussed above)
* we can apply mediactl
* we can run mplayer
* but we get a green image (meaning: no video stream from camera)

The core reason seems to be that the camera isn't powered on again when we open
the video stream. And ov965x_open() is not called.

So I think our driver is missing something else to be registered correctly.

Maybe, please can you take a second look into the camera driver and especially
why ov965x_open() is not called:

	http://git.goldelico.com/?p=gta04-kernel.git;a=blob;f=drivers/media/i2c/ov9650.c;h=b7515b15b012c6ecc591ac80cf622628edd00c4e;hb=d4b18326ab42d99bbb60686c9d6cfa2fab9d7e4f

although the ov965x_sd_internal_ops are defined.

Our shell script to display a video stream is:

	http://git.goldelico.com/?p=gta04-kernel.git;a=blob;f=Letux/root/camera-demo;h=2858acac4fad5b684a0ec3a899a0c7b706f64292;hb=d4b18326ab42d99bbb60686c9d6cfa2fab9d7e4f

I think as soon as we get the camera enabled when needed we can then debug
video format issues etc.

Or whom should we contact with this question?

BR and thanks,
Nikolaus