Created attachment 30756 [details] [review] ACPI backlight support for FreeBSD Currently ACPI backlight control is only implemented for Linux. The attached patch adds support for FreeBSD via acpi_video(4).
Created attachment 30757 [details] [review] ACPI backlight support for FreeBSD (revised) Please ignore the previous one.
Wouldn't it be better to have: ... for (i = 0; i < num_levels - 1; i++) if (levels[i] <= *val && levels[i+1] > *val) break; if (i == num_level && levels[i] != *val) i++; free(levels); if (i >= num_levels) return FALSE; ... Also, don't you have to normalize the range to what the driver uses (0...RHD_BACKLIGHT_PROPERTY_MAX)? Could you please look into this? I'll reassign the ticket to you. Also I'm going to commit your patch.
As DragonFly BSD uses the very same acpi_video(4) code from FreeBSD, please add "|| defined(__DragonFly__)" into relevant ifdefs. Thanks. But I also have a question - is it guaranteed that it's the lcd0 entry that user has to change? Shouldn't it test for active device at first (hw.acpi.video.lcdX.active)? I have seen at least sysctl outputs with several lcd output entries. Although it might make sense to fallback to lcd0 probably because I've also seen variants like "several lcd entries, but none of them is active (in reality it's active, but the relevant sysctl is 0)" and "several lcd entries and all entries are active". I don't remember whether these are relevant to radeons though.
(In reply to comment #2) > Wouldn't it be better to have: > ... > for (i = 0; i < num_levels - 1; i++) > if (levels[i] <= *val && levels[i+1] > *val) > break; > if (i == num_level && levels[i] != *val) > i++; > free(levels); > if (i >= num_levels) > return FALSE; > ... > > Also, don't you have to normalize the range to what the driver uses > (0...RHD_BACKLIGHT_PROPERTY_MAX)? > > Could you please look into this? I'll reassign the ticket to you. Oh, I didn't know that I had to normalize it. Please review the new patch. > Also I'm going to commit your patch. Thanks! Jung-uk Kim
Created attachment 30780 [details] [review] Add support for ACPI backlight control on FreeBSD and DragonFlyBSD
(In reply to comment #3) > As DragonFly BSD uses the very same acpi_video(4) code from FreeBSD, please add > "|| defined(__DragonFly__)" into relevant ifdefs. Thanks. Done. > But I also have a question - is it guaranteed that it's the lcd0 entry that > user has to change? Shouldn't it test for active device at first > (hw.acpi.video.lcdX.active)? I have seen at least sysctl outputs with several > lcd output entries. ACPI specification says only built-in display is supported by the _BCL method and its friends. That is, it is very unlikely to have more than one internal display. :-) And it doesn't have to be active to get/set brightness, AFAIK. > Although it might make sense to fallback to lcd0 probably because I've also > seen variants like "several lcd entries, but none of them is active (in reality > it's active, but the relevant sysctl is 0)" and "several lcd entries and all > entries are active". I don't remember whether these are relevant to radeons > though. I may be wrong but I do not believe RadeonHD can drive multiple *internal* LCDs. Jung-uk Kim
Created attachment 30783 [details] [review] Fixed rounding bug This should be the final, I hope.
Created attachment 30784 [details] [review] Add support for ACPI backlight control for FreeBSD and DrangonFlyBSD The previous patch was not the final as I hoped. :-(
Created attachment 30785 [details] [review] Add support for ACPI backlight control for FreeBSD and DrangonFlyBSD (revised) This is little bit more cleaner.
Thanks for the new patch. I'm not sure if this is correct: + max_levels = levels[max] - levels[min] + 1; You are mapping a range of 0..RHD_BACKLIGHT_PROPERTY_MAX onto a range of levels[min]..levels[max]. This pretty much means you want to subtract the bias levels[min] to get a range between 0...(levels[max] - levels[min]) leaving out the '+ 1'. + step = RHD_BL_NORMALIZE(1, max_levels); This is critical: if (max_levels > RHD_BACKLIGHT_PROPERTY_MAX) : step == 0 Doesn't this also work: if (do_write) { int level = -1; int d1 = level_max * RHD_BACKLIGHT_PROPERTY_MAX + 1; for (i = 0; i < num_levels; i++) { int d2 = abs(*val * level_max - (levels[i] - levels[min]) * RHD_BACKLIGHT_PROPERTY_MAX); if (d2 < d1) { level = i; d1 = d2; } } free(levels); if (level == -1) return FALSE; .... } else { free(levels); len = sizeof(level); if (sysctlbyname(ACPI_VIDEO_BRIGHTNESS, &level, &len, NULL, 0) != 0) return FALSE; *val = (level - levels[min]) * RHD_BACKLIGHT_PROPERTY_MAX / level_max; ... }
(In reply to comment #10) > Thanks for the new patch. > > I'm not sure if this is correct: > > + max_levels = levels[max] - levels[min] + 1; > > You are mapping a range of 0..RHD_BACKLIGHT_PROPERTY_MAX onto a range of > levels[min]..levels[max]. This pretty much means you want to subtract the > bias levels[min] to get a range between 0...(levels[max] - levels[min]) leaving > out the '+ 1'. max_levels is number of possible levels (sorry for the bad naming), i.e., if it is 1:1 (levels[max] == RHD_BACKLIGHT_PROPERTY_MAX and levels[min] == 0), then we have (RHD_BACKLIGHT_PROPERTY_MAX + 1) possible levels. > + step = RHD_BL_NORMALIZE(1, max_levels); > > This is critical: if (max_levels > RHD_BACKLIGHT_PROPERTY_MAX) : step == 0 It never happens because ACPI specification says it must be 0...100 and FreeBSD driver rejects anything out of the range. > Doesn't this also work: > > if (do_write) { > int level = -1; > int d1 = level_max * RHD_BACKLIGHT_PROPERTY_MAX + 1; > for (i = 0; i < num_levels; i++) { > int d2 = abs(*val * level_max - (levels[i] - levels[min]) > * RHD_BACKLIGHT_PROPERTY_MAX); As a kernel hacker, I have a bad habit of not using useful libc functions, e.g., abs(3). ;-) > if (d2 < d1) { > level = i; > d1 = d2; > } > } > free(levels); > if (level == -1) > return FALSE; > .... > } else { > free(levels); > len = sizeof(level); > if (sysctlbyname(ACPI_VIDEO_BRIGHTNESS, &level, &len, NULL, 0) != 0) > return FALSE; > *val = (level - levels[min]) * RHD_BACKLIGHT_PROPERTY_MAX / level_max; > ... > } > That should work, too, I guess. Please stay tuned. Thanks! Jung-uk Kim
Created attachment 30812 [details] [review] Revised patch I removed min because ACPI specification says it has to be 0 and it may not be a supported level. Jung-uk Kim
Nice! Thanks! Committed and pushed :) commit 209505b3af2558920578a8572a9930b413380190 Author: Jung-uk Kim <jkim@FreeBSD.org> Date: Fri Oct 30 09:37:07 2009 +0100 Backlight: ACPI backlight support for FreeBSD This patch adds support for FreeBSD and DragonFlyBSD via acpi_video(4). Signed-off-by: Egbert Eich <eich@freedesktop.org>
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.