| 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: <b483e725-4e3e-45b9-9a20-001fe1c7acf0@ideasonboard.com>
Date: Tue, 16 Dec 2025 14:58:57 +0200
From: Tomi Valkeinen <tomi.valkeinen+renesas@...asonboard.com>
To: Laurent Pinchart <laurent.pinchart@...asonboard.com>
Cc: Niklas Söderlund <niklas.soderlund@...natech.se>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
Sakari Ailus <sakari.ailus@...ux.intel.com>, linux-media@...r.kernel.org,
linux-renesas-soc@...r.kernel.org, linux-kernel@...r.kernel.org,
Niklas Söderlund <niklas.soderlund+renesas@...natech.se>,
Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
Jacopo Mondi <jacopo.mondi@...asonboard.com>
Subject: Re: [PATCH v3 13/15] media: rcar-csi2: Call get_frame_desc to find
out VC & DT (Gen3)
Hi,
On 02/06/2025 16:49, Laurent Pinchart wrote:
> Hi Tomi,
>
> Thank you for the patch.
>
> On Fri, May 30, 2025 at 04:50:42PM +0300, Tomi Valkeinen wrote:
>> Call get_frame_desc to find out VC & DT, for Gen3 platforms, instead of
>> hardcoding the VC routing and deducing the DT based on the mbus format.
>>
>> If the source subdevice doesn't implement .get_frame_desc, we use a
>> fallback case where we assume there's a single stream with VC = 0 and DT
>> based on the mbus format.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@...asonboard.com>
>> ---
>> drivers/media/platform/renesas/rcar-csi2.c | 113 +++++++++++++++++++----------
>> 1 file changed, 76 insertions(+), 37 deletions(-)
>>
>> diff --git a/drivers/media/platform/renesas/rcar-csi2.c b/drivers/media/platform/renesas/rcar-csi2.c
>> index b9f83aae725a..8f708196ef49 100644
>> --- a/drivers/media/platform/renesas/rcar-csi2.c
>> +++ b/drivers/media/platform/renesas/rcar-csi2.c
>> @@ -71,10 +71,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(ch) BIT(ch)
>>
>> /* Automatic Standby Control */
>> #define ASTBY_REG 0x20
>> @@ -1066,52 +1063,94 @@ static int rcsi2_calc_mbps(struct rcar_csi2 *priv,
>> static int rcsi2_start_receiver_gen3(struct rcar_csi2 *priv,
>> struct v4l2_subdev_state *state)
>> {
>> - const struct rcar_csi2_format *format;
>> - u32 phycnt, vcdt = 0, vcdt2 = 0, fld = 0;
>> - const struct v4l2_mbus_framefmt *fmt;
>> + u32 phycnt, vcdt = 0, vcdt2 = 0;
>> + u32 fld = FLD_DET_SEL(1);
>> + struct v4l2_mbus_frame_desc source_fd;
>> + struct v4l2_subdev_route *route;
>> unsigned int lanes;
>> - unsigned int i;
>> int mbps, ret;
>> + u8 ch = 0;
>>
>> - /* Use the format on the sink pad to compute the receiver config. */
>> - fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK, 0);
>> + ret = v4l2_subdev_call(priv->remote, pad, get_frame_desc,
>> + priv->remote_pad, &source_fd);
>> + if (ret && ret != -ENOIOCTLCMD) {
>> + return ret;
>> + } else if (ret == -ENOIOCTLCMD) {
>
> if (ret && ret != -ENOIOCTLCMD)
> return ret;
>
> if (ret == -ENOIOCTLCMD) {
Yep, that's cleaner.
>> + /* Create a fallback source_fd */
>> + struct v4l2_mbus_frame_desc *fd = &source_fd;
>> + const struct rcar_csi2_format *format;
>> + struct v4l2_mbus_framefmt *fmt;
>>
>> - dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
>> - fmt->width, fmt->height,
>> - fmt->field == V4L2_FIELD_NONE ? 'p' : 'i');
>> + fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK, 0);
>> + if (!fmt)
>> + return -EINVAL;
>>
>> - /* Code is validated in set_fmt. */
>> - format = rcsi2_code_to_fmt(fmt->code);
>> - if (!format)
>> - return -EINVAL;
>> + format = rcsi2_code_to_fmt(fmt->code);
>> + if (!format)
>> + return -EINVAL;
>>
>> - /*
>> - * Enable all supported CSI-2 channels with virtual channel and
>> - * data type matching.
>> - *
>> - * 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.
>> - */
>> - for (i = 0; i < priv->info->num_channels; i++) {
>> + memset(fd, 0, sizeof(*fd));
>> +
>> + fd->num_entries = 1;
>> + fd->type = V4L2_MBUS_FRAME_DESC_TYPE_CSI2;
>> + fd->entry[0].stream = 0;
>> + fd->entry[0].pixelcode = fmt->code;
>> + fd->entry[0].bus.csi2.vc = 0;
>> + fd->entry[0].bus.csi2.dt = format->datatype;
>> + }
>> +
>> + for_each_active_route(&state->routing, route) {
>> + struct v4l2_mbus_frame_desc_entry *source_entry = NULL;
>
> const
Yep.
>> + const struct v4l2_mbus_framefmt *fmt;
>> + const struct rcar_csi2_format *format;
>> + unsigned int i;
>> + u8 vc, dt;
>> u32 vcdt_part;
>>
>> - if (priv->channel_vc[i] < 0)
>> - continue;
>> + for (i = 0; i < source_fd.num_entries; i++) {
>> + if (source_fd.entry[i].stream == route->sink_stream) {
>
> No need to check the pad ?
We only have a single sink pad in CSI-2.
>> + source_entry = &source_fd.entry[i];
>> + break;
>> + }
>> + }
>> +
>> + if (!source_entry) {
>> + dev_err(priv->dev,
>> + "Failed to find stream from source frame desc\n");
>> + return -EPIPE;
>> + }
>>
>> - vcdt_part = VCDT_SEL_VC(priv->channel_vc[i]) | VCDT_VCDTN_EN |
>> - VCDT_SEL_DTN_ON | VCDT_SEL_DT(format->datatype);
>> + vc = source_entry->bus.csi2.vc;
>> + dt = source_entry->bus.csi2.dt;
>> +
>> + vcdt_part = VCDT_SEL_VC(vc) | VCDT_VCDTN_EN |
>> + VCDT_SEL_DTN_ON | VCDT_SEL_DT(dt);
>
> I would drop the vc and dt variables and write
>
> vcdt_part = VCDT_SEL_VC(source_entry->bus.csi2.vc)
> | VCDT_VCDTN_EN | VCDT_SEL_DTN_ON
> | VCDT_SEL_DT(source_entry->bus.csi2.dt);
Yes, I think this is fine too.
>>
>> /* Store in correct reg and offset. */
>> - if (i < 2)
>> - vcdt |= vcdt_part << ((i % 2) * 16);
>> + if (ch < 2)
>> + vcdt |= vcdt_part << ((ch % 2) * 16);
>> else
>> - vcdt2 |= vcdt_part << ((i % 2) * 16);
>> - }
>> + vcdt2 |= vcdt_part << ((ch % 2) * 16);
>> +
>> + fmt = v4l2_subdev_state_get_format(state, RCAR_CSI2_SINK,
>> + route->sink_stream);
>> + if (!fmt)
>> + return -EINVAL;
>> +
>> + dev_dbg(priv->dev, "Input size (%ux%u%c)\n",
>> + fmt->width, fmt->height,
>> + fmt->field == V4L2_FIELD_NONE ? 'p' : 'i');
>>
>> - if (fmt->field == V4L2_FIELD_ALTERNATE)
>> - fld = FLD_DET_SEL(1) | FLD_FLD_EN4 | FLD_FLD_EN3 | FLD_FLD_EN2
>> - | FLD_FLD_EN;
>> + /* Code is validated in set_fmt. */
>
> Then why don't you drop the !format check ?
It's a copy of the existing code. I've been reluctant to change gen3
code, as I can't test. But I can't really see how this could be wrong,
so I'll just drop the check here.
Tomi
>> + format = rcsi2_code_to_fmt(fmt->code);
>> + if (!format)
>> + return -EINVAL;
>> +
>> + if (fmt->field == V4L2_FIELD_ALTERNATE)
>> + fld |= FLD_FLD_EN(ch);
>> +
>> + ch++;
>> + }
>>
>> /*
>> * Get the number of active data lanes inspecting the remote mbus
>
Powered by blists - more mailing lists