[PATCH 0/4] Backlight fixes

[PATCH 0/4] Backlight fixes

[Alex Deucher]

This builds on the debugging and patches that Takashi did on the
bugs referenced in the patches.  It seems some latops claim to support
aux backlight control, but actually use the old pwm controller.

The code also currently warns in the backlight control function
if there is no pipe assigned, but this is a common case if the
display happens to be off when the backlight level is changed
and it just spams the logs, so remove it.

This set wires up the aux controller to the backlight control,
cleans up the code an bit, removes the assert, and adds an option
to override the backlight type.

@Display team, can you check if there are any quirks or differences
in the backlight control detection on windows?  Presumably this
is properly detected there. The systems in question claim to support
aux backlight control in their DPCDs, but that doesn't seem to work.

Alex Deucher (3):
  drm/amdgpu/display: simplify backlight setting
  drm/amdgpu/display: don't assert in set backlight function
  drm/amdgpu/display: handle aux backlight in backlight_get_brightness

Takashi Iwai (1):
  drm/amd/display: Add a backlight module option

 drivers/gpu/drm/amd/amdgpu/amdgpu.h           |  1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c       |  4 ++
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 49 +++++++++++--------
 drivers/gpu/drm/amd/display/dc/core/dc_link.c |  1 -
 4 files changed, 34 insertions(+), 21 deletions(-)

-- 
2.29.2

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 1/4] drm/amdgpu/display: simplify backlight setting

[Alex Deucher]

Avoid the extra wrapper function.

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 20 ++++---------------
 1 file changed, 4 insertions(+), 16 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 278a11bde30f..175a4cd8b982 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3200,19 +3200,6 @@ static void amdgpu_dm_update_backlight_caps(struct amdgpu_display_manager *dm)
 #endif
 }
 
-static int set_backlight_via_aux(struct dc_link *link, uint32_t brightness)
-{
-	bool rc;
-
-	if (!link)
-		return 1;
-
-	rc = dc_link_set_backlight_level_nits(link, true, brightness,
-					      AUX_BL_DEFAULT_TRANSITION_TIME_MS);
-
-	return rc ? 0 : 1;
-}
-
 static int get_brightness_range(const struct amdgpu_dm_backlight_caps *caps,
 				unsigned *min, unsigned *max)
 {
@@ -3275,9 +3262,10 @@ static int amdgpu_dm_backlight_update_status(struct backlight_device *bd)
 	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);
-
-	rc = dc_link_set_backlight_level(dm->backlight_link, brightness, 0);
+		rc = dc_link_set_backlight_level_nits(link, true, brightness,
+						      AUX_BL_DEFAULT_TRANSITION_TIME_MS);
+	else
+		rc = dc_link_set_backlight_level(dm->backlight_link, brightness, 0);
 
 	return rc ? 0 : 1;
 }
-- 
2.29.2

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 2/4] drm/amdgpu/display: don't assert in set backlight function

[Alex Deucher]

It just spams the logs.

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/display/dc/core/dc_link.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link.c b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
index fa9a62dc174b..974b70f21837 100644
--- a/drivers/gpu/drm/amd/display/dc/core/dc_link.c
+++ b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
@@ -2614,7 +2614,6 @@ bool dc_link_set_backlight_level(const struct dc_link *link,
 			if (pipe_ctx->plane_state == NULL)
 				frame_ramp = 0;
 		} else {
-			ASSERT(false);
 			return false;
 		}
 
-- 
2.29.2

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/4] drm/amdgpu/display: don't assert in set backlight function

[Kazlauskas, Nicholas]

On 2021-03-04 12:41 p.m., Alex Deucher wrote:
> It just spams the logs.
> 
> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>

This series in general looks reasonable to me:
Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>

> ---
>   drivers/gpu/drm/amd/display/dc/core/dc_link.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link.c b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> index fa9a62dc174b..974b70f21837 100644
> --- a/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> @@ -2614,7 +2614,6 @@ bool dc_link_set_backlight_level(const struct dc_link *link,
>   			if (pipe_ctx->plane_state == NULL)
>   				frame_ramp = 0;
>   		} else {
> -			ASSERT(false);

Just a comment on what's actually going on here with this warning:

Technically we can't apply the backlight level without a plane_state in 
the context but the panel is also off anyway.

I think there might be a bug here when the panel turns on and we're not 
applying values set when it was off but I don't think anyone's reported 
this as an issue.

I'm not entirely sure if the value gets cached and reapplied with the 
correct value later, but it's something to keep in mind.

Regards,
Nicholas Kazlauskas

>   			return false;
>   		}
>   
> 

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/4] drm/amdgpu/display: don't assert in set backlight function

[Alex Deucher]

On Thu, Mar 4, 2021 at 1:33 PM Kazlauskas, Nicholas
<nicholas.kazlauskas@amd.com> wrote:
>
> On 2021-03-04 12:41 p.m., Alex Deucher wrote:
> > It just spams the logs.
> >
> > Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>
> This series in general looks reasonable to me:
> Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>
> > ---
> >   drivers/gpu/drm/amd/display/dc/core/dc_link.c | 1 -
> >   1 file changed, 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link.c b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> > index fa9a62dc174b..974b70f21837 100644
> > --- a/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> > +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
> > @@ -2614,7 +2614,6 @@ bool dc_link_set_backlight_level(const struct dc_link *link,
> >                       if (pipe_ctx->plane_state == NULL)
> >                               frame_ramp = 0;
> >               } else {
> > -                     ASSERT(false);
>
> Just a comment on what's actually going on here with this warning:
>
> Technically we can't apply the backlight level without a plane_state in
> the context but the panel is also off anyway.
>
> I think there might be a bug here when the panel turns on and we're not
> applying values set when it was off but I don't think anyone's reported
> this as an issue.
>
> I'm not entirely sure if the value gets cached and reapplied with the
> correct value later, but it's something to keep in mind.

It doesn't.  I have additional patches here to cache it:
https://cgit.freedesktop.org/~agd5f/linux/log/?h=backlight_wip

Alex

>
> Regards,
> Nicholas Kazlauskas
>
> >                       return false;
> >               }
> >
> >
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH 2/4] drm/amdgpu/display: don't assert in set backlight function

[Kazlauskas, Nicholas]

On 2021-03-04 1:41 p.m., Alex Deucher wrote:
> On Thu, Mar 4, 2021 at 1:33 PM Kazlauskas, Nicholas
> <nicholas.kazlauskas@amd.com> wrote:
>>
>> On 2021-03-04 12:41 p.m., Alex Deucher wrote:
>>> It just spams the logs.
>>>
>>> Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
>>
>> This series in general looks reasonable to me:
>> Reviewed-by: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
>>
>>> ---
>>>    drivers/gpu/drm/amd/display/dc/core/dc_link.c | 1 -
>>>    1 file changed, 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/display/dc/core/dc_link.c b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
>>> index fa9a62dc174b..974b70f21837 100644
>>> --- a/drivers/gpu/drm/amd/display/dc/core/dc_link.c
>>> +++ b/drivers/gpu/drm/amd/display/dc/core/dc_link.c
>>> @@ -2614,7 +2614,6 @@ bool dc_link_set_backlight_level(const struct dc_link *link,
>>>                        if (pipe_ctx->plane_state == NULL)
>>>                                frame_ramp = 0;
>>>                } else {
>>> -                     ASSERT(false);
>>
>> Just a comment on what's actually going on here with this warning:
>>
>> Technically we can't apply the backlight level without a plane_state in
>> the context but the panel is also off anyway.
>>
>> I think there might be a bug here when the panel turns on and we're not
>> applying values set when it was off but I don't think anyone's reported
>> this as an issue.
>>
>> I'm not entirely sure if the value gets cached and reapplied with the
>> correct value later, but it's something to keep in mind.
> 
> It doesn't.  I have additional patches here to cache it:
> https://nam11.safelinks.protection.outlook.com/?url=https:%2F%2Fcgit.freedesktop.org%2F~agd5f%2Flinux%2Flog%2F%3Fh%3Dbacklight_wip&amp;data=04%7C01%7Cnicholas.kazlauskas%40amd.com%7Cf259e9290b0e4ffbc87308d8df3d3121%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637504801203988045%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=4ROSclecKkfu2km3YeeM7sZK%2FP%2BcC8BajHSxJXQQCRw%3D&amp;reserved=0
> 
> Alex

That's aligned with my expectations then.

I can take a peek at the branch and help review the patches.

Regards,
Nicholas Kazlauskas

> 
>>
>> Regards,
>> Nicholas Kazlauskas
>>
>>>                        return false;
>>>                }
>>>
>>>
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Cnicholas.kazlauskas%40amd.com%7Cf259e9290b0e4ffbc87308d8df3d3121%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637504801203988045%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=%2BbwO5oOXuXFWD%2F8qNTygROj%2B9ZItRsXJb1U7ilcICh4%3D&amp;reserved=0

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 3/4] drm/amdgpu/display: handle aux backlight in backlight_get_brightness

[Alex Deucher]

Need to fetch it via aux.

Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 24 +++++++++++++++----
 1 file changed, 20 insertions(+), 4 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 175a4cd8b982..fb1ad1426cd1 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -3273,11 +3273,27 @@ static int amdgpu_dm_backlight_update_status(struct backlight_device *bd)
 static int amdgpu_dm_backlight_get_brightness(struct backlight_device *bd)
 {
 	struct amdgpu_display_manager *dm = bl_get_data(bd);
-	int ret = dc_link_get_backlight_level(dm->backlight_link);
+	struct amdgpu_dm_backlight_caps caps;
+
+	amdgpu_dm_update_backlight_caps(dm);
+	caps = dm->backlight_caps;
 
-	if (ret == DC_ERROR_UNEXPECTED)
-		return bd->props.brightness;
-	return convert_brightness_to_user(&dm->backlight_caps, ret);
+	if (caps.aux_support) {
+		struct dc_link *link = (struct dc_link *)dm->backlight_link;
+		u32 avg, peak;
+		bool rc;
+
+		rc = dc_link_get_backlight_level_nits(link, &avg, &peak);
+		if (!rc)
+			return bd->props.brightness;
+		return convert_brightness_to_user(&caps, avg);
+	} else {
+		int ret = dc_link_get_backlight_level(dm->backlight_link);
+
+		if (ret == DC_ERROR_UNEXPECTED)
+			return bd->props.brightness;
+		return convert_brightness_to_user(&caps, ret);
+	}
 }
 
 static const struct backlight_ops amdgpu_dm_backlight_ops = {
-- 
2.29.2

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH 4/4] drm/amd/display: Add a backlight module option

[Alex Deucher]

From: Takashi Iwai <tiwai@suse.de>

There seem devices that don't work with the aux channel backlight
control.  For allowing such users to test with the other backlight
control method, provide a new module option, aux_backlight, to specify
enabling or disabling the aux backport support explicitly.  As
default, the aux support is detected by the hardware capability.

v2: make the backlight option generic in case we add future
backlight types (Alex)

BugLink: https://bugzilla.opensuse.org/show_bug.cgi?id=1180749
BugLink: https://gitlab.freedesktop.org/drm/amd/-/issues/1438
Signed-off-by: Takashi Iwai <tiwai@suse.de>
Signed-off-by: Alex Deucher <alexander.deucher@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu.h               | 1 +
 drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c           | 4 ++++
 drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 5 +++++
 3 files changed, 10 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
index 9c0e1d43b470..2673457fba58 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h
@@ -181,6 +181,7 @@ extern uint amdgpu_dc_feature_mask;
 extern uint amdgpu_freesync_vid_mode;
 extern uint amdgpu_dc_debug_mask;
 extern uint amdgpu_dm_abm_level;
+extern int amdgpu_backlight;
 extern struct amdgpu_mgpu_info mgpu_info;
 extern int amdgpu_ras_enable;
 extern uint amdgpu_ras_mask;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
index 3a6f52a4b2ec..b3832eb16eb1 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
@@ -784,6 +784,10 @@ uint amdgpu_dm_abm_level;
 MODULE_PARM_DESC(abmlevel, "ABM level (0 = off (default), 1-4 = backlight reduction level) ");
 module_param_named(abmlevel, amdgpu_dm_abm_level, uint, 0444);
 
+int amdgpu_backlight = -1;
+MODULE_PARM_DESC(backlight, "Backlight control (0 = pwm, 1 = aux, -1 auto (default))");
+module_param_named(backlight, amdgpu_backlight, bint, 0444);
+
 /**
  * DOC: tmz (int)
  * Trusted Memory Zone (TMZ) is a method to protect data being written
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 fb1ad1426cd1..e91af2efb2ae 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
@@ -2282,6 +2282,11 @@ static void update_connector_ext_caps(struct amdgpu_dm_connector *aconnector)
 	    caps->ext_caps->bits.hdr_aux_backlight_control == 1)
 		caps->aux_support = true;
 
+	if (amdgpu_backlight == 0)
+		caps->aux_support = false;
+	else if (amdgpu_backlight == 1)
+		caps->aux_support = true;
+
 	/* From the specification (CTA-861-G), for calculating the maximum
 	 * luminance we need to use:
 	 *	Luminance = 50*2**(CV/32)
-- 
2.29.2

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx