[PATCH] drm/amd/display: use correct scale for actual_brightness

[PATCH] drm/amd/display: use correct scale for actual_brightness

[Alexander Monakov]

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
-- 
2.26.2

Re: [PATCH] drm/amd/display: use correct scale for actual_brightness

[Kazlauskas, Nicholas]

This is a cleaner change the other proposed patch since it doesn't need 
to modify the exist conversion functions but I'd be worried about broken 
userspace relying on 0-255 as the only acceptable range.

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
> 

Re: [PATCH] drm/amd/display: use correct scale for actual_brightness

[Kazlauskas, Nicholas]

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>

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
> 

Re: [PATCH] drm/amd/display: use correct scale for actual_brightness

[Alexander Monakov]

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?

> 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
> > 
> 
> 

Re: [PATCH] drm/amd/display: use correct scale for actual_brightness

[Kazlauskas, Nicholas]

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
>>>
>>
>>