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

[Kuogee Hsieh <quic_khsieh@quicinc.com>]

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@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@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.
>>>
>>
>> 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
>