| 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: <175096968502.8144.14459853899575450802@freya>
Date: Thu, 26 Jun 2025 13:28:05 -0700
From: Jai Luthra <jai.luthra@...asonboard.com>
To: Sakari Ailus <sakari.ailus@...ux.intel.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 Sakari,
Thanks for the review.
Quoting Sakari Ailus (2025-06-25 23:00:22)
> 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.
>
Ah, that's my bad, rest of the driver does already use BIT().. will fix in
v3.
> > +#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.
>
Will do in v3.
> >
> > /*
> > * 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.
>
Good catch, will fix.
> > +
> > + 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?
>
Ah I wasn't aware of different namespaces. I think a module-specific one as
documented here [1] might make the most sense in this case. Will try that,
else will use _GPL.
[1] https://docs.kernel.org/core-api/symbol-namespaces.html#using-the-export-symbol-gpl-for-modules-macro
> > +
> > 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.
>
I was unsure about it, as while these are two separate drivers it's
essentially the same "device".. but I don't see a harm in keeping this
under include/media. Will do in v3.
> > 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
Thanks,
Jai
Powered by blists - more mailing lists