On 5/17/2023 11:48 AM, Dmitry Baryshkov wrote:
On Wed, 17 May 2023 at 20:42, Kuogee Hsieh <quic_khs...@quicinc.com> wrote:
On 5/16/2023 4:20 PM, Dmitry Baryshkov wrote:
On 17/05/2023 01:35, Abhinav Kumar wrote:
On 5/16/2023 6:50 AM, Dmitry Baryshkov wrote:
On 13/05/2023 00:28, Abhinav Kumar wrote:
Hi Bjorn and Dmitry
On 5/12/2023 12:34 PM, Kuogee Hsieh wrote:
On 5/12/2023 10:28 AM, Dmitry Baryshkov wrote:
On Fri, 12 May 2023 at 19:52, Kuogee Hsieh
<quic_khs...@quicinc.com> wrote:
On 5/11/2023 5:54 PM, Dmitry Baryshkov wrote:
On Fri, 12 May 2023 at 03:16, Kuogee Hsieh
<quic_khs...@quicinc.com> wrote:
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_khs...@quicinc.com>
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_khs...@quicinc.com>
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.
No we cannot. The HPD logic works in a flip-flop model. When we get
the plug interrupt, we need to flip to tell the hw to wait for
unplug and when we get unplug, we need to tell the hw to wait for
plug.
But, doesn't dp_display_config_hpd() (current code) or
dp_bridge_hpd_enable() (after this patch) enable both plug and
unplug interrupts? This doesn't fit well into the flip-flop
description.
Let me clarify / correct the response. Ideally thats what is usually
done to wait for disconnect when we get connect and vice-versa. HDMI
still does it the same way.
https://gitlab.freedesktop.org/drm/msm/-/blob/msm-next/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c#L196
So, HDMI HPD is real flip-flop, sounds fine.
But I checked with kuogee that DP always enabled HPD connect and
disconnect interrupts by default and he mentioned its mainly because
we wanted to enable HPD connect / disconnect by default but not the
others.
That being said, the logic is close to flip-flop that when you get a
connect event, you wait for the "other" interrupts which are possible
which is IRQ_HPD and REPLUG and during disconnect, those are not
possible so disable them. Thats why the calls to
dp_catalog_hpd_config_intr() are present in the plug_handle /
unplug_handle to enable the "other possible" interrupts.
Can we keep them always enabled? Are these interrupts edge-triggered
or level-triggered? What prevents us from enabling these interrupts
all the time? Or enabling all 4 interrupts in hpd_enable() and
disabling them in hpd_disable()? Can IRQ_HPD or REPLUG fire when the
cable is disconnected?
1) edge-trigger at hpd pin low. However hpd block will start
debouncing logic and set status bit (plug, unplug, hpd_irq ore replug)
properly base on the result of debunce.
2) there should be fine to have all interrupts enabled
3) IRQ_HPD and REPLUG will not happen when disconnected.
The logic from dp_display_config_hpd() is getting removed in this
patch, in case you didnt check to align just with hpd_enable /
hpd_disable callbacks.
I saw this. The code is being moved to dp_bridge_hpd_enable(), as I
mentioned in the email.
The two calls in plug_handle() / unplug_handle() are doing that
whereas hpd_enable/hpd_disable are disabling the hpd interrupts
altogether.
In other words, we cannot rely on hpd_enable() / hpd_disable()
calls to do the flip part as that has to be done every plug/unplug.
In addition we need to handle the compliance test cases with REPLUG.
Thank you for the explanation. Would it be possible to keep
mask/unmask, but make hpd_enable/hpd_disable toggle
DP_DP_HPD_CTRL_HPD_EN instead?
Yes, this should be possible but we would like to test this. But what
about the interrupt masks then. So you are saying, hpd_enable will
only toggle the DP_DP_HPD_CTRL_HPD_EN but leave the HPD connect and
disconnect interrupts intact? That also doesnt sound right.
enabling the block all the time and then toggling the interrupt masks
seemed like a better thing.
Why? We do not need the block outside of the
hpd_enable()/hpd_disable() pair. Even from the power consumption
perspective, disabling the unused block sounds better to me.
We are mean within the block of hpd_enabled and hdp_disabled pair,
At hpd_enabled, we will do both item 1) and 2) instead of just only item
1) as you mentioned. you still need power on hpd block to just do item2).
1) enabled DP_DP_HPD_CTRL_HPD_EN
2) enable HDP interrupts (plug, unplug, hpd_irq and replug)
If I got you correct (and you are proposing to move
DP_DP_HPD_CTRL_HPD_EN and toggling all 4 interrupts to
hpd_enable/hpd_disable), this should be fine with me. It will allow us
to remove most of the internal_hpd cruft while keeping the logic
functional. I'm looking forward to seeing this patch.
Yes, thats what he meant. It wont make sense to only toggle HPD_EN so we
will give it a try to toggle both HPD_EN and the 4 interrupts and
related logic.
The scope of this patch has increased resulting in the need for more
re-validation. So the v2 shall be only posted next week.
