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@xxxxxxxxxxxx>
> > ---
> > 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
> >
--
Regards,
Niklas Söderlund
