linux-kernel - Re: [PATCH v1 1/2] drm/msm/dp: enable HDP plugin/unplugged interrupts to hpd

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-0n51Q0crYrELM1GnY5xbBezeOryRa4D5STR7TG_yqz0YOUA@mail.gmail.com>
Date:   Thu, 11 May 2023 10:55:10 -0700
From:   Stephen Boyd <swboyd@...omium.org>
To:     Abhinav Kumar <quic_abhinavk@...cinc.com>,
        Bjorn Andersson <andersson@...nel.org>
Cc:     Kuogee Hsieh <quic_khsieh@...cinc.com>, agross@...nel.org,
        airlied@...il.com, daniel@...ll.ch, dianders@...omium.org,
        dmitry.baryshkov@...aro.org, dri-devel@...ts.freedesktop.org,
        robdclark@...il.com, sean@...rly.run, vkoul@...nel.org,
        quic_jesszhan@...cinc.com, quic_sbillaka@...cinc.com,
        marijn.suijten@...ainline.org, freedreno@...ts.freedesktop.org,
        linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/2] drm/msm/dp: enable HDP plugin/unplugged interrupts
 to hpd_enable/disable

Quoting Bjorn Andersson (2023-05-11 08:44:16)
> On Wed, May 10, 2023 at 05:39:07PM -0700, Abhinav Kumar wrote:
> >
> >
> > On 5/10/2023 4:55 PM, Stephen Boyd wrote:
> > > Quoting Kuogee Hsieh (2023-05-10 13:31:04)
> > > > The internal_hpd flag was introduced to handle external DP HPD derived from GPIO
> > > > pinmuxed into DP controller.
> > >
> > > Was it? It looks more like it was done to differentiate between eDP and
> > > DP, because internal_hpd is set only if DRM_BRIDGE_OP_HPD is set on the
> > > bridge and we only set the bridge op if the connector type is DP. The
> > > assumption looks like if you have DP connector_type, you have the gpio
> > > pinmuxed for "dp_hot" mode, which isn't exactly true. We don't treat
> > > that gpio as an irq either, because it isn't. Instead the gpio is muxed
> > > to the mdss inside the SoC and then that generates an mdss interrupt
> > > that's combined with non-HPD things like "video ready".
> > >
> > > If that all follows, then I don't quite understand why we're setting
> > > internal_hpd to false at all at runtime. It should be set to true at
> > > some point, but ideally that point is during probe.
> > >
> >
> > Kuogee had the same thought originally but were not entirely sure of this
> > part of the commit message in Bjorn's original commit which introduced these
> > changes.
> >
> > "This difference is not appropriately represented by the "is_edp"
> > boolean, but is properly represented by the frameworks invocation of the
> > hpd_enable() and hpd_disable() callbacks. Switch the current condition
> > to rely on these callbacks instead"
> >
> > Does this along with below documentation mean we should generate the hpd
> > interrupts only after hpd_enable callback happens?
> >
> > " * Call &drm_bridge_funcs.hpd_enable if implemented and register the given
> > @cb
> >  * and @data as hot plug notification callback. From now on the @cb will be
> >  * called with @data when an output status change is detected by the bridge,
> >  * until hot plug notification gets disabled with drm_bridge_hpd_disable().
> > "
> >
> > Bjorn, can you please clarify this?
> >
>
> We currently have 3 cases:
>
> 1) DP with GPIO: No downstream drm_bridge are connected, is_edp = false
> and internal HPD-logic is in used (internal_hpd = true). Power needs to
> be on at all times etc.
>
> 2) DP without GPIO: Downstream drm_bridge connected, is_edp = false and
> internal HPD-logic should not be used/enabled (internal_hpd = false).
> Power doesn't need to be on unless hpd_notify is invoked to tell us that
> there's something connected...
>
> 3) eDP with or without HPD signal and/or HPD gpio. Downstream
> drm_bridge/panel is connected, is_edp = true and internal HPD logic is
> short-circuited regardless of the panel providing HPD signal or not.

Oh weird. I thought that the "is_edp" controller on sc7280 didn't have
HPD hardware in the PHY (phy@...2a00), hence all the logic to avoid
using the HPD interrupts and bits there. What is "is_edp" about then?

>
>
> In #1 dp_bridge_hpd_enable() will be invoked to indicate that the DP
> controller is expected to perform HPD handling. In #2
> dp_bridge_hpd_enable() will _not_ be invoked, instead some downstream
> drm_bridge/panel will get the hpd_enable() callback and will be
> responsible to updating the HPD state of the chain, which will cause
> hpd_notify to be invoked.
>
>
> Note that #3 is based entirely on the controller, it has currently no
> relation to what is attached. It seems reasonable that this is just
> another case of #2 (perhaps just always reporting
> connector_status_connected?).
>

Looking at drm_bridge_connector_detect() the default is to consider
DRM_MODE_CONNECTOR_eDP as connector_status_connected. I wonder if
panel_bridge_bridge_funcs can gain a 'detect' function and also set
DRM_BRIDGE_OP_DETECT if the connector_type is DRM_MODE_CONNECTOR_eDP.