> From: "Sven M. Hallberg" <pe...@khjk.org> > Date: Mon, 03 Jun 2019 21:20:29 +0200 > > I've spent some more time poking through the drm code and think I have a > good picture now of what is going on. > > The inteldrm (i915) driver is essentially implementing a three-stage > pipeline: setup - hardware - cleanup. > > The first and second stage are perfomed in the originating > thread/process (so usually an ioctl from userspace) unless > nonblocking behavior has been requested. In that case, only setup is > performed in the originating context and hardware is deferred to either > 'system_unbound_wq' or a special 'modeset_wq'. > > sys/dev/pci/drm/i915/intel_display.c intel_atomic_commit(): > > if (nonblock && intel_state->modeset) { > queue_work(dev_priv->modeset_wq, &state->commit_work); > } else if (nonblock) { > queue_work(system_unbound_wq, &state->commit_work); > } else { > if (intel_state->modeset) > flush_workqueue(dev_priv->modeset_wq); > intel_atomic_commit_tail(state); > } > > This mirrors the generic (reference/example?) implementation > sys/dev/pci/drm/drm_atomic_helper.c drm_atomic_helper_commit(): > > if (nonblock) > queue_work(system_unbound_wq, &state->commit_work); > else > commit_tail(state); > > Finally, the cleanup stage is deferred by i915 to 'system_wq'. > sys/dev/pci/drm/i915/intel_display.c intel_atomic_commit_tail(): > > INIT_WORK(&state->commit_work, intel_atomic_cleanup_work); > schedule_work(&state->commit_work);
Thanks for helping figuring this out. > Mark Kettenis on Thu, May 30 2019: > > I wonder whether the issue is that on OpenBSD we use a single-threaded > > task queue, where Linux is pretty much always multi-threaded. That > > means that on OpenBSD, if a task blocks, no other task can run. At > > some point I ran into that issue, but worked around it by using an > > additional task queue. > > I'm pretty sure that this is accurate and using an additional queue > would fix it (see below). > > > I believe Linux uses a thread per cpu. I think that means there is > > actually a bug in the Linux code somewhere but nobody notices because > > most systems have more than one cpu. > > Yes, I think so, unless Linux dynamically spawns new workers when > the "worker pool" runs out or something. > > > Making the task queues multi-threaded on OpenBSD is hard since it > > breaks the synchronization mechanism such as takq_barrier(9). > > Agreed. > > > In summary, I see three main options for fixing this: > > 1. Multithread the work queue somehow. A few tricks come to mind, but > like you say, it's going to be fiddly.* > > 2. Put the cleanup tasks in their own queue. The rationale being that > this properly assigns a worker to the third stage of the pipeline. > > 3. Only defer cleanups in the nonblocking case. Rationale: This is > arguably expected behavior out of a blocking call and if you care > about the latency introduced by the cleanup, then you probably also > care about the setup and are doing nonblocking calls already.** > > Note that the nonblocking case will not stall, because nonblocking > commits go on system_unbound_wq or modeset_wq. Cleanups therefore end > up separated on system_wq. In fact, there is a comment that seems to > echo this in drm_atomic_helper_commit(): > > /* [...] > * NOTE: Commit work has multiple phases, first hardware commit, then > * cleanup. We want them to overlap, hence need system_unbound_wq to > * make sure work items don't artifically stall on each another. > */ > > I'm attaching diffs for options 2 and 3. Both work for me. Nice. Nevertheless, I think we need to implement option 1. We want to keep the local diffs to a bare minimum such that we can easily diff the code to the Linux DRM code. I have some ideas, essentially wrapping the native OpenBSD task/taskq interface to implement a more Linuxy behaviour. But it will take me some time to figure out the details. > * For example: In wait_for_completion(), before going to sleep, check > whether we are a queue worker and if so, quickly spawn another to > continue in our place. > > ** I guess the prime example would be fancy compositors that potentially > shout an ioctl at the kernel every frame. I have not checked whether > they do blocking or nonblocking calls. > > > [2:text/x-patch Hide] > > Index: dev/pci/drm/i915/i915_drv.c > =================================================================== > RCS file: /cvs/src/sys/dev/pci/drm/i915/i915_drv.c,v > retrieving revision 1.118 > diff -u -p -r1.118 i915_drv.c > --- dev/pci/drm/i915/i915_drv.c 8 May 2019 15:55:56 -0000 1.118 > +++ dev/pci/drm/i915/i915_drv.c 3 Jun 2019 19:04:30 -0000 > @@ -869,8 +869,14 @@ static int i915_workqueues_init(struct d > if (dev_priv->hotplug.dp_wq == NULL) > goto out_free_wq; > > + dev_priv->cleanup_wq = alloc_ordered_workqueue("i915-cleanup", 0); > + if (dev_priv->cleanup_wq == NULL) > + goto out_free_wq2; > + > return 0; > > +out_free_wq2: > + destroy_workqueue(dev_priv->hotplug.dp_wq); > out_free_wq: > destroy_workqueue(dev_priv->wq); > out_err: > Index: dev/pci/drm/i915/i915_drv.h > =================================================================== > RCS file: /cvs/src/sys/dev/pci/drm/i915/i915_drv.h,v > retrieving revision 1.82 > diff -u -p -r1.82 i915_drv.h > --- dev/pci/drm/i915/i915_drv.h 4 May 2019 11:34:47 -0000 1.82 > +++ dev/pci/drm/i915/i915_drv.h 3 Jun 2019 19:04:30 -0000 > @@ -1876,6 +1876,9 @@ struct inteldrm_softc { > /* ordered wq for modesets */ > struct workqueue_struct *modeset_wq; > > + /* ordered wq for cleanups */ > + struct workqueue_struct *cleanup_wq; > + > /* Display functions */ > struct drm_i915_display_funcs display; > > Index: dev/pci/drm/i915/intel_display.c > =================================================================== > RCS file: /cvs/src/sys/dev/pci/drm/i915/intel_display.c,v > retrieving revision 1.63 > diff -u -p -r1.63 intel_display.c > --- dev/pci/drm/i915/intel_display.c 14 Apr 2019 10:14:52 -0000 1.63 > +++ dev/pci/drm/i915/intel_display.c 3 Jun 2019 19:04:31 -0000 > @@ -12761,9 +12761,14 @@ static void intel_atomic_commit_tail(str > * deferring to a new worker seems overkill, but we would place a > * schedule point (cond_resched()) here anyway to keep latencies > * down. > + * > + * NOTE: Some driver code paths (e.g. i915_driver_register) perform > + * blocking commits (via drm_atomic_commit) from system_wq tasks while > + * nonblocking commits use system_unbound_wq or our own modeset_wq. > + * Deferring cleanup to the same queue could cause workers to deadlock. > */ > INIT_WORK(&state->commit_work, intel_atomic_cleanup_work); > - schedule_work(&state->commit_work); > + queue_work(dev_priv->cleanup_wq, &state->commit_work); > } > > static void intel_atomic_commit_work(struct work_struct *work) > > [3:text/x-patch Hide] > > Index: dev/pci/drm/i915/intel_display.c > =================================================================== > RCS file: /cvs/src/sys/dev/pci/drm/i915/intel_display.c,v > retrieving revision 1.63 > diff -u -p -r1.63 intel_display.c > --- dev/pci/drm/i915/intel_display.c 14 Apr 2019 10:14:52 -0000 1.63 > +++ dev/pci/drm/i915/intel_display.c 3 Jun 2019 18:33:39 -0000 > @@ -12586,10 +12586,8 @@ static void intel_atomic_commit_fence_wa > #endif > } > > -static void intel_atomic_cleanup_work(struct work_struct *work) > +static void intel_atomic_cleanup(struct drm_atomic_state *state) > { > - struct drm_atomic_state *state = > - container_of(work, struct drm_atomic_state, commit_work); > struct drm_i915_private *i915 = to_i915(state->dev); > > drm_atomic_helper_cleanup_planes(&i915->drm, state); > @@ -12599,7 +12597,15 @@ static void intel_atomic_cleanup_work(st > intel_atomic_helper_free_state(i915); > } > > -static void intel_atomic_commit_tail(struct drm_atomic_state *state) > +static void intel_atomic_cleanup_work(struct work_struct *work) > +{ > + struct drm_atomic_state *state = > + container_of(work, struct drm_atomic_state, commit_work); > + > + intel_atomic_cleanup(state); > +} > + > +static void intel_atomic_commit_tail(struct drm_atomic_state *state, bool > nonblock) > { > struct drm_device *dev = state->dev; > struct intel_atomic_state *intel_state = to_intel_atomic_state(state); > @@ -12754,16 +12760,22 @@ static void intel_atomic_commit_tail(str > intel_display_power_put(dev_priv, POWER_DOMAIN_MODESET); > } > > - /* > - * Defer the cleanup of the old state to a separate worker to not > - * impede the current task (userspace for blocking modesets) that > - * are executed inline. For out-of-line asynchronous modesets/flips, > - * deferring to a new worker seems overkill, but we would place a > - * schedule point (cond_resched()) here anyway to keep latencies > - * down. > - */ > - INIT_WORK(&state->commit_work, intel_atomic_cleanup_work); > - schedule_work(&state->commit_work); > + if (nonblock) { > + /* > + * Defer the cleanup of the old state to a separate worker to > + * keep latencies down. > + */ > + INIT_WORK(&state->commit_work, intel_atomic_cleanup_work); > + schedule_work(&state->commit_work); > + } else { > + /* > + * NOTE: Some driver code paths (e.g. i915_driver_register) > + * perform blocking commits (via drm_atomic_commit) from > + * system_wq tasks. Deferring cleanup to system_wq as well > + * could cause workers to deadlock. > + */ > + intel_atomic_cleanup(state); > + } > } > > static void intel_atomic_commit_work(struct work_struct *work) > @@ -12771,7 +12783,7 @@ static void intel_atomic_commit_work(str > struct drm_atomic_state *state = > container_of(work, struct drm_atomic_state, commit_work); > > - intel_atomic_commit_tail(state); > + intel_atomic_commit_tail(state, true); > } > > static int __i915_sw_fence_call > @@ -12906,7 +12918,7 @@ static int intel_atomic_commit(struct dr > } else { > if (intel_state->modeset) > flush_workqueue(dev_priv->modeset_wq); > - intel_atomic_commit_tail(state); > + intel_atomic_commit_tail(state, false); > } > > return 0;