Hi Jacopo,
On Mon, Nov 13, 2023 at 10:19:32AM +0100, Jacopo Mondi wrote:
...
> > > + bool vbin;
> > > + bool hbin;
> >
> > I recall bool is 32 bits on arm. Is it 64 bits on arm64? Just a note, I
> > don't have a great suggestion here. :-)
> >
> > So only binning up to 2x2 is supported?
> >
>
> yes, further downscaling is obtained by skipping.
>
> The 0x3820 register has BIT(1) "vbin_vc" than enables binning, so the
> binning factor doesn't seem to be configurable. Of course there might
> be other bits/registers to control this, but I'm not aware of those
Ack.
> > > +static int ov64a40_start_streaming(struct ov64a40 *ov64a40,
> > > + struct v4l2_subdev_state *state)
> > > +{
> > > + const struct ov64a40_reglist *reglist = &ov64a40->mode->reglist;
> > > + unsigned long delay;
> > > + int ret;
> > > +
> > > + ret = pm_runtime_resume_and_get(ov64a40->dev);
> >
> > You seem to be using autosuspend, but you still do not try to avoid writes
> > of presumably the same register values if the sensor was powered on. The
> > register writes usually take the most of the time there.
>
> I'm not sure I get this comment. Are you afraid of multiple calls to
> "start_streaming" being made ? Isn't it responsibility of the bridge
> driver to handle this correctly ?
It is.
What I'm saying is that you're re-writing a lot of unchanged sensor
configuration below even if the sensor has not been powered off in the
meantime.
>
> One thing for sure, I should grab a few controls (flips, link_freq)
> avoid them being written to HW while the sensor is streaming.
>
> >
> > pm_runtime_get_sync() returns 1 if the sensor was already in active state.
> >
> > I'm about to send a patchset related to this actually, I can cc you...
> >
> > > + if (ret < 0)
> > > + return ret;
> > > +
> > > + ret = cci_multi_reg_write(ov64a40->cci, ov64a40_init,
> > > + ARRAY_SIZE(ov64a40_init), NULL);
> > > + if (ret)
> > > + goto error_power_off;
> > > +
> > > + ret = cci_multi_reg_write(ov64a40->cci, reglist->regvals,
> > > + reglist->num_regs, NULL);
> > > + if (ret)
> > > + goto error_power_off;
> > > +
> > > + ret = ov64a40_program_geometry(ov64a40);
> > > + if (ret)
> > > + goto error_power_off;
> > > +
> > > + ret = ov64a40_program_subsampling(ov64a40);
> > > + if (ret)
> > > + goto error_power_off;
> > > +
> > > + ret = __v4l2_ctrl_handler_setup(&ov64a40->ctrl_handler);
> > > + if (ret)
> > > + goto error_power_off;
> > > +
> > > + ret = cci_write(ov64a40->cci, OV64A40_REG_SMIA,
> > > + OV64A40_REG_SMIA_STREAMING, NULL);
> > > + if (ret)
> > > + goto error_power_off;
> > > +
> > > + /* delay: max(4096 xclk pulses, 150usec) + exposure time */
> > > + delay = DIV_ROUND_UP(4096, OV64A40_XCLK_FREQ / 1000 / 1000);
> > > + delay = max(delay, 150ul);
> > > + delay += DIV_ROUND_UP(ov64a40->mode->ppl * ov64a40->exposure->cur.val,
> > > + OV64A40_PIXEL_RATE / 1000 / 1000);
> > > + fsleep(delay);
> > > +
> > > + return 0;
> > > +
> > > +error_power_off:
> > > + pm_runtime_mark_last_busy(ov64a40->dev);
> > > + pm_runtime_put_autosuspend(ov64a40->dev);
> > > +
> > > + return ret;
> > > +}
...
> > > +static int ov64a40_set_ctrl(struct v4l2_ctrl *ctrl)
> > > +{
> > > + struct ov64a40 *ov64a40 = container_of(ctrl->handler, struct ov64a40,
> > > + ctrl_handler);
> > > + int ret;
> > > +
> > > + if (ctrl->id == V4L2_CID_VBLANK) {
> > > + int exp_max = ov64a40->mode->height + ctrl->val
> > > + - OV64A40_EXPOSURE_MARGIN;
> > > + int exp_val = min(ov64a40->exposure->cur.val, exp_max);
> > > +
> > > + __v4l2_ctrl_modify_range(ov64a40->exposure,
> > > + ov64a40->exposure->minimum,
> > > + exp_max, 1, exp_val);
> > > + }
> > > +
> > > + if (pm_runtime_get_if_in_use(ov64a40->dev) == 0)
> >
> > The function you should use here is actually called
> > pm_runtime_get_if_active(), but this change would better be postponed after
> > my patchset.
>
> `int pm_runtime_get_if_in_use(struct device *dev);`
> - return -EINVAL if 'power.disable_depth' is nonzero; otherwise, if the
> runtime PM status is RPM_ACTIVE and the runtime PM usage counter is
> nonzero, increment the counter and return 1; otherwise return 0 without
> changing the counter
>
> `int pm_runtime_get_if_active(struct device *dev, bool ign_usage_count);`
> - return -EINVAL if 'power.disable_depth' is nonzero; otherwise, if the
> runtime PM status is RPM_ACTIVE, and either ign_usage_count is true
> or the device's usage_count is non-zero, increment the counter and
> return 1; otherwise return 0 without changing the counter
>
>
> The only difference I see here is the additional 'ign_usage_count'
> which allows to forcefully resume the device by ignoring the usage
> counter ? Why would you forcefully resume the device here ? Don't we
> actually want the opposite and use this check to only access the HW if
> the device is powered already ?
>
It ignores the usage_count before the call while incrementing it if the
device was in active state. Although this change isn't useful if you
continue to re-write the configuration to sensor's registers in streamon
nevertheless.
> >
> > > + return 0;
> > > +
> > > + switch (ctrl->id) {
> > > + case V4L2_CID_EXPOSURE:
> > > + ret = cci_write(ov64a40->cci, OV64A40_REG_MEC_LONG_EXPO,
> > > + ctrl->val, NULL);
> > > + break;
> > > + case V4L2_CID_ANALOGUE_GAIN:
> > > + ret = ov64a40_set_anagain(ctrl);
> > > + break;
> > > + case V4L2_CID_VBLANK:
> > > + ret = cci_write(ov64a40->cci, OV64A40_REG_TIMINGS_VTS,
> > > + ctrl->val + ov64a40->mode->height, NULL);
> > > + break;
> > > + case V4L2_CID_VFLIP:
> > > + ret = cci_update_bits(ov64a40->cci, OV64A40_REG_TIMING_CTRL_20,
> > > + OV64A40_TIMING_CTRL_20_VFLIP,
> > > + ctrl->val << 2,
> > > + NULL);
> > > + break;
> > > + case V4L2_CID_HFLIP:
> > > + ret = cci_update_bits(ov64a40->cci, OV64A40_REG_TIMING_CTRL_21,
> > > + OV64A40_TIMING_CTRL_21_HFLIP,
> > > + ctrl->val << 2,
> > > + NULL);
> > > + break;
> > > + default:
> > > + dev_err(ov64a40->dev, "Unhandled control: %#x\n", ctrl->id);
> > > + ret = -EINVAL;
> > > + break;
> > > + }
> > > +
> > > + pm_runtime_put(ov64a40->dev);
> > > +
> > > + return ret;
> > > +}
...
> > > + xclk_freq = clk_get_rate(ov64a40->xclk);
> > > + if (xclk_freq != OV64A40_XCLK_FREQ) {
> > > + dev_err(&client->dev, "Unsupported xclk frequency %u\n",
> > > + xclk_freq);
> > > + return -EINVAL;
> >
> > Feel free to handle this not as an error. Up to you.
> >
>
> Well, the driver won't work correctly if the clock is not running
> at the expected frequency..
There is some number of systems where you can't get this exact frequency
but typically something fairly close. The above check prevents the driver
from working in that case.
As I wrote, this is up to you.
--
Regards,
Sakari Ailus
