Message-ID: <5fb872d0-9b0a-4398-9472-eea3fdf61940@amd.com>
Date: Tue, 5 Aug 2025 11:05:18 +0200
From: Christian König <christian.koenig@....com>
To: Philipp Stanner <pstanner@...hat.com>, Philipp Stanner
 <phasta@...nel.org>, Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
 Maxime Ripard <mripard@...nel.org>, Thomas Zimmermann <tzimmermann@...e.de>,
 David Airlie <airlied@...il.com>, Simona Vetter <simona@...ll.ch>,
 Jonathan Corbet <corbet@....net>, Matthew Brost <matthew.brost@...el.com>,
 Danilo Krummrich <dakr@...nel.org>,
 Christian König <ckoenig.leichtzumerken@...il.com>,
 Sumit Semwal <sumit.semwal@...aro.org>
Cc: dri-devel@...ts.freedesktop.org, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org, linux-media@...r.kernel.org
Subject: Re: [PATCH] drm/sched: Extend and update documentation

On 24.07.25 17:07, Philipp Stanner wrote:
>> +/**
>> + * DOC: Scheduler Fence Object
>> + *
>> + * The scheduler fence object (&struct drm_sched_fence) encapsulates the whole
>> + * time from pushing the job into the scheduler until the hardware has finished
>> + * processing it. It is managed by the scheduler. The implementation provides
>> + * dma_fence interfaces for signaling both scheduling of a command submission
>> + * as well as finishing of processing.
>> + *
>> + * The lifetime of this object also follows normal dma_fence refcounting rules.
>> + */
> 
> The relict I'm most unsure about is this docu for the scheduler fence.
> I know that some drivers are accessing the s_fence, but I strongly
> suspect that this is a) unncessary and b) dangerous.

Which s_fence member do you mean? The one in the job? That should be harmless as far as I can see.

> But the original draft from Christian hinted at that. So, @Christian,
> this would be an opportunity to discuss this matter.
> 
> Otherwise I'd drop this docu section in v2. What users don't know, they
> cannot misuse.

I would rather like to keep that to avoid misusing the job as the object for tracking the submission lifetime.

>> +/**
>> + * DOC: Error and Timeout handling
>> + *
>> + * Errors are signaled by using dma_fence_set_error() on the hardware fence
>> + * object before signaling it with dma_fence_signal(). Errors are then bubbled
>> + * up from the hardware fence to the scheduler fence.
>> + *
>> + * The entity allows querying errors on the last run submission using the
>> + * drm_sched_entity_error() function which can be used to cancel queued
>> + * submissions in &struct drm_sched_backend_ops.run_job as well as preventing
>> + * pushing further ones into the entity in the driver's submission function.
>> + *
>> + * When the hardware fence doesn't signal within a configurable amount of time
>> + * &struct drm_sched_backend_ops.timedout_job gets invoked. The driver should
>> + * then follow the procedure described in that callback's documentation.
>> + *
>> + * (TODO: The timeout handler should probably switch to using the hardware
>> + * fence as parameter instead of the job. Otherwise the handling will always
>> + * race between timing out and signaling the fence).
> 
> This TODO can probably removed, too. The recently merged
> DRM_GPU_SCHED_STAT_NO_HANG has solved this issue.

No, it only scratched on the surface of problems here.

I'm seriously considering sending a RFC patch to cleanup the job lifetime and implementing this change.

Not necessarily giving the HW fence as parameter to the timeout callback, but more generally not letting the scheduler depend on driver behavior.

Regards,
Christian.

> 
> 
> P.
> 
>> + *
>> + * The scheduler also used to provided functionality for re-submitting jobs
>> + * and, thereby, replaced the hardware fence during reset handling. This
>> + * functionality is now deprecated. This has proven to be fundamentally racy
>> + * and not compatible with dma_fence rules and shouldn't be used in new code.
>> + *
>> + * Additionally, there is the function drm_sched_increase_karma() which tries
>> + * to find the entity which submitted a job and increases its 'karma' atomic
>> + * variable to prevent resubmitting jobs from this entity. This has quite some
>> + * overhead and resubmitting jobs is now marked as deprecated. Thus, using this
>> + * function is discouraged.
>> + *
>> + * Drivers can still recreate the GPU state in case it should be lost during
>> + * timeout handling *if* they can guarantee that forward progress will be made
>> + * and this doesn't cause another timeout. But this is strongly hardware
>> + * specific and out of the scope of the general GPU scheduler.
>> + */
>>  #include <linux/export.h>
>>  #include <linux/wait.h>
>>  #include <linux/sched.h>
>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>> index 323a505e6e6a..0f0687b7ae9c 100644
>> --- a/include/drm/gpu_scheduler.h
>> +++ b/include/drm/gpu_scheduler.h
>> @@ -458,8 +458,8 @@ struct drm_sched_backend_ops {
>>  	struct dma_fence *(*run_job)(struct drm_sched_job *sched_job);
>>  
>>  	/**
>> -	 * @timedout_job: Called when a job has taken too long to execute,
>> -	 * to trigger GPU recovery.
>> +	 * @timedout_job: Called when a hardware fence didn't signal within a
>> +	 * configurable amount of time. Triggers GPU recovery.
>>  	 *
>>  	 * @sched_job: The job that has timed out
>>  	 *
>> @@ -506,7 +506,6 @@ struct drm_sched_backend_ops {
>>  	 * that timeout handlers are executed sequentially.
>>  	 *
>>  	 * Return: The scheduler's status, defined by &enum drm_gpu_sched_stat
>> -	 *
>>  	 */
>>  	enum drm_gpu_sched_stat (*timedout_job)(struct drm_sched_job *sched_job);
>>  
> 

Powered by blists - more mailing lists