Re: [PATCH v3 3/4] media i.MX27 camera: improve discard buffer handling.

2012-02-07 Thread javier Martin 
Hi Guennadi,
thank you for your attention.

On 6 February 2012 19:33, Guennadi Liakhovetski g.liakhovet...@gmx.de wrote:
 Hi Javier

 Thanks for the update! Let's see, whether this one can be improved a bit
 more.

 On Mon, 30 Jan 2012, Javier Martin wrote:

 The way discard buffer was previously handled lead
 to possible races that made a buffer that was not
 yet ready to be overwritten by new video data. This
 is easily detected at 25fps just adding #define DEBUG
 to enable the memset check and seeing how the image
 is corrupted.

 A new discard queue and two discard buffers have
 been added to make them flow trough the pipeline
 of queues and thus provide suitable event ordering.

 Signed-off-by: Javier Martin javier.mar...@vista-silicon.com
 ---
  Changes since v2:
  - Remove BUG_ON when active list is empty.
  - Replace empty list checks with warnings.

 I think, the best would be to warn and bail out, instead of implicitly
 crashing.


 ---
  drivers/media/video/mx2_camera.c |  280 
 +-
  1 files changed, 153 insertions(+), 127 deletions(-)

 diff --git a/drivers/media/video/mx2_camera.c 
 b/drivers/media/video/mx2_camera.c
 index 35ab971..e7ccd97 100644
 --- a/drivers/media/video/mx2_camera.c
 +++ b/drivers/media/video/mx2_camera.c

 [snip]

 @@ -706,8 +806,9 @@ static int mx2_stop_streaming(struct vb2_queue *q)
       unsigned long flags;
       u32 cntl;

 -     spin_lock_irqsave(pcdev-lock, flags);
       if (mx27_camera_emma(pcdev)) {
 +             spin_lock_irqsave(pcdev-lock, flags);
 +
               cntl = readl(pcdev-base_emma + PRP_CNTL);
               if (prp-cfg.channel == 1) {
                       writel(cntl  ~PRP_CNTL_CH1EN,
 @@ -716,8 +817,18 @@ static int mx2_stop_streaming(struct vb2_queue *q)
                       writel(cntl  ~PRP_CNTL_CH2EN,
                              pcdev-base_emma + PRP_CNTL);
               }
 +             INIT_LIST_HEAD(pcdev-capture);
 +             INIT_LIST_HEAD(pcdev-active_bufs);
 +             INIT_LIST_HEAD(pcdev-discard);
 +
 +             spin_unlock_irqrestore(pcdev-lock, flags);
 +
 +             dma_free_coherent(ici-v4l2_dev.dev,
 +                     pcdev-discard_size, pcdev-discard_buffer,
 +                     pcdev-discard_buffer_dma);
 +             pcdev-discard_buffer = NULL;

 AFAICS, the IRQ handler runs without taking any locks, so, there's a
 theoretical SMP race here with using the discard buffers from the ISR. So,
 I think, you'd have to add some locking to the ISR and here do something
 like

 +               x = pcdev-discard_buffer;
 +               pcdev-discard_buffer = NULL;
 +
 +               spin_unlock_irqrestore(pcdev-lock, flags);
 +
 +               dma_free_coherent(ici-v4l2_dev.dev,
 +                       pcdev-discard_size, x,
 +                       pcdev-discard_buffer_dma);

Hmm, you are definitely right. I have to protect access to
discard_buffer too. Good you noticed.


       }
 -     spin_unlock_irqrestore(pcdev-lock, flags);
 +

 You're adding an empty line here.

Sorry, I'll fix it.


       return 0;
  }

 [snip]

 @@ -1179,18 +1212,23 @@ static struct soc_camera_host_ops 
 mx2_soc_camera_host_ops = {
  static void mx27_camera_frame_done_emma(struct mx2_camera_dev *pcdev,
               int bufnum)
  {

 This function is called from the ISR, so, I presume, you'll have to
 spin_lock() somewhere here.

Yes, otherwise I can have a lot of possible races here.

 -     u32 imgsize = pcdev-icd-user_height * pcdev-icd-user_width;
       struct mx2_fmt_cfg *prp = pcdev-emma_prp;
       struct mx2_buffer *buf;
       struct vb2_buffer *vb;
       unsigned long phys;

 -     if (!list_empty(pcdev-active_bufs)) {
 -             buf = list_entry(pcdev-active_bufs.next,
 -                     struct mx2_buffer, queue);
 +     buf = list_entry(pcdev-active_bufs.next,
 +                      struct mx2_buffer, queue);

 -             BUG_ON(buf-bufnum != bufnum);
 +     BUG_ON(buf-bufnum != bufnum);

 +     if (buf-discard) {
 +             /*
 +              * Discard buffer must not be returned to user space.
 +              * Just return it to the discard queue.
 +              */
 +             list_move_tail(pcdev-active_bufs.next, pcdev-discard);
 +     } else {
               vb = buf-vb;
  #ifdef DEBUG
               phys = vb2_dma_contig_plane_dma_addr(vb, 0);
 @@ -1212,6 +1250,7 @@ static void mx27_camera_frame_done_emma(struct 
 mx2_camera_dev *pcdev,
                       }
               }
  #endif
 +
               dev_dbg(pcdev-dev, %s (vb=0x%p) 0x%p %lu\n, __func__, vb,
                               vb2_plane_vaddr(vb, 0),
                               vb2_get_plane_payload(vb, 0));
 @@ -1225,29 +1264,23 @@ static void mx27_camera_frame_done_emma(struct 
 mx2_camera_dev *pcdev,
       pcdev-frame_count++;

       if (list_empty(pcdev-capture)) {
 -             if (prp-cfg.channel == 1) {
 -                     writel(pcdev-discard_buffer_dma,

Re: [PATCH v3 3/4] media i.MX27 camera: improve discard buffer handling.

2012-02-07 Thread Guennadi Liakhovetski 
Hi Javier

On Tue, 7 Feb 2012, javier Martin wrote:

[snip]

 Guennadi, before I send v4:
 Do you agree with the other patches 1, 2 and 4?

Yes, this time I haven't found anything their to complain about:-) So, 
feel free to just send a v4 of this patch and then your proposed follow-up 
fixes / improvements.

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 v3 3/4] media i.MX27 camera: improve discard buffer handling.

2012-02-07 Thread javier Martin 
On 7 February 2012 09:22, Guennadi Liakhovetski g.liakhovet...@gmx.de wrote:
 Hi Javier

 On Tue, 7 Feb 2012, javier Martin wrote:

 [snip]

 Guennadi, before I send v4:
 Do you agree with the other patches 1, 2 and 4?

 Yes, this time I haven't found anything their to complain about:-) So,
 feel free to just send a v4 of this patch and then your proposed follow-up
 fixes / improvements.

Great!
On my way.

-- 
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com
--
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 v3 3/4] media i.MX27 camera: improve discard buffer handling.

2012-02-06 Thread Guennadi Liakhovetski 
Hi Javier

Thanks for the update! Let's see, whether this one can be improved a bit 
more.

On Mon, 30 Jan 2012, Javier Martin wrote:

 The way discard buffer was previously handled lead
 to possible races that made a buffer that was not
 yet ready to be overwritten by new video data. This
 is easily detected at 25fps just adding #define DEBUG
 to enable the memset check and seeing how the image
 is corrupted.
 
 A new discard queue and two discard buffers have
 been added to make them flow trough the pipeline
 of queues and thus provide suitable event ordering.
 
 Signed-off-by: Javier Martin javier.mar...@vista-silicon.com
 ---
  Changes since v2:
  - Remove BUG_ON when active list is empty.
  - Replace empty list checks with warnings.

I think, the best would be to warn and bail out, instead of implicitly 
crashing.

 
 ---
  drivers/media/video/mx2_camera.c |  280 
 +-
  1 files changed, 153 insertions(+), 127 deletions(-)
 
 diff --git a/drivers/media/video/mx2_camera.c 
 b/drivers/media/video/mx2_camera.c
 index 35ab971..e7ccd97 100644
 --- a/drivers/media/video/mx2_camera.c
 +++ b/drivers/media/video/mx2_camera.c

[snip]

 @@ -706,8 +806,9 @@ static int mx2_stop_streaming(struct vb2_queue *q)
   unsigned long flags;
   u32 cntl;
  
 - spin_lock_irqsave(pcdev-lock, flags);
   if (mx27_camera_emma(pcdev)) {
 + spin_lock_irqsave(pcdev-lock, flags);
 +
   cntl = readl(pcdev-base_emma + PRP_CNTL);
   if (prp-cfg.channel == 1) {
   writel(cntl  ~PRP_CNTL_CH1EN,
 @@ -716,8 +817,18 @@ static int mx2_stop_streaming(struct vb2_queue *q)
   writel(cntl  ~PRP_CNTL_CH2EN,
  pcdev-base_emma + PRP_CNTL);
   }
 + INIT_LIST_HEAD(pcdev-capture);
 + INIT_LIST_HEAD(pcdev-active_bufs);
 + INIT_LIST_HEAD(pcdev-discard);
 +
 + spin_unlock_irqrestore(pcdev-lock, flags);
 +
 + dma_free_coherent(ici-v4l2_dev.dev,
 + pcdev-discard_size, pcdev-discard_buffer,
 + pcdev-discard_buffer_dma);
 + pcdev-discard_buffer = NULL;

AFAICS, the IRQ handler runs without taking any locks, so, there's a 
theoretical SMP race here with using the discard buffers from the ISR. So, 
I think, you'd have to add some locking to the ISR and here do something 
like

+   x = pcdev-discard_buffer;
+   pcdev-discard_buffer = NULL;
+
+   spin_unlock_irqrestore(pcdev-lock, flags);
+
+   dma_free_coherent(ici-v4l2_dev.dev,
+   pcdev-discard_size, x,
+   pcdev-discard_buffer_dma);


   }
 - spin_unlock_irqrestore(pcdev-lock, flags);
 +

You're adding an empty line here.

  
   return 0;
  }

[snip]

 @@ -1179,18 +1212,23 @@ static struct soc_camera_host_ops 
 mx2_soc_camera_host_ops = {
  static void mx27_camera_frame_done_emma(struct mx2_camera_dev *pcdev,
   int bufnum)
  {

This function is called from the ISR, so, I presume, you'll have to 
spin_lock() somewhere here.

 - u32 imgsize = pcdev-icd-user_height * pcdev-icd-user_width;
   struct mx2_fmt_cfg *prp = pcdev-emma_prp;
   struct mx2_buffer *buf;
   struct vb2_buffer *vb;
   unsigned long phys;
  
 - if (!list_empty(pcdev-active_bufs)) {
 - buf = list_entry(pcdev-active_bufs.next,
 - struct mx2_buffer, queue);
 + buf = list_entry(pcdev-active_bufs.next,
 +  struct mx2_buffer, queue);
  
 - BUG_ON(buf-bufnum != bufnum);
 + BUG_ON(buf-bufnum != bufnum);
  
 + if (buf-discard) {
 + /*
 +  * Discard buffer must not be returned to user space.
 +  * Just return it to the discard queue.
 +  */
 + list_move_tail(pcdev-active_bufs.next, pcdev-discard);
 + } else {
   vb = buf-vb;
  #ifdef DEBUG
   phys = vb2_dma_contig_plane_dma_addr(vb, 0);
 @@ -1212,6 +1250,7 @@ static void mx27_camera_frame_done_emma(struct 
 mx2_camera_dev *pcdev,
   }
   }
  #endif
 +
   dev_dbg(pcdev-dev, %s (vb=0x%p) 0x%p %lu\n, __func__, vb,
   vb2_plane_vaddr(vb, 0),
   vb2_get_plane_payload(vb, 0));
 @@ -1225,29 +1264,23 @@ static void mx27_camera_frame_done_emma(struct 
 mx2_camera_dev *pcdev,
   pcdev-frame_count++;
  
   if (list_empty(pcdev-capture)) {
 - if (prp-cfg.channel == 1) {
 - writel(pcdev-discard_buffer_dma, pcdev-base_emma +
 - PRP_DEST_RGB1_PTR + 4 * bufnum);
 - } else {
 - writel(pcdev-discard_buffer_dma, pcdev-base_emma +
 - PRP_DEST_Y_PTR -
 - 0x14 *

[PATCH v3 3/4] media i.MX27 camera: improve discard buffer handling.

2012-01-30 Thread Javier Martin 
The way discard buffer was previously handled lead
to possible races that made a buffer that was not
yet ready to be overwritten by new video data. This
is easily detected at 25fps just adding #define DEBUG
to enable the memset check and seeing how the image
is corrupted.

A new discard queue and two discard buffers have
been added to make them flow trough the pipeline
of queues and thus provide suitable event ordering.

Signed-off-by: Javier Martin javier.mar...@vista-silicon.com
---
 Changes since v2:
 - Remove BUG_ON when active list is empty.
 - Replace empty list checks with warnings.

---
 drivers/media/video/mx2_camera.c |  280 +-
 1 files changed, 153 insertions(+), 127 deletions(-)

diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
index 35ab971..e7ccd97 100644
--- a/drivers/media/video/mx2_camera.c
+++ b/drivers/media/video/mx2_camera.c
@@ -237,7 +237,8 @@ struct mx2_buffer {
struct list_headqueue;
enum mx2_buffer_state   state;
 
-   int bufnum;
+   int bufnum;
+   booldiscard;
 };
 
 struct mx2_camera_dev {
@@ -256,6 +257,7 @@ struct mx2_camera_dev {
 
struct list_headcapture;
struct list_headactive_bufs;
+   struct list_headdiscard;
 
spinlock_t  lock;
 
@@ -268,6 +270,7 @@ struct mx2_camera_dev {
 
u32 csicr1;
 
+   struct mx2_buffer   buf_discard[2];
void*discard_buffer;
dma_addr_t  discard_buffer_dma;
size_t  discard_size;
@@ -329,6 +332,29 @@ static struct mx2_fmt_cfg *mx27_emma_prp_get_format(
return mx27_emma_prp_table[0];
 };
 
+static void mx27_update_emma_buf(struct mx2_camera_dev *pcdev,
+unsigned long phys, int bufnum)
+{
+   struct mx2_fmt_cfg *prp = pcdev-emma_prp;
+
+   if (prp-cfg.channel == 1) {
+   writel(phys, pcdev-base_emma +
+   PRP_DEST_RGB1_PTR + 4 * bufnum);
+   } else {
+   writel(phys, pcdev-base_emma +
+   PRP_DEST_Y_PTR - 0x14 * bufnum);
+   if (prp-out_fmt == V4L2_PIX_FMT_YUV420) {
+   u32 imgsize = pcdev-icd-user_height *
+   pcdev-icd-user_width;
+
+   writel(phys + imgsize, pcdev-base_emma +
+   PRP_DEST_CB_PTR - 0x14 * bufnum);
+   writel(phys + ((5 * imgsize) / 4), pcdev-base_emma +
+   PRP_DEST_CR_PTR - 0x14 * bufnum);
+   }
+   }
+}
+
 static void mx2_camera_deactivate(struct mx2_camera_dev *pcdev)
 {
unsigned long flags;
@@ -377,7 +403,7 @@ static int mx2_camera_add_device(struct soc_camera_device 
*icd)
writel(pcdev-csicr1, pcdev-base_csi + CSICR1);
 
pcdev-icd = icd;
-   pcdev-frame_count = -1;
+   pcdev-frame_count = 0;
 
dev_info(icd-parent, Camera driver attached to camera %d\n,
 icd-devnum);
@@ -397,13 +423,6 @@ static void mx2_camera_remove_device(struct 
soc_camera_device *icd)
 
mx2_camera_deactivate(pcdev);
 
-   if (pcdev-discard_buffer) {
-   dma_free_coherent(ici-v4l2_dev.dev, pcdev-discard_size,
-   pcdev-discard_buffer,
-   pcdev-discard_buffer_dma);
-   pcdev-discard_buffer = NULL;
-   }
-
pcdev-icd = NULL;
 }
 
@@ -640,7 +659,6 @@ static void mx2_videobuf_release(struct vb2_buffer *vb)
 */
 
spin_lock_irqsave(pcdev-lock, flags);
-   list_del_init(buf-queue);
if (cpu_is_mx25()  buf-state == MX2_STATE_ACTIVE) {
if (pcdev-fb1_active == buf) {
pcdev-csicr1 = ~CSICR1_FB1_DMA_INTEN;
@@ -656,6 +674,34 @@ static void mx2_videobuf_release(struct vb2_buffer *vb)
spin_unlock_irqrestore(pcdev-lock, flags);
 }
 
+static void mx27_camera_emma_buf_init(struct soc_camera_device *icd,
+   int bytesperline)
+{
+   struct soc_camera_host *ici =
+   to_soc_camera_host(icd-parent);
+   struct mx2_camera_dev *pcdev = ici-priv;
+   struct mx2_fmt_cfg *prp = pcdev-emma_prp;
+
+   writel((icd-user_width  16) | icd-user_height,
+  pcdev-base_emma + PRP_SRC_FRAME_SIZE);
+   writel(prp-cfg.src_pixel,
+  pcdev-base_emma + PRP_SRC_PIXEL_FORMAT_CNTL);
+   if (prp-cfg.channel == 1) {
+   writel((icd-user_width  16) | icd-user_height,
+   pcdev-base_emma + PRP_CH1_OUT_IMAGE_SIZE);
+   writel(bytesperline,
+   pcdev-base_emma + PRP_DEST_CH1_LINE_STRIDE);
+   writel(prp-cfg.ch1_pixel,
+   pcdev-base_emma + PRP_CH1_PIXEL_FORMAT_CNTL);
+