[PATCH] acpi: Populate DIDL before registering ACPI video device on Intel

[PATCH] acpi: Populate DIDL before registering ACPI video device on Intel

[Matthew Garrett]

Intel graphics hardware that implements the ACPI IGD OpRegion spec 
requires that the list of display devices be populated before any ACPI 
video methods are called. Detect when this is the case and defer 
registration until the opregion code calls it. Fixes crashes on HP 
laptops as seen in kernel bugzilla #11259.
    
Signed-off-by: Matthew Garrett <mjg@redhat.com>

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index bb5ed05..64e987c 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -37,6 +37,8 @@
 #include <linux/thermal.h>
 #include <linux/video_output.h>
 #include <linux/sort.h>
+#include <linux/pci.h>
+#include <linux/pci_ids.h>
 #include <asm/uaccess.h>
 
 #include <acpi/acpi_bus.h>
@@ -2124,7 +2126,27 @@ static int acpi_video_bus_remove(struct acpi_device *device, int type)
 	return 0;
 }
 
-static int __init acpi_video_init(void)
+static int __init intel_opregion_present(void)
+{
+#if defined(CONFIG_DRM_I915) || defined(CONFIG_DRM_I915_MODULE)
+	struct pci_dev *dev = NULL;
+	u32 address;
+
+	for_each_pci_dev(dev) {
+		if ((dev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
+			continue;
+		if (dev->vendor != PCI_VENDOR_ID_INTEL)
+			continue;
+		pci_read_config_dword(dev, 0xfc, &address);
+		if (!address)
+			continue;
+		return 1;
+	}
+#endif
+	return 0;
+}
+
+int acpi_video_register(void)
 {
 	int result = 0;
 
@@ -2141,6 +2163,22 @@ static int __init acpi_video_init(void)
 
 	return 0;
 }
+EXPORT_SYMBOL(acpi_video_register);
+
+/*
+ * This is kind of nasty. Hardware using Intel chipsets may require
+ * the video opregion code to be run first in order to initialise
+ * state before any ACPI video calls are made. To handle this we defer
+ * registration of the video class until the opregion code has run.
+ */
+
+static int __init acpi_video_init(void)
+{
+	if (intel_opregion_present())
+		return 0;
+
+	return acpi_video_register();
+}
 
 static void __exit acpi_video_exit(void)
 {
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 6dab63b..70a9904 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1144,8 +1144,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	if (!IS_I945G(dev) && !IS_I945GM(dev))
 		pci_enable_msi(dev->pdev);
 
-	intel_opregion_init(dev);
-
 	spin_lock_init(&dev_priv->user_irq_lock);
 	dev_priv->user_irq_refcount = 0;
 
@@ -1164,6 +1162,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		}
 	}
 
+	/* Must be done after probing outputs */
+	intel_opregion_init(dev);
+
 	return 0;
 
 out_iomapfree:
diff --git a/drivers/gpu/drm/i915/i915_opregion.c b/drivers/gpu/drm/i915/i915_opregion.c
index ff01283..60b746f 100644
--- a/drivers/gpu/drm/i915/i915_opregion.c
+++ b/drivers/gpu/drm/i915/i915_opregion.c
@@ -26,6 +26,7 @@
  */
 
 #include <linux/acpi.h>
+#include <acpi/video.h>
 
 #include "drmP.h"
 #include "i915_drm.h"
@@ -136,6 +137,12 @@ struct opregion_asle {
 
 #define ASLE_CBLV_VALID         (1<<31)
 
+#define ACPI_OTHER_OUTPUT (0<<8)
+#define ACPI_VGA_OUTPUT (1<<8)
+#define ACPI_TV_OUTPUT (2<<8)
+#define ACPI_DIGITAL_OUTPUT (3<<8)
+#define ACPI_LVDS_OUTPUT (4<<8)
+
 static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -282,6 +289,57 @@ static struct notifier_block intel_opregion_notifier = {
 	.notifier_call = intel_opregion_video_event,
 };
 
+/*
+ * Initialise the DIDL field in opregion. This passes a list of devices to
+ * the firmware. Values are defined by section B.4.2 of the ACPI specification
+ * (version 3)
+ */
+
+static void intel_didl_outputs(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_opregion *opregion = &dev_priv->opregion;
+	struct drm_connector *connector;
+	int i = 0;
+
+	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
+		int output_type = ACPI_OTHER_OUTPUT;
+		if (i >= 8) {
+			dev_printk (KERN_ERR, &dev->pdev->dev,
+				    "More than 8 outputs detected\n");
+			return;
+		}
+		switch (connector->connector_type) {
+		case DRM_MODE_CONNECTOR_VGA:
+		case DRM_MODE_CONNECTOR_DVIA:
+			output_type = ACPI_VGA_OUTPUT;
+			break;
+		case DRM_MODE_CONNECTOR_Composite:
+		case DRM_MODE_CONNECTOR_SVIDEO:
+		case DRM_MODE_CONNECTOR_Component:
+		case DRM_MODE_CONNECTOR_9PinDIN:
+			output_type = ACPI_TV_OUTPUT;
+			break;
+		case DRM_MODE_CONNECTOR_DVII:
+		case DRM_MODE_CONNECTOR_DVID:
+		case DRM_MODE_CONNECTOR_DisplayPort:
+		case DRM_MODE_CONNECTOR_HDMIA:
+		case DRM_MODE_CONNECTOR_HDMIB:
+			output_type = ACPI_DIGITAL_OUTPUT;
+			break;
+		case DRM_MODE_CONNECTOR_LVDS:
+			output_type = ACPI_LVDS_OUTPUT;
+			break;
+		}
+		opregion->acpi->didl[i] |= (1<<31) | output_type | i;
+		i++;
+	}
+
+	/* If fewer than 8 outputs, the list must be null terminated */
+	if (i < 8)
+		opregion->acpi->didl[i] = 0;
+}
+
 int intel_opregion_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -312,6 +370,10 @@ int intel_opregion_init(struct drm_device *dev)
 	if (mboxes & MBOX_ACPI) {
 		DRM_DEBUG("Public ACPI methods supported\n");
 		opregion->acpi = base + OPREGION_ACPI_OFFSET;
+		if (drm_core_check_feature(dev, DRIVER_MODESET)) {
+			intel_didl_outputs(dev);
+			acpi_video_register();
+		}
 	} else {
 		DRM_DEBUG("Public ACPI methods not supported\n");
 		err = -ENOTSUPP;
diff --git a/include/acpi/video.h b/include/acpi/video.h
new file mode 100644
index 0000000..f0275bb
--- /dev/null
+++ b/include/acpi/video.h
@@ -0,0 +1,11 @@
+#ifndef __ACPI_VIDEO_H
+#define __ACPI_VIDEO_H
+
+#if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE)
+extern int acpi_video_register(void);
+#else
+static inline int acpi_video_register(void) { return 0; }
+#endif
+
+#endif
+


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

Re: [PATCH] acpi: Populate DIDL before registering ACPI video device on Intel

[yakui_zhao]

On Thu, 2009-03-12 at 05:55 +0800, Matthew Garrett wrote:
cc: intel-gfx@lists.freedesktop.org

     On some HP boxes the acpi video driver depends on the ACPI IGD
Opregion. If the ACPI IGD opregion is not initialized, the _DOD object
will return the incorrect supported devices. In such case when LID is
closed, the SMI will be called and crash will happen. (The DIDL field of
ACPI IGD opregion is filled with the supported devices, which is
enumerated by graphics driver).
If the ACPI IGD opregion is initialized correctly, the ACPI video driver
can be loaded correctly and there is no crash.

IMO the idea in this patch is very reasonable. 

The ACPI video driver will be deferred if the ACPI IGD opregion is
detected. Only when ACPI IGD opregion is initialized, the acpi video
driver will be bound with the ACPI video device. 
    The supported device list is enumerated by graphics driver, which
depends on the KMS.

Hi, Len/Jesse
    Any comments about this patch?
   
     
> Intel graphics hardware that implements the ACPI IGD OpRegion spec 
> requires that the list of display devices be populated before any ACPI 
> video methods are called. Detect when this is the case and defer 
> registration until the opregion code calls it. Fixes crashes on HP 
> laptops as seen in kernel bugzilla #11259.
>     
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> 
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index bb5ed05..64e987c 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -37,6 +37,8 @@
>  #include <linux/thermal.h>
>  #include <linux/video_output.h>
>  #include <linux/sort.h>
> +#include <linux/pci.h>
> +#include <linux/pci_ids.h>
>  #include <asm/uaccess.h>
>  
>  #include <acpi/acpi_bus.h>
> @@ -2124,7 +2126,27 @@ static int acpi_video_bus_remove(struct acpi_device *device, int type)
>  	return 0;
>  }
>  
> -static int __init acpi_video_init(void)
> +static int __init intel_opregion_present(void)
> +{
> +#if defined(CONFIG_DRM_I915) || defined(CONFIG_DRM_I915_MODULE)
> +	struct pci_dev *dev = NULL;
> +	u32 address;
> +
> +	for_each_pci_dev(dev) {
> +		if ((dev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
> +			continue;
> +		if (dev->vendor != PCI_VENDOR_ID_INTEL)
> +			continue;
> +		pci_read_config_dword(dev, 0xfc, &address);
> +		if (!address)
> +			continue;
> +		return 1;
> +	}
> +#endif
> +	return 0;
> +}
> +
> +int acpi_video_register(void)
>  {
>  	int result = 0;
>  
> @@ -2141,6 +2163,22 @@ static int __init acpi_video_init(void)
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL(acpi_video_register);
> +
> +/*
> + * This is kind of nasty. Hardware using Intel chipsets may require
> + * the video opregion code to be run first in order to initialise
> + * state before any ACPI video calls are made. To handle this we defer
> + * registration of the video class until the opregion code has run.
> + */
> +
> +static int __init acpi_video_init(void)
> +{
> +	if (intel_opregion_present())
> +		return 0;
> +
> +	return acpi_video_register();
> +}
>  
>  static void __exit acpi_video_exit(void)
>  {
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 6dab63b..70a9904 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1144,8 +1144,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	if (!IS_I945G(dev) && !IS_I945GM(dev))
>  		pci_enable_msi(dev->pdev);
>  
> -	intel_opregion_init(dev);
> -
>  	spin_lock_init(&dev_priv->user_irq_lock);
>  	dev_priv->user_irq_refcount = 0;
>  
> @@ -1164,6 +1162,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  		}
>  	}
>  
> +	/* Must be done after probing outputs */
> +	intel_opregion_init(dev);
> +
>  	return 0;
>  
>  out_iomapfree:
> diff --git a/drivers/gpu/drm/i915/i915_opregion.c b/drivers/gpu/drm/i915/i915_opregion.c
> index ff01283..60b746f 100644
> --- a/drivers/gpu/drm/i915/i915_opregion.c
> +++ b/drivers/gpu/drm/i915/i915_opregion.c
> @@ -26,6 +26,7 @@
>   */
>  
>  #include <linux/acpi.h>
> +#include <acpi/video.h>
>  
>  #include "drmP.h"
>  #include "i915_drm.h"
> @@ -136,6 +137,12 @@ struct opregion_asle {
>  
>  #define ASLE_CBLV_VALID         (1<<31)
>  
> +#define ACPI_OTHER_OUTPUT (0<<8)
> +#define ACPI_VGA_OUTPUT (1<<8)
> +#define ACPI_TV_OUTPUT (2<<8)
> +#define ACPI_DIGITAL_OUTPUT (3<<8)
> +#define ACPI_LVDS_OUTPUT (4<<8)
> +
>  static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -282,6 +289,57 @@ static struct notifier_block intel_opregion_notifier = {
>  	.notifier_call = intel_opregion_video_event,
>  };
>  
> +/*
> + * Initialise the DIDL field in opregion. This passes a list of devices to
> + * the firmware. Values are defined by section B.4.2 of the ACPI specification
> + * (version 3)
> + */
> +
> +static void intel_didl_outputs(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_opregion *opregion = &dev_priv->opregion;
> +	struct drm_connector *connector;
> +	int i = 0;
> +
> +	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> +		int output_type = ACPI_OTHER_OUTPUT;
> +		if (i >= 8) {
> +			dev_printk (KERN_ERR, &dev->pdev->dev,
> +				    "More than 8 outputs detected\n");
> +			return;
> +		}
> +		switch (connector->connector_type) {
> +		case DRM_MODE_CONNECTOR_VGA:
> +		case DRM_MODE_CONNECTOR_DVIA:
> +			output_type = ACPI_VGA_OUTPUT;
> +			break;
> +		case DRM_MODE_CONNECTOR_Composite:
> +		case DRM_MODE_CONNECTOR_SVIDEO:
> +		case DRM_MODE_CONNECTOR_Component:
> +		case DRM_MODE_CONNECTOR_9PinDIN:
> +			output_type = ACPI_TV_OUTPUT;
> +			break;
> +		case DRM_MODE_CONNECTOR_DVII:
> +		case DRM_MODE_CONNECTOR_DVID:
> +		case DRM_MODE_CONNECTOR_DisplayPort:
> +		case DRM_MODE_CONNECTOR_HDMIA:
> +		case DRM_MODE_CONNECTOR_HDMIB:
> +			output_type = ACPI_DIGITAL_OUTPUT;
> +			break;
> +		case DRM_MODE_CONNECTOR_LVDS:
> +			output_type = ACPI_LVDS_OUTPUT;
> +			break;
> +		}
> +		opregion->acpi->didl[i] |= (1<<31) | output_type | i;
> +		i++;
> +	}
> +
> +	/* If fewer than 8 outputs, the list must be null terminated */
> +	if (i < 8)
> +		opregion->acpi->didl[i] = 0;
> +}
> +
>  int intel_opregion_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -312,6 +370,10 @@ int intel_opregion_init(struct drm_device *dev)
>  	if (mboxes & MBOX_ACPI) {
>  		DRM_DEBUG("Public ACPI methods supported\n");
>  		opregion->acpi = base + OPREGION_ACPI_OFFSET;
> +		if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> +			intel_didl_outputs(dev);
> +			acpi_video_register();
> +		}
>  	} else {
>  		DRM_DEBUG("Public ACPI methods not supported\n");
>  		err = -ENOTSUPP;
> diff --git a/include/acpi/video.h b/include/acpi/video.h
> new file mode 100644
> index 0000000..f0275bb
> --- /dev/null
> +++ b/include/acpi/video.h
> @@ -0,0 +1,11 @@
> +#ifndef __ACPI_VIDEO_H
> +#define __ACPI_VIDEO_H
> +
> +#if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE)
> +extern int acpi_video_register(void);
> +#else
> +static inline int acpi_video_register(void) { return 0; }
> +#endif
> +
> +#endif
> +
> 
> 

Re: [PATCH] acpi: Populate DIDL before registering ACPI video device on Intel

[yakui_zhao]

On Thu, 2009-03-12 at 05:55 +0800, Matthew Garrett wrote:


     On some HP boxes the acpi video driver depends on the ACPI IGD
Opregion. If the ACPI IGD opregion is not initialized, the _DOD object
will return the incorrect supported devices. In such case when LID is
closed, the SMI will be called and crash will happen. The DIDL field of
ACPI IGD opregion is filled with the supported devices, which is
enumerated by graphics driver(Of course this also depends on the KMS).
If the ACPI IGD opregion is initialized correctly, the ACPI video driver
can be loaded correctly and there is no crash.

IMO the idea in this patch is very reasonable. 

The ACPI video driver will be deferred if the ACPI IGD opregion is
detected. Only when ACPI IGD opregion is initialized, the acpi video
driver will be bound with the ACPI video device. 
    The supported device list is enumerated by graphics driver, which
depends on the KMS.

Hi, Len/Jesse
    Any comments about this patch?
    Can the idea in this patch be accepted? 

Thanks.
   
     
> Intel graphics hardware that implements the ACPI IGD OpRegion spec 
> requires that the list of display devices be populated before any ACPI 
> video methods are called. Detect when this is the case and defer 
> registration until the opregion code calls it. Fixes crashes on HP 
> laptops as seen in kernel bugzilla #11259.
>     
> Signed-off-by: Matthew Garrett <mjg@redhat.com>
> 
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index bb5ed05..64e987c 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -37,6 +37,8 @@
>  #include <linux/thermal.h>
>  #include <linux/video_output.h>
>  #include <linux/sort.h>
> +#include <linux/pci.h>
> +#include <linux/pci_ids.h>
>  #include <asm/uaccess.h>
>  
>  #include <acpi/acpi_bus.h>
> @@ -2124,7 +2126,27 @@ static int acpi_video_bus_remove(struct acpi_device *device, int type)
>  	return 0;
>  }
>  
> -static int __init acpi_video_init(void)
> +static int __init intel_opregion_present(void)
> +{
> +#if defined(CONFIG_DRM_I915) || defined(CONFIG_DRM_I915_MODULE)
> +	struct pci_dev *dev = NULL;
> +	u32 address;
> +
> +	for_each_pci_dev(dev) {
> +		if ((dev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
> +			continue;
> +		if (dev->vendor != PCI_VENDOR_ID_INTEL)
> +			continue;
> +		pci_read_config_dword(dev, 0xfc, &address);
> +		if (!address)
> +			continue;
> +		return 1;
> +	}
> +#endif
> +	return 0;
> +}
> +
> +int acpi_video_register(void)
>  {
>  	int result = 0;
>  
> @@ -2141,6 +2163,22 @@ static int __init acpi_video_init(void)
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL(acpi_video_register);
> +
> +/*
> + * This is kind of nasty. Hardware using Intel chipsets may require
> + * the video opregion code to be run first in order to initialise
> + * state before any ACPI video calls are made. To handle this we defer
> + * registration of the video class until the opregion code has run.
> + */
> +
> +static int __init acpi_video_init(void)
> +{
> +	if (intel_opregion_present())
> +		return 0;
> +
> +	return acpi_video_register();
> +}
>  
>  static void __exit acpi_video_exit(void)
>  {
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 6dab63b..70a9904 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1144,8 +1144,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	if (!IS_I945G(dev) && !IS_I945GM(dev))
>  		pci_enable_msi(dev->pdev);
>  
> -	intel_opregion_init(dev);
> -
>  	spin_lock_init(&dev_priv->user_irq_lock);
>  	dev_priv->user_irq_refcount = 0;
>  
> @@ -1164,6 +1162,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  		}
>  	}
>  
> +	/* Must be done after probing outputs */
> +	intel_opregion_init(dev);
> +
>  	return 0;
>  
>  out_iomapfree:
> diff --git a/drivers/gpu/drm/i915/i915_opregion.c b/drivers/gpu/drm/i915/i915_opregion.c
> index ff01283..60b746f 100644
> --- a/drivers/gpu/drm/i915/i915_opregion.c
> +++ b/drivers/gpu/drm/i915/i915_opregion.c
> @@ -26,6 +26,7 @@
>   */
>  
>  #include <linux/acpi.h>
> +#include <acpi/video.h>
>  
>  #include "drmP.h"
>  #include "i915_drm.h"
> @@ -136,6 +137,12 @@ struct opregion_asle {
>  
>  #define ASLE_CBLV_VALID         (1<<31)
>  
> +#define ACPI_OTHER_OUTPUT (0<<8)
> +#define ACPI_VGA_OUTPUT (1<<8)
> +#define ACPI_TV_OUTPUT (2<<8)
> +#define ACPI_DIGITAL_OUTPUT (3<<8)
> +#define ACPI_LVDS_OUTPUT (4<<8)
> +
>  static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -282,6 +289,57 @@ static struct notifier_block intel_opregion_notifier = {
>  	.notifier_call = intel_opregion_video_event,
>  };
>  
> +/*
> + * Initialise the DIDL field in opregion. This passes a list of devices to
> + * the firmware. Values are defined by section B.4.2 of the ACPI specification
> + * (version 3)
> + */
> +
> +static void intel_didl_outputs(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_opregion *opregion = &dev_priv->opregion;
> +	struct drm_connector *connector;
> +	int i = 0;
> +
> +	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> +		int output_type = ACPI_OTHER_OUTPUT;
> +		if (i >= 8) {
> +			dev_printk (KERN_ERR, &dev->pdev->dev,
> +				    "More than 8 outputs detected\n");
> +			return;
> +		}
> +		switch (connector->connector_type) {
> +		case DRM_MODE_CONNECTOR_VGA:
> +		case DRM_MODE_CONNECTOR_DVIA:
> +			output_type = ACPI_VGA_OUTPUT;
> +			break;
> +		case DRM_MODE_CONNECTOR_Composite:
> +		case DRM_MODE_CONNECTOR_SVIDEO:
> +		case DRM_MODE_CONNECTOR_Component:
> +		case DRM_MODE_CONNECTOR_9PinDIN:
> +			output_type = ACPI_TV_OUTPUT;
> +			break;
> +		case DRM_MODE_CONNECTOR_DVII:
> +		case DRM_MODE_CONNECTOR_DVID:
> +		case DRM_MODE_CONNECTOR_DisplayPort:
> +		case DRM_MODE_CONNECTOR_HDMIA:
> +		case DRM_MODE_CONNECTOR_HDMIB:
> +			output_type = ACPI_DIGITAL_OUTPUT;
> +			break;
> +		case DRM_MODE_CONNECTOR_LVDS:
> +			output_type = ACPI_LVDS_OUTPUT;
> +			break;
> +		}
> +		opregion->acpi->didl[i] |= (1<<31) | output_type | i;
> +		i++;
> +	}
> +
> +	/* If fewer than 8 outputs, the list must be null terminated */
> +	if (i < 8)
> +		opregion->acpi->didl[i] = 0;
> +}
> +
>  int intel_opregion_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -312,6 +370,10 @@ int intel_opregion_init(struct drm_device *dev)
>  	if (mboxes & MBOX_ACPI) {
>  		DRM_DEBUG("Public ACPI methods supported\n");
>  		opregion->acpi = base + OPREGION_ACPI_OFFSET;
> +		if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> +			intel_didl_outputs(dev);
> +			acpi_video_register();
> +		}
>  	} else {
>  		DRM_DEBUG("Public ACPI methods not supported\n");
>  		err = -ENOTSUPP;
> diff --git a/include/acpi/video.h b/include/acpi/video.h
> new file mode 100644
> index 0000000..f0275bb
> --- /dev/null
> +++ b/include/acpi/video.h
> @@ -0,0 +1,11 @@
> +#ifndef __ACPI_VIDEO_H
> +#define __ACPI_VIDEO_H
> +
> +#if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE)
> +extern int acpi_video_register(void);
> +#else
> +static inline int acpi_video_register(void) { return 0; }
> +#endif
> +
> +#endif
> +
> 
> 

[PATCH v2] acpi: Populate DIDL before registering ACPI video device on Intel

[Matthew Garrett]

[ACPI] Populate DIDL before registering ACPI video device on Intel

Intel graphics hardware that implements the ACPI IGD OpRegion spec 
requires that the list of display devices be populated before any ACPI 
video methods are called. Detect when this is the case and defer 
registration until the opregion code calls it. Fixes crashes on HP 
laptops as seen in kernel bugzilla #11259.
    
Signed-off-by: Matthew Garrett <mjg@redhat.com>

---

This version fixes an attempted re-registration of the video device on 
resume.

diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
index bb5ed05..64e987c 100644
--- a/drivers/acpi/video.c
+++ b/drivers/acpi/video.c
@@ -37,6 +37,8 @@
 #include <linux/thermal.h>
 #include <linux/video_output.h>
 #include <linux/sort.h>
+#include <linux/pci.h>
+#include <linux/pci_ids.h>
 #include <asm/uaccess.h>
 
 #include <acpi/acpi_bus.h>
@@ -2124,7 +2126,27 @@ static int acpi_video_bus_remove(struct acpi_device *device, int type)
 	return 0;
 }
 
-static int __init acpi_video_init(void)
+static int __init intel_opregion_present(void)
+{
+#if defined(CONFIG_DRM_I915) || defined(CONFIG_DRM_I915_MODULE)
+	struct pci_dev *dev = NULL;
+	u32 address;
+
+	for_each_pci_dev(dev) {
+		if ((dev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
+			continue;
+		if (dev->vendor != PCI_VENDOR_ID_INTEL)
+			continue;
+		pci_read_config_dword(dev, 0xfc, &address);
+		if (!address)
+			continue;
+		return 1;
+	}
+#endif
+	return 0;
+}
+
+int acpi_video_register(void)
 {
 	int result = 0;
 
@@ -2141,6 +2163,22 @@ static int __init acpi_video_init(void)
 
 	return 0;
 }
+EXPORT_SYMBOL(acpi_video_register);
+
+/*
+ * This is kind of nasty. Hardware using Intel chipsets may require
+ * the video opregion code to be run first in order to initialise
+ * state before any ACPI video calls are made. To handle this we defer
+ * registration of the video class until the opregion code has run.
+ */
+
+static int __init acpi_video_init(void)
+{
+	if (intel_opregion_present())
+		return 0;
+
+	return acpi_video_register();
+}
 
 static void __exit acpi_video_exit(void)
 {
diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 6dab63b..5881b6a 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1144,8 +1144,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 	if (!IS_I945G(dev) && !IS_I945GM(dev))
 		pci_enable_msi(dev->pdev);
 
-	intel_opregion_init(dev);
-
 	spin_lock_init(&dev_priv->user_irq_lock);
 	dev_priv->user_irq_refcount = 0;
 
@@ -1164,6 +1162,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
 		}
 	}
 
+	/* Must be done after probing outputs */
+	intel_opregion_init(dev, 0);
+
 	return 0;
 
 out_iomapfree:
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index b293ef0..209592f 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -99,7 +99,7 @@ static int i915_resume(struct drm_device *dev)
 
 	i915_restore_state(dev);
 
-	intel_opregion_init(dev);
+	intel_opregion_init(dev, 1);
 
 	/* KMS EnterVT equivalent */
 	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 17fa408..aee6f9e 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -654,7 +654,7 @@ extern int i915_restore_state(struct drm_device *dev);
 
 #ifdef CONFIG_ACPI
 /* i915_opregion.c */
-extern int intel_opregion_init(struct drm_device *dev);
+extern int intel_opregion_init(struct drm_device *dev, int resume);
 extern void intel_opregion_free(struct drm_device *dev);
 extern void opregion_asle_intr(struct drm_device *dev);
 extern void opregion_enable_asle(struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/i915_opregion.c b/drivers/gpu/drm/i915/i915_opregion.c
index ff01283..6942772 100644
--- a/drivers/gpu/drm/i915/i915_opregion.c
+++ b/drivers/gpu/drm/i915/i915_opregion.c
@@ -26,6 +26,7 @@
  */
 
 #include <linux/acpi.h>
+#include <acpi/video.h>
 
 #include "drmP.h"
 #include "i915_drm.h"
@@ -136,6 +137,12 @@ struct opregion_asle {
 
 #define ASLE_CBLV_VALID         (1<<31)
 
+#define ACPI_OTHER_OUTPUT (0<<8)
+#define ACPI_VGA_OUTPUT (1<<8)
+#define ACPI_TV_OUTPUT (2<<8)
+#define ACPI_DIGITAL_OUTPUT (3<<8)
+#define ACPI_LVDS_OUTPUT (4<<8)
+
 static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -282,7 +289,58 @@ static struct notifier_block intel_opregion_notifier = {
 	.notifier_call = intel_opregion_video_event,
 };
 
-int intel_opregion_init(struct drm_device *dev)
+/*
+ * Initialise the DIDL field in opregion. This passes a list of devices to
+ * the firmware. Values are defined by section B.4.2 of the ACPI specification
+ * (version 3)
+ */
+
+static void intel_didl_outputs(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_opregion *opregion = &dev_priv->opregion;
+	struct drm_connector *connector;
+	int i = 0;
+
+	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
+		int output_type = ACPI_OTHER_OUTPUT;
+		if (i >= 8) {
+			dev_printk (KERN_ERR, &dev->pdev->dev,
+				    "More than 8 outputs detected\n");
+			return;
+		}
+		switch (connector->connector_type) {
+		case DRM_MODE_CONNECTOR_VGA:
+		case DRM_MODE_CONNECTOR_DVIA:
+			output_type = ACPI_VGA_OUTPUT;
+			break;
+		case DRM_MODE_CONNECTOR_Composite:
+		case DRM_MODE_CONNECTOR_SVIDEO:
+		case DRM_MODE_CONNECTOR_Component:
+		case DRM_MODE_CONNECTOR_9PinDIN:
+			output_type = ACPI_TV_OUTPUT;
+			break;
+		case DRM_MODE_CONNECTOR_DVII:
+		case DRM_MODE_CONNECTOR_DVID:
+		case DRM_MODE_CONNECTOR_DisplayPort:
+		case DRM_MODE_CONNECTOR_HDMIA:
+		case DRM_MODE_CONNECTOR_HDMIB:
+			output_type = ACPI_DIGITAL_OUTPUT;
+			break;
+		case DRM_MODE_CONNECTOR_LVDS:
+			output_type = ACPI_LVDS_OUTPUT;
+			break;
+		}
+		opregion->acpi->didl[i] |= (1<<31) | output_type | i;
+		i++;
+	}
+
+	/* If fewer than 8 outputs, the list must be null terminated */
+	if (i < 8)
+		opregion->acpi->didl[i] = 0;
+}
+
+int intel_opregion_init(struct drm_device *dev, int resume)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_opregion *opregion = &dev_priv->opregion;
@@ -312,6 +370,11 @@ int intel_opregion_init(struct drm_device *dev)
 	if (mboxes & MBOX_ACPI) {
 		DRM_DEBUG("Public ACPI methods supported\n");
 		opregion->acpi = base + OPREGION_ACPI_OFFSET;
+		if (drm_core_check_feature(dev, DRIVER_MODESET)) {
+			intel_didl_outputs(dev);
+			if (!resume)
+				acpi_video_register();
+		}
 	} else {
 		DRM_DEBUG("Public ACPI methods not supported\n");
 		err = -ENOTSUPP;
diff --git a/include/acpi/video.h b/include/acpi/video.h
new file mode 100644
index 0000000..f0275bb
--- /dev/null
+++ b/include/acpi/video.h
@@ -0,0 +1,11 @@
+#ifndef __ACPI_VIDEO_H
+#define __ACPI_VIDEO_H
+
+#if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE)
+extern int acpi_video_register(void);
+#else
+static inline int acpi_video_register(void) { return 0; }
+#endif
+
+#endif
+

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

Re: [PATCH v2] acpi: Populate DIDL before registering ACPI video device on Intel

[Eric Anholt]

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

On Thu, 2009-03-19 at 21:35 +0000, Matthew Garrett wrote:
> [ACPI] Populate DIDL before registering ACPI video device on Intel
> 
> Intel graphics hardware that implements the ACPI IGD OpRegion spec 
> requires that the list of display devices be populated before any ACPI 
> video methods are called. Detect when this is the case and defer 
> registration until the opregion code calls it. Fixes crashes on HP 
> laptops as seen in kernel bugzilla #11259.
>     
> Signed-off-by: Matthew Garrett <mjg@redhat.com>

As far as DRM changes,

Acked-by: Eric Anholt <eric@anholt.net>

(I'm assuming this'll go through the ACPI tree)


> ---
> 
> This version fixes an attempted re-registration of the video device on 
> resume.
> 
> diff --git a/drivers/acpi/video.c b/drivers/acpi/video.c
> index bb5ed05..64e987c 100644
> --- a/drivers/acpi/video.c
> +++ b/drivers/acpi/video.c
> @@ -37,6 +37,8 @@
>  #include <linux/thermal.h>
>  #include <linux/video_output.h>
>  #include <linux/sort.h>
> +#include <linux/pci.h>
> +#include <linux/pci_ids.h>
>  #include <asm/uaccess.h>
>  
>  #include <acpi/acpi_bus.h>
> @@ -2124,7 +2126,27 @@ static int acpi_video_bus_remove(struct acpi_device *device, int type)
>  	return 0;
>  }
>  
> -static int __init acpi_video_init(void)
> +static int __init intel_opregion_present(void)
> +{
> +#if defined(CONFIG_DRM_I915) || defined(CONFIG_DRM_I915_MODULE)
> +	struct pci_dev *dev = NULL;
> +	u32 address;
> +
> +	for_each_pci_dev(dev) {
> +		if ((dev->class >> 8) != PCI_CLASS_DISPLAY_VGA)
> +			continue;
> +		if (dev->vendor != PCI_VENDOR_ID_INTEL)
> +			continue;
> +		pci_read_config_dword(dev, 0xfc, &address);
> +		if (!address)
> +			continue;
> +		return 1;
> +	}
> +#endif
> +	return 0;
> +}
> +
> +int acpi_video_register(void)
>  {
>  	int result = 0;
>  
> @@ -2141,6 +2163,22 @@ static int __init acpi_video_init(void)
>  
>  	return 0;
>  }
> +EXPORT_SYMBOL(acpi_video_register);
> +
> +/*
> + * This is kind of nasty. Hardware using Intel chipsets may require
> + * the video opregion code to be run first in order to initialise
> + * state before any ACPI video calls are made. To handle this we defer
> + * registration of the video class until the opregion code has run.
> + */
> +
> +static int __init acpi_video_init(void)
> +{
> +	if (intel_opregion_present())
> +		return 0;
> +
> +	return acpi_video_register();
> +}
>  
>  static void __exit acpi_video_exit(void)
>  {
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index 6dab63b..5881b6a 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1144,8 +1144,6 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  	if (!IS_I945G(dev) && !IS_I945GM(dev))
>  		pci_enable_msi(dev->pdev);
>  
> -	intel_opregion_init(dev);
> -
>  	spin_lock_init(&dev_priv->user_irq_lock);
>  	dev_priv->user_irq_refcount = 0;
>  
> @@ -1164,6 +1162,9 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
>  		}
>  	}
>  
> +	/* Must be done after probing outputs */
> +	intel_opregion_init(dev, 0);
> +
>  	return 0;
>  
>  out_iomapfree:
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index b293ef0..209592f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -99,7 +99,7 @@ static int i915_resume(struct drm_device *dev)
>  
>  	i915_restore_state(dev);
>  
> -	intel_opregion_init(dev);
> +	intel_opregion_init(dev, 1);
>  
>  	/* KMS EnterVT equivalent */
>  	if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 17fa408..aee6f9e 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -654,7 +654,7 @@ extern int i915_restore_state(struct drm_device *dev);
>  
>  #ifdef CONFIG_ACPI
>  /* i915_opregion.c */
> -extern int intel_opregion_init(struct drm_device *dev);
> +extern int intel_opregion_init(struct drm_device *dev, int resume);
>  extern void intel_opregion_free(struct drm_device *dev);
>  extern void opregion_asle_intr(struct drm_device *dev);
>  extern void opregion_enable_asle(struct drm_device *dev);
> diff --git a/drivers/gpu/drm/i915/i915_opregion.c b/drivers/gpu/drm/i915/i915_opregion.c
> index ff01283..6942772 100644
> --- a/drivers/gpu/drm/i915/i915_opregion.c
> +++ b/drivers/gpu/drm/i915/i915_opregion.c
> @@ -26,6 +26,7 @@
>   */
>  
>  #include <linux/acpi.h>
> +#include <acpi/video.h>
>  
>  #include "drmP.h"
>  #include "i915_drm.h"
> @@ -136,6 +137,12 @@ struct opregion_asle {
>  
>  #define ASLE_CBLV_VALID         (1<<31)
>  
> +#define ACPI_OTHER_OUTPUT (0<<8)
> +#define ACPI_VGA_OUTPUT (1<<8)
> +#define ACPI_TV_OUTPUT (2<<8)
> +#define ACPI_DIGITAL_OUTPUT (3<<8)
> +#define ACPI_LVDS_OUTPUT (4<<8)
> +
>  static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
> @@ -282,7 +289,58 @@ static struct notifier_block intel_opregion_notifier = {
>  	.notifier_call = intel_opregion_video_event,
>  };
>  
> -int intel_opregion_init(struct drm_device *dev)
> +/*
> + * Initialise the DIDL field in opregion. This passes a list of devices to
> + * the firmware. Values are defined by section B.4.2 of the ACPI specification
> + * (version 3)
> + */
> +
> +static void intel_didl_outputs(struct drm_device *dev)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_opregion *opregion = &dev_priv->opregion;
> +	struct drm_connector *connector;
> +	int i = 0;
> +
> +	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> +		int output_type = ACPI_OTHER_OUTPUT;
> +		if (i >= 8) {
> +			dev_printk (KERN_ERR, &dev->pdev->dev,
> +				    "More than 8 outputs detected\n");
> +			return;
> +		}
> +		switch (connector->connector_type) {
> +		case DRM_MODE_CONNECTOR_VGA:
> +		case DRM_MODE_CONNECTOR_DVIA:
> +			output_type = ACPI_VGA_OUTPUT;
> +			break;
> +		case DRM_MODE_CONNECTOR_Composite:
> +		case DRM_MODE_CONNECTOR_SVIDEO:
> +		case DRM_MODE_CONNECTOR_Component:
> +		case DRM_MODE_CONNECTOR_9PinDIN:
> +			output_type = ACPI_TV_OUTPUT;
> +			break;
> +		case DRM_MODE_CONNECTOR_DVII:
> +		case DRM_MODE_CONNECTOR_DVID:
> +		case DRM_MODE_CONNECTOR_DisplayPort:
> +		case DRM_MODE_CONNECTOR_HDMIA:
> +		case DRM_MODE_CONNECTOR_HDMIB:
> +			output_type = ACPI_DIGITAL_OUTPUT;
> +			break;
> +		case DRM_MODE_CONNECTOR_LVDS:
> +			output_type = ACPI_LVDS_OUTPUT;
> +			break;
> +		}
> +		opregion->acpi->didl[i] |= (1<<31) | output_type | i;
> +		i++;
> +	}
> +
> +	/* If fewer than 8 outputs, the list must be null terminated */
> +	if (i < 8)
> +		opregion->acpi->didl[i] = 0;
> +}
> +
> +int intel_opregion_init(struct drm_device *dev, int resume)
>  {
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct intel_opregion *opregion = &dev_priv->opregion;
> @@ -312,6 +370,11 @@ int intel_opregion_init(struct drm_device *dev)
>  	if (mboxes & MBOX_ACPI) {
>  		DRM_DEBUG("Public ACPI methods supported\n");
>  		opregion->acpi = base + OPREGION_ACPI_OFFSET;
> +		if (drm_core_check_feature(dev, DRIVER_MODESET)) {
> +			intel_didl_outputs(dev);
> +			if (!resume)
> +				acpi_video_register();
> +		}
>  	} else {
>  		DRM_DEBUG("Public ACPI methods not supported\n");
>  		err = -ENOTSUPP;
> diff --git a/include/acpi/video.h b/include/acpi/video.h
> new file mode 100644
> index 0000000..f0275bb
> --- /dev/null
> +++ b/include/acpi/video.h
> @@ -0,0 +1,11 @@
> +#ifndef __ACPI_VIDEO_H
> +#define __ACPI_VIDEO_H
> +
> +#if (defined CONFIG_ACPI_VIDEO || defined CONFIG_ACPI_VIDEO_MODULE)
> +extern int acpi_video_register(void);
> +#else
> +static inline int acpi_video_register(void) { return 0; }
> +#endif
> +
> +#endif
> +
> 
-- 
Eric Anholt
eric@anholt.net                         eric.anholt@intel.com



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH v2] acpi: Populate DIDL before registering ACPI video device on Intel

[Len Brown]

applied to acpi-test

thanks,
Len Brown, Intel Open Source Technology Center

Re: [PATCH v2] acpi: Populate DIDL before registering ACPI video device on Intel

[Len Brown]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1130 bytes --]

From: Len Brown <len.brown@intel.com>
Subject: [PATCH] ACPI: fix CONFIG_ACPI=n build

drivers/gpu/drm/i915/i915_drv.c:102: error: too many arguments to
function ‘intel_opregion_init’

Signed-off-by: Len Brown <len.brown@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 1679a95..b9a92c2 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -664,7 +664,7 @@ extern void intel_opregion_free(struct drm_device *dev);
 extern void opregion_asle_intr(struct drm_device *dev);
 extern void opregion_enable_asle(struct drm_device *dev);
 #else
-static inline int intel_opregion_init(struct drm_device *dev) { return 0; }
+static inline int intel_opregion_init(struct drm_device *dev, int resume) { return 0; }
 static inline void intel_opregion_free(struct drm_device *dev) { return; }
 static inline void opregion_asle_intr(struct drm_device *dev) { return; }
 static inline void opregion_enable_asle(struct drm_device *dev) { return; }
-- 
1.6.2.1.389.geed1