Message-ID: <CAD=FV=W6Z1TG4vQcDDeNsGkjZVAR8=A1L1pDfo1rDFCh84H4Rg@mail.gmail.com>
Date:   Thu, 12 May 2022 16:24:13 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Stephen Boyd <swboyd@...omium.org>
Cc:     dri-devel <dri-devel@...ts.freedesktop.org>,
        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>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 1/4] drm/dp: Add wait_hpd_asserted() callback to struct drm_dp_aux

Hi,

On Wed, May 11, 2022 at 6:58 PM Stephen Boyd <swboyd@...omium.org> wrote:
>
> 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?

Not that I'm aware of. Originally I was planning to have it so that a
timeout of "0" meant to just poll without sleeping at all, but it
ended up making the code a lot more complicated because everywhere
else we had the "readx" semantics where 0 meant wait forever. It
didn't seem worth it. I can go back to that behavior if need be.


> > +        *
> > +        * 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)

There's a reason why I changed the API to "wait" from "get". If you
can think of a good place to document this, I'm all ears.

The basic problem is ps8640 (my nemesis, apparently). On ps8640,
because of the black box firmware blob that's on it, we have a crazy
long delay in its runtime resume (300ms). So what happens with ps8640
is that if we make the API "get_immediate_hpd()" it wasn't so
immediate. Even with autosuspend, that first "get" could take 300 ms,
which really screwed with everyone else who was waiting with a 200 ms
timeout.

Now, in theory, one could argue that the fact that ps8640 had a 300 ms
sleep would mean that the very first "get" of the panel would already
show HPD high. I don't know why that wasn't the case, but ps8640 is an
annoying black box.

In general, though, the DP controller might need some amount of time
to power itself back up and configure itself. Even though the ps8640
case is extreme, it wouldn't be totally extreme to assume that an AUX
controller might take 20 ms or 50 ms to power up. That could still
throw timings off. Implementing the API as a "wait" style API gets
around this problem. Now the DP controller can take as long as it
needs to power itself up and it can then wait with the requested
timeout.


> 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.
> >          */

Powered by blists - more mailing lists