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

["Niklas Söderlund" <niklas.soderlund@ragnatech.se>]

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

* 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
> 

-- 
Regards,
Niklas Söderlund