linux-renesas-soc.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/4] rcar-vin: add support for suspend and resume
@ 2018-12-14  6:18 Niklas Söderlund
  2018-12-14  6:18 ` [PATCH 1/4] rcar-vin: fix wrong return value in rvin_set_channel_routing() Niklas Söderlund
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Niklas Söderlund @ 2018-12-14  6:18 UTC (permalink / raw)
  To: Laurent Pinchart, linux-media; +Cc: linux-renesas-soc, Niklas Söderlund

Hi,

This series add suspend and resume support for rcar-vin pipelines. It 
suspends all active pipelines and implicitly adds support for 
suspend/resume to rcar-csi2. It is tested on Gen3 and Gen2 and based 
on-top of latest media-tree.

Patch 1/4 fixes a bug in the driver which prevents suspend/resume to 
function properly. While patch 2/4 and 3/4 prepares for the feature and 
finally 4/4 enables support.

Niklas Söderlund (4):
  rcar-vin: fix wrong return value in rvin_set_channel_routing()
  rcar-vin: cache the CSI-2 channel selection value
  rcar-vin: make rvin_{start,stop}_streaming() available for internal
    use
  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  | 24 +++++++---
 drivers/media/platform/rcar-vin/rcar-vin.h  | 15 ++++--
 3 files changed, 79 insertions(+), 11 deletions(-)

-- 
2.19.2


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

* [PATCH 1/4] rcar-vin: fix wrong return value in rvin_set_channel_routing()
  2018-12-14  6:18 [PATCH 0/4] rcar-vin: add support for suspend and resume Niklas Söderlund
@ 2018-12-14  6:18 ` Niklas Söderlund
  2018-12-14  8:19   ` Laurent Pinchart
  2018-12-14  6:18 ` [PATCH 2/4] rcar-vin: cache the CSI-2 channel selection value Niklas Söderlund
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 9+ messages in thread
From: Niklas Söderlund @ 2018-12-14  6:18 UTC (permalink / raw)
  To: Laurent Pinchart, linux-media; +Cc: linux-renesas-soc, Niklas Söderlund

If the operation in rvin_set_channel_routing() is successful the 'ret'
variable contains the runtime PM use count for the VIN master device.
The intention is not to return the use count to the caller but to return
0 on success else none zero.

Fix this by always returning 0 if the operation is successful.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 92323310f7352147..beb9248992a48a74 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -1341,5 +1341,5 @@ int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel)
 
 	pm_runtime_put(vin->dev);
 
-	return ret;
+	return 0;
 }
-- 
2.19.2


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

* [PATCH 2/4] rcar-vin: cache the CSI-2 channel selection value
  2018-12-14  6:18 [PATCH 0/4] rcar-vin: add support for suspend and resume Niklas Söderlund
  2018-12-14  6:18 ` [PATCH 1/4] rcar-vin: fix wrong return value in rvin_set_channel_routing() Niklas Söderlund
@ 2018-12-14  6:18 ` Niklas Söderlund
  2018-12-14  8:23   ` Laurent Pinchart
  2018-12-14  6:18 ` [PATCH 3/4] rcar-vin: make rvin_{start,stop}_streaming() available for internal use Niklas Söderlund
  2018-12-14  6:18 ` [PATCH 4/4] rcar-vin: add support for suspend and resume Niklas Söderlund
  3 siblings, 1 reply; 9+ messages in thread
From: Niklas Söderlund @ 2018-12-14  6:18 UTC (permalink / raw)
  To: Laurent Pinchart, linux-media; +Cc: linux-renesas-soc, Niklas Söderlund

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

Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
---
 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 beb9248992a48a74..64f7636f94d6a0a3 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -1336,6 +1336,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 0b13b34d03e3dce4..d21fc991b7a9da36 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -170,6 +170,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
@@ -207,6 +208,7 @@ struct rvin_dev {
 	enum rvin_dma_state state;
 
 	bool is_csi;
+	unsigned int chsel;
 
 	u32 mbus_code;
 	struct v4l2_pix_format format;
-- 
2.19.2


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

* [PATCH 3/4] rcar-vin: make rvin_{start,stop}_streaming() available for internal use
  2018-12-14  6:18 [PATCH 0/4] rcar-vin: add support for suspend and resume Niklas Söderlund
  2018-12-14  6:18 ` [PATCH 1/4] rcar-vin: fix wrong return value in rvin_set_channel_routing() Niklas Söderlund
  2018-12-14  6:18 ` [PATCH 2/4] rcar-vin: cache the CSI-2 channel selection value Niklas Söderlund
@ 2018-12-14  6:18 ` Niklas Söderlund
  2018-12-14  8:27   ` Laurent Pinchart
  2018-12-14  6:18 ` [PATCH 4/4] rcar-vin: add support for suspend and resume Niklas Söderlund
  3 siblings, 1 reply; 9+ messages in thread
From: Niklas Söderlund @ 2018-12-14  6:18 UTC (permalink / raw)
  To: Laurent Pinchart, linux-media; +Cc: linux-renesas-soc, Niklas Söderlund

To support suspend/resume rvin_{start,stop}_streaming() needs to be
accessible from the suspend and resume callbacks. Up until now the only
users of these functions have been the callbacks in struct vb2_ops so
the arguments to the functions are not suitable for use by the driver it
self.

Fix this by adding wrappers for the struct vb2_ops callbacks which calls
the new rvin_{start,stop}_streaming() using more friendly arguments.

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

diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c b/drivers/media/platform/rcar-vin/rcar-dma.c
index 64f7636f94d6a0a3..d11d4df1906a8962 100644
--- a/drivers/media/platform/rcar-vin/rcar-dma.c
+++ b/drivers/media/platform/rcar-vin/rcar-dma.c
@@ -1143,9 +1143,8 @@ 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;
 
@@ -1187,9 +1186,13 @@ static int rvin_start_streaming(struct vb2_queue *vq, unsigned int count)
 	return ret;
 }
 
-static void rvin_stop_streaming(struct vb2_queue *vq)
+static int rvin_start_streaming_vq(struct vb2_queue *vq, unsigned int count)
+{
+	return rvin_start_streaming(vb2_get_drv_priv(vq));
+}
+
+void rvin_stop_streaming(struct rvin_dev *vin)
 {
-	struct rvin_dev *vin = vb2_get_drv_priv(vq);
 	unsigned long flags;
 	int retries = 0;
 
@@ -1238,12 +1241,17 @@ static void rvin_stop_streaming(struct vb2_queue *vq)
 			  vin->scratch_phys);
 }
 
+static void rvin_stop_streaming_vq(struct vb2_queue *vq)
+{
+	rvin_stop_streaming(vb2_get_drv_priv(vq));
+}
+
 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 d21fc991b7a9da36..700fae1c1225a2f3 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -269,4 +269,7 @@ void rvin_crop_scale_comp(struct rvin_dev *vin);
 
 int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel);
 
+int rvin_start_streaming(struct rvin_dev *vin);
+void rvin_stop_streaming(struct rvin_dev *vin);
+
 #endif
-- 
2.19.2


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

* [PATCH 4/4] rcar-vin: add support for suspend and resume
  2018-12-14  6:18 [PATCH 0/4] rcar-vin: add support for suspend and resume Niklas Söderlund
                   ` (2 preceding siblings ...)
  2018-12-14  6:18 ` [PATCH 3/4] rcar-vin: make rvin_{start,stop}_streaming() available for internal use Niklas Söderlund
@ 2018-12-14  6:18 ` Niklas Söderlund
  2018-12-14  8:37   ` Laurent Pinchart
  3 siblings, 1 reply; 9+ messages in thread
From: Niklas Söderlund @ 2018-12-14  6:18 UTC (permalink / raw)
  To: Laurent Pinchart, linux-media; +Cc: linux-renesas-soc, Niklas Söderlund

To be able to properly support suspend and resume the VIN and all
subdevices involved in a running capture needs to be stopped before the
system is suspended. Likewise the whole pipeline needs to be started
once the system is resumed if it was running.

Achieve this by using the existing rvin_{start,stop}_stream() functions
while making sure the CSI-2 channel selection is applied to the VIN
master before restarting the capture. To be able to do keep track of
which VINs should be resumed a new internal state SUSPENDED is added to
describe this state.

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 f0719ce24b97a9f9..7b34d69a97f4771d 100644
--- a/drivers/media/platform/rcar-vin/rcar-core.c
+++ b/drivers/media/platform/rcar-vin/rcar-core.c
@@ -862,6 +862,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
  */
@@ -1313,9 +1361,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 700fae1c1225a2f3..9bbc5a57fcb2915e 100644
--- a/drivers/media/platform/rcar-vin/rcar-vin.h
+++ b/drivers/media/platform/rcar-vin/rcar-vin.h
@@ -48,16 +48,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.19.2


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

* Re: [PATCH 1/4] rcar-vin: fix wrong return value in rvin_set_channel_routing()
  2018-12-14  6:18 ` [PATCH 1/4] rcar-vin: fix wrong return value in rvin_set_channel_routing() Niklas Söderlund
@ 2018-12-14  8:19   ` Laurent Pinchart
  0 siblings, 0 replies; 9+ messages in thread
From: Laurent Pinchart @ 2018-12-14  8:19 UTC (permalink / raw)
  To: Niklas Söderlund; +Cc: linux-media, linux-renesas-soc

Hi Niklas,

Thank you for the patch.

On Friday, 14 December 2018 08:18:21 EET Niklas Söderlund wrote:
> If the operation in rvin_set_channel_routing() is successful the 'ret'
> variable contains the runtime PM use count for the VIN master device.
> The intention is not to return the use count to the caller but to return
> 0 on success else none zero.
> 
> Fix this by always returning 0 if the operation is successful.
> 
> 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 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> b/drivers/media/platform/rcar-vin/rcar-dma.c index
> 92323310f7352147..beb9248992a48a74 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -1341,5 +1341,5 @@ int rvin_set_channel_routing(struct rvin_dev *vin, u8
> chsel)
> 
>  	pm_runtime_put(vin->dev);
> 
> -	return ret;
> +	return 0;
>  }


-- 
Regards,

Laurent Pinchart




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

* Re: [PATCH 2/4] rcar-vin: cache the CSI-2 channel selection value
  2018-12-14  6:18 ` [PATCH 2/4] rcar-vin: cache the CSI-2 channel selection value Niklas Söderlund
@ 2018-12-14  8:23   ` Laurent Pinchart
  0 siblings, 0 replies; 9+ messages in thread
From: Laurent Pinchart @ 2018-12-14  8:23 UTC (permalink / raw)
  To: Niklas Söderlund; +Cc: linux-media, linux-renesas-soc

Hi Niklas,

Thank you for the patch.

On Friday, 14 December 2018 08:18:22 EET Niklas Söderlund wrote:
> In preparation of suspend/resume support cache the chsel value when we
> write it to the register so it can be restored on resume if needed.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  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
> beb9248992a48a74..64f7636f94d6a0a3 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -1336,6 +1336,8 @@ int rvin_set_channel_routing(struct rvin_dev *vin, u8
> chsel)
> 
>  	vin_dbg(vin, "Set IFMD 0x%x\n", ifmd);
> 
> +	vin->chsel = chsel;
> +

Would it be useful to add a

	if (vin->chsel == chsel)
		return 0;

at the beginning of the function, or is that impossible ?

>  	/* 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
> 0b13b34d03e3dce4..d21fc991b7a9da36 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -170,6 +170,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

Nitpicking, the documentation for other fields don't start with a capital 
letter.

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

>   *
>   * @mbus_code:		media bus format code
>   * @format:		active V4L2 pixel format
> @@ -207,6 +208,7 @@ struct rvin_dev {
>  	enum rvin_dma_state state;
> 
>  	bool is_csi;
> +	unsigned int chsel;
> 
>  	u32 mbus_code;
>  	struct v4l2_pix_format format;

-- 
Regards,

Laurent Pinchart




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

* Re: [PATCH 3/4] rcar-vin: make rvin_{start,stop}_streaming() available for internal use
  2018-12-14  6:18 ` [PATCH 3/4] rcar-vin: make rvin_{start,stop}_streaming() available for internal use Niklas Söderlund
@ 2018-12-14  8:27   ` Laurent Pinchart
  0 siblings, 0 replies; 9+ messages in thread
From: Laurent Pinchart @ 2018-12-14  8:27 UTC (permalink / raw)
  To: Niklas Söderlund; +Cc: linux-media, linux-renesas-soc

Hi Niklas,

Thank you for the patch.

On Friday, 14 December 2018 08:18:23 EET Niklas Söderlund wrote:
> To support suspend/resume rvin_{start,stop}_streaming() needs to be
> accessible from the suspend and resume callbacks. Up until now the only
> users of these functions have been the callbacks in struct vb2_ops so
> the arguments to the functions are not suitable for use by the driver it
> self.
> 
> Fix this by adding wrappers for the struct vb2_ops callbacks which calls
> the new rvin_{start,stop}_streaming() using more friendly arguments.
> 
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/media/platform/rcar-vin/rcar-dma.c | 20 ++++++++++++++------
>  drivers/media/platform/rcar-vin/rcar-vin.h |  3 +++
>  2 files changed, 17 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/platform/rcar-vin/rcar-dma.c
> b/drivers/media/platform/rcar-vin/rcar-dma.c index
> 64f7636f94d6a0a3..d11d4df1906a8962 100644
> --- a/drivers/media/platform/rcar-vin/rcar-dma.c
> +++ b/drivers/media/platform/rcar-vin/rcar-dma.c
> @@ -1143,9 +1143,8 @@ 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;
> 
> @@ -1187,9 +1186,13 @@ static int rvin_start_streaming(struct vb2_queue *vq,
> unsigned int count) return ret;
>  }
> 
> -static void rvin_stop_streaming(struct vb2_queue *vq)
> +static int rvin_start_streaming_vq(struct vb2_queue *vq, unsigned int
> count)
> +{
> +	return rvin_start_streaming(vb2_get_drv_priv(vq));
> +}
> +
> +void rvin_stop_streaming(struct rvin_dev *vin)
>  {
> -	struct rvin_dev *vin = vb2_get_drv_priv(vq);
>  	unsigned long flags;
>  	int retries = 0;
> 
> @@ -1238,12 +1241,17 @@ static void rvin_stop_streaming(struct vb2_queue
> *vq) vin->scratch_phys);
>  }
> 
> +static void rvin_stop_streaming_vq(struct vb2_queue *vq)
> +{
> +	rvin_stop_streaming(vb2_get_drv_priv(vq));
> +}

You'll need a bit more than this. rvin_stop_streaming() calls 
return_all_buffers() and dma_free_coherent() which you don't want at suspend 
time. Buffers should not be returned to userspace but kept for reuse, and it's 
pointless to free the scratch buffer at suspend time to reallocate it at 
resume time.

>  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
> d21fc991b7a9da36..700fae1c1225a2f3 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -269,4 +269,7 @@ void rvin_crop_scale_comp(struct rvin_dev *vin);
> 
>  int rvin_set_channel_routing(struct rvin_dev *vin, u8 chsel);
> 
> +int rvin_start_streaming(struct rvin_dev *vin);
> +void rvin_stop_streaming(struct rvin_dev *vin);
> +
>  #endif


-- 
Regards,

Laurent Pinchart




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

* Re: [PATCH 4/4] rcar-vin: add support for suspend and resume
  2018-12-14  6:18 ` [PATCH 4/4] rcar-vin: add support for suspend and resume Niklas Söderlund
@ 2018-12-14  8:37   ` Laurent Pinchart
  0 siblings, 0 replies; 9+ messages in thread
From: Laurent Pinchart @ 2018-12-14  8:37 UTC (permalink / raw)
  To: Niklas Söderlund; +Cc: linux-media, linux-renesas-soc

Hi Niklas,

Thank you for the patch.

On Friday, 14 December 2018 08:18:24 EET Niklas Söderlund wrote:
> To be able to properly support suspend and resume the VIN and all
> subdevices involved in a running capture needs to be stopped before the
> system is suspended. Likewise the whole pipeline needs to be started
> once the system is resumed if it was running.
> 
> Achieve this by using the existing rvin_{start,stop}_stream() functions
> while making sure the CSI-2 channel selection is applied to the VIN
> master before restarting the capture. To be able to do keep track of
> which VINs should be resumed a new internal state SUSPENDED is added to
> describe this state.
> 
> 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
> f0719ce24b97a9f9..7b34d69a97f4771d 100644
> --- a/drivers/media/platform/rcar-vin/rcar-core.c
> +++ b/drivers/media/platform/rcar-vin/rcar-core.c
> @@ -862,6 +862,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;

Could this race with userspace starting or stopping a stream ?

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

What happens if the master isn't resumed yet, could it cause access to 
hardware with clocks disabled ? I don't expect pm_runtime_get_sync() to 
happily handle suspended devices.

> +	}
> +
> +	return rvin_start_streaming(vin);
> +}

Note for later, it would be nice to have suspend/resume helpers in V4L2 that 
would stop/start streaming and generally exercise the driver through its V4L2 
API only, to avoid the need for custom suspend/resume code.

>  /* ------------------------------------------------------------------------
>   * Platform Device Driver
>   */
> @@ -1313,9 +1361,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
> 700fae1c1225a2f3..9bbc5a57fcb2915e 100644
> --- a/drivers/media/platform/rcar-vin/rcar-vin.h
> +++ b/drivers/media/platform/rcar-vin/rcar-vin.h
> @@ -48,16 +48,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

While at it, could you convert this to proper kerneldoc ?

>   */
>  enum rvin_dma_state {
>  	STOPPED = 0,
>  	STARTING,
>  	RUNNING,
>  	STOPPING,
> +	SUSPENDED,
>  };
> 
>  /**

-- 
Regards,

Laurent Pinchart




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

end of thread, other threads:[~2018-12-14  8:36 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-14  6:18 [PATCH 0/4] rcar-vin: add support for suspend and resume Niklas Söderlund
2018-12-14  6:18 ` [PATCH 1/4] rcar-vin: fix wrong return value in rvin_set_channel_routing() Niklas Söderlund
2018-12-14  8:19   ` Laurent Pinchart
2018-12-14  6:18 ` [PATCH 2/4] rcar-vin: cache the CSI-2 channel selection value Niklas Söderlund
2018-12-14  8:23   ` Laurent Pinchart
2018-12-14  6:18 ` [PATCH 3/4] rcar-vin: make rvin_{start,stop}_streaming() available for internal use Niklas Söderlund
2018-12-14  8:27   ` Laurent Pinchart
2018-12-14  6:18 ` [PATCH 4/4] rcar-vin: add support for suspend and resume Niklas Söderlund
2018-12-14  8:37   ` Laurent Pinchart

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).