Re: [PATCH v2 1/6] SoC Camera: add driver for OMAP1 camera interface
On Thu, 23 Sep 2010, Janusz Krzysztofik wrote:
> Thursday 23 September 2010 15:33:54 Guennadi Liakhovetski napisał(a):
> > 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.
>
> Guennadi,
> Yes, but only if pcdev->active is NULL, ie. both DMA and FIFO are idle, never
> if active:
>
> + list_add_tail(&vb->queue, &pcdev->capture);
> + vb->state = VIDEOBUF_QUEUED;
> +
> + if (pcdev->active)
> + return;
>
> Since the transfer of the DMA programming register set content to the DMA
> working register set is done automatically by the DMA hardware, this can
> pretty well happen while I keep the lock here, so I can't be sure if it's not
> too late for entering new data into the programming register set. Then, I
> decided that this operation should be done only just after the DMA interrupt
> occured, ie. the current DMA programming register set content has just been
> used and can be overwriten.
>
> I'll emphasize the above return; with a comment.
Ok
> > 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?
>
> It happens after the above mentioned prepare_next_vb() gets nothing from an
> empty queue, so nothing is entered into the DMA programming register set,
> only the last, just activated, buffer is processed, then
> omap1_videobuf_queue() puts a new buffer into the queue while the active
> buffer is still filled in, and finally the DMA ISR is called on this last
> active buffer completion.
>
> I hope this helps.
Let's assume it does:) You seem to really understand how this is working
and even be willing to document the driver, thus making it possibly the
best documented soc-camera related piece of software;)
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-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/6] SoC Camera: add driver for OMAP1 camera interface
Thursday 23 September 2010 15:33:54 Guennadi Liakhovetski napisał(a):
> 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.
Guennadi,
Yes, but only if pcdev->active is NULL, ie. both DMA and FIFO are idle, never
if active:
+ list_add_tail(&vb->queue, &pcdev->capture);
+ vb->state = VIDEOBUF_QUEUED;
+
+ if (pcdev->active)
+ return;
Since the transfer of the DMA programming register set content to the DMA
working register set is done automatically by the DMA hardware, this can
pretty well happen while I keep the lock here, so I can't be sure if it's not
too late for entering new data into the programming register set. Then, I
decided that this operation should be done only just after the DMA interrupt
occured, ie. the current DMA programming register set content has just been
used and can be overwriten.
I'll emphasize the above return; with a comment.
> 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?
It happens after the above mentioned prepare_next_vb() gets nothing from an
empty queue, so nothing is entered into the DMA programming register set,
only the last, just activated, buffer is processed, then
omap1_videobuf_queue() puts a new buffer into the queue while the active
buffer is still filled in, and finally the DMA ISR is called on this last
active buffer completion.
I hope this helps.
> > 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.0 +0200 +++
> > > > linux-2.6.36-rc3/include/media/omap1_camera.h 2010-09-08
> > > > 23:41:12.0 +0200 @@ -0,0 +1,35 @@
> > > > +/*
> > > > + * Header for V4L2 SoC Camera driver for OMAP1 Camera Interface
> > > > + *
> > > > + * Copyright (C) 2010, Janusz Krzysztofik
> > > > + *
> > > > + * 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
> > > > +
> > > > +#define OMAP1_CAMERA_IOSIZE0x1c
> > > > +
> > > > +enum omap1_cam_vb_mode {
> >
Re: [PATCH v2 1/6] SoC Camera: add driver for OMAP1 camera interface
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.h2010-09-03
> > > 22:34:02.0 +0200 +++
> > > linux-2.6.36-rc3/include/media/omap1_camera.h 2010-09-08
> > > 23:41:12.0 +0200 @@ -0,0 +1,35 @@
> > > +/*
> > > + * Header for V4L2 SoC Camera driver for OMAP1 Camera Interface
> > > + *
> > > + * Copyright (C) 2010, Janusz Krzysztofik
> > > + *
> > > + * 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
> > > +
> > > +#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-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/6] SoC Camera: add driver for OMAP1 camera interface
Am Mittwoch, den 22.09.2010, 08:08 +0200 schrieb Guennadi Liakhovetski:
> On Wed, 22 Sep 2010, hermann pitton wrote:
>
> > Am Mittwoch, den 22.09.2010, 01:23 +0200 schrieb Guennadi Liakhovetski:
> > > On Sat, 11 Sep 2010, Janusz Krzysztofik wrote:
> > >
> > > > This is a V4L2 driver for TI OMAP1 SoC camera interface.
> >
> > [snip]
> >
> > > > +
> > > > + } else {
> > > > + dev_warn(dev, "%s: unhandled camera interrupt, status
> > > > == "
> > > > + "0x%0x\n", __func__, it_status);
> > >
> > > Please, don't split strings
> >
> > sorry for any OT interference.
> >
> > But, are there any new rules out?
> >
> > Maybe I missed them.
> >
> > Either way, the above was forced during the last three years.
> >
> > Not at all ?
>
> No. Splitting print strings has always been discouraged, because it makes
> tracking back kernel logs difficult. The reason for this has been the 80
> character line length limit, which has now been effectively lifted. I'm
> sure you can find enough links on the internet for any of these topics.
>
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
Guennadi,
thanks for the update!
If somebody ever would like to waste time on it, lots of patches and
whole drivers have been forced into this limitations for strings too.
In fact, fixing only a few lines, including the offset, you almost
always did hit it.
I'm pleased to hear now, that this problem never did exist ;))
Cheers,
Hermann
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/6] SoC Camera: add driver for OMAP1 camera interface
Wednesday 22 September 2010 01:23:22 Guennadi Liakhovetski napisał(a):
> On Sat, 11 Sep 2010, Janusz Krzysztofik wrote:
> > This is a V4L2 driver for TI OMAP1 SoC camera interface.
> >
> > Both videobuf-dma versions are supported, contig and sg, selectable with
> > a module option. The former uses less processing power, but often fails
> > to allocate contignuous buffer memory. The latter is free of this
> > problem, but generates tens of DMA interrupts per frame. If contig memory
> > allocation ever fails, the driver falls back to sg automatically on next
> > open, but still can be switched back to contig manually. Both paths work
> > stable for me, even under heavy load, on my OMAP1510 based Amstrad Delta
> > videophone, that is the oldest, least powerfull OMAP1 implementation.
> >
> > The interface generally works in pass-through mode. Since input data byte
> > endianess can be swapped, it provides up to two v4l2 pixel formats per
> > each of several soc_mbus formats that have their swapped endian
> > counterparts.
> >
> > Boards using this driver can provide it with the followning information:
> > - if and what freqency clock is expected by an on-board camera sensor,
> > - what is the maximum pixel clock that should be accepted from the
> > sensor, - what is the polarity of the sensor provided pixel clock,
> > - if the interface GPIO line is connected to a sensor reset/powerdown
> > input and what is the input polarity.
> >
> > Created and tested against linux-2.6.36-rc3 on Amstrad Delta.
> >
> > Signed-off-by: Janusz Krzysztofik
> > ---
> >
> > Friday 30 July 2010 13:07:42 Guennadi Liakhovetski wrote:
> > > So, I think, a very welcome improvement to the driver would be a
> > > cleaner separation between the two cases. Don't try that much to reuse
> > > the code as much as possible. Would be much better to have clean
> > > separation between the two implementations - whether dynamically
> > > switchable at runtime or at config time - would be best to separate the
> > > two
> > > implementations to the point, where each of them becomes understandable
> > > and maintainable.
> >
> > Guennadi,
> > I've tried to rearrange them spearated, as you requested, but finally
> > decided to keep them together, as before, only better documented and
> > cleaned up as much as possible. I'm rather satisfied with the result, but
> > if you think it is still not enough understandable and maintainable, I'll
> > take one more iteration and split both paths.
>
> Well, I think, I'll move a bit towards the "if it breaks - someone gets to
> fix it, or it gets dropped" policy, i.e., I'll give you more freedom
> (don't know what's wrong with me today;))
Hi Guennadi,
Thanks!
...
> > +#define DMA_FRAME_SHIFT(x) (x ? DMA_FRAME_SHIFT_SG : \
> > + DMA_FRAME_SHIFT_CONTIG)
>
> Don't you want to compare (x) against CONTIG and you want to put x in
> parenthesis in the DMA_FRAME_SHIFT macro.
Sure.
> Besides, CONTIG and SG are not
> good enough names to be defined in a header under include/... Looks like
> you don't need them at all in the header? You only use them in this file,
> so, just move them inside.
Please see my very last comment below.
...
> > +static void videobuf_done(struct omap1_cam_dev *pcdev,
> > + enum videobuf_state result)
> > +{
> > + struct omap1_cam_buf *buf = pcdev->active;
> > + struct videobuf_buffer *vb;
> > + struct device *dev = pcdev->icd->dev.parent;
> > +
> > + if (WARN_ON(!buf)) {
> > + suspend_capture(pcdev);
> > + disable_capture(pcdev);
> > + return;
> > + }
> > +
> > + if (result == VIDEOBUF_ERROR)
> > + suspend_capture(pcdev);
> > +
> > + 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
Re: [PATCH v2 1/6] SoC Camera: add driver for OMAP1 camera interface
On Wed, 22 Sep 2010, hermann pitton wrote:
> Am Mittwoch, den 22.09.2010, 01:23 +0200 schrieb Guennadi Liakhovetski:
> > On Sat, 11 Sep 2010, Janusz Krzysztofik wrote:
> >
> > > This is a V4L2 driver for TI OMAP1 SoC camera interface.
>
> [snip]
>
> > > +
> > > + } else {
> > > + dev_warn(dev, "%s: unhandled camera interrupt, status == "
> > > + "0x%0x\n", __func__, it_status);
> >
> > Please, don't split strings
>
> sorry for any OT interference.
>
> But, are there any new rules out?
>
> Maybe I missed them.
>
> Either way, the above was forced during the last three years.
>
> Not at all ?
No. Splitting print strings has always been discouraged, because it makes
tracking back kernel logs difficult. The reason for this has been the 80
character line length limit, which has now been effectively lifted. I'm
sure you can find enough links on the internet for any of these topics.
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-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/6] SoC Camera: add driver for OMAP1 camera interface
Hi,
Am Mittwoch, den 22.09.2010, 01:23 +0200 schrieb Guennadi Liakhovetski:
> On Sat, 11 Sep 2010, Janusz Krzysztofik wrote:
>
> > This is a V4L2 driver for TI OMAP1 SoC camera interface.
[snip]
> > +
> > + } else {
> > + dev_warn(dev, "%s: unhandled camera interrupt, status == "
> > + "0x%0x\n", __func__, it_status);
>
> Please, don't split strings
sorry for any OT interference.
But, are there any new rules out?
Maybe I missed them.
Either way, the above was forced during the last three years.
Not at all ?
Cheers,
Hermann
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 1/6] SoC Camera: add driver for OMAP1 camera interface
Saturday 11 September 2010 04:06:02 Jasmine Strong wrote: > On 10 Sep 2010, at 18:21, Janusz Krzysztofik wrote: > > Both paths work stable for me, even > > under heavy load, on my OMAP1510 based Amstrad Delta videophone, that is > > the oldest, least powerfull OMAP1 implementation. > > You say that, but the ARM925 in OMAP1510 is known not to exhibit the bug Then, lucky me ;) > in > OMAP1610, which causes severe slowdown to DRAM writes when the first > address of an STM instruction is not aligned to a d-cache line boundary. > This means that at least last time I looked, the Linux ARM memcpy() > implementation is often faster on 1510 than 1610. This sounds like a problem that should be solved at a machine support level rather than a camera driver. I don't follow general OMAP development closely enough to know if this bug has ever been addressed or is going to be. Unfortunatelly, I have no access to any OMAP machine other than Amstrad Delta, so I'm not able to test the driver, including its performance, on other OMAP1 implementations. Anyways, I think there is always a room for improvement, either in my omap1_camera or maybe in the omap24xxcam driver, if someone tries to add support for a camera to an OMAP1 board other than 1510, and identifies a more optimal, 1610 or higher specific way of handling the OMAP camera interface. Do you think I should rename the driver to something like "omap1510cam" rather than "omap1_camera" then? Thanks, Janusz > > -J. > ___ > e3-hacking mailing list > e3-hack...@earth.li > http://www.earth.li/cgi-bin/mailman/listinfo/e3-hacking -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
