Guennadi Liakhovetski <g.liakhovetski@xxxxxx> writes:
> On Tue, 10 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.
>
> Double-check. I still do not see how the order can be swapped. But don't
> worry, this doesn't diminish the value of your work, I'm just trying to be
> fair to the existing driver:-)
Yes :) I thought I had sent a mail to apologize for the remaining comment ... I
have fully removed the order issue, I only left the poll/select issue.
And yes, it's not fair to the previous code, you're perfectly right.
>> @@ -324,7 +324,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
>
> s/si/if/ s'il vous plait:-)
Argh. I was sure I had amended that ... urg.
> Nice, how about putting this in Documentation/video4linux/pxa_camera.txt
> and a reference to it in the comment?
Yes, it will make the code lighter, won't it ? I'll need a bit of help to
correct my english here ...
>> + 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;
> This function is now called "live" with running DMA, and you first append
> the chain, and only then terminate it... It should be ok because it is
> done with switched off IRQs, and DMA must be still at tail - 1 to
> automatically continue onto the appended chain, so, you should have enough
> time in 100% of cases, still it would look better to first terminate the
> chain and then append it.
Correct. I'll invert the 2 assignments.
>
>> +static void pxa_camera_start_capture(struct pxa_camera_dev *pcdev)
>> +{
>> + unsigned long cicr0, cifr;
>> +
>> + dev_dbg(pcdev->dev, "%s\n", __func__);
>
> <quote>
> I originally had a "reset the FIFOs" comment here, wouldn't hurt to add it
> now too.
> </quote>
Yes. I re-added it. I added also your "Enable End-Of-Frame Interrupt" back.
>> @@ -524,81 +659,19 @@ 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);
>
> I can understand adding an empty line between dev_dbg() and
> spin_lock_irqsave(), but I do not understand why you removed one between
> spin_lock_irqsave() and list_add_tail().
My bad. I had a mdelay() around to test the corner case :) I'll amend that.
Cheers.
--
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
