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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3945551d-47d2-1974-f637-1dbc61e14702@amd.com>
Date:   Thu, 17 Mar 2022 11:06:18 +0100
From:   Christian König <christian.koenig@....com>
To:     Rob Clark <robdclark@...il.com>,
        Andrey Grodzovsky <andrey.grodzovsky@....com>,
        dri-devel@...ts.freedesktop.org, freedreno@...ts.freedesktop.org,
        linux-arm-msm@...r.kernel.org, Rob Clark <robdclark@...omium.org>,
        Sean Paul <sean@...rly.run>,
        Abhinav Kumar <quic_abhinavk@...cinc.com>,
        David Airlie <airlied@...ux.ie>,
        Akhil P Oommen <quic_akhilpo@...cinc.com>,
        Jonathan Marek <jonathan@...ek.ca>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        Bjorn Andersson <bjorn.andersson@...aro.org>,
        Vladimir Lypak <vladimir.lypak@...il.com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/3] drm/msm/gpu: Park scheduler threads for system
 suspend

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