Re: [RFC PATCH 1/4] media: rcar-vin: use v4l2_get_link_freq() to calculate phypll frequency

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

Hi Niklas,

On Mon, Mar 08, 2021 at 02:46:52PM +0100, Niklas Söderlund wrote:
> Hi Andrey,
> 
> Thanks for your patch.
> 
> On 2021-03-03 21:08:14 +0300, Andrey Konovalov wrote:
> > To get the link frequency value, or to calculate a parameter depending on
> > it the receiver driver should use V4L2_CID_LINK_FREQ. If V4L2_CID_LINK_FREQ
> > control is not implemented in the remote subdevice, the link frequency
> > can be calculated from V4L2_CID_PIXEL_RATE control value. But the latter
> > may not give the correct link frequency, and should only be used as the
> > last resort. v4l2_get_link_freq() does exactly that, so use it instead
> > of reading V4L2_CID_PIXEL_RATE directly.
> 
> I like the direction this patch is taking, but I'm a bit concerned about 
> that V4L2_CID_LINK_FREQ is not able to replace V4L2_CID_PIXEL_RATE as it 
> is designed today. Maybe my concern is unfounded and only reflects my 
> own misunderstanding of the API.
> 
> When I wrote this code I tried to first do it using V4L2_CID_LINK_FREQ 
> but I found no way to be able to express the wide rang of values needed 
> for my use-case given that V4L2_CID_LINK_FREQ is a menu control. I had 

I think we could make it alternatively a 64-bit integer control if that
helps. The helper needs to be adjusted accordingly.

> to use V4L2_CID_PIXEL_RATE as it allowed me to at runtime calculate and 
> report the link speed based on input formats. The Use-cases I need to 
> address are where CSI-2 transmitter themself are a bridge in the video 
> pipeline, for example

Is the actual bus frequency changed based on this?

Depending on the system where this chip is being used, only certain
frequencies may be allowed on that bus. It would be most straightforward to
use only those, but on the other hand, if any frequency can be used and
that is certain, then I have no objections to allowing that either. We
simply would make the link-frequencies property optional.

> 
> * Case 1 - HDMI video source
> 
> HDMI source -> ADV748x (HDMI-to-CSI-2) ->[CSI-2 bus]-> R-Car CSI-2 receiver
> 
> The R-Car CSI-2 receiver needs to know the CSI-2 link frequency and 
> queries the ADV748x using V4L2_CID_PIXEL_RATE. The ADV748x reports the 
> pixel rate based on the HDMI format detected on its sink pad.
> 
> This could be done using V4L2_CID_LINK_FREQ, but as it's a menu control 
> it becomes rather tricky to populate it with all possible values, but I 
> guess it could be doable?
> 
> * Case 2 - Multiple video streams over a CSI-2 bus (GMSL)
> 
> Camera 1 -|
> Camera 2 -|
> Camera 3 -|---> MAX9286 (GMSL-to CSI-2) ->[CSI-2 bus]-> R-Car CSI-2 receiver
> Camera 4 -|
> 
> The MAX9286 has 4 sink pads each connected to an independent camera and 
> a single CSI-2 source pad. When streaming starts the MAX9286 computes 
> the total CSI-2 link speed as V4L2_CID_PIXEL_RATE based on the format on 
> each of it's 4 sink pads.
> 
> As in case 1 this could be reported by V4L2_CID_LINK_FREQ but I don't 
> see it as feasible to populate the menu control with all possible 
> frequencies before hand.
> 
> Hopefully this is all easily solvable and I have only misunderstood how 
> menu controls work. If not I think this needs to be considered as part 
> of this series as otherwise it could leave the CSI-2 bridge drivers 
> without a path forward.
> 
> > 
> > Signed-off-by: Andrey Konovalov <andrey.konovalov@linaro.org>
> 
> I tested this and it works as expected. Also as expected it prints lots 
> of warnings about the usage of V4L2_CID_PIXEL_RATE :-) Once I understand 
> how I can fix the CSI-2 transmitters used as bridges in the R-Car boards 
> I will be happy to add my tag to this series as well as fix the bridge 
> drivers.
> 
> > ---
> >  drivers/media/platform/rcar-vin/rcar-csi2.c | 18 +++++++-----------
> >  1 file changed, 7 insertions(+), 11 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > index e06cd512aba2..eec8f9dd9060 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> > @@ -455,29 +455,25 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp,
> >  			   unsigned int lanes)
> >  {
> >  	struct v4l2_subdev *source;
> > -	struct v4l2_ctrl *ctrl;
> > -	u64 mbps;
> > +	s64 mbps;
> >  
> >  	if (!priv->remote)
> >  		return -ENODEV;
> >  
> >  	source = priv->remote;
> >  
> > -	/* Read the pixel rate control from remote. */
> > -	ctrl = v4l2_ctrl_find(source->ctrl_handler, V4L2_CID_PIXEL_RATE);
> > -	if (!ctrl) {
> > -		dev_err(priv->dev, "no pixel rate control in subdev %s\n",
> > +	/* Read the link frequency from the remote subdev. */
> > +	mbps = v4l2_get_link_freq(source->ctrl_handler, bpp, 2 * lanes);
> > +	if (mbps < 0) {
> > +		dev_err(priv->dev, "failed to get link rate from subdev %s\n",
> >  			source->name);
> > -		return -EINVAL;
> > +		return mbps;
> >  	}
> > -
> >  	/*
> >  	 * Calculate the phypll in mbps.
> > -	 * link_freq = (pixel_rate * bits_per_sample) / (2 * nr_of_lanes)
> >  	 * bps = link_freq * 2
> >  	 */
> > -	mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
> > -	do_div(mbps, lanes * 1000000);
> > +	do_div(mbps, 1000000 / 2);
> >  
> >  	return mbps;
> >  }
> > -- 
> > 2.17.1
> > 

-- 
Kind regards,

Sakari Ailus