Re: [PATCH v2 07/10] drm/bridge: analogix_dp: Use platform-specific HPD detection scheme

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] The 2ms HPD deglitch filter (0xbb80) on Rockchip platforms completely filters out valid DP short pulses, breaking link loss detection and PSR.

Pre-existing issues:
- [High] The threaded IRQ handler ignores DP_IRQ_TYPE_IRQ_HPD (short pulses), preventing drm_helper_hpd_irq_event() from processing link loss or PSR events.
--

--- Patch [7]: [PATCH v2 07/10] drm/bridge: analogix_dp: Use platform-specific HPD detection scheme ---
commit 9400555d3cb8cd686c3464c5b0945a71464ab506
Author: Damon Ding <[email protected]>

drm/bridge: analogix_dp: Use platform-specific HPD detection scheme

Hotplug detection can be implemented either via HOTPLUG_CHG interrupt, or the
combination of PLUG and HPD_LOST interrupts. This patch configures HPD
deglitch to 2ms and uses HOTPLUG_CHG for hotplug detection on Rockchip
platforms.

> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 608d8d098329c..2399febb539e7 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -721,13 +721,18 @@ 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)
>  		analogix_dp_clear_hotplug_interrupts(dp, irq_type);
>  
> -	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);
> +

[Severity: High]
This is a pre-existing issue, but does the calculation of hpd_detected in
analogix_dp_irq_thread() miss DP_IRQ_TYPE_IRQ_HPD?

It appears that short pulse interrupts (DP_IRQ_TYPE_IRQ_HPD) are ignored in
this threaded IRQ handler and never propagated to the DRM core via
drm_helper_hpd_irq_event(). This could cause link loss events to go unhandled
until a physical cable replug.

> +	if (hpd_detected) {
>  		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..fa8e2f104d6c5 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_reg.c
> @@ -149,8 +149,13 @@ void analogix_dp_reset(struct analogix_dp_device *dp)
>  	writel(0x0, dp->reg_base + ANALOGIX_DP_PKT_SEND_CTL);
>  	writel(0x0, dp->reg_base + ANALOGIX_DP_HDCP_CTL);
>  
> -	writel(0x5e, dp->reg_base + ANALOGIX_DP_HPD_DEGLITCH_L);
> -	writel(0x1a, dp->reg_base + ANALOGIX_DP_HPD_DEGLITCH_H);
> +	if (analogix_dp_is_rockchip(dp->plat_data->dev_type)) {
> +		writel(0x80, dp->reg_base + ANALOGIX_DP_HPD_DEGLITCH_L);
> +		writel(0xbb, dp->reg_base + ANALOGIX_DP_HPD_DEGLITCH_H);

[Severity: High]
Will configuring the HPD deglitch filter to 2ms (0xbb80) completely filter
out valid DP short pulses?

The DP specification requires short pulses to be between 0.5ms and 1.0ms. It
seems a 2.0ms filter will unconditionally drop them before they can trigger
an interrupt, potentially breaking link loss recovery and PSR sink events on
Rockchip platforms.

> +	} else {
> +		writel(0x5e, dp->reg_base + ANALOGIX_DP_HPD_DEGLITCH_L);
> +		writel(0x1a, dp->reg_base + ANALOGIX_DP_HPD_DEGLITCH_H);
> +	}
>  

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7