Re: [PATCH v1 6/7] drm/bridge: analogix_dp: Use platform-specific HPD detection scheme

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] DisplayPort short HPD pulses are completely filtered out by the 2ms hardware deglitch filter, breaking link loss detection and PSR.
- [High] Hotplug interrupt unmasking logic unintentionally re-enables PLUG and HPD_LOST interrupts on Rockchip platforms, defeating the patch's purpose.
--

--- Patch [6]: [PATCH v1 6/7] drm/bridge: analogix_dp: Use platform-specific HPD detection scheme ---
commit af35433f355c1427bbfdea612a689613337f11ab
Author: Damon Ding <damon.ding@rock-chips.com>
Date:   Thu Jul 30 03:27:43 2026 +0000

    drm/bridge: analogix_dp: Use platform-specific HPD detection scheme
    
This commit configures the HPD deglitch filter to 2ms on Rockchip platforms 
and updates the interrupt handling to rely exclusively on the HOTPLUG_CHG 
interrupt instead of the PLUG and HPD_LOST combination. The threaded IRQ 
handler is also adjusted to evaluate platform-specific interrupt flags.

> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 9f6ecaa1aeb5e..b7aa10daae2f2 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -723,10 +723,15 @@ static irqreturn_t analogix_dp_irq_thread(int irq, void *arg)
>  {
>  	struct analogix_dp_device *dp = arg;
>  	u32 irq_type;
> +	bool hpd_detected;
>  
>  	irq_type = analogix_dp_get_irq_type(dp);
> -	if (irq_type & DP_IRQ_TYPE_HP_CABLE_IN ||
> -	    irq_type & DP_IRQ_TYPE_HP_CABLE_OUT) {
> +	if (!dp->hpd_gpiod && analogix_dp_is_rockchip(dp->plat_data->dev_type))
> +		hpd_detected = irq_type & DP_IRQ_TYPE_HP_CHANGE;
> +	else
> +		hpd_detected = (irq_type & DP_IRQ_TYPE_HP_CABLE_IN) ||
> +			       (irq_type & DP_IRQ_TYPE_HP_CABLE_OUT);
> +	if (hpd_detected) {

[Severity: High]
Could this logic inadvertently re-enable the PLUG and HPD_LOST interrupts
that were intentionally masked during initialization?

When analogix_dp_get_irq_type() is called, it reads the raw interrupt
status register without checking against the active mask:

analogix_dp_get_irq_type() {
    ...
    reg = readl(dp->reg_base + ANALOGIX_DP_COMMON_INT_STA_4);
    if (reg & PLUG)
        irq_type |= DP_IRQ_TYPE_HP_CABLE_IN;
    if (reg & HPD_LOST)
        irq_type |= DP_IRQ_TYPE_HP_CABLE_OUT;
    ...
}

If the hardware continues to set the PLUG and HPD_LOST status bits on cable
events even while they are masked, analogix_dp_get_irq_type() will still
return them in irq_type.

Later in analogix_dp_irq_thread(), this irq_type mask is passed to
analogix_dp_unmute_hpd_interrupt():

    if (!dp->hpd_gpiod && irq_type) {
        analogix_dp_clear_hotplug_interrupts(dp, irq_type);
        analogix_dp_unmute_hpd_interrupt(dp, irq_type);
    }

Would passing the unintentionally detected PLUG and HPD_LOST bits into
analogix_dp_unmute_hpd_interrupt() permanently unmask those interrupts,
defeating the platform-specific interrupt scheme on the very first event?

Additionally, because DP_IRQ_TYPE_IRQ_HPD is no longer included in the
hpd_detected calculation for Rockchip, will this prevent
drm_helper_hpd_irq_event() from being called to process link retraining
and Panel Self Refresh (PSR) events?

>  		dev_dbg(dp->dev, "Detected cable status changed!\n");
>  		if (dp->drm_dev)
>  			drm_helper_hpd_irq_event(dp->drm_dev);
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> index 42463e18f392c..981ce3810e90d 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
[ ... ]
> @@ -177,7 +177,21 @@ 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);
>  
> -	analogix_dp_unmute_hpd_interrupt(dp, HPD_IRQ);
> +	if (analogix_dp_is_rockchip(dp->plat_data->dev_type)) {
> +		/*
> +		 * Either HOTPLUG_CHG interrupt or PLUG + HPD_LOST interrupt
> +		 * pair can be used to implement hotplug detection.
> +		 *
> +		 * On Rockchip platforms, configuring HPD deglitch to 2ms and
> +		 * using HOTPLUG_CHG interrupt for hotplug detection is proven
> +		 * as a better solution via engineering verification.
> +		 */
> +		writel(0x80, dp->reg_base + ANALOGIX_DP_HPD_DEGLITCH_L);
> +		writel(0xbb, dp->reg_base + ANALOGIX_DP_HPD_DEGLITCH_H);

[Severity: High]
Does configuring the deglitch filter to 2ms risk filtering out valid 
DisplayPort short HPD pulses?

The DP specification defines short HPD pulses (which are used for sink
events like PSR and link retraining) as a low pulse lasting between 0.5ms
and 1.0ms.

Because the new 2ms filter configuration exceeds the maximum short pulse
duration of 1.0ms, will the hardware treat all valid DP short pulses as
noise and silently drop them before they can trigger an interrupt?

> +		analogix_dp_unmute_hpd_interrupt(dp, DP_IRQ_TYPE_HP_CHANGE);
> +	} else {
> +		analogix_dp_unmute_hpd_interrupt(dp, HPD_IRQ);
> +	}
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730032744.381566-1-damon.ding@rock-chips.com?part=6