| Message ID | 20220205185429.2278860-38-paul.kocialkowski@bootlin.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Allwinner A31/A83T MIPI CSI-2 Support and A31 ISP Support | expand |
Hi Paul, On Sat, Feb 05, 2022 at 07:54:00PM +0100, Paul Kocialkowski wrote: > Let's just enable the module when we start using it (at stream on) > and benefit from runtime pm instead of enabling it at first open. > > Also reorder the call to v4l2_pipeline_pm_get. > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> Nice patch! Do you still need v4l2_pipeline_pm_put()? Removing it would be a separate patch of course.
Hi Sakari, On Mon 14 Feb 22, 20:30, Sakari Ailus wrote: > Hi Paul, > > On Sat, Feb 05, 2022 at 07:54:00PM +0100, Paul Kocialkowski wrote: > > Let's just enable the module when we start using it (at stream on) > > and benefit from runtime pm instead of enabling it at first open. > > > > Also reorder the call to v4l2_pipeline_pm_get. > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > Nice patch! Thanks! > Do you still need v4l2_pipeline_pm_put()? Removing it would be a separate > patch of course. My understanding is that this is still useful if there are drivers in the pipeline that rely on s_power instead of rpm (a typical case could be an old sensor driver). So that's why this is kept around, but all other components of the pipeline (isp/csi/mipi csi-2) are using rpm now. Cheers, Paul
Hi Paul, On Tue, Feb 15, 2022 at 10:56:22AM +0100, Paul Kocialkowski wrote: > On Mon 14 Feb 22, 20:30, Sakari Ailus wrote: > > On Sat, Feb 05, 2022 at 07:54:00PM +0100, Paul Kocialkowski wrote: > > > Let's just enable the module when we start using it (at stream on) > > > and benefit from runtime pm instead of enabling it at first open. > > > > > > Also reorder the call to v4l2_pipeline_pm_get. > > > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > > > Nice patch! > > Thanks! > > > Do you still need v4l2_pipeline_pm_put()? Removing it would be a separate > > patch of course. > > My understanding is that this is still useful if there are drivers in the > pipeline that rely on s_power instead of rpm (a typical case could be an > old sensor driver). So that's why this is kept around, but all other components > of the pipeline (isp/csi/mipi csi-2) are using rpm now. If that's not the case on your test platforms, I think it would be better to drop support for this old API, and convert drivers that still use .s_power() if someone needs to use one on an Allwinner platform.
Hi Laurent, On Tue 15 Feb 22, 12:04, Laurent Pinchart wrote: > Hi Paul, > > On Tue, Feb 15, 2022 at 10:56:22AM +0100, Paul Kocialkowski wrote: > > On Mon 14 Feb 22, 20:30, Sakari Ailus wrote: > > > On Sat, Feb 05, 2022 at 07:54:00PM +0100, Paul Kocialkowski wrote: > > > > Let's just enable the module when we start using it (at stream on) > > > > and benefit from runtime pm instead of enabling it at first open. > > > > > > > > Also reorder the call to v4l2_pipeline_pm_get. > > > > > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > > > > > Nice patch! > > > > Thanks! > > > > > Do you still need v4l2_pipeline_pm_put()? Removing it would be a separate > > > patch of course. > > > > My understanding is that this is still useful if there are drivers in the > > pipeline that rely on s_power instead of rpm (a typical case could be an > > old sensor driver). So that's why this is kept around, but all other components > > of the pipeline (isp/csi/mipi csi-2) are using rpm now. > > If that's not the case on your test platforms, I think it would be > better to drop support for this old API, and convert drivers that still > use .s_power() if someone needs to use one on an Allwinner platform. I agree this is the path to follow but it feels like we're not quite there yet and a bunch of driver were not converted at this point, including some popular ones like ov5640, which I know for sure is used with Allwinner devices. Honestly I'd be happy to get rid of these legacy functions as soon as the transition is done, but doing it now would mean breaking a significant number of use cases (which I'm trying to avoid here despite all the changes). I definitely wouldn't be confident making that transition here and it probably wouldn't be a good idea to make that a requirement to merge this (already quite big) series. What do you think? Paul
Hi Paul, On Tue, Feb 15, 2022 at 11:21:17AM +0100, Paul Kocialkowski wrote: > Hi Laurent, > > On Tue 15 Feb 22, 12:04, Laurent Pinchart wrote: > > Hi Paul, > > > > On Tue, Feb 15, 2022 at 10:56:22AM +0100, Paul Kocialkowski wrote: > > > On Mon 14 Feb 22, 20:30, Sakari Ailus wrote: > > > > On Sat, Feb 05, 2022 at 07:54:00PM +0100, Paul Kocialkowski wrote: > > > > > Let's just enable the module when we start using it (at stream on) > > > > > and benefit from runtime pm instead of enabling it at first open. > > > > > > > > > > Also reorder the call to v4l2_pipeline_pm_get. > > > > > > > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> > > > > > > > > Nice patch! > > > > > > Thanks! > > > > > > > Do you still need v4l2_pipeline_pm_put()? Removing it would be a separate > > > > patch of course. > > > > > > My understanding is that this is still useful if there are drivers in the > > > pipeline that rely on s_power instead of rpm (a typical case could be an > > > old sensor driver). So that's why this is kept around, but all other components > > > of the pipeline (isp/csi/mipi csi-2) are using rpm now. > > > > If that's not the case on your test platforms, I think it would be > > better to drop support for this old API, and convert drivers that still > > use .s_power() if someone needs to use one on an Allwinner platform. > > I agree this is the path to follow but it feels like we're not quite there > yet and a bunch of driver were not converted at this point, including some > popular ones like ov5640, which I know for sure is used with Allwinner devices. > > Honestly I'd be happy to get rid of these legacy functions as soon as the > transition is done, but doing it now would mean breaking a significant number > of use cases (which I'm trying to avoid here despite all the changes). > > I definitely wouldn't be confident making that transition here and it > probably wouldn't be a good idea to make that a requirement to merge this > (already quite big) series. > > What do you think? Feel free to keep it if you prefer that. All sensor drivers that implement s_power are old but there are quite a few of them. Converting them isn't trivial so best done by someone who has access to the hardware.
diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c index 22779baf8c09..7e3727c86e7a 100644 --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c @@ -148,30 +148,6 @@ bool sun6i_csi_is_format_supported(struct sun6i_csi_device *csi_dev, return false; } -int sun6i_csi_set_power(struct sun6i_csi_device *csi_dev, bool enable) -{ - struct device *dev = csi_dev->dev; - struct regmap *regmap = csi_dev->regmap; - int ret; - - if (!enable) { - regmap_update_bits(regmap, SUN6I_CSI_EN_REG, - SUN6I_CSI_EN_CSI_EN, 0); - pm_runtime_put(dev); - - return 0; - } - - ret = pm_runtime_resume_and_get(dev); - if (ret < 0) - return ret; - - regmap_update_bits(regmap, SUN6I_CSI_EN_REG, SUN6I_CSI_EN_CSI_EN, - SUN6I_CSI_EN_CSI_EN); - - return 0; -} - static enum csi_input_fmt get_csi_input_format(struct sun6i_csi_device *csi_dev, u32 mbus_code, u32 pixformat) { diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h index 1f235b8e240c..0271587e8520 100644 --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.h @@ -74,13 +74,6 @@ struct sun6i_csi_device { bool sun6i_csi_is_format_supported(struct sun6i_csi_device *csi_dev, u32 pixformat, u32 mbus_code); -/** - * sun6i_csi_set_power() - power on/off the csi - * @csi: pointer to the csi - * @enable: on/off - */ -int sun6i_csi_set_power(struct sun6i_csi_device *csi_dev, bool enable); - /** * sun6i_csi_update_config() - update the csi register settings * @csi: pointer to the csi diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c index ab6e298864ed..05eb9aae2975 100644 --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_capture.c @@ -6,6 +6,7 @@ */ #include <linux/of.h> +#include <linux/pm_runtime.h> #include <linux/regmap.h> #include <media/v4l2-device.h> @@ -141,6 +142,9 @@ static void sun6i_csi_capture_enable(struct sun6i_csi_device *csi_dev) { struct regmap *regmap = csi_dev->regmap; + regmap_update_bits(regmap, SUN6I_CSI_EN_REG, SUN6I_CSI_EN_CSI_EN, + SUN6I_CSI_EN_CSI_EN); + regmap_update_bits(regmap, SUN6I_CSI_CAP_REG, SUN6I_CSI_CAP_VCAP_ON, SUN6I_CSI_CAP_VCAP_ON); } @@ -150,6 +154,7 @@ static void sun6i_csi_capture_disable(struct sun6i_csi_device *csi_dev) struct regmap *regmap = csi_dev->regmap; regmap_update_bits(regmap, SUN6I_CSI_CAP_REG, SUN6I_CSI_CAP_VCAP_ON, 0); + regmap_update_bits(regmap, SUN6I_CSI_EN_REG, SUN6I_CSI_EN_CSI_EN, 0); } static void @@ -382,6 +387,7 @@ static int sun6i_csi_capture_start_streaming(struct vb2_queue *queue, struct sun6i_csi_capture *capture = &csi_dev->capture; struct sun6i_csi_capture_state *state = &capture->state; struct video_device *video_dev = &capture->video_dev; + struct device *dev = csi_dev->dev; struct v4l2_subdev *subdev; int ret; @@ -402,6 +408,12 @@ static int sun6i_csi_capture_start_streaming(struct vb2_queue *queue, goto error_media_pipeline; } + /* PM */ + + ret = pm_runtime_resume_and_get(dev); + if (ret < 0) + goto error_media_pipeline; + /* Clear */ sun6i_csi_capture_irq_clear(csi_dev); @@ -429,6 +441,8 @@ static int sun6i_csi_capture_start_streaming(struct vb2_queue *queue, sun6i_csi_capture_disable(csi_dev); sun6i_csi_capture_irq_disable(csi_dev); + pm_runtime_put(dev); + error_media_pipeline: media_pipeline_stop(&video_dev->entity); @@ -442,6 +456,7 @@ static void sun6i_csi_capture_stop_streaming(struct vb2_queue *queue) { struct sun6i_csi_device *csi_dev = vb2_get_drv_priv(queue); struct sun6i_csi_capture *capture = &csi_dev->capture; + struct device *dev = csi_dev->dev; struct v4l2_subdev *subdev; subdev = sun6i_csi_capture_remote_subdev(capture, NULL); @@ -451,6 +466,8 @@ static void sun6i_csi_capture_stop_streaming(struct vb2_queue *queue) sun6i_csi_capture_disable(csi_dev); sun6i_csi_capture_irq_disable(csi_dev); + pm_runtime_put(dev); + media_pipeline_stop(&capture->video_dev.entity); sun6i_csi_capture_state_cleanup(csi_dev, true); @@ -635,27 +652,20 @@ static int sun6i_csi_capture_open(struct file *file) if (mutex_lock_interruptible(&capture->lock)) return -ERESTARTSYS; - ret = v4l2_fh_open(file); + ret = v4l2_pipeline_pm_get(&capture->video_dev.entity); if (ret < 0) goto error_lock; - ret = v4l2_pipeline_pm_get(&capture->video_dev.entity); + ret = v4l2_fh_open(file); if (ret < 0) - goto error_v4l2_fh; - - /* Power on at first open. */ - if (v4l2_fh_is_singular_file(file)) { - ret = sun6i_csi_set_power(csi_dev, true); - if (ret < 0) - goto error_v4l2_fh; - } + goto error_pipeline; mutex_unlock(&capture->lock); return 0; -error_v4l2_fh: - v4l2_fh_release(file); +error_pipeline: + v4l2_pipeline_pm_put(&capture->video_dev.entity); error_lock: mutex_unlock(&capture->lock); @@ -667,19 +677,12 @@ static int sun6i_csi_capture_close(struct file *file) { struct sun6i_csi_device *csi_dev = video_drvdata(file); struct sun6i_csi_capture *capture = &csi_dev->capture; - bool last_close; mutex_lock(&capture->lock); - last_close = v4l2_fh_is_singular_file(file); - _vb2_fop_release(file, NULL); v4l2_pipeline_pm_put(&capture->video_dev.entity); - /* Power off at last close. */ - if (last_close) - sun6i_csi_set_power(csi_dev, false); - mutex_unlock(&capture->lock); return 0;
Let's just enable the module when we start using it (at stream on) and benefit from runtime pm instead of enabling it at first open. Also reorder the call to v4l2_pipeline_pm_get. Signed-off-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com> --- .../platform/sunxi/sun6i-csi/sun6i_csi.c | 24 ----------- .../platform/sunxi/sun6i-csi/sun6i_csi.h | 7 ---- .../sunxi/sun6i-csi/sun6i_csi_capture.c | 41 ++++++++++--------- 3 files changed, 22 insertions(+), 50 deletions(-)