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
 
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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

Powered by Openwall GNU/*/Linux Powered by OpenVZ