On Fri, Aug 08, 2025 at 05:35:20PM -0700, Jessica Zhang wrote:
> Since the HPD IRQ handler directly notifies DRM core for hotplug events,
> there is no need to maintain a separate event waitqueue.
>
> Drop the event waitqueue and all related structs and helpers.
>
> Signed-off-by: Jessica Zhang <jessica.zh...@oss.qualcomm.com>
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 255
> +-----------------------------------
> 1 file changed, 7 insertions(+), 248 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c
> b/drivers/gpu/drm/msm/dp/dp_display.c
> index b9e2e368c4a8..eabd6e6981fb 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -52,27 +52,6 @@ enum {
> ST_DISPLAY_OFF,
> };
>
> -enum {
> - EV_NO_EVENT,
> - /* hpd events */
> - EV_HPD_PLUG_INT,
> - EV_IRQ_HPD_INT,
> - EV_HPD_UNPLUG_INT,
> -};
> -
> -#define EVENT_TIMEOUT (HZ/10) /* 100ms */
> -#define DP_EVENT_Q_MAX 8
> -
> -#define DP_TIMEOUT_NONE 0
> -
> -#define WAIT_FOR_RESUME_TIMEOUT_JIFFIES (HZ / 2)
> -
> -struct msm_dp_event {
> - u32 event_id;
> - u32 data;
> - u32 delay;
> -};
> -
> struct msm_dp_display_private {
> int irq;
>
> @@ -100,16 +79,7 @@ struct msm_dp_display_private {
> spinlock_t irq_thread_lock;
> u32 hpd_isr_status;
>
> - /* event related only access by event thread */
> - struct mutex event_mutex;
> - wait_queue_head_t event_q;
> u32 hpd_state;
> - u32 event_pndx;
> - u32 event_gndx;
> - struct task_struct *ev_tsk;
> - struct msm_dp_event event_list[DP_EVENT_Q_MAX];
> - spinlock_t event_lock;
> -
> bool wide_bus_supported;
>
> struct msm_dp_audio *audio;
> @@ -212,60 +182,6 @@ static struct msm_dp_display_private
> *dev_get_dp_display_private(struct device *
> return container_of(dp, struct msm_dp_display_private, msm_dp_display);
> }
>
> -static int msm_dp_add_event(struct msm_dp_display_private *msm_dp_priv, u32
> event,
> - u32 data, u32 delay)
> -{
> - unsigned long flag;
> - struct msm_dp_event *todo;
> - int pndx;
> -
> - spin_lock_irqsave(&msm_dp_priv->event_lock, flag);
> - pndx = msm_dp_priv->event_pndx + 1;
> - pndx %= DP_EVENT_Q_MAX;
> - if (pndx == msm_dp_priv->event_gndx) {
> - pr_err("event_q is full: pndx=%d gndx=%d\n",
> - msm_dp_priv->event_pndx, msm_dp_priv->event_gndx);
> - spin_unlock_irqrestore(&msm_dp_priv->event_lock, flag);
> - return -EPERM;
> - }
> - todo = &msm_dp_priv->event_list[msm_dp_priv->event_pndx++];
> - msm_dp_priv->event_pndx %= DP_EVENT_Q_MAX;
> - todo->event_id = event;
> - todo->data = data;
> - todo->delay = delay;
> - wake_up(&msm_dp_priv->event_q);
> - spin_unlock_irqrestore(&msm_dp_priv->event_lock, flag);
> -
> - return 0;
> -}
> -
> -static int msm_dp_del_event(struct msm_dp_display_private *msm_dp_priv, u32
> event)
> -{
> - unsigned long flag;
> - struct msm_dp_event *todo;
> - u32 gndx;
> -
> - spin_lock_irqsave(&msm_dp_priv->event_lock, flag);
> - if (msm_dp_priv->event_pndx == msm_dp_priv->event_gndx) {
> - spin_unlock_irqrestore(&msm_dp_priv->event_lock, flag);
> - return -ENOENT;
> - }
> -
> - gndx = msm_dp_priv->event_gndx;
> - while (msm_dp_priv->event_pndx != gndx) {
> - todo = &msm_dp_priv->event_list[gndx];
> - if (todo->event_id == event) {
> - todo->event_id = EV_NO_EVENT; /* deleted */
> - todo->delay = 0;
> - }
> - gndx++;
> - gndx %= DP_EVENT_Q_MAX;
> - }
> - spin_unlock_irqrestore(&msm_dp_priv->event_lock, flag);
> -
> - return 0;
> -}
> -
> void msm_dp_display_signal_audio_start(struct msm_dp *msm_dp_display)
> {
> struct msm_dp_display_private *dp;
> @@ -284,8 +200,6 @@ void msm_dp_display_signal_audio_complete(struct msm_dp
> *msm_dp_display)
> complete_all(&dp->audio_comp);
> }
>
> -static int msm_dp_hpd_event_thread_start(struct msm_dp_display_private
> *msm_dp_priv);
> -
> static int msm_dp_display_bind(struct device *dev, struct device *master,
> void *data)
> {
> @@ -305,12 +219,6 @@ static int msm_dp_display_bind(struct device *dev,
> struct device *master,
> goto end;
> }
>
> - rc = msm_dp_hpd_event_thread_start(dp);
> - if (rc) {
> - DRM_ERROR("Event thread create failed\n");
> - goto end;
> - }
> -
> return 0;
> end:
> return rc;
> @@ -322,8 +230,6 @@ static void msm_dp_display_unbind(struct device *dev,
> struct device *master,
> struct msm_dp_display_private *dp = dev_get_dp_display_private(dev);
> struct msm_drm_private *priv = dev_get_drvdata(master);
>
> - kthread_stop(dp->ev_tsk);
> -
> of_dp_aux_depopulate_bus(dp->aux);
>
> msm_dp_aux_unregister(dp->aux);
> @@ -585,33 +491,22 @@ static int msm_dp_hpd_plug_handle(struct
> msm_dp_display_private *dp, u32 data)
>
> msm_dp_aux_enable_xfers(dp->aux, true);
>
> - mutex_lock(&dp->event_mutex);
I think the event_mutex should be kept as is. Otherwise we don't have
protection against delivering the next event while we still process the
previous one.
> -
> state = dp->hpd_state;
> drm_dbg_dp(dp->drm_dev, "Before, type=%d hpd_state=%d\n",
> dp->msm_dp_display.connector_type, state);
>
--
With best wishes
Dmitry
