[Freedreno] [PATCH v1 1/2] drm/msm/dp: enable HDP plugin/unplugged interrupts to hpd_enable/disable

[Abhinav Kumar]

On 5/17/2023 11:48 AM, Dmitry Baryshkov wrote:
> On Wed, 17 May 2023 at 20:42, Kuogee Hsieh <quic_khsieh at 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_khsieh at quicinc.com> wrote:
>>>>>>>>>
>>>>>>>>> On 5/11/2023 5:54 PM, Dmitry Baryshkov wrote:
>>>>>>>>>> On Fri, 12 May 2023 at 03:16, Kuogee Hsieh
>>>>>>>>>> <quic_khsieh at 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_khsieh at 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_khsieh at 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.