[RFC] i915/acpi: add lid status notification and detection

[RFC] i915/acpi: add lid status notification and detection

[Jesse Barnes]

In the i915 driver we want to know if the lid is open or closed and
when its status changes, since we want to use the closed state to
indicate that the LFP is unavailable (disconnected in KMS terms) and
open to indicate that it's usable.

To that end, this patch adds some code to the ACPI button driver to
send us lid notifications as they occur and provide us with current lid
status when called.

It's a bit ugly, and I don't handle the inter-module dependencies very
well at this point, but you should get the idea.

There's also a policy question here.  On some machines, a lid close
will cause the ACPI firmware to program the GPU, disabling the pipe
associated with the panel.  Should we detect this and turn it back on
at open time?  That could be dangerous if userspace has received the
LVDS hotplug event and changed the config out from under us...

Comments?

-- 
Jesse Barnes, Intel Open Source Technology Center

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 9195deb..ebb593e 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -113,6 +113,9 @@ static const struct file_operations acpi_button_state_fops = {
 	.release = single_release,
 };
 
+static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
+static struct acpi_device *lid_device;
+
 /* --------------------------------------------------------------------------
                               FS Interface (/proc)
    -------------------------------------------------------------------------- */
@@ -229,11 +232,38 @@ static int acpi_button_remove_fs(struct acpi_device *device)
 /* --------------------------------------------------------------------------
                                 Driver Interface
    -------------------------------------------------------------------------- */
+int acpi_lid_notifier_register(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&acpi_lid_notifier, nb);
+}
+EXPORT_SYMBOL(acpi_lid_notifier_register);
+
+int acpi_lid_notifier_unregister(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&acpi_lid_notifier, nb);
+}
+EXPORT_SYMBOL(acpi_lid_notifier_unregister);
+
+int acpi_lid_open(void)
+{
+	acpi_status status;
+	unsigned long long state;
+
+	status = acpi_evaluate_integer(lid_device->handle, "_LID", NULL,
+				       &state);
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+
+	return !!state;
+}
+EXPORT_SYMBOL(acpi_lid_open);
+
 static int acpi_lid_send_state(struct acpi_device *device)
 {
 	struct acpi_button *button = acpi_driver_data(device);
 	unsigned long long state;
 	acpi_status status;
+	int ret;
 
 	status = acpi_evaluate_integer(device->handle, "_LID", NULL, &state);
 	if (ACPI_FAILURE(status))
@@ -242,7 +272,12 @@ static int acpi_lid_send_state(struct acpi_device *device)
 	/* input layer checks if event is redundant */
 	input_report_switch(button->input, SW_LID, !state);
 	input_sync(button->input);
-	return 0;
+
+	ret = blocking_notifier_call_chain(&acpi_lid_notifier, state, device);
+	if (ret == NOTIFY_DONE)
+		ret = blocking_notifier_call_chain(&acpi_lid_notifier, state,
+						   device);
+	return ret;
 }
 
 static void acpi_button_notify(struct acpi_device *device, u32 event)
@@ -364,8 +399,14 @@ static int acpi_button_add(struct acpi_device *device)
 	error = input_register_device(input);
 	if (error)
 		goto err_remove_fs;
-	if (button->type == ACPI_BUTTON_TYPE_LID)
+	if (button->type == ACPI_BUTTON_TYPE_LID) {
 		acpi_lid_send_state(device);
+		/*
+		 * This assumes there's only one lid device, or if there are
+		 * more we only care about the last one...
+		 */
+		lid_device = device;
+	}
 
 	if (device->wakeup.flags.valid) {
 		/* Button's GPE is run-wake GPE */
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2506592..fb12fb9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -190,6 +190,8 @@ typedef struct drm_i915_private {
 	unsigned int lvds_use_ssc:1;
 	int lvds_ssc_freq;
 
+	struct notifier_block lid_notifier;
+
 	struct drm_i915_fence_reg fence_regs[16]; /* assume 965 */
 	int fence_reg_start; /* 4 if userland hasn't ioctl'd us yet */
 	int num_fence_regs; /* 8 on pre-965, 16 otherwise */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bdcda36..08c1260 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -24,6 +24,8 @@
  *	Eric Anholt <eric@anholt.net>
  */
 
+#include <linux/module.h>
+#include <linux/input.h>
 #include <linux/i2c.h>
 #include "drmP.h"
 #include "intel_drv.h"
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 6619f26..8f05b09 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -27,6 +27,7 @@
  *      Jesse Barnes <jesse.barnes@intel.com>
  */
 
+#include <acpi/button.h>
 #include <linux/dmi.h>
 #include <linux/i2c.h>
 #include "drmP.h"
@@ -277,12 +278,18 @@ static void intel_lvds_mode_set(struct drm_encoder *encoder,
 /**
  * Detect the LVDS connection.
  *
- * This always returns CONNECTOR_STATUS_CONNECTED.  This connector should only have
- * been set up if the LVDS was actually connected anyway.
+ * Since LVDS doesn't have hotlug, we use the lid as a proxy.  Open means
+ * connected and closed means disconnected.  We also send hotplug events as
+ * needed, using lid status notification from the input layer.
  */
 static enum drm_connector_status intel_lvds_detect(struct drm_connector *connector)
 {
-	return connector_status_connected;
+	enum drm_connector_status status = connector_status_connected;
+
+	if (!acpi_lid_open())
+		status = connector_status_disconnected;
+
+	return status;
 }
 
 /**
@@ -321,6 +328,17 @@ static int intel_lvds_get_modes(struct drm_connector *connector)
 	return 0;
 }
 
+static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
+			    void *unused)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(nb, struct drm_i915_private, lid_notifier);
+
+	drm_sysfs_hotplug_event(dev_priv->dev);
+
+	return NOTIFY_OK;
+}
+
 /**
  * intel_lvds_destroy - unregister and free LVDS structures
  * @connector: connector to free
@@ -330,10 +348,14 @@ static int intel_lvds_get_modes(struct drm_connector *connector)
  */
 static void intel_lvds_destroy(struct drm_connector *connector)
 {
+	struct drm_device *dev = connector->dev;
 	struct intel_output *intel_output = to_intel_output(connector);
+	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	if (intel_output->ddc_bus)
 		intel_i2c_destroy(intel_output->ddc_bus);
+	if (dev_priv->lid_notifier.notifier_call)
+		acpi_lid_notifier_unregister(&dev_priv->lid_notifier);
 	drm_sysfs_connector_remove(connector);
 	drm_connector_cleanup(connector);
 	kfree(connector);
@@ -508,6 +530,11 @@ void intel_lvds_init(struct drm_device *dev)
 		goto failed;
 
 out:
+	dev_priv->lid_notifier.notifier_call = intel_lid_notify;
+	if (acpi_lid_notifier_register(&dev_priv->lid_notifier)) {
+		DRM_DEBUG("lid notifier registration failed\n");
+		dev_priv->lid_notifier.notifier_call = NULL;
+	}
 	drm_sysfs_connector_add(connector);
 	return;
 
diff --git a/include/acpi/button.h b/include/acpi/button.h
new file mode 100644
index 0000000..bb643a7
--- /dev/null
+++ b/include/acpi/button.h
@@ -0,0 +1,10 @@
+#ifndef ACPI_BUTTON_H
+#define ACPI_BUTTON_H
+
+#include <linux/notifier.h>
+
+extern int acpi_lid_notifier_register(struct notifier_block *nb);
+extern int acpi_lid_notifier_unregister(struct notifier_block *nb);
+extern int acpi_lid_open(void);
+
+#endif /* ACPI_BUTTON_H */

Re: [RFC] i915/acpi: add lid status notification and detection

[yakui_zhao]

On Thu, 2009-05-14 at 02:57 +0800, Jesse Barnes wrote:
> In the i915 driver we want to know if the lid is open or closed and
> when its status changes, since we want to use the closed state to
> indicate that the LFP is unavailable (disconnected in KMS terms) and
> open to indicate that it's usable.
> 
> To that end, this patch adds some code to the ACPI button driver to
> send us lid notifications as they occur and provide us with current lid
> status when called.
> 
> It's a bit ugly, and I don't handle the inter-module dependencies very
> well at this point, but you should get the idea.
The tight module dependency. If the ACPI button is compiled as module
and i915 is compiled as built-in kernel, we will fail in kernel
compilation. 
> There's also a policy question here.  On some machines, a lid close
> will cause the ACPI firmware to program the GPU, disabling the pipe
> associated with the panel.  Should we detect this and turn it back on
> at open time?  That could be dangerous if userspace has received the
> LVDS hotplug event and changed the config out from under us...
> 
> Comments?
It seems that the LID status is used to determine whether the LVDS is
connected.
It is not reliable. On some boxes the initial LID status is incorrect.
Maybe the LID status is open. But the ACPI returns that the LID is
close. In such case the LVDS is not initialized and user can't get the
output.
> 
> -- 
> Jesse Barnes, Intel Open Source Technology Center
> 
> diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
> index 9195deb..ebb593e 100644
> --- a/drivers/acpi/button.c
> +++ b/drivers/acpi/button.c
> @@ -113,6 +113,9 @@ static const struct file_operations acpi_button_state_fops = {
>  	.release = single_release,
>  };
>  
> +static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
> +static struct acpi_device *lid_device;
> +
>  /* --------------------------------------------------------------------------
>                                FS Interface (/proc)
>     -------------------------------------------------------------------------- */
> @@ -229,11 +232,38 @@ static int acpi_button_remove_fs(struct acpi_device *device)
>  /* --------------------------------------------------------------------------
>                                  Driver Interface
>     -------------------------------------------------------------------------- */
> +int acpi_lid_notifier_register(struct notifier_block *nb)
> +{
> +	return blocking_notifier_chain_register(&acpi_lid_notifier, nb);
> +}
> +EXPORT_SYMBOL(acpi_lid_notifier_register);
> +
> +int acpi_lid_notifier_unregister(struct notifier_block *nb)
> +{
> +	return blocking_notifier_chain_unregister(&acpi_lid_notifier, nb);
> +}
> +EXPORT_SYMBOL(acpi_lid_notifier_unregister);
> +
> +int acpi_lid_open(void)
> +{
> +	acpi_status status;
> +	unsigned long long state;
> +
> +	status = acpi_evaluate_integer(lid_device->handle, "_LID", NULL,
> +				       &state);
> +	if (ACPI_FAILURE(status))
> +		return -ENODEV;
> +
> +	return !!state;
> +}
> +EXPORT_SYMBOL(acpi_lid_open);
> +
>  static int acpi_lid_send_state(struct acpi_device *device)
>  {
>  	struct acpi_button *button = acpi_driver_data(device);
>  	unsigned long long state;
>  	acpi_status status;
> +	int ret;
>  
>  	status = acpi_evaluate_integer(device->handle, "_LID", NULL, &state);
>  	if (ACPI_FAILURE(status))
> @@ -242,7 +272,12 @@ static int acpi_lid_send_state(struct acpi_device *device)
>  	/* input layer checks if event is redundant */
>  	input_report_switch(button->input, SW_LID, !state);
>  	input_sync(button->input);
> -	return 0;
> +
> +	ret = blocking_notifier_call_chain(&acpi_lid_notifier, state, device);
> +	if (ret == NOTIFY_DONE)
> +		ret = blocking_notifier_call_chain(&acpi_lid_notifier, state,
> +						   device);
> +	return ret;
>  }
>  
>  static void acpi_button_notify(struct acpi_device *device, u32 event)
> @@ -364,8 +399,14 @@ static int acpi_button_add(struct acpi_device *device)
>  	error = input_register_device(input);
>  	if (error)
>  		goto err_remove_fs;
> -	if (button->type == ACPI_BUTTON_TYPE_LID)
> +	if (button->type == ACPI_BUTTON_TYPE_LID) {
>  		acpi_lid_send_state(device);
> +		/*
> +		 * This assumes there's only one lid device, or if there are
> +		 * more we only care about the last one...
> +		 */
> +		lid_device = device;
> +	}
>  
>  	if (device->wakeup.flags.valid) {
>  		/* Button's GPE is run-wake GPE */
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 2506592..fb12fb9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -190,6 +190,8 @@ typedef struct drm_i915_private {
>  	unsigned int lvds_use_ssc:1;
>  	int lvds_ssc_freq;
>  
> +	struct notifier_block lid_notifier;
> +
>  	struct drm_i915_fence_reg fence_regs[16]; /* assume 965 */
>  	int fence_reg_start; /* 4 if userland hasn't ioctl'd us yet */
>  	int num_fence_regs; /* 8 on pre-965, 16 otherwise */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index bdcda36..08c1260 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -24,6 +24,8 @@
>   *	Eric Anholt <eric@anholt.net>
>   */
>  
> +#include <linux/module.h>
> +#include <linux/input.h>
>  #include <linux/i2c.h>
>  #include "drmP.h"
>  #include "intel_drv.h"
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 6619f26..8f05b09 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -27,6 +27,7 @@
>   *      Jesse Barnes <jesse.barnes@intel.com>
>   */
>  
> +#include <acpi/button.h>
>  #include <linux/dmi.h>
>  #include <linux/i2c.h>
>  #include "drmP.h"
> @@ -277,12 +278,18 @@ static void intel_lvds_mode_set(struct drm_encoder *encoder,
>  /**
>   * Detect the LVDS connection.
>   *
> - * This always returns CONNECTOR_STATUS_CONNECTED.  This connector should only have
> - * been set up if the LVDS was actually connected anyway.
> + * Since LVDS doesn't have hotlug, we use the lid as a proxy.  Open means
> + * connected and closed means disconnected.  We also send hotplug events as
> + * needed, using lid status notification from the input layer.
>   */
>  static enum drm_connector_status intel_lvds_detect(struct drm_connector *connector)
>  {
> -	return connector_status_connected;
> +	enum drm_connector_status status = connector_status_connected;
> +
> +	if (!acpi_lid_open())
> +		status = connector_status_disconnected;
> +
> +	return status;
>  }
>  
>  /**
> @@ -321,6 +328,17 @@ static int intel_lvds_get_modes(struct drm_connector *connector)
>  	return 0;
>  }
>  
> +static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
> +			    void *unused)
> +{
> +	struct drm_i915_private *dev_priv =
> +		container_of(nb, struct drm_i915_private, lid_notifier);
> +
> +	drm_sysfs_hotplug_event(dev_priv->dev);
> +
> +	return NOTIFY_OK;
> +}
> +
>  /**
>   * intel_lvds_destroy - unregister and free LVDS structures
>   * @connector: connector to free
> @@ -330,10 +348,14 @@ static int intel_lvds_get_modes(struct drm_connector *connector)
>   */
>  static void intel_lvds_destroy(struct drm_connector *connector)
>  {
> +	struct drm_device *dev = connector->dev;
>  	struct intel_output *intel_output = to_intel_output(connector);
> +	struct drm_i915_private *dev_priv = dev->dev_private;
>  
>  	if (intel_output->ddc_bus)
>  		intel_i2c_destroy(intel_output->ddc_bus);
> +	if (dev_priv->lid_notifier.notifier_call)
> +		acpi_lid_notifier_unregister(&dev_priv->lid_notifier);
>  	drm_sysfs_connector_remove(connector);
>  	drm_connector_cleanup(connector);
>  	kfree(connector);
> @@ -508,6 +530,11 @@ void intel_lvds_init(struct drm_device *dev)
>  		goto failed;
>  
>  out:
> +	dev_priv->lid_notifier.notifier_call = intel_lid_notify;
> +	if (acpi_lid_notifier_register(&dev_priv->lid_notifier)) {
> +		DRM_DEBUG("lid notifier registration failed\n");
> +		dev_priv->lid_notifier.notifier_call = NULL;
> +	}
>  	drm_sysfs_connector_add(connector);
>  	return;
>  
> diff --git a/include/acpi/button.h b/include/acpi/button.h
> new file mode 100644
> index 0000000..bb643a7
> --- /dev/null
> +++ b/include/acpi/button.h
> @@ -0,0 +1,10 @@
> +#ifndef ACPI_BUTTON_H
> +#define ACPI_BUTTON_H
> +
> +#include <linux/notifier.h>
> +
> +extern int acpi_lid_notifier_register(struct notifier_block *nb);
> +extern int acpi_lid_notifier_unregister(struct notifier_block *nb);
> +extern int acpi_lid_open(void);
> +
> +#endif /* ACPI_BUTTON_H */
> 
> --
> 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

[PATCH] i915/acpi: add lid status notification and detection

[Jesse Barnes]

On Thu, 14 May 2009 09:22:24 +0800
yakui_zhao <yakui.zhao@intel.com> wrote:

> On Thu, 2009-05-14 at 02:57 +0800, Jesse Barnes wrote:
> > In the i915 driver we want to know if the lid is open or closed and
> > when its status changes, since we want to use the closed state to
> > indicate that the LFP is unavailable (disconnected in KMS terms) and
> > open to indicate that it's usable.
> > 
> > To that end, this patch adds some code to the ACPI button driver to
> > send us lid notifications as they occur and provide us with current
> > lid status when called.
> > 
> > It's a bit ugly, and I don't handle the inter-module dependencies
> > very well at this point, but you should get the idea.
> The tight module dependency. If the ACPI button is compiled as module
> and i915 is compiled as built-in kernel, we will fail in kernel
> compilation. 

I think a select ACPI_BUTTON in the i915 kconfig should handle that...

> > There's also a policy question here.  On some machines, a lid close
> > will cause the ACPI firmware to program the GPU, disabling the pipe
> > associated with the panel.  Should we detect this and turn it back
> > on at open time?  That could be dangerous if userspace has received
> > the LVDS hotplug event and changed the config out from under us...
> > 
> > Comments?
> It seems that the LID status is used to determine whether the LVDS is
> connected.
> It is not reliable. On some boxes the initial LID status is incorrect.
> Maybe the LID status is open. But the ACPI returns that the LID is
> close. In such case the LVDS is not initialized and user can't get the
> output.

Do we have a blacklist for that?  If the lid status isn't reliable and
the firmware messes with the GPU at lid event time we'll be in
trouble...

If it looks ok other than that, can we send it up through Eric's tree?

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center


In the i915 driver we want to know if the lid is open or closed and
when its status changes, since we want to use the closed state to
indicate that the LFP is unavailable (disconnected in KMS terms) and
open to indicate that it's usable.

To that end, this patch adds some code to the ACPI button driver to
send us lid notifications as they occur and provide us with current lid
status when called.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 9195deb..ebb593e 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -113,6 +113,9 @@ static const struct file_operations acpi_button_state_fops = {
 	.release = single_release,
 };
 
+static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
+static struct acpi_device *lid_device;
+
 /* --------------------------------------------------------------------------
                               FS Interface (/proc)
    -------------------------------------------------------------------------- */
@@ -229,11 +232,38 @@ static int acpi_button_remove_fs(struct acpi_device *device)
 /* --------------------------------------------------------------------------
                                 Driver Interface
    -------------------------------------------------------------------------- */
+int acpi_lid_notifier_register(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&acpi_lid_notifier, nb);
+}
+EXPORT_SYMBOL(acpi_lid_notifier_register);
+
+int acpi_lid_notifier_unregister(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&acpi_lid_notifier, nb);
+}
+EXPORT_SYMBOL(acpi_lid_notifier_unregister);
+
+int acpi_lid_open(void)
+{
+	acpi_status status;
+	unsigned long long state;
+
+	status = acpi_evaluate_integer(lid_device->handle, "_LID", NULL,
+				       &state);
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+
+	return !!state;
+}
+EXPORT_SYMBOL(acpi_lid_open);
+
 static int acpi_lid_send_state(struct acpi_device *device)
 {
 	struct acpi_button *button = acpi_driver_data(device);
 	unsigned long long state;
 	acpi_status status;
+	int ret;
 
 	status = acpi_evaluate_integer(device->handle, "_LID", NULL, &state);
 	if (ACPI_FAILURE(status))
@@ -242,7 +272,12 @@ static int acpi_lid_send_state(struct acpi_device *device)
 	/* input layer checks if event is redundant */
 	input_report_switch(button->input, SW_LID, !state);
 	input_sync(button->input);
-	return 0;
+
+	ret = blocking_notifier_call_chain(&acpi_lid_notifier, state, device);
+	if (ret == NOTIFY_DONE)
+		ret = blocking_notifier_call_chain(&acpi_lid_notifier, state,
+						   device);
+	return ret;
 }
 
 static void acpi_button_notify(struct acpi_device *device, u32 event)
@@ -364,8 +399,14 @@ static int acpi_button_add(struct acpi_device *device)
 	error = input_register_device(input);
 	if (error)
 		goto err_remove_fs;
-	if (button->type == ACPI_BUTTON_TYPE_LID)
+	if (button->type == ACPI_BUTTON_TYPE_LID) {
 		acpi_lid_send_state(device);
+		/*
+		 * This assumes there's only one lid device, or if there are
+		 * more we only care about the last one...
+		 */
+		lid_device = device;
+	}
 
 	if (device->wakeup.flags.valid) {
 		/* Button's GPE is run-wake GPE */
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 3a22eb9..dde56b6 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -71,6 +71,7 @@ config DRM_I915
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
 	select FB
+	select ACPI_BUTTON
 	tristate "i915 driver"
 	help
 	  Choose this option if you have a system that has Intel 830M, 845G,
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2506592..fb12fb9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -190,6 +190,8 @@ typedef struct drm_i915_private {
 	unsigned int lvds_use_ssc:1;
 	int lvds_ssc_freq;
 
+	struct notifier_block lid_notifier;
+
 	struct drm_i915_fence_reg fence_regs[16]; /* assume 965 */
 	int fence_reg_start; /* 4 if userland hasn't ioctl'd us yet */
 	int num_fence_regs; /* 8 on pre-965, 16 otherwise */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bdcda36..08c1260 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -24,6 +24,8 @@
  *	Eric Anholt <eric@anholt.net>
  */
 
+#include <linux/module.h>
+#include <linux/input.h>
 #include <linux/i2c.h>
 #include "drmP.h"
 #include "intel_drv.h"
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 6619f26..8f05b09 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -27,6 +27,7 @@
  *      Jesse Barnes <jesse.barnes@intel.com>
  */
 
+#include <acpi/button.h>
 #include <linux/dmi.h>
 #include <linux/i2c.h>
 #include "drmP.h"
@@ -277,12 +278,18 @@ static void intel_lvds_mode_set(struct drm_encoder *encoder,
 /**
  * Detect the LVDS connection.
  *
- * This always returns CONNECTOR_STATUS_CONNECTED.  This connector should only have
- * been set up if the LVDS was actually connected anyway.
+ * Since LVDS doesn't have hotlug, we use the lid as a proxy.  Open means
+ * connected and closed means disconnected.  We also send hotplug events as
+ * needed, using lid status notification from the input layer.
  */
 static enum drm_connector_status intel_lvds_detect(struct drm_connector *connector)
 {
-	return connector_status_connected;
+	enum drm_connector_status status = connector_status_connected;
+
+	if (!acpi_lid_open())
+		status = connector_status_disconnected;
+
+	return status;
 }
 
 /**
@@ -321,6 +328,17 @@ static int intel_lvds_get_modes(struct drm_connector *connector)
 	return 0;
 }
 
+static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
+			    void *unused)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(nb, struct drm_i915_private, lid_notifier);
+
+	drm_sysfs_hotplug_event(dev_priv->dev);
+
+	return NOTIFY_OK;
+}
+
 /**
  * intel_lvds_destroy - unregister and free LVDS structures
  * @connector: connector to free
@@ -330,10 +348,14 @@ static int intel_lvds_get_modes(struct drm_connector *connector)
  */
 static void intel_lvds_destroy(struct drm_connector *connector)
 {
+	struct drm_device *dev = connector->dev;
 	struct intel_output *intel_output = to_intel_output(connector);
+	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	if (intel_output->ddc_bus)
 		intel_i2c_destroy(intel_output->ddc_bus);
+	if (dev_priv->lid_notifier.notifier_call)
+		acpi_lid_notifier_unregister(&dev_priv->lid_notifier);
 	drm_sysfs_connector_remove(connector);
 	drm_connector_cleanup(connector);
 	kfree(connector);
@@ -508,6 +530,11 @@ void intel_lvds_init(struct drm_device *dev)
 		goto failed;
 
 out:
+	dev_priv->lid_notifier.notifier_call = intel_lid_notify;
+	if (acpi_lid_notifier_register(&dev_priv->lid_notifier)) {
+		DRM_DEBUG("lid notifier registration failed\n");
+		dev_priv->lid_notifier.notifier_call = NULL;
+	}
 	drm_sysfs_connector_add(connector);
 	return;
 
diff --git a/include/acpi/button.h b/include/acpi/button.h
new file mode 100644
index 0000000..bb643a7
--- /dev/null
+++ b/include/acpi/button.h
@@ -0,0 +1,10 @@
+#ifndef ACPI_BUTTON_H
+#define ACPI_BUTTON_H
+
+#include <linux/notifier.h>
+
+extern int acpi_lid_notifier_register(struct notifier_block *nb);
+extern int acpi_lid_notifier_unregister(struct notifier_block *nb);
+extern int acpi_lid_open(void);
+
+#endif /* ACPI_BUTTON_H */

Re: [RFC] i915/acpi: add lid status notification and detection

[Matthew Garrett]

On Thu, May 14, 2009 at 09:22:24AM +0800, yakui_zhao wrote:
> On Thu, 2009-05-14 at 02:57 +0800, Jesse Barnes wrote:
> > In the i915 driver we want to know if the lid is open or closed and
> > when its status changes, since we want to use the closed state to
> > indicate that the LFP is unavailable (disconnected in KMS terms) and
> > open to indicate that it's usable.
> > 
> > To that end, this patch adds some code to the ACPI button driver to
> > send us lid notifications as they occur and provide us with current lid
> > status when called.
> > 
> > It's a bit ugly, and I don't handle the inter-module dependencies very
> > well at this point, but you should get the idea.
> The tight module dependency. If the ACPI button is compiled as module
> and i915 is compiled as built-in kernel, we will fail in kernel
> compilation. 

Yes, it needs a Kconfig fixup.

> > There's also a policy question here.  On some machines, a lid close
> > will cause the ACPI firmware to program the GPU, disabling the pipe
> > associated with the panel.  Should we detect this and turn it back on
> > at open time?  That could be dangerous if userspace has received the
> > LVDS hotplug event and changed the config out from under us...
> > 
> > Comments?
> It seems that the LID status is used to determine whether the LVDS is
> connected.
> It is not reliable. On some boxes the initial LID status is incorrect.
> Maybe the LID status is open. But the ACPI returns that the LID is
> close. In such case the LVDS is not initialized and user can't get the
> output.

Really? I haven't seen any cases of this. They'll fail in all sorts of 
fun ways with modern userland.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [RFC] i915/acpi: add lid status notification and detection

[Zhang Rui]

On Wed, 2009-05-20 at 01:15 +0800, Matthew Garrett wrote:
> > > There's also a policy question here.  On some machines, a lid close
> > > will cause the ACPI firmware to program the GPU, disabling the pipe
> > > associated with the panel.  Should we detect this and turn it back on
> > > at open time?  That could be dangerous if userspace has received the
> > > LVDS hotplug event and changed the config out from under us...
> > > 
> > > Comments?
> > It seems that the LID status is used to determine whether the LVDS is
> > connected.
> > It is not reliable. On some boxes the initial LID status is incorrect.
> > Maybe the LID status is open. But the ACPI returns that the LID is
> > close. In such case the LVDS is not initialized and user can't get the
> > output.
> 
> Really? I haven't seen any cases of this. They'll fail in all sorts of 
> fun ways with modern userland.
> 
This is rare, and if this happens, a bug should be filed against ACPI.
BTW: we have fixed/root caused all such kind of bugs that have been
reported.
So I think it makes sense to trust the Lid state reported by ACPI button
driver.

thanks,
rui

Re: [RFC] i915/acpi: add lid status notification and detection

[Jesse Barnes]

[-- Attachment #1: Type: text/plain, Size: 1458 bytes --]

On Thu, 21 May 2009 16:57:53 +0800
Zhang Rui <rui.zhang@intel.com> wrote:

> On Wed, 2009-05-20 at 01:15 +0800, Matthew Garrett wrote:
> > > > There's also a policy question here.  On some machines, a lid
> > > > close will cause the ACPI firmware to program the GPU,
> > > > disabling the pipe associated with the panel.  Should we detect
> > > > this and turn it back on at open time?  That could be dangerous
> > > > if userspace has received the LVDS hotplug event and changed
> > > > the config out from under us...
> > > > 
> > > > Comments?
> > > It seems that the LID status is used to determine whether the
> > > LVDS is connected.
> > > It is not reliable. On some boxes the initial LID status is
> > > incorrect. Maybe the LID status is open. But the ACPI returns
> > > that the LID is close. In such case the LVDS is not initialized
> > > and user can't get the output.
> > 
> > Really? I haven't seen any cases of this. They'll fail in all sorts
> > of fun ways with modern userland.
> > 
> This is rare, and if this happens, a bug should be filed against ACPI.
> BTW: we have fixed/root caused all such kind of bugs that have been
> reported.
> So I think it makes sense to trust the Lid state reported by ACPI
> button driver.

So is that two acks for the patch?  If so, should it be split or can it
just go in through the i915 driver tree?

Len?  (Patch attached for reference.)

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

[-- Attachment #2: i915-lvds-lid-detect-2.patch --]
[-- Type: text/x-patch, Size: 7062 bytes --]

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 9195deb..ebb593e 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -113,6 +113,9 @@ static const struct file_operations acpi_button_state_fops = {
 	.release = single_release,
 };
 
+static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
+static struct acpi_device *lid_device;
+
 /* --------------------------------------------------------------------------
                               FS Interface (/proc)
    -------------------------------------------------------------------------- */
@@ -229,11 +232,38 @@ static int acpi_button_remove_fs(struct acpi_device *device)
 /* --------------------------------------------------------------------------
                                 Driver Interface
    -------------------------------------------------------------------------- */
+int acpi_lid_notifier_register(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&acpi_lid_notifier, nb);
+}
+EXPORT_SYMBOL(acpi_lid_notifier_register);
+
+int acpi_lid_notifier_unregister(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&acpi_lid_notifier, nb);
+}
+EXPORT_SYMBOL(acpi_lid_notifier_unregister);
+
+int acpi_lid_open(void)
+{
+	acpi_status status;
+	unsigned long long state;
+
+	status = acpi_evaluate_integer(lid_device->handle, "_LID", NULL,
+				       &state);
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+
+	return !!state;
+}
+EXPORT_SYMBOL(acpi_lid_open);
+
 static int acpi_lid_send_state(struct acpi_device *device)
 {
 	struct acpi_button *button = acpi_driver_data(device);
 	unsigned long long state;
 	acpi_status status;
+	int ret;
 
 	status = acpi_evaluate_integer(device->handle, "_LID", NULL, &state);
 	if (ACPI_FAILURE(status))
@@ -242,7 +272,12 @@ static int acpi_lid_send_state(struct acpi_device *device)
 	/* input layer checks if event is redundant */
 	input_report_switch(button->input, SW_LID, !state);
 	input_sync(button->input);
-	return 0;
+
+	ret = blocking_notifier_call_chain(&acpi_lid_notifier, state, device);
+	if (ret == NOTIFY_DONE)
+		ret = blocking_notifier_call_chain(&acpi_lid_notifier, state,
+						   device);
+	return ret;
 }
 
 static void acpi_button_notify(struct acpi_device *device, u32 event)
@@ -364,8 +399,14 @@ static int acpi_button_add(struct acpi_device *device)
 	error = input_register_device(input);
 	if (error)
 		goto err_remove_fs;
-	if (button->type == ACPI_BUTTON_TYPE_LID)
+	if (button->type == ACPI_BUTTON_TYPE_LID) {
 		acpi_lid_send_state(device);
+		/*
+		 * This assumes there's only one lid device, or if there are
+		 * more we only care about the last one...
+		 */
+		lid_device = device;
+	}
 
 	if (device->wakeup.flags.valid) {
 		/* Button's GPE is run-wake GPE */
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 3a22eb9..dde56b6 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -71,6 +71,7 @@ config DRM_I915
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
 	select FB
+	select ACPI_BUTTON
 	tristate "i915 driver"
 	help
 	  Choose this option if you have a system that has Intel 830M, 845G,
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2506592..fb12fb9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -190,6 +190,8 @@ typedef struct drm_i915_private {
 	unsigned int lvds_use_ssc:1;
 	int lvds_ssc_freq;
 
+	struct notifier_block lid_notifier;
+
 	struct drm_i915_fence_reg fence_regs[16]; /* assume 965 */
 	int fence_reg_start; /* 4 if userland hasn't ioctl'd us yet */
 	int num_fence_regs; /* 8 on pre-965, 16 otherwise */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bdcda36..08c1260 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -24,6 +24,8 @@
  *	Eric Anholt <eric@anholt.net>
  */
 
+#include <linux/module.h>
+#include <linux/input.h>
 #include <linux/i2c.h>
 #include "drmP.h"
 #include "intel_drv.h"
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 6619f26..8f05b09 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -27,6 +27,7 @@
  *      Jesse Barnes <jesse.barnes@intel.com>
  */
 
+#include <acpi/button.h>
 #include <linux/dmi.h>
 #include <linux/i2c.h>
 #include "drmP.h"
@@ -277,12 +278,18 @@ static void intel_lvds_mode_set(struct drm_encoder *encoder,
 /**
  * Detect the LVDS connection.
  *
- * This always returns CONNECTOR_STATUS_CONNECTED.  This connector should only have
- * been set up if the LVDS was actually connected anyway.
+ * Since LVDS doesn't have hotlug, we use the lid as a proxy.  Open means
+ * connected and closed means disconnected.  We also send hotplug events as
+ * needed, using lid status notification from the input layer.
  */
 static enum drm_connector_status intel_lvds_detect(struct drm_connector *connector)
 {
-	return connector_status_connected;
+	enum drm_connector_status status = connector_status_connected;
+
+	if (!acpi_lid_open())
+		status = connector_status_disconnected;
+
+	return status;
 }
 
 /**
@@ -321,6 +328,17 @@ static int intel_lvds_get_modes(struct drm_connector *connector)
 	return 0;
 }
 
+static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
+			    void *unused)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(nb, struct drm_i915_private, lid_notifier);
+
+	drm_sysfs_hotplug_event(dev_priv->dev);
+
+	return NOTIFY_OK;
+}
+
 /**
  * intel_lvds_destroy - unregister and free LVDS structures
  * @connector: connector to free
@@ -330,10 +348,14 @@ static int intel_lvds_get_modes(struct drm_connector *connector)
  */
 static void intel_lvds_destroy(struct drm_connector *connector)
 {
+	struct drm_device *dev = connector->dev;
 	struct intel_output *intel_output = to_intel_output(connector);
+	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	if (intel_output->ddc_bus)
 		intel_i2c_destroy(intel_output->ddc_bus);
+	if (dev_priv->lid_notifier.notifier_call)
+		acpi_lid_notifier_unregister(&dev_priv->lid_notifier);
 	drm_sysfs_connector_remove(connector);
 	drm_connector_cleanup(connector);
 	kfree(connector);
@@ -508,6 +530,11 @@ void intel_lvds_init(struct drm_device *dev)
 		goto failed;
 
 out:
+	dev_priv->lid_notifier.notifier_call = intel_lid_notify;
+	if (acpi_lid_notifier_register(&dev_priv->lid_notifier)) {
+		DRM_DEBUG("lid notifier registration failed\n");
+		dev_priv->lid_notifier.notifier_call = NULL;
+	}
 	drm_sysfs_connector_add(connector);
 	return;
 
diff --git a/include/acpi/button.h b/include/acpi/button.h
new file mode 100644
index 0000000..bb643a7
--- /dev/null
+++ b/include/acpi/button.h
@@ -0,0 +1,10 @@
+#ifndef ACPI_BUTTON_H
+#define ACPI_BUTTON_H
+
+#include <linux/notifier.h>
+
+extern int acpi_lid_notifier_register(struct notifier_block *nb);
+extern int acpi_lid_notifier_unregister(struct notifier_block *nb);
+extern int acpi_lid_open(void);
+
+#endif /* ACPI_BUTTON_H */

Re: [Intel-gfx] [RFC] i915/acpi: add lid status notification and detection

[Fu Michael]

Jesse Barnes wrote:
> On Thu, 21 May 2009 16:57:53 +0800
> Zhang Rui <rui.zhang@intel.com> wrote:
>
>   
>> On Wed, 2009-05-20 at 01:15 +0800, Matthew Garrett wrote:
>>     
>>>>> There's also a policy question here.  On some machines, a lid
>>>>> close will cause the ACPI firmware to program the GPU,
>>>>> disabling the pipe associated with the panel.  Should we detect
>>>>> this and turn it back on at open time?  That could be dangerous
>>>>> if userspace has received the LVDS hotplug event and changed
>>>>> the config out from under us...
>>>>>
>>>>> Comments?
>>>>>           
>>>> It seems that the LID status is used to determine whether the
>>>> LVDS is connected.
>>>> It is not reliable. On some boxes the initial LID status is
>>>> incorrect. Maybe the LID status is open. But the ACPI returns
>>>> that the LID is close. In such case the LVDS is not initialized
>>>> and user can't get the output.
>>>>         
>>> Really? I haven't seen any cases of this. They'll fail in all sorts
>>> of fun ways with modern userland.
>>>
>>>       
>> This is rare, and if this happens, a bug should be filed against ACPI.
>> BTW: we have fixed/root caused all such kind of bugs that have been
>> reported.
>> So I think it makes sense to trust the Lid state reported by ACPI
>> button driver.
>>     
>
> So is that two acks for the patch?  If so, should it be split or can it
> just go in through the i915 driver tree?
>
> Len?  (Patch attached for reference.)
>
> Thanks,
>   
Jesse, Just talked with Rui,  the above status is based on "BIOS upgrade 
or FW fix is acceptable as a bug fix solution". are you ok with this? :) 
Many lid status has to be fixed via action such as DSDT upgrade...


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

Re: [Intel-gfx] [RFC] i915/acpi: add lid status notification and detection

[Matthew Garrett]

On Fri, May 22, 2009 at 09:22:31AM +0800, Fu Michael wrote:
> Jesse Barnes wrote:
>> So is that two acks for the patch?  If so, should it be split or can it
>> just go in through the i915 driver tree?
>>
>> Len?  (Patch attached for reference.)
>>
>> Thanks,
>>   
> Jesse, Just talked with Rui,  the above status is based on "BIOS upgrade  
> or FW fix is acceptable as a bug fix solution". are you ok with this? :)  
> Many lid status has to be fixed via action such as DSDT upgrade...

Really? As I said, I'd be surprised if this is in any way common. What's 
the exact problem description?

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [Intel-gfx] [RFC] i915/acpi: add lid status notification and detection

[Zhang Rui]

On Fri, 2009-05-22 at 09:26 +0800, Matthew Garrett wrote:
> On Fri, May 22, 2009 at 09:22:31AM +0800, Fu Michael wrote:
> > Jesse Barnes wrote:
> >> So is that two acks for the patch?  If so, should it be split or can it
> >> just go in through the i915 driver tree?
> >>
> >> Len?  (Patch attached for reference.)
> >>
> >> Thanks,
> >>   
> > Jesse, Just talked with Rui,  the above status is based on "BIOS upgrade  
> > or FW fix is acceptable as a bug fix solution". are you ok with this? :)  
> > Many lid status has to be fixed via action such as DSDT upgrade...
> 
> Really? As I said, I'd be surprised if this is in any way common. What's 
> the exact problem description?
> 
for example,
http://bugzilla.kernel.org/show_bug.cgi?id=13263
http://bugzilla.kernel.org/show_bug.cgi?id=5809
http://bugzilla.kernel.org/show_bug.cgi?id=5904
and 
https://bugs.launchpad.net/ubuntu/+bug/34389

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

Re: [Intel-gfx] [RFC] i915/acpi: add lid status notification and detection

[Jesse Barnes]

On Fri, 22 May 2009 09:22:31 +0800
Fu Michael <michael_fu@linux.intel.com> wrote:

> Jesse Barnes wrote:
> > On Thu, 21 May 2009 16:57:53 +0800
> > Zhang Rui <rui.zhang@intel.com> wrote:
> >
> >   
> >> On Wed, 2009-05-20 at 01:15 +0800, Matthew Garrett wrote:
> >>     
> >>>>> There's also a policy question here.  On some machines, a lid
> >>>>> close will cause the ACPI firmware to program the GPU,
> >>>>> disabling the pipe associated with the panel.  Should we detect
> >>>>> this and turn it back on at open time?  That could be dangerous
> >>>>> if userspace has received the LVDS hotplug event and changed
> >>>>> the config out from under us...
> >>>>>
> >>>>> Comments?
> >>>>>           
> >>>> It seems that the LID status is used to determine whether the
> >>>> LVDS is connected.
> >>>> It is not reliable. On some boxes the initial LID status is
> >>>> incorrect. Maybe the LID status is open. But the ACPI returns
> >>>> that the LID is close. In such case the LVDS is not initialized
> >>>> and user can't get the output.
> >>>>         
> >>> Really? I haven't seen any cases of this. They'll fail in all
> >>> sorts of fun ways with modern userland.
> >>>
> >>>       
> >> This is rare, and if this happens, a bug should be filed against
> >> ACPI. BTW: we have fixed/root caused all such kind of bugs that
> >> have been reported.
> >> So I think it makes sense to trust the Lid state reported by ACPI
> >> button driver.
> >>     
> >
> > So is that two acks for the patch?  If so, should it be split or
> > can it just go in through the i915 driver tree?
> >
> > Len?  (Patch attached for reference.)
> >
> > Thanks,
> >   
> Jesse, Just talked with Rui,  the above status is based on "BIOS
> upgrade or FW fix is acceptable as a bug fix solution". are you ok
> with this? :) Many lid status has to be fixed via action such as DSDT
> upgrade...

Yeah, I think that's ok, even if we need quirks for some platforms.  I
really hate relying on BIOS vendors/OEMs to provide BIOS updates in
general:  if Windows works on a given platform, why should Linux
require a BIOS "fix" on it?  In this case though, we can work around
broken platforms by just returning "open" all the time, if it comes to
that.

Jesse

Re: [Intel-gfx] [RFC] i915/acpi: add lid status notification and detection

[Fu Michael]

Jesse Barnes wrote:
> On Fri, 22 May 2009 09:22:31 +0800
> Fu Michael <michael_fu@linux.intel.com> wrote:
>
>   
>> Jesse Barnes wrote:
>>     
>>> On Thu, 21 May 2009 16:57:53 +0800
>>> Zhang Rui <rui.zhang@intel.com> wrote:
>>>
>>>   
>>>       
>>>> On Wed, 2009-05-20 at 01:15 +0800, Matthew Garrett wrote:
>>>>     
>>>>         
>>>>>>> There's also a policy question here.  On some machines, a lid
>>>>>>> close will cause the ACPI firmware to program the GPU,
>>>>>>> disabling the pipe associated with the panel.  Should we detect
>>>>>>> this and turn it back on at open time?  That could be dangerous
>>>>>>> if userspace has received the LVDS hotplug event and changed
>>>>>>> the config out from under us...
>>>>>>>
>>>>>>> Comments?
>>>>>>>           
>>>>>>>               
>>>>>> It seems that the LID status is used to determine whether the
>>>>>> LVDS is connected.
>>>>>> It is not reliable. On some boxes the initial LID status is
>>>>>> incorrect. Maybe the LID status is open. But the ACPI returns
>>>>>> that the LID is close. In such case the LVDS is not initialized
>>>>>> and user can't get the output.
>>>>>>         
>>>>>>             
>>>>> Really? I haven't seen any cases of this. They'll fail in all
>>>>> sorts of fun ways with modern userland.
>>>>>
>>>>>       
>>>>>           
>>>> This is rare, and if this happens, a bug should be filed against
>>>> ACPI. BTW: we have fixed/root caused all such kind of bugs that
>>>> have been reported.
>>>> So I think it makes sense to trust the Lid state reported by ACPI
>>>> button driver.
>>>>     
>>>>         
>>> So is that two acks for the patch?  If so, should it be split or
>>> can it just go in through the i915 driver tree?
>>>
>>> Len?  (Patch attached for reference.)
>>>
>>> Thanks,
>>>   
>>>       
>> Jesse, Just talked with Rui,  the above status is based on "BIOS
>> upgrade or FW fix is acceptable as a bug fix solution". are you ok
>> with this? :) Many lid status has to be fixed via action such as DSDT
>> upgrade...
>>     
>
> Yeah, I think that's ok, even if we need quirks for some platforms.  I
> really hate relying on BIOS vendors/OEMs to provide BIOS updates in
> general:  if Windows works on a given platform, why should Linux
> require a BIOS "fix" on it?  In this case though, we can work around
> broken platforms by just returning "open" all the time, if it comes to
> that.
>
>   
I'm not sure if acpi module has this kind of quirk for lid detection, if 
we prepare to take this way, we'd better to quirk from our driver 
directly...
> Jesse
>
>   

Re: [Intel-gfx] [RFC] i915/acpi: add lid status notification and detection

[yakui_zhao]

On Wed, 2009-05-27 at 16:58 +0800, Jesse Barnes wrote:
> > >   
> > Jesse, Just talked with Rui,  the above status is based on "BIOS
> > upgrade or FW fix is acceptable as a bug fix solution". are you ok
> > with this? :) Many lid status has to be fixed via action such as DSDT
> > upgrade...
> 
> Yeah, I think that's ok, even if we need quirks for some platforms.  I
> really hate relying on BIOS vendors/OEMs to provide BIOS updates in
> general:  if Windows works on a given platform, why should Linux
> require a BIOS "fix" on it?  In this case though, we can work around
> broken platforms by just returning "open" all the time, if it comes to
> that.
Hi, 
    It is a good point that the LID status is used to decide whether the
LVDS is connected or not.
    As Rui said in the previous thread, sometimes the initial status of
LID is incorrect on some laptops. If we expect that LVDS can be
initialized correctly on such boxes, we will have to add the quirk so
that the LID status is not used for LVDS detection.
   But maybe on such boxes the LID initial status is correct after BIOS
upgrading or using custom DSDT. Do we need to delete the quirk for such
box?  It is difficult to manage.
    
   So IMO we had better not use the LID status to determine whether the
LVDS is connected or not.
   Thanks.
> 
> Jesse
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [RFC] i915/acpi: add lid status notification and detection

[Jesse Barnes]

On Thu, 11 Jun 2009 15:16:27 +0800
yakui_zhao <yakui.zhao@intel.com> wrote:

> On Wed, 2009-05-27 at 16:58 +0800, Jesse Barnes wrote:
> > > >   
> > > Jesse, Just talked with Rui,  the above status is based on "BIOS
> > > upgrade or FW fix is acceptable as a bug fix solution". are you ok
> > > with this? :) Many lid status has to be fixed via action such as
> > > DSDT upgrade...
> > 
> > Yeah, I think that's ok, even if we need quirks for some
> > platforms.  I really hate relying on BIOS vendors/OEMs to provide
> > BIOS updates in general:  if Windows works on a given platform, why
> > should Linux require a BIOS "fix" on it?  In this case though, we
> > can work around broken platforms by just returning "open" all the
> > time, if it comes to that.
> Hi, 
>     It is a good point that the LID status is used to decide whether
> the LVDS is connected or not.
>     As Rui said in the previous thread, sometimes the initial status
> of LID is incorrect on some laptops. If we expect that LVDS can be
> initialized correctly on such boxes, we will have to add the quirk so
> that the LID status is not used for LVDS detection.
>    But maybe on such boxes the LID initial status is correct after
> BIOS upgrading or using custom DSDT. Do we need to delete the quirk
> for such box?  It is difficult to manage.
>     
>    So IMO we had better not use the LID status to determine whether
> the LVDS is connected or not.

Well, what should we use then?  Think of a common use case: you plug in
an external monitor and shut your lid.  Do we want to make the user
manually change their configuration?  Or detect that the lid is no
longer in use?  And what about the case where they boot with the lid
closed (e.g. in a docked scenario)?  We want to support that
automatically too...

If we can solve these issues some other way, that's fine, but right now
we have nothing; I think my patch is an improvement over that, even if
it won't work everywhere w/o quirks.

Len or Matthew, any comments?

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [Intel-gfx] [RFC] i915/acpi: add lid status notification and detection

[Corentin Chary]

On Tue, Jun 16, 2009 at 8:33 PM, Jesse Barnes<jbarnes@virtuousgeek.org> wrote:
> On Thu, 11 Jun 2009 15:16:27 +0800
> yakui_zhao <yakui.zhao@intel.com> wrote:
>
>> On Wed, 2009-05-27 at 16:58 +0800, Jesse Barnes wrote:
>> > > >
>> > > Jesse, Just talked with Rui,  the above status is based on "BIOS
>> > > upgrade or FW fix is acceptable as a bug fix solution". are you ok
>> > > with this? :) Many lid status has to be fixed via action such as
>> > > DSDT upgrade...
>> >
>> > Yeah, I think that's ok, even if we need quirks for some
>> > platforms.  I really hate relying on BIOS vendors/OEMs to provide
>> > BIOS updates in general:  if Windows works on a given platform, why
>> > should Linux require a BIOS "fix" on it?  In this case though, we
>> > can work around broken platforms by just returning "open" all the
>> > time, if it comes to that.
>> Hi,
>>     It is a good point that the LID status is used to decide whether
>> the LVDS is connected or not.
>>     As Rui said in the previous thread, sometimes the initial status
>> of LID is incorrect on some laptops. If we expect that LVDS can be
>> initialized correctly on such boxes, we will have to add the quirk so
>> that the LID status is not used for LVDS detection.
>>    But maybe on such boxes the LID initial status is correct after
>> BIOS upgrading or using custom DSDT. Do we need to delete the quirk
>> for such box?  It is difficult to manage.
>>
>>    So IMO we had better not use the LID status to determine whether
>> the LVDS is connected or not.
>
> Well, what should we use then?  Think of a common use case: you plug in
> an external monitor and shut your lid.  Do we want to make the user
> manually change their configuration?  Or detect that the lid is no
> longer in use?  And what about the case where they boot with the lid
> closed (e.g. in a docked scenario)?  We want to support that
> automatically too...

Just another use case for this patch :
Michael Ringe recently sended me a patch to handle backlight power in
eeepc-laptop.

> There is another minor problem I could not solve. When the lid is closed, the
> status of the backlight device driver is not updated, so reading from
> /sys/class/backlight/eeepc/bl_power still yields 0 (=power on). Or, if you
> switch off the backlight and then close and reopen the lid, the backlight is on,
> but the driver still thinks it's off. If you reboot in this status, the notebook
> will come up with the backlight switched off, and you have to press Fn-F7 to
> switch it on.

> To solve this problem, eeepc-laptop would need to register a notification handler
> with \_SB.LIB. But this is not possible because a handler is already registered by
> the acpi button driver. Maybe there is a way too register a handler with the button driver?

We ended up with an input handler, checking on SW_LID, but it's not
very clean, and
 I'd better use your patch.

-- 
Corentin Chary
http://xf.iksaif.net - http://uffs.org
--
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

Re: [Intel-gfx] [RFC] i915/acpi: add lid status notification and detection

[yakui_zhao]

On Wed, 2009-06-17 at 02:33 +0800, Jesse Barnes wrote:
> Well, what should we use then?  Think of a common use case: you plug
> in
> an external monitor and shut your lid.  Do we want to make the user
> manually change their configuration?  Or detect that the lid is no
> longer in use?  And what about the case where they boot with the lid
> closed (e.g. in a docked scenario)?  We want to support that
> automatically too...
This feature should work on most laptops. When the LID is closed, the
LVDS is marked as disconnected.

But this can't work for some boxes on which the initial Lid state is
incorrect. We see such an exception on several laptops in ACPI bugzilla.

If we expect this feature for most laptops, how about adding the
exception boxes into the blacklist that doesn't support this feature?

Thanks.
> 
> If we can solve these issues some other way, that's fine, but right
> now
> we have nothing; I think my patch is an improvement over that, even if
> it won't work everywhere w/o quirks.
> 
> Len or Matthew, any comments

Re: [Intel-gfx] [RFC] i915/acpi: add lid status notification and detection

[Jesse Barnes]

On Wed, 17 Jun 2009 10:32:31 +0800
yakui_zhao <yakui.zhao@intel.com> wrote:

> On Wed, 2009-06-17 at 02:33 +0800, Jesse Barnes wrote:
> > Well, what should we use then?  Think of a common use case: you plug
> > in
> > an external monitor and shut your lid.  Do we want to make the user
> > manually change their configuration?  Or detect that the lid is no
> > longer in use?  And what about the case where they boot with the lid
> > closed (e.g. in a docked scenario)?  We want to support that
> > automatically too...
> This feature should work on most laptops. When the LID is closed, the
> LVDS is marked as disconnected.
> 
> But this can't work for some boxes on which the initial Lid state is
> incorrect. We see such an exception on several laptops in ACPI
> bugzilla.
> 
> If we expect this feature for most laptops, how about adding the
> exception boxes into the blacklist that doesn't support this feature?

Yeah, that's fine with me.  Is there a list somewhere?  Please don't
make me troll through ACPI bugzilla! :)

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [Intel-gfx] [RFC] i915/acpi: add lid status notification and detection

[Thomas Renninger]

On Tuesday 16 June 2009 20:33:20 Jesse Barnes wrote:
> On Thu, 11 Jun 2009 15:16:27 +0800
> yakui_zhao <yakui.zhao@intel.com> wrote:
> 
> > On Wed, 2009-05-27 at 16:58 +0800, Jesse Barnes wrote:
> > > > >   
> > > > Jesse, Just talked with Rui,  the above status is based on "BIOS
> > > > upgrade or FW fix is acceptable as a bug fix solution". are you ok
> > > > with this? :) Many lid status has to be fixed via action such as
> > > > DSDT upgrade...
> > > 
> > > Yeah, I think that's ok, even if we need quirks for some
> > > platforms.  I really hate relying on BIOS vendors/OEMs to provide
> > > BIOS updates in general:  if Windows works on a given platform, why
> > > should Linux require a BIOS "fix" on it?  In this case though, we
> > > can work around broken platforms by just returning "open" all the
> > > time, if it comes to that.
> > Hi, 
> >     It is a good point that the LID status is used to decide whether
> > the LVDS is connected or not.
> >     As Rui said in the previous thread, sometimes the initial status
> > of LID is incorrect on some laptops. If we expect that LVDS can be
> > initialized correctly on such boxes, we will have to add the quirk so
> > that the LID status is not used for LVDS detection.
> >    But maybe on such boxes the LID initial status is correct after
> > BIOS upgrading or using custom DSDT. Do we need to delete the quirk
> > for such box?  It is difficult to manage.
> >     
> >    So IMO we had better not use the LID status to determine whether
> > the LVDS is connected or not.
> 
> Well, what should we use then?  Think of a common use case: you plug in
> an external monitor and shut your lid.  Do we want to make the user
> manually change their configuration?  Or detect that the lid is no
> longer in use?  And what about the case where they boot with the lid
> closed (e.g. in a docked scenario)?  We want to support that
> automatically too...
> 
> If we can solve these issues some other way, that's fine, but right now
> we have nothing; I think my patch is an improvement over that, even if
> it won't work everywhere w/o quirks.

Not sure whether it matters here, but Acer (One?) netbooks seem to only
throw a lid close and not a lid open event.
Suspend/resume still seem to work, something seem to wake the
machine up, but a Lid open ACPI notification event is not triggered.

   Thomas

Re: [Intel-gfx] [RFC] i915/acpi: add lid status notification and detection

[Jesse Barnes]

On Wed, 17 Jun 2009 16:10:39 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> On Wed, 17 Jun 2009 10:32:31 +0800
> yakui_zhao <yakui.zhao@intel.com> wrote:
> 
> > On Wed, 2009-06-17 at 02:33 +0800, Jesse Barnes wrote:
> > > Well, what should we use then?  Think of a common use case: you
> > > plug in
> > > an external monitor and shut your lid.  Do we want to make the
> > > user manually change their configuration?  Or detect that the lid
> > > is no longer in use?  And what about the case where they boot
> > > with the lid closed (e.g. in a docked scenario)?  We want to
> > > support that automatically too...
> > This feature should work on most laptops. When the LID is closed,
> > the LVDS is marked as disconnected.
> > 
> > But this can't work for some boxes on which the initial Lid state is
> > incorrect. We see such an exception on several laptops in ACPI
> > bugzilla.
> > 
> > If we expect this feature for most laptops, how about adding the
> > exception boxes into the blacklist that doesn't support this
> > feature?
> 
> Yeah, that's fine with me.  Is there a list somewhere?  Please don't
> make me troll through ACPI bugzilla! :)

Here's an update of this patch, that restores the last configuration at
lid open time.  That's needed on at least some machines since the
firmware will disable planes at lid close time to save power.

-- 
Jesse Barnes, Intel Open Source Technology Center

diff --git a/drivers/acpi/button.c b/drivers/acpi/button.c
index 9195deb..ebb593e 100644
--- a/drivers/acpi/button.c
+++ b/drivers/acpi/button.c
@@ -113,6 +113,9 @@ static const struct file_operations acpi_button_state_fops = {
 	.release = single_release,
 };
 
+static BLOCKING_NOTIFIER_HEAD(acpi_lid_notifier);
+static struct acpi_device *lid_device;
+
 /* --------------------------------------------------------------------------
                               FS Interface (/proc)
    -------------------------------------------------------------------------- */
@@ -229,11 +232,38 @@ static int acpi_button_remove_fs(struct acpi_device *device)
 /* --------------------------------------------------------------------------
                                 Driver Interface
    -------------------------------------------------------------------------- */
+int acpi_lid_notifier_register(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_register(&acpi_lid_notifier, nb);
+}
+EXPORT_SYMBOL(acpi_lid_notifier_register);
+
+int acpi_lid_notifier_unregister(struct notifier_block *nb)
+{
+	return blocking_notifier_chain_unregister(&acpi_lid_notifier, nb);
+}
+EXPORT_SYMBOL(acpi_lid_notifier_unregister);
+
+int acpi_lid_open(void)
+{
+	acpi_status status;
+	unsigned long long state;
+
+	status = acpi_evaluate_integer(lid_device->handle, "_LID", NULL,
+				       &state);
+	if (ACPI_FAILURE(status))
+		return -ENODEV;
+
+	return !!state;
+}
+EXPORT_SYMBOL(acpi_lid_open);
+
 static int acpi_lid_send_state(struct acpi_device *device)
 {
 	struct acpi_button *button = acpi_driver_data(device);
 	unsigned long long state;
 	acpi_status status;
+	int ret;
 
 	status = acpi_evaluate_integer(device->handle, "_LID", NULL, &state);
 	if (ACPI_FAILURE(status))
@@ -242,7 +272,12 @@ static int acpi_lid_send_state(struct acpi_device *device)
 	/* input layer checks if event is redundant */
 	input_report_switch(button->input, SW_LID, !state);
 	input_sync(button->input);
-	return 0;
+
+	ret = blocking_notifier_call_chain(&acpi_lid_notifier, state, device);
+	if (ret == NOTIFY_DONE)
+		ret = blocking_notifier_call_chain(&acpi_lid_notifier, state,
+						   device);
+	return ret;
 }
 
 static void acpi_button_notify(struct acpi_device *device, u32 event)
@@ -364,8 +399,14 @@ static int acpi_button_add(struct acpi_device *device)
 	error = input_register_device(input);
 	if (error)
 		goto err_remove_fs;
-	if (button->type == ACPI_BUTTON_TYPE_LID)
+	if (button->type == ACPI_BUTTON_TYPE_LID) {
 		acpi_lid_send_state(device);
+		/*
+		 * This assumes there's only one lid device, or if there are
+		 * more we only care about the last one...
+		 */
+		lid_device = device;
+	}
 
 	if (device->wakeup.flags.valid) {
 		/* Button's GPE is run-wake GPE */
diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index 39b393d..5873865 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -86,6 +86,7 @@ config DRM_I915
 	select FB_CFB_COPYAREA
 	select FB_CFB_IMAGEBLIT
 	select FB
+	select ACPI_BUTTON
 	select FRAMEBUFFER_CONSOLE if !EMBEDDED
 	# i915 depends on ACPI_VIDEO when ACPI is enabled
 	# but for select to work, need to select ACPI_VIDEO's dependencies, ick
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 47ecb61..166b5ae 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -221,6 +221,8 @@ typedef struct drm_i915_private {
 	unsigned int lvds_use_ssc:1;
 	int lvds_ssc_freq;
 
+	struct notifier_block lid_notifier;
+
 	struct drm_i915_fence_reg fence_regs[16]; /* assume 965 */
 	int fence_reg_start; /* 4 if userland hasn't ioctl'd us yet */
 	int num_fence_regs; /* 8 on pre-965, 16 otherwise */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 508838e..d08f286 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -24,6 +24,8 @@
  *	Eric Anholt <eric@anholt.net>
  */
 
+#include <linux/module.h>
+#include <linux/input.h>
 #include <linux/i2c.h>
 #include <linux/kernel.h>
 #include "drmP.h"
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index f65044b..9020e74 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -27,6 +27,7 @@
  *      Jesse Barnes <jesse.barnes@intel.com>
  */
 
+#include <acpi/button.h>
 #include <linux/dmi.h>
 #include <linux/i2c.h>
 #include "drmP.h"
@@ -597,12 +598,18 @@ static void intel_lvds_mode_set(struct drm_encoder *encoder,
 /**
  * Detect the LVDS connection.
  *
- * This always returns CONNECTOR_STATUS_CONNECTED.  This connector should only have
- * been set up if the LVDS was actually connected anyway.
+ * Since LVDS doesn't have hotlug, we use the lid as a proxy.  Open means
+ * connected and closed means disconnected.  We also send hotplug events as
+ * needed, using lid status notification from the input layer.
  */
 static enum drm_connector_status intel_lvds_detect(struct drm_connector *connector)
 {
-	return connector_status_connected;
+	enum drm_connector_status status = connector_status_connected;
+
+	if (!acpi_lid_open())
+		status = connector_status_disconnected;
+
+	return status;
 }
 
 /**
@@ -641,6 +648,20 @@ static int intel_lvds_get_modes(struct drm_connector *connector)
 	return 0;
 }
 
+static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
+			    void *unused)
+{
+	struct drm_i915_private *dev_priv =
+		container_of(nb, struct drm_i915_private, lid_notifier);
+	struct drm_device *dev = dev_priv->dev;
+
+	if (acpi_lid_open())
+		drm_helper_resume_force_mode(dev);
+	drm_sysfs_hotplug_event(dev_priv->dev);
+
+	return NOTIFY_OK;
+}
+
 /**
  * intel_lvds_destroy - unregister and free LVDS structures
  * @connector: connector to free
@@ -650,10 +671,14 @@ static int intel_lvds_get_modes(struct drm_connector *connector)
  */
 static void intel_lvds_destroy(struct drm_connector *connector)
 {
+	struct drm_device *dev = connector->dev;
 	struct intel_output *intel_output = to_intel_output(connector);
+	struct drm_i915_private *dev_priv = dev->dev_private;
 
 	if (intel_output->ddc_bus)
 		intel_i2c_destroy(intel_output->ddc_bus);
+	if (dev_priv->lid_notifier.notifier_call)
+		acpi_lid_notifier_unregister(&dev_priv->lid_notifier);
 	drm_sysfs_connector_remove(connector);
 	drm_connector_cleanup(connector);
 	kfree(connector);
@@ -939,6 +964,11 @@ out:
 		pwm |= PWM_PCH_ENABLE;
 		I915_WRITE(BLC_PWM_PCH_CTL1, pwm);
 	}
+ 	dev_priv->lid_notifier.notifier_call = intel_lid_notify;
+ 	if (acpi_lid_notifier_register(&dev_priv->lid_notifier)) {
+ 		DRM_DEBUG("lid notifier registration failed\n");
+ 		dev_priv->lid_notifier.notifier_call = NULL;
+ 	}
 	drm_sysfs_connector_add(connector);
 	return;