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@xxxxxxxxx>
> 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@xxxxxxxxx>
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@xxxxxxxxxxxxx>
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@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
