On Fri, Oct 2, 2020 at 4:01 AM Qais Yousef qais.yousef@arm.com wrote:
On 09/30/20 14:17, Rob Clark 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.
For me thees 2 properties
1. Run ASAP 2. Finish the work un-interruptedScream the workers need to be SCHED_FIFO by default. CFS can't give you these guarantees.
fwiw, commit_work does sleep/block for some time until fences are signalled, but then once that happens we want it to run ASAP, preempting lower priority SCHED_FIFO.
IMO using sched_set_fifo() for these workers is the right thing.
Possibly, but we still have limited prioritization options (ie. not enough) to set these from the kernel. Giving userspace the control, so it can pick sensible priorities for commit_work and vblank_work, which fits in with the priorities of the other userspace threads seems like the sensible thing.
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.
I'm not sure I agree with this. I think it's better to characterize tasks based on their properties/requirements rather than what the rest of the userspace is using.
I mean, the issue is that userspace is already using a few different rt priority levels for different SF threads. We want commit_work to run ASAP once fences are signalled, and vblank_work to run at a slightly higher priority still. But the correct choice for priorities here depends on what userspace is using, it all needs to fit together properly.
I do appreciate that maybe some of these tasks have varying requirements during their life time. e.g: they have RT property during specific critical section but otherwise are CFS tasks. I think the UI thread in Android behaves like that.
It's worth IMO trying that approach I pointed out earlier to see if making RT try to pick an idle CPU rather than preempt CFS helps. Not sure if it'd be accepted but IMHO it's a better direction to consider and discuss.
The problem I was seeing was actually the opposite.. commit_work becomes runnable (fences signalled) but doesn't get a chance to run because a SCHED_FIFO SF thread is running. (Maybe I misunderstood and you're approach would help this case too?)
Or maybe you can wrap userspace pipeline critical section lock such that any task holding it will automatically be promoted to SCHED_FIFO and then demoted to CFS once it releases it.
The SCHED_DEADLINE + token passing approach that the lwn article mentioned sounds interesting, if that eventually becomes possible. But doesn't really help today..
BR, -R
Haven't worked with display pipelines before, so hopefully this makes sense :-)
Thanks
-- Qais Yousef
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.
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