Re: [PATCH v3] drm/msm/dp: check core_initialized flag at both host_init() and host_deinit()

[Kuogee Hsieh <quic_khsieh@quicinc.com>]

On 3/1/2023 1:15 PM, Dmitry Baryshkov wrote:
> On 01/03/2023 18:57, Kuogee Hsieh wrote:
>>
>> On 2/28/2023 6:16 PM, Dmitry Baryshkov wrote:
>>> On Wed, 1 Mar 2023 at 02:17, Kuogee Hsieh <quic_khsieh@quicinc.com> 
>>> wrote:
>>>> There is a reboot/suspend test case where system suspend is forced
>>>> during system booting up. Since dp_display_host_init() of external
>>>> DP is executed at hpd thread context, this test case may created a
>>>> scenario that dp_display_host_deinit() from pm_suspend() run before
>>>> dp_display_host_init() if hpd thread has no chance to run during
>>>> booting up while suspend request command was issued. At this scenario
>>>> system will crash at aux register access at dp_display_host_deinit()
>>>> since aux clock had not yet been enabled by dp_display_host_init().
>>>> Therefore we have to ensure aux clock enabled by checking
>>>> core_initialized flag before access aux registers at pm_suspend.
>>> Can a call to dp_display_host_init() be moved from
>>> dp_display_config_hpd() to dp_display_bind()?
>>
>> yes,  Sankeerth's  "drm/msm/dp: enable pm_runtime support for dp 
>> driver" patch is doing that which is under review.
>>
>> https://patchwork.freedesktop.org/patch/523879/?series=114297&rev=1
>
> No, he is doing another thing. He is moving these calls to pm_runtime 
> callbacks, not to the dp_display_bind().
>
>>> Related question: what is the primary reason for having
>>> EV_HPD_INIT_SETUP and calling dp_display_config_hpd() via the event
>>> thread? Does DP driver really depend on DPU irqs being installed? As
>>> far as I understand, DP device uses MDSS interrupts and those IRQs are
>>> available and working at the time of dp_display_probe() /
>>> dp_display_bind().
>>
>> HDP gpio pin has to run through DP aux module 100ms denouncing logic 
>> and have its mask bits.
>>
>> Therefore DP irq has to be enabled to receive DP isr with mask bits set.
>
> So... DP irq is enabled by the MDSS, not by the DPU. Again, why does 
> DP driver depend on DPU irqs being installed?

sorry, previously i mis understand your question -- why does DP driver 
depend on DPU irqs being installed?

now, I think you are asking why  dpu_irq_postinstall() ==> 
msm_dp_irq_postinstall() ==> event_thread ==> dp_display_config_hdp() 
==> enable_irq(dp->irq)

With the below test i had run, i think the reason is to make sure 
dp->irq be requested before enable it.

I just run the execution timing order test and collect execution order 
as descending order at below,

1) dp_display_probe() -- start

2) dp_display_bind()

3) msm_dp_modeset_init()  ==> dp_display_request_irq() ==> 
dp_display_get_next_bridge()

4) dpu_irq_postinstall() ==> msm_dp_irq_postinstall() ==> 
enable_irq(dp->irq)

5) dp_display_probe() -- end

dp->irq is request at msm_dp_modeset_init() and enabled after.

That bring up the issue to move DP's dp_display_host_init() executed at 
dp_display_bind().

Since eDP have dp_dispaly_host_init() executed at 
dp_display_get_next_bridge() which executed after dp_display_bind().

If moved DP's dp_display_host_init() to dp_dispaly_bind() which means DP 
will be ready to receive HPD irq before eDP ready.

This may create some uncertainties at execution flow and complicate 
things up.


>
>> Similar mechanism is used for mdp, dsi, etc.
>
> And none of them uses irq_postinstall callback.
>
>>
>>
>>>> Changes in v2:
>>>> -- at commit text, dp_display_host_init() instead of host_init()
>>>> -- at commit text, dp_display_host_deinit() instead of host_deinit()
>>>>
>>>> Changes in v3:
>>>> -- re arrange to avoid commit text line over 75 chars
>>>>
>>>> Fixes: 989ebe7bc446 ("drm/msm/dp: do not initialize phy until 
>>>> plugin interrupt received")
>>>> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
>>>> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
>>>> ---
>>>>   drivers/gpu/drm/msm/dp/dp_display.c | 20 ++++++++++++--------
>>>>   1 file changed, 12 insertions(+), 8 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
>>>> b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> index bde1a7c..1850738 100644
>>>> --- a/drivers/gpu/drm/msm/dp/dp_display.c
>>>> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
>>>> @@ -460,10 +460,12 @@ static void dp_display_host_init(struct 
>>>> dp_display_private *dp)
>>>>                  dp->dp_display.connector_type, dp->core_initialized,
>>>>                  dp->phy_initialized);
>>>>
>>>> -       dp_power_init(dp->power, false);
>>>> -       dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
>>>> -       dp_aux_init(dp->aux);
>>>> -       dp->core_initialized = true;
>>>> +       if (!dp->core_initialized) {
>>>> +               dp_power_init(dp->power, false);
>>>> +               dp_ctrl_reset_irq_ctrl(dp->ctrl, true);
>>>> +               dp_aux_init(dp->aux);
>>>> +               dp->core_initialized = true;
>>>> +       }
>>>>   }
>>>>
>>>>   static void dp_display_host_deinit(struct dp_display_private *dp)
>>>> @@ -472,10 +474,12 @@ static void dp_display_host_deinit(struct 
>>>> dp_display_private *dp)
>>>>                  dp->dp_display.connector_type, dp->core_initialized,
>>>>                  dp->phy_initialized);
>>>>
>>>> -       dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
>>>> -       dp_aux_deinit(dp->aux);
>>>> -       dp_power_deinit(dp->power);
>>>> -       dp->core_initialized = false;
>>>> +       if (dp->core_initialized) {
>>>> +               dp_ctrl_reset_irq_ctrl(dp->ctrl, false);
>>>> +               dp_aux_deinit(dp->aux);
>>>> +               dp_power_deinit(dp->power);
>>>> +               dp->core_initialized = false;
>>>> +       }
>>>>   }
>>>>
>>>>   static int dp_display_usbpd_configure_cb(struct device *dev)
>>>> -- 
>>>> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
>>>> Forum,
>>>> a Linux Foundation Collaborative Project
>>>>
>>>
>