linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: "Kazlauskas, Nicholas" <nicholas.kazlauskas@amd.com>
To: Alexander Monakov <amonakov@ispras.ru>
Cc: amd-gfx@lists.freedesktop.org,
	Alex Deucher <alexander.deucher@amd.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] drm/amd/display: use correct scale for actual_brightness
Date: Tue, 4 Aug 2020 12:35:45 -0400	[thread overview]
Message-ID: <f1bc6b02-0761-c3c0-d629-7b8f97bef8d7@amd.com> (raw)
In-Reply-To: <alpine.LNX.2.20.13.2008041922170.2454@monopod.intra.ispras.ru>

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


  reply	other threads:[~2020-08-04 16:35 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-03 20:02 Alexander Monakov
2020-08-04 16:18 ` Kazlauskas, Nicholas
2020-08-04 16:28   ` Alexander Monakov
2020-08-04 16:35     ` Kazlauskas, Nicholas [this message]
2020-08-04 16:21 ` Kazlauskas, Nicholas

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f1bc6b02-0761-c3c0-d629-7b8f97bef8d7@amd.com \
    --to=nicholas.kazlauskas@amd.com \
    --cc=alexander.deucher@amd.com \
    --cc=amd-gfx@lists.freedesktop.org \
    --cc=amonakov@ispras.ru \
    --cc=linux-kernel@vger.kernel.org \
    --subject='Re: [PATCH] drm/amd/display: use correct scale for actual_brightness' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

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