Message-ID: <ZIhuEcUGdK4spvoy@tom-HP-ZBook-Fury-15-G7-Mobile-Workstation>
Date:   Tue, 13 Jun 2023 15:24:33 +0200
From:   Tommaso Merciai <tomm.merciai@...il.com>
To:     Sakari Ailus <sakari.ailus@...ux.intel.com>
Cc:     Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        jacopo.mondi@...asonboard.com, martin.hecht@...et.eu,
        linuxfancy@...glegroups.com,
        Mauro Carvalho Chehab <mchehab@...nel.org>,
        Hans Verkuil <hverkuil@...all.nl>,
        Marco Felsch <m.felsch@...gutronix.de>,
        Gerald Loacker <gerald.loacker@...fvision.net>,
        Nicholas Roth <nicholas@...hemail.net>,
        Shawn Tu <shawnx.tu@...el.com>,
        Linus Walleij <linus.walleij@...aro.org>,
        Benjamin Mugnier <benjamin.mugnier@...s.st.com>,
        linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
        Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>
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

Powered by blists - more mailing lists