Re: [Nouveau] [PATCH] drm/nouveau/kms: Implement KDB debug hooks for nouveau KMS.

[Maxim Levitsky <maximlevitsky@gmail.com>]

On Wed, 2010-09-22 at 16:06 +0200, Maxim Levitsky wrote: 
> On Wed, 2010-09-22 at 16:03 +0200, Maxim Levitsky wrote: 
> > On Wed, 2010-09-22 at 03:42 +0200, Maxim Levitsky wrote: 
> > > On Thu, 2010-09-02 at 13:46 +0300, Maxim Levitsky wrote: 
> > > > On Wed, 2010-09-01 at 06:35 -0500, Jason Wessel wrote: 
> > > > > On 09/01/2010 04:56 AM, Maxim Levitsky wrote:
> > > > > > On Thu, 2010-08-19 at 13:55 -0400, Chris Ball wrote: 
> > > > > >   
> > > > > >> Hi,
> > > > > >>
> > > > > >> Here's a patch to add support for KMS debugging to Nouveau, along the
> > > > > >> style of the previous patches for Intel¹ and Radeon².  I'm only able
> > > > > >> to test on nv50 here, so a test on nv04 would be much appreciated,
> > > > > >> and I've published instructions on how to test here³.  Thanks!
> > > > > >>
> > > > > >> - Chris.
> > > > > >>     
> > > > > >
> > > > > > Hi,
> > > > > >
> > > > > > I just tried that patch, but unfortunately nether with nor without it
> > > > > > kdb seems not to work.
> > > > > > It could be id10t error from my side, but I did test the kdb in the past
> > > > > > with few KMS patches, and it seemed to work.
> > > > > >
> > > > > > Now I can't even get its prompt on the console.
> > > > > >
> > > > > > This is what I do:
> > > > > >
> > > > > > echo kbd | sudo tee /sys/module/kgdboc/parameters/kgdboc
> > > > > > (also tried booting with kgdboc=kbd)
> > > > > >   
> > > > > 
> > > > > Try changing it to kgdboc=kms,kbd  or the "echo kms,kbd"
> > > > > 
> > > > > When you use only the kbd, the kms feature is not activated.
> > > > This doesn't help.
> > > > 
> > > > I am afraid that this bug isn't related to kms, but rather is generic.
> > > 
> > > I turns out that it was the NMI watchdog that I had enabled.
> > > Without it kdb works very well, including the kms support.
> > Please disregard this. kdb works with nmi watchdog now as well.
> > Probably something was fixed, maybe unrelated to it.
> > 
> > > 
> > > It would be better if you were to detect kms instead of adding an
> > > explicit param to kgdboc cmd line.
> > > 
> > > Also found out that after a debug session with Alt+SysRQ+g and X
> > > running, these keys aren't released. I had to press on all of them to
> > > make them release.
> > > It makes sense as kgdboc in that case reads directly from keyboard port.
> > And I see that kgdb actually has a code that works that around.
> > I suspect that what happens is that keys are released before X continues
> > running, and therefore it doesn't pick these events up.
> 
> However an evtest on input event running on kernel tty, still only picks
> release of the alt key.

[Dropped nouveau list, because this is offtopic there]

I pretty much got to the bottom of this.
There are 2 separate issues:


1. SysRq handler is now a input 'filter', which means that it can 'eat'
input events, so they don't show up on input bus.
It does so while sysrq key is down.
So sysrq and 'g' events never reach the kernel kbd driver and therefore
the hack to release them doesn't work.

2. The kbd_clear_keys_helper injects the keyup events alright, but it
doesn't inject SYN events, and therefore X evdev driver doesn't pick
these injected events untill next SYN event. 

This patch makes key release work in expense of showing sysrq key to userspace, which isn't that good,
because now Alt+SysRQ causes a screen capture by default.
In my opinion the sysrq filter should stay.
We should just make kdb hook into atkbd and do the key release there.
This should both result in cleaner/more robust code, and make this issue disappear.
I'll look at doing that.


diff --git a/drivers/char/keyboard.c b/drivers/char/keyboard.c
index 0c6c641..7df6af5 100644
--- a/drivers/char/keyboard.c
+++ b/drivers/char/keyboard.c
@@ -368,6 +368,7 @@ static int kbd_clear_keys_helper(struct input_handle *handle, void *data)
 {
 	unsigned int *keycode = data;
 	input_inject_event(handle, EV_KEY, *keycode, 0);
+	input_inject_event(handle, EV_SYN, SYN_REPORT, 0);
 	return 0;
 }
 
diff --git a/drivers/char/sysrq.c b/drivers/char/sysrq.c
index ef31bb8..db1eb12 100644
--- a/drivers/char/sysrq.c
+++ b/drivers/char/sysrq.c
@@ -601,7 +601,7 @@ static bool sysrq_filter(struct input_handle *handle, unsigned int type,
 	}
 
 out:
-	return sysrq_down;
+	return 0;
 }
 
 static int sysrq_connect(struct input_handler *handler,


Best regards,
	Maxim Levitsky

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html