[PATCH v2 0/8] rcar-vin: Merge Gen2 and Gen3 file operations

[PATCH v2 0/8] rcar-vin: Merge Gen2 and Gen3 file operations

[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

[PATCH v2 1/8] rcar-vin: Do not call pm_runtime_{resume,suspend}()

[Niklas Söderlund]

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

[PATCH v2 2/8] rcar-vin: Remove unneeded calls to pm_runtime_{enable,disable}

[Niklas Söderlund]

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

[PATCH v2 3/8] rcar-vin: Allow interrupting lock when trying to open the video device

[Niklas Söderlund]

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

[PATCH v2 4/8] rcar-vin: Do not sync subdevice format when opening the video device

[Niklas Söderlund]

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

[PATCH v2 5/8] rcar-vin: Move pm_runtime_{get,put} out of helpers

[Niklas Söderlund]

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

[PATCH v2 6/8] rcar-vin: Merge helpers dealing with powering the parallel subdevice

[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

[PATCH v2 7/8] rcar-vin: Fold rvin_initialize_device() into rvin_open()

[Niklas Söderlund]

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

[PATCH v2 8/8] rcar-vin: Merge Gen2 and Gen3 file operations

[Niklas Söderlund]

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

Re: [PATCH v2 1/8] rcar-vin: Do not call pm_runtime_{resume,suspend}()

[Geert Uytterhoeven]

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

Re: [PATCH v2 2/8] rcar-vin: Remove unneeded calls to pm_runtime_{enable,disable}

[Geert Uytterhoeven]

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

Re: [PATCH v2 6/8] rcar-vin: Merge helpers dealing with powering the parallel subdevice

[Ulrich Hecht]

> 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

Re: [PATCH v2 3/8] rcar-vin: Allow interrupting lock when trying to open the video device

[Laurent Pinchart]

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

Re: [PATCH v2 4/8] rcar-vin: Do not sync subdevice format when opening the video device

[Laurent Pinchart]

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

Re: [PATCH v2 1/8] rcar-vin: Do not call pm_runtime_{resume,suspend}()

[Laurent Pinchart]

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

Re: [PATCH v2 2/8] rcar-vin: Remove unneeded calls to pm_runtime_{enable,disable}

[Laurent Pinchart]

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

Re: [PATCH v2 5/8] rcar-vin: Move pm_runtime_{get,put} out of helpers

[Laurent Pinchart]

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

Re: [PATCH v2 6/8] rcar-vin: Merge helpers dealing with powering the parallel subdevice

[Laurent Pinchart]

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

Re: [PATCH v2 7/8] rcar-vin: Fold rvin_initialize_device() into rvin_open()

[Laurent Pinchart]

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

Re: [PATCH v2 8/8] rcar-vin: Merge Gen2 and Gen3 file operations

[Laurent Pinchart]

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

Re: [PATCH v2 4/8] rcar-vin: Do not sync subdevice format when opening the video device

[Niklas Söderlund]

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