Date: Mon, 10 Nov 2003 15:21:03 +0100 From: Morten Johansen <mail@morten-johansen.net> To: Bruce Evans <bde@zeta.org.au> Cc: freebsd-current@freebsd.org Subject: Re: the PS/2 mouse problem Message-ID: <3FAF9ECF.6020204@morten-johansen.net> In-Reply-To: <20031110205212.V2817@gamplex.bde.org> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?3FAF9ECF.6020204>