Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <3945551d-47d2-1974-f637-1dbc61e14702@amd.com>
Date:   Thu, 17 Mar 2022 11:06:18 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Subject: Re: [PATCH 2/3] drm/msm/gpu: Park scheduler threads for system
 suspend
Content-Language: en-US
To:     Rob Clark <robdclark@gmail.com>,
        Andrey Grodzovsky <andrey.grodzovsky@amd.com>,
        dri-devel@lists.freedesktop.org, freedreno@lists.freedesktop.org,
        linux-arm-msm@vger.kernel.org, Rob Clark <robdclark@chromium.org>,
        Sean Paul <sean@poorly.run>,
        Abhinav Kumar <quic_abhinavk@quicinc.com>,
        David Airlie <airlied@linux.ie>,
        Akhil P Oommen <quic_akhilpo@quicinc.com>,
        Jonathan Marek <jonathan@marek.ca>,
        AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@collabora.com>,
        Bjorn Andersson <bjorn.andersson@linaro.org>,
        Vladimir Lypak <vladimir.lypak@gmail.com>,
        open list <linux-kernel@vger.kernel.org>
References: <20220310234611.424743-1-robdclark@gmail.com>
 <20220310234611.424743-3-robdclark@gmail.com>
 <YjMGac4Hnjmg1wE8@phenom.ffwll.local>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
In-Reply-To: <YjMGac4Hnjmg1wE8@phenom.ffwll.local>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ekpCdDV5ajVjb00wMU9sblhNT2xHVEhKdm5FRUU2RlZxVlZ5bUpTdzhLanFi?=
 =?utf-8?B?ckp4QUN6d3UxTVZ5dXhlSERIbS8vWE5zUENzUU51RTZLRjY5YTBqYWdTRXhj?=
 =?utf-8?B?WFdXWWVmN0Q3WEk2d3ZXNFlWMXJraTVkWkl3VThpSHlRWDRLNlprN0oxa1VG?=
 =?utf-8?B?ZmwvMjl5OGxMWlRINitLWlZXcGNCY2IyeFlQQ041K3d3Zy9DSUdTbjZQNGZr?=
 =?utf-8?B?c254ZncwdkNGdml6TUhPUktXaXlDbVgyMmtFWVQyYTRZWEU1S2FTWnBuTzNB?=
 =?utf-8?B?TGFDbTJ0MjB3Z0w3VGFrZTRmSStIaWpPNVpSRzdxcEw1OVl5bWdYbG96WnNT?=
 =?utf-8?B?ZmgrNDVYS3l4eW5VUlRqNUVnK203SHBFVHJSTklVcGZkdUY5c3BDZ3Ewdk5R?=
 =?utf-8?B?UFBTL1RKUmxxMEpGMFpHM0c5RFVhNEtwTTRmUS90MGtrU1BOR2lNMXl0eFhn?=
 =?utf-8?B?eVAwMk9DQzV1TXNXeTRsMmRiL1lyMGlqRFU5MTlXcUVycm92ZDQ5cHZNc2tT?=
 =?utf-8?B?cFNHK1h6UjFibU1zZE5kbXlKbXVHOXhJRis1dGluemR5Ly95WEExMVp6djJD?=
 =?utf-8?B?VGJMSFVxcjFGak90andCZ2JGMUp0R0o1N04rQmVLSEFubWk1S2QwdG5MNCtM?=
 =?utf-8?B?Z0Uxdy9ranE5TGNxakdmcFZGU0NuSnJGbVZWOTMyUERkQmM2cmx3NmkvQzVt?=
 =?utf-8?B?WHN1MDZ1QTdUdHVWeEcvVUR3a1ZlcEZEMkJBTjhpYjBGOElaRTNaOGh2cVBD?=
 =?utf-8?B?azJsVTh0eTViaTBjWUJhZUsyV0F3VHFNaDZrMzRmU3JDelJGUGR1dmdRUWFr?=
 =?utf-8?B?YXltdUJTOXR6eTl1WC9GOWxtT2VlRXZIU01Sb0ZtWGl1UHlQY01NdXlHRkt2?=
 =?utf-8?B?eXpQT01nYVRqdkZRY2RMbWFkZzZzbUN6RlY4RTc0ZmY4UG8yMmFsblhBUlNv?=
 =?utf-8?B?TGFoT2FWSXA3VkFzWk5rdnlIOTFqSTcyMkJlekVRNUJjaFJDWTdoTm0zbVlG?=
 =?utf-8?B?VlEwazhGWlNKZU1leGpFbFltZy9RcUNFdU5xZm9sVElkbzZObHBBQXJTTlN3?=
 =?utf-8?B?RXFnTytBTDVRc0R3VHEyRk1pTkdkdVFta1dnTlpVR3dYUWNobSszZlJDZVdD?=
 =?utf-8?B?YTU3K0xQSEh5RDJETnN4VEZtbW5YV2dNbEdvNTN5QVhpMFVOcG84bXNFWEZK?=
 =?utf-8?B?NGk0UEFRcjZ0cXBVWUdISWlQQ3J4ZUFzd3hqWWQ1aXQvUDc4NUVRUndOVXk5?=
 =?utf-8?B?NG9yRTFMNXJwb1NSQldkSUk4TTBXQ3h0YzFpT0ljQjFzUDR1M051YWNRczI1?=
 =?utf-8?B?QWJMRGhHbVlOamFtaFovTmhVTE1lcVVUa3BTNzZsZllpWmc4bG4wZnBOb3R4?=
 =?utf-8?B?dDIxV2FPWW82bHcrZy84Y3JmTVhPQlR3QnBwVGNpMm81MFF2ajZYQWUvdGow?=
 =?utf-8?B?ZmpkUjhiRS9WR0labWIyakVJRE5aeWJPa2lOUmQyS1lGZ0Q3TWh6Qkd5RmJu?=
 =?utf-8?B?V1U0YmFVMmFnZmUvamN4M3JDN1B5bEZYSDRMTFQ1MEt3ak1zaEtVeTRUY2ZN?=
 =?utf-8?B?YWtXcGs5RmlhVGJGTE1zRUlQdVhSM3dkOWdPbVFqTXFzTHRKUy9hMXYvZlpZ?=
 =?utf-8?B?emxBanBxVTNvT3dDeVAzZ0xkM0w0ditsSk9qQUV6N3Z6bHBsR0hHcnBrK2p0?=
 =?utf-8?B?eTRKd2d1WFdpV3lCaFBLWGJQNlFhRmVoMkRFRjRTS1hrWm8wSnFVMjVJZFBR?=
 =?utf-8?B?cVlKWFJmYWQxVWZ5Y2VwVHRJQ29BdmhwMXA0L0ExUk5Yd3FoWjVIaFcvazZS?=
 =?utf-8?B?dWVrdDE1cmpxOHkrU3JvSjVlcTEzUlY1aE13VG5XY0duOUFheXkxQzZ2eWJY?=
 =?utf-8?B?OWxjalVWSEhXZHIxdFh2VHNBMExYaVZGNjRBeVhRZFlmR0VqU3JhcnB2bVh2?=
 =?utf-8?Q?ZA2dxs8J5B7WAVbmhTdSRigSLoaQypPO?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a8982b78-4e0a-43ea-ed85-08da07fdcbcc
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3587.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Mar 2022 10:06:25.5702
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: YC6Cdpb5HY9IRvqjBXaWl/RCLedIB1DTXhwT9VwkfXpDbTGobkiWZzKH11ktyOxJ
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR12MB3608
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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
>>