[PATCH 0/5] rcar-vin: Support suspend and resume

[PATCH 0/5] rcar-vin: Support suspend and resume

[Niklas Söderlund]

Hi,

This series add suspend and resume support directly to R-Car VIN and 
indirectly to R-Car CSI-2 and other subdevices in the VIN capture 
pipeline. The capture pipeline is stopped when suspending and started 
when resuming, all while retaining the buffers provided from user-space.  
This makes the start and stop of the pipeline transparent from an 
application point of view.

As the pipeline is switched off subdevices that poweroff themself when 
not in use (such as R-Car CSI-2) are also switched off and are 
indirectly serviced by the suspend support in VIN.

This work is based on-top of the media-tree and is tested on both R-Car 
Gen2 and Gen3 without any regressions.

Niklas Söderlund (5):
  rcar-vin: Use scratch buffer when not in running state
  rcar-vin: Remove handling of user-space buffers when stopping
  rcar-vin: Cache the CSI-2 channel selection value
  rcar-vin: Break out hardware start and stop to new methods
  rcar-vin: Add support for suspend and resume

 drivers/media/platform/rcar-vin/rcar-core.c |  51 ++++++++
 drivers/media/platform/rcar-vin/rcar-dma.c  | 129 +++++++++++---------
 drivers/media/platform/rcar-vin/rcar-vin.h  |  15 ++-
 3 files changed, 131 insertions(+), 64 deletions(-)

-- 
2.28.0

[PATCH 1/5] rcar-vin: Use scratch buffer when not in running state

[Niklas Söderlund]

In its early stages the VIN driver did not use an internal scratch
buffer. This lead to a unnecessary complex start and stop behavior,
specially for TB/BT. The driver now always allocates a scratch buffer to
deal with buffer under-runs, use the scratch buffer to also simplify
starting and stopping.

When capture is starting use the scratch buffer instead of a user-space
buffers while syncing the driver with the hardware state. This allows
the driver to know that no user-space buffers is given to the hardware
before the running state is reached.

When capture is stopping use the scratch buffer instead of leaving the
user-space buffers in place and add a check that all user-space buffers
are processed by the hardware before transitioning from the stopping to
stopped state. This allows the driver to know all user-space buffers
given to the hardware are fully processed.

This change in itself does not improve the driver much but it paves way
for future simplifications and enhancements. One direct improvement of
this change is that TB/BT buffers returned to user-space wile stopping
will always contains both fields, that was not guaranteed before.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 692dea300b0de8b5..ca90bde8d29f77d1 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -905,7 +905,7 @@ static void rvin_fill_hw_slot(struct rvin_dev *vin, int slot)
 				vin->format.sizeimage / 2;
 			break;
 		}
-	} else if (list_empty(&vin->buf_list)) {
+	} else if (vin->state != RUNNING || list_empty(&vin->buf_list)) {
 		vin->buf_hw[slot].buffer = NULL;
 		vin->buf_hw[slot].type = FULL;
 		phys_addr = vin->scratch_phys;
@@ -998,12 +998,6 @@ static irqreturn_t rvin_irq(int irq, void *data)
 		goto done;
 	}
 
-	/* Nothing to do if capture status is 'STOPPING' */
-	if (vin->state == STOPPING) {
-		vin_dbg(vin, "IRQ while state stopping\n");
-		goto done;
-	}
-
 	/* Prepare for capture and update state */
 	vnms = rvin_read(vin, VNMS_REG);
 	slot = (vnms & VNMS_FBS_MASK) >> VNMS_FBS_SHIFT;
@@ -1313,14 +1307,32 @@ static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
 static void rvin_stop_streaming(struct vb2_queue *vq)
 {
 	struct rvin_dev *vin = vb2_get_drv_priv(vq);
+	unsigned int i, retries;
 	unsigned long flags;
-	int retries = 0;
+	bool buffersFreed;
 
 	spin_lock_irqsave(&vin->qlock, flags);
 
 	vin->state = STOPPING;
 
+	/* Wait until only scratch buffer is used, max 3 interrupts. */
+	retries = 0;
+	while (retries++ < RVIN_RETRIES) {
+		buffersFreed = true;
+		for (i = 0; i < HW_BUFFER_NUM; i++)
+			if (vin->buf_hw[i].buffer)
+				buffersFreed = false;
+
+		if (buffersFreed)
+			break;
+
+		spin_unlock_irqrestore(&vin->qlock, flags);
+		msleep(RVIN_TIMEOUT_MS);
+		spin_lock_irqsave(&vin->qlock, flags);
+	}
+
 	/* Wait for streaming to stop */
+	retries = 0;
 	while (retries++ < RVIN_RETRIES) {
 
 		rvin_capture_stop(vin);
@@ -1336,7 +1348,7 @@ static void rvin_stop_streaming(struct vb2_queue *vq)
 		spin_lock_irqsave(&vin->qlock, flags);
 	}
 
-	if (vin->state != STOPPED) {
+	if (!buffersFreed || vin->state != STOPPED) {
 		/*
 		 * If this happens something have gone horribly wrong.
 		 * Set state to stopped to prevent the interrupt handler
-- 
2.28.0

[PATCH 2/5] rcar-vin: Remove handling of user-space buffers when stopping

[Niklas Söderlund]

When returning buffers to user-space it's no longer needed to examine
the buffers given to hardware as recent changes guarantees the only
buffer present in the hardware registers when the driver is not in the
running state is the scratch buffer.

Remove the special case and rename the function to better describe it
now only deals with buffers queued to the driver but not yet recorded in
the hardware registers.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index ca90bde8d29f77d1..680160f9f851d8a3 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -1051,27 +1051,10 @@ static irqreturn_t rvin_irq(int irq, void *data)
 }
 
 /* Need to hold qlock before calling */
-static void return_all_buffers(struct rvin_dev *vin,
-			       enum vb2_buffer_state state)
+static void return_unused_buffers(struct rvin_dev *vin,
+				  enum vb2_buffer_state state)
 {
 	struct rvin_buffer *buf, *node;
-	struct vb2_v4l2_buffer *freed[HW_BUFFER_NUM];
-	unsigned int i, n;
-
-	for (i = 0; i < HW_BUFFER_NUM; i++) {
-		freed[i] = vin->buf_hw[i].buffer;
-		vin->buf_hw[i].buffer = NULL;
-
-		for (n = 0; n < i; n++) {
-			if (freed[i] == freed[n]) {
-				freed[i] = NULL;
-				break;
-			}
-		}
-
-		if (freed[i])
-			vb2_buffer_done(&freed[i]->vb2_buf, state);
-	}
 
 	list_for_each_entry_safe(buf, node, &vin->buf_list, list) {
 		vb2_buffer_done(&buf->vb.vb2_buf, state);
@@ -1271,7 +1254,7 @@ static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
 					  &vin->scratch_phys, GFP_KERNEL);
 	if (!vin->scratch) {
 		spin_lock_irqsave(&vin->qlock, flags);
-		return_all_buffers(vin, VB2_BUF_STATE_QUEUED);
+		return_unused_buffers(vin, VB2_BUF_STATE_QUEUED);
 		spin_unlock_irqrestore(&vin->qlock, flags);
 		vin_err(vin, "Failed to allocate scratch buffer\n");
 		return -ENOMEM;
@@ -1280,7 +1263,7 @@ static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
 	ret = rvin_set_stream(vin, 1);
 	if (ret) {
 		spin_lock_irqsave(&vin->qlock, flags);
-		return_all_buffers(vin, VB2_BUF_STATE_QUEUED);
+		return_unused_buffers(vin, VB2_BUF_STATE_QUEUED);
 		spin_unlock_irqrestore(&vin->qlock, flags);
 		goto out;
 	}
@@ -1291,7 +1274,7 @@ static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
 
 	ret = rvin_capture_start(vin);
 	if (ret) {
-		return_all_buffers(vin, VB2_BUF_STATE_QUEUED);
+		return_unused_buffers(vin, VB2_BUF_STATE_QUEUED);
 		rvin_set_stream(vin, 0);
 	}
 
@@ -1358,8 +1341,8 @@ static void rvin_stop_streaming(struct vb2_queue *vq)
 		vin->state = STOPPED;
 	}
 
-	/* Release all active buffers */
-	return_all_buffers(vin, VB2_BUF_STATE_ERROR);
+	/* Return all unused buffers. */
+	return_unused_buffers(vin, VB2_BUF_STATE_ERROR);
 
 	spin_unlock_irqrestore(&vin->qlock, flags);
 
-- 
2.28.0

[PATCH 3/5] rcar-vin: Cache the CSI-2 channel selection value

[Niklas Söderlund]

In preparation of suspend/resume support cache the chsel value when
written to the register so it can be restored on resume if needed.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/platform/rcar-vin/rcar-dma.c | 2 ++
 drivers/media/platform/rcar-vin/rcar-vin.h | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 680160f9f851d8a3..f65deac4c2dbed54 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -1456,6 +1456,8 @@ int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel)
 
 	vin_dbg(vin, "Set IFMD 0x%x\n", ifmd);
 
+	vin->chsel = chsel;
+
 	/* Restore VNMC. */
 	rvin_write(vin, vnmc, VNMC_REG);
 
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index 8396e0e45478fe4f..2fef23470e3ddfe3 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -189,6 +189,7 @@ struct rvin_info {
  * @state:		keeps track of operation state
  *
  * @is_csi:		flag to mark the VIN as using a CSI-2 subdevice
+ * @chsel		Cached value of the current CSI-2 channel selection
  *
  * @mbus_code:		media bus format code
  * @format:		active V4L2 pixel format
@@ -232,6 +233,7 @@ struct rvin_dev {
 	enum rvin_dma_state state;
 
 	bool is_csi;
+	unsigned int chsel;
 
 	u32 mbus_code;
 	struct v4l2_pix_format format;
-- 
2.28.0

[PATCH 4/5] rcar-vin: Break out hardware start and stop to new methods

[Niklas Söderlund]

To support suspend and resume the ability to start and stop the hardware
needs to be available internally in the driver. Currently this code is
in the start and stop callbacks of the vb2_ops struct. In these
callbacks the code is intertwined with buffer allocation and freeing.

Prepare for suspend and resume support by braking out the hardware
start/stop code into new methods.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-dma.c | 78 +++++++++++++---------
 drivers/media/platform/rcar-vin/rcar-vin.h |  3 +
 2 files changed, 48 insertions(+), 33 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index f65deac4c2dbed54..5a5f0e5007478c8d 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -1050,16 +1050,20 @@ static irqreturn_t rvin_irq(int irq, void *data)
 	return IRQ_RETVAL(handled);
 }
 
-/* Need to hold qlock before calling */
 static void return_unused_buffers(struct rvin_dev *vin,
 				  enum vb2_buffer_state state)
 {
 	struct rvin_buffer *buf, *node;
+	unsigned long flags;
+
+	spin_lock_irqsave(&vin->qlock, flags);
 
 	list_for_each_entry_safe(buf, node, &vin->buf_list, list) {
 		vb2_buffer_done(&buf->vb.vb2_buf, state);
 		list_del(&buf->list);
 	}
+
+	spin_unlock_irqrestore(&vin->qlock, flags);
 }
 
 static int rvin_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
@@ -1243,53 +1247,55 @@ static int rvin_set_stream(struct rvin_dev *vin, int on)
 	return ret;
 }
 
-static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
+int rvin_start_streaming(struct rvin_dev *vin)
 {
-	struct rvin_dev *vin = vb2_get_drv_priv(vq);
 	unsigned long flags;
 	int ret;
 
-	/* Allocate scratch buffer. */
-	vin->scratch = dma_alloc_coherent(vin->dev, vin->format.sizeimage,
-					  &vin->scratch_phys, GFP_KERNEL);
-	if (!vin->scratch) {
-		spin_lock_irqsave(&vin->qlock, flags);
-		return_unused_buffers(vin, VB2_BUF_STATE_QUEUED);
-		spin_unlock_irqrestore(&vin->qlock, flags);
-		vin_err(vin, "Failed to allocate scratch buffer\n");
-		return -ENOMEM;
-	}
-
 	ret = rvin_set_stream(vin, 1);
-	if (ret) {
-		spin_lock_irqsave(&vin->qlock, flags);
-		return_unused_buffers(vin, VB2_BUF_STATE_QUEUED);
-		spin_unlock_irqrestore(&vin->qlock, flags);
-		goto out;
-	}
+	if (ret)
+		return ret;
 
 	spin_lock_irqsave(&vin->qlock, flags);
 
 	vin->sequence = 0;
 
 	ret = rvin_capture_start(vin);
-	if (ret) {
-		return_unused_buffers(vin, VB2_BUF_STATE_QUEUED);
+	if (ret)
 		rvin_set_stream(vin, 0);
-	}
 
 	spin_unlock_irqrestore(&vin->qlock, flags);
-out:
-	if (ret)
-		dma_free_coherent(vin->dev, vin->format.sizeimage, vin->scratch,
-				  vin->scratch_phys);
 
 	return ret;
 }
 
-static void rvin_stop_streaming(struct vb2_queue *vq)
+static int rvin_start_streaming_vq(struct vb2_queue *vq, unsigned int count)
 {
 	struct rvin_dev *vin = vb2_get_drv_priv(vq);
+	int ret = -ENOMEM;
+
+	/* Allocate scratch buffer. */
+	vin->scratch = dma_alloc_coherent(vin->dev, vin->format.sizeimage,
+					  &vin->scratch_phys, GFP_KERNEL);
+	if (!vin->scratch)
+		goto err_scratch;
+
+	ret = rvin_start_streaming(vin);
+	if (ret)
+		goto err_start;
+
+	return 0;
+err_start:
+	dma_free_coherent(vin->dev, vin->format.sizeimage, vin->scratch,
+			  vin->scratch_phys);
+err_scratch:
+	return_unused_buffers(vin, VB2_BUF_STATE_QUEUED);
+
+	return ret;
+}
+
+void rvin_stop_streaming(struct rvin_dev *vin)
+{
 	unsigned int i, retries;
 	unsigned long flags;
 	bool buffersFreed;
@@ -1341,27 +1347,33 @@ static void rvin_stop_streaming(struct vb2_queue *vq)
 		vin->state = STOPPED;
 	}
 
-	/* Return all unused buffers. */
-	return_unused_buffers(vin, VB2_BUF_STATE_ERROR);
-
 	spin_unlock_irqrestore(&vin->qlock, flags);
 
 	rvin_set_stream(vin, 0);
 
 	/* disable interrupts */
 	rvin_disable_interrupts(vin);
+}
+
+static void rvin_stop_streaming_vq(struct vb2_queue *vq)
+{
+	struct rvin_dev *vin = vb2_get_drv_priv(vq);
+
+	rvin_stop_streaming(vin);
 
 	/* Free scratch buffer. */
 	dma_free_coherent(vin->dev, vin->format.sizeimage, vin->scratch,
 			  vin->scratch_phys);
+
+	return_unused_buffers(vin, VB2_BUF_STATE_ERROR);
 }
 
 static const struct vb2_ops rvin_qops = {
 	.queue_setup		= rvin_queue_setup,
 	.buf_prepare		= rvin_buffer_prepare,
 	.buf_queue		= rvin_buffer_queue,
-	.start_streaming	= rvin_start_streaming,
-	.stop_streaming		= rvin_stop_streaming,
+	.start_streaming	= rvin_start_streaming_vq,
+	.stop_streaming		= rvin_stop_streaming_vq,
 	.wait_prepare		= vb2_ops_wait_prepare,
 	.wait_finish		= vb2_ops_wait_finish,
 };
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index 2fef23470e3ddfe3..4ec8584709c847a9 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -299,4 +299,7 @@ void rvin_crop_scale_comp(struct rvin_dev *vin);
 int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel);
 void rvin_set_alpha(struct rvin_dev *vin, unsigned int alpha);
 
+int rvin_start_streaming(struct rvin_dev *vin);
+void rvin_stop_streaming(struct rvin_dev *vin);
+
 #endif
-- 
2.28.0

[PATCH 5/5] rcar-vin: Add support for suspend and resume

[Niklas Söderlund]

Add support for suspend and resume by stopping and starting the video
pipeline while still retaining all buffers given to the driver by
user-space and internally allocated ones, this gives the application a
seamless experience.

Buffers are never returned to user-space unprocessed so user-space don't
notice when suspending. When resuming the driver restarts the capture
session using the internal scratch buffer, this happens before
user-space is unfrozen, this leads to speedy response times once the
application resumes its execution.

As the entire pipeline is stopped on suspend all subdevices in use are
also stopped, and if they enter a shutdown state when not streaming
(such as the R-Car CSI-2 driver) they too will be suspended and resumed
in sync with the VIN driver.

To be able to do keep track of which VINs should be resumed a new
internal state SUSPENDED is added to recode this.

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 drivers/media/platform/rcar-vin/rcar-core.c | 51 +++++++++++++++++++++
 drivers/media/platform/rcar-vin/rcar-vin.h  | 10 ++--
 2 files changed, 57 insertions(+), 4 deletions(-)

diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
index 34d003e0e9b9c25a..4adf4ce518f79c93 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -918,6 +918,54 @@ static int rvin_mc_init(struct rvin_dev *vin)
 	return ret;
 }
 
+/* -----------------------------------------------------------------------------
+ * Suspend / Resume
+ */
+
+static int __maybe_unused rvin_suspend(struct device *dev)
+{
+	struct rvin_dev *vin = dev_get_drvdata(dev);
+
+	if (vin->state != RUNNING)
+		return 0;
+
+	rvin_stop_streaming(vin);
+
+	vin->state = SUSPENDED;
+
+	return 0;
+}
+
+static int __maybe_unused rvin_resume(struct device *dev)
+{
+	struct rvin_dev *vin = dev_get_drvdata(dev);
+
+	if (vin->state != SUSPENDED)
+		return 0;
+
+	/*
+	 * Restore group master CHSEL setting.
+	 *
+	 * This needs to be by every VIN resuming not only the master
+	 * as we don't know if and in which order the master VINs will
+	 * be resumed.
+	 */
+	if (vin->info->use_mc) {
+		unsigned int master_id = rvin_group_id_to_master(vin->id);
+		struct rvin_dev *master = vin->group->vin[master_id];
+		int ret;
+
+		if (WARN_ON(!master))
+			return -ENODEV;
+
+		ret = rvin_set_channel_routing(master, master->chsel);
+		if (ret)
+			return ret;
+	}
+
+	return rvin_start_streaming(vin);
+}
+
 /* -----------------------------------------------------------------------------
  * Platform Device Driver
  */
@@ -1421,9 +1469,12 @@ static int rcar_vin_remove(struct platform_device *pdev)
 	return 0;
 }
 
+static SIMPLE_DEV_PM_OPS(rvin_pm_ops, rvin_suspend, rvin_resume);
+
 static struct platform_driver rcar_vin_driver = {
 	.driver = {
 		.name = "rcar-vin",
+		.pm = &rvin_pm_ops,
 		.of_match_table = rvin_of_id_table,
 	},
 	.probe = rcar_vin_probe,
diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
index 4ec8584709c847a9..4539bd53d9d41e9c 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -49,16 +49,18 @@ enum rvin_csi_id {
 };
 
 /**
- * STOPPED  - No operation in progress
- * STARTING - Capture starting up
- * RUNNING  - Operation in progress have buffers
- * STOPPING - Stopping operation
+ * STOPPED   - No operation in progress
+ * STARTING  - Capture starting up
+ * RUNNING   - Operation in progress have buffers
+ * STOPPING  - Stopping operation
+ * SUSPENDED - Capture is suspended
  */
 enum rvin_dma_state {
 	STOPPED = 0,
 	STARTING,
 	RUNNING,
 	STOPPING,
+	SUSPENDED,
 };
 
 /**
-- 
2.28.0

Re: [PATCH 5/5] rcar-vin: Add support for suspend and resume

[Geert Uytterhoeven]

Hi Niklas,

On Fri, Oct 16, 2020 at 4:01 AM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> Add support for suspend and resume by stopping and starting the video
> pipeline while still retaining all buffers given to the driver by
> user-space and internally allocated ones, this gives the application a
> seamless experience.
>
> Buffers are never returned to user-space unprocessed so user-space don't
> notice when suspending. When resuming the driver restarts the capture
> session using the internal scratch buffer, this happens before
> user-space is unfrozen, this leads to speedy response times once the
> application resumes its execution.
>
> As the entire pipeline is stopped on suspend all subdevices in use are
> also stopped, and if they enter a shutdown state when not streaming
> (such as the R-Car CSI-2 driver) they too will be suspended and resumed
> in sync with the VIN driver.
>
> To be able to do keep track of which VINs should be resumed a new
> internal state SUSPENDED is added to recode this.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Thanks for your patch!

> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c

> +static int __maybe_unused rvin_resume(struct device *dev)
> +{
> +       struct rvin_dev *vin = dev_get_drvdata(dev);
> +
> +       if (vin->state != SUSPENDED)
> +               return 0;
> +
> +       /*
> +        * Restore group master CHSEL setting.
> +        *
> +        * This needs to be by every VIN resuming not only the master

to be done?

> +        * as we don't know if and in which order the master VINs will
> +        * be resumed.
> +        */
> +       if (vin->info->use_mc) {
> +               unsigned int master_id = rvin_group_id_to_master(vin->id);
> +               struct rvin_dev *master = vin->group->vin[master_id];
> +               int ret;
> +
> +               if (WARN_ON(!master))
> +                       return -ENODEV;
> +
> +               ret = rvin_set_channel_routing(master, master->chsel);
> +               if (ret)
> +                       return ret;
> +       }
> +
> +       return rvin_start_streaming(vin);
> +}
> +

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 0/5] rcar-vin: Support suspend and resume

[Geert Uytterhoeven]

Hi Niklas,

On Fri, Oct 16, 2020 at 4:01 AM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> This series add suspend and resume support directly to R-Car VIN and
> indirectly to R-Car CSI-2 and other subdevices in the VIN capture
> pipeline. The capture pipeline is stopped when suspending and started
> when resuming, all while retaining the buffers provided from user-space.
> This makes the start and stop of the pipeline transparent from an
> application point of view.
>
> As the pipeline is switched off subdevices that poweroff themself when
> not in use (such as R-Car CSI-2) are also switched off and are
> indirectly serviced by the suspend support in VIN.

Thanks for your series!

> This work is based on-top of the media-tree and is tested on both R-Car
> Gen2 and Gen3 without any regressions.

FTR: did you test on Gen3 with both s2idle and s2ram, the latter powering
off the SoC?

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 0/5] rcar-vin: Support suspend and resume

[Niklas Söderlund]

Hi Geert,

On 2020-10-16 09:06:20 +0200, Geert Uytterhoeven wrote:
> Hi Niklas,
> 
> On Fri, Oct 16, 2020 at 4:01 AM Niklas Söderlund
> <niklas.soderlund+renesas@ragnatech.se> wrote:
> > This series add suspend and resume support directly to R-Car VIN and
> > indirectly to R-Car CSI-2 and other subdevices in the VIN capture
> > pipeline. The capture pipeline is stopped when suspending and started
> > when resuming, all while retaining the buffers provided from user-space.
> > This makes the start and stop of the pipeline transparent from an
> > application point of view.
> >
> > As the pipeline is switched off subdevices that poweroff themself when
> > not in use (such as R-Car CSI-2) are also switched off and are
> > indirectly serviced by the suspend support in VIN.
> 
> Thanks for your series!
> 
> > This work is based on-top of the media-tree and is tested on both R-Car
> > Gen2 and Gen3 without any regressions.
> 
> FTR: did you test on Gen3 with both s2idle and s2ram, the latter powering
> off the SoC?

I have only been able to test it with s2idle. My issue is that s2ram 
fails to reconnect the Ethernet (ravb) and I use nfsroot. If I instead 
use a initramfs I can resume from s2ram but I don't have the setup to 
test capture in that environment.

My procedure for s2idle that works with nfsroot is,

    # path=$(find /sys -path '/sys/devices/platform/*/ttySC0/power/wakeup')
    # echo enabled > $path
    # echo N > /sys/module/printk/parameters/console_suspend
    # echo s2idle > /sys/power/mem_slee
    # echo mem > /sys/power/state
    ** Wait a while and then wakeup using the console **
    #

My procedure for s2ram that does _not_ work with nfsroot (but do with 
initramfs). Both tests are done on a M3-N.

    # echo N > /sys/module/printk/parameters/console_suspend
    # echo on > /sys/bus/i2c/drivers/bd9571mwv/*/bd9571mwv-regulator*/backup_mode
    ** flipp SW23 off **
    # echo mem > /sys/power/state
    [  347.096336] PM: suspend entry (deep)
    [  347.104251] Filesystems sync: 0.003 seconds
    [  347.123751] Freezing user space processes ... (elapsed 0.007 seconds) done.
    [  347.138760] OOM killer disabled.
    [  347.142099] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done.
    [  347.167466] ravb e6800000.ethernet eth0: Link is Down
    [  347.440549] Disabling non-boot CPUs ...
    [  347.448805] CPU1: shutdown
    [  347.451731] psci: CPU1 killed (polled 0 ms)
    ** Waits 30+ seconds then switch SW23 on **
    INFO:    ARM GICv2 driver initialized
    NOTICE:  BL2: R-Car Gen3 Initial Program Loader(CA57) Rev.2.0.6
    NOTICE:  BL2: PRR is R-Car M3N Ver.1.0
    NOTICE:  BL2: Board is Salvator-XS Rev.1.0
    NOTICE:  BL2: Boot device is HyperFlash(160MHz)
    NOTICE:  BL2: LCM state is CM
    NOTICE:  AVS setting succeeded. DVFS_SetVID=0x53
    NOTICE:  BL2: DDR3200(rev.0.40)
    NOTICE:  BL2: [WARM_BOOT]
    NOTICE:  BL2: DRAM Split is OFF
    NOTICE:  BL2: QoS is default setting(rev.0.09)
    NOTICE:  BL2: DRAM refresh interval 1.95 usec
    NOTICE:  BL2: Periodic Write DQ Training
    NOTICE:  BL2: CH0: 400000000 - 47fffffff, 2 GiB
    NOTICE:  BL2: FDT at 0xe6322508
    NOTICE:  BL2: v2.3(release):v2.3-188-g9935047b2086faa3
    NOTICE:  BL2: Built : 23:31:02, Jun 18 2020
    NOTICE:  BL2: Normal boot
    INFO:    BL2: Doing platform setup
    [  347.461237] Enabling non-boot CPUs ...
    [  347.465551] Detected PIPT I-cache on CPU1
    [  347.465611] CPU1: Booted secondary processor 0x0000000001 [0x411fd073]
    [  347.468691] CPU1 is up
    [  347.607806] usb usb2: root hub lost power or was reset
    [  347.613594] usb usb1: root hub lost power or was reset
    [  347.767713] usb usb4: root hub lost power or was reset
    [  347.773424] usb usb3: root hub lost power or was reset
    [  347.775223] libphy: ravb_mii: probed
    [  347.782808] mdio_bus e6800000.ethernet-ffffffff: MDIO device at address 0 is missing.
    [  347.794508] ravb e6800000.ethernet eth0: failed to connect PHY
    [  347.802223] PM: dpm_run_callback(): ravb_resume+0x0/0x190 returns -2
    [  347.808739] PM: Device e6800000.ethernet failed to resume: error -2
    [  347.929701] ata1: link resume succeeded after 1 retries
    [  347.989934] OOM killer enabled.
    [  347.993184] Restarting tasks ... done.
    [  348.004321] PM: suspend exit
    [  348.039400] ata1: SATA link down (SStatus 0 SControl 300)
    [  529.376515] nfs: server 10.0.1.1 not responding, still trying
    [  529.376702] nfs: server 10.0.1.1 not responding, still trying
    [  529.385628] nfs: server 10.0.1.1 not responding, still trying
    ** Board never reaches user-space **

Is there a known fix for this?

> 
> 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

-- 
Regards,
Niklas Söderlund

Re: [PATCH 0/5] rcar-vin: Support suspend and resume

[Geert Uytterhoeven]

Hi Niklas,

On Fri, Oct 16, 2020 at 12:46 PM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> On 2020-10-16 09:06:20 +0200, Geert Uytterhoeven wrote:
> > On Fri, Oct 16, 2020 at 4:01 AM Niklas Söderlund
> > <niklas.soderlund+renesas@ragnatech.se> wrote:
> > > This series add suspend and resume support directly to R-Car VIN and
> > > indirectly to R-Car CSI-2 and other subdevices in the VIN capture
> > > pipeline. The capture pipeline is stopped when suspending and started
> > > when resuming, all while retaining the buffers provided from user-space.
> > > This makes the start and stop of the pipeline transparent from an
> > > application point of view.
> > >
> > > As the pipeline is switched off subdevices that poweroff themself when
> > > not in use (such as R-Car CSI-2) are also switched off and are
> > > indirectly serviced by the suspend support in VIN.
> >
> > Thanks for your series!
> >
> > > This work is based on-top of the media-tree and is tested on both R-Car
> > > Gen2 and Gen3 without any regressions.
> >
> > FTR: did you test on Gen3 with both s2idle and s2ram, the latter powering
> > off the SoC?
>
> I have only been able to test it with s2idle. My issue is that s2ram
> fails to reconnect the Ethernet (ravb) and I use nfsroot. If I instead
> use a initramfs I can resume from s2ram but I don't have the setup to
> test capture in that environment.

>     [  347.775223] libphy: ravb_mii: probed
>     [  347.782808] mdio_bus e6800000.ethernet-ffffffff: MDIO device at address 0 is missing.
>     [  347.794508] ravb e6800000.ethernet eth0: failed to connect PHY
>     [  347.802223] PM: dpm_run_callback(): ravb_resume+0x0/0x190 returns -2
>     [  347.808739] PM: Device e6800000.ethernet failed to resume: error -2
>     [  347.929701] ata1: link resume succeeded after 1 retries
>     [  347.989934] OOM killer enabled.
>     [  347.993184] Restarting tasks ... done.
>     [  348.004321] PM: suspend exit
>     [  348.039400] ata1: SATA link down (SStatus 0 SControl 300)
>     [  529.376515] nfs: server 10.0.1.1 not responding, still trying
>     [  529.376702] nfs: server 10.0.1.1 not responding, still trying
>     [  529.385628] nfs: server 10.0.1.1 not responding, still trying
>     ** Board never reaches user-space **
>
> Is there a known fix for this?

Please try cherry-picking commit 77972b55fb9d35d4 ("Revert "ravb: Fixed
to be able to unload modules"") from v5.9.

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 0/5] rcar-vin: Support suspend and resume

[Niklas Söderlund]

Hi Geert,

On 2020-10-16 13:26:25 +0200, Geert Uytterhoeven wrote:
> Hi Niklas,
> 
> On Fri, Oct 16, 2020 at 12:46 PM Niklas Söderlund
> <niklas.soderlund+renesas@ragnatech.se> wrote:
> > On 2020-10-16 09:06:20 +0200, Geert Uytterhoeven wrote:
> > > On Fri, Oct 16, 2020 at 4:01 AM Niklas Söderlund
> > > <niklas.soderlund+renesas@ragnatech.se> wrote:
> > > > This series add suspend and resume support directly to R-Car VIN and
> > > > indirectly to R-Car CSI-2 and other subdevices in the VIN capture
> > > > pipeline. The capture pipeline is stopped when suspending and started
> > > > when resuming, all while retaining the buffers provided from user-space.
> > > > This makes the start and stop of the pipeline transparent from an
> > > > application point of view.
> > > >
> > > > As the pipeline is switched off subdevices that poweroff themself when
> > > > not in use (such as R-Car CSI-2) are also switched off and are
> > > > indirectly serviced by the suspend support in VIN.
> > >
> > > Thanks for your series!
> > >
> > > > This work is based on-top of the media-tree and is tested on both R-Car
> > > > Gen2 and Gen3 without any regressions.
> > >
> > > FTR: did you test on Gen3 with both s2idle and s2ram, the latter powering
> > > off the SoC?
> >
> > I have only been able to test it with s2idle. My issue is that s2ram
> > fails to reconnect the Ethernet (ravb) and I use nfsroot. If I instead
> > use a initramfs I can resume from s2ram but I don't have the setup to
> > test capture in that environment.
> 
> >     [  347.775223] libphy: ravb_mii: probed
> >     [  347.782808] mdio_bus e6800000.ethernet-ffffffff: MDIO device at address 0 is missing.
> >     [  347.794508] ravb e6800000.ethernet eth0: failed to connect PHY
> >     [  347.802223] PM: dpm_run_callback(): ravb_resume+0x0/0x190 returns -2
> >     [  347.808739] PM: Device e6800000.ethernet failed to resume: error -2
> >     [  347.929701] ata1: link resume succeeded after 1 retries
> >     [  347.989934] OOM killer enabled.
> >     [  347.993184] Restarting tasks ... done.
> >     [  348.004321] PM: suspend exit
> >     [  348.039400] ata1: SATA link down (SStatus 0 SControl 300)
> >     [  529.376515] nfs: server 10.0.1.1 not responding, still trying
> >     [  529.376702] nfs: server 10.0.1.1 not responding, still trying
> >     [  529.385628] nfs: server 10.0.1.1 not responding, still trying
> >     ** Board never reaches user-space **
> >
> > Is there a known fix for this?
> 
> Please try cherry-picking commit 77972b55fb9d35d4 ("Revert "ravb: Fixed
> to be able to unload modules"") from v5.9.

Thanks that did the trick!

Unfortunately it revealed a new problem related to capturing after a 
s2ram, the ADV7482 does not support s2ram and when it gets reset it no 
longer is possible to communicate to it over i2c. This in turn breaks 
the VIN s2ram test as of course it can not resume capture if it can't 
talk to the ADV7482.

I reproduced this issue directly on top of v5.9. I'm not streaming at 
the time of s2ram. The test is to read the video standard reported by 
the AFE subdevice provided by the ADV7482 before and after s2ram. As 
shown it does not respond after a s2ram but unbind/bind it forcing it to 
reinit itself solves the issue.

Test case:

    # subdev=$(grep -l "adv748x 4-0070 afe" /sys/class/video4linux/*/name | sed 's#.*video4linux\(.*\)/name#/dev\1#g')
    # v4l2-ctl --get-detected-standard -d $subdev
    # echo N > /sys/module/printk/parameters/console_suspend
    # echo on > /sys/bus/i2c/drivers/bd9571mwv/*/bd9571mwv-regulator*/backup_mode 
    ** flipp SW23 off **
    # echo mem > /sys/power/state
    ** Waits 30+ seconds then switch SW23 on ** 
    # subdev=$(grep -l "adv748x 4-0070 afe" /sys/class/video4linux/*/name | sed 's#.*video4linux\(.*\)/name#/dev\1#g')
    # v4l2-ctl --get-detected-standard -d $subdev
    # echo 4-0070 > /sys/bus/i2c/drivers/adv748x/unbind
    # echo 4-0070 > /sys/bus/i2c/drivers/adv748x/bind
    # subdev=$(grep -l "adv748x 4-0070 afe" /sys/class/video4linux/*/name | sed 's#.*video4linux\(.*\)/name#/dev\1#g')
    # v4l2-ctl --get-detected-standard -d $subdev

Output:

    # subdev=$(grep -l "adv748x 4-0070 afe" /sys/class/video4linux/*/name | sed 's#.*video4linux\(.*\)/name#/dev\1#g')
    # echo $subdev
    /dev/v4l-subdev20
    # v4l2-ctl --get-detected-standard -d $subdev
    Video Standard = 0x000000ff
	    PAL-B/B1/G/H/I/D/D1/K
    # echo N > /sys/module/printk/parameters/console_suspend 
    # echo on > /sys/bus/i2c/drivers/bd9571mwv/*/bd9571mwv-regulator*/backup_mode
    # echo mem > /sys/power/state
    [  477.555286] PM: suspend entry (deep)
    [  477.560233] Filesystems sync: 0.000 seconds
    [  477.572660] Freezing user space processes ... (elapsed 0.002 seconds) done.
    [  477.575590] OOM killer disabled.
    [  477.575598] Freezing remaining freezable tasks ... (elapsed 0.002 seconds) done.
    [  477.587659] ravb e6800000.ethernet eth0: Link is Down
    [  477.857405] Disabling non-boot CPUs ...
    [  477.863418] CPU1: shutdown
    [  477.866275] psci: CPU1 killed (polled 0 ms)
    INFO:    ARM GICv2 driver initialized
    NOTICE:  BL2: R-Car Gen3 Initial Program Loader(CA57) Rev.2.0.6
    NOTICE:  BL2: PRR is R-Car M3N Ver.1.0
    NOTICE:  BL2: Board is Salvator-XS Rev.1.0
    NOTICE:  BL2: Boot device is HyperFlash(160MHz)
    NOTICE:  BL2: LCM state is CM
    NOTICE:  AVS setting succeeded. DVFS_SetVID=0x53
    NOTICE:  BL2: DDR3200(rev.0.40)
    NOTICE:  BL2: [WARM_BOOT]
    NOTICE:  BL2: DRAM Split is OFF
    NOTICE:  BL2: QoS is default setting(rev.0.09)
    NOTICE:  BL2: DRAM refresh interval 1.95 usec
    NOTICE:  BL2: Periodic Write DQ Training
    NOTICE:  BL2: CH0: 400000000 - 47fffffff, 2 GiB
    NOTICE:  BL2: FDT at 0xe6322508
    NOTICE:  BL2: v2.3(release):v2.3-188-g9935047b2086faa3
    NOTICE:  BL2: Built : 23:31:02, Jun 18 2020
    NOTICE:  BL2: Normal boot
    INFO:    BL2: Doing platform setup
    [  477.872578] Enabling non-boot CPUs ...
    [  477.876839] Detected PIPT I-cache on CPU1
    [  477.876897] CPU1: Booted secondary processor 0x0000000001 [0x411fd073]
    [  477.878379] CPU1 is up
    [  478.019935] usb usb2: root hub lost power or was reset
    [  478.025283] usb usb1: root hub lost power or was reset
    [  478.171928] usb usb4: root hub lost power or was reset
    [  478.178011] usb usb3: root hub lost power or was reset
    [  478.217119] Micrel KSZ9031 Gigabit PHY e6800000.ethernet-ffffffff:00: attached PHY driver [Micrel KSZ9031 Gigabit PHY] (mii_bus:phy_addr=e6800000.ethernet-ffffffff:00, irq=197)
    [  478.341930] ata1: link resume succeeded after 1 retries
    [  478.403262] OOM killer enabled.
    [  478.406730] Restarting tasks ... done.
    [  478.417250] PM: suspend exit
    [  478.451992] ata1: SATA link down (SStatus 0 SControl 300)
    [  485.716683] ravb e6800000.ethernet eth0: Link is Up - 1Gbps/Full - flow control off
    # subdev=$(grep -l "adv748x 4-0070 afe" /sys/class/video4linux/*/name | sed 's#.*video4linux\(.*\)/name#/dev\1#g')
    # v4l2-ctl --get-detected-standard -d $subdev
    [  502.753723] adv748x 4-0070: error reading 63, 02
    [  502.866437] adv748x 4-0070: error reading 63, 02
    VIDIOC_QUERYSTD: failed: No such device or address
    # echo 4-0070 > /sys/bus/i2c/drivers/adv748x/unbind
    # echo 4-0070 > /sys/bus/i2c/drivers/adv748x/bind
    [  511.830105] adv748x 4-0070: Endpoint /soc/i2c@e66d8000/video-receiver@70/port@7/endpoint on port 7
    [  511.839766] adv748x 4-0070: Endpoint /soc/i2c@e66d8000/video-receiver@70/port@8/endpoint on port 8
    [  511.849682] adv748x 4-0070: Endpoint /soc/i2c@e66d8000/video-receiver@70/port@a/endpoint on port 10
    [  511.859509] adv748x 4-0070: Endpoint /soc/i2c@e66d8000/video-receiver@70/port@b/endpoint on port 11
    [  511.870845] adv748x 4-0070: chip found @ 0xe0 revision 2143
    # subdev=$(grep -l "adv748x 4-0070 afe" /sys/class/video4linux/*/name | sed 's#.*video4linux\(.*\)/name#/dev\1#g')
    # v4l2-ctl --get-detected-standard -d $subdev
    Video Standard = 0x000000ff
	    PAL-B/B1/G/H/I/D/D1/K
    #

I think this issue needs to be resolved before I can truly verify the 
operation of this series post s2ram. Do you think this series should be 
held until then or does it bring enough value (s2idle) while not 
introducing any regressions? s2ram is just as broken before as after :-)

> 
> 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

-- 
Regards,
Niklas Söderlund

Re: [PATCH 0/5] rcar-vin: Support suspend and resume

[Geert Uytterhoeven]

Hi,

On Fri, Oct 16, 2020 at 2:23 PM Niklas Söderlund
<niklas.soderlund+renesas@ragnatech.se> wrote:
> On 2020-10-16 13:26:25 +0200, Geert Uytterhoeven wrote:
> > On Fri, Oct 16, 2020 at 12:46 PM Niklas Söderlund
> > <niklas.soderlund+renesas@ragnatech.se> wrote:
> > > On 2020-10-16 09:06:20 +0200, Geert Uytterhoeven wrote:
> > > > On Fri, Oct 16, 2020 at 4:01 AM Niklas Söderlund
> > > > <niklas.soderlund+renesas@ragnatech.se> wrote:
> > > > > This series add suspend and resume support directly to R-Car VIN and
> > > > > indirectly to R-Car CSI-2 and other subdevices in the VIN capture
> > > > > pipeline. The capture pipeline is stopped when suspending and started
> > > > > when resuming, all while retaining the buffers provided from user-space.
> > > > > This makes the start and stop of the pipeline transparent from an
> > > > > application point of view.
> > > > >
> > > > > As the pipeline is switched off subdevices that poweroff themself when
> > > > > not in use (such as R-Car CSI-2) are also switched off and are
> > > > > indirectly serviced by the suspend support in VIN.
> > > >
> > > > Thanks for your series!
> > > >
> > > > > This work is based on-top of the media-tree and is tested on both R-Car
> > > > > Gen2 and Gen3 without any regressions.
> > > >
> > > > FTR: did you test on Gen3 with both s2idle and s2ram, the latter powering
> > > > off the SoC?
> > >
> > > I have only been able to test it with s2idle. My issue is that s2ram
> > > fails to reconnect the Ethernet (ravb) and I use nfsroot. If I instead
> > > use a initramfs I can resume from s2ram but I don't have the setup to
> > > test capture in that environment.
> >
> > >     [  347.775223] libphy: ravb_mii: probed
> > >     [  347.782808] mdio_bus e6800000.ethernet-ffffffff: MDIO device at address 0 is missing.
> > >     [  347.794508] ravb e6800000.ethernet eth0: failed to connect PHY
> > >     [  347.802223] PM: dpm_run_callback(): ravb_resume+0x0/0x190 returns -2
> > >     [  347.808739] PM: Device e6800000.ethernet failed to resume: error -2
> > >     [  347.929701] ata1: link resume succeeded after 1 retries
> > >     [  347.989934] OOM killer enabled.
> > >     [  347.993184] Restarting tasks ... done.
> > >     [  348.004321] PM: suspend exit
> > >     [  348.039400] ata1: SATA link down (SStatus 0 SControl 300)
> > >     [  529.376515] nfs: server 10.0.1.1 not responding, still trying
> > >     [  529.376702] nfs: server 10.0.1.1 not responding, still trying
> > >     [  529.385628] nfs: server 10.0.1.1 not responding, still trying
> > >     ** Board never reaches user-space **
> > >
> > > Is there a known fix for this?
> >
> > Please try cherry-picking commit 77972b55fb9d35d4 ("Revert "ravb: Fixed
> > to be able to unload modules"") from v5.9.
>
> Thanks that did the trick!

Glad to hear that!

> Unfortunately it revealed a new problem related to capturing after a
> s2ram, the ADV7482 does not support s2ram and when it gets reset it no
> longer is possible to communicate to it over i2c. This in turn breaks
> the VIN s2ram test as of course it can not resume capture if it can't
> talk to the ADV7482.
>
> I reproduced this issue directly on top of v5.9. I'm not streaming at
> the time of s2ram. The test is to read the video standard reported by
> the AFE subdevice provided by the ADV7482 before and after s2ram. As
> shown it does not respond after a s2ram but unbind/bind it forcing it to
> reinit itself solves the issue.

I guess the pesky adv7482 needs its secondary addresses to be reprogrammed
in its resume handler?

> I think this issue needs to be resolved before I can truly verify the
> operation of this series post s2ram. Do you think this series should be
> held until then or does it bring enough value (s2idle) while not
> introducing any regressions? s2ram is just as broken before as after :-)

Well, my main worry is that it would hide a so far unknown s2ram resume
issue in the rcar-vin driver.  I've seen too many  "Add suspend/resume"
commits yielding drivers where suspend/resume still didn't work
afterwards.

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 5/5] rcar-vin: Add support for suspend and resume

[Niklas Söderlund]

Hi Jacopo,

Thanks for your feedback.

On 2020-10-16 18:07:18 +0200, Jacopo Mondi wrote:
> Hi Niklas,
> 
> On Fri, Oct 16, 2020 at 01:14:08AM +0200, Niklas Söderlund wrote:
> > Add support for suspend and resume by stopping and starting the video
> > pipeline while still retaining all buffers given to the driver by
> > user-space and internally allocated ones, this gives the application a
> > seamless experience.
> >
> > Buffers are never returned to user-space unprocessed so user-space don't
> > notice when suspending. When resuming the driver restarts the capture
> > session using the internal scratch buffer, this happens before
> > user-space is unfrozen, this leads to speedy response times once the
> > application resumes its execution.
> >
> > As the entire pipeline is stopped on suspend all subdevices in use are
> > also stopped, and if they enter a shutdown state when not streaming
> > (such as the R-Car CSI-2 driver) they too will be suspended and resumed
> > in sync with the VIN driver.
> >
> > To be able to do keep track of which VINs should be resumed a new
> 
> s/to do/to/
> 
> > internal state SUSPENDED is added to recode this.
> >
> > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > ---
> >  drivers/media/platform/rcar-vin/rcar-core.c | 51 +++++++++++++++++++++
> >  drivers/media/platform/rcar-vin/rcar-vin.h  | 10 ++--
> >  2 files changed, 57 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > index 34d003e0e9b9c25a..4adf4ce518f79c93 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > @@ -918,6 +918,54 @@ static int rvin_mc_init(struct rvin_dev *vin)
> >  	return ret;
> >  }
> >
> > +/* -----------------------------------------------------------------------------
> > + * Suspend / Resume
> > + */
> > +
> > +static int __maybe_unused rvin_suspend(struct device *dev)
> > +{
> > +	struct rvin_dev *vin = dev_get_drvdata(dev);
> > +
> > +	if (vin->state != RUNNING)
> > +		return 0;
> > +
> > +	rvin_stop_streaming(vin);
> 
> This delay suspend untill all the userspace queued buffers are not
> completed, right ?

Yes it will delay suspend until all the buffers queued by user-space AND 
have been written to one of the 3 hardware slots are completed. So the 
worst case scenario is a delay of 3 frames to complete.

Buffers queued by an application not yet commited to a slot are not 
waited for. Instead they are used when capture is resumed.

> 
> > +
> > +	vin->state = SUSPENDED;
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused rvin_resume(struct device *dev)
> > +{
> > +	struct rvin_dev *vin = dev_get_drvdata(dev);
> > +
> > +	if (vin->state != SUSPENDED)
> > +		return 0;
> > +
> > +	/*
> > +	 * Restore group master CHSEL setting.
> > +	 *
> > +	 * This needs to be by every VIN resuming not only the master
> > +	 * as we don't know if and in which order the master VINs will
> > +	 * be resumed.
> > +	 */
> > +	if (vin->info->use_mc) {
> > +		unsigned int master_id = rvin_group_id_to_master(vin->id);
> > +		struct rvin_dev *master = vin->group->vin[master_id];
> > +		int ret;
> > +
> > +		if (WARN_ON(!master))
> > +			return -ENODEV;
> > +
> > +		ret = rvin_set_channel_routing(master, master->chsel);
> > +		if (ret)
> > +			return ret;
> > +	}
> > +
> > +	return rvin_start_streaming(vin);
> > +}
> > +
> >  /* -----------------------------------------------------------------------------
> >   * Platform Device Driver
> >   */
> > @@ -1421,9 +1469,12 @@ static int rcar_vin_remove(struct platform_device *pdev)
> >  	return 0;
> >  }
> >
> > +static SIMPLE_DEV_PM_OPS(rvin_pm_ops, rvin_suspend, rvin_resume);
> > +
> >  static struct platform_driver rcar_vin_driver = {
> >  	.driver = {
> >  		.name = "rcar-vin",
> > +		.pm = &rvin_pm_ops,
> >  		.of_match_table = rvin_of_id_table,
> >  	},
> >  	.probe = rcar_vin_probe,
> > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> > index 4ec8584709c847a9..4539bd53d9d41e9c 100644
> > --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> > @@ -49,16 +49,18 @@ enum rvin_csi_id {
> >  };
> >
> >  /**
> > - * STOPPED  - No operation in progress
> > - * STARTING - Capture starting up
> > - * RUNNING  - Operation in progress have buffers
> > - * STOPPING - Stopping operation
> > + * STOPPED   - No operation in progress
> > + * STARTING  - Capture starting up
> > + * RUNNING   - Operation in progress have buffers
> > + * STOPPING  - Stopping operation
> > + * SUSPENDED - Capture is suspended
> >   */
> >  enum rvin_dma_state {
> >  	STOPPED = 0,
> >  	STARTING,
> >  	RUNNING,
> >  	STOPPING,
> > +	SUSPENDED,
> >  };
> >
> >  /**
> > --
> > 2.28.0
> >

-- 
Regards,
Niklas Söderlund

Re: [PATCH 1/5] rcar-vin: Use scratch buffer when not in running state

[Jacopo Mondi]

Hi Niklas,

On Fri, Oct 16, 2020 at 01:14:04AM +0200, Niklas Söderlund wrote:
> In its early stages the VIN driver did not use an internal scratch
> buffer. This lead to a unnecessary complex start and stop behavior,

This leads

> specially for TB/BT. The driver now always allocates a scratch buffer to
> deal with buffer under-runs, use the scratch buffer to also simplify
> starting and stopping.
>
> When capture is starting use the scratch buffer instead of a user-space
> buffers while syncing the driver with the hardware state. This allows
> the driver to know that no user-space buffers is given to the hardware

s/buffers/buffer

> before the running state is reached.
>
> When capture is stopping use the scratch buffer instead of leaving the
> user-space buffers in place and add a check that all user-space buffers
> are processed by the hardware before transitioning from the stopping to
> stopped state. This allows the driver to know all user-space buffers
> given to the hardware are fully processed.
>
> This change in itself does not improve the driver much but it paves way

paves the way ?

> for future simplifications and enhancements. One direct improvement of
> this change is that TB/BT buffers returned to user-space wile stopping

s/wile/while

> will always contains both fields, that was not guaranteed before.

contain

>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 30 +++++++++++++++-------
>  1 file changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index 692dea300b0de8b5..ca90bde8d29f77d1 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -905,7 +905,7 @@ static void rvin_fill_hw_slot(struct rvin_dev *vin, int slot)
>  				vin->format.sizeimage / 2;
>  			break;
>  		}
> -	} else if (list_empty(&vin->buf_list)) {
> +	} else if (vin->state != RUNNING || list_empty(&vin->buf_list)) {

Does this make the

	for (slot = 0; slot < HW_BUFFER_NUM; slot++)
		rvin_fill_hw_slot(vin, slot);

cycle in rvin_capture_start() a no-op, as it already sets

	for (slot = 0; slot < HW_BUFFER_NUM; slot++) {
		vin->buf_hw[slot].buffer = NULL;
		vin->buf_hw[slot].type = FULL;
	}

just before that entering the loop ?

>  		vin->buf_hw[slot].buffer = NULL;
>  		vin->buf_hw[slot].type = FULL;
>  		phys_addr = vin->scratch_phys;
> @@ -998,12 +998,6 @@ static irqreturn_t rvin_irq(int irq, void *data)
>  		goto done;
>  	}
>
> -	/* Nothing to do if capture status is 'STOPPING' */
> -	if (vin->state == STOPPING) {
> -		vin_dbg(vin, "IRQ while state stopping\n");
> -		goto done;
> -	}
> -
>  	/* Prepare for capture and update state */
>  	vnms = rvin_read(vin, VNMS_REG);
>  	slot = (vnms & VNMS_FBS_MASK) >> VNMS_FBS_SHIFT;
> @@ -1313,14 +1307,32 @@ static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
>  static void rvin_stop_streaming(struct vb2_queue *vq)
>  {
>  	struct rvin_dev *vin = vb2_get_drv_priv(vq);
> +	unsigned int i, retries;
>  	unsigned long flags;
> -	int retries = 0;
> +	bool buffersFreed;
>
>  	spin_lock_irqsave(&vin->qlock, flags);
>
>  	vin->state = STOPPING;
>
> +	/* Wait until only scratch buffer is used, max 3 interrupts. */

nit: I see RVIN_RETRIES being 10. The number of buffers is 3. The number
of interrutps is less relevant than the fact we might end up waiting
1 second (10 * 100ms) before stopping ?

> +	retries = 0;
> +	while (retries++ < RVIN_RETRIES) {

> +		for (i = 0; i < HW_BUFFER_NUM; i++)
> +			if (vin->buf_hw[i].buffer)
> +				buffersFreed = false;
> +
> +		if (buffersFreed)
> +			break;
> +
> +		spin_unlock_irqrestore(&vin->qlock, flags);
> +		msleep(RVIN_TIMEOUT_MS);

As you're waiting for an interrupt, msleep_interruptible() might be a
better choice ?

Mostly just nits, so:
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
  j

> +		spin_lock_irqsave(&vin->qlock, flags);
> +	}
> +
>  	/* Wait for streaming to stop */
> +	retries = 0;
>  	while (retries++ < RVIN_RETRIES) {
>
>  		rvin_capture_stop(vin);
> @@ -1336,7 +1348,7 @@ static void rvin_stop_streaming(struct vb2_queue *vq)
>  		spin_lock_irqsave(&vin->qlock, flags);
>  	}
>
> -	if (vin->state != STOPPED) {
> +	if (!buffersFreed || vin->state != STOPPED) {
>  		/*
>  		 * If this happens something have gone horribly wrong.
>  		 * Set state to stopped to prevent the interrupt handler
> --
> 2.28.0
>

Re: [PATCH 2/5] rcar-vin: Remove handling of user-space buffers when stopping

[Jacopo Mondi]

Hi Niklas,

On Fri, Oct 16, 2020 at 01:14:05AM +0200, Niklas Söderlund wrote:
> When returning buffers to user-space it's no longer needed to examine
> the buffers given to hardware as recent changes guarantees the only
> buffer present in the hardware registers when the driver is not in the
> running state is the scratch buffer.
>
> Remove the special case and rename the function to better describe it
> now only deals with buffers queued to the driver but not yet recorded in
> the hardware registers.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
  j

> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 31 +++++-----------------
>  1 file changed, 7 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index ca90bde8d29f77d1..680160f9f851d8a3 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -1051,27 +1051,10 @@ static irqreturn_t rvin_irq(int irq, void *data)
>  }
>
>  /* Need to hold qlock before calling */
> -static void return_all_buffers(struct rvin_dev *vin,
> -			       enum vb2_buffer_state state)
> +static void return_unused_buffers(struct rvin_dev *vin,
> +				  enum vb2_buffer_state state)
>  {
>  	struct rvin_buffer *buf, *node;
> -	struct vb2_v4l2_buffer *freed[HW_BUFFER_NUM];
> -	unsigned int i, n;
> -
> -	for (i = 0; i < HW_BUFFER_NUM; i++) {
> -		freed[i] = vin->buf_hw[i].buffer;
> -		vin->buf_hw[i].buffer = NULL;
> -
> -		for (n = 0; n < i; n++) {
> -			if (freed[i] == freed[n]) {
> -				freed[i] = NULL;
> -				break;
> -			}
> -		}
> -
> -		if (freed[i])
> -			vb2_buffer_done(&freed[i]->vb2_buf, state);
> -	}
>
>  	list_for_each_entry_safe(buf, node, &vin->buf_list, list) {
>  		vb2_buffer_done(&buf->vb.vb2_buf, state);
> @@ -1271,7 +1254,7 @@ static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
>  					  &vin->scratch_phys, GFP_KERNEL);
>  	if (!vin->scratch) {
>  		spin_lock_irqsave(&vin->qlock, flags);
> -		return_all_buffers(vin, VB2_BUF_STATE_QUEUED);
> +		return_unused_buffers(vin, VB2_BUF_STATE_QUEUED);
>  		spin_unlock_irqrestore(&vin->qlock, flags);
>  		vin_err(vin, "Failed to allocate scratch buffer\n");
>  		return -ENOMEM;
> @@ -1280,7 +1263,7 @@ static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
>  	ret = rvin_set_stream(vin, 1);
>  	if (ret) {
>  		spin_lock_irqsave(&vin->qlock, flags);
> -		return_all_buffers(vin, VB2_BUF_STATE_QUEUED);
> +		return_unused_buffers(vin, VB2_BUF_STATE_QUEUED);
>  		spin_unlock_irqrestore(&vin->qlock, flags);
>  		goto out;
>  	}
> @@ -1291,7 +1274,7 @@ static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
>
>  	ret = rvin_capture_start(vin);
>  	if (ret) {
> -		return_all_buffers(vin, VB2_BUF_STATE_QUEUED);
> +		return_unused_buffers(vin, VB2_BUF_STATE_QUEUED);
>  		rvin_set_stream(vin, 0);
>  	}
>
> @@ -1358,8 +1341,8 @@ static void rvin_stop_streaming(struct vb2_queue *vq)
>  		vin->state = STOPPED;
>  	}
>
> -	/* Release all active buffers */
> -	return_all_buffers(vin, VB2_BUF_STATE_ERROR);
> +	/* Return all unused buffers. */
> +	return_unused_buffers(vin, VB2_BUF_STATE_ERROR);
>
>  	spin_unlock_irqrestore(&vin->qlock, flags);
>
> --
> 2.28.0
>

Re: [PATCH 3/5] rcar-vin: Cache the CSI-2 channel selection value

[Jacopo Mondi]

Hi Niklas,

On Fri, Oct 16, 2020 at 01:14:06AM +0200, Niklas Söderlund wrote:
> In preparation of suspend/resume support cache the chsel value when
> written to the register so it can be restored on resume if needed.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 2 ++
>  drivers/media/platform/rcar-vin/rcar-vin.h | 2 ++
>  2 files changed, 4 insertions(+)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index 680160f9f851d8a3..f65deac4c2dbed54 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -1456,6 +1456,8 @@ int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel)
>
>  	vin_dbg(vin, "Set IFMD 0x%x\n", ifmd);
>
> +	vin->chsel = chsel;
> +
>  	/* Restore VNMC. */
>  	rvin_write(vin, vnmc, VNMC_REG);
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index 8396e0e45478fe4f..2fef23470e3ddfe3 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -189,6 +189,7 @@ struct rvin_info {
>   * @state:		keeps track of operation state
>   *
>   * @is_csi:		flag to mark the VIN as using a CSI-2 subdevice
> + * @chsel		Cached value of the current CSI-2 channel selection
>   *
>   * @mbus_code:		media bus format code
>   * @format:		active V4L2 pixel format
> @@ -232,6 +233,7 @@ struct rvin_dev {
>  	enum rvin_dma_state state;
>
>  	bool is_csi;
> +	unsigned int chsel;

Could be a u8, I'm not sure we gain anything though

Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
  j
>
>  	u32 mbus_code;
>  	struct v4l2_pix_format format;
> --
> 2.28.0
>

Re: [PATCH 4/5] rcar-vin: Break out hardware start and stop to new methods

[Jacopo Mondi]

Hi Niklas,

On Fri, Oct 16, 2020 at 01:14:07AM +0200, Niklas Söderlund wrote:
> To support suspend and resume the ability to start and stop the hardware
> needs to be available internally in the driver. Currently this code is
> in the start and stop callbacks of the vb2_ops struct. In these
> callbacks the code is intertwined with buffer allocation and freeing.
>
> Prepare for suspend and resume support by braking out the hardware

breaking

> start/stop code into new methods.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>

I very much like this, it really simplifies the code
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Thanks
   j

> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 78 +++++++++++++---------
>  drivers/media/platform/rcar-vin/rcar-vin.h |  3 +
>  2 files changed, 48 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
> index f65deac4c2dbed54..5a5f0e5007478c8d 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -1050,16 +1050,20 @@ static irqreturn_t rvin_irq(int irq, void *data)
>  	return IRQ_RETVAL(handled);
>  }
>
> -/* Need to hold qlock before calling */
>  static void return_unused_buffers(struct rvin_dev *vin,
>  				  enum vb2_buffer_state state)
>  {
>  	struct rvin_buffer *buf, *node;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&vin->qlock, flags);
>
>  	list_for_each_entry_safe(buf, node, &vin->buf_list, list) {
>  		vb2_buffer_done(&buf->vb.vb2_buf, state);
>  		list_del(&buf->list);
>  	}
> +
> +	spin_unlock_irqrestore(&vin->qlock, flags);
>  }
>
>  static int rvin_queue_setup(struct vb2_queue *vq, unsigned int *nbuffers,
> @@ -1243,53 +1247,55 @@ static int rvin_set_stream(struct rvin_dev *vin, int on)
>  	return ret;
>  }
>
> -static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
> +int rvin_start_streaming(struct rvin_dev *vin)
>  {
> -	struct rvin_dev *vin = vb2_get_drv_priv(vq);
>  	unsigned long flags;
>  	int ret;
>
> -	/* Allocate scratch buffer. */
> -	vin->scratch = dma_alloc_coherent(vin->dev, vin->format.sizeimage,
> -					  &vin->scratch_phys, GFP_KERNEL);
> -	if (!vin->scratch) {
> -		spin_lock_irqsave(&vin->qlock, flags);
> -		return_unused_buffers(vin, VB2_BUF_STATE_QUEUED);
> -		spin_unlock_irqrestore(&vin->qlock, flags);
> -		vin_err(vin, "Failed to allocate scratch buffer\n");
> -		return -ENOMEM;
> -	}
> -
>  	ret = rvin_set_stream(vin, 1);
> -	if (ret) {
> -		spin_lock_irqsave(&vin->qlock, flags);
> -		return_unused_buffers(vin, VB2_BUF_STATE_QUEUED);
> -		spin_unlock_irqrestore(&vin->qlock, flags);
> -		goto out;
> -	}
> +	if (ret)
> +		return ret;
>
>  	spin_lock_irqsave(&vin->qlock, flags);
>
>  	vin->sequence = 0;
>
>  	ret = rvin_capture_start(vin);
> -	if (ret) {
> -		return_unused_buffers(vin, VB2_BUF_STATE_QUEUED);
> +	if (ret)
>  		rvin_set_stream(vin, 0);
> -	}
>
>  	spin_unlock_irqrestore(&vin->qlock, flags);
> -out:
> -	if (ret)
> -		dma_free_coherent(vin->dev, vin->format.sizeimage, vin->scratch,
> -				  vin->scratch_phys);
>
>  	return ret;
>  }
>
> -static void rvin_stop_streaming(struct vb2_queue *vq)
> +static int rvin_start_streaming_vq(struct vb2_queue *vq, unsigned int count)
>  {
>  	struct rvin_dev *vin = vb2_get_drv_priv(vq);
> +	int ret = -ENOMEM;
> +
> +	/* Allocate scratch buffer. */
> +	vin->scratch = dma_alloc_coherent(vin->dev, vin->format.sizeimage,
> +					  &vin->scratch_phys, GFP_KERNEL);
> +	if (!vin->scratch)
> +		goto err_scratch;
> +
> +	ret = rvin_start_streaming(vin);
> +	if (ret)
> +		goto err_start;
> +
> +	return 0;
> +err_start:
> +	dma_free_coherent(vin->dev, vin->format.sizeimage, vin->scratch,
> +			  vin->scratch_phys);
> +err_scratch:
> +	return_unused_buffers(vin, VB2_BUF_STATE_QUEUED);
> +
> +	return ret;
> +}
> +
> +void rvin_stop_streaming(struct rvin_dev *vin)
> +{
>  	unsigned int i, retries;
>  	unsigned long flags;
>  	bool buffersFreed;
> @@ -1341,27 +1347,33 @@ static void rvin_stop_streaming(struct vb2_queue *vq)
>  		vin->state = STOPPED;
>  	}
>
> -	/* Return all unused buffers. */
> -	return_unused_buffers(vin, VB2_BUF_STATE_ERROR);
> -
>  	spin_unlock_irqrestore(&vin->qlock, flags);
>
>  	rvin_set_stream(vin, 0);
>
>  	/* disable interrupts */
>  	rvin_disable_interrupts(vin);
> +}
> +
> +static void rvin_stop_streaming_vq(struct vb2_queue *vq)
> +{
> +	struct rvin_dev *vin = vb2_get_drv_priv(vq);
> +
> +	rvin_stop_streaming(vin);
>
>  	/* Free scratch buffer. */
>  	dma_free_coherent(vin->dev, vin->format.sizeimage, vin->scratch,
>  			  vin->scratch_phys);
> +
> +	return_unused_buffers(vin, VB2_BUF_STATE_ERROR);
>  }
>
>  static const struct vb2_ops rvin_qops = {
>  	.queue_setup		= rvin_queue_setup,
>  	.buf_prepare		= rvin_buffer_prepare,
>  	.buf_queue		= rvin_buffer_queue,
> -	.start_streaming	= rvin_start_streaming,
> -	.stop_streaming		= rvin_stop_streaming,
> +	.start_streaming	= rvin_start_streaming_vq,
> +	.stop_streaming		= rvin_stop_streaming_vq,
>  	.wait_prepare		= vb2_ops_wait_prepare,
>  	.wait_finish		= vb2_ops_wait_finish,
>  };
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index 2fef23470e3ddfe3..4ec8584709c847a9 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -299,4 +299,7 @@ void rvin_crop_scale_comp(struct rvin_dev *vin);
>  int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel);
>  void rvin_set_alpha(struct rvin_dev *vin, unsigned int alpha);
>
> +int rvin_start_streaming(struct rvin_dev *vin);
> +void rvin_stop_streaming(struct rvin_dev *vin);
> +
>  #endif
> --
> 2.28.0
>

Re: [PATCH 5/5] rcar-vin: Add support for suspend and resume

[Jacopo Mondi]

Hi Niklas,

On Fri, Oct 16, 2020 at 01:14:08AM +0200, Niklas Söderlund wrote:
> Add support for suspend and resume by stopping and starting the video
> pipeline while still retaining all buffers given to the driver by
> user-space and internally allocated ones, this gives the application a
> seamless experience.
>
> Buffers are never returned to user-space unprocessed so user-space don't
> notice when suspending. When resuming the driver restarts the capture
> session using the internal scratch buffer, this happens before
> user-space is unfrozen, this leads to speedy response times once the
> application resumes its execution.
>
> As the entire pipeline is stopped on suspend all subdevices in use are
> also stopped, and if they enter a shutdown state when not streaming
> (such as the R-Car CSI-2 driver) they too will be suspended and resumed
> in sync with the VIN driver.
>
> To be able to do keep track of which VINs should be resumed a new

s/to do/to/

> internal state SUSPENDED is added to recode this.
>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-core.c | 51 +++++++++++++++++++++
>  drivers/media/platform/rcar-vin/rcar-vin.h  | 10 ++--
>  2 files changed, 57 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> index 34d003e0e9b9c25a..4adf4ce518f79c93 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -918,6 +918,54 @@ static int rvin_mc_init(struct rvin_dev *vin)
>  	return ret;
>  }
>
> +/* -----------------------------------------------------------------------------
> + * Suspend / Resume
> + */
> +
> +static int __maybe_unused rvin_suspend(struct device *dev)
> +{
> +	struct rvin_dev *vin = dev_get_drvdata(dev);
> +
> +	if (vin->state != RUNNING)
> +		return 0;
> +
> +	rvin_stop_streaming(vin);

This delay suspend untill all the userspace queued buffers are not
completed, right ?

> +
> +	vin->state = SUSPENDED;
> +
> +	return 0;
> +}
> +
> +static int __maybe_unused rvin_resume(struct device *dev)
> +{
> +	struct rvin_dev *vin = dev_get_drvdata(dev);
> +
> +	if (vin->state != SUSPENDED)
> +		return 0;
> +
> +	/*
> +	 * Restore group master CHSEL setting.
> +	 *
> +	 * This needs to be by every VIN resuming not only the master
> +	 * as we don't know if and in which order the master VINs will
> +	 * be resumed.
> +	 */
> +	if (vin->info->use_mc) {
> +		unsigned int master_id = rvin_group_id_to_master(vin->id);
> +		struct rvin_dev *master = vin->group->vin[master_id];
> +		int ret;
> +
> +		if (WARN_ON(!master))
> +			return -ENODEV;
> +
> +		ret = rvin_set_channel_routing(master, master->chsel);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return rvin_start_streaming(vin);
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * Platform Device Driver
>   */
> @@ -1421,9 +1469,12 @@ static int rcar_vin_remove(struct platform_device *pdev)
>  	return 0;
>  }
>
> +static SIMPLE_DEV_PM_OPS(rvin_pm_ops, rvin_suspend, rvin_resume);
> +
>  static struct platform_driver rcar_vin_driver = {
>  	.driver = {
>  		.name = "rcar-vin",
> +		.pm = &rvin_pm_ops,
>  		.of_match_table = rvin_of_id_table,
>  	},
>  	.probe = rcar_vin_probe,
> diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> index 4ec8584709c847a9..4539bd53d9d41e9c 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -49,16 +49,18 @@ enum rvin_csi_id {
>  };
>
>  /**
> - * STOPPED  - No operation in progress
> - * STARTING - Capture starting up
> - * RUNNING  - Operation in progress have buffers
> - * STOPPING - Stopping operation
> + * STOPPED   - No operation in progress
> + * STARTING  - Capture starting up
> + * RUNNING   - Operation in progress have buffers
> + * STOPPING  - Stopping operation
> + * SUSPENDED - Capture is suspended
>   */
>  enum rvin_dma_state {
>  	STOPPED = 0,
>  	STARTING,
>  	RUNNING,
>  	STOPPING,
> +	SUSPENDED,
>  };
>
>  /**
> --
> 2.28.0
>

Re: [PATCH 5/5] rcar-vin: Add support for suspend and resume

[Jacopo Mondi]

Hi Niklas,

On Fri, Oct 16, 2020 at 04:15:03PM +0200, Niklas Söderlund wrote:
> Hi Jacopo,
>
> Thanks for your feedback.
>
> On 2020-10-16 18:07:18 +0200, Jacopo Mondi wrote:
> > Hi Niklas,
> >
> > On Fri, Oct 16, 2020 at 01:14:08AM +0200, Niklas Söderlund wrote:
> > > Add support for suspend and resume by stopping and starting the video
> > > pipeline while still retaining all buffers given to the driver by
> > > user-space and internally allocated ones, this gives the application a
> > > seamless experience.
> > >
> > > Buffers are never returned to user-space unprocessed so user-space don't
> > > notice when suspending. When resuming the driver restarts the capture
> > > session using the internal scratch buffer, this happens before
> > > user-space is unfrozen, this leads to speedy response times once the
> > > application resumes its execution.
> > >
> > > As the entire pipeline is stopped on suspend all subdevices in use are
> > > also stopped, and if they enter a shutdown state when not streaming
> > > (such as the R-Car CSI-2 driver) they too will be suspended and resumed
> > > in sync with the VIN driver.
> > >
> > > To be able to do keep track of which VINs should be resumed a new
> >
> > s/to do/to/
> >
> > > internal state SUSPENDED is added to recode this.
> > >
> > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> > > ---
> > >  drivers/media/platform/rcar-vin/rcar-core.c | 51 +++++++++++++++++++++
> > >  drivers/media/platform/rcar-vin/rcar-vin.h  | 10 ++--
> > >  2 files changed, 57 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c
> > > index 34d003e0e9b9c25a..4adf4ce518f79c93 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-core.c
> > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> > > @@ -918,6 +918,54 @@ static int rvin_mc_init(struct rvin_dev *vin)
> > >  	return ret;
> > >  }
> > >
> > > +/* -----------------------------------------------------------------------------
> > > + * Suspend / Resume
> > > + */
> > > +
> > > +static int __maybe_unused rvin_suspend(struct device *dev)
> > > +{
> > > +	struct rvin_dev *vin = dev_get_drvdata(dev);
> > > +
> > > +	if (vin->state != RUNNING)
> > > +		return 0;
> > > +
> > > +	rvin_stop_streaming(vin);
> >
> > This delay suspend untill all the userspace queued buffers are not
> > completed, right ?
>
> Yes it will delay suspend until all the buffers queued by user-space AND
> have been written to one of the 3 hardware slots are completed. So the
> worst case scenario is a delay of 3 frames to complete.
>
> Buffers queued by an application not yet commited to a slot are not
> waited for. Instead they are used when capture is resumed.

Ah right! I think exhausting the 3 filled slots it's an acceptable
delay during suspend operation.

Please add:
Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
which I forgot in the previous reply

Thanks
  j

>
> >
> > > +
> > > +	vin->state = SUSPENDED;
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static int __maybe_unused rvin_resume(struct device *dev)
> > > +{
> > > +	struct rvin_dev *vin = dev_get_drvdata(dev);
> > > +
> > > +	if (vin->state != SUSPENDED)
> > > +		return 0;
> > > +
> > > +	/*
> > > +	 * Restore group master CHSEL setting.
> > > +	 *
> > > +	 * This needs to be by every VIN resuming not only the master
> > > +	 * as we don't know if and in which order the master VINs will
> > > +	 * be resumed.
> > > +	 */
> > > +	if (vin->info->use_mc) {
> > > +		unsigned int master_id = rvin_group_id_to_master(vin->id);
> > > +		struct rvin_dev *master = vin->group->vin[master_id];
> > > +		int ret;
> > > +
> > > +		if (WARN_ON(!master))
> > > +			return -ENODEV;
> > > +
> > > +		ret = rvin_set_channel_routing(master, master->chsel);
> > > +		if (ret)
> > > +			return ret;
> > > +	}
> > > +
> > > +	return rvin_start_streaming(vin);
> > > +}
> > > +
> > >  /* -----------------------------------------------------------------------------
> > >   * Platform Device Driver
> > >   */
> > > @@ -1421,9 +1469,12 @@ static int rcar_vin_remove(struct platform_device *pdev)
> > >  	return 0;
> > >  }
> > >
> > > +static SIMPLE_DEV_PM_OPS(rvin_pm_ops, rvin_suspend, rvin_resume);
> > > +
> > >  static struct platform_driver rcar_vin_driver = {
> > >  	.driver = {
> > >  		.name = "rcar-vin",
> > > +		.pm = &rvin_pm_ops,
> > >  		.of_match_table = rvin_of_id_table,
> > >  	},
> > >  	.probe = rcar_vin_probe,
> > > diff --git a/drivers/media/platform/rcar-vin/rcar-vin.h b/drivers/media/platform/rcar-vin/rcar-vin.h
> > > index 4ec8584709c847a9..4539bd53d9d41e9c 100644
> > > --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> > > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> > > @@ -49,16 +49,18 @@ enum rvin_csi_id {
> > >  };
> > >
> > >  /**
> > > - * STOPPED  - No operation in progress
> > > - * STARTING - Capture starting up
> > > - * RUNNING  - Operation in progress have buffers
> > > - * STOPPING - Stopping operation
> > > + * STOPPED   - No operation in progress
> > > + * STARTING  - Capture starting up
> > > + * RUNNING   - Operation in progress have buffers
> > > + * STOPPING  - Stopping operation
> > > + * SUSPENDED - Capture is suspended
> > >   */
> > >  enum rvin_dma_state {
> > >  	STOPPED = 0,
> > >  	STARTING,
> > >  	RUNNING,
> > >  	STOPPING,
> > > +	SUSPENDED,
> > >  };
> > >
> > >  /**
> > > --
> > > 2.28.0
> > >
>
> --
> Regards,
> Niklas Söderlund