* [PATCH] i915: Fix opregion notifications
@ 2011-07-12 21:51 Matthew Garrett
2011-07-12 22:20 ` Keith Packard
2011-07-12 22:30 ` [PATCH V2] " Matthew Garrett
0 siblings, 2 replies; 4+ messages in thread
From: Matthew Garrett @ 2011-07-12 21:51 UTC (permalink / raw)
To: keithp; +Cc: intel-gfx, linux-acpi, Matthew Garrett
opregion-based platforms will send ACPI video event 0x80 for a range of
notification types for legacy compatibility. This is interpreted as a
display switch event, which may not be appropriate in the circumstances.
When we receive such an event we should make sure that the platform is
genuinely requesting a display switch before passing that event through
to userspace.
Signed-off-by: Matthew Garrett <mjg@redhat.com>
Tested-by: Adam Jackson <ajax@redhat.com>
---
drivers/acpi/video.c | 7 ++++---
drivers/gpu/drm/i915/intel_opregion.c | 15 +++++++++++----
include/acpi/video.h | 2 ++
3 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index db39e9e..ada4b4d 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -46,7 +46,6 @@
#define PREFIX "ACPI: "
-#define ACPI_VIDEO_CLASS "video"
#define ACPI_VIDEO_BUS_NAME "Video Bus"
#define ACPI_VIDEO_DEVICE_NAME "Video Device"
#define ACPI_VIDEO_NOTIFY_SWITCH 0x80
@@ -1445,7 +1444,8 @@ 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);
- keycode = KEY_SWITCHVIDEOMODE;
+ if (!acpi_notifier_call_chain(device, event, 0))
+ keycode = KEY_SWITCHVIDEOMODE;
break;
case ACPI_VIDEO_NOTIFY_PROBE: /* User plugged in or removed a video
@@ -1475,7 +1475,8 @@ static void acpi_video_bus_notify(struct acpi_device *device, u32 event)
break;
}
- acpi_notifier_call_chain(device, event, 0);
+ if (event != ACPI_VIDEO_NOTIFY_SWITCH)
+ acpi_notifier_call_chain(device, event, 0);
if (keycode) {
input_report_key(input, keycode, 1);
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index d2c7104..3443fc1c 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -297,19 +297,26 @@ static int intel_opregion_video_event(struct notifier_block *nb,
/* The only video events relevant to opregion are 0x80. These indicate
either a docking event, lid switch or display switch request. In
Linux, these are handled by the dock, button and video drivers.
- We might want to fix the video driver to be opregion-aware in
- future, but right now we just indicate to the firmware that the
- request has been handled */
+ */
struct opregion_acpi *acpi;
+ struct acpi_bus_event *event = data;
+ int ret = NOTIFY_OK;
+
+ if (strcmp(event->device_class, ACPI_VIDEO_CLASS))
+ return NOTIFY_DONE;
if (!system_opregion)
return NOTIFY_DONE;
acpi = system_opregion->acpi;
+
+ if (event->type == 0x80 && !(acpi->cevt & 0x1))
+ ret = NOTIFY_BAD;
+
acpi->csts = 0;
- return NOTIFY_OK;
+ return ret;
}
static struct notifier_block intel_opregion_notifier = {
diff --git a/include/acpi/video.h b/include/acpi/video.h
index 0e98e67..61109f2 100644
--- a/include/acpi/video.h
+++ b/include/acpi/video.h
@@ -5,6 +5,8 @@
struct acpi_device;
+#define ACPI_VIDEO_CLASS "video"
+
#define ACPI_VIDEO_DISPLAY_CRT 1
#define ACPI_VIDEO_DISPLAY_TV 2
#define ACPI_VIDEO_DISPLAY_DVI 3
--
1.7.6
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] i915: Fix opregion notifications
2011-07-12 21:51 [PATCH] i915: Fix opregion notifications Matthew Garrett
@ 2011-07-12 22:20 ` Keith Packard
2011-07-12 22:30 ` Matthew Garrett
2011-07-12 22:30 ` [PATCH V2] " Matthew Garrett
1 sibling, 1 reply; 4+ messages in thread
From: Keith Packard @ 2011-07-12 22:20 UTC (permalink / raw)
Cc: intel-gfx, linux-acpi, Matthew Garrett
[-- Attachment #1: Type: text/plain, Size: 1688 bytes --]
On Tue, 12 Jul 2011 17:51:36 -0400, Matthew Garrett <mjg@redhat.com> wrote:
> - keycode = KEY_SWITCHVIDEOMODE;
> + if (!acpi_notifier_call_chain(device, event, 0))
> + keycode = KEY_SWITCHVIDEOMODE;
Right, acpi_notify_call_chain returns -EINVAL when the underlying
function call returns NOTIFY_BAD.
> - acpi_notifier_call_chain(device, event, 0);
> + if (event != ACPI_VIDEO_NOTIFY_SWITCH)
> + acpi_notifier_call_chain(device, event, 0);
Not ideal to have two calls to this function only one of which will be
used. Could look like
bool check_call_chain = false;
...
case ACPI_VIDEO_NOTIFY_SWITCH:
acpi_bus_generate_proc_event(device, event, 0);
keycode = KEY_SWITCHVIDEOMODE;
check_call_chain = true
break;
...
if (acpi_notifier_call_chain(device, event, 0) < 0)
if (check_call_chain)
keycode = 0;
Feel free to just call me for bikeshedding.
> + if (strcmp(event->device_class, ACPI_VIDEO_CLASS))
> + return NOTIFY_DONE;
I'm not entirely clear on accepted coding style here, but I'd much
rather this look like:
if (strcmp(event->device_class, ACPI_VIDEO_CLASS) != 0)
I must have read this four times before I figured out that you weren't
early returning on all "video" devices...
> +
> + if (event->type == 0x80 && !(acpi->cevt & 0x1))
> + ret = NOTIFY_BAD;
> +
Seriously? It's some kind of magic non-switch switch event? And you can
tell by checking magic bits within the event?
How can I test this and know if it works?
--
keith.packard@intel.com
[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] i915: Fix opregion notifications
2011-07-12 22:20 ` Keith Packard
@ 2011-07-12 22:30 ` Matthew Garrett
0 siblings, 0 replies; 4+ messages in thread
From: Matthew Garrett @ 2011-07-12 22:30 UTC (permalink / raw)
To: Keith Packard; +Cc: intel-gfx, linux-acpi
On Tue, Jul 12, 2011 at 03:20:23PM -0700, Keith Packard wrote:
> Seriously? It's some kind of magic non-switch switch event? And you can
> tell by checking magic bits within the event?
The Intel drivers on Windows appear to have used 0x80 as a generic
"Something's chanegd" notification, overloading it for docking, lid
state change and display switch press. As a result of that right now
we're sending a display switch event whenever one of these occurs. Less
than ideal. Opregion gives us a magic field in shared OS/firmware memory
that can be used to distinguish between these events.
> How can I test this and know if it works?
Tested on a Thinkpad X200. Make sure suspend is disabled, then close the
lid. Open it again. The ACPI video input device will send a display
switch event. Add this patch and repeat and the event will vanish. The
display switch itself will still generate an event.
I'll send v2 now.
--
Matthew Garrett | mjg59@srcf.ucam.org
^ permalink raw reply [flat|nested] 4+ messages in thread
* [PATCH V2] i915: Fix opregion notifications
2011-07-12 21:51 [PATCH] i915: Fix opregion notifications Matthew Garrett
2011-07-12 22:20 ` Keith Packard
@ 2011-07-12 22:30 ` Matthew Garrett
1 sibling, 0 replies; 4+ messages in thread
From: Matthew Garrett @ 2011-07-12 22:30 UTC (permalink / raw)
To: keithp; +Cc: intel-gfx, linux-acpi, Matthew Garrett
opregion-based platforms will send ACPI video event 0x80 for a range of
notification types for legacy compatibility. This is interpreted as a
display switch event, which may not be appropriate in the circumstances.
When we receive such an event we should make sure that the platform is
genuinely requesting a display switch before passing that event through
to userspace.
Signed-off-by: Matthew Garrett <mjg@redhat.com>
Tested-by: Adam Jackson <ajax@redhat.com>
---
drivers/acpi/video.c | 7 ++++---
drivers/gpu/drm/i915/intel_opregion.c | 15 +++++++++++----
include/acpi/video.h | 2 ++
3 files changed, 17 insertions(+), 7 deletions(-)
diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index db39e9e..ada4b4d 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -46,7 +46,6 @@
#define PREFIX "ACPI: "
-#define ACPI_VIDEO_CLASS "video"
#define ACPI_VIDEO_BUS_NAME "Video Bus"
#define ACPI_VIDEO_DEVICE_NAME "Video Device"
#define ACPI_VIDEO_NOTIFY_SWITCH 0x80
@@ -1445,7 +1444,8 @@ 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);
- keycode = KEY_SWITCHVIDEOMODE;
+ if (!acpi_notifier_call_chain(device, event, 0))
+ keycode = KEY_SWITCHVIDEOMODE;
break;
case ACPI_VIDEO_NOTIFY_PROBE: /* User plugged in or removed a video
@@ -1475,7 +1475,8 @@ static void acpi_video_bus_notify(struct acpi_device *device, u32 event)
break;
}
- acpi_notifier_call_chain(device, event, 0);
+ if (event != ACPI_VIDEO_NOTIFY_SWITCH)
+ acpi_notifier_call_chain(device, event, 0);
if (keycode) {
input_report_key(input, keycode, 1);
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index d2c7104..b7c5ddb 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -297,19 +297,26 @@ static int intel_opregion_video_event(struct notifier_block *nb,
/* The only video events relevant to opregion are 0x80. These indicate
either a docking event, lid switch or display switch request. In
Linux, these are handled by the dock, button and video drivers.
- We might want to fix the video driver to be opregion-aware in
- future, but right now we just indicate to the firmware that the
- request has been handled */
+ */
struct opregion_acpi *acpi;
+ struct acpi_bus_event *event = data;
+ int ret = NOTIFY_OK;
+
+ if (strcmp(event->device_class, ACPI_VIDEO_CLASS) != 0)
+ return NOTIFY_DONE;
if (!system_opregion)
return NOTIFY_DONE;
acpi = system_opregion->acpi;
+
+ if (event->type == 0x80 && !(acpi->cevt & 0x1))
+ ret = NOTIFY_BAD;
+
acpi->csts = 0;
- return NOTIFY_OK;
+ return ret;
}
static struct notifier_block intel_opregion_notifier = {
diff --git a/include/acpi/video.h b/include/acpi/video.h
index 0e98e67..61109f2 100644
--- a/include/acpi/video.h
+++ b/include/acpi/video.h
@@ -5,6 +5,8 @@
struct acpi_device;
+#define ACPI_VIDEO_CLASS "video"
+
#define ACPI_VIDEO_DISPLAY_CRT 1
#define ACPI_VIDEO_DISPLAY_TV 2
#define ACPI_VIDEO_DISPLAY_DVI 3
--
1.7.6
^ permalink raw reply related [flat|nested] 4+ messages in thread
end of thread, other threads:[~2011-07-12 22:30 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-07-12 21:51 [PATCH] i915: Fix opregion notifications Matthew Garrett
2011-07-12 22:20 ` Keith Packard
2011-07-12 22:30 ` Matthew Garrett
2011-07-12 22:30 ` [PATCH V2] " Matthew Garrett
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).