| Message ID | 20221020195533.114049-18-hdegoede@redhat.com |
|---|---|
| State | Accepted |
| Commit | c5fafbadaeae0303514e194f37347a461577d0fb |
| Headers | show |
| Series | media: atomisp: Convert to videobuf2 | expand |
On Thu, Oct 20, 2022 at 09:55:33PM +0200, Hans de Goede wrote: > Depending on which order userspace makes various v4l2 calls, the sensor > might still be powered down when set_fmt is called. > > What should really happen here is delay the writing of the mode-related > registers till streaming is started, but for now use the same quick fix > as the atomisp_ov2680 code and call power_up() from set_fmt() in > combination with keeping track of the power-state to avoid doing the > power-up sequence twice. ... > + /* s_power has not been called yet for std v4l2 clients (camorama) */ > + power_up(sd); if camorama is fixed, will this become a problem?
Em Thu, 20 Oct 2022 21:55:33 +0200 Hans de Goede <hdegoede@redhat.com> escreveu: > Depending on which order userspace makes various v4l2 calls, the sensor > might still be powered down when set_fmt is called. > > What should really happen here is delay the writing of the mode-related > registers till streaming is started, but for now use the same quick fix > as the atomisp_ov2680 code and call power_up() from set_fmt() in > combination with keeping track of the power-state to avoid doing the > power-up sequence twice. Yes, that seems to be the right solution: to store the mode and use it during stream on. I was thinking on doing this at ov2680, but I opted not to. The rationale is that we need first to ensure that atomisp will use the standard API for the sensor drivers (this is currently not the case). As far as I recall, the try_fmt logic requires some things to come from the driver on a non-standard way, and for some, the device needs to be in power up state. It also uses it at atomisp/sensor's probe time. Regards, Mauro
Hi, On 11/14/22 13:20, Andy Shevchenko wrote: > On Thu, Oct 20, 2022 at 09:55:33PM +0200, Hans de Goede wrote: >> Depending on which order userspace makes various v4l2 calls, the sensor >> might still be powered down when set_fmt is called. >> >> What should really happen here is delay the writing of the mode-related >> registers till streaming is started, but for now use the same quick fix >> as the atomisp_ov2680 code and call power_up() from set_fmt() in >> combination with keeping track of the power-state to avoid doing the >> power-up sequence twice. > > ... > >> + /* s_power has not been called yet for std v4l2 clients (camorama) */ >> + power_up(sd); > > if camorama is fixed, will this become a problem? This is not a camorama issue but an issue with the atomisp2 + sensor driver combination. camorama uses a slightly different order in which various v4l2 ioctls are done so it needs the power_up() here. Note that power_up() checks + sets a flag so that if it gets called multiple times it only does the actual power-up once. Regards, Hans
diff --git a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c index 783f1b88ebf2..87a634bf9ff5 100644 --- a/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c +++ b/drivers/staging/media/atomisp/i2c/atomisp-gc0310.c @@ -786,8 +786,6 @@ static int gpio_ctrl(struct v4l2_subdev *sd, bool flag) return ret; } -static int power_down(struct v4l2_subdev *sd); - static int power_up(struct v4l2_subdev *sd) { struct gc0310_device *dev = to_gc0310_sensor(sd); @@ -800,6 +798,9 @@ static int power_up(struct v4l2_subdev *sd) return -ENODEV; } + if (dev->power_on) + return 0; /* Already on */ + /* power control */ ret = power_ctrl(sd, 1); if (ret) @@ -820,6 +821,7 @@ static int power_up(struct v4l2_subdev *sd) msleep(100); + dev->power_on = true; return 0; fail_gpio: @@ -844,6 +846,9 @@ static int power_down(struct v4l2_subdev *sd) return -ENODEV; } + if (!dev->power_on) + return 0; /* Already off */ + /* gpio ctrl */ ret = gpio_ctrl(sd, 0); if (ret) { @@ -861,6 +866,7 @@ static int power_down(struct v4l2_subdev *sd) if (ret) dev_err(&client->dev, "vprog failed.\n"); + dev->power_on = false; return ret; } @@ -935,6 +941,9 @@ static int gc0310_set_fmt(struct v4l2_subdev *sd, return 0; } + /* s_power has not been called yet for std v4l2 clients (camorama) */ + power_up(sd); + dev_dbg(&client->dev, "%s: before gc0310_write_reg_array %s\n", __func__, dev->res->desc); ret = startup(sd); @@ -1073,6 +1082,7 @@ static int gc0310_s_config(struct v4l2_subdev *sd, * as first power on by board may not fulfill the * power on sequqence needed by the module */ + dev->power_on = true; /* force power_down() to run */ ret = power_down(sd); if (ret) { dev_err(&client->dev, "gc0310 power-off err.\n"); diff --git a/drivers/staging/media/atomisp/i2c/gc0310.h b/drivers/staging/media/atomisp/i2c/gc0310.h index db643ebc3909..4b9ce681bd93 100644 --- a/drivers/staging/media/atomisp/i2c/gc0310.h +++ b/drivers/staging/media/atomisp/i2c/gc0310.h @@ -152,6 +152,7 @@ struct gc0310_device { int vt_pix_clk_freq_mhz; struct gc0310_resolution *res; u8 type; + bool power_on; }; enum gc0310_tok_type {
Depending on which order userspace makes various v4l2 calls, the sensor might still be powered down when set_fmt is called. What should really happen here is delay the writing of the mode-related registers till streaming is started, but for now use the same quick fix as the atomisp_ov2680 code and call power_up() from set_fmt() in combination with keeping track of the power-state to avoid doing the power-up sequence twice. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/staging/media/atomisp/i2c/atomisp-gc0310.c | 14 ++++++++++++-- drivers/staging/media/atomisp/i2c/gc0310.h | 1 + 2 files changed, 13 insertions(+), 2 deletions(-)