Re: [PATCH/RFC] v4l: vsp1: Fix Suspend-to-RAM
Hello Kaneko-san,
I'm so sorry for the very late reply, I had missed your patch :-/
First of all, thank you for the patch. Please see below for a few comments.
On Sunday 15 March 2015 23:33:07 Yoshihiro Kaneko wrote:
From: Sei Fumizono sei.fumizono...@hitachi-solutions.com
Fix Suspend-to-RAM
so that VSP1 driver continues to work after resuming.
In detail,
- Fix the judgment of ref count in resuming.
- Add stopping VSP1 during suspend.
That's two fixes, I believe they should be split in two patches, one to fix
the ref_count bug at resume time, and the other one to stop/restart the video
stream.
Signed-off-by: Sei Fumizono sei.fumizono...@hitachi-solutions.com
Signed-off-by: Yoshifumi Hosoya yoshifumi.hosoya...@renesas.com
Signed-off-by: Yoshihiro Kaneko ykaneko0...@gmail.com
---
This patch is based on the master branch of linuxtv.org/media_tree.git.
drivers/media/platform/vsp1/vsp1_drv.c | 39 +
drivers/media/platform/vsp1/vsp1_video.c | 31 -
drivers/media/platform/vsp1/vsp1_video.h | 5 +++-
3 files changed, 69 insertions(+), 6 deletions(-)
diff --git a/drivers/media/platform/vsp1/vsp1_drv.c
b/drivers/media/platform/vsp1/vsp1_drv.c index 913485a..b6e9cbc 100644
--- a/drivers/media/platform/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/vsp1/vsp1_drv.c
@@ -1,7 +1,7 @@
/*
* vsp1_drv.c -- R-Car VSP1 Driver
*
- * Copyright (C) 2013-2014 Renesas Electronics Corporation
+ * Copyright (C) 2013-2015 Renesas Electronics Corporation
*
* Contact: Laurent Pinchart (laurent.pinch...@ideasonboard.com)
*
@@ -397,26 +397,57 @@ void vsp1_device_put(struct vsp1_device *vsp1)
static int vsp1_pm_suspend(struct device *dev)
{
struct vsp1_device *vsp1 = dev_get_drvdata(dev);
+ unsigned int i = 0;
There's no need to initialize i to 0 here, the for loop takes care of it.
+ int ret = 0;
WARN_ON(mutex_is_locked(vsp1-lock));
if (vsp1-ref_count == 0)
return 0;
+ /* Suspend pipeline */
+ for (i = 0; i vsp1-pdata.wpf_count; ++i) {
+ struct vsp1_rwpf *wpf = vsp1-wpf[i];
+ struct vsp1_pipeline *pipe;
+
+ if (wpf == NULL)
+ continue;
+
+ pipe = to_vsp1_pipeline(wpf-entity.subdev.entity);
+ ret = vsp1_pipeline_suspend(pipe);
+ if (ret 0)
+ break;
+ }
This will stop all running pipelines sequentially, which would increase the
system suspend time quite a bit if all pipelines are running. I would instead
first set the state of all pipelines to VSP1_PIPELINE_STOPPING in one loop,
and then loop over all pipelines again to wait for them to finish. Something
like
/* To avoid increasing the system suspend time needlessly, loop over the
* pipelines twice, first to set them all to the stopping state, and
then
* to wait for the stop to complete.
*/
for (i = 0; i vsp1-pdata.wpf_count; ++i) {
struct vsp1_rwpf *wpf = vsp1-wpf[i];
struct vsp1_pipeline *pipe;
if (wpf == NULL)
continue;
pipe = to_vsp1_pipeline(wpf-entity.subdev.entity);
if (pipe == NULL)
continue;
spin_lock_irqsave(pipe-irqlock, flags);
if (pipe-state == VSP1_PIPELINE_RUNNING)
pipe-state = VSP1_PIPELINE_STOPPING;
spin_unlock_irqrestore(pipe-irqlock, flags);
}
for (i = 0; i vsp1-pdata.wpf_count; ++i) {
struct vsp1_rwpf *wpf = vsp1-wpf[i];
struct vsp1_pipeline *pipe;
if (wpf == NULL)
continue;
pipe = to_vsp1_pipeline(wpf-entity.subdev.entity);
if (pipe == NULL)
continue;
ret = wait_event_timeout(pipe-wq,
pipe-state == VSP1_PIPELINE_STOPPED,
msecs_to_jiffies(500));
if (ret == 0)
dev_warn(vsp1-dev, pipeline stop timeout\n);
}
You could move all that code to a vsp1_pipelines_suspend() function in
vsp1_video.c.
I've only printed a warning message instead of returning an error in case the
pipeline fails to stop, as I believe it shouldn't prevent the system from
entering suspend.
+
clk_disable_unprepare(vsp1-clock);
- return 0;
+ return ret;
}
static int vsp1_pm_resume(struct device *dev)
{
struct vsp1_device *vsp1 = dev_get_drvdata(dev);
+ unsigned int i = 0;
There's no need to initialize i to 0 here either.
WARN_ON(mutex_is_locked(vsp1-lock));
- if (vsp1-ref_count)
+ if (vsp1-ref_count == 0)
return 0;
- return clk_prepare_enable(vsp1-clock);
+ clk_prepare_enable(vsp1-clock);
+
+ /*
Re: [PATCH/RFC] v4l: vsp1: Fix Suspend-to-RAM
Hi Laurent,
I'm wondering if you could find a moment to look over this patch.
Regards,
Kaneko
2015-03-15 23:33 GMT+09:00 Yoshihiro Kaneko ykaneko0...@gmail.com:
From: Sei Fumizono sei.fumizono...@hitachi-solutions.com
Fix Suspend-to-RAM
so that VSP1 driver continues to work after resuming.
In detail,
- Fix the judgment of ref count in resuming.
- Add stopping VSP1 during suspend.
Signed-off-by: Sei Fumizono sei.fumizono...@hitachi-solutions.com
Signed-off-by: Yoshifumi Hosoya yoshifumi.hosoya...@renesas.com
Signed-off-by: Yoshihiro Kaneko ykaneko0...@gmail.com
---
This patch is based on the master branch of linuxtv.org/media_tree.git.
drivers/media/platform/vsp1/vsp1_drv.c | 39
drivers/media/platform/vsp1/vsp1_video.c | 31 -
drivers/media/platform/vsp1/vsp1_video.h | 5 +++-
3 files changed, 69 insertions(+), 6 deletions(-)
diff --git a/drivers/media/platform/vsp1/vsp1_drv.c
b/drivers/media/platform/vsp1/vsp1_drv.c
index 913485a..b6e9cbc 100644
--- a/drivers/media/platform/vsp1/vsp1_drv.c
+++ b/drivers/media/platform/vsp1/vsp1_drv.c
@@ -1,7 +1,7 @@
/*
* vsp1_drv.c -- R-Car VSP1 Driver
*
- * Copyright (C) 2013-2014 Renesas Electronics Corporation
+ * Copyright (C) 2013-2015 Renesas Electronics Corporation
*
* Contact: Laurent Pinchart (laurent.pinch...@ideasonboard.com)
*
@@ -397,26 +397,57 @@ void vsp1_device_put(struct vsp1_device *vsp1)
static int vsp1_pm_suspend(struct device *dev)
{
struct vsp1_device *vsp1 = dev_get_drvdata(dev);
+ unsigned int i = 0;
+ int ret = 0;
WARN_ON(mutex_is_locked(vsp1-lock));
if (vsp1-ref_count == 0)
return 0;
+ /* Suspend pipeline */
+ for (i = 0; i vsp1-pdata.wpf_count; ++i) {
+ struct vsp1_rwpf *wpf = vsp1-wpf[i];
+ struct vsp1_pipeline *pipe;
+
+ if (wpf == NULL)
+ continue;
+
+ pipe = to_vsp1_pipeline(wpf-entity.subdev.entity);
+ ret = vsp1_pipeline_suspend(pipe);
+ if (ret 0)
+ break;
+ }
+
clk_disable_unprepare(vsp1-clock);
- return 0;
+ return ret;
}
static int vsp1_pm_resume(struct device *dev)
{
struct vsp1_device *vsp1 = dev_get_drvdata(dev);
+ unsigned int i = 0;
WARN_ON(mutex_is_locked(vsp1-lock));
- if (vsp1-ref_count)
+ if (vsp1-ref_count == 0)
return 0;
- return clk_prepare_enable(vsp1-clock);
+ clk_prepare_enable(vsp1-clock);
+
+ /* Resume pipeline */
+ for (i = 0; i vsp1-pdata.wpf_count; ++i) {
+ struct vsp1_rwpf *wpf = vsp1-wpf[i];
+ struct vsp1_pipeline *pipe;
+
+ if (wpf == NULL)
+ continue;
+
+ pipe = to_vsp1_pipeline(wpf-entity.subdev.entity);
+ vsp1_pipeline_resume(pipe);
+ }
+
+ return 0;
}
#endif
diff --git a/drivers/media/platform/vsp1/vsp1_video.c
b/drivers/media/platform/vsp1/vsp1_video.c
index d91f19a..c744608 100644
--- a/drivers/media/platform/vsp1/vsp1_video.c
+++ b/drivers/media/platform/vsp1/vsp1_video.c
@@ -1,7 +1,7 @@
/*
* vsp1_video.c -- R-Car VSP1 Video Node
*
- * Copyright (C) 2013-2014 Renesas Electronics Corporation
+ * Copyright (C) 2013-2015 Renesas Electronics Corporation
*
* Contact: Laurent Pinchart (laurent.pinch...@ideasonboard.com)
*
@@ -662,6 +662,35 @@ done:
spin_unlock_irqrestore(pipe-irqlock, flags);
}
+int vsp1_pipeline_suspend(struct vsp1_pipeline *pipe)
+{
+ unsigned long flags;
+ int ret;
+
+ if (pipe == NULL)
+ return 0;
+
+ spin_lock_irqsave(pipe-irqlock, flags);
+ if (pipe-state == VSP1_PIPELINE_RUNNING)
+ pipe-state = VSP1_PIPELINE_STOPPING;
+ spin_unlock_irqrestore(pipe-irqlock, flags);
+
+ ret = wait_event_timeout(pipe-wq, pipe-state ==
VSP1_PIPELINE_STOPPED,
+msecs_to_jiffies(500));
+ ret = ret == 0 ? -ETIMEDOUT : 0;
+
+ return ret;
+}
+
+void vsp1_pipeline_resume(struct vsp1_pipeline *pipe)
+{
+ if (pipe == NULL)
+ return;
+
+ if (vsp1_pipeline_ready(pipe))
+ vsp1_pipeline_run(pipe);
+}
+
/*
* Propagate the alpha value through the pipeline.
*
diff --git a/drivers/media/platform/vsp1/vsp1_video.h
b/drivers/media/platform/vsp1/vsp1_video.h
index fd2851a..958a166 100644
--- a/drivers/media/platform/vsp1/vsp1_video.h
+++ b/drivers/media/platform/vsp1/vsp1_video.h
@@ -1,7 +1,7 @@
/*
* vsp1_video.h -- R-Car VSP1 Video Node
*
- * Copyright (C) 2013-2014 Renesas Electronics Corporation
+ * Copyright (C) 2013-2015 Renesas Electronics Corporation
*
* Contact: Laurent Pinchart
