Re: [PATCH 16/19] media: rcar-csi2: Support multiplexed transmitters

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Laurent, Niklas

On Fri, May 03, 2024 at 12:56:47AM GMT, Laurent Pinchart wrote:
> On Thu, May 02, 2024 at 04:30:54PM +0200, Niklas Söderlund wrote:
> > On 2024-04-30 12:39:52 +0200, Jacopo Mondi wrote:
> > > Rework the R-Car CSI-2 start routine to support multiplexed
> > > transmitters.
> > >
> > > Configure the CSI-2 receiver MIPI CSI-2 Data Type filtering by inspecting
> > > the remote subdev frame_desc instead of relying on the image format
> > > configured on the sink pad.
> > >
> > > Enable MIPI CSI-2 Data Type filtering by inspecting the remote subdevice
> > > frame descriptor to discern which Data Type is transmitted on which
> > > Virtual Channel. If multiple Data Types are transmitted on the same VC
> > > then Data Type filtering is disabled.
> > >
> > > Rework the per-lane bandwidth calculation to use the LINK_FREQ control
> > > if a transmitter sends multiple streams on the same data link. The
> > > current usage of the PIXEL_RATE control does not support multiplexed
> > > transmitters, as there's not a unique pixel rate among all the possible
> > > source streams.
>
> It sounds like this should be split in two patches, the LINK_FREQ change
> seems separate from the DT/VC filtering and routing handling.
>
> > >
> > > This change makes mandatory for any subdevice that operates with
> > > the R-Car CSI-2 receiver to implement the .get_frame_desc() operation.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo.mondi@xxxxxxxxxxxxxxxx>
> > > ---
> > >  drivers/media/platform/renesas/rcar-csi2.c | 280 ++++++++++++++-------
> > >  1 file changed, 191 insertions(+), 89 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/renesas/rcar-csi2.c b/drivers/media/platform/renesas/rcar-csi2.c
> > > index 82dc0b92b8d3..ffb73272543b 100644
> > > --- a/drivers/media/platform/renesas/rcar-csi2.c
> > > +++ b/drivers/media/platform/renesas/rcar-csi2.c
> > > @@ -69,10 +69,7 @@ struct rcar_csi2;
> > >  #define FLD_REG				0x1c
> > >  #define FLD_FLD_NUM(n)			(((n) & 0xff) << 16)
> > >  #define FLD_DET_SEL(n)			(((n) & 0x3) << 4)
> > > -#define FLD_FLD_EN4			BIT(3)
> > > -#define FLD_FLD_EN3			BIT(2)
> > > -#define FLD_FLD_EN2			BIT(1)
> > > -#define FLD_FLD_EN			BIT(0)
> > > +#define FLD_FLD_EN(n)			BIT((n) & 0xf)
> > >
> > >  /* Automatic Standby Control */
> > >  #define ASTBY_REG			0x20
> > > @@ -575,6 +572,16 @@ static const struct rcar_csi2_format *rcsi2_code_to_fmt(unsigned int code)
> > >  	return NULL;
> > >  }
> > >
> > > +static const struct rcar_csi2_format *rcsi2_datatype_to_fmt(unsigned int dt)
> > > +{
> > > +	for (unsigned int i = 0; i < ARRAY_SIZE(rcar_csi2_formats); i++) {
> > > +		if (rcar_csi2_formats[i].datatype == dt)
> > > +			return &rcar_csi2_formats[i];
> > > +	}
> > > +
> > > +	return NULL;
> > > +}
> > > +
> > >  enum rcar_csi2_pads {
> > >  	RCAR_CSI2_SINK,
> > >  	RCAR_CSI2_SOURCE_VC0,
> > > @@ -587,7 +594,8 @@ enum rcar_csi2_pads {
> > >  struct rcar_csi2_info {
> > >  	int (*init_phtw)(struct rcar_csi2 *priv, unsigned int mbps);
> > >  	int (*phy_post_init)(struct rcar_csi2 *priv);
> > > -	int (*start_receiver)(struct rcar_csi2 *priv);
> > > +	int (*start_receiver)(struct rcar_csi2 *priv,
> > > +			      struct v4l2_subdev_state *state);
> > >  	void (*enter_standby)(struct rcar_csi2 *priv);
> > >  	const struct rcsi2_mbps_reg *hsfreqrange;
> > >  	unsigned int csi0clkfreqrange;
> > > @@ -676,6 +684,32 @@ static int rcsi2_exit_standby(struct rcar_csi2 *priv)
> > >  	return 0;
> > >  }
> > >
> > > +static int rcsi2_get_remote_frame_desc(struct rcar_csi2 *priv,
> > > +				       struct v4l2_mbus_frame_desc *fd)
> > > +{
> > > +	struct media_pad *pad;
> > > +	int ret;
> > > +
> > > +	if (!priv->remote)
> > > +		return -ENOLINK;
> > > +
> > > +	pad = media_pad_remote_pad_unique(&priv->pads[RCAR_CSI2_SINK]);
> > > +	if (IS_ERR(pad))
> > > +		return PTR_ERR(pad);
> > > +
> > > +	ret = v4l2_subdev_call(priv->remote, pad, get_frame_desc,
> > > +			       pad->index, fd);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	if (fd->type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) {
> > > +		dev_err(priv->dev, "Frame desc does not describe a CSI-2 link");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int rcsi2_wait_phy_start(struct rcar_csi2 *priv,
> > >  				unsigned int lanes)
> > >  {
> > > @@ -726,41 +760,6 @@ static int rcsi2_set_phypll(struct rcar_csi2 *priv, unsigned int mbps)
> > >  	return 0;
> > >  }
> > >
> > > -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;
> > > -
> > > -	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",
> > > -			source->name);
> > > -		return -EINVAL;
> > > -	}
> > > -
> > > -	/*
> > > -	 * 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);
> > > -
> > > -	/* Adjust for C-PHY, divide by 2.8. */
> > > -	if (priv->cphy)
> > > -		mbps = div_u64(mbps * 5, 14);
> > > -
> > > -	return mbps;
> > > -}
> > > -
> > >  static int rcsi2_get_active_lanes(struct rcar_csi2 *priv,
> > >  				  unsigned int *lanes)
> > >  {
> > > @@ -808,52 +807,146 @@ static int rcsi2_get_active_lanes(struct rcar_csi2 *priv,
> > >  	return 0;
> > >  }
> > >
> > > -static int rcsi2_start_receiver_gen3(struct rcar_csi2 *priv)
> > > +static int rcsi2_calc_mbps(struct rcar_csi2 *priv,
> > > +			   struct v4l2_mbus_frame_desc *fd, unsigned int lanes)
> > >  {
> >
> > Is there a reason you need to move the rcsi2_calc_mbps()? No problem if
> > so, else if you keep it where it is the diff is easier to read ;-)

It's a leftover from where I had to call rcsi2_get_active_lanes() from
rcsi2_calc_mbps() and had to move the former above the latter.

I forgt to re-sort the two function, sorry about that

>
> It's a good practice to move functions around without changing them in a
> separate patch first, it makes review easier.
>
> > > -	const struct rcar_csi2_format *format;
> > > +	struct v4l2_subdev *source;
> > > +	unsigned int bpp;
> > > +	s64 link_freq;
> > > +	u64 mbps;
> > > +
> > > +	if (!priv->remote)
> > > +		return -ENODEV;
> > > +
> > > +	source = priv->remote;
> > > +
> > > +	/*
> > > +	 * v4l2_get_link_freq() uses V4L2_CID_LINK_FREQ first, and falls back
> > > +	 * to V4L2_CID_PIXEL_RATE if V4L2_CID_LINK_FREQ is not available.
> > > +	 *
> > > +	 * With multistream input there is no single pixel rate, and thus we
> > > +	 * cannot use V4L2_CID_PIXEL_RATE, so we pass 0 as the bpp which
> > > +	 * causes v4l2_get_link_freq() to return an error if it falls back to
> > > +	 * V4L2_CID_PIXEL_RATE.
> > > +	 */
> > > +
> > > +	if (fd->num_entries > 1) {
> > > +		bpp = 0;
> > > +	} else {
> > > +		struct v4l2_mbus_frame_desc_entry *entry = &fd->entry[0];
> > > +		const struct rcar_csi2_format *format;
> > > +
> > > +		format = rcsi2_datatype_to_fmt(entry->bus.csi2.dt);
> > > +		if (WARN_ON(!format))
> > > +			return -EINVAL;
> > > +
> > > +		bpp = format->bpp;
> > > +	}
> > > +
> > > +	/*
> > > +	 * Calculate the phypll in mbps.
> > > +	 * link_freq = (pixel_rate * bits_per_sample) / (2 * nr_of_lanes)
> > > +	 * bps = link_freq * 2
> > > +	 */
> > > +	link_freq = v4l2_get_link_freq(source->ctrl_handler, bpp, 2 * lanes);
> > > +	if (link_freq < 0) {
> > > +		dev_err(priv->dev, "Failed to get remote subdev link freq\n");
> > > +		return link_freq;
> > > +	}
> >
> > I just want to make sure I understand our discussion about on IRC about
> > this. It's this call you aim to replace by getting the link frequency
> > from the frame descriptor once it's available there? If so I think it
> > would be good to make this series depend on that work already and list
> > it as a dependency.
> >
> > > +
> > > +	mbps = 2 * link_freq;
> > > +	do_div(mbps, 1000000);
> > > +
> > > +	/* Adjust for C-PHY, divide by 2.8. */
> > > +	if (priv->cphy)
> > > +		mbps = div_u64(mbps * 5, 14);
> > > +
> > > +	return mbps;
> > > +}
> > > +
> > > +static int rcsi2_start_receiver_gen3(struct rcar_csi2 *priv,
> > > +				     struct v4l2_subdev_state *state)
> > > +{
> > > +	const struct v4l2_subdev_stream_configs *configs;
> > >  	u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0;
> > > +	struct v4l2_mbus_frame_desc fd;
> > >  	unsigned int lanes;
> > > -	unsigned int i;
> > >  	int mbps, ret;
> > >
> > > -	dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
> > > -		priv->mf.width, priv->mf.height,
> > > -		priv->mf.field == V4L2_FIELD_NONE ? 'p' : 'i');
> > > -
> > > -	/* Code is validated in set_fmt. */
> > > -	format = rcsi2_code_to_fmt(priv->mf.code);
> > > -	if (!format)
> > > -		return -EINVAL;
> > > +	/* Get information about multiplexed link. */
> > > +	ret = rcsi2_get_remote_frame_desc(priv, &fd);
> > > +	if (ret)
> > > +		return ret;
> > >
> > >  	/*
> > > -	 * Enable all supported CSI-2 channels with virtual channel and
> > > -	 * data type matching.
> > > +	 * Configure and enable the R-Car CSI-2 channels.
> > >  	 *
> > > -	 * NOTE: It's not possible to get individual datatype for each
> > > -	 *       source virtual channel. Once this is possible in V4L2
> > > -	 *       it should be used here.
> > > +	 * The VC transmitted on the channel is configured by the [CSI-2->VIN]
> > > +	 * link_setup operation, while the data type to match comes from the
> > > +	 * remote subdevice.
> > >  	 */
> > > -	for (i = 0; i < priv->info->num_channels; i++) {
> > > -		u32 vcdt_part;
> > > +	for (unsigned int i = 0; i < priv->info->num_channels; i++) {
> > > +		struct v4l2_mbus_frame_desc_entry *entry = NULL;
> > >
> > > +		/* CSI-2 channel disabled (not linked to any VIN). */
> > >  		if (priv->channel_vc[i] < 0)
> > >  			continue;
> > >
> > > -		vcdt_part = VCDT_SEL_VC(priv->channel_vc[i]) | VCDT_VCDTN_EN |
> > > -			VCDT_SEL_DTN_ON | VCDT_SEL_DT(format->datatype);
> > > +		u32 vcdt_part = VCDT_SEL_VC(priv->channel_vc[i]) |
> > > +				VCDT_VCDTN_EN | VCDT_SEL_DTN_ON;
> > > +
> > > +		/*
> > > +		 * Search the entries that describe the data types on the
> > > +		 * MIPI CSI-2 Virtual Channel assigned to this CSI-2 channel.
> > > +		 */
> > > +		for (unsigned int e = 0; e < fd.num_entries; e++) {
> > > +			if (fd.entry[e].bus.csi2.vc != priv->channel_vc[i])
> > > +				continue;
> > > +
> > > +			/*
> > > +			 * If multiple data types are sent on the same MIPI
> > > +			 * CSI-2 Virtual Channel, disable Data Type matching.
> > > +			 */
> > > +			if (entry) {
> > > +				vcdt_part &= ~VCDT_SEL_DTN_ON;
> > > +				break;
> > > +			}
> > > +
> > > +			entry = &fd.entry[e];
> > > +			vcdt_part |= VCDT_SEL_DT(entry->bus.csi2.dt);
> > > +		}
> > > +
> > > +		if (!entry)
> > > +			continue;
> > >
> > >  		/* Store in correct reg and offset. */
> > >  		if (i < 2)
> > >  			vcdt |= vcdt_part << ((i % 2) * 16);
> > >  		else
> > >  			vcdt2 |= vcdt_part << ((i % 2) * 16);
> > > +
> > > +		dev_dbg(priv->dev, "channel %u: VC = %d, datatype = 0x%x\n",
> > > +			i, priv->channel_vc[i], entry->bus.csi2.dt);
> > >  	}
> > >
> > > -	if (priv->mf.field == V4L2_FIELD_ALTERNATE) {
> > > -		fld = FLD_DET_SEL(1) | FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2
> > > -			| FLD_FLD_EN;
> > > +	/*
> > > +	 * Configure field handling inspecting the formats of the
> > > +	 * sink pad streams.
> > > +	 */
> > > +	configs = &state->stream_configs;
> > > +	for (unsigned int i = 0; i < configs->num_configs; ++i) {
> > > +		const struct v4l2_subdev_stream_config *config = configs->configs;
> > > +
> > > +		if (config->pad != RCAR_CSI2_SINK)
> > > +			continue;
> > >
> > > -		if (priv->mf.height == 240)
> > > +		if (config->fmt.field != V4L2_FIELD_ALTERNATE)
> > > +			continue;
> > > +
> > > +		fld |= FLD_DET_SEL(1) | FLD_FLD_EN(config->stream);
>
> The stream ID is set by the source, and could be anything. You can't
> assume it's a virtual channel number.
>
> > > +
> > > +		/* PAL vs NTSC. */
> > > +		if (config->fmt.height == 240)
> > >  			fld |= FLD_FLD_NUM(0);
> > >  		else
> > >  			fld |= FLD_FLD_NUM(1);
> > > @@ -870,7 +963,7 @@ static int rcsi2_start_receiver_gen3(struct rcar_csi2 *priv)
> > >  	phycnt = PHYCNT_ENABLECLK;
> > >  	phycnt |= (1 << lanes) - 1;
> > >
> > > -	mbps = rcsi2_calc_mbps(priv, format->bpp, lanes);
> > > +	mbps = rcsi2_calc_mbps(priv, &fd, lanes);
> > >  	if (mbps < 0)
> > >  		return mbps;
> > >
> > > @@ -1049,23 +1142,24 @@ static int rcsi2_c_phy_setting_v4h(struct rcar_csi2 *priv, int msps)
> > >  	return 0;
> > >  }
> > >
> > > -static int rcsi2_start_receiver_v4h(struct rcar_csi2 *priv)
> > > +static int rcsi2_start_receiver_v4h(struct rcar_csi2 *priv,
> > > +				    struct v4l2_subdev_state *state)
> > >  {
> > > -	const struct rcar_csi2_format *format;
> > > +	struct v4l2_mbus_frame_desc fd;
> > >  	unsigned int lanes;
> > >  	int msps;
> > >  	int ret;
> > >
> > >  	/* Calculate parameters */
> > > -	format = rcsi2_code_to_fmt(priv->mf.code);
> > > -	if (!format)
> > > -		return -EINVAL;
> > > -
> > >  	ret = rcsi2_get_active_lanes(priv, &lanes);
> > >  	if (ret)
> > >  		return ret;
> > >
> > > -	msps = rcsi2_calc_mbps(priv, format->bpp, lanes);
> > > +	ret = rcsi2_get_remote_frame_desc(priv, &fd);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	msps = rcsi2_calc_mbps(priv, &fd, lanes);
> > >  	if (msps < 0)
> > >  		return msps;
> > >
> > > @@ -1114,7 +1208,7 @@ static int rcsi2_start_receiver_v4h(struct rcar_csi2 *priv)
> > >  	return 0;
> > >  }
> > >
> > > -static int rcsi2_start(struct rcar_csi2 *priv)
> > > +static int rcsi2_start(struct rcar_csi2 *priv, struct v4l2_subdev_state *state)
> > >  {
> > >  	int ret;
> > >
> > > @@ -1122,7 +1216,7 @@ static int rcsi2_start(struct rcar_csi2 *priv)
> > >  	if (ret < 0)
> > >  		return ret;
> > >
> > > -	ret = priv->info->start_receiver(priv);
> > > +	ret = priv->info->start_receiver(priv, state);
> > >  	if (ret) {
> > >  		rcsi2_enter_standby(priv);
> > >  		return ret;
> > > @@ -1146,26 +1240,24 @@ static void rcsi2_stop(struct rcar_csi2 *priv)
> > >  static int rcsi2_s_stream(struct v4l2_subdev *sd, int enable)
> > >  {
> > >  	struct rcar_csi2 *priv = sd_to_csi2(sd);
> > > +	struct v4l2_subdev_state *state;
> > >  	int ret = 0;
> > >
> > > -	mutex_lock(&priv->lock);
> > > +	if (!priv->remote)
> > > +		return -ENODEV;
> > >
> > > -	if (!priv->remote) {
> > > -		ret = -ENODEV;
> > > -		goto out;
> > > -	}
> > > +	state = v4l2_subdev_lock_and_get_active_state(&priv->subdev);
> > >
> > > -	if (enable && priv->stream_count == 0) {
> > > -		ret = rcsi2_start(priv);
> > > -		if (ret)
> > > -			goto out;
> > > -	} else if (!enable && priv->stream_count == 1) {
> > > +	if (enable && priv->stream_count == 0)
> > > +		ret = rcsi2_start(priv, state);
> > > +	else if (!enable && priv->stream_count == 1)
> > >  		rcsi2_stop(priv);
> > > -	}
> > > +	if (ret)
> > > +		goto out;
> > >
> > >  	priv->stream_count += enable ? 1 : -1;
> > >  out:
> > > -	mutex_unlock(&priv->lock);
> > > +	v4l2_subdev_unlock_state(state);
> > >
> > >  	return ret;
> > >  }
> > > @@ -1310,14 +1402,17 @@ static irqreturn_t rcsi2_irq(int irq, void *data)
> > >
> > >  static irqreturn_t rcsi2_irq_thread(int irq, void *data)
> > >  {
> > > +	struct v4l2_subdev_state *state;
> > >  	struct rcar_csi2 *priv = data;
> > >
> > > -	mutex_lock(&priv->lock);
> > > +	state = v4l2_subdev_lock_and_get_active_state(&priv->subdev);
> > > +
> > >  	rcsi2_stop(priv);
> > >  	usleep_range(1000, 2000);
> > > -	if (rcsi2_start(priv))
> > > +	if (rcsi2_start(priv, state))
> > >  		dev_warn(priv->dev, "Failed to restart CSI-2 receiver\n");
> > > -	mutex_unlock(&priv->lock);
> > > +
> > > +	v4l2_subdev_unlock_state(state);
> > >
> > >  	return IRQ_HANDLED;
> > >  }
> > > @@ -1340,6 +1435,13 @@ static int rcsi2_notify_bound(struct v4l2_async_notifier *notifier,
> > >  		return pad;
> > >  	}
> > >
> > > +	if (!v4l2_subdev_has_op(subdev, pad, get_frame_desc)) {
> > > +		dev_err(priv->dev,
> > > +			"Subdev %s bound failed: missing get_frame_desc()\n",
> > > +			subdev->name);
> > > +		return -EINVAL;
> > > +	}
> > > +
> > >  	priv->remote = subdev;
> > >  	priv->remote_pad = pad;
> > >
>
> --
> Regards,
>
> Laurent Pinchart




[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux