From: Shravan Chippa <[email protected]>
Update pixel_rate and link frequency for 1920x1080@30
while changing mode.
Add dummy ctrl cases for pixel_rate and link frequency
to avoid error while changing the modes dynamically
Suggested-by: Sakari Ailus <[email protected]>
Signed-off-by: Shravan Chippa <[email protected]>
---
drivers/media/i2c/imx334.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index 0315e1c9541d..8c3ba660abae 100644
--- a/drivers/media/i2c/imx334.c
+++ b/drivers/media/i2c/imx334.c
@@ -50,6 +50,7 @@
/* CSI2 HW configuration */
#define IMX334_LINK_FREQ 891000000
+#define IMX334_LINK_FREQ_445M 445500000
#define IMX334_NUM_DATA_LANES 4
#define IMX334_REG_MIN 0x00
@@ -145,6 +146,7 @@ struct imx334 {
static const s64 link_freq[] = {
IMX334_LINK_FREQ,
+ IMX334_LINK_FREQ_445M,
};
/* Sensor mode registers */
@@ -468,7 +470,7 @@ static const struct imx334_mode supported_modes[] = {
.vblank_min = 90,
.vblank_max = 132840,
.pclk = 74250000,
- .link_freq_idx = 0,
+ .link_freq_idx = 1,
.reg_list = {
.num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
.regs = mode_1920x1080_regs,
@@ -598,6 +600,11 @@ static int imx334_update_controls(struct imx334 *imx334,
if (ret)
return ret;
+ ret = __v4l2_ctrl_modify_range(imx334->pclk_ctrl, mode->pclk,
+ mode->pclk, 1, mode->pclk);
+ if (ret)
+ return ret;
+
ret = __v4l2_ctrl_modify_range(imx334->hblank_ctrl, mode->hblank,
mode->hblank, 1, mode->hblank);
if (ret)
@@ -698,6 +705,8 @@ static int imx334_set_ctrl(struct v4l2_ctrl *ctrl)
pm_runtime_put(imx334->dev);
break;
+ case V4L2_CID_PIXEL_RATE:
+ case V4L2_CID_LINK_FREQ:
case V4L2_CID_HBLANK:
ret = 0;
break;
--
2.34.1
Hi Shravan
On Fri, Jan 06, 2023 at 12:59:31PM +0530, shravan kumar wrote:
> From: Shravan Chippa <[email protected]>
>
> Update pixel_rate and link frequency for 1920x1080@30
> while changing mode.
>
> Add dummy ctrl cases for pixel_rate and link frequency
> to avoid error while changing the modes dynamically
>
> Suggested-by: Sakari Ailus <[email protected]>
> Signed-off-by: Shravan Chippa <[email protected]>
> ---
> drivers/media/i2c/imx334.c | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> index 0315e1c9541d..8c3ba660abae 100644
> --- a/drivers/media/i2c/imx334.c
> +++ b/drivers/media/i2c/imx334.c
> @@ -50,6 +50,7 @@
>
> /* CSI2 HW configuration */
> #define IMX334_LINK_FREQ 891000000
I guess you want to rename this one to IMX334_LINK_FREQ_891M
Give our previous discussion this seems correct for the following mode
{
.width = 3840,
.height = 2160,
.hblank = 560,
.vblank = 2340,
.vblank_min = 90,
.vblank_max = 132840,
.pclk = 594000000,
.link_freq_idx = 0,
.reg_list = {
.num_of_regs = ARRAY_SIZE(mode_3840x2160_regs),
.regs = mode_3840x2160_regs,
},
}, {
duration: (3840+560) * (2160+2340) / 594000000 = 33sec = 30FPS
link_freq (3840+560) * (2160+2340) * 30 * 12 / 8 = 891000000
Which works well if we use min_vblank = 90 for 60FPS
duration: (3840+560) * (2160+90) / 594000000 = 0.16 = 60 FPS
link_freq (3840+560) * (2160+90) * 60 * 12 / 8 = 891000000
> +#define IMX334_LINK_FREQ_445M 445500000
But this doesn't work well for me
{
.width = 1920,
.height = 1080,
.hblank = 280,
.vblank = 1170,
.vblank_min = 90,
.vblank_max = 132840,
.pclk = 74250000,
.link_freq_idx = 1,
.reg_list = {
.num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
.regs = mode_1920x1080_regs,
},
},
duration: (1920+280) * (1080+1170) / 74250000 = 66msec = 16FPS
link_freq = (1920+280) * (1080+1170) * 60 * 10 / 8 = 371250000
Do you agree with the above or have I missed something ?
I understand you get 30 FPS with the 1920*1080 mode so could you
please check in the newly introduce mode register table what are the
actual values for the blankings and compute the pixel_rate and
link_freq accordingly ?
> #define IMX334_NUM_DATA_LANES 4
>
> #define IMX334_REG_MIN 0x00
> @@ -145,6 +146,7 @@ struct imx334 {
>
> static const s64 link_freq[] = {
> IMX334_LINK_FREQ,
> + IMX334_LINK_FREQ_445M,
> };
>
> /* Sensor mode registers */
> @@ -468,7 +470,7 @@ static const struct imx334_mode supported_modes[] = {
> .vblank_min = 90,
> .vblank_max = 132840,
> .pclk = 74250000,
> - .link_freq_idx = 0,
> + .link_freq_idx = 1,
> .reg_list = {
> .num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
> .regs = mode_1920x1080_regs,
> @@ -598,6 +600,11 @@ static int imx334_update_controls(struct imx334 *imx334,
> if (ret)
> return ret;
>
> + ret = __v4l2_ctrl_modify_range(imx334->pclk_ctrl, mode->pclk,
> + mode->pclk, 1, mode->pclk);
> + if (ret)
> + return ret;
> +
> ret = __v4l2_ctrl_modify_range(imx334->hblank_ctrl, mode->hblank,
> mode->hblank, 1, mode->hblank);
> if (ret)
> @@ -698,6 +705,8 @@ static int imx334_set_ctrl(struct v4l2_ctrl *ctrl)
> pm_runtime_put(imx334->dev);
>
> break;
> + case V4L2_CID_PIXEL_RATE:
> + case V4L2_CID_LINK_FREQ:
> case V4L2_CID_HBLANK:
Same question as for patch 1/4: Do we need these safety checks for
read-only controls ?
Thanks
j
> ret = 0;
> break;
> --
> 2.34.1
>
> -----Original Message-----
> From: Jacopo Mondi <[email protected]>
> Sent: 06 January 2023 04:35 PM
> To: shravan Chippa - I35088 <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; Sakari Ailus <[email protected]>
> Subject: Re: [PATCH v8 4/4] media: i2c: imx334: update pixel and link frequency
>
> [You don't often get email from [email protected]. Learn why
> this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> Hi Shravan
>
> On Fri, Jan 06, 2023 at 12:59:31PM +0530, shravan kumar wrote:
> > From: Shravan Chippa <[email protected]>
> >
> > Update pixel_rate and link frequency for 1920x1080@30 while changing
> > mode.
> >
> > Add dummy ctrl cases for pixel_rate and link frequency to avoid error
> > while changing the modes dynamically
> >
> > Suggested-by: Sakari Ailus <[email protected]>
> > Signed-off-by: Shravan Chippa <[email protected]>
> > ---
> > drivers/media/i2c/imx334.c | 11 ++++++++++-
> > 1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > index 0315e1c9541d..8c3ba660abae 100644
> > --- a/drivers/media/i2c/imx334.c
> > +++ b/drivers/media/i2c/imx334.c
> > @@ -50,6 +50,7 @@
> >
> > /* CSI2 HW configuration */
> > #define IMX334_LINK_FREQ 891000000
>
> I guess you want to rename this one to IMX334_LINK_FREQ_891M
>
> Give our previous discussion this seems correct for the following mode
>
> {
> .width = 3840,
> .height = 2160,
> .hblank = 560,
> .vblank = 2340,
> .vblank_min = 90,
> .vblank_max = 132840,
> .pclk = 594000000,
> .link_freq_idx = 0,
> .reg_list = {
> .num_of_regs = ARRAY_SIZE(mode_3840x2160_regs),
> .regs = mode_3840x2160_regs,
> },
> }, {
>
> duration: (3840+560) * (2160+2340) / 594000000 = 33sec = 30FPS
> link_freq (3840+560) * (2160+2340) * 30 * 12 / 8 = 891000000
>
> Which works well if we use min_vblank = 90 for 60FPS
>
> duration: (3840+560) * (2160+90) / 594000000 = 0.16 = 60 FPS
> link_freq (3840+560) * (2160+90) * 60 * 12 / 8 = 891000000
>
>
> > +#define IMX334_LINK_FREQ_445M 445500000
>
> But this doesn't work well for me
>
> {
> .width = 1920,
> .height = 1080,
> .hblank = 280,
> .vblank = 1170,
> .vblank_min = 90,
> .vblank_max = 132840,
> .pclk = 74250000,
> .link_freq_idx = 1,
> .reg_list = {
> .num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
> .regs = mode_1920x1080_regs,
> },
> },
>
> duration: (1920+280) * (1080+1170) / 74250000 = 66msec = 16FPS
> link_freq = (1920+280) * (1080+1170) * 60 * 10 / 8 = 371250000
>
> Do you agree with the above or have I missed something ?
>
> I understand you get 30 FPS with the 1920*1080 mode so could you please
> check in the newly introduce mode register table what are the actual values for
> the blankings and compute the pixel_rate and link_freq accordingly ?
I will try to correct the hblank and vblank_min.
Thanks,
Shravan
>
> > #define IMX334_NUM_DATA_LANES 4
> >
> > #define IMX334_REG_MIN 0x00
> > @@ -145,6 +146,7 @@ struct imx334 {
> >
> > static const s64 link_freq[] = {
> > IMX334_LINK_FREQ,
> > + IMX334_LINK_FREQ_445M,
> > };
> >
> > /* Sensor mode registers */
> > @@ -468,7 +470,7 @@ static const struct imx334_mode supported_modes[]
> = {
> > .vblank_min = 90,
> > .vblank_max = 132840,
> > .pclk = 74250000,
> > - .link_freq_idx = 0,
> > + .link_freq_idx = 1,
> > .reg_list = {
> > .num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
> > .regs = mode_1920x1080_regs, @@ -598,6 +600,11
> > @@ static int imx334_update_controls(struct imx334 *imx334,
> > if (ret)
> > return ret;
> >
> > + ret = __v4l2_ctrl_modify_range(imx334->pclk_ctrl, mode->pclk,
> > + mode->pclk, 1, mode->pclk);
> > + if (ret)
> > + return ret;
> > +
> > ret = __v4l2_ctrl_modify_range(imx334->hblank_ctrl, mode->hblank,
> > mode->hblank, 1, mode->hblank);
> > if (ret)
> > @@ -698,6 +705,8 @@ static int imx334_set_ctrl(struct v4l2_ctrl *ctrl)
> > pm_runtime_put(imx334->dev);
> >
> > break;
> > + case V4L2_CID_PIXEL_RATE:
> > + case V4L2_CID_LINK_FREQ:
> > case V4L2_CID_HBLANK:
>
> Same question as for patch 1/4: Do we need these safety checks for read-only
> controls ?
>
> Thanks
> j
>
> > ret = 0;
> > break;
> > --
> > 2.34.1
> >