Re: [PATCH 4/4] rcar-vin: add support for suspend and resume

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

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