[Intel-gfx] [PATCH] i915: Fix opregion notifications

[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 m...@redhat.com
Tested-by: Adam Jackson a...@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_NAMEVideo 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

___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] i915: Fix opregion notifications

[Keith Packard]

On Tue, 12 Jul 2011 17:51:36 -0400, Matthew Garrett m...@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

boolcheck_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.pack...@intel.com


pgpMLeEcwWqaX.pgp
Description: PGP signature
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] i915: Fix opregion notifications

[Matthew Garrett]

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 | mj...@srcf.ucam.org
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx