On Sat, Nov 18, 2023 at 10:38:00AM +0000, Sakari Ailus wrote:
> Hi Jacopo,
>
> On Fri, Nov 17, 2023 at 03:59:39PM +0100, Jacopo Mondi wrote:
> > Hi Sakari
> >
> > On Tue, Nov 14, 2023 at 06:30:42PM +0000, Sakari Ailus wrote:
> > > Hi Jacopo,
> > >
> > > On Tue, Nov 14, 2023 at 06:24:20PM +0100, Jacopo Mondi wrote:
> > > > Hi Sakari
> > > >
> > > > On Mon, Nov 13, 2023 at 04:16:18PM +0000, Sakari Ailus wrote:
> > > > > 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.
> > > > >
> > > >
> > > > Indeed! I can move programming the long table of registers to
> > > > power_on() so that it gets written only when the device actually
> > > > resumes, and close calls to start_stream() won't need to do so.
> > >
> > > That is one option.
> >
> > Unforuntately I can't do that easily. Mixing modes does not give good
> > results if I skip re-writing the long init sequence, as the per-mode
> > register tables, specifically the max resolution one, are designed to
> > be applied to a configured sensor.
> >
> > I tried fixing the max res mode to set all the register it needs, but
> > I wasn't able to set all the requested registers (did I mention most
> > of them are undocumented ?)
>
> Should these registers be returned to the default values then? It seems the
> mode specific lists are very short compared to the init sequence so if
> you're at all concerned with the time it takes to write the long sequence,
> this would look like a simple optimisation that would have a notable
> effect.
>
> >
> > >
> > > Do note that you can't set up controls there using control handler's setup
> > > function as pm_runtime_get_if_in_use() (or ..._active()) will return an
> > > error there.
> > >
> > > >
> > > > > >
> > > > > > 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...
> > > > > > >
> > > >
> > > > Please, I'm not sure I get the issue still
> > > >
> > > > > > > > + 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.
> > > > >
> > > >
> > > > I didn't get why the two things are related :)
> > >
> >
> > Ok, now that I've read your patch series this makes sense.
> >
> > Did you mean:
> >
> > > Consider the following:
> > >
> > > 1. The application stops streaming. Autosuspend timer prevents suspending
> > > the sensor immediately even if usage_count becomes zero.
> > > 2. The application set a control. The sensor is still powered on so
> >
> > s/so/but/
> >
> > > pm_runtime_get_if_in_use() returns 0 and the control value is written to
> > > registers.
> >
> > s/is written/is not written/
>
> Ah, indeed.
>
> >
> > > 3. pm_runtime_put() at the end of s_ctrl() callback will power the sensor
> > > off immediately.
> > > 4. Application proceeds to start streaming and the sensor is powered on
> > > again.
> > >
> > > The purpose of autosuspend is to avoid (needlessly) powering off and on the
> > > device --- which takes time.
> > >
> >
> > Should I base my next version on top of "[PATCH v2 0/7] Small Runtime
> > PM API changes" ? Remember I'm going to write the init sequence in
> > start_stream() so even if I use get_if_active() I won't save much.
>
> If you're going to, yes.
>
> My point is that if you're using Runtime PM autosuspend properly (as in
> checking pm_runtime_get_sync() returns 1), you have to use get_if_active()
> there, not get_if_in_use(), as the latter will return 0 even if the device
> is in active state.
Regarding the series --- don't depend on the new functions but otherwise I
think the guidance should be sound. It'll probably take a while until we
have this in media tree.
--
Sakari Ailus
