[PATCH v4 0/2] drm/i915/opregion: proper handling of DIDL and CADL

[PATCH v4 0/2] drm/i915/opregion: proper handling of DIDL and CADL

[Jani Nikula]

This is v4 of [1]. The first three have already been pushed to
drm-intel-next-queued. The only change here is the atomic commit.

Review and testing would be much appreciated to move this forward. For
testing, I've pushed this to opregion-didl-v4 branch of my repo at [2].

Maarten, please check the hunk touching the atomic code in patch 2.

BR,
Jani.


[1] http://mid.gmane.org/cover.1465810007.git.jani.nikula@intel.com
[2] https://cgit.freedesktop.org/~jani/drm/

Jani Nikula (2):
  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  |   4 +
 drivers/gpu/drm/i915/intel_drv.h      |   3 +
 drivers/gpu/drm/i915/intel_opregion.c | 155 +++++++++++++---------------------
 4 files changed, 69 insertions(+), 95 deletions(-)

-- 
2.1.4

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

[PATCH v4 1/2] 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.

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

Reviewed-and-tested-by: Peter Wu <peter@lekensteyn.nl>
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, 27 insertions(+), 63 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 98a5be4ec8c5..2fbd0a34c9f5 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -263,6 +263,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 82e687dd09c3..632f0178c2b0 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -674,11 +674,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;
@@ -707,7 +707,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;
 	}
@@ -718,34 +718,9 @@ static u32 acpi_display_type(struct drm_connector *connector)
 static void intel_didl_outputs(struct drm_i915_private *dev_priv)
 {
 	struct intel_opregion *opregion = &dev_priv->opregion;
-	struct pci_dev *pdev = dev_priv->dev->pdev;
-	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(&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
@@ -757,45 +732,31 @@ static void intel_didl_outputs(struct drm_i915_private *dev_priv)
 	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_priv->dev, connector) {
+		u32 device_id, type;
+
+		device_id = acpi_display_type(connector);
+
+		/* 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_priv->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_i915_private *dev_priv)
-- 
2.1.4

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

[PATCH v4 2/2] 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.

v3: actually git add the dev->dev_priv change.

v4: update cadl in intel_shared_dpll_commit() if intel_state->modeset
    (Maarten)

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reviewed-and-tested-by: Peter Wu <peter@lekensteyn.nl>
Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h       |  2 +
 drivers/gpu/drm/i915/intel_display.c  |  4 ++
 drivers/gpu/drm/i915/intel_opregion.c | 70 ++++++++++++++++++-----------------
 3 files changed, 43 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 724d34b00196..64ab52529be8 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3692,6 +3692,7 @@ extern int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
 extern int intel_opregion_notify_adapter(struct drm_i915_private *dev_priv,
 					 pci_power_t state);
 extern int intel_opregion_get_panel_type(struct drm_i915_private *dev_priv);
+extern void intel_opregion_update_cadl(struct drm_i915_private *dev_priv);
 #else
 static inline int intel_opregion_setup(struct drm_i915_private *dev) { return 0; }
 static inline void intel_opregion_register(struct drm_i915_private *dev_priv) { }
@@ -3713,6 +3714,7 @@ static inline int intel_opregion_get_panel_type(struct drm_i915_private *dev)
 {
 	return -ENODEV;
 }
+static inline void intel_opregion_update_cadl(struct drm_i915_private *dev_priv) { }
 #endif
 
 /* intel_acpi.c */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d902a70edb84..4f404900f610 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -13953,6 +13953,10 @@ static int intel_atomic_commit(struct drm_device *dev,
 	dev_priv->wm.distrust_bios_wm = false;
 	dev_priv->wm.skl_results = intel_state->wm_results;
 	intel_shared_dpll_commit(state);
+
+	if (intel_state->modeset)
+		intel_opregion_update_cadl(dev_priv);
+
 	intel_atomic_track_fbs(state);
 
 	if (nonblock)
diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
index 632f0178c2b0..8b3f7e6ae4bb 100644
--- a/drivers/gpu/drm/i915/intel_opregion.c
+++ b/drivers/gpu/drm/i915/intel_opregion.c
@@ -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)) {
@@ -674,6 +656,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;
@@ -759,22 +749,36 @@ static void intel_didl_outputs(struct drm_i915_private *dev_priv)
 		set_did(opregion, i, 0);
 }
 
-static void intel_setup_cadls(struct drm_i915_private *dev_priv)
+/* Update CADL to reflect active outputs. */
+void intel_opregion_update_cadl(struct drm_i915_private *dev_priv)
 {
 	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_priv->dev, crtc) {
+		struct intel_encoder *encoder;
+
+		if (!crtc->active)
+			continue;
+
+		for_each_encoder_on_crtc(dev_priv->dev, &crtc->base, encoder) {
+			struct intel_connector *connector;
+
+			for_each_connector_on_encoder(dev_priv->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_register(struct drm_i915_private *dev_priv)
@@ -786,7 +790,7 @@ void intel_opregion_register(struct drm_i915_private *dev_priv)
 
 	if (opregion->acpi) {
 		intel_didl_outputs(dev_priv);
-		intel_setup_cadls(dev_priv);
+		intel_opregion_update_cadl(dev_priv);
 
 		/* 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

✗ Ro.CI.BAT: failure for drm/i915/opregion: proper handling of DIDL and CADL (rev4)

[Patchwork]

== Series Details ==

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

== Summary ==

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

Test drv_module_reload_basic:
                pass       -> SKIP       (fi-skl-i5-6260u)
Test gem_exec_flush:
        Subgroup basic-batch-kernel-default-cmd:
                pass       -> FAIL       (ro-byt-n2820)
Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-a-frame-sequence:
                pass       -> FAIL       (ro-bdw-i7-5557U)

fi-kbl-qkkr      total:229  pass:161  dwarn:29  dfail:0   fail:0   skip:39 
fi-skl-i5-6260u  total:229  pass:201  dwarn:0   dfail:0   fail:2   skip:26 
fi-snb-i7-2600   total:229  pass:174  dwarn:0   dfail:0   fail:2   skip:53 
ro-bdw-i5-5250u  total:229  pass:202  dwarn:1   dfail:1   fail:2   skip:23 
ro-bdw-i7-5557U  total:229  pass:201  dwarn:1   dfail:1   fail:3   skip:23 
ro-bdw-i7-5600u  total:229  pass:190  dwarn:0   dfail:1   fail:0   skip:38 
ro-bsw-n3050     total:229  pass:177  dwarn:0   dfail:1   fail:2   skip:49 
ro-byt-n2820     total:229  pass:178  dwarn:0   dfail:1   fail:5   skip:45 
ro-hsw-i3-4010u  total:229  pass:195  dwarn:0   dfail:1   fail:2   skip:31 
ro-hsw-i7-4770r  total:229  pass:195  dwarn:0   dfail:1   fail:2   skip:31 
ro-ilk-i7-620lm  total:229  pass:155  dwarn:0   dfail:1   fail:3   skip:70 
ro-ilk1-i5-650   total:224  pass:155  dwarn:0   dfail:1   fail:3   skip:65 
ro-skl3-i5-6260u total:229  pass:206  dwarn:1   dfail:1   fail:2   skip:19 
ro-snb-i7-2620M  total:229  pass:179  dwarn:0   dfail:1   fail:1   skip:48 
fi-hsw-i7-4770k failed to connect after reboot
fi-skl-i7-6700k failed to connect after reboot
ro-ivb2-i7-3770 failed to connect after reboot
ro-ivb-i7-3770 failed to connect after reboot

Results at /archive/results/CI_IGT_test/RO_Patchwork_1333/

63f6b6c drm-intel-nightly: 2016y-06m-29d-14h-53m-39s UTC integration manifest
538feac drm/i915/opregion: update cadl based on actually active outputs
2d1aff8 drm/i915: make i915 the source of acpi device ids for _DOD

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

Re: [PATCH v4 0/2] drm/i915/opregion: proper handling of DIDL and CADL

[Peter Wu]

Hi Jani,

I just tested your branch (2f9a317) and can confirm that the fix is still valid. evtest reports brightness up/down events.

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


On 29 June 2016 17:36:40 CEST, Jani Nikula <jani.nikula@intel.com> wrote:
>This is v4 of [1]. The first three have already been pushed to
>drm-intel-next-queued. The only change here is the atomic commit.
>
>Review and testing would be much appreciated to move this forward. For
>testing, I've pushed this to opregion-didl-v4 branch of my repo at [2].
>
>Maarten, please check the hunk touching the atomic code in patch 2.
>
>BR,
>Jani.
>
>
>[1] http://mid.gmane.org/cover.1465810007.git.jani.nikula@intel.com
>[2] https://cgit.freedesktop.org/~jani/drm/
>
>Jani Nikula (2):
>  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  |   4 +
> drivers/gpu/drm/i915/intel_drv.h      |   3 +
>drivers/gpu/drm/i915/intel_opregion.c | 155
>+++++++++++++---------------------
> 4 files changed, 69 insertions(+), 95 deletions(-)

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

Re: [PATCH v4 0/2] drm/i915/opregion: proper handling of DIDL and CADL

[Rainer Koenig]

Am 29.06.2016 um 17:36 schrieb Jani Nikula:
> This is v4 of [1]. The first three have already been pushed to
> drm-intel-next-queued. The only change here is the atomic commit.
>
> Review and testing would be much appreciated to move this forward. For
> testing, I've pushed this to opregion-didl-v4 branch of my repo at [2].
>
Tested on a Fujitsu LIFEBOOK E736: Brightness keys are working now.

Best regards
Rainer

-- 
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:Rainer.Koenig@ts.fujitsu.com 

Internet         ts.fujtsu.com
Company Details  ts.fujitsu.com/imprint.html

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

Re: [PATCH v4 0/2] drm/i915/opregion: proper handling of DIDL and CADL

[Rainer Koenig]

Found a problem: After screensaver kicked in and display was turned off
the brightness keys stop working.

Problem can be reproduced like that:

1. Boot laptop
2. Test brightness keys, they are working
3. open Terminal and issue "xset -display :0 dpms force off"
4. the screen goes blank (like after the screensaver timeout)
5. push a key to bring the screen back
6. test brightness keys again, now they don't work

If the system is sent to suspend and woken up everything is fine again.

Behaviour happens on the 4.7.0-rc5 kernel from the opregion-didl-v4 branch.
Before I compiled the 4.7.0-r4 from the same git repository. On this
(v3) everything still works after the screen was blanked.

Best regards
Rainer

Am 30.06.2016 um 11:19 schrieb Rainer Koenig:
> Am 29.06.2016 um 17:36 schrieb Jani Nikula:
>> This is v4 of [1]. The first three have already been pushed to
>> drm-intel-next-queued. The only change here is the atomic commit.
>>
>> Review and testing would be much appreciated to move this forward. For
>> testing, I've pushed this to opregion-didl-v4 branch of my repo at [2].
>>
> Tested on a Fujitsu LIFEBOOK E736: Brightness keys are working now.
>
> Best regards
> Rainer
>


-- 
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:Rainer.Koenig@ts.fujitsu.com 

Internet         ts.fujtsu.com
Company Details  ts.fujitsu.com/imprint.html

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

Re: [PATCH v4 0/2] drm/i915/opregion: proper handling of DIDL and CADL

[Jani Nikula]

On Fri, 01 Jul 2016, Rainer Koenig <Rainer.Koenig@ts.fujitsu.com> wrote:
> Found a problem: After screensaver kicked in and display was turned off
> the brightness keys stop working.
>
> Problem can be reproduced like that:
>
> 1. Boot laptop
> 2. Test brightness keys, they are working
> 3. open Terminal and issue "xset -display :0 dpms force off"
> 4. the screen goes blank (like after the screensaver timeout)
> 5. push a key to bring the screen back
> 6. test brightness keys again, now they don't work
>
> If the system is sent to suspend and woken up everything is fine again.
>
> Behaviour happens on the 4.7.0-rc5 kernel from the opregion-didl-v4 branch.
> Before I compiled the 4.7.0-r4 from the same git repository. On this
> (v3) everything still works after the screen was blanked.

Maarten, I think the difference is between where and when the calls to
cadl update are made.

BR,
Jani.


>
> Best regards
> Rainer
>
> Am 30.06.2016 um 11:19 schrieb Rainer Koenig:
>> Am 29.06.2016 um 17:36 schrieb Jani Nikula:
>>> This is v4 of [1]. The first three have already been pushed to
>>> drm-intel-next-queued. The only change here is the atomic commit.
>>>
>>> Review and testing would be much appreciated to move this forward. For
>>> testing, I've pushed this to opregion-didl-v4 branch of my repo at [2].
>>>
>> Tested on a Fujitsu LIFEBOOK E736: Brightness keys are working now.
>>
>> Best regards
>> Rainer
>>

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v4 2/2] drm/i915/opregion: update cadl based on actually active outputs

[Maarten Lankhorst]

Op 29-06-16 om 17:36 schreef 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.
>
> v3: actually git add the dev->dev_priv change.
>
> v4: update cadl in intel_shared_dpll_commit() if intel_state->modeset
>     (Maarten)
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Reviewed-and-tested-by: Peter Wu <peter@lekensteyn.nl>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h       |  2 +
>  drivers/gpu/drm/i915/intel_display.c  |  4 ++
>  drivers/gpu/drm/i915/intel_opregion.c | 70 ++++++++++++++++++-----------------
>  3 files changed, 43 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 724d34b00196..64ab52529be8 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3692,6 +3692,7 @@ extern int intel_opregion_notify_encoder(struct intel_encoder *intel_encoder,
>  extern int intel_opregion_notify_adapter(struct drm_i915_private *dev_priv,
>  					 pci_power_t state);
>  extern int intel_opregion_get_panel_type(struct drm_i915_private *dev_priv);
> +extern void intel_opregion_update_cadl(struct drm_i915_private *dev_priv);
>  #else
>  static inline int intel_opregion_setup(struct drm_i915_private *dev) { return 0; }
>  static inline void intel_opregion_register(struct drm_i915_private *dev_priv) { }
> @@ -3713,6 +3714,7 @@ static inline int intel_opregion_get_panel_type(struct drm_i915_private *dev)
>  {
>  	return -ENODEV;
>  }
> +static inline void intel_opregion_update_cadl(struct drm_i915_private *dev_priv) { }
>  #endif
>  
>  /* intel_acpi.c */
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d902a70edb84..4f404900f610 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -13953,6 +13953,10 @@ static int intel_atomic_commit(struct drm_device *dev,
>  	dev_priv->wm.distrust_bios_wm = false;
>  	dev_priv->wm.skl_results = intel_state->wm_results;
>  	intel_shared_dpll_commit(state);
> +
> +	if (intel_state->modeset)
> +		intel_opregion_update_cadl(dev_priv);
> +
>  	intel_atomic_track_fbs(state);
>  
>  	if (nonblock)
> diff --git a/drivers/gpu/drm/i915/intel_opregion.c b/drivers/gpu/drm/i915/intel_opregion.c
> index 632f0178c2b0..8b3f7e6ae4bb 100644
> --- a/drivers/gpu/drm/i915/intel_opregion.c
> +++ b/drivers/gpu/drm/i915/intel_opregion.c
> @@ -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)) {
> @@ -674,6 +656,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;
> @@ -759,22 +749,36 @@ static void intel_didl_outputs(struct drm_i915_private *dev_priv)
>  		set_did(opregion, i, 0);
>  }
>  
> -static void intel_setup_cadls(struct drm_i915_private *dev_priv)
> +/* Update CADL to reflect active outputs. */
> +void intel_opregion_update_cadl(struct drm_i915_private *dev_priv)
>  {
>  	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_priv->dev, crtc) {
> +		struct intel_encoder *encoder;
> +
> +		if (!crtc->active)
> +			continue;
Argh, don't use crtc->active!
We have intel_state->active_crtcs + for_each_intel_crtc_mask for this, it should fix the issue Koenig mentioned.

If fixed,

Reviewed-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

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

Re: [PATCH v4 0/2] drm/i915/opregion: proper handling of DIDL and CADL

[Maarten Lankhorst]

Op 01-07-16 om 15:51 schreef Jani Nikula:
> On Fri, 01 Jul 2016, Rainer Koenig <Rainer.Koenig@ts.fujitsu.com> wrote:
>> Found a problem: After screensaver kicked in and display was turned off
>> the brightness keys stop working.
>>
>> Problem can be reproduced like that:
>>
>> 1. Boot laptop
>> 2. Test brightness keys, they are working
>> 3. open Terminal and issue "xset -display :0 dpms force off"
>> 4. the screen goes blank (like after the screensaver timeout)
>> 5. push a key to bring the screen back
>> 6. test brightness keys again, now they don't work
>>
>> If the system is sent to suspend and woken up everything is fine again.
>>
>> Behaviour happens on the 4.7.0-rc5 kernel from the opregion-didl-v4 branch.
>> Before I compiled the 4.7.0-r4 from the same git repository. On this
>> (v3) everything still works after the screen was blanked.
> Maarten, I think the difference is between where and when the calls to
> cadl update are made.
Did you see my comment on patch 2?

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