| Message ID | 1355796739-2580-1-git-send-email-william.swanson@fuel7.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi William, Thanks for the patch. On Monday 17 December 2012 18:12:19 William Swanson wrote: > If the remote video sensor reports an interlaced video mode, the CCDC block > should configure itself appropriately. What will the CCDC do in that case ? Will it capture fields or frames to memory ? If frames, what's the field layout ? You will most likely need to modify ispvideo.c as well, to support interlacing in the V4L2 API, and possibly add interlaced formats support to the media bus API. > --- > drivers/media/platform/omap3isp/ispccdc.c | 16 ++++++++++++++-- > include/media/omap3isp.h | 3 +++ > 2 files changed, 17 insertions(+), 2 deletions(-) > > diff --git a/drivers/media/platform/omap3isp/ispccdc.c > b/drivers/media/platform/omap3isp/ispccdc.c index 60e60aa..5443ef4 100644 > --- a/drivers/media/platform/omap3isp/ispccdc.c > +++ b/drivers/media/platform/omap3isp/ispccdc.c > @@ -970,10 +970,11 @@ void omap3isp_ccdc_max_rate(struct isp_ccdc_device > *ccdc, * @ccdc: Pointer to ISP CCDC device. > * @pdata: Parallel interface platform data (may be NULL) > * @data_size: Data size > + * @interlaced: Use interlaced mode instead of progressive mode > */ > static void ccdc_config_sync_if(struct isp_ccdc_device *ccdc, > struct isp_parallel_platform_data *pdata, > - unsigned int data_size) > + unsigned int data_size, bool interlaced) > { > struct isp_device *isp = to_isp_device(ccdc); > const struct v4l2_mbus_framefmt *format; > @@ -1004,9 +1005,15 @@ static void ccdc_config_sync_if(struct > isp_ccdc_device *ccdc, break; > } > > + if (interlaced) > + syn_mode |= ISPCCDC_SYN_MODE_FLDMODE; > + > if (pdata && pdata->data_pol) > syn_mode |= ISPCCDC_SYN_MODE_DATAPOL; > > + if (pdata && pdata->fld_pol) > + syn_mode |= ISPCCDC_SYN_MODE_FLDPOL; > + > if (pdata && pdata->hs_pol) > syn_mode |= ISPCCDC_SYN_MODE_HDPOL; > > @@ -1111,6 +1118,7 @@ static void ccdc_configure(struct isp_ccdc_device > *ccdc) const struct v4l2_rect *crop; > const struct isp_format_info *fmt_info; > struct v4l2_subdev_format fmt_src; > + bool src_interlaced = false; > unsigned int depth_out; > unsigned int depth_in = 0; > struct media_pad *pad; > @@ -1132,6 +1140,10 @@ static void ccdc_configure(struct isp_ccdc_device > *ccdc) fmt_src.pad = pad->index; > fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE; > if (!v4l2_subdev_call(sensor, pad, get_fmt, NULL, &fmt_src)) { > + if (fmt_src.format.field == V4L2_FIELD_INTERLACED || > + fmt_src.format.field == V4L2_FIELD_INTERLACED_TB || > + fmt_src.format.field == V4L2_FIELD_INTERLACED_BT) > + src_interlaced = true; > fmt_info = omap3isp_video_format_info(fmt_src.format.code); > depth_in = fmt_info->width; > } > @@ -1150,7 +1162,7 @@ static void ccdc_configure(struct isp_ccdc_device > *ccdc) > > omap3isp_configure_bridge(isp, ccdc->input, pdata, shift, bridge); > > - ccdc_config_sync_if(ccdc, pdata, depth_out); > + ccdc_config_sync_if(ccdc, pdata, depth_out, src_interlaced); > > syn_mode = isp_reg_readl(isp, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_SYN_MODE); > > diff --git a/include/media/omap3isp.h b/include/media/omap3isp.h > index 9584269..32d85c2 100644 > --- a/include/media/omap3isp.h > +++ b/include/media/omap3isp.h > @@ -57,6 +57,8 @@ enum { > * ISP_LANE_SHIFT_6 - CAMEXT[13:6] -> CAM[7:0] > * @clk_pol: Pixel clock polarity > * 0 - Sample on rising edge, 1 - Sample on falling edge > + * @fld_pol: Field identification signal polarity > + * 0 - Active high, 1 - Active low > * @hs_pol: Horizontal synchronization polarity > * 0 - Active high, 1 - Active low > * @vs_pol: Vertical synchronization polarity > @@ -67,6 +69,7 @@ enum { > struct isp_parallel_platform_data { > unsigned int data_lane_shift:2; > unsigned int clk_pol:1; > + unsigned int fld_pol:1; > unsigned int hs_pol:1; > unsigned int vs_pol:1; > unsigned int data_pol:1;
Em Tue, 25 Dec 2012 22:05:48 +0100 Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: > Hi William, > > Thanks for the patch. Btw, you missed to add a Signed-off-by: line on it. > > On Monday 17 December 2012 18:12:19 William Swanson wrote: > > If the remote video sensor reports an interlaced video mode, the CCDC block > > should configure itself appropriately. > > What will the CCDC do in that case ? Will it capture fields or frames to > memory ? If frames, what's the field layout ? You will most likely need to > modify ispvideo.c as well, to support interlacing in the V4L2 API, and > possibly add interlaced formats support to the media bus API. > > > --- > > drivers/media/platform/omap3isp/ispccdc.c | 16 ++++++++++++++-- > > include/media/omap3isp.h | 3 +++ > > 2 files changed, 17 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/media/platform/omap3isp/ispccdc.c > > b/drivers/media/platform/omap3isp/ispccdc.c index 60e60aa..5443ef4 100644 > > --- a/drivers/media/platform/omap3isp/ispccdc.c > > +++ b/drivers/media/platform/omap3isp/ispccdc.c > > @@ -970,10 +970,11 @@ void omap3isp_ccdc_max_rate(struct isp_ccdc_device > > *ccdc, * @ccdc: Pointer to ISP CCDC device. > > * @pdata: Parallel interface platform data (may be NULL) > > * @data_size: Data size > > + * @interlaced: Use interlaced mode instead of progressive mode > > */ > > static void ccdc_config_sync_if(struct isp_ccdc_device *ccdc, > > struct isp_parallel_platform_data *pdata, > > - unsigned int data_size) > > + unsigned int data_size, bool interlaced) > > { > > struct isp_device *isp = to_isp_device(ccdc); > > const struct v4l2_mbus_framefmt *format; > > @@ -1004,9 +1005,15 @@ static void ccdc_config_sync_if(struct > > isp_ccdc_device *ccdc, break; > > } > > > > + if (interlaced) > > + syn_mode |= ISPCCDC_SYN_MODE_FLDMODE; > > + > > if (pdata && pdata->data_pol) > > syn_mode |= ISPCCDC_SYN_MODE_DATAPOL; > > > > + if (pdata && pdata->fld_pol) > > + syn_mode |= ISPCCDC_SYN_MODE_FLDPOL; > > + > > if (pdata && pdata->hs_pol) > > syn_mode |= ISPCCDC_SYN_MODE_HDPOL; > > > > @@ -1111,6 +1118,7 @@ static void ccdc_configure(struct isp_ccdc_device > > *ccdc) const struct v4l2_rect *crop; > > const struct isp_format_info *fmt_info; > > struct v4l2_subdev_format fmt_src; > > + bool src_interlaced = false; > > unsigned int depth_out; > > unsigned int depth_in = 0; > > struct media_pad *pad; > > @@ -1132,6 +1140,10 @@ static void ccdc_configure(struct isp_ccdc_device > > *ccdc) fmt_src.pad = pad->index; > > fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE; > > if (!v4l2_subdev_call(sensor, pad, get_fmt, NULL, &fmt_src)) { > > + if (fmt_src.format.field == V4L2_FIELD_INTERLACED || > > + fmt_src.format.field == V4L2_FIELD_INTERLACED_TB || > > + fmt_src.format.field == V4L2_FIELD_INTERLACED_BT) > > + src_interlaced = true; > > fmt_info = omap3isp_video_format_info(fmt_src.format.code); > > depth_in = fmt_info->width; > > } > > @@ -1150,7 +1162,7 @@ static void ccdc_configure(struct isp_ccdc_device > > *ccdc) > > > > omap3isp_configure_bridge(isp, ccdc->input, pdata, shift, bridge); > > > > - ccdc_config_sync_if(ccdc, pdata, depth_out); > > + ccdc_config_sync_if(ccdc, pdata, depth_out, src_interlaced); > > > > syn_mode = isp_reg_readl(isp, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_SYN_MODE); > > > > diff --git a/include/media/omap3isp.h b/include/media/omap3isp.h > > index 9584269..32d85c2 100644 > > --- a/include/media/omap3isp.h > > +++ b/include/media/omap3isp.h > > @@ -57,6 +57,8 @@ enum { > > * ISP_LANE_SHIFT_6 - CAMEXT[13:6] -> CAM[7:0] > > * @clk_pol: Pixel clock polarity > > * 0 - Sample on rising edge, 1 - Sample on falling edge > > + * @fld_pol: Field identification signal polarity > > + * 0 - Active high, 1 - Active low > > * @hs_pol: Horizontal synchronization polarity > > * 0 - Active high, 1 - Active low > > * @vs_pol: Vertical synchronization polarity > > @@ -67,6 +69,7 @@ enum { > > struct isp_parallel_platform_data { > > unsigned int data_lane_shift:2; > > unsigned int clk_pol:1; > > + unsigned int fld_pol:1; > > unsigned int hs_pol:1; > > unsigned int vs_pol:1; > > unsigned int data_pol:1;
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: >> On Monday 17 December 2012 18:12:19 William Swanson wrote: >>> If the remote video sensor reports an interlaced video mode, the CCDC block >>> should configure itself appropriately. >> >> What will the CCDC do in that case ? Will it capture fields or frames to >> memory ? If frames, what's the field layout ? You will most likely need to >> modify ispvideo.c as well, to support interlacing in the V4L2 API, and >> possibly add interlaced formats support to the media bus API. Sorry for the delay in responding; today is my first day back at the office. I do not know the answers to these questions, and the documentation doesn't discuss interlacing much. Our application has the following pipeline: composite video -> TVP5146 decoder -> CCDC parallel interface -> memory -> application One of the wires in the parallel interface, cam_fld, indicates the current field, and this patch simply enables that wire. Without the patch, every other line in our memory buffer is garbage; with the patch, the image comes out correctly. As a matter of fact, an earlier version of the ISP driver actually contained code for dealing with this flag; it was removed in cf7a3d91ade6c56bfd860b377f84bd58132f7a81 along with a bunch of other cleanup work. This patch simply adds the code back, but in a way that is compatible with the new media pipeline stuff. I believe that the CCDC simply captures image data a line at a time and writes it directly to memory, at least in our use case. The CCDC_SDOFST register controls the layout, and the default value (which is what the driver uses now) is basically correct. I am not familiar enough with the V4L2 architecture to tell you how the driver decides that it now has a complete frame, or what that even means in an interlaced case. On 12/27/2012 12:27 PM, Mauro Carvalho Chehab wrote: > Btw, you missed to add a Signed-off-by: line on it. Oops, this was a problem with my git setup. Both email addresses are mine; I can re-send the patch with them both set to the same address if you would prefer that. -William -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi William, On Friday 04 January 2013 11:52:28 William Swanson wrote: > > Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu: > >> On Monday 17 December 2012 18:12:19 William Swanson wrote: > >>> If the remote video sensor reports an interlaced video mode, the CCDC > >>> block should configure itself appropriately. > >> > >> What will the CCDC do in that case ? Will it capture fields or frames to > >> memory ? If frames, what's the field layout ? You will most likely need > >> to modify ispvideo.c as well, to support interlacing in the V4L2 API, and > >> possibly add interlaced formats support to the media bus API. > > Sorry for the delay in responding; today is my first day back at the > office. I do not know the answers to these questions, and the > documentation doesn't discuss interlacing much. Our application has the > following pipeline: > > composite video -> TVP5146 decoder > -> CCDC parallel interface -> memory -> application > > One of the wires in the parallel interface, cam_fld, indicates the > current field, and this patch simply enables that wire. Without the > patch, every other line in our memory buffer is garbage; with the patch, > the image comes out correctly. What do you get in the memory buffers ? Are fields captured in separate buffers or combined in a single buffer ? If they're combined, are they interleaved or sequential in memory ? > As a matter of fact, an earlier version of the ISP driver actually > contained code for dealing with this flag; it was removed in > cf7a3d91ade6c56bfd860b377f84bd58132f7a81 along with a bunch of other > cleanup work. This patch simply adds the code back, but in a way that is > compatible with the new media pipeline stuff. > > I believe that the CCDC simply captures image data a line at a time and > writes it directly to memory, at least in our use case. The CCDC_SDOFST > register controls the layout, and the default value (which is what the > driver uses now) is basically correct. I am not familiar enough with the > V4L2 architecture to tell you how the driver decides that it now has a > complete frame, or what that even means in an interlaced case. > > On 12/27/2012 12:27 PM, Mauro Carvalho Chehab wrote: > > Btw, you missed to add a Signed-off-by: line on it. > > Oops, this was a problem with my git setup. Both email addresses are > mine; I can re-send the patch with them both set to the same address if > you would prefer that.
On 01/07/2013 04:20 AM, Laurent Pinchart wrote: > What do you get in the memory buffers ? Are fields captured in separate > buffers or combined in a single buffer ? If they're combined, are they > interleaved or sequential in memory ? I believe the data is combined in a single buffer, with alternate fields interleaved. -William -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi William, On Tuesday 08 January 2013 14:49:41 William Swanson wrote: > On 01/07/2013 04:20 AM, Laurent Pinchart wrote: > > What do you get in the memory buffers ? Are fields captured in separate > > buffers or combined in a single buffer ? If they're combined, are they > > interleaved or sequential in memory ? > > I believe the data is combined in a single buffer, with alternate fields > interleaved. Could you please double-check that ? I'd like to be sure, not just believe :-) In that case the OMAP3 ISP driver should set the v4l2_pix_format::field to V4L2_FIELD_INTERLACED in the format-related ioctl when an interlaced format is used. I suppose this could be added later - Sakari, any opinion ?
On 01/09/2013 02:35 PM, Laurent Pinchart wrote: > On Tuesday 08 January 2013 14:49:41 William Swanson wrote: >> I believe the data is combined in a single buffer, with alternate fields >> interleaved. > > Could you please double-check that ? I'd like to be sure, not just believe :-) Sorry for the delay in getting back to you. I have checked it, and the fields are indeed interlaced into a single buffer. On the other hand, looking at this caused me to discover another problem with the patch. According to the TI documentation, the CCDC_SDOFST register controls the deinterlacing process. My patch never configures this register, however, which is surprising. The reason things work on our boards is because we are carrying a separate patch which changes the register by accident. Oops! I have fixed this, and will be sending another patch with the CCDC_SDOFST changes. > In that case the OMAP3 ISP driver should set the v4l2_pix_format::field to > V4L2_FIELD_INTERLACED in the format-related ioctl when an interlaced format is > used. I suppose this could be added later - Sakari, any opinion ? I don't have a lot of time to work on this stuff, so my main focus is just getting the data to flow. Changing the output format information involves other parts of the driver that I am not familiar with, so I don't know if I will be able to work on that bit. By the way, thanks for taking the time to review this, Laurent. -William -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi William, On Monday 14 January 2013 14:21:29 William Swanson wrote: > On 01/09/2013 02:35 PM, Laurent Pinchart wrote: > > On Tuesday 08 January 2013 14:49:41 William Swanson wrote: > >> I believe the data is combined in a single buffer, with alternate fields > >> interleaved. > > > > Could you please double-check that ? I'd like to be sure, not just believe > > :-) > > Sorry for the delay in getting back to you. I have checked it, and the > fields are indeed interlaced into a single buffer. On the other hand, > looking at this caused me to discover another problem with the patch. > > According to the TI documentation, the CCDC_SDOFST register controls the > deinterlacing process. My patch never configures this register, however, > which is surprising. The reason things work on our boards is because we are > carrying a separate patch which changes the register by accident. Oops! I > have fixed this, and will be sending another patch with the CCDC_SDOFST > changes. > > > In that case the OMAP3 ISP driver should set the v4l2_pix_format::field to > > V4L2_FIELD_INTERLACED in the format-related ioctl when an interlaced > > format is used. I suppose this could be added later - Sakari, any opinion > > ? > > I don't have a lot of time to work on this stuff, so my main focus is just > getting the data to flow. Changing the output format information involves > other parts of the driver that I am not familiar with, so I don't know if I > will be able to work on that bit. OK. I will wait for the patch you mention above and I will then try to fix the field reporting issue. I might need your help to test the result. > By the way, thanks for taking the time to review this, Laurent. You're welcome.
diff --git a/drivers/media/platform/omap3isp/ispccdc.c b/drivers/media/platform/omap3isp/ispccdc.c index 60e60aa..5443ef4 100644 --- a/drivers/media/platform/omap3isp/ispccdc.c +++ b/drivers/media/platform/omap3isp/ispccdc.c @@ -970,10 +970,11 @@ void omap3isp_ccdc_max_rate(struct isp_ccdc_device *ccdc, * @ccdc: Pointer to ISP CCDC device. * @pdata: Parallel interface platform data (may be NULL) * @data_size: Data size + * @interlaced: Use interlaced mode instead of progressive mode */ static void ccdc_config_sync_if(struct isp_ccdc_device *ccdc, struct isp_parallel_platform_data *pdata, - unsigned int data_size) + unsigned int data_size, bool interlaced) { struct isp_device *isp = to_isp_device(ccdc); const struct v4l2_mbus_framefmt *format; @@ -1004,9 +1005,15 @@ static void ccdc_config_sync_if(struct isp_ccdc_device *ccdc, break; } + if (interlaced) + syn_mode |= ISPCCDC_SYN_MODE_FLDMODE; + if (pdata && pdata->data_pol) syn_mode |= ISPCCDC_SYN_MODE_DATAPOL; + if (pdata && pdata->fld_pol) + syn_mode |= ISPCCDC_SYN_MODE_FLDPOL; + if (pdata && pdata->hs_pol) syn_mode |= ISPCCDC_SYN_MODE_HDPOL; @@ -1111,6 +1118,7 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc) const struct v4l2_rect *crop; const struct isp_format_info *fmt_info; struct v4l2_subdev_format fmt_src; + bool src_interlaced = false; unsigned int depth_out; unsigned int depth_in = 0; struct media_pad *pad; @@ -1132,6 +1140,10 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc) fmt_src.pad = pad->index; fmt_src.which = V4L2_SUBDEV_FORMAT_ACTIVE; if (!v4l2_subdev_call(sensor, pad, get_fmt, NULL, &fmt_src)) { + if (fmt_src.format.field == V4L2_FIELD_INTERLACED || + fmt_src.format.field == V4L2_FIELD_INTERLACED_TB || + fmt_src.format.field == V4L2_FIELD_INTERLACED_BT) + src_interlaced = true; fmt_info = omap3isp_video_format_info(fmt_src.format.code); depth_in = fmt_info->width; } @@ -1150,7 +1162,7 @@ static void ccdc_configure(struct isp_ccdc_device *ccdc) omap3isp_configure_bridge(isp, ccdc->input, pdata, shift, bridge); - ccdc_config_sync_if(ccdc, pdata, depth_out); + ccdc_config_sync_if(ccdc, pdata, depth_out, src_interlaced); syn_mode = isp_reg_readl(isp, OMAP3_ISP_IOMEM_CCDC, ISPCCDC_SYN_MODE); diff --git a/include/media/omap3isp.h b/include/media/omap3isp.h index 9584269..32d85c2 100644 --- a/include/media/omap3isp.h +++ b/include/media/omap3isp.h @@ -57,6 +57,8 @@ enum { * ISP_LANE_SHIFT_6 - CAMEXT[13:6] -> CAM[7:0] * @clk_pol: Pixel clock polarity * 0 - Sample on rising edge, 1 - Sample on falling edge + * @fld_pol: Field identification signal polarity + * 0 - Active high, 1 - Active low * @hs_pol: Horizontal synchronization polarity * 0 - Active high, 1 - Active low * @vs_pol: Vertical synchronization polarity @@ -67,6 +69,7 @@ enum { struct isp_parallel_platform_data { unsigned int data_lane_shift:2; unsigned int clk_pol:1; + unsigned int fld_pol:1; unsigned int hs_pol:1; unsigned int vs_pol:1; unsigned int data_pol:1;