Skip to contentSkip to conversation

mx2_camera: mx25 fixes and enhancements

PATCH New Baruch Siach 4 participants 13 messages Jul 27, 2010 – Aug 29, 2010 Media (V4L/DVB)

lore
Share & tools
lore

Baruch Siach

author
#0 lore raw

cover mx2_camera: mx25 fixes and enhancements

The first 3 pathces in this series are fixes for the mx2_camera driver which is going upstream via the imx git tree. The last patch implements forced active buffer termination on mx25.

Baruch Siach (4):
mx2_camera: fix a race causing NULL dereference
mx2_camera: return IRQ_NONE when doing nothing
mx2_camera: fix comment typo
mx2_camera: implement forced termination of active buffer for mx25

 drivers/media/video/mx2_camera.c |   34 ++++++++++++++++++++++++++--------
 1 files changed, 26 insertions(+), 8 deletions(-)
author
#1 lore raw

PATCH 1/4 mx2_camera: fix a race causing NULL dereference

The mx25_camera_irq irq handler may get called after the camera has been deactivated (from mx2_camera_deactivate). Detect this situation, and bail out.

Signed-off-by Baruch Siach <[email protected]>
---
 drivers/media/video/mx2_camera.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)
drivers/media/video/mx2_camera.c +4 -0
index 881d5d8..1536bd4 100644
@@ -384,6 +384,9 @@ static void mx25_camera_frame_done(struct mx2_camera_dev *pcdev, int fb,
 
 	spin_lock_irqsave(&pcdev->lock, flags);
 
+	if (*fb_active == NULL)
+		goto out;
+
 	vb = &(*fb_active)->vb;
 	dev_dbg(pcdev->dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__,
 		vb, vb->baddr, vb->bsize);

@@ -408,6 +411,7 @@ static void mx25_camera_frame_done(struct mx2_camera_dev *pcdev, int fb,
 
 	*fb_active = buf;
 
+out:
 	spin_unlock_irqrestore(&pcdev->lock, flags);
 }
Signature · 1 line
1.7.1
author
#2 lore raw

PATCH 2/4 mx2_camera: return IRQ_NONE when doing nothing

In reply to Baruch Siach
Signed-off-by Baruch Siach <[email protected]>
---
 drivers/media/video/mx2_camera.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)
drivers/media/video/mx2_camera.c +5 -3
index 1536bd4..b42ad8d 100644
@@ -420,15 +420,17 @@ static irqreturn_t mx25_camera_irq(int irq_csi, void *data)
 	struct mx2_camera_dev *pcdev = data;
 	u32 status = readl(pcdev->base_csi + CSISR);
 
-	if (status & CSISR_DMA_TSF_FB1_INT)
+	writel(status, pcdev->base_csi + CSISR);
+
+	if (!(status & (CSISR_DMA_TSF_FB1_INT | CSISR_DMA_TSF_FB2_INT)))
+		return IRQ_NONE;
+	else if (status & CSISR_DMA_TSF_FB1_INT)
 		mx25_camera_frame_done(pcdev, 1, VIDEOBUF_DONE);
 	else if (status & CSISR_DMA_TSF_FB2_INT)
 		mx25_camera_frame_done(pcdev, 2, VIDEOBUF_DONE);
 
 	/* FIXME: handle CSISR_RFF_OR_INT */
 
-	writel(status, pcdev->base_csi + CSISR);
-
 	return IRQ_HANDLED;
 }
Signature · 1 line
1.7.1
#3 lore raw

PATCH 2/4 mx2_camera: return IRQ_NONE when doing nothing

Quoted 10 lines · Baruch Siach
Signed-off-by Baruch Siach <[email protected]>
---
 drivers/media/video/mx2_camera.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)
drivers/media/video/mx2_camera.c +0 -0
index 1536bd4..b42ad8d 100644

drivers/media/video/mx2_camera.c static irqreturn_t mx25_camera_irq(int irq_csi, void *data) · line 426

Quoted 8 lines
drivers/media/video/mx2_camera.c +4 -1
@@ -420,15 +420,17 @@ static irqreturn_t mx25_camera_irq(int irq_csi, void *data)
 	struct mx2_camera_dev *pcdev = data;
 	u32 status = readl(pcdev->base_csi + CSISR);
 
-	if (status & CSISR_DMA_TSF_FB1_INT)
+	writel(status, pcdev->base_csi + CSISR);
+
+	if (!(status & (CSISR_DMA_TSF_FB1_INT | CSISR_DMA_TSF_FB2_INT)))
+		return IRQ_NONE;

I'm not sure this is correct. When we get here, the interrupt definitely is from the camera, it's not a shared interrupt. So this only provokes a 'nobody cared' message from the kernel (if it's still present, I don't know).

Sascha

Quoted 16 lines
drivers/media/video/mx2_camera.c +1 -2
+	else if (status & CSISR_DMA_TSF_FB1_INT)
 		mx25_camera_frame_done(pcdev, 1, VIDEOBUF_DONE);
 	else if (status & CSISR_DMA_TSF_FB2_INT)
 		mx25_camera_frame_done(pcdev, 2, VIDEOBUF_DONE);
 
 	/* FIXME: handle CSISR_RFF_OR_INT */
 
-	writel(status, pcdev->base_csi + CSISR);
-
 	return IRQ_HANDLED;
 }
Signature · 1 line
1.7.1
Signature · 4 lines
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
#4 lore raw

PATCH 2/4 mx2_camera: return IRQ_NONE when doing nothing

Quoted 14 lines · Sascha Hauer
Quoted 13 lines · Baruch Siach
Signed-off-by Baruch Siach <[email protected]>
---
 drivers/media/video/mx2_camera.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)
drivers/media/video/mx2_camera.c +0 -0
index 1536bd4..b42ad8d 100644
@@ -420,15 +420,17 @@ static irqreturn_t mx25_camera_irq(int irq_csi, void *data)
 	struct mx2_camera_dev *pcdev = data;
 	u32 status = readl(pcdev->base_csi + CSISR);
Quoted 10 lines

drivers/media/video/mx2_camera.c static irqreturn_t mx25_camera_irq(int irq_csi, void *data) · line 426

Quoted 5 lines
drivers/media/video/mx2_camera.c +4 -1
-	if (status & CSISR_DMA_TSF_FB1_INT)
+	writel(status, pcdev->base_csi + CSISR);
+
+	if (!(status & (CSISR_DMA_TSF_FB1_INT | CSISR_DMA_TSF_FB2_INT)))
+		return IRQ_NONE;

I'm not sure this is correct. When we get here, the interrupt definitely is from the camera, it's not a shared interrupt. So this only provokes a 'nobody cared' message from the kernel (if it's still present, I don't know).

You'll only get the 'nobody cared' message if it's happened many times in a short space of time. The odd spurious IRQ_NONE has little effect.

It is good practice to return IRQ_NONE if there's nothing pending - it allows stuck IRQs to be detected and disabled without taking the system down. In other words, it should make the system more robust.

#5 lore raw

PATCH 2/4 mx2_camera: return IRQ_NONE when doing nothing

In reply to Baruch Siach

A general comment to your patches: the actual driver is going to be merged via the ARM tree, all other your incremental patches should rather go via the v4l tree. So, we'll have to synchronise with ARM, let's hope ARM patches go in early enough.

Quoted 19 lines · Baruch Siach
Signed-off-by Baruch Siach <[email protected]>
---
 drivers/media/video/mx2_camera.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)
drivers/media/video/mx2_camera.c +5 -1
index 1536bd4..b42ad8d 100644
@@ -420,15 +420,17 @@ static irqreturn_t mx25_camera_irq(int irq_csi, void *data)
 	struct mx2_camera_dev *pcdev = data;
 	u32 status = readl(pcdev->base_csi + CSISR);
 
-	if (status & CSISR_DMA_TSF_FB1_INT)
+	writel(status, pcdev->base_csi + CSISR);
+
+	if (!(status & (CSISR_DMA_TSF_FB1_INT | CSISR_DMA_TSF_FB2_INT)))
+		return IRQ_NONE;
+	else if (status & CSISR_DMA_TSF_FB1_INT)

drivers/media/video/mx2_camera.c static irqreturn_t mx25_camera_irq(int irq_csi, void *data) · line 435

Quoted 10 lines
drivers/media/video/mx2_camera.c +0 -2
 		mx25_camera_frame_done(pcdev, 1, VIDEOBUF_DONE);
 	else if (status & CSISR_DMA_TSF_FB2_INT)
 		mx25_camera_frame_done(pcdev, 2, VIDEOBUF_DONE);
 
 	/* FIXME: handle CSISR_RFF_OR_INT */
 
-	writel(status, pcdev->base_csi + CSISR);
-
 	return IRQ_HANDLED;
 }

I don't think this is correct. You should return IRQ_NONE if this is not an interrupt from your device at all. In this case you don't have to ack your interrupts, which, I presume, is what the write to CSISR is doing. OTOH, if this is an interrupt from your device, but you're just not interested in it, you should ack it and return IRQ_HANDLED. So, the original behaviour was more correct, than what this your patch is doing. The only improvement I can think of is, that you can return IRQ_NONE if status is 0, but then you don't have to ack it.

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

author
#6 lore raw

PATCH 2/4 mx2_camera: return IRQ_NONE when doing nothing

Hi Guennadi,

Quoted 4 lines · Guennadi Liakhovetski

A general comment to your patches: the actual driver is going to be merged via the ARM tree, all other your incremental patches should rather go via the v4l tree. So, we'll have to synchronise with ARM, let's hope ARM patches go in early enough.

Since the driver is now merged upstream this series can go via the v4l tree.

Quoted 27 lines · Baruch Siach
Quoted 26 lines
Signed-off-by Baruch Siach <[email protected]>
---
 drivers/media/video/mx2_camera.c |    8 +++++---
 1 files changed, 5 insertions(+), 3 deletions(-)
drivers/media/video/mx2_camera.c +5 -1
index 1536bd4..b42ad8d 100644
@@ -420,15 +420,17 @@ static irqreturn_t mx25_camera_irq(int irq_csi, void *data)
 	struct mx2_camera_dev *pcdev = data;
 	u32 status = readl(pcdev->base_csi + CSISR);
 
-	if (status & CSISR_DMA_TSF_FB1_INT)
+	writel(status, pcdev->base_csi + CSISR);
+
+	if (!(status & (CSISR_DMA_TSF_FB1_INT | CSISR_DMA_TSF_FB2_INT)))
+		return IRQ_NONE;
+	else if (status & CSISR_DMA_TSF_FB1_INT)
 		mx25_camera_frame_done(pcdev, 1, VIDEOBUF_DONE);
 	else if (status & CSISR_DMA_TSF_FB2_INT)
 		mx25_camera_frame_done(pcdev, 2, VIDEOBUF_DONE);
 
 	/* FIXME: handle CSISR_RFF_OR_INT */
Quoted 13 lines

drivers/media/video/mx2_camera.c static irqreturn_t mx25_camera_irq(int irq_csi, void *data) · line 435

Quoted 4 lines
drivers/media/video/mx2_camera.c +0 -2
-	writel(status, pcdev->base_csi + CSISR);
-
 	return IRQ_HANDLED;
 }

I don't think this is correct. You should return IRQ_NONE if this is not an interrupt from your device at all. In this case you don't have to ack your interrupts, which, I presume, is what the write to CSISR is doing. OTOH, if this is an interrupt from your device, but you're just not interested in it, you should ack it and return IRQ_HANDLED. So, the original behaviour was more correct, than what this your patch is doing. The only improvement I can think of is, that you can return IRQ_NONE if status is 0, but then you don't have to ack it.

OK. Drop this one, then. Patches in this series are independent from each other, so the others can go in.

baruch

Signature · 3 lines
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
author
#7 lore raw

PATCH 3/4 mx2_camera: fix comment typo

In reply to Baruch Siach
Signed-off-by Baruch Siach <[email protected]>
---
 drivers/media/video/mx2_camera.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/media/video/mx2_camera.c +1 -1
index b42ad8d..d327d11 100644
@@ -469,7 +469,7 @@ static void free_buffer(struct videobuf_queue *vq, struct mx2_buffer *buf)
 
 	/*
 	 * This waits until this buffer is out of danger, i.e., until it is no
-	 * longer in STATE_QUEUED or STATE_ACTIVE
+	 * longer in state VIDEOBUF_QUEUED or VIDEOBUF_ACTIVE
 	 */
 	videobuf_waiton(vb, 0, 0);
Signature · 1 line
1.7.1
author
#8 lore raw

PATCH 4/4 mx2_camera: implement forced termination of active buffer for mx25

In reply to Baruch Siach

This allows userspace to terminate a capture without waiting for the current frame to complete.

Signed-off-by Baruch Siach <[email protected]>
---
 drivers/media/video/mx2_camera.c |   20 ++++++++++++++++----
 1 files changed, 16 insertions(+), 4 deletions(-)
drivers/media/video/mx2_camera.c +16 -4
index d327d11..396542b 100644
@@ -648,15 +648,27 @@ static void mx2_videobuf_release(struct videobuf_queue *vq,
 	 * Terminate only queued but inactive buffers. Active buffers are
 	 * released when they become inactive after videobuf_waiton().
 	 *
-	 * FIXME: implement forced termination of active buffers, so that the
-	 * user won't get stuck in an uninterruptible state. This requires a
-	 * specific handling for each of the three DMA types that this driver
-	 * supports.
+	 * FIXME: implement forced termination of active buffers for mx27 and 
+	 * mx27 eMMA, so that the user won't get stuck in an uninterruptible
+	 * state. This requires a specific handling for each of the these DMA
+	 * types.
 	 */
 	spin_lock_irqsave(&pcdev->lock, flags);
 	if (vb->state == VIDEOBUF_QUEUED) {
 		list_del(&vb->queue);
 		vb->state = VIDEOBUF_ERROR;
+	} else if (cpu_is_mx25() && vb->state == VIDEOBUF_ACTIVE) {
+		if (pcdev->fb1_active == buf) {
+			pcdev->csicr1 &= ~CSICR1_FB1_DMA_INTEN;
+			writel(0, pcdev->base_csi + CSIDMASA_FB1);
+			pcdev->fb1_active = NULL;
+		} else if (pcdev->fb2_active == buf) {
+			pcdev->csicr1 &= ~CSICR1_FB2_DMA_INTEN;
+			writel(0, pcdev->base_csi + CSIDMASA_FB2);
+			pcdev->fb2_active = NULL;
+		}
+		writel(pcdev->csicr1, pcdev->base_csi + CSICR1);
+		vb->state = VIDEOBUF_ERROR;
 	}
 	spin_unlock_irqrestore(&pcdev->lock, flags);
Signature · 1 line
1.7.1
#9 lore raw

PATCH 4/4 mx2_camera: implement forced termination of active buffer for mx25

Hi Baruch

Quoted 2 lines · Baruch Siach

This allows userspace to terminate a capture without waiting for the current frame to complete.

This is an improvement, not a fix, right? Without this patch the termination just have to wait a couple of ms longer? so, it is ok to schedule it for 2.6.37?

Thanks
Guennadi

Quoted 50 lines
Signed-off-by Baruch Siach <[email protected]>
---
 drivers/media/video/mx2_camera.c |   20 ++++++++++++++++----
 1 files changed, 16 insertions(+), 4 deletions(-)
drivers/media/video/mx2_camera.c +16 -4
index d327d11..396542b 100644
@@ -648,15 +648,27 @@ static void mx2_videobuf_release(struct videobuf_queue *vq,
 	 * Terminate only queued but inactive buffers. Active buffers are
 	 * released when they become inactive after videobuf_waiton().
 	 *
-	 * FIXME: implement forced termination of active buffers, so that the
-	 * user won't get stuck in an uninterruptible state. This requires a
-	 * specific handling for each of the three DMA types that this driver
-	 * supports.
+	 * FIXME: implement forced termination of active buffers for mx27 and 
+	 * mx27 eMMA, so that the user won't get stuck in an uninterruptible
+	 * state. This requires a specific handling for each of the these DMA
+	 * types.
 	 */
 	spin_lock_irqsave(&pcdev->lock, flags);
 	if (vb->state == VIDEOBUF_QUEUED) {
 		list_del(&vb->queue);
 		vb->state = VIDEOBUF_ERROR;
+	} else if (cpu_is_mx25() && vb->state == VIDEOBUF_ACTIVE) {
+		if (pcdev->fb1_active == buf) {
+			pcdev->csicr1 &= ~CSICR1_FB1_DMA_INTEN;
+			writel(0, pcdev->base_csi + CSIDMASA_FB1);
+			pcdev->fb1_active = NULL;
+		} else if (pcdev->fb2_active == buf) {
+			pcdev->csicr1 &= ~CSICR1_FB2_DMA_INTEN;
+			writel(0, pcdev->base_csi + CSIDMASA_FB2);
+			pcdev->fb2_active = NULL;
+		}
+		writel(pcdev->csicr1, pcdev->base_csi + CSICR1);
+		vb->state = VIDEOBUF_ERROR;
 	}
 	spin_unlock_irqrestore(&pcdev->lock, flags);
Signature · 6 lines
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

author
#10 lore raw

PATCH 4/4 mx2_camera: implement forced termination of active buffer for mx25

Hi Guennadi,

Quoted 8 lines · Guennadi Liakhovetski
Quoted 3 lines · Baruch Siach

This allows userspace to terminate a capture without waiting for the current
frame to complete.

This is an improvement, not a fix, right? Without this patch the termination just have to wait a couple of ms longer? so, it is ok to schedule it for 2.6.37?

In my situation the image data source may stop sending data in the middle of a frame. In this case userspace is stuck forever. So, this is a real fix for me. This is not the usual use case, I guess, so I leave it for you to decide.

baruch

Quoted 54 lines
Quoted 49 lines
Signed-off-by Baruch Siach <[email protected]>
---
 drivers/media/video/mx2_camera.c |   20 ++++++++++++++++----
 1 files changed, 16 insertions(+), 4 deletions(-)
drivers/media/video/mx2_camera.c +16 -4
index d327d11..396542b 100644
@@ -648,15 +648,27 @@ static void mx2_videobuf_release(struct videobuf_queue *vq,
 	 * Terminate only queued but inactive buffers. Active buffers are
 	 * released when they become inactive after videobuf_waiton().
 	 *
-	 * FIXME: implement forced termination of active buffers, so that the
-	 * user won't get stuck in an uninterruptible state. This requires a
-	 * specific handling for each of the three DMA types that this driver
-	 * supports.
+	 * FIXME: implement forced termination of active buffers for mx27 and 
+	 * mx27 eMMA, so that the user won't get stuck in an uninterruptible
+	 * state. This requires a specific handling for each of the these DMA
+	 * types.
 	 */
 	spin_lock_irqsave(&pcdev->lock, flags);
 	if (vb->state == VIDEOBUF_QUEUED) {
 		list_del(&vb->queue);
 		vb->state = VIDEOBUF_ERROR;
+	} else if (cpu_is_mx25() && vb->state == VIDEOBUF_ACTIVE) {
+		if (pcdev->fb1_active == buf) {
+			pcdev->csicr1 &= ~CSICR1_FB1_DMA_INTEN;
+			writel(0, pcdev->base_csi + CSIDMASA_FB1);
+			pcdev->fb1_active = NULL;
+		} else if (pcdev->fb2_active == buf) {
+			pcdev->csicr1 &= ~CSICR1_FB2_DMA_INTEN;
+			writel(0, pcdev->base_csi + CSIDMASA_FB2);
+			pcdev->fb2_active = NULL;
+		}
+		writel(pcdev->csicr1, pcdev->base_csi + CSICR1);
+		vb->state = VIDEOBUF_ERROR;
 	}
 	spin_unlock_irqrestore(&pcdev->lock, flags);
Signature · 6 lines
1.7.1

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo at vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Signature · 3 lines
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
author
#11 lore raw

cover mx2_camera: mx25 fixes and enhancements

In reply to Baruch Siach

Hi Guennadi,

Quoted 3 lines · Baruch Siach

The first 3 pathces in this series are fixes for the mx2_camera driver which is going upstream via the imx git tree. The last patch implements forced active buffer termination on mx25.

Ping?

Quoted 8 lines

Baruch Siach (4):
mx2_camera: fix a race causing NULL dereference
mx2_camera: return IRQ_NONE when doing nothing
mx2_camera: fix comment typo
mx2_camera: implement forced termination of active buffer for mx25

 drivers/media/video/mx2_camera.c |   34 ++++++++++++++++++++++++++--------
 1 files changed, 26 insertions(+), 8 deletions(-)

baruch

Signature · 3 lines
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch at tkos.co.il - tel: +972.2.679.5364, http://www.tkos.co.il -
#12 lore raw

cover mx2_camera: mx25 fixes and enhancements

Hi Baruch

Quoted 8 lines · Baruch Siach

Hi Guennadi,

Quoted 3 lines · Baruch Siach

The first 3 pathces in this series are fixes for the mx2_camera driver which is going upstream via the imx git tree. The last patch implements forced active buffer termination on mx25.

Ping?

Sorry for taking a bit long to push your patches, but, I think, we still have a bit of time. Fixes are ok to go in after -rc2 (or even -rc3, or -rc4...), with features we're anyway late for 2.6.36, so, they will have to wait until 2.6.37, for which we have some time too. So, I think we're doing fine so far. Of the four patches below patch two was unclear, whether we want it or not, patches one and three were ok, AFAIR, will have to double-check, patch 4 I'll have to decide whether that's a fix or a feature;)

Quoted 8 lines
Quoted 8 lines

Baruch Siach (4):
mx2_camera: fix a race causing NULL dereference
mx2_camera: return IRQ_NONE when doing nothing
mx2_camera: fix comment typo
mx2_camera: implement forced termination of active buffer for mx25

 drivers/media/video/mx2_camera.c |   34 ++++++++++++++++++++++++++--------
 1 files changed, 26 insertions(+), 8 deletions(-)

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