Re: [PATCH v15 1/4] drm/msm/dp: do not initialize phy until plugin interrupt received

From: Stephen Boyd <swboyd@chromium.org>
To: Kuogee Hsieh <quic_khsieh@quicinc.com>,
	agross@kernel.org, airlied@linux.ie, bjorn.andersson@linaro.org,
	daniel@ffwll.ch, dmitry.baryshkov@linaro.org,
	dri-devel@lists.freedesktop.org, robdclark@gmail.com,
	sean@poorly.run, vkoul@kernel.org
Cc: quic_abhinavk@quicinc.com, aravindh@codeaurora.org,
	quic_sbillaka@quicinc.com, freedreno@lists.freedesktop.org,
	linux-arm-msm@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v15 1/4] drm/msm/dp: do not initialize phy until plugin interrupt received
Date: Fri, 14 Jan 2022 17:23:37 -0600	[thread overview]
Message-ID: <CAE-0n53GTt5W8DjVCGJ+D8E6N3HgZ3jhPoZVCw1jLOx4uxJdKA@mail.gmail.com> (raw)
In-Reply-To: <f5a282ed-0b4a-9fb2-fc04-17842745feaa@quicinc.com>

Quoting Kuogee Hsieh (2022-01-14 14:28:52)
>
> On 1/14/2022 1:41 PM, Stephen Boyd wrote:
> > Quoting Kuogee Hsieh (2022-01-14 13:11:47)
> >> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> >> index 7cc4d21..7cd6222 100644
> >> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> >> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> >> @@ -696,12 +699,9 @@ static int dp_irq_hpd_handle(struct dp_display_private *dp, u32 data)
> >>           * dp core (ahb/aux clks) must be initialized before
> >>           * irq_hpd be handled
> >>           */
> >> -       if (dp->core_initialized) {
> >> -               ret = dp_display_usbpd_attention_cb(&dp->pdev->dev);
> >> -               if (ret == -ECONNRESET) { /* cable unplugged */
> >> -                       dp->core_initialized = false;
> >> -               }
> >> -       }
> >> +       if (dp->core_initialized)
> > When is this condition false? The irq isn't unmasked until the core has
> > been initialized. On the resume path I suppose the irq is enabled in
> > dp_display_host_init() calling dp_ctrl_reset_irq_ctrl(), and then we
> > could immediately get the interrupt but it will block on the event_mutex
> > lock.
>
> This is left over form Lazor.
>
> I remember that there is an extreme case that several irq_hpd interrupts
> happen right after dongle plug in  (happen at resume too) and sometime
> cause system crash at dpcd read due to AHB clock is not enabled yet. It
> took some time to debug it.
>
>  From looking into code, it does not look likely it will happen. But it
> did happen at real world.

How does it happen after this patch is applied? I remember the duplicate
irq_hpd problem but that should have been solved by de-duplicating the
irq in the hardware by leaving it pending until it was handled. Note,
I'm not suggesting we remove the enabling of the core, just the check
that the core is initialized. Now that the check for core_initialized is
removed from the init function we have to make sure we only call the
function one time to match what was there before.

>
> So that I would like to keep this condition checking.

I'd rather not leave around dead code in this driver. The sharp edges
are what we need to smooth out.