Re: [PATCH v1 1/2] drm/msm/dp: enable HDP plugin/unplugged interrupts to hpd_enable/disable
From: Abhinav Kumar
Date: Wed May 10 2023 - 20:39:26 EST
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?
HPD plug/unplug interrupts cannot be enabled until
internal_hpd flag is set to true.
At both bootup and resume time, the DP driver will enable external DP
plugin interrupts and handle plugin interrupt accordingly. Unfortunately
dp_bridge_hpd_enable() bridge ops function was called to set internal_hpd
flag to true later than where DP driver expected during bootup time.
This causes external DP plugin event to not get detected and display stays blank.
Move enabling HDP plugin/unplugged interrupts to dp_bridge_hpd_enable()/disable() to
set internal_hpd to true along with enabling HPD plugin/unplugged interrupts
simultaneously to avoid timing issue during bootup and resume.
Fixes: cd198caddea7 ("drm/msm/dp: Rely on hpd_enable/disable callbacks")
Signed-off-by: Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx>
---
drivers/gpu/drm/msm/dp/dp_display.c | 27 ++++++++++++++-------------
1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
index 3e13acdf..71aa944 100644
--- a/drivers/gpu/drm/msm/dp/dp_display.c
+++ b/drivers/gpu/drm/msm/dp/dp_display.c
@@ -1801,15 +1788,29 @@ void dp_bridge_hpd_enable(struct drm_bridge *bridge)
{
struct msm_dp_bridge *dp_bridge = to_dp_bridge(bridge);
struct msm_dp *dp_display = dp_bridge->dp_display;
+ struct dp_display_private *dp;
+
+ dp = container_of(dp_display, struct dp_display_private, dp_display);
dp_display->internal_hpd = true;
Can we set internal_hpd to true during probe when we see that the hpd
pinmux exists? Or do any of these bits toggle in the irq status register
when the gpio isn't muxed to "dp_hot" or the controller is for eDP and
it doesn't have any gpio connection internally? I'm wondering if we can
get by with simply enabling the "dp_hot" pin interrupts
(plug/unplug/replug/irq_hpd) unconditionally and not worrying about them
if eDP is there (because the pin doesn't exist inside the SoC), or if DP
HPD is being signalled out of band through type-c framework.