Linux-Renesas-SoC Archive on lore.kernel.org
 help / Atom feed
* [PATCH v2 0/8] rcar-vin: Merge Gen2 and Gen3 file operations
@ 2019-05-16  1:14 Niklas Söderlund
  2019-05-16  1:14 ` [PATCH v2 1/8] rcar-vin: Do not call pm_runtime_{resume,suspend}() Niklas Söderlund
                   ` (7 more replies)
  0 siblings, 8 replies; 21+ messages in thread
From: Niklas Söderlund @ 2019-05-16  1:14 UTC (permalink / raw)
  To: Laurent Pinchart, Ulrich Hecht, linux-media
  Cc: linux-renesas-soc, Niklas Söderlund

Hi,

This series aims to merge the two different set of file operations used
in the rcar-vin driver. One set was used on Renesas Gen2 boards while
the other set was used on Gen3. Main difference between the two is that
Gen2 uses a device node centric world view while Gen3 subscribes to the
media controller way of looking at things ;-)

After refactoring out a lot of code left over from the Gen2 mode which
stems from the drivers origin in soc_camera it became apparent that a
lot of code could me removed my merging the two sets.

Tested on both Gen2 and Gen3 no regressions found.

Niklas Söderlund (8):
  rcar-vin: Do not call pm_runtime_{resume,suspend}()
  rcar-vin: Remove unneeded calls to pm_runtime_{enable,disable}
  rcar-vin: Allow interrupting lock when trying to open the video device
  rcar-vin: Do not sync subdevice format when opening the video device
  rcar-vin: Move pm_runtime_{get,put} out of helpers
  rcar-vin: Merge helpers dealing with powering the parallel subdevice
  rcar-vin: Fold rvin_initialize_device() into rvin_open()
  rcar-vin: Merge Gen2 and Gen3 file operations

 drivers/media/platform/rcar-vin/rcar-v4l2.c | 188 ++++----------------
 1 file changed, 38 insertions(+), 150 deletions(-)

-- 
2.21.0


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v2 1/8] rcar-vin: Do not call pm_runtime_{resume,suspend}()
  2019-05-16  1:14 [PATCH v2 0/8] rcar-vin: Merge Gen2 and Gen3 file operations Niklas Söderlund
@ 2019-05-16  1:14 ` Niklas Söderlund
  2019-05-16  7:23   ` Geert Uytterhoeven
  2019-05-16  1:14 ` [PATCH v2 2/8] rcar-vin: Remove unneeded calls to pm_runtime_{enable,disable} Niklas Söderlund
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Niklas Söderlund @ 2019-05-16  1:14 UTC (permalink / raw)
  To: Laurent Pinchart, Ulrich Hecht, linux-media
  Cc: linux-renesas-soc, Niklas Söderlund, Ulrich Hecht

The driver does not implement runtime resume and suspend function so
there is little point in trying to call them. This is a leftover from
the drivers soc_camera beginnings.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 7cbdcbf9b090c638..b821ea01786eb1ff 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -798,9 +798,6 @@ static int rvin_initialize_device(struct file *file)
 		return ret;
 
 	pm_runtime_enable(&vin->vdev.dev);
-	ret = pm_runtime_resume(&vin->vdev.dev);
-	if (ret < 0 && ret != -ENOSYS)
-		goto eresume;
 
 	/*
 	 * Try to configure with default parameters. Notice: this is the
@@ -817,7 +814,6 @@ static int rvin_initialize_device(struct file *file)
 	return 0;
 esfmt:
 	pm_runtime_disable(&vin->vdev.dev);
-eresume:
 	rvin_power_off(vin);
 
 	return ret;
@@ -868,7 +864,6 @@ static int rvin_release(struct file *file)
 	 * Then de-initialize hw module.
 	 */
 	if (fh_singular) {
-		pm_runtime_suspend(&vin->vdev.dev);
 		pm_runtime_disable(&vin->vdev.dev);
 		rvin_power_off(vin);
 	}
-- 
2.21.0


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v2 2/8] rcar-vin: Remove unneeded calls to pm_runtime_{enable,disable}
  2019-05-16  1:14 [PATCH v2 0/8] rcar-vin: Merge Gen2 and Gen3 file operations Niklas Söderlund
  2019-05-16  1:14 ` [PATCH v2 1/8] rcar-vin: Do not call pm_runtime_{resume,suspend}() Niklas Söderlund
@ 2019-05-16  1:14 ` Niklas Söderlund
  2019-05-16  7:27   ` Geert Uytterhoeven
  2019-05-16  1:14 ` [PATCH v2 3/8] rcar-vin: Allow interrupting lock when trying to open the video device Niklas Söderlund
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Niklas Söderlund @ 2019-05-16  1:14 UTC (permalink / raw)
  To: Laurent Pinchart, Ulrich Hecht, linux-media
  Cc: linux-renesas-soc, Niklas Söderlund, Ulrich Hecht

Runtime PM is already enabled unconditionally when the driver is probed
and disabled when it's removed. There is no point in doing it again for
Gen2 when opening and closing the video device.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index b821ea01786eb1ff..0841f1a0bfd7ba3a 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -797,8 +797,6 @@ static int rvin_initialize_device(struct file *file)
 	if (ret < 0)
 		return ret;
 
-	pm_runtime_enable(&vin->vdev.dev);
-
 	/*
 	 * Try to configure with default parameters. Notice: this is the
 	 * very first open, so, we cannot race against other calls,
@@ -813,7 +811,6 @@ static int rvin_initialize_device(struct file *file)
 
 	return 0;
 esfmt:
-	pm_runtime_disable(&vin->vdev.dev);
 	rvin_power_off(vin);
 
 	return ret;
@@ -863,10 +860,8 @@ static int rvin_release(struct file *file)
 	 * If this was the last open file.
 	 * Then de-initialize hw module.
 	 */
-	if (fh_singular) {
-		pm_runtime_disable(&vin->vdev.dev);
+	if (fh_singular)
 		rvin_power_off(vin);
-	}
 
 	mutex_unlock(&vin->lock);
 
-- 
2.21.0


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v2 3/8] rcar-vin: Allow interrupting lock when trying to open the video device
  2019-05-16  1:14 [PATCH v2 0/8] rcar-vin: Merge Gen2 and Gen3 file operations Niklas Söderlund
  2019-05-16  1:14 ` [PATCH v2 1/8] rcar-vin: Do not call pm_runtime_{resume,suspend}() Niklas Söderlund
  2019-05-16  1:14 ` [PATCH v2 2/8] rcar-vin: Remove unneeded calls to pm_runtime_{enable,disable} Niklas Söderlund
@ 2019-05-16  1:14 ` Niklas Söderlund
  2019-05-16 11:21   ` Laurent Pinchart
  2019-05-16  1:14 ` [PATCH v2 4/8] rcar-vin: Do not sync subdevice format when opening " Niklas Söderlund
                   ` (4 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Niklas Söderlund @ 2019-05-16  1:14 UTC (permalink / raw)
  To: Laurent Pinchart, Ulrich Hecht, linux-media
  Cc: linux-renesas-soc, Niklas Söderlund, Ulrich Hecht

The user should be allowed to break waiting for the lock when opening
the video device.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 0841f1a0bfd7ba3a..f67cef97b89a3bd4 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -821,7 +821,9 @@ static int rvin_open(struct file *file)
 	struct rvin_dev *vin = video_drvdata(file);
 	int ret;
 
-	mutex_lock(&vin->lock);
+	ret = mutex_lock_interruptible(&vin->lock);
+	if (ret)
+		return ret;
 
 	file->private_data = vin;
 
-- 
2.21.0


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v2 4/8] rcar-vin: Do not sync subdevice format when opening the video device
  2019-05-16  1:14 [PATCH v2 0/8] rcar-vin: Merge Gen2 and Gen3 file operations Niklas Söderlund
                   ` (2 preceding siblings ...)
  2019-05-16  1:14 ` [PATCH v2 3/8] rcar-vin: Allow interrupting lock when trying to open the video device Niklas Söderlund
@ 2019-05-16  1:14 ` " Niklas Söderlund
  2019-05-16 11:26   ` Laurent Pinchart
  2019-05-16  1:14 ` [PATCH v2 5/8] rcar-vin: Move pm_runtime_{get,put} out of helpers Niklas Söderlund
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Niklas Söderlund @ 2019-05-16  1:14 UTC (permalink / raw)
  To: Laurent Pinchart, Ulrich Hecht, linux-media
  Cc: linux-renesas-soc, Niklas Söderlund, Ulrich Hecht

The format is already synced when the subdevice is bound, there is no
need to do do it every time the video device is opened.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 25 ---------------------
 1 file changed, 25 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index f67cef97b89a3bd4..71651c5a69483367 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -782,38 +782,13 @@ static int rvin_initialize_device(struct file *file)
 	struct rvin_dev *vin = video_drvdata(file);
 	int ret;
 
-	struct v4l2_format f = {
-		.type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
-		.fmt.pix = {
-			.width		= vin->format.width,
-			.height		= vin->format.height,
-			.field		= vin->format.field,
-			.colorspace	= vin->format.colorspace,
-			.pixelformat	= vin->format.pixelformat,
-		},
-	};
-
 	ret = rvin_power_on(vin);
 	if (ret < 0)
 		return ret;
 
-	/*
-	 * Try to configure with default parameters. Notice: this is the
-	 * very first open, so, we cannot race against other calls,
-	 * apart from someone else calling open() simultaneously, but
-	 * .host_lock is protecting us against it.
-	 */
-	ret = rvin_s_fmt_vid_cap(file, NULL, &f);
-	if (ret < 0)
-		goto esfmt;
-
 	v4l2_ctrl_handler_setup(&vin->ctrl_handler);
 
 	return 0;
-esfmt:
-	rvin_power_off(vin);
-
-	return ret;
 }
 
 static int rvin_open(struct file *file)
-- 
2.21.0


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v2 5/8] rcar-vin: Move pm_runtime_{get,put} out of helpers
  2019-05-16  1:14 [PATCH v2 0/8] rcar-vin: Merge Gen2 and Gen3 file operations Niklas Söderlund
                   ` (3 preceding siblings ...)
  2019-05-16  1:14 ` [PATCH v2 4/8] rcar-vin: Do not sync subdevice format when opening " Niklas Söderlund
@ 2019-05-16  1:14 ` Niklas Söderlund
  2019-05-16 11:39   ` Laurent Pinchart
  2019-05-16  1:14 ` [PATCH v2 6/8] rcar-vin: Merge helpers dealing with powering the parallel subdevice Niklas Söderlund
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 21+ messages in thread
From: Niklas Söderlund @ 2019-05-16  1:14 UTC (permalink / raw)
  To: Laurent Pinchart, Ulrich Hecht, linux-media
  Cc: linux-renesas-soc, Niklas Söderlund, Ulrich Hecht

The helpers rvin_power_{on,off} deal with both VIN and the parallel
subdevice power. This makes it hard to merge the Gen2 and Gen3
open/release functions. Move the VIN power handling directly to the
open/release functions to prepare for the merge.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 34 +++++++++++++--------
 1 file changed, 21 insertions(+), 13 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 71651c5a69483367..5a9658b7d848fc86 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -754,8 +754,6 @@ static int rvin_power_on(struct rvin_dev *vin)
 	int ret;
 	struct v4l2_subdev *sd = vin_to_source(vin);
 
-	pm_runtime_get_sync(vin->v4l2_dev.dev);
-
 	ret = v4l2_subdev_call(sd, core, s_power, 1);
 	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
 		return ret;
@@ -768,9 +766,6 @@ static int rvin_power_off(struct rvin_dev *vin)
 	struct v4l2_subdev *sd = vin_to_source(vin);
 
 	ret = v4l2_subdev_call(sd, core, s_power, 0);
-
-	pm_runtime_put(vin->v4l2_dev.dev);
-
 	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
 		return ret;
 
@@ -802,20 +797,31 @@ static int rvin_open(struct file *file)
 
 	file->private_data = vin;
 
+	ret = pm_runtime_get_sync(vin->dev);
+	if (ret < 0)
+		goto err_unlock;
+
 	ret = v4l2_fh_open(file);
 	if (ret)
-		goto unlock;
+		goto err_pm;
 
-	if (!v4l2_fh_is_singular_file(file))
-		goto unlock;
-
-	if (rvin_initialize_device(file)) {
-		v4l2_fh_release(file);
-		ret = -ENODEV;
+	if (v4l2_fh_is_singular_file(file)) {
+		if (rvin_initialize_device(file)) {
+			ret = -ENODEV;
+			goto err_open;
+		}
 	}
 
-unlock:
 	mutex_unlock(&vin->lock);
+
+	return 0;
+err_open:
+	v4l2_fh_release(file);
+err_pm:
+	pm_runtime_put(vin->dev);
+err_unlock:
+	mutex_unlock(&vin->lock);
+
 	return ret;
 }
 
@@ -840,6 +846,8 @@ static int rvin_release(struct file *file)
 	if (fh_singular)
 		rvin_power_off(vin);
 
+	pm_runtime_put(vin->dev);
+
 	mutex_unlock(&vin->lock);
 
 	return ret;
-- 
2.21.0


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v2 6/8] rcar-vin: Merge helpers dealing with powering the parallel subdevice
  2019-05-16  1:14 [PATCH v2 0/8] rcar-vin: Merge Gen2 and Gen3 file operations Niklas Söderlund
                   ` (4 preceding siblings ...)
  2019-05-16  1:14 ` [PATCH v2 5/8] rcar-vin: Move pm_runtime_{get,put} out of helpers Niklas Söderlund
@ 2019-05-16  1:14 ` Niklas Söderlund
  2019-05-16  7:40   ` Ulrich Hecht
  2019-05-16 11:44   ` Laurent Pinchart
  2019-05-16  1:14 ` [PATCH v2 7/8] rcar-vin: Fold rvin_initialize_device() into rvin_open() Niklas Söderlund
  2019-05-16  1:14 ` [PATCH v2 8/8] rcar-vin: Merge Gen2 and Gen3 file operations Niklas Söderlund
  7 siblings, 2 replies; 21+ messages in thread
From: Niklas Söderlund @ 2019-05-16  1:14 UTC (permalink / raw)
  To: Laurent Pinchart, Ulrich Hecht, linux-media
  Cc: linux-renesas-soc, Niklas Söderlund

The two power helpers are now only dealing with the parallel subdevice,
merge them into a single rvin_power_parallel() helper to reduce code
duplication.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 5a9658b7d848fc86..7c8ba4b310706ceb 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -749,23 +749,13 @@ static const struct v4l2_ioctl_ops rvin_mc_ioctl_ops = {
  * File Operations
  */
 
-static int rvin_power_on(struct rvin_dev *vin)
+static int rvin_power_parallel(struct rvin_dev *vin, bool on)
 {
-	int ret;
 	struct v4l2_subdev *sd = vin_to_source(vin);
-
-	ret = v4l2_subdev_call(sd, core, s_power, 1);
-	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
-		return ret;
-	return 0;
-}
-
-static int rvin_power_off(struct rvin_dev *vin)
-{
+	int power = on ? 1 : 0;
 	int ret;
-	struct v4l2_subdev *sd = vin_to_source(vin);
 
-	ret = v4l2_subdev_call(sd, core, s_power, 0);
+	ret = v4l2_subdev_call(sd, core, s_power, power);
 	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
 		return ret;
 
@@ -777,7 +767,7 @@ static int rvin_initialize_device(struct file *file)
 	struct rvin_dev *vin = video_drvdata(file);
 	int ret;
 
-	ret = rvin_power_on(vin);
+	ret = rvin_power_parallel(vin, true);
 	if (ret < 0)
 		return ret;
 
@@ -844,7 +834,7 @@ static int rvin_release(struct file *file)
 	 * Then de-initialize hw module.
 	 */
 	if (fh_singular)
-		rvin_power_off(vin);
+		rvin_power_parallel(vin, false);
 
 	pm_runtime_put(vin->dev);
 
-- 
2.21.0


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v2 7/8] rcar-vin: Fold rvin_initialize_device() into rvin_open()
  2019-05-16  1:14 [PATCH v2 0/8] rcar-vin: Merge Gen2 and Gen3 file operations Niklas Söderlund
                   ` (5 preceding siblings ...)
  2019-05-16  1:14 ` [PATCH v2 6/8] rcar-vin: Merge helpers dealing with powering the parallel subdevice Niklas Söderlund
@ 2019-05-16  1:14 ` Niklas Söderlund
  2019-05-16 11:45   ` Laurent Pinchart
  2019-05-16  1:14 ` [PATCH v2 8/8] rcar-vin: Merge Gen2 and Gen3 file operations Niklas Söderlund
  7 siblings, 1 reply; 21+ messages in thread
From: Niklas Söderlund @ 2019-05-16  1:14 UTC (permalink / raw)
  To: Laurent Pinchart, Ulrich Hecht, linux-media
  Cc: linux-renesas-soc, Niklas Söderlund, Ulrich Hecht

The function no longer serve a purpose as most tasks it performed have
been refactored, fold what remains of it into the only caller.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 21 ++++-----------------
 1 file changed, 4 insertions(+), 17 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 7c8ba4b310706ceb..169639416121f204 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -762,20 +762,6 @@ static int rvin_power_parallel(struct rvin_dev *vin, bool on)
 	return 0;
 }
 
-static int rvin_initialize_device(struct file *file)
-{
-	struct rvin_dev *vin = video_drvdata(file);
-	int ret;
-
-	ret = rvin_power_parallel(vin, true);
-	if (ret < 0)
-		return ret;
-
-	v4l2_ctrl_handler_setup(&vin->ctrl_handler);
-
-	return 0;
-}
-
 static int rvin_open(struct file *file)
 {
 	struct rvin_dev *vin = video_drvdata(file);
@@ -796,10 +782,11 @@ static int rvin_open(struct file *file)
 		goto err_pm;
 
 	if (v4l2_fh_is_singular_file(file)) {
-		if (rvin_initialize_device(file)) {
-			ret = -ENODEV;
+		ret = rvin_power_parallel(vin, true);
+		if (ret < 0)
 			goto err_open;
-		}
+
+		v4l2_ctrl_handler_setup(&vin->ctrl_handler);
 	}
 
 	mutex_unlock(&vin->lock);
-- 
2.21.0


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH v2 8/8] rcar-vin: Merge Gen2 and Gen3 file operations
  2019-05-16  1:14 [PATCH v2 0/8] rcar-vin: Merge Gen2 and Gen3 file operations Niklas Söderlund
                   ` (6 preceding siblings ...)
  2019-05-16  1:14 ` [PATCH v2 7/8] rcar-vin: Fold rvin_initialize_device() into rvin_open() Niklas Söderlund
@ 2019-05-16  1:14 ` Niklas Söderlund
  2019-05-16 11:48   ` Laurent Pinchart
  7 siblings, 1 reply; 21+ messages in thread
From: Niklas Söderlund @ 2019-05-16  1:14 UTC (permalink / raw)
  To: Laurent Pinchart, Ulrich Hecht, linux-media
  Cc: linux-renesas-soc, Niklas Söderlund, Ulrich Hecht

After the rework of the Gen2 file operations it's now trivial to merge
the Gen2 and Gen3 versions.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
---
 drivers/media/platform/rcar-vin/rcar-v4l2.c | 96 ++++-----------------
 1 file changed, 16 insertions(+), 80 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
index 169639416121f204..8e4afa4278fe9d30 100644
--- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
+++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
@@ -781,14 +781,19 @@ static int rvin_open(struct file *file)
 	if (ret)
 		goto err_pm;
 
-	if (v4l2_fh_is_singular_file(file)) {
-		ret = rvin_power_parallel(vin, true);
+	if (vin->info->use_mc) {
+		ret = v4l2_pipeline_pm_use(&vin->vdev.entity, 1);
 		if (ret < 0)
 			goto err_open;
+	} else {
+		if (v4l2_fh_is_singular_file(file)) {
+			ret = rvin_power_parallel(vin, true);
+			if (ret < 0)
+				goto err_open;
 
-		v4l2_ctrl_handler_setup(&vin->ctrl_handler);
+			v4l2_ctrl_handler_setup(&vin->ctrl_handler);
+		}
 	}
-
 	mutex_unlock(&vin->lock);
 
 	return 0;
@@ -816,12 +821,12 @@ static int rvin_release(struct file *file)
 	/* the release helper will cleanup any on-going streaming */
 	ret = _vb2_fop_release(file, NULL);
 
-	/*
-	 * If this was the last open file.
-	 * Then de-initialize hw module.
-	 */
-	if (fh_singular)
-		rvin_power_parallel(vin, false);
+	if (vin->info->use_mc) {
+		v4l2_pipeline_pm_use(&vin->vdev.entity, 0);
+	} else {
+		if (fh_singular)
+			rvin_power_parallel(vin, false);
+	}
 
 	pm_runtime_put(vin->dev);
 
@@ -840,74 +845,6 @@ static const struct v4l2_file_operations rvin_fops = {
 	.read		= vb2_fop_read,
 };
 
-/* -----------------------------------------------------------------------------
- * Media controller file operations
- */
-
-static int rvin_mc_open(struct file *file)
-{
-	struct rvin_dev *vin = video_drvdata(file);
-	int ret;
-
-	ret = mutex_lock_interruptible(&vin->lock);
-	if (ret)
-		return ret;
-
-	ret = pm_runtime_get_sync(vin->dev);
-	if (ret < 0)
-		goto err_unlock;
-
-	ret = v4l2_pipeline_pm_use(&vin->vdev.entity, 1);
-	if (ret < 0)
-		goto err_pm;
-
-	file->private_data = vin;
-
-	ret = v4l2_fh_open(file);
-	if (ret)
-		goto err_v4l2pm;
-
-	mutex_unlock(&vin->lock);
-
-	return 0;
-err_v4l2pm:
-	v4l2_pipeline_pm_use(&vin->vdev.entity, 0);
-err_pm:
-	pm_runtime_put(vin->dev);
-err_unlock:
-	mutex_unlock(&vin->lock);
-
-	return ret;
-}
-
-static int rvin_mc_release(struct file *file)
-{
-	struct rvin_dev *vin = video_drvdata(file);
-	int ret;
-
-	mutex_lock(&vin->lock);
-
-	/* the release helper will cleanup any on-going streaming. */
-	ret = _vb2_fop_release(file, NULL);
-
-	v4l2_pipeline_pm_use(&vin->vdev.entity, 0);
-	pm_runtime_put(vin->dev);
-
-	mutex_unlock(&vin->lock);
-
-	return ret;
-}
-
-static const struct v4l2_file_operations rvin_mc_fops = {
-	.owner		= THIS_MODULE,
-	.unlocked_ioctl	= video_ioctl2,
-	.open		= rvin_mc_open,
-	.release	= rvin_mc_release,
-	.poll		= vb2_fop_poll,
-	.mmap		= vb2_fop_mmap,
-	.read		= vb2_fop_read,
-};
-
 void rvin_v4l2_unregister(struct rvin_dev *vin)
 {
 	if (!video_is_registered(&vin->vdev))
@@ -948,6 +885,7 @@ int rvin_v4l2_register(struct rvin_dev *vin)
 	snprintf(vdev->name, sizeof(vdev->name), "VIN%u output", vin->id);
 	vdev->release = video_device_release_empty;
 	vdev->lock = &vin->lock;
+	vdev->fops = &rvin_fops;
 	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING |
 		V4L2_CAP_READWRITE;
 
@@ -959,10 +897,8 @@ int rvin_v4l2_register(struct rvin_dev *vin)
 	vin->format.colorspace = RVIN_DEFAULT_COLORSPACE;
 
 	if (vin->info->use_mc) {
-		vdev->fops = &rvin_mc_fops;
 		vdev->ioctl_ops = &rvin_mc_ioctl_ops;
 	} else {
-		vdev->fops = &rvin_fops;
 		vdev->ioctl_ops = &rvin_ioctl_ops;
 		rvin_reset_format(vin);
 	}
-- 
2.21.0


^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/8] rcar-vin: Do not call pm_runtime_{resume,suspend}()
  2019-05-16  1:14 ` [PATCH v2 1/8] rcar-vin: Do not call pm_runtime_{resume,suspend}() Niklas Söderlund
@ 2019-05-16  7:23   ` Geert Uytterhoeven
  2019-05-16 11:32     ` Laurent Pinchart
  0 siblings, 1 reply; 21+ messages in thread
From: Geert Uytterhoeven @ 2019-05-16  7:23 UTC (permalink / raw)
  To: Niklas Söderlund
  Cc: Laurent Pinchart, Ulrich Hecht, Linux Media Mailing List,
	Linux-Renesas, Ulrich Hecht

Hi Niklas,

On Thu, May 16, 2019 at 3:46 AM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> The driver does not implement runtime resume and suspend function so
> there is little point in trying to call them. This is a leftover from
> the drivers soc_camera beginnings.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 5 -----
>  1 file changed, 5 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 7cbdcbf9b090c638..b821ea01786eb1ff 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -798,9 +798,6 @@ static int rvin_initialize_device(struct file *file)
>                 return ret;
>
>         pm_runtime_enable(&vin->vdev.dev);
> -       ret = pm_runtime_resume(&vin->vdev.dev);

Please pardon my ignorance, but which device is vin->vdev.dev?
Who calls pm_runtime_get_sync() on it, and where?

I see this function calls rvin_power_on(vin->v4l2_dev.dev) (before the
call to pm_runtime_enable()), but presumably that's a different device?

Thanks!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 2/8] rcar-vin: Remove unneeded calls to pm_runtime_{enable,disable}
  2019-05-16  1:14 ` [PATCH v2 2/8] rcar-vin: Remove unneeded calls to pm_runtime_{enable,disable} Niklas Söderlund
@ 2019-05-16  7:27   ` Geert Uytterhoeven
  2019-05-16 11:34     ` Laurent Pinchart
  0 siblings, 1 reply; 21+ messages in thread
From: Geert Uytterhoeven @ 2019-05-16  7:27 UTC (permalink / raw)
  To: Niklas Söderlund
  Cc: Laurent Pinchart, Ulrich Hecht, Linux Media Mailing List,
	Linux-Renesas, Ulrich Hecht

Hi Niklas,

On Thu, May 16, 2019 at 3:49 AM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> Runtime PM is already enabled unconditionally when the driver is probed
> and disabled when it's removed. There is no point in doing it again for
> Gen2 when opening and closing the video device.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index b821ea01786eb1ff..0841f1a0bfd7ba3a 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -797,8 +797,6 @@ static int rvin_initialize_device(struct file *file)
>         if (ret < 0)
>                 return ret;
>
> -       pm_runtime_enable(&vin->vdev.dev);

Ah, this already (partly) answers my question on patch 1/8.

> -
>         /*
>          * Try to configure with default parameters. Notice: this is the
>          * very first open, so, we cannot race against other calls,

Gr{oetje,eeting}s,

                        Geert


--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 6/8] rcar-vin: Merge helpers dealing with powering the parallel subdevice
  2019-05-16  1:14 ` [PATCH v2 6/8] rcar-vin: Merge helpers dealing with powering the parallel subdevice Niklas Söderlund
@ 2019-05-16  7:40   ` Ulrich Hecht
  2019-05-16 11:44   ` Laurent Pinchart
  1 sibling, 0 replies; 21+ messages in thread
From: Ulrich Hecht @ 2019-05-16  7:40 UTC (permalink / raw)
  To: Niklas Söderlund, Laurent Pinchart, linux-media; +Cc: linux-renesas-soc


> On May 16, 2019 at 3:14 AM Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> wrote:
> 
> 
> The two power helpers are now only dealing with the parallel subdevice,
> merge them into a single rvin_power_parallel() helper to reduce code
> duplication.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 20 +++++---------------
>  1 file changed, 5 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 5a9658b7d848fc86..7c8ba4b310706ceb 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -749,23 +749,13 @@ static const struct v4l2_ioctl_ops rvin_mc_ioctl_ops = {
>   * File Operations
>   */
>  
> -static int rvin_power_on(struct rvin_dev *vin)
> +static int rvin_power_parallel(struct rvin_dev *vin, bool on)
>  {
> -	int ret;
>  	struct v4l2_subdev *sd = vin_to_source(vin);
> -
> -	ret = v4l2_subdev_call(sd, core, s_power, 1);
> -	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
> -		return ret;
> -	return 0;
> -}
> -
> -static int rvin_power_off(struct rvin_dev *vin)
> -{
> +	int power = on ? 1 : 0;
>  	int ret;
> -	struct v4l2_subdev *sd = vin_to_source(vin);
>  
> -	ret = v4l2_subdev_call(sd, core, s_power, 0);
> +	ret = v4l2_subdev_call(sd, core, s_power, power);
>  	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
>  		return ret;
>  
> @@ -777,7 +767,7 @@ static int rvin_initialize_device(struct file *file)
>  	struct rvin_dev *vin = video_drvdata(file);
>  	int ret;
>  
> -	ret = rvin_power_on(vin);
> +	ret = rvin_power_parallel(vin, true);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -844,7 +834,7 @@ static int rvin_release(struct file *file)
>  	 * Then de-initialize hw module.
>  	 */
>  	if (fh_singular)
> -		rvin_power_off(vin);
> +		rvin_power_parallel(vin, false);
>  
>  	pm_runtime_put(vin->dev);
>  
> -- 
> 2.21.0
>

Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

CU
Uli

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 3/8] rcar-vin: Allow interrupting lock when trying to open the video device
  2019-05-16  1:14 ` [PATCH v2 3/8] rcar-vin: Allow interrupting lock when trying to open the video device Niklas Söderlund
@ 2019-05-16 11:21   ` Laurent Pinchart
  0 siblings, 0 replies; 21+ messages in thread
From: Laurent Pinchart @ 2019-05-16 11:21 UTC (permalink / raw)
  To: Niklas Söderlund
  Cc: Ulrich Hecht, linux-media, linux-renesas-soc, Ulrich Hecht

Hi Niklas,

Thank you for the patch.

On Thu, May 16, 2019 at 03:14:12AM +0200, Niklas Söderlund wrote:
> The user should be allowed to break waiting for the lock when opening
> the video device.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 0841f1a0bfd7ba3a..f67cef97b89a3bd4 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -821,7 +821,9 @@ static int rvin_open(struct file *file)
>  	struct rvin_dev *vin = video_drvdata(file);
>  	int ret;
>  
> -	mutex_lock(&vin->lock);
> +	ret = mutex_lock_interruptible(&vin->lock);
> +	if (ret)
> +		return ret;
>  
>  	file->private_data = vin;
>  

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 4/8] rcar-vin: Do not sync subdevice format when opening the video device
  2019-05-16  1:14 ` [PATCH v2 4/8] rcar-vin: Do not sync subdevice format when opening " Niklas Söderlund
@ 2019-05-16 11:26   ` Laurent Pinchart
  2019-05-16 13:25     ` Niklas Söderlund
  0 siblings, 1 reply; 21+ messages in thread
From: Laurent Pinchart @ 2019-05-16 11:26 UTC (permalink / raw)
  To: Niklas Söderlund
  Cc: Ulrich Hecht, linux-media, linux-renesas-soc, Ulrich Hecht

Hi Niklas,

Thank you for the patch.

On Thu, May 16, 2019 at 03:14:13AM +0200, Niklas Söderlund wrote:
> The format is already synced when the subdevice is bound, there is no
> need to do do it every time the video device is opened.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>

This seems a bug fix to me, formats should not be modified at open()
time according to the V4L2 spec. You may want to add a Fixes: line,
although I suppose this would go back to the origins of the driver, and
this series is likely not a candidate for the stable tree, so it may not
be a good idea.

I'm slightly worried of side effects as rvin_s_fmt_vid_cap() calls
rvin_try_format() which in turn calls v4l2_subdev_call(sd, pad, set_fmt,
pad_cfg, &format). If you're confident that there's no risk of breakage,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 25 ---------------------
>  1 file changed, 25 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index f67cef97b89a3bd4..71651c5a69483367 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -782,38 +782,13 @@ static int rvin_initialize_device(struct file *file)
>  	struct rvin_dev *vin = video_drvdata(file);
>  	int ret;
>  
> -	struct v4l2_format f = {
> -		.type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
> -		.fmt.pix = {
> -			.width		= vin->format.width,
> -			.height		= vin->format.height,
> -			.field		= vin->format.field,
> -			.colorspace	= vin->format.colorspace,
> -			.pixelformat	= vin->format.pixelformat,
> -		},
> -	};
> -
>  	ret = rvin_power_on(vin);
>  	if (ret < 0)
>  		return ret;
>  
> -	/*
> -	 * Try to configure with default parameters. Notice: this is the
> -	 * very first open, so, we cannot race against other calls,
> -	 * apart from someone else calling open() simultaneously, but
> -	 * .host_lock is protecting us against it.
> -	 */
> -	ret = rvin_s_fmt_vid_cap(file, NULL, &f);
> -	if (ret < 0)
> -		goto esfmt;
> -
>  	v4l2_ctrl_handler_setup(&vin->ctrl_handler);
>  
>  	return 0;
> -esfmt:
> -	rvin_power_off(vin);
> -
> -	return ret;
>  }
>  
>  static int rvin_open(struct file *file)

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 1/8] rcar-vin: Do not call pm_runtime_{resume,suspend}()
  2019-05-16  7:23   ` Geert Uytterhoeven
@ 2019-05-16 11:32     ` Laurent Pinchart
  0 siblings, 0 replies; 21+ messages in thread
From: Laurent Pinchart @ 2019-05-16 11:32 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Niklas Söderlund, Ulrich Hecht, Linux Media Mailing List,
	Linux-Renesas, Ulrich Hecht

On Thu, May 16, 2019 at 09:23:41AM +0200, Geert Uytterhoeven wrote:
> On Thu, May 16, 2019 at 3:46 AM Niklas Söderlund wrote:
> > The driver does not implement runtime resume and suspend function so
> > there is little point in trying to call them. This is a leftover from
> > the drivers soc_camera beginnings.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-v4l2.c | 5 -----
> >  1 file changed, 5 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > index 7cbdcbf9b090c638..b821ea01786eb1ff 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -798,9 +798,6 @@ static int rvin_initialize_device(struct file *file)
> >                 return ret;
> >
> >         pm_runtime_enable(&vin->vdev.dev);
> > -       ret = pm_runtime_resume(&vin->vdev.dev);
> 
> Please pardon my ignorance, but which device is vin->vdev.dev?

This is the V4L2 video node class device, we shouldn't call any runtime
PM function with this. I assume the current code works because the class
device is a child of the physical device.

> Who calls pm_runtime_get_sync() on it, and where?
> 
> I see this function calls rvin_power_on(vin->v4l2_dev.dev) (before the
> call to pm_runtime_enable()), but presumably that's a different device?

That one is the physical platform device.

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 2/8] rcar-vin: Remove unneeded calls to pm_runtime_{enable,disable}
  2019-05-16  7:27   ` Geert Uytterhoeven
@ 2019-05-16 11:34     ` Laurent Pinchart
  0 siblings, 0 replies; 21+ messages in thread
From: Laurent Pinchart @ 2019-05-16 11:34 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Niklas Söderlund, Ulrich Hecht, Linux Media Mailing List,
	Linux-Renesas, Ulrich Hecht

On Thu, May 16, 2019 at 09:27:03AM +0200, Geert Uytterhoeven wrote:
> On Thu, May 16, 2019 at 3:49 AM Niklas Söderlund wrote:
> > Runtime PM is already enabled unconditionally when the driver is probed
> > and disabled when it's removed. There is no point in doing it again for
> > Gen2 when opening and closing the video device.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-v4l2.c | 7 +------
> >  1 file changed, 1 insertion(+), 6 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > index b821ea01786eb1ff..0841f1a0bfd7ba3a 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -797,8 +797,6 @@ static int rvin_initialize_device(struct file *file)
> >         if (ret < 0)
> >                 return ret;
> >
> > -       pm_runtime_enable(&vin->vdev.dev);
> 
> Ah, this already (partly) answers my question on patch 1/8.

Note that those are two different devices, here we enable runtime PM in
the V4L2 video node class device, while at probe time we enable it on
the platform device. I agree that this call should go, but that's only
because all runtime PM calls on the class device should go :-)

> 
> > -
> >         /*
> >          * Try to configure with default parameters. Notice: this is the
> >          * very first open, so, we cannot race against other calls,

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 5/8] rcar-vin: Move pm_runtime_{get,put} out of helpers
  2019-05-16  1:14 ` [PATCH v2 5/8] rcar-vin: Move pm_runtime_{get,put} out of helpers Niklas Söderlund
@ 2019-05-16 11:39   ` Laurent Pinchart
  0 siblings, 0 replies; 21+ messages in thread
From: Laurent Pinchart @ 2019-05-16 11:39 UTC (permalink / raw)
  To: Niklas Söderlund
  Cc: Ulrich Hecht, linux-media, linux-renesas-soc, Ulrich Hecht

Hi Niklas,

Thank you for the patch.

On Thu, May 16, 2019 at 03:14:14AM +0200, Niklas Söderlund wrote:
> The helpers rvin_power_{on,off} deal with both VIN and the parallel
> subdevice power. This makes it hard to merge the Gen2 and Gen3
> open/release functions. Move the VIN power handling directly to the
> open/release functions to prepare for the merge.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 34 +++++++++++++--------
>  1 file changed, 21 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 71651c5a69483367..5a9658b7d848fc86 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -754,8 +754,6 @@ static int rvin_power_on(struct rvin_dev *vin)
>  	int ret;
>  	struct v4l2_subdev *sd = vin_to_source(vin);
>  
> -	pm_runtime_get_sync(vin->v4l2_dev.dev);
> -
>  	ret = v4l2_subdev_call(sd, core, s_power, 1);
>  	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
>  		return ret;
> @@ -768,9 +766,6 @@ static int rvin_power_off(struct rvin_dev *vin)
>  	struct v4l2_subdev *sd = vin_to_source(vin);
>  
>  	ret = v4l2_subdev_call(sd, core, s_power, 0);
> -
> -	pm_runtime_put(vin->v4l2_dev.dev);
> -
>  	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
>  		return ret;
>  
> @@ -802,20 +797,31 @@ static int rvin_open(struct file *file)
>  
>  	file->private_data = vin;
>  
> +	ret = pm_runtime_get_sync(vin->dev);

I like operating on vin->dev much more than on vin->v4l2_dev.dev, even
if the two point to the same platform device.

> +	if (ret < 0)
> +		goto err_unlock;

Would it be useful to do this before locking the mutex, as PM runtime
doesn't need to be protected by that lock, to minimise the time spent
with the mutex held ?

> +
>  	ret = v4l2_fh_open(file);
>  	if (ret)
> -		goto unlock;
> +		goto err_pm;
>  
> -	if (!v4l2_fh_is_singular_file(file))
> -		goto unlock;
> -
> -	if (rvin_initialize_device(file)) {
> -		v4l2_fh_release(file);
> -		ret = -ENODEV;
> +	if (v4l2_fh_is_singular_file(file)) {
> +		if (rvin_initialize_device(file)) {
> +			ret = -ENODEV;

Should you propagate the return value of rvin_initialize_device ?

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +			goto err_open;
> +		}
>  	}
>  
> -unlock:
>  	mutex_unlock(&vin->lock);
> +
> +	return 0;
> +err_open:
> +	v4l2_fh_release(file);
> +err_pm:
> +	pm_runtime_put(vin->dev);
> +err_unlock:
> +	mutex_unlock(&vin->lock);
> +
>  	return ret;
>  }
>  
> @@ -840,6 +846,8 @@ static int rvin_release(struct file *file)
>  	if (fh_singular)
>  		rvin_power_off(vin);
>  
> +	pm_runtime_put(vin->dev);
> +

This one could also be moved after mutex_unlock().

>  	mutex_unlock(&vin->lock);
>  
>  	return ret;

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 6/8] rcar-vin: Merge helpers dealing with powering the parallel subdevice
  2019-05-16  1:14 ` [PATCH v2 6/8] rcar-vin: Merge helpers dealing with powering the parallel subdevice Niklas Söderlund
  2019-05-16  7:40   ` Ulrich Hecht
@ 2019-05-16 11:44   ` Laurent Pinchart
  1 sibling, 0 replies; 21+ messages in thread
From: Laurent Pinchart @ 2019-05-16 11:44 UTC (permalink / raw)
  To: Niklas Söderlund; +Cc: Ulrich Hecht, linux-media, linux-renesas-soc

Hi Niklas,

Thank you for the patch.

On Thu, May 16, 2019 at 03:14:15AM +0200, Niklas Söderlund wrote:
> The two power helpers are now only dealing with the parallel subdevice,
> merge them into a single rvin_power_parallel() helper to reduce code
> duplication.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 20 +++++---------------
>  1 file changed, 5 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 5a9658b7d848fc86..7c8ba4b310706ceb 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -749,23 +749,13 @@ static const struct v4l2_ioctl_ops rvin_mc_ioctl_ops = {
>   * File Operations
>   */
>  
> -static int rvin_power_on(struct rvin_dev *vin)
> +static int rvin_power_parallel(struct rvin_dev *vin, bool on)
>  {
> -	int ret;
>  	struct v4l2_subdev *sd = vin_to_source(vin);
> -
> -	ret = v4l2_subdev_call(sd, core, s_power, 1);
> -	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
> -		return ret;
> -	return 0;
> -}
> -
> -static int rvin_power_off(struct rvin_dev *vin)
> -{
> +	int power = on ? 1 : 0;

You could use on directly instead of going through a separate variable.

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

>  	int ret;
> -	struct v4l2_subdev *sd = vin_to_source(vin);
>  
> -	ret = v4l2_subdev_call(sd, core, s_power, 0);
> +	ret = v4l2_subdev_call(sd, core, s_power, power);
>  	if (ret < 0 && ret != -ENOIOCTLCMD && ret != -ENODEV)
>  		return ret;
>  
> @@ -777,7 +767,7 @@ static int rvin_initialize_device(struct file *file)
>  	struct rvin_dev *vin = video_drvdata(file);
>  	int ret;
>  
> -	ret = rvin_power_on(vin);
> +	ret = rvin_power_parallel(vin, true);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -844,7 +834,7 @@ static int rvin_release(struct file *file)
>  	 * Then de-initialize hw module.
>  	 */
>  	if (fh_singular)
> -		rvin_power_off(vin);
> +		rvin_power_parallel(vin, false);
>  
>  	pm_runtime_put(vin->dev);
>  

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 7/8] rcar-vin: Fold rvin_initialize_device() into rvin_open()
  2019-05-16  1:14 ` [PATCH v2 7/8] rcar-vin: Fold rvin_initialize_device() into rvin_open() Niklas Söderlund
@ 2019-05-16 11:45   ` Laurent Pinchart
  0 siblings, 0 replies; 21+ messages in thread
From: Laurent Pinchart @ 2019-05-16 11:45 UTC (permalink / raw)
  To: Niklas Söderlund
  Cc: Ulrich Hecht, linux-media, linux-renesas-soc, Ulrich Hecht

Hi Niklas,

Thank you for the patch.

On Thu, May 16, 2019 at 03:14:16AM +0200, Niklas Söderlund wrote:
> The function no longer serve a purpose as most tasks it performed have
> been refactored, fold what remains of it into the only caller.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 21 ++++-----------------
>  1 file changed, 4 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 7c8ba4b310706ceb..169639416121f204 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -762,20 +762,6 @@ static int rvin_power_parallel(struct rvin_dev *vin, bool on)
>  	return 0;
>  }
>  
> -static int rvin_initialize_device(struct file *file)
> -{
> -	struct rvin_dev *vin = video_drvdata(file);
> -	int ret;
> -
> -	ret = rvin_power_parallel(vin, true);
> -	if (ret < 0)
> -		return ret;
> -
> -	v4l2_ctrl_handler_setup(&vin->ctrl_handler);
> -
> -	return 0;
> -}
> -
>  static int rvin_open(struct file *file)
>  {
>  	struct rvin_dev *vin = video_drvdata(file);
> @@ -796,10 +782,11 @@ static int rvin_open(struct file *file)
>  		goto err_pm;
>  
>  	if (v4l2_fh_is_singular_file(file)) {
> -		if (rvin_initialize_device(file)) {
> -			ret = -ENODEV;
> +		ret = rvin_power_parallel(vin, true);
> +		if (ret < 0)
>  			goto err_open;
> -		}
> +
> +		v4l2_ctrl_handler_setup(&vin->ctrl_handler);

I think you should handle errors here.

>  	}
>  
>  	mutex_unlock(&vin->lock);

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 8/8] rcar-vin: Merge Gen2 and Gen3 file operations
  2019-05-16  1:14 ` [PATCH v2 8/8] rcar-vin: Merge Gen2 and Gen3 file operations Niklas Söderlund
@ 2019-05-16 11:48   ` Laurent Pinchart
  0 siblings, 0 replies; 21+ messages in thread
From: Laurent Pinchart @ 2019-05-16 11:48 UTC (permalink / raw)
  To: Niklas Söderlund
  Cc: Ulrich Hecht, linux-media, linux-renesas-soc, Ulrich Hecht

Hi Niklas,

Thank you for the patch.

On Thu, May 16, 2019 at 03:14:17AM +0200, Niklas Söderlund wrote:
> After the rework of the Gen2 file operations it's now trivial to merge
> the Gen2 and Gen3 versions.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
> ---
>  drivers/media/platform/rcar-vin/rcar-v4l2.c | 96 ++++-----------------
>  1 file changed, 16 insertions(+), 80 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> index 169639416121f204..8e4afa4278fe9d30 100644
> --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> @@ -781,14 +781,19 @@ static int rvin_open(struct file *file)
>  	if (ret)
>  		goto err_pm;
>  
> -	if (v4l2_fh_is_singular_file(file)) {
> -		ret = rvin_power_parallel(vin, true);
> +	if (vin->info->use_mc) {
> +		ret = v4l2_pipeline_pm_use(&vin->vdev.entity, 1);
>  		if (ret < 0)
>  			goto err_open;
> +	} else {
> +		if (v4l2_fh_is_singular_file(file)) {
> +			ret = rvin_power_parallel(vin, true);
> +			if (ret < 0)
> +				goto err_open;
>  
> -		v4l2_ctrl_handler_setup(&vin->ctrl_handler);
> +			v4l2_ctrl_handler_setup(&vin->ctrl_handler);
> +		}
>  	}

I wonder if you shouldn't abstract the first open init and last close
cleanup operations in separate functions, with MC and non-MC variants,
as you will need to handle the v4l2_ctrl_handler_setup() failure here,
making error handling more complex.

> -
>  	mutex_unlock(&vin->lock);
>  
>  	return 0;
> @@ -816,12 +821,12 @@ static int rvin_release(struct file *file)
>  	/* the release helper will cleanup any on-going streaming */
>  	ret = _vb2_fop_release(file, NULL);
>  
> -	/*
> -	 * If this was the last open file.
> -	 * Then de-initialize hw module.
> -	 */
> -	if (fh_singular)
> -		rvin_power_parallel(vin, false);
> +	if (vin->info->use_mc) {
> +		v4l2_pipeline_pm_use(&vin->vdev.entity, 0);
> +	} else {
> +		if (fh_singular)
> +			rvin_power_parallel(vin, false);
> +	}
>  
>  	pm_runtime_put(vin->dev);
>  
> @@ -840,74 +845,6 @@ static const struct v4l2_file_operations rvin_fops = {
>  	.read		= vb2_fop_read,
>  };
>  
> -/* -----------------------------------------------------------------------------
> - * Media controller file operations
> - */
> -
> -static int rvin_mc_open(struct file *file)
> -{
> -	struct rvin_dev *vin = video_drvdata(file);
> -	int ret;
> -
> -	ret = mutex_lock_interruptible(&vin->lock);
> -	if (ret)
> -		return ret;
> -
> -	ret = pm_runtime_get_sync(vin->dev);
> -	if (ret < 0)
> -		goto err_unlock;
> -
> -	ret = v4l2_pipeline_pm_use(&vin->vdev.entity, 1);
> -	if (ret < 0)
> -		goto err_pm;
> -
> -	file->private_data = vin;
> -
> -	ret = v4l2_fh_open(file);
> -	if (ret)
> -		goto err_v4l2pm;
> -
> -	mutex_unlock(&vin->lock);
> -
> -	return 0;
> -err_v4l2pm:
> -	v4l2_pipeline_pm_use(&vin->vdev.entity, 0);
> -err_pm:
> -	pm_runtime_put(vin->dev);
> -err_unlock:
> -	mutex_unlock(&vin->lock);
> -
> -	return ret;
> -}
> -
> -static int rvin_mc_release(struct file *file)
> -{
> -	struct rvin_dev *vin = video_drvdata(file);
> -	int ret;
> -
> -	mutex_lock(&vin->lock);
> -
> -	/* the release helper will cleanup any on-going streaming. */
> -	ret = _vb2_fop_release(file, NULL);
> -
> -	v4l2_pipeline_pm_use(&vin->vdev.entity, 0);
> -	pm_runtime_put(vin->dev);
> -
> -	mutex_unlock(&vin->lock);
> -
> -	return ret;
> -}
> -
> -static const struct v4l2_file_operations rvin_mc_fops = {
> -	.owner		= THIS_MODULE,
> -	.unlocked_ioctl	= video_ioctl2,
> -	.open		= rvin_mc_open,
> -	.release	= rvin_mc_release,
> -	.poll		= vb2_fop_poll,
> -	.mmap		= vb2_fop_mmap,
> -	.read		= vb2_fop_read,
> -};
> -
>  void rvin_v4l2_unregister(struct rvin_dev *vin)
>  {
>  	if (!video_is_registered(&vin->vdev))
> @@ -948,6 +885,7 @@ int rvin_v4l2_register(struct rvin_dev *vin)
>  	snprintf(vdev->name, sizeof(vdev->name), "VIN%u output", vin->id);
>  	vdev->release = video_device_release_empty;
>  	vdev->lock = &vin->lock;
> +	vdev->fops = &rvin_fops;
>  	vdev->device_caps = V4L2_CAP_VIDEO_CAPTURE | V4L2_CAP_STREAMING |
>  		V4L2_CAP_READWRITE;
>  
> @@ -959,10 +897,8 @@ int rvin_v4l2_register(struct rvin_dev *vin)
>  	vin->format.colorspace = RVIN_DEFAULT_COLORSPACE;
>  
>  	if (vin->info->use_mc) {
> -		vdev->fops = &rvin_mc_fops;
>  		vdev->ioctl_ops = &rvin_mc_ioctl_ops;
>  	} else {
> -		vdev->fops = &rvin_fops;
>  		vdev->ioctl_ops = &rvin_ioctl_ops;
>  		rvin_reset_format(vin);
>  	}

-- 
Regards,

Laurent Pinchart

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH v2 4/8] rcar-vin: Do not sync subdevice format when opening the video device
  2019-05-16 11:26   ` Laurent Pinchart
@ 2019-05-16 13:25     ` Niklas Söderlund
  0 siblings, 0 replies; 21+ messages in thread
From: Niklas Söderlund @ 2019-05-16 13:25 UTC (permalink / raw)
  To: Laurent Pinchart
  Cc: Ulrich Hecht, linux-media, linux-renesas-soc, Ulrich Hecht

Hi Laurent,

Thanks for your feedback.

On 2019-05-16 14:26:44 +0300, Laurent Pinchart wrote:
> Hi Niklas,
> 
> Thank you for the patch.
> 
> On Thu, May 16, 2019 at 03:14:13AM +0200, Niklas Söderlund wrote:
> > The format is already synced when the subdevice is bound, there is no
> > need to do do it every time the video device is opened.
> > 
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > Reviewed-by: Ulrich Hecht <uli+renesas@fpond.eu>
> 
> This seems a bug fix to me, formats should not be modified at open()
> time according to the V4L2 spec. You may want to add a Fixes: line,
> although I suppose this would go back to the origins of the driver, and
> this series is likely not a candidate for the stable tree, so it may not
> be a good idea.

I agree, a fixes tag could have been useful. But as you deduce this go 
way back to the beginning pre Gen3 support and I don't think it would 
backport easily. As it's not critical I'm fine moving forward without 
such a tag.

> 
> I'm slightly worried of side effects as rvin_s_fmt_vid_cap() calls
> rvin_try_format() which in turn calls v4l2_subdev_call(sd, pad, set_fmt,
> pad_cfg, &format). If you're confident that there's no risk of breakage,

I was worried to, but then I concluded the risk for breakage is the same 
with or without the call to v4l2_subdev_call(..., set_fmt, ...) at bound 
time. The breakage is if the video source format is changed between 
bound to stream on without the user setting the format. The same risk 
exists between open() and stream on before this patch. The user should 
always set the format before it starts to stream. And once I'm daring 
enough to drop the devnode centric support from rcar-vin this will go 
away entirely.

> 
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> 
> > ---
> >  drivers/media/platform/rcar-vin/rcar-v4l2.c | 25 ---------------------
> >  1 file changed, 25 deletions(-)
> > 
> > diff --git a/drivers/media/platform/rcar-vin/rcar-v4l2.c b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > index f67cef97b89a3bd4..71651c5a69483367 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-v4l2.c
> > @@ -782,38 +782,13 @@ static int rvin_initialize_device(struct file *file)
> >  	struct rvin_dev *vin = video_drvdata(file);
> >  	int ret;
> >  
> > -	struct v4l2_format f = {
> > -		.type = V4L2_BUF_TYPE_VIDEO_CAPTURE,
> > -		.fmt.pix = {
> > -			.width		= vin->format.width,
> > -			.height		= vin->format.height,
> > -			.field		= vin->format.field,
> > -			.colorspace	= vin->format.colorspace,
> > -			.pixelformat	= vin->format.pixelformat,
> > -		},
> > -	};
> > -
> >  	ret = rvin_power_on(vin);
> >  	if (ret < 0)
> >  		return ret;
> >  
> > -	/*
> > -	 * Try to configure with default parameters. Notice: this is the
> > -	 * very first open, so, we cannot race against other calls,
> > -	 * apart from someone else calling open() simultaneously, but
> > -	 * .host_lock is protecting us against it.
> > -	 */
> > -	ret = rvin_s_fmt_vid_cap(file, NULL, &f);
> > -	if (ret < 0)
> > -		goto esfmt;
> > -
> >  	v4l2_ctrl_handler_setup(&vin->ctrl_handler);
> >  
> >  	return 0;
> > -esfmt:
> > -	rvin_power_off(vin);
> > -
> > -	return ret;
> >  }
> >  
> >  static int rvin_open(struct file *file)
> 
> -- 
> Regards,
> 
> Laurent Pinchart

-- 
Regards,
Niklas Söderlund

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, back to index

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-16  1:14 [PATCH v2 0/8] rcar-vin: Merge Gen2 and Gen3 file operations Niklas Söderlund
2019-05-16  1:14 ` [PATCH v2 1/8] rcar-vin: Do not call pm_runtime_{resume,suspend}() Niklas Söderlund
2019-05-16  7:23   ` Geert Uytterhoeven
2019-05-16 11:32     ` Laurent Pinchart
2019-05-16  1:14 ` [PATCH v2 2/8] rcar-vin: Remove unneeded calls to pm_runtime_{enable,disable} Niklas Söderlund
2019-05-16  7:27   ` Geert Uytterhoeven
2019-05-16 11:34     ` Laurent Pinchart
2019-05-16  1:14 ` [PATCH v2 3/8] rcar-vin: Allow interrupting lock when trying to open the video device Niklas Söderlund
2019-05-16 11:21   ` Laurent Pinchart
2019-05-16  1:14 ` [PATCH v2 4/8] rcar-vin: Do not sync subdevice format when opening " Niklas Söderlund
2019-05-16 11:26   ` Laurent Pinchart
2019-05-16 13:25     ` Niklas Söderlund
2019-05-16  1:14 ` [PATCH v2 5/8] rcar-vin: Move pm_runtime_{get,put} out of helpers Niklas Söderlund
2019-05-16 11:39   ` Laurent Pinchart
2019-05-16  1:14 ` [PATCH v2 6/8] rcar-vin: Merge helpers dealing with powering the parallel subdevice Niklas Söderlund
2019-05-16  7:40   ` Ulrich Hecht
2019-05-16 11:44   ` Laurent Pinchart
2019-05-16  1:14 ` [PATCH v2 7/8] rcar-vin: Fold rvin_initialize_device() into rvin_open() Niklas Söderlund
2019-05-16 11:45   ` Laurent Pinchart
2019-05-16  1:14 ` [PATCH v2 8/8] rcar-vin: Merge Gen2 and Gen3 file operations Niklas Söderlund
2019-05-16 11:48   ` Laurent Pinchart

Linux-Renesas-SoC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-renesas-soc/0 linux-renesas-soc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-renesas-soc linux-renesas-soc/ https://lore.kernel.org/linux-renesas-soc \
		linux-renesas-soc@vger.kernel.org linux-renesas-soc@archiver.kernel.org
	public-inbox-index linux-renesas-soc


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-renesas-soc


AGPL code for this site: git clone https://public-inbox.org/ public-inbox