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+rene...@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
