Setting backlight brightness doesn't work.

[pulkomandy]

The shortcut (function key + home/end) used to work fine, now they don't. My BIOS defaults to low brightness so it's quite annoying.

I think this went in in r41xxx. Will try to affine that if I get some time.

Anything I can do to help with it ?

[luroh]

I confirm, disabling IO-APIC makes the brightness buttons work again on my T60 laptop.

[mmlr]

Not an IO-APIC problem. It just means now that the system is in real ACPI mode, it'll require ACPI brightness controls. I think we have some kind of driver concerning that already, don't know in what state it is though.

[tqh]

Was ACPI-module loaded fine and backlight working before? If it was it sounds like ACPI or its embedded controller handler are not setup correctly. A guess would be that something is not right when it is setup before the ioapic int controller. I'll provide a patch that is a big hack (only apply if you can run with ioapic atm) to see if setting up ACPI with the ioapic controller works better. Please test.

[tqh]

A crude patch that sets arch_int_set_interrupt_controller(ioapicController) before acpi is initialised.

[luroh]

Not sure how I'd check for a successfully loaded ACPI module, but yes, backlight adjustment worked fine before and with your patch it does so again.

[pulkomandy]

Works with the patch here too. (also a T60, FWIW).

[mmlr]

This patch can't possibly work correctly. What happens now is that the SCI is installed incorrectly and/or not enabled anymore as the interrupt setup will run into the void (i.e. non-setup IO-APIC). I've implemented legacy PIC to IO-APIC handover as well as interrupt source overrides, so the SCI should be routed and enabled in all cases. Maybe that's actually the "problem" here, that with a working SCI the OS is now responsible for these brightness settings (which is likely not implemented).

[mmlr]

Please try the KDL "ints" command after pushing the brightness controls both with and without this patch. If they are implemented through the SCI then you might see that in the handled/unhandled count of the corresponding handler (if it is set up correctly in either case).

[tqh]

Perhaps a patch with just _PIC = 1 before installing the int handlers works just as well.

I think the issue here is that _PIC = 0 and _PIC = 1 can be completly different configurations in the vendors ACPI impl, so migrating the interrupt handlers over isn't enough, you probably need to reinit the handlers or modify how ACPI and embedded controller init or install their handlers so that they do that with _PIC set correctly.

[mmlr]

Replying to tqh:

Perhaps a patch with just _PIC = 1 before installing the int handlers works just as well.

I think the issue here is that _PIC = 0 and _PIC = 1 can be completly different configurations in the vendors ACPI impl, so migrating the interrupt handlers over isn't enough, you probably need to reinit the handlers or modify how ACPI and embedded controller init or install their handlers so that they do that with _PIC set correctly.

The problem is that if this was the case we wouldn't be able to fall back to the other PIC model, we either go full on IO-APIC or we don't, only depending on the IO-APIC presence and IO-APIC safemode setting.

Referencing FreeBSD though, that can't really be it. Their acpi_attach method...

​http://fxr.watson.org/fxr/source/dev/acpica/acpi.c#L431

... has both, the call to AcpiEnableSubsystem() ...

​http://fxr.watson.org/fxr/source/dev/acpica/acpi.c#L521

... which in turn installs the SCI interrupt handler ...

​http://fxr.watson.org/fxr/source/contrib/dev/acpica/utilities/utxface.c#L244

... and the _PIC setting through the call to acpi_machdep_init() ...

​http://fxr.watson.org/fxr/source/dev/acpica/acpi.c#L661

... which in turn calls acpi_SetIntrModel() ...

​http://fxr.watson.org/fxr/source/dev/acpica/acpi.c#L2322

... with the "default" interrupt model set up on MADT parsing. But if you look at the sequence of things here, the AcpiEnableSubsystem() (called with flags 0, so really installing the interrupt handler) happens far earlier than setting the _PIC model. The model to be used may be decided earlier, but it really only is set after the installation of the SCI.

The more likely scenario IMO is that the interrupts are simply broken due to either the override mechanism not working correctly (I was only able to test it using the PIT, so quite possibly there's a bug in the SCI case), or the source override that configures the SCI is bogus (seems to happen according to other implementations). FreeBSD forces the SCI to a level low interrupt (as described in the ACPI specs) whereas we currently rely on the interrupt soruce override. If the source override is simply broken, that'd explain things for example. So can the affected people please add a syslog to this ticket to see what configuration the SCI would result in and what routing table is in effect. Maybe forcing the SCI to level low would be enough. It's ugly in either case, since the code configuring the interrupts doesn't actually know the SCI, but the kernel and acpi module can work that out by the acpi module explicitly configuring the interrupt line after some kind of "interrupt config done"-callback.

[luroh]

Observing the output of the "ints" command didn't yield much in my case, too many numbers fluctuating with or without the patch to make any sense to me. Attaching syslog from hrev41667 (without patch).

[mmlr]

Replying to luroh:

Observing the output of the "ints" command didn't yield much in my case, too many numbers fluctuating with or without the patch to make any sense to me. Attaching syslog from hrev41667 (without patch).

The source override in that syslog looks fine, so no non-standard config there. Can you get a syslog with this patch applied or just with IO-APICs disabled? I wonder if this error is there in that case as well:

ACPI Error: Method parse/execution failed [\_SB_.PCI0.LPC_.EC__._INI] (Node 0x82193648), AE_NOT_EXIST (20101013/psparse-633)

[luroh]

Attaching syslog, IO-APIC disabled (brightness buttons working).

[mmlr]

So the error is there in either case...

Replying to luroh:

Observing the output of the "ints" command didn't yield much in my case, too many numbers fluctuating with or without the patch to make any sense to me.

The only thing I'm interested in is whether or not there are any handled and/or unhandled interrupts on int 9, where the ACPI interrupt handler should be installed. The thing is that the polarity and trigger mode were previously unset when only using the PIC (and when applying this patch, as the info will get lost because there's no IO-APIC structure to hold it). It's therefore possible that ACPI SCIs simply didn't work before and therefore SMIs were generated that the BIOS then handled (adjusting the brigthness for example). With IO-APIC support on and with the source override in place that, in your case, specifies the SCI interrupt to 9, active high, level triggered it might actually work now, shutting out the BIOS and therefore leaving the buttons to the OS.

What you could try to see if the source override is the "problem" would be to comment out the line:

acpi_configure_source_overrides(madt);

In browser:haiku/trunk/src/system/kernel/arch/x86/ioapic.cpp#L743 and see if that changes things.

I have checked FreeBSD again and they only force the SCI to level/low if there is no interrupt source override or if the user explicitly demanded it via settings. So I don't think doing the configuration this way is wrong per se.

[luroh]

Attaching photos of "ints", having pressed the brightness buttons a few times, with and without IO-APIC. I'll comment out that line next as suggested and revert.

[luroh]

Sorry, need more directions, getting a build failure: src/system/kernel/arch/x86/ioapic.cpp:509: warning: `void acpi_configure_source_overrides(acpi_table_madt *)' defined but not used

[mmlr]

Replying to luroh:

Sorry, need more directions, getting a build failure: src/system/kernel/arch/x86/ioapic.cpp:509: warning: `void acpi_configure_source_overrides(acpi_table_madt *)' defined but not used

Wrap the whole acpi_configure_source_overrides() function into an #if 0 block. To do that put #if 0 at line 506 and an #endif at line 605. Alternatively you could just delete the whole function.

[luroh]

Thanks, now I'm getting: src/system/kernel/arch/x86/ioapic.cpp:387: warning: `int32 ioapic_source_override_handler(void *)' defined but not used

[mmlr]

Replying to luroh:

Thanks, now I'm getting: src/system/kernel/arch/x86/ioapic.cpp:387: warning: `int32 ioapic_source_override_handler(void *)' defined but not used

Then remove that function as well. It starts at line 385.

[luroh]

Ok, it now built successfully but the removed lines had seemingly no effect on the brightness adjustments or the handled/unhandled interrupts of int 9, with or without IO-APIC.

[mmlr]

Can you please retry with hrev41685? It most probably fixes this. The ACPI SCI was simply not enabled because I messed up the handover.

[luroh]

Indeed, that seems to have fixed it. Attaching syslog. Again, nice work!

[mmlr]

Great. Thanks for the testing effort! Fixed in hrev41685.

[dukzcry]

In addition to mmlr's right comment regarding ACPI mode, one should remember that there are some vendors which produce laptops without support of generic ACPI methods, using instead proprietary ones. In such case you'll need to write platform-specific driver, like i did for Fujitsu-Siemens notebooks: ​https://github.com/druga/haiku-stuff/tree/master/acpi_fujitsu_laptop .

[tqh]

dukzcry, nice stuff. I'm happy someone else does a driver using ACPI and it also shows that it is quite simple. Well done. If I get the time I will take a closer look and move it over to Haiku if you don't mind.

[dukzcry]

Great to hear that, tqh. But it's a draft in it's current form. Things to do, in descending order: 1) figure out what's wrong with HID names reading; 2) extend it to support alternative methods (SBL2 vs. SBLL, GBLS vs. GBLL) to cover more FS machines; 3) do general code cleanup (remove hacks at first) and formatting, adjust it to follow HCG.