diff mbox

[PATCH/RFC] drm/radeon: ACPI: veto the keypress on ATIF events

Message ID 20120802134612.GA30802@growl (mailing list archive)
State New, archived
Headers show

Commit Message

Luca Tettamanti Aug. 2, 2012, 1:46 p.m. UTC
On Thu, Aug 02, 2012 at 08:45:30AM +0800, Zhang Rui wrote:
> On ?, 2012-08-01 at 15:49 +0200, Luca Tettamanti wrote:
> > AMD ACPI interface may overload the standard event
> > ACPI_VIDEO_NOTIFY_PROBE (0x81) to signal AMD-specific events. In such
> > cases we don't want to send the keypress (KEY_SWITCHVIDEOMODE) to the
> > userspace because the user did not press the mode switch key (the
> > spurious keypress confuses the DE which usually changes the
> > display configuration and messes up a dual-screen setup).
> > This patch gives the radeon driver the chance to examine the event and
> > block the keypress if the event is an "AMD event".
> > 
> > Signed-off-by: Luca Tettamanti <kronos.it@gmail.com>
> > ---
> > Any comment from ACPI front?
> > 
> it looks good to me.
> But I'm wondering if we can use the following code for ACPI part, which
> looks cleaner.
> I know this may change the behavior of other events, but in theory, we
> should not send any input event if we know something wrong in kernel.
> 
> what do you think?

I like it, it's cleaner.
I've split the patch in two pieces (one for video, the other for
radeon) and adopted your suggestion.

BTW, I'm leaving for vacation in a few hours, I'll be offline till mid
August.

Luca
From acce30c96b90d1bc550e82a9e7f19226fa194d5e Mon Sep 17 00:00:00 2001
From: Luca Tettamanti <kronos.it@gmail.com>
Date: Thu, 2 Aug 2012 15:30:27 +0200
Subject: [PATCH 1/2] ACPI video: allow events handlers to veto the keypress

The standard video events may be overloaded for device specific
purposes. For example AMD ACPI interface overloads
ACPI_VIDEO_NOTIFY_PROBE (0x81) to signal AMD-specific events. In such
cases we don't want to send the keypress (KEY_SWITCHVIDEOMODE) to the
userspace because the user did not press the mode switch key (the
spurious keypress confuses the DE which usually changes the
display configuration and messes up a dual-screen setup).
This patch gives the handlers the chance to examine the event and
block the keypress if the event is device specific.
v2: refactor as suggested by Zhang Rui <rui.zhang@intel.com>

Signed-off-by: Luca Tettamanti <kronos.it@gmail.com>
---
 drivers/acpi/video.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

Comments

Zhang Rui Aug. 3, 2012, 1:40 a.m. UTC | #1
On ?, 2012-08-02 at 15:46 +0200, Luca Tettamanti wrote:
> On Thu, Aug 02, 2012 at 08:45:30AM +0800, Zhang Rui wrote:
> > On ?, 2012-08-01 at 15:49 +0200, Luca Tettamanti wrote:
> > > AMD ACPI interface may overload the standard event
> > > ACPI_VIDEO_NOTIFY_PROBE (0x81) to signal AMD-specific events. In such
> > > cases we don't want to send the keypress (KEY_SWITCHVIDEOMODE) to the
> > > userspace because the user did not press the mode switch key (the
> > > spurious keypress confuses the DE which usually changes the
> > > display configuration and messes up a dual-screen setup).
> > > This patch gives the radeon driver the chance to examine the event and
> > > block the keypress if the event is an "AMD event".
> > > 
> > > Signed-off-by: Luca Tettamanti <kronos.it@gmail.com>
> > > ---
> > > Any comment from ACPI front?
> > > 
> > it looks good to me.
> > But I'm wondering if we can use the following code for ACPI part, which
> > looks cleaner.
> > I know this may change the behavior of other events, but in theory, we
> > should not send any input event if we know something wrong in kernel.
> > 
> > what do you think?
> 
> I like it, it's cleaner.
> I've split the patch in two pieces (one for video, the other for
> radeon) and adopted your suggestion.
> 
Great.
Acked-by: Zhang Rui <rui.zhang@intel.com>

hmm, who should take these two patches?
and which tree the second patch is based on?

thanks,
rui

--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Alex Deucher Aug. 3, 2012, 1:45 a.m. UTC | #2
On Thu, Aug 2, 2012 at 9:40 PM, Zhang Rui <rui.zhang@intel.com> wrote:
> On ?, 2012-08-02 at 15:46 +0200, Luca Tettamanti wrote:
>> On Thu, Aug 02, 2012 at 08:45:30AM +0800, Zhang Rui wrote:
>> > On ?, 2012-08-01 at 15:49 +0200, Luca Tettamanti wrote:
>> > > AMD ACPI interface may overload the standard event
>> > > ACPI_VIDEO_NOTIFY_PROBE (0x81) to signal AMD-specific events. In such
>> > > cases we don't want to send the keypress (KEY_SWITCHVIDEOMODE) to the
>> > > userspace because the user did not press the mode switch key (the
>> > > spurious keypress confuses the DE which usually changes the
>> > > display configuration and messes up a dual-screen setup).
>> > > This patch gives the radeon driver the chance to examine the event and
>> > > block the keypress if the event is an "AMD event".
>> > >
>> > > Signed-off-by: Luca Tettamanti <kronos.it@gmail.com>
>> > > ---
>> > > Any comment from ACPI front?
>> > >
>> > it looks good to me.
>> > But I'm wondering if we can use the following code for ACPI part, which
>> > looks cleaner.
>> > I know this may change the behavior of other events, but in theory, we
>> > should not send any input event if we know something wrong in kernel.
>> >
>> > what do you think?
>>
>> I like it, it's cleaner.
>> I've split the patch in two pieces (one for video, the other for
>> radeon) and adopted your suggestion.
>>
> Great.
> Acked-by: Zhang Rui <rui.zhang@intel.com>
>
> hmm, who should take these two patches?

I'm happy to take the patches.

> and which tree the second patch is based on?

I've got a tree with all the radeon ACPI patches on the acpi_patches
branches of my git tree:
git://people.freedesktop.org/~agd5f/linux

Alex

>
> thanks,
> rui
>
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Zhang Rui Aug. 3, 2012, 2:06 a.m. UTC | #3
On ?, 2012-08-02 at 21:45 -0400, Alex Deucher wrote:
> On Thu, Aug 2, 2012 at 9:40 PM, Zhang Rui <rui.zhang@intel.com> wrote:
> > On ?, 2012-08-02 at 15:46 +0200, Luca Tettamanti wrote:
> >> On Thu, Aug 02, 2012 at 08:45:30AM +0800, Zhang Rui wrote:
> >> > On ?, 2012-08-01 at 15:49 +0200, Luca Tettamanti wrote:
> >> > > AMD ACPI interface may overload the standard event
> >> > > ACPI_VIDEO_NOTIFY_PROBE (0x81) to signal AMD-specific events. In such
> >> > > cases we don't want to send the keypress (KEY_SWITCHVIDEOMODE) to the
> >> > > userspace because the user did not press the mode switch key (the
> >> > > spurious keypress confuses the DE which usually changes the
> >> > > display configuration and messes up a dual-screen setup).
> >> > > This patch gives the radeon driver the chance to examine the event and
> >> > > block the keypress if the event is an "AMD event".
> >> > >
> >> > > Signed-off-by: Luca Tettamanti <kronos.it@gmail.com>
> >> > > ---
> >> > > Any comment from ACPI front?
> >> > >
> >> > it looks good to me.
> >> > But I'm wondering if we can use the following code for ACPI part, which
> >> > looks cleaner.
> >> > I know this may change the behavior of other events, but in theory, we
> >> > should not send any input event if we know something wrong in kernel.
> >> >
> >> > what do you think?
> >>
> >> I like it, it's cleaner.
> >> I've split the patch in two pieces (one for video, the other for
> >> radeon) and adopted your suggestion.
> >>
> > Great.
> > Acked-by: Zhang Rui <rui.zhang@intel.com>
> >
> > hmm, who should take these two patches?
> 
> I'm happy to take the patches.
> 
> > and which tree the second patch is based on?
> 
> I've got a tree with all the radeon ACPI patches on the acpi_patches
> branches of my git tree:
> git://people.freedesktop.org/~agd5f/linux
> 
great.

thanks,
rui

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

Patch

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index 1e0a9e1..d75642a 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -1448,8 +1448,7 @@  static void acpi_video_bus_notify(struct acpi_device *device, u32 event)
 	case ACPI_VIDEO_NOTIFY_SWITCH:	/* User requested a switch,
 					 * most likely via hotkey. */
 		acpi_bus_generate_proc_event(device, event, 0);
-		if (!acpi_notifier_call_chain(device, event, 0))
-			keycode = KEY_SWITCHVIDEOMODE;
+		keycode = KEY_SWITCHVIDEOMODE;
 		break;
 
 	case ACPI_VIDEO_NOTIFY_PROBE:	/* User plugged in or removed a video
@@ -1479,8 +1478,9 @@  static void acpi_video_bus_notify(struct acpi_device *device, u32 event)
 		break;
 	}
 
-	if (event != ACPI_VIDEO_NOTIFY_SWITCH)
-		acpi_notifier_call_chain(device, event, 0);
+	if (acpi_notifier_call_chain(device, event, 0))
+		/* Something vetoed the keypress. */
+		keycode = 0;
 
 	if (keycode) {
 		input_report_key(input, keycode, 1);