[libcamera-devel] [PATCH v2] media: i2c: ov8858 Add driver for ov8858
Robert Mader
robert.mader at posteo.de
Wed Dec 21 11:28:39 CET 2022
Hi!
Just wanted to add that I experience this darkness/gain issue on both a
very early/old PPP and a recent from from the latest batch, so I'd
assume most PPP users are affected.
Regards
On 29.11.22 16:17, Jacopo Mondi wrote:
> 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 at jmondi.org> 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
>>
>>
>>
More information about the libcamera-devel
mailing list