| 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: <CAG3jFyuT09KOG9Xn-wbrr2DSOB=jSRvBVj96Vx4ja0PQWPKh6g@mail.gmail.com>
Date: Wed, 28 Jul 2021 13:22:12 +0200
From: Robert Foss <robert.foss@...aro.org>
To: Hsin-Yi Wang <hsinyi@...omium.org>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>,
Dongchun Zhu <dongchun.zhu@...iatek.com>,
Sakari Ailus <sakari.ailus@...ux.intel.com>,
Andrey Konovalov <andrey.konovalov@...aro.org>,
linux-media <linux-media@...r.kernel.org>,
Tomasz Figa <tfiga@...omium.org>,
linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] media: ov8856: Set default mbus format but allow caller
to alter
Hey Hsin-Yi,
Thanks for looking into this.
On Mon, 5 Jul 2021 at 10:17, Hsin-Yi Wang <hsinyi@...omium.org> wrote:
>
> Setting the value of V_WIN_OFF (0x3818) from 0x02 to 0x01 to use GRBG
> format still results in wrong color output if data is tuned in BGGR mode
> before.
>
> Set default mbus format for the supported modes, but allow the caller of
> set(get)_fmt to change the bayer format between BGGR and GRBG.
>
> Set the default mbus format for 3264x2448 (and 1632x1224) to BGGR as the
> data sheet states the value of this reg should be 0x02 by default.
>
> If new modes are added in the future, they can add the
> mipi_data_mbus_{format} settings into bayer_offset_configs to adjust their
> offset regs.
>
> Fixes: 2984b0ddd557 ("media: ov8856: Configure sensor for GRBG Bayer for all modes")
> Signed-off-by: Hsin-Yi Wang <hsinyi@...omium.org>
> ---
> drivers/media/i2c/ov8856.c | 83 +++++++++++++++++++++++++++++++++-----
> 1 file changed, 72 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/media/i2c/ov8856.c b/drivers/media/i2c/ov8856.c
> index 88e19f30d3762..5aac76cca6801 100644
> --- a/drivers/media/i2c/ov8856.c
> +++ b/drivers/media/i2c/ov8856.c
> @@ -107,6 +107,11 @@ static const char * const ov8856_supply_names[] = {
> "dvdd", /* Digital core power */
> };
>
> +enum {
> + OV8856_MEDIA_BUS_FMT_SBGGR10_1X10,
> + OV8856_MEDIA_BUS_FMT_SGRBG10_1X10,
> +};
> +
> struct ov8856_reg {
> u16 address;
> u8 val;
> @@ -145,6 +150,9 @@ struct ov8856_mode {
>
> /* Number of data lanes */
> u8 data_lanes;
> +
> + /* Default MEDIA_BUS_FMT for this mode */
> + u32 default_mbus_index;
> };
>
> struct ov8856_mipi_data_rates {
> @@ -1055,7 +1063,7 @@ static const struct ov8856_reg lane_4_mode_3264x2448[] = {
> {0x3810, 0x00},
> {0x3811, 0x04},
> {0x3812, 0x00},
> - {0x3813, 0x01},
> + {0x3813, 0x02},
> {0x3814, 0x01},
> {0x3815, 0x01},
> {0x3816, 0x00},
> @@ -1259,7 +1267,7 @@ static const struct ov8856_reg lane_4_mode_1632x1224[] = {
> {0x3810, 0x00},
> {0x3811, 0x02},
> {0x3812, 0x00},
> - {0x3813, 0x01},
> + {0x3813, 0x02},
> {0x3814, 0x03},
> {0x3815, 0x01},
> {0x3816, 0x00},
> @@ -1372,6 +1380,19 @@ static const struct ov8856_reg lane_4_mode_1632x1224[] = {
> {0x5e10, 0xfc}
> };
>
> +static const struct ov8856_reg mipi_data_mbus_sbggr10_1x10[] = {
> + {0x3813, 0x02},
> +};
> +
> +static const struct ov8856_reg mipi_data_mbus_sgrbg10_1x10[] = {
> + {0x3813, 0x01},
> +};
> +
> +static const u32 ov8856_mbus_codes[] = {
> + MEDIA_BUS_FMT_SBGGR10_1X10,
> + MEDIA_BUS_FMT_SGRBG10_1X10
> +};
> +
> static const char * const ov8856_test_pattern_menu[] = {
> "Disabled",
> "Standard Color Bar",
> @@ -1380,6 +1401,17 @@ static const char * const ov8856_test_pattern_menu[] = {
> "Bottom-Top Darker Color Bar"
> };
>
> +static const struct ov8856_reg_list bayer_offset_configs[] = {
> + [OV8856_MEDIA_BUS_FMT_SBGGR10_1X10] = {
> + .num_of_regs = ARRAY_SIZE(mipi_data_mbus_sbggr10_1x10),
> + .regs = mipi_data_mbus_sbggr10_1x10,
> + },
> + [OV8856_MEDIA_BUS_FMT_SGRBG10_1X10] = {
> + .num_of_regs = ARRAY_SIZE(mipi_data_mbus_sgrbg10_1x10),
> + .regs = mipi_data_mbus_sgrbg10_1x10,
> + }
> +};
> +
> struct ov8856 {
> struct v4l2_subdev sd;
> struct media_pad pad;
> @@ -1399,6 +1431,9 @@ struct ov8856 {
> /* Current mode */
> const struct ov8856_mode *cur_mode;
>
> + /* Application specified mbus format */
> + u32 cur_mbus_index;
> +
> /* To serialize asynchronus callbacks */
> struct mutex mutex;
>
> @@ -1450,6 +1485,7 @@ static const struct ov8856_lane_cfg lane_cfg_2 = {
> },
> .link_freq_index = 0,
> .data_lanes = 2,
> + .default_mbus_index = OV8856_MEDIA_BUS_FMT_SGRBG10_1X10,
> },
> {
> .width = 1640,
> @@ -1464,6 +1500,7 @@ static const struct ov8856_lane_cfg lane_cfg_2 = {
> },
> .link_freq_index = 1,
> .data_lanes = 2,
> + .default_mbus_index = OV8856_MEDIA_BUS_FMT_SGRBG10_1X10,
> }}
> };
>
> @@ -1499,6 +1536,7 @@ static const struct ov8856_lane_cfg lane_cfg_4 = {
> },
> .link_freq_index = 0,
> .data_lanes = 4,
> + .default_mbus_index = OV8856_MEDIA_BUS_FMT_SGRBG10_1X10,
> },
> {
> .width = 1640,
> @@ -1513,6 +1551,7 @@ static const struct ov8856_lane_cfg lane_cfg_4 = {
> },
> .link_freq_index = 1,
> .data_lanes = 4,
> + .default_mbus_index = OV8856_MEDIA_BUS_FMT_SGRBG10_1X10,
> },
> {
> .width = 3264,
> @@ -1527,6 +1566,7 @@ static const struct ov8856_lane_cfg lane_cfg_4 = {
> },
> .link_freq_index = 0,
> .data_lanes = 4,
> + .default_mbus_index = OV8856_MEDIA_BUS_FMT_SBGGR10_1X10,
> },
> {
> .width = 1632,
> @@ -1541,6 +1581,7 @@ static const struct ov8856_lane_cfg lane_cfg_4 = {
> },
> .link_freq_index = 1,
> .data_lanes = 4,
> + .default_mbus_index = OV8856_MEDIA_BUS_FMT_SBGGR10_1X10,
> }}
> };
>
> @@ -1904,12 +1945,21 @@ static int ov8856_init_controls(struct ov8856 *ov8856)
> return 0;
> }
>
> -static void ov8856_update_pad_format(const struct ov8856_mode *mode,
> +static void ov8856_update_pad_format(struct ov8856 *ov8856,
> + const struct ov8856_mode *mode,
> struct v4l2_mbus_framefmt *fmt)
> {
> + int index;
> +
> fmt->width = mode->width;
> fmt->height = mode->height;
> - fmt->code = MEDIA_BUS_FMT_SGRBG10_1X10;
> + for (index = 0; index < ARRAY_SIZE(ov8856_mbus_codes); ++index)
> + if (ov8856_mbus_codes[index] == fmt->code)
> + break;
> + if (index == ARRAY_SIZE(ov8856_mbus_codes))
> + index = mode->default_mbus_index;
> + fmt->code = ov8856_mbus_codes[index];
> + ov8856->cur_mbus_index = index;
> fmt->field = V4L2_FIELD_NONE;
> }
>
> @@ -1935,6 +1985,13 @@ static int ov8856_start_streaming(struct ov8856 *ov8856)
> return ret;
> }
>
> + reg_list = &bayer_offset_configs[ov8856->cur_mbus_index];
> + ret = ov8856_write_reg_list(ov8856, reg_list);
> + if (ret) {
> + dev_err(&client->dev, "failed to set mbus format");
> + return ret;
> + }
> +
> ret = __v4l2_ctrl_handler_setup(ov8856->sd.ctrl_handler);
> if (ret)
> return ret;
> @@ -2096,7 +2153,7 @@ static int ov8856_set_format(struct v4l2_subdev *sd,
> fmt->format.height);
>
> mutex_lock(&ov8856->mutex);
> - ov8856_update_pad_format(mode, &fmt->format);
> + ov8856_update_pad_format(ov8856, mode, &fmt->format);
> if (fmt->which == V4L2_SUBDEV_FORMAT_TRY) {
> *v4l2_subdev_get_try_format(sd, sd_state, fmt->pad) = fmt->format;
> } else {
> @@ -2140,7 +2197,7 @@ static int ov8856_get_format(struct v4l2_subdev *sd,
> sd_state,
> fmt->pad);
> else
> - ov8856_update_pad_format(ov8856->cur_mode, &fmt->format);
> + ov8856_update_pad_format(ov8856, ov8856->cur_mode, &fmt->format);
>
> mutex_unlock(&ov8856->mutex);
>
> @@ -2151,11 +2208,10 @@ static int ov8856_enum_mbus_code(struct v4l2_subdev *sd,
> struct v4l2_subdev_state *sd_state,
> struct v4l2_subdev_mbus_code_enum *code)
> {
> - /* Only one bayer order GRBG is supported */
> - if (code->index > 0)
> + if (code->index >= ARRAY_SIZE(ov8856_mbus_codes))
> return -EINVAL;
>
> - code->code = MEDIA_BUS_FMT_SGRBG10_1X10;
> + code->code = ov8856_mbus_codes[code->index];
>
> return 0;
> }
> @@ -2165,11 +2221,15 @@ static int ov8856_enum_frame_size(struct v4l2_subdev *sd,
> struct v4l2_subdev_frame_size_enum *fse)
> {
> struct ov8856 *ov8856 = to_ov8856(sd);
> + int index;
>
> if (fse->index >= ov8856->modes_size)
> return -EINVAL;
>
> - if (fse->code != MEDIA_BUS_FMT_SGRBG10_1X10)
> + for (index = 0; index < ARRAY_SIZE(ov8856_mbus_codes); ++index)
> + if (fse->code == ov8856_mbus_codes[index])
> + break;
> + if (index == ARRAY_SIZE(ov8856_mbus_codes))
> return -EINVAL;
>
> fse->min_width = ov8856->priv_lane->supported_modes[fse->index].width;
> @@ -2185,7 +2245,7 @@ static int ov8856_open(struct v4l2_subdev *sd, struct v4l2_subdev_fh *fh)
> struct ov8856 *ov8856 = to_ov8856(sd);
>
> mutex_lock(&ov8856->mutex);
> - ov8856_update_pad_format(&ov8856->priv_lane->supported_modes[0],
> + ov8856_update_pad_format(ov8856, &ov8856->priv_lane->supported_modes[0],
> v4l2_subdev_get_try_format(sd, fh->state, 0));
> mutex_unlock(&ov8856->mutex);
>
> @@ -2425,6 +2485,7 @@ static int ov8856_probe(struct i2c_client *client)
>
> mutex_init(&ov8856->mutex);
> ov8856->cur_mode = &ov8856->priv_lane->supported_modes[0];
> + ov8856->cur_mbus_index = ov8856->cur_mode->default_mbus_index;
> ret = ov8856_init_controls(ov8856);
> if (ret) {
> dev_err(&client->dev, "failed to init controls: %d", ret);
> --
> 2.32.0.93.g670b81a890-goog
>
Looks good to me.
Reviewed-by: Robert Foss <robert.foss@...aro.org>
Powered by blists - more mailing lists