| 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: <0e0a99e1-0ea2-4683-8e0d-b5511b7a9d5e@ideasonboard.com>
Date: Tue, 16 Dec 2025 15:35:15 +0200
From: Tomi Valkeinen <tomi.valkeinen+renesas@...asonboard.com>
To: Niklas Söderlund <niklas.soderlund@...natech.se>
Cc: 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,
Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Jacopo Mondi <jacopo.mondi@...asonboard.com>
Subject: Re: [PATCH v3 11/15] media: rcar-isp: Call get_frame_desc to find out
VC & DT
Hi,
On 06/06/2025 15:20, Niklas Söderlund wrote:
> Hi Tomi,
>
> Thanks for your work.
>
> On 2025-05-30 16:50:40 +0300, Tomi Valkeinen wrote:
>> Call get_frame_desc to find out VC & DT, instead of hardcoding the VC
>> routing and deducing the DT based on the mbus format.
>>
>> Signed-off-by: Tomi Valkeinen <tomi.valkeinen+renesas@...asonboard.com>
>> ---
>> drivers/media/platform/renesas/rcar-isp/csisp.c | 108 +++++++++++++++++-------
>> 1 file changed, 77 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/media/platform/renesas/rcar-isp/csisp.c b/drivers/media/platform/renesas/rcar-isp/csisp.c
>> index a04cbf96b809..887d8eb21a3a 100644
>> --- a/drivers/media/platform/renesas/rcar-isp/csisp.c
>> +++ b/drivers/media/platform/renesas/rcar-isp/csisp.c
>> @@ -225,24 +225,86 @@ static void risp_power_off(struct rcar_isp *isp)
>> pm_runtime_put(isp->dev);
>> }
>>
>> -static int risp_start(struct rcar_isp *isp, struct v4l2_subdev_state *state)
>> +static int risp_configure_routing(struct rcar_isp *isp,
>> + struct v4l2_subdev_state *state)
>> {
>> - const struct v4l2_mbus_framefmt *fmt;
>> - const struct rcar_isp_format *format;
>> - unsigned int vc;
>> - u32 sel_csi = 0;
>> + struct v4l2_mbus_frame_desc source_fd;
>> + struct v4l2_subdev_route *route;
>> int ret;
>>
>> - fmt = v4l2_subdev_state_get_format(state, RCAR_ISP_SINK, 0);
>> - if (!fmt)
>> - return -EINVAL;
>> + ret = v4l2_subdev_call(isp->remote, pad, get_frame_desc,
>> + isp->remote_pad, &source_fd);
>> + if (ret)
>> + return ret;
>>
>> - format = risp_code_to_fmt(fmt->code);
>> - if (!format) {
>> - dev_err(isp->dev, "Unsupported bus format\n");
>> - return -EINVAL;
>> + /* Clear the channel registers */
>> + for (unsigned int ch = 0; ch < 12; ++ch) {
>> + risp_write_cs(isp, ISPCS_FILTER_ID_CH_REG(ch), 0);
>> + risp_write_cs(isp, ISPCS_DT_CODE03_CH_REG(ch), 0);
>> }
>>
>> + /* Clear the proc mode registers */
>> + for (unsigned int dt = 0; dt < 64; ++dt)
>> + risp_write_cs(isp, ISPPROCMODE_DT_REG(dt), 0);
>
> I agree with Laurent's comments, do we really need to clear these
> registers?
>
>> +
>> + for_each_active_route(&state->routing, route) {
>> + struct v4l2_mbus_frame_desc_entry *source_entry = NULL;
>> + const struct rcar_isp_format *format;
>> + const struct v4l2_mbus_framefmt *fmt;
>> + unsigned int i;
>> + u8 vc, dt, ch;
>> + u32 v;
>> +
>> + 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(isp->dev,
>> + "Failed to find stream from source frame desc\n");
>> + return -EPIPE;
>> + }
>> +
>> + vc = source_entry->bus.csi2.vc;
>> + dt = source_entry->bus.csi2.dt;
>> + /* Channels 4 - 11 go to VIN */
>> + ch = route->source_pad - 1 + 4;
>> +
>> + fmt = v4l2_subdev_state_get_format(state, route->sink_pad,
>> + route->sink_stream);
>> + if (!fmt)
>> + return -EINVAL;
>> +
>> + format = risp_code_to_fmt(fmt->code);
>> + if (!format) {
>> + dev_err(isp->dev, "Unsupported bus format\n");
>> + return -EINVAL;
>> + }
>> +
>> + /* VC Filtering */
>> + risp_write_cs(isp, ISPCS_FILTER_ID_CH_REG(ch), BIT(vc));
>> +
>> + /* DT Filtering */
>> + risp_write_cs(isp, ISPCS_DT_CODE03_CH_REG(ch),
>> + ISPCS_DT_CODE03_EN0 | ISPCS_DT_CODE03_DT0(dt));
>> +
>> + /* Proc mode */
>> + v = risp_read_cs(isp, ISPPROCMODE_DT_REG(dt));
>> + v |= ISPPROCMODE_DT_PROC_MODE_VCn(vc, format->procmode);
>> + risp_write_cs(isp, ISPPROCMODE_DT_REG(dt), v);
>
> Also as Laurent suggested I thin it would be nicer to build these
> registers up in a local variable and do the writes once outside the
> loop. That way the clearing of the register will take care of itself ;-)
I can do that, but I'm not sure I see the need from performance
perspective. We're talking about a few register reads & writes, once,
when enabling the first stream.
Would the code be more readable? I'm not sure... Here we do all the
writes related to a single stream in one iteration, which is nice.
But I don't have strong opinions either way.
Tomi
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int risp_start(struct rcar_isp *isp, struct v4l2_subdev_state *state)
>> +{
>> + u32 sel_csi = 0;
>> + int ret;
>> +
>> ret = risp_power_on(isp);
>> if (ret) {
>> dev_err(isp->dev, "Failed to power on ISP\n");
>> @@ -256,25 +318,9 @@ static int risp_start(struct rcar_isp *isp, struct v4l2_subdev_state *state)
>> risp_write_cs(isp, ISPINPUTSEL0_REG,
>> risp_read_cs(isp, ISPINPUTSEL0_REG) | sel_csi);
>>
>> - /* Configure Channel Selector. */
>> - for (vc = 0; vc < 4; vc++) {
>> - u8 ch = vc + 4;
>> - u8 dt = format->datatype;
>> -
>> - risp_write_cs(isp, ISPCS_FILTER_ID_CH_REG(ch), BIT(vc));
>> - risp_write_cs(isp, ISPCS_DT_CODE03_CH_REG(ch),
>> - ISPCS_DT_CODE03_EN3 | ISPCS_DT_CODE03_DT3(dt) |
>> - ISPCS_DT_CODE03_EN2 | ISPCS_DT_CODE03_DT2(dt) |
>> - ISPCS_DT_CODE03_EN1 | ISPCS_DT_CODE03_DT1(dt) |
>> - ISPCS_DT_CODE03_EN0 | ISPCS_DT_CODE03_DT0(dt));
>> - }
>> -
>> - /* Setup processing method. */
>> - risp_write_cs(isp, ISPPROCMODE_DT_REG(format->datatype),
>> - ISPPROCMODE_DT_PROC_MODE_VCn(3, format->procmode) |
>> - ISPPROCMODE_DT_PROC_MODE_VCn(2, format->procmode) |
>> - ISPPROCMODE_DT_PROC_MODE_VCn(1, format->procmode) |
>> - ISPPROCMODE_DT_PROC_MODE_VCn(0, format->procmode));
>> + ret = risp_configure_routing(isp, state);
>> + if (ret)
>> + return ret;
>>
>> /* Start ISP. */
>> risp_write_cs(isp, ISPSTART_REG, ISPSTART_START);
>>
>> --
>> 2.43.0
>>
>
Powered by blists - more mailing lists