* [PATCH v2] drm/amd/display: use correct scale for actual_brightness
@ 2020-08-04 20:13 Alexander Monakov
2020-08-09 16:26 ` Alexander Monakov
2020-08-16 8:49 ` Alexander Monakov
0 siblings, 2 replies; 6+ messages in thread
From: Alexander Monakov @ 2020-08-04 20:13 UTC (permalink / raw)
To: amd-gfx
Cc: linux-kernel, Alexander Monakov, Alex Deucher, Nicholas Kazlauskas
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. Use it to
reimplement 'convert_brightness' as 'convert_brightness_from_user' and
introduce 'convert_brightness_to_user'.
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>
Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
Signed-off-by: Alexander Monakov <amonakov@ispras.ru>
---
v2: split convert_brightness to &_from_user and &_to_user (Nicholas)
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 81 +++++++++----------
1 file changed, 40 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..b60a763f3f95 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,50 @@ 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_from_user(const struct amdgpu_dm_backlight_caps *caps,
+ uint32_t brightness)
+{
+ unsigned min, max;
+
+ if (!get_brightness_range(caps, &min, &max))
+ return brightness;
+
+ // Rescale 0..255 to min..max
+ return min + DIV_ROUND_CLOSEST((max - min) * brightness,
+ AMDGPU_MAX_BL_LEVEL);
+}
+
+static u32 convert_brightness_to_user(const struct amdgpu_dm_backlight_caps *caps,
+ uint32_t brightness)
+{
+ unsigned min, max;
+
+ if (!get_brightness_range(caps, &min, &max))
+ return brightness;
+
+ 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 +2940,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_from_user(&caps, bd->props.brightness);
// Change brightness based on AUX property
if (caps.aux_support)
return set_backlight_via_aux(link, brightness);
@@ -2958,7 +2957,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_to_user(&dm->backlight_caps, ret);
}
static const struct backlight_ops amdgpu_dm_backlight_ops = {
base-commit: bcf876870b95592b52519ed4aafcf9d95999bc9c
--
2.26.2
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] drm/amd/display: use correct scale for actual_brightness
2020-08-04 20:13 [PATCH v2] drm/amd/display: use correct scale for actual_brightness Alexander Monakov
@ 2020-08-09 16:26 ` Alexander Monakov
2020-08-16 8:49 ` Alexander Monakov
1 sibling, 0 replies; 6+ messages in thread
From: Alexander Monakov @ 2020-08-09 16:26 UTC (permalink / raw)
To: amd-gfx; +Cc: linux-kernel, Alex Deucher, Nicholas Kazlauskas
On Tue, 4 Aug 2020, 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. Use it to
> reimplement 'convert_brightness' as 'convert_brightness_from_user' and
> introduce 'convert_brightness_to_user'.
>
> 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>
> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> Signed-off-by: Alexander Monakov <amonakov@ispras.ru>
> ---
> v2: split convert_brightness to &_from_user and &_to_user (Nicholas)
Nicholas, does this implement the kind of split you had in mind?
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 81 +++++++++----------
> 1 file changed, 40 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..b60a763f3f95 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,50 @@ 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_from_user(const struct amdgpu_dm_backlight_caps *caps,
> + uint32_t brightness)
> +{
> + unsigned min, max;
> +
> + if (!get_brightness_range(caps, &min, &max))
> + return brightness;
> +
> + // Rescale 0..255 to min..max
> + return min + DIV_ROUND_CLOSEST((max - min) * brightness,
> + AMDGPU_MAX_BL_LEVEL);
> +}
> +
> +static u32 convert_brightness_to_user(const struct amdgpu_dm_backlight_caps *caps,
> + uint32_t brightness)
> +{
> + unsigned min, max;
> +
> + if (!get_brightness_range(caps, &min, &max))
> + return brightness;
> +
> + 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 +2940,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_from_user(&caps, bd->props.brightness);
> // Change brightness based on AUX property
> if (caps.aux_support)
> return set_backlight_via_aux(link, brightness);
> @@ -2958,7 +2957,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_to_user(&dm->backlight_caps, ret);
> }
>
> static const struct backlight_ops amdgpu_dm_backlight_ops = {
>
> base-commit: bcf876870b95592b52519ed4aafcf9d95999bc9c
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] drm/amd/display: use correct scale for actual_brightness
2020-08-04 20:13 [PATCH v2] drm/amd/display: use correct scale for actual_brightness Alexander Monakov
2020-08-09 16:26 ` Alexander Monakov
@ 2020-08-16 8:49 ` Alexander Monakov
2020-08-17 17:59 ` Alex Deucher
1 sibling, 1 reply; 6+ messages in thread
From: Alexander Monakov @ 2020-08-16 8:49 UTC (permalink / raw)
To: amd-gfx; +Cc: linux-kernel, Alex Deucher, Nicholas Kazlauskas
Ping.
On Tue, 4 Aug 2020, 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. Use it to
> reimplement 'convert_brightness' as 'convert_brightness_from_user' and
> introduce 'convert_brightness_to_user'.
>
> 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>
> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> Signed-off-by: Alexander Monakov <amonakov@ispras.ru>
> ---
> v2: split convert_brightness to &_from_user and &_to_user (Nicholas)
>
> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 81 +++++++++----------
> 1 file changed, 40 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..b60a763f3f95 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,50 @@ 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_from_user(const struct amdgpu_dm_backlight_caps *caps,
> + uint32_t brightness)
> +{
> + unsigned min, max;
> +
> + if (!get_brightness_range(caps, &min, &max))
> + return brightness;
> +
> + // Rescale 0..255 to min..max
> + return min + DIV_ROUND_CLOSEST((max - min) * brightness,
> + AMDGPU_MAX_BL_LEVEL);
> +}
> +
> +static u32 convert_brightness_to_user(const struct amdgpu_dm_backlight_caps *caps,
> + uint32_t brightness)
> +{
> + unsigned min, max;
> +
> + if (!get_brightness_range(caps, &min, &max))
> + return brightness;
> +
> + 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 +2940,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_from_user(&caps, bd->props.brightness);
> // Change brightness based on AUX property
> if (caps.aux_support)
> return set_backlight_via_aux(link, brightness);
> @@ -2958,7 +2957,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_to_user(&dm->backlight_caps, ret);
> }
>
> static const struct backlight_ops amdgpu_dm_backlight_ops = {
>
> base-commit: bcf876870b95592b52519ed4aafcf9d95999bc9c
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] drm/amd/display: use correct scale for actual_brightness
2020-08-16 8:49 ` Alexander Monakov
@ 2020-08-17 17:59 ` Alex Deucher
2020-08-18 16:15 ` Alex Deucher
0 siblings, 1 reply; 6+ messages in thread
From: Alex Deucher @ 2020-08-17 17:59 UTC (permalink / raw)
To: Alexander Monakov; +Cc: amd-gfx list, Alex Deucher, LKML, Nicholas Kazlauskas
On Mon, Aug 17, 2020 at 3:09 AM Alexander Monakov <amonakov@ispras.ru> wrote:
>
> Ping.
Patch looks good to me:
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
Nick, unless you have any objections, I'll go ahead and apply it.
Alex
>
> On Tue, 4 Aug 2020, 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. Use it to
> > reimplement 'convert_brightness' as 'convert_brightness_from_user' and
> > introduce 'convert_brightness_to_user'.
> >
> > 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>
> > Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> > Signed-off-by: Alexander Monakov <amonakov@ispras.ru>
> > ---
> > v2: split convert_brightness to &_from_user and &_to_user (Nicholas)
> >
> > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 81 +++++++++----------
> > 1 file changed, 40 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..b60a763f3f95 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,50 @@ 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_from_user(const struct amdgpu_dm_backlight_caps *caps,
> > + uint32_t brightness)
> > +{
> > + unsigned min, max;
> > +
> > + if (!get_brightness_range(caps, &min, &max))
> > + return brightness;
> > +
> > + // Rescale 0..255 to min..max
> > + return min + DIV_ROUND_CLOSEST((max - min) * brightness,
> > + AMDGPU_MAX_BL_LEVEL);
> > +}
> > +
> > +static u32 convert_brightness_to_user(const struct amdgpu_dm_backlight_caps *caps,
> > + uint32_t brightness)
> > +{
> > + unsigned min, max;
> > +
> > + if (!get_brightness_range(caps, &min, &max))
> > + return brightness;
> > +
> > + 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 +2940,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_from_user(&caps, bd->props.brightness);
> > // Change brightness based on AUX property
> > if (caps.aux_support)
> > return set_backlight_via_aux(link, brightness);
> > @@ -2958,7 +2957,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_to_user(&dm->backlight_caps, ret);
> > }
> >
> > static const struct backlight_ops amdgpu_dm_backlight_ops = {
> >
> > base-commit: bcf876870b95592b52519ed4aafcf9d95999bc9c
> >
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] drm/amd/display: use correct scale for actual_brightness
2020-08-17 17:59 ` Alex Deucher
@ 2020-08-18 16:15 ` Alex Deucher
2020-08-18 16:48 ` Kazlauskas, Nicholas
0 siblings, 1 reply; 6+ messages in thread
From: Alex Deucher @ 2020-08-18 16:15 UTC (permalink / raw)
To: Alexander Monakov; +Cc: amd-gfx list, Alex Deucher, LKML, Nicholas Kazlauskas
Applied. Thanks!
Alex
On Mon, Aug 17, 2020 at 1:59 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>
> On Mon, Aug 17, 2020 at 3:09 AM Alexander Monakov <amonakov@ispras.ru> wrote:
> >
> > Ping.
>
> Patch looks good to me:
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
>
> Nick, unless you have any objections, I'll go ahead and apply it.
>
> Alex
>
> >
> > On Tue, 4 Aug 2020, 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. Use it to
> > > reimplement 'convert_brightness' as 'convert_brightness_from_user' and
> > > introduce 'convert_brightness_to_user'.
> > >
> > > 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>
> > > Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> > > Signed-off-by: Alexander Monakov <amonakov@ispras.ru>
> > > ---
> > > v2: split convert_brightness to &_from_user and &_to_user (Nicholas)
> > >
> > > .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 81 +++++++++----------
> > > 1 file changed, 40 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..b60a763f3f95 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,50 @@ 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_from_user(const struct amdgpu_dm_backlight_caps *caps,
> > > + uint32_t brightness)
> > > +{
> > > + unsigned min, max;
> > > +
> > > + if (!get_brightness_range(caps, &min, &max))
> > > + return brightness;
> > > +
> > > + // Rescale 0..255 to min..max
> > > + return min + DIV_ROUND_CLOSEST((max - min) * brightness,
> > > + AMDGPU_MAX_BL_LEVEL);
> > > +}
> > > +
> > > +static u32 convert_brightness_to_user(const struct amdgpu_dm_backlight_caps *caps,
> > > + uint32_t brightness)
> > > +{
> > > + unsigned min, max;
> > > +
> > > + if (!get_brightness_range(caps, &min, &max))
> > > + return brightness;
> > > +
> > > + 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 +2940,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_from_user(&caps, bd->props.brightness);
> > > // Change brightness based on AUX property
> > > if (caps.aux_support)
> > > return set_backlight_via_aux(link, brightness);
> > > @@ -2958,7 +2957,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_to_user(&dm->backlight_caps, ret);
> > > }
> > >
> > > static const struct backlight_ops amdgpu_dm_backlight_ops = {
> > >
> > > base-commit: bcf876870b95592b52519ed4aafcf9d95999bc9c
> > >
> > _______________________________________________
> > amd-gfx mailing list
> > amd-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] drm/amd/display: use correct scale for actual_brightness
2020-08-18 16:15 ` Alex Deucher
@ 2020-08-18 16:48 ` Kazlauskas, Nicholas
0 siblings, 0 replies; 6+ messages in thread
From: Kazlauskas, Nicholas @ 2020-08-18 16:48 UTC (permalink / raw)
To: Alex Deucher, Alexander Monakov; +Cc: amd-gfx list, Alex Deucher, LKML
No objections from my side - and thanks for addressing my feedback.
Regards,
Nicholas Kazlauskas
On 2020-08-18 12:15 p.m., Alex Deucher wrote:
> Applied. Thanks!
>
> Alex
>
> On Mon, Aug 17, 2020 at 1:59 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>>
>> On Mon, Aug 17, 2020 at 3:09 AM Alexander Monakov <amonakov@ispras.ru> wrote:
>>>
>>> Ping.
>>
>> Patch looks good to me:
>> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
>>
>> Nick, unless you have any objections, I'll go ahead and apply it.
>>
>> Alex
>>
>>>
>>> On Tue, 4 Aug 2020, 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. Use it to
>>>> reimplement 'convert_brightness' as 'convert_brightness_from_user' and
>>>> introduce 'convert_brightness_to_user'.
>>>>
>>>> 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>
>>>> Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>>>> Signed-off-by: Alexander Monakov <amonakov@ispras.ru>
>>>> ---
>>>> v2: split convert_brightness to &_from_user and &_to_user (Nicholas)
>>>>
>>>> .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 81 +++++++++----------
>>>> 1 file changed, 40 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..b60a763f3f95 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,50 @@ 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_from_user(const struct amdgpu_dm_backlight_caps *caps,
>>>> + uint32_t brightness)
>>>> +{
>>>> + unsigned min, max;
>>>> +
>>>> + if (!get_brightness_range(caps, &min, &max))
>>>> + return brightness;
>>>> +
>>>> + // Rescale 0..255 to min..max
>>>> + return min + DIV_ROUND_CLOSEST((max - min) * brightness,
>>>> + AMDGPU_MAX_BL_LEVEL);
>>>> +}
>>>> +
>>>> +static u32 convert_brightness_to_user(const struct amdgpu_dm_backlight_caps *caps,
>>>> + uint32_t brightness)
>>>> +{
>>>> + unsigned min, max;
>>>> +
>>>> + if (!get_brightness_range(caps, &min, &max))
>>>> + return brightness;
>>>> +
>>>> + 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 +2940,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_from_user(&caps, bd->props.brightness);
>>>> // Change brightness based on AUX property
>>>> if (caps.aux_support)
>>>> return set_backlight_via_aux(link, brightness);
>>>> @@ -2958,7 +2957,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_to_user(&dm->backlight_caps, ret);
>>>> }
>>>>
>>>> static const struct backlight_ops amdgpu_dm_backlight_ops = {
>>>>
>>>> base-commit: bcf876870b95592b52519ed4aafcf9d95999bc9c
>>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx@lists.freedesktop.org
>>> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-08-18 16:49 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-04 20:13 [PATCH v2] drm/amd/display: use correct scale for actual_brightness Alexander Monakov
2020-08-09 16:26 ` Alexander Monakov
2020-08-16 8:49 ` Alexander Monakov
2020-08-17 17:59 ` Alex Deucher
2020-08-18 16:15 ` Alex Deucher
2020-08-18 16:48 ` Kazlauskas, Nicholas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).