| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <e50ba842-95c0-4ca2-b830-0938ca0c5696@ti.com>
Date: Thu, 6 Nov 2025 16:05:35 +0530
From: Rishikesh Donadkar <r-donadkar@...com>
To: Tomi Valkeinen <tomi.valkeinen@...asonboard.com>, <jai.luthra@...ux.dev>,
<laurent.pinchart@...asonboard.com>, <mripard@...nel.org>
CC: <y-abhilashchandra@...com>, <devarsht@...com>, <s-jain1@...com>,
<vigneshr@...com>, <mchehab@...nel.org>, <robh@...nel.org>,
<krzk+dt@...nel.org>, <p.zabel@...gutronix.de>, <conor+dt@...nel.org>,
<sakari.ailus@...ux.intel.com>, <hverkuil-cisco@...all.nl>,
<jai.luthra@...asonboard.com>, <changhuang.liang@...rfivetech.com>,
<jack.zhu@...rfivetech.com>, <sjoerd@...labora.com>,
<hverkuil+cisco@...nel.org>, <linux-kernel@...r.kernel.org>,
<linux-media@...r.kernel.org>, <devicetree@...r.kernel.org>
Subject: Re: [PATCH v7 09/16] media: ti: j721e-csi2rx: add support for
processing virtual channels
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@...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@...com>
>> Co-developed-by: Pratyush Yadav <p.yadav@...com>
>> Signed-off-by: Pratyush Yadav <p.yadav@...com>
>> Signed-off-by: Jai Luthra <j-luthra@...com>
>> Signed-off-by: Rishikesh Donadkar <r-donadkar@...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);
Powered by blists - more mailing lists