| Message ID | 20201015231408.2399933-6-niklas.soderlund+renesas@ragnatech.se (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Kieran Bingham |
| Headers | show |
| Series | rcar-vin: Support suspend and resume | expand |
Hi Niklas, On Fri, Oct 16, 2020 at 4:01 AM Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> wrote: > Add support for suspend and resume by stopping and starting the video > pipeline while still retaining all buffers given to the driver by > user-space and internally allocated ones, this gives the application a > seamless experience. > > Buffers are never returned to user-space unprocessed so user-space don't > notice when suspending. When resuming the driver restarts the capture > session using the internal scratch buffer, this happens before > user-space is unfrozen, this leads to speedy response times once the > application resumes its execution. > > As the entire pipeline is stopped on suspend all subdevices in use are > also stopped, and if they enter a shutdown state when not streaming > (such as the R-Car CSI-2 driver) they too will be suspended and resumed > in sync with the VIN driver. > > To be able to do keep track of which VINs should be resumed a new > internal state SUSPENDED is added to recode this. > > Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se> Thanks for your patch! > --- a/drivers/media/platform/rcar-vin/rcar-core.c > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > +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 to be done? > + * 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); > +} > + Gr{oetje,eeting}s, Geert
Hi Jacopo, Thanks for your feedback. On 2020-10-16 18:07:18 +0200, Jacopo Mondi wrote: > Hi Niklas, > > On Fri, Oct 16, 2020 at 01:14:08AM +0200, Niklas Söderlund wrote: > > Add support for suspend and resume by stopping and starting the video > > pipeline while still retaining all buffers given to the driver by > > user-space and internally allocated ones, this gives the application a > > seamless experience. > > > > Buffers are never returned to user-space unprocessed so user-space don't > > notice when suspending. When resuming the driver restarts the capture > > session using the internal scratch buffer, this happens before > > user-space is unfrozen, this leads to speedy response times once the > > application resumes its execution. > > > > As the entire pipeline is stopped on suspend all subdevices in use are > > also stopped, and if they enter a shutdown state when not streaming > > (such as the R-Car CSI-2 driver) they too will be suspended and resumed > > in sync with the VIN driver. > > > > To be able to do keep track of which VINs should be resumed a new > > s/to do/to/ > > > internal state SUSPENDED is added to recode this. > > > > 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 34d003e0e9b9c25a..4adf4ce518f79c93 100644 > > --- a/drivers/media/platform/rcar-vin/rcar-core.c > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > > @@ -918,6 +918,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); > > This delay suspend untill all the userspace queued buffers are not > completed, right ? Yes it will delay suspend until all the buffers queued by user-space AND have been written to one of the 3 hardware slots are completed. So the worst case scenario is a delay of 3 frames to complete. Buffers queued by an application not yet commited to a slot are not waited for. Instead they are used when capture is resumed. > > > + > > + 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 > > */ > > @@ -1421,9 +1469,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 4ec8584709c847a9..4539bd53d9d41e9c 100644 > > --- a/drivers/media/platform/rcar-vin/rcar-vin.h > > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h > > @@ -49,16 +49,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, > > }; > > > > /** > > -- > > 2.28.0 > >
Hi Niklas, On Fri, Oct 16, 2020 at 01:14:08AM +0200, Niklas Söderlund wrote: > Add support for suspend and resume by stopping and starting the video > pipeline while still retaining all buffers given to the driver by > user-space and internally allocated ones, this gives the application a > seamless experience. > > Buffers are never returned to user-space unprocessed so user-space don't > notice when suspending. When resuming the driver restarts the capture > session using the internal scratch buffer, this happens before > user-space is unfrozen, this leads to speedy response times once the > application resumes its execution. > > As the entire pipeline is stopped on suspend all subdevices in use are > also stopped, and if they enter a shutdown state when not streaming > (such as the R-Car CSI-2 driver) they too will be suspended and resumed > in sync with the VIN driver. > > To be able to do keep track of which VINs should be resumed a new s/to do/to/ > internal state SUSPENDED is added to recode this. > > 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 34d003e0e9b9c25a..4adf4ce518f79c93 100644 > --- a/drivers/media/platform/rcar-vin/rcar-core.c > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > @@ -918,6 +918,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); This delay suspend untill all the userspace queued buffers are not completed, right ? > + > + 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 > */ > @@ -1421,9 +1469,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 4ec8584709c847a9..4539bd53d9d41e9c 100644 > --- a/drivers/media/platform/rcar-vin/rcar-vin.h > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h > @@ -49,16 +49,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, > }; > > /** > -- > 2.28.0 >
Hi Niklas, On Fri, Oct 16, 2020 at 04:15:03PM +0200, Niklas Söderlund wrote: > Hi Jacopo, > > Thanks for your feedback. > > On 2020-10-16 18:07:18 +0200, Jacopo Mondi wrote: > > Hi Niklas, > > > > On Fri, Oct 16, 2020 at 01:14:08AM +0200, Niklas Söderlund wrote: > > > Add support for suspend and resume by stopping and starting the video > > > pipeline while still retaining all buffers given to the driver by > > > user-space and internally allocated ones, this gives the application a > > > seamless experience. > > > > > > Buffers are never returned to user-space unprocessed so user-space don't > > > notice when suspending. When resuming the driver restarts the capture > > > session using the internal scratch buffer, this happens before > > > user-space is unfrozen, this leads to speedy response times once the > > > application resumes its execution. > > > > > > As the entire pipeline is stopped on suspend all subdevices in use are > > > also stopped, and if they enter a shutdown state when not streaming > > > (such as the R-Car CSI-2 driver) they too will be suspended and resumed > > > in sync with the VIN driver. > > > > > > To be able to do keep track of which VINs should be resumed a new > > > > s/to do/to/ > > > > > internal state SUSPENDED is added to recode this. > > > > > > 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 34d003e0e9b9c25a..4adf4ce518f79c93 100644 > > > --- a/drivers/media/platform/rcar-vin/rcar-core.c > > > +++ b/drivers/media/platform/rcar-vin/rcar-core.c > > > @@ -918,6 +918,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); > > > > This delay suspend untill all the userspace queued buffers are not > > completed, right ? > > Yes it will delay suspend until all the buffers queued by user-space AND > have been written to one of the 3 hardware slots are completed. So the > worst case scenario is a delay of 3 frames to complete. > > Buffers queued by an application not yet commited to a slot are not > waited for. Instead they are used when capture is resumed. Ah right! I think exhausting the 3 filled slots it's an acceptable delay during suspend operation. Please add: Reviewed-by: Jacopo Mondi <jacopo+renesas@jmondi.org> which I forgot in the previous reply Thanks j > > > > > > + > > > + 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 > > > */ > > > @@ -1421,9 +1469,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 4ec8584709c847a9..4539bd53d9d41e9c 100644 > > > --- a/drivers/media/platform/rcar-vin/rcar-vin.h > > > +++ b/drivers/media/platform/rcar-vin/rcar-vin.h > > > @@ -49,16 +49,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, > > > }; > > > > > > /** > > > -- > > > 2.28.0 > > > > > -- > Regards, > Niklas Söderlund
diff --git a/drivers/media/platform/rcar-vin/rcar-core.c b/drivers/media/platform/rcar-vin/rcar-core.c index 34d003e0e9b9c25a..4adf4ce518f79c93 100644 --- a/drivers/media/platform/rcar-vin/rcar-core.c +++ b/drivers/media/platform/rcar-vin/rcar-core.c @@ -918,6 +918,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 */ @@ -1421,9 +1469,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 4ec8584709c847a9..4539bd53d9d41e9c 100644 --- a/drivers/media/platform/rcar-vin/rcar-vin.h +++ b/drivers/media/platform/rcar-vin/rcar-vin.h @@ -49,16 +49,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, }; /**
Add support for suspend and resume by stopping and starting the video pipeline while still retaining all buffers given to the driver by user-space and internally allocated ones, this gives the application a seamless experience. Buffers are never returned to user-space unprocessed so user-space don't notice when suspending. When resuming the driver restarts the capture session using the internal scratch buffer, this happens before user-space is unfrozen, this leads to speedy response times once the application resumes its execution. As the entire pipeline is stopped on suspend all subdevices in use are also stopped, and if they enter a shutdown state when not streaming (such as the R-Car CSI-2 driver) they too will be suspended and resumed in sync with the VIN driver. To be able to do keep track of which VINs should be resumed a new internal state SUSPENDED is added to recode this. 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(-)