06:18jekstrand:is tempted to throw this structurizer out and start over. 👿
06:18jekstrand:should go to bed
12:36samuelig: HdkR, Lyude, fixed XDC website. You are right, it was confusing.
16:08robclark: danvet: so turns out system_unbound_wq is horrible.. I'm seeing cases where it is in runnable state, but not scheduled yet for many ms.. looks like the android folks are instead using kthread's with SCHED_FIFO, and are recommending this. (They actually use kthread per crtc and then schedule the commit_work on the kthread matching the first crtc in the atomic state, so that independent commits on different crtc's
16:08robclark: are not blocking each other.)
16:09robclark: danvet: so not sure about other drivers, but I think the options are to splitup drm_atomic_helper_commit() so drivers can do their own thing.. or have drm core allocate some kthread's
16:09danvet: robclark, imo if we have one machine which needs kthread
16:09robclark: not sure if you have a particular preference one way or another before I start typing
16:09danvet: they probably all do
16:10danvet: so I'd say gather data about how much unbound_wq sucks, put that in commit message
16:10danvet: maybe cc: tejun from workqueue for comments
16:10robclark: that is kinda my thought.. just wasn't sure if it would break other drivers, esp if we have per-crtc kthread..
16:10danvet: iirc tejun had some useful input when Lyude was doing the kworker inspired stuff for vblank work
16:10danvet: robclark, per-crtc thread is less concurrent than current concurrent
16:11danvet: so at most, less races
16:11danvet: I can't see how it breaks anything, since atomic helpers sync crtc commits already
16:11robclark: oh, right, I guess we already have that
16:11danvet: it does break something :-/
16:11danvet: so the workers are overlapping
16:11danvet: the hw commit done happens before we clean up the old framebuffers
16:12danvet: so maybe still work queue, but fifo priority?
16:12danvet: other option is that we'd split up things in the commit function
16:13danvet: which is awkward, since drivers tend to have their "hw commit is done now" point at different points :-/
16:13danvet: the real trouble is the vblank/flip done wait
16:13danvet: it's not really expensive stuff we're doing
16:14danvet: expensive in terms of cpu time I mean
16:14robclark: hmm, does wq guarantee that work's don't overlap even if they happen on other workers?
16:15danvet: unbound guarantees that if a work takes forever and there's more
16:15danvet: more workers will be launched
16:15danvet: to handle that kind of overlap
16:15danvet: if you only have a single per-crtc kthread, you can't handle the overlap
16:15danvet: which is ok as long as the overlap is small enough to not matter
16:16robclark: I mean, you are kinda going to be limited by vblank anyways, so meh?
16:17danvet: shitty hw you have to prep the next update before vblank, but delay the cleanup after the vblank?
16:17danvet: hm should still work
16:17danvet: the thing is, we've had cases where we ended up with "oops half frame rate" with vblank waits in the wrong spot
16:17danvet: on various drivers iirc
16:18danvet: maybe I'm just irrationally silly, dunno
16:20robclark: I think it will work out.. but looks like it should be fairly trivial to make kthread vs system_unbound_wq an opt-in thing for a bit so other drivers can test it on their own schedule and not be broken by a global change
16:20danvet: robclark, does using the highpri queue help?
16:20danvet: that's the one that the i915 workqueue uses
16:20robclark: I guess that is an easy enough thing to try
16:22danvet: robclark, c26a058680dcf seems to have been enough
16:22danvet: the fifo thing we now have for vblank worker
16:22danvet: which generally is a lot more rt than the commit_tail stuff
16:23danvet: if we make the normal commit worker also fifo, then we might starve the real important stuff
16:23danvet: and kernel-internally the fifo priority stuff was simplified massively
16:24danvet: essentially kernel only gets like 1-2 fifo levels now or so
16:24danvet: for itself
16:24danvet: 1 for driver stuff, and one for the real emergency stuff
16:24danvet: so I think if system_unbind_hiprio is enough, that would be cleanest from an overall pov
17:11robclark: danvet: doesn't look like system_unbound_hipri_wq does the trick.. fwiw, feedback from android guy is "I didn't realize upstream drm was using kworkers; moving drm to RT kthreads is a necessary change imo. I have not seen an Android device in the past five years that could reliably deliver frames to the panel or fences without getting CFS threads out of the driver stack, and kworkers were always part of the problem.
17:11robclark: bound vs unbound workqueue didn't matter, high-priority workqueue didn't matter, only RT kthreads worked reliably enough for GPU events."
17:12robclark: (fwiw, this is a problem when running at very low cpu freqs so could be partially due to how cpufreq governor works I guess.. missing vblanks means more sleeping so it tricks cpufreq into going in the wrong direction)
17:36FLHerne: robclark: That last bit is the same thing Mutter is fighting https://gitlab.gnome.org/GNOME/mutter/-/merge_requests/1383 (and multiple iterations linked therefrom)
17:37FLHerne: There still seems to be an argument about whether working around it is necessary or an ugly hack :p
17:39robclark: in this case it is more about cpu freq, but I guess there are some analogies..
18:01danvet: robclark, hm yeah the cpufreq tailspin of death on low loads :-/
18:03danvet: robclark, I guess would be good to know where the latency problem is
18:03danvet: like is it unbound wq not scheduling the task
18:03danvet: or is it the wakeup latency in the wait_for_fences part
18:03danvet: or something else
18:03robclark: one case I saw was surfaceflinger (which is SCHED_FIFO) preempting commit_work ;-)
18:04danvet: yeah that doesn't work
18:04danvet: the thing is, it wont even work with sched fifo kthread
18:04danvet: because the new rule in the kernel iirc is that you get the lowest sched fifo
18:04robclark: so yeah, I guess if userspace using SCHED_FIFO, it might not matter
18:05robclark: android may not have rebased into that yet
18:05danvet: essentially scheduler people are on the "SCHED_FIFO is terminally busted"
18:05danvet: as a concept
18:05danvet: so the only knob you get is "I want to preempt all SCHED_NORMAL"
18:05danvet: and the other knob is "I want to preempt _all_ userspace"
18:05danvet: and everything else is "userspace needs to figure this out with SCHED_DEADLINE"
18:06robclark: when did 'kernel SCHED_FIFO is lower than userspace' happen?
18:06danvet: robclark, maybe what we could do is ask tejun for a work primitive which does run in the sched context of the process that queues it
18:07danvet: not lower
18:07danvet: let me check again
18:07danvet: iirc 5.9
18:07robclark: hmm, that wouldn't totally help here (but partly just because all the different processes involved when you combine android and cros)
18:09danvet: robclark, 7318d4cc14c8c
18:09danvet: I mixed it up, there's sched_fifo at MAX_RT/2
18:09danvet: and sched_fifo_low, which is the lowest
18:10danvet: but yeah so if surfaceflinger is sched fifo
18:10danvet: then commit work should be higher
18:10danvet: and vblank work should be even higher
18:10danvet: and the take from scheduler people seems to be "don't do that"
18:11danvet: that's the trouble with sched_fifo, as soon as you set that on one task, you _have_ to correctly manage all other things and guarantee there's no inversion
18:11danvet: or it's totally broken
18:27robclark: I guess for display there are few enough pieces to the puzzle that with a bit more than two priority levels it could be managed.. but maybe not great in a general sense
18:46robclark: so looks like SF is using prio=2 so having only MAX_RT/2 and 1 avail on kernel side isn't really going to do the trick :-/
19:14robclark: danvet: so crazy idea, commit_work and vblank_work could actually manage to survive at same RT prio, as long as vblank_work is kicked before we send pageflip/vblank events (or signal out-fence)
19:24danvet: robclark, yeah this is a bit too fragile
19:24danvet: the priorities are already hard to manage if you combine with other stuff
19:24danvet: throwing in some reliance on the fifo nature doesn't make it better :-)
19:25danvet: I still think best if we ask tejun and scheduler folks what they advice
19:25danvet: maybe right answer is "don't run surfaceflinger at sched_fifo, only elevated nice levels"
19:41robclark: danvet: well, for a given display, same priority can work.. just as long as we take care about the "first in first out" part of it ;-)
20:44airlied: jekstrand: did you sleep or rewrite it?
20:45jekstrand: airlied: I slept
20:45jekstrand: airlied: But then I wrote something to torture it some more
21:34jekstrand: airlied: I think it's probably mostly ok but there's something pretty fundamentally busted about some of the stacks.
21:34jekstrand: I'm still not sure what
21:38airlied: guess it needs more torturing in the real world
21:39airlied: agd5f: just fyi somewhere between 5.6 and 5.7 my fiji nano card started falling over on dpms
21:39airlied: it gpu resets it seems
22:13agd5f: airlied, does amdgpu.runpm=0 fix it?
22:30airlied: agd5f: seems to