Re: [PATCH v2 6/6] media: rcar-csi2: Negotiate data lanes number

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Jacopo,

Thank you for the patch.

On Wed, Apr 15, 2020 at 12:50:03PM +0200, Jacopo Mondi wrote:
> Use the newly introduced get_mbus_config() subdevice pad operation to
> retrieve the remote subdevice MIPI CSI-2 bus configuration and configure
> the number of active data lanes accordingly.
> 
> In order to be able to call the remote subdevice operation cache the
> index of the remote pad connected to the single CSI-2 input port.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
> 
> Niklas I've not been able to fully address comments as -ENOIOCTL command
> has to be handled separately as reported by email.
> 
> ---
>  drivers/media/platform/rcar-vin/rcar-csi2.c | 53 +++++++++++++++++++--
>  1 file changed, 50 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-csi2.c b/drivers/media/platform/rcar-vin/rcar-csi2.c
> index faa9fb23a2e9..0061d5ff37e3 100644
> --- a/drivers/media/platform/rcar-vin/rcar-csi2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-csi2.c
> @@ -363,6 +363,7 @@ struct rcar_csi2 {
>  	struct v4l2_async_notifier notifier;
>  	struct v4l2_async_subdev asd;
>  	struct v4l2_subdev *remote;
> +	unsigned short remote_pad;

You can make this an unsigned int, it will be more efficient and won't
consume more memory.

> 
>  	struct v4l2_mbus_framefmt mf;
> 
> @@ -371,6 +372,7 @@ struct rcar_csi2 {
> 
>  	unsigned short lanes;
>  	unsigned char lane_swap[4];
> +	unsigned short active_lanes;
>  };
> 
>  static inline struct rcar_csi2 *sd_to_csi2(struct v4l2_subdev *sd)
> @@ -414,7 +416,7 @@ static int rcsi2_wait_phy_start(struct rcar_csi2 *priv)
> 
>  	/* Wait for the clock and data lanes to enter LP-11 state. */
>  	for (timeout = 0; timeout <= 20; timeout++) {
> -		const u32 lane_mask = (1 << priv->lanes) - 1;
> +		const u32 lane_mask = (1 << priv->active_lanes) - 1;
> 
>  		if ((rcsi2_read(priv, PHCLM_REG) & PHCLM_STOPSTATECKL)  &&
>  		    (rcsi2_read(priv, PHDLM_REG) & lane_mask) == lane_mask)
> @@ -471,11 +473,49 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv, unsigned int bpp)
>  	 * bps = link_freq * 2
>  	 */
>  	mbps = v4l2_ctrl_g_ctrl_int64(ctrl) * bpp;
> -	do_div(mbps, priv->lanes * 1000000);
> +	do_div(mbps, priv->active_lanes * 1000000);
> 
>  	return mbps;
>  }
> 
> +static int rcsi2_config_active_lanes(struct rcar_csi2 *priv)
> +{
> +	struct v4l2_mbus_pad_config mbus_config;

 = { 0 };

here and you can remove the memset().

> +	int ret;
> +
> +	priv->active_lanes = priv->lanes;
> +
> +	memset(&mbus_config, 0, sizeof(mbus_config));
> +	ret = v4l2_subdev_call(priv->remote, pad, get_mbus_config,
> +			       priv->remote_pad, &mbus_config);
> +	if (ret == -ENOIOCTLCMD) {
> +		dev_dbg(priv->dev, "No remote mbus configuration available\n");
> +		return 0;
> +	}
> +
> +	if (ret) {
> +		dev_err(priv->dev, "Failed to get remote mbus configuration\n");

In the unlikely case that the source implements the operation but only
supports it on other pads (the adv748x driver returns -EINVAL when
called on the sink pad for instance), is this really an error ? I think
it's related to the question I've asked in another patch, regarding what
error values are allowed for this operation and under what conditions.

> +		return ret;
> +	}
> +
> +	if (mbus_config.type != V4L2_MBUS_CSI2_DPHY) {
> +		dev_err(priv->dev, "Unsupported mbus type %u\n",
> +			mbus_config.type);
> +		return -EINVAL;
> +	}
> +
> +	if (mbus_config.csi2_dphy.data_lanes > priv->lanes) {
> +		dev_err(priv->dev,
> +			"Unsupported mbus config: too many data lanes %u\n",
> +			mbus_config.csi2_dphy.data_lanes);
> +		return -EINVAL;
> +	}
> +
> +	priv->active_lanes = mbus_config.csi2_dphy.data_lanes;
> +
> +	return 0;
> +}
> +
>  static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>  {
>  	const struct rcar_csi2_format *format;
> @@ -490,6 +530,11 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>  	/* Code is validated in set_fmt. */
>  	format = rcsi2_code_to_fmt(priv->mf.code);
> 
> +	/* Get the remote mbus config to get the number of enabled lanes. */
> +	ret = rcsi2_config_active_lanes(priv);
> +	if (ret)
> +		return ret;
> +
>  	/*
>  	 * Enable all supported CSI-2 channels with virtual channel and
>  	 * data type matching.
> @@ -522,7 +567,7 @@ static int rcsi2_start_receiver(struct rcar_csi2 *priv)
>  	}
> 
>  	phycnt = PHYCNT_ENABLECLK;
> -	phycnt |= (1 << priv->lanes) - 1;
> +	phycnt |= (1 << priv->active_lanes) - 1;
> 
>  	mbps = rcsi2_calc_mbps(priv, format->bpp);
>  	if (mbps < 0)
> @@ -748,6 +793,7 @@ static int rcsi2_notify_bound(struct v4l2_async_notifier *notifier,
>  	}
> 
>  	priv->remote = subdev;
> +	priv->remote_pad = pad;
> 
>  	dev_dbg(priv->dev, "Bound %s pad: %d\n", subdev->name, pad);
> 
> @@ -793,6 +839,7 @@ static int rcsi2_parse_v4l2(struct rcar_csi2 *priv,
>  			priv->lanes);
>  		return -EINVAL;
>  	}
> +	priv->active_lanes = priv->lanes;
> 
>  	for (i = 0; i < ARRAY_SIZE(priv->lane_swap); i++) {
>  		priv->lane_swap[i] = i < priv->lanes ?

-- 
Regards,

Laurent Pinchart