Re: [PATCH] amd/display: set backlight only if required
Dear Shirish,
Thank you for the patch.
Am 11.03.22 um 16:33 schrieb Shirish S:
[Why]
comparing pwm bl values (coverted) with user brightness(converted)
1. co*n*verted
2. Please add a space before the (.
levels in commit_tail leads to continuous setting of backlight via dmub
as they don't to match.
Maybe add a blank line between paragraphs.
This leads overdrive in queuing of commands to DMCU that sometimes lead
What is “overdrive in queuing”?
lead*s*
to depending on load on DMCU fw:
Leads to what? Words missing after *to*.
"[drm:dc_dmub_srv_wait_idle] *ERROR* Error waiting for DMUB idle: status=3"
You could also add the example from your discussion with Harry.
[How]
Store last successfully set backlight value and compare with it instead
of pwm reads which is not what we should compare with.
Maybe extend it with the information gotten from Harry, that this is
expected, when ABM is enabled.
Kind regards,
Paul
Signed-off-by: Shirish S
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 7 ---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 6 ++
2 files changed, 10 insertions(+), 3 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 df0980ff9a63..2b8337e47861 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3972,7 +3972,7 @@ static u32 convert_brightness_to_user(const struct
amdgpu_dm_backlight_caps *cap
max - min);
}
-static int amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
+static void amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
int bl_idx,
u32 user_brightness)
{
@@ -4003,7 +4003,8 @@ static int amdgpu_dm_backlight_set_level(struct
amdgpu_display_manager *dm,
DRM_DEBUG("DM: Failed to update backlight on
eDP[%d]\n", bl_idx);
}
- return rc ? 0 : 1;
+ if (rc)
+ dm->actual_brightness[bl_idx] = user_brightness;
}
static int amdgpu_dm_backlight_update_status(struct backlight_device *bd)
@@ -9944,7 +9945,7 @@ static void amdgpu_dm_atomic_commit_tail(struct
drm_atomic_state *state)
/* restore the backlight level */
for (i = 0; i < dm->num_of_edps; i++) {
if (dm->backlight_dev[i] &&
- (amdgpu_dm_backlight_get_level(dm, i) != dm->brightness[i]))
+ (dm->actual_brightness[i] != dm->brightness[i]))
amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]);
}
#endif
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index 372f9adf091a..321279bc877b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -540,6 +540,12 @@ struct amdgpu_display_manager {
* cached backlight values.
*/
u32 brightness[AMDGPU_DM_MAX_NUM_EDP];
+ /**
+* @actual_brightness:
+*
+* last successfully applied backlight values.
+*/
+ u32 actual_brightness[AMDGPU_DM_MAX_NUM_EDP];
};
enum dsc_clock_force_state {
Re: [PATCH] amd/display: set backlight only if required
On 2022-03-20 23:47, S, Shirish wrote:
> [AMD Official Use Only]
>
> Ping!
>
After internal discussions it turns out that the readback
is indeed not expected to match the value being set if ABM
is enabled. The suggested way to deal with this is to track
the previous value in SW, i.e. exactly what this patch is doing.
Based on that this patch is
Reviewed-by: Harry Wentland
Harry
>
>
>
> Regards,
> Shirish S
>
> -Original Message-
> From: S, Shirish
> Sent: Monday, March 14, 2022 12:24 PM
> To: Wentland, Harry ; S, Shirish ;
> Wentland, Harry ; Kazlauskas, Nicholas
> ; Lakha, Bhawanpreet
> Cc: amd-gfx@lists.freedesktop.org
> Subject: Re: [PATCH] amd/display: set backlight only if required
>
>
> On 3/11/2022 9:11 PM, Harry Wentland wrote:
>>
>> On 3/11/22 10:33, Shirish S wrote:
>>> [Why]
>>> comparing pwm bl values (coverted) with user brightness(converted)
>>> levels in commit_tail leads to continuous setting of backlight via
>>> dmub as they don't to match.
>> Why do the values not match?
>
> Here is a sample of values:
>
> dmub_abm_get_current_backlight() reads backlight value as 11526 =>
> convert_to_user() as 45.
>
> user_brightness value to be set at this point is 159 =>
> convert_from_user() gives 40863.
>
> Now, we are continuously comparing 45 (current backlight) with 159 (to be set
> from user space) in every commit tail till any actual changes happen to
> brightness.
>
> Ideally, current brightness/backlight value read from pwm register, when
> converted should yield 159 but it returns 45.
>
> Hence, I believe, there's a bug either in conversion back and forth of user
> space levels or pwm register is not the right way to arrive at current
> brightness values.
>
>> It looks like the value mismatch
>> is our root cause.
> Yes, apparently I could not find any other register read that could bail us
> out here and provide actual/proper values, hence this patch.
>> I remember a while back looking at an issue where we the readback was
>> from DMCU while we were setting BL directly via PWM. I wonder if the
>> opposite is happening now.
>>
>> See this for the previous fix:
>> 2bf3d62dabcc drm/amd/display: Get backlight from PWM if DMCU is not
>> initialized
>
> The sample values mentioned above are with this patch applied.
>
> Is there a better way of reading current backlight levels, that reflect user
> space ones?
>
>
>>> This leads overdrive in queuing of commands to DMCU that sometimes lead
>>> to depending on load on DMCU fw:
>>>
>>> "[drm:dc_dmub_srv_wait_idle] *ERROR* Error waiting for DMUB idle: status=3"
>>>
>>> [How]
>>> Store last successfully set backlight value and compare with it instead
>>> of pwm reads which is not what we should compare with.
>>>
>> Does BL work reliably after S3 or S4 with your change? I wonder if
>> there are use-cases that might break because we're no longer comparing
>> against the actual BL value but against a stored variable.
> I've verified this patch for boot, S0i3 and GUI method of changing
> brightness on ChromeOS
>>
>>> Signed-off-by: Shirish S
>>> ---
>>> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 7 ---
>>> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 6 ++
>>> 2 files changed, 10 insertions(+), 3 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 df0980ff9a63..2b8337e47861 100644
>>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>>> @@ -3972,7 +3972,7 @@ static u32 convert_brightness_to_user(const struct
>>> amdgpu_dm_backlight_caps *cap
>>> max - min);
>>> }
>>>
>>> -static int amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
>>> +static void amdgpu_dm_backlight_set_level(struct amdgpu_display_manager
>>> *dm,
>>> int bl_idx,
>>> u32 user_brightness)
>>> {
>>> @@ -4003,7 +4003,8 @@ static int amdgpu_dm_backlight_set_level(struct
>>> amdgpu_display_manager *dm,
>>> DRM_DEBUG("DM: Failed to update backlight on
>>> eDP[%d]\n", bl_idx);
>>> }
>>>
>>> - return rc ? 0 : 1;
>>> + if (rc)
>>> +
RE: [PATCH] amd/display: set backlight only if required
[AMD Official Use Only]
Ping!
Regards,
Shirish S
-Original Message-
From: S, Shirish
Sent: Monday, March 14, 2022 12:24 PM
To: Wentland, Harry ; S, Shirish ;
Wentland, Harry ; Kazlauskas, Nicholas
; Lakha, Bhawanpreet
Cc: amd-gfx@lists.freedesktop.org
Subject: Re: [PATCH] amd/display: set backlight only if required
On 3/11/2022 9:11 PM, Harry Wentland wrote:
>
> On 3/11/22 10:33, Shirish S wrote:
>> [Why]
>> comparing pwm bl values (coverted) with user brightness(converted)
>> levels in commit_tail leads to continuous setting of backlight via
>> dmub as they don't to match.
> Why do the values not match?
Here is a sample of values:
dmub_abm_get_current_backlight() reads backlight value as 11526 =>
convert_to_user() as 45.
user_brightness value to be set at this point is 159 =>
convert_from_user() gives 40863.
Now, we are continuously comparing 45 (current backlight) with 159 (to be set
from user space) in every commit tail till any actual changes happen to
brightness.
Ideally, current brightness/backlight value read from pwm register, when
converted should yield 159 but it returns 45.
Hence, I believe, there's a bug either in conversion back and forth of user
space levels or pwm register is not the right way to arrive at current
brightness values.
> It looks like the value mismatch
> is our root cause.
Yes, apparently I could not find any other register read that could bail us out
here and provide actual/proper values, hence this patch.
> I remember a while back looking at an issue where we the readback was
> from DMCU while we were setting BL directly via PWM. I wonder if the
> opposite is happening now.
>
> See this for the previous fix:
> 2bf3d62dabcc drm/amd/display: Get backlight from PWM if DMCU is not
> initialized
The sample values mentioned above are with this patch applied.
Is there a better way of reading current backlight levels, that reflect user
space ones?
>> This leads overdrive in queuing of commands to DMCU that sometimes lead
>> to depending on load on DMCU fw:
>>
>> "[drm:dc_dmub_srv_wait_idle] *ERROR* Error waiting for DMUB idle: status=3"
>>
>> [How]
>> Store last successfully set backlight value and compare with it instead
>> of pwm reads which is not what we should compare with.
>>
> Does BL work reliably after S3 or S4 with your change? I wonder if
> there are use-cases that might break because we're no longer comparing
> against the actual BL value but against a stored variable.
I've verified this patch for boot, S0i3 and GUI method of changing
brightness on ChromeOS
>
>> Signed-off-by: Shirish S
>> ---
>> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 7 ---
>> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 6 ++
>> 2 files changed, 10 insertions(+), 3 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 df0980ff9a63..2b8337e47861 100644
>> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
>> @@ -3972,7 +3972,7 @@ static u32 convert_brightness_to_user(const struct
>> amdgpu_dm_backlight_caps *cap
>> max - min);
>> }
>>
>> -static int amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
>> +static void amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
>> int bl_idx,
>> u32 user_brightness)
>> {
>> @@ -4003,7 +4003,8 @@ static int amdgpu_dm_backlight_set_level(struct
>> amdgpu_display_manager *dm,
>> DRM_DEBUG("DM: Failed to update backlight on
>> eDP[%d]\n", bl_idx);
>> }
>>
>> -return rc ? 0 : 1;
>> +if (rc)
>> +dm->actual_brightness[bl_idx] = user_brightness;
>> }
>>
>> static int amdgpu_dm_backlight_update_status(struct backlight_device *bd)
>> @@ -9944,7 +9945,7 @@ static void amdgpu_dm_atomic_commit_tail(struct
>> drm_atomic_state *state)
>> /* restore the backlight level */
>> for (i = 0; i < dm->num_of_edps; i++) {
>> if (dm->backlight_dev[i] &&
>> -(amdgpu_dm_backlight_get_level(dm, i) != dm->brightness[i]))
>> +(dm->actual_brightness[i] != dm->brightness[i]))
>> amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]);
>> }
>> #endif
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
>> b/drivers/gpu/drm/amd
Re: [PATCH] amd/display: set backlight only if required
On 3/11/2022 9:11 PM, Harry Wentland wrote:
On 3/11/22 10:33, Shirish S wrote:
[Why]
comparing pwm bl values (coverted) with user brightness(converted)
levels in commit_tail leads to continuous setting of backlight via dmub
as they don't to match.
Why do the values not match?
Here is a sample of values:
dmub_abm_get_current_backlight() reads backlight value as 11526 =>
convert_to_user() as 45.
user_brightness value to be set at this point is 159 =>
convert_from_user() gives 40863.
Now, we are continuously comparing 45 (current backlight) with 159 (to
be set from user space) in every commit tail till any actual changes
happen to brightness.
Ideally, current brightness/backlight value read from pwm register, when
converted should yield 159 but it returns 45.
Hence, I believe, there's a bug either in conversion back and forth of
user space levels or pwm register is not the right way to arrive at
current brightness values.
It looks like the value mismatch
is our root cause.
Yes, apparently I could not find any other register read that could bail
us out here and provide actual/proper values, hence this patch.
I remember a while back looking at an issue
where we the readback was from DMCU while we were setting BL
directly via PWM. I wonder if the opposite is happening now.
See this for the previous fix:
2bf3d62dabcc drm/amd/display: Get backlight from PWM if DMCU is not initialized
The sample values mentioned above are with this patch applied.
Is there a better way of reading current backlight levels, that reflect
user space ones?
This leads overdrive in queuing of commands to DMCU that sometimes lead
to depending on load on DMCU fw:
"[drm:dc_dmub_srv_wait_idle] *ERROR* Error waiting for DMUB idle: status=3"
[How]
Store last successfully set backlight value and compare with it instead
of pwm reads which is not what we should compare with.
Does BL work reliably after S3 or S4 with your change? I wonder if
there are use-cases that might break because we're no longer comparing
against the actual BL value but against a stored variable.
I've verified this patch for boot, S0i3 and GUI method of changing
brightness on ChromeOS
Signed-off-by: Shirish S
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 7 ---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 6 ++
2 files changed, 10 insertions(+), 3 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 df0980ff9a63..2b8337e47861 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3972,7 +3972,7 @@ static u32 convert_brightness_to_user(const struct
amdgpu_dm_backlight_caps *cap
max - min);
}
-static int amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
+static void amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
int bl_idx,
u32 user_brightness)
{
@@ -4003,7 +4003,8 @@ static int amdgpu_dm_backlight_set_level(struct
amdgpu_display_manager *dm,
DRM_DEBUG("DM: Failed to update backlight on
eDP[%d]\n", bl_idx);
}
- return rc ? 0 : 1;
+ if (rc)
+ dm->actual_brightness[bl_idx] = user_brightness;
}
static int amdgpu_dm_backlight_update_status(struct backlight_device *bd)
@@ -9944,7 +9945,7 @@ static void amdgpu_dm_atomic_commit_tail(struct
drm_atomic_state *state)
/* restore the backlight level */
for (i = 0; i < dm->num_of_edps; i++) {
if (dm->backlight_dev[i] &&
- (amdgpu_dm_backlight_get_level(dm, i) != dm->brightness[i]))
+ (dm->actual_brightness[i] != dm->brightness[i]))
amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]);
}
#endif
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index 372f9adf091a..321279bc877b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -540,6 +540,12 @@ struct amdgpu_display_manager {
* cached backlight values.
*/
u32 brightness[AMDGPU_DM_MAX_NUM_EDP];
+ /**
+* @actual_brightness:
"actual" seems misleading here. We might want to call this
"last" or something along those lines.
But let's first see if we can fix the mismatch of BL reads
and writes.
Yes, lets thoroughly evaluate if there is any other way.
Regards,
Shirish S
Harry
+*
+* last successfully applied backlight values.
+*/
+ u32 actual_brightness[AMDGPU_DM_MAX_NUM_EDP];
};
enum dsc_clock_force_state {
Re: [PATCH] amd/display: set backlight only if required
On 3/11/22 10:33, Shirish S wrote:
> [Why]
> comparing pwm bl values (coverted) with user brightness(converted)
> levels in commit_tail leads to continuous setting of backlight via dmub
> as they don't to match.
Why do the values not match? It looks like the value mismatch
is our root cause. I remember a while back looking at an issue
where we the readback was from DMCU while we were setting BL
directly via PWM. I wonder if the opposite is happening now.
See this for the previous fix:
2bf3d62dabcc drm/amd/display: Get backlight from PWM if DMCU is not initialized
> This leads overdrive in queuing of commands to DMCU that sometimes lead
> to depending on load on DMCU fw:
>
> "[drm:dc_dmub_srv_wait_idle] *ERROR* Error waiting for DMUB idle: status=3"
>
> [How]
> Store last successfully set backlight value and compare with it instead
> of pwm reads which is not what we should compare with.
>
Does BL work reliably after S3 or S4 with your change? I wonder if
there are use-cases that might break because we're no longer comparing
against the actual BL value but against a stored variable.
> Signed-off-by: Shirish S
> ---
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 7 ---
> drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 6 ++
> 2 files changed, 10 insertions(+), 3 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 df0980ff9a63..2b8337e47861 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -3972,7 +3972,7 @@ static u32 convert_brightness_to_user(const struct
> amdgpu_dm_backlight_caps *cap
>max - min);
> }
>
> -static int amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
> +static void amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
>int bl_idx,
>u32 user_brightness)
> {
> @@ -4003,7 +4003,8 @@ static int amdgpu_dm_backlight_set_level(struct
> amdgpu_display_manager *dm,
> DRM_DEBUG("DM: Failed to update backlight on
> eDP[%d]\n", bl_idx);
> }
>
> - return rc ? 0 : 1;
> + if (rc)
> + dm->actual_brightness[bl_idx] = user_brightness;
> }
>
> static int amdgpu_dm_backlight_update_status(struct backlight_device *bd)
> @@ -9944,7 +9945,7 @@ static void amdgpu_dm_atomic_commit_tail(struct
> drm_atomic_state *state)
> /* restore the backlight level */
> for (i = 0; i < dm->num_of_edps; i++) {
> if (dm->backlight_dev[i] &&
> - (amdgpu_dm_backlight_get_level(dm, i) != dm->brightness[i]))
> + (dm->actual_brightness[i] != dm->brightness[i]))
> amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]);
> }
> #endif
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index 372f9adf091a..321279bc877b 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -540,6 +540,12 @@ struct amdgpu_display_manager {
>* cached backlight values.
>*/
> u32 brightness[AMDGPU_DM_MAX_NUM_EDP];
> + /**
> + * @actual_brightness:
"actual" seems misleading here. We might want to call this
"last" or something along those lines.
But let's first see if we can fix the mismatch of BL reads
and writes.
Harry
> + *
> + * last successfully applied backlight values.
> + */
> + u32 actual_brightness[AMDGPU_DM_MAX_NUM_EDP];
> };
>
> enum dsc_clock_force_state {
[PATCH] amd/display: set backlight only if required
[Why]
comparing pwm bl values (coverted) with user brightness(converted)
levels in commit_tail leads to continuous setting of backlight via dmub
as they don't to match.
This leads overdrive in queuing of commands to DMCU that sometimes lead
to depending on load on DMCU fw:
"[drm:dc_dmub_srv_wait_idle] *ERROR* Error waiting for DMUB idle: status=3"
[How]
Store last successfully set backlight value and compare with it instead
of pwm reads which is not what we should compare with.
Signed-off-by: Shirish S
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 7 ---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 6 ++
2 files changed, 10 insertions(+), 3 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 df0980ff9a63..2b8337e47861 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3972,7 +3972,7 @@ static u32 convert_brightness_to_user(const struct
amdgpu_dm_backlight_caps *cap
max - min);
}
-static int amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
+static void amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
int bl_idx,
u32 user_brightness)
{
@@ -4003,7 +4003,8 @@ static int amdgpu_dm_backlight_set_level(struct
amdgpu_display_manager *dm,
DRM_DEBUG("DM: Failed to update backlight on
eDP[%d]\n", bl_idx);
}
- return rc ? 0 : 1;
+ if (rc)
+ dm->actual_brightness[bl_idx] = user_brightness;
}
static int amdgpu_dm_backlight_update_status(struct backlight_device *bd)
@@ -9944,7 +9945,7 @@ static void amdgpu_dm_atomic_commit_tail(struct
drm_atomic_state *state)
/* restore the backlight level */
for (i = 0; i < dm->num_of_edps; i++) {
if (dm->backlight_dev[i] &&
- (amdgpu_dm_backlight_get_level(dm, i) != dm->brightness[i]))
+ (dm->actual_brightness[i] != dm->brightness[i]))
amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]);
}
#endif
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index 372f9adf091a..321279bc877b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -540,6 +540,12 @@ struct amdgpu_display_manager {
* cached backlight values.
*/
u32 brightness[AMDGPU_DM_MAX_NUM_EDP];
+ /**
+* @actual_brightness:
+*
+* last successfully applied backlight values.
+*/
+ u32 actual_brightness[AMDGPU_DM_MAX_NUM_EDP];
};
enum dsc_clock_force_state {
--
2.17.1
[PATCH] amd/display: set backlight only if required
[Why]
comparing pwm bl values (coverted) with user brightness(converted)
levels in commit_tail leads to continuous setting of backlight via dmub
as they don't to match.
This leads overdrive in queuing of commands to DMCU that sometimes lead
to depending on load on DMCU fw:
"[drm:dc_dmub_srv_wait_idle] *ERROR* Error waiting for DMUB idle: status=3"
Here status 3 => DMUB_STATUS_QUEUE_FULL
[How]
Store last successfully set backlight value and compare with it instead
of pwm reads which is not what we should compare with.
Signed-off-by: Shirish S
---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 7 ---
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h | 6 ++
2 files changed, 10 insertions(+), 3 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 df0980ff9a63..2b8337e47861 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3972,7 +3972,7 @@ static u32 convert_brightness_to_user(const struct
amdgpu_dm_backlight_caps *cap
max - min);
}
-static int amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
+static void amdgpu_dm_backlight_set_level(struct amdgpu_display_manager *dm,
int bl_idx,
u32 user_brightness)
{
@@ -4003,7 +4003,8 @@ static int amdgpu_dm_backlight_set_level(struct
amdgpu_display_manager *dm,
DRM_DEBUG("DM: Failed to update backlight on
eDP[%d]\n", bl_idx);
}
- return rc ? 0 : 1;
+ if (rc)
+ dm->actual_brightness[bl_idx] = user_brightness;
}
static int amdgpu_dm_backlight_update_status(struct backlight_device *bd)
@@ -9944,7 +9945,7 @@ static void amdgpu_dm_atomic_commit_tail(struct
drm_atomic_state *state)
/* restore the backlight level */
for (i = 0; i < dm->num_of_edps; i++) {
if (dm->backlight_dev[i] &&
- (amdgpu_dm_backlight_get_level(dm, i) != dm->brightness[i]))
+ (dm->actual_brightness[i] != dm->brightness[i]))
amdgpu_dm_backlight_set_level(dm, i, dm->brightness[i]);
}
#endif
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
index 372f9adf091a..321279bc877b 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
@@ -540,6 +540,12 @@ struct amdgpu_display_manager {
* cached backlight values.
*/
u32 brightness[AMDGPU_DM_MAX_NUM_EDP];
+ /**
+* @actual_brightness:
+*
+* last successfully applied backlight values.
+*/
+ u32 actual_brightness[AMDGPU_DM_MAX_NUM_EDP];
};
enum dsc_clock_force_state {
--
2.17.1
