linux-kernel - Re: [PATCH v3 1/4] drm/dp: Add wait_hpd_asserted() callback to 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAE-0n51iNXN4oOP-wAqrm9U6qC84fQ+qMUBu0BODXjsCDk+H=w@mail.gmail.com>
Date:   Wed, 11 May 2022 18:58:32 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     Douglas Anderson <dianders@...omium.org>,
        dri-devel@...ts.freedesktop.org
Cc:     Hsin-Yi Wang <hsinyi@...omium.org>,
        Sankeerth Billakanti <quic_sbillaka@...cinc.com>,
        Philip Chen <philipchen@...omium.org>,
        Robert Foss <robert.foss@...aro.org>,
        Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Abhinav Kumar <quic_abhinavk@...cinc.com>,
        Daniel Vetter <daniel@...ll.ch>,
        David Airlie <airlied@...ux.ie>,
        Jani Nikula <jani.nikula@...el.com>,
        Kees Cook <keescook@...omium.org>,
        Lyude Paul <lyude@...hat.com>,
        Maxime Ripard <maxime@...no.tech>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/4] drm/dp: Add wait_hpd_asserted() callback to struct drm_dp_aux

Quoting Douglas Anderson (2022-04-18 10:17:54)
> Sometimes it's useful for users of the DP AUX bus (like panels) to be
> able to poll HPD. Let's add a callback that allows DP AUX busses
> drivers to provide this.
>
> Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
> Signed-off-by: Douglas Anderson <dianders@...omium.org>
> ---
> Left Dmitry's Reviewed-by tag off since patch changed enough.
>
> (no changes since v2)
>
> Changes in v2:
> - Change is_hpd_asserted() to wait_hpd_asserted()
>
>  include/drm/dp/drm_dp_helper.h | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
>
> diff --git a/include/drm/dp/drm_dp_helper.h b/include/drm/dp/drm_dp_helper.h
> index 53d1e722f4de..0940c415db8c 100644
> --- a/include/drm/dp/drm_dp_helper.h
> +++ b/include/drm/dp/drm_dp_helper.h
> @@ -2035,6 +2035,32 @@ struct drm_dp_aux {
>         ssize_t (*transfer)(struct drm_dp_aux *aux,
>                             struct drm_dp_aux_msg *msg);
>
> +       /**
> +        * @wait_hpd_asserted: wait for HPD to be asserted
> +        *
> +        * This is mainly useful for eDP panels drivers to wait for an eDP
> +        * panel to finish powering on. This is an optional function.

Is there any use for the opposite direction? For example, does anything
care that HPD is deasserted?

> +        *
> +        * This function will efficiently wait for up to `wait_us` microseconds
> +        * for HPD to be asserted and might sleep.
> +        *
> +        * This function returns 0 if HPD was asserted or -ETIMEDOUT if time
> +        * expired and HPD wasn't asserted. This function should not print
> +        * timeout errors to the log.
> +        *
> +        * The semantics of this function are designed to match the
> +        * readx_poll_timeout() function. That means a `wait_us` of 0 means
> +        * to wait forever. If you want to do a quick poll you could pass 1
> +        * for `wait_us`.

It would also make sense to have a drm_dp_wait_hpd_asserted() API

  int drm_dp_wait_hpd_asserted(struct drm_dp_aux *aux, unsigned long wait_us);

and then this aux function could be implemented in various ways. The API
could poll if the aux can only read immediate state of HPD, or it could
sleep (is sleeping allowed? that isn't clear) and wake up the process
once HPD goes high. Or if this op isn't implemented maybe there's a
fixed timeout member that is non-zero which means "sleep this long".
Either way, making each drm_dp_aux implement that logic seems error
prone vs. having the drm_dp_aux implement some function for

	get_immediate_hpd(struct drm_dp_aux *aux)

or

	notify_on_hpd(struct drm_dp_aux *auxstruct completion *comp)

> +        *
> +        * NOTE: this function specifically reports the state of the HPD pin
> +        * that's associated with the DP AUX channel. This is different from
> +        * the HPD concept in much of the rest of DRM which is more about
> +        * physical presence of a display. For eDP, for instance, a display is
> +        * assumed always present even if the HPD pin is deasserted.
> +        */
> +       int (*wait_hpd_asserted)(struct drm_dp_aux *aux, unsigned long wait_us);
> +
>         /**
>          * @i2c_nack_count: Counts I2C NACKs, used for DP validation.
>          */