| Message ID | 1247695886-18432-3-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
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>
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; > -- 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
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,
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, -- 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
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.
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;
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(-)