On Wed, 22 Sep 2010, Janusz Krzysztofik wrote:
> Wednesday 22 September 2010 01:23:22 Guennadi Liakhovetski napisaÅ(a):
> > On Sat, 11 Sep 2010, Janusz Krzysztofik wrote:
> > > +
> > > + vb = &buf->vb;
> > > + if (waitqueue_active(&vb->done)) {
> > > + if (!pcdev->ready && result != VIDEOBUF_ERROR)
> > > + /*
> > > + * No next buffer has been entered into the DMA
> > > + * programming register set on time, so best we can do
> > > + * is stopping the capture before last DMA block,
> > > + * whether our CONTIG mode whole buffer or its last
> > > + * sgbuf in SG mode, gets overwritten by next frame.
> > > + */
> >
> > Hm, why do you think it's a good idea? This specific buffer completed
> > successfully, but you want to fail it just because the next buffer is
> > missing? Any specific reason for this?
>
> Maybe my comment is not clear enough, but the below suspend_capture() doesn't
> indicate any failure on a frame just captured. It only prevents the frame
> from being overwritten by the already autoreinitialized DMA engine, pointing
> back to the same buffer once again.
>
> > Besides, you seem to also be
> > considering the possibility of your ->ready == NULL, but the queue
> > non-empty, in which case you just take the next buffer from the queue and
> > continue with it. Why error out in this case?
>
> pcdev->ready == NULL means no buffer was available when it was time to put it
> into the DMA programming register set.
But how? Buffers are added onto the list in omap1_videobuf_queue() under
spin_lock_irqsave(); and there you also check ->ready and fill it in. In
your completion you set ->ready = NULL, but then also call
prepare_next_vb() to get the next buffer from the list - if there are any,
so, how can it be NULL with a non-empty list?
> As a result, a next DMA transfer has
> just been autoreinitialized with the same buffer parameters as before. To
> protect the buffer from being overwriten unintentionally, we have to stop the
> DMA transfer as soon as possible, hopefully before the sensor starts sending
> out next frame data.
>
> If a new buffer has been queued meanwhile, best we can do is stopping
> everything, programming the DMA with the new buffer, and setting up for a new
> transfer hardware auto startup on nearest frame start, be it the next one if
> we are lucky enough, or one after the next if we are too slow.
>
> > And even if also the queue
> > is empty - still not sure, why.
>
> I hope the above explanation clarifies why.
>
> I'll try to rework the above comment to be more clear, OK? Any hints?
> > > linux-2.6.36-rc3.orig/include/media/omap1_camera.h 2010-09-03
> > > 22:34:02.000000000 +0200 +++
> > > linux-2.6.36-rc3/include/media/omap1_camera.h 2010-09-08
> > > 23:41:12.000000000 +0200 @@ -0,0 +1,35 @@
> > > +/*
> > > + * Header for V4L2 SoC Camera driver for OMAP1 Camera Interface
> > > + *
> > > + * Copyright (C) 2010, Janusz Krzysztofik <jkrzyszt@xxxxxxxxxxxx>
> > > + *
> > > + * This program is free software; you can redistribute it and/or modify
> > > + * it under the terms of the GNU General Public License version 2 as
> > > + * published by the Free Software Foundation.
> > > + */
> > > +
> > > +#ifndef __MEDIA_OMAP1_CAMERA_H_
> > > +#define __MEDIA_OMAP1_CAMERA_H_
> > > +
> > > +#include <linux/bitops.h>
> > > +
> > > +#define OMAP1_CAMERA_IOSIZE 0x1c
> > > +
> > > +enum omap1_cam_vb_mode {
> > > + CONTIG = 0,
> > > + SG,
> > > +};
> >
> > See above - are these needed here?
> >
> > > +
> > > +#define OMAP1_CAMERA_MIN_BUF_COUNT(x) ((x) == CONTIG ? 3 : 2)
> >
> > ditto
>
> I moved them both over to the header file because I was using the
> OMAP1_CAMERA_MIN_BUF_COUNT(CONTIG) macro once from the platform code in order
> to calculate the buffer size when calling the then NAKed
> dma_preallocate_coherent_memory(). Now I could put them back into the driver
> code, but if we ever get back to the concept of preallocating a contignuos
> piece of memory from the platform init code, we might need them back here, so
> maybe I should rather keep them, only rename the two enum values using a
> distinct name space. What do you think is better for now?
Yeah, up to you, I'd say, but if you decide to keep them in the header,
please, use a namespace.
I'm satisfied with your answers to the rest of my questions / comments:)
Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
