Skip to contentSkip to conversation

drm/i915/opregion: proper handling of DIDL, and some hacks on CADL

PATCH In review Jani Nikula 5 participants 10 messages Nov 16, 2016 – Nov 21, 2016

Subsystems (2)
DRM drivers Intel i915
lore
Share & tools
lore

Jani Nikula

author
#0 lore raw

cover drm/i915/opregion: proper handling of DIDL, and some hacks on CADL

Another spin of [1]. The pain point seems to be the CADL update based on the list of active connectors. So I left it out for now, and instead just hack it to ensure CADL contains the internal displays.

Let's see if this sticks. It shouldn't prevent us from fixing CADL update properly down the line.

BR,
Jani.

[1] http://mid.mail-archive.com/[email protected]

Cc Peter Wu <[email protected]>
Cc Rainer Koenig <[email protected]>
Cc Jan-Marek Glogowski <[email protected]>
Cc Maarten Lankhorst <[email protected]>
Cc Marcos Paulo de Souza <[email protected]>
Cc Paolo Stivanin <[email protected]>

Jani Nikula (3):
drm/i915: make i915 the source of acpi device ids for _DOD drm/i915/opregion: fill in the CADL from connector list, not DIDL drm/i915/opregion: put internal panels to the front of CADL

 drivers/gpu/drm/i915/intel_drv.h      |   3 +
 drivers/gpu/drm/i915/intel_opregion.c | 152 +++++++++++++---------------------
 2 files changed, 61 insertions(+), 94 deletions(-)
Signature · 6 lines
2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
author
#1 lore raw

PATCH 1/3 drm/i915: make i915 the source of acpi device ids for _DOD

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 <[email protected]>
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.

v2: do not set ACPI_DEVICE_ID_SCHEME in the device id (Peter Wu)

v3: Rebase

Cc Peter Wu <[email protected]>
Cc Rainer Koenig <[email protected]>
Cc Jan-Marek Glogowski <[email protected]>
Cc Maarten Lankhorst <[email protected]>
Cc Marcos Paulo de Souza <[email protected]>
Cc Paolo Stivanin <[email protected]>
Tested-by Marcos Paulo de Souza <[email protected]>
Reviewed-and-tested-by Peter Wu <[email protected]>
Signed-off-by Jani Nikula <[email protected]>
---
 drivers/gpu/drm/i915/intel_drv.h      |  3 ++
 drivers/gpu/drm/i915/intel_opregion.c | 88 ++++++++++-------------------------
 2 files changed, 27 insertions(+), 64 deletions(-)
drivers/gpu/drm/i915/intel_drv.h +3 -0
index 0be407e50d61..5b93455b98b7 100644
@@ -294,6 +294,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 *);
drivers/gpu/drm/i915/intel_opregion.c +24 -64

122 lines not on this page · raw message

Signature · 6 lines
2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
author
#2 lore raw

PATCH 2/3 drm/i915/opregion: fill in the CADL from connector list, not DIDL

In reply to Jani Nikula

This is essentially the same thing as duplicating DIDL now that the connector list has the ACPI device IDs.

Cc Peter Wu <[email protected]>
Cc Rainer Koenig <[email protected]>
Cc Jan-Marek Glogowski <[email protected]>
Cc Maarten Lankhorst <[email protected]>
Cc Marcos Paulo de Souza <[email protected]>
Cc Paolo Stivanin <[email protected]>
Signed-off-by Jani Nikula <[email protected]>
---
 drivers/gpu/drm/i915/intel_opregion.c | 48 +++++++++++++----------------------
 1 file changed, 18 insertions(+), 30 deletions(-)
drivers/gpu/drm/i915/intel_opregion.c +18 -30
index 6bac71c26e94..024ae9b393e7 100644
@@ -642,24 +642,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)) {

@@ -762,19 +744,25 @@ static void intel_didl_outputs(struct drm_i915_private *dev_priv)
 static void intel_setup_cadls(struct drm_i915_private *dev_priv)
 {
 	struct intel_opregion *opregion = &dev_priv->opregion;
+	struct intel_connector *connector;
 	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);
+
+	/*
+	 * Initialize the CADL field from the connector device ids. This is
+	 * essentially the same as copying from the DIDL. 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.
+	 */
+	for_each_intel_connector(&dev_priv->drm, connector) {
+		if (i >= ARRAY_SIZE(opregion->acpi->cadl))
+			break;
+		opregion->acpi->cadl[i++] = connector->acpi_device_id;
+	}
+
+	/* If fewer than 8 active devices, the list must be null terminated */
+	if (i < ARRAY_SIZE(opregion->acpi->cadl))
+		opregion->acpi->cadl[i] = 0;
 }
 
 void intel_opregion_register(struct drm_i915_private *dev_priv)
Signature · 6 lines
2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
author
#3 lore raw

PATCH 3/3 drm/i915/opregion: put internal panels to the front of CADL

In reply to Jani Nikula

The attempts to update CADL based on the actual active connectors have not been successful. That is the right thing to do ultimately, but there must be something we're still missing.

In the mean time, change the dumb CADL initialization we currently have to put internal panels in front of the CADL to ensure they're included in the list. (There's a max of eight.) Hopefully this ensures the BIOS does not block backlight hotkey events, thinking the internal panel is off.

Cc Peter Wu <[email protected]>
Cc Rainer Koenig <[email protected]>
Cc Jan-Marek Glogowski <[email protected]>
Cc Maarten Lankhorst <[email protected]>
Cc Marcos Paulo de Souza <[email protected]>
Cc Paolo Stivanin <[email protected]>
Signed-off-by Jani Nikula <[email protected]>
---
 drivers/gpu/drm/i915/intel_opregion.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
drivers/gpu/drm/i915/intel_opregion.c +16 -0
index 024ae9b393e7..cb58194973ae 100644
@@ -753,8 +753,24 @@ static void intel_setup_cadls(struct drm_i915_private *dev_priv)
 	 * 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.
+	 * Put internal panels in front of the list to ensure they're not left
+	 * out.
 	 */
 	for_each_intel_connector(&dev_priv->drm, connector) {
+		if ((connector->acpi_device_id & ACPI_DISPLAY_TYPE_MASK) !=
+		    ACPI_DISPLAY_TYPE_INTERNAL_DIGITAL)
+			continue;
+
+		if (i >= ARRAY_SIZE(opregion->acpi->cadl))
+			break;
+		opregion->acpi->cadl[i++] = connector->acpi_device_id;
+	}
+
+	for_each_intel_connector(&dev_priv->drm, connector) {
+		if ((connector->acpi_device_id & ACPI_DISPLAY_TYPE_MASK) ==
+		    ACPI_DISPLAY_TYPE_INTERNAL_DIGITAL)
+			continue;
+
 		if (i >= ARRAY_SIZE(opregion->acpi->cadl))
 			break;
 		opregion->acpi->cadl[i++] = connector->acpi_device_id;
Signature · 6 lines
2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
CI
#4 lore raw

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

In reply to Jani Nikula
== Series Details ==

== Series Details ==

Series: drm/i915/opregion: proper handling of DIDL, and some hacks on CADL
URL : https://patchwork.freedesktop.org/series/15403/
State : failure

== Summary ==

Series 15403v1 drm/i915/opregion: proper handling of DIDL, and some hacks on CADL
https://patchwork.freedesktop.org/api/1.0/series/15403/revisions/1/mbox/

Test gem_busy:

        Subgroup basic-hang-default:
                pass       -> FAIL       (fi-hsw-4770r)
fi-bdw-5557u     total:244  pass:229  dwarn:0   dfail:0   fail:0   skip:15 
fi-bsw-n3050     total:244  pass:204  dwarn:0   dfail:0   fail:0   skip:40 
fi-byt-j1900     total:244  pass:216  dwarn:0   dfail:0   fail:0   skip:28 
fi-byt-n2820     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-hsw-4770      total:244  pass:224  dwarn:0   dfail:0   fail:0   skip:20 
fi-hsw-4770r     total:244  pass:223  dwarn:0   dfail:0   fail:1   skip:20 
fi-ilk-650       total:244  pass:191  dwarn:0   dfail:0   fail:0   skip:53 
fi-ivb-3520m     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-ivb-3770      total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-7200u     total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-skl-6260u     total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-skl-6700hq    total:244  pass:223  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6700k     total:244  pass:222  dwarn:1   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:244  pass:230  dwarn:0   dfail:0   fail:0   skip:14 
fi-snb-2520m     total:244  pass:212  dwarn:0   dfail:0   fail:0   skip:32 
fi-snb-2600      total:244  pass:211  dwarn:0   dfail:0   fail:0   skip:33

6294f67611ebe69006c0e85c372efadcac8e9d66 drm-intel-nightly: 2016y-11m-16d-09h-57m-25s UTC integration manifest 1c5b376 drm/i915/opregion: put internal panels to the front of CADL
d8c1b3b drm/i915/opregion: fill in the CADL from connector list, not DIDL
c095ca7 drm/i915: make i915 the source of acpi device ids for _DOD

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3017/
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

#5 lore raw
In reply to Jani Nikula

Hi Jani,

applied those patches to the 4.9.0-rc4 from
http://anongit.freedesktop.org/git/drm-intel.git
and tested it on the Fujitsu LIFEBOOK E736.

Works perfectly, even after suspend and resume.

Adding my hopes that this will be merged into the mainline soon.

Best regards and thanks a lot
Rainer

Am 16.11.2016 um 12:29 schrieb Jani Nikula:

Quoted 22 lines

Another spin of [1]. The pain point seems to be the CADL update based on the list of active connectors. So I left it out for now, and instead just hack it to ensure CADL contains the internal displays.

Let's see if this sticks. It shouldn't prevent us from fixing CADL update properly down the line.

BR,
Jani.

[1] http://mid.mail-archive.com/[email protected]

Jani Nikula (3):
drm/i915: make i915 the source of acpi device ids for _DOD drm/i915/opregion: fill in the CADL from connector list, not DIDL drm/i915/opregion: put internal panels to the front of CADL

 drivers/gpu/drm/i915/intel_drv.h      |   3 +
 drivers/gpu/drm/i915/intel_opregion.c | 152 +++++++++++++---------------------
 2 files changed, 61 insertions(+), 94 deletions(-)
Signature · 19 lines
Dipl.-Inf. (FH) Rainer Koenig
Project Manager Linux Clients
FJ EMEIA PR PSO PM&D CCD ENG SW OSS&C

Fujitsu Technology Solutions
Bürgermeister-Ullrich-Str. 100
86199 Augsburg
Germany

Telephone: +49-821-804-3321
Telefax:   +49-821-804-2131
Mail:      mailto:[email protected]

Internet         ts.fujtsu.com
Company Details  ts.fujitsu.com/imprint.html
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
author
#6 lore raw
Quoted 9 lines · Rainer Koenig

Hi Jani,

applied those patches to the 4.9.0-rc4 from
http://anongit.freedesktop.org/git/drm-intel.git
and tested it on the Fujitsu LIFEBOOK E736.

Works perfectly, even after suspend and resume.

Adding my hopes that this will be merged into the mainline soon.

Pushed patches 1-2 to drm-intel-next-queued, with Daniel's review on patch 2. He observed that patch 3 should not be needed, as we already sort the connector list to have internal panels in front.

I presume this will make it to v4.10-rc1. I'm thinking we could do a stable backport request once this has had more exposure.

Thanks for the review and testing. And patience!

BR,
Jani.

Signature · 5 lines
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
#7 lore raw

Hello,
I can confirm that patch 3 is not needed.
I applied only patch 1 and 2 and everything works super fine!

Thanks again!

BR

Paolo

Signature · 3 lines
  Paolo Stivanin
  [email protected]
  Public Key ID: E71B7545
Quoted 15 lines · Jani Nikula
Quoted 9 lines · Rainer Koenig

Hi Jani,

applied those patches to the 4.9.0-rc4 from
http://anongit.freedesktop.org/git/drm-intel.git
and tested it on the Fujitsu LIFEBOOK E736.

Works perfectly, even after suspend and resume.

Adding my hopes that this will be merged into the mainline soon.

Pushed patches 1-2 to drm-intel-next-queued, with Daniel's review on patch 2. He observed that patch 3 should not be needed, as we already sort the connector list to have internal panels in front.

Quoted 12 lines

I presume this will make it to v4.10-rc1. I'm thinking we could do a stable backport request once this has had more exposure.

Thanks for the review and testing. And patience!

BR,
Jani.

Signature · 1 line
Jani Nikula, Intel Open Source Technology Center

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

author
#8 lore raw
Quoted 3 lines · Paolo Stivanin

Hello,
I can confirm that patch 3 is not needed.
I applied only patch 1 and 2 and everything works super fine!

Thanks for confirming this.

BR,
Jani.

Signature · 5 lines
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
#9 lore raw

Hi Jani,

I can also confirm that patches 1+2 (applied in top of v4.9-rc5ish) fixes it for me (Clevo P651RA). The previous issue where the keys broke with PRIME/monitor hotplugging is also gone. After system suspend/resume stuff is also still working.

Thanks!

Kind regards,
Peter
https://lekensteyn.nl
(pardon my brevity, top-posting and formatting, sent from my phone)

Quoted 9 lines · Jani Nikula
Quoted 3 lines · Paolo Stivanin

Hello,
I can confirm that patch 3 is not needed.
I applied only patch 1 and 2 and everything works super fine!

Thanks for confirming this.

BR,
Jani.

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx