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 > > >
