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,

On Tue, Mar 09, 2021 at 02:24:41PM +0300, Andrey Konovalov wrote:
> Hi Sakari,
> 
> On 05.03.2021 18:41, Sakari Ailus wrote:
> > 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.
> 
> Hard to tell.
> This might be possible as all this logic to prevent multiple v4l2_subdev_call(get_fmt)
> and v4l2_get_link_freq() calls per single iss_video_streamon() seems to be needed
> only when ISS operates in memory-to-memory mode. Not sure how link frequency, and
> used_lanes are related to memory-to-memory mode... Will try to find out.

It seems the same pattern is used in the omap3isp driver. Both should be
changed at the same time. May well be out of scope now.

-- 
Kind regards,

Sakari Ailus