[Suggestions] [Experimental] Ice Lake backlight fix.

[ghost]

Related Issues:

• Ice Lake Brightness bugtracker#1256

Tested on Surface Laptop Go.

Short explanations:

• CFL: wrapCFLWriteRegisterPWMFreq1 -> wrapCFLWriteRegisterPWMDuty1
• ICL: wrapCFLWriteRegisterPWMDuty1 -> LightUpEDP (might not be called at startup) -> wrapCFLWriteRegisterPWMFreq1 -> ...

[vit9696]

CC @0xFireWolf @usr-sse2

[0xFireWolf]

It took me a while to understand what you are doing here.
It would be greatly appreciated if you elaborate on the changes you have made in your pull request. : )

So let's begin with a quick recap on the backlight issue on Ice Lake platforms.
Previously, the revised Backlight Register Fix (BLR) submodule (#78) added support for ICL platforms to avoid the 3-minute black screen but did not solve the issue that the maximum brightness under macOS was lower than that under other operating systems on some laptops. As far as I know, some Lenovo laptops have this issue.

I assume that your code snippet that sets Apple's hard-coded frequency value is extracted from the AppleIntelFramebufferController::start() function. The code snippet is not new as it is in the CFL driver as well. In the start() function, Apple's driver chooses which frequency to use based upon whether the bit SFUSE_STRAP_RAW_FREQUENCY = (1 << 8) is set in the register SFUSE_STRAP at 0xc2014, and it then stores the frequency to an instance variable of the controller. Later, the driver fetches the stored frequency and writes it to the register BXT_BLC_PWM_FREQ1 at 0xC8254 in the LightUpEDP() function. However, depending upon the hardware CRTC state set by the firmware, the driver may complete the modeset without invoking LightUpEDP() at boot time. As a result, when a user requests to change the brightness, WEG may not scale the value of the duty register.

So that's my interpretation of the change you have made. You basically sets the frequency manually if the driver does not invoke the light up function before changing the brightness level. Please feel free to correct me if my interpretation is wrong.

Now I would like to confirm the following things.

• Did you see the line BLR: [CFL+] WriteRegister32<BXT_BLC_PWM_DUTY1>: Write PWM_DUTY1 has zero frequency driver in the kernel log without your changes?
• Does this change fix the low brightness issue on ALL ICL laptops?
• I am not sure whether we need this change for CFL platforms as well.

Perhaps, we need to invite people who own ICL laptops to test the patch before merging it to the main branch.
Incidentally, please add the register and the bit field definition in kern_igfx_backlight.hpp instead of using them as magic values in the code.

[ghost]

@0xFireWolf

It took me a while to understand what you are doing here.

It would be greatly appreciated if you elaborate on the changes you have made in your pull request. : )

Your following explanations are correct in fact, and sorry for the inconvenience.

I assume that your code snippet that sets Apple's hard-coded frequency value is extracted from the AppleIntelFramebufferController::start() function.

Yes. But I'm not sure filling the same value as Apple did is reasonable.

So that's my interpretation of the change you have made. You basically sets the frequency manually if the driver does not invoke the light up function before changing the brightness level. Please feel free to correct me if my interpretation is wrong.

Nothing needs to be corrected.

Did you see the line BLR: [CFL+] WriteRegister32<BXT_BLC_PWM_DUTY1>: Write PWM_DUTY1 has zero frequency driver in the kernel log without your changes?

Yes.

Does this change fix the low brightness issue on ALL ICL laptops?

I cannot confirm it, for the shortage of ICL laptops.

I am not sure whether we need this change for CFL platforms as well.

The driver of CFL will never match value && callbackIGFX->modBacklightRegistersFix.driverBacklightFrequency == 0 so I thought we can keep that if-block for both.

But I'm still not sure.

Perhaps, we need to invite people who own ICL laptops to test the patch before merging it to the main branch.

The PR was only a suggestion and it was welcomed to be discussed more further.

Incidentally, please add the register and the bit field definition in kern_igfx_backlight.hpp instead of using them as magic values in the code.

OK, I did it.

[0xFireWolf]

Yes. But I'm not sure filling the same value as Apple did is reasonable.

It looks fine to me, because WEG needs the frequency requested by Apple's driver to rescale the value of the duty register.

Did you see the line BLR: [CFL+] WriteRegister32<BXT_BLC_PWM_DUTY1>: Write PWM_DUTY1 has zero frequency driver in the kernel log without your changes?

Yes.

As expected, otherwise it does not make sense.

Does this change fix the low brightness issue on ALL ICL laptops?

I cannot confirm it, for the shortage of ICL laptops.

No worries. I mean it would be better to invite other people to test your patch as well.

I am not sure whether we need this change for CFL platforms as well.

The driver of CFL will never match value && callbackIGFX->modBacklightRegistersFix.driverBacklightFrequency == 0 so I thought we can keep that if-block for both.
But I'm still not sure.

Well, I think it does not hurt if we keep that for CFL as well, because the CFL driver uses the same hard-coded frequencies and it is possible that LightUpEDP() is not invoked on some CFL laptops at boot.

@vit9696 This fix makes sense to me, and I suggest to merge it, but it would be better if we can get some feedback from other folks. What's your suggestion?

[vit9696]

I believe we should merge this too and wait for the reports. If anyone gets issues with it, I will CC. Perhaps we can make it configurable at worst.

[0xFireWolf]

I believe we should merge this too and wait for the reports. If anyone gets issues with it, I will CC. Perhaps we can make it configurable at worst.

Sounds like a good plan.
We can add a boot argument to enable this fix and leave it there for a month or two.
If everything goes well, we then remove the boot argument from the future releases.

[ghost]

@0xFireWolf That’s nice. I’ll do that later. (Adding -igfxblrexpr)

[vit9696]

Nah, better enable by default. And then disable on reports. Otherwise nobody will enable it.

[ghost]

@vit9696 @0xFireWolf Changes were made. Now it can be merged.

[vit9696]

Errng, like I said, no need for an argument right now. Better introduce it in a subsequent commit if there are issues. It will require a device property check as well anyway.

[Lorys89]

ice lake users can run tests on chat gitter https://gitter.im/ICE-LAKE-HACKINTOSH-DEVELOPMENT/community

[ghost]

I thought similarly to @0xFireWolf that we need more usage reports before making it as a default behavior.

[vit9696]

You will not get usage reports from the people that do not need this flag, because they have no reasons to set it. And we need the opinions of these users most to determine whether this flag can be enabled by default. There is no better way to get this opinion other than force them having this feature and then wait for issues.

[ghost]

The argument was removed now.

[Lorys89]

The argument was removed now.

in my ice lake DELL laptop, there is no change or improvement,it worked before, and it works now.

hopefully on other laptops it will solve the problems.

[CobanRamo]

I can confirm that on my Lenovo S340 IceLake (81VW00C4GE) it finally solved the problem with dimmed backlight. The backlight was only after the sleep fully on Windows level.
The latest Whatevergreen works fine for me with Catalina, BigSur and Monterey.

[Lorys89]

@0xFireWolf I wanted to ask you a question, I use ig platform 0200518a and everything works fine, if I use 0200538a comes to black screen desk, but ariva safe to desk pouche if I turn up and down the volume I hear the sounds of macos, ig platform 0200538a is the one used natively by mbp 16.2
your opinion about it?

[Core-i99]

@0xFireWolf I wanted to ask you a question, I use ig platform 0200518a and everything works fine, if I use 0200538a comes to black screen desk, but ariva safe to desk pouche if I turn up and down the volume I hear the sounds of macos, ig platform 0200538a is the one used natively by mbp 16.2
your opinion about it?

Different platform id's have different connectors, busids, flags, etc. It's pretty much normal one will work and another one doesn't.