[PATCH] ACPI/Intel: Rework Opregion support

Indan Zupancic indan at nul.nu
Tue Mar 15 04:13:11 PDT 2011

On Tue, March 15, 2011 09:37, Chris Wilson wrote:
> On Tue, 15 Mar 2011 02:18:02 +0100 (CET), "Indan Zupancic" <indan at nul.nu> wrote:
>> Hello,
>> Some nitpicks below.
>> On Mon, March 14, 2011 18:59, Chris Wilson wrote:
>> > Note: neither the opregion_dev interface or the alse_set_* properly report
>> > failures. As such we have a slight change in behaviour on Ironlake+
>> > platforms and an uncorrected bug for earlier chipsets.
>> > -Chris
>> What uncorrected bug?
> For later chipsets we currently report the failure to respond to the ALSE
> requests, for earlier we do not. The patch harmonises the two code paths
> reusing the earlier code for later chipsets, hence the change in behaviour
> and potential regression. Alternatively, actually reporting the failure
> for earlier chipsets may also break existing setups.

Hopefully this change doesn't cause regressions, that would be sad.

>> And are there earlier chipsets with ASLE support at all, besides gen 4?
>> If there are no gen 2 or gen 3 chipsets with ASLE then the backlight
>> code can be simplified further.
> The OpRegion interface was devised midway through gen3 (afaik), and you
> find it on some i915-class hw and not others. In theory, there is nothing
> to prevent a BIOS/ACPI from being rewritten for it to be of use in gen2,
> and who knows one such beast may already exist (considering much to our
> horror you can still buy gen2 chipsets).

Pity, if they're still sold any bets are off.

>> > diff --git a/drivers/gpu/drm/i915/intel_panel.c b/drivers/gpu/drm/i915/intel_panel.c
>> > index 4e5ff59..51565bb 100644
>> > --- a/drivers/gpu/drm/i915/intel_panel.c
>> > +++ b/drivers/gpu/drm/i915/intel_panel.c
>> > @@ -261,8 +261,8 @@ intel_panel_detect(struct drm_device *dev)
>> >  	 * appears to be either the BIOS or Linux ACPI fault */
>> >  #if 0
>> >  	/* Assume that the BIOS does not lie through the OpRegion... */
>> > -	if (dev_priv->opregion.lid_state)
>> > -		return ioread32(dev_priv->opregion.lid_state) & 0x1 ?
>> > +	if (dev_priv->opregion_dev.opregion.acpi)
>> > +		return ioread32(&dev_priv->opregion_dev.opregion.acpi->clid) & 0x1 ?
>> What guarantees that opregion.acpi != NULL here?
> You mean other than the explicit test for opregion.acpi != NULL?

I'm blind. I checked all the rest of the code, but not the line just above it.

>> Or perhaps just remove that #if 0 code chunk altogether?
> Read the changelog and thread on the patch that disabled this logic, the

I just subscribed to intel-gfx, seemed like a good idea after the reject.

> failure (or at least inconsistent behaviour with the expectations of the
> HP BIOS authors) appears to be in how we initialise ACPI on the HP
> machines that causes the initial value of lid state to be incorrect. Since
> one of the laptops that Dave tests drm-next on is a HP, he was bitten by
> the bug and temporarily (we hope) disabled the logic. Or else once again,
> we will continue to light up the panel on a closed laptop.

Hopefully it's fixed by the next BIOS upgrade by HP...

Everything would be a lot simpler if the BIOSes were open source.

It's shocking with what you guys have to deal with.

Good luck and thanks for all the hard work!


More information about the dri-devel mailing list