RE: [PATCH 1/2] drm/amd/display: Fix the brightness read via aux

2021-02-08 Thread Deucher, Alexander 
[AMD Public Use]

> -Original Message-
> From: Takashi Iwai 
> Sent: Saturday, February 6, 2021 7:29 AM
> To: Alex Deucher 
> Cc: Deucher, Alexander ; Koenig, Christian
> ; Li, Sun peng (Leo) ;
> Wentland, Harry ; Maling list - DRI developers
> ; amd-gfx list  g...@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  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 
> >
> > 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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/amd/display: Fix the brightness read via aux

2021-02-06 Thread Takashi Iwai 
On Fri, 05 Feb 2021 17:36:44 +0100,
Alex Deucher wrote:
> 
> On Wed, Feb 3, 2021 at 7:42 AM Takashi Iwai  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 
> 
> 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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm/amd/display: Fix the brightness read via aux

2021-02-05 Thread Alex Deucher 
On Wed, Feb 3, 2021 at 7:42 AM Takashi Iwai  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 

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-...@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel