Re: [PATCH 2/3] dt-bindings: drm/bridge: ti-sn65dsi86: Add hpd-gpios to the bindings

From: Doug Anderson <dianders@chromium.org>
To: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Cc: Stephen Boyd <swboyd@chromium.org>,
	Andrzej Hajda <a.hajda@samsung.com>,
	David Airlie <airlied@linux.ie>, Daniel Vetter <daniel@ffwll.ch>,
	Neil Armstrong <narmstrong@baylibre.com>,
	Rob Herring <robh+dt@kernel.org>,
	Sandeep Panda <spanda@codeaurora.org>,
	Jonas Karlman <jonas@kwiboo.se>,
	Bjorn Andersson <bjorn.andersson@linaro.org>,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>,
	Jeffrey Hugo <jeffrey.l.hugo@gmail.com>,
	Jernej Skrabec <jernej.skrabec@siol.net>,
	linux-arm-msm <linux-arm-msm@vger.kernel.org>,
	Rob Clark <robdclark@chromium.org>,
	dri-devel <dri-devel@lists.freedesktop.org>,
	LKML <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH 2/3] dt-bindings: drm/bridge: ti-sn65dsi86: Add hpd-gpios to the bindings
Date: Mon, 20 Apr 2020 22:10:17 -0700	[thread overview]
Message-ID: <CAD=FV=UpYALN6xrN5bpZTqqPLVUDB-MJ7BaQE28vrSRR3b+8MA@mail.gmail.com> (raw)
In-Reply-To: <20200417180819.GE5861@pendragon.ideasonboard.com>

Hi,

On Fri, Apr 17, 2020 at 11:08 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> As for the hpd-gpios, it should be specified in the DT node of the
> component that provides the HPD signal, and contain a GPIO specifier
> describing what the signal is connected to. When dealing with a physical
> DP connector and external monitor, the HPD signal is provided by the DP
> connector, the hpd-gpios property shall then be specified in the DP
> connector DT node. The display-connector driver already handles that
> property. When dealing with an eDP panel, the HPD signal is provided by
> the panel, the hpd-gpios property shall be specified in the panel DT
> node.

OK, patch posted to add "hpd-gpios" to "panel-common.yaml" which is I
think the summary of what you're saying above.

I _think_ this also means that I need to add support to panel-simple.c
for it so I've posted got a patch for that.  If I followed your whole
description of the future plans it might eventually move somewhere
else but we're not there yet.  If I screwed this up hopefully it's OK
to continue the conversation in v2.  It seemed nice to have code to
talk about.

> As the SN65DSI86 has native HPD detect capability with a dedicated HPD
> input (note that this doesn't make the SN65DSI86 a providder of the HPD
> signal in the sense described above), the bridge driver, in the new
> model, shall implement the HPD-related operations and the .detect()
> operation. The drm_bridge_connector_init() helper will then delegate HPD
> and detection to the ti-sn65dsi86 driver.

I guess this assumes that anyone ever uses it.  Right now the driver
hardcodes HPD to be off and it seems hard for me to imagine anyone
would have a real use for the hardware line given the terrible
debouncing.  Maybe a panel whose hardcoded delay is super bad?

> The new drm_bridge model has support for this use case. It makes a
> difference between the intrinsic capability of a device to provide a
> feature (for instance the SN65DSI86 has the intrinsic capability to
> provide the HPD feature), and the fact that the feature is actually
> provided by that device on a particular system (in the case you describe
> here, the SN65DSI86 intrinsic HPD capability isn't used, as the HPD
> signal isn't connect to the SN65DSI86 HPD input). The former is reported
> by implementing the corresponding drm_bridge_funcs operations, the
> latter is reported by setting DRM_BRIGE_OP_* flags in drm_bridge.ops.
> This mechanism allows bridge drivers to unconditionally set function
> pointers in their drm_bridge_funcs structure (allowing the structure to
> make stored in read-only memory), while exposing, for each device
> instance, whether the feature is actually provided or not.
>
> The drm_bridge_connector_init() helper, to delegate drm_connector
> operations to bridges, will look for the first bridge in the chain,
> starting at the output of the pipeline (connector or panel), that
> supports the corresponding feature. If your DP connector DT node, or
> your eDP connector DT node, specifies that the HPD signal is routed to a
> GPIO (through the hpd-gpios property), then the corresponding bridge
> driver shall reprot the DRM_BRIDGE_OP_DETECT and DRM_BRIDGE_OP_HPD
> capabilities. The display-connector driver already supports this, the
> panel bridge driver doesn't and needs to be extended. The
> drm_bridge_connector_init() helper will then detect that the drm_bridge
> for the DP connector or eDP panel supports HPD, and will delegate the
> related drm_connector operations to that bridge. If the HPD signal is
> routed to the HPD pin of the SN65DSI86, the DP connector or eDP panel DT
> node should not contain an hpd-gpios property, the corresponding
> drm_bridge will not set DRM_BRIDGE_OP_DETECT and DRM_BRIDGE_OP_HPD, and
> the drm_bridge_connector_init() will look at the next component in the
> next bridge in the chain, which will be the ti-sn65dsi86. That bridge
> will report support for the HPD-related operations, and will be used.
>
> To be fully correct the ti-sn65dsi86 shouldn't set the
> DRM_BRIDGE_OP_DETECT and DRM_BRIDGE_OP_HPD flags when the HPD signal
> isn't routed to its HPD input pin. As it should not peek into the DT
> node of the DP connector or eDP panel for its output, it should have an
> additional no-hpd DT property in this case. In practice that's may not
> always be required, as if an hpd-gpios property is specified in the DP
> connector or eDP panel DT node, the drm_bridge_connector_init() will not
> look further, but for the case where the HPD signal isn't routed
> anywhere, we need to make sure that the ti-sn65dsi86 driver will not
> incorrectly advertise HPD support.

Sounds like you've thought out a lot of the corner cases!

Right now the 'ti-sn65dsi86' driver is hardcoded not to look at HPD
but its bindings doesn't have the 'no-hpd' property.  Sounds like that
should be OK-ish as long as the panel either has "hpd-gpios" or
"no-hpd" because then nobody will actually query the bridge.  ...but
it would be cleaner to add it.

> > 5. The GPIOs on 'ti,sn65dsi86' cannot generate IRQs and can only be
> > polled.  ...but this is OK.  I'm specifically trying to support the
> > case of a panel that is always connected and I just want HPD to be the
> > signal that the panel is ready for me to talk to it.  Polling is fine.
> > Specifically the bridge driver doesn't try to poll HPD to decide if we
> > have something connected--it always returns
> > 'connector_status_connected'.  ...and this is the correct behavior for
> > eDP because you know the hardware is always there and HPD won't even
> > be asserted until you start to power up the panel.
>
> If you look at bridge/display-connector.c, you will see that it reports
> DRM_BRIDGE_OP_DETECT if there's an hpd-gpios property, and additionally
> reports DRM_BRIDGE_OP_HPD if that GPIO has interrupt capability. If a
> bridge in the pipeline reports DRM_BRIDGE_OP_DETECT but no bridge
> reports DRM_BRIDGE_OP_HPD, drm_bridge_connector_init() creates a
> connector that uses polling. This is another reason why a no-hpd
> property is needed for the ti,sn65dsi86, as otherwise the helper would
> incorrectly consider that the SN65DSI86 will report HPD through an
> interrupt.

Hrm.  I guess technically it breaks bindings compatibility that
"no-hpd" wasn't there before but there's something that will break if
we don't specify it.  ...but it won't break anything until someone
actually tries to add DRM_BRIDGE_OP_HPD to ti-sn65dsi86.  Maybe we're
OK as long as we fix it before then?

I've put this in v2 so we can discuss.

> > 6. My current implementation in patch #3 actually doesn't fully
> > implement a Linux GPIO provider in the bridge driver.  See that patch
> > for justification.  While I could do the work to do this and I'll do
> > it if folks insist, I think the current simpler code is nice.  If
> > there was a separate "edp-connector" driver then presumably I'd have
> > to add the complexity of implementing the GPIO provider API.
>
> This is the only reason why I don't like asking you to change your
> implementation, due to the additional complexity required to expose a
> GPIO provider. However, I think that the new bridge usage model is much
> cleaner than the current one, and this justifies in my opinion
> additional complexity in a small number of places, even if it's
> unfortunate. That being said, if we can put the DT properties where they
> belong for the new model with isolated bridge drivers to only handle the
> features of the hardware they correspond to, I wouldn't be opposed to a
> localized hack (without any derogatory meaning implied) on the driver
> side to ease the implementation. I'm willing to look at you at how this
> could be done, once we complete this discussion about the new model,
> with the hard rule that DT bindings should be designed based on the new
> model.

OK, I managed to implement the GPIO controller.  Let's see how it
looks.  I threw GPIO folks on the series too so hopefully they can
tell me if I'm doing something stupid.

-Doug