Re: [PATCH 06/13] drm/msm/dp: Report stream enable failures through link status

[Dmitry Baryshkov]

On Wed, Sep 30, 2026 at 08:41:50PM +0800, Xilin Wu wrote:
> Atomic bridge enable callbacks cannot return an error to userspace.
> After a failed enable, leaving link-status unchanged gives userspace
> no indication that it needs to retry the configuration.
> 
> Mark the connector link status bad from a work item after unwinding
> the failed enable. Send one connector hotplug notification per failure
> episode: fbdev can synchronously retry the modeset from the
> notification, so notifying on every failure would create an unbounded
> retry loop. Subsequent failures still restore BAD after a retry has
> set the property to GOOD.
> 
> Allow notifications again after a successful enable or an external
> sink connection change. Do not reset the notification latch during
> eDP's internal plug and unplug handling, which runs on every retry.
> Ignore queued work superseded by recovery or an external unplug.
> Serialize the failure state with plugged_lock and update link-status
> under the connection mutex before notifying clients with both locks
> released.
> 
> Initialize the work at probe and cancel it before unbinding the
> display.
> 
> Assisted-by: LLM
> Signed-off-by: Xilin Wu <sophon@xxxxxxxxx>
> ---
>  drivers/gpu/drm/msm/dp/dp_display.c | 63 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 61 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index ae967ca652c9..1bfa6696d904 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -12,9 +12,12 @@
>  #include <linux/phy/phy.h>
>  #include <linux/delay.h>
>  #include <linux/string_choices.h>
> +#include <linux/workqueue.h>
>  #include <drm/display/drm_dp_aux_bus.h>
>  #include <drm/display/drm_hdmi_audio_helper.h>
>  #include <drm/drm_edid.h>
> +#include <drm/drm_modeset_lock.h>
> +#include <drm/drm_probe_helper.h>
>  
>  #include "msm_drv.h"
>  #include "msm_kms.h"
> @@ -54,9 +57,13 @@ struct msm_dp_display_private {
>  	bool audio_supported;
>  	bool stream_pm_active;
>  	bool stream_link_attempted;
> +	struct work_struct link_status_work;
>  
>  	struct mutex plugged_lock;
>  	bool plugged;
> +	/* Protected by plugged_lock, including accesses from link_status_work. */
> +	bool link_failed;
> +	bool link_status_notified;

Do we need it? I think, it's easier to send several notifications.

>  
>  	struct drm_device *drm_dev;
>  
> @@ -204,6 +211,39 @@ void msm_dp_display_signal_audio_complete(struct msm_dp *msm_dp_display)
>  	complete_all(&dp->audio_comp);
>  }
>  
> +static void msm_dp_display_reset_link_status(struct msm_dp_display_private *dp)
> +{
> +	lockdep_assert_held(&dp->plugged_lock);
> +
> +	dp->link_failed = false;
> +	dp->link_status_notified = false;
> +}
> +
> +static void msm_dp_display_link_status_work(struct work_struct *work)
> +{
> +	struct msm_dp_display_private *dp = container_of(work,
> +			struct msm_dp_display_private, link_status_work);
> +	struct drm_connector *connector = dp->msm_dp_display.connector;
> +	struct drm_device *dev = connector->dev;
> +	bool notify = false;
> +
> +	/* Match atomic check's connection_mutex -> plugged_lock ordering. */
> +	drm_modeset_lock(&dev->mode_config.connection_mutex, NULL);
> +	scoped_guard(mutex, &dp->plugged_lock) {

Please use drm_connector_set_link_status_property() here. See
intel_connector_modeset_retry_work_fn(). I think, there is a TODO in the
driver code, you can drop it too with this patch.

> +		/* A successful enable or unplug may have superseded this work. */
> +		if (dp->link_failed) {
> +			connector->state->link_status = DRM_MODE_LINK_STATUS_BAD;
> +			notify = !dp->link_status_notified;
> +			dp->link_status_notified = true;
> +		}
> +	}
> +	drm_modeset_unlock(&dev->mode_config.connection_mutex);
> +
> +	/* fbdev can retry the modeset synchronously from this notification. */
> +	if (notify)
> +		drm_kms_helper_connector_hotplug_event(connector);
> +}
> +
>  static int msm_dp_display_bind(struct device *dev, struct device *master,
>  			   void *data)
>  {

-- 
With best wishes
Dmitry