I think we should stick to the plan where kernel contexts stay "stuck"
after a GPU reset. This is the most robust behavior for the kernel.
Even if the OpenGL spec says that an OpenGL context can be re-used
without destroying and re-creating it, the UMD can take care of
re-creating the kernel context.
This means amdgpu_ctx_query should *not* reset ctx->reset_counter.
Cheers,
Nicolai
On 12.10.2017 10:41, Nicolai Hähnle wrote:
Hi Monk,
Thanks for the summary. Most of it looks good to me, though I can't
speak to all the kernel internals.
Just some comments:
On 12.10.2017 10:03, Liu, Monk wrote:
lFor cs_submit() IOCTL:
1.check if current ctx been marked “*guilty*”and return “*ECANCELED*”
if so.
2.set job->*vram_lost_counter* with adev->*vram_lost_counter*, and
return “*ECANCELED*” if ctx->*vram_lost_counter* !=
job->*vram_lost_counter* (Christian already submitted this patch)
a)discussion: can we return “ENODEV” if vram_lost_counter mismatch ?
that way UMD know this context is under “device lost”
My plan for UMD is to always query the VRAM lost counter when any kind
of context lost situation is detected. So cs_submit() should return an
error in this situation, but it could just be ECANCELED. We don't need
to distinguish between different types of errors here.
lIntroduce a new IOCTL to let UMD query latest adev->*vram_lost_counter*:
Christian already sent a patch for this.
lFor amdgpu_ctx_query():
n*Don’t update ctx->reset_counter when querying this function,
otherwise the query result is not consistent *
Hmm. I misremembered part of the spec, see below.
nSet out->state.reset_status to “AMDGPU_CTX_GUILTY_RESET” if the ctx
is “*guilty*”, no need to check “ctx->reset_counter”
Agreed.
nSet out->state.reset_status to “AMDGPU_CTX_INNOCENT_RESET” *if the
ctx isn’t “guilty” && ctx->reset_counter != adev->reset_counter *
I disagree. The meaning of AMDGPU_CTX_*_RESET should reflect the
corresponding enums in user space APIs. I don't know how it works in
Vulkan, but in OpenGL, returning GL_INNOCENT_CONTEXT_RESET_ARB means
that the context was lost.
However, !guilty && ctx->reset_counter != adev->reset_counter does not
imply that the context was lost.
The way I understand it, we should return AMDGPU_CTX_INNOCENT_RESET if
!guilty && ctx->vram_lost_counter != adev->vram_lost_counter.
As far as I understand it, the case of !guilty && ctx->reset_counter !=
adev->reset_counter && ctx->vram_lost_counter == adev->vram_lost_counter
should return AMDGPU_CTX_NO_RESET, because a GPU reset occurred, but it
didn't affect our context.
I unfortunately noticed another subtlety while re-reading the OpenGL
spec. OpenGL says that the OpenGL context itself does *not* have to be
re-created in order to recover from the reset. Re-creating all objects
in the context is sufficient.
I believe this is the original motivation for why amdgpu_ctx_query()
will reset the ctx->reset_counter.
For Mesa, it's still okay if the kernel keeps blocking submissions as we
can just recreate the kernel context. But OrcaGL is also affected.
Does anybody know off-hand where the relevant parts of the Vulkan spec
are? I didn't actually find anything in a quick search.
[snip]
For UMD behavior we still have something need to consider:
If MESA creates a new context from an old context (share list?? I’m
not familiar with UMD , David Mao shall have some discuss on it with
Nicolai), the new created context’s vram_lost_counter
And reset_counter shall all be ported from that old context ,
otherwise CS_SUBMIT will not block it which isn’t correct
The kernel doesn't have to do anything for this, it is entirely the
UMD's responsibility. All UMD needs from KMD is the function for
querying the vram_lost_counter.
Cheers,
Nicolai
Need your feedback, thx
*From:*amd-gfx [mailto:[email protected]] *On
Behalf Of *Liu, Monk
*Sent:* 2017年10月11日13:34
*To:* Koenig, Christian <[email protected]>; Haehnle, Nicolai
<[email protected]>; Olsak, Marek <[email protected]>;
Deucher, Alexander <[email protected]>
*Cc:* Ramirez, Alejandro <[email protected]>;
[email protected]; Filipas, Mario <[email protected]>;
Ding, Pixel <[email protected]>; Li, Bingley <[email protected]>;
Jiang, Jerry (SW) <[email protected]>
*Subject:* TDR and VRAM lost handling in KMD:
Hi Christian & Nicolai,
We need to achieve some agreements on what should MESA/UMD do and what
should KMD do, *please give your comments with **“okay”or “No”and your
idea on below items,*
lWhen a job timed out (set from lockup_timeout kernel parameter), What
KMD should do in TDR routine :
1.Update adev->*gpu_reset_counter*, and stop scheduler first,
(*gpu_reset_counter* is used to force vm flush after GPU reset, out of
this thread’s scope so no more discussion on it)
2.Set its fence error status to “*ETIME*”,
3.Find the entity/ctx behind this job, and set this ctx as “*guilty*”
4.Kick out this job from scheduler’s mirror list, so this job won’t
get re-scheduled to ring anymore.
5.Kick out all jobs in this “guilty”ctx’s KFIFO queue, and set all
their fence status to “*ECANCELED*”
*6.*Force signal all fences that get kicked out by above two
steps,*otherwise UMD will block forever if waiting on those fences*
7.Do gpu reset, which is can be some callbacks to let bare-metal and
SR-IOV implement with their favor style
8.After reset, KMD need to aware if the VRAM lost happens or not,
bare-metal can implement some function to judge, while for SR-IOV I
prefer to read it from GIM side (for initial version we consider it’s
always VRAM lost, till GIM side change aligned)
9.If VRAM lost not hit, continue, otherwise:
a)Update adev->*vram_lost_counter*,
b)Iterate over all living ctx, and set all ctx as “*guilty*”since VRAM
lost actually ruins all VRAM contents
c)Kick out all jobs in all ctx’s KFIFO queue, and set all their fence
status to “*ECANCELDED*”
10.Do GTT recovery and VRAM page tables/entries recovery (optional, do
we need it ???)
11.Re-schedule all JOBs remains in mirror list to ring again and
restart scheduler (for VRAM lost case, no JOB will re-scheduled)
lFor cs_wait() IOCTL:
After it found fence signaled, it should check with
*“dma_fence_get_status” *to see if there is error there,
And return the error status of fence
lFor cs_wait_fences() IOCTL:
Similar with above approach
lFor cs_submit() IOCTL:
It need to check if current ctx been marked as “*guilty*”and return
“*ECANCELED*”if so
lIntroduce a new IOCTL to let UMD query *vram_lost_counter*:
This way, UMD can also block app from submitting, like @Nicolai
mentioned, we can cache one copy of *vram_lost_counter* when enumerate
physical device, and deny all
gl-context from submitting if the counter queried bigger than that one
cached in physical device. (looks a little overkill to me, but easy to
implement )
UMD can also return error to APP when creating gl-context if found
current queried*vram_lost_counter *bigger than that one cached in
physical device.
BTW: I realized that gl-context is a little different with kernel’s
context. Because for kernel. BO is not related with context but only
with FD, while in UMD, BO have a backend
gl-context, so block submitting in UMD layer is also needed although
KMD will do its job as bottom line
lBasically “vram_lost_counter”is exposure by kernel to let UMD take
the control of robust extension feature, it will be UMD’s call to
move, KMD only deny “guilty”context from submitting
Need your feedback, thx
We’d better make TDR feature landed ASAP
BR Monk
_______________________________________________
amd-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
