Received-SPF: pass (google.com: domain of linux-kernel+bounces-35802-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <ed29dcb8-bd2e-4de9-8310-81d8eead6be3@amd.com>
Date: Tue, 23 Jan 2024 11:46:27 -0600
User-Agent: Mozilla Thunderbird
Subject: Re: [RFC] Revert "drm/sched: Split free_job into own work item"
Content-Language: en-US
To: Luben Tuikov <ltuikov89@gmail.com>,
 Matthew Brost <matthew.brost@intel.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>,
 Maxime Ripard <mripard@kernel.org>, Thomas Zimmermann <tzimmermann@suse.de>,
 David Airlie <airlied@gmail.com>, Daniel Vetter <daniel@ffwll.ch>,
 =?UTF-8?Q?Christian_K=C3=B6nig?= <christian.koenig@amd.com>,
 "open list:DRM GPU SCHEDULER" <dri-devel@lists.freedesktop.org>,
 open list <linux-kernel@vger.kernel.org>
References: <20240123021155.2775-1-mario.limonciello@amd.com>
From: Mario Limonciello <mario.limonciello@amd.com>
In-Reply-To: <20240123021155.2775-1-mario.limonciello@amd.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?Q3JZSGhaVU54V2Q3N3NodUFtTnlHdk40ZGFKd3cwZXl5N0xzTkwzK05ZZStJ?=
 =?utf-8?B?bXpYaEdFUnhBdllQSlhoNjVnNnl4MElwdFlLZk5PR093WXhGZUxWME1GTC9E?=
 =?utf-8?B?VmpFOFYyaVREWm80a2dkN0hLODZmOWc1QlBmQ2svNE1TVUkwK0oxUnJ4ZGl3?=
 =?utf-8?B?YmtWWWVqY01BNnlCa3h6dEp6OHVMNlgyS0NqYktiMWhKV2YyeXJEV2tlQldJ?=
 =?utf-8?B?NGF5UVlhNE5UcXhjTVgxcjdUVFF4N3NOTjIvREw3ZDFya0Y5UU9iUnlGM3Jw?=
 =?utf-8?B?bC8yMWRBTVVWY211K2JrRVdJL0JadlZqd0dkRDFBNkxsY3FxZlVYVmtPTHBO?=
 =?utf-8?B?S3ZTWEloN3RaajROOXZmTW9FMDBXaXVOaEc2d21FZ0hxMmNzZDU3am55Wm1n?=
 =?utf-8?B?ZDd2UElXV21pL3VyZFZBU3ozNTRRaGx6VG5qdGtKODJmZGRZK3hPODZhSnAx?=
 =?utf-8?B?Q25scWZxejdJeG8zQnpyRDd0M3kvVU15VHRhOW4vVzRvN3ptamVOeVVmUnBP?=
 =?utf-8?B?b0ZUbnpQbHJnU0dEWkxmMTFHTVplRnBWcXhzSXVHV1hWQUprRmtnWWw3ZXQ2?=
 =?utf-8?B?UlhuUmx0WjJldVc0K1JKNDJ0YjlnY3MyV3FBQzR5NHNRRGlIcXNvSW0vc28z?=
 =?utf-8?B?R2krSkV0NVVsSEFjVUU0Q0pFdzhZaVA2NVN4TjdJaktkSElaSllzajAvNVgv?=
 =?utf-8?B?WmFndEFmQlA1WFVMRTZCUUpKWmpGMm1tSFQ4SytLbzVrcXc1MGUzWjdSOGVG?=
 =?utf-8?B?Zm1OOXQ2ZHJOcGVyUHdleWNQVHNMNnpRYWNma3hUcHZ1bjFOdmc3SjF3UkY0?=
 =?utf-8?B?aFdlRWlMM0wrYmN3QnpWSjZGd3JGdG9YRlM5em0ydzZXVFIrOWNOcUo4NzJX?=
 =?utf-8?B?em9VSHZ1USs3cXA2WVcrQlZuM3pNNnlzNW1pektiMGV5NUw3R2MzMTdKbHhF?=
 =?utf-8?B?Wk9iOWFxelBETkhsYmF0MzlaNFU1WDJVWXg1MDhmNm0rM21wZmxUcTg3ekxW?=
 =?utf-8?B?c0dmTGVtRFRrVWtCN24vRjI0bCt6Z3NnSHMvSWRpNUFnUjU3QjZibUFOdjJT?=
 =?utf-8?B?Mlh6d1hoUm5QQ3JSOUNzaGQweVhpSGMwNzdQMjFyZ1BsMHNDQWdSK0tPN1hO?=
 =?utf-8?B?dFhxTlJ1V0FIRXBoQk1PcFY5dmdReE5OZXdJbzdEWUdBYURpYVJhTWRaZkRr?=
 =?utf-8?B?OXdjNGpLOTU4bWcxNDEraGo0QmhFVExnQ3NhcXc1YTVnSzZIWm9pcWlqa29j?=
 =?utf-8?B?YklXL0RnRlhyV2ZvYzh1MTFaV1RsTUFkN05FeFl4aEdYYlM4enpsU3B1S3lt?=
 =?utf-8?B?eXRHaWUyak9walF4azhLMlNqcHhZOStqNjlnUHVkNm02ZHhDdDZPU1dxT3lw?=
 =?utf-8?B?c2hrdUwxMXc5KzFnSnBrMDZzYk9mN1FjVWpzYU1WL1BkektNY2tEcEZSaG5n?=
 =?utf-8?B?em05bnUrNVJWRm1XNlpDeWZXYVgrNkRQK0dhZkgvWjlVTU5KeTFOS3crSUNZ?=
 =?utf-8?B?MFN0U2xFZEc2NGUzM1pXNk9HWGNEd09SVlY5SkdDVm1ja2R5anJmSVFMbHRW?=
 =?utf-8?B?a29wRTIwZHpGUXZzWFdMSUluSlVzQXlIU1ZNR1RkYU5kc0xSN0E3NXpuMTBx?=
 =?utf-8?B?OUxmU1ZMK3Bxa3dXY1V6MmxyckpDdWJWOXhWV0xxbzh5cjEyb0EvL3FXYXdK?=
 =?utf-8?B?d2cyRWpuSjkvYTZkOUlqbGlBRTluU0RCK3JlZGpHZnZ6Nk5mNVNhY0Rvbk1V?=
 =?utf-8?B?SmdBL29tT3VYTk0zNEFmaG85NldIUnFTRFlKekVHV2tJNmR4L1FhNjZWb1cw?=
 =?utf-8?B?ZlZQMVRzTjdPclhBUjMySFo5cERKT1BPMEU5Q0V3ZnVodHlIc0thM2wxRytw?=
 =?utf-8?B?aEk5NjlTRHZZZnMxZmdEeW1oSFFncFB5ZjA3eHdoaUg3bUlPQWh0NmRwbExv?=
 =?utf-8?B?cWY5VVJmRy91TiswVWc1S0pPaXhlUGl1SVIrSERLb0gxc2lpQno0M1lCZG9Q?=
 =?utf-8?B?OFlVZjN0MWlkdEN1VjhoVC9nRVBmbGNzUDBYanB3bExKdVF2NXFHKzhNVjFy?=
 =?utf-8?B?NzZ6UVRSeWlZRlNIS1R2cVlJd0dLdmo4ZUJ5TGhha2VDaTRqdHY1ZHJhM2ZL?=
 =?utf-8?Q?QWQs6tzvFFjdhK/7Jl6dBlQwx?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 00ebba99-8a93-4772-d868-08dc1c3b3b63
X-MS-Exchange-CrossTenant-AuthSource: MN0PR12MB6101.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Jan 2024 17:46:30.8881
 (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: JgI3jzuyfcAP5GSqpR5g9lWc5TMqB5UqB2jpcFAjfrSFUJZKRl8Cb2vZhGk3a+dAoFuefwvGvMQEF3sdRRhIZQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR12MB5014

On 1/22/2024 20:11, Mario Limonciello wrote:
> commit f7fe64ad0f22 ("drm/sched: Split free_job into own work item")
> causes graphics hangs at GDM or right after logging in on a
> Framework 13 AMD laptop (containing a Phoenix APU).
> 
> This reverts commit f7fe64ad0f22ff034f8ebcfbd7299ee9cc9b57d7.
> 
> Fixes: f7fe64ad0f22 ("drm/sched: Split free_job into own work item")
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
> This is a regression introduced in 6.8-rc1, bisected from 6.7.
> This revert done on top of 6.8-rc1 fixes the issue.
> 
> I'm happy to gather any data to use to properly debug if that is
> preferable to a revert.

BTW - nothing in kernel log when this happens.
I did catch an ftrace with all the GPU scheduler trace events though:

https://gist.github.com/superm1/1d3fc752a414f324fbde526052573acd

> ---
>   drivers/gpu/drm/scheduler/sched_main.c | 133 +++++++++----------------
>   include/drm/gpu_scheduler.h            |   4 +-
>   2 files changed, 48 insertions(+), 89 deletions(-)
> 
> diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c
> index 550492a7a031..91c96ab53fb5 100644
> --- a/drivers/gpu/drm/scheduler/sched_main.c
> +++ b/drivers/gpu/drm/scheduler/sched_main.c
> @@ -369,32 +369,6 @@ static void drm_sched_run_job_queue(struct drm_gpu_scheduler *sched)
>   		queue_work(sched->submit_wq, &sched->work_run_job);
>   }
>   
> -/**
> - * __drm_sched_run_free_queue - enqueue free-job work
> - * @sched: scheduler instance
> - */
> -static void __drm_sched_run_free_queue(struct drm_gpu_scheduler *sched)
> -{
> -	if (!READ_ONCE(sched->pause_submit))
> -		queue_work(sched->submit_wq, &sched->work_free_job);
> -}
> -
> -/**
> - * drm_sched_run_free_queue - enqueue free-job work if ready
> - * @sched: scheduler instance
> - */
> -static void drm_sched_run_free_queue(struct drm_gpu_scheduler *sched)
> -{
> -	struct drm_sched_job *job;
> -
> -	spin_lock(&sched->job_list_lock);
> -	job = list_first_entry_or_null(&sched->pending_list,
> -				       struct drm_sched_job, list);
> -	if (job && dma_fence_is_signaled(&job->s_fence->finished))
> -		__drm_sched_run_free_queue(sched);
> -	spin_unlock(&sched->job_list_lock);
> -}
> -
>   /**
>    * drm_sched_job_done - complete a job
>    * @s_job: pointer to the job which is done
> @@ -414,7 +388,7 @@ static void drm_sched_job_done(struct drm_sched_job *s_job, int result)
>   	dma_fence_get(&s_fence->finished);
>   	drm_sched_fence_finished(s_fence, result);
>   	dma_fence_put(&s_fence->finished);
> -	__drm_sched_run_free_queue(sched);
> +	drm_sched_run_job_queue(sched);
>   }
>   
>   /**
> @@ -1092,10 +1066,8 @@ drm_sched_get_finished_job(struct drm_gpu_scheduler *sched)
>   						typeof(*next), list);
>   
>   		if (next) {
> -			if (test_bit(DMA_FENCE_FLAG_TIMESTAMP_BIT,
> -				     &next->s_fence->scheduled.flags))
> -				next->s_fence->scheduled.timestamp =
> -					dma_fence_timestamp(&job->s_fence->finished);
> +			next->s_fence->scheduled.timestamp =
> +				dma_fence_timestamp(&job->s_fence->finished);
>   			/* start TO timer for next job */
>   			drm_sched_start_timeout(sched);
>   		}
> @@ -1145,29 +1117,7 @@ drm_sched_pick_best(struct drm_gpu_scheduler **sched_list,
>   EXPORT_SYMBOL(drm_sched_pick_best);
>   
>   /**
> - * drm_sched_free_job_work - worker to call free_job
> - *
> - * @w: free job work
> - */
> -static void drm_sched_free_job_work(struct work_struct *w)
> -{
> -	struct drm_gpu_scheduler *sched =
> -		container_of(w, struct drm_gpu_scheduler, work_free_job);
> -	struct drm_sched_job *job;
> -
> -	if (READ_ONCE(sched->pause_submit))
> -		return;
> -
> -	job = drm_sched_get_finished_job(sched);
> -	if (job)
> -		sched->ops->free_job(job);
> -
> -	drm_sched_run_free_queue(sched);
> -	drm_sched_run_job_queue(sched);
> -}
> -
> -/**
> - * drm_sched_run_job_work - worker to call run_job
> + * drm_sched_run_job_work - main scheduler thread
>    *
>    * @w: run job work
>    */
> @@ -1176,50 +1126,64 @@ static void drm_sched_run_job_work(struct work_struct *w)
>   	struct drm_gpu_scheduler *sched =
>   		container_of(w, struct drm_gpu_scheduler, work_run_job);
>   	struct drm_sched_entity *entity;
> -	struct dma_fence *fence;
> -	struct drm_sched_fence *s_fence;
> -	struct drm_sched_job *sched_job;
> +	struct drm_sched_job *cleanup_job;
>   	int r;
>   
>   	if (READ_ONCE(sched->pause_submit))
>   		return;
>   
> +	cleanup_job = drm_sched_get_finished_job(sched);
>   	entity = drm_sched_select_entity(sched);
> -	if (!entity)
> -		return;
>   
> -	sched_job = drm_sched_entity_pop_job(entity);
> -	if (!sched_job) {
> -		complete_all(&entity->entity_idle);
> +	if (!entity && !cleanup_job)
>   		return;	/* No more work */
> -	}
>   
> -	s_fence = sched_job->s_fence;
> +	if (cleanup_job)
> +		sched->ops->free_job(cleanup_job);
>   
> -	atomic_add(sched_job->credits, &sched->credit_count);
> -	drm_sched_job_begin(sched_job);
> +	if (entity) {
> +		struct dma_fence *fence;
> +		struct drm_sched_fence *s_fence;
> +		struct drm_sched_job *sched_job;
> +
> +		sched_job = drm_sched_entity_pop_job(entity);
> +		if (!sched_job) {
> +			complete_all(&entity->entity_idle);
> +			if (!cleanup_job)
> +				return;	/* No more work */
> +			goto again;
> +		}
>   
> -	trace_drm_run_job(sched_job, entity);
> -	fence = sched->ops->run_job(sched_job);
> -	complete_all(&entity->entity_idle);
> -	drm_sched_fence_scheduled(s_fence, fence);
> +		s_fence = sched_job->s_fence;
>   
> -	if (!IS_ERR_OR_NULL(fence)) {
> -		/* Drop for original kref_init of the fence */
> -		dma_fence_put(fence);
> +		atomic_inc(&sched->credit_count);
> +		drm_sched_job_begin(sched_job);
>   
> -		r = dma_fence_add_callback(fence, &sched_job->cb,
> -					   drm_sched_job_done_cb);
> -		if (r == -ENOENT)
> -			drm_sched_job_done(sched_job, fence->error);
> -		else if (r)
> -			DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n", r);
> -	} else {
> -		drm_sched_job_done(sched_job, IS_ERR(fence) ?
> -				   PTR_ERR(fence) : 0);
> +		trace_drm_run_job(sched_job, entity);
> +		fence = sched->ops->run_job(sched_job);
> +		complete_all(&entity->entity_idle);
> +		drm_sched_fence_scheduled(s_fence, fence);
> +
> +		if (!IS_ERR_OR_NULL(fence)) {
> +			/* Drop for original kref_init of the fence */
> +			dma_fence_put(fence);
> +
> +			r = dma_fence_add_callback(fence, &sched_job->cb,
> +						   drm_sched_job_done_cb);
> +			if (r == -ENOENT)
> +				drm_sched_job_done(sched_job, fence->error);
> +			else if (r)
> +				DRM_DEV_ERROR(sched->dev, "fence add callback failed (%d)\n",
> +					  r);
> +		} else {
> +			drm_sched_job_done(sched_job, IS_ERR(fence) ?
> +					   PTR_ERR(fence) : 0);
> +		}
> +
> +		wake_up(&sched->job_scheduled);
>   	}
>   
> -	wake_up(&sched->job_scheduled);
> +again:
>   	drm_sched_run_job_queue(sched);
>   }
>   
> @@ -1304,7 +1268,6 @@ int drm_sched_init(struct drm_gpu_scheduler *sched,
>   	atomic_set(&sched->credit_count, 0);
>   	INIT_DELAYED_WORK(&sched->work_tdr, drm_sched_job_timedout);
>   	INIT_WORK(&sched->work_run_job, drm_sched_run_job_work);
> -	INIT_WORK(&sched->work_free_job, drm_sched_free_job_work);
>   	atomic_set(&sched->_score, 0);
>   	atomic64_set(&sched->job_id_count, 0);
>   	sched->pause_submit = false;
> @@ -1432,7 +1395,6 @@ void drm_sched_wqueue_stop(struct drm_gpu_scheduler *sched)
>   {
>   	WRITE_ONCE(sched->pause_submit, true);
>   	cancel_work_sync(&sched->work_run_job);
> -	cancel_work_sync(&sched->work_free_job);
>   }
>   EXPORT_SYMBOL(drm_sched_wqueue_stop);
>   
> @@ -1445,6 +1407,5 @@ void drm_sched_wqueue_start(struct drm_gpu_scheduler *sched)
>   {
>   	WRITE_ONCE(sched->pause_submit, false);
>   	queue_work(sched->submit_wq, &sched->work_run_job);
> -	queue_work(sched->submit_wq, &sched->work_free_job);
>   }
>   EXPORT_SYMBOL(drm_sched_wqueue_start);
> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
> index 5acc64954a88..72f64bd0a6eb 100644
> --- a/include/drm/gpu_scheduler.h
> +++ b/include/drm/gpu_scheduler.h
> @@ -495,10 +495,9 @@ struct drm_sched_backend_ops {
>    *                 waits on this wait queue until all the scheduled jobs are
>    *                 finished.
>    * @job_id_count: used to assign unique id to the each job.
> - * @submit_wq: workqueue used to queue @work_run_job and @work_free_job
> + * @submit_wq: workqueue used to queue @work_run_job
>    * @timeout_wq: workqueue used to queue @work_tdr
>    * @work_run_job: work which calls run_job op of each scheduler.
> - * @work_free_job: work which calls free_job op of each scheduler.
>    * @work_tdr: schedules a delayed call to @drm_sched_job_timedout after the
>    *            timeout interval is over.
>    * @pending_list: the list of jobs which are currently in the job queue.
> @@ -528,7 +527,6 @@ struct drm_gpu_scheduler {
>   	struct workqueue_struct		*submit_wq;
>   	struct workqueue_struct		*timeout_wq;
>   	struct work_struct		work_run_job;
> -	struct work_struct		work_free_job;
>   	struct delayed_work		work_tdr;
>   	struct list_head		pending_list;
>   	spinlock_t			job_list_lock;