On 10/09/2024 16:03, Christian König wrote:
Am 10.09.24 um 11:46 schrieb Tvrtko Ursulin:
On 10/09/2024 10:08, Christian König wrote:
Am 09.09.24 um 19:19 schrieb Tvrtko Ursulin:
From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxx>
Having removed one re-lock cycle on the entity->lock in a patch titled
"drm/sched: Optimise drm_sched_entity_push_job", with only a tiny bit
larger refactoring we can do the same optimisation on the rq->lock.
(Currently both drm_sched_rq_add_entity() and
drm_sched_rq_update_fifo_locked() take and release the same lock.)
I think that goes into the wrong direction.
Probably better to move this here into drm_sched_rq_add_entity():
if (drm_sched_policy == DRM_SCHED_POLICY_FIFO)
drm_sched_rq_update_fifo_locked(entity, submit_ts);
We can then also drop adding the entity to the rr list when FIFO is
in use.
Unfortuntely there is a few other places which appear to rely on the
list. Like drm_sched_fini,
That should be only a warning.
Warning as in?
drm_sched_increase_karma and
The karma handling was another bad idea from AMD how to populate back
errors to userspace and I've just recently documented together with Sima
that we should use dma-fence errors instead.
Just didn't had time to tackle cleaning that up yet.
even amdgpu_job_stop_all_jobs_on_sched.
Uff, seeing that for the first time just now. Another bad idea how to
handle things which doesn't take the appropriate locks and looks racy to
me.
Latter could perhaps be solved by adding an iterator helper to the
scheduler, which would perhaps be a good move for component isolation.
And first two could be handled by implementing a complete and mutually
exclusive duality of how entities are walked depending on scheduling
mode. Plus making the scheduling mode only be configurable at boot. It
feels doable but significant work and in the meantime removing the
double re-lock maybe acceptable?
I don't think we should optimize for something we want to remove in the
long term.
I knew using the term optimise would just making things more difficult
for myself. :) Lets view this as cleaning up the API to avoid the
inelegance of taking the same lock twice right next to each other.
If we can achieve this while not making the API worse then there is
nothing to lose either short, med or long term.
If possible I would rather say that we should completely drop the RR
approach and only use FIFO or even something more sophisticated.
No complaints from me, but I don't know how that would work other than
putting a depreciation warning if someone selected RR. And keeping that
for a good number of kernel releases. Any other ideas?
Regards,
Tvrtko
