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

[Yongxing Mou <yongxing.mou@oss.qualcomm.com>]

On 6/16/2026 8:50 AM, Dmitry Baryshkov wrote:
> 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@quicinc.com>
>>>>
>>>> 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@quicinc.com>
>>>> Signed-off-by: Yongxing Mou <yongxing.mou@oss.qualcomm.com>
>>>> ---
>>>>    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.
> 
Yeah. will test USB-C also. Could you rebase this series on top of the 
HPD refactor series? Thanks.
>>>> +			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.
> 
There is one concern here: if we use drm_aux_hpd_bridge_notify_*() to 
notify IRQ events, does that mean every IRQ notification would also 
trigger a hotplug event?
If so, this may not be necessary, since the MST core framework already 
notifies userspace/client on its own. Additional hotplug events could 
cause userspace to query connector status again, which in turn might 
trigger the bridge notification repeatedly.
Please correct me if my understanding is wrong.
>>>> +		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.
> 
Let me double-check this.
>>    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.
> 
Got it. Let me see if there’s a better way to handle this.
>>
>>>> +			msm_dp_hpd_unplug_handle(dp);
>>>>    	}
>>>>    	pm_runtime_put_sync(&msm_dp_display->pdev->dev);
>