Message-ID: <CAE-0n50RXmaUsu5F9syJT-ZXzX8WacpJDFnhb1xQaou1Pxizng@mail.gmail.com>
Date:   Thu, 19 May 2022 17:34:18 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     Doug Anderson <dianders@...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

Quoting Doug Anderson (2022-05-12 16:24:13)
> On Wed, May 11, 2022 at 6:58 PM Stephen Boyd <swboyd@...omium.org> wrote:
> > Quoting Douglas Anderson (2022-04-18 10:17:54)
> > > 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.
>

Got it.

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

To clarify, are you saying that the 'wait' passed in will be added to
whatever time it takes for the driver to runtime resume to check HPD
status? Or is the driver supposed to subtract any time to power up from the
'wait' passed in and then poll or wait for an irq about HPD?

Would it be incorrect to somehow have the pm_runtime_get_sync() call in
the mythical wrapper API with a ktime_get() before and after and then
subtract that from the 'wait' time and call "get_immediate_hpd()"?

It would help me understand further if the 'wait' is described as a
maximum time we're willing to wait or a minimum time we're willing to
wait for hpd to be asserted. Usually a timeout is the maximum we're
willing to wait so I think you're saying the wait is the maximum time
after we know the drm_dp_aux is fully powered up and ready to check the
state.

Powered by blists - more mailing lists