Re: [PATCH v2] media: i2c: ov8858 Add driver for ov8858

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



To continue following up on this

I found an old version of a driver for the 8858 from a very old
android BSP, which mentions
https://android.googlesource.com/kernel/x86/+/android-x86-grant-3.10-marshmallow-mr1-wear-release/drivers/external_drivers/camera/drivers/media/i2c/ov8858.h#417

        /*
         * [10:7] are integer gain, [6:0] are fraction gain.  For
         * example: 0x80 is 1x gain, 0x100 is 2x gain, 0x1C0 is 3.5x
         * gain

	{OV8858_8BIT, 0x3508, 0x02}, /* long gain = 0x0200 */
	{OV8858_8BIT, 0x3509, 0x00}, /* long gain = 0x0200 *//

Which suggests the gain format is actually Q4.7

This results in a camera sensor helper with { m0 = 1, c1 = 128 }

        class CameraSensorHelperOv8858 : public CameraSensorHelper
        {
        public:
                CameraSensorHelperOv8858()
                {
                        gainType_ = AnalogueGainLinear;
                        gainConstants_.linear = { 1, 0, 0, 128 };
                }
        };
        REGISTER_CAMERA_SENSOR_HELPER("ov8858", CameraSensorHelperOv8858)

Which, surprise surprise, it's very similar to the ov8865 one (might be
a pure coincidence though)

        class CameraSensorHelperOv8865 : public CameraSensorHelper
        {
        public:
                CameraSensorHelperOv8865()
                {
                        gainType_ = AnalogueGainLinear;
                        gainConstants_.linear = { 1, 0, 0, 128 };
                }
        };
        REGISTER_CAMERA_SENSOR_HELPER("ov8865", CameraSensorHelperOv8865)

With this, I get much better results, however the image is still a
little dark.

I got brighter results poking bits above the 10th as I said in the
previous email the brighter images I can get were obtained with 8192
(0x1fff) obtained by setting the higher 12th and 11th MSBs.

The register description mentions it has 13 valid bits, but whenever
I use the 2 MSB ones I hit non-linearity issues. I wonder if the analog
gain could actually be pushed above 16x [1], but it is limited to that in
software for stable results.

Added Robert in cc which is experiencing dark images as I am

[1] The chip manual says "The OV8858 has a maximum 16x analog gain."

On Mon, Nov 28, 2022 at 12:04:50PM -0600, Nicholas Roth wrote:
> I took pictures with both sensors and posted them to https://github.com/
> waydroid/waydroid/issues/519 without any issues. However, someone else
> following along on the ticket also noted that their pictures were unusable dark
> as well with the same configuration I used.
>
> I was using Megi’s driver and kernel, the same one I submitted for upstreaming.
> I wonder if there may be some variability in the hardware?
>
> On Nov 28, 2022, at 11:53 AM, Jacopo Mondi <jacopo@xxxxxxxxxx> wrote:
>
>
>
>     Hi Nicholas,
>       a few notes on testing the analogue gain response on your driver
>
>     On Sun, Nov 06, 2022 at 11:11:30AM -0600, Nicholas Roth wrote:
>     [snip]
>
>
>         +    case V4L2_CID_ANALOGUE_GAIN:
>
>         +        ret = ov8858_write_reg(ov8858->client,
>
>         +                    OV8858_REG_GAIN_H,
>
>         +                    OV8858_REG_VALUE_08BIT,
>
>         +                    (ctrl->val >> OV8858_GAIN_H_SHIFT) &
>
>         +                    OV8858_GAIN_H_MASK);
>
>         +        ret |= ov8858_write_reg(ov8858->client,
>
>         +                    OV8858_REG_GAIN_L,
>
>         +                    OV8858_REG_VALUE_08BIT,
>
>         +                    ctrl->val & OV8858_GAIN_L_MASK);
>
>         +        break;
>
>
>     I've started this investigation because I have very dark images when
>     running from this sensor.
>
>     I'm running libcamera with the camera sensor helper that you have
>     submitted, which implements a linear gain model with (m0=1,c1=16)
>     which seems to match the registers documentation when running in "real
>     mode". Quoting:
>
>     0x3503[2]=0, gain[7:0] is real gain format, where low 4 bits are
>     fraction bits, for example, 0x10 is 1x gain, 0x28 is 2.5x gain
>
>     Unfortunately I don't get usable images with this configuration.
>     My testing procedure has been
>
>     1) Disconnect DelayedControls from RkISP1 pipeline handler to avoid it
>       writing controls to the sensor
>
>     --- a/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>     +++ b/src/libcamera/pipeline/rkisp1/rkisp1.cpp
>     @@ -393,9 +393,9 @@ void RkISP1CameraData::paramFilled(unsigned int frame)
>     }
>
>     void RkISP1CameraData::setSensorControls([[maybe_unused]] unsigned int
>     frame,
>     -                                        const ControlList &sensorControls)
>     +                                        [[maybe_unused]] const ControlList
>     &sensorControls)
>     {
>     -       delayedCtrls_->push(sensorControls);
>     +       //delayedCtrls_->push(sensorControls);
>     }
>
>     2) Disable Agc and Awb algorithm by removing the entries in
>       /usr/share/libcamera/rkisp1/uncalibrated.yaml
>
>     3) Run a preview window and observe how it changes by issuing controls
>       with v4l2-ctl:
>
>       1) set a reasonable exposure value
>          $ v4l2-ctl -c 0x00980911=2047 -d /dev/v4l-subdev4
>
>       2) pump up digital gain a bit
>         $ v4l2-ctl -c 0x009f0905=800 -d /dev/v4l-subdev4
>
>       3) Start sending analogue gain controls to see how the image
>          response changes
>
>         $ v4l2-ctl -c 0x009e0903=128 -d /dev/v4l-subdev4
>         $ v4l2-ctl -c 0x009e0903=512 -d /dev/v4l-subdev4
>         $ v4l2-ctl -c 0x009e0903=1024 -d /dev/v4l-subdev4
>         $ v4l2-ctl -c 0x009e0903=2048 -d /dev/v4l-subdev4
>
>     My observations are that
>     - The highest gain I can apply is obtained with 8191, which
>      corresponds to 0x1fff which matches to the register description
>      of:
>
>      0x3508 LONG_GAIN_HIGH = gain[12:6]
>      0x3509 LONG_GAIN_LOW = gain[7:0]
>
>      This seems to indicate the gain register is actually 13-bits long
>      and not
>
>     - The gain is not linear:
>
>      127 is more brilliant than 128
>      2047 is more brilliant than 2048
>
>     I'm afraid the description we get for the register is not accurate and
>     doesn't tell exactly how the gain value is assembled in the 0x3508 and
>     0x3509 register ?
>
>     Are you experiencing anything similar ?
>
>     Thanks
>      j
>
>
>         +    case V4L2_CID_DIGITAL_GAIN:
>
>         +        ret = ov8858_write_reg(ov8858->client,
>
>         +                    OV8858_REG_DGAIN_H,
>
>         +                    OV8858_REG_VALUE_08BIT,
>
>         +                    (ctrl->val >> OV8858_DGAIN_H_SHIFT) &
>
>         +                    OV8858_DGAIN_H_MASK);
>
>         +        ret |= ov8858_write_reg(ov8858->client,
>
>         +                    OV8858_REG_DGAIN_L,
>
>         +                    OV8858_REG_VALUE_08BIT,
>
>         +                    ctrl->val & OV8858_DGAIN_L_MASK);
>
>         +        break;
>
>         +    case V4L2_CID_VBLANK:
>
>         +        ret = ov8858_write_reg(ov8858->client,
>
>         +                    OV8858_REG_VTS,
>
>         +                    OV8858_REG_VALUE_16BIT,
>
>         +                    ctrl->val + ov8858->cur_mode->height);
>
>         +        break;
>
>         +    case V4L2_CID_TEST_PATTERN:
>
>         +        ret = ov8858_enable_test_pattern(ov8858, ctrl->val);
>
>         +        break;
>
>         +    default:
>
>         +        dev_warn(&client->dev, "%s Unhandled id:0x%x, val:0x%x\n",
>
>         +             __func__, ctrl->id, ctrl->val);
>
>         +        break;
>
>         +    }
>
>         +
>
>         +    pm_runtime_put(&client->dev);
>
>         +
>
>         +    return ret;
>
>         +}
>
>         +
>
>         +static const struct v4l2_ctrl_ops ov8858_ctrl_ops = {
>
>         +    .s_ctrl = ov8858_set_ctrl,
>
>         +};
>
>         +
>
>         +static int ov8858_initialize_controls(struct ov8858 *ov8858)
>
>         +{
>
>         +    const struct ov8858_mode *mode;
>
>         +    struct v4l2_ctrl_handler *handler;
>
>         +    struct v4l2_ctrl *ctrl;
>
>         +    s64 exposure_max, vblank_def;
>
>         +    u32 h_blank;
>
>         +    int ret;
>
>         +
>
>         +    handler = &ov8858->ctrl_handler;
>
>         +    mode = ov8858->cur_mode;
>
>         +    ret = v4l2_ctrl_handler_init(handler, 8);
>
>         +    if (ret)
>
>         +        return ret;
>
>         +    handler->lock = &ov8858->mutex;
>
>         +
>
>         +    ctrl = v4l2_ctrl_new_int_menu(handler, NULL, V4L2_CID_LINK_FREQ,
>
>         +                      0, 0, link_freq_menu_items);
>
>         +    if (ctrl)
>
>         +        ctrl->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>
>         +
>
>         +    v4l2_ctrl_new_std(handler, NULL, V4L2_CID_PIXEL_RATE,
>
>         +              0, ov8858->pixel_rate, 1, ov8858->pixel_rate);
>
>         +
>
>         +    h_blank = mode->hts_def - mode->width;
>
>         +    ov8858->hblank = v4l2_ctrl_new_std(handler, NULL, V4L2_CID_HBLANK,
>
>         +                h_blank, h_blank, 1, h_blank);
>
>         +    if (ov8858->hblank)
>
>         +        ov8858->hblank->flags |= V4L2_CTRL_FLAG_READ_ONLY;
>
>         +
>
>         +    vblank_def = mode->vts_def - mode->height;
>
>         +    ov8858->vblank = v4l2_ctrl_new_std(handler, &ov8858_ctrl_ops,
>
>         +                V4L2_CID_VBLANK, vblank_def,
>
>         +                OV8858_VTS_MAX - mode->height,
>
>         +                1, vblank_def);
>
>         +
>
>         +    exposure_max = mode->vts_def - 4;
>
>         +    ov8858->exposure = v4l2_ctrl_new_std(handler, &ov8858_ctrl_ops,
>
>         +                V4L2_CID_EXPOSURE, OV8858_EXPOSURE_MIN,
>
>         +                exposure_max, OV8858_EXPOSURE_STEP,
>
>         +                mode->exp_def);
>
>         +
>
>         +    ov8858->anal_gain = v4l2_ctrl_new_std(handler, &ov8858_ctrl_ops,
>
>         +                V4L2_CID_ANALOGUE_GAIN, OV8858_GAIN_MIN,
>
>         +                OV8858_GAIN_MAX, OV8858_GAIN_STEP,
>
>         +                OV8858_GAIN_DEFAULT);
>
>         +
>
>         +    ov8858->digi_gain = v4l2_ctrl_new_std(handler, &ov8858_ctrl_ops,
>
>         +                V4L2_CID_DIGITAL_GAIN, OV8858_DGAIN_MIN,
>
>         +                OV8858_DGAIN_MAX, OV8858_DGAIN_STEP,
>
>         +                OV8858_DGAIN_DEFAULT);
>
>         +
>
>         +    ov8858->test_pattern = v4l2_ctrl_new_std_menu_items(handler,
>
>         +                &ov8858_ctrl_ops, V4L2_CID_TEST_PATTERN,
>
>         +                ARRAY_SIZE(ov8858_test_pattern_menu) - 1,
>
>         +                0, 0, ov8858_test_pattern_menu);
>
>         +
>
>         +    if (handler->error) {
>
>         +        ret = handler->error;
>
>         +        dev_err(&ov8858->client->dev,
>
>         +            "Failed to init controls(%d)\n", ret);
>
>         +        goto err_free_handler;
>
>         +    }
>
>         +
>
>         +    ov8858->subdev.ctrl_handler = handler;
>
>         +
>
>         +    return 0;
>
>         +
>
>         +err_free_handler:
>
>         +    v4l2_ctrl_handler_free(handler);
>
>         +
>
>         +    return ret;
>
>         +}
>
>         +
>
>         +static int ov8858_check_sensor_id(struct ov8858 *ov8858,
>
>         +                   struct i2c_client *client)
>
>         +{
>
>         +    struct device *dev = &ov8858->client->dev;
>
>         +    u32 id = 0;
>
>         +    int ret;
>
>         +
>
>         +    ret = ov8858_read_reg(client, OV8858_REG_CHIP_ID,
>
>         +                  OV8858_REG_VALUE_24BIT, &id);
>
>         +    if (id != CHIP_ID) {
>
>         +        dev_err(dev, "Unexpected sensor id(%06x), ret(%d)\n", id,
>         ret);
>
>         +        return ret;
>
>         +    }
>
>         +
>
>         +    ret = ov8858_read_reg(client, OV8858_CHIP_REVISION_REG,
>
>         +                  OV8858_REG_VALUE_08BIT, &id);
>
>         +    if (ret) {
>
>         +        dev_err(dev, "Read chip revision register error\n");
>
>         +        return ret;
>
>         +    }
>
>         +
>
>         +    dev_info(dev, "Detected OV%06x sensor, REVISION 0x%x\n", CHIP_ID,
>         id);
>
>         +
>
>         +    if (id == OV8858_R2A) {
>
>         +        if (4 == ov8858->lane_num) {
>
>         +            ov8858_global_regs = ov8858_global_regs_r2a_4lane;
>
>         +        } else {
>
>         +            ov8858_global_regs = ov8858_global_regs_r2a_2lane;
>
>         +        }
>
>         +
>
>         +        ov8858->is_r2a = true;
>
>         +    } else {
>
>         +        ov8858_global_regs = ov8858_global_regs_r1a;
>
>         +        ov8858->is_r2a = false;
>
>         +        dev_warn(dev, "R1A may not work well current!\n");
>
>         +    }
>
>         +
>
>         +    return 0;
>
>         +}
>
>         +
>
>         +static int ov8858_configure_regulators(struct ov8858 *ov8858)
>
>         +{
>
>         +    unsigned int i;
>
>         +
>
>         +    for (i = 0; i < OV8858_NUM_SUPPLIES; i++)
>
>         +        ov8858->supplies[i].supply = ov8858_supply_names[i];
>
>         +
>
>         +    return devm_regulator_bulk_get(&ov8858->client->dev,
>
>         +                       OV8858_NUM_SUPPLIES,
>
>         +                       ov8858->supplies);
>
>         +}
>
>         +
>
>         +static int ov8858_parse_of(struct ov8858 *ov8858)
>
>         +{
>
>         +    struct device *dev = &ov8858->client->dev;
>
>         +    struct device_node *endpoint;
>
>         +    struct fwnode_handle *fwnode;
>
>         +    int rval;
>
>         +
>
>         +    endpoint = of_graph_get_next_endpoint(dev->of_node, NULL);
>
>         +    if (!endpoint) {
>
>         +        dev_err(dev, "Failed to get endpoint\n");
>
>         +        return -EINVAL;
>
>         +    }
>
>         +
>
>         +    fwnode = of_fwnode_handle(endpoint);
>
>         +    rval = fwnode_property_read_u32_array(fwnode, "data-lanes", NULL,
>         0);
>
>         +    if (rval <= 0) {
>
>         +        dev_warn(dev, " Get mipi lane num failed!\n");
>
>         +        return -1;
>
>         +    }
>
>         +
>
>         +    ov8858->lane_num = rval;
>
>         +    if (4 == ov8858->lane_num) {
>
>         +        ov8858->cur_mode = &supported_modes_4lane[0];
>
>         +        supported_modes = supported_modes_4lane;
>
>         +        ov8858->cfg_num = ARRAY_SIZE(supported_modes_4lane);
>
>         +
>
>         +        /* pixel rate = link frequency * 2 * lanes / BITS_PER_SAMPLE *
>         /
>
>         +        ov8858->pixel_rate = MIPI_FREQ * 2U * ov8858->lane_num / 10U;
>
>         +        dev_info(dev, "lane_num(%d)  pixel_rate(%u)\n",
>
>         +                 ov8858->lane_num, ov8858->pixel_rate);
>
>         +    } else {
>
>         +        ov8858->cur_mode = &supported_modes_2lane[0];
>
>         +        supported_modes = supported_modes_2lane;
>
>         +        ov8858->cfg_num = ARRAY_SIZE(supported_modes_2lane);
>
>         +
>
>         +        /*pixel rate = link frequency * 2 * lanes / BITS_PER_SAMPLE */
>
>         +        ov8858->pixel_rate = MIPI_FREQ * 2U * (ov8858->lane_num) /
>         10U;
>
>         +        dev_info(dev, "lane_num(%d)  pixel_rate(%u)\n",
>
>         +                 ov8858->lane_num, ov8858->pixel_rate);
>
>         +    }
>
>         +    return 0;
>
>         +}
>
>         +
>
>         +static int ov8858_probe(struct i2c_client *client,
>
>         +             const struct i2c_device_id *id)
>
>         +{
>
>         +    struct device *dev = &client->dev;
>
>         +    struct device_node *node = dev->of_node;
>
>         +    struct ov8858 *ov8858;
>
>         +    struct v4l2_subdev *sd;
>
>         +    int ret;
>
>         +
>
>         +    ov8858 = devm_kzalloc(dev, sizeof(*ov8858), GFP_KERNEL);
>
>         +    if (!ov8858)
>
>         +        return -ENOMEM;
>
>         +
>
>         +    ov8858->client = client;
>
>         +
>
>         +    ov8858->xvclk = devm_clk_get(dev, "xvclk");
>
>         +    if (IS_ERR(ov8858->xvclk))
>
>         +        return dev_err_probe(dev, PTR_ERR(ov8858->xvclk),
>
>         +                     "Failed to get xvclk\n");
>
>         +
>
>         +    ov8858->reset_gpio = devm_gpiod_get_optional(dev, "reset",
>
>         +                             GPIOD_OUT_HIGH);
>
>         +    if (IS_ERR(ov8858->reset_gpio))
>
>         +        return dev_err_probe(dev, PTR_ERR(ov8858->reset_gpio),
>
>         +                     "Failed to get reset gpio\n");
>
>         +
>
>         +    ov8858->pwdn_gpio = devm_gpiod_get_optional(dev, "powerdown",
>
>         +                            GPIOD_OUT_HIGH);
>
>         +    if (IS_ERR(ov8858->pwdn_gpio))
>
>         +        return dev_err_probe(dev, PTR_ERR(ov8858->pwdn_gpio),
>
>         +                     "Failed to get powerdown gpio\n");
>
>         +
>
>         +    ret = ov8858_configure_regulators(ov8858);
>
>         +    if (ret)
>
>         +        return dev_err_probe(dev, ret,
>
>         +                     "Failed to get power regulators\n");
>
>         +
>
>         +    ret = ov8858_parse_of(ov8858);
>
>         +    if (ret != 0)
>
>         +        return -EINVAL;
>
>         +
>
>         +    mutex_init(&ov8858->mutex);
>
>         +
>
>         +    sd = &ov8858->subdev;
>
>         +    v4l2_i2c_subdev_init(sd, client, &ov8858_subdev_ops);
>
>         +    ret = ov8858_initialize_controls(ov8858);
>
>         +    if (ret)
>
>         +        goto err_destroy_mutex;
>
>         +
>
>         +    ret = __ov8858_power_on(ov8858);
>
>         +    if (ret)
>
>         +        goto err_free_handler;
>
>         +
>
>         +    ret = ov8858_check_sensor_id(ov8858, client);
>
>         +    if (ret)
>
>         +        goto err_power_off;
>
>         +
>
>         +    sd->internal_ops = &ov8858_internal_ops;
>
>         +    sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>
>         +    ov8858->pad.flags = MEDIA_PAD_FL_SOURCE;
>
>         +    sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
>
>         +    ret = media_entity_pads_init(&sd->entity, 1, &ov8858->pad);
>
>         +    if (ret < 0)
>
>         +        goto err_power_off;
>
>         +
>
>         +    ret = v4l2_async_register_subdev_sensor(sd);
>
>         +    if (ret) {
>
>         +        dev_err(dev, "v4l2 async register subdev failed\n");
>
>         +        goto err_clean_entity;
>
>         +    }
>
>         +
>
>         +    pm_runtime_set_active(dev);
>
>         +    pm_runtime_enable(dev);
>
>         +    pm_runtime_idle(dev);
>
>         +
>
>         +    return 0;
>
>         +
>
>         +err_clean_entity:
>
>         +    media_entity_cleanup(&sd->entity);
>
>         +err_power_off:
>
>         +    __ov8858_power_off(ov8858);
>
>         +err_free_handler:
>
>         +    v4l2_ctrl_handler_free(&ov8858->ctrl_handler);
>
>         +err_destroy_mutex:
>
>         +    mutex_destroy(&ov8858->mutex);
>
>         +
>
>         +    return ret;
>
>         +}
>
>         +
>
>         +static int ov8858_remove(struct i2c_client *client)
>
>         +{
>
>         +    struct v4l2_subdev *sd = i2c_get_clientdata(client);
>
>         +    struct ov8858 *ov8858 = to_ov8858(sd);
>
>         +
>
>         +    v4l2_async_unregister_subdev(sd);
>
>         +#ifdef CONFIG_MEDIA_CONTROLLER
>
>         +    media_entity_cleanup(&sd->entity);
>
>         +#endif
>
>         +    v4l2_ctrl_handler_free(&ov8858->ctrl_handler);
>
>         +    mutex_destroy(&ov8858->mutex);
>
>         +
>
>         +    pm_runtime_disable(&client->dev);
>
>         +    if (!pm_runtime_status_suspended(&client->dev))
>
>         +        __ov8858_power_off(ov8858);
>
>         +    pm_runtime_set_suspended(&client->dev);
>
>         +
>
>         +    return 0;
>
>         +}
>
>         +
>
>         +#if IS_ENABLED(CONFIG_OF)
>
>         +static const struct of_device_id ov8858_of_match[] = {
>
>         +    { .compatible = "ovti,ov8858" },
>
>         +    {},
>
>         +};
>
>         +MODULE_DEVICE_TABLE(of, ov8858_of_match);
>
>         +#endif
>
>         +
>
>         +static const struct i2c_device_id ov8858_match_id[] = {
>
>         +    { "ovti,ov8858", 0 },
>
>         +    { },
>
>         +};
>
>         +
>
>         +static struct i2c_driver ov8858_i2c_driver = {
>
>         +    .driver = {
>
>         +        .name = OV8858_NAME,
>
>         +        .pm = &ov8858_pm_ops,
>
>         +        .of_match_table = of_match_ptr(ov8858_of_match),
>
>         +    },
>
>         +    .probe        = &ov8858_probe,
>
>         +    .remove        = &ov8858_remove,
>
>         +    .id_table    = ov8858_match_id,
>
>         +};
>
>         +
>
>         +module_i2c_driver(ov8858_i2c_driver);
>
>         +
>
>         +MODULE_DESCRIPTION("OmniVision ov8858 sensor driver");
>
>         +MODULE_LICENSE("GPL v2");
>
>         --
>
>         2.34.1
>
>
>



[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]

  Powered by Linux