Re: [PATCH 2/3] drm/msm/gpu: Park scheduler threads for system suspend

[Christian König]

Am 17.03.22 um 10:59 schrieb Daniel Vetter:
> On Thu, Mar 10, 2022 at 03:46:05PM -0800, Rob Clark wrote:
> > From: Rob Clark <robdclark@xxxxxxxxxxxx>
> >
> > In the system suspend path, we don't want to be racing with the
> > scheduler kthreads pushing additional queued up jobs to the hw
> > queue (ringbuffer).  So park them first.  While we are at it,
> > move the wait for active jobs to complete into the new system-
> > suspend path.
> >
> > Signed-off-by: Rob Clark <robdclark@xxxxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/msm/adreno/adreno_device.c | 68 ++++++++++++++++++++--
> >   1 file changed, 64 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/adreno/adreno_device.c b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > index 8859834b51b8..0440a98988fc 100644
> > --- a/drivers/gpu/drm/msm/adreno/adreno_device.c
> > +++ b/drivers/gpu/drm/msm/adreno/adreno_device.c
> > @@ -619,22 +619,82 @@ static int active_submits(struct msm_gpu *gpu)
> >   static int adreno_runtime_suspend(struct device *dev)
> >   {
> >   	struct msm_gpu *gpu = dev_to_gpu(dev);
> > -	int remaining;
> > +
> > +	/*
> > +	 * We should be holding a runpm ref, which will prevent
> > +	 * runtime suspend.  In the system suspend path, we've
> > +	 * already waited for active jobs to complete.
> > +	 */
> > +	WARN_ON_ONCE(gpu->active_submits);
> > +
> > +	return gpu->funcs->pm_suspend(gpu);
> > +}
> > +
> > +static void suspend_scheduler(struct msm_gpu *gpu)
> > +{
> > +	int i;
> > +
> > +	/*
> > +	 * Shut down the scheduler before we force suspend, so that
> > +	 * suspend isn't racing with scheduler kthread feeding us
> > +	 * more work.
> > +	 *
> > +	 * Note, we just want to park the thread, and let any jobs
> > +	 * that are already on the hw queue complete normally, as
> > +	 * opposed to the drm_sched_stop() path used for handling
> > +	 * faulting/timed-out jobs.  We can't really cancel any jobs
> > +	 * already on the hw queue without racing with the GPU.
> > +	 */
> > +	for (i = 0; i < gpu->nr_rings; i++) {
> > +		struct drm_gpu_scheduler *sched = &gpu->rb[i]->sched;
> > +		kthread_park(sched->thread);
> Shouldn't we have some proper interfaces for this?

If I'm not completely mistaken we already should have one, yes.

> Also I'm kinda wondering how other drivers do this, feels like we should have a standard
> way.
>
> Finally not flushing out all in-flight requests sounds a bit like a bad
> idea for system suspend/resume since that's also the hibernation path, and
> that would mean your shrinker/page reclaim stops working. At least in full
> generality. Which ain't good for hibernation.

Completely agree, that looks like an incorrect workaround to me.

During suspend all userspace applications should be frozen and all f 
their hardware activity flushed out and waited for completion.

I do remember that our internal guys came up with pretty much the same 
idea and it sounded broken to me back then as well.

Regards,
Christian.

> Adding Christian and Andrey.
> -Daniel
>
> > +	}
> > +}
> > +
> > +static void resume_scheduler(struct msm_gpu *gpu)
> > +{
> > +	int i;
> > +
> > +	for (i = 0; i < gpu->nr_rings; i++) {
> > +		struct drm_gpu_scheduler *sched = &gpu->rb[i]->sched;
> > +		kthread_unpark(sched->thread);
> > +	}
> > +}
> > +
> > +static int adreno_system_suspend(struct device *dev)
> > +{
> > +	struct msm_gpu *gpu = dev_to_gpu(dev);
> > +	int remaining, ret;
> > +
> > +	suspend_scheduler(gpu);
> >   
> >   	remaining = wait_event_timeout(gpu->retire_event,
> >   				       active_submits(gpu) == 0,
> >   				       msecs_to_jiffies(1000));
> >   	if (remaining == 0) {
> >   		dev_err(dev, "Timeout waiting for GPU to suspend\n");
> > -		return -EBUSY;
> > +		ret = -EBUSY;
> > +		goto out;
> >   	}
> >   
> > -	return gpu->funcs->pm_suspend(gpu);
> > +	ret = pm_runtime_force_suspend(dev);
> > +out:
> > +	if (ret)
> > +		resume_scheduler(gpu);
> > +
> > +	return ret;
> >   }
> > +
> > +static int adreno_system_resume(struct device *dev)
> > +{
> > +	resume_scheduler(dev_to_gpu(dev));
> > +	return pm_runtime_force_resume(dev);
> > +}
> > +
> >   #endif
> >   
> >   static const struct dev_pm_ops adreno_pm_ops = {
> > -	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, pm_runtime_force_resume)
> > +	SET_SYSTEM_SLEEP_PM_OPS(adreno_system_suspend, adreno_system_resume)
> >   	SET_RUNTIME_PM_OPS(adreno_runtime_suspend, adreno_runtime_resume, NULL)
> >   };
> >   
> > --
> > 2.35.1