Messages in this thread | | | Subject | Re: [PATCH] drm/amd/display: use correct scale for actual_brightness | From | "Kazlauskas, Nicholas" <> | Date | Tue, 4 Aug 2020 12:35:45 -0400 |
| |
On 2020-08-04 12:28 p.m., Alexander Monakov wrote: > > > On Tue, 4 Aug 2020, Kazlauskas, Nicholas wrote: > >> This is a cleaner change the other proposed patch since it doesn't need to > > Can you give a URL to the other patch please?
Sorry, replied to the wrong email by accident here.
The other change was modifying the max_brightness range and rescaling internal min/max defaults.
I don't think it was sent out to the list yet.
Regards, Nicholas Kazlauskas
> >> modify the exist conversion functions but I'd be worried about broken >> userspace relying on 0-255 as the only acceptable range. > > Not sure what you mean by this. Userspace simply reads the maximum value from > max_brightness sysfs file. On other gpu/firmware combinations it can be 7 or 9 > for example, it just happens to be 255 with modern amdgpu. Minimum value is > always zero. > > Value seen in max_brightness remains 255 with this patch, so as far as userspace > is concerned nothing is changed apart from value given by actual_brightness file. > > Alexander > >> >> Not an expert on existing implementations to point out a specific one though. >> >> Regards, >> Nicholas Kazlauskas >> >> On 2020-08-03 4:02 p.m., Alexander Monakov wrote: >>> Documentation for sysfs backlight level interface requires that >>> values in both 'brightness' and 'actual_brightness' files are >>> interpreted to be in range from 0 to the value given in the >>> 'max_brightness' file. >>> >>> With amdgpu, max_brightness gives 255, and values written by the user >>> into 'brightness' are internally rescaled to a wider range. However, >>> reading from 'actual_brightness' gives the raw register value without >>> inverse rescaling. This causes issues for various userspace tools such >>> as PowerTop and systemd that expect the value to be in the correct >>> range. >>> >>> Introduce a helper to retrieve internal backlight range. Extend the >>> existing 'convert_brightness' function to handle conversion in both >>> directions. >>> >>> Bug: https://bugzilla.kernel.org/show_bug.cgi?id=203905 >>> Bug: https://gitlab.freedesktop.org/drm/amd/-/issues/1242 >>> Cc: Alex Deucher <alexander.deucher@amd.com> >>> Signed-off-by: Alexander Monakov <amonakov@ispras.ru> >>> --- >>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 73 ++++++++----------- >>> 1 file changed, 32 insertions(+), 41 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>> index 710edc70e37e..03e21e7b7917 100644 >>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c >>> @@ -2881,51 +2881,42 @@ static int set_backlight_via_aux(struct dc_link >>> *link, uint32_t brightness) >>> return rc ? 0 : 1; >>> } >>> -static u32 convert_brightness(const struct amdgpu_dm_backlight_caps >>> *caps, >>> - const uint32_t user_brightness) >>> +static int get_brightness_range(const struct amdgpu_dm_backlight_caps >>> *caps, >>> + unsigned *min, unsigned *max) >>> { >>> - u32 min, max, conversion_pace; >>> - u32 brightness = user_brightness; >>> - >>> if (!caps) >>> - goto out; >>> + return 0; >>> - if (!caps->aux_support) { >>> - max = caps->max_input_signal; >>> - min = caps->min_input_signal; >>> - /* >>> - * The brightness input is in the range 0-255 >>> - * It needs to be rescaled to be between the >>> - * requested min and max input signal >>> - * It also needs to be scaled up by 0x101 to >>> - * match the DC interface which has a range of >>> - * 0 to 0xffff >>> - */ >>> - conversion_pace = 0x101; >>> - brightness = >>> - user_brightness >>> - * conversion_pace >>> - * (max - min) >>> - / AMDGPU_MAX_BL_LEVEL >>> - + min * conversion_pace; >>> + if (caps->aux_support) { >>> + // Firmware limits are in nits, DC API wants millinits. >>> + *max = 1000 * caps->aux_max_input_signal; >>> + *min = 1000 * caps->aux_min_input_signal; >>> } else { >>> - /* TODO >>> - * We are doing a linear interpolation here, which is OK but >>> - * does not provide the optimal result. We probably want >>> - * something close to the Perceptual Quantizer (PQ) curve. >>> - */ >>> - max = caps->aux_max_input_signal; >>> - min = caps->aux_min_input_signal; >>> - >>> - brightness = (AMDGPU_MAX_BL_LEVEL - user_brightness) * min >>> - + user_brightness * max; >>> - // Multiple the value by 1000 since we use millinits >>> - brightness *= 1000; >>> - brightness = DIV_ROUND_CLOSEST(brightness, >>> AMDGPU_MAX_BL_LEVEL); >>> + // Firmware limits are 8-bit, PWM control is 16-bit. >>> + *max = 0x101 * caps->max_input_signal; >>> + *min = 0x101 * caps->min_input_signal; >>> } >>> + return 1; >>> +} >>> -out: >>> - return brightness; >>> +static u32 convert_brightness(const struct amdgpu_dm_backlight_caps *caps, >>> + const uint32_t brightness, int from_user) >>> +{ >>> + unsigned min, max; >>> + >>> + if (!get_brightness_range(caps, &min, &max)) >>> + return brightness; >>> + >>> + if (from_user) >>> + // Rescale 0..255 to min..max >>> + return min + DIV_ROUND_CLOSEST((max - min) * brightness, >>> + AMDGPU_MAX_BL_LEVEL); >>> + >>> + if (brightness < min) >>> + return 0; >>> + // Rescale min..max to 0..255 >>> + return DIV_ROUND_CLOSEST(AMDGPU_MAX_BL_LEVEL * (brightness - min), >>> + max - min); >>> } >>> static int amdgpu_dm_backlight_update_status(struct backlight_device >>> *bd) >>> @@ -2941,7 +2932,7 @@ static int amdgpu_dm_backlight_update_status(struct >>> backlight_device *bd) >>> link = (struct dc_link *)dm->backlight_link; >>> - brightness = convert_brightness(&caps, bd->props.brightness); >>> + brightness = convert_brightness(&caps, bd->props.brightness, 1); >>> // Change brightness based on AUX property >>> if (caps.aux_support) >>> return set_backlight_via_aux(link, brightness); >>> @@ -2958,7 +2949,7 @@ static int amdgpu_dm_backlight_get_brightness(struct >>> backlight_device *bd) >>> if (ret == DC_ERROR_UNEXPECTED) >>> return bd->props.brightness; >>> - return ret; >>> + return convert_brightness(&dm->backlight_caps, ret, 0); >>> } >>> static const struct backlight_ops amdgpu_dm_backlight_ops = { >>> >>> base-commit: bcf876870b95592b52519ed4aafcf9d95999bc9c >>> >> >>
| |