linux-kernel - Re: [PATCH v4 3/5] drm/dp: Disable unsupported features in DP_EDP_BACKLIGHT_MODE_SET

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 28 Oct 2021 11:27:00 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Lyude Paul <lyude@...hat.com>
Cc:     Intel Graphics <intel-gfx@...ts.freedesktop.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        "open list:DRM DRIVER FOR NVIDIA GEFORCE/QUADRO GPUS" 
        <nouveau@...ts.freedesktop.org>,
        Satadru Pramanik <satadru@...il.com>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 3/5] drm/dp: Disable unsupported features in DP_EDP_BACKLIGHT_MODE_SET_REGISTER

Hi,

On Tue, Oct 26, 2021 at 3:09 PM Lyude Paul <lyude@...hat.com> wrote:
>
> As it turns out, apparently some machines will actually leave additional
> backlight functionality like dynamic backlight control on before the OS
> loads. Currently we don't take care to disable unsupported features when
> writing back the backlight mode, which can lead to some rather strange
> looking behavior when adjusting the backlight.
>
> So, let's fix this by ensuring we only keep supported features enabled for
> panel backlights - which should fix some of the issues we were seeing from
> this on fi-bdw-samus.
>
> Signed-off-by: Lyude Paul <lyude@...hat.com>
> Fixes: 867cf9cd73c3 ("drm/dp: Extract i915's eDP backlight code into DRM helpers")
> ---
>  drivers/gpu/drm/drm_dp_helper.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c
> index ada0a1ff262d..8f2032a955cf 100644
> --- a/drivers/gpu/drm/drm_dp_helper.c
> +++ b/drivers/gpu/drm/drm_dp_helper.c
> @@ -3372,7 +3372,9 @@ int drm_edp_backlight_enable(struct drm_dp_aux *aux, const struct drm_edp_backli
>                 return ret < 0 ? ret : -EIO;
>         }
>
> -       new_dpcd_buf = dpcd_buf;
> +       /* Disable any backlight functionality we don't support that might be on */
> +       new_dpcd_buf = dpcd_buf & (DP_EDP_BACKLIGHT_CONTROL_MODE_MASK |
> +                                  DP_EDP_BACKLIGHT_FREQ_AUX_SET_ENABLE);

My first thought when reading the above was: if we're masking so much
stuff out, why do we bother reading the old value back out at all?

I guess the two places you use the old value for are:

1. You avoid setting the "DP_EDP_PWMGEN_BIT_COUNT" if the backlight
was already configured for DPCD mode.

2. You avoid writing the register if you didn't change it.

I would actually argue that use #1 is probably a bug. If you're
worried about the firmware leaving the backlight configured in a
strange way, it could very well have left the backlight configured in
DPCD mode but set a different "bit count" than you want, right? Maybe
you should just always set the bit count?

Use #2 is fine, but does it buy you anything? Are writes to the DCPD
bus somehow more expensive than reads? ...or maybe you're expecting
that a display will glitch / act badly if you write the same value
that's already there?

So I guess my instinct here is that you should avoid reading all
together and just program the value you want.

-Doug