linux-kernel - Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux

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: <CAD=FV=UG7k4A+hMXxwju-0mLddD1oJdGngXMkMA-dO3AxOx0rQ@mail.gmail.com>
Date:   Fri, 8 Apr 2022 06:43:28 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc:     Abhinav Kumar <quic_abhinavk@...cinc.com>,
        "Sankeerth Billakanti (QUIC)" <quic_sbillaka@...cinc.com>,
        quic_kalyant <quic_kalyant@...cinc.com>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        quic_vproddut <quic_vproddut@...cinc.com>,
        David Airlie <airlied@...ux.ie>,
        linux-arm-msm <linux-arm-msm@...r.kernel.org>,
        "Kuogee Hsieh (QUIC)" <quic_khsieh@...cinc.com>,
        freedreno <freedreno@...ts.freedesktop.org>,
        dri-devel <dri-devel@...ts.freedesktop.org>,
        "bjorn.andersson@...aro.org" <bjorn.andersson@...aro.org>,
        Sean Paul <seanpaul@...omium.org>,
        "Aravind Venkateswaran (QUIC)" <quic_aravindh@...cinc.com>,
        Stephen Boyd <swboyd@...omium.org>,
        Sean Paul <sean@...rly.run>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v6 1/8] drm/msm/dp: Add eDP support via aux_bus

Hi,

On Fri, Apr 8, 2022 at 5:20 AM Dmitry Baryshkov
<dmitry.baryshkov@...aro.org> wrote:
>
> > I guess my thought was that in DP you could still create the AUX bus
> > at probe time. Then for DP you just return an instant "transfer
> > failed" from the AUX bus if HPD isn't asserted. For eDP (as discussed
> > elsewhere) when we try to do an AUX transfer then we delay until HPD
> > is there.
>
> I think panel-edp would already handle the delay, so we do not need to
> have this logic in the DP driver.

There's a whole discussion about this between Stephen and me in patch
#5 ("drm/msm/dp: wait for hpd high before any sink interaction").
Basically:

* If panel HPD is hooked up to the dedicated HPD pin on the eDP
controller then the panel driver doesn't have a way to read it.

* We can't leverage the existing "HPD" query functions in DRM because
those indicate whether a panel is _physically_ connected. For eDP, it
always is.

For now the rule is that the AUX transfer function is in charge of
waiting for HPD for eDP if the dedicated HPD pin is used. If we want
to re-invent this we could, but that system works, isn't _too_ ugly,
and we're already making big enough changes in this series.

> > So we can still acquire resources (clocks, PHY, io maps, etc) at probe
> > time for DP and create the AUX bus, right? It will just return
> > "-ENODEV" if HPD isn't asserted and you're DP?
>
> Yes, please. I still suppose that we'd need a separate case to
> power_on eDP's PHY during the probe time. Maybe I'm mistaken here.

I think the ideal way is to do it like Kieran's proposal for sn65dsi86:

https://lore.kernel.org/r/20220317131250.1481275-4-kieran.bingham+renesas@ideasonboard.com/

* When enabling HPD (physical hot plug detect) in the hpd_enable()
callback you do a pm_runtime_get(). You do the
pm_runtime_put_autosuspend() when disabling. This is only used for DP
since we only provide DRM_BRIDGE_OP_HPD for DP, not for eDP.

* We do a pm_runtime_get() / pm_runtime_put_autosuspend() in the AUX
transfer routine. While holding the pm_runtime reference we check HPD.
For DP we return immediately if HPD isn't asserted. For eDP, we delay.

* We do the pm_runtime_get() in pre_enable and the pm_runtime_put() in
post_disable. For DP this will add a 2nd refcount (since we probably
were holding the reference for HPD). For eDP this will cause us to
power on.

* If there's any other time we need to read HW registers, and we
aren't guaranteed to already have a pm_runtime reference (like during
probe), we can do a temporary pm_runtime_get() /
pm_runtime_put_autosuspend().