diff mbox series

[v8,4/4] media: i2c: imx334: update pixel and link frequency

Message ID 20230106072931.2317597-5-shravan.chippa@microchip.com (mailing list archive)
State New, archived
Headers show
Series media: i2c: imx334: support lower bandwidth mode | expand

Commit Message

shravan chippa Jan. 6, 2023, 7:29 a.m. UTC 
From: Shravan Chippa <shravan.chippa@microchip.com>

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 <sakari.ailus@iki.fi>
Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
---
 drivers/media/i2c/imx334.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

Comments

Jacopo Mondi Jan. 6, 2023, 11:04 a.m. UTC | #1 
Hi Shravan

On Fri, Jan 06, 2023 at 12:59:31PM +0530, shravan kumar wrote:
> From: Shravan Chippa <shravan.chippa@microchip.com>
>
> 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 <sakari.ailus@iki.fi>
> Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> ---
>  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
>
shravan chippa Jan. 9, 2023, 3:39 p.m. UTC | #2 
> -----Original Message-----
> From: Jacopo Mondi <jacopo.mondi@ideasonboard.com>
> Sent: 06 January 2023 04:35 PM
> To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>
> Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com;
> mchehab@kernel.org; linux-media@vger.kernel.org; linux-
> kernel@vger.kernel.org; Sakari Ailus <sakari.ailus@iki.fi>
> Subject: Re: [PATCH v8 4/4] media: i2c: imx334: update pixel and link frequency
> 
> [You don't often get email from jacopo.mondi@ideasonboard.com. 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 <shravan.chippa@microchip.com>
> >
> > 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 <sakari.ailus@iki.fi>
> > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com>
> > ---
> >  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
> >
diff mbox series

Patch

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;