by Sakari Ailus

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] media: i2c: Add support for alvium camera

Hi Laurent, Tommaso,

On Fri, Jun 02, 2023 at 07:31:26AM +0300, Laurent Pinchart wrote:
> > > > > > diff --git a/drivers/media/i2c/alvium.c b/drivers/media/i2c/alvium.c
> > > > > > new file mode 100644
> > > > > > index 000000000000..e77fb6bda64b
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/media/i2c/alvium.c
> > > > > > @@ -0,0 +1,3547 @@
>
> [snip]
>
> > > > > > +static int alvium_probe(struct i2c_client *client)
> > > > > > +{
> > > > > > + struct device *dev = &client->dev;
> > > > > > + struct v4l2_subdev *sd;
> > > > > > + struct alvium_dev *alvium;
> > > > > > + int ret;
> > > > > > +
> > > > > > + alvium = devm_kzalloc(dev, sizeof(*alvium), GFP_KERNEL);
> > > > > > + if (!alvium)
> > > > > > + return -ENOMEM;
> > > > > > +
> > > > > > + alvium->i2c_client = client;
> > > > > > + ret = alvium_get_dt_data(alvium);
> > > > > > + if (ret)
> > > > > > + return ret;
> > > > > > +
> > > > > > + mutex_init(&alvium->lock);
> > > > > > +
> > > > > > + sd = &alvium->sd;
> > > > > > +
> > > > > > + /* init alvium sd */
> > > > > > + v4l2_i2c_subdev_init(sd, client, &alvium_subdev_ops);
> > > > > > +
> > > > > > + sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS | V4L2_SUBDEV_FL_HAS_DEVNODE;
> > > > > > + alvium->pad.flags = MEDIA_PAD_FL_SOURCE;
> > > > > > + sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
> > > > > > + sd->entity.ops = &alvium_sd_media_ops;
> > > > > > +
> > > > > > + ret = media_entity_pads_init(&sd->entity, 1, &alvium->pad);
> > > > > > + if (ret)
> > > > > > + return ret;
> > > > > > +
> > > > > > + sd->dev = dev;
> > > > > > +
> > > > > > + ret = alvium_power_on(alvium);
> > > > >
> > > > > The driver should use runtime PM (with autosuspend), and power on/off in
> > > > > the .s_stream() handler.
> > > >
> > > > Can we delay the pm implementation as a future patchset?
> > > > Alvium pm would be tricky (cause is the boot time of the camera)
> > > > and if is possible I want work on pm later.
> > > > Let me know. Thanks! :)
> > >
> > > With autosuspend the camera can remain powered up between stream stop
> > > and stream start, if they happen quickly enough. An autosuspend delay of
> > > a few seconds is usually a good value. It should be fairly easy to
> > > implement runtime PM support, you just need to
> > >
> > > - Call alvium_power_on() from the runtime PM resume handler and
> > > alvium_power_off() from the runtime PM suspend handler.
> > >
> > > - Call pm_runtime_resume_and_get() and stream on time, and
> > > pm_runtime_mark_last_busy() and pm_runtime_put_autosuspend() at stream
> > > stop time.
> > >
> > > - Initialize runtime PM at probe time (and clean up at remove time).
> > > There's a bit of boilerplate code needed to get that right, but it's
> > > not difficult. You can copy it from the imx290 driver.
> >
> > Back to you to clarify this point.
> >
> > Plan as you suggest is handling pm of camera using external
> > regulator. Problem is that the boot time of the camera is around 5s.
>
> 5s ? Ouch !!
>
> This has two consequences:
>
> - Just probing the camera would take 5s, which is insanely long.
> - There will be a 5s delay when starting video capture.
>
> There's no 5s delay in the current code, so I assume things work fine
> because the power regulator is always on, and turned on 5s or more
> before the driver is loaded. That's pretty fragile.
>
> That camera is clearly not a good fit for an embedded system that cares
> about power consumption and performance, but we still have to support
> it. The probe time issue isn't something we can fix, a 5s delay is
> required.
>
> The stream start issue can be alleviated by keeping the camera on, or
> offering a way for userspace to turn it on ahead of stream start.
> Runtime PM autosuspend will help with the former, and I would push the
> autosuspend delay up as a result of the huge camera boot time. We don't
> have a good solution of the latter at the moment, it used to be that
> opening video nodes would power up the whole pipeline, but that has been
> dropped some time ago in V4L2. Another API extension for this kind of

And that was never a good solution.

> use cases would be useful I think. Sakari, any opinion ?

I'd approach this with autosuspend, but going forward we could research
adding an API for V4L2 sub-devices to access PM QoS. This way the device
could be powered down while the user would have a way to ensure resuming
the device wouldn't take excessively long.

--
Kind regards,

Sakari Ailus

2023-06-13 13:36:02

by Tommaso Merciai

[permalink] [raw]

Subject: Re: [PATCH v2 2/2] media: i2c: Add support for alvium camera

Hi Sakari, Laurent,

On Tue, Jun 13, 2023 at 12:00:34PM +0000, Sakari Ailus wrote:
> Hi Laurent, Tommaso,
>
> On Fri, Jun 02, 2023 at 07:31:26AM +0300, Laurent Pinchart wrote:
> > > > > > > diff --git a/drivers/media/i2c/alvium.c b/drivers/media/i2c/alvium.c
> > > > > > > new file mode 100644
> > > > > > > index 000000000000..e77fb6bda64b
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/media/i2c/alvium.c
> > > > > > > @@ -0,0 +1,3547 @@
> >
> > [snip]
> >
> > > > > > > +static int alvium_probe(struct i2c_client *client)
> > > > > > > +{
> > > > > > > + struct device *dev = &client->dev;
> > > > > > > + struct v4l2_subdev *sd;
> > > > > > > + struct alvium_dev *alvium;
> > > > > > > + int ret;
> > > > > > > +
> > > > > > > + alvium = devm_kzalloc(dev, sizeof(*alvium), GFP_KERNEL);
> > > > > > > + if (!alvium)
> > > > > > > + return -ENOMEM;
> > > > > > > +
> > > > > > > + alvium->i2c_client = client;
> > > > > > > + ret = alvium_get_dt_data(alvium);
> > > > > > > + if (ret)
> > > > > > > + return ret;
> > > > > > > +
> > > > > > > + mutex_init(&alvium->lock);
> > > > > > > +
> > > > > > > + sd = &alvium->sd;
> > > > > > > +
> > > > > > > + /* init alvium sd */
> > > > > > > + v4l2_i2c_subdev_init(sd, client, &alvium_subdev_ops);
> > > > > > > +
> > > > > > > + sd->flags |= V4L2_SUBDEV_FL_HAS_EVENTS | V4L2_SUBDEV_FL_HAS_DEVNODE;
> > > > > > > + alvium->pad.flags = MEDIA_PAD_FL_SOURCE;
> > > > > > > + sd->entity.function = MEDIA_ENT_F_CAM_SENSOR;
> > > > > > > + sd->entity.ops = &alvium_sd_media_ops;
> > > > > > > +
> > > > > > > + ret = media_entity_pads_init(&sd->entity, 1, &alvium->pad);
> > > > > > > + if (ret)
> > > > > > > + return ret;
> > > > > > > +
> > > > > > > + sd->dev = dev;
> > > > > > > +
> > > > > > > + ret = alvium_power_on(alvium);
> > > > > >
> > > > > > The driver should use runtime PM (with autosuspend), and power on/off in
> > > > > > the .s_stream() handler.
> > > > >
> > > > > Can we delay the pm implementation as a future patchset?
> > > > > Alvium pm would be tricky (cause is the boot time of the camera)
> > > > > and if is possible I want work on pm later.
> > > > > Let me know. Thanks! :)
> > > >
> > > > With autosuspend the camera can remain powered up between stream stop
> > > > and stream start, if they happen quickly enough. An autosuspend delay of
> > > > a few seconds is usually a good value. It should be fairly easy to
> > > > implement runtime PM support, you just need to
> > > >
> > > > - Call alvium_power_on() from the runtime PM resume handler and
> > > > alvium_power_off() from the runtime PM suspend handler.
> > > >
> > > > - Call pm_runtime_resume_and_get() and stream on time, and
> > > > pm_runtime_mark_last_busy() and pm_runtime_put_autosuspend() at stream
> > > > stop time.
> > > >
> > > > - Initialize runtime PM at probe time (and clean up at remove time).
> > > > There's a bit of boilerplate code needed to get that right, but it's
> > > > not difficult. You can copy it from the imx290 driver.
> > >
> > > Back to you to clarify this point.
> > >
> > > Plan as you suggest is handling pm of camera using external
> > > regulator. Problem is that the boot time of the camera is around 5s.
> >
> > 5s ? Ouch !!
> >
> > This has two consequences:
> >
> > - Just probing the camera would take 5s, which is insanely long.
> > - There will be a 5s delay when starting video capture.
> >
> > There's no 5s delay in the current code, so I assume things work fine
> > because the power regulator is always on, and turned on 5s or more
> > before the driver is loaded. That's pretty fragile.
> >
> > That camera is clearly not a good fit for an embedded system that cares
> > about power consumption and performance, but we still have to support
> > it. The probe time issue isn't something we can fix, a 5s delay is
> > required.
> >
> > The stream start issue can be alleviated by keeping the camera on, or
> > offering a way for userspace to turn it on ahead of stream start.
> > Runtime PM autosuspend will help with the former, and I would push the
> > autosuspend delay up as a result of the huge camera boot time. We don't
> > have a good solution of the latter at the moment, it used to be that
> > opening video nodes would power up the whole pipeline, but that has been
> > dropped some time ago in V4L2. Another API extension for this kind of
>
> And that was never a good solution.
>
> > use cases would be useful I think. Sakari, any opinion ?

:'(

>
> I'd approach this with autosuspend, but going forward we could research
> adding an API for V4L2 sub-devices to access PM QoS. This way the device
> could be powered down while the user would have a way to ensure resuming
> the device wouldn't take excessively long.

What's the plan? :)

I test regulator/pm implementation in v5
Take your time to check that.

(I'm waiting for your feedback on some questions on
first review of v5) :)

If you have better ideas to handle this
very long boot time please don't esitate to share
with me. I can test on my side.

In my opinion using regulator and skipping power up
if regulator is already enabled as suggested by Laurent,
is not a so bad idea :)

Tests on the real hw are giving good results.

Let me know. Many thanks!

Regards,
Tommaso

>
> --
> Kind regards,
>
> Sakari Ailus