Re: [PATCH 5/5] rcar-vin: Add support for suspend and resume

From: Jacopo Mondi <jacopo@jmondi.org>
To: "Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>,
	linux-media@vger.kernel.org, linux-renesas-soc@vger.kernel.org
Subject: Re: [PATCH 5/5] rcar-vin: Add support for suspend and resume
Date: Fri, 16 Oct 2020 19:26:00 +0200	[thread overview]
Message-ID: <20201016172600.n66gz2qmwmjjdwb3@uno.localdomain> (raw)
In-Reply-To: <20201016141503.kas7h5lumwm3wmal@oden.dyn.berto.se>

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