Skip to content

[display] Fix HBM and make it's path configurable#41

Open
Michal-Szczepaniak wants to merge 1 commit intosailfishos:masterfrom
Michal-Szczepaniak:hbm-fix
Open

[display] Fix HBM and make it's path configurable#41
Michal-Szczepaniak wants to merge 1 commit intosailfishos:masterfrom
Michal-Szczepaniak:hbm-fix

Conversation

@Michal-Szczepaniak
Copy link
Copy Markdown

So, I discovered that my phone has HBM and that HBM is a thing and that MCE has HBM support but it doesn't work. So first I've investigated the mdy_display_type_get method and my phone lands on the mdy_display_type_get_from_sysfs_probe method. Path /sys/class/graphics/fb0/device/panel doesn't exist and /sys/class/backlight is just empty. So then it gets brightness and max_brightness value but never gets HBM path which does exist in /sys/class/graphics/fb0/hbm so device/panel was close. So in mdy_display_type_get_from_config I added fetching the config for HBM and enabling it (if its wrong place I can move it) and I thought that would be it! But sadly, it wasn't.

So the method that decides whether or not to enable hbm is here https://github.com/sailfishos/mce/blob/master/modules/display.c#L3705 it takes first 8 bits as brightness and other 8 bits as hbm? Now that is very strange because at 255 it will be max brightness and no hbm but at 256 it will enable hbm and set brightness to 0? That makes no sense. And if it goes any higher HBM won't accept input as it only accepts 0/1. But that's not the only strange thing. Another one is that the input is CAPPED AT 100 https://github.com/sailfishos/mce/blob/master/modules/display.c#L11726 so HBM won't ever be enabled!

Maybe its legacy code I don't know but I've fixed that too and now it works fine

@Michal-Szczepaniak Michal-Szczepaniak force-pushed the hbm-fix branch 7 times, most recently from cb8a8a9 to f732e83 Compare June 14, 2025 23:54
@Michal-Szczepaniak
Copy link
Copy Markdown
Author

Ping? Can I have any review on this?

@pvuorela pvuorela requested a review from spiiroin August 4, 2025 06:50
Copy link
Copy Markdown
Contributor

@spiiroin spiiroin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using config to override hbm control path looks ok.

Changing interpretation of "hbm_and_level" value is not ok.

Comment thread modules/display.c Outdated
gint new_brightness = (hbm_and_level >> 0) & 0xff;
gint new_hbm_level = (hbm_and_level >> 8) & 0xff;
gint new_brightness = hbm_and_level;
gint new_hbm_level = hbm_and_level == 100;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change goes against design (excerpt from datapipe.c)

Display brightness;
bits 0-7 is brightness in percent (0-100)
upper 8 bits is high brightness boost (0-2)

AFAIK, IIRC, etc the idea is basically that

  1. hbm is triggered by automatic brightness tuning
  2. duration is limited even if conditions persist
  3. you can't choose hbm from settings (i.e. having 100 trigger hbm is not ok)

So, instead of code change like this you should have a custom als configuration file where high lux values yield brightness value like 356 instead of 100 and install it as e.g. "/etc/mce/60-als-devicename.ini" so that it overrides defaults originating from /etc/mce/20als-defaults.ini.

Copy link
Copy Markdown
Author

@Michal-Szczepaniak Michal-Szczepaniak Aug 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah limiting the input to 100 was very confusing to me so I'll check out what you wrote and see how it goes

But also from my testing on 2 different devices, HBM is just 0-1, writting 2 yields no difference

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spiiroin I did the changes but I couldn't figure out the als values to get it to work reliably. I can force it if I put 500 in every spot but that's not really a solution and I can't figure out how it works, so could you suggest sample config?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spiiroin bump

Comment thread modules/display.c Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants