[RFC] omap3isp : fix image corruption after underrun when using resizer

Commit Message

Jean-Philippe François Sept. 26, 2013, 1:52 p.m. UTC

Hi Laurent,

I was able to reliably get corrupted image when placing the pipeline in underrun
condition. The pipeline looks like this :
YUYV sensor -> CCDC -> Resizer -> V4L output

It seems that triggering 'frame sync event'  before last line leads to
possible corrupted images
when using the resizer.

With current code, ISP resizer is always configured in oneshot, and
must be restarted after
each frame. However, has stated by a comment in ispresizer.c,
restarting the resizer while
a frame is sent to the ccdc leads to corrupted images.

The current resizer code takes care of this restart in two places :

- in normal situation, when the 'resizer done' IRQ is triggered, a
buffer is available
  and the resizer is restarted in the resizer_isr_buffer function

- in underrun situation, no buffer is available when the resizer done
irq triggers. After a buffer
 has eventually been queued, the resizer is restarted on the following
frame sync.

However, the frame sync event is not generated by the hardware frame sync,
but by the VD0 interrupt of the CCDC. But VD0 event is triggered a
bit early, since it is
configured to trigger after height - 1 lines. It is therefore possible
to restart the resizer while a frame's
last line is being sent.

The following patch configures VD0 to trigger after the last line, and
solves the image
corruption issue. However, the previous value does not look like an
off by one error.
What are the reasons to configure VD0 before the last line ? What are
the possible issues
triggered by a change like this ?

Thank you for your comments,

Jean-Philippe François

the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Sakari Ailus Sept. 27, 2013, 9:58 p.m. UTC | #1

Hi Jean-Philippe,

On Thu, Sep 26, 2013 at 03:52:37PM +0200, jean-philippe francois wrote:
> Hi Laurent,
> 
> I was able to reliably get corrupted image when placing the pipeline in underrun
> condition. The pipeline looks like this :
> YUYV sensor -> CCDC -> Resizer -> V4L output
> 
> It seems that triggering 'frame sync event'  before last line leads to
> possible corrupted images
> when using the resizer.

I think this was rather understood to be somewhat unoptimal way to trigger
the interrupt at the end of the frame.

> With current code, ISP resizer is always configured in oneshot, and
> must be restarted after
> each frame. However, has stated by a comment in ispresizer.c,
> restarting the resizer while
> a frame is sent to the ccdc leads to corrupted images.
> 
> The current resizer code takes care of this restart in two places :
> 
> - in normal situation, when the 'resizer done' IRQ is triggered, a
> buffer is available
>   and the resizer is restarted in the resizer_isr_buffer function
> 
> - in underrun situation, no buffer is available when the resizer done
> irq triggers. After a buffer
>  has eventually been queued, the resizer is restarted on the following
> frame sync.
> 
> However, the frame sync event is not generated by the hardware frame sync,
> but by the VD0 interrupt of the CCDC. But VD0 event is triggered a
> bit early, since it is
> configured to trigger after height - 1 lines. It is therefore possible
> to restart the resizer while a frame's
> last line is being sent.

I guess one should then wait for the CCDC to become idle as well. Currently
this is not being done --- i.e. ccdc_sbl_wait_idle() should then be called
first.

> The following patch configures VD0 to trigger after the last line, and
> solves the image
> corruption issue. However, the previous value does not look like an
> off by one error.
> What are the reasons to configure VD0 before the last line ? What are
> the possible issues
> triggered by a change like this ?

The VD counter is incremented by the hsync singal. Depending on the
polarity, the value of the VD counter at the beginning of last line is
either height - 2 or height - 1, and this should be reflected in the
configuration as well. Which one do you have?

Patch

Index: b/drivers/media/platform/omap3isp/ispccdc.c
===================================================================
--- a/drivers/media/platform/omap3isp/ispccdc.c
+++ b/drivers/media/platform/omap3isp/ispccdc.c
@@ -1196,7 +1196,7 @@ 
  /* Generate VD0 on the last line of the image and VD1 on the
  * 2/3 height line.
  */
- isp_reg_writel(isp, ((format->height - 2) << ISPCCDC_VDINT_0_SHIFT) |
+ isp_reg_writel(isp, ((format->height - 1) << ISPCCDC_VDINT_0_SHIFT) |
        ((format->height * 2 / 3) << ISPCCDC_VDINT_1_SHIFT),
        OMAP3_ISP_IOMEM_CCDC, ISPCCDC_VDINT);
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in