| Message ID | 20181214061824.10296-5-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | rcar-vin: add support for suspend and resume | expand |
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, > }; > > /**
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, }; /**
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(-)