| 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: <Z7xCY2c8AvdgFNfm@kekkonen.localdomain>
Date: Mon, 24 Feb 2025 09:56:51 +0000
From: Sakari Ailus <sakari.ailus@...ux.intel.com>
To: Shravan.Chippa@...rochip.com
Cc: mchehab@...nel.org, kieran.bingham@...asonboard.com,
linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
Conor.Dooley@...rochip.com, Valentina.FernandezAlanis@...rochip.com,
Praveen.Kumar@...rochip.com
Subject: Re: [PATCH V4] media: i2c: imx334: Add support for 1280x720 &
640x480 resolutions
Hi Shravan,
On Mon, Feb 24, 2025 at 07:37:23AM +0000, Shravan.Chippa@...rochip.com wrote:
>
>
> > -----Original Message-----
> > From: Sakari Ailus <sakari.ailus@...ux.intel.com>
> > Sent: Thursday, February 20, 2025 6:50 PM
> > To: shravan Chippa - I35088 <Shravan.Chippa@...rochip.com>
> > Cc: mchehab@...nel.org; kieran.bingham@...asonboard.com; linux-
> > media@...r.kernel.org; linux-kernel@...r.kernel.org; Conor Dooley -
> > M52691 <Conor.Dooley@...rochip.com>; Valentina Fernandez Alanis -
> > M63239 <Valentina.FernandezAlanis@...rochip.com>; Praveen Kumar -
> > I30718 <Praveen.Kumar@...rochip.com>
> > Subject: Re: [PATCH V4] media: i2c: imx334: Add support for 1280x720 &
> > 640x480 resolutions
> >
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > content is safe
> >
> > Hi Shravan,
> >
> > On Tue, Feb 18, 2025 at 03:03:56PM +0530, shravan kumar wrote:
> > > From: Shravan Chippa <shravan.chippa@...rochip.com>
> > >
> > > Added support for 1280x720@30 and 640x480@30 resolutions and
> > optimized
> > > the resolution arrays by incorporating a common register array.
> > >
> > > Updated the register array values for 1920x1080@30 and 3840x2160@30
> > > resolutions in accordance with the common register array values.
> > >
> > > Signed-off-by: Shravan Chippa <shravan.chippa@...rochip.com>
> > >
> > > ---
> > > changelog:
> > > v3 -> v4
> > > In response to the review request, the dependency of the common
> > > register value array on the 445M link frequency has been eliminated.
> > >
> > > Although I do not have a test setup for 4k resolution at an 819M link
> > > frequency, I have made adjustments based on the IMX334 data sheet, the
> > > latest 4k resolution, and the common register value array.
> > >
> > > Additionally, the 4k resolution has been switched to master mode to
> > > ensure consistency with other resolutions that also use master mode.
> > >
> > > changelog:
> > > v2 -> v3
> > > removied blank lines reported by media CI robot
> > >
> > > v1 -> v2
> > > Optimized the resolution arrays by added common register array
> > > ---
> > >
> > > drivers/media/i2c/imx334.c | 214
> > > +++++++++++++++++++------------------
> > > 1 file changed, 109 insertions(+), 105 deletions(-)
> > >
> > > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > > index a544fc3df39c..0172406780df 100644
> > > --- a/drivers/media/i2c/imx334.c
> > > +++ b/drivers/media/i2c/imx334.c
> > > @@ -167,8 +167,8 @@ static const s64 link_freq[] = {
> > > IMX334_LINK_FREQ_445M,
> > > };
> > >
> > > -/* Sensor mode registers for 1920x1080@...ps */ -static const struct
> > > imx334_reg mode_1920x1080_regs[] = {
> > > +/* Sensor common mode registers values */ static const struct
> > > +imx334_reg common_mode_regs[] = {
> > > {0x3000, 0x01},
> > > {0x3018, 0x04},
> > > {0x3030, 0xca},
> > > @@ -176,26 +176,10 @@ static const struct imx334_reg
> > mode_1920x1080_regs[] = {
> > > {0x3032, 0x00},
> > > {0x3034, 0x4c},
> > > {0x3035, 0x04},
> > > - {0x302c, 0xf0},
> > > - {0x302d, 0x03},
> > > - {0x302e, 0x80},
> > > - {0x302f, 0x07},
> > > - {0x3074, 0xcc},
> > > - {0x3075, 0x02},
> > > - {0x308e, 0xcd},
> > > - {0x308f, 0x02},
> > > - {0x3076, 0x38},
> > > - {0x3077, 0x04},
> > > - {0x3090, 0x38},
> > > - {0x3091, 0x04},
> > > - {0x3308, 0x38},
> > > - {0x3309, 0x04},
> > > - {0x30C6, 0x00},
> > > + {0x30c6, 0x00},
> > > {0x30c7, 0x00},
> > > {0x30ce, 0x00},
> > > {0x30cf, 0x00},
> > > - {0x30d8, 0x18},
> > > - {0x30d9, 0x0a},
> > > {0x304c, 0x00},
> > > {0x304e, 0x00},
> > > {0x304f, 0x00},
> > > @@ -210,7 +194,7 @@ static const struct imx334_reg
> > mode_1920x1080_regs[] = {
> > > {0x300d, 0x29},
> > > {0x314c, 0x29},
> > > {0x314d, 0x01},
> > > - {0x315a, 0x06},
> > > + {0x315a, 0x0a},
> >
> > What's the effect of this change and why is it done?
>
> This change is to increase the horizontal blanking to get more time to process the image line by line.
That needs to be accompanied by similar changes to the horizontal blanking
control. What actually needs this?
Then the question is whether this should be user configurable. The current
users may well prefer keeping this as-is.
>
>
> >
> > > {0x3168, 0xa0},
> > > {0x316a, 0x7e},
> > > {0x319e, 0x02},
> > > @@ -330,116 +314,103 @@ static const struct imx334_reg
> > mode_1920x1080_regs[] = {
> > > {0x3002, 0x00},
> > > };
> > >
> > > +/* Sensor mode registers for 640x480@...ps */ static const struct
> > > +imx334_reg mode_640x480_regs[] = {
> > > + {0x302c, 0x70},
> > > + {0x302d, 0x06},
> > > + {0x302e, 0x80},
> > > + {0x302f, 0x02},
> > > + {0x3074, 0x48},
> > > + {0x3075, 0x07},
> > > + {0x308e, 0x49},
> > > + {0x308f, 0x07},
> > > + {0x3076, 0xe0},
> > > + {0x3077, 0x01},
> > > + {0x3090, 0xe0},
> > > + {0x3091, 0x01},
> > > + {0x3308, 0xe0},
> > > + {0x3309, 0x01},
> > > + {0x30d8, 0x30},
> > > + {0x30d9, 0x0b},
> > > +};
> > > +
> > > +/* Sensor mode registers for 1280x720@...ps */ static const struct
> > > +imx334_reg mode_1280x720_regs[] = {
> > > + {0x302c, 0x30},
> > > + {0x302d, 0x05},
> > > + {0x302e, 0x00},
> > > + {0x302f, 0x05},
> > > + {0x3074, 0x84},
> > > + {0x3075, 0x03},
> > > + {0x308e, 0x85},
> > > + {0x308f, 0x03},
> > > + {0x3076, 0xd0},
> > > + {0x3077, 0x02},
> > > + {0x3090, 0xd0},
> > > + {0x3091, 0x02},
> > > + {0x3308, 0xd0},
> > > + {0x3309, 0x02},
> > > + {0x30d8, 0x30},
> > > + {0x30d9, 0x0b},
> > > +};
> > > +
> > > +/* Sensor mode registers for 1920x1080@...ps */ static const struct
> > > +imx334_reg mode_1920x1080_regs[] = {
> > > + {0x302c, 0xf0},
> > > + {0x302d, 0x03},
> > > + {0x302e, 0x80},
> > > + {0x302f, 0x07},
> > > + {0x3074, 0xcc},
> > > + {0x3075, 0x02},
> > > + {0x308e, 0xcd},
> > > + {0x308f, 0x02},
> > > + {0x3076, 0x38},
> > > + {0x3077, 0x04},
> > > + {0x3090, 0x38},
> > > + {0x3091, 0x04},
> > > + {0x3308, 0x38},
> > > + {0x3309, 0x04},
> > > + {0x30d8, 0x18},
> > > + {0x30d9, 0x0a},
> > > +};
> > > +
> > > /* Sensor mode registers for 3840x2160@...ps */ static const struct
> > > imx334_reg mode_3840x2160_regs[] = {
> > > - {0x3000, 0x01},
> > > - {0x3002, 0x00},
> > > - {0x3018, 0x04},
> > > - {0x37b0, 0x36},
> > > - {0x304c, 0x00},
> > > - {0x300c, 0x3b},
> >
> > This was the only location this register was written to.
>
> This register value is same for all link frequencies (default value 0x3b for 24Mhz crystal clock),
Please do this in a separate patch.
>
> >
> > Please split this into two to make it more reviewable: splitting register lists
> > into two and then to adding new modes.
>
> I will try to make one patch for common register values
> One more patch for new modes of 640x480 and 1280x720
Please add one for the hblank changes and more if there are similar cases.
>
> Thanks,
> Shravan
>
> >
> > > - {0x300d, 0x2a},
> > > - {0x3034, 0x26},
> > > - {0x3035, 0x02},
> > > - {0x314c, 0x29},
> > > - {0x314d, 0x01},
> > > - {0x315a, 0x02},
> > > - {0x3168, 0xa0},
> > > - {0x316a, 0x7e},
> > > - {0x3288, 0x21},
> > > - {0x328a, 0x02},
> > > {0x302c, 0x3c},
> > > {0x302d, 0x00},
> > > {0x302e, 0x00},
> > > {0x302f, 0x0f},
> > > + {0x3074, 0xb0},
> > > + {0x3075, 0x00},
> > > + {0x308e, 0xb1},
> > > + {0x308f, 0x00},
> > > {0x3076, 0x70},
> > > {0x3077, 0x08},
> > > {0x3090, 0x70},
> > > {0x3091, 0x08},
> > > - {0x30d8, 0x20},
> > > - {0x30d9, 0x12},
> > > {0x3308, 0x70},
> > > {0x3309, 0x08},
> > > - {0x3414, 0x05},
> > > - {0x3416, 0x18},
> > > - {0x35ac, 0x0e},
> > > - {0x3648, 0x01},
> > > - {0x364a, 0x04},
> > > - {0x364c, 0x04},
> > > - {0x3678, 0x01},
> > > - {0x367c, 0x31},
> > > - {0x367e, 0x31},
> > > - {0x3708, 0x02},
> > > - {0x3714, 0x01},
> > > - {0x3715, 0x02},
> > > - {0x3716, 0x02},
> > > - {0x3717, 0x02},
> > > - {0x371c, 0x3d},
> > > - {0x371d, 0x3f},
> > > - {0x372c, 0x00},
> > > - {0x372d, 0x00},
> > > - {0x372e, 0x46},
> > > - {0x372f, 0x00},
> > > - {0x3730, 0x89},
> > > - {0x3731, 0x00},
> > > - {0x3732, 0x08},
> > > - {0x3733, 0x01},
> > > - {0x3734, 0xfe},
> > > - {0x3735, 0x05},
> > > - {0x375d, 0x00},
> > > - {0x375e, 0x00},
> > > - {0x375f, 0x61},
> > > - {0x3760, 0x06},
> > > - {0x3768, 0x1b},
> > > - {0x3769, 0x1b},
> > > - {0x376a, 0x1a},
> > > - {0x376b, 0x19},
> > > - {0x376c, 0x18},
> > > - {0x376d, 0x14},
> > > - {0x376e, 0x0f},
> > > - {0x3776, 0x00},
> > > - {0x3777, 0x00},
> > > - {0x3778, 0x46},
> > > - {0x3779, 0x00},
> > > - {0x377a, 0x08},
> > > - {0x377b, 0x01},
> > > - {0x377c, 0x45},
> > > - {0x377d, 0x01},
> > > - {0x377e, 0x23},
> > > - {0x377f, 0x02},
> > > - {0x3780, 0xd9},
> > > - {0x3781, 0x03},
> > > - {0x3782, 0xf5},
> > > - {0x3783, 0x06},
> > > - {0x3784, 0xa5},
> > > - {0x3788, 0x0f},
> > > - {0x378a, 0xd9},
> > > - {0x378b, 0x03},
> > > - {0x378c, 0xeb},
> > > - {0x378d, 0x05},
> > > - {0x378e, 0x87},
> > > - {0x378f, 0x06},
> > > - {0x3790, 0xf5},
> > > - {0x3792, 0x43},
> > > - {0x3794, 0x7a},
> > > - {0x3796, 0xa1},
> > > - {0x3e04, 0x0e},
> > > + {0x30d8, 0x20},
> > > + {0x30d9, 0x12},
> > > {0x319e, 0x00},
> > > {0x3a00, 0x01},
> > > {0x3a18, 0xbf},
> > > - {0x3a19, 0x00},
> > > {0x3a1a, 0x67},
> > > - {0x3a1b, 0x00},
> > > {0x3a1c, 0x6f},
> > > - {0x3a1d, 0x00},
> > > {0x3a1e, 0xd7},
> > > {0x3a1f, 0x01},
> > > + {0x300d, 0x2a},
> > > + {0x3034, 0x26},
> > > + {0x3035, 0x02},
> > > + {0x315a, 0x02},
> > > {0x3a20, 0x6f},
> > > {0x3a21, 0x00},
> > > {0x3a22, 0xcf},
> > > {0x3a23, 0x00},
> > > {0x3a24, 0x6f},
> > > {0x3a25, 0x00},
> > > + {0x3a24, 0x6f},
> > > + {0x3a25, 0x00},
> > > {0x3a26, 0xb7},
> > > {0x3a27, 0x00},
> > > {0x3a28, 0x5f},
> > > @@ -505,6 +476,32 @@ static const struct imx334_mode
> > supported_modes[] = {
> > > .num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
> > > .regs = mode_1920x1080_regs,
> > > },
> > > + }, {
> > > + .width = 1280,
> > > + .height = 720,
> > > + .hblank = 2480,
> > > + .vblank = 1170,
> > > + .vblank_min = 45,
> > > + .vblank_max = 132840,
> > > + .pclk = 297000000,
> > > + .link_freq_idx = 1,
> > > + .reg_list = {
> > > + .num_of_regs = ARRAY_SIZE(mode_1280x720_regs),
> > > + .regs = mode_1280x720_regs,
> > > + },
> > > + }, {
> > > + .width = 640,
> > > + .height = 480,
> > > + .hblank = 2480,
> > > + .vblank = 1170,
> > > + .vblank_min = 45,
> > > + .vblank_max = 132840,
> > > + .pclk = 297000000,
> > > + .link_freq_idx = 1,
> > > + .reg_list = {
> > > + .num_of_regs = ARRAY_SIZE(mode_640x480_regs),
> > > + .regs = mode_640x480_regs,
> > > + },
> > > },
> > > };
> > >
> > > @@ -989,6 +986,13 @@ static int imx334_start_streaming(struct imx334
> > *imx334)
> > > const struct imx334_reg_list *reg_list;
> > > int ret;
> > >
> > > + ret = imx334_write_regs(imx334, common_mode_regs,
> > > + ARRAY_SIZE(common_mode_regs));
> > > + if (ret) {
> > > + dev_err(imx334->dev, "fail to write common registers");
> > > + return ret;
> > > + }
> > > +
> > > /* Write sensor mode registers */
> > > reg_list = &imx334->cur_mode->reg_list;
> > > ret = imx334_write_regs(imx334, reg_list->regs,
> >
--
Kind regards,
Sakari Ailus
Powered by blists - more mailing lists