On 5/11/2023 8:57 AM, Dmitry Baryshkov wrote:
On 11/05/2023 18:53, Bjorn Andersson wrote:
On Thu, May 11, 2023 at 07:24:46AM +0300, Dmitry Baryshkov wrote:
On Wed, 10 May 2023 at 23:31, Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx>
wrote:
The internal_hpd flag was introduced to handle external DP HPD
derived from GPIO
pinmuxed into DP controller. 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>
Thanks for debugging this!
However after looking at the driver I think there is more than this.
We have several other places gated on internal_hpd flag, where we do
not have a strict ordering of events.
I see that dp_hpd_plug_handle() and dp_hpd_unplug_handle() also toggle
DP_DP_IRQ_HPD_INT_MASK and DP_DP_HPD_REPLUG_INT_MASK depending on
internal_hpd. Can we toggle all 4 interrupts from the
hpd_enable/hpd_disable functions? If we can do it, then I think we can
drop the internal_hpd flag completely.
Yes, that's what I believe the DRM framework intend us to do.
The problem, and reason why I didn't do tat in my series, was that in
order to update the INT_MASKs you need to clock the IP-block and that's
done elsewhere.
So, for the internal_hpd case, it seems appropriate to pm_runtime_get()
in hpd_enable() and unmask the HPD interrupts, and mask the interrupts
and pm_runtime_put() in hpd_disable().
But for edp and external HPD-signal we also need to make sure power is
on while something is connected...
I think this is already handled by the existing code, see calls to the
dp_display_host_init().
I went on and checked other places where it is used:
- dp_hpd_unplug_handle(), guarding DP_DP_HPD_PLUG_INT_MASK toggling. I
think we can drop these two calls completely. The function is under
the event_mutex protection, so other events can not interfere.
- dp_bridge_hpd_notify(). What is the point of this check? If some
other party informs us of the HPD event, we'd better handle it instead
of dropping it. Correct? In other words, I'd prefer seeing the
hpd_event_thread removal. Instead of that I think that on
HPD/plug/unplug/etc. IRQ the driver should call into the drm stack,
then the hpd_notify call should process those events.
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...
- dp_bridge_hpd_notify(). What is the point of this check? <== i have
below two questions,
1) can you explain when/what this dp_bridge_hpd_notify() will be called?
2) is dp_bridge_hpd_notify() only will be called at above case #2? and
it will not be used by case #1?
I agree, that seems to be what's expected of us from the DRM framework.
Regards,
Bjorn
---
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
@@ -1088,13 +1088,6 @@ static void dp_display_config_hpd(struct
dp_display_private *dp)
dp_display_host_init(dp);
dp_catalog_ctrl_hpd_config(dp->catalog);
- /* Enable plug and unplug interrupts only if requested */
- if (dp->dp_display.internal_hpd)
- dp_catalog_hpd_config_intr(dp->catalog,
- DP_DP_HPD_PLUG_INT_MASK |
- DP_DP_HPD_UNPLUG_INT_MASK,
- true);
-
/* Enable interrupt first time
* we are leaving dp clocks on during disconnect
* and never disable interrupt
@@ -1396,12 +1389,6 @@ static int dp_pm_resume(struct device *dev)
dp_catalog_ctrl_hpd_config(dp->catalog);
- if (dp->dp_display.internal_hpd)
- dp_catalog_hpd_config_intr(dp->catalog,
- DP_DP_HPD_PLUG_INT_MASK |
- DP_DP_HPD_UNPLUG_INT_MASK,
- true);
-
if (dp_catalog_link_is_connected(dp->catalog)) {
/*
* set sink to normal operation mode -- D0
@@ -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;
+ dp_catalog_hpd_config_intr(dp->catalog,
+ DP_DP_HPD_PLUG_INT_MASK |
+ DP_DP_HPD_UNPLUG_INT_MASK,
+ true);
}
void dp_bridge_hpd_disable(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_catalog_hpd_config_intr(dp->catalog,
+ DP_DP_HPD_PLUG_INT_MASK |
+ DP_DP_HPD_UNPLUG_INT_MASK,
+ false);
dp_display->internal_hpd = false;
}
--
With best wishes
Dmitry
