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

Christian König
2022-03-17T10:06:43Z

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@chromium.org>
>>
>> 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@chromium.org>
>> ---
>> 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
>>