From: Shravan Chippa <[email protected]>
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
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 <[email protected]>
-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 <[email protected]>
-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[]
media: i2c: imx334: support lower bandwidth mode
media: i2c: imx334: update pixel and link frequency
drivers/media/i2c/imx334.c | 337 ++++++++++++++++++++++++++++++++++---
1 file changed, 309 insertions(+), 28 deletions(-)
--
2.34.1
From: Shravan Chippa <[email protected]>
Currently imx334 sensor driver is configured for 1782Mbps/lane for
3840x2160@60 resolution with reqired reg mode values but if we run the
command "v4l2-ctl --all -d /dev/v4l-subdevX" it is showing incorrect link
frequeny, This is because of the incorrect value of IMX334_LINK_FREQ
witch is 891000000. it should be 1782000000.
In general with the value of 891000000 link frequency it is not possible
to configure 3840x2160@60 resolution.
Fixes: 9746b11715c3 ("media: i2c: Add imx334 camera sensor driver")
Signed-off-by: Shravan Chippa <[email protected]>
---
drivers/media/i2c/imx334.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index 7b0a9086447d..acc9f9f15e47 100644
--- a/drivers/media/i2c/imx334.c
+++ b/drivers/media/i2c/imx334.c
@@ -49,7 +49,7 @@
#define IMX334_INCLK_RATE 24000000
/* CSI2 HW configuration */
-#define IMX334_LINK_FREQ 891000000
+#define IMX334_LINK_FREQ 1782000000
#define IMX334_NUM_DATA_LANES 4
#define IMX334_REG_MIN 0x00
--
2.34.1
From: Shravan Chippa <[email protected]>
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
CC: Conor Dooley <[email protected]>
Signed-off-by: Prakash Battu <[email protected]>
Signed-off-by: Shravan Chippa <[email protected]>
---
drivers/media/i2c/imx334.c | 300 +++++++++++++++++++++++++++++++++----
1 file changed, 274 insertions(+), 26 deletions(-)
diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index 1fa7e3711c3d..e6c97e5c42d2 100644
--- a/drivers/media/i2c/imx334.c
+++ b/drivers/media/i2c/imx334.c
@@ -79,7 +79,6 @@ struct imx334_reg_list {
* struct imx334_mode - imx334 sensor mode structure
* @width: Frame width
* @height: Frame height
- * @code: Format code
* @hblank: Horizontal blanking in lines
* @vblank: Vertical blanking in lines
* @vblank_min: Minimal vertical blanking in lines
@@ -91,7 +90,6 @@ struct imx334_reg_list {
struct imx334_mode {
u32 width;
u32 height;
- u32 code;
u32 hblank;
u32 vblank;
u32 vblank_min;
@@ -119,6 +117,7 @@ struct imx334_mode {
* @vblank: Vertical blanking in lines
* @cur_mode: Pointer to current selected sensor mode
* @mutex: Mutex for serializing sensor controls
+ * @cur_code: current selected format code
* @streaming: Flag indicating streaming state
*/
struct imx334 {
@@ -140,6 +139,7 @@ struct imx334 {
u32 vblank;
const struct imx334_mode *cur_mode;
struct mutex mutex;
+ u32 cur_code;
bool streaming;
};
@@ -147,6 +147,169 @@ static const s64 link_freq[] = {
IMX334_LINK_FREQ,
};
+/* Sensor mode registers */
+static const struct imx334_reg mode_1920x1080_regs[] = {
+ {0x3000, 0x01},
+ {0x3018, 0x04},
+ {0x3030, 0xCA},
+ {0x3031, 0x08},
+ {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},
+ {0x30C7, 0x00},
+ {0x30CE, 0x00},
+ {0x30CF, 0x00},
+ {0x30D8, 0x18},
+ {0x30D9, 0x0A},
+ {0x304C, 0x00},
+ {0x304E, 0x00},
+ {0x304F, 0x00},
+ {0x3050, 0x00},
+ {0x30B6, 0x00},
+ {0x30B7, 0x00},
+ {0x3116, 0x08},
+ {0x3117, 0x00},
+ {0x31A0, 0x20},
+ {0x31A1, 0x0F},
+ {0x300C, 0x3B},
+ {0x300D, 0x29},
+ {0x314C, 0x29},
+ {0x314D, 0x01},
+ {0x315A, 0x06},
+ {0x3168, 0xA0},
+ {0x316A, 0x7E},
+ {0x319E, 0x02},
+ {0x3199, 0x00},
+ {0x319D, 0x00},
+ {0x31DD, 0x03},
+ {0x3300, 0x00},
+ {0x341C, 0xFF},
+ {0x341D, 0x01},
+ {0x3A01, 0x03},
+ {0x3A18, 0x7F},
+ {0x3A19, 0x00},
+ {0x3A1A, 0x37},
+ {0x3A1B, 0x00},
+ {0x3A1C, 0x37},
+ {0x3A1D, 0x00},
+ {0x3A1E, 0xF7},
+ {0x3A1F, 0x00},
+ {0x3A20, 0x3F},
+ {0x3A21, 0x00},
+ {0x3A20, 0x6F},
+ {0x3A21, 0x00},
+ {0x3A20, 0x3F},
+ {0x3A21, 0x00},
+ {0x3A20, 0x5F},
+ {0x3A21, 0x00},
+ {0x3A20, 0x2F},
+ {0x3A21, 0x00},
+ {0x3078, 0x02},
+ {0x3079, 0x00},
+ {0x307A, 0x00},
+ {0x307B, 0x00},
+ {0x3080, 0x02},
+ {0x3081, 0x00},
+ {0x3082, 0x00},
+ {0x3083, 0x00},
+ {0x3088, 0x02},
+ {0x3094, 0x00},
+ {0x3095, 0x00},
+ {0x3096, 0x00},
+ {0x309B, 0x02},
+ {0x309C, 0x00},
+ {0x309D, 0x00},
+ {0x309E, 0x00},
+ {0x30A4, 0x00},
+ {0x30A5, 0x00},
+ {0x3288, 0x21},
+ {0x328A, 0x02},
+ {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},
+ {0x37B0, 0x37},
+ {0x3E04, 0x0E},
+ {0x30E8, 0x50},
+ {0x30E9, 0x00},
+ {0x3E04, 0x0E},
+ {0x3002, 0x00},
+};
+
/* Sensor mode registers */
static const struct imx334_reg mode_3840x2160_regs[] = {
{0x3000, 0x01},
@@ -263,20 +426,53 @@ static const struct imx334_reg mode_3840x2160_regs[] = {
{0x3A29, 0x00},
};
+static const struct imx334_reg raw10_framefmt_regs[] = {
+ {0x3050, 0x00},
+ {0x319D, 0x00},
+ {0x341C, 0xFF},
+ {0x341D, 0x01},
+};
+
+static const struct imx334_reg raw12_framefmt_regs[] = {
+ {0x3050, 0x01},
+ {0x319D, 0x01},
+ {0x341C, 0x47},
+ {0x341D, 0x00},
+};
+
+static const u32 imx334_mbus_codes[] = {
+ MEDIA_BUS_FMT_SRGGB12_1X12,
+ MEDIA_BUS_FMT_SRGGB10_1X10,
+};
+
/* Supported sensor mode configurations */
-static const struct imx334_mode supported_mode = {
- .width = 3840,
- .height = 2160,
- .hblank = 560,
- .vblank = 2340,
- .vblank_min = 90,
- .vblank_max = 132840,
- .pclk = 594000000,
- .link_freq_idx = 0,
- .code = MEDIA_BUS_FMT_SRGGB12_1X12,
- .reg_list = {
- .num_of_regs = ARRAY_SIZE(mode_3840x2160_regs),
- .regs = mode_3840x2160_regs,
+static const struct imx334_mode supported_modes[] = {
+ {
+ .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,
+ },
+ }, {
+ .width = 1920,
+ .height = 1080,
+ .hblank = 280,
+ .vblank = 1170,
+ .vblank_min = 90,
+ .vblank_max = 132840,
+ .pclk = 74250000,
+ .link_freq_idx = 0,
+ .reg_list = {
+ .num_of_regs = ARRAY_SIZE(mode_1920x1080_regs),
+ .regs = mode_1920x1080_regs,
+ },
},
};
@@ -518,6 +714,23 @@ static const struct v4l2_ctrl_ops imx334_ctrl_ops = {
.s_ctrl = imx334_set_ctrl,
};
+static int imx334_get_format_code(struct imx334 *imx334, u32 code)
+{
+ unsigned int i;
+
+ lockdep_assert_held(&imx334->mutex);
+
+ for (i = 0; i < ARRAY_SIZE(imx334_mbus_codes); i++) {
+ if (imx334_mbus_codes[i] == code)
+ break;
+ }
+
+ if (i >= ARRAY_SIZE(imx334_mbus_codes))
+ i = 0;
+
+ return imx334_mbus_codes[i];
+}
+
/**
* imx334_enum_mbus_code() - Enumerate V4L2 sub-device mbus codes
* @sd: pointer to imx334 V4L2 sub-device structure
@@ -530,10 +743,10 @@ static int imx334_enum_mbus_code(struct v4l2_subdev *sd,
struct v4l2_subdev_state *sd_state,
struct v4l2_subdev_mbus_code_enum *code)
{
- if (code->index > 0)
+ if (code->index >= ARRAY_SIZE(imx334_mbus_codes))
return -EINVAL;
- code->code = supported_mode.code;
+ code->code = imx334_mbus_codes[code->index];
return 0;
}
@@ -550,15 +763,21 @@ static int imx334_enum_frame_size(struct v4l2_subdev *sd,
struct v4l2_subdev_state *sd_state,
struct v4l2_subdev_frame_size_enum *fsize)
{
- if (fsize->index > 0)
+ struct imx334 *imx334 = to_imx334(sd);
+ u32 code;
+
+ if (fsize->index >= ARRAY_SIZE(supported_modes))
return -EINVAL;
- if (fsize->code != supported_mode.code)
+ mutex_lock(&imx334->mutex);
+ code = imx334_get_format_code(imx334, fsize->code);
+ mutex_unlock(&imx334->mutex);
+ if (fsize->code != code)
return -EINVAL;
- fsize->min_width = supported_mode.width;
+ fsize->min_width = supported_modes[fsize->index].width;
fsize->max_width = fsize->min_width;
- fsize->min_height = supported_mode.height;
+ fsize->min_height = supported_modes[fsize->index].height;
fsize->max_height = fsize->min_height;
return 0;
@@ -577,7 +796,6 @@ static void imx334_fill_pad_format(struct imx334 *imx334,
{
fmt->format.width = mode->width;
fmt->format.height = mode->height;
- fmt->format.code = mode->code;
fmt->format.field = V4L2_FIELD_NONE;
fmt->format.colorspace = V4L2_COLORSPACE_RAW;
fmt->format.ycbcr_enc = V4L2_YCBCR_ENC_DEFAULT;
@@ -633,7 +851,13 @@ static int imx334_set_pad_format(struct v4l2_subdev *sd,
mutex_lock(&imx334->mutex);
- mode = &supported_mode;
+ fmt->format.code = imx334_get_format_code(imx334, fmt->format.code);
+
+ mode = v4l2_find_nearest_size(supported_modes,
+ ARRAY_SIZE(supported_modes),
+ width, height,
+ fmt->format.width, fmt->format.height);
+
imx334_fill_pad_format(imx334, mode, fmt);
if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
@@ -641,7 +865,8 @@ static int imx334_set_pad_format(struct v4l2_subdev *sd,
framefmt = v4l2_subdev_get_try_format(sd, sd_state, fmt->pad);
*framefmt = fmt->format;
- } else {
+ } else if (imx334->cur_mode != mode || imx334->cur_code != fmt->format.code) {
+ imx334->cur_code = fmt->format.code;
ret = imx334_update_controls(imx334, mode);
if (!ret)
imx334->cur_mode = mode;
@@ -666,11 +891,26 @@ static int imx334_init_pad_cfg(struct v4l2_subdev *sd,
struct v4l2_subdev_format fmt = { 0 };
fmt.which = sd_state ? V4L2_SUBDEV_FORMAT_TRY : V4L2_SUBDEV_FORMAT_ACTIVE;
- imx334_fill_pad_format(imx334, &supported_mode, &fmt);
+ imx334_fill_pad_format(imx334, &supported_modes[0], &fmt);
return imx334_set_pad_format(sd, sd_state, &fmt);
}
+static int imx334_set_framefmt(struct imx334 *imx334)
+{
+ switch (imx334->cur_code) {
+ case MEDIA_BUS_FMT_SRGGB10_1X10:
+ return imx334_write_regs(imx334, raw10_framefmt_regs,
+ ARRAY_SIZE(raw10_framefmt_regs));
+
+ case MEDIA_BUS_FMT_SRGGB12_1X12:
+ return imx334_write_regs(imx334, raw12_framefmt_regs,
+ ARRAY_SIZE(raw12_framefmt_regs));
+ }
+
+ return -EINVAL;
+}
+
/**
* imx334_start_streaming() - Start sensor stream
* @imx334: pointer to imx334 device
@@ -691,6 +931,13 @@ static int imx334_start_streaming(struct imx334 *imx334)
return ret;
}
+ ret = imx334_set_framefmt(imx334);
+ if (ret) {
+ dev_err(imx334->dev, "%s failed to set frame format: %d\n",
+ __func__, ret);
+ return ret;
+ }
+
/* Setup handler will write actual exposure and gain */
ret = __v4l2_ctrl_handler_setup(imx334->sd.ctrl_handler);
if (ret) {
@@ -1061,7 +1308,8 @@ static int imx334_probe(struct i2c_client *client)
}
/* Set default mode to max resolution */
- imx334->cur_mode = &supported_mode;
+ imx334->cur_mode = &supported_modes[0];
+ imx334->cur_code = imx334_mbus_codes[0];
imx334->vblank = imx334->cur_mode->vblank;
ret = imx334_init_controls(imx334);
--
2.34.1
From: Shravan Chippa <[email protected]>
There are some missing reset reg_mode values for the 3840x2160@60
resolution. The camera sensor still works in 3840x2160@60 resolution mode
because of the register reset values. This is an issue when we change the
modes dynamically. As an example, when we change the mode from 1920x1080@30
resolution to 3840x2160@60 resoultion then the mode values will be written
to the registers from the array mode_3840x2160_regs[] which gives the wrong
output which is incorrect resolution.
So add the missing reset values to the mode_3840x2160_regs[].
Signed-off-by: Shravan Chippa <[email protected]>
---
drivers/media/i2c/imx334.c | 20 ++++++++++++++++++++
1 file changed, 20 insertions(+)
diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
index a742b60ea3b0..1fa7e3711c3d 100644
--- a/drivers/media/i2c/imx334.c
+++ b/drivers/media/i2c/imx334.c
@@ -166,6 +166,7 @@ static const struct imx334_reg mode_3840x2160_regs[] = {
{0x3288, 0x21},
{0x328a, 0x02},
{0x302c, 0x3c},
+ {0x302d, 0x00},
{0x302e, 0x00},
{0x302f, 0x0f},
{0x3076, 0x70},
@@ -240,7 +241,26 @@ static const struct imx334_reg mode_3840x2160_regs[] = {
{0x3794, 0x7a},
{0x3796, 0xa1},
{0x3e04, 0x0e},
+ {0x319e, 0x00},
{0x3a00, 0x01},
+ {0x3A18, 0xBF},
+ {0x3A19, 0x00},
+ {0x3A1A, 0x67},
+ {0x3A1B, 0x00},
+ {0x3A1C, 0x6F},
+ {0x3A1D, 0x00},
+ {0x3A1E, 0xD7},
+ {0x3A1F, 0x01},
+ {0x3A20, 0x6F},
+ {0x3A21, 0x00},
+ {0x3A22, 0xCF},
+ {0x3A23, 0x00},
+ {0x3A24, 0x6F},
+ {0x3A25, 0x00},
+ {0x3A26, 0xB7},
+ {0x3A27, 0x00},
+ {0x3A28, 0x5F},
+ {0x3A29, 0x00},
};
/* Supported sensor mode configurations */
--
2.34.1
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 e6c97e5c42d2..4335c73f4d5c 100644
--- a/drivers/media/i2c/imx334.c
+++ b/drivers/media/i2c/imx334.c
@@ -50,6 +50,7 @@
/* CSI2 HW configuration */
#define IMX334_LINK_FREQ 1782000000
+#define IMX334_LINK_FREQ_891M 891000000
#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_891M,
};
/* 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
From: Shravan Chippa <[email protected]>
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 <[email protected]>
Signed-off-by: Shravan Chippa <[email protected]>
---
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 acc9f9f15e47..a742b60ea3b0 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;
break;
default:
dev_err(imx334->dev, "Invalid control %d", ctrl->id);
--
2.34.1
Hi Shravan
On Mon, Dec 19, 2022 at 11:45:22AM +0530, shravan kumar wrote:
> From: Shravan Chippa <[email protected]>
>
> Currently imx334 sensor driver is configured for 1782Mbps/lane for
> 3840x2160@60 resolution with reqired reg mode values but if we run the
> command "v4l2-ctl --all -d /dev/v4l-subdevX" it is showing incorrect link
> frequeny, This is because of the incorrect value of IMX334_LINK_FREQ
> witch is 891000000. it should be 1782000000.
>
> In general with the value of 891000000 link frequency it is not possible
> to configure 3840x2160@60 resolution.
>
> Fixes: 9746b11715c3 ("media: i2c: Add imx334 camera sensor driver")
>
> Signed-off-by: Shravan Chippa <[email protected]>
> ---
> drivers/media/i2c/imx334.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> index 7b0a9086447d..acc9f9f15e47 100644
> --- a/drivers/media/i2c/imx334.c
> +++ b/drivers/media/i2c/imx334.c
> @@ -49,7 +49,7 @@
> #define IMX334_INCLK_RATE 24000000
>
> /* CSI2 HW configuration */
> -#define IMX334_LINK_FREQ 891000000
> +#define IMX334_LINK_FREQ 1782000000
Is this your reasoning ?
width: 3840
hblank: 560
height: 2160
vblank: 2340
bpp: 12
fps: 60
lanes: 4
Total bandwidth: (3840 + 560) * (2160 + 2340) * 60 * 12 = 14.256.000.000
Bandwidth per lane = Total / 4 = 3.564.000.000
mipi clock = Bandwidth_per_lane / 2 = 1.782.000.000
Two questions:
- Should you update the pixel clock as well ? It is currently set to
594000000 while as per the above reasoning it should be doubled too.
- Where is the sensor's clock tree programmed in the driver ?
It's kind of weird that the pixel_clock and link_freq in the driver
are half of what they theoretically should be...
> #define IMX334_NUM_DATA_LANES 4
>
> #define IMX334_REG_MIN 0x00
> --
> 2.34.1
>
> -----Original Message-----
> From: Jacopo Mondi <[email protected]>
> Sent: 19 December 2022 08:14 PM
> To: shravan Chippa - I35088 <[email protected]>
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH v7 1/5] media: i2c: imx334: modify link frequency as for
> the configureation
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
>
> Hi Shravan
>
> On Mon, Dec 19, 2022 at 11:45:22AM +0530, shravan kumar wrote:
> > From: Shravan Chippa <[email protected]>
> >
> > Currently imx334 sensor driver is configured for 1782Mbps/lane for
> > 3840x2160@60 resolution with reqired reg mode values but if we run the
> > command "v4l2-ctl --all -d /dev/v4l-subdevX" it is showing incorrect
> > link frequeny, This is because of the incorrect value of
> > IMX334_LINK_FREQ witch is 891000000. it should be 1782000000.
> >
> > In general with the value of 891000000 link frequency it is not
> > possible to configure 3840x2160@60 resolution.
> >
> > Fixes: 9746b11715c3 ("media: i2c: Add imx334 camera sensor driver")
> >
> > Signed-off-by: Shravan Chippa <[email protected]>
> > ---
> > drivers/media/i2c/imx334.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > index 7b0a9086447d..acc9f9f15e47 100644
> > --- a/drivers/media/i2c/imx334.c
> > +++ b/drivers/media/i2c/imx334.c
> > @@ -49,7 +49,7 @@
> > #define IMX334_INCLK_RATE 24000000
> >
> > /* CSI2 HW configuration */
> > -#define IMX334_LINK_FREQ 891000000
> > +#define IMX334_LINK_FREQ 1782000000
>
> Is this your reasoning ?
>
> width: 3840
> hblank: 560
> height: 2160
> vblank: 2340
> bpp: 12
> fps: 60
> lanes: 4
>
> Total bandwidth: (3840 + 560) * (2160 + 2340) * 60 * 12 = 14.256.000.000
> Bandwidth per lane = Total / 4 = 3.564.000.000 mipi clock =
> Bandwidth_per_lane / 2 = 1.782.000.000
>
> Two questions:
>
> - Should you update the pixel clock as well ? It is currently set to
> 594000000 while as per the above reasoning it should be doubled too.
>
> - Where is the sensor's clock tree programmed in the driver ?
> It's kind of weird that the pixel_clock and link_freq in the driver
> are half of what they theoretically should be...
>
>
As per my understanding.
the mode_3840x2160_regs[] array value which is written through the i2c bus is 4k resolution, 60fps, link frequency 1782Mbps per lane
but the vblank value is dynamic from user space.
Min-90 to Max-130000, default value is 2340. With the default value, we will get 30fps.
if we set vblank value from user space it will change FPS.
Total bandwidth: (3840 + 560) * (2160 + 2340) * 30 * 12 = 7.128.000.000
Bandwidth per lane = Total / 4 = 1.782.000.000
Thanks,
Shravan
>
> > #define IMX334_NUM_DATA_LANES 4
> >
> > #define IMX334_REG_MIN 0x00
> > --
> > 2.34.1
> >
Hi Shravan
On Tue, Dec 20, 2022 at 11:11:15AM +0000, [email protected] wrote:
>
>
> > -----Original Message-----
> > From: Jacopo Mondi <[email protected]>
> > Sent: 19 December 2022 08:14 PM
> > To: shravan Chippa - I35088 <[email protected]>
> > Cc: [email protected]; [email protected];
> > [email protected]; [email protected]; linux-
> > [email protected]
> > Subject: Re: [PATCH v7 1/5] media: i2c: imx334: modify link frequency as for
> > the configureation
> >
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> > content is safe
> >
> > Hi Shravan
> >
> > On Mon, Dec 19, 2022 at 11:45:22AM +0530, shravan kumar wrote:
> > > From: Shravan Chippa <[email protected]>
> > >
> > > Currently imx334 sensor driver is configured for 1782Mbps/lane for
> > > 3840x2160@60 resolution with reqired reg mode values but if we run the
> > > command "v4l2-ctl --all -d /dev/v4l-subdevX" it is showing incorrect
> > > link frequeny, This is because of the incorrect value of
> > > IMX334_LINK_FREQ witch is 891000000. it should be 1782000000.
> > >
> > > In general with the value of 891000000 link frequency it is not
> > > possible to configure 3840x2160@60 resolution.
> > >
> > > Fixes: 9746b11715c3 ("media: i2c: Add imx334 camera sensor driver")
> > >
> > > Signed-off-by: Shravan Chippa <[email protected]>
> > > ---
> > > drivers/media/i2c/imx334.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/i2c/imx334.c b/drivers/media/i2c/imx334.c
> > > index 7b0a9086447d..acc9f9f15e47 100644
> > > --- a/drivers/media/i2c/imx334.c
> > > +++ b/drivers/media/i2c/imx334.c
> > > @@ -49,7 +49,7 @@
> > > #define IMX334_INCLK_RATE 24000000
> > >
> > > /* CSI2 HW configuration */
> > > -#define IMX334_LINK_FREQ 891000000
> > > +#define IMX334_LINK_FREQ 1782000000
> >
> > Is this your reasoning ?
> >
> > width: 3840
> > hblank: 560
> > height: 2160
> > vblank: 2340
> > bpp: 12
> > fps: 60
> > lanes: 4
> >
> > Total bandwidth: (3840 + 560) * (2160 + 2340) * 60 * 12 = 14.256.000.000
> > Bandwidth per lane = Total / 4 = 3.564.000.000 mipi clock =
> > Bandwidth_per_lane / 2 = 1.782.000.000
> >
> > Two questions:
> >
> > - Should you update the pixel clock as well ? It is currently set to
> > 594000000 while as per the above reasoning it should be doubled too.
> >
> > - Where is the sensor's clock tree programmed in the driver ?
> > It's kind of weird that the pixel_clock and link_freq in the driver
> > are half of what they theoretically should be...
> >
> >
> As per my understanding.
> the mode_3840x2160_regs[] array value which is written through the i2c bus is 4k resolution, 60fps, link frequency 1782Mbps per lane
> but the vblank value is dynamic from user space.
> Min-90 to Max-130000, default value is 2340. With the default value, we will get 30fps.
Ah, it's 30, not 60. So my calculations above should be halved
>
> if we set vblank value from user space it will change FPS.
>
Sure, but the link frequency stays the same, and it should be computed
with the FPS resulting from the current blankings
> Total bandwidth: (3840 + 560) * (2160 + 2340) * 30 * 12 = 7.128.000.000
Correct
> Bandwidth per lane = Total / 4 = 1.782.000.000
Correct.
But with CID_LINK_FREQ you're reporting the bus link frequency, not
the lane bandwidth. As MIPI CSI-2 uses DDR read mode, two bits per
clock cycle are transmitted, hence the bus frequency is half of the lane
bandwidth.
TL;DR you don't need this patch, the current value is correct as it is in my
understanding.
Thanks
j
>
> Thanks,
> Shravan
>
> >
> > > #define IMX334_NUM_DATA_LANES 4
> > >
> > > #define IMX334_REG_MIN 0x00
> > > --
> > > 2.34.1
> > >