Re: [PATCH 2/2] omap3isp: ccdc: Add support to ITU-R BT.656 video data format

2011-10-09 Thread Laurent Pinchart 
Hi Javier,

Thanks for the patch.

On Sunday 09 October 2011 04:37:33 Javier Martinez Canillas wrote:
 The ITU-R BT.656 standard data format provides interlaced video data.
 
 This patch adds to the ISP CCDC driver the ability to deinterlace the
 video data and send progressive frames to user-space applications.
 
 The changes are:
 - Maintain two buffers (struct isp_buffer), current and last.
 - Decouple next buffer obtaining from last buffer releasing.
 - Move most of the logic to the VD1 interrupt handler since the
   ISP is not busy there.

Could you please explain why this is needed ?

 Signed-off-by: Javier Martinez Canillas martinez.jav...@gmail.com
 ---
  drivers/media/video/omap3isp/ispccdc.c |  195
 ++-- drivers/media/video/omap3isp/ispccdc.h | 
   6 +
  include/media/omap3isp.h   |3 +
  3 files changed, 146 insertions(+), 58 deletions(-)
 
 diff --git a/drivers/media/video/omap3isp/ispccdc.c
 b/drivers/media/video/omap3isp/ispccdc.c index c25db54..fff1ae1 100644
 --- a/drivers/media/video/omap3isp/ispccdc.c
 +++ b/drivers/media/video/omap3isp/ispccdc.c
 @@ -40,6 +40,7 @@
  static struct v4l2_mbus_framefmt *
  __ccdc_get_format(struct isp_ccdc_device *ccdc, struct v4l2_subdev_fh *fh,
 unsigned int pad, enum v4l2_subdev_format_whence which);
 +static bool ccdc_input_is_bt656(struct isp_ccdc_device *ccdc);
 
  static const unsigned int ccdc_fmts[] = {
   V4L2_MBUS_FMT_Y8_1X8,
 @@ -893,7 +894,7 @@ static void ccdc_config_outlineoffset(struct
 isp_ccdc_device *ccdc, ISPCCDC_SDOFST_FINV);
 
   isp_reg_clr(isp, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_SDOFST,
 - ISPCCDC_SDOFST_FOFST_4L);
 + ISPCCDC_SDOFST_FOFST_1L);

Are you sure this is needed ? isp_reg_clr() clears the bits (which are as far 
as I see not set anyway).

   switch (oddeven) {
   case EVENEVEN:
 @@ -1010,6 +1011,9 @@ static void ccdc_config_sync_if(struct
 isp_ccdc_device *ccdc, if (pdata  pdata-vs_pol)
   syn_mode |= ISPCCDC_SYN_MODE_VDPOL;
 
 + if (pdata  pdata-fldmode)
 + syn_mode |= ISPCCDC_SYN_MODE_FLDMODE;
 +

Addition and usage of the fldmode field can be split to a patch of its own.

   isp_reg_writel(isp, syn_mode, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_SYN_MODE);
 
   if (format-code == V4L2_MBUS_FMT_UYVY8_2X8)
 @@ -1115,6 +1119,10 @@ static void ccdc_configure(struct isp_ccdc_device
 *ccdc) unsigned int shift;
   u32 syn_mode;
   u32 ccdc_pattern;
 + u32 nph;
 + u32 nlv;
 + u32 vd0;
 + u32 vd1;
 
   pad = media_entity_remote_source(ccdc-pads[CCDC_PAD_SINK]);
   sensor = media_entity_to_v4l2_subdev(pad-entity);
 @@ -1185,26 +1193,44 @@ static void ccdc_configure(struct isp_ccdc_device
 *ccdc) }
   ccdc_config_imgattr(ccdc, ccdc_pattern);
 
 + if (pdata-bt656) {
 + vd0 = nlv = format-height / 2 - 1;
 + vd1 = format-height / 3 - 1;
 + nph = format-width * 2 - 1;

Isn't this applicable to interlaced non-BT.656 modes as well ? Same for the 
other bt656 checks you introduce below.

 + } else {
 + vd0 = nlv = format-height - 2;
 + vd1 = format-height * 2 / 3;
 + nph = format-width - 1;
 + }
 +
   /* 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) |
 -((format-height * 2 / 3)  ISPCCDC_VDINT_1_SHIFT),
 + isp_reg_writel(isp, (vd0  ISPCCDC_VDINT_0_SHIFT) |
 +(vd1  ISPCCDC_VDINT_1_SHIFT),
  OMAP3_ISP_IOMEM_CCDC, ISPCCDC_VDINT);
 
   /* CCDC_PAD_SOURCE_OF */
   format = ccdc-formats[CCDC_PAD_SOURCE_OF];
 
   isp_reg_writel(isp, (0  ISPCCDC_HORZ_INFO_SPH_SHIFT) |
 -((format-width - 1)  ISPCCDC_HORZ_INFO_NPH_SHIFT),
 +(nph  ISPCCDC_HORZ_INFO_NPH_SHIFT),
  OMAP3_ISP_IOMEM_CCDC, ISPCCDC_HORZ_INFO);
 + isp_reg_writel(isp, nlv  ISPCCDC_VERT_LINES_NLV_SHIFT,
 +OMAP3_ISP_IOMEM_CCDC, ISPCCDC_VERT_LINES);
   isp_reg_writel(isp, 0  ISPCCDC_VERT_START_SLV0_SHIFT,
  OMAP3_ISP_IOMEM_CCDC, ISPCCDC_VERT_START);
 - isp_reg_writel(isp, (format-height - 1)
 -  ISPCCDC_VERT_LINES_NLV_SHIFT,
 -OMAP3_ISP_IOMEM_CCDC, ISPCCDC_VERT_LINES);
 + isp_reg_writel(isp, 0  ISPCCDC_VERT_START_SLV1_SHIFT,
 +OMAP3_ISP_IOMEM_CCDC, ISPCCDC_VERT_START);
 
 - ccdc_config_outlineoffset(ccdc, ccdc-video_out.bpl_value, 0, 0);
 +
 + if (pdata-bt656) {
 + ccdc_config_outlineoffset(ccdc, nph, EVENEVEN, 1);
 + ccdc_config_outlineoffset(ccdc, nph, EVENODD, 1);
 + ccdc_config_outlineoffset(ccdc, nph, ODDEVEN, 1);
 + ccdc_config_outlineoffset(ccdc, nph, ODDODD, 1);
 + } else
 +

Re: [PATCH 2/2] omap3isp: ccdc: Add support to ITU-R BT.656 video data format

2011-10-09 Thread Javier Martinez Canillas 
On Sun, Oct 9, 2011 at 12:02 PM, Laurent Pinchart
laurent.pinch...@ideasonboard.com wrote:
 Hi Javier,

 Thanks for the patch.


Hi Laurent, I'm so glad that you are providing feedback on this :)

 On Sunday 09 October 2011 04:37:33 Javier Martinez Canillas wrote:
 The ITU-R BT.656 standard data format provides interlaced video data.

 This patch adds to the ISP CCDC driver the ability to deinterlace the
 video data and send progressive frames to user-space applications.

 The changes are:
     - Maintain two buffers (struct isp_buffer), current and last.
     - Decouple next buffer obtaining from last buffer releasing.
     - Move most of the logic to the VD1 interrupt handler since the
       ISP is not busy there.

 Could you please explain why this is needed ?


Well, reading the TRM I came to the conclusion (I may be wrong with
this assumption) that it is not correct to do the buffer management
for the CCDC on the VD0 interrupt handler. Since by the time you are
processing the end of frame interrupt handler a new frame has already
started.

The ISP shadowed registers values can't be updated there because the
values written to them will not take effect until the start of the
next frame. But since a new frame has already started, by the time we
are setting for example the CCDC_SDR_ADDR register, this means that
the frame already being processed will use the last value and not the
one we expect it to use.

For me, made more sense to move all the buffer management code to the
VD1 interrupt handler. Since this occurs before a frame ends, if we
update there the shadowed registers, the values will take effect when
the next frame starts.

But you can't release the las buffer yet, since the ISP is still
processing the frame and copying to memory. We have to release the
last buffer (and wake the pending process) either on the VD0 handler
when the frame processing finish or during the VD1 handler when the
next frame has already started.

This means that the next buffer obtaining and the last buffer
releasing occur at different moment. So, we have to decouple these two
actions.

I hope I made myself clear with this. Am I right or completely lost with this?

 Signed-off-by: Javier Martinez Canillas martinez.jav...@gmail.com
 ---
  drivers/media/video/omap3isp/ispccdc.c |  195
 ++-- drivers/media/video/omap3isp/ispccdc.h |
   6 +
  include/media/omap3isp.h               |    3 +
  3 files changed, 146 insertions(+), 58 deletions(-)

 diff --git a/drivers/media/video/omap3isp/ispccdc.c
 b/drivers/media/video/omap3isp/ispccdc.c index c25db54..fff1ae1 100644
 --- a/drivers/media/video/omap3isp/ispccdc.c
 +++ b/drivers/media/video/omap3isp/ispccdc.c
 @@ -40,6 +40,7 @@
  static struct v4l2_mbus_framefmt *
  __ccdc_get_format(struct isp_ccdc_device *ccdc, struct v4l2_subdev_fh *fh,
                 unsigned int pad, enum v4l2_subdev_format_whence which);
 +static bool ccdc_input_is_bt656(struct isp_ccdc_device *ccdc);

  static const unsigned int ccdc_fmts[] = {
       V4L2_MBUS_FMT_Y8_1X8,
 @@ -893,7 +894,7 @@ static void ccdc_config_outlineoffset(struct
 isp_ccdc_device *ccdc, ISPCCDC_SDOFST_FINV);

       isp_reg_clr(isp, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_SDOFST,
 -                 ISPCCDC_SDOFST_FOFST_4L);
 +                 ISPCCDC_SDOFST_FOFST_1L);

 Are you sure this is needed ? isp_reg_clr() clears the bits (which are as far
 as I see not set anyway).


The value 0x0 on the FOFST field configures the line offset value to
+1. But looking at the TRM now this is the default value so you are
right, this is not needed and I'll remove it.

       switch (oddeven) {
       case EVENEVEN:
 @@ -1010,6 +1011,9 @@ static void ccdc_config_sync_if(struct
 isp_ccdc_device *ccdc, if (pdata  pdata-vs_pol)
               syn_mode |= ISPCCDC_SYN_MODE_VDPOL;

 +     if (pdata  pdata-fldmode)
 +             syn_mode |= ISPCCDC_SYN_MODE_FLDMODE;
 +

 Addition and usage of the fldmode field can be split to a patch of its own.


Ok, will do.

       isp_reg_writel(isp, syn_mode, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_SYN_MODE);

       if (format-code == V4L2_MBUS_FMT_UYVY8_2X8)
 @@ -1115,6 +1119,10 @@ static void ccdc_configure(struct isp_ccdc_device
 *ccdc) unsigned int shift;
       u32 syn_mode;
       u32 ccdc_pattern;
 +     u32 nph;
 +     u32 nlv;
 +     u32 vd0;
 +     u32 vd1;

       pad = media_entity_remote_source(ccdc-pads[CCDC_PAD_SINK]);
       sensor = media_entity_to_v4l2_subdev(pad-entity);
 @@ -1185,26 +1193,44 @@ static void ccdc_configure(struct isp_ccdc_device
 *ccdc) }
       ccdc_config_imgattr(ccdc, ccdc_pattern);

 +     if (pdata-bt656) {
 +             vd0 = nlv = format-height / 2 - 1;
 +             vd1 = format-height / 3 - 1;
 +             nph = format-width * 2 - 1;

 Isn't this applicable to interlaced non-BT.656 modes as well ? Same for the
 other bt656 checks you introduce below.


You are right, I will check pdata-fldmode instead. The only one that
is BT.656 specific is width * 2

Re: [PATCH 2/2] omap3isp: ccdc: Add support to ITU-R BT.656 video data format

2011-10-09 Thread Enrico 
On Sun, Oct 9, 2011 at 12:02 PM, Laurent Pinchart
laurent.pinch...@ideasonboard.com wrote:
 Hi Javier,

 Thanks for the patch.

Laurent, apart from the specific comments on Javier code, did you have
a look at Deepthy patches too?

I say this because asking Javier for fixes means (to me) that you are
going to merge his patches (if testing will confirm it works of
course). Am i wrong?

Btw i'm trying to get these patches on 3.1.0rc9 (from igep repository,
that should be just like mainline 3.1.0rc9 with some bsp patches), i
hope i will report good news.

Enrico
--
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 2/2] omap3isp: ccdc: Add support to ITU-R BT.656 video data format

2011-10-09 Thread Javier Martinez Canillas 
On Sun, Oct 9, 2011 at 2:58 PM, Enrico ebut...@users.berlios.de wrote:
 On Sun, Oct 9, 2011 at 12:02 PM, Laurent Pinchart
 laurent.pinch...@ideasonboard.com wrote:
 Hi Javier,

 Thanks for the patch.

 Laurent, apart from the specific comments on Javier code, did you have
 a look at Deepthy patches too?

 I say this because asking Javier for fixes means (to me) that you are
 going to merge his patches (if testing will confirm it works of
 course). Am i wrong?

I don't think these patches are in merge-able state because I only
compile tested.
Now based on Laurent's comments I just sent a v2 patch-set based in
one of our earlier changes.

This was before we decide to move the logic to VD1 so the patch-set is
less intrusive and will be easier to review.

Also, now I check for fldmode not bt656, Laurent's is right that
bt.656 is interlaced but not every interlaced video is bt.656.


 Btw i'm trying to get these patches on 3.1.0rc9 (from igep repository,
 that should be just like mainline 3.1.0rc9 with some bsp patches), i
 hope i will report good news.

 Enrico


Great, thank you very much for your efforts Enrico.

Best regards,

-- 
Javier Martínez Canillas
(+34) 682 39 81 69
Barcelona, Spain
--
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 2/2] omap3isp: ccdc: Add support to ITU-R BT.656 video data format

2011-10-09 Thread Javier Martinez Canillas 
On Sun, Oct 9, 2011 at 2:58 PM, Enrico ebut...@users.berlios.de wrote:
 On Sun, Oct 9, 2011 at 12:02 PM, Laurent Pinchart
 laurent.pinch...@ideasonboard.com wrote:
 Hi Javier,

 Thanks for the patch.

 Laurent, apart from the specific comments on Javier code, did you have
 a look at Deepthy patches too?

 I say this because asking Javier for fixes means (to me) that you are
 going to merge his patches (if testing will confirm it works of
 course). Am i wrong?

 Btw i'm trying to get these patches on 3.1.0rc9 (from igep repository,
 that should be just like mainline 3.1.0rc9 with some bsp patches), i
 hope i will report good news.

 Enrico


I just created a repository on my personal github account to keep the
patches [1] and make it more easily accessible to people that want to
try.
These already have the fix to the fact that ccdc_input_is_fldmode()
was returning pdata-bt655 instead of pdata-fldmode.

I will try to test the patches tomorrow when I have access to the
hardware but I can't promise because I have lots of other tasks to do.

[1]: https://github.com/martinezjavier/omap3isp/tree/master/omap3isp-yuv-patches

Best regards,

-- 
Javier Martínez Canillas
(+34) 682 39 81 69
Barcelona, Spain
--
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

[PATCH 2/2] omap3isp: ccdc: Add support to ITU-R BT.656 video data format

2011-10-08 Thread Javier Martinez Canillas 
The ITU-R BT.656 standard data format provides interlaced video data.

This patch adds to the ISP CCDC driver the ability to deinterlace the
video data and send progressive frames to user-space applications.

The changes are:
- Maintain two buffers (struct isp_buffer), current and last.
- Decouple next buffer obtaining from last buffer releasing.
- Move most of the logic to the VD1 interrupt handler since the
  ISP is not busy there.

Signed-off-by: Javier Martinez Canillas martinez.jav...@gmail.com
---
 drivers/media/video/omap3isp/ispccdc.c |  195 ++--
 drivers/media/video/omap3isp/ispccdc.h |6 +
 include/media/omap3isp.h   |3 +
 3 files changed, 146 insertions(+), 58 deletions(-)

diff --git a/drivers/media/video/omap3isp/ispccdc.c 
b/drivers/media/video/omap3isp/ispccdc.c
index c25db54..fff1ae1 100644
--- a/drivers/media/video/omap3isp/ispccdc.c
+++ b/drivers/media/video/omap3isp/ispccdc.c
@@ -40,6 +40,7 @@
 static struct v4l2_mbus_framefmt *
 __ccdc_get_format(struct isp_ccdc_device *ccdc, struct v4l2_subdev_fh *fh,
  unsigned int pad, enum v4l2_subdev_format_whence which);
+static bool ccdc_input_is_bt656(struct isp_ccdc_device *ccdc);
 
 static const unsigned int ccdc_fmts[] = {
V4L2_MBUS_FMT_Y8_1X8,
@@ -893,7 +894,7 @@ static void ccdc_config_outlineoffset(struct 
isp_ccdc_device *ccdc,
ISPCCDC_SDOFST_FINV);
 
isp_reg_clr(isp, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_SDOFST,
-   ISPCCDC_SDOFST_FOFST_4L);
+   ISPCCDC_SDOFST_FOFST_1L);
 
switch (oddeven) {
case EVENEVEN:
@@ -1010,6 +1011,9 @@ static void ccdc_config_sync_if(struct isp_ccdc_device 
*ccdc,
if (pdata  pdata-vs_pol)
syn_mode |= ISPCCDC_SYN_MODE_VDPOL;
 
+   if (pdata  pdata-fldmode)
+   syn_mode |= ISPCCDC_SYN_MODE_FLDMODE;
+
isp_reg_writel(isp, syn_mode, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_SYN_MODE);
 
if (format-code == V4L2_MBUS_FMT_UYVY8_2X8)
@@ -1115,6 +1119,10 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
unsigned int shift;
u32 syn_mode;
u32 ccdc_pattern;
+   u32 nph;
+   u32 nlv;
+   u32 vd0;
+   u32 vd1;
 
pad = media_entity_remote_source(ccdc-pads[CCDC_PAD_SINK]);
sensor = media_entity_to_v4l2_subdev(pad-entity);
@@ -1185,26 +1193,44 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
}
ccdc_config_imgattr(ccdc, ccdc_pattern);
 
+   if (pdata-bt656) {
+   vd0 = nlv = format-height / 2 - 1;
+   vd1 = format-height / 3 - 1;
+   nph = format-width * 2 - 1;
+   } else {
+   vd0 = nlv = format-height - 2;
+   vd1 = format-height * 2 / 3;
+   nph = format-width - 1;
+   }
+
/* 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) |
-  ((format-height * 2 / 3)  ISPCCDC_VDINT_1_SHIFT),
+   isp_reg_writel(isp, (vd0  ISPCCDC_VDINT_0_SHIFT) |
+  (vd1  ISPCCDC_VDINT_1_SHIFT),
   OMAP3_ISP_IOMEM_CCDC, ISPCCDC_VDINT);
 
/* CCDC_PAD_SOURCE_OF */
format = ccdc-formats[CCDC_PAD_SOURCE_OF];
 
isp_reg_writel(isp, (0  ISPCCDC_HORZ_INFO_SPH_SHIFT) |
-  ((format-width - 1)  ISPCCDC_HORZ_INFO_NPH_SHIFT),
+  (nph  ISPCCDC_HORZ_INFO_NPH_SHIFT),
   OMAP3_ISP_IOMEM_CCDC, ISPCCDC_HORZ_INFO);
+   isp_reg_writel(isp, nlv  ISPCCDC_VERT_LINES_NLV_SHIFT,
+  OMAP3_ISP_IOMEM_CCDC, ISPCCDC_VERT_LINES);
isp_reg_writel(isp, 0  ISPCCDC_VERT_START_SLV0_SHIFT,
   OMAP3_ISP_IOMEM_CCDC, ISPCCDC_VERT_START);
-   isp_reg_writel(isp, (format-height - 1)
-ISPCCDC_VERT_LINES_NLV_SHIFT,
-  OMAP3_ISP_IOMEM_CCDC, ISPCCDC_VERT_LINES);
+   isp_reg_writel(isp, 0  ISPCCDC_VERT_START_SLV1_SHIFT,
+  OMAP3_ISP_IOMEM_CCDC, ISPCCDC_VERT_START);
 
-   ccdc_config_outlineoffset(ccdc, ccdc-video_out.bpl_value, 0, 0);
+
+   if (pdata-bt656) {
+   ccdc_config_outlineoffset(ccdc, nph, EVENEVEN, 1);
+   ccdc_config_outlineoffset(ccdc, nph, EVENODD, 1);
+   ccdc_config_outlineoffset(ccdc, nph, ODDEVEN, 1);
+   ccdc_config_outlineoffset(ccdc, nph, ODDODD, 1);
+   } else
+   ccdc_config_outlineoffset(ccdc, ccdc-video_out.bpl_value, 0, 
0);
 
/* CCDC_PAD_SOURCE_VP */
format = ccdc-formats[CCDC_PAD_SOURCE_VP];
@@ -1212,13 +1238,12 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc)
isp_reg_writel(isp, (0  ISPCCDC_FMT_HORZ_FMTSPH_SHIFT) |
   (format-width  ISPCCDC_FMT_HORZ_FMTLNH_SHIFT),