| 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:
<PN3P287MB18294408F7BFA1B93162FF9E8BA12@PN3P287MB1829.INDP287.PROD.OUTLOOK.COM>
Date: Thu, 27 Mar 2025 15:38:24 +0000
From: Tarang Raval <tarang.raval@...iconsignals.io>
To: Dave Stevenson <dave.stevenson@...pberrypi.com>
CC: Sakari Ailus <sakari.ailus@...ux.intel.com>,
"kieran.bingham@...asonboard.com" <kieran.bingham@...asonboard.com>,
"Shravan.Chippa@...rochip.com" <Shravan.Chippa@...rochip.com>, Mauro Carvalho
Chehab <mchehab@...nel.org>, Hans Verkuil <hverkuil@...all.nl>, Laurent
Pinchart <laurent.pinchart@...asonboard.com>, Umang Jain
<umang.jain@...asonboard.com>, Zhi Mao <zhi.mao@...iatek.com>, Julien Massot
<julien.massot@...labora.com>, Luis Garcia <git@...gi311.com>, Benjamin
Mugnier <benjamin.mugnier@...s.st.com>, "linux-media@...r.kernel.org"
<linux-media@...r.kernel.org>, "linux-kernel@...r.kernel.org"
<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 4/6] media: i2c: imx334: Support 4 or 8 lane operation
modes
Hi Dave,
Thank you for your detailed feedback.
> On Thu, 27 Mar 2025 at 10:55, Tarang Raval
> <tarang.raval@...iconsignals.io> wrote:
> >
> > Hi Sakari,
> >
> > Thanks for the review.
> >
> > > On Mon, Mar 10, 2025 at 12:47:46PM +0530, Tarang Raval wrote:
> > > > imx334 can support both 4 and 8 lane configurations.
> > > > Extend the driver to configure the lane mode accordingly.
> > > >
> > > > Signed-off-by: Tarang Raval <tarang.raval@...iconsignals.io>
> > > > ---
> > > > drivers/media/i2c/imx334.c | 22 +++++++++++++++++++---
> > > > 1 file changed, 19 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > > > index 24ccfd1d0986..23bfc64969cc 100644
> > > > --- a/drivers/media/i2c/imx334.c
> > > > +++ b/drivers/media/i2c/imx334.c
> > > > @@ -47,6 +47,8 @@
> > > > #define IMX334_EXPOSURE_DEFAULT 0x0648
> > > >
> > > > #define IMX334_REG_LANEMODE CCI_REG8(0x3a01)
> > > > +#define IMX334_CSI_4_LANE_MODE 3
> > > > +#define IMX334_CSI_8_LANE_MODE 7
> > > >
> > > > /* Window cropping Settings */
> > > > #define IMX334_REG_AREA3_ST_ADR_1 CCI_REG16_LE(0x3074)
> > > > @@ -107,7 +109,6 @@
> > > > /* CSI2 HW configuration */
> > > > #define IMX334_LINK_FREQ_891M 891000000
> > > > #define IMX334_LINK_FREQ_445M 445500000
> > > > -#define IMX334_NUM_DATA_LANES 4
> > > >
> > > > #define IMX334_REG_MIN 0x00
> > > > #define IMX334_REG_MAX 0xfffff
> > > > @@ -181,6 +182,7 @@ struct imx334_mode {
> > > > * @exp_ctrl: Pointer to exposure control
> > > > * @again_ctrl: Pointer to analog gain control
> > > > * @vblank: Vertical blanking in lines
> > > > + * @lane_mode: Mode for number of connected data lanes
> > > > * @cur_mode: Pointer to current selected sensor mode
> > > > * @mutex: Mutex for serializing sensor controls
> > > > * @link_freq_bitmap: Menu bitmap for link_freq_ctrl
> > > > @@ -204,6 +206,7 @@ struct imx334 {
> > > > struct v4l2_ctrl *again_ctrl;
> > > > };
> > > > u32 vblank;
> > > > + u32 lane_mode;
> > > > const struct imx334_mode *cur_mode;
> > > > struct mutex mutex;
> > > > unsigned long link_freq_bitmap;
> > > > @@ -240,7 +243,6 @@ static const struct cci_reg_sequence common_mode_regs[] = {
> > > > { IMX334_REG_HADD_VADD, 0x00},
> > > > { IMX334_REG_VALID_EXPAND, 0x03},
> > > > { IMX334_REG_TCYCLE, 0x00},
> > > > - { IMX334_REG_LANEMODE, 0x03},
> > >
> > > Not a fault of this patch but also the closing brace should have a space
> > > before it. Could you address it in the earlier patches?
> >
> > Okay, I will correct it.
> >
> > > > { IMX334_REG_TCLKPOST, 0x007f},
> > > > { IMX334_REG_TCLKPREPARE, 0x0037},
> > > > { IMX334_REG_TCLKTRAIL, 0x0037},
> > > > @@ -876,6 +878,13 @@ static int imx334_start_streaming(struct imx334 *imx334)
> > > > return ret;
> > > > }
> > > >
> > > > + ret = cci_write(imx334->cci, IMX334_REG_LANEMODE,
> > > > + imx334->lane_mode, NULL);
> > > > + if (ret) {
> > > > + dev_err(imx334->dev, "failed to configure lanes\n");
> > > > + return ret;
> > > > + }
> > > > +
> > > > ret = imx334_set_framefmt(imx334);
> > > > if (ret) {
> > > > dev_err(imx334->dev, "%s failed to set frame format: %d\n",
> > > > @@ -1022,7 +1031,14 @@ static int imx334_parse_hw_config(struct imx334 *imx334)
> > > > if (ret)
> > > > return ret;
> > > >
> > > > - if (bus_cfg.bus.mipi_csi2.num_data_lanes != IMX334_NUM_DATA_LANES) {
> > > > + switch (bus_cfg.bus.mipi_csi2.num_data_lanes) {
> > > > + case 4:
> > > > + imx334->lane_mode = IMX334_CSI_4_LANE_MODE;
> > > > + break;
> > > > + case 8:
> > > > + imx334->lane_mode = IMX334_CSI_8_LANE_MODE;
> > >
> > > Doesn't this affect the PLL configuration? Presumably higher frame rates
> > > could be achieved at least.
> >
> > Sorry, my commit message is misleading. The intention of this patch is to
> > configure the lane mode dynamically from the streaming function instead
> > of using a hardcoded value.
> >
> > You are correct that supporting an 8-lane mode requires changes to the PLL
> > configuration. This patch does not address that aspect yet.
>
> Is it actually required, or just a nicety?
> The datasheet [1] says:
> "Maximum frame rate in All-pixel scan mode 3840(H)×2160(V) AD12bit: 60
> frame / s"
> The current driver configuration for the 3840x2160 mode is a pixel
> clock 594MHz with total timings of (3840+560) x (2160+90), which gives
> a framerate of 60fps. So you already have the maximum capabilities of
> the sensor exposed.
>
> Adding the 8 lane mode gives you the option to run at half the link
> frequency of the 4 lane, but Sony Starvis sensors have a FIFO between
> pixel array and MIPI block. All the other Starvis sensors I've
> encountered are quite happy at any of the link frequencies as long as
> the horizontal blanking makes the line period sufficient to send each
> line.
>
> The datasheet does say "The bit rate maximum value are 1782 Mbps /
> Lane in 4 Lane mode and 1188 Mbps / Lane in 8 Lane mode. " (page 78
> "CSI-2 output"), but then also "The maximum bit rate of each Lane are
> 1782 Mbps / Lane." (page 81 "MIPI transmitter"). Surely all lanes can
> either do 1782Mbps, or they can't. They won't have downrated just
> lanes 5-8.
> Presumably it works at 1782Mbps/lane in 8 lane mode or you wouldn't
> have submitted the patch,
My patch aimed to make lane mode dynamic (4 or 8 lanes) based on hardware.
not to add 8-lane support my commit message was off, and I haven’t tested
8 lanes.
Thanks for pointing out the PLL and datasheet inconsistencies. I’ll fix the message
and leave 8-lane support for a well-tested implementation in the future.
Best Regards,
Tarang
> We've been here before with the imx290 and imx415 drivers and what can
> be supported with each combination of lanes and link frequency.
>
> Cheers
> Dave
>
> [1] https://en.sunnywale.com/uploadfile/2022/1205/IMX334LQR-C%20full%20datasheet_Awin.pdf
>
> > Best Regards,
> > Tarang
> > > > + break;
> > > > + default:
> > > > dev_err(imx334->dev,
> > > > "number of CSI2 data lanes %d is not supported\n",
> > > > bus_cfg.bus.mipi_csi2.num_data_lanes);
> > >
> > > --
> > > Regards,
> > >
> > > Sakari Ailus
Powered by blists - more mailing lists