[prev in list] [next in list] [prev in thread] [next in thread]
List: openbsd-bugs
Subject: Re: inteldrm: cleanup_done timed out with external monitor
From: Mark Kettenis <mark.kettenis () xs4all ! nl>
Date: 2019-06-11 21:31:28
Message-ID: 54386705c11c18d8 () bloch ! sibelius ! xs4all ! nl
[Download RAW message or body]
> Date: Mon, 3 Jun 2019 23:00:31 +0200 (CEST)
> From: Mark Kettenis <mark.kettenis@xs4all.nl>
>
> > From: "Sven M. Hallberg" <pesco@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:
> > 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.
I just posted a diff that implements option 1 to tech@. It is
included below for your convenience. Can you give this a go?
Index: dev/pci/drm/drm_linux.c
===================================================================
RCS file: /cvs/src/sys/dev/pci/drm/drm_linux.c,v
retrieving revision 1.38
diff -u -p -r1.38 drm_linux.c
--- dev/pci/drm/drm_linux.c 9 Jun 2019 12:58:30 -0000 1.38
+++ dev/pci/drm/drm_linux.c 11 Jun 2019 18:54:38 -0000
@@ -1399,15 +1399,15 @@ drm_linux_init(void)
{
if (system_wq == NULL) {
system_wq = (struct workqueue_struct *)
- taskq_create("drmwq", 1, IPL_HIGH, 0);
+ taskq_create("drmwq", 4, IPL_HIGH, 0);
}
if (system_unbound_wq == NULL) {
system_unbound_wq = (struct workqueue_struct *)
- taskq_create("drmubwq", 1, IPL_HIGH, 0);
+ taskq_create("drmubwq", 4, IPL_HIGH, 0);
}
if (system_long_wq == NULL) {
system_long_wq = (struct workqueue_struct *)
- taskq_create("drmlwq", 1, IPL_HIGH, 0);
+ taskq_create("drmlwq", 4, IPL_HIGH, 0);
}
if (taskletq == NULL)
Index: dev/pci/drm/i915/intel_hotplug.c
===================================================================
RCS file: /cvs/src/sys/dev/pci/drm/i915/intel_hotplug.c,v
retrieving revision 1.2
diff -u -p -r1.2 intel_hotplug.c
--- dev/pci/drm/i915/intel_hotplug.c 14 Apr 2019 10:14:52 -0000 1.2
+++ dev/pci/drm/i915/intel_hotplug.c 11 Jun 2019 18:54:38 -0000
@@ -619,7 +619,6 @@ void intel_hpd_init_work(struct drm_i915
INIT_WORK(&dev_priv->hotplug.hotplug_work, i915_hotplug_work_func);
INIT_WORK(&dev_priv->hotplug.dig_port_work, i915_digport_work_func);
INIT_WORK(&dev_priv->hotplug.poll_init_work, i915_hpd_poll_init_work);
- dev_priv->hotplug.poll_init_work.tq = systq;
INIT_DELAYED_WORK(&dev_priv->hotplug.reenable_work,
intel_hpd_irq_storm_reenable_work);
}
Index: kern/kern_task.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_task.c,v
retrieving revision 1.25
diff -u -p -r1.25 kern_task.c
--- kern/kern_task.c 28 Apr 2019 04:20:40 -0000 1.25
+++ kern/kern_task.c 11 Jun 2019 18:54:39 -0000
@@ -43,6 +43,7 @@ struct taskq {
TQ_S_DESTROYED
} tq_state;
unsigned int tq_running;
+ unsigned int tq_barrier;
unsigned int tq_nthreads;
unsigned int tq_flags;
const char *tq_name;
@@ -59,6 +60,7 @@ static const char taskq_sys_name[] = "sy
struct taskq taskq_sys = {
TQ_S_CREATED,
0,
+ 0,
1,
0,
taskq_sys_name,
@@ -77,6 +79,7 @@ static const char taskq_sys_mp_name[] =
struct taskq taskq_sys_mp = {
TQ_S_CREATED,
0,
+ 0,
1,
TASKQ_MPSAFE,
taskq_sys_mp_name,
@@ -122,6 +125,7 @@ taskq_create(const char *name, unsigned
tq->tq_state = TQ_S_CREATED;
tq->tq_running = 0;
+ tq->tq_barrier = 0;
tq->tq_nthreads = nthreads;
tq->tq_name = name;
tq->tq_flags = flags;
@@ -161,6 +165,7 @@ taskq_destroy(struct taskq *tq)
panic("unexpected %s tq state %u", tq->tq_name, tq->tq_state);
}
+ tq->tq_barrier = 0;
while (tq->tq_running > 0) {
wakeup(tq);
msleep(&tq->tq_running, &tq->tq_mtx, PWAIT, "tqdestroy", 0);
@@ -223,6 +228,7 @@ taskq_barrier(struct taskq *tq)
WITNESS_CHECKORDER(&tq->tq_lock_object, LOP_NEWORDER, NULL);
+ SET(t.t_flags, TASK_BARRIER);
task_add(tq, &t);
cond_wait(&c, "tqbar");
}
@@ -238,6 +244,7 @@ taskq_del_barrier(struct taskq *tq, stru
if (task_del(tq, del))
return;
+ SET(t.t_flags, TASK_BARRIER);
task_add(tq, &t);
cond_wait(&c, "tqbar");
}
@@ -304,6 +311,7 @@ taskq_next_work(struct taskq *tq, struct
struct task *next;
mtx_enter(&tq->tq_mtx);
+retry:
while ((next = TAILQ_FIRST(&tq->tq_worklist)) == NULL) {
if (tq->tq_state != TQ_S_RUNNING) {
mtx_leave(&tq->tq_mtx);
@@ -311,6 +319,17 @@ taskq_next_work(struct taskq *tq, struct
}
msleep(tq, &tq->tq_mtx, PWAIT, "bored", 0);
+ }
+
+ if (ISSET(next->t_flags, TASK_BARRIER)) {
+ if (++tq->tq_barrier == tq->tq_nthreads) {
+ tq->tq_barrier = 0;
+ wakeup(tq);
+ } else {
+ while (tq->tq_barrier > 0)
+ msleep(tq, &tq->tq_mtx, PWAIT, "tqblk", 0);
+ goto retry;
+ }
}
TAILQ_REMOVE(&tq->tq_worklist, next, t_entry);
Index: sys/task.h
===================================================================
RCS file: /cvs/src/sys/sys/task.h,v
retrieving revision 1.15
diff -u -p -r1.15 task.h
--- sys/task.h 28 Apr 2019 04:20:40 -0000 1.15
+++ sys/task.h 11 Jun 2019 18:54:39 -0000
@@ -31,6 +31,7 @@ struct task {
};
#define TASK_ONQUEUE 1
+#define TASK_BARRIER 2
TAILQ_HEAD(task_list, task);
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic