Re: [PATCH v6 07/10] drm/msm/dp: rework HPD handling

[Konrad Dybcio]

On 5/24/26 12:33 PM, Dmitry Baryshkov wrote:
> From: Jessica Zhang <jessica.zhang@xxxxxxxxxxxxxxxx>
> 
> Handling of the HPD events in the MSM DP driver is plagued with lots of
> problems. It tries to work aside of the main DRM framework, handling the
> HPD signals on its own. There are two separate paths, one for the HPD
> signals coming from the DP HPD pin and another path for signals coming
> from outside (e.g. from the Type-C AltMode). It lies about the connected
> state, returning the link established state instead. It is not easy to
> understand or modify it. Having a separate event machine doesn't add
> extra clarity.
> 
> Drop the whole event machine. When the DP receives a HPD event, send it
> to the DRM core. Then handle the events in the hpd_notify callback,
> unifying paths for HPD signals.
> 
> Signed-off-by: Jessica Zhang <jessica.zhang@xxxxxxxxxxxxxxxx>
> Tested-by: Val Packett <val@xxxxxxxxxxxx> # x1e80100-dell-latitude-7455
> Tested-by: Yongxing Mou <yongxing.mou@xxxxxxxxxxxxxxxx> # Hamoa IOT EVK, QCS8300 Ride
> Co-developed-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxxxxxxxx>
> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxxxxxxxx>
> ---

This is way too complex for me to grasp at once, so I have a couple nits

[...]

> +	drm_dbg_dp(dp->drm_dev, "After, type=%d\n",
> +			dp->msm_dp_display.connector_type);

Most debug prints added in this patch have odd indent and some
have vague messages

[...]

> +	/* 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);
> +
> +	ret = IRQ_HANDLED;
> +
> +	return ret;

Return directly

[...]


> +}
> +
>  static int msm_dp_display_request_irq(struct msm_dp_display_private *dp)
>  {
>  	int rc = 0;
> @@ -1247,9 +933,13 @@ static int msm_dp_display_request_irq(struct msm_dp_display_private *dp)
>  		return dp->irq;
>  	}
>  
> -	rc = devm_request_irq(&pdev->dev, dp->irq, msm_dp_display_irq_handler,
> -			      IRQF_TRIGGER_HIGH|IRQF_NO_AUTOEN,
> -			      "dp_display_isr", dp);
> +	spin_lock_init(&dp->irq_thread_lock);
> +	irq_set_status_flags(dp->irq, IRQ_NOAUTOEN);
> +	rc = devm_request_threaded_irq(&pdev->dev, dp->irq,
> +				       msm_dp_display_irq_handler,
> +				       msm_dp_display_irq_thread,
> +				       IRQ_TYPE_LEVEL_HIGH,

I think this field expects IRQF_-prefixed flags too

Konrad