2008-08-05 18:37:44

by Matthew Garrett

[permalink] [raw]
Subject: [PATCH] Add Intel ACPI IGD OpRegion support

This adds the support necessary for allowing ACPI backlight control to
work on some newer Intel-based graphics systems. Tested on Thinkpad T61
and HP 2510p hardware.

Signed-off-by: Matthew Garrett <[email protected]>

---

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index a9e6046..b032808 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -3,7 +3,7 @@
# Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.

ccflags-y := -Iinclude/drm
-i915-y := i915_drv.o i915_dma.o i915_irq.o i915_mem.o
+i915-y := i915_drv.o i915_dma.o i915_irq.o i915_mem.o i915_opregion.o

i915-$(CONFIG_COMPAT) += i915_ioc32.o

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index b3c4ac9..cead62f 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -810,6 +810,8 @@ 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);

return ret;
@@ -827,6 +829,8 @@ int i915_driver_unload(struct drm_device *dev)
if (dev_priv->mmio_map)
drm_rmmap(dev, dev_priv->mmio_map);

+ intel_opregion_free(dev);
+
drm_free(dev->dev_private, sizeof(drm_i915_private_t),
DRM_MEM_DRIVER);

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 6c99aab..d95eca2 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -371,6 +371,8 @@ static int i915_suspend(struct drm_device *dev, pm_message_t state)

i915_save_vga(dev);

+ intel_opregion_free(dev);
+
if (state.event == PM_EVENT_SUSPEND) {
/* Shut down the device */
pci_disable_device(dev->pdev);
@@ -532,6 +534,8 @@ static int i915_resume(struct drm_device *dev)

i915_restore_vga(dev);

+ intel_opregion_init(dev);
+
return 0;
}

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 8daf0d8..e4bd01c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -82,6 +82,14 @@ typedef struct _drm_i915_vbl_swap {
unsigned int sequence;
} drm_i915_vbl_swap_t;

+struct intel_opregion {
+ struct opregion_header *header;
+ struct opregion_acpi *acpi;
+ struct opregion_swsci *swsci;
+ struct opregion_asle *asle;
+ int enabled;
+};
+
typedef struct drm_i915_private {
drm_local_map_t *sarea;
drm_local_map_t *mmio_map;
@@ -122,6 +130,8 @@ typedef struct drm_i915_private {
drm_i915_vbl_swap_t vbl_swaps;
unsigned int swaps_pending;

+ struct intel_opregion opregion;
+
/* Register state */
u8 saveLBB;
u32 saveDSPACNTR;
@@ -244,6 +254,7 @@ extern int i915_vblank_pipe_get(struct drm_device *dev, void *data,
struct drm_file *file_priv);
extern int i915_vblank_swap(struct drm_device *dev, void *data,
struct drm_file *file_priv);
+extern void i915_enable_irq(drm_i915_private_t *dev_priv, u32 mask);

/* i915_mem.c */
extern int i915_mem_alloc(struct drm_device *dev, void *data,
@@ -258,6 +269,12 @@ extern void i915_mem_takedown(struct mem_block **heap);
extern void i915_mem_release(struct drm_device * dev,
struct drm_file *file_priv, struct mem_block *heap);

+/* i915_opregion.c */
+extern int intel_opregion_init(struct drm_device *dev);
+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);
+
#define I915_READ(reg) DRM_READ32(dev_priv->mmio_map, (reg))
#define I915_WRITE(reg,val) DRM_WRITE32(dev_priv->mmio_map, (reg), (val))
#define I915_READ16(reg) DRM_READ16(dev_priv->mmio_map, (reg))
diff --git a/drivers/gpu/drm/i915/i915_irq.c b/drivers/gpu/drm/i915/i915_irq.c
index 24d11ed..ae7d3a8 100644
--- a/drivers/gpu/drm/i915/i915_irq.c
+++ b/drivers/gpu/drm/i915/i915_irq.c
@@ -36,9 +36,11 @@
/** These are the interrupts used by the driver */
#define I915_INTERRUPT_ENABLE_MASK (I915_USER_INTERRUPT | \
I915_DISPLAY_PIPE_A_VBLANK_INTERRUPT | \
- I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT)
+ I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT | \
+ I915_ASLE_INTERRUPT | \
+ I915_DISPLAY_PIPE_B_EVENT_INTERRUPT)

-static inline void
+void
i915_enable_irq(drm_i915_private_t *dev_priv, u32 mask)
{
if ((dev_priv->irq_mask_reg & mask) != 0) {
@@ -274,6 +276,9 @@ irqreturn_t i915_driver_irq_handler(DRM_IRQ_ARGS)
return IRQ_NONE;
}

+ I915_WRITE(PIPEASTAT, pipea_stats);
+ I915_WRITE(PIPEBSTAT, pipeb_stats);
+
I915_WRITE(IIR, iir);
if (dev->pdev->msi_enabled)
I915_WRITE(IMR, dev_priv->irq_mask_reg);
@@ -306,14 +311,14 @@ irqreturn_t i915_driver_irq_handler(DRM_IRQ_ARGS)

if (dev_priv->swaps_pending > 0)
drm_locked_tasklet(dev, i915_vblank_tasklet);
- I915_WRITE(PIPEASTAT,
- pipea_stats|I915_VBLANK_INTERRUPT_ENABLE|
- PIPE_VBLANK_INTERRUPT_STATUS);
- I915_WRITE(PIPEBSTAT,
- pipeb_stats|I915_VBLANK_INTERRUPT_ENABLE|
- PIPE_VBLANK_INTERRUPT_STATUS);
}

+ if (iir & I915_ASLE_INTERRUPT)
+ opregion_asle_intr(dev);
+
+ if (iir & I915_DISPLAY_PIPE_B_EVENT_INTERRUPT)
+ opregion_asle_intr(dev);
+
return IRQ_HANDLED;
}

@@ -661,10 +666,14 @@ void i915_driver_irq_postinstall(struct drm_device * dev)
if (dev_priv->vblank_pipe & DRM_I915_VBLANK_PIPE_B)
dev_priv->irq_mask_reg &= ~I915_DISPLAY_PIPE_B_VBLANK_INTERRUPT;

+ dev_priv->irq_mask_reg &= I915_INTERRUPT_ENABLE_MASK;
+
I915_WRITE(IMR, dev_priv->irq_mask_reg);
I915_WRITE(IER, I915_INTERRUPT_ENABLE_MASK);
(void) I915_READ(IER);

+ opregion_enable_asle(dev);
+
DRM_INIT_WAITQUEUE(&dev_priv->irq_queue);
}

diff --git a/drivers/gpu/drm/i915/i915_opregion.c b/drivers/gpu/drm/i915/i915_opregion.c
new file mode 100644
index 0000000..1787a0c
--- /dev/null
+++ b/drivers/gpu/drm/i915/i915_opregion.c
@@ -0,0 +1,371 @@
+/*
+ * Copyright 2008 Intel Corporation <[email protected]>
+ * Copyright 2008 Red Hat <[email protected]>
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining
+ * a copy of this software and associated documentation files (the
+ * "Software"), to deal in the Software without restriction, including
+ * without limitation the rights to use, copy, modify, merge, publish,
+ * distribute, sub license, and/or sell copies of the Software, and to
+ * permit persons to whom the Software is furnished to do so, subject to
+ * the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the
+ * next paragraph) shall be included in all copies or substantial
+ * portions of the Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+ * EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+ * MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND
+ * NON-INFRINGEMENT. IN NO EVENT SHALL INTEL AND/OR ITS SUPPLIERS BE
+ * LIABLE FOR ANY CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN
+ * ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN
+ * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
+ * SOFTWARE.
+ *
+ */
+
+#include <linux/acpi.h>
+
+#include "drmP.h"
+#include "i915_drm.h"
+#include "i915_drv.h"
+
+#define PCI_ASLE 0xe4
+#define PCI_LBPC 0xf4
+#define PCI_ASLS 0xfc
+
+#define OPREGION_SZ (8*1024)
+#define OPREGION_HEADER_OFFSET 0
+#define OPREGION_ACPI_OFFSET 0x100
+#define OPREGION_SWSCI_OFFSET 0x200
+#define OPREGION_ASLE_OFFSET 0x300
+#define OPREGION_VBT_OFFSET 0x1000
+
+#define OPREGION_SIGNATURE "IntelGraphicsMem"
+#define MBOX_ACPI (1<<0)
+#define MBOX_SWSCI (1<<1)
+#define MBOX_ASLE (1<<2)
+
+struct opregion_header {
+ u8 signature[16];
+ u32 size;
+ u32 opregion_ver;
+ u8 bios_ver[32];
+ u8 vbios_ver[16];
+ u8 driver_ver[16];
+ u32 mboxes;
+ u8 reserved[164];
+} __attribute__((packed));
+
+/* OpRegion mailbox #1: public ACPI methods */
+struct opregion_acpi {
+ u32 drdy; /* driver readiness */
+ u32 csts; /* notification status */
+ u32 cevt; /* current event */
+ u8 rsvd1[20];
+ u32 didl[8]; /* supported display devices ID list */
+ u32 cpdl[8]; /* currently presented display list */
+ u32 cadl[8]; /* currently active display list */
+ u32 nadl[8]; /* next active devices list */
+ u32 aslp; /* ASL sleep time-out */
+ u32 tidx; /* toggle table index */
+ u32 chpd; /* current hotplug enable indicator */
+ u32 clid; /* current lid state*/
+ u32 cdck; /* current docking state */
+ u32 sxsw; /* Sx state resume */
+ u32 evts; /* ASL supported events */
+ u32 cnot; /* current OS notification */
+ u32 nrdy; /* driver status */
+ u8 rsvd2[60];
+} __attribute__((packed));
+
+/* OpRegion mailbox #2: SWSCI */
+struct opregion_swsci {
+ u32 scic; /* SWSCI command|status|data */
+ u32 parm; /* command parameters */
+ u32 dslp; /* driver sleep time-out */
+ u8 rsvd[244];
+} __attribute__((packed));
+
+/* OpRegion mailbox #3: ASLE */
+struct opregion_asle {
+ u32 ardy; /* driver readiness */
+ u32 aslc; /* ASLE interrupt command */
+ u32 tche; /* technology enabled indicator */
+ u32 alsi; /* current ALS illuminance reading */
+ u32 bclp; /* backlight brightness to set */
+ u32 pfit; /* panel fitting state */
+ u32 cblv; /* current brightness level */
+ u16 bclm[20]; /* backlight level duty cycle mapping table */
+ u32 cpfm; /* current panel fitting mode */
+ u32 epfm; /* enabled panel fitting modes */
+ u8 plut[74]; /* panel LUT and identifier */
+ u32 pfmb; /* PWM freq and min brightness */
+ u8 rsvd[102];
+} __attribute__((packed));
+
+/* ASLE irq request bits */
+#define ASLE_SET_ALS_ILLUM (1 << 0)
+#define ASLE_SET_BACKLIGHT (1 << 1)
+#define ASLE_SET_PFIT (1 << 2)
+#define ASLE_SET_PWM_FREQ (1 << 3)
+#define ASLE_REQ_MSK 0xf
+
+/* response bits of ASLE irq request */
+#define ASLE_ALS_ILLUM_FAIL (2<<10)
+#define ASLE_BACKLIGHT_FAIL (2<<12)
+#define ASLE_PFIT_FAIL (2<<14)
+#define ASLE_PWM_FREQ_FAIL (2<<16)
+
+/* ASLE backlight brightness to set */
+#define ASLE_BCLP_VALID (1<<31)
+#define ASLE_BCLP_MSK (~(1<<31))
+
+/* ASLE panel fitting request */
+#define ASLE_PFIT_VALID (1<<31)
+#define ASLE_PFIT_CENTER (1<<0)
+#define ASLE_PFIT_STRETCH_TEXT (1<<1)
+#define ASLE_PFIT_STRETCH_GFX (1<<2)
+
+/* PWM frequency and minimum brightness */
+#define ASLE_PFMB_BRIGHTNESS_MASK (0xff)
+#define ASLE_PFMB_BRIGHTNESS_VALID (1<<8)
+#define ASLE_PFMB_PWM_MASK (0x7ffffe00)
+#define ASLE_PFMB_PWM_VALID (1<<31)
+
+#define ASLE_CBLV_VALID (1<<31)
+
+static u32 asle_set_backlight(struct drm_device *dev, u32 bclp)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ struct opregion_asle *asle = dev_priv->opregion.asle;
+ u32 blc_pwm_ctl, blc_pwm_ctl2;
+
+ if (!(bclp & ASLE_BCLP_VALID))
+ return ASLE_BACKLIGHT_FAIL;
+
+ bclp &= ASLE_BCLP_MSK;
+ if (bclp < 0 || bclp > 255)
+ return ASLE_BACKLIGHT_FAIL;
+
+ blc_pwm_ctl = I915_READ(BLC_PWM_CTL);
+ blc_pwm_ctl &= ~BACKLIGHT_DUTY_CYCLE_MASK;
+ blc_pwm_ctl2 = I915_READ(BLC_PWM_CTL2);
+
+ if (blc_pwm_ctl2 & BLM_COMBINATION_MODE)
+ pci_write_config_dword(dev->pdev, PCI_LBPC, bclp);
+ else
+ I915_WRITE(BLC_PWM_CTL, blc_pwm_ctl | ((bclp * 0x101)-1));
+
+ asle->cblv = (bclp*0x64)/0xff | ASLE_CBLV_VALID;
+
+ return 0;
+}
+
+static u32 asle_set_als_illum(struct drm_device *dev, u32 alsi)
+{
+ /* alsi is the current ALS reading in lux. 0 indicates below sensor
+ range, 0xffff indicates above sensor range. 1-0xfffe are valid */
+ return 0;
+}
+
+static u32 asle_set_pwm_freq(struct drm_device *dev, u32 pfmb)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ if (pfmb & ASLE_PFMB_PWM_VALID) {
+ u32 blc_pwm_ctl = I915_READ(BLC_PWM_CTL);
+ u32 pwm = pfmb & ASLE_PFMB_PWM_MASK;
+ blc_pwm_ctl &= BACKLIGHT_DUTY_CYCLE_MASK;
+ pwm = pwm >> 9;
+ /* FIXME - what do we do with the PWM? */
+ }
+ return 0;
+}
+
+static u32 asle_set_pfit(struct drm_device *dev, u32 pfit)
+{
+ /* Panel fitting is currently controlled by the X code, so this is a
+ noop until modesetting support works fully */
+ if (!(pfit & ASLE_PFIT_VALID))
+ return ASLE_PFIT_FAIL;
+ return 0;
+}
+
+void opregion_asle_intr(struct drm_device *dev)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ struct opregion_asle *asle = dev_priv->opregion.asle;
+ u32 asle_stat = 0;
+ u32 asle_req;
+
+ if (!asle)
+ return;
+
+ asle_req = asle->aslc & ASLE_REQ_MSK;
+
+ if (!asle_req) {
+ DRM_DEBUG("non asle set request??\n");
+ return;
+ }
+
+ if (asle_req & ASLE_SET_ALS_ILLUM)
+ asle_stat |= asle_set_als_illum(dev, asle->alsi);
+
+ if (asle_req & ASLE_SET_BACKLIGHT)
+ asle_stat |= asle_set_backlight(dev, asle->bclp);
+
+ if (asle_req & ASLE_SET_PFIT)
+ asle_stat |= asle_set_pfit(dev, asle->pfit);
+
+ if (asle_req & ASLE_SET_PWM_FREQ)
+ asle_stat |= asle_set_pwm_freq(dev, asle->pfmb);
+
+ asle->aslc = asle_stat;
+}
+
+#define ASLE_ALS_EN (1<<0)
+#define ASLE_BLC_EN (1<<1)
+#define ASLE_PFIT_EN (1<<2)
+#define ASLE_PFMB_EN (1<<3)
+
+void opregion_enable_asle(struct drm_device *dev)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ struct opregion_asle *asle = dev_priv->opregion.asle;
+
+ if (asle) {
+ u32 pipeb_stats = I915_READ(PIPEBSTAT);
+ if (IS_MOBILE(dev)) {
+ /* Many devices trigger events with a write to the
+ legacy backlight controller, so we need to ensure
+ that it's able to generate interrupts */
+ I915_WRITE(PIPEBSTAT, pipeb_stats |=
+ I915_LEGACY_BLC_EVENT_ENABLE);
+ i915_enable_irq(dev_priv, I915_ASLE_INTERRUPT |
+ I915_DISPLAY_PIPE_B_EVENT_INTERRUPT);
+ } else
+ i915_enable_irq(dev_priv, I915_ASLE_INTERRUPT);
+
+ asle->tche = ASLE_ALS_EN | ASLE_BLC_EN | ASLE_PFIT_EN |
+ ASLE_PFMB_EN;
+ asle->ardy = 1;
+ }
+}
+
+#define ACPI_EV_DISPLAY_SWITCH (1<<0)
+#define ACPI_EV_LID (1<<1)
+#define ACPI_EV_DOCK (1<<2)
+
+static struct intel_opregion *system_opregion;
+
+int intel_opregion_video_event(struct notifier_block *nb, unsigned long val,
+ void *data)
+{
+ /* The only video events relevant to opregion are 0x80. These indicate
+ either a docking event, lid switch or display switch request. In
+ Linux, these are handled by the dock, button and video drivers.
+ We might want to fix the video driver to be opregion-aware in
+ future, but right now we just indicate to the firmware that the
+ request has been handled */
+
+ struct opregion_acpi *acpi;
+
+ if (!system_opregion)
+ return NOTIFY_DONE;
+
+ acpi = system_opregion->acpi;
+ acpi->csts = 0;
+
+ return NOTIFY_OK;
+}
+
+static struct notifier_block intel_opregion_notifier = {
+ .notifier_call = intel_opregion_video_event,
+};
+
+int intel_opregion_init(struct drm_device *dev)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ struct intel_opregion *opregion = &dev_priv->opregion;
+ void *base;
+ u32 asls, mboxes;
+ int err = 0;
+
+ pci_read_config_dword(dev->pdev, PCI_ASLS, &asls);
+ DRM_DEBUG("graphic opregion physical addr: 0x%x\n", asls);
+ if (asls == 0) {
+ DRM_DEBUG("ACPI OpRegion not supported!\n");
+ return -ENOTSUPP;
+ }
+
+ base = ioremap(asls, OPREGION_SZ);
+ if (!base)
+ return -ENOMEM;
+
+ opregion->header = base;
+ if (memcmp(opregion->header->signature, OPREGION_SIGNATURE, 16)) {
+ DRM_DEBUG("opregion signature mismatch\n");
+ err = -EINVAL;
+ goto err_out;
+ }
+
+ mboxes = opregion->header->mboxes;
+ if (mboxes & MBOX_ACPI) {
+ DRM_DEBUG("Public ACPI methods supported\n");
+ opregion->acpi = base + OPREGION_ACPI_OFFSET;
+ } else {
+ DRM_DEBUG("Public ACPI methods not supported\n");
+ err = -ENOTSUPP;
+ goto err_out;
+ }
+ opregion->enabled = 1;
+
+ if (mboxes & MBOX_SWSCI) {
+ DRM_DEBUG("SWSCI supported\n");
+ opregion->swsci = base + OPREGION_SWSCI_OFFSET;
+ }
+ if (mboxes & MBOX_ASLE) {
+ DRM_DEBUG("ASLE supported\n");
+ opregion->asle = base + OPREGION_ASLE_OFFSET;
+ }
+
+ /* Notify BIOS we are ready to handle ACPI video ext notifs.
+ * Right now, all the events are handled by the ACPI video module.
+ * We don't actually need to do anything with them. */
+ opregion->acpi->csts = 0;
+ opregion->acpi->drdy = 1;
+
+ system_opregion = opregion;
+ register_acpi_notifier(&intel_opregion_notifier);
+
+ return 0;
+
+err_out:
+ iounmap(opregion->header);
+ opregion->header = NULL;
+ return err;
+}
+
+void intel_opregion_free(struct drm_device *dev)
+{
+ struct drm_i915_private *dev_priv = dev->dev_private;
+ struct intel_opregion *opregion = &dev_priv->opregion;
+
+ if (!opregion->enabled)
+ return;
+
+ opregion->acpi->drdy = 0;
+
+ system_opregion = NULL;
+ unregister_acpi_notifier(&intel_opregion_notifier);
+
+ /* just clear all opregion memory pointers now */
+ iounmap(opregion->header);
+ opregion->header = NULL;
+ opregion->acpi = NULL;
+ opregion->swsci = NULL;
+ opregion->asle = NULL;
+
+ opregion->enabled = 0;
+}
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 477c64e..43ad2cb 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -740,6 +740,7 @@
#define BLC_PWM_CTL 0x61254
#define BACKLIGHT_MODULATION_FREQ_SHIFT (17)
#define BLC_PWM_CTL2 0x61250 /* 965+ only */
+#define BLM_COMBINATION_MODE (1 << 30)
/*
* This is the most significant 15 bits of the number of backlight cycles in a
* complete cycle of the modulated backlight control.

--
Matthew Garrett | [email protected]


2008-08-05 19:53:33

by Yves-Alexis Perez

[permalink] [raw]
Subject: Re: [PATCH] Add Intel ACPI IGD OpRegion support

On mar, 2008-08-05 at 19:37 +0100, Matthew Garrett wrote:
> This adds the support necessary for allowing ACPI backlight control
> to
> work on some newer Intel-based graphics systems. Tested on Thinkpad
> T61
> and HP 2510p hardware.

Ok, I managed to apply it against drm-2.6/drm-next but it won't apply
against linux-acpi-2.6/test and I'm not sure how hard it is to cherry
pick the needed stuff from drm to acpi.

I'll try to build the drm-2.6/drm-next + patch but I guess it won't be
enough to test brightness keys?

Cheersm
--
Yves-Alexis


Attachments:
signature.asc (197.00 B)
This is a digitally signed message part

2008-08-05 21:48:14

by Yves-Alexis Perez

[permalink] [raw]
Subject: Re: [PATCH] Add Intel ACPI IGD OpRegion support

On mar, 2008-08-05 at 21:44 +0200, Yves-Alexis Perez wrote:
> On mar, 2008-08-05 at 19:37 +0100, Matthew Garrett wrote:
> > This adds the support necessary for allowing ACPI backlight control
> > to
> > work on some newer Intel-based graphics systems. Tested on Thinkpad
> > T61
> > and HP 2510p hardware.
>
> Ok, I managed to apply it against drm-2.6/drm-next but it won't apply
> against linux-acpi-2.6/test and I'm not sure how hard it is to cherry
> pick the needed stuff from drm to acpi.
>
> I'll try to build the drm-2.6/drm-next + patch but I guess it won't be
> enough to test brightness keys?

Ok, I checked out drm-2.6/drm-next, created a local branch drm-next and
applied OpRegion.
Then I checked out linux-acpi-2.6/test, created a local branched, and
merged then my local drm-next into it.

Built fine, rebooted into single user (without using acpi_backlight=).
In single user, brightness keys still don't work.

In booted into X (with hal, powersaved, dbus etc. running), and then,
tada, brightness keys work. There is a 750ms (I guess) delay, and I
don't know who is really responsible, I guess X is. (and thus I guess
the opregion is the one needed, and X manages the brightness. If the
kernel was, it would be working even in single user)

Hope that can help understand the issues. Feel free to ask more info.

Cheers,
--
Yves-Alexis


Attachments:
signature.asc (197.00 B)
This is a digitally signed message part

2008-08-06 00:13:19

by Matthew Garrett

[permalink] [raw]
Subject: Re: [PATCH] Add Intel ACPI IGD OpRegion support

On Tue, Aug 05, 2008 at 11:47:54PM +0200, Yves-Alexis Perez wrote:

> In booted into X (with hal, powersaved, dbus etc. running), and then,
> tada, brightness keys work. There is a 750ms (I guess) delay, and I
> don't know who is really responsible, I guess X is. (and thus I guess
> the opregion is the one needed, and X manages the brightness. If the
> kernel was, it would be working even in single user)

The 750ms delay is from thinkpad-acpi. I sent a patch to Henrique which
makes it go away, but I'm not entirely sure what the ACPI method
concerned is supposed to be doing. The opregion code won't currently run
until X is started because the drm layer requires X to be the foreground
vt before handling IRQs.

--
Matthew Garrett | [email protected]

Subject: Re: [PATCH] Add Intel ACPI IGD OpRegion support

On Wed, 06 Aug 2008, Matthew Garrett wrote:
> On Tue, Aug 05, 2008 at 11:47:54PM +0200, Yves-Alexis Perez wrote:
> > In booted into X (with hal, powersaved, dbus etc. running), and then,
> > tada, brightness keys work. There is a 750ms (I guess) delay, and I
> > don't know who is really responsible, I guess X is. (and thus I guess
> > the opregion is the one needed, and X manages the brightness. If the
> > kernel was, it would be working even in single user)
>
> The 750ms delay is from thinkpad-acpi. I sent a patch to Henrique which
> makes it go away, but I'm not entirely sure what the ACPI method
> concerned is supposed to be doing. The opregion code won't currently run
> until X is started because the drm layer requires X to be the foreground
> vt before handling IRQs.

Well, for what is it worth, thinkpad-acpi has a knob (brightness_mode) which
can be used. Set it to CMOS mode (see docs). From what I recall, it should
do what your patch does.

--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh

2008-08-06 05:44:55

by Yves-Alexis Perez

[permalink] [raw]
Subject: Re: [PATCH] Add Intel ACPI IGD OpRegion support

On mar, 2008-08-05 at 23:32 -0300, Henrique de Moraes Holschuh wrote:
> Well, for what is it worth, thinkpad-acpi has a knob (brightness_mode)
> which
> can be used. Set it to CMOS mode (see docs). From what I recall, it
> should
> do what your patch does.

It's worth noting that I get the 750 ms delay even when I reload
thinkpad-acpi with brightness_mode=2 it got it (but maybe I should
reboot before to be sure no one messed with EC).
What's strange too is that brightness keys work fine when thinkpad-acpi
isn't loaded, so everything seems done in video.ko.

Cheers,
--
Yves-Alexis


Attachments:
signature.asc (197.00 B)
This is a digitally signed message part

2008-08-06 06:34:58

by Matthew Garrett

[permalink] [raw]
Subject: Re: [PATCH] Add Intel ACPI IGD OpRegion support

On Tue, Aug 05, 2008 at 11:32:48PM -0300, Henrique de Moraes Holschuh wrote:
> On Wed, 06 Aug 2008, Matthew Garrett wrote:
> > The 750ms delay is from thinkpad-acpi. I sent a patch to Henrique which
> > makes it go away, but I'm not entirely sure what the ACPI method
> > concerned is supposed to be doing. The opregion code won't currently run
> > until X is started because the drm layer requires X to be the foreground
> > vt before handling IRQs.
>
> Well, for what is it worth, thinkpad-acpi has a knob (brightness_mode) which
> can be used. Set it to CMOS mode (see docs). From what I recall, it should
> do what your patch does.

It doesn't seem to, no. I should have been clearer - the delay is in the
DSDT (not thinkpad-acpi itself), but there's a Thinkpad-specific ACPI
call that seems to be needed in order to delay it. Here's the patch
again.

diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
index b596929..bbc45c8 100644
--- a/drivers/misc/thinkpad_acpi.c
+++ b/drivers/misc/thinkpad_acpi.c
@@ -899,6 +899,9 @@ static int __init tpacpi_check_std_acpi_brightness_support(void)

if (ACPI_SUCCESS(status) && bcl_levels > 2) {
tp_features.bright_acpimode = 1;
+ /* Set ACPI mode */
+ if (!acpi_evalf(hkey_handle, NULL, "PWMS", "vd", 0))
+ printk(TPACPI_INFO "Failed to claim backlight\n");
return (bcl_levels - 2);
}

--
Matthew Garrett | [email protected]

2008-08-06 06:50:40

by Yves-Alexis Perez

[permalink] [raw]
Subject: Re: [PATCH] Add Intel ACPI IGD OpRegion support

On mer, 2008-08-06 at 07:34 +0100, Matthew Garrett wrote:
> It doesn't seem to, no. I should have been clearer - the delay is in
> the
> DSDT (not thinkpad-acpi itself), but there's a Thinkpad-specific ACPI
> call that seems to be needed in order to delay it. Here's the patch
> again.

Woah, it works perfectly fine here. Thanks :)

Cheers,
--
Yves-Alexis


Attachments:
signature.asc (197.00 B)
This is a digitally signed message part
Subject: Re: [PATCH] Add Intel ACPI IGD OpRegion support

Hi Matthew!

On Wed, 06 Aug 2008, Matthew Garrett wrote:

> On Tue, Aug 05, 2008 at 11:32:48PM -0300, Henrique de Moraes Holschuh wrote:
> > On Wed, 06 Aug 2008, Matthew Garrett wrote:
> > > The 750ms delay is from thinkpad-acpi. I sent a patch to Henrique which
> > > makes it go away, but I'm not entirely sure what the ACPI method
> > > concerned is supposed to be doing. The opregion code won't currently run
> > > until X is started because the drm layer requires X to be the foreground
> > > vt before handling IRQs.
> >
> > Well, for what is it worth, thinkpad-acpi has a knob (brightness_mode) which
> > can be used. Set it to CMOS mode (see docs). From what I recall, it should
> > do what your patch does.
>
> It doesn't seem to, no. I should have been clearer - the delay is in the
> DSDT (not thinkpad-acpi itself), but there's a Thinkpad-specific ACPI
> call that seems to be needed in order to delay it. Here's the patch
> again.
>
> diff --git a/drivers/misc/thinkpad_acpi.c b/drivers/misc/thinkpad_acpi.c
> index b596929..bbc45c8 100644
> --- a/drivers/misc/thinkpad_acpi.c
> +++ b/drivers/misc/thinkpad_acpi.c
> @@ -899,6 +899,9 @@ static int __init tpacpi_check_std_acpi_brightness_support(void)
>
> if (ACPI_SUCCESS(status) && bcl_levels > 2) {
> tp_features.bright_acpimode = 1;
> + /* Set ACPI mode */
> + if (!acpi_evalf(hkey_handle, NULL, "PWMS", "vd", 0))
> + printk(TPACPI_INFO "Failed to claim backlight\n");
> return (bcl_levels - 2);
> }

Ah, THIS patch. Yes, a more intelligent version of it (that doesn't scream
blood murder on thinkpads without PWMS) needs to go into thinkpad-acpi
along with your patch.

--
"One disk to rule them all, One disk to find them. One disk to bring
them all and in the darkness grind them. In the Land of Redmond
where the shadows lie." -- The Silicon Valley Tarot
Henrique Holschuh