| 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: <aFzh9jF0ZgODThJF@kekkonen.localdomain>
Date: Thu, 26 Jun 2025 06:00:22 +0000
From: Sakari Ailus <sakari.ailus@...ux.intel.com>
To: Jai Luthra <jai.luthra@...asonboard.com>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
Hans Verkuil <hverkuil@...all.nl>,
Tomi Valkeinen <tomi.valkeinen@...asonboard.com>,
Maxime Ripard <mripard@...nel.org>,
Devarsh Thakkar <devarsht@...com>,
Rishikesh Donadkar <r-donadkar@...com>,
Vaishnav Achath <vaishnav.a@...com>,
Changhuang Liang <changhuang.liang@...rfivetech.com>,
linux-media@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 5/6] media: cadence: cdns-csi2rx: Support multiple
pixels per clock cycle
Hi Jai,
Thanks for the patchset.
On Thu, Apr 10, 2025 at 12:19:03PM +0530, Jai Luthra wrote:
> The output pixel interface is a parallel bus (32 bits), which
> supports sending multiple pixels (1, 2 or 4) per clock cycle for
> smaller pixel widths like RAW8-RAW16.
>
> Dual-pixel and Quad-pixel modes can be a requirement if the export rate
> of the Cadence IP in Single-pixel mode maxes out before the maximum
> supported DPHY-RX frequency, which is the case with TI's integration of
> this IP [1].
>
> So, we export a function that lets the downstream hardware block request
> a higher pixel-per-clock on a particular output pad.
>
> We check if we can support the requested pixels per clock given the
> known maximum for the currently configured format. If not, we set it
> to the highest feasible value and return this value to the caller.
>
> [1] Section 12.6.1.4.8.14 CSI_RX_IF Programming Restrictions of AM62 TRM
>
> Link: https://www.ti.com/lit/pdf/spruj16
> Signed-off-by: Jai Luthra <jai.luthra@...asonboard.com>
> ---
> drivers/media/platform/cadence/cdns-csi2rx.c | 75 +++++++++++++++++++++-------
> drivers/media/platform/cadence/cdns-csi2rx.h | 19 +++++++
> 2 files changed, 76 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.c b/drivers/media/platform/cadence/cdns-csi2rx.c
> index 608298c72462031515d9ad01c6b267bf7375a5bf..154eaacc39ad294db0524e88be888bd0929af071 100644
> --- a/drivers/media/platform/cadence/cdns-csi2rx.c
> +++ b/drivers/media/platform/cadence/cdns-csi2rx.c
> @@ -5,6 +5,7 @@
> * Copyright (C) 2017 Cadence Design Systems Inc.
> */
>
> +#include <linux/bitfield.h>
> #include <linux/clk.h>
> #include <linux/delay.h>
> #include <linux/io.h>
> @@ -22,6 +23,8 @@
> #include <media/v4l2-fwnode.h>
> #include <media/v4l2-subdev.h>
>
> +#include "cdns-csi2rx.h"
> +
> #define CSI2RX_DEVICE_CFG_REG 0x000
>
> #define CSI2RX_SOFT_RESET_REG 0x004
> @@ -53,6 +56,8 @@
>
> #define CSI2RX_STREAM_CFG_REG(n) (CSI2RX_STREAM_BASE(n) + 0x00c)
> #define CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF (1 << 8)
Not a fault of this patch but this should use BIT(). Or at the very least
(1U << 8). I.e. this isn't a bug but the pattern is bad. It'd be nice to
fix this in a separate patch.
> +#define CSI2RX_STREAM_CFG_NUM_PIXELS_MASK GENMASK(5, 4)
> +#define CSI2RX_STREAM_CFG_NUM_PIXELS(n) ((n) >> 1)
>
> #define CSI2RX_LANES_MAX 4
> #define CSI2RX_STREAMS_MAX 4
> @@ -68,7 +73,10 @@ enum csi2rx_pads {
>
> struct csi2rx_fmt {
> u32 code;
> + /* width of a single pixel on CSI-2 bus */
> u8 bpp;
> + /* max pixels per clock supported on output bus */
> + u8 max_pixels;
> };
>
> struct csi2rx_priv {
> @@ -90,6 +98,7 @@ struct csi2rx_priv {
> struct reset_control *pixel_rst[CSI2RX_STREAMS_MAX];
> struct phy *dphy;
>
> + u8 num_pixels[CSI2RX_STREAMS_MAX];
> u8 lanes[CSI2RX_LANES_MAX];
> u8 num_lanes;
> u8 max_lanes;
> @@ -106,22 +115,22 @@ struct csi2rx_priv {
> };
>
> static const struct csi2rx_fmt formats[] = {
> - { .code = MEDIA_BUS_FMT_YUYV8_1X16, .bpp = 16, },
> - { .code = MEDIA_BUS_FMT_UYVY8_1X16, .bpp = 16, },
> - { .code = MEDIA_BUS_FMT_YVYU8_1X16, .bpp = 16, },
> - { .code = MEDIA_BUS_FMT_VYUY8_1X16, .bpp = 16, },
> - { .code = MEDIA_BUS_FMT_SBGGR8_1X8, .bpp = 8, },
> - { .code = MEDIA_BUS_FMT_SGBRG8_1X8, .bpp = 8, },
> - { .code = MEDIA_BUS_FMT_SGRBG8_1X8, .bpp = 8, },
> - { .code = MEDIA_BUS_FMT_SRGGB8_1X8, .bpp = 8, },
> - { .code = MEDIA_BUS_FMT_Y8_1X8, .bpp = 8, },
> - { .code = MEDIA_BUS_FMT_SBGGR10_1X10, .bpp = 10, },
> - { .code = MEDIA_BUS_FMT_SGBRG10_1X10, .bpp = 10, },
> - { .code = MEDIA_BUS_FMT_SGRBG10_1X10, .bpp = 10, },
> - { .code = MEDIA_BUS_FMT_SRGGB10_1X10, .bpp = 10, },
> - { .code = MEDIA_BUS_FMT_RGB565_1X16, .bpp = 16, },
> - { .code = MEDIA_BUS_FMT_RGB888_1X24, .bpp = 24, },
> - { .code = MEDIA_BUS_FMT_BGR888_1X24, .bpp = 24, },
> + { .code = MEDIA_BUS_FMT_YUYV8_1X16, .bpp = 16, .max_pixels = 2, },
> + { .code = MEDIA_BUS_FMT_UYVY8_1X16, .bpp = 16, .max_pixels = 2, },
> + { .code = MEDIA_BUS_FMT_YVYU8_1X16, .bpp = 16, .max_pixels = 2, },
> + { .code = MEDIA_BUS_FMT_VYUY8_1X16, .bpp = 16, .max_pixels = 2, },
> + { .code = MEDIA_BUS_FMT_SBGGR8_1X8, .bpp = 8, .max_pixels = 4, },
> + { .code = MEDIA_BUS_FMT_SGBRG8_1X8, .bpp = 8, .max_pixels = 4, },
> + { .code = MEDIA_BUS_FMT_SGRBG8_1X8, .bpp = 8, .max_pixels = 4, },
> + { .code = MEDIA_BUS_FMT_SRGGB8_1X8, .bpp = 8, .max_pixels = 4, },
> + { .code = MEDIA_BUS_FMT_Y8_1X8, .bpp = 8, .max_pixels = 4, },
> + { .code = MEDIA_BUS_FMT_SBGGR10_1X10, .bpp = 10, .max_pixels = 2, },
> + { .code = MEDIA_BUS_FMT_SGBRG10_1X10, .bpp = 10, .max_pixels = 2, },
> + { .code = MEDIA_BUS_FMT_SGRBG10_1X10, .bpp = 10, .max_pixels = 2, },
> + { .code = MEDIA_BUS_FMT_SRGGB10_1X10, .bpp = 10, .max_pixels = 2, },
> + { .code = MEDIA_BUS_FMT_RGB565_1X16, .bpp = 16, .max_pixels = 1, },
> + { .code = MEDIA_BUS_FMT_RGB888_1X24, .bpp = 24, .max_pixels = 1, },
> + { .code = MEDIA_BUS_FMT_BGR888_1X24, .bpp = 24, .max_pixels = 1, },
> };
>
> static const struct csi2rx_fmt *csi2rx_get_fmt_by_code(u32 code)
> @@ -276,8 +285,10 @@ static int csi2rx_start(struct csi2rx_priv *csi2rx)
>
> reset_control_deassert(csi2rx->pixel_rst[i]);
>
> - writel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF,
> - csi2rx->base + CSI2RX_STREAM_CFG_REG(i));
> + reg = CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF;
> + reg |= FIELD_PREP(CSI2RX_STREAM_CFG_NUM_PIXELS_MASK,
> + csi2rx->num_pixels[i]);
> + writel(reg, csi2rx->base + CSI2RX_STREAM_CFG_REG(i));
I'd write this as:
writel(CSI2RX_STREAM_CFG_FIFO_MODE_LARGE_BUF |
FIELD_PREP(CSI2RX_STREAM_CFG_NUM_PIXELS_MASK,
csi2rx->num_pixels[i]),
csi2rx->base + CSI2RX_STREAM_CFG_REG(i));
But up to you.
>
> /*
> * Enable one virtual channel. When multiple virtual channels
> @@ -458,6 +469,34 @@ static int csi2rx_init_state(struct v4l2_subdev *subdev,
> return csi2rx_set_fmt(subdev, state, &format);
> }
>
> +int cdns_csi2rx_negotiate_ppc(struct v4l2_subdev *subdev, unsigned int pad,
> + u8 *ppc)
> +{
> + struct csi2rx_priv *csi2rx = v4l2_subdev_to_csi2rx(subdev);
> + const struct csi2rx_fmt *csi_fmt;
> + struct v4l2_subdev_state *state;
> + struct v4l2_mbus_framefmt *fmt;
> + int ret = 0;
ret is redundant.
> +
> + if (!ppc || pad < CSI2RX_PAD_SOURCE_STREAM0 || pad >= CSI2RX_PAD_MAX)
> + return -EINVAL;
> +
> + state = v4l2_subdev_lock_and_get_active_state(subdev);
> + fmt = v4l2_subdev_state_get_format(state, pad);
> + csi_fmt = csi2rx_get_fmt_by_code(fmt->code);
> +
> + /* Reduce requested PPC if it is too high */
> + *ppc = min(*ppc, csi_fmt->max_pixels);
> +
> + v4l2_subdev_unlock_state(state);
> +
> + csi2rx->num_pixels[pad - CSI2RX_PAD_SOURCE_STREAM0] =
> + CSI2RX_STREAM_CFG_NUM_PIXELS(*ppc);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL(cdns_csi2rx_negotiate_ppc);
EXPORT_SYMBOL_GPL(). Or maybe use a namespace?
> +
> static const struct v4l2_subdev_pad_ops csi2rx_pad_ops = {
> .enum_mbus_code = csi2rx_enum_mbus_code,
> .get_fmt = v4l2_subdev_get_fmt,
> diff --git a/drivers/media/platform/cadence/cdns-csi2rx.h b/drivers/media/platform/cadence/cdns-csi2rx.h
I wonder if it'd be better to put this under include/media.
> new file mode 100644
> index 0000000000000000000000000000000000000000..128d47e8513c99c083f49e249e876be6d19389f6
> --- /dev/null
> +++ b/drivers/media/platform/cadence/cdns-csi2rx.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +#ifndef CDNS_CSI2RX_H
> +#define CDNS_CSI2RX_H
> +
> +#include <media/v4l2-subdev.h>
> +
> +/**
> + * cdns_csi2rx_negotiate_ppc - Negotiate pixel-per-clock on output interface
> + *
> + * @subdev: point to &struct v4l2_subdev
> + * @pad: pad number of the source pad
> + * @ppc: pointer to requested pixel-per-clock value
> + *
> + * Returns 0 on success, negative error code otherwise.
> + */
> +int cdns_csi2rx_negotiate_ppc(struct v4l2_subdev *subdev, unsigned int pad,
> + u8 *ppc);
> +
> +#endif
>
--
Regards,
Sakari Ailus
Powered by blists - more mailing lists