From owner-freebsd-current@FreeBSD.ORG Mon Nov 10 06:21:09 2003 Return-Path: Delivered-To: freebsd-current@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [216.136.204.125]) by hub.freebsd.org (Postfix) with ESMTP id B9FB916A4D4; Mon, 10 Nov 2003 06:21:09 -0800 (PST) Received: from home2.morten-johansen.net (213.234.112.58.adsl.o-d.tiscali.no [213.234.112.58]) by mx1.FreeBSD.org (Postfix) with ESMTP id AC51043F85; Mon, 10 Nov 2003 06:21:07 -0800 (PST) (envelope-from mail@morten-johansen.net) Received: from morten-johansen.net (localhost [127.0.0.1]) hAAEL3xX000654; Mon, 10 Nov 2003 15:21:05 +0100 (CET) (envelope-from mail@morten-johansen.net) Message-ID: <3FAF9ECF.6020204@morten-johansen.net> Date: Mon, 10 Nov 2003 15:21:03 +0100 From: Morten Johansen User-Agent: Mozilla/5.0 (X11; U; FreeBSD i386; en-US; rv:1.4) Gecko/20030817 X-Accept-Language: no, nb, en-us, en MIME-Version: 1.0 To: Bruce Evans References: <3FA966B2.9040704@morten-johansen.net> <20031105202947.A43448@pooker.samsco.home> <3FAAF50B.9030105@morten-johansen.net> <20031107230107.O3769@gamplex.bde.org> <3FACB0DC.7020802@freebsd.org> <3FACEC1B.2040903@morten-johansen.net> <20031110205212.V2817@gamplex.bde.org> In-Reply-To: <20031110205212.V2817@gamplex.bde.org> Content-Type: text/plain; charset=us-ascii; format=flowed Content-Transfer-Encoding: 7bit cc: freebsd-current@freebsd.org Subject: Re: the PS/2 mouse problem X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.1 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 10 Nov 2003 14:21:09 -0000 Bruce Evans wrote: > On Sat, 8 Nov 2003, Morten Johansen wrote: > > >>Scott Long wrote: >> >>>Bruce Evans wrote: >>> >>>>[... possibly too much trimmed] >>> >>>The problem here is that the keyboard controller driver tries to be too >>>smart. If it detects that the hardware FIFO is full, it'll drain it into >>>a per-softc, per-function ring buffer. So having psm(4) just directly >>>read the hardware is insufficient in this scheme. > > > What is the per-function part? (I'm not very familar with psm, but once > understood simpler versions of the keyboard driver.) Several layers of > buffering might not be too bad for slow devices. The i/o times tend to > dominate unless you do silly things like a context switch to move each > character from one buffer to other, and even that can be fast enough > (I believe it is normal for interactive input on ptys; then there's often > a remote IPC or two per character as well). > > >>>>- it sometimes calls the DELAY() function, which is not permitted in fast >>>> interrupt handlers since apart from locking issues, fast interrupt >>>>handlers >>>> are not permitted to busy-wait. >>> >>>Again, the keyboard controller driver is too smart for its own good. To >>>summarize: >>> >>>read_aux_data_no_wait() >>>{ >>> Does softc->aux ring buffer contain data? >>> return ring buffer data >>> >>> Check the status register >>> Is the keyboard fifo full? >>> DELAY(7us) >>> read keyboard fifo into softc->kbd ring buffer >>> Check the status register >>> >>> Is the aux fifo full? >>> DELAY(7us) >>> return aux fifo data >>>} >>> >>>So you can wind up stalling for 14us in there, presumably because you >>>cannot read the status and data registers back-to-back without a delay. >>>I don't have the atkbd spec handy so I'm not sure how to optimize this. >>>Do you really need to check the status register before reading the data >>>register? > > > At least it's a bounded delay. I believe such delays are required for > some layers of the keyboard. Perhaps only for the keyboard (old hardware > only?) and not for the keyboard controller or the mouse. > > >>>>Many of the complications for fast interrupt handlers shouldn't be needed >>>>in psm. Just make psmintr() INTR_MPSAFE. >>> >>>I believe that the previous poster actually tried making it INTR_MPSAFE, >>>but didn't see a measurable benefit because the latency of scheduling >>>the ithread is still unacceptable. >> >>That is 100% correct. >>In the meantime I have taken your's and Bruce's advice and rearranged >>the interrupt handler to look like this: >> >>mtx_lock(&sc->input_mtx); > > > Er, this is reasonable for INTR_MPSAFE but not for INTR_FAST. > mtx_lock() is a "sleep" lock so it cannot be used in fast interrupt > handlers. mtx_lock_spin() must be used. (My version doesn't permit > use of mtx_lock_spin() either; more primitive locking must be used.) > > >>while((c = read_aux_data_no_wait(sc->kbdc)) != -1) { > > > This is probably INTR_FAST-safe enough in practice. > > >> sc->input_queue.buf[sc->input_queue.tail] = c; >> if ((++ sc->input_queue.tail) >= PSM_BUFSIZE) >> sc->input_queue.tail = 0; >> count = (++ sc->input_queue.count); >>} >>mtx_unlock(&sc->input_mtx); > > > The locking for the queue seems to be correct except this should operate > on a spinlock too. > > >>if (count >= sc->mode.packetsize) >> taskqueue_enqueue(taskqueue_swi_giant, &sc->psm_task); > > > taskqueue_enqueue() can only be used in non-fast interrupt handlers. > taskqueue_enqueue_fast() must be used in fast interrupt handlers (except > in my version, it is not permitted so it shouldn't exist). Note that > the spinlock/fast versions can be used for normal interrupt handlers > too, so not much more code is needed to support handlers whose fastness > is dynamically configured. > Yes, I have submitted a PR (kern/59067), with an INTR_FAST version that uses spinlocks and taskqueue_fast. You can find it here if you have time to look at it: http://dsm.oslonett.no/patch-psm-fast I would appreciate comments on it's correctness. > >>And it works, but having it INTR_MPSAFE still does NOT help my problem. >>It looks to me like data is getting lost because the interrupt handler >>is unable to read it before it's gone, and the driver gets out of sync, >>and has to reset itself. >>However it now takes a few more tries to provoke the problem, so >>something seems to have improved somewhere. > > > This is a bit surprising. There are still so few INTR_MPSAFE handlers > that there aren't many system activities that get in the way of running > the INTR_MPSAFE ones. Shared interrupts prevent running of a handler > while other handlers on the same interrupt are running, and the mouse > interrupt is often shared, but if it is shared then it couldn't be fast > until recently and still can't be fast unless all the other handlers on > it are fast. > > Bruce It seems odd that it should be necessary to have it INTR_FAST. But somehow it is, on my system. Morten