Re: [PATCH v4 37/39] drm/msm/dp: add HPD callback for dp MST

[Dmitry Baryshkov]

On Mon, Jun 15, 2026 at 06:05:07PM +0800, Yongxing Mou wrote:
> 
> 
> On 4/12/2026 6:00 AM, Dmitry Baryshkov wrote:
> > On Fri, Apr 10, 2026 at 05:34:12PM +0800, Yongxing Mou wrote:
> > > From: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
> > > 
> > > Add HPD callback for the MST module which shall be invoked from the
> > > dp_display's HPD handler to perform MST specific operations in case
> > > of HPD. In MST case, route the HPD messages to MST module.
> > > 
> > > Signed-off-by: Abhinav Kumar <quic_abhinavk@xxxxxxxxxxx>
> > > Signed-off-by: Yongxing Mou <yongxing.mou@xxxxxxxxxxxxxxxx>
> > > ---
> > >   drivers/gpu/drm/msm/dp/dp_display.c | 23 +++++++++++++++++++----
> > >   drivers/gpu/drm/msm/dp/dp_mst_drm.c | 34 ++++++++++++++++++++++++++++++++++
> > >   drivers/gpu/drm/msm/dp/dp_mst_drm.h |  1 +
> > >   3 files changed, 54 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > > index 919767945ba5..ca89e20b7563 100644
> > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > @@ -454,6 +454,9 @@ static int msm_dp_hpd_plug_handle(struct msm_dp_display_private *dp)
> > >   			dp->msm_dp_display.connector_type,
> > >   			dp->link->sink_count);
> > > +	if (dp->plugged)
> > > +		return 0;
> > > +
> > >   	mutex_lock(&dp->plugged_lock);
> > >   	ret = pm_runtime_resume_and_get(&pdev->dev);
> > > @@ -556,12 +559,19 @@ static int msm_dp_irq_hpd_handle(struct msm_dp_display_private *dp)
> > >   {
> > >   	u32 sink_request;
> > >   	int rc = 0;
> > > +	struct msm_dp *msm_dp_display = &dp->msm_dp_display;
> > >   	/* irq_hpd can happen at either connected or disconnected state */
> > >   	drm_dbg_dp(dp->drm_dev, "Before, type=%d, sink_count=%d\n",
> > >   			dp->msm_dp_display.connector_type,
> > >   			dp->link->sink_count);
> > > +	if (msm_dp_display->mst_active) {
> > > +		if (msm_dp_aux_is_link_connected(dp->aux) != ISR_DISCONNECTED)
> > 
> > Will this work for USB-C?
> > 
> Hmm not work for USB-C. We can remove this check here, as the IRQ thread can
> handle the disconnect case itself.

Please. Start testing with USB-C too.

> > > +			msm_dp_mst_display_hpd_irq(&dp->msm_dp_display);
> > > +		return 0;
> > > +	}
> > > +
> > >   	/* check for any test request issued by sink */
> > >   	rc = msm_dp_link_process_request(dp->link);
> > >   	if (!rc) {
> > > @@ -1125,9 +1135,13 @@ static irqreturn_t msm_dp_display_irq_thread(int irq, void *dev_id)
> > >   				      connector_status_connected);
> > >   	/* Send HPD as connected and distinguish it in the notifier */
> > > -	if (hpd_isr_status & DP_DP_IRQ_HPD_INT_MASK)
> > > -		drm_bridge_hpd_notify(dp->msm_dp_display.bridge,
> > > -				      connector_status_connected);
> > > +	if (hpd_isr_status & DP_DP_IRQ_HPD_INT_MASK) {
> > > +		if (dp->msm_dp_display.mst_active)
> > > +			msm_dp_irq_hpd_handle(dp);
> > 
> > No, don't touch this code. HPD notifications might be coming from the
> > other entities. This IRQ thread can only send the HPD notification.
> > There rest should be handled in the notifier.
> > 
> Ok. From my understanding, after this series
> (https://patchwork.freedesktop.org/series/164954/#rev5) is rebased, we
> should use drm_aux_hpd_bridge_notify_extra() here to notify the IRQ?

No. There is no aux bridge here. But yes, I'd need to call a different
function in that series.

> > > +		else
> > > +			drm_bridge_hpd_notify(dp->msm_dp_display.bridge,
> > > +					      connector_status_connected);
> > > +	}
> > >   	ret = IRQ_HANDLED;
> > > @@ -1793,7 +1807,8 @@ void msm_dp_bridge_hpd_notify(struct drm_bridge *bridge,
> > >   			msm_dp_hpd_plug_handle(dp);
> > >   		}
> > >   	} else {
> > > -		msm_dp_hpd_unplug_handle(dp);
> > > +		if (hpd_link_status == ISR_DISCONNECTED)
> > 
> > Why?
> > 
> Let me explain this in more detail here.
> Currently, MST hotplug and IRQ events are handled through the SST bridge.
> This guards against spurious unplug handling caused by
> msm_dp_bridge_hpd_notify() being invoked from non-HPD contexts where status
> == connector_status_disconnected does not actually mean the cable is gone.
> 
> In addition to the real HPD IRQ path, drm_bridge_connector_detect() also
> calls drm_bridge_connector_hpd_notify() to broadcast the detect result to
> all bridges in the chain. So a single physical plug-in produces multiple
> msm_dp_bridge_hpd_notify() calls — one from the real IRQ, then several more
> from various probe/poll paths. Stack traces from a single insertion on
> QCS8300:
> 
>   1. msm_dp_display_irq_thread → real HPD plug, status=connected
>   2. fbdev probe triggered by (1) → drm_bridge_connector_detect →
> status=disconnected (link not ready yet)

This should not be happening. We don't use link status anymore to return
connected status.

>   3. output_poll_execute worker → same path → status=disconnected
>   4. drm_dp_mst_link_probe_work → same path → status=disconnected
>   5. output_poll_execute again → status=disconnected
> 
> Here not work for USB-C case yet, I’d like to switch to using
> drm_dp_read_sink_count to detect whether the sink is actually disconnected
> or no sink devices.

drm_dp_read_sink_count() isn't enough here. See the plugged flag. Maybe
we need more flags here.

> 
> > > +			msm_dp_hpd_unplug_handle(dp);
> > >   	}
> > >   	pm_runtime_put_sync(&msm_dp_display->pdev->dev);

-- 
With best wishes
Dmitry