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@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.
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@xxxxxxxxxx>
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
