Hi,
here are two patches handling the issues in the backlight control. The first one is to fix the sysfs brightness read for devices with the backlight controlled by aux channel. The second one is to add a new module option, aux_backlight, to forcibly enable/disable the aux channel backlight control. It's no direct solution for the bug we've hit, but it gives at least a workaround.
BugLink: https://bugzilla.opensuse.org/show_bug.cgi?id=1180749 BugLink: https://gitlab.freedesktop.org/drm/amd/-/issues/1438
thanks,
Takashi
===
Takashi Iwai (2): drm/amd/display: Fix the brightness read via aux drm/amd/display: Add aux_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 | 20 ++++++++++++++++++- 3 files changed, 24 insertions(+), 1 deletion(-)
The current code tries to read the brightness value via dc_link_get_backlight_level() no matter whether it's controlled via aux or not, and this results in a bogus value returned. Fix it to read the current value via dc_link_get_backlight_level_nits() for the aux.
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 --- .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
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 c6da89df055d..fb62886ae013 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -3140,6 +3140,16 @@ static int set_backlight_via_aux(struct dc_link *link, uint32_t brightness) return rc ? 0 : 1; }
+static int get_backlight_via_aux(struct dc_link *link) +{ + uint32_t avg, peak; + + if (!link || + !dc_link_get_backlight_level_nits(link, &avg, &peak)) + return DC_ERROR_UNEXPECTED; + return avg; +} + static int get_brightness_range(const struct amdgpu_dm_backlight_caps *caps, unsigned *min, unsigned *max) { @@ -3212,8 +3222,13 @@ 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 dc_link *link = (struct dc_link *)dm->backlight_link; + int ret;
+ if (dm->backlight_caps.aux_support) + ret = get_backlight_via_aux(link); + else + ret = dc_link_get_backlight_level(link); if (ret == DC_ERROR_UNEXPECTED) return bd->props.brightness; return convert_brightness_to_user(&dm->backlight_caps, ret);
On Wed, Feb 3, 2021 at 7:42 AM Takashi Iwai tiwai@suse.de wrote:
The current code tries to read the brightness value via dc_link_get_backlight_level() no matter whether it's controlled via aux or not, and this results in a bogus value returned. Fix it to read the current value via dc_link_get_backlight_level_nits() for the aux.
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
This looks fine to me. FWIW, I have a similar patch set here: https://cgit.freedesktop.org/~agd5f/linux/log/?h=backlight_wip
Alex
.../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-)
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 c6da89df055d..fb62886ae013 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -3140,6 +3140,16 @@ static int set_backlight_via_aux(struct dc_link *link, uint32_t brightness) return rc ? 0 : 1; }
+static int get_backlight_via_aux(struct dc_link *link) +{
uint32_t avg, peak;if (!link ||!dc_link_get_backlight_level_nits(link, &avg, &peak))return DC_ERROR_UNEXPECTED;return avg;+}
static int get_brightness_range(const struct amdgpu_dm_backlight_caps *caps, unsigned *min, unsigned *max) { @@ -3212,8 +3222,13 @@ 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 dc_link *link = (struct dc_link *)dm->backlight_link;int ret;if (dm->backlight_caps.aux_support)ret = get_backlight_via_aux(link);elseret = dc_link_get_backlight_level(link); if (ret == DC_ERROR_UNEXPECTED) return bd->props.brightness; return convert_brightness_to_user(&dm->backlight_caps, ret);-- 2.26.2
amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On Fri, 05 Feb 2021 17:36:44 +0100, Alex Deucher wrote:
On Wed, Feb 3, 2021 at 7:42 AM Takashi Iwai tiwai@suse.de wrote:
The current code tries to read the brightness value via dc_link_get_backlight_level() no matter whether it's controlled via aux or not, and this results in a bogus value returned. Fix it to read the current value via dc_link_get_backlight_level_nits() for the aux.
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
This looks fine to me. FWIW, I have a similar patch set here: https://cgit.freedesktop.org/~agd5f/linux/log/?h=backlight_wip
I'm fine to scratch mine as long as the issue gets fixed :)
FWIW, the biggest problem so far was the aux channel backlight didn't work as expected, the actual backlight isn't changed by the backlight sysfs write. (And the sysfs read gives a bogus value, but it's not the cause of the non-working backlight control.)
Does the aux channel backlight really work with the current code? Or is this rather a device-specific issue (e.g. broken BIOS) and we might need to come up with a deny list or such?
thanks,
Takashi
[AMD Public Use]
-----Original Message----- From: Takashi Iwai tiwai@suse.de Sent: Saturday, February 6, 2021 7:29 AM To: Alex Deucher alexdeucher@gmail.com Cc: Deucher, Alexander Alexander.Deucher@amd.com; Koenig, Christian Christian.Koenig@amd.com; Li, Sun peng (Leo) Sunpeng.Li@amd.com; Wentland, Harry Harry.Wentland@amd.com; Maling list - DRI developers dri-devel@lists.freedesktop.org; amd-gfx list <amd- gfx@lists.freedesktop.org> Subject: Re: [PATCH 1/2] drm/amd/display: Fix the brightness read via aux
On Fri, 05 Feb 2021 17:36:44 +0100, Alex Deucher wrote:
On Wed, Feb 3, 2021 at 7:42 AM Takashi Iwai tiwai@suse.de wrote:
The current code tries to read the brightness value via dc_link_get_backlight_level() no matter whether it's controlled via aux or not, and this results in a bogus value returned. Fix it to read the current value via dc_link_get_backlight_level_nits() for the aux.
BugLink:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbu
gzilla.opensuse.org%2Fshow_bug.cgi%3Fid%3D1180749&data=04%7C01 %7
Calexander.deucher%40amd.com%7Ce5579cfe56f74b572f1508d8ca9ad0ac%7 C3d
d8961fe4884e608e11a82d994e183d%7C0%7C0%7C637482113562863043%7CU nknow
n%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1ha Wwi
LCJXVCI6Mn0%3D%7C1000&sdata=HVtqM2r6oxSWd3XGGQZotO8wrvM qCTcwfq1L
2%2FeCmSE%3D&reserved=0 BugLink:
https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgi
tlab.freedesktop.org%2Fdrm%2Famd%2F-
%2Fissues%2F1438&data=04%7C0
1%7Calexander.deucher%40amd.com%7Ce5579cfe56f74b572f1508d8ca9ad0 ac%7
C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637482113562863043% 7CUnk
nown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6I k1ha
WwiLCJXVCI6Mn0%3D%7C1000&sdata=TdYgwNJ%2FvkuoDLNb9ATFb1P yznlp%2F
P8TLuYSR%2BVkNqY%3D&reserved=0 Signed-off-by: Takashi Iwai tiwai@suse.de
This looks fine to me. FWIW, I have a similar patch set here: https://nam11.safelinks.protection.outlook.com/?url=https:%2F%2Fcgit.f
reedesktop.org%2F~agd5f%2Flinux%2Flog%2F%3Fh%3Dbacklight_wip& data=
04%7C01%7Calexander.deucher%40amd.com%7Ce5579cfe56f74b572f1508d8 ca9ad0
ac%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637482113562863 043%7CU
nknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI 6Ik1ha
WwiLCJXVCI6Mn0%3D%7C1000&sdata=aoMSY0nvHjrLocUPJtdgckqIH7x LUPbwpH0
ZjhuuJO8%3D&reserved=0
I'm fine to scratch mine as long as the issue gets fixed :)
FWIW, the biggest problem so far was the aux channel backlight didn't work as expected, the actual backlight isn't changed by the backlight sysfs write. (And the sysfs read gives a bogus value, but it's not the cause of the non- working backlight control.)
Does the aux channel backlight really work with the current code? Or is this rather a device-specific issue (e.g. broken BIOS) and we might need to come up with a deny list or such?
@Kazlauskas, Nicholas, @Siqueira, Rodrigo
Has there been any progress on the backlight fixes?
Alex
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.
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 --- 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 | 3 +++ 3 files changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index 5993dd0fdd8e..4793cd5e69f9 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h @@ -179,6 +179,7 @@ extern uint amdgpu_smu_memory_pool_size; extern uint amdgpu_dc_feature_mask; extern uint amdgpu_dc_debug_mask; extern uint amdgpu_dm_abm_level; +extern int amdgpu_aux_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 7169fb5e3d9c..5b66822da954 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -777,6 +777,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_aux_backlight = -1; +MODULE_PARM_DESC(aux_backlight, "Aux backlight control (0 = off, 1 = on, default auto)"); +module_param_named(aux_backlight, amdgpu_aux_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 fb62886ae013..6ad384ef61b8 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -2209,6 +2209,9 @@ 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_aux_backlight >= 0) + caps->aux_support = amdgpu_aux_backlight; + /* From the specification (CTA-861-G), for calculating the maximum * luminance we need to use: * Luminance = 50*2**(CV/32)
On Wed, Feb 3, 2021 at 7:42 AM Takashi Iwai tiwai@suse.de wrote:
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.
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
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 | 3 +++ 3 files changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index 5993dd0fdd8e..4793cd5e69f9 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h @@ -179,6 +179,7 @@ extern uint amdgpu_smu_memory_pool_size; extern uint amdgpu_dc_feature_mask; extern uint amdgpu_dc_debug_mask; extern uint amdgpu_dm_abm_level; +extern int amdgpu_aux_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 7169fb5e3d9c..5b66822da954 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -777,6 +777,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_aux_backlight = -1; +MODULE_PARM_DESC(aux_backlight, "Aux backlight control (0 = off, 1 = on, default auto)"); +module_param_named(aux_backlight, amdgpu_aux_backlight, bint, 0444);
I'd suggest making this something more generic like "backlight" and make -1 auto, 0 pwm, 1 aux. That way we can handle potential future types more cleanly.
Alex
/**
- 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 fb62886ae013..6ad384ef61b8 100644 --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c @@ -2209,6 +2209,9 @@ 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_aux_backlight >= 0)caps->aux_support = amdgpu_aux_backlight;/* From the specification (CTA-861-G), for calculating the maximum * luminance we need to use: * Luminance = 50*2**(CV/32)-- 2.26.2
amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
On Fri, 05 Feb 2021 17:34:36 +0100, Alex Deucher wrote:
On Wed, Feb 3, 2021 at 7:42 AM Takashi Iwai tiwai@suse.de wrote:
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.
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
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 | 3 +++ 3 files changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index 5993dd0fdd8e..4793cd5e69f9 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h @@ -179,6 +179,7 @@ extern uint amdgpu_smu_memory_pool_size; extern uint amdgpu_dc_feature_mask; extern uint amdgpu_dc_debug_mask; extern uint amdgpu_dm_abm_level; +extern int amdgpu_aux_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 7169fb5e3d9c..5b66822da954 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -777,6 +777,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_aux_backlight = -1; +MODULE_PARM_DESC(aux_backlight, "Aux backlight control (0 = off, 1 = on, default auto)"); +module_param_named(aux_backlight, amdgpu_aux_backlight, bint, 0444);
I'd suggest making this something more generic like "backlight" and make -1 auto, 0 pwm, 1 aux. That way we can handle potential future types more cleanly.
OK, will respin later.
thanks,
Takashi
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Deucher, Alexander -
Takashi Iwai