Re: [RFC PATCH 4/4] staging: media: omap4iss: use v4l2_get_link_freq() to get the external rate

[Sakari Ailus <sakari.ailus@linux.intel.com>]

Hi Andrey,

Thanks for the set.

On Wed, Mar 03, 2021 at 09:08:17PM +0300, Andrey Konovalov wrote:
> This driver uses V4L2_CID_PIXEL_RATE to calculate the CSI2 link frequency,
> but this may give incorrect result in some cases. Use v4l2_get_link_freq()
> instead.
> 
> Also the driver used the external_rate field in struct iss_pipeline as a
> flag to prevent excessive v4l2_subdev_call's when processing the frames
> in single-shot mode. Replace the external_rate with external_lfreq, and
> use external_bpp and external_lfreq to call v4l2_subdev_call(get_fmt) and
> v4l2_get_link_freq() respectively only once per iss_video_streamon().
> 
> Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> ---
>  drivers/staging/media/omap4iss/iss.c        | 12 +-----------
>  drivers/staging/media/omap4iss/iss_csiphy.c | 19 ++++++++++++++++---
>  drivers/staging/media/omap4iss/iss_video.c  |  2 +-
>  drivers/staging/media/omap4iss/iss_video.h  |  2 +-
>  4 files changed, 19 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/staging/media/omap4iss/iss.c b/drivers/staging/media/omap4iss/iss.c
> index dae9073e7d3c..0eb7b1b5dcc4 100644
> --- a/drivers/staging/media/omap4iss/iss.c
> +++ b/drivers/staging/media/omap4iss/iss.c
> @@ -131,7 +131,7 @@ int omap4iss_get_external_info(struct iss_pipeline *pipe,
>  	if (!pipe->external)
>  		return 0;
>  
> -	if (pipe->external_rate)
> +	if (pipe->external_bpp)
>  		return 0;
>  
>  	memset(&fmt, 0, sizeof(fmt));
> @@ -145,16 +145,6 @@ int omap4iss_get_external_info(struct iss_pipeline *pipe,
>  
>  	pipe->external_bpp = omap4iss_video_format_info(fmt.format.code)->bpp;
>  
> -	ctrl = v4l2_ctrl_find(pipe->external->ctrl_handler,
> -			      V4L2_CID_PIXEL_RATE);
> -	if (!ctrl) {
> -		dev_warn(iss->dev, "no pixel rate control in subdev %s\n",
> -			 pipe->external->name);
> -		return -EPIPE;
> -	}
> -
> -	pipe->external_rate = v4l2_ctrl_g_ctrl_int64(ctrl);
> -
>  	return 0;
>  }
>  
> diff --git a/drivers/staging/media/omap4iss/iss_csiphy.c b/drivers/staging/media/omap4iss/iss_csiphy.c
> index 96f2ce045138..cec0cd21f7e0 100644
> --- a/drivers/staging/media/omap4iss/iss_csiphy.c
> +++ b/drivers/staging/media/omap4iss/iss_csiphy.c
> @@ -119,6 +119,7 @@ int omap4iss_csiphy_config(struct iss_device *iss,
>  	struct iss_pipeline *pipe = to_iss_pipeline(&csi2_subdev->entity);
>  	struct iss_v4l2_subdevs_group *subdevs = pipe->external->host_priv;
>  	struct iss_csiphy_dphy_cfg csi2phy;
> +	s64 link_freq;
>  	int csi2_ddrclk_khz;
>  	struct iss_csiphy_lanes_cfg *lanes;
>  	unsigned int used_lanes = 0;
> @@ -193,9 +194,21 @@ int omap4iss_csiphy_config(struct iss_device *iss,
>  	if (lanes->clk.pos == 0 || used_lanes & (1 << lanes->clk.pos))
>  		return -EINVAL;
>  
> -	csi2_ddrclk_khz = pipe->external_rate / 1000
> -		/ (2 * csi2->phy->used_data_lanes)
> -		* pipe->external_bpp;
> +	if (!pipe->external_lfreq) {
> +		link_freq = v4l2_get_link_freq(pipe->external->ctrl_handler,

I wonder if you could this unconditionally, and remove external_lfreq field
altogether. The same could be done for external_bpp but that's out of scope
for this patch.

> +					       pipe->external_bpp,
> +					       2 * csi2->phy->used_data_lanes);
> +		if (link_freq < 0) {
> +			dev_warn(iss->dev,
> +				 "failed to read the link frequency fromn subdev %s\n",
> +				 pipe->external->name);
> +			return -EINVAL;
> +		}
> +
> +		pipe->external_lfreq = link_freq;
> +	}
> +
> +	csi2_ddrclk_khz = div_s64(pipe->external_lfreq, 1000);
>  
>  	/*
>  	 * THS_TERM: Programmed value = ceil(12.5 ns/DDRClk period) - 1.
> diff --git a/drivers/staging/media/omap4iss/iss_video.c b/drivers/staging/media/omap4iss/iss_video.c
> index 66975a37dc85..a654c8d18bbc 100644
> --- a/drivers/staging/media/omap4iss/iss_video.c
> +++ b/drivers/staging/media/omap4iss/iss_video.c
> @@ -872,7 +872,7 @@ iss_video_streamon(struct file *file, void *fh, enum v4l2_buf_type type)
>  	pipe = entity->pipe
>  	     ? to_iss_pipeline(entity) : &video->pipe;
>  	pipe->external = NULL;
> -	pipe->external_rate = 0;
> +	pipe->external_lfreq = 0;
>  	pipe->external_bpp = 0;
>  
>  	ret = media_entity_enum_init(&pipe->ent_enum, entity->graph_obj.mdev);
> diff --git a/drivers/staging/media/omap4iss/iss_video.h b/drivers/staging/media/omap4iss/iss_video.h
> index 526281bf0051..2ad5c8483958 100644
> --- a/drivers/staging/media/omap4iss/iss_video.h
> +++ b/drivers/staging/media/omap4iss/iss_video.h
> @@ -86,7 +86,7 @@ struct iss_pipeline {
>  	bool error;
>  	struct v4l2_fract max_timeperframe;
>  	struct v4l2_subdev *external;
> -	unsigned int external_rate;
> +	s64 external_lfreq;
>  	int external_bpp;
>  };
>  

-- 
Sakari Ailus