| Message ID | 20210203124241.8512-2-tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/amd/display: some backlight fixes | expand |
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); > + 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); > -- > 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
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);
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(-)