Re: [PATCH v2 11/13] media: cadence: csi2rx: Enable multi-stream support

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

 



Hi Jai

On Tue, Jul 16, 2024 at 03:04:55PM GMT, Jai Luthra wrote:
> Hi Jacopo,
>
> Thanks for the review.
>
> On Jul 12, 2024 at 18:09:48 +0200, Jacopo Mondi wrote:
> > Hi Jai
> >
> > On Thu, Jun 27, 2024 at 06:40:06PM GMT, Jai Luthra wrote:
> > > Cadence CSI-2 bridge IP supports capturing multiple virtual "streams"
> > > of data over the same physical interface using MIPI Virtual Channels.
> > >
> > > The V4L2 subdev APIs should reflect this capability and allow per-stream
> > > routing and controls.
> > >
> > > While the hardware IP supports usecases where streams coming in the sink
> > > pad can be broadcasted to multiple source pads, the driver will need
> > > significant re-architecture to make that possible. The two users of this
> > > IP in mainline linux are TI Shim and StarFive JH7110 CAMSS, and both
> > > have only integrated the first source pad i.e stream0 of this IP. So for
> > > now keep it simple and only allow 1-to-1 mapping of streams from sink to
> > > source, without any broadcasting.
> > >
> > > With stream routing now supported in the driver, implement the
> > > enable_stream and disable_stream hooks in place of the stream-unaware
> > > s_stream hook.
> > >
> > > This allows consumer devices like a DMA bridge or ISP, to enable
> > > particular streams on a source pad, which in turn can be used to enable
> > > only particular streams on the CSI-TX device connected on the sink pad.
> > >
> > > Implement a fallback s_stream hook that internally calls enable_stream
> > > on each source pad, for consumer drivers that don't use multi-stream
> > > APIs to still work.
> > >
> > > Signed-off-by: Jai Luthra <j-luthra@xxxxxx>
> > > ---
> > >  drivers/media/platform/cadence/cdns-csi2rx.c | 407 ++++++++++++++++++++-------
> > >  1 file changed, 313 insertions(+), 94 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> > > index 2ec34fc9c524..b0c91a9c65e8 100644
> > > --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> > > +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> > > @@ -90,6 +90,7 @@ struct csi2rx_priv {
> > >  	struct reset_control		*pixel_rst[CSI2RX_STREAMS_MAX];
> > >  	struct phy			*dphy;
> > >
> > > +	u32				vc_select[CSI2RX_STREAMS_MAX];
> > >  	u8				lanes[CSI2RX_LANES_MAX];
> > >  	u8				num_lanes;
> > >  	u8				max_lanes;
> > > @@ -179,27 +180,43 @@ static void csi2rx_reset(struct csi2rx_priv *csi2rx)
> > >
> > >  static int csi2rx_configure_ext_dphy(struct csi2rx_priv *csi2rx)
> > >  {
> > > +	struct v4l2_ctrl_handler *handler = csi2rx->source_subdev->ctrl_handler;
> > >  	union phy_configure_opts opts = { };
> > >  	struct phy_configure_opts_mipi_dphy *cfg = &opts.mipi_dphy;
> > > -	struct v4l2_subdev_format sd_fmt = {
> > > -		.which	= V4L2_SUBDEV_FORMAT_ACTIVE,
> > > -		.pad	= CSI2RX_PAD_SINK,
> > > -	};
> > > +	struct v4l2_mbus_framefmt *framefmt;
> > > +	struct v4l2_subdev_state *state;
> > >  	const struct csi2rx_fmt *fmt;
> > >  	s64 link_freq;
> > >  	int ret;
> > >
> > > -	ret = v4l2_subdev_call_state_active(&csi2rx->subdev, pad, get_fmt,
> > > -					    &sd_fmt);
> > > -	if (ret < 0)
> > > -		return ret;
> > > +	if (v4l2_ctrl_find(handler, V4L2_CID_LINK_FREQ)) {
> >
> > Do you need to do this by yourself ? afaict v4l2_get_link_freq()
> > already checks if V4L2_CID_LINK_FREQ is available, and if not,
> > fallsback to use PIXEL_RATE.
> >
> > > +		link_freq = v4l2_get_link_freq(handler, 0, 0);
> > > +	} else {
> > > +		state = v4l2_subdev_get_locked_active_state(&csi2rx->subdev);
> > > +		framefmt = v4l2_subdev_state_get_format(state, CSI2RX_PAD_SINK,
> > > +							0);
> > > +
> > > +		if (framefmt) {
> > > +			fmt = csi2rx_get_fmt_by_code(framefmt->code);
> > > +		} else {
> > > +			dev_err(csi2rx->dev,
> > > +				"Did not find active sink format\n");
> > > +			return -EINVAL;
> >
> > Is this possibile ?
> >
> > > +		}
> > >
> > > -	fmt = csi2rx_get_fmt_by_code(sd_fmt.format.code);
> > > +		link_freq = v4l2_get_link_freq(handler, fmt->bpp,
> > > +					       2 * csi2rx->num_lanes);
> >
> > Do we want to allow falling back on PIXEL_RATE for multiplexed
> > transmitters ? I presume this will give you invalid results anyway.
>
> This is mostly done to avoid breaking any single stream sensor that does
> not have the LINK_FREQ control, and was working with this bridge before.
> Thus the warning below for multi-format sources.

Is it possible to allow usage of PIXEL_LINK only for non-multiplexed
transmitters ?

> >
> > I would simply call v4l2_get_link_freq(handler, 0, 0) to force the
> > usage of LINK_FREQ which will become mandatory for transmitters
>
> Ah I did not know LINK_FREQ will be mandatory soon. Any threads I can
> look at where this was discussed?
>

I meant mandatory for multiplexed transmitters, which will have to be
'forced' to use LINK_FREQ as PIXEL_RATE doesn't make much sense for
them

> >
> > >
> > > -	link_freq = v4l2_get_link_freq(csi2rx->source_subdev->ctrl_handler,
> > > -				       fmt->bpp, 2 * csi2rx->num_lanes);
> > > -	if (link_freq < 0)
> > > +		dev_warn(csi2rx->dev,
> > > +			 "Guessing link frequency using bitdepth of stream 0.\n");
> > > +		dev_warn(csi2rx->dev,
> > > +			 "V4L2_CID_LINK_FREQ control is required for multi format sources.\n");
> >
> > Exactly :)
> >
> > > +	}
> > > +
> > > +	if (link_freq < 0) {
> > > +		dev_err(csi2rx->dev, "Unable to calculate link frequency\n");
> > >  		return link_freq;
> > > +	}
> > >
> > >  	ret = phy_mipi_dphy_get_default_config_for_hsclk(link_freq,
> > >  							 csi2rx->num_lanes, cfg);
> > > @@ -222,18 +239,10 @@ static int csi2rx_configure_ext_dphy(struct csi2rx_priv *csi2rx)
> > >  static int csi2rx_start(struct csi2rx_priv *csi2rx)
> > >  {
> > >  	unsigned int i;
> > > -	struct media_pad *remote_pad;
> > >  	unsigned long lanes_used = 0;
> > >  	u32 reg;
> > >  	int ret;
> > >
> > > -	remote_pad = media_pad_remote_pad_first(&csi2rx->pads[CSI2RX_PAD_SINK]);
> > > -	if (!remote_pad) {
> > > -		dev_err(csi2rx->dev,
> > > -			"Failed to find connected source\n");
> > > -		return -ENODEV;
> > > -	}
> > > -
> > >  	ret = clk_prepare_enable(csi2rx->p_clk);
> > >  	if (ret)
> > >  		return ret;
> > > @@ -300,11 +309,7 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
> > >  		writel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF,
> > >  		       csi2rx->base + CSI2RX_STREAM_CFG_REG(i));
> > >
> > > -		/*
> > > -		 * Enable one virtual channel. When multiple virtual channels
> > > -		 * are supported this will have to be changed.
> > > -		 */
> > > -		writel(CSI2RX_STREAM_DATA_CFG_VC_SELECT(0),
> > > +		writel(csi2rx->vc_select[i],
> > >  		       csi2rx->base + CSI2RX_STREAM_DATA_CFG_REG(i));
> > >
> > >  		writel(CSI2RX_STREAM_CTRL_START,
> > > @@ -317,17 +322,10 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
> > >
> > >  	reset_control_deassert(csi2rx->sys_rst);
> > >
> > > -	ret = v4l2_subdev_enable_streams(csi2rx->source_subdev,
> > > -					 remote_pad->index, BIT(0));
> > > -	if (ret)
> > > -		goto err_disable_sysclk;
> > > -
> > >  	clk_disable_unprepare(csi2rx->p_clk);
> > >
> > >  	return 0;
> > >
> > > -err_disable_sysclk:
> > > -	clk_disable_unprepare(csi2rx->sys_clk);
> > >  err_disable_pixclk:
> > >  	for (; i > 0; i--) {
> > >  		reset_control_assert(csi2rx->pixel_rst[i - 1]);
> > > @@ -346,7 +344,6 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
> > >
> > >  static void csi2rx_stop(struct csi2rx_priv *csi2rx)
> > >  {
> > > -	struct media_pad *remote_pad;
> > >  	unsigned int i;
> > >  	u32 val;
> > >  	int ret;
> > > @@ -375,13 +372,6 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx)
> > >  	reset_control_assert(csi2rx->p_rst);
> > >  	clk_disable_unprepare(csi2rx->p_clk);
> > >
> > > -	remote_pad = media_pad_remote_pad_first(&csi2rx->pads[CSI2RX_PAD_SINK]);
> > > -	if (!remote_pad ||
> > > -	    v4l2_subdev_disable_streams(csi2rx->source_subdev,
> > > -					remote_pad->index, BIT(0))) {
> > > -		dev_warn(csi2rx->dev, "Couldn't disable our subdev\n");
> > > -	}
> > > -
> > >  	if (csi2rx->dphy) {
> > >  		writel(0, csi2rx->base + CSI2RX_DPHY_LANE_CTRL_REG);
> > >
> > > @@ -390,47 +380,167 @@ static void csi2rx_stop(struct csi2rx_priv *csi2rx)
> > >  	}
> > >  }
> > >
> > > -static int csi2rx_s_stream(struct v4l2_subdev *subdev, int enable)
> > > +static void csi2rx_update_vc_select(struct csi2rx_priv *csi2rx,
> > > +				    struct v4l2_subdev_state *state)
> > >  {
> > > -	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> > > -	int ret = 0;
> > > +	struct v4l2_mbus_frame_desc fd = {0};
> > > +	struct v4l2_subdev_route *route;
> > > +	unsigned int i;
> > > +	int ret;
> > >
> > > -	if (enable) {
> > > -		ret = pm_runtime_resume_and_get(csi2rx->dev);
> > > -		if (ret < 0)
> > > -			return ret;
> > > +	for (i = 0; i < CSI2RX_STREAMS_MAX; i++)
> > > +		csi2rx->vc_select[i] = 0;
> >
> > or just memset ?
>
> Will do.
>
> >
> > > +
> > > +	ret = csi2rx_get_frame_desc_from_source(csi2rx, &fd);
> > > +	if (ret || fd.type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) {
> > > +		dev_dbg(csi2rx->dev,
> > > +			"Failed to get source frame desc, allowing only VC=0\n");
> >
> > I'm still not sure if picking a default fallback is a good idea or not,
> > as this will only work by chance
> >
> > > +		goto err_no_fd;
> > >  	}
> > >
> > > -	mutex_lock(&csi2rx->lock);
> > > +	/* If source provides per-stream VC info, use it to filter by VC */
> > > +	for_each_active_route(&state->routing, route) {
> > > +		int cdns_stream = route->source_pad - CSI2RX_PAD_SOURCE_STREAM0;
> >
> > unsigned maybe ?
>
> Will fix.
>
> >
> > > +		u8 used_vc = 0;
> > >
> > > -	if (enable) {
> > > -		/*
> > > -		 * If we're not the first users, there's no need to
> > > -		 * enable the whole controller.
> > > -		 */
> > > -		if (!csi2rx->count) {
> > > -			ret = csi2rx_start(csi2rx);
> > > -			if (ret) {
> > > -				pm_runtime_put(csi2rx->dev);
> > > -				goto out;
> > > +		for (i = 0; i < fd.num_entries; i++) {
> > > +			if (fd.entry[i].stream == route->sink_stream) {
> > > +				used_vc = fd.entry[i].bus.csi2.vc;
> > > +				break;
> > >  			}
> > >  		}
> > > +		csi2rx->vc_select[cdns_stream] |=
> >
> > Why |= ? do you expect to be able to capture multiple VC per stream ?
> > I don't think that's allowed ?
>
> The intention here is to allow multiple VC on the same source pad.
> Confusingly enough cadence calls the source pads as "stream0,1,2,3" and
> this driver uses that convention in the macros :/
>
> This cdns-csi2rx bridge is an intermediary between the sensor and ti's
> wrapper IP (platform/ti/j721e-csi2rx.c) which is connected to the first
> source pad of cdns-csi2rx on TI SoCs like AM62A [1]
>
> Ultimately the wrapper will separate incoming streams by VC/DT into
> different DMA channels, so no filtering is done here.
>
> [1]: Section 12.6.4.1 CSI_RX_IF Block Diagram in AM62A TRM
> https://www.ti.com/lit/pdf/spruj16
>
> >
> > > +			CSI2RX_STREAM_DATA_CFG_VC_SELECT(used_vc);
> >
> > In case no stream matching route->sink_stream is found in the remote's
> > frame descriptor this sets CSI2RX_STREAM_DATA_CFG_VC_SELECT(0) for
> > vc_select[x]...
> >
> > > +	}
> > >
> > > -		csi2rx->count++;
> > > -	} else {
> > > -		csi2rx->count--;
> > > +err_no_fd:
> > > +	for (i = 0; i < CSI2RX_STREAMS_MAX; i++) {
> > > +		if (!csi2rx->vc_select[i]) {
> > > +			csi2rx->vc_select[i] =
> > > +				CSI2RX_STREAM_DATA_CFG_VC_SELECT(0);
> >
> > ... so why doing it again here (assuming the no_fd case should be
> > dropped).
>
> Ah good catch, will fix.
>
> >
> > Also, setting multiple streams to capture on VC=0 will multiplex the
> > same stream or cause troubles ?
> >
> > Also, as you have an initial loop already this could maybe be:
> >
> >         /* Initialize all streams to capture VC 0 by default. */
> > 	for (i = 0; i < CSI2RX_STREAMS_MAX; i++)
> > 		csi2rx->vc_select[i] =  CSI2RX_STREAM_DATA_CFG_VC_SELECT(0);
> >
> > 	ret = csi2rx_get_frame_desc_from_source(csi2rx, &fd);
> > 	if (ret || fd.type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2) {
> > 		dev_dbg(csi2rx->dev,
> > 			"Failed to get source frame desc, allowing only VC=0\n");
> > 		return;
> > 	}
> >
> > 	/* If source provides per-stream VC info, use it to filter by VC */
> > 	for_each_active_route(&state->routing, route) {
> > 		int cdns_stream = route->source_pad - CSI2RX_PAD_SOURCE_STREAM0;
> >
> > 		for (i = 0; i < fd.num_entries; i++) {
> > 			if (fd.entry[i].stream != route->sink_stream)
> >                                 continue;
> >
> >                         csi2rx->vc_select[cdns_stream] =
> >                                 CSI2RX_STREAM_DATA_CFG_VC_SELECT(fd.entry[i].bus.csi2.vc);
>
> Thanks will do this in v3.
>
> > 		}
> > 	}
> >
> > > +		}
> > > +	}
> > > +}
> > >
> > > -		/*
> > > -		 * Let the last user turn off the lights.
> > > -		 */
> > > -		if (!csi2rx->count)
> > > -			csi2rx_stop(csi2rx);
> > > +static int csi2rx_enable_streams(struct v4l2_subdev *subdev,
> > > +				 struct v4l2_subdev_state *state, u32 pad,
> > > +				 u64 streams_mask)
> > > +{
> > > +	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> > > +	struct media_pad *remote_pad;
> > > +	u64 sink_streams;
> > > +	int ret;
> > > +
> > > +	remote_pad = media_pad_remote_pad_first(&csi2rx->pads[CSI2RX_PAD_SINK]);
> > > +	if (!remote_pad) {
> > > +		dev_err(csi2rx->dev,
> > > +			"Failed to find connected source\n");
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	ret = pm_runtime_resume_and_get(csi2rx->dev);
> > > +	if (ret < 0)
> > > +		return ret;
> > > +
> > > +	sink_streams = v4l2_subdev_state_xlate_streams(state, pad,
> > > +						       CSI2RX_PAD_SINK,
> > > +						       &streams_mask);
> > >
> > > -		pm_runtime_put(csi2rx->dev);
> > > +	mutex_lock(&csi2rx->lock);
> > > +	/*
> > > +	 * If we're not the first users, there's no need to
> > > +	 * enable the whole controller.
> > > +	 */
> > > +	if (!csi2rx->count) {
> > > +		ret = csi2rx_start(csi2rx);
> > > +		if (ret)
> > > +			goto err_stream_start;
> >
> > I'm not 100% familiar with this new API but I was expecting you
> > wouldn't need to do so
> >
> > >  	}
> > >
> > > -out:
> > > +	/* Start streaming on the source */
> > > +	ret = v4l2_subdev_enable_streams(csi2rx->source_subdev, remote_pad->index,
> > > +					 sink_streams);
> > > +	if (ret) {
> > > +		dev_err(csi2rx->dev,
> > > +			"Failed to start streams %#llx on subdev\n",
> > > +			sink_streams);
> > > +		goto err_subdev_enable;
> > > +	}
> > > +
> > > +	csi2rx->count++;
> > > +	mutex_unlock(&csi2rx->lock);
> > > +
> > > +	return 0;
> > > +
> > > +err_subdev_enable:
> > > +	if (!csi2rx->count)
> > > +		csi2rx_stop(csi2rx);
> > > +err_stream_start:
> > >  	mutex_unlock(&csi2rx->lock);
> > > +	pm_runtime_put(csi2rx->dev);
> > > +	return ret;
> > > +}
> > > +
> > > +static int csi2rx_disable_streams(struct v4l2_subdev *subdev,
> > > +				  struct v4l2_subdev_state *state, u32 pad,
> > > +				  u64 streams_mask)
> > > +{
> > > +	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> > > +	struct media_pad *remote_pad;
> > > +	u64 sink_streams;
> > > +
> > > +	sink_streams = v4l2_subdev_state_xlate_streams(state, pad,
> > > +						       CSI2RX_PAD_SINK,
> > > +						       &streams_mask);
> > > +
> > > +	remote_pad = media_pad_remote_pad_first(&csi2rx->pads[CSI2RX_PAD_SINK]);
> > > +	if (!remote_pad ||
> > > +	    v4l2_subdev_disable_streams(csi2rx->source_subdev,
> > > +					remote_pad->index, sink_streams)) {
> > > +		dev_err(csi2rx->dev, "Couldn't disable our subdev\n");
> > > +	}
> > > +
> > > +	mutex_lock(&csi2rx->lock);
> > > +	csi2rx->count--;
> > > +	/*
> > > +	 * Let the last user turn off the lights.
> > > +	 */
> > > +	if (!csi2rx->count)
> > > +		csi2rx_stop(csi2rx);
> > > +	mutex_unlock(&csi2rx->lock);
> > > +
> > > +	pm_runtime_put(csi2rx->dev);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int csi2rx_s_stream_fallback(struct v4l2_subdev *sd, int enable)
> > > +{
> > > +	struct v4l2_subdev_state *state;
> > > +	struct v4l2_subdev_route *route;
> > > +	u64 mask[CSI2RX_PAD_MAX] = {0};
> > > +	int i, ret;
> > > +
> > > +	/* Find the stream mask on all source pads */
> > > +	state = v4l2_subdev_lock_and_get_active_state(sd);
> > > +	for (i = CSI2RX_PAD_SOURCE_STREAM0; i < CSI2RX_PAD_MAX; i++) {
> > > +		for_each_active_route(&state->routing, route) {
> > > +			if (route->source_pad == i)
> > > +				mask[i] |= BIT_ULL(route->source_stream);
> > > +		}
> > > +	}
> > > +	v4l2_subdev_unlock_state(state);
> > > +
> > > +	/* Start streaming on each pad */
> > > +	for (i = CSI2RX_PAD_SOURCE_STREAM0; i < CSI2RX_PAD_MAX; i++) {
> > > +		if (enable)
> > > +			ret = v4l2_subdev_enable_streams(sd, i, mask[i]);
> > > +		else
> > > +			ret = v4l2_subdev_disable_streams(sd, i, mask[i]);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > >  	return ret;
> > >  }
> > >
> > > @@ -446,12 +556,63 @@ static int csi2rx_enum_mbus_code(struct v4l2_subdev *subdev,
> > >  	return 0;
> > >  }
> > >
> > > +static int _csi2rx_set_routing(struct v4l2_subdev *subdev,
> > > +			       struct v4l2_subdev_state *state,
> > > +			       struct v4l2_subdev_krouting *routing)
> > > +{
> > > +	static const struct v4l2_mbus_framefmt format = {
> > > +		.width = 640,
> > > +		.height = 480,
> > > +		.code = MEDIA_BUS_FMT_UYVY8_1X16,
> > > +		.field = V4L2_FIELD_NONE,
> > > +		.colorspace = V4L2_COLORSPACE_SRGB,
> > > +		.ycbcr_enc = V4L2_YCBCR_ENC_601,
> > > +		.quantization = V4L2_QUANTIZATION_LIM_RANGE,
> > > +		.xfer_func = V4L2_XFER_FUNC_SRGB,
> > > +	};
> > > +	int ret;
> > > +
> > > +	if (routing->num_routes > V4L2_FRAME_DESC_ENTRY_MAX)
> > > +		return -EINVAL;
> >
> > Isn't there a max number of routes supported by the HW ?
> >
>
> I guess yes, 16 VC x 4 output pads should be the total routes possible,
> but that is much greater than the current FRAME_DESC_ENTRY_MAX (= 8)

I was not expecting FRAME_DESC_ENTRY_MAX to be that small :/

>
> This check was re-used from ds90ub9xx.c drivers.
>
> > > +
> > > +	ret = v4l2_subdev_routing_validate(subdev, routing,
> > > +					   V4L2_SUBDEV_ROUTING_ONLY_1_TO_1);
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	ret = v4l2_subdev_set_routing_with_fmt(subdev, state, routing, &format);
> >
> >         return ...
>
> Will fix.
>
> >
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int csi2rx_set_routing(struct v4l2_subdev *subdev,
> > > +			      struct v4l2_subdev_state *state,
> > > +			      enum v4l2_subdev_format_whence which,
> > > +			      struct v4l2_subdev_krouting *routing)
> > > +{
> > > +	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> > > +	int ret;
> > > +
> > > +	if (which == V4L2_SUBDEV_FORMAT_ACTIVE && csi2rx->count)
> > > +		return -EBUSY;
> > > +
> > > +	ret = _csi2rx_set_routing(subdev, state, routing);
> > > +
> >
> > no empty line
>
> Will fix.
>
> >
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	csi2rx_update_vc_select(csi2rx, state);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > >  static int csi2rx_set_fmt(struct v4l2_subdev *subdev,
> > >  			  struct v4l2_subdev_state *state,
> > >  			  struct v4l2_subdev_format *format)
> > >  {
> > >  	struct v4l2_mbus_framefmt *fmt;
> > > -	unsigned int i;
> > >
> > >  	/* No transcoding, source and sink formats must match. */
> > >  	if (format->pad != CSI2RX_PAD_SINK)
> > > @@ -463,14 +624,19 @@ static int csi2rx_set_fmt(struct v4l2_subdev *subdev,
> > >  	format->format.field = V4L2_FIELD_NONE;
> > >
> > >  	/* Set sink format */
> > > -	fmt = v4l2_subdev_state_get_format(state, format->pad);
> > > +	fmt = v4l2_subdev_state_get_format(state, format->pad, format->stream);
> > > +	if (!fmt)
> > > +		return -EINVAL;
> > > +
> > >  	*fmt = format->format;
> > >
> > > -	/* Propagate to source formats */
> > > -	for (i = CSI2RX_PAD_SOURCE_STREAM0; i < CSI2RX_PAD_MAX; i++) {
> > > -		fmt = v4l2_subdev_state_get_format(state, i);
> > > -		*fmt = format->format;
> > > -	}
> > > +	/* Propagate to source format */
> > > +	fmt = v4l2_subdev_state_get_opposite_stream_format(state, format->pad,
> > > +							   format->stream);
> > > +	if (!fmt)
> > > +		return -EINVAL;
> >
> > again I'm not sure this can happen
>
> Will fix.
>
> >
> > > +
> > > +	*fmt = format->format;
> > >
> > >  	return 0;
> > >  }
> > > @@ -478,40 +644,92 @@ static int csi2rx_set_fmt(struct v4l2_subdev *subdev,
> > >  static int csi2rx_init_state(struct v4l2_subdev *subdev,
> > >  			     struct v4l2_subdev_state *state)
> > >  {
> > > -	struct v4l2_subdev_format format = {
> > > -		.pad = CSI2RX_PAD_SINK,
> > > -		.format = {
> > > -			.width = 640,
> > > -			.height = 480,
> > > -			.code = MEDIA_BUS_FMT_UYVY8_1X16,
> > > -			.field = V4L2_FIELD_NONE,
> > > -			.colorspace = V4L2_COLORSPACE_SRGB,
> > > -			.ycbcr_enc = V4L2_YCBCR_ENC_601,
> > > -			.quantization = V4L2_QUANTIZATION_LIM_RANGE,
> > > -			.xfer_func = V4L2_XFER_FUNC_SRGB,
> > > +	struct v4l2_subdev_route routes[] = {
> >
> > static const ?
>
> Will fix.
>
> >
> > > +		{
> > > +			.sink_pad = CSI2RX_PAD_SINK,
> > > +			.sink_stream = 0,
> > > +			.source_pad = CSI2RX_PAD_SOURCE_STREAM0,
> > > +			.source_stream = 0,
> > > +			.flags = V4L2_SUBDEV_ROUTE_FL_ACTIVE,
> > >  		},
> > >  	};
> > >
> > > -	return csi2rx_set_fmt(subdev, state, &format);
> > > +	struct v4l2_subdev_krouting routing = {
> >
> > static const ?
>
> Will fix.
>
> >
> > > +		.num_routes = ARRAY_SIZE(routes),
> > > +		.routes = routes,
> > > +	};
> > > +
> > > +	return _csi2rx_set_routing(subdev, state, &routing);
> > >  }
> > >
> > >  static int csi2rx_get_frame_desc(struct v4l2_subdev *subdev, unsigned int pad,
> > >  				 struct v4l2_mbus_frame_desc *fd)
> > >  {
> > >  	struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> > > +	struct v4l2_mbus_frame_desc source_fd = {0};
> > > +	struct v4l2_subdev_route *route;
> > > +	struct v4l2_subdev_state *state;
> > > +	int ret;
> > > +
> > > +	ret = csi2rx_get_frame_desc_from_source(csi2rx, &source_fd);
> >
> > Should this inspect the remote frame desc or only the local routing
> > table ?
>
> We want to use the correct VC, if the source is sending two streams with
> VC=1 and VC=2, we want to make sure the output streams are also setting
> that data in the frame descriptors for this subdev.
>

Isn't it something related to the routing table validation ?

> >
> > > +	if (ret)
> > > +		return ret;
> > > +
> > > +	fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
> > >
> > > -	return csi2rx_get_frame_desc_from_source(csi2rx, fd);
> > > +	state = v4l2_subdev_lock_and_get_active_state(subdev);
> > > +
> > > +	for_each_active_route(&state->routing, route) {
> > > +		struct v4l2_mbus_frame_desc_entry *source_entry = NULL;
> > > +		unsigned int i;
> > > +
> > > +		if (route->source_pad != pad)
> > > +			continue;
> > > +
> > > +		for (i = 0; i < source_fd.num_entries; i++) {
> > > +			if (source_fd.entry[i].stream == route->sink_stream) {
> > > +				source_entry = &source_fd.entry[i];
> > > +				break;
> > > +			}
> > > +		}
> > > +
> > > +		if (!source_entry) {
> > > +			dev_err(csi2rx->dev,
> > > +				"Failed to find stream from source frame desc\n");
> > > +			ret = -EPIPE;
> > > +			goto err_missing_stream;
> > > +		}
> > > +
> > > +		fd->entry[fd->num_entries].stream = route->source_stream;
> > > +		fd->entry[fd->num_entries].flags = source_entry->flags;
> > > +		fd->entry[fd->num_entries].length = source_entry->length;
> > > +		fd->entry[fd->num_entries].pixelcode = source_entry->pixelcode;
> > > +		fd->entry[fd->num_entries].bus.csi2.vc =
> > > +			source_entry->bus.csi2.vc;
> > > +		fd->entry[fd->num_entries].bus.csi2.dt =
> > > +			source_entry->bus.csi2.dt;
> > > +
> > > +		fd->num_entries++;
> >
> > as you
> >
> > 		if (route->source_pad != pad)
> > 			continue;
> >
> > can you have multiple entris on a pad ?
>
> Yes, this IP will pass through all incoming streams on sink to the first
> source.
>
> >
> > > +	}
> > > +
> > > +err_missing_stream:
> > > +	v4l2_subdev_unlock_state(state);
> > > +
> > > +	return ret;
> > >  }
> > >
> > >  static const struct v4l2_subdev_pad_ops csi2rx_pad_ops = {
> > > -	.enum_mbus_code	= csi2rx_enum_mbus_code,
> > > -	.get_fmt	= v4l2_subdev_get_fmt,
> > > -	.set_fmt	= csi2rx_set_fmt,
> > > -	.get_frame_desc = csi2rx_get_frame_desc,
> > > +	.enum_mbus_code		= csi2rx_enum_mbus_code,
> > > +	.get_fmt		= v4l2_subdev_get_fmt,
> > > +	.set_fmt		= csi2rx_set_fmt,
> > > +	.get_frame_desc		= csi2rx_get_frame_desc,
> > > +	.set_routing		= csi2rx_set_routing,
> > > +	.enable_streams		= csi2rx_enable_streams,
> > > +	.disable_streams	= csi2rx_disable_streams,
> > >  };
> > >
> > >  static const struct v4l2_subdev_video_ops csi2rx_video_ops = {
> > > -	.s_stream	= csi2rx_s_stream,
> > > +	.s_stream	= csi2rx_s_stream_fallback,
> > >  };
> > >
> > >  static const struct v4l2_subdev_ops csi2rx_subdev_ops = {
> > > @@ -766,7 +984,8 @@ static int csi2rx_probe(struct platform_device *pdev)
> > >  	csi2rx->pads[CSI2RX_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
> > >  	for (i = CSI2RX_PAD_SOURCE_STREAM0; i < CSI2RX_PAD_MAX; i++)
> > >  		csi2rx->pads[i].flags = MEDIA_PAD_FL_SOURCE;
> > > -	csi2rx->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> > > +	csi2rx->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> > > +		V4L2_SUBDEV_FL_STREAMS;
> >
> > nit: please align to the previous one
> >
> > 	csi2rx->subdev.flags |= V4L2_SUBDEV_FL_HAS_DEVNODE |
> > 		                V4L2_SUBDEV_FL_STREAMS;
> >
> > and maybe the |=  should be just an =
>
> Will fix.
>
> >
> > Thanks
> >   j
> >
> > >  	csi2rx->subdev.entity.ops = &csi2rx_media_ops;
> > >
> > >  	ret = media_entity_pads_init(&csi2rx->subdev.entity, CSI2RX_PAD_MAX,
> > >
> > > --
> > > 2.43.0
> > >
> > >
>
> --
> Thanks,
> Jai
>
> GPG Fingerprint: 4DE0 D818 E5D5 75E8 D45A AFC5 43DE 91F9 249A 7145


Attachment: signature.asc
Description: PGP signature


[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