[omap3camera] Camera bring-up on Zoom3 (OMAP3630)

[omap3camera] Camera bring-up on Zoom3 (OMAP3630)

[Aguirre, Sergio]

[-- Attachment #1: Type: text/plain, Size: 771 bytes --]

Hi Laurent, Sakari,

I'm trying to get latest Sakari's tree (gitorious.org/omap3camera)
'devel' branch running on my Zoom3 HW (which has an OMAP3630, and a
Sony IMX046 8MP sensor).

I had first one NULL pointer dereference while the driver was
registering devices and creating entities, which I resolved with
the attached patch. (Is this patch acceptable, or maybe I am missing
something...)

And now, I don't get quite clear on how the created nodes work out.

Now I have /dev/video[0-5], but I don't know how I'm I supposed to handle
them...

Here's my current work-in-progress kernel:

	http://dev.omapzoom.org/?p=saaguirre/linux-omap-camera.git;a=shortlog;h=refs/heads/omap-devel-wip

Can you please give some guidance on it?

Regards,
Sergio

[-- Attachment #2: 0001-omap34xxcam-Add-check-for-null-subdev-platform-data.patch --]
[-- Type: application/octet-stream, Size: 1252 bytes --]

From 0eaf3e91919ddaf2306ff63aaff895547865e111 Mon Sep 17 00:00:00 2001
From: Sergio Aguirre <saaguirre@ti.com>
Date: Fri, 5 Mar 2010 08:44:57 -0600
Subject: [PATCH] omap34xxcam: Add check for null subdev platform data

Signed-off-by: Sergio Aguirre <saaguirre@ti.com>
---
 drivers/media/video/omap34xxcam.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/omap34xxcam.c b/drivers/media/video/omap34xxcam.c
index 9d9f0d3..16683cc 100644
--- a/drivers/media/video/omap34xxcam.c
+++ b/drivers/media/video/omap34xxcam.c
@@ -1365,6 +1365,9 @@ static int omap34xxcam_probe(struct platform_device *pdev)
 
 	for (i = 0; i < OMAP34XXCAM_VIDEODEVS; i++) {
 		struct omap34xxcam_videodev *vdev = &cam->vdevs[i];
+		
+		if (pdata->subdevs[i] == NULL)
+			continue;
 
 		mutex_init(&vdev->mutex);
 		vdev->index = i;
@@ -1397,6 +1400,9 @@ static int omap34xxcam_probe(struct platform_device *pdev)
 	for (i = 0; i < OMAP34XXCAM_VIDEODEVS; i++) {
 		struct omap34xxcam_videodev *vdev = &cam->vdevs[i];
 
+		if (pdata->subdevs[i] == NULL)
+			continue;
+
 		ret = isp_video_register(&vdev->video, &vdev->cam->v4l2_dev);
 		if (ret < 0) {
 			printk(KERN_ERR "%s: could not register video device (%d)\n",
-- 
1.6.3.3

Re: [omap3camera] Camera bring-up on Zoom3 (OMAP3630)

[Laurent Pinchart]

Hi Sergio,

On Friday 05 March 2010 15:54:58 Aguirre, Sergio wrote:
> Hi Laurent, Sakari,

Oops, just noticed I forgot to answer your e-mail, sorry.

> I'm trying to get latest Sakari's tree (gitorious.org/omap3camera)
> 'devel' branch running on my Zoom3 HW (which has an OMAP3630, and a
> Sony IMX046 8MP sensor).
> 
> I had first one NULL pointer dereference while the driver was
> registering devices and creating entities, which I resolved with
> the attached patch. (Is this patch acceptable, or maybe I am missing
> something...)

Either that, or make OMAP34XXCAM_VIDEODEVS dynamic (the value would be passed 
through platform data). The code will be removed (hopefully soon) anyway when 
the legacy video nodes will disappear.

> And now, I don't get quite clear on how the created nodes work out.
> 
> Now I have /dev/video[0-5], but I don't know how I'm I supposed to handle
> them...
> 
> Here's my current work-in-progress kernel:
> 
> 	http://dev.omapzoom.org/?p=saaguirre/linux-omap-camera.git;a=shortlog;h=re
> fs/heads/omap-devel-wip
> 
> Can you please give some guidance on it?

Basically, the driver creates OMAP34XXCAM_VIDEODEVS "legacy" video nodes, one 
for each sensor connected to the ISP. As your board has a single sensor, the 
driver will create the /dev/video0 legacy video node.

Legacy video nodes use hard-coded assumptions that were implemented according 
to Nokia's use cases on the N900. They can only offer a subset of the 
functions available in the hardware.

For full access to the ISP, you will need to use the new video nodes (1 to 5). 
Those video nodes are to be used in conjunction with the media controller. All 
the necessary patches aren't available yet, but they should be soon (it's 
hopefully a matter of days to get the userspace API there).

I will try to make a userspace test application available when the patches 
will be pushed to the linux-omap-camera tree.

-- 
Regards,

Laurent Pinchart

RE: [omap3camera] Camera bring-up on Zoom3 (OMAP3630)

[Aguirre, Sergio]

> -----Original Message-----
> From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com]
> Sent: Tuesday, March 16, 2010 5:30 PM
> To: Aguirre, Sergio
> Cc: Sakari Ailus; linux-media@vger.kernel.org
> Subject: Re: [omap3camera] Camera bring-up on Zoom3 (OMAP3630)
> 
> Hi Sergio,
> 
> On Friday 05 March 2010 15:54:58 Aguirre, Sergio wrote:
> > Hi Laurent, Sakari,
> 
> Oops, just noticed I forgot to answer your e-mail, sorry.

No problem.

> 
> > I'm trying to get latest Sakari's tree (gitorious.org/omap3camera)
> > 'devel' branch running on my Zoom3 HW (which has an OMAP3630, and a
> > Sony IMX046 8MP sensor).
> >
> > I had first one NULL pointer dereference while the driver was
> > registering devices and creating entities, which I resolved with
> > the attached patch. (Is this patch acceptable, or maybe I am missing
> > something...)
> 
> Either that, or make OMAP34XXCAM_VIDEODEVS dynamic (the value would be
> passed
> through platform data). The code will be removed (hopefully soon) anyway
> when
> the legacy video nodes will disappear.

Ok, so should I keep this patch only to myself until this code is removed?

> 
> > And now, I don't get quite clear on how the created nodes work out.
> >
> > Now I have /dev/video[0-5], but I don't know how I'm I supposed to
> handle
> > them...
> >
> > Here's my current work-in-progress kernel:
> >
> > 	http://dev.omapzoom.org/?p=saaguirre/linux-omap-
> camera.git;a=shortlog;h=re
> > fs/heads/omap-devel-wip
> >
> > Can you please give some guidance on it?
> 
> Basically, the driver creates OMAP34XXCAM_VIDEODEVS "legacy" video nodes,
> one
> for each sensor connected to the ISP. As your board has a single sensor,
> the
> driver will create the /dev/video0 legacy video node.

Ok.

> 
> Legacy video nodes use hard-coded assumptions that were implemented
> according
> to Nokia's use cases on the N900. They can only offer a subset of the
> functions available in the hardware.

Hmm... Well, that should get me also at least a simple stream out of it for the moment. :)

> 
> For full access to the ISP, you will need to use the new video nodes (1 to
> 5).
> Those video nodes are to be used in conjunction with the media controller.
> All
> the necessary patches aren't available yet, but they should be soon (it's
> hopefully a matter of days to get the userspace API there).

Great!

> 
> I will try to make a userspace test application available when the patches
> will be pushed to the linux-omap-camera tree.

That would be really nice. :)

Thanks and Regards,
Sergio

> 
> --
> Regards,
> 
> Laurent Pinchart

Re: [omap3camera] Camera bring-up on Zoom3 (OMAP3630)

[Laurent Pinchart]

Hi Sergio,

On Wednesday 17 March 2010 14:52:17 Aguirre, Sergio wrote:
> > -----Original Message-----
> > From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com]
> > Sent: Tuesday, March 16, 2010 5:30 PM
> > To: Aguirre, Sergio
> > Cc: Sakari Ailus; linux-media@vger.kernel.org
> > Subject: Re: [omap3camera] Camera bring-up on Zoom3 (OMAP3630)
> > 
> > Hi Sergio,
> > 
> > On Friday 05 March 2010 15:54:58 Aguirre, Sergio wrote:
> > > Hi Laurent, Sakari,
> > 
> > Oops, just noticed I forgot to answer your e-mail, sorry.
> 
> No problem.
> 
> > > I'm trying to get latest Sakari's tree (gitorious.org/omap3camera)
> > > 'devel' branch running on my Zoom3 HW (which has an OMAP3630, and a
> > > Sony IMX046 8MP sensor).
> > > 
> > > I had first one NULL pointer dereference while the driver was
> > > registering devices and creating entities, which I resolved with
> > > the attached patch. (Is this patch acceptable, or maybe I am missing
> > > something...)
> > 
> > Either that, or make OMAP34XXCAM_VIDEODEVS dynamic (the value would be
> > passed through platform data). The code will be removed (hopefully soon)
> > anyway when the legacy video nodes will disappear.
> 
> Ok, so should I keep this patch only to myself until this code is removed?

I'll let Sakari answer that, but I think they can still go in in the meantime.

-- 
Regards,

Laurent Pinchart

Re: [omap3camera] Camera bring-up on Zoom3 (OMAP3630)

[Sakari Ailus]

Laurent Pinchart wrote:
>>>> I'm trying to get latest Sakari's tree (gitorious.org/omap3camera)
>>>> 'devel' branch running on my Zoom3 HW (which has an OMAP3630, and a
>>>> Sony IMX046 8MP sensor).
>>>>
>>>> I had first one NULL pointer dereference while the driver was
>>>> registering devices and creating entities, which I resolved with
>>>> the attached patch. (Is this patch acceptable, or maybe I am missing
>>>> something...)
>>>
>>> Either that, or make OMAP34XXCAM_VIDEODEVS dynamic (the value would be
>>> passed through platform data). The code will be removed (hopefully soon)
>>> anyway when the legacy video nodes will disappear.
>>
>> Ok, so should I keep this patch only to myself until this code is removed?
> 
> I'll let Sakari answer that, but I think they can still go in in the meantime.

Is there a need for the patch? The other possible device is just left
unused, right?

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

Re: [omap3camera] Camera bring-up on Zoom3 (OMAP3630)

[Laurent Pinchart]

On Wednesday 17 March 2010 15:16:20 Sakari Ailus wrote:
> Laurent Pinchart wrote:
> >>>> I'm trying to get latest Sakari's tree (gitorious.org/omap3camera)
> >>>> 'devel' branch running on my Zoom3 HW (which has an OMAP3630, and a
> >>>> Sony IMX046 8MP sensor).
> >>>> 
> >>>> I had first one NULL pointer dereference while the driver was
> >>>> registering devices and creating entities, which I resolved with
> >>>> the attached patch. (Is this patch acceptable, or maybe I am missing
> >>>> something...)
> >>> 
> >>> Either that, or make OMAP34XXCAM_VIDEODEVS dynamic (the value would be
> >>> passed through platform data). The code will be removed (hopefully
> >>> soon) anyway when the legacy video nodes will disappear.
> >> 
> >> Ok, so should I keep this patch only to myself until this code is
> >> removed?
> > 
> > I'll let Sakari answer that, but I think they can still go in in the
> > meantime.
> 
> Is there a need for the patch? The other possible device is just left
> unused, right?

Without the patch you get an oops because the driver dereferences NULL 
pointers.

-- 
Regards,

Laurent Pinchart

RE: [omap3camera] Camera bring-up on Zoom3 (OMAP3630)

[Aguirre, Sergio]

> -----Original Message-----
> From: Sakari Ailus [mailto:sakari.ailus@maxwell.research.nokia.com]
> Sent: Wednesday, March 17, 2010 9:16 AM
> To: Laurent Pinchart
> Cc: Aguirre, Sergio; linux-media@vger.kernel.org
> Subject: Re: [omap3camera] Camera bring-up on Zoom3 (OMAP3630)
> 
> Laurent Pinchart wrote:
> >>>> I'm trying to get latest Sakari's tree (gitorious.org/omap3camera)
> >>>> 'devel' branch running on my Zoom3 HW (which has an OMAP3630, and a
> >>>> Sony IMX046 8MP sensor).
> >>>>
> >>>> I had first one NULL pointer dereference while the driver was
> >>>> registering devices and creating entities, which I resolved with
> >>>> the attached patch. (Is this patch acceptable, or maybe I am missing
> >>>> something...)
> >>>
> >>> Either that, or make OMAP34XXCAM_VIDEODEVS dynamic (the value would be
> >>> passed through platform data). The code will be removed (hopefully
> soon)
> >>> anyway when the legacy video nodes will disappear.
> >>
> >> Ok, so should I keep this patch only to myself until this code is
> removed?
> >
> > I'll let Sakari answer that, but I think they can still go in in the
> meantime.
> 
> Is there a need for the patch? The other possible device is just left
> unused, right?

There is need for it _if_ I don't change OMAP34XXCAM_VIDEODEVS in
drivers/media/video/omap34xxcam.h, and if I have less devices listed
in the platform data passed from the boardfile.

In this case, OMAP34XXCAM_VIDEODEVS is hardcoded to N900 case, which is 2,
and I only have 1 sensor in my Zoom3.

I guess the patch is just protecting for potential pointer dereferencing, unless we get rid of current OMAP34XXCAM_VIDEODEVS hardcoded value.

Regards,
Sergio

> 
> --
> Sakari Ailus
> sakari.ailus@maxwell.research.nokia.com

Re: [omap3camera] Camera bring-up on Zoom3 (OMAP3630)

[Sakari Ailus]

Aguirre, Sergio wrote:
> 
> 
>> -----Original Message-----
>> From: Sakari Ailus [mailto:sakari.ailus@maxwell.research.nokia.com]
>> Sent: Wednesday, March 17, 2010 9:16 AM
>> To: Laurent Pinchart
>> Cc: Aguirre, Sergio; linux-media@vger.kernel.org
>> Subject: Re: [omap3camera] Camera bring-up on Zoom3 (OMAP3630)
>>
>> Laurent Pinchart wrote:
>>>>>> I'm trying to get latest Sakari's tree (gitorious.org/omap3camera)
>>>>>> 'devel' branch running on my Zoom3 HW (which has an OMAP3630, and a
>>>>>> Sony IMX046 8MP sensor).
>>>>>>
>>>>>> I had first one NULL pointer dereference while the driver was
>>>>>> registering devices and creating entities, which I resolved with
>>>>>> the attached patch. (Is this patch acceptable, or maybe I am missing
>>>>>> something...)
>>>>>
>>>>> Either that, or make OMAP34XXCAM_VIDEODEVS dynamic (the value would be
>>>>> passed through platform data). The code will be removed (hopefully
>> soon)
>>>>> anyway when the legacy video nodes will disappear.
>>>>
>>>> Ok, so should I keep this patch only to myself until this code is
>> removed?
>>>
>>> I'll let Sakari answer that, but I think they can still go in in the
>> meantime.
>>
>> Is there a need for the patch? The other possible device is just left
>> unused, right?
> 
> There is need for it _if_ I don't change OMAP34XXCAM_VIDEODEVS in
> drivers/media/video/omap34xxcam.h, and if I have less devices listed
> in the platform data passed from the boardfile.
> 
> In this case, OMAP34XXCAM_VIDEODEVS is hardcoded to N900 case, which is 2,
> and I only have 1 sensor in my Zoom3.
> 
> I guess the patch is just protecting for potential pointer dereferencing, unless we get rid of current OMAP34XXCAM_VIDEODEVS hardcoded value.

What exactly does not work?

The video devices are registered dynamically based on the number of
sensors available so in this case the second video device is not even
registered.

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

RE: [omap3camera] Camera bring-up on Zoom3 (OMAP3630)

[Aguirre, Sergio]

> -----Original Message-----
> From: Sakari Ailus [mailto:sakari.ailus@maxwell.research.nokia.com]
> Sent: Wednesday, March 17, 2010 9:59 AM
> To: Aguirre, Sergio
> Cc: Laurent Pinchart; linux-media@vger.kernel.org
> Subject: Re: [omap3camera] Camera bring-up on Zoom3 (OMAP3630)
> 
> Aguirre, Sergio wrote:
> >
> >
> >> -----Original Message-----
> >> From: Sakari Ailus [mailto:sakari.ailus@maxwell.research.nokia.com]
> >> Sent: Wednesday, March 17, 2010 9:16 AM
> >> To: Laurent Pinchart
> >> Cc: Aguirre, Sergio; linux-media@vger.kernel.org
> >> Subject: Re: [omap3camera] Camera bring-up on Zoom3 (OMAP3630)
> >>
> >> Laurent Pinchart wrote:
> >>>>>> I'm trying to get latest Sakari's tree (gitorious.org/omap3camera)
> >>>>>> 'devel' branch running on my Zoom3 HW (which has an OMAP3630, and a
> >>>>>> Sony IMX046 8MP sensor).
> >>>>>>
> >>>>>> I had first one NULL pointer dereference while the driver was
> >>>>>> registering devices and creating entities, which I resolved with
> >>>>>> the attached patch. (Is this patch acceptable, or maybe I am
> missing
> >>>>>> something...)
> >>>>>
> >>>>> Either that, or make OMAP34XXCAM_VIDEODEVS dynamic (the value would
> be
> >>>>> passed through platform data). The code will be removed (hopefully
> >> soon)
> >>>>> anyway when the legacy video nodes will disappear.
> >>>>
> >>>> Ok, so should I keep this patch only to myself until this code is
> >> removed?
> >>>
> >>> I'll let Sakari answer that, but I think they can still go in in the
> >> meantime.
> >>
> >> Is there a need for the patch? The other possible device is just left
> >> unused, right?
> >
> > There is need for it _if_ I don't change OMAP34XXCAM_VIDEODEVS in
> > drivers/media/video/omap34xxcam.h, and if I have less devices listed
> > in the platform data passed from the boardfile.
> >
> > In this case, OMAP34XXCAM_VIDEODEVS is hardcoded to N900 case, which is
> 2,
> > and I only have 1 sensor in my Zoom3.
> >
> > I guess the patch is just protecting for potential pointer
> dereferencing, unless we get rid of current OMAP34XXCAM_VIDEODEVS
> hardcoded value.
> 
> What exactly does not work?

Ok, let me explain:

In my boardfile, I have:

static struct omap34xxcam_platform_data zoom_camera_pdata = {
	.isp			= &omap3isp_device,
	.subdevs[0] = zoom_camera_primary_subdevs,
	.sensors[0] = {
		.capture_mem = IMX046_BIGGEST_FRAME_BYTE_SIZE * 2,
		.ival_default	= { 1, 10 },
	},
};

As I only have 1 sensor.

However, when omap34xxcam_probe runs, it loops through pdata->subdevs[i], in which 'i' goes from 0 to OMAP34XXCAM_VIDEODEVS - 1.

And this is what generates an "oops" message in the driver.

Regards,
Sergio

> 
> The video devices are registered dynamically based on the number of
> sensors available so in this case the second video device is not even
> registered.
> 
> --
> Sakari Ailus
> sakari.ailus@maxwell.research.nokia.com

Re: [omap3camera] Camera bring-up on Zoom3 (OMAP3630)

[Sakari Ailus]

Aguirre, Sergio wrote:
>> What exactly does not work?
> 
> Ok, let me explain:
> 
> In my boardfile, I have:
> 
> static struct omap34xxcam_platform_data zoom_camera_pdata = {
> 	.isp			= &omap3isp_device,
> 	.subdevs[0] = zoom_camera_primary_subdevs,
> 	.sensors[0] = {
> 		.capture_mem = IMX046_BIGGEST_FRAME_BYTE_SIZE * 2,
> 		.ival_default	= { 1, 10 },
> 	},
> };
> 
> As I only have 1 sensor.
> 
> However, when omap34xxcam_probe runs, it loops through pdata->subdevs[i], in which 'i' goes from 0 to OMAP34XXCAM_VIDEODEVS - 1.
> 
> And this is what generates an "oops" message in the driver.

Ok, now I get it. Thanks. :-)

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com