| Message ID | 1236282351-28471-3-git-send-email-robert.jarzmik@free.fr (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
On Thu, 5 Mar 2009, Robert Jarzmik wrote: > The DMA transfers in pxa_camera showed some weaknesses in > multiple queued buffers context : > - poll/select problem > The order between list pcdev->capture and DMA chain was > not the same. This creates a discrepancy between video > buffers marked as "done" by the IRQ handler, and the > really finished video buffer. > > The bug shows up with capture_example tool from v4l2 hg > tree. The process just "stalls" on a "select timeout". > > The key problem is in pxa_videobuf_queue(), where the > queued buffer is chained before the active buffer, while > it should have been the active buffer first, and queued > buffer tailed after. > > - multiple buffers DMA starting > When multiple buffers were queued, the DMA channels were > always started right away. This is not optimal, as a > special case appears when the first EOF was not yet > reached, and the DMA channels were prematurely started. > > - Maintainability > DMA code was a bit obfuscated. Rationalize the code to be > easily maintainable by anyone. > > This patch attemps to address these issues. > > Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> > --- > drivers/media/video/pxa_camera.c | 264 ++++++++++++++++++++------------------ > 1 files changed, 139 insertions(+), 125 deletions(-) > > diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c > index 54df071..2d79ded 100644 > --- a/drivers/media/video/pxa_camera.c > +++ b/drivers/media/video/pxa_camera.c > @@ -325,7 +325,7 @@ static int calculate_dma_sglen(struct scatterlist *sglist, int sglen, > * Prepares the pxa dma descriptors to transfer one camera channel. > * Beware sg_first and sg_first_ofs are both input and output parameters. > * > - * Returns 0 > + * Returns 0 or -ENOMEM si no coherent memory is available Let's stay with English for now:-) s/si/if/ > */ > static int pxa_init_dma_channel(struct pxa_camera_dev *pcdev, > struct pxa_buffer *buf, > @@ -369,7 +369,8 @@ static int pxa_init_dma_channel(struct pxa_camera_dev *pcdev, > pxa_dma->sg_cpu[i].dsadr = pcdev->res->start + cibr; > pxa_dma->sg_cpu[i].dtadr = sg_dma_address(sg) + offset; > pxa_dma->sg_cpu[i].dcmd = > - DCMD_FLOWSRC | DCMD_BURST8 | DCMD_INCTRGADDR | xfer_len; > + DCMD_FLOWSRC | DCMD_BURST8 | DCMD_INCTRGADDR | xfer_len > + | ((i == 0) ? DCMD_STARTIRQEN : 0); If DCMD_STARTIRQEN is still for debugging only, maybe put it under #ifdef DEBUG if (!i) pxa_dma->sg_cpu[i].dcmd |= DCMD_STARTIRQEN; #endif you anyway only see any effect of this interrupt with dev_dbg(). > pxa_dma->sg_cpu[i].ddadr = > pxa_dma->sg_dma + (i + 1) * sizeof(struct pxa_dma_desc); > > @@ -516,6 +517,97 @@ out: > return ret; > } > > +/** > + * pxa_dma_start_channels - start DMA channel for active buffer > + * @pcdev: pxa camera device > + * > + * Initialize DMA channels to the beginning of the active video buffer, and > + * start these channels. > + */ > +static void pxa_dma_start_channels(struct pxa_camera_dev *pcdev) > +{ > + int i; > + struct pxa_buffer *active; > + > + active = pcdev->active; > + > + for (i = 0; i < pcdev->channels; i++) { > + dev_dbg(pcdev->dev, "%s (channel=%d) ddadr=%08x\n", __func__, > + i, active->dmas[i].sg_dma); > + DDADR(pcdev->dma_chans[i]) = active->dmas[i].sg_dma; > + DCSR(pcdev->dma_chans[i]) = DCSR_RUN; > + } > +} > + > +static void pxa_dma_stop_channels(struct pxa_camera_dev *pcdev) > +{ > + int i; > + > + for (i = 0; i < pcdev->channels; i++) { > + dev_dbg(pcdev->dev, "%s (channel=%d)\n", __func__, i); > + DCSR(pcdev->dma_chans[i]) = 0; > + } > +} > + > +static void pxa_dma_update_sg_tail(struct pxa_camera_dev *pcdev, > + struct pxa_buffer *buf) > +{ > + int i; > + > + for (i = 0; i < pcdev->channels; i++) { > + pcdev->sg_tail[i] = buf->dmas[i].sg_cpu + buf->dmas[i].sglen; > + pcdev->sg_tail[i]->ddadr = DDADR_STOP; Do I understand it right, assuming capture is running, i.e., active != NULL: before your patch sg_tail points to the last real DMA descriptor the last real DMA descriptor has DDADR_STOP on queuing of the next buffer we 1. stop DMA 2. link the last real descriptor to the new first descriptor 3. allocate an additional dummy descriptor, fill it with DMA engine's current state and use it to 4. re-start DMA after your patch sg_tail points to the additional DMA descriptor the last valid DMA descriptor points to the additional descriptor the additional descriptor has DDADR_STOP on queuing of the next buffer 1. stop DMA 2. the additional dummy descriptor at the tail of the current chain is reconfigured to point to the new start 3. pxa_dma_start_channels() is called, which drops the current partial transfer and re-starts the frame?... If I am right, this doesn't seem right. If I am wrong, please, explain and add explanatory comments, so, the next one (or the same one 2 months later) does not have to spend time trying to figure out. > + } > +} > + > +static void pxa_dma_add_tail_buf(struct pxa_camera_dev *pcdev, > + struct pxa_buffer *buf) > +{ > + int i; > + > + for (i = 0; i < pcdev->channels; i++) { > + if (!pcdev->sg_tail[i]) > + continue; > + pcdev->sg_tail[i]->ddadr = buf->dmas[i].sg_dma; > + } > + > + pxa_dma_update_sg_tail(pcdev, buf); > +} > + > +/** > + * pxa_camera_start_capture - start video capturing > + * @pcdev: camera device > + * > + * Launch capturing. DMA channels should not be active yet. They should get > + * activated at the end of frame interrupt, to capture only whole frames, and > + * never begin the capture of a partial frame. > + */ > +static void pxa_camera_start_capture(struct pxa_camera_dev *pcdev) > +{ > + unsigned long cicr0, cifr; > + > + dev_dbg(pcdev->dev, "%s\n", __func__); I originally had a "reset the FIFOs" comment here, wouldn't hurt to add it now too. > + cifr = __raw_readl(pcdev->base + CIFR) | CIFR_RESET_F; > + __raw_writel(cifr, pcdev->base + CIFR); > + > + cicr0 = __raw_readl(pcdev->base + CICR0) | CICR0_ENB | CISR_IFO_0 > + | CISR_IFO_1 | CISR_IFO_2; CISR_* flags have nothing to do with the CICR register. > + cicr0 &= ~CICR0_EOFM; > + __raw_writel(cicr0, pcdev->base + CICR0); > +} It is nice to synchronise on a frame start, but you're relying on being "fast," i.e., on servicing the End of Frame interrupt between the two frames and having enough time to configure DMA. With smaller frames with short inter-frame times this can be difficult, I think. But, well, that's the best we can do, I guess. And yes, I know, I'm already doing this in the overrun case. > + > +static void pxa_camera_stop_capture(struct pxa_camera_dev *pcdev) > +{ > + unsigned long cicr0; > + > + pxa_dma_stop_channels(pcdev); > + > + cicr0 = __raw_readl(pcdev->base + CICR0) & ~CICR0_ENB; > + __raw_writel(cicr0, pcdev->base + CICR0); > + > + dev_dbg(pcdev->dev, "%s\n", __func__); > +} > + > static void pxa_videobuf_queue(struct videobuf_queue *vq, > struct videobuf_buffer *vb) > { > @@ -523,81 +615,23 @@ static void pxa_videobuf_queue(struct videobuf_queue *vq, > struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent); > struct pxa_camera_dev *pcdev = ici->priv; > struct pxa_buffer *buf = container_of(vb, struct pxa_buffer, vb); > - struct pxa_buffer *active; > unsigned long flags; > - int i; > > - dev_dbg(&icd->dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__, > - vb, vb->baddr, vb->bsize); > - spin_lock_irqsave(&pcdev->lock, flags); > + dev_dbg(&icd->dev, "%s (vb=0x%p) 0x%08lx %d active=%p\n", __func__, > + vb, vb->baddr, vb->bsize, pcdev->active); > > + spin_lock_irqsave(&pcdev->lock, flags); > list_add_tail(&vb->queue, &pcdev->capture); > > vb->state = VIDEOBUF_ACTIVE; > - active = pcdev->active; > - > - if (!active) { > - unsigned long cifr, cicr0; > - > - cifr = __raw_readl(pcdev->base + CIFR) | CIFR_RESET_F; > - __raw_writel(cifr, pcdev->base + CIFR); > - > - for (i = 0; i < pcdev->channels; i++) { > - DDADR(pcdev->dma_chans[i]) = buf->dmas[i].sg_dma; > - DCSR(pcdev->dma_chans[i]) = DCSR_RUN; > - pcdev->sg_tail[i] = buf->dmas[i].sg_cpu + buf->dmas[i].sglen - 1; > - } > > - pcdev->active = buf; > + pxa_dma_stop_channels(pcdev); > + pxa_dma_add_tail_buf(pcdev, buf); > > - cicr0 = __raw_readl(pcdev->base + CICR0) | CICR0_ENB; > - __raw_writel(cicr0, pcdev->base + CICR0); > - } else { > - struct pxa_cam_dma *buf_dma; > - struct pxa_cam_dma *act_dma; > - int nents; > - > - for (i = 0; i < pcdev->channels; i++) { > - buf_dma = &buf->dmas[i]; > - act_dma = &active->dmas[i]; > - nents = buf_dma->sglen; > - > - /* Stop DMA engine */ > - DCSR(pcdev->dma_chans[i]) = 0; > - > - /* Add the descriptors we just initialized to > - the currently running chain */ > - pcdev->sg_tail[i]->ddadr = buf_dma->sg_dma; > - pcdev->sg_tail[i] = buf_dma->sg_cpu + buf_dma->sglen - 1; > - > - /* Setup a dummy descriptor with the DMA engines current > - * state > - */ > - buf_dma->sg_cpu[nents].dsadr = > - pcdev->res->start + 0x28 + i*8; /* CIBRx */ > - buf_dma->sg_cpu[nents].dtadr = > - DTADR(pcdev->dma_chans[i]); > - buf_dma->sg_cpu[nents].dcmd = > - DCMD(pcdev->dma_chans[i]); > - > - if (DDADR(pcdev->dma_chans[i]) == DDADR_STOP) { > - /* The DMA engine is on the last > - descriptor, set the next descriptors > - address to the descriptors we just > - initialized */ > - buf_dma->sg_cpu[nents].ddadr = buf_dma->sg_dma; > - } else { > - buf_dma->sg_cpu[nents].ddadr = > - DDADR(pcdev->dma_chans[i]); > - } > - > - /* The next descriptor is the dummy descriptor */ > - DDADR(pcdev->dma_chans[i]) = buf_dma->sg_dma + nents * > - sizeof(struct pxa_dma_desc); > - > - DCSR(pcdev->dma_chans[i]) = DCSR_RUN; > - } > - } > + if (!pcdev->active) > + pxa_camera_start_capture(pcdev); > + else > + pxa_dma_start_channels(pcdev); > > spin_unlock_irqrestore(&pcdev->lock, flags); > } > @@ -635,7 +669,7 @@ static void pxa_camera_wakeup(struct pxa_camera_dev *pcdev, > struct videobuf_buffer *vb, > struct pxa_buffer *buf) > { > - unsigned long cicr0; > + int i; > > /* _init is used to debug races, see comment in pxa_camera_reqbufs() */ > list_del_init(&vb->queue); > @@ -643,15 +677,13 @@ static void pxa_camera_wakeup(struct pxa_camera_dev *pcdev, > do_gettimeofday(&vb->ts); > vb->field_count++; > wake_up(&vb->done); > + dev_dbg(pcdev->dev, "%s dequeud buffer (vb=0x%p)\n", __func__, vb); > > if (list_empty(&pcdev->capture)) { > + pxa_camera_stop_capture(pcdev); > pcdev->active = NULL; > - DCSR(pcdev->dma_chans[0]) = 0; > - DCSR(pcdev->dma_chans[1]) = 0; > - DCSR(pcdev->dma_chans[2]) = 0; > - > - cicr0 = __raw_readl(pcdev->base + CICR0) & ~CICR0_ENB; > - __raw_writel(cicr0, pcdev->base + CICR0); > + for (i = 0; i < pcdev->channels; i++) > + pcdev->sg_tail[i] = NULL; > return; > } > You're now also stopping capture here, should work, yes... > @@ -666,19 +698,23 @@ static void pxa_camera_dma_irq(int channel, struct pxa_camera_dev *pcdev, > unsigned long flags; > u32 status, camera_status, overrun; > struct videobuf_buffer *vb; > - unsigned long cifr, cicr0; > > spin_lock_irqsave(&pcdev->lock, flags); > > status = DCSR(channel); > - DCSR(channel) = status | DCSR_ENDINTR; > + DCSR(channel) = status | DCSR_STARTINTR | DCSR_ENDINTR; Now as I look at it, actually, this is racy. If for whatever reason we entered here without ENDINTR set, so status & DCSR_ENDINTR == 0, then it got immediately set and we clear it, thus we lose it. I think, there's no reason here not to use the standard irq_reason = read(IRQ_REASON_REG); write(irq_reason, IRQ_REASON_REG); > + > + camera_status = __raw_readl(pcdev->base + CISR); > + overrun = CISR_IFO_0; > + if (pcdev->channels == 3) > + overrun |= CISR_IFO_1 | CISR_IFO_2; > > if (status & DCSR_BUSERR) { > dev_err(pcdev->dev, "DMA Bus Error IRQ!\n"); > goto out; > } > > - if (!(status & DCSR_ENDINTR)) { > + if (!(status & (DCSR_ENDINTR | DCSR_STARTINTR))) { > dev_err(pcdev->dev, "Unknown DMA IRQ source, " > "status: 0x%08x\n", status); > goto out; > @@ -689,38 +725,27 @@ static void pxa_camera_dma_irq(int channel, struct pxa_camera_dev *pcdev, > goto out; > } > > - camera_status = __raw_readl(pcdev->base + CISR); > - overrun = CISR_IFO_0; > - if (pcdev->channels == 3) > - overrun |= CISR_IFO_1 | CISR_IFO_2; > - if (camera_status & overrun) { > - dev_dbg(pcdev->dev, "FIFO overrun! CISR: %x\n", camera_status); > - /* Stop the Capture Interface */ > - cicr0 = __raw_readl(pcdev->base + CICR0) & ~CICR0_ENB; > - __raw_writel(cicr0, pcdev->base + CICR0); > - > - /* Stop DMA */ > - DCSR(channel) = 0; > - /* Reset the FIFOs */ > - cifr = __raw_readl(pcdev->base + CIFR) | CIFR_RESET_F; > - __raw_writel(cifr, pcdev->base + CIFR); > - /* Enable End-Of-Frame Interrupt */ > - cicr0 &= ~CICR0_EOFM; > - __raw_writel(cicr0, pcdev->base + CICR0); > - /* Restart the Capture Interface */ > - __raw_writel(cicr0 | CICR0_ENB, pcdev->base + CICR0); > - goto out; > - } > - > vb = &pcdev->active->vb; > buf = container_of(vb, struct pxa_buffer, vb); > WARN_ON(buf->inwork || list_empty(&vb->queue)); > - dev_dbg(pcdev->dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__, > - vb, vb->baddr, vb->bsize); > > - buf->active_dma &= ~act_dma; > - if (!buf->active_dma) > - pxa_camera_wakeup(pcdev, vb, buf); > + dev_dbg(pcdev->dev, "%s channel=%d %s%s(vb=0x%p) dma.desc=%x\n", > + __func__, channel, status & DCSR_STARTINTR ? "SOF " : "", > + status & DCSR_ENDINTR ? "EOF " : "", vb, DDADR(channel)); > + > + if (status & DCSR_ENDINTR) { > + if (camera_status & overrun) { > + 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); > + } > > out: > spin_unlock_irqrestore(&pcdev->lock, flags); > @@ -859,12 +884,11 @@ static irqreturn_t pxa_camera_irq(int irq, void *data) > __raw_writel(status, pcdev->base + CISR); > > if (status & CISR_EOF) { > - int i; > - for (i = 0; i < pcdev->channels; i++) { > - DDADR(pcdev->dma_chans[i]) = > - pcdev->active->dmas[i].sg_dma; > - DCSR(pcdev->dma_chans[i]) = DCSR_RUN; > - } > + pcdev->active = list_first_entry(&pcdev->capture, > + struct pxa_buffer, vb.queue); > + > + pxa_dma_start_channels(pcdev); > + > cicr0 = __raw_readl(pcdev->base + CICR0) | CICR0_EOFM; > __raw_writel(cicr0, pcdev->base + CICR0); > } > @@ -1404,18 +1428,8 @@ static int pxa_camera_resume(struct soc_camera_device *icd) > ret = pcdev->icd->ops->resume(pcdev->icd); > > /* Restart frame capture if active buffer exists */ > - if (!ret && pcdev->active) { > - unsigned long cifr, cicr0; > - > - /* Reset the FIFOs */ > - cifr = __raw_readl(pcdev->base + CIFR) | CIFR_RESET_F; > - __raw_writel(cifr, pcdev->base + CIFR); > - > - cicr0 = __raw_readl(pcdev->base + CICR0); > - cicr0 &= ~CICR0_EOFM; /* Enable End-Of-Frame Interrupt */ > - cicr0 |= CICR0_ENB; /* Restart the Capture Interface */ > - __raw_writel(cicr0, pcdev->base + CICR0); > - } > + if (!ret && pcdev->active) > + pxa_camera_start_capture(pcdev); > > return ret; > } > -- > 1.5.6.5 > 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: >> + * Returns 0 or -ENOMEM si no coherent memory is available > > Let's stay with English for now:-) s/si/if/ Oups ... sorry ... the froggish touch is back :) > >> */ >> static int pxa_init_dma_channel(struct pxa_camera_dev *pcdev, >> struct pxa_buffer *buf, >> @@ -369,7 +369,8 @@ static int pxa_init_dma_channel(struct pxa_camera_dev *pcdev, >> pxa_dma->sg_cpu[i].dsadr = pcdev->res->start + cibr; >> pxa_dma->sg_cpu[i].dtadr = sg_dma_address(sg) + offset; >> pxa_dma->sg_cpu[i].dcmd = >> - DCMD_FLOWSRC | DCMD_BURST8 | DCMD_INCTRGADDR | xfer_len; >> + DCMD_FLOWSRC | DCMD_BURST8 | DCMD_INCTRGADDR | xfer_len >> + | ((i == 0) ? DCMD_STARTIRQEN : 0); > > If DCMD_STARTIRQEN is still for debugging only, maybe put it under > > #ifdef DEBUG > if (!i) > pxa_dma->sg_cpu[i].dcmd |= DCMD_STARTIRQEN; > #endif OK. Will amend. >> +static void pxa_dma_update_sg_tail(struct pxa_camera_dev *pcdev, >> + struct pxa_buffer *buf) >> +{ >> + int i; >> + >> + for (i = 0; i < pcdev->channels; i++) { >> + pcdev->sg_tail[i] = buf->dmas[i].sg_cpu + buf->dmas[i].sglen; >> + pcdev->sg_tail[i]->ddadr = DDADR_STOP; > > Do I understand it right, assuming capture is running, i.e., active != > NULL: > > before your patch > > sg_tail points to the last real DMA descriptor > the last real DMA descriptor has DDADR_STOP > on queuing of the next buffer we > 1. stop DMA > 2. link the last real descriptor to the new first descriptor > 3. allocate an additional dummy descriptor, fill it with DMA engine's > current state and use it to > 4. re-start DMA Yes, but you forget : 5. link the last new buffer descriptor (the called dummy buffer) to the running chain. I see it that way, after former pxa_video_queue() : +----------+-----------+------------+ | First vb | Second vb | Third vb | | +----^-----+-----------+-----------|+ | | +----------------+ | +----> | New vb | dummy | | +------------|---+ | | +-------------------------------------------------+ This is my understanding. The DMA is restarted at the dummy descriptor, which re-reads the current DMA descriptor (is that correct, if 16 bytes were already transfered ?), then comes back to the head of DMA chain. Then first vb is finished, then second and third, and then new vb is re-filled. Would you comment to see where I'm wrong please ? > after your patch > > sg_tail points to the additional DMA descriptor Which additional ? Do you mean "the last DMA descriptor of the last video buffer queued which never transfers any data" ? (which is what I point it at, yes) > the last valid DMA descriptor points to the additional descriptor > the additional descriptor has DDADR_STOP Yes. > on queuing of the next buffer > 1. stop DMA > 2. the additional dummy descriptor at the tail of the current chain is > reconfigured to point to the new start Yes. > 3. pxa_dma_start_channels() is called, which drops the current partial > transfer and re-starts the frame?... Yes, that is wrong. The trick is, if I restart the DMA channel where it was, I remember having my "select stalled" message. I see it that way, after new pxa_video_queue() : +----------+-----------+------------+ | First vb | Second vb | Third vb | | +----------+-----------+-----------|+ ^ | +----------------+ | +----> | New vb | dummy | \restart +----------------+ > If I am right, this doesn't seem right. If I am wrong, please, explain and > add explanatory comments, so, the next one (or the same one 2 months > later) does not have to spend time trying to figure out. Well, you've got a point. There is something to dig here. By experiment, it is working. But I will search why, as my patch does restart the frame :( I will investigate : - if stopping the DMA chain and restarting in the middle of a DMA transfer (ie. in the middle of the 4096 bytes, on byte 2040 for example) does work. - how my DMA chain does work. As a matter of fact, before this patch, I had a pxa_dma_restart_channels() called in pxa_videobuf_queue(), which just "restarted" the DMA channel without touching the DADR() register. I will search why this wasn't working. >> +static void pxa_camera_start_capture(struct pxa_camera_dev *pcdev) >> +{ >> + unsigned long cicr0, cifr; >> + >> + dev_dbg(pcdev->dev, "%s\n", __func__); > > I originally had a "reset the FIFOs" comment here, wouldn't hurt to add it > now too. Sorry, I'll reput it there. Will amend. > >> + cifr = __raw_readl(pcdev->base + CIFR) | CIFR_RESET_F; >> + __raw_writel(cifr, pcdev->base + CIFR); >> + >> + cicr0 = __raw_readl(pcdev->base + CICR0) | CICR0_ENB | CISR_IFO_0 >> + | CISR_IFO_1 | CISR_IFO_2; > > CISR_* flags have nothing to do with the CICR register. Right, good catch. I'll remove all the CISR* stuff. I must have been confused, it's the CIFR_RESET_F which was meant there (fifo flush). > It is nice to synchronise on a frame start, but you're relying on being > "fast," i.e., on servicing the End of Frame interrupt between the two > frames and having enough time to configure DMA. With smaller frames with > short inter-frame times this can be difficult, I think. But, well, that's > the best we can do, I guess. And yes, I know, I'm already doing this in > the overrun case. Yep. But you're right. I'll expand my testcases to 32x32 frames, and bombard my PXA with interrupts, at low cpufreq. We'll see what happens then :) >> @@ -666,19 +698,23 @@ static void pxa_camera_dma_irq(int channel, struct pxa_camera_dev *pcdev, >> unsigned long flags; >> u32 status, camera_status, overrun; >> struct videobuf_buffer *vb; >> - unsigned long cifr, cicr0; >> >> spin_lock_irqsave(&pcdev->lock, flags); >> >> status = DCSR(channel); >> - DCSR(channel) = status | DCSR_ENDINTR; >> + DCSR(channel) = status | DCSR_STARTINTR | DCSR_ENDINTR; > > Now as I look at it, actually, this is racy. If for whatever reason we > entered here without ENDINTR set, so status & DCSR_ENDINTR == 0, then it > got immediately set and we clear it, thus we lose it. I think, there's no > reason here not to use the standard > > irq_reason = read(IRQ_REASON_REG); > write(irq_reason, IRQ_REASON_REG); Right. It is racy. Will amend. OK, I have work to do on that one. Would please just check my understanding of the chain (the superb ascii-art I draw :)), so that we could speak on the same ground. That will help me understand better. 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
I'll answer all points tomorrow, but so you can start thinking about it earlier and get used to it:-), I'll explain the current driver behaviour now: On Mon, 9 Mar 2009, Robert Jarzmik wrote: > Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes: > > > before your patch > > > > sg_tail points to the last real DMA descriptor > > the last real DMA descriptor has DDADR_STOP > > on queuing of the next buffer we > > 1. stop DMA > > 2. link the last real descriptor to the new first descriptor > > 3. allocate an additional dummy descriptor, fill it with DMA engine's > > current state and use it to > > 4. re-start DMA > Yes, but you forget : > 5. link the last new buffer descriptor (the called dummy buffer) to the > running chain. > > I see it that way, after former pxa_video_queue() : > > +----------+-----------+------------+ > | First vb | Second vb | Third vb | | > +----^-----+-----------+-----------|+ > | | +----------------+ > | +----> | New vb | dummy | > | +------------|---+ > | | > +-------------------------------------------------+ > > This is my understanding. The DMA is restarted at the dummy descriptor, which > re-reads the current DMA descriptor (is that correct, if 16 bytes were already > transfered ?), then comes back to the head of DMA chain. > Then first vb is finished, then second and third, and then new vb is re-filled. > > Would you comment to see where I'm wrong please ? IIUYC, you mean, that the dummy descriptor re-starts the interrupted transfer from the beginning. This is wrong: With the current code, let's say we capture frames 80x60=4800 at 1 byte per pixel - monochrome or Bayer. Then we allocate 3 sg-elements: static int pxa_init_dma_channel() { ... pxa_dma->sg_size = (sglen + 1) * sizeof(struct pxa_dma_desc); pxa_dma->sg_cpu = dma_alloc_coherent(pcdev->dev, pxa_dma->sg_size, &pxa_dma->sg_dma, GFP_KERNEL); ... and they are initialised pxa_dma->sg_cpu[0].dsadr = pcdev->res->start + cibr; pxa_dma->sg_cpu[0].dtadr = sg_dma_address(&sg[0]); pxa_dma->sg_cpu[0].dcmd = DCMD_FLOWSRC | DCMD_BURST8 | DCMD_INCTRGADDR | 4096; pxa_dma->sg_cpu[0].ddadr = pxa_dma->sg_dma + sizeof(struct pxa_dma_desc); pxa_dma->sg_cpu[1].dsadr = pcdev->res->start + cibr; pxa_dma->sg_cpu[1].dtadr = sg_dma_address(&sg[1]); pxa_dma->sg_cpu[1].dcmd = DCMD_FLOWSRC | DCMD_BURST8 | DCMD_INCTRGADDR | 704; pxa_dma->sg_cpu[1].ddadr = pxa_dma->sg_dma + 2 * sizeof(struct pxa_dma_desc); pxa_dma->sg_cpu[1].ddadr = DDADR_STOP; pxa_dma->sg_cpu[1].dcmd |= DCMD_ENDIRQEN; Notice, sg_cpu[2] (the dummy) is not used yet. So, in normal case the DMA engine would process 0, 1, and stop. } Now, as this buffer is queued in let's say, the previou pxa_videobuf_queue() { ... With locked interrupts we stop the DMA engine, and hope, that there's still enough space in the FIFO left and that we won't be getting an overrun... /* Stop DMA engine */ DCSR(pcdev->dma_chans[i]) = 0; From now on we have to be fast until we re-enable DMA. /* Add the descriptors we just initialized to the currently running chain */ pcdev->sg_tail[i]->ddadr = buf_dma->sg_dma; pcdev->sg_tail[i] = buf_dma->sg_cpu + buf_dma->sglen - 1; See, sg_tail is set to point to the last valid (not dummy) PXA DMA descriptor, i.e., to sg_cpu[1] in our example. So, before it also pointed to the last valid descriptor from the previous buffer, which now links to the beginning of our new buffer. Now, this is the trick: we use a dummy descriptor (actually, the one from the new video buffer, but it doesn't matter) to set up a descriptor to finish the interrupted transfer. For this we set dtadr to the _current_ DTADR to continue filling the buffer exactly where we stopped. /* Setup a dummy descriptor with the DMA engines current * state */ buf_dma->sg_cpu[nents].dsadr = pcdev->res->start + 0x28 + i*8; /* CIBRx */ buf_dma->sg_cpu[nents].dtadr = DTADR(pcdev->dma_chans[i]); buf_dma->sg_cpu[nents].dcmd = DCMD(pcdev->dma_chans[i]); Now we just check where we should link this our linking partial transfer descriptor - either to the first descriptor in our new buffer, if DMA was currently processing the last descriptor currently queued, or to the same descriptor to which it used to be linked. if (DDADR(pcdev->dma_chans[i]) == DDADR_STOP) { /* The DMA engine is on the last descriptor, set the next descriptors address to the descriptors we just initialized */ buf_dma->sg_cpu[nents].ddadr = buf_dma->sg_dma; } else { buf_dma->sg_cpu[nents].ddadr = DDADR(pcdev->dma_chans[i]); } Now we restart DMA at our "dummy" descriptor. Actually, it is not dummy any more, it is "linking," "partial," or whatever you call it. /* The next descriptor is the dummy descriptor */ DDADR(pcdev->dma_chans[i]) = buf_dma->sg_dma + nents * sizeof(struct pxa_dma_desc); DCSR(pcdev->dma_chans[i]) = DCSR_RUN; 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 54df071..2d79ded 100644 --- a/drivers/media/video/pxa_camera.c +++ b/drivers/media/video/pxa_camera.c @@ -325,7 +325,7 @@ static int calculate_dma_sglen(struct scatterlist *sglist, int sglen, * Prepares the pxa dma descriptors to transfer one camera channel. * Beware sg_first and sg_first_ofs are both input and output parameters. * - * Returns 0 + * Returns 0 or -ENOMEM si no coherent memory is available */ static int pxa_init_dma_channel(struct pxa_camera_dev *pcdev, struct pxa_buffer *buf, @@ -369,7 +369,8 @@ static int pxa_init_dma_channel(struct pxa_camera_dev *pcdev, pxa_dma->sg_cpu[i].dsadr = pcdev->res->start + cibr; pxa_dma->sg_cpu[i].dtadr = sg_dma_address(sg) + offset; pxa_dma->sg_cpu[i].dcmd = - DCMD_FLOWSRC | DCMD_BURST8 | DCMD_INCTRGADDR | xfer_len; + DCMD_FLOWSRC | DCMD_BURST8 | DCMD_INCTRGADDR | xfer_len + | ((i == 0) ? DCMD_STARTIRQEN : 0); pxa_dma->sg_cpu[i].ddadr = pxa_dma->sg_dma + (i + 1) * sizeof(struct pxa_dma_desc); @@ -516,6 +517,97 @@ out: return ret; } +/** + * pxa_dma_start_channels - start DMA channel for active buffer + * @pcdev: pxa camera device + * + * Initialize DMA channels to the beginning of the active video buffer, and + * start these channels. + */ +static void pxa_dma_start_channels(struct pxa_camera_dev *pcdev) +{ + int i; + struct pxa_buffer *active; + + active = pcdev->active; + + for (i = 0; i < pcdev->channels; i++) { + dev_dbg(pcdev->dev, "%s (channel=%d) ddadr=%08x\n", __func__, + i, active->dmas[i].sg_dma); + DDADR(pcdev->dma_chans[i]) = active->dmas[i].sg_dma; + DCSR(pcdev->dma_chans[i]) = DCSR_RUN; + } +} + +static void pxa_dma_stop_channels(struct pxa_camera_dev *pcdev) +{ + int i; + + for (i = 0; i < pcdev->channels; i++) { + dev_dbg(pcdev->dev, "%s (channel=%d)\n", __func__, i); + DCSR(pcdev->dma_chans[i]) = 0; + } +} + +static void pxa_dma_update_sg_tail(struct pxa_camera_dev *pcdev, + struct pxa_buffer *buf) +{ + int i; + + for (i = 0; i < pcdev->channels; i++) { + pcdev->sg_tail[i] = buf->dmas[i].sg_cpu + buf->dmas[i].sglen; + pcdev->sg_tail[i]->ddadr = DDADR_STOP; + } +} + +static void pxa_dma_add_tail_buf(struct pxa_camera_dev *pcdev, + struct pxa_buffer *buf) +{ + int i; + + for (i = 0; i < pcdev->channels; i++) { + if (!pcdev->sg_tail[i]) + continue; + pcdev->sg_tail[i]->ddadr = buf->dmas[i].sg_dma; + } + + pxa_dma_update_sg_tail(pcdev, buf); +} + +/** + * pxa_camera_start_capture - start video capturing + * @pcdev: camera device + * + * Launch capturing. DMA channels should not be active yet. They should get + * activated at the end of frame interrupt, to capture only whole frames, and + * never begin the capture of a partial frame. + */ +static void pxa_camera_start_capture(struct pxa_camera_dev *pcdev) +{ + unsigned long cicr0, cifr; + + dev_dbg(pcdev->dev, "%s\n", __func__); + cifr = __raw_readl(pcdev->base + CIFR) | CIFR_RESET_F; + __raw_writel(cifr, pcdev->base + CIFR); + + cicr0 = __raw_readl(pcdev->base + CICR0) | CICR0_ENB | CISR_IFO_0 + | CISR_IFO_1 | CISR_IFO_2; + cicr0 &= ~CICR0_EOFM; + __raw_writel(cicr0, pcdev->base + CICR0); +} + +static void pxa_camera_stop_capture(struct pxa_camera_dev *pcdev) +{ + unsigned long cicr0; + + pxa_dma_stop_channels(pcdev); + + cicr0 = __raw_readl(pcdev->base + CICR0) & ~CICR0_ENB; + __raw_writel(cicr0, pcdev->base + CICR0); + + dev_dbg(pcdev->dev, "%s\n", __func__); +} + static void pxa_videobuf_queue(struct videobuf_queue *vq, struct videobuf_buffer *vb) { @@ -523,81 +615,23 @@ static void pxa_videobuf_queue(struct videobuf_queue *vq, struct soc_camera_host *ici = to_soc_camera_host(icd->dev.parent); struct pxa_camera_dev *pcdev = ici->priv; struct pxa_buffer *buf = container_of(vb, struct pxa_buffer, vb); - struct pxa_buffer *active; unsigned long flags; - int i; - dev_dbg(&icd->dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__, - vb, vb->baddr, vb->bsize); - spin_lock_irqsave(&pcdev->lock, flags); + dev_dbg(&icd->dev, "%s (vb=0x%p) 0x%08lx %d active=%p\n", __func__, + vb, vb->baddr, vb->bsize, pcdev->active); + spin_lock_irqsave(&pcdev->lock, flags); list_add_tail(&vb->queue, &pcdev->capture); vb->state = VIDEOBUF_ACTIVE; - active = pcdev->active; - - if (!active) { - unsigned long cifr, cicr0; - - cifr = __raw_readl(pcdev->base + CIFR) | CIFR_RESET_F; - __raw_writel(cifr, pcdev->base + CIFR); - - for (i = 0; i < pcdev->channels; i++) { - DDADR(pcdev->dma_chans[i]) = buf->dmas[i].sg_dma; - DCSR(pcdev->dma_chans[i]) = DCSR_RUN; - pcdev->sg_tail[i] = buf->dmas[i].sg_cpu + buf->dmas[i].sglen - 1; - } - pcdev->active = buf; + pxa_dma_stop_channels(pcdev); + pxa_dma_add_tail_buf(pcdev, buf); - cicr0 = __raw_readl(pcdev->base + CICR0) | CICR0_ENB; - __raw_writel(cicr0, pcdev->base + CICR0); - } else { - struct pxa_cam_dma *buf_dma; - struct pxa_cam_dma *act_dma; - int nents; - - for (i = 0; i < pcdev->channels; i++) { - buf_dma = &buf->dmas[i]; - act_dma = &active->dmas[i]; - nents = buf_dma->sglen; - - /* Stop DMA engine */ - DCSR(pcdev->dma_chans[i]) = 0; - - /* Add the descriptors we just initialized to - the currently running chain */ - pcdev->sg_tail[i]->ddadr = buf_dma->sg_dma; - pcdev->sg_tail[i] = buf_dma->sg_cpu + buf_dma->sglen - 1; - - /* Setup a dummy descriptor with the DMA engines current - * state - */ - buf_dma->sg_cpu[nents].dsadr = - pcdev->res->start + 0x28 + i*8; /* CIBRx */ - buf_dma->sg_cpu[nents].dtadr = - DTADR(pcdev->dma_chans[i]); - buf_dma->sg_cpu[nents].dcmd = - DCMD(pcdev->dma_chans[i]); - - if (DDADR(pcdev->dma_chans[i]) == DDADR_STOP) { - /* The DMA engine is on the last - descriptor, set the next descriptors - address to the descriptors we just - initialized */ - buf_dma->sg_cpu[nents].ddadr = buf_dma->sg_dma; - } else { - buf_dma->sg_cpu[nents].ddadr = - DDADR(pcdev->dma_chans[i]); - } - - /* The next descriptor is the dummy descriptor */ - DDADR(pcdev->dma_chans[i]) = buf_dma->sg_dma + nents * - sizeof(struct pxa_dma_desc); - - DCSR(pcdev->dma_chans[i]) = DCSR_RUN; - } - } + if (!pcdev->active) + pxa_camera_start_capture(pcdev); + else + pxa_dma_start_channels(pcdev); spin_unlock_irqrestore(&pcdev->lock, flags); } @@ -635,7 +669,7 @@ static void pxa_camera_wakeup(struct pxa_camera_dev *pcdev, struct videobuf_buffer *vb, struct pxa_buffer *buf) { - unsigned long cicr0; + int i; /* _init is used to debug races, see comment in pxa_camera_reqbufs() */ list_del_init(&vb->queue); @@ -643,15 +677,13 @@ static void pxa_camera_wakeup(struct pxa_camera_dev *pcdev, do_gettimeofday(&vb->ts); vb->field_count++; wake_up(&vb->done); + dev_dbg(pcdev->dev, "%s dequeud buffer (vb=0x%p)\n", __func__, vb); if (list_empty(&pcdev->capture)) { + pxa_camera_stop_capture(pcdev); pcdev->active = NULL; - DCSR(pcdev->dma_chans[0]) = 0; - DCSR(pcdev->dma_chans[1]) = 0; - DCSR(pcdev->dma_chans[2]) = 0; - - cicr0 = __raw_readl(pcdev->base + CICR0) & ~CICR0_ENB; - __raw_writel(cicr0, pcdev->base + CICR0); + for (i = 0; i < pcdev->channels; i++) + pcdev->sg_tail[i] = NULL; return; } @@ -666,19 +698,23 @@ static void pxa_camera_dma_irq(int channel, struct pxa_camera_dev *pcdev, unsigned long flags; u32 status, camera_status, overrun; struct videobuf_buffer *vb; - unsigned long cifr, cicr0; spin_lock_irqsave(&pcdev->lock, flags); status = DCSR(channel); - DCSR(channel) = status | DCSR_ENDINTR; + DCSR(channel) = status | DCSR_STARTINTR | DCSR_ENDINTR; + + camera_status = __raw_readl(pcdev->base + CISR); + overrun = CISR_IFO_0; + if (pcdev->channels == 3) + overrun |= CISR_IFO_1 | CISR_IFO_2; if (status & DCSR_BUSERR) { dev_err(pcdev->dev, "DMA Bus Error IRQ!\n"); goto out; } - if (!(status & DCSR_ENDINTR)) { + if (!(status & (DCSR_ENDINTR | DCSR_STARTINTR))) { dev_err(pcdev->dev, "Unknown DMA IRQ source, " "status: 0x%08x\n", status); goto out; @@ -689,38 +725,27 @@ static void pxa_camera_dma_irq(int channel, struct pxa_camera_dev *pcdev, goto out; } - camera_status = __raw_readl(pcdev->base + CISR); - overrun = CISR_IFO_0; - if (pcdev->channels == 3) - overrun |= CISR_IFO_1 | CISR_IFO_2; - if (camera_status & overrun) { - dev_dbg(pcdev->dev, "FIFO overrun! CISR: %x\n", camera_status); - /* Stop the Capture Interface */ - cicr0 = __raw_readl(pcdev->base + CICR0) & ~CICR0_ENB; - __raw_writel(cicr0, pcdev->base + CICR0); - - /* Stop DMA */ - DCSR(channel) = 0; - /* Reset the FIFOs */ - cifr = __raw_readl(pcdev->base + CIFR) | CIFR_RESET_F; - __raw_writel(cifr, pcdev->base + CIFR); - /* Enable End-Of-Frame Interrupt */ - cicr0 &= ~CICR0_EOFM; - __raw_writel(cicr0, pcdev->base + CICR0); - /* Restart the Capture Interface */ - __raw_writel(cicr0 | CICR0_ENB, pcdev->base + CICR0); - goto out; - } - vb = &pcdev->active->vb; buf = container_of(vb, struct pxa_buffer, vb); WARN_ON(buf->inwork || list_empty(&vb->queue)); - dev_dbg(pcdev->dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__, - vb, vb->baddr, vb->bsize); - buf->active_dma &= ~act_dma; - if (!buf->active_dma) - pxa_camera_wakeup(pcdev, vb, buf); + dev_dbg(pcdev->dev, "%s channel=%d %s%s(vb=0x%p) dma.desc=%x\n", + __func__, channel, status & DCSR_STARTINTR ? "SOF " : "", + status & DCSR_ENDINTR ? "EOF " : "", vb, DDADR(channel)); + + if (status & DCSR_ENDINTR) { + if (camera_status & overrun) { + 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); + } out: spin_unlock_irqrestore(&pcdev->lock, flags); @@ -859,12 +884,11 @@ static irqreturn_t pxa_camera_irq(int irq, void *data) __raw_writel(status, pcdev->base + CISR); if (status & CISR_EOF) { - int i; - for (i = 0; i < pcdev->channels; i++) { - DDADR(pcdev->dma_chans[i]) = - pcdev->active->dmas[i].sg_dma; - DCSR(pcdev->dma_chans[i]) = DCSR_RUN; - } + pcdev->active = list_first_entry(&pcdev->capture, + struct pxa_buffer, vb.queue); + + pxa_dma_start_channels(pcdev); + cicr0 = __raw_readl(pcdev->base + CICR0) | CICR0_EOFM; __raw_writel(cicr0, pcdev->base + CICR0); } @@ -1404,18 +1428,8 @@ static int pxa_camera_resume(struct soc_camera_device *icd) ret = pcdev->icd->ops->resume(pcdev->icd); /* Restart frame capture if active buffer exists */ - if (!ret && pcdev->active) { - unsigned long cifr, cicr0; - - /* Reset the FIFOs */ - cifr = __raw_readl(pcdev->base + CIFR) | CIFR_RESET_F; - __raw_writel(cifr, pcdev->base + CIFR); - - cicr0 = __raw_readl(pcdev->base + CICR0); - cicr0 &= ~CICR0_EOFM; /* Enable End-Of-Frame Interrupt */ - cicr0 |= CICR0_ENB; /* Restart the Capture Interface */ - __raw_writel(cicr0, pcdev->base + CICR0); - } + if (!ret && pcdev->active) + pxa_camera_start_capture(pcdev); return ret; }
The DMA transfers in pxa_camera showed some weaknesses in multiple queued buffers context : - poll/select problem The order between list pcdev->capture and DMA chain was not the same. This creates a discrepancy between video buffers marked as "done" by the IRQ handler, and the really finished video buffer. The bug shows up with capture_example tool from v4l2 hg tree. The process just "stalls" on a "select timeout". The key problem is in pxa_videobuf_queue(), where the queued buffer is chained before the active buffer, while it should have been the active buffer first, and queued buffer tailed after. - multiple buffers DMA starting When multiple buffers were queued, the DMA channels were always started right away. This is not optimal, as a special case appears when the first EOF was not yet reached, and the DMA channels were prematurely started. - Maintainability DMA code was a bit obfuscated. Rationalize the code to be easily maintainable by anyone. This patch attemps to address these issues. Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr> --- drivers/media/video/pxa_camera.c | 264 ++++++++++++++++++++------------------ 1 files changed, 139 insertions(+), 125 deletions(-)