[PATCH 1/1] drm/scheduler: Job timeout handler returns status (v2)

[Luben Tuikov]

On 2020-12-10 4:46 a.m., Steven Price wrote:
> On 10/12/2020 02:14, Luben Tuikov wrote:
>> This patch does not change current behaviour.
>>
>> The driver's job timeout handler now returns
>> status indicating back to the DRM layer whether
>> the task (job) was successfully aborted or whether
>> more time should be given to the task to complete.
> 
> I find the definitions given a little confusing, see below.
> 
>> Default behaviour as of this patch, is preserved,
>> except in obvious-by-comment case in the Panfrost
>> driver, as documented below.
>>
>> All drivers which make use of the
>> drm_sched_backend_ops' .timedout_job() callback
>> have been accordingly renamed and return the
>> would've-been default value of
>> DRM_TASK_STATUS_ALIVE to restart the task's
>> timeout timer--this is the old behaviour, and
>> is preserved by this patch.
>>
>> In the case of the Panfrost driver, its timedout
>> callback correctly first checks if the job had
>> completed in due time and if so, it now returns
>> DRM_TASK_STATUS_COMPLETE to notify the DRM layer
>> that the task can be moved to the done list, to be
>> freed later. In the other two subsequent checks,
>> the value of DRM_TASK_STATUS_ALIVE is returned, as
>> per the default behaviour.
>>
>> A more involved driver's solutions can be had
>> in subequent patches.
> 
> NIT: ^^^^^^^^^ subsequent

Thank you--no idea how my spellcheck and checkpatch.pl missed that.

> 
>>
>> v2: Use enum as the status of a driver's job
>>      timeout callback method.
>>
>> Cc: Alexander Deucher <Alexander.Deucher at amd.com>
>> Cc: Andrey Grodzovsky <Andrey.Grodzovsky at amd.com>
>> Cc: Christian König <christian.koenig at amd.com>
>> Cc: Daniel Vetter <daniel.vetter at ffwll.ch>
>> Cc: Lucas Stach <l.stach at pengutronix.de>
>> Cc: Russell King <linux+etnaviv at armlinux.org.uk>
>> Cc: Christian Gmeiner <christian.gmeiner at gmail.com>
>> Cc: Qiang Yu <yuq825 at gmail.com>
>> Cc: Rob Herring <robh at kernel.org>
>> Cc: Tomeu Vizoso <tomeu.vizoso at collabora.com>
>> Cc: Steven Price <steven.price at arm.com>
>> Cc: Alyssa Rosenzweig <alyssa.rosenzweig at collabora.com>
>> Cc: Eric Anholt <eric at anholt.net>
>> Reported-by: kernel test robot <lkp at intel.com>
> 
> This reported-by seems a little odd for this patch.

I got this because I wasn't (originally) changing all drivers
which were using the task timeout callback.
Should I remove it?

> 
>> Signed-off-by: Luben Tuikov <luben.tuikov at amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/amdgpu_job.c |  6 +++--
>>   drivers/gpu/drm/etnaviv/etnaviv_sched.c | 10 +++++++-
>>   drivers/gpu/drm/lima/lima_sched.c       |  4 +++-
>>   drivers/gpu/drm/panfrost/panfrost_job.c |  9 ++++---
>>   drivers/gpu/drm/scheduler/sched_main.c  |  4 +---
>>   drivers/gpu/drm/v3d/v3d_sched.c         | 32 +++++++++++++------------
>>   include/drm/gpu_scheduler.h             | 20 +++++++++++++---
>>   7 files changed, 57 insertions(+), 28 deletions(-)
>>
> 
> [....]
> 
>> diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h
>> index 2e0c368e19f6..cedfc5394e52 100644
>> --- a/include/drm/gpu_scheduler.h
>> +++ b/include/drm/gpu_scheduler.h
>> @@ -206,6 +206,11 @@ static inline bool drm_sched_invalidate_job(struct drm_sched_job *s_job,
>>   	return s_job && atomic_inc_return(&s_job->karma) > threshold;
>>   }
>>   
>> +enum drm_task_status {
>> +	DRM_TASK_STATUS_COMPLETE,
>> +	DRM_TASK_STATUS_ALIVE
>> +};
>> +
>>   /**
>>    * struct drm_sched_backend_ops
>>    *
>> @@ -230,10 +235,19 @@ 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 job has taken too long to execute,
>> +	 * to trigger GPU recovery.
>> +	 *
>> +	 * Return DRM_TASK_STATUS_ALIVE, if the task (job) is healthy
>> +	 * and executing in the hardware, i.e. it needs more time.
> 
> So 'alive' means the job (was) alive, and GPU recovery is happening. 
> I.e. it's the job just takes too long. Panfrost will trigger a GPU reset 
> (killing the job) in this case while returning DRM_TASK_STATUS_ALIVE.

"ALIVE" means "at this moment the task is in the hardware executing,
using memories, DMA engines, compute units, the whole thing." You,
can also call it active, executing, busy, etc.

"ALIVE" makes no assumptions about how the task got there. Maybe
this was original submission, and the task is taking its sweet time.
Maybe the driver aborted it and reissued it, all in the timer timeout
callback, etc.

It merely tells the DRM layer that the task is actively executing
in the hardware, so no assumptions can be made about freeing memories,
decrementing krefs, etc. IOW, it's executing, check back later.

> 
>> +	 *
>> +	 * Return DRM_TASK_STATUS_COMPLETE, if the task (job) has
>> +	 * been aborted or is unknown to the hardware, i.e. if
>> +	 * the task is out of the hardware, and maybe it is now
>> +	 * in the done list, or it was completed long ago, or
>> +	 * if it is unknown to the hardware.
> 
> Where 'complete' seems to mean a variety of things:
> 
>   * The job completed successfully (i.e. the timeout raced), this is the 
> situation that Panfrost detects. In this case (and only this case) the 
> GPU reset will *not* happen.

Sounds good!

> 
>   * The job failed (aborted) and is no longer on the hardware. Panfrost 
> currently handles a job failure by triggering drm_sched_fault() to 
> trigger the timeout handler. But the timeout handler doesn't handle this 
> differently so will return DRM_TASK_STATUS_ALIVE.

Panfrost seems to do the right thing here by triggering. But I wonder,
could the Panfrost driver simply return the task back to the DRM
layer?

If the task is out of the hardware, it should return COMPLETE, a la,

Driver: COMPLETE!
DRM: Fine, I can free memories and decrement krefs and toss it over
     to the application client. (The application client would
     then find out if the execution status was success of failure.
     For instance, division by 0, or tan(pi/2), etc.)

Driver: ALIVE!
DRM: Fine, I cannot make any assumptions of any kind. I'll back off
     for a bit, and if I don't hear from you in some time, I'll
     check back later.

>   * The job is "unknown to hardware". There are some corner cases in 
> Panfrost (specifically two early returns from panfrost_job_hw_submit()) 
> where the job never actually lands on the hardware, but the scheduler 
> isn't informed. We currently rely on the timeout handling to recover 
> from that. However, again, the timeout handler doesn't know about this 
> soo will return DRM_TASK_STATUS_ALIVE.

"We currently rely on the timeout handling to recover from that."
Do you mean that when the timeout handler kicks in, you find
that the task never landed in the hardware and then you send it
to the hardware? Yes? Then return "ALIVE".
If however, you never decide to send it to the hardware and simply
abandon it (in hopes that the DRM or the Application Client will
reissue it), then you should send it back to DRM with status "COMPLETE".

> So of the four cases listed in these comments, Panfrost is only getting 
> 2 'correct' after this change.

The Panfrost driver knows the hardware intimately, and maybe the hardware,
or a newer version of it, can give a finer control over task execution
on a GPU. If you can abort the task and reissue it, all in the timer
callback, return ALIVE. If you abort it and kick it out of the hardware,
return COMPLETE.

I'd say, this is about information, and passing information back to
the DRM scheduler. It could be any information you'd like. I thought
that the minimal case of information described above is fine.

> 
> But what I really want to know is what the scheduler is planning to do 
> in these situations? The Panfrost return value in this patch is really a 
> "did we trigger a GPU reset" - and doesn't seem to match the 
> descriptions above.

The scheduler would like to know if the task is used by the GPU, i.e.
it is in the hardware and its memories are used and krefs are upped,
and DMA is taking place, etc. Or, if the task is out of the hardware,
and the DRM can free relevant memories, decrement relevant krefs,
free ancillary memories and return the task back to the application
client.

It's just about information. Ultimately driver maintainers decide
the most appropriate action in the timer timeout callback and then
the most appropriate return code. This patch really mimics (tries to)
default (current) behaviour, so as to have as minimal impact as possible,
yet show that finer grained status is attainable from the timer timeout
callback.

No differing action is taken by the scheduler at the moment,
as shown in this patch.

Regards,
Luben


> 
> Steve
> 
>>   	 */
>> -	void (*timedout_job)(struct drm_sched_job *sched_job);
>> +	enum drm_task_status (*timedout_job)(struct drm_sched_job *sched_job);
>>   
>>   	/**
>>            * @free_job: Called once the job's finished fence has been signaled
>>
>