Re: [PATCH/RFC] v4l: vsp1: Fix Suspend-to-RAM

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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.jw@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.jw@hitachi-solutions.com>
> Signed-off-by: Yoshifumi Hosoya <yoshifumi.hosoya.wj@renesas.com>
> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@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.pinchart@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);
> +
> +	/* 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);
> +	}

If you move the suspend code to a vsp1_pipelines_suspend() function, I believe 
it would make sense to move that loop to a vsp1_pipelines_resume() function as 
well. I would then just move to code from vsp1_pipeline_resume() inside the 
loop.

Would you like to submit a new version of this patch (split in two) ? I can 
also perform the modifications myself if you prefer.

> +
> +	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.pinchart@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 (laurent.pinchart@ideasonboard.com)
>   *
> @@ -149,4 +149,7 @@ void vsp1_pipeline_propagate_alpha(struct vsp1_pipeline
> *pipe, struct vsp1_entity *input,
>  				   unsigned int alpha);
> 
> +int vsp1_pipeline_suspend(struct vsp1_pipeline *pipe);
> +void vsp1_pipeline_resume(struct vsp1_pipeline *pipe);
> +
>  #endif /* __VSP1_VIDEO_H__ */

-- 
Regards,

Laurent Pinchart