Re: About CITOR register value for pxa_camera

Re: About CITOR register value for pxa_camera

[fpantaleao]

On Fri, 7 Nov 2008, g.liakhovetski@gmx.de wrote:

> On Fri, 7 Nov 2008, fpantaleao@mobisensesystems.com wrote:
>
> > To be more general (sensor clocked by MCLK or not), I think we  
> should request
> > the sensor its PCLK, by adding a get_pclk member in soc_camera_ops for
> > example.
>
> Yes, that should be even better, and pass master-clock frequency as  
> a parameter? What shall we take for default? pclk == mclk or some  
> safe conservative value? Don't think requiring every camera driver  
> to provide this method would be very good.

MCLK as a parameter is fine.
For the default, I suggest:
   PCLK = MCLK when MCLK is enabled,
   13MHz otherwise which is a low value for PCLK.

I agree we must keep things simple even if a correct value for CITOR  
really improves acquisition performance.

>
> Another question, are there situations, when a sensor might decide  
> to change its pixelclock dynamically? Then it might need to inform  
> the host driver about the change?

PCLK changes dynamically with sensor resolution, subsampling, pixel  
format and so on. Every time a format change occurs, CITOR could be  
updated in 2 steps:
1. PCLK value should be updated by the camera driver.
The computation is based on the sensor input clock (provided by  
platform data) and current settings.
To make things more simple, the camera driver can use a constant equal  
to the lowest possible value.
2. PCLK value should be read to compute the new CITOR value.

Best regards.

Florian


----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.




--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: About CITOR register value for pxa_camera

[Guennadi Liakhovetski]

I finally got round to trying this...

On Fri, 7 Nov 2008, fpantaleao@mobisensesystems.com wrote:

> On Fri, 7 Nov 2008, g.liakhovetski@gmx.de wrote:
> 
> > On Fri, 7 Nov 2008, fpantaleao@mobisensesystems.com wrote:
> > 
> > > To be more general (sensor clocked by MCLK or not), I think we should
> > > request
> > > the sensor its PCLK, by adding a get_pclk member in soc_camera_ops for
> > > example.
> > 
> > Yes, that should be even better, and pass master-clock frequency as a
> > parameter? What shall we take for default? pclk == mclk or some safe
> > conservative value? Don't think requiring every camera driver to provide
> > this method would be very good.
> 
> MCLK as a parameter is fine.
> For the default, I suggest:
>  PCLK = MCLK when MCLK is enabled,
>  13MHz otherwise which is a low value for PCLK.
> 
> I agree we must keep things simple even if a correct value for CITOR really
> improves acquisition performance.

Please, have a look at this patch. I decided against another function call 
for a number of reasons: first, if the host calls into the camera to ask 
whether the frequency has changed, it is not easy to recognise, whether it 
changed _now_, if you let camera notify the host about frequency change 
this breaks the hierarchy. Second, you don't know how many more similar 
parameters will have to be communicated between the camera and the host. 
So, I decided to add an extensible sense struct.

Only compile tested so far, will run-test later, maybe tomorrow (actually, 
already today). Comments welcome, tests even more so:-)

> > Another question, are there situations, when a sensor might decide to change
> > its pixelclock dynamically? Then it might need to inform the host driver
> > about the change?
> 
> PCLK changes dynamically with sensor resolution, subsampling, pixel format and
> so on. Every time a format change occurs, CITOR could be updated in 2 steps:
> 1. PCLK value should be updated by the camera driver.
> The computation is based on the sensor input clock (provided by platform data)
> and current settings.
> To make things more simple, the camera driver can use a constant equal to the
> lowest possible value.
> 2. PCLK value should be read to compute the new CITOR value.

Patch below. Loosely based on top of our current format-negotiation 
stack...

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
index 660ffd4..a437d6e 100644
--- a/drivers/media/video/pxa_camera.c
+++ b/drivers/media/video/pxa_camera.c
@@ -120,7 +120,9 @@ struct pxa_camera_dev {
 	struct pxacamera_platform_data *pdata;
 	struct resource		*res;
 	unsigned long		platform_flags;
-	unsigned long		platform_mclk_10khz;
+	unsigned long		ciclk;
+	unsigned long		mclk;
+	u32			mclk_divisor;
 
 	struct list_head	capture;
 
@@ -598,24 +600,43 @@ static void pxa_camera_init_videobuf(struct videobuf_queue *q,
 				sizeof(struct pxa_buffer), icd);
 }
 
-static int mclk_get_divisor(struct pxa_camera_dev *pcdev)
+static u32 mclk_get_divisor(struct pxa_camera_dev *pcdev)
 {
-	unsigned int mclk_10khz = pcdev->platform_mclk_10khz;
-	unsigned long div;
+	unsigned long mclk = pcdev->mclk;
+	u32 div;
 	unsigned long lcdclk;
 
-	lcdclk = clk_get_rate(pcdev->clk) / 10000;
+	lcdclk = clk_get_rate(pcdev->clk);
+	pcdev->ciclk = lcdclk;
 
-	/* We verify platform_mclk_10khz != 0, so if anyone breaks it, here
-	 * they get a nice Oops */
-	div = (lcdclk + 2 * mclk_10khz - 1) / (2 * mclk_10khz) - 1;
+	/* mclk <= ciclk / 4 (27.4.2) */
+	if (mclk > lcdclk / 4) {
+		mclk = lcdclk / 4;
+		dev_warn(pcdev->dev, "Limiting master clock to %lu\n", mclk);
+	}
+
+	/* We verify mclk != 0, so if anyone breaks it, here comes their Oops */
+	div = (lcdclk + 2 * mclk - 1) / (2 * mclk) - 1;
+
+	/* If we're not supplying MCLK, leave it at 0 */
+	if (pcdev->platform_flags & PXA_CAMERA_MCLK_EN)
+		pcdev->mclk = lcdclk / (2 * (div - 1));
 
-	dev_dbg(pcdev->dev, "LCD clock %lukHz, target freq %dkHz, "
-		"divisor %lu\n", lcdclk * 10, mclk_10khz * 10, div);
+	dev_dbg(pcdev->dev, "LCD clock %luHz, target freq %dHz, "
+		"divisor %lu\n", lcdclk, mclk, div);
 
 	return div;
 }
 
+static void recalculate_fifo_timeout(struct pxa_camera_dev *pcdev,
+				     unsigned long pclk)
+{
+	/* We want a timeout > 1 pixel time, not ">=" */
+	u32 ciclk_per_pixel = pcdev->ciclk / pclk + 1;
+
+	CITOR = ciclk_per_pixel;
+}
+
 static void pxa_camera_activate(struct pxa_camera_dev *pcdev)
 {
 	struct pxacamera_platform_data *pdata = pcdev->pdata;
@@ -642,7 +663,14 @@ static void pxa_camera_activate(struct pxa_camera_dev *pcdev)
 	if (pcdev->platform_flags & PXA_CAMERA_VSP)
 		cicr4 |= CICR4_VSP;
 
-	CICR4 = mclk_get_divisor(pcdev) | cicr4;
+	CICR4 = pcdev->mclk_divisor | cicr4;
+
+	if (pcdev->platform_flags & PXA_CAMERA_MCLK_EN)
+		/* Initialise the timeout under the assumption pclk = mclk */
+		recalculate_fifo_timeout(pcdev, pcdev->mclk);
+	else
+		/* "Safe default" - 13MHz */
+		recalculate_fifo_timeout(pcdev, 13000000);
 
 	clk_enable(pcdev->clk);
 }
@@ -888,7 +916,7 @@ static int pxa_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
 	CICR2 = 0;
 	CICR3 = CICR3_LPF_VAL(icd->height - 1) |
 		CICR3_BFW_VAL(min((unsigned short)255, icd->y_skip_top));
-	CICR4 = mclk_get_divisor(pcdev) | cicr4;
+	CICR4 = pcdev->mclk_divisor | cicr4;
 
 	/* CIF interrupts are not used, only DMA */
 	CICR0 = (pcdev->platform_flags & PXA_CAMERA_MASTER ?
@@ -901,8 +929,7 @@ static int pxa_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
 static int pxa_camera_try_bus_param(struct soc_camera_device *icd,
 				    unsigned char buswidth)
 {
-	struct soc_camera_host *ici =
-		to_soc_camera_host(icd->dev.parent);
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
 	struct pxa_camera_dev *pcdev = ici->priv;
 	unsigned long bus_flags, camera_flags;
 	int ret = test_platform_param(pcdev, buswidth, &bus_flags);
@@ -1014,13 +1041,17 @@ static int pxa_camera_get_formats(struct soc_camera_device *icd, int idx,
 	return formats;
 }
 
-
 static int pxa_camera_set_fmt(struct soc_camera_device *icd,
 			      __u32 pixfmt, struct v4l2_rect *rect)
 {
 	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+	struct pxa_camera_dev *pcdev = ici->priv;
 	const struct soc_camera_data_format *host_fmt, *cam_fmt = NULL;
 	const struct soc_camera_format_xlate *xlate;
+	struct soc_camera_sense sense = {
+		.master_clock = pcdev->mclk,
+		.pixel_clock_max = pcdev->ciclk / 4,
+	};
 	int ret, buswidth;
 
 	xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
@@ -1033,6 +1064,10 @@ static int pxa_camera_set_fmt(struct soc_camera_device *icd,
 	host_fmt = xlate->host_fmt;
 	cam_fmt = xlate->cam_fmt;
 
+	/* If PCLK is used to latch data from the sensor, check sense */
+	if (pcdev->platform_flags & PXA_CAMERA_PCLK_EN)
+		icd->sense = &sense;
+
 	switch (pixfmt) {
 	case 0:				/* Only geometry change */
 		ret = icd->ops->set_fmt(icd, pixfmt, rect);
@@ -1041,9 +1076,20 @@ static int pxa_camera_set_fmt(struct soc_camera_device *icd,
 		ret = icd->ops->set_fmt(icd, cam_fmt->fourcc, rect);
 	}
 
-	if (ret < 0)
+	icd->sense = NULL;
+
+	if (ret < 0) {
 		dev_warn(&ici->dev, "Failed to configure for format %x\n",
 			 pixfmt);
+	} else if (sense.flags & SOCAM_SENSE_PCLK_CHANGED) {
+		if (sense.pixel_clock > sense.pixel_clock_max) {
+			dev_err(&ici->dev,
+				"pixel clock %lu set by the camera too high!",
+				sense.pixel_clock);
+			return -EIO;
+		}
+		recalculate_fifo_timeout(pcdev, sense.pixel_clock);
+	}
 
 	if (pixfmt && !ret) {
 		icd->buswidth = buswidth;
@@ -1248,14 +1294,16 @@ static int pxa_camera_probe(struct platform_device *pdev)
 			 "data widths, using default 10 bit\n");
 		pcdev->platform_flags |= PXA_CAMERA_DATAWIDTH_10;
 	}
-	pcdev->platform_mclk_10khz = pcdev->pdata->mclk_10khz;
-	if (!pcdev->platform_mclk_10khz) {
+	pcdev->mclk = pcdev->pdata->mclk_10khz * 10000;
+	if (!pcdev->mclk) {
 		dev_warn(&pdev->dev,
-			 "mclk_10khz == 0! Please, fix your platform data. "
+			 "mclk == 0! Please, fix your platform data. "
 			 "Using default 20MHz\n");
-		pcdev->platform_mclk_10khz = 2000;
+		pcdev->mclk = 20000000;
 	}
 
+	pcdev->mclk_divisor = mclk_get_divisor(pcdev);
+
 	INIT_LIST_HEAD(&pcdev->capture);
 	spin_lock_init(&pcdev->lock);
 
diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
index b1126b1..cf9bf8c 100644
--- a/include/media/soc_camera.h
+++ b/include/media/soc_camera.h
@@ -36,6 +36,7 @@ struct soc_camera_device {
 	unsigned char iface;		/* Host number */
 	unsigned char devnum;		/* Device number per host */
 	unsigned char buswidth;		/* See comment in .c */
+	struct soc_camera_sense *sense;	/* See comment in struct definition */
 	struct soc_camera_ops *ops;
 	struct video_device *vdev;
 	const struct soc_camera_data_format *current_fmt;
@@ -162,6 +163,32 @@ struct soc_camera_ops {
 	int num_controls;
 };
 
+#define SOCAM_SENSE_PCLK_CHANGED	(1 << 0)
+
+/**
+ * This struct can be attached to struct soc_camera_device by the host driver
+ * to request sense from the camera, for example, when calling .set_fmt(). The
+ * host then can check which flags are set and verify respective values if any.
+ * For example, if SOCAM_SENSE_PCLK_CHANGED is set, it means, pixclock has
+ * changed during this operation. After completion the host should detach sense.
+ *
+ * @flags		ored SOCAM_SENSE_* flags
+ * @master_clock	if the host wants to be informed about pixel-clock
+ *			change, it better set master_clock.
+ * @pixel_clock_max	maximum pixel clock frequency supported by the host,
+ *			camera is not allowed to exceed this.
+ * @pixel_clock		if the camera driver changed pixel clock during this
+ *			operation, it sets SOCAM_SENSE_PCLK_CHANGED, uses
+ *			master_clock to calculate the new pixel-clock and
+ *			sets it.
+ */
+struct soc_camera_sense {
+	unsigned long flags;
+	unsigned long master_clock;
+	unsigned long pixel_clock_max;
+	unsigned long pixel_clock;
+};
+
 static inline struct v4l2_queryctrl const *soc_camera_find_qctrl(
 	struct soc_camera_ops *ops, int id)
 {

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: About CITOR register value for pxa_camera

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> I finally got round to trying this...
>
> Please, have a look at this patch. I decided against another function call 
> for a number of reasons: first, if the host calls into the camera to ask 
> whether the frequency has changed, it is not easy to recognise, whether it 
> changed _now_, if you let camera notify the host about frequency change 
> this breaks the hierarchy. Second, you don't know how many more similar 
> parameters will have to be communicated between the camera and the host. 
> So, I decided to add an extensible sense struct.
>
> Only compile tested so far, will run-test later, maybe tomorrow (actually, 
> already today). Comments welcome, tests even more so:-)
The first test crashes in the pxa_camera_probe() for me, something like :
[  247.554669] [<c014f2f8>] (dev_driver_string+0x0/0x48) from [<bf01eac8>] (pxa_camera_probe+0x2c8/0x424 [pxa_camera])
[  247.564907] [<bf01e800>] (pxa_camera_probe+0x0/0x424 [pxa_camera]) from [<c01538fc>] (platform_drv_probe+0x20/0x24)
[  247.575129] [<c01538dc>] (platform_drv_probe+0x0/0x24) from [<c0152a3c>] (driver_probe_device+0xac/0x1b0)
[  247.585308] [<c0152990>] (driver_probe_device+0x0/0x1b0) from [<c0152bd0>] (__driver_attach+0x90/0x94)

I'll take some time tomorrow, to review and test.

> Patch below. Loosely based on top of our current format-negotiation 
> stack...
Applies nicely, thanks.

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: About CITOR register value for pxa_camera

[Guennadi Liakhovetski]

On Sun, 30 Nov 2008, Robert Jarzmik wrote:

> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> 
> > I finally got round to trying this...
> >
> > Please, have a look at this patch. I decided against another function call 
> > for a number of reasons: first, if the host calls into the camera to ask 
> > whether the frequency has changed, it is not easy to recognise, whether it 
> > changed _now_, if you let camera notify the host about frequency change 
> > this breaks the hierarchy. Second, you don't know how many more similar 
> > parameters will have to be communicated between the camera and the host. 
> > So, I decided to add an extensible sense struct.
> >
> > Only compile tested so far, will run-test later, maybe tomorrow (actually, 
> > already today). Comments welcome, tests even more so:-)
> The first test crashes in the pxa_camera_probe() for me, something like :
> [  247.554669] [<c014f2f8>] (dev_driver_string+0x0/0x48) from [<bf01eac8>] (pxa_camera_probe+0x2c8/0x424 [pxa_camera])
> [  247.564907] [<bf01e800>] (pxa_camera_probe+0x0/0x424 [pxa_camera]) from [<c01538fc>] (platform_drv_probe+0x20/0x24)
> [  247.575129] [<c01538dc>] (platform_drv_probe+0x0/0x24) from [<c0152a3c>] (driver_probe_device+0xac/0x1b0)
> [  247.585308] [<c0152990>] (driver_probe_device+0x0/0x1b0) from [<c0152bd0>] (__driver_attach+0x90/0x94)
> 
> I'll take some time tomorrow, to review and test.

Ok, that was an easy one. A (hopefully) fixed version below. But - again - 
untested, so, you might just wait until I test it (tomorrow).

Thanks
Guennadi
---
diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
index 8219a6c..0d4513b 100644
--- a/drivers/media/video/pxa_camera.c
+++ b/drivers/media/video/pxa_camera.c
@@ -120,7 +120,9 @@ struct pxa_camera_dev {
 	struct pxacamera_platform_data *pdata;
 	struct resource		*res;
 	unsigned long		platform_flags;
-	unsigned long		platform_mclk_10khz;
+	unsigned long		ciclk;
+	unsigned long		mclk;
+	u32			mclk_divisor;
 
 	struct list_head	capture;
 
@@ -598,24 +600,43 @@ static void pxa_camera_init_videobuf(struct videobuf_queue *q,
 				sizeof(struct pxa_buffer), icd);
 }
 
-static int mclk_get_divisor(struct pxa_camera_dev *pcdev)
+static u32 mclk_get_divisor(struct pxa_camera_dev *pcdev)
 {
-	unsigned int mclk_10khz = pcdev->platform_mclk_10khz;
-	unsigned long div;
+	unsigned long mclk = pcdev->mclk;
+	u32 div;
 	unsigned long lcdclk;
 
-	lcdclk = clk_get_rate(pcdev->clk) / 10000;
+	lcdclk = clk_get_rate(pcdev->clk);
+	pcdev->ciclk = lcdclk;
 
-	/* We verify platform_mclk_10khz != 0, so if anyone breaks it, here
-	 * they get a nice Oops */
-	div = (lcdclk + 2 * mclk_10khz - 1) / (2 * mclk_10khz) - 1;
+	/* mclk <= ciclk / 4 (27.4.2) */
+	if (mclk > lcdclk / 4) {
+		mclk = lcdclk / 4;
+		dev_warn(pcdev->dev, "Limiting master clock to %lu\n", mclk);
+	}
+
+	/* We verify mclk != 0, so if anyone breaks it, here comes their Oops */
+	div = (lcdclk + 2 * mclk - 1) / (2 * mclk) - 1;
+
+	/* If we're not supplying MCLK, leave it at 0 */
+	if (pcdev->platform_flags & PXA_CAMERA_MCLK_EN)
+		pcdev->mclk = lcdclk / (2 * (div - 1));
 
-	dev_dbg(pcdev->dev, "LCD clock %lukHz, target freq %dkHz, "
-		"divisor %lu\n", lcdclk * 10, mclk_10khz * 10, div);
+	dev_dbg(pcdev->dev, "LCD clock %luHz, target freq %dHz, "
+		"divisor %lu\n", lcdclk, mclk, div);
 
 	return div;
 }
 
+static void recalculate_fifo_timeout(struct pxa_camera_dev *pcdev,
+				     unsigned long pclk)
+{
+	/* We want a timeout > 1 pixel time, not ">=" */
+	u32 ciclk_per_pixel = pcdev->ciclk / pclk + 1;
+
+	CITOR = ciclk_per_pixel;
+}
+
 static void pxa_camera_activate(struct pxa_camera_dev *pcdev)
 {
 	struct pxacamera_platform_data *pdata = pcdev->pdata;
@@ -642,7 +663,14 @@ static void pxa_camera_activate(struct pxa_camera_dev *pcdev)
 	if (pcdev->platform_flags & PXA_CAMERA_VSP)
 		cicr4 |= CICR4_VSP;
 
-	CICR4 = mclk_get_divisor(pcdev) | cicr4;
+	CICR4 = pcdev->mclk_divisor | cicr4;
+
+	if (pcdev->platform_flags & PXA_CAMERA_MCLK_EN)
+		/* Initialise the timeout under the assumption pclk = mclk */
+		recalculate_fifo_timeout(pcdev, pcdev->mclk);
+	else
+		/* "Safe default" - 13MHz */
+		recalculate_fifo_timeout(pcdev, 13000000);
 
 	clk_enable(pcdev->clk);
 }
@@ -888,7 +916,7 @@ static int pxa_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
 	CICR2 = 0;
 	CICR3 = CICR3_LPF_VAL(icd->height - 1) |
 		CICR3_BFW_VAL(min((unsigned short)255, icd->y_skip_top));
-	CICR4 = mclk_get_divisor(pcdev) | cicr4;
+	CICR4 = pcdev->mclk_divisor | cicr4;
 
 	/* CIF interrupts are not used, only DMA */
 	CICR0 = (pcdev->platform_flags & PXA_CAMERA_MASTER ?
@@ -901,8 +929,7 @@ static int pxa_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
 static int pxa_camera_try_bus_param(struct soc_camera_device *icd,
 				    unsigned char buswidth)
 {
-	struct soc_camera_host *ici =
-		to_soc_camera_host(icd->dev.parent);
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
 	struct pxa_camera_dev *pcdev = ici->priv;
 	unsigned long bus_flags, camera_flags;
 	int ret = test_platform_param(pcdev, buswidth, &bus_flags);
@@ -1018,8 +1045,13 @@ static int pxa_camera_set_fmt(struct soc_camera_device *icd,
 			      __u32 pixfmt, struct v4l2_rect *rect)
 {
 	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+	struct pxa_camera_dev *pcdev = ici->priv;
 	const struct soc_camera_data_format *host_fmt, *cam_fmt = NULL;
 	const struct soc_camera_format_xlate *xlate;
+	struct soc_camera_sense sense = {
+		.master_clock = pcdev->mclk,
+		.pixel_clock_max = pcdev->ciclk / 4,
+	};
 	int ret, buswidth;
 
 	xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
@@ -1032,6 +1064,10 @@ static int pxa_camera_set_fmt(struct soc_camera_device *icd,
 	host_fmt = xlate->host_fmt;
 	cam_fmt = xlate->cam_fmt;
 
+	/* If PCLK is used to latch data from the sensor, check sense */
+	if (pcdev->platform_flags & PXA_CAMERA_PCLK_EN)
+		icd->sense = &sense;
+
 	switch (pixfmt) {
 	case 0:				/* Only geometry change */
 		ret = icd->ops->set_fmt(icd, pixfmt, rect);
@@ -1040,9 +1076,20 @@ static int pxa_camera_set_fmt(struct soc_camera_device *icd,
 		ret = icd->ops->set_fmt(icd, cam_fmt->fourcc, rect);
 	}
 
-	if (ret < 0)
+	icd->sense = NULL;
+
+	if (ret < 0) {
 		dev_warn(&ici->dev, "Failed to configure for format %x\n",
 			 pixfmt);
+	} else if (sense.flags & SOCAM_SENSE_PCLK_CHANGED) {
+		if (sense.pixel_clock > sense.pixel_clock_max) {
+			dev_err(&ici->dev,
+				"pixel clock %lu set by the camera too high!",
+				sense.pixel_clock);
+			return -EIO;
+		}
+		recalculate_fifo_timeout(pcdev, sense.pixel_clock);
+	}
 
 	if (pixfmt && !ret) {
 		icd->buswidth = buswidth;
@@ -1247,14 +1294,17 @@ static int pxa_camera_probe(struct platform_device *pdev)
 			 "data widths, using default 10 bit\n");
 		pcdev->platform_flags |= PXA_CAMERA_DATAWIDTH_10;
 	}
-	pcdev->platform_mclk_10khz = pcdev->pdata->mclk_10khz;
-	if (!pcdev->platform_mclk_10khz) {
+	pcdev->mclk = pcdev->pdata->mclk_10khz * 10000;
+	if (!pcdev->mclk) {
 		dev_warn(&pdev->dev,
-			 "mclk_10khz == 0! Please, fix your platform data. "
+			 "mclk == 0! Please, fix your platform data. "
 			 "Using default 20MHz\n");
-		pcdev->platform_mclk_10khz = 2000;
+		pcdev->mclk = 20000000;
 	}
 
+	pcdev->dev = &pdev->dev;
+	pcdev->mclk_divisor = mclk_get_divisor(pcdev);
+
 	INIT_LIST_HEAD(&pcdev->capture);
 	spin_lock_init(&pcdev->lock);
 
@@ -1274,7 +1324,6 @@ static int pxa_camera_probe(struct platform_device *pdev)
 	}
 	pcdev->irq = irq;
 	pcdev->base = base;
-	pcdev->dev = &pdev->dev;
 
 	/* request dma */
 	err = pxa_request_dma("CI_Y", DMA_PRIO_HIGH,
diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
index da57ffd..7832d97 100644
--- a/include/media/soc_camera.h
+++ b/include/media/soc_camera.h
@@ -36,6 +36,7 @@ struct soc_camera_device {
 	unsigned char iface;		/* Host number */
 	unsigned char devnum;		/* Device number per host */
 	unsigned char buswidth;		/* See comment in .c */
+	struct soc_camera_sense *sense;	/* See comment in struct definition */
 	struct soc_camera_ops *ops;
 	struct video_device *vdev;
 	const struct soc_camera_data_format *current_fmt;
@@ -164,6 +165,32 @@ struct soc_camera_ops {
 	int num_controls;
 };
 
+#define SOCAM_SENSE_PCLK_CHANGED	(1 << 0)
+
+/**
+ * This struct can be attached to struct soc_camera_device by the host driver
+ * to request sense from the camera, for example, when calling .set_fmt(). The
+ * host then can check which flags are set and verify respective values if any.
+ * For example, if SOCAM_SENSE_PCLK_CHANGED is set, it means, pixclock has
+ * changed during this operation. After completion the host should detach sense.
+ *
+ * @flags		ored SOCAM_SENSE_* flags
+ * @master_clock	if the host wants to be informed about pixel-clock
+ *			change, it better set master_clock.
+ * @pixel_clock_max	maximum pixel clock frequency supported by the host,
+ *			camera is not allowed to exceed this.
+ * @pixel_clock		if the camera driver changed pixel clock during this
+ *			operation, it sets SOCAM_SENSE_PCLK_CHANGED, uses
+ *			master_clock to calculate the new pixel-clock and
+ *			sets it.
+ */
+struct soc_camera_sense {
+	unsigned long flags;
+	unsigned long master_clock;
+	unsigned long pixel_clock_max;
+	unsigned long pixel_clock;
+};
+
 static inline struct v4l2_queryctrl const *soc_camera_find_qctrl(
 	struct soc_camera_ops *ops, int id)
 {

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

[PATCH 1/2] soc-camera: add camera sense data

[Guennadi Liakhovetski]

Add a struct soc_camera_sense, that can be used by camera host drivers to
request additional information from a camera driver, for example, when
changing data format. This struct can be extended in the future, its first use
is to request the camera driver whether the pixel-clock frequency has changed.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 include/media/soc_camera.h |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/include/media/soc_camera.h b/include/media/soc_camera.h
index da57ffd..7832d97 100644
--- a/include/media/soc_camera.h
+++ b/include/media/soc_camera.h
@@ -36,6 +36,7 @@ struct soc_camera_device {
 	unsigned char iface;		/* Host number */
 	unsigned char devnum;		/* Device number per host */
 	unsigned char buswidth;		/* See comment in .c */
+	struct soc_camera_sense *sense;	/* See comment in struct definition */
 	struct soc_camera_ops *ops;
 	struct video_device *vdev;
 	const struct soc_camera_data_format *current_fmt;
@@ -164,6 +165,32 @@ struct soc_camera_ops {
 	int num_controls;
 };
 
+#define SOCAM_SENSE_PCLK_CHANGED	(1 << 0)
+
+/**
+ * This struct can be attached to struct soc_camera_device by the host driver
+ * to request sense from the camera, for example, when calling .set_fmt(). The
+ * host then can check which flags are set and verify respective values if any.
+ * For example, if SOCAM_SENSE_PCLK_CHANGED is set, it means, pixclock has
+ * changed during this operation. After completion the host should detach sense.
+ *
+ * @flags		ored SOCAM_SENSE_* flags
+ * @master_clock	if the host wants to be informed about pixel-clock
+ *			change, it better set master_clock.
+ * @pixel_clock_max	maximum pixel clock frequency supported by the host,
+ *			camera is not allowed to exceed this.
+ * @pixel_clock		if the camera driver changed pixel clock during this
+ *			operation, it sets SOCAM_SENSE_PCLK_CHANGED, uses
+ *			master_clock to calculate the new pixel-clock and
+ *			sets it.
+ */
+struct soc_camera_sense {
+	unsigned long flags;
+	unsigned long master_clock;
+	unsigned long pixel_clock_max;
+	unsigned long pixel_clock;
+};
+
 static inline struct v4l2_queryctrl const *soc_camera_find_qctrl(
 	struct soc_camera_ops *ops, int id)
 {
-- 
1.5.4

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

[PATCH 2/2] pxa-camera: setup the FIFO inactivity time-out register

[Guennadi Liakhovetski]

Using PXA270's FIFO inactivity time-out register (CITOR) reduces FIFO overruns.
The time-out is calculated in CICLK / LCDCLK ticks and has to be longer than
one pixel time. For this we have to know the pixel clock frequency, which
usually is provided by the camera. We use the struct soc_camera_sense to
request PCLK frequency from the camera driver upon each data format change.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---
 drivers/media/video/pxa_camera.c |   89 +++++++++++++++++++++++++++++--------
 1 files changed, 69 insertions(+), 20 deletions(-)

diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
index 8219a6c..e1fff1c 100644
--- a/drivers/media/video/pxa_camera.c
+++ b/drivers/media/video/pxa_camera.c
@@ -120,7 +120,9 @@ struct pxa_camera_dev {
 	struct pxacamera_platform_data *pdata;
 	struct resource		*res;
 	unsigned long		platform_flags;
-	unsigned long		platform_mclk_10khz;
+	unsigned long		ciclk;
+	unsigned long		mclk;
+	u32			mclk_divisor;
 
 	struct list_head	capture;
 
@@ -598,24 +600,43 @@ static void pxa_camera_init_videobuf(struct videobuf_queue *q,
 				sizeof(struct pxa_buffer), icd);
 }
 
-static int mclk_get_divisor(struct pxa_camera_dev *pcdev)
+static u32 mclk_get_divisor(struct pxa_camera_dev *pcdev)
 {
-	unsigned int mclk_10khz = pcdev->platform_mclk_10khz;
-	unsigned long div;
+	unsigned long mclk = pcdev->mclk;
+	u32 div;
 	unsigned long lcdclk;
 
-	lcdclk = clk_get_rate(pcdev->clk) / 10000;
+	lcdclk = clk_get_rate(pcdev->clk);
+	pcdev->ciclk = lcdclk;
 
-	/* We verify platform_mclk_10khz != 0, so if anyone breaks it, here
-	 * they get a nice Oops */
-	div = (lcdclk + 2 * mclk_10khz - 1) / (2 * mclk_10khz) - 1;
+	/* mclk <= ciclk / 4 (27.4.2) */
+	if (mclk > lcdclk / 4) {
+		mclk = lcdclk / 4;
+		dev_warn(pcdev->dev, "Limiting master clock to %lu\n", mclk);
+	}
+
+	/* We verify mclk != 0, so if anyone breaks it, here comes their Oops */
+	div = (lcdclk + 2 * mclk - 1) / (2 * mclk) - 1;
+
+	/* If we're not supplying MCLK, leave it at 0 */
+	if (pcdev->platform_flags & PXA_CAMERA_MCLK_EN)
+		pcdev->mclk = lcdclk / (2 * (div + 1));
 
-	dev_dbg(pcdev->dev, "LCD clock %lukHz, target freq %dkHz, "
-		"divisor %lu\n", lcdclk * 10, mclk_10khz * 10, div);
+	dev_dbg(pcdev->dev, "LCD clock %luHz, target freq %luHz, "
+		"divisor %u\n", lcdclk, mclk, div);
 
 	return div;
 }
 
+static void recalculate_fifo_timeout(struct pxa_camera_dev *pcdev,
+				     unsigned long pclk)
+{
+	/* We want a timeout > 1 pixel time, not ">=" */
+	u32 ciclk_per_pixel = pcdev->ciclk / pclk + 1;
+
+	CITOR = ciclk_per_pixel;
+}
+
 static void pxa_camera_activate(struct pxa_camera_dev *pcdev)
 {
 	struct pxacamera_platform_data *pdata = pcdev->pdata;
@@ -642,7 +663,14 @@ static void pxa_camera_activate(struct pxa_camera_dev *pcdev)
 	if (pcdev->platform_flags & PXA_CAMERA_VSP)
 		cicr4 |= CICR4_VSP;
 
-	CICR4 = mclk_get_divisor(pcdev) | cicr4;
+	CICR4 = pcdev->mclk_divisor | cicr4;
+
+	if (pcdev->platform_flags & PXA_CAMERA_MCLK_EN)
+		/* Initialise the timeout under the assumption pclk = mclk */
+		recalculate_fifo_timeout(pcdev, pcdev->mclk);
+	else
+		/* "Safe default" - 13MHz */
+		recalculate_fifo_timeout(pcdev, 13000000);
 
 	clk_enable(pcdev->clk);
 }
@@ -888,7 +916,7 @@ static int pxa_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
 	CICR2 = 0;
 	CICR3 = CICR3_LPF_VAL(icd->height - 1) |
 		CICR3_BFW_VAL(min((unsigned short)255, icd->y_skip_top));
-	CICR4 = mclk_get_divisor(pcdev) | cicr4;
+	CICR4 = pcdev->mclk_divisor | cicr4;
 
 	/* CIF interrupts are not used, only DMA */
 	CICR0 = (pcdev->platform_flags & PXA_CAMERA_MASTER ?
@@ -901,8 +929,7 @@ static int pxa_camera_set_bus_param(struct soc_camera_device *icd, __u32 pixfmt)
 static int pxa_camera_try_bus_param(struct soc_camera_device *icd,
 				    unsigned char buswidth)
 {
-	struct soc_camera_host *ici =
-		to_soc_camera_host(icd->dev.parent);
+	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
 	struct pxa_camera_dev *pcdev = ici->priv;
 	unsigned long bus_flags, camera_flags;
 	int ret = test_platform_param(pcdev, buswidth, &bus_flags);
@@ -1018,8 +1045,13 @@ static int pxa_camera_set_fmt(struct soc_camera_device *icd,
 			      __u32 pixfmt, struct v4l2_rect *rect)
 {
 	struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent);
+	struct pxa_camera_dev *pcdev = ici->priv;
 	const struct soc_camera_data_format *host_fmt, *cam_fmt = NULL;
 	const struct soc_camera_format_xlate *xlate;
+	struct soc_camera_sense sense = {
+		.master_clock = pcdev->mclk,
+		.pixel_clock_max = pcdev->ciclk / 4,
+	};
 	int ret, buswidth;
 
 	xlate = soc_camera_xlate_by_fourcc(icd, pixfmt);
@@ -1032,6 +1064,10 @@ static int pxa_camera_set_fmt(struct soc_camera_device *icd,
 	host_fmt = xlate->host_fmt;
 	cam_fmt = xlate->cam_fmt;
 
+	/* If PCLK is used to latch data from the sensor, check sense */
+	if (pcdev->platform_flags & PXA_CAMERA_PCLK_EN)
+		icd->sense = &sense;
+
 	switch (pixfmt) {
 	case 0:				/* Only geometry change */
 		ret = icd->ops->set_fmt(icd, pixfmt, rect);
@@ -1040,9 +1076,20 @@ static int pxa_camera_set_fmt(struct soc_camera_device *icd,
 		ret = icd->ops->set_fmt(icd, cam_fmt->fourcc, rect);
 	}
 
-	if (ret < 0)
+	icd->sense = NULL;
+
+	if (ret < 0) {
 		dev_warn(&ici->dev, "Failed to configure for format %x\n",
 			 pixfmt);
+	} else if (sense.flags & SOCAM_SENSE_PCLK_CHANGED) {
+		if (sense.pixel_clock > sense.pixel_clock_max) {
+			dev_err(&ici->dev,
+				"pixel clock %lu set by the camera too high!",
+				sense.pixel_clock);
+			return -EIO;
+		}
+		recalculate_fifo_timeout(pcdev, sense.pixel_clock);
+	}
 
 	if (pixfmt && !ret) {
 		icd->buswidth = buswidth;
@@ -1247,14 +1294,17 @@ static int pxa_camera_probe(struct platform_device *pdev)
 			 "data widths, using default 10 bit\n");
 		pcdev->platform_flags |= PXA_CAMERA_DATAWIDTH_10;
 	}
-	pcdev->platform_mclk_10khz = pcdev->pdata->mclk_10khz;
-	if (!pcdev->platform_mclk_10khz) {
+	pcdev->mclk = pcdev->pdata->mclk_10khz * 10000;
+	if (!pcdev->mclk) {
 		dev_warn(&pdev->dev,
-			 "mclk_10khz == 0! Please, fix your platform data. "
+			 "mclk == 0! Please, fix your platform data. "
 			 "Using default 20MHz\n");
-		pcdev->platform_mclk_10khz = 2000;
+		pcdev->mclk = 20000000;
 	}
 
+	pcdev->dev = &pdev->dev;
+	pcdev->mclk_divisor = mclk_get_divisor(pcdev);
+
 	INIT_LIST_HEAD(&pcdev->capture);
 	spin_lock_init(&pcdev->lock);
 
@@ -1274,7 +1324,6 @@ static int pxa_camera_probe(struct platform_device *pdev)
 	}
 	pcdev->irq = irq;
 	pcdev->base = base;
-	pcdev->dev = &pdev->dev;
 
 	/* request dma */
 	err = pxa_request_dma("CI_Y", DMA_PRIO_HIGH,
-- 
1.5.4

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: About CITOR register value for pxa_camera

[Guennadi Liakhovetski]

On Sun, 30 Nov 2008, Robert Jarzmik wrote:

> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> 
> > I finally got round to trying this...
> >
> > Please, have a look at this patch. I decided against another function call 
> > for a number of reasons: first, if the host calls into the camera to ask 
> > whether the frequency has changed, it is not easy to recognise, whether it 
> > changed _now_, if you let camera notify the host about frequency change 
> > this breaks the hierarchy. Second, you don't know how many more similar 
> > parameters will have to be communicated between the camera and the host. 
> > So, I decided to add an extensible sense struct.
> >
> > Only compile tested so far, will run-test later, maybe tomorrow (actually, 
> > already today). Comments welcome, tests even more so:-)
> The first test crashes in the pxa_camera_probe() for me, something like :
> [  247.554669] [<c014f2f8>] (dev_driver_string+0x0/0x48) from [<bf01eac8>] (pxa_camera_probe+0x2c8/0x424 [pxa_camera])
> [  247.564907] [<bf01e800>] (pxa_camera_probe+0x0/0x424 [pxa_camera]) from [<c01538fc>] (platform_drv_probe+0x20/0x24)
> [  247.575129] [<c01538dc>] (platform_drv_probe+0x0/0x24) from [<c0152a3c>] (driver_probe_device+0xac/0x1b0)
> [  247.585308] [<c0152990>] (driver_probe_device+0x0/0x1b0) from [<c0152bd0>] (__driver_attach+0x90/0x94)
> 
> I'll take some time tomorrow, to review and test.
> 
> > Patch below. Loosely based on top of our current format-negotiation 
> > stack...
> Applies nicely, thanks.

As you might have seen, I posted two patches on the list earlier today 
(with you on cc), which fix this Oops and one more bug in a formula. If 
the patches look fine to you or better yet, if you can test them and they 
pass your test, I'll push them upstream with a next request.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: About CITOR register value for pxa_camera

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

>
> As you might have seen, I posted two patches on the list earlier today 
> (with you on cc), which fix this Oops and one more bug in a formula. If 
> the patches look fine to you or better yet, if you can test them and they 
> pass your test, I'll push them upstream with a next request.
Tested-by: Robert Jarzmik <robert.jarzmik@free.fr>

The only comment I would have is about suspend/resume, when lcd clock rate might
have changed. But since the patch doesn't either break or improve the legacy
behaviour, I'm for leaving it as it is.

Cheers.

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: About CITOR register value for pxa_camera

[fpantaleao]

> On Thu, 6 Nov 2008, fpantaleao@mobisensesystems.com wrote:
>
> > Sure we can do it that way if the sensor is clocked by MCLK and no  
> prescale is
> > done by the sensor.
> > In other cases, PCLK is controlled by sensor:
> > - sensor has its own clock
> > - MCLK is prescaled by the sensor.
>
> Right, then maybe we should request the sensor what quotient it's  
> running its pixel clock respective our master clock? In the master  
> mode, of course.

To be more general (sensor clocked by MCLK or not), I think we should  
request the sensor its PCLK, by adding a get_pclk member in  
soc_camera_ops for example.
Then CITOR value can be computed.
On the sensor side, PCLK value can be defined in its platform data. I  
agree this is redundant for a MT9V022 clocked by MCLK but to my point  
of view, this is not the general case.

Best regards

Florian

>
> (Please, don't top-post)
>
> Thanks
> Guennadi
>
> > > Florian
> > > ----- Original Message -----
> > From: "Guennadi Liakhovetski" <g.liakhovetski@gmx.de>
> > To: <fpantaleao@mobisensesystems.com>
> > Cc: <video4linux-list@redhat.com>
> > Sent: Wednesday, November 05, 2008 8:31 PM
> > Subject: Re: About CITOR register value for pxa_camera
> > > > > On Wed, 5 Nov 2008, fpantaleao@mobisensesystems.com wrote:
> > > > > > Thank you for your answer.
> > > > I have tested with "67 x 1" resolution (and many others), I can't with
> > > > "1619 x
> > > > 1".
> > > > I don't get overruns with CITOR != 0.
> > > > > Before submitting a patch, I would like to have the opinion of other
> > > > developpers about the CITOR value.
> > > > The resulting time-out is CITOR/CICLK. What we need is a time-out a bit
> > > > longer than 1 pixel period (1/PCLK).
> > > > The condition for CITOR is then: CITOR > CICLK/PCLK.
> > > > Since PCLK is a platform dependent value, I suggest to add a field in
> > > > pxacamera_platform_data.
> > > > If no value is assigned, a value of 16 can be used which equals 2 pixel
> > > > periods when PCLK=13MHz ("slow" sensor) and CICLK=104MHz (highest CI
> > > > clock).
> > > > CITOR can be set in pxa_camera_activate.
> > > > > Don't think we need any extra fields in  
> pxacamera_platform_data, look at
> > > mclk_get_divisor() - it gets already the lcdclk frequency, which  
> is the same
> > > as CICLK, and our target MCLK frequency is set in  
> platform_mclk_10khz, so,
> > > you should have all the data you need...
> > > > > Thanks
> > > Guennadi
> > > > > > > Best regards
> > > > > Florian
> > > > > > fpantaleao@mobisensesystems.com writes:
> > > > > > > > Hi all,
> > > > > > > > > While testing, I think I have found one reason why overruns
> > > > occur with
> > > > > > pxa_camera.
> > > > > > I propose to set CITOR to a non-null value.
> > > > > Yes, seconded.
> > > > > > > > I would appreciate any comment about this.
> > > > > Well, at first sight I would advice to test some corner case  
> to see if
> > > > DMA
> > > > > trailing bytes are handled well. I know this can be a pain,  
> but you seem
> > > > to be
> > > > > testing thouroughly ..
> > > > > > > So, if your configuration/sensor is able to, try some funny
> > > > resolution
> > > > like
> > > > > "1619 x 1", and then "67 x 1", and see what happens. If you  
> don't have
> > > > any
> > > > > capture issue, you're done, and post a patch (only CITOR or CITOR +
> > > > trailling
> > > > > bytes handling).
> > > > > > > Have fun.
> > > > > > > --
> > > > > Robert
> > > > > > ----------------------------------------------------------------
> > > > This message was sent using IMP, the Internet Messaging Program.
> > > > > > > > --
> > > > video4linux-list mailing list
> > > > Unsubscribe  
> mailto:video4linux-list-request@redhat.com?subject=unsubscribe
> > > > https://www.redhat.com/mailman/listinfo/video4linux-list
> > > > > ---
> > > Guennadi Liakhovetski, Ph.D.
> > > Freelance Open-Source Software Developer
> > > ----------------------------------------------------------------
> > This message was sent using IMP, the Internet Messaging Program.
> > > ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer

----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.




--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: About CITOR register value for pxa_camera

[Guennadi Liakhovetski]

On Fri, 7 Nov 2008, fpantaleao@mobisensesystems.com wrote:

> > On Thu, 6 Nov 2008, fpantaleao@mobisensesystems.com wrote:
> > 
> > > Sure we can do it that way if the sensor is clocked by MCLK and no
> > > prescale is
> > > done by the sensor.
> > > In other cases, PCLK is controlled by sensor:
> > > - sensor has its own clock
> > > - MCLK is prescaled by the sensor.
> > 
> > Right, then maybe we should request the sensor what quotient it's running
> > its pixel clock respective our master clock? In the master mode, of course.
> 
> To be more general (sensor clocked by MCLK or not), I think we should request
> the sensor its PCLK, by adding a get_pclk member in soc_camera_ops for
> example.

Yes, that should be even better, and pass master-clock frequency as a 
parameter? What shall we take for default? pclk == mclk or some safe 
conservative value? Don't think requiring every camera driver to provide 
this method would be very good.

Another question, are there situations, when a sensor might decide to 
change its pixelclock dynamically? Then it might need to inform the host 
driver about the change?

> Then CITOR value can be computed.
> On the sensor side, PCLK value can be defined in its platform data. I agree
> this is redundant for a MT9V022 clocked by MCLK but to my point of view, this
> is not the general case.

Ack.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: About CITOR register value for pxa_camera

[fpantaleao]

Sure we can do it that way if the sensor is clocked by MCLK and no  
prescale is done by the sensor.
In other cases, PCLK is controlled by sensor:
  - sensor has its own clock
  - MCLK is prescaled by the sensor.

Florian

----- Original Message -----
From: "Guennadi Liakhovetski" <g.liakhovetski@gmx.de>
To: <fpantaleao@mobisensesystems.com>
Cc: <video4linux-list@redhat.com>
Sent: Wednesday, November 05, 2008 8:31 PM
Subject: Re: About CITOR register value for pxa_camera


> On Wed, 5 Nov 2008, fpantaleao@mobisensesystems.com wrote:
>
> > Thank you for your answer.
> > I have tested with "67 x 1" resolution (and many others), I can't  
> with "1619 x
> > 1".
> > I don't get overruns with CITOR != 0.
> > > Before submitting a patch, I would like to have the opinion of other
> > developpers about the CITOR value.
> > The resulting time-out is CITOR/CICLK. What we need is a time-out a bit
> > longer than 1 pixel period (1/PCLK).
> > The condition for CITOR is then: CITOR > CICLK/PCLK.
> > Since PCLK is a platform dependent value, I suggest to add a field in
> > pxacamera_platform_data.
> > If no value is assigned, a value of 16 can be used which equals 2 pixel
> > periods when PCLK=13MHz ("slow" sensor) and CICLK=104MHz (highest  
> CI clock).
> > CITOR can be set in pxa_camera_activate.
>
> Don't think we need any extra fields in pxacamera_platform_data,  
> look at mclk_get_divisor() - it gets already the lcdclk frequency,  
> which is the same as CICLK, and our target MCLK frequency is set in  
> platform_mclk_10khz, so, you should have all the data you need...
>
> Thanks
> Guennadi
>
> > > Best regards
> > > Florian
> > > > fpantaleao@mobisensesystems.com writes:
> > > > > > Hi all,
> > > > > > > While testing, I think I have found one reason why  
> overruns occur with
> > > > pxa_camera.
> > > > I propose to set CITOR to a non-null value.
> > > Yes, seconded.
> > > > > > I would appreciate any comment about this.
> > > Well, at first sight I would advice to test some corner case to  
> see if DMA
> > > trailing bytes are handled well. I know this can be a pain, but you seem
> > to be
> > > testing thouroughly ..
> > > > > So, if your configuration/sensor is able to, try some funny  
> resolution
> > like
> > > "1619 x 1", and then "67 x 1", and see what happens. If you  
> don't have any
> > > capture issue, you're done, and post a patch (only CITOR or CITOR +
> > trailling
> > > bytes handling).
> > > > > Have fun.
> > > > > --
> > > Robert
> > > > ----------------------------------------------------------------
> > This message was sent using IMP, the Internet Messaging Program.
> > > > > > --
> > video4linux-list mailing list
> > Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
> > https://www.redhat.com/mailman/listinfo/video4linux-list
>
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer

----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.




--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: About CITOR register value for pxa_camera

[Guennadi Liakhovetski]

On Thu, 6 Nov 2008, fpantaleao@mobisensesystems.com wrote:

> Sure we can do it that way if the sensor is clocked by MCLK and no prescale is
> done by the sensor.
> In other cases, PCLK is controlled by sensor:
> - sensor has its own clock
> - MCLK is prescaled by the sensor.

Right, then maybe we should request the sensor what quotient it's running 
its pixel clock respective our master clock? In the master mode, of 
course.

(Please, don't top-post)

Thanks
Guennadi

> 
> Florian
> 
> ----- Original Message -----
> From: "Guennadi Liakhovetski" <g.liakhovetski@gmx.de>
> To: <fpantaleao@mobisensesystems.com>
> Cc: <video4linux-list@redhat.com>
> Sent: Wednesday, November 05, 2008 8:31 PM
> Subject: Re: About CITOR register value for pxa_camera
> 
> 
> > On Wed, 5 Nov 2008, fpantaleao@mobisensesystems.com wrote:
> > 
> > > Thank you for your answer.
> > > I have tested with "67 x 1" resolution (and many others), I can't with
> > > "1619 x
> > > 1".
> > > I don't get overruns with CITOR != 0.
> > > > Before submitting a patch, I would like to have the opinion of other
> > > developpers about the CITOR value.
> > > The resulting time-out is CITOR/CICLK. What we need is a time-out a bit
> > > longer than 1 pixel period (1/PCLK).
> > > The condition for CITOR is then: CITOR > CICLK/PCLK.
> > > Since PCLK is a platform dependent value, I suggest to add a field in
> > > pxacamera_platform_data.
> > > If no value is assigned, a value of 16 can be used which equals 2 pixel
> > > periods when PCLK=13MHz ("slow" sensor) and CICLK=104MHz (highest CI
> > > clock).
> > > CITOR can be set in pxa_camera_activate.
> > 
> > Don't think we need any extra fields in pxacamera_platform_data, look at
> > mclk_get_divisor() - it gets already the lcdclk frequency, which is the same
> > as CICLK, and our target MCLK frequency is set in platform_mclk_10khz, so,
> > you should have all the data you need...
> > 
> > Thanks
> > Guennadi
> > 
> > > > Best regards
> > > > Florian
> > > > > fpantaleao@mobisensesystems.com writes:
> > > > > > > Hi all,
> > > > > > > > While testing, I think I have found one reason why overruns
> > > occur with
> > > > > pxa_camera.
> > > > > I propose to set CITOR to a non-null value.
> > > > Yes, seconded.
> > > > > > > I would appreciate any comment about this.
> > > > Well, at first sight I would advice to test some corner case to see if
> > > DMA
> > > > trailing bytes are handled well. I know this can be a pain, but you seem
> > > to be
> > > > testing thouroughly ..
> > > > > > So, if your configuration/sensor is able to, try some funny
> > > resolution
> > > like
> > > > "1619 x 1", and then "67 x 1", and see what happens. If you don't have
> > > any
> > > > capture issue, you're done, and post a patch (only CITOR or CITOR +
> > > trailling
> > > > bytes handling).
> > > > > > Have fun.
> > > > > > --
> > > > Robert
> > > > > ----------------------------------------------------------------
> > > This message was sent using IMP, the Internet Messaging Program.
> > > > > > > --
> > > video4linux-list mailing list
> > > Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
> > > https://www.redhat.com/mailman/listinfo/video4linux-list
> > 
> > ---
> > Guennadi Liakhovetski, Ph.D.
> > Freelance Open-Source Software Developer
> 
> ----------------------------------------------------------------
> This message was sent using IMP, the Internet Messaging Program.
> 
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: About CITOR register value for pxa_camera

[fpantaleao]

Thank you for your answer.
I have tested with "67 x 1" resolution (and many others), I can't with  
"1619 x 1".
I don't get overruns with CITOR != 0.

Before submitting a patch, I would like to have the opinion of other
developpers about the CITOR value.
The resulting time-out is CITOR/CICLK. What we need is a time-out a bit
longer than 1 pixel period (1/PCLK).
The condition for CITOR is then: CITOR > CICLK/PCLK.
Since PCLK is a platform dependent value, I suggest to add a field in
pxacamera_platform_data.
If no value is assigned, a value of 16 can be used which equals 2 pixel
periods when PCLK=13MHz ("slow" sensor) and CICLK=104MHz (highest CI clock).
CITOR can be set in pxa_camera_activate.

Best regards

Florian

> fpantaleao@mobisensesystems.com writes:
>
> > Hi all,
> >
> > While testing, I think I have found one reason why overruns occur with
> > pxa_camera.
> > I propose to set CITOR to a non-null value.
> Yes, seconded.
>
> > I would appreciate any comment about this.
> Well, at first sight I would advice to test some corner case to see if DMA
> trailing bytes are handled well. I know this can be a pain, but you seem
to be
> testing thouroughly ..
>
> So, if your configuration/sensor is able to, try some funny resolution
like
> "1619 x 1", and then "67 x 1", and see what happens. If you don't have any
> capture issue, you're done, and post a patch (only CITOR or CITOR +
trailling
> bytes handling).
>
> Have fun.
>
> --
> Robert


----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.




--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: About CITOR register value for pxa_camera

[Guennadi Liakhovetski]

On Wed, 5 Nov 2008, fpantaleao@mobisensesystems.com wrote:

> Thank you for your answer.
> I have tested with "67 x 1" resolution (and many others), I can't with "1619 x
> 1".
> I don't get overruns with CITOR != 0.
> 
> Before submitting a patch, I would like to have the opinion of other
> developpers about the CITOR value.
> The resulting time-out is CITOR/CICLK. What we need is a time-out a bit
> longer than 1 pixel period (1/PCLK).
> The condition for CITOR is then: CITOR > CICLK/PCLK.
> Since PCLK is a platform dependent value, I suggest to add a field in
> pxacamera_platform_data.
> If no value is assigned, a value of 16 can be used which equals 2 pixel
> periods when PCLK=13MHz ("slow" sensor) and CICLK=104MHz (highest CI clock).
> CITOR can be set in pxa_camera_activate.

Don't think we need any extra fields in pxacamera_platform_data, look at 
mclk_get_divisor() - it gets already the lcdclk frequency, which is the 
same as CICLK, and our target MCLK frequency is set in 
platform_mclk_10khz, so, you should have all the data you need...

Thanks
Guennadi

> 
> Best regards
> 
> Florian
> 
> > fpantaleao@mobisensesystems.com writes:
> > 
> > > Hi all,
> > > 
> > > While testing, I think I have found one reason why overruns occur with
> > > pxa_camera.
> > > I propose to set CITOR to a non-null value.
> > Yes, seconded.
> > 
> > > I would appreciate any comment about this.
> > Well, at first sight I would advice to test some corner case to see if DMA
> > trailing bytes are handled well. I know this can be a pain, but you seem
> to be
> > testing thouroughly ..
> > 
> > So, if your configuration/sensor is able to, try some funny resolution
> like
> > "1619 x 1", and then "67 x 1", and see what happens. If you don't have any
> > capture issue, you're done, and post a patch (only CITOR or CITOR +
> trailling
> > bytes handling).
> > 
> > Have fun.
> > 
> > --
> > Robert
> 
> 
> ----------------------------------------------------------------
> This message was sent using IMP, the Internet Messaging Program.
> 
> 
> 
> 
> --
> video4linux-list mailing list
> Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
> https://www.redhat.com/mailman/listinfo/video4linux-list

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

About CITOR register value for pxa_camera

[fpantaleao]

Hi all,

While testing, I think I have found one reason why overruns occur with  
pxa_camera.
I propose to set CITOR to a non-null value.
I have no more overrun for single image grab with CITOR=16 and much
greater reactivity.
There are still overrun for multiple image grab, I investigate that also and
post a message about it soon.

I would appreciate any comment about this.

Best regards.

--
Florian PANTALEAO
MOBISENSE SYSTEMS SARL


----------------------------------------------------------------
This message was sent using IMP, the Internet Messaging Program.



--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: About CITOR register value for pxa_camera

[Robert Jarzmik]

fpantaleao@mobisensesystems.com writes:

> Hi all,
>
> While testing, I think I have found one reason why overruns occur with
> pxa_camera.
> I propose to set CITOR to a non-null value.
Yes, seconded.

> I would appreciate any comment about this.
Well, at first sight I would advice to test some corner case to see if DMA
trailing bytes are handled well. I know this can be a pain, but you seem to be
testing thouroughly ..

So, if your configuration/sensor is able to, try some funny resolution like
"1619 x 1", and then "67 x 1", and see what happens. If you don't have any
capture issue, you're done, and post a patch (only CITOR or CITOR + trailling
bytes handling).

Have fun.

--
Robert

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list