| Message ID | 1236282351-28471-5-git-send-email-robert.jarzmik@free.fr (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
On Thu, 5 Mar 2009, Robert Jarzmik wrote: > The last buffer queued will often overrun, as the DMA chain > is finished, and the time the dma irq handler is activated, s/and the time/and during the time/ ? > the QIF fifos are filled by the sensor. > > The fix is to ignore the overrun condition on the last > queued buffer, and restart the capture only on intermediate > buffers of the chain. > > Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> > --- > drivers/media/video/pxa_camera.c | 8 ++++++-- > 1 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c > index 16bf0a3..dd56c35 100644 > --- a/drivers/media/video/pxa_camera.c > +++ b/drivers/media/video/pxa_camera.c > @@ -734,14 +734,18 @@ static void pxa_camera_dma_irq(int channel, struct pxa_camera_dev *pcdev, > status & DCSR_ENDINTR ? "EOF " : "", vb, DDADR(channel)); > > if (status & DCSR_ENDINTR) { > - if (camera_status & overrun) { > + /* > + * It's normal if the last frame creates an overrun, as there > + * are no more DMA descriptors to fetch from QIF fifos > + */ > + if (camera_status & overrun > + && !list_is_last(pcdev->capture.next, &pcdev->capture)) { > dev_dbg(pcdev->dev, "FIFO overrun! CISR: %x\n", > camera_status); > pxa_camera_stop_capture(pcdev); > pxa_camera_start_capture(pcdev); > goto out; > } > - > buf->active_dma &= ~act_dma; This empty like removal doesn't belong to the fix, I'll remove it when committing, and amend the commit message as above. Please, comment if you disagree. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes: > On Thu, 5 Mar 2009, Robert Jarzmik wrote: > >> The last buffer queued will often overrun, as the DMA chain >> is finished, and the time the dma irq handler is activated, > > s/and the time/and during the time/ ? If you wish, or might be simply "and before the dma irq handler is activated". As you see fit. >> + if (camera_status & overrun >> + && !list_is_last(pcdev->capture.next, &pcdev->capture)) { >> dev_dbg(pcdev->dev, "FIFO overrun! CISR: %x\n", >> camera_status); >> pxa_camera_stop_capture(pcdev); >> pxa_camera_start_capture(pcdev); >> goto out; >> } >> - >> buf->active_dma &= ~act_dma; > > This empty like removal doesn't belong to the fix, I'll remove it when > committing, and amend the commit message as above. Please, comment if you > disagree. I totally agree with you, remove that "removal" :) Cheers. -- Robert -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 5 Mar 2009, Robert Jarzmik wrote: > The last buffer queued will often overrun, as the DMA chain > is finished, and the time the dma irq handler is activated, > the QIF fifos are filled by the sensor. > > The fix is to ignore the overrun condition on the last > queued buffer, and restart the capture only on intermediate > buffers of the chain. > > Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> > --- > drivers/media/video/pxa_camera.c | 8 ++++++-- > 1 files changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c > index 16bf0a3..dd56c35 100644 > --- a/drivers/media/video/pxa_camera.c > +++ b/drivers/media/video/pxa_camera.c > @@ -734,14 +734,18 @@ static void pxa_camera_dma_irq(int channel, struct pxa_camera_dev *pcdev, > status & DCSR_ENDINTR ? "EOF " : "", vb, DDADR(channel)); > > if (status & DCSR_ENDINTR) { > - if (camera_status & overrun) { > + /* > + * It's normal if the last frame creates an overrun, as there > + * are no more DMA descriptors to fetch from QIF fifos > + */ > + if (camera_status & overrun > + && !list_is_last(pcdev->capture.next, &pcdev->capture)) { On a second look - didn't you want to test for ->active being the last? Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes: >> diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c >> index 16bf0a3..dd56c35 100644 >> --- a/drivers/media/video/pxa_camera.c >> +++ b/drivers/media/video/pxa_camera.c >> @@ -734,14 +734,18 @@ static void pxa_camera_dma_irq(int channel, struct pxa_camera_dev *pcdev, >> status & DCSR_ENDINTR ? "EOF " : "", vb, DDADR(channel)); >> >> if (status & DCSR_ENDINTR) { >> - if (camera_status & overrun) { >> + /* >> + * It's normal if the last frame creates an overrun, as there >> + * are no more DMA descriptors to fetch from QIF fifos >> + */ >> + if (camera_status & overrun >> + && !list_is_last(pcdev->capture.next, &pcdev->capture)) { > > On a second look - didn't you want to test for ->active being the last? Mmm, I'm not sure I get you right here. AFAICR pcdev->active has no direct link with pcdev->capture (it has nothing to do with a list_head *). Of course with a bit of "container_of" magic (or list_entry equivalent), I'll find it ... If that list_is_last is not good, would you provide me with a better alternative ? Cheers. -- Robert -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 12 Mar 2009, Robert Jarzmik wrote: > Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes: > > >> diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c > >> index 16bf0a3..dd56c35 100644 > >> --- a/drivers/media/video/pxa_camera.c > >> +++ b/drivers/media/video/pxa_camera.c > >> @@ -734,14 +734,18 @@ static void pxa_camera_dma_irq(int channel, struct pxa_camera_dev *pcdev, > >> status & DCSR_ENDINTR ? "EOF " : "", vb, DDADR(channel)); > >> > >> if (status & DCSR_ENDINTR) { > >> - if (camera_status & overrun) { > >> + /* > >> + * It's normal if the last frame creates an overrun, as there > >> + * are no more DMA descriptors to fetch from QIF fifos > >> + */ > >> + if (camera_status & overrun > >> + && !list_is_last(pcdev->capture.next, &pcdev->capture)) { > > > > On a second look - didn't you want to test for ->active being the last? > > Mmm, I'm not sure I get you right here. AFAICR pcdev->active has no direct link > with pcdev->capture (it has nothing to do with a list_head *). Of course with a > bit of "container_of" magic (or list_entry equivalent), I'll find it ... Ah, sorry, scratch it, I now understand what you're doing here, looks ok. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c index 16bf0a3..dd56c35 100644 --- a/drivers/media/video/pxa_camera.c +++ b/drivers/media/video/pxa_camera.c @@ -734,14 +734,18 @@ static void pxa_camera_dma_irq(int channel, struct pxa_camera_dev *pcdev, status & DCSR_ENDINTR ? "EOF " : "", vb, DDADR(channel)); if (status & DCSR_ENDINTR) { - if (camera_status & overrun) { + /* + * It's normal if the last frame creates an overrun, as there + * are no more DMA descriptors to fetch from QIF fifos + */ + if (camera_status & overrun + && !list_is_last(pcdev->capture.next, &pcdev->capture)) { dev_dbg(pcdev->dev, "FIFO overrun! CISR: %x\n", camera_status); pxa_camera_stop_capture(pcdev); pxa_camera_start_capture(pcdev); goto out; } - buf->active_dma &= ~act_dma; if (!buf->active_dma) pxa_camera_wakeup(pcdev, vb, buf);
The last buffer queued will often overrun, as the DMA chain is finished, and the time the dma irq handler is activated, the QIF fifos are filled by the sensor. The fix is to ignore the overrun condition on the last queued buffer, and restart the capture only on intermediate buffers of the chain. Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> --- drivers/media/video/pxa_camera.c | 8 ++++++-- 1 files changed, 6 insertions(+), 2 deletions(-)