linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Alex Deucher <alexdeucher@gmail.com>
To: Alexander Monakov <amonakov@ispras.ru>
Cc: amd-gfx list <amd-gfx@lists.freedesktop.org>,
	Alex Deucher <alexander.deucher@amd.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
Subject: Re: [PATCH v2] drm/amd/display: use correct scale for actual_brightness
Date: Tue, 18 Aug 2020 12:15:00 -0400	[thread overview]
Message-ID: <CADnq5_OqtuHy6oAQkuZvDxR4OsLSZRDoLazXxD15m7y0hWtH2w@mail.gmail.com> (raw)
In-Reply-To: <CADnq5_OaTdfVGCvENTr3tDrU+9jR0VYo-49sjByOQR4EwDWwPA@mail.gmail.com>

Applied.  Thanks!

Alex

On Mon, Aug 17, 2020 at 1:59 PM Alex Deucher <alexdeucher@gmail.com> wrote:
>
> On Mon, Aug 17, 2020 at 3:09 AM Alexander Monakov <amonakov@ispras.ru> wrote:
> >
> > Ping.
>
> Patch looks good to me:
> Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
>
> Nick, unless you have any objections, I'll go ahead and apply it.
>
> Alex
>
> >
> > On Tue, 4 Aug 2020, Alexander Monakov wrote:
> >
> > > Documentation for sysfs backlight level interface requires that
> > > values in both 'brightness' and 'actual_brightness' files are
> > > interpreted to be in range from 0 to the value given in the
> > > 'max_brightness' file.
> > >
> > > With amdgpu, max_brightness gives 255, and values written by the user
> > > into 'brightness' are internally rescaled to a wider range. However,
> > > reading from 'actual_brightness' gives the raw register value without
> > > inverse rescaling. This causes issues for various userspace tools such
> > > as PowerTop and systemd that expect the value to be in the correct
> > > range.
> > >
> > > Introduce a helper to retrieve internal backlight range. Use it to
> > > reimplement 'convert_brightness' as 'convert_brightness_from_user' and
> > > introduce 'convert_brightness_to_user'.
> > >
> > > Bug: https://bugzilla.kernel.org/show_bug.cgi?id=203905
> > > Bug: https://gitlab.freedesktop.org/drm/amd/-/issues/1242
> > > Cc: Alex Deucher <alexander.deucher@amd.com>
> > > Cc: Nicholas Kazlauskas <nicholas.kazlauskas@amd.com>
> > > Signed-off-by: Alexander Monakov <amonakov@ispras.ru>
> > > ---
> > > v2: split convert_brightness to &_from_user and &_to_user (Nicholas)
> > >
> > >  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c | 81 +++++++++----------
> > >  1 file changed, 40 insertions(+), 41 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 710edc70e37e..b60a763f3f95 100644
> > > --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> > > @@ -2881,51 +2881,50 @@ static int set_backlight_via_aux(struct dc_link *link, uint32_t brightness)
> > >       return rc ? 0 : 1;
> > >  }
> > >
> > > -static u32 convert_brightness(const struct amdgpu_dm_backlight_caps *caps,
> > > -                           const uint32_t user_brightness)
> > > +static int get_brightness_range(const struct amdgpu_dm_backlight_caps *caps,
> > > +                             unsigned *min, unsigned *max)
> > >  {
> > > -     u32 min, max, conversion_pace;
> > > -     u32 brightness = user_brightness;
> > > -
> > >       if (!caps)
> > > -             goto out;
> > > +             return 0;
> > >
> > > -     if (!caps->aux_support) {
> > > -             max = caps->max_input_signal;
> > > -             min = caps->min_input_signal;
> > > -             /*
> > > -              * The brightness input is in the range 0-255
> > > -              * It needs to be rescaled to be between the
> > > -              * requested min and max input signal
> > > -              * It also needs to be scaled up by 0x101 to
> > > -              * match the DC interface which has a range of
> > > -              * 0 to 0xffff
> > > -              */
> > > -             conversion_pace = 0x101;
> > > -             brightness =
> > > -                     user_brightness
> > > -                     * conversion_pace
> > > -                     * (max - min)
> > > -                     / AMDGPU_MAX_BL_LEVEL
> > > -                     + min * conversion_pace;
> > > +     if (caps->aux_support) {
> > > +             // Firmware limits are in nits, DC API wants millinits.
> > > +             *max = 1000 * caps->aux_max_input_signal;
> > > +             *min = 1000 * caps->aux_min_input_signal;
> > >       } else {
> > > -             /* TODO
> > > -              * We are doing a linear interpolation here, which is OK but
> > > -              * does not provide the optimal result. We probably want
> > > -              * something close to the Perceptual Quantizer (PQ) curve.
> > > -              */
> > > -             max = caps->aux_max_input_signal;
> > > -             min = caps->aux_min_input_signal;
> > > -
> > > -             brightness = (AMDGPU_MAX_BL_LEVEL - user_brightness) * min
> > > -                            + user_brightness * max;
> > > -             // Multiple the value by 1000 since we use millinits
> > > -             brightness *= 1000;
> > > -             brightness = DIV_ROUND_CLOSEST(brightness, AMDGPU_MAX_BL_LEVEL);
> > > +             // Firmware limits are 8-bit, PWM control is 16-bit.
> > > +             *max = 0x101 * caps->max_input_signal;
> > > +             *min = 0x101 * caps->min_input_signal;
> > >       }
> > > +     return 1;
> > > +}
> > >
> > > -out:
> > > -     return brightness;
> > > +static u32 convert_brightness_from_user(const struct amdgpu_dm_backlight_caps *caps,
> > > +                                     uint32_t brightness)
> > > +{
> > > +     unsigned min, max;
> > > +
> > > +     if (!get_brightness_range(caps, &min, &max))
> > > +             return brightness;
> > > +
> > > +     // Rescale 0..255 to min..max
> > > +     return min + DIV_ROUND_CLOSEST((max - min) * brightness,
> > > +                                    AMDGPU_MAX_BL_LEVEL);
> > > +}
> > > +
> > > +static u32 convert_brightness_to_user(const struct amdgpu_dm_backlight_caps *caps,
> > > +                                   uint32_t brightness)
> > > +{
> > > +     unsigned min, max;
> > > +
> > > +     if (!get_brightness_range(caps, &min, &max))
> > > +             return brightness;
> > > +
> > > +     if (brightness < min)
> > > +             return 0;
> > > +     // Rescale min..max to 0..255
> > > +     return DIV_ROUND_CLOSEST(AMDGPU_MAX_BL_LEVEL * (brightness - min),
> > > +                              max - min);
> > >  }
> > >
> > >  static int amdgpu_dm_backlight_update_status(struct backlight_device *bd)
> > > @@ -2941,7 +2940,7 @@ static int amdgpu_dm_backlight_update_status(struct backlight_device *bd)
> > >
> > >       link = (struct dc_link *)dm->backlight_link;
> > >
> > > -     brightness = convert_brightness(&caps, bd->props.brightness);
> > > +     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);
> > > @@ -2958,7 +2957,7 @@ static int amdgpu_dm_backlight_get_brightness(struct backlight_device *bd)
> > >
> > >       if (ret == DC_ERROR_UNEXPECTED)
> > >               return bd->props.brightness;
> > > -     return ret;
> > > +     return convert_brightness_to_user(&dm->backlight_caps, ret);
> > >  }
> > >
> > >  static const struct backlight_ops amdgpu_dm_backlight_ops = {
> > >
> > > base-commit: bcf876870b95592b52519ed4aafcf9d95999bc9c
> > >
> > _______________________________________________
> > amd-gfx mailing list
> > amd-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/amd-gfx

  reply	other threads:[~2020-08-18 16:15 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-04 20:13 Alexander Monakov
2020-08-09 16:26 ` Alexander Monakov
2020-08-16  8:49 ` Alexander Monakov
2020-08-17 17:59   ` Alex Deucher
2020-08-18 16:15     ` Alex Deucher [this message]
2020-08-18 16:48       ` Kazlauskas, Nicholas

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CADnq5_OqtuHy6oAQkuZvDxR4OsLSZRDoLazXxD15m7y0hWtH2w@mail.gmail.com \
    --to=alexdeucher@gmail.com \
    --cc=alexander.deucher@amd.com \
    --cc=amd-gfx@lists.freedesktop.org \
    --cc=amonakov@ispras.ru \
    --cc=linux-kernel@vger.kernel.org \
    --cc=nicholas.kazlauskas@amd.com \
    --subject='Re: [PATCH v2] drm/amd/display: use correct scale for actual_brightness' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).