Re: [PATCH v7 09/16] media: ti: j721e-csi2rx: add support for processing virtual channels

[Rishikesh Donadkar <r-donadkar@ti.com>]

On 22/09/25 19:00, Tomi Valkeinen wrote:
> Hi,


Hi Tomi,

Thank you for the review.

>
> On 11/09/2025 13:28, Rishikesh Donadkar wrote:
>> From: Jai Luthra <j-luthra@ti.com>
>>
>> Use get_frame_desc() to get the frame desc from the connected source,
>> and use the provided virtual channel instead of hardcoded one.
>>
>> get_frame_desc() returns the same information when called on each stream
>> start, so instead get the VCs for all the routed stream at first
>> stream start and cache this information in the driver.
>>
>> get_frame_desc() works per stream, but as we don't support multiple
>> streams yet, we will just always use stream 0. If the source doesn't
>> support get_frame_desc(), fall back to the previous method of always
>> capturing virtual channel 0.
>>
>> Reviewed-by: Yemike Abhilash Chandra <y-abhilashchandra@ti.com>
>> Co-developed-by: Pratyush Yadav <p.yadav@ti.com>
>> Signed-off-by: Pratyush Yadav <p.yadav@ti.com>
>> Signed-off-by: Jai Luthra <j-luthra@ti.com>
>> Signed-off-by: Rishikesh Donadkar <r-donadkar@ti.com>
>> ---
>>   .../platform/ti/j721e-csi2rx/j721e-csi2rx.c   | 107 ++++++++++++++++++
>>   1 file changed, 107 insertions(+)
>>
>> diff --git a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
>> index f3443c3ab01a..903722b5aea7 100644
>> --- a/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
>> +++ b/drivers/media/platform/ti/j721e-csi2rx/j721e-csi2rx.c
>> @@ -32,6 +32,7 @@
>>   #define SHIM_DMACNTX_YUV422		GENMASK(27, 26)
>>   #define SHIM_DMACNTX_DUAL_PCK_CFG	BIT(24)
>>   #define SHIM_DMACNTX_SIZE		GENMASK(21, 20)
>> +#define SHIM_DMACNTX_VC			GENMASK(9, 6)
>>   #define SHIM_DMACNTX_FMT		GENMASK(5, 0)
>>   #define SHIM_DMACNTX_YUV422_MODE_11	3
>>   #define SHIM_DMACNTX_SIZE_8		0
>> @@ -103,6 +104,7 @@ struct ti_csi2rx_dev;
>>   
>>   struct ti_csi2rx_ctx {
>>   	struct ti_csi2rx_dev		*csi;
>> +	struct v4l2_subdev_route	*route;
>>   	struct video_device		vdev;
>>   	struct vb2_queue		vidq;
>>   	struct mutex			mutex; /* To serialize ioctls. */
>> @@ -111,6 +113,8 @@ struct ti_csi2rx_ctx {
>>   	struct media_pad		pad;
>>   	u32				sequence;
>>   	u32				idx;
>> +	u32				vc;
>> +	u32				stream;
>>   };
>>   
>>   struct ti_csi2rx_dev {
>> @@ -134,6 +138,7 @@ struct ti_csi2rx_dev {
>>   		dma_addr_t		paddr;
>>   		size_t			len;
>>   	} drain;
>> +	bool				vc_cached;
>>   };
>>   
>>   static inline struct ti_csi2rx_dev *to_csi2rx_dev(struct v4l2_subdev *sd)
>> @@ -610,6 +615,7 @@ static void ti_csi2rx_setup_shim(struct ti_csi2rx_ctx *ctx)
>>   	}
>>   
>>   	reg |= FIELD_PREP(SHIM_DMACNTX_SIZE, fmt->size);
>> +	reg |= FIELD_PREP(SHIM_DMACNTX_VC, ctx->vc);
>>   
>>   	writel(reg, csi->shim + SHIM_DMACNTX(ctx->idx));
>>   
>> @@ -884,6 +890,82 @@ static void ti_csi2rx_buffer_queue(struct vb2_buffer *vb)
>>   	}
>>   }
>>   
>> +static int ti_csi2rx_get_route(struct ti_csi2rx_ctx *ctx)
>> +{
>> +	struct ti_csi2rx_dev *csi = ctx->csi;
>> +	struct media_pad *pad;
>> +	struct v4l2_subdev_state *state;
>> +	struct v4l2_subdev_route *r;
>> +
>> +	/* Get the source pad connected to this ctx */
>> +	pad = media_entity_remote_source_pad_unique(ctx->pad.entity);
>> +	if (!pad) {
>> +		dev_err(csi->dev, "No pad connected to ctx %d\n", ctx->idx);
>> +		return -ENODEV;
>> +	}
>> +
>> +	state = v4l2_subdev_lock_and_get_active_state(&csi->subdev);
>> +
>> +	for_each_active_route(&state->routing, r) {
>> +		if (!(r->flags & V4L2_SUBDEV_ROUTE_FL_ACTIVE))
>> +			continue;
>> +		if (r->source_pad != pad->index)
>> +			continue;
>> +
>> +		ctx->route = r;
>> +	}
>> +
>> +	v4l2_subdev_unlock_state(state);
> I'm still learning the structure for this driver, but the pattern here
> does not look right. If you unlock the state, the state can change. You
> can't store the route and expect it to stay the same after unlock.
> Here you are even storing a pointer to the state, which can be dangerous.


In the driver we don't allow anyone to update the routes when we are 
streaming (i.e. if csi->enable_count > 0, return -EBUSY). So, it is 
unlikely that the route may change here.

Also, looking at it more I realized that this changes should not be done 
in this patch as routing support is added later in this series([PATCH v7 
11/16] media: ti: j721e-csi2rx: add multistream support).


I will shift these changes to the above patch in the next revision.

Rishikesh


>
> Our current v4l2 state locking is not perfect and we need to do some
> manual locking/unlocking in the drivers for now, and, indeed, we just
> have to expect that it doesn't change. But that has to happen inside the
> enable/disable path, with such locking that it's "as safe as possible",
> which means locking early, as much as possible and keeping the lock
> during the processing. In many cases the framework deals with that (and
> that's our future target generally).
>
> In the subdev enable path, .enable_streams already has the state locked
> and the driver doesn't have to deal with it. If using s_stream (but why
> would you?) you need to lock it yourself, and it usually makes sense to
> lock almost as the first thing, and pass the state forward (or use
> v4l2_subdev_get_locked_active_state()).
>
> Here all this processing is happening from ti_csi2rx_start_streaming(),
> so not even from the subdev's ops. So I'm not sure yet how to clean this
> up, but, as I said, it just doesn't look right.
>
>   Tomi
>
>> +
>> +	if (!ctx->route)
>> +		return -ENODEV;
>> +
>> +	return 0;
>> +}
>> +
>> +static int ti_csi2rx_get_vc(struct ti_csi2rx_ctx *ctx)
>> +{
>> +	struct ti_csi2rx_dev *csi = ctx->csi;
>> +	struct ti_csi2rx_ctx *curr_ctx;
>> +	struct v4l2_mbus_frame_desc fd;
>> +	struct media_pad *source_pad;
>> +	struct v4l2_subdev_route *curr_route;
>> +	int ret;
>> +	unsigned int i, j;
>> +
>> +	/* Get the frame desc form source */
>> +	source_pad = media_entity_remote_pad_unique(&csi->subdev.entity, MEDIA_PAD_FL_SOURCE);
>> +	if (!source_pad)
>> +		return -ENODEV;
>> +
>> +	ret = v4l2_subdev_call(csi->source, pad, get_frame_desc, source_pad->index, &fd);
>> +	if (ret)
>> +		return ret;
>> +
>> +	if (fd.type != V4L2_MBUS_FRAME_DESC_TYPE_CSI2)
>> +		return -EINVAL;
>> +
>> +	for (i = 0; i < csi->num_ctx; i++) {
>> +		curr_ctx = &csi->ctx[i];
>> +
>> +		/* Capture VC 0 by default */
>> +		curr_ctx->vc = 0;
>> +
>> +		ret = ti_csi2rx_get_route(curr_ctx);
>> +		if (ret)
>> +			continue;
>> +
>> +		curr_route = curr_ctx->route;
>> +		curr_ctx->stream = curr_route->sink_stream;
>> +
>> +		for (j = 0; j < fd.num_entries; j++)
>> +			if (curr_ctx->stream == fd.entry[j].stream)
>> +				curr_ctx->vc = fd.entry[j].bus.csi2.vc;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static int ti_csi2rx_start_streaming(struct vb2_queue *vq, unsigned int count)
>>   {
>>   	struct ti_csi2rx_ctx *ctx = vb2_get_drv_priv(vq);
>> @@ -904,6 +986,25 @@ static int ti_csi2rx_start_streaming(struct vb2_queue *vq, unsigned int count)
>>   	if (ret)
>>   		goto err;
>>   
>> +	/* If no stream is routed to this ctx, exit early */
>> +	ret = ti_csi2rx_get_route(ctx);
>> +	if (ret)
>> +		goto err;
>> +
>> +	/* Get the VC for all enabled ctx on first stream start */
>> +	mutex_lock(&csi->mutex);
>> +	if (!csi->vc_cached) {
>> +		ret = ti_csi2rx_get_vc(ctx);
>> +		if (ret == -ENOIOCTLCMD) {
>> +			ctx->vc = 0;
>> +		} else if (ret < 0) {
>> +			mutex_unlock(&csi->mutex);
>> +			goto err;
>> +		}
>> +		csi->vc_cached = true;
>> +	}
>> +	mutex_unlock(&csi->mutex);
>> +
>>   	ti_csi2rx_setup_shim(ctx);
>>   
>>   	ctx->sequence = 0;
>> @@ -950,6 +1051,10 @@ static void ti_csi2rx_stop_streaming(struct vb2_queue *vq)
>>   	writel(0, csi->shim + SHIM_CNTL);
>>   	writel(0, csi->shim + SHIM_DMACNTX(ctx->idx));
>>   
>> +	mutex_lock(&csi->mutex);
>> +	csi->vc_cached = false;
>> +	mutex_unlock(&csi->mutex);
>> +
>>   	ret = v4l2_subdev_call(&csi->subdev, video, s_stream, 0);
>>   	if (ret)
>>   		dev_err(csi->dev, "Failed to stop subdev stream\n");
>> @@ -1303,6 +1408,8 @@ static int ti_csi2rx_init_ctx(struct ti_csi2rx_ctx *ctx)
>>   
>>   	ti_csi2rx_fill_fmt(fmt, &ctx->v_fmt);
>>   
>> +	ctx->route = NULL;
>> +
>>   	ctx->pad.flags = MEDIA_PAD_FL_SINK;
>>   	vdev->entity.ops = &ti_csi2rx_video_entity_ops;
>>   	ret = media_entity_pads_init(&ctx->vdev.entity, 1, &ctx->pad);