Hi Tim,
On Monday 13 April 2015 23:39:28 Laurent Pinchart wrote:
> On Friday 20 March 2015 15:32:20 Tim Nordell wrote:
> > The OMAP3ISP can selectively stream either the top or bottom
> > field by setting the start line vertical field to a high value
> > for the field that one doesn't want to stream. The driver
> > can switch between these utilizing the vertical start feature
> > of the CCDC.
> >
> > Additionally, we need to ensure that the FLDMODE bit is set
> > when we're doing this as we need to differentiate between
> > the two frames.
Do you plan to resubmit this patch with the comments below taken into account
?
> > Signed-off-by: Tim Nordell <tim.nordell@xxxxxxxxxxx>
> > ---
> >
> > drivers/media/platform/omap3isp/ispccdc.c | 29 +++++++++++++++++++++++--
> > drivers/media/platform/omap3isp/ispvideo.c | 4 ++--
> > 2 files changed, 29 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/platform/omap3isp/ispccdc.c
> > b/drivers/media/platform/omap3isp/ispccdc.c index 882ebde..beb8d96 100644
> > --- a/drivers/media/platform/omap3isp/ispccdc.c
> > +++ b/drivers/media/platform/omap3isp/ispccdc.c
> > @@ -1131,6 +1131,7 @@ static void ccdc_configure(struct isp_ccdc_device
> > *ccdc) unsigned int sph;
> >
> > u32 syn_mode;
> > u32 ccdc_pattern;
> >
> > + int slv0, slv1;
>
> slv0 and slv1 are positive integers, you can use unsigned int. Could you
> please declare one variable per line to match the coding style of the
> driver, and move them right after the declaration of sph ?
>
> > ccdc->bt656 = false;
> > ccdc->fields = 0;
> >
> > @@ -1237,11 +1238,27 @@ static void ccdc_configure(struct isp_ccdc_device
> > *ccdc) nph = crop->width - 1;
> >
> > }
> >
> > + /* Default the start vertical line offset to the crop point */
> > + slv0 = slv1 = crop->top;
> > +
> > + /* When streaming just the top or bottom field, enable processing
> > + * of the field input signal so that SLV1 is processed.
> > + */
>
> The slv[01] trick below doesn't seem trivial to me, it would make sense to
> document it. How about
>
> /* When capturing only the top or bottom field, enable processing of the
> * field input signal and reject the unneeded field by setting its start
> * line to a value larger than the frame height.
> */
>
> > + if (ccdc->formats[CCDC_PAD_SINK].field == V4L2_FIELD_ALTERNATE) {
> > + if (format->field == V4L2_FIELD_TOP) {
> > + slv1 = 0x7FFF;
>
> Could you please use lowercase for hex constants ?
>
> > + syn_mode |= ISPCCDC_SYN_MODE_FLDMODE;
> > + } else if (format->field == V4L2_FIELD_BOTTOM) {
>
> Can format->field be equal to V4L2_FIELD_TOP or V4L2_FIELD_BOTTOM if ccdc-
>
> >formats[CCDC_PAD_SINK].field is not equal to V4L2_FIELD_ALTERNATE ? If not
>
> you can remove the outer condition check.
>
> > + slv0 = 0x7FFF;
> > + syn_mode |= ISPCCDC_SYN_MODE_FLDMODE;
> > + }
> > + }
> > +
> >
> > isp_reg_writel(isp, (sph << ISPCCDC_HORZ_INFO_SPH_SHIFT) |
> >
> > (nph << ISPCCDC_HORZ_INFO_NPH_SHIFT),
> > OMAP3_ISP_IOMEM_CCDC, ISPCCDC_HORZ_INFO);
> >
> > - isp_reg_writel(isp, (crop->top << ISPCCDC_VERT_START_SLV0_SHIFT) |
> > - (crop->top << ISPCCDC_VERT_START_SLV1_SHIFT),
> > + isp_reg_writel(isp, (slv0 << ISPCCDC_VERT_START_SLV0_SHIFT) |
> > + (slv1 << ISPCCDC_VERT_START_SLV1_SHIFT),
> >
> > OMAP3_ISP_IOMEM_CCDC, ISPCCDC_VERT_START);
> >
> > isp_reg_writel(isp, (crop->height - 1)
> >
> > << ISPCCDC_VERT_LINES_NLV_SHIFT,
> >
> > @@ -2064,6 +2081,14 @@ ccdc_try_format(struct isp_ccdc_device *ccdc,
> > struct
> > v4l2_subdev_fh *fh, fmt->height *= 2;
> >
> > }
> >
> > + /* When input format is interlaced with alternating fields the
> > + * CCDC can pick out just the top or bottom field.
> > + */
> > + if (fmt->field == V4L2_FIELD_ALTERNATE &&
> > + (field == V4L2_FIELD_TOP ||
> > + field == V4L2_FIELD_BOTTOM))
>
> You could combine this check with the one right above into something like
> the following (untested).
>
> /* When the input format is interlaced with alternating fields
> * the CCDC can interleave the fields or selectively capture the
> * top or bottom field.
> */
> if (fmt->field == V4L2_FIELD_ALTERNATE) {
> switch (field) {
> case V4L2_FIELD_INTERLACED_TB:
> case V4L2_FIELD_INTERLACED_BT:
> fmt->height *= 2;
> /* Fall-through */
> case V4L2_FIELD_TOP:
> case V4L2_FIELD_BOTTOM:
> fmt->field = field;
> break;
> }
> }
>
> (an empty default case might be needed to silence compiler warnings)
>
> > + fmt->field = field;
> > +
> >
> > break;
> >
> > case CCDC_PAD_SOURCE_VP:
>
> There's something else that bothers me. If I'm not mistaken, the CCDC will
> generate VD0 and VD1 interrupts for both fields, and the
> ccdc_has_all_fields() logic will wait until both fields have been captured
> before returning the buffer to userspace. This seems to work by chance, and
> will possibly delay the buffer by one field. Shouldn't the function be
> modified to return true when the captured field corresponds to the desired
> field and false otherwise ?
>
> > diff --git a/drivers/media/platform/omap3isp/ispvideo.c
> > b/drivers/media/platform/omap3isp/ispvideo.c index bbbe55d..e636168 100644
> > --- a/drivers/media/platform/omap3isp/ispvideo.c
> > +++ b/drivers/media/platform/omap3isp/ispvideo.c
> > @@ -797,12 +797,12 @@ isp_video_set_format(struct file *file, void *fh,
> > struct v4l2_format *format) /* Fall-through */
> > case V4L2_FIELD_INTERLACED_TB:
> > case V4L2_FIELD_INTERLACED_BT:
> > + case V4L2_FIELD_TOP:
> > + case V4L2_FIELD_BOTTOM:
> > /* Interlaced orders are only supported at the CCDC output. */
> > if (video != &video->isp->isp_ccdc.video_out)
> > format->fmt.pix.field = V4L2_FIELD_NONE;
> > break;
> >
> > - case V4L2_FIELD_TOP:
> > - case V4L2_FIELD_BOTTOM:
> > case V4L2_FIELD_SEQ_TB:
> > case V4L2_FIELD_SEQ_BT:
> > default:
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
