linux-kernel - Re: [PATCH v2 3/4] drm/panel: atna33xc20: Take advantage of wait_hpd_asserted() in struct drm_dp

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] [day] [month] [year] [list]

Message-ID: <CAD=FV=Xd27oxROMN18svqhLi5pdyi0AED3ksCwwPYB7ESr8J0g@mail.gmail.com>
Date:   Mon, 18 Apr 2022 10:03:14 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     dri-devel <dri-devel@...ts.freedesktop.org>
Cc:     Stephen Boyd <swboyd@...omium.org>,
        Abhinav Kumar <quic_abhinavk@...cinc.com>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Philip Chen <philipchen@...omium.org>,
        Robert Foss <robert.foss@...aro.org>,
        Sankeerth Billakanti <quic_sbillaka@...cinc.com>,
        Hsin-Yi Wang <hsinyi@...omium.org>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...ux.ie>,
        Sam Ravnborg <sam@...nborg.org>,
        Thierry Reding <thierry.reding@...il.com>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2 3/4] drm/panel: atna33xc20: Take advantage of
 wait_hpd_asserted() in struct drm_dp_aux

Hi,

On Mon, Apr 18, 2022 at 9:58 AM Douglas Anderson <dianders@...omium.org> wrote:
>
> Let's add support for being able to read the HPD pin even if it's
> hooked directly to the controller. This will let us take away the
> waiting in the AUX transfer functions of the eDP controller drivers.
>
> Signed-off-by: Douglas Anderson <dianders@...omium.org>
> ---
>
> Changes in v2:
> - Change is_hpd_asserted() to wait_hpd_asserted()
>
>  .../gpu/drm/panel/panel-samsung-atna33xc20.c  | 38 ++++++++++++-------
>  1 file changed, 25 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c b/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
> index 20666b6217e7..7f9af3e9aeb8 100644
> --- a/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
> +++ b/drivers/gpu/drm/panel/panel-samsung-atna33xc20.c
> @@ -19,6 +19,10 @@
>  #include <drm/drm_edid.h>
>  #include <drm/drm_panel.h>
>
> +/* T3 VCC to HPD high is max 200 ms */
> +#define HPD_MAX_MS     200
> +#define HPD_MAX_US     (HPD_MAX_MS * 1000)
> +
>  struct atana33xc20_panel {
>         struct drm_panel base;
>         bool prepared;
> @@ -30,6 +34,7 @@ struct atana33xc20_panel {
>
>         struct regulator *supply;
>         struct gpio_desc *el_on3_gpio;
> +       struct drm_dp_aux *aux;
>
>         struct edid *edid;
>
> @@ -90,20 +95,25 @@ static int atana33xc20_resume(struct device *dev)
>                 return ret;
>         p->powered_on_time = ktime_get();
>
> -       /*
> -        * Handle HPD. Note: if HPD is hooked up to a dedicated pin on the
> -        * eDP controller then "no_hpd" will be false _and_ "hpd_gpio" will be
> -        * NULL. It's up to the controller driver to wait for HPD after
> -        * preparing the panel in that case.
> -        */
>         if (p->no_hpd) {
> -               /* T3 VCC to HPD high is max 200 ms */
> -               msleep(200);
> -       } else if (p->hpd_gpio) {
> -               ret = readx_poll_timeout(gpiod_get_value_cansleep, p->hpd_gpio,
> -                                        hpd_asserted, hpd_asserted,
> -                                        1000, 200000);
> -               if (!hpd_asserted)
> +               msleep(HPD_MAX_MS);
> +       } else {
> +               if (p->hpd_gpio)
> +                       ret = readx_poll_timeout(gpiod_get_value_cansleep,
> +                                                p->hpd_gpio, hpd_asserted,
> +                                                hpd_asserted, 1000, HPD_MAX_US);
> +               else if (p->aux->wait_hpd_asserted)
> +                       ret = p->aux->wait_hpd_asserted(p->aux, HPD_MAX_US);
> +
> +               /*
> +                * Note that it's possible that no_hpd is false, hpd_gpio is
> +                * NULL, and wait_hpd_asserted is NULL. This is because
> +                * wait_hpd_asserted() is optional even if HPD is hooked up to
> +                * a dedicated pin on the eDP controller. In this case we just
> +                * assume that the controller driver will wait for HPD at the
> +                * right times.
> +                */
> +               if (!hpd_asserted && (p->hpd_gpio || p->aux->wait_hpd_asserted))
>                         dev_warn(dev, "Timeout waiting for HPD\n");

Ugh, right after I sent this out I found a bug! :( It should be
checking for "ret", not "hpd_asserted" in the above "if" test. I'll
spin out a quick v3 right away!

-Doug