| Message ID | 20230106072931.2317597-1-shravan.chippa@microchip.com |
|---|---|
| Headers | show |
| Series | media: i2c: imx334: support lower bandwidth mode | expand |
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 >
> -----Original Message----- > From: Jacopo Mondi <jacopo.mondi@ideasonboard.com> > Sent: 06 January 2023 03:15 PM > To: shravan Chippa - I35088 <Shravan.Chippa@microchip.com>; Hans Verkuil > <hverkuil@xs4all.nl> > Cc: paul.j.murphy@intel.com; daniele.alessandrelli@intel.com; > mchehab@kernel.org; linux-media@vger.kernel.org; linux- > kernel@vger.kernel.org > Subject: Re: [PATCH v8 1/4] media: i2c: imx334: replace __v4l2_ctrl_s_ctrl to > __v4l2_ctrl_modify_range > > [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 Sharavan, > > I'm a bit confused here > > On Fri, Jan 06, 2023 at 12:59:28PM +0530, shravan kumar wrote: > > From: Shravan Chippa <shravan.chippa@microchip.com> > > > > For evry mode we will get new set of values for hbalnk so use > > __v4l2_ctrl_modify_range() to support multi modes for hblank. > > > > The hblank value is readonly in the driver. because of this the > > function returns error if we try to change. so added dumy return case > > in imx334_set_ctrl function > > > > Suggested-by: Jacopo Mondi <jacopo@jmondi.org> > > Signed-off-by: Shravan Chippa <shravan.chippa@microchip.com> > > --- > > drivers/media/i2c/imx334.c | 6 +++++- > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c > > index 7b0a9086447d..ebacba3059b3 100644 > > --- a/drivers/media/i2c/imx334.c > > +++ b/drivers/media/i2c/imx334.c > > @@ -382,7 +382,8 @@ static int imx334_update_controls(struct imx334 > *imx334, > > if (ret) > > return ret; > > > > - ret = __v4l2_ctrl_s_ctrl(imx334->hblank_ctrl, mode->hblank); > > + ret = __v4l2_ctrl_modify_range(imx334->hblank_ctrl, mode->hblank, > > + mode->hblank, 1, mode->hblank); > > if (ret) > > return ret; > > > > @@ -480,6 +481,9 @@ static int imx334_set_ctrl(struct v4l2_ctrl *ctrl) > > > > pm_runtime_put(imx334->dev); > > > > + break; > > + case V4L2_CID_HBLANK: > > + ret = 0; > > Hblank is said to be read-only Yes, read-only. > > if (imx334->hblank_ctrl) > imx334->hblank_ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY; > > So you shouldn't need this safety measure here. > > However I see that __v4l2_ctrl_modify_range() can call s_ctrl() if the current > value has to be adjusted to the new limits. > > Hans, how does this work ? Do we need the above even if the control is said > to be RO ? > > Sharavan: have you experienced failures here, or is this just for safety ? The value is changing if we change the mode. While changing mode. I have experienced failures. Thanks, Shravan. > > > > break; > > default: > > dev_err(imx334->dev, "Invalid control %d", ctrl->id); > > -- > > 2.34.1 > >
> -----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 > >
From: Shravan Chippa <shravan.chippa@microchip.com> Hi This patch series is for imx334 sensor driver support for lower bandwidth Some platforms may not be capable of supporting the bandwidth required for 12 bit or 3840x2160@60 resolutions. Add support for dynamically selecting 10 bit and 1920x1080@30 resolutions while leaving the existing configuration as default V7 -> V8 -patch drop "mimx334-odify-link-frequency" as per the commnets linkfrquncy will be half of the line bandwidth -changed 1920x1080@30 mode link frequency from (891000000Mbps) to (445500000Mbps). linkfrquncy will be half of the line bandwidth V6 -> V7 Reloved: kernel test robot warning "drivers/media/i2c/imx334.c:767:15: warning: unused variable 'i' " V5 -> V6 -Drop the dt-binding patch -Optimize the code to avoid duplicating the lines -Added proper mutex while imx334_mbus_codes array -Modified Function __v4l2_ctrl_modify_range arguments as per the review commants -Added hblank dummy set ctrl -Removed Redundant comment -corrected code alignment -All commit msgs are re-written V4 -> V5 -Added 5 more patchs as per the review comments witch has below updates -Updated 1782000000Mbps link frequency for 3840x2160@60 as per the mode values -Updated 1782000000Mbps link frequency in dt-bindings also -Updated 3840x2160@60 mode array with default(reset) values -Updated hblank __v4l2_ctrl_s_ctrl() to __v4l2_ctrl_modify_range() Suggested-by: Jacopo Mondi <jacopo@jmondi.org> -Current mode update only when we try to set V4L2_SUBDEV_FORMAT_ACTIVE -Added link frequency (891000000Mbps) and pixel rate (74250000) to 1920x1080@30 mode Suggested-by: Sakari Ailus <sakari.ailus@iki.fi> -Updated commit message V3 -> V4 - Make the 12 bit and 3840x2160 as default - Set bus code SRGGB12 if set format fails V2 -> V3 - Fixed the warning reported by kernel test robot V1 -> V2 - Addressed the review comment given by Jacopo Mondi, Which has bug in imx334_enum_frame_size() loop function, - Renamed array codes[] to imx334_mbus_codes[] Shravan Chippa (5): media: i2c: imx334: modify link frequency as for the configureation media: i2c: imx334: replace __v4l2_ctrl_s_ctrl to __v4l2_ctrl_modify_range media: i2c: imx334: add missing reset values for mode 3840x2160_regs[] Shravan Chippa (4): media: i2c: imx334: replace __v4l2_ctrl_s_ctrl to __v4l2_ctrl_modify_range media: i2c: imx334: add missing reset values for mode 3840x2160_regs[] media: i2c: imx334: support lower bandwidth mode media: i2c: imx334: update pixel and link frequency drivers/media/i2c/imx334.c | 335 ++++++++++++++++++++++++++++++++++--- 1 file changed, 308 insertions(+), 27 deletions(-)