| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <a3a167d9-9117-edc1-c761-b31fa3194f8b@amd.com>
Date: Tue, 4 Aug 2020 12:21:08 -0400
From: "Kazlauskas, Nicholas" <nicholas.kazlauskas@....com>
To: Alexander Monakov <amonakov@...ras.ru>,
amd-gfx@...ts.freedesktop.org
Cc: Alex Deucher <alexander.deucher@....com>,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] drm/amd/display: use correct scale for actual_brightness
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@....com>
> Signed-off-by: Alexander Monakov <amonakov@...ras.ru>
Overall approach seems reasonable, nice catch.
I suggest to add convert_to_user_brightness() instead of making
from_user a flag and extending the current functionality though. It
makes it more clear from the call site what's happening.
Regards,
Nicholas Kazlauskas
> ---
> .../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
>
Powered by blists - more mailing lists