[PATCH 00/25] Fix some PM runtime issues at the media subsystem

[PATCH 00/25] Fix some PM runtime issues at the media subsystem

[Mauro Carvalho Chehab]

As part of an effort to cleanup pm_runtime*get* calls, I detected a number
of issues at the media subsystem.

Most of the patches here were submitted previously at:

	https://lore.kernel.org/linux-media/cover.1619621413.git.mchehab+huawei@kernel.org/

This series contain just the bug fixes and other related issues that are
present with the current code on media.

It address the points from the existing reviews. I also did my own
set of reviews, in order to avoid regressions.

Changes from v4 of the previous changeset:

- reworked i2c/css RPM get logic;
- dropped two patches that could cause regressions;
- am437x: keep using pm_runtime_get_sync on suspend/resume;
- atmel: fix the returned code and add a print on failures at start streaming;
- simplify some checks for return code > 0;
- mdk-vcodec: properly handle RPM errors at device on logic;
- venus: rework venus_sys_error_handler() logic;
- sti/delta: fix an issue at the error checking logic.

Mauro Carvalho Chehab (25):
  staging: media: rkvdec: fix pm_runtime_get_sync() usage count
  staging: media: imx7-mipi-csis: fix pm_runtime_get_sync() usage count
  media: venus: Rework error fail recover logic
  media: s5p_cec: decrement usage count if disabled
  media: i2c: ccs-core: return the right error code at suspend
  media: i2c: imx334: fix the pm runtime get logic
  media: exynos-gsc: don't resume at remove time
  media: atmel: properly get pm_runtime
  media: hantro: do a PM resume earlier
  media: marvel-ccic: fix some issues when getting pm_runtime
  media: mdk-mdp: fix pm_runtime_get_sync() usage count
  media: rcar_fdp1: simplify error check logic at fdp_open()
  media: rcar_fdp1: fix pm_runtime_get_sync() usage count
  media: renesas-ceu: Properly check for PM errors
  media: s5p: fix pm_runtime_get_sync() usage count
  media: am437x: fix pm_runtime_get_sync() usage count
  media: sh_vou: fix pm_runtime_get_sync() usage count
  media: mtk-vcodec: fix PM runtime get logic
  media: s5p-jpeg: fix pm_runtime_get_sync() usage count
  media: sti/delta: use pm_runtime_resume_and_get()
  media: sunxi: fix pm_runtime_get_sync() usage count
  media: sti/bdisp: fix pm_runtime_get_sync() usage count
  media: exynos4-is: fix pm_runtime_get_sync() usage count
  media: exynos-gsc: fix pm_runtime_get_sync() usage count
  media: i2c: ccs-core: fix pm_runtime_get_sync() usage count

 drivers/media/cec/platform/s5p/s5p_cec.c      |  7 ++-
 drivers/media/i2c/ccs/ccs-core.c              | 41 ++++++++-----
 drivers/media/i2c/imx334.c                    |  7 ++-
 drivers/media/platform/am437x/am437x-vpfe.c   | 15 ++++-
 drivers/media/platform/atmel/atmel-isc-base.c | 30 +++++++---
 drivers/media/platform/atmel/atmel-isi.c      | 19 ++++--
 drivers/media/platform/exynos-gsc/gsc-core.c  | 11 ++--
 drivers/media/platform/exynos-gsc/gsc-m2m.c   |  4 +-
 .../media/platform/exynos4-is/fimc-capture.c  |  6 +-
 drivers/media/platform/exynos4-is/fimc-is.c   |  4 +-
 .../platform/exynos4-is/fimc-isp-video.c      |  3 +-
 drivers/media/platform/exynos4-is/fimc-isp.c  |  7 +--
 drivers/media/platform/exynos4-is/fimc-lite.c |  5 +-
 drivers/media/platform/exynos4-is/fimc-m2m.c  |  5 +-
 drivers/media/platform/exynos4-is/media-dev.c |  9 +--
 drivers/media/platform/exynos4-is/mipi-csis.c | 10 ++--
 .../media/platform/marvell-ccic/mcam-core.c   |  9 ++-
 drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c  |  6 +-
 .../platform/mtk-vcodec/mtk_vcodec_dec_drv.c  |  4 +-
 .../platform/mtk-vcodec/mtk_vcodec_dec_pm.c   |  8 ++-
 .../platform/mtk-vcodec/mtk_vcodec_dec_pm.h   |  2 +-
 drivers/media/platform/qcom/venus/core.c      | 59 +++++++++++++++----
 drivers/media/platform/rcar_fdp1.c            | 28 ++++++---
 drivers/media/platform/renesas-ceu.c          |  4 +-
 drivers/media/platform/s5p-jpeg/jpeg-core.c   |  5 +-
 drivers/media/platform/sh_vou.c               |  6 +-
 drivers/media/platform/sti/bdisp/bdisp-v4l2.c |  7 ++-
 drivers/media/platform/sti/delta/delta-v4l2.c |  8 +--
 .../sunxi/sun8i-rotate/sun8i_rotate.c         |  2 +-
 drivers/staging/media/hantro/hantro_drv.c     |  7 ++-
 drivers/staging/media/imx/imx7-mipi-csis.c    |  7 +--
 drivers/staging/media/rkvdec/rkvdec.c         |  2 +-
 32 files changed, 220 insertions(+), 127 deletions(-)

-- 
2.30.2



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 02/25] staging: media: imx7-mipi-csis: fix pm_runtime_get_sync() usage count

[Mauro Carvalho Chehab]

The pm_runtime_get_sync() internally increments the
dev->power.usage_count without decrementing it, even on errors.
Replace it by the new pm_runtime_resume_and_get(), introduced by:
commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
in order to properly decrement the usage counter, avoiding
a potential PM usage counter leak.

Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/staging/media/imx/imx7-mipi-csis.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
index 025fdc488bd6..1dc680d94a46 100644
--- a/drivers/staging/media/imx/imx7-mipi-csis.c
+++ b/drivers/staging/media/imx/imx7-mipi-csis.c
@@ -695,11 +695,10 @@ static int mipi_csis_s_stream(struct v4l2_subdev *mipi_sd, int enable)
 
 		mipi_csis_clear_counters(state);
 
-		ret = pm_runtime_get_sync(&state->pdev->dev);
-		if (ret < 0) {
-			pm_runtime_put_noidle(&state->pdev->dev);
+		ret = pm_runtime_resume_and_get(&state->pdev->dev);
+		if (ret < 0)
 			return ret;
-		}
+
 		ret = v4l2_subdev_call(state->src_sd, core, s_power, 1);
 		if (ret < 0 && ret != -ENOIOCTLCMD)
 			goto done;
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 07/25] media: exynos-gsc: don't resume at remove time

[Mauro Carvalho Chehab]

Calling pm_runtime_get_sync() at driver's removal time is not
needed, as this will resume PM runtime. Also, the PM runtime
code at pm_runtime_disable() already calls it, if it detects
the need.

So, change the logic in order to disable PM runtime earlier.

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/exynos-gsc/gsc-core.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
index 9f41c2e7097a..f49f3322f835 100644
--- a/drivers/media/platform/exynos-gsc/gsc-core.c
+++ b/drivers/media/platform/exynos-gsc/gsc-core.c
@@ -1210,18 +1210,19 @@ static int gsc_remove(struct platform_device *pdev)
 	struct gsc_dev *gsc = platform_get_drvdata(pdev);
 	int i;
 
-	pm_runtime_get_sync(&pdev->dev);
-
 	gsc_unregister_m2m_device(gsc);
 	v4l2_device_unregister(&gsc->v4l2_dev);
 
 	vb2_dma_contig_clear_max_seg_size(&pdev->dev);
-	for (i = 0; i < gsc->num_clocks; i++)
-		clk_disable_unprepare(gsc->clock[i]);
 
-	pm_runtime_put_noidle(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
+	if (!pm_runtime_status_suspended(&pdev->dev))
+		for (i = 0; i < gsc->num_clocks; i++)
+			clk_disable_unprepare(gsc->clock[i]);
+
+	pm_runtime_set_suspended(&pdev->dev);
+
 	dev_dbg(&pdev->dev, "%s driver unloaded\n", pdev->name);
 	return 0;
 }
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 08/25] media: atmel: properly get pm_runtime

[Mauro Carvalho Chehab]

There are several issues in the way the atmel driver handles
pm_runtime_get_sync():

- it doesn't check return codes;
- it doesn't properly decrement the usage_count on all places;
- it starts streaming even if pm_runtime_get_sync() fails.
- while it tries to get pm_runtime at the clock enable logic,
  it doesn't check if the operation was suceeded.

Replace all occurrences of it to use the new kAPI:
pm_runtime_resume_and_get(), which ensures that, if the
return code is not negative, the usage_count was incremented.

With that, add additional checks when this is called, in order
to ensure that errors will be properly addressed.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/atmel/atmel-isc-base.c | 30 ++++++++++++++-----
 drivers/media/platform/atmel/atmel-isi.c      | 19 +++++++++---
 2 files changed, 38 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
index fe3ec8d0eaee..ce8e1351fa53 100644
--- a/drivers/media/platform/atmel/atmel-isc-base.c
+++ b/drivers/media/platform/atmel/atmel-isc-base.c
@@ -294,9 +294,13 @@ static int isc_wait_clk_stable(struct clk_hw *hw)
 static int isc_clk_prepare(struct clk_hw *hw)
 {
 	struct isc_clk *isc_clk = to_isc_clk(hw);
+	int ret;
 
-	if (isc_clk->id == ISC_ISPCK)
-		pm_runtime_get_sync(isc_clk->dev);
+	if (isc_clk->id == ISC_ISPCK) {
+		ret = pm_runtime_resume_and_get(isc_clk->dev);
+		if (ret < 0)
+			return ret;
+	}
 
 	return isc_wait_clk_stable(hw);
 }
@@ -353,9 +357,13 @@ static int isc_clk_is_enabled(struct clk_hw *hw)
 {
 	struct isc_clk *isc_clk = to_isc_clk(hw);
 	u32 status;
+	int ret;
 
-	if (isc_clk->id == ISC_ISPCK)
-		pm_runtime_get_sync(isc_clk->dev);
+	if (isc_clk->id == ISC_ISPCK) {
+		ret = pm_runtime_resume_and_get(isc_clk->dev);
+		if (ret < 0)
+			return 0;
+	}
 
 	regmap_read(isc_clk->regmap, ISC_CLKSR, &status);
 
@@ -807,7 +815,12 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
 		goto err_start_stream;
 	}
 
-	pm_runtime_get_sync(isc->dev);
+	ret = pm_runtime_resume_and_get(isc->dev);
+	if (ret < 0) {
+		v4l2_err(&isc->v4l2_dev, "RPM resume failed in subdev %d\n",
+			 ret);
+		goto err_pm_get;
+	}
 
 	ret = isc_configure(isc);
 	if (unlikely(ret))
@@ -838,7 +851,7 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
 
 err_configure:
 	pm_runtime_put_sync(isc->dev);
-
+err_pm_get:
 	v4l2_subdev_call(isc->current_subdev->sd, video, s_stream, 0);
 
 err_start_stream:
@@ -1809,6 +1822,7 @@ static void isc_awb_work(struct work_struct *w)
 	u32 baysel;
 	unsigned long flags;
 	u32 min, max;
+	int ret;
 
 	/* streaming is not active anymore */
 	if (isc->stop)
@@ -1831,7 +1845,9 @@ static void isc_awb_work(struct work_struct *w)
 	ctrls->hist_id = hist_id;
 	baysel = isc->config.sd_format->cfa_baycfg << ISC_HIS_CFG_BAYSEL_SHIFT;
 
-	pm_runtime_get_sync(isc->dev);
+	ret = pm_runtime_resume_and_get(isc->dev);
+	if (ret < 0)
+		return;
 
 	/*
 	 * only update if we have all the required histograms and controls
diff --git a/drivers/media/platform/atmel/atmel-isi.c b/drivers/media/platform/atmel/atmel-isi.c
index e392b3efe363..5b1dd358f2e6 100644
--- a/drivers/media/platform/atmel/atmel-isi.c
+++ b/drivers/media/platform/atmel/atmel-isi.c
@@ -422,7 +422,9 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
 	struct frame_buffer *buf, *node;
 	int ret;
 
-	pm_runtime_get_sync(isi->dev);
+	ret = pm_runtime_resume_and_get(isi->dev);
+	if (ret < 0)
+		return ret;
 
 	/* Enable stream on the sub device */
 	ret = v4l2_subdev_call(isi->entity.subdev, video, s_stream, 1);
@@ -782,9 +784,10 @@ static int isi_enum_frameintervals(struct file *file, void *fh,
 	return 0;
 }
 
-static void isi_camera_set_bus_param(struct atmel_isi *isi)
+static int isi_camera_set_bus_param(struct atmel_isi *isi)
 {
 	u32 cfg1 = 0;
+	int ret;
 
 	/* set bus param for ISI */
 	if (isi->pdata.hsync_act_low)
@@ -801,12 +804,16 @@ static void isi_camera_set_bus_param(struct atmel_isi *isi)
 	cfg1 |= ISI_CFG1_THMASK_BEATS_16;
 
 	/* Enable PM and peripheral clock before operate isi registers */
-	pm_runtime_get_sync(isi->dev);
+	ret = pm_runtime_resume_and_get(isi->dev);
+	if (ret < 0)
+		return ret;
 
 	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
 	isi_writel(isi, ISI_CFG1, cfg1);
 
 	pm_runtime_put(isi->dev);
+
+	return 0;
 }
 
 /* -----------------------------------------------------------------------*/
@@ -1085,7 +1092,11 @@ static int isi_graph_notify_complete(struct v4l2_async_notifier *notifier)
 		dev_err(isi->dev, "No supported mediabus format found\n");
 		return ret;
 	}
-	isi_camera_set_bus_param(isi);
+	ret = isi_camera_set_bus_param(isi);
+	if (ret) {
+		dev_err(isi->dev, "Can't wake up device\n");
+		return ret;
+	}
 
 	ret = isi_set_default_fmt(isi);
 	if (ret) {
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 11/25] media: mdk-mdp: fix pm_runtime_get_sync() usage count

[Mauro Carvalho Chehab]

The pm_runtime_get_sync() internally increments the
dev->power.usage_count without decrementing it, even on errors.
Replace it by the new pm_runtime_resume_and_get(), introduced by:
commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
in order to properly decrement the usage counter, avoiding
a potential PM usage counter leak.

While here, fix the return contition of mtk_mdp_m2m_start_streaming(),
as it doesn't make any sense to return 0 if the PM runtime failed
to resume.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
index ace4528cdc5e..f14779e7596e 100644
--- a/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
+++ b/drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c
@@ -391,12 +391,12 @@ static int mtk_mdp_m2m_start_streaming(struct vb2_queue *q, unsigned int count)
 	struct mtk_mdp_ctx *ctx = q->drv_priv;
 	int ret;
 
-	ret = pm_runtime_get_sync(&ctx->mdp_dev->pdev->dev);
+	ret = pm_runtime_resume_and_get(&ctx->mdp_dev->pdev->dev);
 	if (ret < 0)
-		mtk_mdp_dbg(1, "[%d] pm_runtime_get_sync failed:%d",
+		mtk_mdp_dbg(1, "[%d] pm_runtime_resume_and_get failed:%d",
 			    ctx->id, ret);
 
-	return 0;
+	return ret;
 }
 
 static void *mtk_mdp_m2m_buf_remove(struct mtk_mdp_ctx *ctx,
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 18/25] media: mtk-vcodec: fix PM runtime get logic

[Mauro Carvalho Chehab]

Currently, the driver just assumes that PM runtime logic
succeded resuming the device.

That may not be the case, as pm_runtime_get_sync()
can fail (but keeping the usage count incremented).

Replace the code to use pm_runtime_resume_and_get(),
and letting it return the error code.

This way, if mtk_vcodec_dec_pw_on() fails, the logic
under fops_vcodec_open() will do the right thing and
return an error, instead of just assuming that the
device is ready to be used.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 4 +++-
 drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c  | 8 +++++---
 drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h  | 2 +-
 3 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
index 147dfef1638d..f87dc47d9e63 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
@@ -126,7 +126,9 @@ static int fops_vcodec_open(struct file *file)
 	mtk_vcodec_dec_set_default_params(ctx);
 
 	if (v4l2_fh_is_singular(&ctx->fh)) {
-		mtk_vcodec_dec_pw_on(&dev->pm);
+		ret = mtk_vcodec_dec_pw_on(&dev->pm);
+		if (ret < 0)
+			goto err_load_fw;
 		/*
 		 * Does nothing if firmware was already loaded.
 		 */
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
index ddee7046ce42..6038db96f71c 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
@@ -88,13 +88,15 @@ void mtk_vcodec_release_dec_pm(struct mtk_vcodec_dev *dev)
 	put_device(dev->pm.larbvdec);
 }
 
-void mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm)
+int mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm)
 {
 	int ret;
 
-	ret = pm_runtime_get_sync(pm->dev);
+	ret = pm_runtime_resume_and_get(pm->dev);
 	if (ret)
-		mtk_v4l2_err("pm_runtime_get_sync fail %d", ret);
+		mtk_v4l2_err("pm_runtime_resume_and_get fail %d", ret);
+
+	return ret;
 }
 
 void mtk_vcodec_dec_pw_off(struct mtk_vcodec_pm *pm)
diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h
index 872d8bf8cfaf..280aeaefdb65 100644
--- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h
+++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h
@@ -12,7 +12,7 @@
 int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *dev);
 void mtk_vcodec_release_dec_pm(struct mtk_vcodec_dev *dev);
 
-void mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm);
+int mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm);
 void mtk_vcodec_dec_pw_off(struct mtk_vcodec_pm *pm);
 void mtk_vcodec_dec_clock_on(struct mtk_vcodec_pm *pm);
 void mtk_vcodec_dec_clock_off(struct mtk_vcodec_pm *pm);
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 19/25] media: s5p-jpeg: fix pm_runtime_get_sync() usage count

[Mauro Carvalho Chehab]

The pm_runtime_get_sync() internally increments the
dev->power.usage_count without decrementing it, even on errors.
Replace it by the new pm_runtime_resume_and_get(), introduced by:
commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
in order to properly decrement the usage counter, avoiding
a potential PM usage counter leak.

As a plus, pm_runtime_resume_and_get() doesn't return
positive numbers, so the return code validation can
be removed.

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Acked-by: Andrzej Pietrasiewicz <andrzejtp2010@gmail.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/s5p-jpeg/jpeg-core.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index 026111505f5a..d402e456f27d 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -2566,11 +2566,8 @@ static void s5p_jpeg_buf_queue(struct vb2_buffer *vb)
 static int s5p_jpeg_start_streaming(struct vb2_queue *q, unsigned int count)
 {
 	struct s5p_jpeg_ctx *ctx = vb2_get_drv_priv(q);
-	int ret;
 
-	ret = pm_runtime_get_sync(ctx->jpeg->dev);
-
-	return ret > 0 ? 0 : ret;
+	return pm_runtime_resume_and_get(ctx->jpeg->dev);
 }
 
 static void s5p_jpeg_stop_streaming(struct vb2_queue *q)
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 21/25] media: sunxi: fix pm_runtime_get_sync() usage count

[Mauro Carvalho Chehab]

The pm_runtime_get_sync() internally increments the
dev->power.usage_count without decrementing it, even on errors.
Replace it by the new pm_runtime_resume_and_get(), introduced by:
commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
in order to properly decrement the usage counter, avoiding
a potential PM usage counter leak.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/sunxi/sun8i-rotate/sun8i_rotate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/sunxi/sun8i-rotate/sun8i_rotate.c b/drivers/media/platform/sunxi/sun8i-rotate/sun8i_rotate.c
index 3f81dd17755c..fbcca59a0517 100644
--- a/drivers/media/platform/sunxi/sun8i-rotate/sun8i_rotate.c
+++ b/drivers/media/platform/sunxi/sun8i-rotate/sun8i_rotate.c
@@ -494,7 +494,7 @@ static int rotate_start_streaming(struct vb2_queue *vq, unsigned int count)
 		struct device *dev = ctx->dev->dev;
 		int ret;
 
-		ret = pm_runtime_get_sync(dev);
+		ret = pm_runtime_resume_and_get(dev);
 		if (ret < 0) {
 			dev_err(dev, "Failed to enable module\n");
 
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 23/25] media: exynos4-is: fix pm_runtime_get_sync() usage count

[Mauro Carvalho Chehab]

The pm_runtime_get_sync() internally increments the
dev->power.usage_count without decrementing it, even on errors.

On some places, this is ok, but on others the usage count
ended being unbalanced on failures.

Replace it by the new pm_runtime_resume_and_get(), introduced by:
commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
in order to properly decrement the usage counter, avoiding
a potential PM usage counter leak.

As a bonus, such function always return zero on success. So,
some code can be simplified.

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/exynos4-is/fimc-capture.c   |  6 ++----
 drivers/media/platform/exynos4-is/fimc-is.c        |  4 ++--
 drivers/media/platform/exynos4-is/fimc-isp-video.c |  3 +--
 drivers/media/platform/exynos4-is/fimc-isp.c       |  7 +++----
 drivers/media/platform/exynos4-is/fimc-lite.c      |  5 +++--
 drivers/media/platform/exynos4-is/fimc-m2m.c       |  5 +----
 drivers/media/platform/exynos4-is/media-dev.c      |  9 +++------
 drivers/media/platform/exynos4-is/mipi-csis.c      | 10 ++++------
 8 files changed, 19 insertions(+), 30 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/fimc-capture.c b/drivers/media/platform/exynos4-is/fimc-capture.c
index 13c838d3f947..0da36443173c 100644
--- a/drivers/media/platform/exynos4-is/fimc-capture.c
+++ b/drivers/media/platform/exynos4-is/fimc-capture.c
@@ -478,11 +478,9 @@ static int fimc_capture_open(struct file *file)
 		goto unlock;
 
 	set_bit(ST_CAPT_BUSY, &fimc->state);
-	ret = pm_runtime_get_sync(&fimc->pdev->dev);
-	if (ret < 0) {
-		pm_runtime_put_sync(&fimc->pdev->dev);
+	ret = pm_runtime_resume_and_get(&fimc->pdev->dev);
+	if (ret < 0)
 		goto unlock;
-	}
 
 	ret = v4l2_fh_open(file);
 	if (ret) {
diff --git a/drivers/media/platform/exynos4-is/fimc-is.c b/drivers/media/platform/exynos4-is/fimc-is.c
index 972d9601d236..1b24f5bfc4af 100644
--- a/drivers/media/platform/exynos4-is/fimc-is.c
+++ b/drivers/media/platform/exynos4-is/fimc-is.c
@@ -828,9 +828,9 @@ static int fimc_is_probe(struct platform_device *pdev)
 			goto err_irq;
 	}
 
-	ret = pm_runtime_get_sync(dev);
+	ret = pm_runtime_resume_and_get(dev);
 	if (ret < 0)
-		goto err_pm;
+		goto err_irq;
 
 	vb2_dma_contig_set_max_seg_size(dev, DMA_BIT_MASK(32));
 
diff --git a/drivers/media/platform/exynos4-is/fimc-isp-video.c b/drivers/media/platform/exynos4-is/fimc-isp-video.c
index 612b9872afc8..8d9dc597deaa 100644
--- a/drivers/media/platform/exynos4-is/fimc-isp-video.c
+++ b/drivers/media/platform/exynos4-is/fimc-isp-video.c
@@ -275,7 +275,7 @@ static int isp_video_open(struct file *file)
 	if (ret < 0)
 		goto unlock;
 
-	ret = pm_runtime_get_sync(&isp->pdev->dev);
+	ret = pm_runtime_resume_and_get(&isp->pdev->dev);
 	if (ret < 0)
 		goto rel_fh;
 
@@ -293,7 +293,6 @@ static int isp_video_open(struct file *file)
 	if (!ret)
 		goto unlock;
 rel_fh:
-	pm_runtime_put_noidle(&isp->pdev->dev);
 	v4l2_fh_release(file);
 unlock:
 	mutex_unlock(&isp->video_lock);
diff --git a/drivers/media/platform/exynos4-is/fimc-isp.c b/drivers/media/platform/exynos4-is/fimc-isp.c
index a77c49b18511..74b49d30901e 100644
--- a/drivers/media/platform/exynos4-is/fimc-isp.c
+++ b/drivers/media/platform/exynos4-is/fimc-isp.c
@@ -304,11 +304,10 @@ static int fimc_isp_subdev_s_power(struct v4l2_subdev *sd, int on)
 	pr_debug("on: %d\n", on);
 
 	if (on) {
-		ret = pm_runtime_get_sync(&is->pdev->dev);
-		if (ret < 0) {
-			pm_runtime_put(&is->pdev->dev);
+		ret = pm_runtime_resume_and_get(&is->pdev->dev);
+		if (ret < 0)
 			return ret;
-		}
+
 		set_bit(IS_ST_PWR_ON, &is->state);
 
 		ret = fimc_is_start_firmware(is);
diff --git a/drivers/media/platform/exynos4-is/fimc-lite.c b/drivers/media/platform/exynos4-is/fimc-lite.c
index fe20af3a7178..4d8b18078ff3 100644
--- a/drivers/media/platform/exynos4-is/fimc-lite.c
+++ b/drivers/media/platform/exynos4-is/fimc-lite.c
@@ -469,9 +469,9 @@ static int fimc_lite_open(struct file *file)
 	}
 
 	set_bit(ST_FLITE_IN_USE, &fimc->state);
-	ret = pm_runtime_get_sync(&fimc->pdev->dev);
+	ret = pm_runtime_resume_and_get(&fimc->pdev->dev);
 	if (ret < 0)
-		goto err_pm;
+		goto err_in_use;
 
 	ret = v4l2_fh_open(file);
 	if (ret < 0)
@@ -499,6 +499,7 @@ static int fimc_lite_open(struct file *file)
 	v4l2_fh_release(file);
 err_pm:
 	pm_runtime_put_sync(&fimc->pdev->dev);
+err_in_use:
 	clear_bit(ST_FLITE_IN_USE, &fimc->state);
 unlock:
 	mutex_unlock(&fimc->lock);
diff --git a/drivers/media/platform/exynos4-is/fimc-m2m.c b/drivers/media/platform/exynos4-is/fimc-m2m.c
index c9704a147e5c..df8e2aa454d8 100644
--- a/drivers/media/platform/exynos4-is/fimc-m2m.c
+++ b/drivers/media/platform/exynos4-is/fimc-m2m.c
@@ -73,17 +73,14 @@ static void fimc_m2m_shutdown(struct fimc_ctx *ctx)
 static int start_streaming(struct vb2_queue *q, unsigned int count)
 {
 	struct fimc_ctx *ctx = q->drv_priv;
-	int ret;
 
-	ret = pm_runtime_get_sync(&ctx->fimc_dev->pdev->dev);
-	return ret > 0 ? 0 : ret;
+	return pm_runtime_resume_and_get(&ctx->fimc_dev->pdev->dev);
 }
 
 static void stop_streaming(struct vb2_queue *q)
 {
 	struct fimc_ctx *ctx = q->drv_priv;
 
-
 	fimc_m2m_shutdown(ctx);
 	fimc_m2m_job_finish(ctx, VB2_BUF_STATE_ERROR);
 	pm_runtime_put(&ctx->fimc_dev->pdev->dev);
diff --git a/drivers/media/platform/exynos4-is/media-dev.c b/drivers/media/platform/exynos4-is/media-dev.c
index 13d192ba4aa6..e025178db06c 100644
--- a/drivers/media/platform/exynos4-is/media-dev.c
+++ b/drivers/media/platform/exynos4-is/media-dev.c
@@ -512,11 +512,9 @@ static int fimc_md_register_sensor_entities(struct fimc_md *fmd)
 	if (!fmd->pmf)
 		return -ENXIO;
 
-	ret = pm_runtime_get_sync(fmd->pmf);
-	if (ret < 0) {
-		pm_runtime_put(fmd->pmf);
+	ret = pm_runtime_resume_and_get(fmd->pmf);
+	if (ret < 0)
 		return ret;
-	}
 
 	fmd->num_sensors = 0;
 
@@ -1291,8 +1289,7 @@ static int cam_clk_prepare(struct clk_hw *hw)
 	if (camclk->fmd->pmf == NULL)
 		return -ENODEV;
 
-	ret = pm_runtime_get_sync(camclk->fmd->pmf);
-	return ret < 0 ? ret : 0;
+	return pm_runtime_resume_and_get(camclk->fmd->pmf);
 }
 
 static void cam_clk_unprepare(struct clk_hw *hw)
diff --git a/drivers/media/platform/exynos4-is/mipi-csis.c b/drivers/media/platform/exynos4-is/mipi-csis.c
index 1aac167abb17..ebf39c856894 100644
--- a/drivers/media/platform/exynos4-is/mipi-csis.c
+++ b/drivers/media/platform/exynos4-is/mipi-csis.c
@@ -494,7 +494,7 @@ static int s5pcsis_s_power(struct v4l2_subdev *sd, int on)
 	struct device *dev = &state->pdev->dev;
 
 	if (on)
-		return pm_runtime_get_sync(dev);
+		return pm_runtime_resume_and_get(dev);
 
 	return pm_runtime_put_sync(dev);
 }
@@ -509,11 +509,9 @@ static int s5pcsis_s_stream(struct v4l2_subdev *sd, int enable)
 
 	if (enable) {
 		s5pcsis_clear_counters(state);
-		ret = pm_runtime_get_sync(&state->pdev->dev);
-		if (ret && ret != 1) {
-			pm_runtime_put_noidle(&state->pdev->dev);
+		ret = pm_runtime_resume_and_get(&state->pdev->dev);
+		if (ret < 0)
 			return ret;
-		}
 	}
 
 	mutex_lock(&state->lock);
@@ -535,7 +533,7 @@ static int s5pcsis_s_stream(struct v4l2_subdev *sd, int enable)
 	if (!enable)
 		pm_runtime_put(&state->pdev->dev);
 
-	return ret == 1 ? 0 : ret;
+	return ret;
 }
 
 static int s5pcsis_enum_mbus_code(struct v4l2_subdev *sd,
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 24/25] media: exynos-gsc: fix pm_runtime_get_sync() usage count

[Mauro Carvalho Chehab]

The pm_runtime_get_sync() internally increments the
dev->power.usage_count without decrementing it, even on errors.
Replace it by the new pm_runtime_resume_and_get(), introduced by:
commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
in order to properly decrement the usage counter, avoiding
a potential PM usage counter leak.

As a bonus, as pm_runtime_get_sync() always return 0 on
success, the logic can be simplified.

Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/exynos-gsc/gsc-m2m.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/media/platform/exynos-gsc/gsc-m2m.c b/drivers/media/platform/exynos-gsc/gsc-m2m.c
index 27a3c92c73bc..f1cf847d1cc2 100644
--- a/drivers/media/platform/exynos-gsc/gsc-m2m.c
+++ b/drivers/media/platform/exynos-gsc/gsc-m2m.c
@@ -56,10 +56,8 @@ static void __gsc_m2m_job_abort(struct gsc_ctx *ctx)
 static int gsc_m2m_start_streaming(struct vb2_queue *q, unsigned int count)
 {
 	struct gsc_ctx *ctx = q->drv_priv;
-	int ret;
 
-	ret = pm_runtime_get_sync(&ctx->gsc_dev->pdev->dev);
-	return ret > 0 ? 0 : ret;
+	return pm_runtime_resume_and_get(&ctx->gsc_dev->pdev->dev);
 }
 
 static void __gsc_m2m_cleanup_queue(struct gsc_ctx *ctx)
-- 
2.30.2


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 02/25] staging: media: imx7-mipi-csis: fix pm_runtime_get_sync() usage count

[Jonathan Cameron]

On Wed, 5 May 2021 11:41:52 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> The pm_runtime_get_sync() internally increments the
> dev->power.usage_count without decrementing it, even on errors.
> Replace it by the new pm_runtime_resume_and_get(), introduced by:
> commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
> in order to properly decrement the usage counter, avoiding
> a potential PM usage counter leak.
> 
> Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Not a fix as far as I can see, just a cleanup - so perhaps not this set?

Jonathan


> ---
>  drivers/staging/media/imx/imx7-mipi-csis.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> index 025fdc488bd6..1dc680d94a46 100644
> --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> @@ -695,11 +695,10 @@ static int mipi_csis_s_stream(struct v4l2_subdev *mipi_sd, int enable)
>  
>  		mipi_csis_clear_counters(state);
>  
> -		ret = pm_runtime_get_sync(&state->pdev->dev);
> -		if (ret < 0) {
> -			pm_runtime_put_noidle(&state->pdev->dev);
> +		ret = pm_runtime_resume_and_get(&state->pdev->dev);
> +		if (ret < 0)
>  			return ret;
> -		}
> +
>  		ret = v4l2_subdev_call(state->src_sd, core, s_power, 1);
>  		if (ret < 0 && ret != -ENOIOCTLCMD)
>  			goto done;


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 02/25] staging: media: imx7-mipi-csis: fix pm_runtime_get_sync() usage count

[Mauro Carvalho Chehab]

Em Wed, 5 May 2021 12:06:52 +0100
Jonathan Cameron <Jonathan.Cameron@Huawei.com> escreveu:

> On Wed, 5 May 2021 11:41:52 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> 
> > The pm_runtime_get_sync() internally increments the
> > dev->power.usage_count without decrementing it, even on errors.
> > Replace it by the new pm_runtime_resume_and_get(), introduced by:
> > commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
> > in order to properly decrement the usage counter, avoiding
> > a potential PM usage counter leak.
> > 
> > Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>  
> 
> Not a fix as far as I can see, just a cleanup - so perhaps not this set?

Yeah, will move it to the non-error patch pile and change the comments.

> 
> Jonathan
> 
> 
> > ---
> >  drivers/staging/media/imx/imx7-mipi-csis.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> > index 025fdc488bd6..1dc680d94a46 100644
> > --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> > +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> > @@ -695,11 +695,10 @@ static int mipi_csis_s_stream(struct v4l2_subdev *mipi_sd, int enable)
> >  
> >  		mipi_csis_clear_counters(state);
> >  
> > -		ret = pm_runtime_get_sync(&state->pdev->dev);
> > -		if (ret < 0) {
> > -			pm_runtime_put_noidle(&state->pdev->dev);
> > +		ret = pm_runtime_resume_and_get(&state->pdev->dev);
> > +		if (ret < 0)
> >  			return ret;
> > -		}
> > +
> >  		ret = v4l2_subdev_call(state->src_sd, core, s_power, 1);
> >  		if (ret < 0 && ret != -ENOIOCTLCMD)
> >  			goto done;  
> 



Thanks,
Mauro

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 08/25] media: atmel: properly get pm_runtime

[Jonathan Cameron]

On Wed, 5 May 2021 11:41:58 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> There are several issues in the way the atmel driver handles
> pm_runtime_get_sync():
> 
> - it doesn't check return codes;
> - it doesn't properly decrement the usage_count on all places;
> - it starts streaming even if pm_runtime_get_sync() fails.
> - while it tries to get pm_runtime at the clock enable logic,
>   it doesn't check if the operation was suceeded.
> 
> Replace all occurrences of it to use the new kAPI:
> pm_runtime_resume_and_get(), which ensures that, if the
> return code is not negative, the usage_count was incremented.
> 
> With that, add additional checks when this is called, in order
> to ensure that errors will be properly addressed.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Hi Mauro, I don't know media enough to know what is the right answer
but in some of this series, a failure in
pm_runtime_resume_and_get() leads to a bunch of buffer cleanup
(patch 22 being an example) and in others return happens without doing
that cleanup.

It might be both are safe, or I'm missing something else, but I'm
certainly not confident enough to give any tags on this one as a result
of that mismatch.

> ---
>  drivers/media/platform/atmel/atmel-isc-base.c | 30 ++++++++++++++-----
>  drivers/media/platform/atmel/atmel-isi.c      | 19 +++++++++---
>  2 files changed, 38 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
> index fe3ec8d0eaee..ce8e1351fa53 100644
> --- a/drivers/media/platform/atmel/atmel-isc-base.c
> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
> @@ -294,9 +294,13 @@ static int isc_wait_clk_stable(struct clk_hw *hw)
>  static int isc_clk_prepare(struct clk_hw *hw)
>  {
>  	struct isc_clk *isc_clk = to_isc_clk(hw);
> +	int ret;
>  
> -	if (isc_clk->id == ISC_ISPCK)
> -		pm_runtime_get_sync(isc_clk->dev);
> +	if (isc_clk->id == ISC_ISPCK) {
> +		ret = pm_runtime_resume_and_get(isc_clk->dev);
> +		if (ret < 0)
> +			return ret;
> +	}
>  
>  	return isc_wait_clk_stable(hw);
>  }
> @@ -353,9 +357,13 @@ static int isc_clk_is_enabled(struct clk_hw *hw)
>  {
>  	struct isc_clk *isc_clk = to_isc_clk(hw);
>  	u32 status;
> +	int ret;
>  
> -	if (isc_clk->id == ISC_ISPCK)
> -		pm_runtime_get_sync(isc_clk->dev);
> +	if (isc_clk->id == ISC_ISPCK) {
> +		ret = pm_runtime_resume_and_get(isc_clk->dev);
> +		if (ret < 0)
> +			return 0;
> +	}
>  
>  	regmap_read(isc_clk->regmap, ISC_CLKSR, &status);
>  
> @@ -807,7 +815,12 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
>  		goto err_start_stream;
>  	}
>  
> -	pm_runtime_get_sync(isc->dev);
> +	ret = pm_runtime_resume_and_get(isc->dev);
> +	if (ret < 0) {
> +		v4l2_err(&isc->v4l2_dev, "RPM resume failed in subdev %d\n",
> +			 ret);
> +		goto err_pm_get;
> +	}
>  
>  	ret = isc_configure(isc);
>  	if (unlikely(ret))
> @@ -838,7 +851,7 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
>  
>  err_configure:
>  	pm_runtime_put_sync(isc->dev);
> -
> +err_pm_get:
>  	v4l2_subdev_call(isc->current_subdev->sd, video, s_stream, 0);
>  
>  err_start_stream:
> @@ -1809,6 +1822,7 @@ static void isc_awb_work(struct work_struct *w)
>  	u32 baysel;
>  	unsigned long flags;
>  	u32 min, max;
> +	int ret;
>  
>  	/* streaming is not active anymore */
>  	if (isc->stop)
> @@ -1831,7 +1845,9 @@ static void isc_awb_work(struct work_struct *w)
>  	ctrls->hist_id = hist_id;
>  	baysel = isc->config.sd_format->cfa_baycfg << ISC_HIS_CFG_BAYSEL_SHIFT;
>  
> -	pm_runtime_get_sync(isc->dev);
> +	ret = pm_runtime_resume_and_get(isc->dev);
> +	if (ret < 0)
> +		return;
>  
>  	/*
>  	 * only update if we have all the required histograms and controls
> diff --git a/drivers/media/platform/atmel/atmel-isi.c b/drivers/media/platform/atmel/atmel-isi.c
> index e392b3efe363..5b1dd358f2e6 100644
> --- a/drivers/media/platform/atmel/atmel-isi.c
> +++ b/drivers/media/platform/atmel/atmel-isi.c
> @@ -422,7 +422,9 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>  	struct frame_buffer *buf, *node;
>  	int ret;
>  
> -	pm_runtime_get_sync(isi->dev);
> +	ret = pm_runtime_resume_and_get(isi->dev);
> +	if (ret < 0)
> +		return ret;
This is the case I'm referring to above.

>  
>  	/* Enable stream on the sub device */
>  	ret = v4l2_subdev_call(isi->entity.subdev, video, s_stream, 1);
> @@ -782,9 +784,10 @@ static int isi_enum_frameintervals(struct file *file, void *fh,
>  	return 0;
>  }
>  
> -static void isi_camera_set_bus_param(struct atmel_isi *isi)
> +static int isi_camera_set_bus_param(struct atmel_isi *isi)
>  {
>  	u32 cfg1 = 0;
> +	int ret;
>  
>  	/* set bus param for ISI */
>  	if (isi->pdata.hsync_act_low)
> @@ -801,12 +804,16 @@ static void isi_camera_set_bus_param(struct atmel_isi *isi)
>  	cfg1 |= ISI_CFG1_THMASK_BEATS_16;
>  
>  	/* Enable PM and peripheral clock before operate isi registers */
> -	pm_runtime_get_sync(isi->dev);
> +	ret = pm_runtime_resume_and_get(isi->dev);
> +	if (ret < 0)
> +		return ret;
>  
>  	isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
>  	isi_writel(isi, ISI_CFG1, cfg1);
>  
>  	pm_runtime_put(isi->dev);
> +
> +	return 0;
>  }
>  
>  /* -----------------------------------------------------------------------*/
> @@ -1085,7 +1092,11 @@ static int isi_graph_notify_complete(struct v4l2_async_notifier *notifier)
>  		dev_err(isi->dev, "No supported mediabus format found\n");
>  		return ret;
>  	}
> -	isi_camera_set_bus_param(isi);
> +	ret = isi_camera_set_bus_param(isi);
> +	if (ret) {
> +		dev_err(isi->dev, "Can't wake up device\n");
> +		return ret;
> +	}
>  
>  	ret = isi_set_default_fmt(isi);
>  	if (ret) {


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 23/25] media: exynos4-is: fix pm_runtime_get_sync() usage count

[Jonathan Cameron]

On Wed, 5 May 2021 11:42:13 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> The pm_runtime_get_sync() internally increments the
> dev->power.usage_count without decrementing it, even on errors.
> 
> On some places, this is ok, but on others the usage count
> ended being unbalanced on failures.
> 
> Replace it by the new pm_runtime_resume_and_get(), introduced by:
> commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
> in order to properly decrement the usage counter, avoiding
> a potential PM usage counter leak.
> 
> As a bonus, such function always return zero on success. So,
> some code can be simplified.
> 
> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>

Might be nice to call out the two odd cases below.

> diff --git a/drivers/media/platform/exynos4-is/mipi-csis.c b/drivers/media/platform/exynos4-is/mipi-csis.c
> index 1aac167abb17..ebf39c856894 100644
> --- a/drivers/media/platform/exynos4-is/mipi-csis.c
> +++ b/drivers/media/platform/exynos4-is/mipi-csis.c
> @@ -494,7 +494,7 @@ static int s5pcsis_s_power(struct v4l2_subdev *sd, int on)
>  	struct device *dev = &state->pdev->dev;
>  
>  	if (on)
> -		return pm_runtime_get_sync(dev);
> +		return pm_runtime_resume_and_get(dev);
>  
>  	return pm_runtime_put_sync(dev);
>  }
> @@ -509,11 +509,9 @@ static int s5pcsis_s_stream(struct v4l2_subdev *sd, int enable)
>  
>  	if (enable) {
>  		s5pcsis_clear_counters(state);
> -		ret = pm_runtime_get_sync(&state->pdev->dev);
> -		if (ret && ret != 1) {
Perhaps add something to the description on this less common case?

> -			pm_runtime_put_noidle(&state->pdev->dev);
> +		ret = pm_runtime_resume_and_get(&state->pdev->dev);
> +		if (ret < 0)
>  			return ret;
> -		}
>  	}
>  
>  	mutex_lock(&state->lock);
> @@ -535,7 +533,7 @@ static int s5pcsis_s_stream(struct v4l2_subdev *sd, int enable)
>  	if (!enable)
>  		pm_runtime_put(&state->pdev->dev);
>  
> -	return ret == 1 ? 0 : ret;
> +	return ret;
>  }
>  
>  static int s5pcsis_enum_mbus_code(struct v4l2_subdev *sd,


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 07/25] media: exynos-gsc: don't resume at remove time

[Jonathan Cameron]

On Wed, 5 May 2021 11:41:57 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Calling pm_runtime_get_sync() at driver's removal time is not
> needed, as this will resume PM runtime. Also, the PM runtime
> code at pm_runtime_disable() already calls it, if it detects
> the need.
> 
> So, change the logic in order to disable PM runtime earlier.
> 
> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Seems sensible and a lot more along the lines of what I'd expect
to see than was originally here.

Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/media/platform/exynos-gsc/gsc-core.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/exynos-gsc/gsc-core.c b/drivers/media/platform/exynos-gsc/gsc-core.c
> index 9f41c2e7097a..f49f3322f835 100644
> --- a/drivers/media/platform/exynos-gsc/gsc-core.c
> +++ b/drivers/media/platform/exynos-gsc/gsc-core.c
> @@ -1210,18 +1210,19 @@ static int gsc_remove(struct platform_device *pdev)
>  	struct gsc_dev *gsc = platform_get_drvdata(pdev);
>  	int i;
>  
> -	pm_runtime_get_sync(&pdev->dev);
> -
>  	gsc_unregister_m2m_device(gsc);
>  	v4l2_device_unregister(&gsc->v4l2_dev);
>  
>  	vb2_dma_contig_clear_max_seg_size(&pdev->dev);
> -	for (i = 0; i < gsc->num_clocks; i++)
> -		clk_disable_unprepare(gsc->clock[i]);
>  
> -	pm_runtime_put_noidle(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
>  
> +	if (!pm_runtime_status_suspended(&pdev->dev))
> +		for (i = 0; i < gsc->num_clocks; i++)
> +			clk_disable_unprepare(gsc->clock[i]);
> +
> +	pm_runtime_set_suspended(&pdev->dev);
> +
>  	dev_dbg(&pdev->dev, "%s driver unloaded\n", pdev->name);
>  	return 0;
>  }


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 18/25] media: mtk-vcodec: fix PM runtime get logic

[Jonathan Cameron]

On Wed,  5 May 2021 11:42:08 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> Currently, the driver just assumes that PM runtime logic
> succeded resuming the device.
> 
> That may not be the case, as pm_runtime_get_sync()
> can fail (but keeping the usage count incremented).
> 
> Replace the code to use pm_runtime_resume_and_get(),
> and letting it return the error code.
> 
> This way, if mtk_vcodec_dec_pw_on() fails, the logic
> under fops_vcodec_open() will do the right thing and
> return an error, instead of just assuming that the
> device is ready to be used.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c | 4 +++-
>  drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c  | 8 +++++---
>  drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h  | 2 +-
>  3 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> index 147dfef1638d..f87dc47d9e63 100644
> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_drv.c
> @@ -126,7 +126,9 @@ static int fops_vcodec_open(struct file *file)
>  	mtk_vcodec_dec_set_default_params(ctx);
>  
>  	if (v4l2_fh_is_singular(&ctx->fh)) {
> -		mtk_vcodec_dec_pw_on(&dev->pm);
> +		ret = mtk_vcodec_dec_pw_on(&dev->pm);
> +		if (ret < 0)
> +			goto err_load_fw;
>  		/*
>  		 * Does nothing if firmware was already loaded.
>  		 */
> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> index ddee7046ce42..6038db96f71c 100644
> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.c
> @@ -88,13 +88,15 @@ void mtk_vcodec_release_dec_pm(struct mtk_vcodec_dev *dev)
>  	put_device(dev->pm.larbvdec);
>  }
>  
> -void mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm)
> +int mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm)
>  {
>  	int ret;
>  
> -	ret = pm_runtime_get_sync(pm->dev);
> +	ret = pm_runtime_resume_and_get(pm->dev);
>  	if (ret)
> -		mtk_v4l2_err("pm_runtime_get_sync fail %d", ret);
> +		mtk_v4l2_err("pm_runtime_resume_and_get fail %d", ret);
> +
> +	return ret;
>  }
>  
>  void mtk_vcodec_dec_pw_off(struct mtk_vcodec_pm *pm)
> diff --git a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h
> index 872d8bf8cfaf..280aeaefdb65 100644
> --- a/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h
> +++ b/drivers/media/platform/mtk-vcodec/mtk_vcodec_dec_pm.h
> @@ -12,7 +12,7 @@
>  int mtk_vcodec_init_dec_pm(struct mtk_vcodec_dev *dev);
>  void mtk_vcodec_release_dec_pm(struct mtk_vcodec_dev *dev);
>  
> -void mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm);
> +int mtk_vcodec_dec_pw_on(struct mtk_vcodec_pm *pm);
>  void mtk_vcodec_dec_pw_off(struct mtk_vcodec_pm *pm);
>  void mtk_vcodec_dec_clock_on(struct mtk_vcodec_pm *pm);
>  void mtk_vcodec_dec_clock_off(struct mtk_vcodec_pm *pm);


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 19/25] media: s5p-jpeg: fix pm_runtime_get_sync() usage count

[Jonathan Cameron]

On Wed, 5 May 2021 11:42:09 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> The pm_runtime_get_sync() internally increments the
> dev->power.usage_count without decrementing it, even on errors.
> Replace it by the new pm_runtime_resume_and_get(), introduced by:
> commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
> in order to properly decrement the usage counter, avoiding
> a potential PM usage counter leak.
> 
> As a plus, pm_runtime_resume_and_get() doesn't return
> positive numbers, so the return code validation can
> be removed.
> 
> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Acked-by: Andrzej Pietrasiewicz <andrzejtp2010@gmail.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/media/platform/s5p-jpeg/jpeg-core.c | 5 +----
>  1 file changed, 1 insertion(+), 4 deletions(-)
> 
> diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> index 026111505f5a..d402e456f27d 100644
> --- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
> +++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
> @@ -2566,11 +2566,8 @@ static void s5p_jpeg_buf_queue(struct vb2_buffer *vb)
>  static int s5p_jpeg_start_streaming(struct vb2_queue *q, unsigned int count)
>  {
>  	struct s5p_jpeg_ctx *ctx = vb2_get_drv_priv(q);
> -	int ret;
>  
> -	ret = pm_runtime_get_sync(ctx->jpeg->dev);
> -
> -	return ret > 0 ? 0 : ret;
> +	return pm_runtime_resume_and_get(ctx->jpeg->dev);
>  }
>  
>  static void s5p_jpeg_stop_streaming(struct vb2_queue *q)


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 24/25] media: exynos-gsc: fix pm_runtime_get_sync() usage count

[Jonathan Cameron]

On Wed, 5 May 2021 11:42:14 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:

> The pm_runtime_get_sync() internally increments the
> dev->power.usage_count without decrementing it, even on errors.
> Replace it by the new pm_runtime_resume_and_get(), introduced by:
> commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
> in order to properly decrement the usage counter, avoiding
> a potential PM usage counter leak.
> 
> As a bonus, as pm_runtime_get_sync() always return 0 on
> success, the logic can be simplified.
> 
> Reviewed-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
>  drivers/media/platform/exynos-gsc/gsc-m2m.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/media/platform/exynos-gsc/gsc-m2m.c b/drivers/media/platform/exynos-gsc/gsc-m2m.c
> index 27a3c92c73bc..f1cf847d1cc2 100644
> --- a/drivers/media/platform/exynos-gsc/gsc-m2m.c
> +++ b/drivers/media/platform/exynos-gsc/gsc-m2m.c
> @@ -56,10 +56,8 @@ static void __gsc_m2m_job_abort(struct gsc_ctx *ctx)
>  static int gsc_m2m_start_streaming(struct vb2_queue *q, unsigned int count)
>  {
>  	struct gsc_ctx *ctx = q->drv_priv;
> -	int ret;
>  
> -	ret = pm_runtime_get_sync(&ctx->gsc_dev->pdev->dev);
> -	return ret > 0 ? 0 : ret;
> +	return pm_runtime_resume_and_get(&ctx->gsc_dev->pdev->dev);
>  }
>  
>  static void __gsc_m2m_cleanup_queue(struct gsc_ctx *ctx)


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 02/25] staging: media: imx7-mipi-csis: fix pm_runtime_get_sync() usage count

[Rui Miguel Silva]

Hi,
On Wed May 5, 2021 at 12:06 PM WEST, Jonathan Cameron wrote:

> On Wed, 5 May 2021 11:41:52 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
>
> > The pm_runtime_get_sync() internally increments the
> > dev->power.usage_count without decrementing it, even on errors.
> > Replace it by the new pm_runtime_resume_and_get(), introduced by:
> > commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
> > in order to properly decrement the usage counter, avoiding
> > a potential PM usage counter leak.
> > 
> > Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
> > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
>
> Not a fix as far as I can see, just a cleanup - so perhaps not this set?

yes, the original changelog of this patch, that I acked,  made it
clear it was a cleanup:

"
Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to
deal with usage counter")                                                                                                                                         
added pm_runtime_resume_and_get() in order to automatically handle 
dev->power.usage_count decrement on errors.

Use the new API, in order to cleanup the error check logic.
"

This one above is new, but I saw Mauro is going change it.

------
Cheers,
     Rui

>
> Jonathan
>
>
> > ---
> >  drivers/staging/media/imx/imx7-mipi-csis.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/media/imx/imx7-mipi-csis.c b/drivers/staging/media/imx/imx7-mipi-csis.c
> > index 025fdc488bd6..1dc680d94a46 100644
> > --- a/drivers/staging/media/imx/imx7-mipi-csis.c
> > +++ b/drivers/staging/media/imx/imx7-mipi-csis.c
> > @@ -695,11 +695,10 @@ static int mipi_csis_s_stream(struct v4l2_subdev *mipi_sd, int enable)
> >  
> >  		mipi_csis_clear_counters(state);
> >  
> > -		ret = pm_runtime_get_sync(&state->pdev->dev);
> > -		if (ret < 0) {
> > -			pm_runtime_put_noidle(&state->pdev->dev);
> > +		ret = pm_runtime_resume_and_get(&state->pdev->dev);
> > +		if (ret < 0)
> >  			return ret;
> > -		}
> > +
> >  		ret = v4l2_subdev_call(state->src_sd, core, s_power, 1);
> >  		if (ret < 0 && ret != -ENOIOCTLCMD)
> >  			goto done;




_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 02/25] staging: media: imx7-mipi-csis: fix pm_runtime_get_sync() usage count

[Mauro Carvalho Chehab]

Em Wed, 05 May 2021 14:56:40 +0100
"Rui Miguel Silva" <rmfrfs@gmail.com> escreveu:

> Hi,
> On Wed May 5, 2021 at 12:06 PM WEST, Jonathan Cameron wrote:
> 
> > On Wed, 5 May 2021 11:41:52 +0200
> > Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> >  
> > > The pm_runtime_get_sync() internally increments the
> > > dev->power.usage_count without decrementing it, even on errors.
> > > Replace it by the new pm_runtime_resume_and_get(), introduced by:
> > > commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to deal with usage counter")
> > > in order to properly decrement the usage counter, avoiding
> > > a potential PM usage counter leak.
> > > 
> > > Acked-by: Rui Miguel Silva <rmfrfs@gmail.com>
> > > Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>  
> >
> > Not a fix as far as I can see, just a cleanup - so perhaps not this set?  
> 
> yes, the original changelog of this patch, that I acked,  made it
> clear it was a cleanup:
> 
> "
> Commit dd8088d5a896 ("PM: runtime: Add pm_runtime_resume_and_get to
> deal with usage counter")                                                                                                                                         
> added pm_runtime_resume_and_get() in order to automatically handle 
> dev->power.usage_count decrement on errors.
> 
> Use the new API, in order to cleanup the error check logic.
> "
> 
> This one above is new, but I saw Mauro is going change it.

Yes, I'll change the subject/description to the
"use pm_runtime_resume_and_get()" one on this patch, as there's
no issue to be fixed here, just a cleanup ;-)

Sorry for the mess. I did lots of rebase on ~80 patch series
over the last couple of days, based on the reviews (and my own
internal reviews)...

See, the current patchset has ~80 patches with ~30% contained
fixes. It shows that writing a balanced PM runtime code is not
so trivial ;-)

Thanks,
Mauro

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 00/25] Fix some PM runtime issues at the media subsystem

[Mauro Carvalho Chehab]

Em Wed,  5 May 2021 11:41:50 +0200
Mauro Carvalho Chehab <mchehab+huawei@kernel.org> escreveu:

> As part of an effort to cleanup pm_runtime*get* calls, I detected a number
> of issues at the media subsystem.
> 
> Most of the patches here were submitted previously at:
> 
> 	https://lore.kernel.org/linux-media/cover.1619621413.git.mchehab+huawei@kernel.org/
> 
> This series contain just the bug fixes and other related issues that are
> present with the current code on media.

Series merged on my stage tree, at:
	https://git.linuxtv.org/media_stage.git/log/

I'll be merging it at media_tree (either for 5.13 or 5.14) after the
end of the merge window (likely next week).

Please let me know if you find any problems on it.

PS.: please notice that my stage tree can be rebased.

Regards,
Mauro

> 
> It address the points from the existing reviews. I also did my own
> set of reviews, in order to avoid regressions.
> 
> Changes from v4 of the previous changeset:
> 
> - reworked i2c/css RPM get logic;
> - dropped two patches that could cause regressions;
> - am437x: keep using pm_runtime_get_sync on suspend/resume;
> - atmel: fix the returned code and add a print on failures at start streaming;
> - simplify some checks for return code > 0;
> - mdk-vcodec: properly handle RPM errors at device on logic;
> - venus: rework venus_sys_error_handler() logic;
> - sti/delta: fix an issue at the error checking logic.
> 
> Mauro Carvalho Chehab (25):
>   staging: media: rkvdec: fix pm_runtime_get_sync() usage count
>   staging: media: imx7-mipi-csis: fix pm_runtime_get_sync() usage count
>   media: venus: Rework error fail recover logic
>   media: s5p_cec: decrement usage count if disabled
>   media: i2c: ccs-core: return the right error code at suspend
>   media: i2c: imx334: fix the pm runtime get logic
>   media: exynos-gsc: don't resume at remove time
>   media: atmel: properly get pm_runtime
>   media: hantro: do a PM resume earlier
>   media: marvel-ccic: fix some issues when getting pm_runtime
>   media: mdk-mdp: fix pm_runtime_get_sync() usage count
>   media: rcar_fdp1: simplify error check logic at fdp_open()
>   media: rcar_fdp1: fix pm_runtime_get_sync() usage count
>   media: renesas-ceu: Properly check for PM errors
>   media: s5p: fix pm_runtime_get_sync() usage count
>   media: am437x: fix pm_runtime_get_sync() usage count
>   media: sh_vou: fix pm_runtime_get_sync() usage count
>   media: mtk-vcodec: fix PM runtime get logic
>   media: s5p-jpeg: fix pm_runtime_get_sync() usage count
>   media: sti/delta: use pm_runtime_resume_and_get()
>   media: sunxi: fix pm_runtime_get_sync() usage count
>   media: sti/bdisp: fix pm_runtime_get_sync() usage count
>   media: exynos4-is: fix pm_runtime_get_sync() usage count
>   media: exynos-gsc: fix pm_runtime_get_sync() usage count
>   media: i2c: ccs-core: fix pm_runtime_get_sync() usage count
> 
>  drivers/media/cec/platform/s5p/s5p_cec.c      |  7 ++-
>  drivers/media/i2c/ccs/ccs-core.c              | 41 ++++++++-----
>  drivers/media/i2c/imx334.c                    |  7 ++-
>  drivers/media/platform/am437x/am437x-vpfe.c   | 15 ++++-
>  drivers/media/platform/atmel/atmel-isc-base.c | 30 +++++++---
>  drivers/media/platform/atmel/atmel-isi.c      | 19 ++++--
>  drivers/media/platform/exynos-gsc/gsc-core.c  | 11 ++--
>  drivers/media/platform/exynos-gsc/gsc-m2m.c   |  4 +-
>  .../media/platform/exynos4-is/fimc-capture.c  |  6 +-
>  drivers/media/platform/exynos4-is/fimc-is.c   |  4 +-
>  .../platform/exynos4-is/fimc-isp-video.c      |  3 +-
>  drivers/media/platform/exynos4-is/fimc-isp.c  |  7 +--
>  drivers/media/platform/exynos4-is/fimc-lite.c |  5 +-
>  drivers/media/platform/exynos4-is/fimc-m2m.c  |  5 +-
>  drivers/media/platform/exynos4-is/media-dev.c |  9 +--
>  drivers/media/platform/exynos4-is/mipi-csis.c | 10 ++--
>  .../media/platform/marvell-ccic/mcam-core.c   |  9 ++-
>  drivers/media/platform/mtk-mdp/mtk_mdp_m2m.c  |  6 +-
>  .../platform/mtk-vcodec/mtk_vcodec_dec_drv.c  |  4 +-
>  .../platform/mtk-vcodec/mtk_vcodec_dec_pm.c   |  8 ++-
>  .../platform/mtk-vcodec/mtk_vcodec_dec_pm.h   |  2 +-
>  drivers/media/platform/qcom/venus/core.c      | 59 +++++++++++++++----
>  drivers/media/platform/rcar_fdp1.c            | 28 ++++++---
>  drivers/media/platform/renesas-ceu.c          |  4 +-
>  drivers/media/platform/s5p-jpeg/jpeg-core.c   |  5 +-
>  drivers/media/platform/sh_vou.c               |  6 +-
>  drivers/media/platform/sti/bdisp/bdisp-v4l2.c |  7 ++-
>  drivers/media/platform/sti/delta/delta-v4l2.c |  8 +--
>  .../sunxi/sun8i-rotate/sun8i_rotate.c         |  2 +-
>  drivers/staging/media/hantro/hantro_drv.c     |  7 ++-
>  drivers/staging/media/imx/imx7-mipi-csis.c    |  7 +--
>  drivers/staging/media/rkvdec/rkvdec.c         |  2 +-
>  32 files changed, 220 insertions(+), 127 deletions(-)
> 



Thanks,
Mauro

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 08/25] media: atmel: properly get pm_runtime

[Eugen.Hristev]

On 5/5/21 3:08 PM, Jonathan Cameron wrote:
> On Wed, 5 May 2021 11:41:58 +0200
> Mauro Carvalho Chehab <mchehab+huawei@kernel.org> wrote:
> 
>> There are several issues in the way the atmel driver handles
>> pm_runtime_get_sync():
>>
>> - it doesn't check return codes;
>> - it doesn't properly decrement the usage_count on all places;
>> - it starts streaming even if pm_runtime_get_sync() fails.
>> - while it tries to get pm_runtime at the clock enable logic,
>>    it doesn't check if the operation was suceeded.
>>
>> Replace all occurrences of it to use the new kAPI:
>> pm_runtime_resume_and_get(), which ensures that, if the
>> return code is not negative, the usage_count was incremented.
>>
>> With that, add additional checks when this is called, in order
>> to ensure that errors will be properly addressed.
>>
>> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> 
> Hi Mauro, I don't know media enough to know what is the right answer
> but in some of this series, a failure in
> pm_runtime_resume_and_get() leads to a bunch of buffer cleanup
> (patch 22 being an example) and in others return happens without doing
> that cleanup.
> 
> It might be both are safe, or I'm missing something else, but I'm
> certainly not confident enough to give any tags on this one as a result
> of that mismatch.
> 
>> ---
>>   drivers/media/platform/atmel/atmel-isc-base.c | 30 ++++++++++++++-----
>>   drivers/media/platform/atmel/atmel-isi.c      | 19 +++++++++---
>>   2 files changed, 38 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
>> index fe3ec8d0eaee..ce8e1351fa53 100644
>> --- a/drivers/media/platform/atmel/atmel-isc-base.c
>> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
>> @@ -294,9 +294,13 @@ static int isc_wait_clk_stable(struct clk_hw *hw)
>>   static int isc_clk_prepare(struct clk_hw *hw)
>>   {
>>        struct isc_clk *isc_clk = to_isc_clk(hw);
>> +     int ret;
>>
>> -     if (isc_clk->id == ISC_ISPCK)
>> -             pm_runtime_get_sync(isc_clk->dev);
>> +     if (isc_clk->id == ISC_ISPCK) {
>> +             ret = pm_runtime_resume_and_get(isc_clk->dev);
>> +             if (ret < 0)
>> +                     return ret;
>> +     }

Hi Mauro,

With this patch, the ISC is broken on latest media tree. It looks like 
pm_runtime_resume_and_get for the ISC_ISPCK clock returns -ENOACCESS and 
thus, the probe of the driver fails:

atmel-sama5d2-isc f0008000.isc: failed to enable ispck: -13
atmel-sama5d2-isc: probe of f0008000.isc failed with error -13


Could you point out how I could fix this ? Maybe the isc_clk->dev is not 
properly handled/initialized in some other part of the code ?

Thanks !
Eugen


>>
>>        return isc_wait_clk_stable(hw);
>>   }
>> @@ -353,9 +357,13 @@ static int isc_clk_is_enabled(struct clk_hw *hw)
>>   {
>>        struct isc_clk *isc_clk = to_isc_clk(hw);
>>        u32 status;
>> +     int ret;
>>
>> -     if (isc_clk->id == ISC_ISPCK)
>> -             pm_runtime_get_sync(isc_clk->dev);
>> +     if (isc_clk->id == ISC_ISPCK) {
>> +             ret = pm_runtime_resume_and_get(isc_clk->dev);
>> +             if (ret < 0)
>> +                     return 0;
>> +     }
>>
>>        regmap_read(isc_clk->regmap, ISC_CLKSR, &status);
>>
>> @@ -807,7 +815,12 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
>>                goto err_start_stream;
>>        }
>>
>> -     pm_runtime_get_sync(isc->dev);
>> +     ret = pm_runtime_resume_and_get(isc->dev);
>> +     if (ret < 0) {
>> +             v4l2_err(&isc->v4l2_dev, "RPM resume failed in subdev %d\n",
>> +                      ret);
>> +             goto err_pm_get;
>> +     }
>>
>>        ret = isc_configure(isc);
>>        if (unlikely(ret))
>> @@ -838,7 +851,7 @@ static int isc_start_streaming(struct vb2_queue *vq, unsigned int count)
>>
>>   err_configure:
>>        pm_runtime_put_sync(isc->dev);
>> -
>> +err_pm_get:
>>        v4l2_subdev_call(isc->current_subdev->sd, video, s_stream, 0);
>>
>>   err_start_stream:
>> @@ -1809,6 +1822,7 @@ static void isc_awb_work(struct work_struct *w)
>>        u32 baysel;
>>        unsigned long flags;
>>        u32 min, max;
>> +     int ret;
>>
>>        /* streaming is not active anymore */
>>        if (isc->stop)
>> @@ -1831,7 +1845,9 @@ static void isc_awb_work(struct work_struct *w)
>>        ctrls->hist_id = hist_id;
>>        baysel = isc->config.sd_format->cfa_baycfg << ISC_HIS_CFG_BAYSEL_SHIFT;
>>
>> -     pm_runtime_get_sync(isc->dev);
>> +     ret = pm_runtime_resume_and_get(isc->dev);
>> +     if (ret < 0)
>> +             return;
>>
>>        /*
>>         * only update if we have all the required histograms and controls
>> diff --git a/drivers/media/platform/atmel/atmel-isi.c b/drivers/media/platform/atmel/atmel-isi.c
>> index e392b3efe363..5b1dd358f2e6 100644
>> --- a/drivers/media/platform/atmel/atmel-isi.c
>> +++ b/drivers/media/platform/atmel/atmel-isi.c
>> @@ -422,7 +422,9 @@ static int start_streaming(struct vb2_queue *vq, unsigned int count)
>>        struct frame_buffer *buf, *node;
>>        int ret;
>>
>> -     pm_runtime_get_sync(isi->dev);
>> +     ret = pm_runtime_resume_and_get(isi->dev);
>> +     if (ret < 0)
>> +             return ret;
> This is the case I'm referring to above.
> 
>>
>>        /* Enable stream on the sub device */
>>        ret = v4l2_subdev_call(isi->entity.subdev, video, s_stream, 1);
>> @@ -782,9 +784,10 @@ static int isi_enum_frameintervals(struct file *file, void *fh,
>>        return 0;
>>   }
>>
>> -static void isi_camera_set_bus_param(struct atmel_isi *isi)
>> +static int isi_camera_set_bus_param(struct atmel_isi *isi)
>>   {
>>        u32 cfg1 = 0;
>> +     int ret;
>>
>>        /* set bus param for ISI */
>>        if (isi->pdata.hsync_act_low)
>> @@ -801,12 +804,16 @@ static void isi_camera_set_bus_param(struct atmel_isi *isi)
>>        cfg1 |= ISI_CFG1_THMASK_BEATS_16;
>>
>>        /* Enable PM and peripheral clock before operate isi registers */
>> -     pm_runtime_get_sync(isi->dev);
>> +     ret = pm_runtime_resume_and_get(isi->dev);
>> +     if (ret < 0)
>> +             return ret;
>>
>>        isi_writel(isi, ISI_CTRL, ISI_CTRL_DIS);
>>        isi_writel(isi, ISI_CFG1, cfg1);
>>
>>        pm_runtime_put(isi->dev);
>> +
>> +     return 0;
>>   }
>>
>>   /* -----------------------------------------------------------------------*/
>> @@ -1085,7 +1092,11 @@ static int isi_graph_notify_complete(struct v4l2_async_notifier *notifier)
>>                dev_err(isi->dev, "No supported mediabus format found\n");
>>                return ret;
>>        }
>> -     isi_camera_set_bus_param(isi);
>> +     ret = isi_camera_set_bus_param(isi);
>> +     if (ret) {
>> +             dev_err(isi->dev, "Can't wake up device\n");
>> +             return ret;
>> +     }
>>
>>        ret = isi_set_default_fmt(isi);
>>        if (ret) {
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 08/25] media: atmel: properly get pm_runtime

[Mauro Carvalho Chehab]

Em Thu, 10 Jun 2021 09:04:07 +0000
<Eugen.Hristev@microchip.com> escreveu:

> >> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
> >> index fe3ec8d0eaee..ce8e1351fa53 100644
> >> --- a/drivers/media/platform/atmel/atmel-isc-base.c
> >> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
> >> @@ -294,9 +294,13 @@ static int isc_wait_clk_stable(struct clk_hw *hw)
> >>   static int isc_clk_prepare(struct clk_hw *hw)
> >>   {
> >>        struct isc_clk *isc_clk = to_isc_clk(hw);
> >> +     int ret;
> >>
> >> -     if (isc_clk->id == ISC_ISPCK)
> >> -             pm_runtime_get_sync(isc_clk->dev);
> >> +     if (isc_clk->id == ISC_ISPCK) {
> >> +             ret = pm_runtime_resume_and_get(isc_clk->dev);
> >> +             if (ret < 0)
> >> +                     return ret;
> >> +     }  
> 
> Hi Mauro,
> 
> With this patch, the ISC is broken on latest media tree. It looks like 
> pm_runtime_resume_and_get for the ISC_ISPCK clock returns -ENOACCESS and 
> thus, the probe of the driver fails:
> 
> atmel-sama5d2-isc f0008000.isc: failed to enable ispck: -13
> atmel-sama5d2-isc: probe of f0008000.isc failed with error -13
> 
> 
> Could you point out how I could fix this ? Maybe the isc_clk->dev is not 
> properly handled/initialized in some other part of the code ?

Looking at RPM implementation:

	static int rpm_resume(struct device *dev, int rpmflags)
	{
...
        if (dev->power.runtime_error)
                retval = -EINVAL;
        else if (dev->power.disable_depth == 1 && dev->power.is_suspended
            && dev->power.runtime_status == RPM_ACTIVE)
                retval = 1;
        else if (dev->power.disable_depth > 0)
                retval = -EACCES;
...

It sounds that RPM is disabled for this clock. Did you call
pm_runtime_enable() before calling isc_clk_prepare()?

Thanks,
Mauro

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 08/25] media: atmel: properly get pm_runtime

[Eugen.Hristev]

On 6/10/21 12:38 PM, Mauro Carvalho Chehab wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> Em Thu, 10 Jun 2021 09:04:07 +0000
> <Eugen.Hristev@microchip.com> escreveu:
> 
>>>> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
>>>> index fe3ec8d0eaee..ce8e1351fa53 100644
>>>> --- a/drivers/media/platform/atmel/atmel-isc-base.c
>>>> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
>>>> @@ -294,9 +294,13 @@ static int isc_wait_clk_stable(struct clk_hw *hw)
>>>>    static int isc_clk_prepare(struct clk_hw *hw)
>>>>    {
>>>>         struct isc_clk *isc_clk = to_isc_clk(hw);
>>>> +     int ret;
>>>>
>>>> -     if (isc_clk->id == ISC_ISPCK)
>>>> -             pm_runtime_get_sync(isc_clk->dev);
>>>> +     if (isc_clk->id == ISC_ISPCK) {
>>>> +             ret = pm_runtime_resume_and_get(isc_clk->dev);
>>>> +             if (ret < 0)
>>>> +                     return ret;
>>>> +     }
>>
>> Hi Mauro,
>>
>> With this patch, the ISC is broken on latest media tree. It looks like
>> pm_runtime_resume_and_get for the ISC_ISPCK clock returns -ENOACCESS and
>> thus, the probe of the driver fails:
>>
>> atmel-sama5d2-isc f0008000.isc: failed to enable ispck: -13
>> atmel-sama5d2-isc: probe of f0008000.isc failed with error -13
>>
>>
>> Could you point out how I could fix this ? Maybe the isc_clk->dev is not
>> properly handled/initialized in some other part of the code ?
> 
> Looking at RPM implementation:
> 
>          static int rpm_resume(struct device *dev, int rpmflags)
>          {
> ...
>          if (dev->power.runtime_error)
>                  retval = -EINVAL;
>          else if (dev->power.disable_depth == 1 && dev->power.is_suspended
>              && dev->power.runtime_status == RPM_ACTIVE)
>                  retval = 1;
>          else if (dev->power.disable_depth > 0)
>                  retval = -EACCES;
> ...
> 
> It sounds that RPM is disabled for this clock. Did you call
> pm_runtime_enable() before calling isc_clk_prepare()?
> 
> Thanks,
> Mauro
> 

I tried to call pm_runtime_enable for the clk at clk_init time, but 
doing that makes the runtime for the ISC fail like this:

atmel-sama5d2-isc f0008000.isc: Unbalanced pm_runtime_enable!
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 08/25] media: atmel: properly get pm_runtime

[Mauro Carvalho Chehab]

Em Thu, 10 Jun 2021 12:00:42 +0000
<Eugen.Hristev@microchip.com> escreveu:

> On 6/10/21 12:38 PM, Mauro Carvalho Chehab wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > Em Thu, 10 Jun 2021 09:04:07 +0000
> > <Eugen.Hristev@microchip.com> escreveu:
> >   
> >>>> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
> >>>> index fe3ec8d0eaee..ce8e1351fa53 100644
> >>>> --- a/drivers/media/platform/atmel/atmel-isc-base.c
> >>>> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
> >>>> @@ -294,9 +294,13 @@ static int isc_wait_clk_stable(struct clk_hw *hw)
> >>>>    static int isc_clk_prepare(struct clk_hw *hw)
> >>>>    {
> >>>>         struct isc_clk *isc_clk = to_isc_clk(hw);
> >>>> +     int ret;
> >>>>
> >>>> -     if (isc_clk->id == ISC_ISPCK)
> >>>> -             pm_runtime_get_sync(isc_clk->dev);
> >>>> +     if (isc_clk->id == ISC_ISPCK) {
> >>>> +             ret = pm_runtime_resume_and_get(isc_clk->dev);
> >>>> +             if (ret < 0)
> >>>> +                     return ret;
> >>>> +     }  
> >>
> >> Hi Mauro,
> >>
> >> With this patch, the ISC is broken on latest media tree. It looks like
> >> pm_runtime_resume_and_get for the ISC_ISPCK clock returns -ENOACCESS and
> >> thus, the probe of the driver fails:
> >>
> >> atmel-sama5d2-isc f0008000.isc: failed to enable ispck: -13
> >> atmel-sama5d2-isc: probe of f0008000.isc failed with error -13
> >>
> >>
> >> Could you point out how I could fix this ? Maybe the isc_clk->dev is not
> >> properly handled/initialized in some other part of the code ?  
> > 
> > Looking at RPM implementation:
> > 
> >          static int rpm_resume(struct device *dev, int rpmflags)
> >          {
> > ...
> >          if (dev->power.runtime_error)
> >                  retval = -EINVAL;
> >          else if (dev->power.disable_depth == 1 && dev->power.is_suspended
> >              && dev->power.runtime_status == RPM_ACTIVE)
> >                  retval = 1;
> >          else if (dev->power.disable_depth > 0)
> >                  retval = -EACCES;
> > ...
> > 
> > It sounds that RPM is disabled for this clock. Did you call
> > pm_runtime_enable() before calling isc_clk_prepare()?
> > 
> > Thanks,
> > Mauro
> >   
> 
> I tried to call pm_runtime_enable for the clk at clk_init time, but 
> doing that makes the runtime for the ISC fail like this:
> 
> atmel-sama5d2-isc f0008000.isc: Unbalanced pm_runtime_enable!

Making RPM balanced is complex ;-)

Yet, clearly there's something weird at the PM code. I mean,
ignoring a -ENOACCESS error like the original code sounds wrong,
as it basically means that pm_runtime_get_sync() were doing
nothing (except by incrementing a refcount).

Some drivers call clk_prepare()/clk_unprepare() at their
.prepare/.unprepare ops. Those functions internally call
RPM get logic recursively, to ensure that the RPM
state of the parents are also at the right state.

Thanks,
Mauro

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 08/25] media: atmel: properly get pm_runtime

[Mauro Carvalho Chehab]

Hi Eugen,

Em Thu, 10 Jun 2021 12:00:42 +0000
<Eugen.Hristev@microchip.com> escreveu:

> On 6/10/21 12:38 PM, Mauro Carvalho Chehab wrote:
> > EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> > 
> > Em Thu, 10 Jun 2021 09:04:07 +0000
> > <Eugen.Hristev@microchip.com> escreveu:
> >   
> >>>> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
> >>>> index fe3ec8d0eaee..ce8e1351fa53 100644
> >>>> --- a/drivers/media/platform/atmel/atmel-isc-base.c
> >>>> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
> >>>> @@ -294,9 +294,13 @@ static int isc_wait_clk_stable(struct clk_hw *hw)
> >>>>    static int isc_clk_prepare(struct clk_hw *hw)
> >>>>    {
> >>>>         struct isc_clk *isc_clk = to_isc_clk(hw);
> >>>> +     int ret;
> >>>>
> >>>> -     if (isc_clk->id == ISC_ISPCK)
> >>>> -             pm_runtime_get_sync(isc_clk->dev);
> >>>> +     if (isc_clk->id == ISC_ISPCK) {
> >>>> +             ret = pm_runtime_resume_and_get(isc_clk->dev);
> >>>> +             if (ret < 0)
> >>>> +                     return ret;
> >>>> +     }  
> >>
> >> Hi Mauro,
> >>
> >> With this patch, the ISC is broken on latest media tree. It looks like
> >> pm_runtime_resume_and_get for the ISC_ISPCK clock returns -ENOACCESS and
> >> thus, the probe of the driver fails:
> >>
> >> atmel-sama5d2-isc f0008000.isc: failed to enable ispck: -13
> >> atmel-sama5d2-isc: probe of f0008000.isc failed with error -13

What's the current status of this issue? 

If the bug still happens, we need a fix for it.

We might revert this patch, but this would just be masking some other
hidden bug.

Regards,
Mauro



> >>
> >>
> >> Could you point out how I could fix this ? Maybe the isc_clk->dev is not
> >> properly handled/initialized in some other part of the code ?  
> > 
> > Looking at RPM implementation:
> > 
> >          static int rpm_resume(struct device *dev, int rpmflags)
> >          {
> > ...
> >          if (dev->power.runtime_error)
> >                  retval = -EINVAL;
> >          else if (dev->power.disable_depth == 1 && dev->power.is_suspended
> >              && dev->power.runtime_status == RPM_ACTIVE)
> >                  retval = 1;
> >          else if (dev->power.disable_depth > 0)
> >                  retval = -EACCES;
> > ...
> > 
> > It sounds that RPM is disabled for this clock. Did you call
> > pm_runtime_enable() before calling isc_clk_prepare()?
> > 
> > Thanks,
> > Mauro
> >   
> 
> I tried to call pm_runtime_enable for the clk at clk_init time, but 
> doing that makes the runtime for the ISC fail like this:
> 
> atmel-sama5d2-isc f0008000.isc: Unbalanced pm_runtime_enable!



Thanks,
Mauro

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 08/25] media: atmel: properly get pm_runtime

[Eugen.Hristev]

On 9/6/21 11:03 AM, Mauro Carvalho Chehab wrote:
> Hi Eugen,
> 
> Em Thu, 10 Jun 2021 12:00:42 +0000
> <Eugen.Hristev@microchip.com> escreveu:
> 
>> On 6/10/21 12:38 PM, Mauro Carvalho Chehab wrote:
>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>
>>> Em Thu, 10 Jun 2021 09:04:07 +0000
>>> <Eugen.Hristev@microchip.com> escreveu:
>>>
>>>>>> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
>>>>>> index fe3ec8d0eaee..ce8e1351fa53 100644
>>>>>> --- a/drivers/media/platform/atmel/atmel-isc-base.c
>>>>>> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
>>>>>> @@ -294,9 +294,13 @@ static int isc_wait_clk_stable(struct clk_hw *hw)
>>>>>>     static int isc_clk_prepare(struct clk_hw *hw)
>>>>>>     {
>>>>>>          struct isc_clk *isc_clk = to_isc_clk(hw);
>>>>>> +     int ret;
>>>>>>
>>>>>> -     if (isc_clk->id == ISC_ISPCK)
>>>>>> -             pm_runtime_get_sync(isc_clk->dev);
>>>>>> +     if (isc_clk->id == ISC_ISPCK) {
>>>>>> +             ret = pm_runtime_resume_and_get(isc_clk->dev);
>>>>>> +             if (ret < 0)
>>>>>> +                     return ret;
>>>>>> +     }
>>>>
>>>> Hi Mauro,
>>>>
>>>> With this patch, the ISC is broken on latest media tree. It looks like
>>>> pm_runtime_resume_and_get for the ISC_ISPCK clock returns -ENOACCESS and
>>>> thus, the probe of the driver fails:
>>>>
>>>> atmel-sama5d2-isc f0008000.isc: failed to enable ispck: -13
>>>> atmel-sama5d2-isc: probe of f0008000.isc failed with error -13
> 
> What's the current status of this issue?

Hi Mauro,

Currently, as far as I know, the ISC is broken in 5.14 and current Linux 
master. I plan to investigate this issue this week (or the next...), 
together with some other things. I want to tryout the PM part of the 
driver to see where is the problem.
When I come up with a fix or patch, I will send it on the mailing list.
If you have any ideas that I can try meanwhile, feel free to contact me 
to send a patch that I can test.


Eugen


> 
> If the bug still happens, we need a fix for it.
> 
> We might revert this patch, but this would just be masking some other
> hidden bug.
> 
> Regards,
> Mauro
> 
> 
> 
>>>>
>>>>
>>>> Could you point out how I could fix this ? Maybe the isc_clk->dev is not
>>>> properly handled/initialized in some other part of the code ?
>>>
>>> Looking at RPM implementation:
>>>
>>>           static int rpm_resume(struct device *dev, int rpmflags)
>>>           {
>>> ...
>>>           if (dev->power.runtime_error)
>>>                   retval = -EINVAL;
>>>           else if (dev->power.disable_depth == 1 && dev->power.is_suspended
>>>               && dev->power.runtime_status == RPM_ACTIVE)
>>>                   retval = 1;
>>>           else if (dev->power.disable_depth > 0)
>>>                   retval = -EACCES;
>>> ...
>>>
>>> It sounds that RPM is disabled for this clock. Did you call
>>> pm_runtime_enable() before calling isc_clk_prepare()?
>>>
>>> Thanks,
>>> Mauro
>>>
>>
>> I tried to call pm_runtime_enable for the clk at clk_init time, but
>> doing that makes the runtime for the ISC fail like this:
>>
>> atmel-sama5d2-isc f0008000.isc: Unbalanced pm_runtime_enable!
> 
> 
> 
> Thanks,
> Mauro
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 08/25] media: atmel: properly get pm_runtime

[Eugen.Hristev]

On 9/6/21 11:13 AM, Eugen Hristev - M18282 wrote:
> On 9/6/21 11:03 AM, Mauro Carvalho Chehab wrote:
>> Hi Eugen,
>>
>> Em Thu, 10 Jun 2021 12:00:42 +0000
>> <Eugen.Hristev@microchip.com> escreveu:
>>
>>> On 6/10/21 12:38 PM, Mauro Carvalho Chehab wrote:
>>>> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>>>>
>>>> Em Thu, 10 Jun 2021 09:04:07 +0000
>>>> <Eugen.Hristev@microchip.com> escreveu:
>>>>
>>>>>>> diff --git a/drivers/media/platform/atmel/atmel-isc-base.c b/drivers/media/platform/atmel/atmel-isc-base.c
>>>>>>> index fe3ec8d0eaee..ce8e1351fa53 100644
>>>>>>> --- a/drivers/media/platform/atmel/atmel-isc-base.c
>>>>>>> +++ b/drivers/media/platform/atmel/atmel-isc-base.c
>>>>>>> @@ -294,9 +294,13 @@ static int isc_wait_clk_stable(struct clk_hw *hw)
>>>>>>>      static int isc_clk_prepare(struct clk_hw *hw)
>>>>>>>      {
>>>>>>>           struct isc_clk *isc_clk = to_isc_clk(hw);
>>>>>>> +     int ret;
>>>>>>>
>>>>>>> -     if (isc_clk->id == ISC_ISPCK)
>>>>>>> -             pm_runtime_get_sync(isc_clk->dev);
>>>>>>> +     if (isc_clk->id == ISC_ISPCK) {
>>>>>>> +             ret = pm_runtime_resume_and_get(isc_clk->dev);
>>>>>>> +             if (ret < 0)
>>>>>>> +                     return ret;
>>>>>>> +     }
>>>>>
>>>>> Hi Mauro,
>>>>>
>>>>> With this patch, the ISC is broken on latest media tree. It looks like
>>>>> pm_runtime_resume_and_get for the ISC_ISPCK clock returns -ENOACCESS and
>>>>> thus, the probe of the driver fails:
>>>>>
>>>>> atmel-sama5d2-isc f0008000.isc: failed to enable ispck: -13
>>>>> atmel-sama5d2-isc: probe of f0008000.isc failed with error -13
>>
>> What's the current status of this issue?
> 
> Hi Mauro,
> 
> Currently, as far as I know, the ISC is broken in 5.14 and current Linux
> master. I plan to investigate this issue this week (or the next...),
> together with some other things. I want to tryout the PM part of the
> driver to see where is the problem.
> When I come up with a fix or patch, I will send it on the mailing list.
> If you have any ideas that I can try meanwhile, feel free to contact me
> to send a patch that I can test.
> 
> 

Hi Mauro,

Regarding this issue, I sent a patch on the ML that should solve it.

https://patchwork.linuxtv.org/project/linux-media/patch/20210913102254.108638-1-eugen.hristev@microchip.com/

Eugen

> Eugen
> 
> 
>>
>> If the bug still happens, we need a fix for it.
>>
>> We might revert this patch, but this would just be masking some other
>> hidden bug.
>>
>> Regards,
>> Mauro
>>
>>
>>
>>>>>
>>>>>
>>>>> Could you point out how I could fix this ? Maybe the isc_clk->dev is not
>>>>> properly handled/initialized in some other part of the code ?
>>>>
>>>> Looking at RPM implementation:
>>>>
>>>>            static int rpm_resume(struct device *dev, int rpmflags)
>>>>            {
>>>> ...
>>>>            if (dev->power.runtime_error)
>>>>                    retval = -EINVAL;
>>>>            else if (dev->power.disable_depth == 1 && dev->power.is_suspended
>>>>                && dev->power.runtime_status == RPM_ACTIVE)
>>>>                    retval = 1;
>>>>            else if (dev->power.disable_depth > 0)
>>>>                    retval = -EACCES;
>>>> ...
>>>>
>>>> It sounds that RPM is disabled for this clock. Did you call
>>>> pm_runtime_enable() before calling isc_clk_prepare()?
>>>>
>>>> Thanks,
>>>> Mauro
>>>>
>>>
>>> I tried to call pm_runtime_enable for the clk at clk_init time, but
>>> doing that makes the runtime for the ISC fail like this:
>>>
>>> atmel-sama5d2-isc f0008000.isc: Unbalanced pm_runtime_enable!
>>
>>
>>
>> Thanks,
>> Mauro
>>
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel