Am 09.06.23 um 16:10 schrieb Boris Brezillon:
Hello Christian,
On Fri, 9 Jun 2023 13:53:59 +0200
Christian König <christian.koenig@xxxxxxx> wrote:
Am 08.06.23 um 08:55 schrieb Boris Brezillon:
If I understand correctly, drm_sched_entity_kill_jobs_cb() is supposed
to wait on all the external dependencies (those added to
drm_sched_job::dependencies) before signaling the job finished fence.
This is done this way to prevent jobs depending on these canceled jobs
from considering the resources they want to access as ready, when
they're actually still used by other components, thus leading to
potential data corruptions.
The problem is, the kill_jobs logic is omitting the last fence popped
from the dependencies array that was waited upon before
drm_sched_entity_kill() was called (drm_sched_entity::dependency field),
so we're basically waiting for all dependencies except one.
This is an attempt at fixing that, but also an opportunity to make sure
I understood the drm_sched_entity_kill(), because I'm not 100% sure if
skipping this currently popped dependency was intentional or not. I can't
see a good reason why we'd want to skip that one, but I might be missing
something.
Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxx>
Cc: Frank Binns <frank.binns@xxxxxxxxxx>
Cc: Sarah Walker <sarah.walker@xxxxxxxxxx>
Cc: Donald Robson <donald.robson@xxxxxxxxxx>
Cc: Luben Tuikov <luben.tuikov@xxxxxxx>
Cc: David Airlie <airlied@xxxxxxxxx>
Cc: Daniel Vetter <daniel@xxxxxxxx>
Cc: Sumit Semwal <sumit.semwal@xxxxxxxxxx>
Cc: "Christian König" <christian.koenig@xxxxxxx>
---
Stumbled across this issue while working on native dependency support
with Donald (which will be posted soon). Flagged as RFC, because I'm
not sure this is legit, and also not sure we want to fix it this way.
I tried re-using drm_sched_entity::dependency, but it's a bit of a mess
because of the asynchronousity of the wait, and the fact we use
drm_sched_entity::dependency to know if we have a clear_dep()
callback registered, so we can easily reset it without removing the
callback.
Well yes, that's a known problem. But this is really not the right
approach to fixing this.
Trying to wait for all the dependencies before killing jobs was added
because of the way we kept track of dma_fences in dma_resv objects.
Basically adding exclusive fences removed all other fences leading to a
bit fragile memory management.
Okay.
This handling was removed by now and so the workaround for waiting for
dependencies is not really necessary any more, but I think it is still
better to do so.
The right approach of getting this waiting for dependencies completely
straight is also not to touch entity->dependency in any way, but to stop
removing them from the XA in drm_sched_job_dependency(). Otherwise you
don't catch the pipeline optimized ones either.
Do you want me to post a v2 doing that, or should I forget about it?
If we decide to keep things like that, it might be good to at least
add a comment explaining why we don't care.
Well, if you have time fixing this fully and keeping the dependency
handling is certainly be preferred.
Regards,
Christian.
Regards,
Boris
