Re: [PATCH v2 00/10] Add HPD support for Rockchip Analogix DP

[Damon Ding <damon.ding@rock-chips.com>]

Hi Heiko,

On 8/6/2026 7:42 AM, Heiko Stübner wrote:
> Am Mittwoch, 5. August 2026, 06:06:48 Mitteleuropäische Sommerzeit schrieb Damon Ding:
>> Hi Heiko,
>>
>> On 8/5/2026 6:39 AM, Heiko Stübner wrote:
>>> Hi Damon,
>>>
>>> Am Dienstag, 4. August 2026, 10:17:07 Mitteleuropäische Sommerzeit schrieb Damon Ding:
>>>> Display-connector mode (DP connector without HPD GPIO):
>>>>
>>>>     &edp_out_conn {
>>>>         remote-endpoint = <&dp_con_in>;
>>>>     };
>>>>
>>>>     dp-con {
>>>>         compatible = "dp-connector";
>>>>         label = "DP OUT";
>>>>         type = "full-size";
>>>>
>>>>         port {
>>>>             dp_con_in: endpoint {
>>>>                 remote-endpoint = <&edp_out_conn>;
>>>>             };
>>>>         };
>>>>     };
>>>>
>>>> Display-connector mode (DP connector with HPD GPIO):
>>>>
>>>>     dp-con {
>>>>         compatible = "dp-connector";
>>>>         label = "DP OUT";
>>>>         type = "full-size";
>>>>         pinctrl-0 = <&edp0_hpd>;
>>>>         pinctrl-names = "default";
>>>>         hpd-gpios = <&gpio4 RK_PC1 GPIO_ACTIVE_HIGH>;
>>>>
>>>>         port {
>>>>             dp_con_in: endpoint {
>>>>                 remote-endpoint = <&edp_out_conn>;
>>>>             };
>>>>         };
>>>>     };
>>>>
>>>> All four configurations detect cable plug/unplug events correctly.
>>
>> Thanks a lot for your testing and feedback. :-)
>>
>> It seems my test setup gave me the false impression that those cases
>> worked well.
>>
>>>
>>> hmm, it wasn't working entirely for me though and was still running into
>>> issues when the display was unplugged on boot.
>>>
>>
>> Are you seeing this boot‑unplug issue for both scenarios: DP‑connector
>> with HPD‑gpio paired with eDP without HPD‑gpio, and DP‑connector without
>> HPD‑gpio paired with eDP with HPD‑gpio? I.e. it wrongly reports
>> connected even with no display plugged in and proceeds into DRM
>> .atomic_enable()?
> 
> Yep it's different. The analogix-dp gpio-hpd works correctly,  because the
> analogix driver can do gpiod_get_value() in analogix_dp_get_plug_in_status()
> 
> The dp-connector does not, as with the patch you pointed to, it lost
> that ability. So on boot a plugged in display is not detected correctly
> because there won't be a hotplug interrupt.
> 
>>
>>> I wiggled around a bit like in the diff below and am now getting correct
>>> plug and unplug events.
>>
>> Oh, I see. For the GPIO HPD case, the IRQ needs to be enabled early so
>> that plug‑in interrupts can be properly responded to. However GPIO HPD
>> does not require a runtime PM get. I will better separate these two
>> scenarios in the next version.
>>
>>>
>>> But of course, as the dp-variant of the connector does not provide
>>> a "detect" and just the "hpd" functionality, it's missing the initial state.
>>>
>>> I'm currently not sure how to find out _if_ a panel is connected on boot.
>>>
>>
>> Based on Dmitry's commit cb640b2ca546 ("drm/bridge: display‑connector:
>> don't set OP_DETECT for DisplayPorts"), HPD events from DP‑variant
>> connector should be handled by the upstream DP controller. Hence I added
>> analogix_dp_bridge_notify() to retrieve HPD status coming from downstream.
>>
>> As expected, under the bridge‑connector framework, the detect result
>> from Analogix DP should in theory reflect the actual connection status.
>> Let's dig into this boot‑time initial‑state issue together.
> 
> One idea I had was, is it possible to do a drm_dp_read_dpcd_caps()
> read during bind/... to see if something answers?
> 

Sorry for the long delay to get back to this series. I was stuck on some 
tricky local issues.

I'll test your suggestion and try to send out v3 within next week.

Best regards,
Damon

> 
>>> In other review comments:
>>> - the bridge could use devm_drm_of_get_bridge() as suggested in
>>>     the documentation of drm_of_find_panel_or_bridge(), as that would
>>>     remove the separate panel_bridge creation
>>> - instead of using plat_data->next_bridge _inside_ the driver
>>>     struct drm_bridge has a field next_bridge already.
>>
>> Great suggestions, I will incorporate them for the next version
>> alongside Sashiko's comments.
>>
>> Best regards,
>> Damon
>>
>>>
>>>
>>> Heiko
>>>
>>>
>>> ------- 8< -------
>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> index 877e1b3ca7525..1388640a27de7 100644
>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>>> @@ -43,7 +43,7 @@ static const bool verify_fast_training;
>>>    static bool analogix_dp_require_pm_for_hpd_irq(struct analogix_dp_device *dp)
>>>    {
>>>    	return analogix_dp_is_rockchip(dp->plat_data->dev_type) && !dp->hpd_gpiod &&
>>> -	       !dp->force_hpd;
>>> +	       !dp->hpd_bridge && !dp->force_hpd;
>>>    }
>>>    
>>>    static void analogix_dp_init_dp(struct analogix_dp_device *dp)
>>> @@ -72,7 +72,7 @@ static int analogix_dp_detect_hpd(struct analogix_dp_device *dp)
>>>    	 * Trust connection status from downstream bridge (e.g.,
>>>    	 * display-connector with hpd-gpios).
>>>    	 */
>>> -	if (dp->plat_data->next_bridge && dp->connection_notified)
>>> +	if (dp->hpd_bridge && dp->connection_notified)
>>>    		return 0;
>>>    
>>>    	while (timeout_loop < DP_TIMEOUT_LOOP_COUNT) {
>>> @@ -926,10 +926,10 @@ analogix_dp_bridge_detect(struct drm_bridge *bridge, struct drm_connector *conne
>>>    	 */
>>>    	if (dp->plat_data->next_bridge && dp->last_bridge_is_panel)
>>>    		status = connector_status_connected;
>>> -
>>> -	if (!analogix_dp_detect_hpd(dp))
>>> +	else if (!analogix_dp_detect_hpd(dp))
>>>    		status = connector_status_connected;
>>>    
>>> +printk("---> %s status %d\n", __func__, status);
>>>    	return status;
>>>    }
>>>    
>>> @@ -1044,7 +1044,7 @@ static int analogix_dp_set_bridge(struct analogix_dp_device *dp)
>>>    		goto out_dp_init;
>>>    	}
>>>    
>>> -	if (!analogix_dp_require_pm_for_hpd_irq(dp))
>>> +	if (!analogix_dp_require_pm_for_hpd_irq(dp) && !dp->hpd_bridge)
>>>    		enable_irq(dp->irq);
>>>    	return 0;
>>>    
>>> @@ -1187,7 +1187,7 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
>>>    	if (dp->dpms_mode != DRM_MODE_DPMS_ON)
>>>    		return;
>>>    
>>> -	if (!analogix_dp_require_pm_for_hpd_irq(dp))
>>> +	if (!analogix_dp_require_pm_for_hpd_irq(dp) && !dp->hpd_bridge)
>>>    		disable_irq(dp->irq);
>>>    
>>>    	analogix_dp_set_analog_power_down(dp, POWER_ALL, 1);
>>> @@ -1264,6 +1264,7 @@ static void analogix_dp_bridge_notify(struct drm_bridge *bridge, struct drm_conn
>>>    	struct analogix_dp_device *dp = to_dp(bridge);
>>>    
>>>    	dp->connection_notified = (status == connector_status_connected);
>>> +printk("---> %s connection_notified %d\n", __func__, dp->connection_notified);
>>>    }
>>>    
>>>    static const struct drm_bridge_funcs analogix_dp_bridge_funcs = {
>>> @@ -1641,6 +1642,13 @@ static int analogix_dp_aux_done_probing(struct drm_dp_aux *aux)
>>>    	if (ret && ret != -ENODEV)
>>>    		return ret;
>>>    
>>> +	/*
>>> +	 * There is a next link in the chain which is not a panel, we should
>>> +	 * expect hotplug-information coming from there.
>>> +	 */
>>> +	if (plat_data->next_bridge && !drm_bridge_is_panel(plat_data->next_bridge))
>>> +		dp->hpd_bridge = true;
>>> +
>>>    	return component_add(dp->dev, plat_data->ops);
>>>    }
>>>    
>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>>> index d0fb25e543ea0..ecca3b87b4456 100644
>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>>> @@ -169,6 +169,7 @@ struct analogix_dp_device {
>>>    	bool			fast_train_enable;
>>>    	bool			psr_supported;
>>>    	bool			last_bridge_is_panel;
>>> +	bool			hpd_bridge;
>>>    	bool			connection_notified;
>>>    
>>>    	u8 dpcd[DP_RECEIVER_CAP_SIZE];
>>> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>>> index ec5950066f838..6f0d642739ffd 100644
>>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
>>> @@ -182,7 +182,7 @@ void analogix_dp_config_interrupt(struct analogix_dp_device *dp)
>>>    	writel(0, dp->reg_base + ANALOGIX_DP_COMMON_INT_MASK_2);
>>>    	writel(0, dp->reg_base + ANALOGIX_DP_COMMON_INT_MASK_3);
>>>    
>>> -	if (dp->hpd_gpiod) {
>>> +	if (dp->hpd_gpiod || dp->hpd_bridge) {
>>>    		analogix_dp_mute_hpd_interrupt(dp, HPD_IRQ);
>>>    	} else {
>>>    		/*
>>> @@ -438,7 +438,7 @@ void analogix_dp_init_hpd(struct analogix_dp_device *dp)
>>>    {
>>>    	u32 reg;
>>>    
>>> -	if (dp->hpd_gpiod)
>>> +	if (dp->hpd_gpiod || dp->hpd_bridge)
>>>    		return;
>>>    
>>>    	analogix_dp_clear_hotplug_interrupts(dp, HPD_IRQ);
>>> @@ -539,6 +539,9 @@ int analogix_dp_get_plug_in_status(struct analogix_dp_device *dp)
>>>    	if (dp->hpd_gpiod) {
>>>    		if (gpiod_get_value(dp->hpd_gpiod))
>>>    			return 0;
>>> +	} else if (dp->hpd_bridge) {
>>> +		if (dp->connection_notified)
>>> +			return 0;
>>>    	} else {
>>>    		reg = readl(dp->reg_base + ANALOGIX_DP_SYS_CTL_3);
>>>    		if (reg & HPD_STATUS)
>>>
>>>
>>>
>>>
>>>
>>
>>
> 
> 
> 
> 
> 
>