Re: [PATCH v3 31/33] omap3isp: Remove isp_validate_pipeline and other old stuff

Laurent Pinchart
2012-02-22T11:26:29Z

Hi Sakari,

Thanks for the patch.

On Monday 20 February 2012 03:57:10 Sakari Ailus wrote:
> Remove isp_set_pixel_clock().
>
> Remove set_pixel_clock() callback from platform callbacks since the same
> information is now passed to the ISP driver by other means.
>
> Remove struct ispccdc_vp since the only field in this structure, pixelclk,
> is no longer used.
>
> Remove isp_video_is_shiftable() --- this will live on as ccdc_is_shiftable
> in ispccdc.c.

Could you please move those changes to the patches where you add them to the
other modules ?

> Remove isp_video_validate_pipeline(). Pipeline validation is now split into
> appropriate subdevs, so this can be removed.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@iki.fi>
> ---
> drivers/media/video/omap3isp/isp.c | 14 ---
> drivers/media/video/omap3isp/isp.h | 1 -
> drivers/media/video/omap3isp/ispccdc.h | 10 --
> drivers/media/video/omap3isp/ispvideo.c | 139
> ------------------------------- 4 files changed, 0 insertions(+), 164
> deletions(-)
>
> diff --git a/drivers/media/video/omap3isp/isp.c
> b/drivers/media/video/omap3isp/isp.c index 89a9bf8..4b2290d 100644
> --- a/drivers/media/video/omap3isp/isp.c
> +++ b/drivers/media/video/omap3isp/isp.c
> @@ -329,19 +329,6 @@ void omap3isp_configure_bridge(struct isp_device *isp,
> isp_reg_writel(isp, ispctrl_val, OMAP3_ISP_IOMEM_MAIN, ISP_CTRL);
> }
>
> -/**
> - * isp_set_pixel_clock - Configures the ISP pixel clock
> - * @isp: OMAP3 ISP device
> - * @pixelclk: Average pixel clock in Hz
> - *
> - * Set the average pixel clock required by the sensor. The ISP will use the
> - * lowest possible memory bandwidth settings compatible with the clock. -
> **/
> -static void isp_set_pixel_clock(struct isp_device *isp, unsigned int
> pixelclk) -{
> - isp->isp_ccdc.vpcfg.pixelclk = pixelclk;
> -}
> -
> void omap3isp_hist_dma_done(struct isp_device *isp)
> {
> if (omap3isp_ccdc_busy(&isp->isp_ccdc) ||
> @@ -2116,7 +2103,6 @@ static int isp_probe(struct platform_device *pdev)
>
> isp->autoidle = autoidle;
> isp->platform_cb.set_xclk = isp_set_xclk;
> - isp->platform_cb.set_pixel_clock = isp_set_pixel_clock;
>
> mutex_init(&isp->isp_mutex);
> spin_lock_init(&isp->stat_lock);
> diff --git a/drivers/media/video/omap3isp/isp.h
> b/drivers/media/video/omap3isp/isp.h index 43a1b16..90f3743 100644
> --- a/drivers/media/video/omap3isp/isp.h
> +++ b/drivers/media/video/omap3isp/isp.h
> @@ -126,7 +126,6 @@ struct isp_reg {
>
> struct isp_platform_callback {
> u32 (*set_xclk)(struct isp_device *isp, u32 xclk, u8 xclksel);
> - void (*set_pixel_clock)(struct isp_device *isp, unsigned int pixelclk);
> };
>
> /*
> diff --git a/drivers/media/video/omap3isp/ispccdc.h
> b/drivers/media/video/omap3isp/ispccdc.h index 6d0264b..e570abe 100644
> --- a/drivers/media/video/omap3isp/ispccdc.h
> +++ b/drivers/media/video/omap3isp/ispccdc.h
> @@ -80,14 +80,6 @@ struct ispccdc_syncif {
> u8 bt_r656_en;
> };
>
> -/*
> - * struct ispccdc_vp - Structure for Video Port parameters
> - * @pixelclk: Input pixel clock in Hz
> - */
> -struct ispccdc_vp {
> - unsigned int pixelclk;
> -};
> -
> enum ispccdc_lsc_state {
> LSC_STATE_STOPPED = 0,
> LSC_STATE_STOPPING = 1,
> @@ -161,7 +153,6 @@ struct ispccdc_lsc {
> * @update: Bitmask of controls to update during the next interrupt
> * @shadow_update: Controls update in progress by userspace
> * @syncif: Interface synchronization configuration
> - * @vpcfg: Video port configuration
> * @underrun: A buffer underrun occurred and a new buffer has been queued
> * @state: Streaming state
> * @lock: Serializes shadow_update with interrupt handler
> @@ -190,7 +181,6 @@ struct isp_ccdc_device {
> unsigned int shadow_update;
>
> struct ispccdc_syncif syncif;
> - struct ispccdc_vp vpcfg;
>
> unsigned int underrun:1;
> enum isp_pipeline_stream_state state;
> diff --git a/drivers/media/video/omap3isp/ispvideo.c
> b/drivers/media/video/omap3isp/ispvideo.c index 2e4786d..07832c8 100644
> --- a/drivers/media/video/omap3isp/ispvideo.c
> +++ b/drivers/media/video/omap3isp/ispvideo.c
> @@ -130,37 +130,6 @@ 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;
> -}
> -
> -/*
> * isp_video_mbus_to_pix - Convert v4l2_mbus_framefmt to v4l2_pix_format
> * @video: ISP video instance
> * @mbus: v4l2_mbus_framefmt format (input)
> @@ -284,107 +253,6 @@ isp_video_far_end(struct isp_video *video)
> return far_end;
> }
>
> -/*
> - * Validate a pipeline by checking both ends of all links for format
> - * discrepancies.
> - *
> - * Compute the minimum time per frame value as the maximum of time per
> frame - * limits reported by every block in the pipeline.
> - *
> - * Return 0 if all formats match, or -EPIPE if at least one link is found
> with - * different formats on its two ends or if the pipeline doesn't start
> with a - * video source (either a subdev with no input pad, or a non-subdev
> entity). - */
> -static int isp_video_validate_pipeline(struct isp_pipeline *pipe)
> -{
> - struct isp_device *isp = pipe->output->isp;
> - struct v4l2_subdev_format fmt_source;
> - struct v4l2_subdev_format fmt_sink;
> - struct media_pad *pad;
> - struct v4l2_subdev *subdev;
> - int ret;
> -
> - subdev = isp_video_remote_subdev(pipe->output, NULL);
> - if (subdev == NULL)
> - return -EPIPE;
> -
> - while (1) {
> - unsigned int shifter_link;
> - /* Retrieve the sink format */
> - pad = &subdev->entity.pads[0];
> - if (!(pad->flags & MEDIA_PAD_FL_SINK))
> - break;
> -
> - fmt_sink.pad = pad->index;
> - fmt_sink.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> - ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt_sink);
> - if (ret < 0 && ret != -ENOIOCTLCMD)
> - return -EPIPE;
> -
> - /* Update the maximum frame rate */
> - if (subdev == &isp->isp_res.subdev)
> - omap3isp_resizer_max_rate(&isp->isp_res,
> - &pipe->max_rate);
> -
> - /* Check ccdc maximum data rate when data comes from sensor
> - * TODO: Include ccdc rate in pipe->max_rate and compare the
> - * total pipe rate with the input data rate from sensor.
> - */
> - if (subdev == &isp->isp_ccdc.subdev && pipe->input == NULL) {
> - unsigned int rate = UINT_MAX;
> -
> - omap3isp_ccdc_max_rate(&isp->isp_ccdc, &rate);
> - if (isp->isp_ccdc.vpcfg.pixelclk > rate)
> - return -ENOSPC;
> - }
> -
> - /* If sink pad is on CCDC, the link has the lane shifter
> - * in the middle of it. */
> - shifter_link = subdev == &isp->isp_ccdc.subdev;
> -
> - /* Retrieve the source format. Return an error if no source
> - * entity can be found, and stop checking the pipeline if the
> - * source entity isn't a subdev.
> - */
> - pad = media_entity_remote_source(pad);
> - if (pad == NULL)
> - return -EPIPE;
> -
> - if (media_entity_type(pad->entity) != MEDIA_ENT_T_V4L2_SUBDEV)
> - break;
> -
> - subdev = media_entity_to_v4l2_subdev(pad->entity);
> -
> - fmt_source.pad = pad->index;
> - fmt_source.which = V4L2_SUBDEV_FORMAT_ACTIVE;
> - ret = v4l2_subdev_call(subdev, pad, get_fmt, NULL, &fmt_source);
> - if (ret < 0 && ret != -ENOIOCTLCMD)
> - return -EPIPE;
> -
> - /* Check if the two ends match */
> - if (fmt_source.format.width != fmt_sink.format.width ||
> - fmt_source.format.height != fmt_sink.format.height)
> - return -EPIPE;
> -
> - if (shifter_link) {
> - 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;
> -}
> -
> static int
> __isp_video_get_format(struct isp_video *video, struct v4l2_format *format)
> {
> @@ -1034,13 +902,6 @@ isp_video_streamon(struct file *file, void *fh, enum
> v4l2_buf_type type) pipe->output = far_end;
> }
>
> - /* Validate the pipeline and update its state. */
> - ret = isp_video_validate_pipeline(pipe);
> - if (ret < 0)
> - goto err_isp_video_check_format;
> -
> - pipe->error = false;
> -
> spin_lock_irqsave(&pipe->lock, flags);
> pipe->state &= ~ISP_PIPELINE_STREAM;
> pipe->state |= state;
--
Regards,

Laurent Pinchart