drm/i915: LVDS lid patches

drm/i915: LVDS lid patches

[Jesse Barnes]

After some discussion with Matthew this morning, I decided to split this
patchset up a bit.  I'd appreciate acks/reviews on stuff that's ok so we
can get this upstream (the first two fix a real bug), and discussion on
the rest if there are issues.

This work was initially motivated by fdo bug #21230 (first to patches
cover that) but also adds some features that have been missing from our
LVDS support (second two patches).

Thanks,
Jesse

[PATCH 1/4] ACPI button: provide lid status functions

[Jesse Barnes]

Some drivers need to know when a lid event occurs and get the current
status.  This can be useful for when a platform firmware clobbers some
hardware state at lid time, and a driver needs to restore things when
the lid is opened again.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/acpi/button.c |   45 +++++++++++++++++++++++++++++++++++++++++++--
 include/acpi/button.h |   10 ++++++++++
 2 files changed, 53 insertions(+), 2 deletions(-)
 create mode 100644 include/acpi/button.h

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/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 */
-- 
1.6.2.5

Re: [PATCH 1/4] ACPI button: provide lid status functions

[Matthew Garrett]

On Wed, Jul 15, 2009 at 03:11:23PM -0700, Jesse Barnes wrote:
> Some drivers need to know when a lid event occurs and get the current
> status.  This can be useful for when a platform firmware clobbers some
> hardware state at lid time, and a driver needs to restore things when
> the lid is opened again.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Acked-by: Matthew Garrett <mjg@redhat.com>

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

Re: [PATCH 1/4] ACPI button: provide lid status functions

[Jesse Barnes]

On Wed, 15 Jul 2009 23:48:55 +0100
Matthew Garrett <mjg59@srcf.ucam.org> wrote:

> On Wed, Jul 15, 2009 at 03:11:23PM -0700, Jesse Barnes wrote:
> > Some drivers need to know when a lid event occurs and get the
> > current status.  This can be useful for when a platform firmware
> > clobbers some hardware state at lid time, and a driver needs to
> > restore things when the lid is opened again.
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> Acked-by: Matthew Garrett <mjg@redhat.com>

Len, is it ok if this ACPI button patch goes through the drm-intel
tree?  We have other patches that depend on it, so that would be
easiest...

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

Re: [Intel-gfx] [PATCH 1/4] ACPI button: provide lid status functions

[ykzhao]

On Thu, 2009-07-16 at 06:11 +0800, Jesse Barnes wrote:
> Some drivers need to know when a lid event occurs and get the current
> status.  This can be useful for when a platform firmware clobbers some
> hardware state at lid time, and a driver needs to restore things when
> the lid is opened again.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/acpi/button.c |   45 +++++++++++++++++++++++++++++++++++++++++++--
>  include/acpi/button.h |   10 ++++++++++
>  2 files changed, 53 insertions(+), 2 deletions(-)
>  create mode 100644 include/acpi/button.h
> 
> 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;
If the box is booted with ACPI disabled, the variable of lid_device
won't be initialized correctly. In such case when we call the function
of acpi_lid_open, we will access the uninitialized variable. This is
incorrect.
At the same time we should assume that the LID status is open.
> +
>  /* --------------------------------------------------------------------------
>                                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/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>
Maybe sometimes the CONFIG_ACPI is disabled in kernel configuration. 
So it will be better that we define another function set when
CONFIG_ACPI is disabled in kernel configuration.

Thanks.
   Yakui
> +
> +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 */

[PATCH 2/4] drm/i915: force mode set at lid open time

[Jesse Barnes]

Some laptop platforms will disable pipes and/or planes at lid close time
and not restore them when the lid is opened again.  So catch the lid
event, and if the lid was opened, force a mode restore.

Fixes fdo bug #21230.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/Kconfig              |    1 +
 drivers/gpu/drm/i915/i915_drv.h      |    2 ++
 drivers/gpu/drm/i915/intel_display.c |    2 ++
 drivers/gpu/drm/i915/intel_lvds.c    |   23 +++++++++++++++++++++++
 4 files changed, 28 insertions(+), 0 deletions(-)

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 b05b44d..11d0e28 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 3fa0d63..6b0c799 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 43c7d9a..1d0d30a 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"
@@ -642,6 +643,19 @@ 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);
+
+	return NOTIFY_OK;
+}
+
 /**
  * intel_lvds_destroy - unregister and free LVDS structures
  * @connector: connector to free
@@ -651,10 +665,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);
@@ -1017,6 +1035,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;
 
-- 
1.6.2.5

Re: [PATCH 2/4] drm/i915: force mode set at lid open time

[Matthew Garrett]

On Wed, Jul 15, 2009 at 03:11:24PM -0700, Jesse Barnes wrote:
> Some laptop platforms will disable pipes and/or planes at lid close time
> and not restore them when the lid is opened again.  So catch the lid
> event, and if the lid was opened, force a mode restore.
> 
> Fixes fdo bug #21230.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Acked-by: Matthew Garrett <mjg@redhat.com>

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

Re: [Intel-gfx] [PATCH 2/4] drm/i915: force mode set at lid open time

[ykzhao]

On Thu, 2009-07-16 at 06:11 +0800, Jesse Barnes wrote:
> Some laptop platforms will disable pipes and/or planes at lid close time
> and not restore them when the lid is opened again.  So catch the lid
> event, and if the lid was opened, force a mode restore.
Why is it necessary to force a mode restore when the LID is reopened?
Do you mean that the pipes/planes are disabled automatically by BIOS
when the LID is closed? 
If so, maybe it will be better that this feature is limited to some
boxes. It is unnecessary to add this feature for all the laptops.
    
Is it enough to mark the LVDS as disconnected/connected according to the
LID status? And this is done by user-space tool.
Thanks.
> 
> Fixes fdo bug #21230.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/Kconfig              |    1 +
>  drivers/gpu/drm/i915/i915_drv.h      |    2 ++
>  drivers/gpu/drm/i915/intel_display.c |    2 ++
>  drivers/gpu/drm/i915/intel_lvds.c    |   23 +++++++++++++++++++++++
>  4 files changed, 28 insertions(+), 0 deletions(-)
> 
> 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
How about 
  +	  select ACPI_BUTTON if ACPI
>  	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 b05b44d..11d0e28 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 3fa0d63..6b0c799 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 43c7d9a..1d0d30a 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"
> @@ -642,6 +643,19 @@ 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);
> +
> +	return NOTIFY_OK;
> +}
> +
>  /**
>   * intel_lvds_destroy - unregister and free LVDS structures
>   * @connector: connector to free
> @@ -651,10 +665,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);
> @@ -1017,6 +1035,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;
>  

Re: [Intel-gfx] [PATCH 2/4] drm/i915: force mode set at lid open time

[Jesse Barnes]

On Thu, 16 Jul 2009 09:36:13 +0800
ykzhao <yakui.zhao@intel.com> wrote:

> On Thu, 2009-07-16 at 06:11 +0800, Jesse Barnes wrote:
> > Some laptop platforms will disable pipes and/or planes at lid close
> > time and not restore them when the lid is opened again.  So catch
> > the lid event, and if the lid was opened, force a mode restore.
> Why is it necessary to force a mode restore when the LID is reopened?
> Do you mean that the pipes/planes are disabled automatically by BIOS
> when the LID is closed? 

Yes, maybe I need to make that clearer in the changelog.

> If so, maybe it will be better that this feature is limited to some
> boxes. It is unnecessary to add this feature for all the laptops.

The downside of doing that is that we'll always be playing catch up,
adding quirks for machines as problems are discovered, as opposed to
everything "just working" out of the box.  A whitelist might be better,
but really forcing a mode set at open isn't expensive, and with some
care in our mode setting routines, need not flicker at all.

> Is it enough to mark the LVDS as disconnected/connected according to
> the LID status? And this is done by user-space tool.

Not sure what you mean here...

> > 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
> How about 
>   +	  select ACPI_BUTTON if ACPI

Yeah I saw your other comments too about non-ACPI configs.  I have a
hard time caring about them though; so much depends on ACPI these days
that disabling it is just a bad idea.  But I guess it's not too much
trouble to make this work w/o.

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

Re: [Intel-gfx] [PATCH 2/4] drm/i915: force mode set at lid open time

[ykzhao]

On Fri, 2009-07-17 at 00:30 +0800, Jesse Barnes wrote:
> On Thu, 16 Jul 2009 09:36:13 +0800
> ykzhao <yakui.zhao@intel.com> wrote:
> 
> > On Thu, 2009-07-16 at 06:11 +0800, Jesse Barnes wrote:
> > > Some laptop platforms will disable pipes and/or planes at lid close
> > > time and not restore them when the lid is opened again.  So catch
> > > the lid event, and if the lid was opened, force a mode restore.
> > Why is it necessary to force a mode restore when the LID is reopened?
> > Do you mean that the pipes/planes are disabled automatically by BIOS
> > when the LID is closed? 
> 
> Yes, maybe I need to make that clearer in the changelog.
> 
> > If so, maybe it will be better that this feature is limited to some
> > boxes. It is unnecessary to add this feature for all the laptops.
> 
> The downside of doing that is that we'll always be playing catch up,
> adding quirks for machines as problems are discovered, as opposed to
> everything "just working" out of the box.  A whitelist might be better,
> but really forcing a mode set at open isn't expensive, and with some
> care in our mode setting routines, need not flicker at all.
thanks for the detailed explanation.
I understand it.
> 
> > Is it enough to mark the LVDS as disconnected/connected according to
> > the LID status? And this is done by user-space tool.
> 
> Not sure what you mean here...
What I said is that we can send the hotplug event to user space. And
then the user space tool can set the mode for LVDS. 
    Of course the LVDS status is marked as disconnected/connected in
course of LVDS detection.
> 
> > > 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
> > How about 
> >   +	  select ACPI_BUTTON if ACPI
> 
> Yeah I saw your other comments too about non-ACPI configs.  I have a
> hard time caring about them though; so much depends on ACPI these days
> that disabling it is just a bad idea.  But I guess it's not too much
> trouble to make this work w/o.
If someone disable the CONFIG_ACPI in kernel configuration, maybe it
will fail in the kernel compilation.

Sometimes the box will be booted with ACPI disabled. In such case the
LID device is not initialized. When we call the function of
acpi_lid_open, we will access the uninitialized variable. It will be
incorrect.

In fact it will be very easy to add this check.
Thanks.

> 
> Thanks,

Re: [Intel-gfx] [PATCH 2/4] drm/i915: force mode set at lid open time

[Jesse Barnes]

On Fri, 17 Jul 2009 09:34:41 +0800
ykzhao <yakui.zhao@intel.com> wrote:
> > > Is it enough to mark the LVDS as disconnected/connected according
> > > to the LID status? And this is done by user-space tool.
> > 
> > Not sure what you mean here...
> What I said is that we can send the hotplug event to user space. And
> then the user space tool can set the mode for LVDS. 
>     Of course the LVDS status is marked as disconnected/connected in
> course of LVDS detection.

Ah yeah, I see what you mean.  But that won't work in the case where a
display manager isn't running; we want the console to come back too.

> > > > 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
> > > How about 
> > >   +	  select ACPI_BUTTON if ACPI
> > 
> > Yeah I saw your other comments too about non-ACPI configs.  I have a
> > hard time caring about them though; so much depends on ACPI these
> > days that disabling it is just a bad idea.  But I guess it's not
> > too much trouble to make this work w/o.
> If someone disable the CONFIG_ACPI in kernel configuration, maybe it
> will fail in the kernel compilation.
> 
> Sometimes the box will be booted with ACPI disabled. In such case the
> LID device is not initialized. When we call the function of
> acpi_lid_open, we will access the uninitialized variable. It will be
> incorrect.
> 
> In fact it will be very easy to add this check.

Yep, I'll fix it.

-- 
Jesse Barnes, Intel Open Source Technology Center

[PATCH 3/4] drm/i915: use ACPI LID status for LVDS ->detect hook

[Jesse Barnes]

We can't load or hotplug detect LVDS like we can other outputs, but if
there's a lid device present we can use it as a proxy.  This allows the
LFP state to be determined at ->detect time, making configurations
requiring manual intervention today "just work" assuming the lid device
status is correct.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_lvds.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 1d0d30a..57c86fd 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -599,12 +599,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;
 }
 
 /**
-- 
1.6.2.5

Re: [PATCH 3/4] drm/i915: use ACPI LID status for LVDS ->detect hook

[Matthew Garrett]

On Wed, Jul 15, 2009 at 03:11:25PM -0700, Jesse Barnes wrote:
> We can't load or hotplug detect LVDS like we can other outputs, but if
> there's a lid device present we can use it as a proxy.  This allows the
> LFP state to be determined at ->detect time, making configurations
> requiring manual intervention today "just work" assuming the lid device
> status is correct.

I'm a bit unhappy with this appearing to be generic functionality but 
ending up implemented in the driver rather than in the KMS core. If this 
is going in then can we at least document this behaviour and that other 
KMS drivers are expected to implement it?

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

Re: [PATCH 3/4] drm/i915: use ACPI LID status for LVDS ->detect hook

[Jesse Barnes]

On Wed, 15 Jul 2009 23:55:58 +0100
Matthew Garrett <mjg59@srcf.ucam.org> wrote:

> On Wed, Jul 15, 2009 at 03:11:25PM -0700, Jesse Barnes wrote:
> > We can't load or hotplug detect LVDS like we can other outputs, but
> > if there's a lid device present we can use it as a proxy.  This
> > allows the LFP state to be determined at ->detect time, making
> > configurations requiring manual intervention today "just work"
> > assuming the lid device status is correct.
> 
> I'm a bit unhappy with this appearing to be generic functionality but 
> ending up implemented in the driver rather than in the KMS core. If
> this is going in then can we at least document this behaviour and
> that other KMS drivers are expected to implement it?

Well, we don't really have generic LVDS helpers where we could put
this, but yeah we can document this as preferred for all LVDS outputs.

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [Intel-gfx] [PATCH 3/4] drm/i915: use ACPI LID status for LVDS ->detect hook

[ykzhao]

On Thu, 2009-07-16 at 06:11 +0800, Jesse Barnes wrote:
> We can't load or hotplug detect LVDS like we can other outputs, but if
> there's a lid device present we can use it as a proxy.  This allows the
> LFP state to be determined at ->detect time, making configurations
> requiring manual intervention today "just work" assuming the lid device
> status is correct.
It is ok that the LID status is to decide whether the LVDS is
connected/disconnected.

But on some boxes the initial LID status is incorrect. It reports that
LID is closed although it is open.
    For example: 
    http://bugzilla.kernel.org/show_bug.cgi?id=5809
    http://bugzilla.kernel.org/show_bug.cgi?id=13263
    http://bugzilla.kernel.org/show_bug.cgi?id=5904
    https://bugs.launchpad.net/ubuntu/+bug/34389
Maybe this feature is not supported on the above laptops. That means
that LVDS is always connected regardless of LID status.

At the same time on some boxes there is no ACPI LID event when the LID
is reopened. In such case we can't send the hotplug event to user space
when the LID is reopened. How about disable this feature on such box?
    For example: Aspire One.


I have another issue about the hotplug event.
On some boxes it will continue to report the LID event  when the LID is
closed. In such case we had better not send the hotplug event to user
space. Otherwise the user space will receive too many hotplug event
about LVDS. For example:
     http://bugzilla.kernel.org/show_bug.cgi?id=10485
     http://bugzilla.kernel.org/show_bug.cgi?id=5853

So it will be better that the feature of hotplug/LID status is not
supported on such boxes.

Thanks
    Yakui
    
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_lvds.c |   12 +++++++++---
>  1 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 1d0d30a..57c86fd 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -599,12 +599,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;
>  }
>  
>  /**

Re: [Intel-gfx] [PATCH 3/4] drm/i915: use ACPI LID status for LVDS ->detect hook

[Jesse Barnes]

On Thu, 16 Jul 2009 09:54:44 +0800
ykzhao <yakui.zhao@intel.com> wrote:

> On Thu, 2009-07-16 at 06:11 +0800, Jesse Barnes wrote:
> > We can't load or hotplug detect LVDS like we can other outputs, but
> > if there's a lid device present we can use it as a proxy.  This
> > allows the LFP state to be determined at ->detect time, making
> > configurations requiring manual intervention today "just work"
> > assuming the lid device status is correct.
> It is ok that the LID status is to decide whether the LVDS is
> connected/disconnected.
> 
> But on some boxes the initial LID status is incorrect. It reports that
> LID is closed although it is open.
>     For example: 
>     http://bugzilla.kernel.org/show_bug.cgi?id=5809
>     http://bugzilla.kernel.org/show_bug.cgi?id=13263
>     http://bugzilla.kernel.org/show_bug.cgi?id=5904
>     https://bugs.launchpad.net/ubuntu/+bug/34389
> Maybe this feature is not supported on the above laptops. That means
> that LVDS is always connected regardless of LID status.
> 
> At the same time on some boxes there is no ACPI LID event when the LID
> is reopened. In such case we can't send the hotplug event to user
> space when the LID is reopened. How about disable this feature on
> such box? For example: Aspire One.
> 
> 
> I have another issue about the hotplug event.
> On some boxes it will continue to report the LID event  when the LID
> is closed. In such case we had better not send the hotplug event to
> user space. Otherwise the user space will receive too many hotplug
> event about LVDS. For example:
>      http://bugzilla.kernel.org/show_bug.cgi?id=10485
>      http://bugzilla.kernel.org/show_bug.cgi?id=5853
> 
> So it will be better that the feature of hotplug/LID status is not
> supported on such boxes.

Yeah those are all good points; thanks for the list of bugs.  Any
suggestions on how to do a blacklist?  Is there an ACPI or DMI
identifier we could use?

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [Intel-gfx] [PATCH 3/4] drm/i915: use ACPI LID status for LVDS ->detect hook

[ykzhao]

On Fri, 2009-07-17 at 00:32 +0800, Jesse Barnes wrote:
> On Thu, 16 Jul 2009 09:54:44 +0800
> ykzhao <yakui.zhao@intel.com> wrote:
> 
> > On Thu, 2009-07-16 at 06:11 +0800, Jesse Barnes wrote:
> > > We can't load or hotplug detect LVDS like we can other outputs, but
> > > if there's a lid device present we can use it as a proxy.  This
> > > allows the LFP state to be determined at ->detect time, making
> > > configurations requiring manual intervention today "just work"
> > > assuming the lid device status is correct.
> > It is ok that the LID status is to decide whether the LVDS is
> > connected/disconnected.
> > 
> > But on some boxes the initial LID status is incorrect. It reports that
> > LID is closed although it is open.
> >     For example: 
> >     http://bugzilla.kernel.org/show_bug.cgi?id=5809
> >     http://bugzilla.kernel.org/show_bug.cgi?id=13263
> >     http://bugzilla.kernel.org/show_bug.cgi?id=5904
> >     https://bugs.launchpad.net/ubuntu/+bug/34389
> > Maybe this feature is not supported on the above laptops. That means
> > that LVDS is always connected regardless of LID status.
> > 
> > At the same time on some boxes there is no ACPI LID event when the LID
> > is reopened. In such case we can't send the hotplug event to user
> > space when the LID is reopened. How about disable this feature on
> > such box? For example: Aspire One.
> > 
> > 
> > I have another issue about the hotplug event.
> > On some boxes it will continue to report the LID event  when the LID
> > is closed. In such case we had better not send the hotplug event to
> > user space. Otherwise the user space will receive too many hotplug
> > event about LVDS. For example:
> >      http://bugzilla.kernel.org/show_bug.cgi?id=10485
> >      http://bugzilla.kernel.org/show_bug.cgi?id=5853
> > 
> > So it will be better that the feature of hotplug/LID status is not
> > supported on such boxes.
> 
> Yeah those are all good points; thanks for the list of bugs.  Any
> suggestions on how to do a blacklist?  Is there an ACPI or DMI
> identifier we could use?
How about not register the lid notifier callback function for the boxes
that fall into the backlist?
And the LVDS detection will always return connected when there is no LID
notifier callback.

Sorry that there is no dmidecode for the above laptops. I will ask the
bug reporter to attach them.

Thanks.
   Yakui
> 

[PATCH 4/4] drm/i915: generate a KMS uevent at lid open/close time

[Jesse Barnes]

With all the other lid pieces in place, it's easy to generate a uevent
for the LVDS connector just like we do for other outputs.  Should make
lid open/close fit in with the rest of a userland based output
reconfiguration scheme.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_lvds.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 57c86fd..7cb6633 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -659,6 +659,8 @@ static int intel_lid_notify(struct notifier_block *nb, unsigned long val,
 	if (acpi_lid_open())
 		drm_helper_resume_force_mode(dev);
 
+	drm_sysfs_hotplug_event(dev_priv->dev);
+
 	return NOTIFY_OK;
 }
 
-- 
1.6.2.5