On Thu, Oct 1, 2020 at 5:15 PM Rob Clark robdclark@gmail.com wrote:
On Thu, Oct 1, 2020 at 12:25 AM Daniel Vetter daniel@ffwll.ch wrote:
On Wed, Sep 30, 2020 at 11:16 PM Rob Clark robdclark@gmail.com wrote:
From: Rob Clark robdclark@chromium.org
The android userspace treats the display pipeline as a realtime problem. And arguably, if your goal is to not miss frame deadlines (ie. vblank), it is. (See https://lwn.net/Articles/809545/ for the best explaination that I found.)
But this presents a problem with using workqueues for non-blocking atomic commit_work(), because the SCHED_FIFO userspace thread(s) can preempt the worker. Which is not really the outcome you want.. once the required fences are scheduled, you want to push the atomic commit down to hw ASAP.
But the decision of whether commit_work should be RT or not really depends on what userspace is doing. For a pure CFS userspace display pipeline, commit_work() should remain SCHED_NORMAL.
To handle this, convert non-blocking commit_work() to use per-CRTC kthread workers, instead of system_unbound_wq. Per-CRTC workers are used to avoid serializing commits when userspace is using a per-CRTC update loop. And the last patch exposes the task id to userspace as a CRTC property, so that userspace can adjust the priority and sched policy to fit it's needs.
v2: Drop client cap and in-kernel setting of priority/policy in favor of exposing the kworker tid to userspace so that user- space can set priority/policy.
Yeah I think this looks more reasonable. Still a bit irky interface, so I'd like to get some kworker/rt ack on this. Other opens:
- needs userspace, the usual drill
fwiw, right now the userspace is "modetest + chrt".. *probably* the userspace will become a standalone helper or daemon, mostly because the chrome gpu-process sandbox does not allow setting SCHED_FIFO. I'm still entertaining the possibility of switching between rt and cfs depending on what is in the foreground (ie. only do rt for android apps).
- we need this also for vblank workers, otherwise this wont work for
drivers needing those because of another priority inversion.
I have a thought on that, see below..
Hm, not seeing anything about vblank worker below?
- we probably want some indication of whether this actually does
something useful, not all drivers use atomic commit helpers. Not sure how to do that.
I'm leaning towards converting the other drivers over to use the per-crtc kwork, and then dropping the 'commit_work` from atomic state. I can add a patch to that, but figured I could postpone that churn until there is some by-in on this whole idea.
i915 has its own commit code, it's not even using the current commit helpers (nor the commit_work). Not sure how much other fun there is.
- not sure whether the vfunc is an awesome idea, I'd frankly just
open-code this inline. We have similar special cases already for e.g. dpms (and in multiple places), this isn't the worst.
I could introduce a "PID" property type. This would be useful if we wanted to also expose the vblank workers.
Hm right, but I think we need at most 2 for commit thread and vblank thread (at least with the current design). So open-coded if with two if (prop == crtc_worker_pid_prop || prop == crtc_vblank_worker_pid_prop) doesn't seem too horrible to me. Otherwise people start creating really funny stuff in their drivers with this backend, and I don't want to spend all the time making sure they don't abuse this :-)
- still feeling we could at least change the default to highpriority niceness.
AFAIU this would still be preempted by something that is SCHED_FIFO. Also, with cfs/SCHED_NORMAL, you can still be preempted by lower priority things that haven't had a chance to run for a while.
i915 uses highprio workqueue, so I guess to avoid regressions we need that (it's also not using the commit helpers right now, but no reason not to afaics, stuff simply happened in parallel back then.
- there's still the problem that commit works can overlap, and a
single worker can't do that anymore. So rolling that out for everyone as-is feels a bit risky.
That is why it is per-CRTC.. I'm not sure there is a need to overlap work for a single CRTC?
We could OFC make this a driver knob, and keep the old 'commit_work' option, but that doesn't really feel like the right direction
drm_atomic_helper_commit_hw_done is what unblocks the next worker on the same set of crtc. It's before we do all the buffer cleanup, which has a full vblank stall beforehand. Most drivers also have the same vblank stall in their next commit, plus generally the fb cleanup is cheap, but neither is a requirement. So yeah you can get overlapping commit work on the same crtc, and that was kinda intentional. Maybe was overkill, I guess minimally just something that needs to be in the commit message. -Daniel
BR, -R
Cheers, Daniel
Rob Clark (3): drm/crtc: Introduce per-crtc kworker drm/atomic: Use kthread worker for nonblocking commits drm: Expose CRTC's kworker task id
drivers/gpu/drm/drm_atomic_helper.c | 13 ++++++++---- drivers/gpu/drm/drm_crtc.c | 14 +++++++++++++ drivers/gpu/drm/drm_mode_config.c | 14 +++++++++++++ drivers/gpu/drm/drm_mode_object.c | 4 ++++ include/drm/drm_atomic.h | 31 +++++++++++++++++++++++++++++ include/drm/drm_crtc.h | 8 ++++++++ include/drm/drm_mode_config.h | 9 +++++++++ include/drm/drm_property.h | 9 +++++++++ 8 files changed, 98 insertions(+), 4 deletions(-)
-- 2.26.2
-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch