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: <5beff5e2-8589-28cf-40b4-9e6b9e45b307@amd.com>
Date:   Thu, 29 Sep 2022 16:57:55 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Thunderbird/91.11.0
Subject: Re: [PATCH v4 5/6] drm/sched: Use parent fence instead of finished
Content-Language: en-US
To:     Steven Price <steven.price@arm.com>,
        Arvind Yadav <Arvind.Yadav@amd.com>, andrey.grodzovsky@amd.com,
        shashank.sharma@amd.com, amaranath.somalapuram@amd.com,
        Arunpravin.PaneerSelvam@amd.com, sumit.semwal@linaro.org,
        gustavo@padovan.org, airlied@linux.ie, daniel@ffwll.ch,
        linux-media@vger.kernel.org, dri-devel@lists.freedesktop.org,
        linaro-mm-sig@lists.linaro.org, linux-kernel@vger.kernel.org
References: <20220914164321.2156-1-Arvind.Yadav@amd.com>
 <20220914164321.2156-6-Arvind.Yadav@amd.com>
 <e613a225-dabf-7e87-2624-a3244df8a877@arm.com>
From:   =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>
In-Reply-To: <e613a225-dabf-7e87-2624-a3244df8a877@arm.com>
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?TkNBYWdDZFJSSGhualRwcDdTTzJxdDVPNlRzUEFmTDh4Qkt0QmM2QTAvR0pX?=
 =?utf-8?B?UVp3L1RzRDY4blM2eXg5Z3V5UGY5dGtpMFI2VWV0QVNkTHZkZ1lrOWVmWWsw?=
 =?utf-8?B?R1RkZlZ2OXNNdk1CL0pIZGsxR0Z0RXlrTU9sZ1MxTWIzWStqOEdpOUNJb2ZQ?=
 =?utf-8?B?WWo5cmpBNmpNRmthVGE2akZadTducXRITEpMU3pCRWJZeGxlYlhVSWt4UUFO?=
 =?utf-8?B?YnBLTmR3N05LQnJpSXRxZUZLR2V1WUIzdmgyZmllM1BUZnNEeTZwR0FKSXIy?=
 =?utf-8?B?Y09oNmNBSkJRZTROVjZsUDVQcW9nVFVoVHAyajRHUFFxR3V0ajUveVlGdzlC?=
 =?utf-8?B?dm82ZUdmYjJYRk45QU1OZGQzY25Neng5LzMzY2ljcndoYm52UFpzSjRLYTlz?=
 =?utf-8?B?Z0tFcGJSN2M1MUtwZ3RRaVBxSUtQVE9FdjMrcU9vemswSnorenF0S3hWLzFQ?=
 =?utf-8?B?WWtDcVM5MGJGc21MZUZyNERmU2dUSXIxSGd5eW1KT1dIWFBjVDlqYmZjbVNU?=
 =?utf-8?B?NS82VWVhVVhMRU9ydkxxU0tseTNlM2Rmekp4bXdkWmFmWWZUQUJsVlFMOWFD?=
 =?utf-8?B?d1gwcjd1bUV2RGhJRlBHMVpCVFJzUDZCNlhaTUxGSXp1c2FHNjJjc3lBdDlN?=
 =?utf-8?B?SmNrcUpPc0g1ZFg0dVh6V1JxSzRIcWFLUHdyT0t3ZitkbmZJeU96M04zTEM2?=
 =?utf-8?B?ejRNSGFnVlJQTTZrSWgzeUlyc3RrN3dYWm5SdDVPWWFmaFJUK2c0Wll5ZWNZ?=
 =?utf-8?B?VjhDMm1jOTI2bWtIYkVQRjBTUXRrWS82REtXQ3M0TjZIYktPVjNGZ2QwRjNp?=
 =?utf-8?B?VE1XeU5uKzM3cnRQd2hQT2JFL21MWDFmQk56Y2JxRWZrTXkza0pFMk9CeFBI?=
 =?utf-8?B?b0cySWx4RDlvVyt6dVoyb3hoeXpnU28yRlY3TzZ1QWZlZjd5dElQeURvTDBS?=
 =?utf-8?B?R1BDVDZUcGtwallFTXg4ZUk4bnQvSEJpOW5Ic1hwNXE2clRQSUNiMmNpbzlv?=
 =?utf-8?B?WUFyL0tXT1RoeWt5dWJZcGljOEZHS1RjNE4xTm4ybVRVaDJiTXVGclNHaVp5?=
 =?utf-8?B?RW1LWi93blVKWDJmbjdiU3QyNkU4YjUzdGxnWDlrL3lnSzMwSW92R0RFaTl6?=
 =?utf-8?B?d2NocFg2K1lnZHEzRGFKcndXYS9SUHZ5SmQ3YUNOVlRRQXFXd2RMeVd3ZVlO?=
 =?utf-8?B?V3JxMmw1Q2hueGRGeW4ySXdIMkwrcnFrL0FEaWdoU2MxTVBrK2VCMXFyQUFD?=
 =?utf-8?B?UXdtcWdsSjlLTmVVL1VwdDFHTG41eHRKcnlxcHhpRzZaajBBTVF4bldyT01Y?=
 =?utf-8?B?R2JTZW1jcTE1RmZndUZ5Mno3YmdybXdLWlVKVE1HMlN1a2crUkRGc2NuRGpD?=
 =?utf-8?B?K1BLVVhNT1Q1dHhXOW5Qa3ZMc2FTOWdrY0NFYkFsK1Q1TVUzUzYzMXpWeVBl?=
 =?utf-8?B?YUl1cEttT2RONDFmUm9UNGhOTUpVYWEwR1FCa3VwTlNKMmJNb1JxVVBRRG9a?=
 =?utf-8?B?Q1diZHltaHBycWlOeXpDUXVnSjAwOC8vWVNteDhSSFZnU2VCejZ5RlIya2ht?=
 =?utf-8?B?NHpxWHdxR2ROSWg2TGhudW5QYXN0QUN5NTJjWE4zL0FIUUxoSHhzazBYV1Y3?=
 =?utf-8?B?UzN5dmRkclJXZGtXRWR4N3p5TzQ3cU5pMzZJa0NSSVV5YzJrOUdENFpyWktx?=
 =?utf-8?B?a1dvTldKR1NDamtSNS9rcDlxVHZmemNOVDZSNVdQNStzazhzeGpFL2lrN2hs?=
 =?utf-8?B?Vm8yZ0ZqWjJxeWhBSXFHVWExM3pmMmFHVDRmZGJDSjhjQ3FIU1VZRGtKcFho?=
 =?utf-8?B?RWVDZmhSSFluVzNWcWExcHlXSHVFQkNkQUNXdCs1b3VWYjBTKysrRTgrNGp6?=
 =?utf-8?B?Z0IxQ1pEaXB3WmlkOWlMcTcxMmZuZEtKdDVYSm9TVUZzMEFYaUtaVTdOZ1FV?=
 =?utf-8?B?Qi9VRm9SSWlXaVRjYzJsOEpvcm9nZ3lqTUxON2kwVUV2aHZIZmROU1RveGlP?=
 =?utf-8?B?NFVJaFkvc01SZlY0WU1TbkwzeTl6MThZY3psZEg2aXhZamlnRWpCOUxqZFBx?=
 =?utf-8?B?Qm5OakJCYXc1clMvNU9odjBEczdSNThURFlFREd6c1ZUcktyTXNMQk1Db0hQ?=
 =?utf-8?B?dm1sQ2MzNWxJMkRUNEhOUnBuclF2ZjhZczRjQllFOGJVSmJYb3JJK3FwN2ti?=
 =?utf-8?Q?OAmL7wLR4NeenO9HDSBOWtghzsPjhOnAO0BDaZwTPIgi?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 4152e2aa-1101-408a-4c8f-08daa22b017c
X-MS-Exchange-CrossTenant-AuthSource: BN8PR12MB3587.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 29 Sep 2022 14:58:02.0750
 (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: TV57kVsLlkReHKhnMevbY9XJkhcBf4CDyimib0eLta7ySbBwUKtPNCardQDTCIRq
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS7PR12MB6191
X-Spam-Status: No, score=-6.2 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 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 29.09.22 um 16:53 schrieb Steven Price:
> On 14/09/2022 17:43, Arvind Yadav wrote:
>> Using the parent fence instead of the finished fence
>> to get the job status. This change is to avoid GPU
>> scheduler timeout error which can cause GPU reset.
> I'm able to reproduce crashes on Panfrost and I believe this commit is
> the cause. Specifically it's possible for job->s_fence->parent to be NULL.
>
> The underlying issue seems to involve drm_sched_resubmit_jobs_ext() - if
> the run_jobs() callback returns an error it will set s_fence->parent to
> NULL after signalling s_fence->finished:
>
>> 		fence = sched->ops->run_job(s_job);
>> 		i++;
>>
>> 		if (IS_ERR_OR_NULL(fence)) {
>> 			if (IS_ERR(fence))
>> 				dma_fence_set_error(&s_fence->finished, PTR_ERR(fence));
>>
>> 			s_job->s_fence->parent = NULL;
> I don't understand the reasoning behind this change, but it doesn't seem
> right to be using the parent fence when we have code which can be
> setting that pointer to NULL.
>
> Since I don't understand the reasoning my only suggestion is to revert
> this patch (and potentially the dependent patch "dma-buf: Check status
> of enable-signaling bit on debug"?).
>
> Can anyone suggest a better fix?

Well, first of all please absolutely don't use 
drm_sched_resubmit_jobs_ext()!

It was an extremely bad idea in amdgpu to approach GPU by re-submitting 
jobs and it was an even worse idea to push this into the scheduler.

The design of dma_fence is that you submit that once and *only* once and 
then get a result for this submission. If re-submission is desirable it 
should be done in userspace or at least higher levels.

Apart from that, yes a NULL check is missing here but that should be 
trivial to fix.

Thanks,
Christian.

>
> Thanks,
>
> Steve
>
>> Signed-off-by: Arvind Yadav <Arvind.Yadav@amd.com>
>> Reviewed-by: Andrey Grodzovsky <andrey.grodzovsky@amd.com>
>> ---
>>
>> changes in v1,v2 - Enable signaling for finished fence in sche_main()
>> is removed
>>
>> ---
>>   drivers/gpu/drm/scheduler/sched_main.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
>> index e0ab14e0fb6b..2ac28ad11432 100644
>> --- a/drivers/gpu/drm/scheduler/sched_main.c
>> +++ b/drivers/gpu/drm/scheduler/sched_main.c
>> @@ -829,7 +829,7 @@ drm_sched_get_cleanup_job(struct drm_gpu_scheduler *sched)
>>   	job = list_first_entry_or_null(&sched->pending_list,
>>   				       struct drm_sched_job, list);
>>   
>> -	if (job && dma_fence_is_signaled(&job->s_fence->finished)) {
>> +	if (job && dma_fence_is_signaled(job->s_fence->parent)) {
>>   		/* remove job from pending_list */
>>   		list_del_init(&job->list);
>>   
>> @@ -841,7 +841,7 @@ drm_sched_get_cleanup_job(struct drm_gpu_scheduler *sched)
>>   
>>   		if (next) {
>>   			next->s_fence->scheduled.timestamp =
>> -				job->s_fence->finished.timestamp;
>> +				job->s_fence->parent->timestamp;
>>   			/* start TO timer for next job */
>>   			drm_sched_start_timeout(sched);
>>   		}