[RFC PATCH 0/5] drm/i915/opregion: proper handling of DIDL and CADL

[RFC PATCH 0/5] drm/i915/opregion: proper handling of DIDL and CADL

[Jani Nikula]

This hopefully handles DIDL and CADL as they're supposed to be
handled. I haven't had the time to give this testing (thus RFC only),
and also I don't have a machine that actually suffers from the CADL not
being handled properly.

BR,
Jani.

References: http://mid.gmane.org/563B7E31.60200@redhat.com


Jani Nikula (5):
  drm/i915/opregion: add acpi defines from the spec
  drm/i915/opregion: abstract acpi display type getter for a connector
  drm/i915/opregion: handle missing connector types for acpi display
    types
  drm/i915: make i915 the source of acpi device ids for _DOD
  drm/i915/opregion: update cadl based on actually active outputs

 drivers/gpu/drm/i915/i915_drv.h       |   2 +
 drivers/gpu/drm/i915/intel_display.c  |   2 +
 drivers/gpu/drm/i915/intel_drv.h      |   3 +
 drivers/gpu/drm/i915/intel_opregion.c | 235 +++++++++++++++++-----------------
 4 files changed, 127 insertions(+), 115 deletions(-)

-- 
2.1.4

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

[RFC PATCH 1/5] drm/i915/opregion: add acpi defines from the spec

[Jani Nikula]

It's easier to read the code when the macro names match the spec. Also
add a bunch of missing ones. No functional changes.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_opregion.c | 37 +++++++++++++++++++++++++----------
 1 file changed, 27 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index c15718b4862a..66dc09aa7960 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -238,11 +238,28 @@ struct opregion_asle_ext {
 #define SWSCI_SBCB_POST_VBE_PM		SWSCI_FUNCTION_CODE(SWSCI_SBCB, 19)
 #define SWSCI_SBCB_ENABLE_DISABLE_AUDIO	SWSCI_FUNCTION_CODE(SWSCI_SBCB, 21)
 
-#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)
+/*
+ * ACPI Specification, Revision 5.0, Appendix B.3.2 _DOD (Enumerate All Devices
+ * Attached to the Display Adapter).
+ */
+#define ACPI_DISPLAY_INDEX_SHIFT		0
+#define ACPI_DISPLAY_INDEX_MASK			(0xf << 0)
+#define ACPI_DISPLAY_PORT_ATTACHMENT_SHIFT	4
+#define ACPI_DISPLAY_PORT_ATTACHMENT_MASK	(0xf << 4)
+#define ACPI_DISPLAY_TYPE_SHIFT			8
+#define ACPI_DISPLAY_TYPE_MASK			(0xf << 8)
+#define ACPI_DISPLAY_TYPE_OTHER			(0 << 8)
+#define ACPI_DISPLAY_TYPE_VGA			(1 << 8)
+#define ACPI_DISPLAY_TYPE_TV			(2 << 8)
+#define ACPI_DISPLAY_TYPE_EXTERNAL_DIGITAL	(3 << 8)
+#define ACPI_DISPLAY_TYPE_INTERNAL_DIGITAL	(4 << 8)
+#define ACPI_VENDOR_SPECIFIC_SHIFT		12
+#define ACPI_VENDOR_SPECIFIC_MASK		(0xf << 12)
+#define ACPI_BIOS_CAN_DETECT			(1 << 16)
+#define ACPI_DEPENDS_ON_VGA			(1 << 17)
+#define ACPI_PIPE_ID_SHIFT			18
+#define ACPI_PIPE_ID_MASK			(7 << 18)
+#define ACPI_DEVICE_ID_SCHEME			(1 << 31)
 
 #define MAX_DSLP	1500
 
@@ -733,7 +750,7 @@ end:
 blind_set:
 	i = 0;
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
-		int output_type = ACPI_OTHER_OUTPUT;
+		int output_type = ACPI_DISPLAY_TYPE_OTHER;
 		if (i >= max_outputs) {
 			DRM_DEBUG_KMS("More than %u outputs in connector list\n",
 				      max_outputs);
@@ -742,23 +759,23 @@ blind_set:
 		switch (connector->connector_type) {
 		case DRM_MODE_CONNECTOR_VGA:
 		case DRM_MODE_CONNECTOR_DVIA:
-			output_type = ACPI_VGA_OUTPUT;
+			output_type = ACPI_DISPLAY_TYPE_VGA;
 			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;
+			output_type = ACPI_DISPLAY_TYPE_TV;
 			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;
+			output_type = ACPI_DISPLAY_TYPE_EXTERNAL_DIGITAL;
 			break;
 		case DRM_MODE_CONNECTOR_LVDS:
-			output_type = ACPI_LVDS_OUTPUT;
+			output_type = ACPI_DISPLAY_TYPE_INTERNAL_DIGITAL;
 			break;
 		}
 		temp = get_did(opregion, i);
-- 
2.1.4

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

[RFC PATCH 2/5] drm/i915/opregion: abstract acpi display type getter for a connector

[Jani Nikula]

No functional changes.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_opregion.c | 58 ++++++++++++++++++++---------------
 1 file changed, 34 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index 66dc09aa7960..bf9de82ccec7 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -682,6 +682,36 @@ static void set_did(struct intel_opregion *opregion, int i, u32 val)
 	}
 }
 
+static u32 acpi_display_type(struct drm_connector *connector)
+{
+	u32 display_type = ACPI_DISPLAY_TYPE_OTHER;
+
+	switch (connector->connector_type) {
+	case DRM_MODE_CONNECTOR_VGA:
+	case DRM_MODE_CONNECTOR_DVIA:
+		display_type = ACPI_DISPLAY_TYPE_VGA;
+		break;
+	case DRM_MODE_CONNECTOR_Composite:
+	case DRM_MODE_CONNECTOR_SVIDEO:
+	case DRM_MODE_CONNECTOR_Component:
+	case DRM_MODE_CONNECTOR_9PinDIN:
+		display_type = ACPI_DISPLAY_TYPE_TV;
+		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:
+		display_type = ACPI_DISPLAY_TYPE_EXTERNAL_DIGITAL;
+		break;
+	case DRM_MODE_CONNECTOR_LVDS:
+		display_type = ACPI_DISPLAY_TYPE_INTERNAL_DIGITAL;
+		break;
+	}
+
+	return display_type;
+}
+
 static void intel_didl_outputs(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -750,36 +780,16 @@ end:
 blind_set:
 	i = 0;
 	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
-		int output_type = ACPI_DISPLAY_TYPE_OTHER;
+		int display_type = acpi_display_type(connector);
+
 		if (i >= max_outputs) {
 			DRM_DEBUG_KMS("More than %u outputs in connector list\n",
 				      max_outputs);
 			return;
 		}
-		switch (connector->connector_type) {
-		case DRM_MODE_CONNECTOR_VGA:
-		case DRM_MODE_CONNECTOR_DVIA:
-			output_type = ACPI_DISPLAY_TYPE_VGA;
-			break;
-		case DRM_MODE_CONNECTOR_Composite:
-		case DRM_MODE_CONNECTOR_SVIDEO:
-		case DRM_MODE_CONNECTOR_Component:
-		case DRM_MODE_CONNECTOR_9PinDIN:
-			output_type = ACPI_DISPLAY_TYPE_TV;
-			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_DISPLAY_TYPE_EXTERNAL_DIGITAL;
-			break;
-		case DRM_MODE_CONNECTOR_LVDS:
-			output_type = ACPI_DISPLAY_TYPE_INTERNAL_DIGITAL;
-			break;
-		}
+
 		temp = get_did(opregion, i);
-		set_did(opregion, i, temp | (1 << 31) | output_type | i);
+		set_did(opregion, i, temp | (1 << 31) | display_type | i);
 		i++;
 	}
 	goto end;
-- 
2.1.4

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

[RFC PATCH 3/5] drm/i915/opregion: handle missing connector types for acpi display types

[Jani Nikula]

Most notably eDP, DSI, and TV. Add MISSING_CASE handling so we won't
miss this in the future.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_opregion.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index bf9de82ccec7..adba17bfe5ba 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -684,7 +684,7 @@ static void set_did(struct intel_opregion *opregion, int i, u32 val)
 
 static u32 acpi_display_type(struct drm_connector *connector)
 {
-	u32 display_type = ACPI_DISPLAY_TYPE_OTHER;
+	u32 display_type;
 
 	switch (connector->connector_type) {
 	case DRM_MODE_CONNECTOR_VGA:
@@ -695,6 +695,7 @@ static u32 acpi_display_type(struct drm_connector *connector)
 	case DRM_MODE_CONNECTOR_SVIDEO:
 	case DRM_MODE_CONNECTOR_Component:
 	case DRM_MODE_CONNECTOR_9PinDIN:
+	case DRM_MODE_CONNECTOR_TV:
 		display_type = ACPI_DISPLAY_TYPE_TV;
 		break;
 	case DRM_MODE_CONNECTOR_DVII:
@@ -705,8 +706,18 @@ static u32 acpi_display_type(struct drm_connector *connector)
 		display_type = ACPI_DISPLAY_TYPE_EXTERNAL_DIGITAL;
 		break;
 	case DRM_MODE_CONNECTOR_LVDS:
+	case DRM_MODE_CONNECTOR_eDP:
+	case DRM_MODE_CONNECTOR_DSI:
 		display_type = ACPI_DISPLAY_TYPE_INTERNAL_DIGITAL;
 		break;
+	case DRM_MODE_CONNECTOR_Unknown:
+	case DRM_MODE_CONNECTOR_VIRTUAL:
+		display_type = ACPI_DISPLAY_TYPE_OTHER;
+		break;
+	default:
+		MISSING_CASE(connector->connector_type);
+		display_type = ACPI_DISPLAY_TYPE_OTHER;
+		break;
 	}
 
 	return display_type;
-- 
2.1.4

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

[RFC PATCH 4/5] drm/i915: make i915 the source of acpi device ids for _DOD

[Jani Nikula]

The graphics driver is supposed to define the DIDL, which are used for
_DOD, not the BIOS. Restore that behaviour.

This is basically a revert of

commit 3143751ff51a163b77f7efd389043e038f3e008e
Author: Zhang Rui <rui.zhang@intel.com>
Date:   Mon Mar 29 15:12:16 2010 +0800

    drm/i915: set DIDL using the ACPI video output device _ADR method return.

which went out of its way to cater to a specific BIOS, setting up DIDL
based on _ADR method. Perhaps that approach worked on that specific
machine, but on the machines I checked the _ADR method invents the
device identifiers out of thin air if DIDL has not been set. The source
for _ADR is also supposed to be the DIDL set by the driver, not the
other way around.

With this, we'll also limit the number of outputs to what the driver
actually has.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_drv.h      |  3 ++
 drivers/gpu/drm/i915/intel_opregion.c | 87 ++++++++++-------------------------
 2 files changed, 28 insertions(+), 62 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c87b4503435d..a5ea643a0df5 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -237,6 +237,9 @@ struct intel_connector {
 	 */
 	struct intel_encoder *encoder;
 
+	/* ACPI device id for ACPI and driver cooperation */
+	u32 acpi_device_id;
+
 	/* Reads out the current hw, returning true if the connector is enabled
 	 * and active (i.e. dpms ON state). */
 	bool (*get_hw_state)(struct intel_connector *);
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index adba17bfe5ba..1af9ed5c1d0a 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -682,11 +682,11 @@ static void set_did(struct intel_opregion *opregion, int i, u32 val)
 	}
 }
 
-static u32 acpi_display_type(struct drm_connector *connector)
+static u32 acpi_display_type(struct intel_connector *connector)
 {
 	u32 display_type;
 
-	switch (connector->connector_type) {
+	switch (connector->base.connector_type) {
 	case DRM_MODE_CONNECTOR_VGA:
 	case DRM_MODE_CONNECTOR_DVIA:
 		display_type = ACPI_DISPLAY_TYPE_VGA;
@@ -715,7 +715,7 @@ static u32 acpi_display_type(struct drm_connector *connector)
 		display_type = ACPI_DISPLAY_TYPE_OTHER;
 		break;
 	default:
-		MISSING_CASE(connector->connector_type);
+		MISSING_CASE(connector->base.connector_type);
 		display_type = ACPI_DISPLAY_TYPE_OTHER;
 		break;
 	}
@@ -727,33 +727,9 @@ 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;
-	acpi_handle handle;
-	struct acpi_device *acpi_dev, *acpi_cdev, *acpi_video_bus = NULL;
-	unsigned long long device_id;
-	acpi_status status;
-	u32 temp, max_outputs;
-	int i = 0;
-
-	handle = ACPI_HANDLE(&dev->pdev->dev);
-	if (!handle || acpi_bus_get_device(handle, &acpi_dev))
-		return;
-
-	if (acpi_is_video_device(handle))
-		acpi_video_bus = acpi_dev;
-	else {
-		list_for_each_entry(acpi_cdev, &acpi_dev->children, node) {
-			if (acpi_is_video_device(acpi_cdev->handle)) {
-				acpi_video_bus = acpi_cdev;
-				break;
-			}
-		}
-	}
-
-	if (!acpi_video_bus) {
-		DRM_DEBUG_KMS("No ACPI video bus found\n");
-		return;
-	}
+	struct intel_connector *connector;
+	int i = 0, max_outputs;
+	int display_index[16] = {};
 
 	/*
 	 * In theory, did2, the extended didl, gets added at opregion version
@@ -765,45 +741,32 @@ static void intel_didl_outputs(struct drm_device *dev)
 	max_outputs = ARRAY_SIZE(opregion->acpi->didl) +
 		ARRAY_SIZE(opregion->acpi->did2);
 
-	list_for_each_entry(acpi_cdev, &acpi_video_bus->children, node) {
-		if (i >= max_outputs) {
-			DRM_DEBUG_KMS("More than %u outputs detected via ACPI\n",
-				      max_outputs);
-			return;
-		}
-		status = acpi_evaluate_integer(acpi_cdev->handle, "_ADR",
-					       NULL, &device_id);
-		if (ACPI_SUCCESS(status)) {
-			if (!device_id)
-				goto blind_set;
-			set_did(opregion, i++, (u32)(device_id & 0x0f0f));
-		}
+	for_each_intel_connector(dev, connector) {
+		u32 device_id, type;
+
+		device_id = acpi_display_type(connector);
+		device_id |= ACPI_DEVICE_ID_SCHEME;
+
+		/* Use display type specific display index. */
+		type = (device_id & ACPI_DISPLAY_TYPE_MASK)
+			>> ACPI_DISPLAY_TYPE_SHIFT;
+		device_id |= display_index[type]++ << ACPI_DISPLAY_INDEX_SHIFT;
+
+		connector->acpi_device_id = device_id;
+		if (i < max_outputs)
+			set_did(opregion, i, device_id);
+		i++;
 	}
 
-end:
 	DRM_DEBUG_KMS("%d outputs detected\n", i);
 
+	if (i > max_outputs)
+		DRM_ERROR("More than %d outputs in connector list\n",
+			  max_outputs);
+
 	/* If fewer than max outputs, the list must be null terminated */
 	if (i < max_outputs)
 		set_did(opregion, i, 0);
-	return;
-
-blind_set:
-	i = 0;
-	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
-		int display_type = acpi_display_type(connector);
-
-		if (i >= max_outputs) {
-			DRM_DEBUG_KMS("More than %u outputs in connector list\n",
-				      max_outputs);
-			return;
-		}
-
-		temp = get_did(opregion, i);
-		set_did(opregion, i, temp | (1 << 31) | display_type | i);
-		i++;
-	}
-	goto end;
 }
 
 static void intel_setup_cadls(struct drm_device *dev)
-- 
2.1.4

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

[RFC PATCH 5/5] drm/i915/opregion: update cadl based on actually active outputs

[Jani Nikula]

Previously we've just shoved the first eight devices in DIDL to CADL
(list of active outputs). Some of the active outputs may have been left
outside of CADL. The problem is, some BIOS implementations prevent
laptop brightness hotkey propagation if the flat panel is not active.

Now that we have connector to acpi device id mapping covered, we can
update CADL based on which outputs are actually active.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h       |  2 +
 drivers/gpu/drm/i915/intel_display.c  |  2 +
 drivers/gpu/drm/i915/intel_opregion.c | 70 ++++++++++++++++++-----------------
 3 files changed, 41 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9d29ab06c99a..27287c7d62d9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3407,6 +3407,7 @@ extern int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
 					 bool enable);
 extern int intel_opregion_notify_adapter(struct drm_device *dev,
 					 pci_power_t state);
+void intel_opregion_update_cadl(struct drm_device *dev);
 #else
 static inline int intel_opregion_setup(struct drm_device *dev) { return 0; }
 static inline void intel_opregion_init(struct drm_device *dev) { return; }
@@ -3422,6 +3423,7 @@ intel_opregion_notify_adapter(struct drm_device *dev, pci_power_t state)
 {
 	return 0;
 }
+void intel_opregion_update_cadl(struct drm_device *dev) { return; }
 #endif
 
 /* intel_acpi.c */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 51f913fb199d..76f48c44782f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13646,6 +13646,8 @@ static int intel_atomic_commit(struct drm_device *dev,
 
 	drm_atomic_state_free(state);
 
+	intel_opregion_update_cadl(dev);
+
 	/* As one of the primary mmio accessors, KMS has a high likelihood
 	 * of triggering bugs in unclaimed access. After we finish
 	 * modesetting, see if an error has been flagged, and if so
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index 1af9ed5c1d0a..d1ff264a82e6 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -650,24 +650,6 @@ static struct notifier_block intel_opregion_notifier = {
  * (version 3)
  */
 
-static u32 get_did(struct intel_opregion *opregion, int i)
-{
-	u32 did;
-
-	if (i < ARRAY_SIZE(opregion->acpi->didl)) {
-		did = opregion->acpi->didl[i];
-	} else {
-		i -= ARRAY_SIZE(opregion->acpi->didl);
-
-		if (WARN_ON(i >= ARRAY_SIZE(opregion->acpi->did2)))
-			return 0;
-
-		did = opregion->acpi->did2[i];
-	}
-
-	return did;
-}
-
 static void set_did(struct intel_opregion *opregion, int i, u32 val)
 {
 	if (i < ARRAY_SIZE(opregion->acpi->didl)) {
@@ -682,6 +664,14 @@ static void set_did(struct intel_opregion *opregion, int i, u32 val)
 	}
 }
 
+static void set_cad(struct intel_opregion *opregion, int i, u32 val)
+{
+	if (WARN_ON(i >= ARRAY_SIZE(opregion->acpi->cadl)))
+		return;
+
+	opregion->acpi->cadl[i] = val;
+}
+
 static u32 acpi_display_type(struct intel_connector *connector)
 {
 	u32 display_type;
@@ -769,23 +759,37 @@ static void intel_didl_outputs(struct drm_device *dev)
 		set_did(opregion, i, 0);
 }
 
-static void intel_setup_cadls(struct drm_device *dev)
+/* Update CADL to reflect active outputs. */
+void intel_opregion_update_cadl(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_opregion *opregion = &dev_priv->opregion;
-	int i = 0;
-	u32 disp_id;
-
-	/* Initialize the CADL field by duplicating the DIDL values.
-	 * Technically, this is not always correct as display outputs may exist,
-	 * but not active. This initialization is necessary for some Clevo
-	 * laptops that check this field before processing the brightness and
-	 * display switching hotkeys. Just like DIDL, CADL is NULL-terminated if
-	 * there are less than eight devices. */
-	do {
-		disp_id = get_did(opregion, i);
-		opregion->acpi->cadl[i] = disp_id;
-	} while (++i < 8 && disp_id != 0);
+	struct intel_crtc *crtc;
+	int i = 0, max_active = ARRAY_SIZE(opregion->acpi->cadl);
+
+	for_each_intel_crtc(dev, crtc) {
+		struct intel_encoder *encoder;
+
+		if (!crtc->active)
+			continue;
+
+		for_each_encoder_on_crtc(dev, &crtc->base, encoder) {
+			struct intel_connector *connector;
+
+			for_each_connector_on_encoder(dev, &encoder->base, connector) {
+				if (i >= max_active) {
+					DRM_DEBUG_KMS("too many outputs active\n");
+					return;
+				}
+
+				set_cad(opregion, i++, connector->acpi_device_id);
+			}
+		}
+	}
+
+	/* If fewer than max active outputs, the list must be null terminated */
+	if (i < max_active)
+		set_cad(opregion, i, 0);
 }
 
 void intel_opregion_init(struct drm_device *dev)
@@ -798,7 +802,7 @@ void intel_opregion_init(struct drm_device *dev)
 
 	if (opregion->acpi) {
 		intel_didl_outputs(dev);
-		intel_setup_cadls(dev);
+		intel_opregion_update_cadl(dev);
 
 		/* Notify BIOS we are ready to handle ACPI video ext notifs.
 		 * Right now, all the events are handled by the ACPI video module.
-- 
2.1.4

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

✗ Fi.CI.BAT: failure for drm/i915/opregion: proper handling of DIDL and CADL

[Patchwork]

== Series Details ==

Series: drm/i915/opregion: proper handling of DIDL and CADL
URL   : https://patchwork.freedesktop.org/series/4783/
State : failure

== Summary ==

Series 4783v1 drm/i915/opregion: proper handling of DIDL and CADL
http://patchwork.freedesktop.org/api/1.0/series/4783/revisions/1/mbox/

Test drv_hangman:
        Subgroup error-state-basic:
                pass       -> FAIL       (ilk-hp8440p)
Test gem_exec_suspend:
        Subgroup basic-s3:
                dmesg-warn -> PASS       (bsw-nuc-2)
Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                pass       -> DMESG-WARN (bsw-nuc-2)
Test pm_rpm:
        Subgroup basic-pci-d3-state:
                pass       -> DMESG-WARN (bsw-nuc-2)
        Subgroup basic-rte:
                dmesg-warn -> PASS       (bsw-nuc-2)

bdw-nuci7        total:192  pass:180  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:192  pass:171  dwarn:0   dfail:0   fail:0   skip:21 
bsw-nuc-2        total:192  pass:153  dwarn:2   dfail:0   fail:0   skip:37 
byt-nuc          total:192  pass:157  dwarn:0   dfail:0   fail:0   skip:35 
hsw-brixbox      total:192  pass:170  dwarn:0   dfail:0   fail:0   skip:22 
hsw-gt2          total:192  pass:175  dwarn:0   dfail:0   fail:0   skip:17 
ilk-hp8440p      total:192  pass:128  dwarn:0   dfail:0   fail:1   skip:63 
skl-i7k-2        total:192  pass:169  dwarn:0   dfail:0   fail:0   skip:23 
skl-nuci5        total:192  pass:181  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:192  pass:158  dwarn:0   dfail:0   fail:0   skip:34 
snb-x220t        total:192  pass:158  dwarn:0   dfail:0   fail:1   skip:33 

Results at /archive/results/CI_IGT_test/Patchwork_1685/

d93cd8c5ded88f5907ad90f1c8cd6625ae0b00b1 drm-intel-nightly: 2016y-03m-23d-10h-01m-57s UTC integration manifest
72586fe418879aeafb3d0880807aa596e15e32d8 drm/i915/opregion: update cadl based on actually active outputs
1c6104d3f1c0aed8ed06704c64544a5e26e3f829 drm/i915: make i915 the source of acpi device ids for _DOD
3db317adc7a62e11e0363808466dd68a080af895 drm/i915/opregion: handle missing connector types for acpi display types
a02958fe5a514bad1b0ff48e89496780f22900af drm/i915/opregion: abstract acpi display type getter for a connector
4074658de308431070f5c596991635579be26ede drm/i915/opregion: add acpi defines from the spec

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

Re: [RFC PATCH 4/5] drm/i915: make i915 the source of acpi device ids for _DOD

[Peter Wu]

On Wed, Mar 23, 2016 at 01:16:21PM +0200, Jani Nikula wrote:
> The graphics driver is supposed to define the DIDL, which are used for
> _DOD, not the BIOS. Restore that behaviour.
> 
> This is basically a revert of
> 
> commit 3143751ff51a163b77f7efd389043e038f3e008e
> Author: Zhang Rui <rui.zhang@intel.com>
> Date:   Mon Mar 29 15:12:16 2010 +0800
> 
>     drm/i915: set DIDL using the ACPI video output device _ADR method return.
> 
> which went out of its way to cater to a specific BIOS, setting up DIDL
> based on _ADR method. Perhaps that approach worked on that specific
> machine, but on the machines I checked the _ADR method invents the
> device identifiers out of thin air if DIDL has not been set. The source
> for _ADR is also supposed to be the DIDL set by the driver, not the
> other way around.
> 
> With this, we'll also limit the number of outputs to what the driver
> actually has.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Patches 1-3 and 5 are fine, some minor notes:
 - in patch 1, maybe use symbolic names instead of numeric shift offsets
 - in patch 5-6, the name "acpi_device_id" seems to suggest that the
   name is part of the ACPI kernel API, but I have no better name
   suggestion.

See below for other comments on this patch.

> ---
>  drivers/gpu/drm/i915/intel_drv.h      |  3 ++
>  drivers/gpu/drm/i915/intel_opregion.c | 87 ++++++++++-------------------------
>  2 files changed, 28 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index c87b4503435d..a5ea643a0df5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -237,6 +237,9 @@ struct intel_connector {
>  	 */
>  	struct intel_encoder *encoder;
>  
> +	/* ACPI device id for ACPI and driver cooperation */
> +	u32 acpi_device_id;
> +
>  	/* Reads out the current hw, returning true if the connector is enabled
>  	 * and active (i.e. dpms ON state). */
>  	bool (*get_hw_state)(struct intel_connector *);
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index adba17bfe5ba..1af9ed5c1d0a 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -682,11 +682,11 @@ static void set_did(struct intel_opregion *opregion, int i, u32 val)
>  	}
>  }
>  
> -static u32 acpi_display_type(struct drm_connector *connector)
> +static u32 acpi_display_type(struct intel_connector *connector)
>  {
>  	u32 display_type;
>  
> -	switch (connector->connector_type) {
> +	switch (connector->base.connector_type) {
>  	case DRM_MODE_CONNECTOR_VGA:
>  	case DRM_MODE_CONNECTOR_DVIA:
>  		display_type = ACPI_DISPLAY_TYPE_VGA;
> @@ -715,7 +715,7 @@ static u32 acpi_display_type(struct drm_connector *connector)
>  		display_type = ACPI_DISPLAY_TYPE_OTHER;
>  		break;
>  	default:
> -		MISSING_CASE(connector->connector_type);
> +		MISSING_CASE(connector->base.connector_type);
>  		display_type = ACPI_DISPLAY_TYPE_OTHER;
>  		break;
>  	}
> @@ -727,33 +727,9 @@ 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;
> -	acpi_handle handle;
> -	struct acpi_device *acpi_dev, *acpi_cdev, *acpi_video_bus = NULL;
> -	unsigned long long device_id;
> -	acpi_status status;
> -	u32 temp, max_outputs;
> -	int i = 0;
> -
> -	handle = ACPI_HANDLE(&dev->pdev->dev);
> -	if (!handle || acpi_bus_get_device(handle, &acpi_dev))
> -		return;
> -
> -	if (acpi_is_video_device(handle))
> -		acpi_video_bus = acpi_dev;
> -	else {
> -		list_for_each_entry(acpi_cdev, &acpi_dev->children, node) {
> -			if (acpi_is_video_device(acpi_cdev->handle)) {
> -				acpi_video_bus = acpi_cdev;
> -				break;
> -			}
> -		}
> -	}
> -
> -	if (!acpi_video_bus) {
> -		DRM_DEBUG_KMS("No ACPI video bus found\n");
> -		return;
> -	}
> +	struct intel_connector *connector;
> +	int i = 0, max_outputs;
> +	int display_index[16] = {};
>  
>  	/*
>  	 * In theory, did2, the extended didl, gets added at opregion version
> @@ -765,45 +741,32 @@ static void intel_didl_outputs(struct drm_device *dev)
>  	max_outputs = ARRAY_SIZE(opregion->acpi->didl) +
>  		ARRAY_SIZE(opregion->acpi->did2);
>  
> -	list_for_each_entry(acpi_cdev, &acpi_video_bus->children, node) {
> -		if (i >= max_outputs) {
> -			DRM_DEBUG_KMS("More than %u outputs detected via ACPI\n",
> -				      max_outputs);
> -			return;
> -		}
> -		status = acpi_evaluate_integer(acpi_cdev->handle, "_ADR",
> -					       NULL, &device_id);
> -		if (ACPI_SUCCESS(status)) {
> -			if (!device_id)
> -				goto blind_set;
> -			set_did(opregion, i++, (u32)(device_id & 0x0f0f));

Previously the device ID included just the display index (0x00f) and
display type (0xf00).

> -		}
> +	for_each_intel_connector(dev, connector) {
> +		u32 device_id, type;
> +
> +		device_id = acpi_display_type(connector);
> +		device_id |= ACPI_DEVICE_ID_SCHEME;

Now it includes an additional bit. The Clevo P651RA firmware expects
that the contents contain a value masked with 0xf0f. If this line is
removed, the Brightness Down/Up Notification is sent to the display
device:

        ElseIf ((^^^GFX0.CDDS (0x0410) == 0x1F)) {
            Notify (^^^GFX0.DD1F, 0x87)
        }

(The items in _DOD are constructed with 0x80000000 | (DIDx & 0xf0f).)

I have tested it also on an older Clevo B7130 laptop which needed the
hack that is removed in patch 6.  There is no regression on that device.
For some reason the old laptop exposed an "acpi_video0" device, pressing
hotkeys generates both a KEY_BRIGHTNESSDOWN event (measured via evtest)
and changes the brightness itself.

On the new laptop (Clevo P651RA) there is an "intel_backlight" device
and only the hotkeys are sent, the actual brightness was not affected.
I guess that a desktop environment should handle it, but this
unfortunately does not really work for the text consoles.

Anyway, these series (with the above fix) are
Reviewed-and-tested-by: Peter Wu <peter@lekensteyn.nl>

Peter

> +		/* Use display type specific display index. */
> +		type = (device_id & ACPI_DISPLAY_TYPE_MASK)
> +			>> ACPI_DISPLAY_TYPE_SHIFT;
> +		device_id |= display_index[type]++ << ACPI_DISPLAY_INDEX_SHIFT;
> 
> +		connector->acpi_device_id = device_id;
> +		if (i < max_outputs)
> +			set_did(opregion, i, device_id);
> +		i++;
>  	}
>  
> -end:
>  	DRM_DEBUG_KMS("%d outputs detected\n", i);
>  
> +	if (i > max_outputs)
> +		DRM_ERROR("More than %d outputs in connector list\n",
> +			  max_outputs);
> +
>  	/* If fewer than max outputs, the list must be null terminated */
>  	if (i < max_outputs)
>  		set_did(opregion, i, 0);
> -	return;
> -
> -blind_set:
> -	i = 0;
> -	list_for_each_entry(connector, &dev->mode_config.connector_list, head) {
> -		int display_type = acpi_display_type(connector);
> -
> -		if (i >= max_outputs) {
> -			DRM_DEBUG_KMS("More than %u outputs in connector list\n",
> -				      max_outputs);
> -			return;
> -		}
> -
> -		temp = get_did(opregion, i);
> -		set_did(opregion, i, temp | (1 << 31) | display_type | i);
> -		i++;
> -	}
> -	goto end;
>  }
>  
>  static void intel_setup_cadls(struct drm_device *dev)
> -- 
> 2.1.4
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx