On Fri, Oct 2, 2020 at 4:01 AM Qais Yousef <qais.yousef@xxxxxxx> wrote: > > On 09/30/20 14:17, Rob Clark wrote: > > From: Rob Clark <robdclark@xxxxxxxxxxxx> > > > > 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-interrupted > > Scream 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 > > _______________________________________________ dri-devel mailing list dri-devel@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/dri-devel
