Hi Sakari,

Thanks for the comments.

On Wednesday 16 March 2011 16:44:49 Sakari Ailus wrote:
> Hi Michael,
> 
> Thanks for the patch. I have some comments below.
> 
> Michael Jones wrote:
> > To use the lane shifter, set different pixel formats at each end of
> > the link at the CCDC input.
> > 
> > Signed-off-by: Michael Jones <michael.jo...@matrix-vision.de>
> > ---
> > 
> >  drivers/media/video/omap3-isp/isp.c      |    7 ++-
> >  drivers/media/video/omap3-isp/isp.h      |    5 +-
> >  drivers/media/video/omap3-isp/ispccdc.c  |   27 ++++++--
> >  drivers/media/video/omap3-isp/ispvideo.c |  108
> >  ++++++++++++++++++++++++------ drivers/media/video/omap3-isp/ispvideo.h
> >  |    3 +
> >  5 files changed, 120 insertions(+), 30 deletions(-)
> > 
> > diff --git a/drivers/media/video/omap3-isp/isp.c
> > b/drivers/media/video/omap3-isp/isp.c index 08d90fe..866ce09 100644
> > --- a/drivers/media/video/omap3-isp/isp.c
> > +++ b/drivers/media/video/omap3-isp/isp.c
> > @@ -285,7 +285,8 @@ static void isp_power_settings(struct isp_device
> > *isp, int idle)
> > 
> >   */
> >  
> >  void omap3isp_configure_bridge(struct isp_device *isp,
> >  
> >                            enum ccdc_input_entity input,
> > 
> > -                          const struct isp_parallel_platform_data *pdata)
> > +                          const struct isp_parallel_platform_data *pdata,
> > +                          int shift)
> 
> This goes more or less directly to register, so what about u32?
> Definitely unsigned at least.

Agreed.

[snip]

> > @@ -98,6 +116,37 @@ omap3isp_video_format_info(enum v4l2_mbus_pixelcode
> > code)
> > 
> >  }
> >  
> >  /*
> > 
> > + * Decide whether desired output pixel code can be obtained with
> > + * the lane shifter by shifting the input pixel code.
> > + * @in: input pixelcode to shifter
> > + * @out: output pixelcode from shifter
> > + * @additional_shift: # of bits the sensor's LSB is offset from
> > CAMEXT[0] + *
> > + * return true if the combination is possible
> > + * return false otherwise
> > + */
> > +static bool isp_video_is_shiftable(enum v4l2_mbus_pixelcode in,
> > +           enum v4l2_mbus_pixelcode out,
> > +           unsigned int additional_shift)
> > +{
> > +   const struct isp_format_info *in_info, *out_info;
> > +
> > +   if (in == out)
> > +           return true;
> > +
> > +   in_info = omap3isp_video_format_info(in);
> > +   out_info = omap3isp_video_format_info(out);
> > +
> > +   if ((in_info->flavor == 0) || (out_info->flavor == 0))
> > +           return false;
> > +
> > +   if (in_info->flavor != out_info->flavor)
> > +           return false;
> > +
> > +   return in_info->bpp - out_info->bpp + additional_shift <= 6;
> 
> Currently there are no formats that would behave badly in this check?
> Perhaps it'd be good idea to take that into consideration. The shift
> that can be done is even.

I've asked Michael to remove the check because we have no misbehaving formats 
:-) Do you think we need to add a check back ?

> > +}
> > +
> > +/*
> > 
> >   * isp_video_mbus_to_pix - Convert v4l2_mbus_framefmt to v4l2_pix_format
> >   * @video: ISP video instance
> >   * @mbus: v4l2_mbus_framefmt format (input)
> > 
> > @@ -247,6 +296,7 @@ static int isp_video_validate_pipeline(struct
> > isp_pipeline *pipe)
> > 
> >             return -EPIPE;
> >     
> >     while (1) {
> > 
> > +           unsigned int link_has_shifter;
> 
> link_has_shifter is only used in one place. Would it be cleaner to test
> below if it's the CCDC? A comment there could be nice, too.

I would like that better as well, but between the line where link_has_shifter 
is set and the line where it is checked, the subdev variable changes so we 
can't just check subdev == &isp->isp_ccdc.subdev there.

> >             /* Retrieve the sink format */
> >             pad = &subdev->entity.pads[0];
> >             if (!(pad->flags & MEDIA_PAD_FL_SINK))
> > 
> > @@ -275,6 +325,10 @@ static int isp_video_validate_pipeline(struct
> > isp_pipeline *pipe)
> > 
> >                             return -ENOSPC;
> >             
> >             }
> > 
> > +           /* If sink pad is on CCDC, the link has the lane shifter
> > +            * in the middle of it. */
> > +           link_has_shifter = subdev == &isp->isp_ccdc.subdev;
> > +
> > 
> >             /* Retrieve the source format */
> >             pad = media_entity_remote_source(pad);
> >             if (pad == NULL ||
> > 
> > @@ -290,10 +344,24 @@ static int isp_video_validate_pipeline(struct
> > isp_pipeline *pipe)
> > 
> >                     return -EPIPE;
> >             
> >             /* Check if the two ends match */
> > 
> > -           if (fmt_source.format.code != fmt_sink.format.code ||
> > -               fmt_source.format.width != fmt_sink.format.width ||
> > +           if (fmt_source.format.width != fmt_sink.format.width ||
> > 
> >                 fmt_source.format.height != fmt_sink.format.height)
> >                     
> >                     return -EPIPE;
> > 
> > +
> > +           if (link_has_shifter) {
> > +                   unsigned int parallel_shift = 0;
> > +                   if (isp->isp_ccdc.input == CCDC_INPUT_PARALLEL) {
> > +                           struct isp_parallel_platform_data *pdata =
> > +                                   &((struct isp_v4l2_subdevs_group *)
> > +                                         subdev->host_priv)->bus.parallel;
> > +                           parallel_shift = pdata->data_lane_shift * 2;
> > +                   }
> > +                   if (!isp_video_is_shiftable(fmt_source.format.code,
> > +                                           fmt_sink.format.code,
> > +                                           parallel_shift))
> > +                           return -EPIPE;
> > +           } else if (fmt_source.format.code != fmt_sink.format.code)
> > +                   return -EPIPE;
> > 
> >     }
> >     
> >     return 0;
> > 

-- 
Regards,

Laurent Pinchart
--
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

Reply via email to