drm/msm: destroy msm threads after config cleanup
Subsystems (4)
PATCH v2 1/5 drm/msm: destroy msm threads after config cleanup
To avoid any possible work queues to msm threads, clean up the threads after the CRTC objects are released in config cleanup.
changes in v2:
- fix race condition before kthread flush and stop (Sean Paul)
- use kthread_destroy_worker for cleaning up kthread (Sean Paul)
--- drivers/gpu/drm/msm/msm_drv.c | 36 +++++++++++++++++------------------- 1 file changed, 17 insertions(+), 19 deletions(-)
drivers/gpu/drm/msm/msm_drv.c +17 -19
@@ -278,6 +278,21 @@ static int msm_drm_uninit(struct device *dev)* work before drm_irq_uninstall() to avoid work re-enabling an * irq after uninstall has disabled it. */ +msm_gem_shrinker_cleanup (ddev); + +drm_kms_helper_poll_fini (ddev); + +drm_dev_unregister (ddev); + +msm_perf_debugfs_cleanup (priv); +msm_rd_debugfs_cleanup (priv); + +#ifdef CONFIG_DRM_FBDEV_EMULATION +if (fbdev && priv->fbdev) +msm_fbdev_free (ddev); +#endif +drm_mode_config_cleanup (ddev); +kthread_flush_work (&vbl_ctrl->work);list_for_each_entry_safe (vbl_ev, tmp, &vbl_ctrl->event_list, node) {list_del (&vbl_ev->node);
@@ -287,33 +302,16 @@ static int msm_drm_uninit(struct device *dev)/* clean up display commit/event worker threads */ for (i =0 ; i < priv->num_crtcs; i++) {if (priv->disp_thread[i].thread) { -kthread_flush_worker (&priv->disp_thread[i].worker); -kthread_stop (priv->disp_thread[i].thread); +kthread_destroy_worker (&priv->disp_thread[i].worker); priv->disp_thread[i].thread =NULL ; }if (priv->event_thread[i].thread) { -kthread_flush_worker (&priv->event_thread[i].worker); -kthread_stop (priv->event_thread[i].thread); +kthread_destroy_worker (&priv->event_thread[i].worker); priv->event_thread[i].thread =NULL ; } } -msm_gem_shrinker_cleanup (ddev); - -drm_kms_helper_poll_fini (ddev); - -drm_dev_unregister (ddev); - -msm_perf_debugfs_cleanup (priv); -msm_rd_debugfs_cleanup (priv); - -#ifdef CONFIG_DRM_FBDEV_EMULATION -if (fbdev && priv->fbdev) -msm_fbdev_free (ddev); -#endif -drm_mode_config_cleanup (ddev); -pm_runtime_get_sync (dev);drm_irq_uninstall (ddev);pm_runtime_put_sync (dev);
Signature · 7 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project _______________________________________________ Freedreno mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/freedreno
PATCH v2 2/5 drm/msm/dpu: use system wq for vblank events
In reply to a message not in the archiveDPU was using one thread per display to dispatch async commits and vblank requests. Since clean up already happened in msm to use the common thread for all the display commits, display threads are only used to cater vblank requests. Since a single thread is sufficient to do the job without any performance hits, use system workqueue to queue requests. A separate patch is submitted later in this series to remove the display threads altogether.
changes in v2:
- switch to system wq before removing disp threads (Sean Paul)
--- drivers/gpu/drm/msm/msm_drv.c | 9 ++++----- drivers/gpu/drm/msm/msm_drv.h | 2 +- 2 files changed, 5 insertions(+), 6 deletions(-)
drivers/gpu/drm/msm/msm_drv.c +4 -5
@@ -209,7 +209,7 @@ struct vblank_event {bool enable; }; -static void vblank_ctrl_worker (struct kthread_work *work) +static void vblank_ctrl_worker (struct work_struct *work) {struct msm_vblank_ctrl *vbl_ctrl =container_of (work, struct msm_vblank_ctrl, work);
@@ -257,8 +257,7 @@ static int vblank_ctrl_queue_work(struct msm_drm_private *priv,list_add_tail (&vbl_ev->node, &vbl_ctrl->event_list);spin_unlock_irqrestore (&vbl_ctrl->lock, flags); -kthread_queue_work (&priv->disp_thread[crtc_id].worker, - &vbl_ctrl->work); +schedule_work (&vbl_ctrl->work);return 0 ; }
@@ -293,7 +292,7 @@ static int msm_drm_uninit(struct device *dev)#endif drm_mode_config_cleanup (ddev); -kthread_flush_work (&vbl_ctrl->work); +flush_work (&vbl_ctrl->work);list_for_each_entry_safe (vbl_ev, tmp, &vbl_ctrl->event_list, node) {list_del (&vbl_ev->node);kfree (vbl_ev);
@@ -476,7 +475,7 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv)INIT_LIST_HEAD (&priv->inactive_list);INIT_LIST_HEAD (&priv->vblank_ctrl.event_list); -kthread_init_work (&priv->vblank_ctrl.work, vblank_ctrl_worker); +INIT_WORK (&priv->vblank_ctrl.work, vblank_ctrl_worker);spin_lock_init (&priv->vblank_ctrl.lock);drm_mode_config_init (ddev);
drivers/gpu/drm/msm/msm_drv.h +1 -1
@@ -78,7 +78,7 @@ enum msm_mdp_plane_property { };struct msm_vblank_ctrl { -struct kthread_work work; +struct work_struct work;struct list_head event_list;spinlock_t lock; };
Signature · 7 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project _______________________________________________ Freedreno mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/freedreno
PATCH v2 3/5 drm/msm/dpu: use system wq for idle power collapse
In reply to Jeykumar Sankaranmsm is using system wq for dispatching commit and vblank events. Switch idle power collapse feature also to use system wq to handle delayed work handlers so that msm can get rid of redundant display threads.
changes in v2:
- patch introduced in v2
--- drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 26 +++++++------------------- 1 file changed, 7 insertions(+), 19 deletions(-)
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +7 -19
@@ -201,7 +201,7 @@ struct dpu_encoder_virt {bool idle_pc_supported;struct mutex rc_lock;enum dpu_enc_rc_states rc_state; -struct kthread_delayed_work delayed_off_work; +struct delayed_work delayed_off_work;struct kthread_work vsync_event_work;struct msm_display_topology topology;bool mode_set_complete;
@@ -740,7 +740,6 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc, {struct dpu_encoder_virt *dpu_enc;struct msm_drm_private *priv; -struct msm_drm_thread *disp_thread;bool is_vid_mode = false; if (!drm_enc || !drm_enc->dev || !drm_enc->dev->dev_private ||
@@ -753,12 +752,6 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc, is_vid_mode = dpu_enc->disp_info.capabilities &MSM_DISPLAY_CAP_VID_MODE ; -if (drm_enc->crtc->index >=ARRAY_SIZE (priv->disp_thread)) { -DPU_ERROR ("invalid crtc index\n" ); -return -EINVAL ; - } - disp_thread = &priv->disp_thread[drm_enc->crtc->index]; -/* * when idle_pc is not supported, process only KICKOFF, STOP and MODESET * events and return early for other events (ie wb display).
@@ -775,8 +768,7 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,switch (sw_event) {case DPU_ENC_RC_EVENT_KICKOFF :/* cancel delayed off work, if any */ -if (kthread_cancel_delayed_work_sync ( - &dpu_enc->delayed_off_work)) +if (cancel_delayed_work_sync (&dpu_enc->delayed_off_work))DPU_DEBUG_ENC (dpu_enc,"sw_event:%d, work cancelled\n" , sw_event);
@@ -835,10 +827,8 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,return 0 ; } -kthread_queue_delayed_work ( - &disp_thread->worker, - &dpu_enc->delayed_off_work, -msecs_to_jiffies (dpu_enc->idle_timeout)); +schedule_delayed_work (&dpu_enc->delayed_off_work, +msecs_to_jiffies (dpu_enc->idle_timeout)); trace_dpu_enc_rc(DRMID(drm_enc), sw_event, dpu_enc->idle_pc_supported, dpu_enc->rc_state,
@@ -847,8 +837,7 @@ static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,case DPU_ENC_RC_EVENT_PRE_STOP :/* cancel delayed off work, if any */ -if (kthread_cancel_delayed_work_sync ( - &dpu_enc->delayed_off_work)) +if (cancel_delayed_work_sync (&dpu_enc->delayed_off_work))DPU_DEBUG_ENC (dpu_enc,"sw_event:%d, work cancelled\n" , sw_event);
@@ -1351,7 +1340,7 @@ static void dpu_encoder_frame_done_callback( } } -static void dpu_encoder_off_work (struct kthread_work *work) +static void dpu_encoder_off_work (struct work_struct *work) {struct dpu_encoder_virt *dpu_enc =container_of (work, struct dpu_encoder_virt, delayed_off_work.work);
@@ -2191,8 +2180,7 @@ int dpu_encoder_setup(struct drm_device *dev, struct drm_encoder *enc,mutex_init (&dpu_enc->rc_lock); -kthread_init_delayed_work (&dpu_enc->delayed_off_work, - dpu_encoder_off_work); +INIT_DELAYED_WORK (&dpu_enc->delayed_off_work, dpu_encoder_off_work); dpu_enc->idle_timeout =IDLE_TIMEOUT ; kthread_init_work(&dpu_enc->vsync_event_work,
Signature · 7 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
PATCH v2 4/5 drm/msm: clean up display thread
In reply to a message not in the archiveSince there are no clients using these threads,
cleaning it up.
changes in v2:
- switch all the dependent clients to use system wq before removing the disp_threads (Sean Paul)
--- drivers/gpu/drm/msm/msm_drv.c | 35 +---------------------------------- drivers/gpu/drm/msm/msm_drv.h | 1 - 2 files changed, 1 insertion(+), 35 deletions(-)
drivers/gpu/drm/msm/msm_drv.c +1 -34
@@ -298,13 +298,8 @@ static int msm_drm_uninit(struct device *dev)kfree (vbl_ev); } -/* clean up display commit/event worker threads */ +/* clean up event worker threads */ for (i =0 ; i < priv->num_crtcs; i++) { -if (priv->disp_thread[i].thread) { -kthread_destroy_worker (&priv->disp_thread[i].worker); - priv->disp_thread[i].thread =NULL ; - } -if (priv->event_thread[i].thread) {kthread_destroy_worker (&priv->event_thread[i].worker); priv->event_thread[i].thread =NULL ;
@@ -541,27 +536,6 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv)*/ param.sched_priority =16 ;for (i =0 ; i < priv->num_crtcs; i++) { - -/* initialize display thread */ - priv->disp_thread[i].crtc_id = priv->crtcs[i]->base.id; -kthread_init_worker (&priv->disp_thread[i].worker); - priv->disp_thread[i].dev = ddev; - priv->disp_thread[i].thread = -kthread_run (kthread_worker_fn, - &priv->disp_thread[i].worker, -"crtc_commit:%d" , priv->disp_thread[i].crtc_id); -if (IS_ERR (priv->disp_thread[i].thread)) { -DRM_DEV_ERROR (dev,"failed to create crtc_commit kthread\n" ); - priv->disp_thread[i].thread =NULL ; - goto err_msm_uninit; - } - - ret =sched_setscheduler (priv->disp_thread[i].thread, -SCHED_FIFO , ¶m); -if (ret) -dev_warn (dev,"disp_thread set priority failed: %d\n" , - ret); -/* initialize event thread */ priv->event_thread[i].crtc_id = priv->crtcs[i]->base.id;kthread_init_worker (&priv->event_thread[i].worker);
@@ -576,13 +550,6 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) goto err_msm_uninit; } -/** -* event thread should also run at same priority as disp_thread -* because it is handling frame_done events. A lower priority -* event thread and higher priority disp_thread can causes -* frame_pending counters beyond 2. This can lead to commit -* failure at crtc commit level. -*/ ret =sched_setscheduler (priv->event_thread[i].thread,SCHED_FIFO , ¶m);if (ret)
drivers/gpu/drm/msm/msm_drv.h +0 -1
@@ -197,7 +197,6 @@ struct msm_drm_private {unsigned int num_crtcs;struct drm_crtc *crtcs[MAX_CRTCS ]; -struct msm_drm_thread disp_thread[MAX_CRTCS ];struct msm_drm_thread event_thread[MAX_CRTCS ];unsigned int num_encoders;
Signature · 7 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project _______________________________________________ Freedreno mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/freedreno
PATCH v2 5/5 drm/msm: subclass work object for vblank events
In reply to a message not in the archivemsm maintains a separate structure to define vblank work definitions and a list to track events submitted to the workqueue. We can avoid this redundant list and its protection mechanism, if we subclass the work object to encapsulate vblank event parameters.
changes in v2:
- subclass optimization on system wq (Sean Paul)
--- drivers/gpu/drm/msm/msm_drv.c | 67 +++++++++++++------------------------------ drivers/gpu/drm/msm/msm_drv.h | 7 ----- 2 files changed, 20 insertions(+), 54 deletions(-)
drivers/gpu/drm/msm/msm_drv.c +20 -47
@@ -203,61 +203,44 @@ u32 msm_readl(const void __iomem *addr)return val; } -struct vblank_event { -struct list_head node; +struct msm_vblank_work { +struct work_struct work;int crtc_id;bool enable; +struct msm_drm_private *priv; };static void vblank_ctrl_worker (struct work_struct *work) { -struct msm_vblank_ctrl *vbl_ctrl =container_of (work, - struct msm_vblank_ctrl, work); -struct msm_drm_private *priv =container_of (vbl_ctrl, - struct msm_drm_private, vblank_ctrl); +struct msm_vblank_work *vbl_work =container_of (work, + struct msm_vblank_work, work); +struct msm_drm_private *priv = vbl_work->priv;struct msm_kms *kms = priv->kms; -struct vblank_event *vbl_ev, *tmp; -unsigned long flags; - -spin_lock_irqsave (&vbl_ctrl->lock, flags); -list_for_each_entry_safe (vbl_ev, tmp, &vbl_ctrl->event_list, node) { -list_del (&vbl_ev->node); -spin_unlock_irqrestore (&vbl_ctrl->lock, flags); - -if (vbl_ev->enable) - kms->funcs->enable_vblank (kms, - priv->crtcs[vbl_ev->crtc_id]); -else - kms->funcs->disable_vblank (kms, - priv->crtcs[vbl_ev->crtc_id]); -kfree (vbl_ev); - -spin_lock_irqsave (&vbl_ctrl->lock, flags); - } +if (vbl_work->enable) + kms->funcs->enable_vblank (kms, priv->crtcs[vbl_work->crtc_id]); +else + kms->funcs->disable_vblank (kms, priv->crtcs[vbl_work->crtc_id]); -spin_unlock_irqrestore (&vbl_ctrl->lock, flags); +kfree (vbl_work); }static int vblank_ctrl_queue_work (struct msm_drm_private *priv,int crtc_id,bool enable) { -struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev; -unsigned long flags; +struct msm_vblank_work *vbl_work; - vbl_ev =kzalloc (sizeof (*vbl_ev),GFP_ATOMIC ); -if (!vbl_ev) + vbl_work =kzalloc (sizeof (*vbl_work),GFP_ATOMIC ); +if (!vbl_work)return -ENOMEM ; - vbl_ev->crtc_id = crtc_id; - vbl_ev->enable = enable; +INIT_WORK (&vbl_work->work, vblank_ctrl_worker); -spin_lock_irqsave (&vbl_ctrl->lock, flags); -list_add_tail (&vbl_ev->node, &vbl_ctrl->event_list); -spin_unlock_irqrestore (&vbl_ctrl->lock, flags); + vbl_work->crtc_id = crtc_id; + vbl_work->enable = enable; + vbl_work->priv = priv; -schedule_work (&vbl_ctrl->work); +schedule_work (&vbl_work->work);return 0 ; }
@@ -269,14 +252,13 @@ static int msm_drm_uninit(struct device *dev)struct msm_drm_private *priv = ddev->dev_private;struct msm_kms *kms = priv->kms;struct msm_mdss *mdss = priv->mdss; -struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev, *tmp;int i;/* We must cancel and cleanup any pending vblank enable/disable * work before drm_irq_uninstall() to avoid work re-enabling an * irq after uninstall has disabled it. */ +msm_gem_shrinker_cleanup (ddev);drm_kms_helper_poll_fini (ddev);
@@ -292,12 +274,6 @@ static int msm_drm_uninit(struct device *dev)#endif drm_mode_config_cleanup (ddev); -flush_work (&vbl_ctrl->work); -list_for_each_entry_safe (vbl_ev, tmp, &vbl_ctrl->event_list, node) { -list_del (&vbl_ev->node); -kfree (vbl_ev); - } -/* clean up event worker threads */ for (i =0 ; i < priv->num_crtcs; i++) {if (priv->event_thread[i].thread) {
@@ -469,9 +445,6 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) priv->wq =alloc_ordered_workqueue ("msm" ,0 );INIT_LIST_HEAD (&priv->inactive_list); -INIT_LIST_HEAD (&priv->vblank_ctrl.event_list); -INIT_WORK (&priv->vblank_ctrl.work, vblank_ctrl_worker); -spin_lock_init (&priv->vblank_ctrl.lock);drm_mode_config_init (ddev);
drivers/gpu/drm/msm/msm_drv.h +0 -7
@@ -77,12 +77,6 @@ enum msm_mdp_plane_property { PLANE_PROP_MAX_NUM }; -struct msm_vblank_ctrl { -struct work_struct work; -struct list_head event_list; -spinlock_t lock; -}; -#define MSM_GPU_MAX_RINGS 4#define MAX_H_TILES_PER_DISPLAY 2
@@ -225,7 +219,6 @@ struct msm_drm_private {struct notifier_block vmap_notifier;struct shrinker shrinker; -struct msm_vblank_ctrl vblank_ctrl;struct drm_atomic_state *pm_state; };
Signature · 7 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project _______________________________________________ Freedreno mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [PATCH v2 5/5] drm/msm: subclass work object for vblank events
In reply to a message not in the archiveQuoted 103 lines · Jeykumar Sankaran
msm maintains a separate structure to define vblank work definitions and a list to track events submitted to the workqueue. We can avoid this redundant list and its protection mechanism, if we subclass the work object to encapsulate vblank event parameters.
changes in v2:
- subclass optimization on system wq (Sean Paul)Signed-off-by Jeykumar Sankaran <[email protected]>--- drivers/gpu/drm/msm/msm_drv.c | 67 +++++++++++++------------------------------ drivers/gpu/drm/msm/msm_drv.h | 7 ----- 2 files changed, 20 insertions(+), 54 deletions(-)drivers/gpu/drm/msm/msm_drv.c +19
-36@@ -203,61 +203,44 @@ u32 msm_readl(const void __iomem *addr)return val; } -struct vblank_event { -struct list_head node; +struct msm_vblank_work { +struct work_struct work;int crtc_id;bool enable; +struct msm_drm_private *priv; };static void vblank_ctrl_worker (struct work_struct *work) { -struct msm_vblank_ctrl *vbl_ctrl =container_of (work, - struct msm_vblank_ctrl, work); -struct msm_drm_private *priv =container_of (vbl_ctrl, - struct msm_drm_private, vblank_ctrl); +struct msm_vblank_work *vbl_work =container_of (work, + struct msm_vblank_work, work); +struct msm_drm_private *priv = vbl_work->priv;struct msm_kms *kms = priv->kms; -struct vblank_event *vbl_ev, *tmp; -unsigned long flags; - -spin_lock_irqsave (&vbl_ctrl->lock, flags); -list_for_each_entry_safe (vbl_ev, tmp, &vbl_ctrl->event_list, node) { -list_del (&vbl_ev->node); -spin_unlock_irqrestore (&vbl_ctrl->lock, flags); - -if (vbl_ev->enable) - kms->funcs->enable_vblank (kms, - priv->crtcs[vbl_ev->crtc_id]); -else - kms->funcs->disable_vblank (kms, - priv->crtcs[vbl_ev->crtc_id]); -kfree (vbl_ev); - -spin_lock_irqsave (&vbl_ctrl->lock, flags); - } +if (vbl_work->enable) + kms->funcs->enable_vblank (kms, priv->crtcs[vbl_work->crtc_id]); +else + kms->funcs->disable_vblank (kms, priv->crtcs[vbl_work->crtc_id]); -spin_unlock_irqrestore (&vbl_ctrl->lock, flags); +kfree (vbl_work); }static int vblank_ctrl_queue_work (struct msm_drm_private *priv,int crtc_id,bool enable) { -struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev; -unsigned long flags; +struct msm_vblank_work *vbl_work; - vbl_ev =kzalloc (sizeof (*vbl_ev),GFP_ATOMIC ); -if (!vbl_ev) + vbl_work =kzalloc (sizeof (*vbl_work),GFP_ATOMIC ); +if (!vbl_work)return -ENOMEM ; - vbl_ev->crtc_id = crtc_id; - vbl_ev->enable = enable; +INIT_WORK (&vbl_work->work, vblank_ctrl_worker); -spin_lock_irqsave (&vbl_ctrl->lock, flags); -list_add_tail (&vbl_ev->node, &vbl_ctrl->event_list); -spin_unlock_irqrestore (&vbl_ctrl->lock, flags); + vbl_work->crtc_id = crtc_id; + vbl_work->enable = enable; + vbl_work->priv = priv; -schedule_work (&vbl_ctrl->work); +schedule_work (&vbl_work->work);return 0 ; }@@ -269,14 +252,13 @@ static int msm_drm_uninit(struct device *dev)struct msm_drm_private *priv = ddev->dev_private;struct msm_kms *kms = priv->kms;
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device *dev) · line 261
Quoted 10 lines
drivers/gpu/drm/msm/msm_drv.c +1
-2struct msm_mdss *mdss = priv->mdss; -struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev, *tmp;int i;/* We must cancel and cleanup any pending vblank enable/disable * work before drm_irq_uninstall() to avoid work re-enabling an * irq after uninstall has disabled it. */ +
Uneeded new line here, but does the comment above still make any sense? It doesn't havec anything to do with the shrinker for sure.
Quoted 50 lines
drivers/gpu/drm/msm/msm_drv.c +0
-9msm_gem_shrinker_cleanup (ddev);drm_kms_helper_poll_fini (ddev);@@ -292,12 +274,6 @@ static int msm_drm_uninit(struct device *dev)#endif drm_mode_config_cleanup (ddev); -flush_work (&vbl_ctrl->work); -list_for_each_entry_safe (vbl_ev, tmp, &vbl_ctrl->event_list, node) { -list_del (&vbl_ev->node); -kfree (vbl_ev); - } -/* clean up event worker threads */ for (i =0 ; i < priv->num_crtcs; i++) {if (priv->event_thread[i].thread) {@@ -469,9 +445,6 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) priv->wq =alloc_ordered_workqueue ("msm" ,0 );INIT_LIST_HEAD (&priv->inactive_list); -INIT_LIST_HEAD (&priv->vblank_ctrl.event_list); -INIT_WORK (&priv->vblank_ctrl.work, vblank_ctrl_worker); -spin_lock_init (&priv->vblank_ctrl.lock);drm_mode_config_init (ddev);drivers/gpu/drm/msm/msm_drv.h +0
-7@@ -77,12 +77,6 @@ enum msm_mdp_plane_property { PLANE_PROP_MAX_NUM }; -struct msm_vblank_ctrl { -struct work_struct work; -struct list_head event_list; -spinlock_t lock; -}; -#define MSM_GPU_MAX_RINGS 4#define MAX_H_TILES_PER_DISPLAY 2@@ -225,7 +219,6 @@ struct msm_drm_private {struct notifier_block vmap_notifier;struct shrinker shrinker; -struct msm_vblank_ctrl vblank_ctrl;struct drm_atomic_state *pm_state; };
Signature · 6 lines
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project _______________________________________________ Freedreno mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/freedreno
Quoted 27 lines · Jeykumar Sankaran
To avoid any possible work queues to msm threads, clean up the threads after the CRTC objects are released in config cleanup.
changes in v2:
- fix race condition before kthread flush and stop (Sean Paul)
- usekthread_destroy_workerfor cleaning up kthread (Sean Paul)Signed-off-by Jeykumar Sankaran <[email protected]>--- drivers/gpu/drm/msm/msm_drv.c | 36 +++++++++++++++++------------------- 1 file changed, 17 insertions(+), 19 deletions(-)drivers/gpu/drm/msm/msm_drv.c +6
-0@@ -278,6 +278,21 @@ static int msm_drm_uninit(struct device *dev)* work before drm_irq_uninstall() to avoid work re-enabling an * irq after uninstall has disabled it. */ +msm_gem_shrinker_cleanup (ddev); + +drm_kms_helper_poll_fini (ddev); + +drm_dev_unregister (ddev); +
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device *dev) · line 296
Quoted 10 lines
drivers/gpu/drm/msm/msm_drv.c +9
-0+msm_perf_debugfs_cleanup (priv); +msm_rd_debugfs_cleanup (priv); + +#ifdef CONFIG_DRM_FBDEV_EMULATION +if (fbdev && priv->fbdev) +msm_fbdev_free (ddev); +#endif +drm_mode_config_cleanup (ddev); +kthread_flush_work (&vbl_ctrl->work);
What happens in the vbl_ctrl thread when it runs with drm resources cleaned up?
Sean
Quoted 46 lines
drivers/gpu/drm/msm/msm_drv.c +2
-19list_for_each_entry_safe (vbl_ev, tmp, &vbl_ctrl->event_list, node) {list_del (&vbl_ev->node);@@ -287,33 +302,16 @@ static int msm_drm_uninit(struct device *dev)/* clean up display commit/event worker threads */ for (i =0 ; i < priv->num_crtcs; i++) {if (priv->disp_thread[i].thread) { -kthread_flush_worker (&priv->disp_thread[i].worker); -kthread_stop (priv->disp_thread[i].thread); +kthread_destroy_worker (&priv->disp_thread[i].worker); priv->disp_thread[i].thread =NULL ; }if (priv->event_thread[i].thread) { -kthread_flush_worker (&priv->event_thread[i].worker); -kthread_stop (priv->event_thread[i].thread); +kthread_destroy_worker (&priv->event_thread[i].worker); priv->event_thread[i].thread =NULL ; } } -msm_gem_shrinker_cleanup (ddev); - -drm_kms_helper_poll_fini (ddev); - -drm_dev_unregister (ddev); - -msm_perf_debugfs_cleanup (priv); -msm_rd_debugfs_cleanup (priv); - -#ifdef CONFIG_DRM_FBDEV_EMULATION -if (fbdev && priv->fbdev) -msm_fbdev_free (ddev); -#endif -drm_mode_config_cleanup (ddev); -pm_runtime_get_sync (dev);drm_irq_uninstall (ddev);pm_runtime_put_sync (dev);Signature · 7 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project _______________________________________________ Freedreno mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/freedreno
Signature · 5 lines
Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ Freedreno mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [PATCH v2 5/5] drm/msm: subclass work object for vblank events
In reply to a message not in the archiveQuoted 8 lines · Jeykumar Sankaran
msm maintains a separate structure to define vblank work definitions and a list to track events submitted to the workqueue. We can avoid this redundant list and its protection mechanism, if we subclass the work object to encapsulate vblank event parameters.
changes in v2:
- subclass optimization on system wq (Sean Paul)
I wouldn't do it like this, tbh. One problem is that you've lost your flush() on unbind, so there's no way to know if you have workers in the wild waiting to enable/disable vblank.
Another issues is that AFAICT, we don't need a queue of enables/disables, but rather just the last requested state (ie: should we be on or off). So things don't need to be this complicated (and we're possibly thrashing vblank on/off for no reason).
I'm still of the mind that you should just make this synchronous and be done with the threads (especially since we're still uncovering/introducing races!).
Sean
Quoted 164 lines
Signed-off-by Jeykumar Sankaran <[email protected]>--- drivers/gpu/drm/msm/msm_drv.c | 67 +++++++++++++------------------------------ drivers/gpu/drm/msm/msm_drv.h | 7 ----- 2 files changed, 20 insertions(+), 54 deletions(-)drivers/gpu/drm/msm/msm_drv.c +20
-47@@ -203,61 +203,44 @@ u32 msm_readl(const void __iomem *addr)return val; } -struct vblank_event { -struct list_head node; +struct msm_vblank_work { +struct work_struct work;int crtc_id;bool enable; +struct msm_drm_private *priv; };static void vblank_ctrl_worker (struct work_struct *work) { -struct msm_vblank_ctrl *vbl_ctrl =container_of (work, - struct msm_vblank_ctrl, work); -struct msm_drm_private *priv =container_of (vbl_ctrl, - struct msm_drm_private, vblank_ctrl); +struct msm_vblank_work *vbl_work =container_of (work, + struct msm_vblank_work, work); +struct msm_drm_private *priv = vbl_work->priv;struct msm_kms *kms = priv->kms; -struct vblank_event *vbl_ev, *tmp; -unsigned long flags; - -spin_lock_irqsave (&vbl_ctrl->lock, flags); -list_for_each_entry_safe (vbl_ev, tmp, &vbl_ctrl->event_list, node) { -list_del (&vbl_ev->node); -spin_unlock_irqrestore (&vbl_ctrl->lock, flags); - -if (vbl_ev->enable) - kms->funcs->enable_vblank (kms, - priv->crtcs[vbl_ev->crtc_id]); -else - kms->funcs->disable_vblank (kms, - priv->crtcs[vbl_ev->crtc_id]); -kfree (vbl_ev); - -spin_lock_irqsave (&vbl_ctrl->lock, flags); - } +if (vbl_work->enable) + kms->funcs->enable_vblank (kms, priv->crtcs[vbl_work->crtc_id]); +else + kms->funcs->disable_vblank (kms, priv->crtcs[vbl_work->crtc_id]); -spin_unlock_irqrestore (&vbl_ctrl->lock, flags); +kfree (vbl_work); }static int vblank_ctrl_queue_work (struct msm_drm_private *priv,int crtc_id,bool enable) { -struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev; -unsigned long flags; +struct msm_vblank_work *vbl_work; - vbl_ev =kzalloc (sizeof (*vbl_ev),GFP_ATOMIC ); -if (!vbl_ev) + vbl_work =kzalloc (sizeof (*vbl_work),GFP_ATOMIC ); +if (!vbl_work)return -ENOMEM ; - vbl_ev->crtc_id = crtc_id; - vbl_ev->enable = enable; +INIT_WORK (&vbl_work->work, vblank_ctrl_worker); -spin_lock_irqsave (&vbl_ctrl->lock, flags); -list_add_tail (&vbl_ev->node, &vbl_ctrl->event_list); -spin_unlock_irqrestore (&vbl_ctrl->lock, flags); + vbl_work->crtc_id = crtc_id; + vbl_work->enable = enable; + vbl_work->priv = priv; -schedule_work (&vbl_ctrl->work); +schedule_work (&vbl_work->work);return 0 ; }@@ -269,14 +252,13 @@ static int msm_drm_uninit(struct device *dev)struct msm_drm_private *priv = ddev->dev_private;struct msm_kms *kms = priv->kms;struct msm_mdss *mdss = priv->mdss; -struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev, *tmp;int i;/* We must cancel and cleanup any pending vblank enable/disable * work before drm_irq_uninstall() to avoid work re-enabling an * irq after uninstall has disabled it. */ +msm_gem_shrinker_cleanup (ddev);drm_kms_helper_poll_fini (ddev);@@ -292,12 +274,6 @@ static int msm_drm_uninit(struct device *dev)#endif drm_mode_config_cleanup (ddev); -flush_work (&vbl_ctrl->work); -list_for_each_entry_safe (vbl_ev, tmp, &vbl_ctrl->event_list, node) { -list_del (&vbl_ev->node); -kfree (vbl_ev); - } -/* clean up event worker threads */ for (i =0 ; i < priv->num_crtcs; i++) {if (priv->event_thread[i].thread) {@@ -469,9 +445,6 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv) priv->wq =alloc_ordered_workqueue ("msm" ,0 );INIT_LIST_HEAD (&priv->inactive_list); -INIT_LIST_HEAD (&priv->vblank_ctrl.event_list); -INIT_WORK (&priv->vblank_ctrl.work, vblank_ctrl_worker); -spin_lock_init (&priv->vblank_ctrl.lock);drm_mode_config_init (ddev);drivers/gpu/drm/msm/msm_drv.h +0
-7@@ -77,12 +77,6 @@ enum msm_mdp_plane_property { PLANE_PROP_MAX_NUM }; -struct msm_vblank_ctrl { -struct work_struct work; -struct list_head event_list; -spinlock_t lock; -}; -#define MSM_GPU_MAX_RINGS 4#define MAX_H_TILES_PER_DISPLAY 2@@ -225,7 +219,6 @@ struct msm_drm_private {struct notifier_block vmap_notifier;struct shrinker shrinker; -struct msm_vblank_ctrl vblank_ctrl;struct drm_atomic_state *pm_state; };Signature · 7 lines
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project _______________________________________________ Freedreno mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/freedreno
Signature · 5 lines
Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ Freedreno mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [PATCH v2 5/5] drm/msm: subclass work object for vblank events
Quoted 24 lines · Sean Paul
Quoted 8 lines · Jeykumar Sankaran
msm maintains a separate structure to define vblank work definitions and a list to track events submitted to the workqueue. We can avoid this redundant list and its protection mechanism, if we subclass the work object to encapsulate vblank event parameters.
changes in v2:
- subclass optimization on system wq (Sean Paul)I wouldn't do it like this, tbh. One problem is that you've lost your
flush()on
unbind, so there's no way to know if you have workers in the wild waiting to
enable/disable vblank.Another issues is that AFAICT, we don't need a queue of enables/disables, but
rather just the last requested state (ie: should we be on or off). So things
don't need to be this complicated (and we're possibly thrashing vblank on/off
for no reason).
Quoted 5 lines
I'm still of the mind that you should just make this synchronous and be done
with the threads (especially since we're still uncovering/introducing races!).
While scoping out the effort to make vblank events synchronous, I found that the spinlock locking order of vblank request sequence and vblank callback sequences are the opposite.
In DPU, drm_vblank_enable acquires vblank_time_lock before registering the crtc to encoder which happens after acquiring encoder_spinlock. But the vblank_callback acquires encoder_spinlock before accessing the registered crtc and calling into drm_vblank_handler which tries to acquire vblank_time_lock. Acquiring both vblank_time_lock and encoder_spinlock in the same thread is leading to deadlock.
In MDP5, I see the same pattern between vblank_time_lock and list_lock which is used to track the irq handlers.
I believe that explains why msm_drv is queuing the vblank enable/disable works to WQ after acquiring vblank_time_lock.
Thanks,
Jeykumar S.
Quoted 180 lines
Sean
Quoted 4 lines
Signed-off-by Jeykumar Sankaran <[email protected]>--- drivers/gpu/drm/msm/msm_drv.c | 67+++++++++++++------------------------------
Review
Quoted 4 lines
drivers/gpu/drm/msm/msm_drv.h | 7 ----- 2 files changed, 20 insertions(+), 54 deletions(-)(quoted diff) +0
-0
b/drivers/gpu/drm/msm/msm_drv.cQuoted 10 lines
index 6d6c73b..8da5be2 100644
drivers/gpu/drm/msm/msm_drv.c +1
-2@@ -203,61 +203,44 @@ u32 msm_readl(const void __iomem *addr)return val; } -struct vblank_event { -struct list_head node; +struct msm_vblank_work {drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 215
Quoted 10 lines
drivers/gpu/drm/msm/msm_drv.c +2
-2+struct work_struct work;int crtc_id;bool enable; +struct msm_drm_private *priv; };static void vblank_ctrl_worker (struct work_struct *work) { - struct msm_vblank_ctrl *vbl_ctrl = container_of(work, - struct msm_vblank_ctrl,work);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 217
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2- struct msm_drm_private *priv = container_of(vbl_ctrl, - struct msm_drm_private,
vblank_ctrl);drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 216
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +2
-0+ struct msm_vblank_work *vbl_work = container_of(work, + struct msm_vblank_work,work);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr)
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c +1
-5+struct msm_drm_private *priv = vbl_work->priv;struct msm_kms *kms = priv->kms; -struct vblank_event *vbl_ev, *tmp; -unsigned long flags; - -spin_lock_irqsave (&vbl_ctrl->lock, flags); -list_for_each_entry_safe (vbl_ev, tmp, &vbl_ctrl->event_list, node){
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 235
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.c +0
-6-list_del (&vbl_ev->node); -spin_unlock_irqrestore (&vbl_ctrl->lock, flags); - -if (vbl_ev->enable) - kms->funcs->enable_vblank(kms, -priv->crtcs[vbl_ev->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 235
Quoted 3 lines
drivers/gpu/drm/msm/msm_drv.c +0
-3-else - kms->funcs->disable_vblank(kms, -priv->crtcs[vbl_ev->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr)
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c +2
-4-kfree (vbl_ev); - -spin_lock_irqsave (&vbl_ctrl->lock, flags); - } +if (vbl_work->enable) + kms->funcs->enable_vblank(kms,priv->crtcs[vbl_work->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 222
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +2
-0+else + kms->funcs->disable_vblank(kms,priv->crtcs[vbl_work->crtc_id]);
Quoted 52 lines
drivers/gpu/drm/msm/msm_drv.c +10
-14-spin_unlock_irqrestore (&vbl_ctrl->lock, flags); +kfree (vbl_work); }static int vblank_ctrl_queue_work (struct msm_drm_private *priv,int crtc_id,bool enable) { -struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev; -unsigned long flags; +struct msm_vblank_work *vbl_work; - vbl_ev =kzalloc (sizeof (*vbl_ev),GFP_ATOMIC ); -if (!vbl_ev) + vbl_work =kzalloc (sizeof (*vbl_work),GFP_ATOMIC ); +if (!vbl_work)return -ENOMEM ; - vbl_ev->crtc_id = crtc_id; - vbl_ev->enable = enable; +INIT_WORK (&vbl_work->work, vblank_ctrl_worker); -spin_lock_irqsave (&vbl_ctrl->lock, flags); -list_add_tail (&vbl_ev->node, &vbl_ctrl->event_list); -spin_unlock_irqrestore (&vbl_ctrl->lock, flags); + vbl_work->crtc_id = crtc_id; + vbl_work->enable = enable; + vbl_work->priv = priv; -schedule_work (&vbl_ctrl->work); +schedule_work (&vbl_work->work);return 0 ; }@@ -269,14 +252,13 @@ static int msm_drm_uninit(struct device *dev)struct msm_drm_private *priv = ddev->dev_private;struct msm_kms *kms = priv->kms;struct msm_mdss *mdss = priv->mdss; -struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev, *tmp;int i;/* We must cancel and cleanup any pending vblank enable/disable * work before drm_irq_uninstall() to avoid work re-enabling an * irq after uninstall has disabled it. */ +msm_gem_shrinker_cleanup (ddev);drm_kms_helper_poll_fini (ddev);drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device *dev) · line 296
Quoted 5 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2@@ -292,12 +274,6 @@ static int msm_drm_uninit(struct device *dev)#endif drm_mode_config_cleanup (ddev); -flush_work (&vbl_ctrl->work); -list_for_each_entry_safe (vbl_ev, tmp, &vbl_ctrl->event_list, node){
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_init(struct device *dev, struct
Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c +0
-4-list_del (&vbl_ev->node); -kfree (vbl_ev); - } -/* clean up event worker threads */ for (i =0 ; i < priv->num_crtcs; i++) {if (priv->event_thread[i].thread) {@@ -469,9 +445,6 @@ static int msm_drm_init(struct device *dev, struct
drm_driver*drv)Review
Quoted 10 lines
drivers/gpu/drm/msm/msm_drv.c +0
-3priv->wq =alloc_ordered_workqueue ("msm" ,0 );INIT_LIST_HEAD (&priv->inactive_list); -INIT_LIST_HEAD (&priv->vblank_ctrl.event_list); -INIT_WORK (&priv->vblank_ctrl.work, vblank_ctrl_worker); -spin_lock_init (&priv->vblank_ctrl.lock);drm_mode_config_init (ddev);(quoted diff) +0
-0
b/drivers/gpu/drm/msm/msm_drv.hQuoted 25 lines
index 05d33a7..d4cbde2 100644
drivers/gpu/drm/msm/msm_drv.h +0
-8@@ -77,12 +77,6 @@ enum msm_mdp_plane_property { PLANE_PROP_MAX_NUM }; -struct msm_vblank_ctrl { -struct work_struct work; -struct list_head event_list; -spinlock_t lock; -}; -#define MSM_GPU_MAX_RINGS 4#define MAX_H_TILES_PER_DISPLAY 2@@ -225,7 +219,6 @@ struct msm_drm_private {struct notifier_block vmap_notifier;struct shrinker shrinker; -struct msm_vblank_ctrl vblank_ctrl;struct drm_atomic_state *pm_state; }; --Quoted 1 line
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
Quoted 6 lines
a Linux Foundation Collaborative Project
_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno
Signature · 5 lines
Jeykumar S _______________________________________________ Freedreno mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [PATCH v2 5/5] drm/msm: subclass work object for vblank events
In reply to a message not in the archiveQuoted 37 lines · Jeykumar Sankaran
Quoted 26 lines · Sean Paul
Quoted 8 lines · Jeykumar Sankaran
msm maintains a separate structure to define vblank work definitions and a list to track events submitted to the workqueue. We can avoid this redundant list and its protection mechanism, if we subclass the work object to encapsulate vblank event parameters.
changes in v2:
- subclass optimization on system wq (Sean Paul)I wouldn't do it like this, tbh. One problem is that you've lost your
flush()on
unbind, so there's no way to know if you have workers in the wild waiting
to
enable/disable vblank.Another issues is that AFAICT, we don't need a queue of enables/disables,
but
rather just the last requested state (ie: should we be on or off). So things
don't need to be this complicated (and we're possibly thrashing vblank on/off
for no reason).Quoted 5 lines
I'm still of the mind that you should just make this synchronous and be done
with the threads (especially since we're still uncovering/introducing races!).While scoping out the effort to make vblank events synchronous, I found that the spinlock locking order of vblank request sequence and vblank callback
sequences are the opposite.
Quoted 8 lines
In DPU,
drm_vblank_enableacquiresvblank_time_lockbefore registering the crtc to encoder which happens after acquiringencoder_spinlock. But thevblank_callbackacquiresencoder_spinlockbefore accessing the registered
crtc and calling intodrm_vblank_handlerwhich tries to acquirevblank_time_lock.
Acquiring bothvblank_time_lockandencoder_spinlockin the same thread is leading to deadlock.
Hmm, I'm not sure I follow. Are you seeing issues where irq overlaps with enable/disable? I hacked in sync vblank enable/disable quickly to see if I could reproduce what you're seeing, but things seemed well behaved.
I do see that there is a chance to call drm_handle_vblank() while holding enc_spinlock, but couldn't find any obvious lock recursion there.
Maybe a callstack or lockdep splat would help?
Sean
Here's my hack to bypass the display thread:
drivers/gpu/drm/msm/msm_drv.c +6 -11
@@ -242,24 +242,19 @@ static void vblank_ctrl_worker(struct kthread_work *work)static int vblank_ctrl_queue_work (struct msm_drm_private *priv,int crtc_id,bool enable) { +struct msm_kms *kms = priv->kms;struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev;unsigned long flags; - vbl_ev =kzalloc (sizeof (*vbl_ev),GFP_ATOMIC ); -if (!vbl_ev) -return -ENOMEM ; +spin_lock_irqsave (&vbl_ctrl->lock, flags); - vbl_ev->crtc_id = crtc_id; - vbl_ev->enable = enable; +if (enable) + kms->funcs->enable_vblank (kms, priv->crtcs[crtc_id]); +else + kms->funcs->disable_vblank (kms, priv->crtcs[crtc_id]); -spin_lock_irqsave (&vbl_ctrl->lock, flags); -list_add_tail (&vbl_ev->node, &vbl_ctrl->event_list);spin_unlock_irqrestore (&vbl_ctrl->lock, flags); -kthread_queue_work (&priv->disp_thread[crtc_id].worker, - &vbl_ctrl->work); -return 0 ; }
Quoted 193 lines
In MDP5, I see the same pattern between
vblank_time_lockandlist_lockwhich is used to track the irq handlers.I believe that explains why
msm_drvis queuing the vblank enable/disable works to WQ after acquiringvblank_time_lock.Thanks,
Jeykumar S.Quoted 180 lines
Sean
Quoted 4 lines
Signed-off-by Jeykumar Sankaran <[email protected]>--- drivers/gpu/drm/msm/msm_drv.c | 67+++++++++++++------------------------------
Review
Quoted 4 lines
drivers/gpu/drm/msm/msm_drv.h | 7 ----- 2 files changed, 20 insertions(+), 54 deletions(-)(quoted diff) +0
-0
b/drivers/gpu/drm/msm/msm_drv.cQuoted 10 lines
index 6d6c73b..8da5be2 100644
drivers/gpu/drm/msm/msm_drv.c +1
-2@@ -203,61 +203,44 @@ u32 msm_readl(const void __iomem *addr)return val; } -struct vblank_event { -struct list_head node; +struct msm_vblank_work {drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 215
Quoted 10 lines
drivers/gpu/drm/msm/msm_drv.c +2
-2+struct work_struct work;int crtc_id;bool enable; +struct msm_drm_private *priv; };static void vblank_ctrl_worker (struct work_struct *work) { - struct msm_vblank_ctrl *vbl_ctrl = container_of(work, - struct msm_vblank_ctrl,work);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 217
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2- struct msm_drm_private *priv = container_of(vbl_ctrl, - struct msm_drm_private,
vblank_ctrl);drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 216
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +2
-0+ struct msm_vblank_work *vbl_work = container_of(work, + struct msm_vblank_work,work);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 223
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c +1
-5+struct msm_drm_private *priv = vbl_work->priv;struct msm_kms *kms = priv->kms; -struct vblank_event *vbl_ev, *tmp; -unsigned long flags; - -spin_lock_irqsave (&vbl_ctrl->lock, flags); -list_for_each_entry_safe (vbl_ev, tmp, &vbl_ctrl->event_list, node){
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 229
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.c +0
-6-list_del (&vbl_ev->node); -spin_unlock_irqrestore (&vbl_ctrl->lock, flags); - -if (vbl_ev->enable) - kms->funcs->enable_vblank(kms, -priv->crtcs[vbl_ev->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 232
Quoted 3 lines
drivers/gpu/drm/msm/msm_drv.c +0
-3-else - kms->funcs->disable_vblank(kms, -priv->crtcs[vbl_ev->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 220
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c +2
-4-kfree (vbl_ev); - -spin_lock_irqsave (&vbl_ctrl->lock, flags); - } +if (vbl_work->enable) + kms->funcs->enable_vblank(kms,priv->crtcs[vbl_work->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 222
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +2
-0+else + kms->funcs->disable_vblank(kms,priv->crtcs[vbl_work->crtc_id]);
Quoted 52 lines
drivers/gpu/drm/msm/msm_drv.c +10
-14-spin_unlock_irqrestore (&vbl_ctrl->lock, flags); +kfree (vbl_work); }static int vblank_ctrl_queue_work (struct msm_drm_private *priv,int crtc_id,bool enable) { -struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev; -unsigned long flags; +struct msm_vblank_work *vbl_work; - vbl_ev =kzalloc (sizeof (*vbl_ev),GFP_ATOMIC ); -if (!vbl_ev) + vbl_work =kzalloc (sizeof (*vbl_work),GFP_ATOMIC ); +if (!vbl_work)return -ENOMEM ; - vbl_ev->crtc_id = crtc_id; - vbl_ev->enable = enable; +INIT_WORK (&vbl_work->work, vblank_ctrl_worker); -spin_lock_irqsave (&vbl_ctrl->lock, flags); -list_add_tail (&vbl_ev->node, &vbl_ctrl->event_list); -spin_unlock_irqrestore (&vbl_ctrl->lock, flags); + vbl_work->crtc_id = crtc_id; + vbl_work->enable = enable; + vbl_work->priv = priv; -schedule_work (&vbl_ctrl->work); +schedule_work (&vbl_work->work);return 0 ; }@@ -269,14 +252,13 @@ static int msm_drm_uninit(struct device *dev)struct msm_drm_private *priv = ddev->dev_private;struct msm_kms *kms = priv->kms;struct msm_mdss *mdss = priv->mdss; -struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev, *tmp;int i;/* We must cancel and cleanup any pending vblank enable/disable * work before drm_irq_uninstall() to avoid work re-enabling an * irq after uninstall has disabled it. */ +msm_gem_shrinker_cleanup (ddev);drm_kms_helper_poll_fini (ddev);drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device *dev) · line 296
Quoted 5 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2@@ -292,12 +274,6 @@ static int msm_drm_uninit(struct device *dev)#endif drm_mode_config_cleanup (ddev); -flush_work (&vbl_ctrl->work); -list_for_each_entry_safe (vbl_ev, tmp, &vbl_ctrl->event_list, node){
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_init(struct device *dev, struct
Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c +0
-4-list_del (&vbl_ev->node); -kfree (vbl_ev); - } -/* clean up event worker threads */ for (i =0 ; i < priv->num_crtcs; i++) {if (priv->event_thread[i].thread) {@@ -469,9 +445,6 @@ static int msm_drm_init(struct device *dev, struct
drm_driver*drv)Review
Quoted 10 lines
drivers/gpu/drm/msm/msm_drv.c +0
-3priv->wq =alloc_ordered_workqueue ("msm" ,0 );INIT_LIST_HEAD (&priv->inactive_list); -INIT_LIST_HEAD (&priv->vblank_ctrl.event_list); -INIT_WORK (&priv->vblank_ctrl.work, vblank_ctrl_worker); -spin_lock_init (&priv->vblank_ctrl.lock);drm_mode_config_init (ddev);(quoted diff) +0
-0
b/drivers/gpu/drm/msm/msm_drv.hQuoted 25 lines
index 05d33a7..d4cbde2 100644
drivers/gpu/drm/msm/msm_drv.h +0
-8@@ -77,12 +77,6 @@ enum msm_mdp_plane_property { PLANE_PROP_MAX_NUM }; -struct msm_vblank_ctrl { -struct work_struct work; -struct list_head event_list; -spinlock_t lock; -}; -#define MSM_GPU_MAX_RINGS 4#define MAX_H_TILES_PER_DISPLAY 2@@ -225,7 +219,6 @@ struct msm_drm_private {struct notifier_block vmap_notifier;struct shrinker shrinker; -struct msm_vblank_ctrl vblank_ctrl;struct drm_atomic_state *pm_state; }; --Quoted 1 line
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
Quoted 6 lines
a Linux Foundation Collaborative Project
_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedrenoSignature · 1 line
Jeykumar S
Signature · 5 lines
Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ Freedreno mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [PATCH v2 5/5] drm/msm: subclass work object for vblank events
Quoted 48 lines · Sean Paul
Quoted 27 lines · Jeykumar Sankaran
Quoted 26 lines · Sean Paul
Quoted 8 lines · Jeykumar Sankaran
msm maintains a separate structure to define vblank work definitions and a list to track events submitted to the workqueue. We can avoid this redundant list and its protection mechanism, if we subclass the work object to encapsulate vblank event parameters.
changes in v2:
- subclass optimization on system wq (Sean Paul)I wouldn't do it like this, tbh. One problem is that you've lost your
flush()on
unbind, so there's no way to know if you have workers in the wild waiting
to
enable/disable vblank.Another issues is that AFAICT, we don't need a queue of enables/disables,
but
rather just the last requested state (ie: should we be on or off). So things
don't need to be this complicated (and we're possibly thrashing vblank on/off
for no reason).Quoted 1 line
Quoted 1 line
I'm still of the mind that you should just make this synchronous and
be
Quoted 9 lines
Quoted 4 lines
done
with the threads (especially since we're still uncovering/introducing races!).While scoping out the effort to make vblank events synchronous, I found that the spinlock locking order of vblank request sequence and vblank callback
sequences are the opposite.Quoted 8 lines
In DPU,
drm_vblank_enableacquiresvblank_time_lockbefore registering the crtc to encoder which happens after acquiringencoder_spinlock. But thevblank_callbackacquiresencoder_spinlockbefore accessing the registered
crtc and calling intodrm_vblank_handlerwhich tries to acquirevblank_time_lock.
Acquiring bothvblank_time_lockandencoder_spinlockin the same thread is leading to deadlock.
Quoted 5 lines
Hmm, I'm not sure I follow. Are you seeing issues where irq overlaps with enable/disable? I hacked in sync vblank enable/disable quickly to see if I could
reproduce what you're seeing, but things seemed well behaved.
The race is between drm_vblank_get/put and vblank_handler contexts.
When made synchronous:
while calling drm_vblank_get, the callstack looks like below:drm_vblank_get -> drm_vblank_enable (acquires vblank_time_lock) -> __enable_vblank -> dpu_crtc_vblank -> dpu_encoder_toggle_vblank_for_crtc (tries to acquire enc_spinlock)
In vblank handler, the call stack will be:dpu_encoder_phys_vid_vblank_irq -> dpu_encoder_vblank_callback (acquires enc_spinlock) -> dpu_crtc_vblank_callback -> drm_handle_vblank (tries to acquire vblank_time_lock)
Quoted 37 lines
I do see that there is a chance to call
drm_handle_vblank()while holdingenc_spinlock, but couldn't find any obvious lock recursion there.Maybe a callstack or lockdep splat would help?
Sean
Here's my hack to bypass the display thread:
drivers/gpu/drm/msm/msm_drv.c +0
-0@@ -242,24 +242,19 @@ static void vblank_ctrl_worker(struct
kthread_work
*work)
static int vblank_ctrl_queue_work(structmsm_drm_private*priv,drivers/gpu/drm/msm/msm_drv.c +6
-6int crtc_id, bool enable) { +struct msm_kms *kms = priv->kms;struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev;unsigned long flags; - vbl_ev =kzalloc (sizeof (*vbl_ev),GFP_ATOMIC ); -if (!vbl_ev) -return -ENOMEM ; +spin_lock_irqsave (&vbl_ctrl->lock, flags); - vbl_ev->crtc_id = crtc_id; - vbl_ev->enable = enable; +if (enable) + kms->funcs->enable_vblank (kms, priv->crtcs[crtc_id]); +else + kms->funcs->disable_vblank (kms, priv->crtcs[crtc_id]);
drivers/gpu/drm/msm/msm_drv.c static void vblank_ctrl_worker(struct kthread_work *work) · line 259
Quoted 10 lines
drivers/gpu/drm/msm/msm_drv.c +0
-5-spin_lock_irqsave (&vbl_ctrl->lock, flags); -list_add_tail (&vbl_ev->node, &vbl_ctrl->event_list);spin_unlock_irqrestore (&vbl_ctrl->lock, flags); -kthread_queue_work (&priv->disp_thread[crtc_id].worker, - &vbl_ctrl->work); -return 0 ; }
Even with your patch above, I see frame is getting stuck but it recovers in a while. The patch I tried was assigning crtc->funcs->enable_vblank/disable_vblank so that __enable_vblank can call crtc directly. But the above callstack is still valid for your patch.
Thanks,
Jeykumar S.
Quoted 197 lines
Quoted 4 lines
In MDP5, I see the same pattern between
vblank_time_lockandlist_lockwhich
Quoted 135 lines
is used to track the irq handlers.
I believe that explains why
msm_drvis queuing the vblank enable/disable works to WQ after acquiringvblank_time_lock.Thanks,
Jeykumar S.Quoted 127 lines
Sean
Quoted 4 lines
Signed-off-by Jeykumar Sankaran <[email protected]>--- drivers/gpu/drm/msm/msm_drv.c | 67+++++++++++++------------------------------
Review
Quoted 4 lines
drivers/gpu/drm/msm/msm_drv.h | 7 ----- 2 files changed, 20 insertions(+), 54 deletions(-)(quoted diff) +0
-0
b/drivers/gpu/drm/msm/msm_drv.cQuoted 10 lines
index 6d6c73b..8da5be2 100644
drivers/gpu/drm/msm/msm_drv.c +1
-2@@ -203,61 +203,44 @@ u32 msm_readl(const void __iomem *addr)return val; } -struct vblank_event { -struct list_head node; +struct msm_vblank_work {drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 215
Quoted 10 lines
drivers/gpu/drm/msm/msm_drv.c +2
-2+struct work_struct work;int crtc_id;bool enable; +struct msm_drm_private *priv; };static void vblank_ctrl_worker (struct work_struct *work) { - struct msm_vblank_ctrl *vbl_ctrl = container_of(work, - struct msm_vblank_ctrl,work);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 217
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2- struct msm_drm_private *priv = container_of(vbl_ctrl, - struct msm_drm_private,
vblank_ctrl);drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 216
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +2
-0+ struct msm_vblank_work *vbl_work = container_of(work, + struct msm_vblank_work,work);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 223
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c +1
-5+struct msm_drm_private *priv = vbl_work->priv;struct msm_kms *kms = priv->kms; -struct vblank_event *vbl_ev, *tmp; -unsigned long flags; - -spin_lock_irqsave (&vbl_ctrl->lock, flags); -list_for_each_entry_safe (vbl_ev, tmp, &vbl_ctrl->event_list, node){
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 229
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.c +0
-6-list_del (&vbl_ev->node); -spin_unlock_irqrestore (&vbl_ctrl->lock, flags); - -if (vbl_ev->enable) - kms->funcs->enable_vblank(kms, -priv->crtcs[vbl_ev->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 232
Quoted 3 lines
drivers/gpu/drm/msm/msm_drv.c +0
-3-else - kms->funcs->disable_vblank(kms, -priv->crtcs[vbl_ev->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 220
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c +2
-4-kfree (vbl_ev); - -spin_lock_irqsave (&vbl_ctrl->lock, flags); - } +if (vbl_work->enable) + kms->funcs->enable_vblank(kms,priv->crtcs[vbl_work->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 222
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +2
-0+else + kms->funcs->disable_vblank(kms,priv->crtcs[vbl_work->crtc_id]);
Quoted 52 lines
drivers/gpu/drm/msm/msm_drv.c +10
-14-spin_unlock_irqrestore (&vbl_ctrl->lock, flags); +kfree (vbl_work); }static int vblank_ctrl_queue_work (struct msm_drm_private *priv,int crtc_id,bool enable) { -struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev; -unsigned long flags; +struct msm_vblank_work *vbl_work; - vbl_ev =kzalloc (sizeof (*vbl_ev),GFP_ATOMIC ); -if (!vbl_ev) + vbl_work =kzalloc (sizeof (*vbl_work),GFP_ATOMIC ); +if (!vbl_work)return -ENOMEM ; - vbl_ev->crtc_id = crtc_id; - vbl_ev->enable = enable; +INIT_WORK (&vbl_work->work, vblank_ctrl_worker); -spin_lock_irqsave (&vbl_ctrl->lock, flags); -list_add_tail (&vbl_ev->node, &vbl_ctrl->event_list); -spin_unlock_irqrestore (&vbl_ctrl->lock, flags); + vbl_work->crtc_id = crtc_id; + vbl_work->enable = enable; + vbl_work->priv = priv; -schedule_work (&vbl_ctrl->work); +schedule_work (&vbl_work->work);return 0 ; }@@ -269,14 +252,13 @@ static int msm_drm_uninit(struct device *dev)struct msm_drm_private *priv = ddev->dev_private;struct msm_kms *kms = priv->kms;struct msm_mdss *mdss = priv->mdss; -struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev, *tmp;int i;/* We must cancel and cleanup any pending vblank enable/disable * work before drm_irq_uninstall() to avoid work re-enabling an * irq after uninstall has disabled it. */ +msm_gem_shrinker_cleanup (ddev);drm_kms_helper_poll_fini (ddev);drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device *dev) · line 296
Quoted 5 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2@@ -292,12 +274,6 @@ static int msm_drm_uninit(struct device *dev)#endif drm_mode_config_cleanup (ddev); -flush_work (&vbl_ctrl->work); -list_for_each_entry_safe (vbl_ev, tmp, &vbl_ctrl->event_list, node){
Quoted 8 lines
Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_init(struct device *dev,
Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c +0
-4-list_del (&vbl_ev->node); -kfree (vbl_ev); - } -/* clean up event worker threads */ for (i =0 ; i < priv->num_crtcs; i++) {if (priv->event_thread[i].thread) {@@ -469,9 +445,6 @@ static int msm_drm_init(struct device *dev,struct
Quoted 48 lines
Quoted 32 lines
drm_driver*drv)Review
Quoted 10 lines
drivers/gpu/drm/msm/msm_drv.c +0
-3priv->wq =alloc_ordered_workqueue ("msm" ,0 );INIT_LIST_HEAD (&priv->inactive_list); -INIT_LIST_HEAD (&priv->vblank_ctrl.event_list); -INIT_WORK (&priv->vblank_ctrl.work, vblank_ctrl_worker); -spin_lock_init (&priv->vblank_ctrl.lock);drm_mode_config_init (ddev);(quoted diff) +0
-0
b/drivers/gpu/drm/msm/msm_drv.hQuoted 20 lines
index 05d33a7..d4cbde2 100644
drivers/gpu/drm/msm/msm_drv.h +0
-6@@ -77,12 +77,6 @@ enum msm_mdp_plane_property { PLANE_PROP_MAX_NUM }; -struct msm_vblank_ctrl { -struct work_struct work; -struct list_head event_list; -spinlock_t lock; -}; -#define MSM_GPU_MAX_RINGS 4#define MAX_H_TILES_PER_DISPLAY 2@@ -225,7 +219,6 @@ struct msm_drm_private {struct notifier_block vmap_notifier;struct shrinker shrinker;Quoted 13 lines
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.h +0
-2-struct msm_vblank_ctrl vblank_ctrl;struct drm_atomic_state *pm_state; }; --The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
Quoted 6 lines
a Linux Foundation Collaborative Project
_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno--
Jeykumar S
Signature · 5 lines
Jeykumar S _______________________________________________ Freedreno mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [PATCH v2 5/5] drm/msm: subclass work object for vblank events
In reply to a message not in the archiveQuoted 69 lines · Jeykumar Sankaran
Quoted 51 lines · Sean Paul
Quoted 27 lines · Jeykumar Sankaran
Quoted 26 lines · Sean Paul
Quoted 8 lines · Jeykumar Sankaran
msm maintains a separate structure to define vblank work definitions and a list to track events submitted to the workqueue. We can avoid this redundant list and its protection mechanism, if we subclass the work object to encapsulate vblank event parameters.
changes in v2:
- subclass optimization on system wq (Sean Paul)I wouldn't do it like this, tbh. One problem is that you've lost your
flush()on
unbind, so there's no way to know if you have workers in the wild waiting
to
enable/disable vblank.Another issues is that AFAICT, we don't need a queue of enables/disables,
but
rather just the last requested state (ie: should we be on or off). So things
don't need to be this complicated (and we're possibly thrashing vblank on/off
for no reason).Quoted 1 line
Quoted 1 line
I'm still of the mind that you should just make this synchronous and
be
Quoted 10 lines
Quoted 4 lines
done
with the threads (especially since we're still uncovering/introducing races!).While scoping out the effort to make vblank events synchronous, I found
that the spinlock locking order of vblank request sequence and vblank callback
sequences are the opposite.Quoted 10 lines
In DPU,
drm_vblank_enableacquiresvblank_time_lockbefore registering the crtc to encoder which happens after acquiringencoder_spinlock. But
thevblank_callbackacquiresencoder_spinlockbefore accessing the registered
crtc and calling intodrm_vblank_handlerwhich tries to acquirevblank_time_lock.
Acquiring bothvblank_time_lockandencoder_spinlockin the same thread
is leading to deadlock.Quoted 7 lines
Hmm, I'm not sure I follow. Are you seeing issues where irq overlaps with
enable/disable? I hacked in sync vblank enable/disable quickly to see if I
could
reproduce what you're seeing, but things seemed well behaved.The race is between drm_vblank_get/put and
vblank_handlercontexts.When made synchronous:
while calling
drm_vblank_get, the callstack looks like below:drm_vblank_get->drm_vblank_enable(acquiresvblank_time_lock) ->__enable_vblank->dpu_crtc_vblank->dpu_encoder_toggle_vblank_for_crtc(tries to acquireenc_spinlock)
Quoted 4 lines
In vblank handler, the call stack will be:
dpu_encoder_phys_vid_vblank_irq->dpu_encoder_vblank_callback(acquiresenc_spinlock) ->dpu_crtc_vblank_callback->drm_handle_vblank(tries to acquirevblank_time_lock)
Hmm, I'm not sure how this can happen. We acquire and release the enc_spinlock before enabling the irq, yes we will hold on to the vbl_time_lock, but we shouldn't be trying to reacquire an encoder's spinlock after we've enabled it. I don't know how that can deadlock, since we should never be running enable and the handler concurrently.
The only thing I can think of is that the vblank interrupts are firing after vblank has been disabled? In that case, it seems like we should properly flush them.
Sean
Quoted 260 lines
Quoted 40 lines
I do see that there is a chance to call
drm_handle_vblank()while holdingenc_spinlock, but couldn't find any obvious lock recursion there.Maybe a callstack or lockdep splat would help?
Sean
Here's my hack to bypass the display thread:
drivers/gpu/drm/msm/msm_drv.c +6
-6@@ -242,24 +242,19 @@ static void vblank_ctrl_worker(struct kthread_work *work)static int vblank_ctrl_queue_work (struct msm_drm_private *priv,int crtc_id,bool enable) { +struct msm_kms *kms = priv->kms;struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev;unsigned long flags; - vbl_ev =kzalloc (sizeof (*vbl_ev),GFP_ATOMIC ); -if (!vbl_ev) -return -ENOMEM ; +spin_lock_irqsave (&vbl_ctrl->lock, flags); - vbl_ev->crtc_id = crtc_id; - vbl_ev->enable = enable; +if (enable) + kms->funcs->enable_vblank (kms, priv->crtcs[crtc_id]); +else + kms->funcs->disable_vblank (kms, priv->crtcs[crtc_id]);drivers/gpu/drm/msm/msm_drv.c static void vblank_ctrl_worker(struct kthread_work · line 260
Quoted 10 lines
drivers/gpu/drm/msm/msm_drv.c +0
-5-spin_lock_irqsave (&vbl_ctrl->lock, flags); -list_add_tail (&vbl_ev->node, &vbl_ctrl->event_list);spin_unlock_irqrestore (&vbl_ctrl->lock, flags); -kthread_queue_work (&priv->disp_thread[crtc_id].worker, - &vbl_ctrl->work); -return 0 ; }Even with your patch above, I see frame is getting stuck but it recovers in a while.
The patch I tried was assigning crtc->funcs->enable_vblank/disable_vblank so that__enable_vblankcan call crtc directly. But the above callstack is still valid for your patch.Thanks,
Jeykumar S.Quoted 198 lines
Quoted 4 lines
In MDP5, I see the same pattern between
vblank_time_lockandlist_lockwhich
Quoted 136 lines
is used to track the irq handlers.
I believe that explains why
msm_drvis queuing the vblank enable/disable
works to WQ after acquiringvblank_time_lock.Thanks,
Jeykumar S.Quoted 127 lines
Sean
Quoted 4 lines
Signed-off-by Jeykumar Sankaran <[email protected]>--- drivers/gpu/drm/msm/msm_drv.c | 67+++++++++++++------------------------------
Review
Quoted 4 lines
drivers/gpu/drm/msm/msm_drv.h | 7 ----- 2 files changed, 20 insertions(+), 54 deletions(-)(quoted diff) +0
-0
b/drivers/gpu/drm/msm/msm_drv.cQuoted 10 lines
index 6d6c73b..8da5be2 100644
drivers/gpu/drm/msm/msm_drv.c +1
-2@@ -203,61 +203,44 @@ u32 msm_readl(const void __iomem *addr)return val; } -struct vblank_event { -struct list_head node; +struct msm_vblank_work {drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 215
Quoted 10 lines
drivers/gpu/drm/msm/msm_drv.c +2
-2+struct work_struct work;int crtc_id;bool enable; +struct msm_drm_private *priv; };static void vblank_ctrl_worker (struct work_struct *work) { - struct msm_vblank_ctrl *vbl_ctrl = container_of(work, - struct msm_vblank_ctrl,work);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 217
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2- struct msm_drm_private *priv = container_of(vbl_ctrl, - struct msm_drm_private,
vblank_ctrl);drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 216
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +2
-0+ struct msm_vblank_work *vbl_work = container_of(work, + struct msm_vblank_work,work);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 223
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c +1
-5+struct msm_drm_private *priv = vbl_work->priv;struct msm_kms *kms = priv->kms; -struct vblank_event *vbl_ev, *tmp; -unsigned long flags; - -spin_lock_irqsave (&vbl_ctrl->lock, flags); -list_for_each_entry_safe (vbl_ev, tmp, &vbl_ctrl->event_list, node){
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 229
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.c +0
-6-list_del (&vbl_ev->node); -spin_unlock_irqrestore (&vbl_ctrl->lock, flags); - -if (vbl_ev->enable) - kms->funcs->enable_vblank(kms, -priv->crtcs[vbl_ev->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 232
Quoted 3 lines
drivers/gpu/drm/msm/msm_drv.c +0
-3-else - kms->funcs->disable_vblank(kms, -priv->crtcs[vbl_ev->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 220
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c +2
-4-kfree (vbl_ev); - -spin_lock_irqsave (&vbl_ctrl->lock, flags); - } +if (vbl_work->enable) + kms->funcs->enable_vblank(kms,priv->crtcs[vbl_work->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 222
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +2
-0+else + kms->funcs->disable_vblank(kms,priv->crtcs[vbl_work->crtc_id]);
Quoted 52 lines
drivers/gpu/drm/msm/msm_drv.c +10
-14-spin_unlock_irqrestore (&vbl_ctrl->lock, flags); +kfree (vbl_work); }static int vblank_ctrl_queue_work (struct msm_drm_private *priv,int crtc_id,bool enable) { -struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev; -unsigned long flags; +struct msm_vblank_work *vbl_work; - vbl_ev =kzalloc (sizeof (*vbl_ev),GFP_ATOMIC ); -if (!vbl_ev) + vbl_work =kzalloc (sizeof (*vbl_work),GFP_ATOMIC ); +if (!vbl_work)return -ENOMEM ; - vbl_ev->crtc_id = crtc_id; - vbl_ev->enable = enable; +INIT_WORK (&vbl_work->work, vblank_ctrl_worker); -spin_lock_irqsave (&vbl_ctrl->lock, flags); -list_add_tail (&vbl_ev->node, &vbl_ctrl->event_list); -spin_unlock_irqrestore (&vbl_ctrl->lock, flags); + vbl_work->crtc_id = crtc_id; + vbl_work->enable = enable; + vbl_work->priv = priv; -schedule_work (&vbl_ctrl->work); +schedule_work (&vbl_work->work);return 0 ; }@@ -269,14 +252,13 @@ static int msm_drm_uninit(struct device *dev)struct msm_drm_private *priv = ddev->dev_private;struct msm_kms *kms = priv->kms;struct msm_mdss *mdss = priv->mdss; -struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev, *tmp;int i;/* We must cancel and cleanup any pending vblank enable/disable * work before drm_irq_uninstall() to avoid work re-enabling an * irq after uninstall has disabled it. */ +msm_gem_shrinker_cleanup (ddev);drm_kms_helper_poll_fini (ddev);drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device *dev) · line 296
Quoted 5 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2@@ -292,12 +274,6 @@ static int msm_drm_uninit(struct device *dev)#endif drm_mode_config_cleanup (ddev); -flush_work (&vbl_ctrl->work); -list_for_each_entry_safe (vbl_ev, tmp, &vbl_ctrl->event_list, node){
Quoted 8 lines
Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_init(struct device *dev,
Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c +0
-4-list_del (&vbl_ev->node); -kfree (vbl_ev); - } -/* clean up event worker threads */ for (i =0 ; i < priv->num_crtcs; i++) {if (priv->event_thread[i].thread) {@@ -469,9 +445,6 @@ static int msm_drm_init(struct device *dev,struct
Quoted 48 lines
Quoted 32 lines
drm_driver*drv)Review
Quoted 10 lines
drivers/gpu/drm/msm/msm_drv.c +0
-3priv->wq =alloc_ordered_workqueue ("msm" ,0 );INIT_LIST_HEAD (&priv->inactive_list); -INIT_LIST_HEAD (&priv->vblank_ctrl.event_list); -INIT_WORK (&priv->vblank_ctrl.work, vblank_ctrl_worker); -spin_lock_init (&priv->vblank_ctrl.lock);drm_mode_config_init (ddev);(quoted diff) +0
-0
b/drivers/gpu/drm/msm/msm_drv.hQuoted 20 lines
index 05d33a7..d4cbde2 100644
drivers/gpu/drm/msm/msm_drv.h +0
-6@@ -77,12 +77,6 @@ enum msm_mdp_plane_property { PLANE_PROP_MAX_NUM }; -struct msm_vblank_ctrl { -struct work_struct work; -struct list_head event_list; -spinlock_t lock; -}; -#define MSM_GPU_MAX_RINGS 4#define MAX_H_TILES_PER_DISPLAY 2@@ -225,7 +219,6 @@ struct msm_drm_private {struct notifier_block vmap_notifier;struct shrinker shrinker;Quoted 13 lines
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.h +0
-2-struct msm_vblank_ctrl vblank_ctrl;struct drm_atomic_state *pm_state; }; --The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
Quoted 6 lines
a Linux Foundation Collaborative Project
_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno--
Jeykumar SSignature · 1 line
Jeykumar S
Signature · 5 lines
Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ Freedreno mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [PATCH v2 5/5] drm/msm: subclass work object for vblank events
Quoted 83 lines · Sean Paul
Quoted 10 lines · Jeykumar Sankaran
Quoted 9 lines · Sean Paul
Quoted 8 lines · Jeykumar Sankaran
Quoted 7 lines · Sean Paul
Quoted 6 lines · Jeykumar Sankaran
msm maintains a separate structure to define vblank work definitions and a list to track events submitted to the workqueue. We can avoid this redundant list and its protection mechanism, if we subclass the work object to encapsulate vblank event parameters.
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
changes in v2:
- subclass optimization on system wq (Sean Paul)I wouldn't do it like this, tbh. One problem is that you've lost
your
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
flush()on
unbind, so there's no way to know if you have workers in the wild waiting
to
enable/disable vblank.Another issues is that AFAICT, we don't need a queue of enables/disables,
but
rather just the last requested state (ie: should we be on or off).So
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
things
don't need to be this complicated (and we're possibly thrashingvblank
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
on/off
for no reason).I'm still of the mind that you should just make this synchronous
and
Quoted 3 lines
Quoted 3 lines
be
Quoted 2 lines
Quoted 2 lines
done
with the threads (especially since we're stilluncovering/introducing
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
races!).
While scoping out the effort to make vblank events synchronous, I found
that the spinlock locking order of vblank request sequence andvblank
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
callback
sequences are the opposite.In DPU,
drm_vblank_enableacquiresvblank_time_lockbeforeregistering
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
the crtc to encoder which happens after acquiring
encoder_spinlock. But
thevblank_callbackacquiresencoder_spinlockbefore accessing the registered
crtc and calling intodrm_vblank_handlerwhich tries to acquirevblank_time_lock.
Acquiring bothvblank_time_lockandencoder_spinlockin the same thread
is leading to deadlock.Quoted 3 lines
Quoted 3 lines
Hmm, I'm not sure I follow. Are you seeing issues where irq overlaps with
enable/disable? I hacked in sync vblank enable/disable quickly to seeif
Quoted 14 lines
Quoted 4 lines
I
could
reproduce what you're seeing, but things seemed well behaved.The race is between drm_vblank_get/put and
vblank_handlercontexts.When made synchronous:
while calling
drm_vblank_get, the callstack looks like below:drm_vblank_get->drm_vblank_enable(acquiresvblank_time_lock) ->__enable_vblank->dpu_crtc_vblank->dpu_encoder_toggle_vblank_for_crtc(tries to acquireenc_spinlock)Quoted 4 lines
In vblank handler, the call stack will be:
dpu_encoder_phys_vid_vblank_irq->dpu_encoder_vblank_callback(acquiresenc_spinlock) ->dpu_crtc_vblank_callback->drm_handle_vblank(tries to acquirevblank_time_lock)
Quoted 5 lines
Hmm, I'm not sure how this can happen. We acquire and release the
enc_spinlock
before enabling the irq, yes we will hold on to thevbl_time_lock, but we shouldn't be trying to reacquire an encoder's spinlock after we've enabled it.
In the synchronous approach dpu_encoder_toggle_vblank_for_crtc(which acquires the enc_spinlock) will be called while we are holding the vbl_time_lock.
Quoted 4 lines
I don't know how that can deadlock, since we should never be running enable and
the handler concurrently.
I agree that vblank_irq handler should not be running before the enable sequence. But don't you expect the handler to be running while calling the vblank_disable sequence? vbl disable will try to acquire the locks in the opposite order to that of irq_handler and the same issue is bound to happen.
With your patch, you should be able to simulate this deadlock if you can inject a delay by adding a pr_err log in vblank_ctrl_queue_work
Thanks,
Jeykumar S.
Quoted 282 lines
The only thing I can think of is that the vblank interrupts are firing after
vblank has been disabled? In that case, it seems like we should properly flush
them.Sean
Quoted 11 lines
Quoted 11 lines
I do see that there is a chance to call
drm_handle_vblank()while holdingenc_spinlock, but couldn't find any obvious lock recursion there.Maybe a callstack or lockdep splat would help?
Sean
Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c static void vblank_ctrl_worker(struct
Quoted 8 lines
Here's my hack to bypass the display thread:
drivers/gpu/drm/msm/msm_drv.c +0
-0@@ -242,24 +242,19 @@ static void vblank_ctrl_worker(struct
kthread_workQuoted 25 lines
Quoted 25 lines
drivers/gpu/drm/msm/msm_drv.c +6
-8*work)static int vblank_ctrl_queue_work (struct msm_drm_private *priv,int crtc_id,bool enable) { +struct msm_kms *kms = priv->kms;struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev;unsigned long flags; - vbl_ev =kzalloc (sizeof (*vbl_ev),GFP_ATOMIC ); -if (!vbl_ev) -return -ENOMEM ; +spin_lock_irqsave (&vbl_ctrl->lock, flags); - vbl_ev->crtc_id = crtc_id; - vbl_ev->enable = enable; +if (enable) + kms->funcs->enable_vblank (kms, priv->crtcs[crtc_id]); +else + kms->funcs->disable_vblank (kms, priv->crtcs[crtc_id]); -spin_lock_irqsave (&vbl_ctrl->lock, flags); -list_add_tail (&vbl_ev->node, &vbl_ctrl->event_list);spin_unlock_irqrestore (&vbl_ctrl->lock, flags);Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c static void vblank_ctrl_worker(struct kthread_work *work) · line 259
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.c +0
-3-kthread_queue_work (&priv->disp_thread[crtc_id].worker, - &vbl_ctrl->work); -return 0 ; }Even with your patch above, I see frame is getting stuck but it recovers
in
Quoted 2 lines
a while.
The patch I tried was assigningcrtc->funcs->enable_vblank/disable_vblank so
Quoted 10 lines
that
__enable_vblankcan call crtc directly. But the above callstack is still valid for your patch.Thanks,
Jeykumar S.Quoted 4 lines
Quoted 4 lines
In MDP5, I see the same pattern between
vblank_time_lockand
list_lockQuoted 38 lines
Quoted 38 lines
which
Quoted 37 lines
is used to track the irq handlers.
I believe that explains why
msm_drvis queuing the vblank enable/disable
works to WQ after acquiringvblank_time_lock.Thanks,
Jeykumar S.Quoted 28 lines
Sean
Quoted 4 lines
Signed-off-by Jeykumar Sankaran <[email protected]>--- drivers/gpu/drm/msm/msm_drv.c | 67+++++++++++++------------------------------
Review
Quoted 4 lines
drivers/gpu/drm/msm/msm_drv.h | 7 ----- 2 files changed, 20 insertions(+), 54 deletions(-)(quoted diff) +0
-0
b/drivers/gpu/drm/msm/msm_drv.cQuoted 16 lines
index 6d6c73b..8da5be2 100644
drivers/gpu/drm/msm/msm_drv.c +3
-2@@ -203,61 +203,44 @@ u32 msm_readl(const void __iomem *addr)return val; } -struct vblank_event { -struct list_head node; +struct msm_vblank_work { +struct work_struct work;int crtc_id;bool enable; +struct msm_drm_private *priv; };Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 215
Quoted 4 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2static void vblank_ctrl_worker (struct work_struct *work) { -struct msm_vblank_ctrl *vbl_ctrl =container_of (work, - struct
msm_vblank_ctrl,Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
work);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 217
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2- struct msm_drm_private *priv = container_of(vbl_ctrl, - struct msm_drm_private,
vblank_ctrl);drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 216
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +2
-0+struct msm_vblank_work *vbl_work =container_of (work, + struct
msm_vblank_work,Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
work);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 223
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c +1
-5+struct msm_drm_private *priv = vbl_work->priv;struct msm_kms *kms = priv->kms; -struct vblank_event *vbl_ev, *tmp; -unsigned long flags; - -spin_lock_irqsave (&vbl_ctrl->lock, flags); - list_for_each_entry_safe(vbl_ev, tmp,
&vbl_ctrl->event_list, node)Quoted 53 lines
Quoted 53 lines
Quoted 53 lines
Quoted 53 lines
{
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 229
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.c +0
-6-list_del (&vbl_ev->node); -spin_unlock_irqrestore (&vbl_ctrl->lock, flags); - -if (vbl_ev->enable) - kms->funcs->enable_vblank(kms, -priv->crtcs[vbl_ev->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 232
Quoted 3 lines
drivers/gpu/drm/msm/msm_drv.c +0
-3-else - kms->funcs->disable_vblank(kms, -priv->crtcs[vbl_ev->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 220
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c +2
-4-kfree (vbl_ev); - -spin_lock_irqsave (&vbl_ctrl->lock, flags); - } +if (vbl_work->enable) + kms->funcs->enable_vblank(kms,priv->crtcs[vbl_work->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 222
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +2
-0+else + kms->funcs->disable_vblank(kms,priv->crtcs[vbl_work->crtc_id]);
Quoted 30 lines
drivers/gpu/drm/msm/msm_drv.c +8
-11-spin_unlock_irqrestore (&vbl_ctrl->lock, flags); +kfree (vbl_work); }static int vblank_ctrl_queue_work (struct msm_drm_private *priv,int crtc_id,bool enable) { -struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev; -unsigned long flags; +struct msm_vblank_work *vbl_work; - vbl_ev =kzalloc (sizeof (*vbl_ev),GFP_ATOMIC ); -if (!vbl_ev) + vbl_work =kzalloc (sizeof (*vbl_work),GFP_ATOMIC ); +if (!vbl_work)return -ENOMEM ; - vbl_ev->crtc_id = crtc_id; - vbl_ev->enable = enable; +INIT_WORK (&vbl_work->work, vblank_ctrl_worker); -spin_lock_irqsave (&vbl_ctrl->lock, flags); -list_add_tail (&vbl_ev->node, &vbl_ctrl->event_list); -spin_unlock_irqrestore (&vbl_ctrl->lock, flags); + vbl_work->crtc_id = crtc_id; + vbl_work->enable = enable; + vbl_work->priv = priv;Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.c +1
-1-schedule_work (&vbl_ctrl->work); +schedule_work (&vbl_work->work);return 0 ; }@@ -269,14 +252,13 @@ static int msm_drm_uninit(struct device*dev)
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device · line 257
Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2struct msm_drm_private *priv = ddev->dev_private;struct msm_kms *kms = priv->kms;struct msm_mdss *mdss = priv->mdss; -struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev, *tmp;int i;/* We must cancel and cleanup any pending vblank enable/disable
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device · line 258
Quoted 1 line
drivers/gpu/drm/msm/msm_drv.c +0
-0* work before drm_irq_uninstall() to avoid work re-enabling an
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c +1
-0* irq after uninstall has disabled it. */ +msm_gem_shrinker_cleanup (ddev);drm_kms_helper_poll_fini (ddev);@@ -292,12 +274,6 @@ static int msm_drm_uninit(struct device*dev)
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device · line 296
Quoted 5 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2#endif drm_mode_config_cleanup (ddev); -flush_work (&vbl_ctrl->work); - list_for_each_entry_safe(vbl_ev, tmp,
&vbl_ctrl->event_list, node)Quoted 42 lines
Quoted 42 lines
Quoted 9 lines
Quoted 9 lines
{
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_init(struct device *dev,
Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c +0
-4-list_del (&vbl_ev->node); -kfree (vbl_ev); - } -/* clean up event worker threads */ for (i =0 ; i < priv->num_crtcs; i++) {if (priv->event_thread[i].thread) {@@ -469,9 +445,6 @@ static int msm_drm_init(struct device *dev,struct
Quoted 32 lines
Quoted 32 lines
drm_driver*drv)Review
Quoted 10 lines
drivers/gpu/drm/msm/msm_drv.c +0
-3priv->wq =alloc_ordered_workqueue ("msm" ,0 );INIT_LIST_HEAD (&priv->inactive_list); -INIT_LIST_HEAD (&priv->vblank_ctrl.event_list); -INIT_WORK (&priv->vblank_ctrl.work, vblank_ctrl_worker); -spin_lock_init (&priv->vblank_ctrl.lock);drm_mode_config_init (ddev);(quoted diff) +0
-0
b/drivers/gpu/drm/msm/msm_drv.hQuoted 20 lines
index 05d33a7..d4cbde2 100644
drivers/gpu/drm/msm/msm_drv.h +0
-6@@ -77,12 +77,6 @@ enum msm_mdp_plane_property { PLANE_PROP_MAX_NUM }; -struct msm_vblank_ctrl { -struct work_struct work; -struct list_head event_list; -spinlock_t lock; -}; -#define MSM_GPU_MAX_RINGS 4#define MAX_H_TILES_PER_DISPLAY 2@@ -225,7 +219,6 @@ struct msm_drm_private {struct notifier_block vmap_notifier;struct shrinker shrinker;Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.h +0
-2-struct msm_vblank_ctrl vblank_ctrl;struct drm_atomic_state *pm_state; }; --The Qualcomm Innovation Center, Inc. is a member of the Code
Aurora
Quoted 13 lines
Quoted 10 lines
Quoted 10 lines
Quoted 7 lines
Forum,
Quoted 6 lines
a Linux Foundation Collaborative Project
_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno--
Jeykumar S--
Jeykumar S
Signature · 5 lines
Jeykumar S _______________________________________________ Freedreno mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [PATCH v2 5/5] drm/msm: subclass work object for vblank events
In reply to a message not in the archiveQuoted 98 lines · Jeykumar Sankaran
Quoted 86 lines · Sean Paul
Quoted 10 lines · Jeykumar Sankaran
Quoted 9 lines · Sean Paul
Quoted 8 lines · Jeykumar Sankaran
Quoted 7 lines · Sean Paul
Quoted 6 lines · Jeykumar Sankaran
msm maintains a separate structure to define vblank work definitions and a list to track events submitted to the workqueue. We can avoid this redundant list and its protection mechanism, if we subclass the work object to encapsulate vblank event parameters.
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
changes in v2:
- subclass optimization on system wq (Sean Paul)I wouldn't do it like this, tbh. One problem is that you've lost
your
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
flush()on
unbind, so there's no way to know if you have workers in the wild waiting
to
enable/disable vblank.Another issues is that AFAICT, we don't need a queue of enables/disables,
but
rather just the last requested state (ie: should we be on or off).So
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
things
don't need to be this complicated (and we're possibly thrashingvblank
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
on/off
for no reason).I'm still of the mind that you should just make this synchronous
and
Quoted 3 lines
Quoted 3 lines
be
Quoted 2 lines
Quoted 2 lines
done
with the threads (especially since we're stilluncovering/introducing
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
races!).
While scoping out the effort to make vblank events synchronous, I found
that the spinlock locking order of vblank request sequence andvblank
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
callback
sequences are the opposite.In DPU,
drm_vblank_enableacquiresvblank_time_lockbeforeregistering
Quoted 10 lines
Quoted 10 lines
Quoted 9 lines
the crtc to encoder which happens after acquiring
encoder_spinlock. But
thevblank_callbackacquiresencoder_spinlockbefore accessing the registered
crtc and calling intodrm_vblank_handlerwhich tries to acquirevblank_time_lock.
Acquiring bothvblank_time_lockandencoder_spinlockin the same thread
is leading to deadlock.Quoted 3 lines
Quoted 3 lines
Hmm, I'm not sure I follow. Are you seeing issues where irq overlaps with
enable/disable? I hacked in sync vblank enable/disable quickly to seeif
Quoted 15 lines
Quoted 4 lines
I
could
reproduce what you're seeing, but things seemed well behaved.The race is between drm_vblank_get/put and
vblank_handlercontexts.When made synchronous:
while calling
drm_vblank_get, the callstack looks like below:drm_vblank_get->drm_vblank_enable(acquiresvblank_time_lock) ->__enable_vblank->dpu_crtc_vblank->dpu_encoder_toggle_vblank_for_crtc
(tries to acquireenc_spinlock)Quoted 6 lines
In vblank handler, the call stack will be:
dpu_encoder_phys_vid_vblank_irq->dpu_encoder_vblank_callback(acquiresenc_spinlock) ->dpu_crtc_vblank_callback->drm_handle_vblank(tries to
acquirevblank_time_lock)Quoted 7 lines
Hmm, I'm not sure how this can happen. We acquire and release the
enc_spinlock
before enabling the irq, yes we will hold on to thevbl_time_lock, but we
shouldn't be trying to reacquire an encoder's spinlock after we've enabled
it.In the synchronous approach
dpu_encoder_toggle_vblank_for_crtc(which acquires the enc_spinlock)will be called while we
are holding thevbl_time_lock.
Quoted 8 lines
Quoted 4 lines
I don't know how that can deadlock, since we should never be running enable and
the handler concurrently.I agree that
vblank_irqhandler should not be running before the enable sequence. But
don't you expect the handler to be running while calling thevblank_disablesequence?
This is an entirely different problem though. It's also one that is easier to fix. I think we could probably grab the enc_spinlock in disable and clear the crtc pointer.
What I'm getting at is that there's no fundamental reason why we need to have async vblank enable/disable.
Sean
Quoted 298 lines
vbl disable will try to acquire the locks in the opposite order to that of
irq_handlerand the
same issue is bound to happen.With your patch, you should be able to simulate this deadlock if you can inject a delay
by adding apr_errlog invblank_ctrl_queue_workThanks,
Jeykumar S.Quoted 284 lines
The only thing I can think of is that the vblank interrupts are firing after
vblank has been disabled? In that case, it seems like we should properly flush
them.Sean
Quoted 11 lines
Quoted 11 lines
I do see that there is a chance to call
drm_handle_vblank()while holdingenc_spinlock, but couldn't find any obvious lock recursion there.Maybe a callstack or lockdep splat would help?
Sean
Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c static void vblank_ctrl_worker(struct
Quoted 8 lines
Here's my hack to bypass the display thread:
drivers/gpu/drm/msm/msm_drv.c +0
-0@@ -242,24 +242,19 @@ static void vblank_ctrl_worker(struct
kthread_workQuoted 25 lines
Quoted 25 lines
drivers/gpu/drm/msm/msm_drv.c +6
-8*work)static int vblank_ctrl_queue_work (struct msm_drm_private *priv,int crtc_id,bool enable) { +struct msm_kms *kms = priv->kms;struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev;unsigned long flags; - vbl_ev =kzalloc (sizeof (*vbl_ev),GFP_ATOMIC ); -if (!vbl_ev) -return -ENOMEM ; +spin_lock_irqsave (&vbl_ctrl->lock, flags); - vbl_ev->crtc_id = crtc_id; - vbl_ev->enable = enable; +if (enable) + kms->funcs->enable_vblank (kms, priv->crtcs[crtc_id]); +else + kms->funcs->disable_vblank (kms, priv->crtcs[crtc_id]); -spin_lock_irqsave (&vbl_ctrl->lock, flags); -list_add_tail (&vbl_ev->node, &vbl_ctrl->event_list);spin_unlock_irqrestore (&vbl_ctrl->lock, flags);Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c static void vblank_ctrl_worker(struct kthread_work *work) · line 259
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.c +0
-3-kthread_queue_work (&priv->disp_thread[crtc_id].worker, - &vbl_ctrl->work); -return 0 ; }Even with your patch above, I see frame is getting stuck but it recovers
in
Quoted 2 lines
a while.
The patch I tried was assigningcrtc->funcs->enable_vblank/disable_vblank so
Quoted 11 lines
that
__enable_vblankcan call crtc directly. But the above callstack is still
valid for your patch.Thanks,
Jeykumar S.Quoted 4 lines
Quoted 4 lines
In MDP5, I see the same pattern between
vblank_time_lockand
list_lockQuoted 38 lines
Quoted 38 lines
which
Quoted 37 lines
is used to track the irq handlers.
I believe that explains why
msm_drvis queuing the vblank enable/disable
works to WQ after acquiringvblank_time_lock.Thanks,
Jeykumar S.Quoted 28 lines
Sean
Quoted 4 lines
Signed-off-by Jeykumar Sankaran <[email protected]>--- drivers/gpu/drm/msm/msm_drv.c | 67+++++++++++++------------------------------
Review
Quoted 4 lines
drivers/gpu/drm/msm/msm_drv.h | 7 ----- 2 files changed, 20 insertions(+), 54 deletions(-)(quoted diff) +0
-0
b/drivers/gpu/drm/msm/msm_drv.cQuoted 16 lines
index 6d6c73b..8da5be2 100644
drivers/gpu/drm/msm/msm_drv.c +3
-2@@ -203,61 +203,44 @@ u32 msm_readl(const void __iomem *addr)return val; } -struct vblank_event { -struct list_head node; +struct msm_vblank_work { +struct work_struct work;int crtc_id;bool enable; +struct msm_drm_private *priv; };Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 215
Quoted 4 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2static void vblank_ctrl_worker (struct work_struct *work) { -struct msm_vblank_ctrl *vbl_ctrl =container_of (work, - struct
msm_vblank_ctrl,Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
work);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 217
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2- struct msm_drm_private *priv = container_of(vbl_ctrl, - struct msm_drm_private,
vblank_ctrl);drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 216
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +2
-0+struct msm_vblank_work *vbl_work =container_of (work, + struct
msm_vblank_work,Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
work);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 223
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c +1
-5+struct msm_drm_private *priv = vbl_work->priv;struct msm_kms *kms = priv->kms; -struct vblank_event *vbl_ev, *tmp; -unsigned long flags; - -spin_lock_irqsave (&vbl_ctrl->lock, flags); - list_for_each_entry_safe(vbl_ev, tmp,
&vbl_ctrl->event_list, node)Quoted 53 lines
Quoted 53 lines
Quoted 53 lines
Quoted 53 lines
{
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 229
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.c +0
-6-list_del (&vbl_ev->node); -spin_unlock_irqrestore (&vbl_ctrl->lock, flags); - -if (vbl_ev->enable) - kms->funcs->enable_vblank(kms, -priv->crtcs[vbl_ev->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 232
Quoted 3 lines
drivers/gpu/drm/msm/msm_drv.c +0
-3-else - kms->funcs->disable_vblank(kms, -priv->crtcs[vbl_ev->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 220
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c +2
-4-kfree (vbl_ev); - -spin_lock_irqsave (&vbl_ctrl->lock, flags); - } +if (vbl_work->enable) + kms->funcs->enable_vblank(kms,priv->crtcs[vbl_work->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 222
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +2
-0+else + kms->funcs->disable_vblank(kms,priv->crtcs[vbl_work->crtc_id]);
Quoted 30 lines
drivers/gpu/drm/msm/msm_drv.c +8
-11-spin_unlock_irqrestore (&vbl_ctrl->lock, flags); +kfree (vbl_work); }static int vblank_ctrl_queue_work (struct msm_drm_private *priv,int crtc_id,bool enable) { -struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev; -unsigned long flags; +struct msm_vblank_work *vbl_work; - vbl_ev =kzalloc (sizeof (*vbl_ev),GFP_ATOMIC ); -if (!vbl_ev) + vbl_work =kzalloc (sizeof (*vbl_work),GFP_ATOMIC ); +if (!vbl_work)return -ENOMEM ; - vbl_ev->crtc_id = crtc_id; - vbl_ev->enable = enable; +INIT_WORK (&vbl_work->work, vblank_ctrl_worker); -spin_lock_irqsave (&vbl_ctrl->lock, flags); -list_add_tail (&vbl_ev->node, &vbl_ctrl->event_list); -spin_unlock_irqrestore (&vbl_ctrl->lock, flags); + vbl_work->crtc_id = crtc_id; + vbl_work->enable = enable; + vbl_work->priv = priv;Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.c +1
-1-schedule_work (&vbl_ctrl->work); +schedule_work (&vbl_work->work);return 0 ; }@@ -269,14 +252,13 @@ static int msm_drm_uninit(struct device*dev)
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device · line 257
Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2struct msm_drm_private *priv = ddev->dev_private;struct msm_kms *kms = priv->kms;struct msm_mdss *mdss = priv->mdss; -struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev, *tmp;int i;/* We must cancel and cleanup any pending vblank enable/disable
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device · line 258
Quoted 1 line
drivers/gpu/drm/msm/msm_drv.c +0
-0* work before drm_irq_uninstall() to avoid work re-enabling an
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c +1
-0* irq after uninstall has disabled it. */ +msm_gem_shrinker_cleanup (ddev);drm_kms_helper_poll_fini (ddev);@@ -292,12 +274,6 @@ static int msm_drm_uninit(struct device*dev)
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device · line 296
Quoted 5 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2#endif drm_mode_config_cleanup (ddev); -flush_work (&vbl_ctrl->work); - list_for_each_entry_safe(vbl_ev, tmp,
&vbl_ctrl->event_list, node)Quoted 42 lines
Quoted 42 lines
Quoted 9 lines
Quoted 9 lines
{
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_init(struct device *dev,
Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c +0
-4-list_del (&vbl_ev->node); -kfree (vbl_ev); - } -/* clean up event worker threads */ for (i =0 ; i < priv->num_crtcs; i++) {if (priv->event_thread[i].thread) {@@ -469,9 +445,6 @@ static int msm_drm_init(struct device *dev,struct
Quoted 32 lines
Quoted 32 lines
drm_driver*drv)Review
Quoted 10 lines
drivers/gpu/drm/msm/msm_drv.c +0
-3priv->wq =alloc_ordered_workqueue ("msm" ,0 );INIT_LIST_HEAD (&priv->inactive_list); -INIT_LIST_HEAD (&priv->vblank_ctrl.event_list); -INIT_WORK (&priv->vblank_ctrl.work, vblank_ctrl_worker); -spin_lock_init (&priv->vblank_ctrl.lock);drm_mode_config_init (ddev);(quoted diff) +0
-0
b/drivers/gpu/drm/msm/msm_drv.hQuoted 20 lines
index 05d33a7..d4cbde2 100644
drivers/gpu/drm/msm/msm_drv.h +0
-6@@ -77,12 +77,6 @@ enum msm_mdp_plane_property { PLANE_PROP_MAX_NUM }; -struct msm_vblank_ctrl { -struct work_struct work; -struct list_head event_list; -spinlock_t lock; -}; -#define MSM_GPU_MAX_RINGS 4#define MAX_H_TILES_PER_DISPLAY 2@@ -225,7 +219,6 @@ struct msm_drm_private {struct notifier_block vmap_notifier;struct shrinker shrinker;Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.h +0
-2-struct msm_vblank_ctrl vblank_ctrl;struct drm_atomic_state *pm_state; }; --The Qualcomm Innovation Center, Inc. is a member of the Code
Aurora
Quoted 13 lines
Quoted 10 lines
Quoted 10 lines
Quoted 7 lines
Forum,
Quoted 6 lines
a Linux Foundation Collaborative Project
_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno--
Jeykumar S--
Jeykumar SSignature · 1 line
Jeykumar S
Signature · 5 lines
Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ Freedreno mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [PATCH v2 5/5] drm/msm: subclass work object for vblank events
Quoted 117 lines · Sean Paul
Quoted 6 lines · Jeykumar Sankaran
Quoted 5 lines · Sean Paul
Quoted 4 lines · Jeykumar Sankaran
Quoted 3 lines · Sean Paul
Quoted 2 lines · Jeykumar Sankaran
Quoted 1 line · Sean Paul
On Tue, Nov 06, 2018 at 02:36:30PM -0800, Jeykumar Sankaran
Quoted 10 lines · wrote:
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 8 lines
msm maintains a separate structure to define vblank work definitions and a list to track events submitted to the workqueue. We can avoid this redundant list and its protection mechanism, if we subclass the work object to encapsulate vblank event parameters.
changes in v2:
- subclass optimization on system wq (Sean Paul)I wouldn't do it like this, tbh. One problem is that you've
lost
Quoted 3 lines
Quoted 3 lines
your
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
flush()on
unbind, so there's no way to know if you have workers in thewild
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
waiting
to
enable/disable vblank.Another issues is that AFAICT, we don't need a queue of enables/disables,
but
rather just the last requested state (ie: should we be on oroff).
Quoted 3 lines
Quoted 3 lines
So
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
things
don't need to be this complicated (and we're possiblythrashing
Quoted 5 lines
Quoted 5 lines
vblank
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
on/off
for no reason).I'm still of the mind that you should just make this
synchronous
Quoted 8 lines
Quoted 8 lines
and
Quoted 3 lines
Quoted 3 lines
be
Quoted 2 lines
Quoted 2 lines
done
with the threads (especially since we're stilluncovering/introducing
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 2 lines
races!).
While scoping out the effort to make vblank events synchronous,
I
Quoted 9 lines
Quoted 9 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
found
that the spinlock locking order of vblank request sequence andvblank
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
callback
sequences are the opposite.In DPU,
drm_vblank_enableacquiresvblank_time_lockbeforeregistering
Quoted 1 line
Quoted 1 line
Quoted 1 line
the crtc to encoder which happens after acquiring
encoder_spinlock.Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
But
thevblank_callbackacquiresencoder_spinlockbefore accessingthe
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
registered
crtc and calling intodrm_vblank_handlerwhich tries to acquirevblank_time_lock.
Acquiring bothvblank_time_lockandencoder_spinlockin the same thread
is leading to deadlock.Hmm, I'm not sure I follow. Are you seeing issues where irq
overlaps
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
with
enable/disable? I hacked in sync vblank enable/disable quickly tosee
Quoted 34 lines
Quoted 23 lines
if
Quoted 15 lines
Quoted 4 lines
I
could
reproduce what you're seeing, but things seemed well behaved.The race is between drm_vblank_get/put and
vblank_handlercontexts.When made synchronous:
while calling
drm_vblank_get, the callstack looks like below:drm_vblank_get->drm_vblank_enable(acquiresvblank_time_lock) ->__enable_vblank->dpu_crtc_vblank->dpu_encoder_toggle_vblank_for_crtc
(tries to acquireenc_spinlock)Quoted 6 lines
In vblank handler, the call stack will be:
dpu_encoder_phys_vid_vblank_irq->dpu_encoder_vblank_callback(acquiresenc_spinlock) ->dpu_crtc_vblank_callback->drm_handle_vblank(tries to
acquirevblank_time_lock)Quoted 7 lines
Hmm, I'm not sure how this can happen. We acquire and release the
enc_spinlock
before enabling the irq, yes we will hold on to thevbl_time_lock, but we
shouldn't be trying to reacquire an encoder's spinlock after we've enabled
it.In the synchronous approach
dpu_encoder_toggle_vblank_for_crtc(which acquires the enc_spinlock)will be called while we
are holding thevbl_time_lock.Quoted 7 lines
Quoted 4 lines
I don't know how that can deadlock, since we should never be running enable and
the handler concurrently.I agree that
vblank_irqhandler should not be running before the enable sequence. But
don't you expect the handler to be running while calling the
Quoted 9 lines
vblank_disableQuoted 1 line
sequence?
This is an entirely different problem though. It's also one that is easier to
fix. I think we could probably grab theenc_spinlockin disable and clear the
crtc pointer.
we do hold enc_spinlock in dpu_encoder_assign_crtc (drm/msm: dpu: Remove vblank_callback from encoder) where we clear the crtc pointer.
Quoted 6 lines
What I'm getting at is that there's no fundamental reason why we need to have
async vblank enable/disable.Sean
There is really no *need* to have them async. But I believe the reason why they are implemented this way is to avoid deadlock between the below two paths.
Restating the above findings:vblank_handlers and vblank enable/disable can run concurrently. The first trying to acquire vbl_time_lock holding enc_spinlock. Other trying to acquire enc_spinlock holding vbl_time_lock.
Thanks,
Jeykumar S.
Quoted 306 lines
Quoted 1 line
vbl disable will try to acquire the locks in the opposite order to that
of
Quoted 13 lines
irq_handlerand the
same issue is bound to happen.With your patch, you should be able to simulate this deadlock if you can inject a delay
by adding apr_errlog invblank_ctrl_queue_workThanks,
Jeykumar S.Quoted 3 lines
The only thing I can think of is that the vblank interrupts are firing after
vblank has been disabled? In that case, it seems like we shouldproperly
Quoted 40 lines
Quoted 40 lines
flush
them.Sean
Quoted 11 lines
Quoted 11 lines
I do see that there is a chance to call
drm_handle_vblank()while holdingenc_spinlock, but couldn't find any obvious lock recursion there.Maybe a callstack or lockdep splat would help?
Sean
Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c static void vblank_ctrl_worker(struct
Quoted 8 lines
Here's my hack to bypass the display thread:
drivers/gpu/drm/msm/msm_drv.c +0
-0@@ -242,24 +242,19 @@ static void vblank_ctrl_worker(struct
kthread_workQuoted 14 lines
Quoted 14 lines
drivers/gpu/drm/msm/msm_drv.c +2
-4*work)static int vblank_ctrl_queue_work (struct msm_drm_private *priv,int crtc_id,bool enable) { +struct msm_kms *kms = priv->kms;struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev;unsigned long flags; - vbl_ev =kzalloc (sizeof (*vbl_ev),GFP_ATOMIC ); -if (!vbl_ev) -return -ENOMEM ; +spin_lock_irqsave (&vbl_ctrl->lock, flags);Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/gpu/drm/msm/msm_drv.c static void vblank_ctrl_worker(struct · line 253
Quoted 4 lines
drivers/gpu/drm/msm/msm_drv.c +2
-2- vbl_ev->crtc_id = crtc_id; - vbl_ev->enable = enable; +if (enable) + kms->funcs->enable_vblank(kms,priv->crtcs[crtc_id]);
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c static void vblank_ctrl_worker(struct · line 255
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +2
-0+else + kms->funcs->disable_vblank(kms,priv->crtcs[crtc_id]);
Quoted 49 lines
Quoted 49 lines
Quoted 5 lines
Quoted 5 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2-spin_lock_irqsave (&vbl_ctrl->lock, flags); -list_add_tail (&vbl_ev->node, &vbl_ctrl->event_list);spin_unlock_irqrestore (&vbl_ctrl->lock, flags);Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c static void vblank_ctrl_worker(struct · line 260
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.c +0
-3-kthread_queue_work (&priv->disp_thread[crtc_id].worker, - &vbl_ctrl->work); -return 0 ; }Even with your patch above, I see frame is getting stuck but it recovers
in
Quoted 2 lines
a while.
The patch I tried was assigningcrtc->funcs->enable_vblank/disable_vblank so
Quoted 11 lines
that
__enable_vblankcan call crtc directly. But the above callstack is still
valid for your patch.Thanks,
Jeykumar S.Quoted 4 lines
Quoted 4 lines
In MDP5, I see the same pattern between
vblank_time_lockand
list_lockQuoted 20 lines
Quoted 20 lines
which
Quoted 19 lines
is used to track the irq handlers.
I believe that explains why
msm_drvis queuing the vblank enable/disable
works to WQ after acquiringvblank_time_lock.Thanks,
Jeykumar S.Quoted 10 lines
Sean
Quoted 4 lines
Signed-off-by Jeykumar Sankaran <[email protected]>--- drivers/gpu/drm/msm/msm_drv.c | 67+++++++++++++------------------------------
Quoted 3 lines
drivers/gpu/drm/msm/msm_drv.h | 7 ----- 2 files changed, 20 insertions(+), 54 deletions(-)Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Review
Quoted 1 line
(quoted diff) +0
-0
b/drivers/gpu/drm/msm/msm_drv.cdrivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem
Quoted 4 lines
index 6d6c73b..8da5be2 100644
drivers/gpu/drm/msm/msm_drv.c +0
-0@@ -203,61 +203,44 @@ u32 msm_readl(const void __iomem*addr)
Quoted 57 lines
Quoted 57 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
drivers/gpu/drm/msm/msm_drv.c +3
-2return val; } -struct vblank_event { -struct list_head node; +struct msm_vblank_work { +struct work_struct work;int crtc_id;bool enable; +struct msm_drm_private *priv; };Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem · line 215
Quoted 4 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2static void vblank_ctrl_worker (struct work_struct *work) { -struct msm_vblank_ctrl *vbl_ctrl =container_of (work, - struct
msm_vblank_ctrl,Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
work);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem · line 217
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2- struct msm_drm_private *priv = container_of(vbl_ctrl, - struct msm_drm_private,
vblank_ctrl);drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem · line 216
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +2
-0+struct msm_vblank_work *vbl_work =container_of (work, + struct
msm_vblank_work,Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
work);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem · line 223
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c +1
-5+struct msm_drm_private *priv = vbl_work->priv;struct msm_kms *kms = priv->kms; -struct vblank_event *vbl_ev, *tmp; -unsigned long flags; - -spin_lock_irqsave (&vbl_ctrl->lock, flags); - list_for_each_entry_safe(vbl_ev, tmp,
&vbl_ctrl->event_list, node)Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
{
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem · line 229
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.c +0
-6-list_del (&vbl_ev->node); -spin_unlock_irqrestore (&vbl_ctrl->lock, flags); - -if (vbl_ev->enable) - kms->funcs->enable_vblank(kms, -priv->crtcs[vbl_ev->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem · line 232
Quoted 3 lines
drivers/gpu/drm/msm/msm_drv.c +0
-3-else - kms->funcs->disable_vblank(kms, -priv->crtcs[vbl_ev->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem · line 220
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c +2
-4-kfree (vbl_ev); - -spin_lock_irqsave (&vbl_ctrl->lock, flags); - } +if (vbl_work->enable) + kms->funcs->enable_vblank(kms,priv->crtcs[vbl_work->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem · line 222
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +2
-0+else + kms->funcs->disable_vblank(kms,priv->crtcs[vbl_work->crtc_id]);
Quoted 1 line
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/gpu/drm/msm/msm_drv.c
Quoted 5 lines
drivers/gpu/drm/msm/msm_drv.c +1
-1-spin_unlock_irqrestore (&vbl_ctrl->lock, flags); +kfree (vbl_work); } static int vblank_ctrl_queue_work(struct msm_drm_private*priv,
Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
drivers/gpu/drm/msm/msm_drv.c +7
-10int crtc_id, bool enable) { -struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev; -unsigned long flags; +struct msm_vblank_work *vbl_work; - vbl_ev =kzalloc (sizeof (*vbl_ev),GFP_ATOMIC ); -if (!vbl_ev) + vbl_work =kzalloc (sizeof (*vbl_work),GFP_ATOMIC ); +if (!vbl_work)return -ENOMEM ; - vbl_ev->crtc_id = crtc_id; - vbl_ev->enable = enable; +INIT_WORK (&vbl_work->work, vblank_ctrl_worker); -spin_lock_irqsave (&vbl_ctrl->lock, flags); -list_add_tail (&vbl_ev->node, &vbl_ctrl->event_list); -spin_unlock_irqrestore (&vbl_ctrl->lock, flags); + vbl_work->crtc_id = crtc_id; + vbl_work->enable = enable; + vbl_work->priv = priv;Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.c +1
-1-schedule_work (&vbl_ctrl->work); +schedule_work (&vbl_work->work);return 0 ; }@@ -269,14 +252,13 @@ static int msm_drm_uninit(structdevice
Quoted 27 lines
Quoted 27 lines
*dev)
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct · line 257
Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2struct msm_drm_private *priv = ddev->dev_private;struct msm_kms *kms = priv->kms;struct msm_mdss *mdss = priv->mdss; -struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev, *tmp;int i;/* We must cancel and cleanup any pending vblank enable/disable
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct · line 258
Quoted 1 line
drivers/gpu/drm/msm/msm_drv.c +0
-0* work before drm_irq_uninstall() to avoid work re-enabling an
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c +1
-0* irq after uninstall has disabled it. */ +msm_gem_shrinker_cleanup (ddev);drm_kms_helper_poll_fini (ddev);@@ -292,12 +274,6 @@ static int msm_drm_uninit(struct device*dev)
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device · line 296
Quoted 5 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2#endif drm_mode_config_cleanup (ddev); -flush_work (&vbl_ctrl->work); - list_for_each_entry_safe(vbl_ev, tmp,
&vbl_ctrl->event_list, node)Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
{
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_init(struct device
Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c +0
-4-list_del (&vbl_ev->node); -kfree (vbl_ev); - } -/* clean up event worker threads */ for (i =0 ; i < priv->num_crtcs; i++) {if (priv->event_thread[i].thread) {@@ -469,9 +445,6 @@ static int msm_drm_init(struct device*dev,
Quoted 56 lines
Quoted 33 lines
Quoted 33 lines
Quoted 33 lines
struct
Quoted 32 lines
Quoted 32 lines
drm_driver*drv)Review
Quoted 10 lines
drivers/gpu/drm/msm/msm_drv.c +0
-3priv->wq =alloc_ordered_workqueue ("msm" ,0 );INIT_LIST_HEAD (&priv->inactive_list); -INIT_LIST_HEAD (&priv->vblank_ctrl.event_list); -INIT_WORK (&priv->vblank_ctrl.work, vblank_ctrl_worker); -spin_lock_init (&priv->vblank_ctrl.lock);drm_mode_config_init (ddev);(quoted diff) +0
-0
b/drivers/gpu/drm/msm/msm_drv.hQuoted 20 lines
index 05d33a7..d4cbde2 100644
drivers/gpu/drm/msm/msm_drv.h +0
-6@@ -77,12 +77,6 @@ enum msm_mdp_plane_property { PLANE_PROP_MAX_NUM }; -struct msm_vblank_ctrl { -struct work_struct work; -struct list_head event_list; -spinlock_t lock; -}; -#define MSM_GPU_MAX_RINGS 4#define MAX_H_TILES_PER_DISPLAY 2@@ -225,7 +219,6 @@ struct msm_drm_private {struct notifier_block vmap_notifier;struct shrinker shrinker;Quoted 20 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.h +0
-2-struct msm_vblank_ctrl vblank_ctrl;struct drm_atomic_state *pm_state; }; --The Qualcomm Innovation Center, Inc. is a member of the Code
Aurora
Quoted 13 lines
Quoted 10 lines
Quoted 10 lines
Quoted 7 lines
Forum,
Quoted 6 lines
a Linux Foundation Collaborative Project
_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno--
Jeykumar S--
Jeykumar S--
Jeykumar S
Signature · 5 lines
Jeykumar S _______________________________________________ Freedreno mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [PATCH v2 5/5] drm/msm: subclass work object for vblank events
In reply to Sean PaulQuoted 7 lines · Sean Paul
Quoted 6 lines · Jeykumar Sankaran
msm maintains a separate structure to define vblank work definitions and a list to track events submitted to the workqueue. We can avoid this redundant list and its protection mechanism, if we subclass the work object to encapsulate vblank event parameters.
Quoted 9 lines
Quoted 2 lines
changes in v2:
- subclass optimization on system wq (Sean Paul)I wouldn't do it like this, tbh. One problem is that you've lost your
flush()on
unbind, so there's no way to know if you have workers in the wild waiting to
enable/disable vblank.
I believe I didnt respond to this quesition of yours. This patch is removing the flush on the work. But flush_workqueue before destroying the WQ will make sure all the queued work jobs are run to completion.
To honor the comment, I will fix the patch to move the flush/destroy wq before the irq_unininstall.
Thanks,
Jeykumar S.
Quoted 193 lines
Another issues is that AFAICT, we don't need a queue of enables/disables, but
rather just the last requested state (ie: should we be on or off). So things
don't need to be this complicated (and we're possibly thrashing vblank on/off
for no reason).I'm still of the mind that you should just make this synchronous and be done
with the threads (especially since we're still uncovering/introducing races!).Sean
Quoted 4 lines
Signed-off-by Jeykumar Sankaran <[email protected]>--- drivers/gpu/drm/msm/msm_drv.c | 67+++++++++++++------------------------------
Review
Quoted 4 lines
drivers/gpu/drm/msm/msm_drv.h | 7 ----- 2 files changed, 20 insertions(+), 54 deletions(-)(quoted diff) +0
-0
b/drivers/gpu/drm/msm/msm_drv.cQuoted 10 lines
index 6d6c73b..8da5be2 100644
drivers/gpu/drm/msm/msm_drv.c +1
-2@@ -203,61 +203,44 @@ u32 msm_readl(const void __iomem *addr)return val; } -struct vblank_event { -struct list_head node; +struct msm_vblank_work {drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 215
Quoted 10 lines
drivers/gpu/drm/msm/msm_drv.c +2
-2+struct work_struct work;int crtc_id;bool enable; +struct msm_drm_private *priv; };static void vblank_ctrl_worker (struct work_struct *work) { - struct msm_vblank_ctrl *vbl_ctrl = container_of(work, - struct msm_vblank_ctrl,work);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 217
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2- struct msm_drm_private *priv = container_of(vbl_ctrl, - struct msm_drm_private,
vblank_ctrl);drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 216
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +2
-0+ struct msm_vblank_work *vbl_work = container_of(work, + struct msm_vblank_work,work);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr)
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c +1
-5+struct msm_drm_private *priv = vbl_work->priv;struct msm_kms *kms = priv->kms; -struct vblank_event *vbl_ev, *tmp; -unsigned long flags; - -spin_lock_irqsave (&vbl_ctrl->lock, flags); -list_for_each_entry_safe (vbl_ev, tmp, &vbl_ctrl->event_list, node){
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 235
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.c +0
-6-list_del (&vbl_ev->node); -spin_unlock_irqrestore (&vbl_ctrl->lock, flags); - -if (vbl_ev->enable) - kms->funcs->enable_vblank(kms, -priv->crtcs[vbl_ev->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 235
Quoted 3 lines
drivers/gpu/drm/msm/msm_drv.c +0
-3-else - kms->funcs->disable_vblank(kms, -priv->crtcs[vbl_ev->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr)
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c +2
-4-kfree (vbl_ev); - -spin_lock_irqsave (&vbl_ctrl->lock, flags); - } +if (vbl_work->enable) + kms->funcs->enable_vblank(kms,priv->crtcs[vbl_work->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 222
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +2
-0+else + kms->funcs->disable_vblank(kms,priv->crtcs[vbl_work->crtc_id]);
Quoted 52 lines
drivers/gpu/drm/msm/msm_drv.c +10
-14-spin_unlock_irqrestore (&vbl_ctrl->lock, flags); +kfree (vbl_work); }static int vblank_ctrl_queue_work (struct msm_drm_private *priv,int crtc_id,bool enable) { -struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev; -unsigned long flags; +struct msm_vblank_work *vbl_work; - vbl_ev =kzalloc (sizeof (*vbl_ev),GFP_ATOMIC ); -if (!vbl_ev) + vbl_work =kzalloc (sizeof (*vbl_work),GFP_ATOMIC ); +if (!vbl_work)return -ENOMEM ; - vbl_ev->crtc_id = crtc_id; - vbl_ev->enable = enable; +INIT_WORK (&vbl_work->work, vblank_ctrl_worker); -spin_lock_irqsave (&vbl_ctrl->lock, flags); -list_add_tail (&vbl_ev->node, &vbl_ctrl->event_list); -spin_unlock_irqrestore (&vbl_ctrl->lock, flags); + vbl_work->crtc_id = crtc_id; + vbl_work->enable = enable; + vbl_work->priv = priv; -schedule_work (&vbl_ctrl->work); +schedule_work (&vbl_work->work);return 0 ; }@@ -269,14 +252,13 @@ static int msm_drm_uninit(struct device *dev)struct msm_drm_private *priv = ddev->dev_private;struct msm_kms *kms = priv->kms;struct msm_mdss *mdss = priv->mdss; -struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev, *tmp;int i;/* We must cancel and cleanup any pending vblank enable/disable * work before drm_irq_uninstall() to avoid work re-enabling an * irq after uninstall has disabled it. */ +msm_gem_shrinker_cleanup (ddev);drm_kms_helper_poll_fini (ddev);drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device *dev) · line 296
Quoted 5 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2@@ -292,12 +274,6 @@ static int msm_drm_uninit(struct device *dev)#endif drm_mode_config_cleanup (ddev); -flush_work (&vbl_ctrl->work); -list_for_each_entry_safe (vbl_ev, tmp, &vbl_ctrl->event_list, node){
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_init(struct device *dev, struct
Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c +0
-4-list_del (&vbl_ev->node); -kfree (vbl_ev); - } -/* clean up event worker threads */ for (i =0 ; i < priv->num_crtcs; i++) {if (priv->event_thread[i].thread) {@@ -469,9 +445,6 @@ static int msm_drm_init(struct device *dev, struct
drm_driver*drv)Review
Quoted 10 lines
drivers/gpu/drm/msm/msm_drv.c +0
-3priv->wq =alloc_ordered_workqueue ("msm" ,0 );INIT_LIST_HEAD (&priv->inactive_list); -INIT_LIST_HEAD (&priv->vblank_ctrl.event_list); -INIT_WORK (&priv->vblank_ctrl.work, vblank_ctrl_worker); -spin_lock_init (&priv->vblank_ctrl.lock);drm_mode_config_init (ddev);(quoted diff) +0
-0
b/drivers/gpu/drm/msm/msm_drv.hQuoted 25 lines
index 05d33a7..d4cbde2 100644
drivers/gpu/drm/msm/msm_drv.h +0
-8@@ -77,12 +77,6 @@ enum msm_mdp_plane_property { PLANE_PROP_MAX_NUM }; -struct msm_vblank_ctrl { -struct work_struct work; -struct list_head event_list; -spinlock_t lock; -}; -#define MSM_GPU_MAX_RINGS 4#define MAX_H_TILES_PER_DISPLAY 2@@ -225,7 +219,6 @@ struct msm_drm_private {struct notifier_block vmap_notifier;struct shrinker shrinker; -struct msm_vblank_ctrl vblank_ctrl;struct drm_atomic_state *pm_state; }; --Quoted 1 line
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
Quoted 6 lines
a Linux Foundation Collaborative Project
_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno
Signature · 5 lines
Jeykumar S _______________________________________________ Freedreno mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/freedreno
Re: [Freedreno] [PATCH v2 5/5] drm/msm: subclass work object for vblank events
Quoted 17 lines · Jeykumar Sankaran
Quoted 7 lines · Sean Paul
Quoted 6 lines · Jeykumar Sankaran
msm maintains a separate structure to define vblank work definitions and a list to track events submitted to the workqueue. We can avoid this redundant list and its protection mechanism, if we subclass the work object to encapsulate vblank event parameters.
Quoted 9 lines
Quoted 2 lines
changes in v2:
- subclass optimization on system wq (Sean Paul)I wouldn't do it like this, tbh. One problem is that you've lost your
flush()on
unbind, so there's no way to know if you have workers in the wild waiting to
enable/disable vblank.
Quoted 8 lines
I believe I didnt respond to this quesition of yours. This patch is removing the flush on the work. But
flush_workqueuebefore destroying the WQ will make sure all the queued work jobs are run to completion.To honor the comment, I will fix the patch to move the flush/destroy wq before the
irq_unininstall.
Above is possible if we use priv->wq for vblank requests. Posted v4 with the above fixes.
Thanks,
Jeykumar S.
Quoted 195 lines
Quoted 195 lines
Another issues is that AFAICT, we don't need a queue of enables/disables, but
rather just the last requested state (ie: should we be on or off). So things
don't need to be this complicated (and we're possibly thrashing vblank on/off
for no reason).I'm still of the mind that you should just make this synchronous and be done
with the threads (especially since we're still uncovering/introducing races!).Sean
Quoted 4 lines
Signed-off-by Jeykumar Sankaran <[email protected]>--- drivers/gpu/drm/msm/msm_drv.c | 67+++++++++++++------------------------------
Review
Quoted 4 lines
drivers/gpu/drm/msm/msm_drv.h | 7 ----- 2 files changed, 20 insertions(+), 54 deletions(-)(quoted diff) +0
-0
b/drivers/gpu/drm/msm/msm_drv.cQuoted 10 lines
index 6d6c73b..8da5be2 100644
drivers/gpu/drm/msm/msm_drv.c +1
-2@@ -203,61 +203,44 @@ u32 msm_readl(const void __iomem *addr)return val; } -struct vblank_event { -struct list_head node; +struct msm_vblank_work {drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 215
Quoted 10 lines
drivers/gpu/drm/msm/msm_drv.c +2
-2+struct work_struct work;int crtc_id;bool enable; +struct msm_drm_private *priv; };static void vblank_ctrl_worker (struct work_struct *work) { - struct msm_vblank_ctrl *vbl_ctrl = container_of(work, - struct msm_vblank_ctrl,work);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 217
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2- struct msm_drm_private *priv = container_of(vbl_ctrl, - struct msm_drm_private,
vblank_ctrl);drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 216
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +2
-0+ struct msm_vblank_work *vbl_work = container_of(work, + struct msm_vblank_work,work);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr)
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c +1
-5+struct msm_drm_private *priv = vbl_work->priv;struct msm_kms *kms = priv->kms; -struct vblank_event *vbl_ev, *tmp; -unsigned long flags; - -spin_lock_irqsave (&vbl_ctrl->lock, flags); -list_for_each_entry_safe (vbl_ev, tmp, &vbl_ctrl->event_list, node){
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 235
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.c +0
-6-list_del (&vbl_ev->node); -spin_unlock_irqrestore (&vbl_ctrl->lock, flags); - -if (vbl_ev->enable) - kms->funcs->enable_vblank(kms, -priv->crtcs[vbl_ev->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 235
Quoted 3 lines
drivers/gpu/drm/msm/msm_drv.c +0
-3-else - kms->funcs->disable_vblank(kms, -priv->crtcs[vbl_ev->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr)
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c +2
-4-kfree (vbl_ev); - -spin_lock_irqsave (&vbl_ctrl->lock, flags); - } +if (vbl_work->enable) + kms->funcs->enable_vblank(kms,priv->crtcs[vbl_work->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 222
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +2
-0+else + kms->funcs->disable_vblank(kms,priv->crtcs[vbl_work->crtc_id]);
Quoted 52 lines
drivers/gpu/drm/msm/msm_drv.c +10
-14-spin_unlock_irqrestore (&vbl_ctrl->lock, flags); +kfree (vbl_work); }static int vblank_ctrl_queue_work (struct msm_drm_private *priv,int crtc_id,bool enable) { -struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev; -unsigned long flags; +struct msm_vblank_work *vbl_work; - vbl_ev =kzalloc (sizeof (*vbl_ev),GFP_ATOMIC ); -if (!vbl_ev) + vbl_work =kzalloc (sizeof (*vbl_work),GFP_ATOMIC ); +if (!vbl_work)return -ENOMEM ; - vbl_ev->crtc_id = crtc_id; - vbl_ev->enable = enable; +INIT_WORK (&vbl_work->work, vblank_ctrl_worker); -spin_lock_irqsave (&vbl_ctrl->lock, flags); -list_add_tail (&vbl_ev->node, &vbl_ctrl->event_list); -spin_unlock_irqrestore (&vbl_ctrl->lock, flags); + vbl_work->crtc_id = crtc_id; + vbl_work->enable = enable; + vbl_work->priv = priv; -schedule_work (&vbl_ctrl->work); +schedule_work (&vbl_work->work);return 0 ; }@@ -269,14 +252,13 @@ static int msm_drm_uninit(struct device *dev)struct msm_drm_private *priv = ddev->dev_private;struct msm_kms *kms = priv->kms;struct msm_mdss *mdss = priv->mdss; -struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev, *tmp;int i;/* We must cancel and cleanup any pending vblank enable/disable * work before drm_irq_uninstall() to avoid work re-enabling an * irq after uninstall has disabled it. */ +msm_gem_shrinker_cleanup (ddev);drm_kms_helper_poll_fini (ddev);drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device *dev) · line 296
Quoted 5 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2@@ -292,12 +274,6 @@ static int msm_drm_uninit(struct device *dev)#endif drm_mode_config_cleanup (ddev); -flush_work (&vbl_ctrl->work); -list_for_each_entry_safe (vbl_ev, tmp, &vbl_ctrl->event_list, node){
Quoted 9 lines
drivers/gpu/drm/msm/msm_drv.c +0
-4-list_del (&vbl_ev->node); -kfree (vbl_ev); - } -/* clean up event worker threads */ for (i =0 ; i < priv->num_crtcs; i++) {if (priv->event_thread[i].thread) {@@ -469,9 +445,6 @@ static int msm_drm_init(struct device *dev,struct
drm_driver*drv)Review
Quoted 10 lines
drivers/gpu/drm/msm/msm_drv.c +0
-3priv->wq =alloc_ordered_workqueue ("msm" ,0 );INIT_LIST_HEAD (&priv->inactive_list); -INIT_LIST_HEAD (&priv->vblank_ctrl.event_list); -INIT_WORK (&priv->vblank_ctrl.work, vblank_ctrl_worker); -spin_lock_init (&priv->vblank_ctrl.lock);drm_mode_config_init (ddev);(quoted diff) +0
-0
b/drivers/gpu/drm/msm/msm_drv.hQuoted 25 lines
index 05d33a7..d4cbde2 100644
drivers/gpu/drm/msm/msm_drv.h +0
-8@@ -77,12 +77,6 @@ enum msm_mdp_plane_property { PLANE_PROP_MAX_NUM }; -struct msm_vblank_ctrl { -struct work_struct work; -struct list_head event_list; -spinlock_t lock; -}; -#define MSM_GPU_MAX_RINGS 4#define MAX_H_TILES_PER_DISPLAY 2@@ -225,7 +219,6 @@ struct msm_drm_private {struct notifier_block vmap_notifier;struct shrinker shrinker; -struct msm_vblank_ctrl vblank_ctrl;struct drm_atomic_state *pm_state; }; --Quoted 1 line
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum,
Quoted 6 lines
a Linux Foundation Collaborative Project
_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno
Signature · 5 lines
Jeykumar S _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Sean Paul
reviewerRe: [Freedreno] [PATCH v2 5/5] drm/msm: subclass work object for vblank events
In reply to Jeykumar SankaranQuoted 138 lines · Jeykumar Sankaran
Quoted 121 lines · Sean Paul
Quoted 6 lines · Jeykumar Sankaran
Quoted 5 lines · Sean Paul
Quoted 4 lines · Jeykumar Sankaran
Quoted 3 lines · Sean Paul
Quoted 2 lines · Jeykumar Sankaran
Quoted 1 line · Sean Paul
On Tue, Nov 06, 2018 at 02:36:30PM -0800, Jeykumar Sankaran
Quoted 10 lines · wrote:
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 8 lines
msm maintains a separate structure to define vblank work definitions and a list to track events submitted to the workqueue. We can avoid this redundant list and its protection mechanism, if we subclass the work object to encapsulate vblank event parameters.
changes in v2:
- subclass optimization on system wq (Sean Paul)I wouldn't do it like this, tbh. One problem is that you've
lost
Quoted 3 lines
Quoted 3 lines
your
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
flush()on
unbind, so there's no way to know if you have workers in thewild
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
waiting
to
enable/disable vblank.Another issues is that AFAICT, we don't need a queue of enables/disables,
but
rather just the last requested state (ie: should we be on oroff).
Quoted 3 lines
Quoted 3 lines
So
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
things
don't need to be this complicated (and we're possiblythrashing
Quoted 5 lines
Quoted 5 lines
vblank
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
on/off
for no reason).I'm still of the mind that you should just make this
synchronous
Quoted 8 lines
Quoted 8 lines
and
Quoted 3 lines
Quoted 3 lines
be
Quoted 2 lines
Quoted 2 lines
done
with the threads (especially since we're stilluncovering/introducing
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 2 lines
races!).
While scoping out the effort to make vblank events synchronous,
I
Quoted 9 lines
Quoted 9 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
found
that the spinlock locking order of vblank request sequence andvblank
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
callback
sequences are the opposite.In DPU,
drm_vblank_enableacquiresvblank_time_lockbeforeregistering
Quoted 1 line
Quoted 1 line
Quoted 1 line
the crtc to encoder which happens after acquiring
encoder_spinlock.Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
But
thevblank_callbackacquiresencoder_spinlockbefore accessingthe
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 6 lines
registered
crtc and calling intodrm_vblank_handlerwhich tries to acquirevblank_time_lock.
Acquiring bothvblank_time_lockandencoder_spinlockin the same thread
is leading to deadlock.Hmm, I'm not sure I follow. Are you seeing issues where irq
overlaps
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
with
enable/disable? I hacked in sync vblank enable/disable quickly tosee
Quoted 34 lines
Quoted 23 lines
if
Quoted 15 lines
Quoted 4 lines
I
could
reproduce what you're seeing, but things seemed well behaved.The race is between drm_vblank_get/put and
vblank_handlercontexts.When made synchronous:
while calling
drm_vblank_get, the callstack looks like below:drm_vblank_get->drm_vblank_enable(acquiresvblank_time_lock) ->__enable_vblank->dpu_crtc_vblank->dpu_encoder_toggle_vblank_for_crtc
(tries to acquireenc_spinlock)Quoted 6 lines
In vblank handler, the call stack will be:
dpu_encoder_phys_vid_vblank_irq->dpu_encoder_vblank_callback(acquiresenc_spinlock) ->dpu_crtc_vblank_callback->drm_handle_vblank(tries to
acquirevblank_time_lock)Quoted 7 lines
Hmm, I'm not sure how this can happen. We acquire and release the
enc_spinlock
before enabling the irq, yes we will hold on to thevbl_time_lock, but we
shouldn't be trying to reacquire an encoder's spinlock after we've enabled
it.In the synchronous approach
dpu_encoder_toggle_vblank_for_crtc(which acquires the enc_spinlock)will be called while we
are holding thevbl_time_lock.Quoted 8 lines
Quoted 4 lines
I don't know how that can deadlock, since we should never be running enable and
the handler concurrently.I agree that
vblank_irqhandler should not be running before the enable
sequence. But
don't you expect the handler to be running while calling the
vblank_disableQuoted 1 line
sequence?
Quoted 8 lines
This is an entirely different problem though. It's also one that is easier
to
fix. I think we could probably grab theenc_spinlockin disable and clear
the
crtc pointer.we do hold
enc_spinlockindpu_encoder_assign_crtc(drm/msm: dpu: Removevblank_callbackfrom encoder)
where we clear the crtc pointer.Quoted 4 lines
What I'm getting at is that there's no fundamental reason why we need to have
async vblank enable/disable.
Quoted 8 lines
Quoted 2 lines
Sean
There is really no *need* to have them async. But I believe the reason why they
are implemented this way is to avoid deadlock between the below two paths.Restating the above findings:
vblank_handlersand vblank enable/disable can run concurrently.
I think this is where we disagree. The handler will only be called when enc->crtc is set.
In the case of disable, we clear the pointer _after_ vblank is disabled, so enc_spinlock should be uncontested.
On enable, the pointer is set _before_ vblank is enabled, so again the enc_spinlock is uncontested.
I tracked down the deadlock you found and submitted a patch for it in [1]. The issue is that vblank was being incorrectly enabled whenever the encoder is on. With the fix, you can add however long delay you want to vblank_enable and you won't produce a deadlock.
Sean
[1]- https://lists.freedesktop.org/archives/dri-devel/2018-December/199670.html
Quoted 321 lines
The first
trying to acquirevbl_time_lockholdingenc_spinlock. Other trying to acquireenc_spinlockholdingvbl_time_lock.Thanks,
Jeykumar S.Quoted 308 lines
Quoted 2 lines
vbl disable will try to acquire the locks in the opposite order to that
of
Quoted 14 lines
irq_handlerand the
same issue is bound to happen.With your patch, you should be able to simulate this deadlock if you can
inject a delay
by adding apr_errlog invblank_ctrl_queue_workThanks,
Jeykumar S.Quoted 3 lines
The only thing I can think of is that the vblank interrupts are firing after
vblank has been disabled? In that case, it seems like we shouldproperly
Quoted 40 lines
Quoted 40 lines
flush
them.Sean
Quoted 11 lines
Quoted 11 lines
I do see that there is a chance to call
drm_handle_vblank()while holdingenc_spinlock, but couldn't find any obvious lock recursion there.Maybe a callstack or lockdep splat would help?
Sean
Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c static void vblank_ctrl_worker(struct
Quoted 8 lines
Here's my hack to bypass the display thread:
drivers/gpu/drm/msm/msm_drv.c +0
-0@@ -242,24 +242,19 @@ static void vblank_ctrl_worker(struct
kthread_workQuoted 14 lines
Quoted 14 lines
drivers/gpu/drm/msm/msm_drv.c +2
-4*work)static int vblank_ctrl_queue_work (struct msm_drm_private *priv,int crtc_id,bool enable) { +struct msm_kms *kms = priv->kms;struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev;unsigned long flags; - vbl_ev =kzalloc (sizeof (*vbl_ev),GFP_ATOMIC ); -if (!vbl_ev) -return -ENOMEM ; +spin_lock_irqsave (&vbl_ctrl->lock, flags);Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/gpu/drm/msm/msm_drv.c static void vblank_ctrl_worker(struct · line 253
Quoted 4 lines
drivers/gpu/drm/msm/msm_drv.c +2
-2- vbl_ev->crtc_id = crtc_id; - vbl_ev->enable = enable; +if (enable) + kms->funcs->enable_vblank(kms,priv->crtcs[crtc_id]);
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c static void vblank_ctrl_worker(struct · line 255
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +2
-0+else + kms->funcs->disable_vblank(kms,priv->crtcs[crtc_id]);
Quoted 49 lines
Quoted 49 lines
Quoted 5 lines
Quoted 5 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2-spin_lock_irqsave (&vbl_ctrl->lock, flags); -list_add_tail (&vbl_ev->node, &vbl_ctrl->event_list);spin_unlock_irqrestore (&vbl_ctrl->lock, flags);Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c static void vblank_ctrl_worker(struct · line 260
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.c +0
-3-kthread_queue_work (&priv->disp_thread[crtc_id].worker, - &vbl_ctrl->work); -return 0 ; }Even with your patch above, I see frame is getting stuck but it recovers
in
Quoted 2 lines
a while.
The patch I tried was assigningcrtc->funcs->enable_vblank/disable_vblank so
Quoted 11 lines
that
__enable_vblankcan call crtc directly. But the above callstack is still
valid for your patch.Thanks,
Jeykumar S.Quoted 4 lines
Quoted 4 lines
In MDP5, I see the same pattern between
vblank_time_lockand
list_lockQuoted 20 lines
Quoted 20 lines
which
Quoted 19 lines
is used to track the irq handlers.
I believe that explains why
msm_drvis queuing the vblank enable/disable
works to WQ after acquiringvblank_time_lock.Thanks,
Jeykumar S.Quoted 10 lines
Sean
Quoted 4 lines
Signed-off-by Jeykumar Sankaran <[email protected]>--- drivers/gpu/drm/msm/msm_drv.c | 67+++++++++++++------------------------------
Quoted 3 lines
drivers/gpu/drm/msm/msm_drv.h | 7 ----- 2 files changed, 20 insertions(+), 54 deletions(-)Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Review
Quoted 1 line
(quoted diff) +0
-0
b/drivers/gpu/drm/msm/msm_drv.cdrivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem
Quoted 4 lines
index 6d6c73b..8da5be2 100644
drivers/gpu/drm/msm/msm_drv.c +0
-0@@ -203,61 +203,44 @@ u32 msm_readl(const void __iomem*addr)
Quoted 57 lines
Quoted 57 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
drivers/gpu/drm/msm/msm_drv.c +3
-2return val; } -struct vblank_event { -struct list_head node; +struct msm_vblank_work { +struct work_struct work;int crtc_id;bool enable; +struct msm_drm_private *priv; };Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem · line 215
Quoted 4 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2static void vblank_ctrl_worker (struct work_struct *work) { -struct msm_vblank_ctrl *vbl_ctrl =container_of (work, - struct
msm_vblank_ctrl,Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
work);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem · line 217
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2- struct msm_drm_private *priv = container_of(vbl_ctrl, - struct msm_drm_private,
vblank_ctrl);drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem · line 216
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +2
-0+struct msm_vblank_work *vbl_work =container_of (work, + struct
msm_vblank_work,Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
work);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem · line 223
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c +1
-5+struct msm_drm_private *priv = vbl_work->priv;struct msm_kms *kms = priv->kms; -struct vblank_event *vbl_ev, *tmp; -unsigned long flags; - -spin_lock_irqsave (&vbl_ctrl->lock, flags); - list_for_each_entry_safe(vbl_ev, tmp,
&vbl_ctrl->event_list, node)Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
{
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem · line 229
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.c +0
-6-list_del (&vbl_ev->node); -spin_unlock_irqrestore (&vbl_ctrl->lock, flags); - -if (vbl_ev->enable) - kms->funcs->enable_vblank(kms, -priv->crtcs[vbl_ev->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem · line 232
Quoted 3 lines
drivers/gpu/drm/msm/msm_drv.c +0
-3-else - kms->funcs->disable_vblank(kms, -priv->crtcs[vbl_ev->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem · line 220
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c +2
-4-kfree (vbl_ev); - -spin_lock_irqsave (&vbl_ctrl->lock, flags); - } +if (vbl_work->enable) + kms->funcs->enable_vblank(kms,priv->crtcs[vbl_work->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem · line 222
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +2
-0+else + kms->funcs->disable_vblank(kms,priv->crtcs[vbl_work->crtc_id]);
Quoted 1 line
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/gpu/drm/msm/msm_drv.c
Quoted 5 lines
drivers/gpu/drm/msm/msm_drv.c +1
-1-spin_unlock_irqrestore (&vbl_ctrl->lock, flags); +kfree (vbl_work); } static int vblank_ctrl_queue_work(struct msm_drm_private*priv,
Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
Quoted 24 lines
drivers/gpu/drm/msm/msm_drv.c +7
-10int crtc_id, bool enable) { -struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev; -unsigned long flags; +struct msm_vblank_work *vbl_work; - vbl_ev =kzalloc (sizeof (*vbl_ev),GFP_ATOMIC ); -if (!vbl_ev) + vbl_work =kzalloc (sizeof (*vbl_work),GFP_ATOMIC ); +if (!vbl_work)return -ENOMEM ; - vbl_ev->crtc_id = crtc_id; - vbl_ev->enable = enable; +INIT_WORK (&vbl_work->work, vblank_ctrl_worker); -spin_lock_irqsave (&vbl_ctrl->lock, flags); -list_add_tail (&vbl_ev->node, &vbl_ctrl->event_list); -spin_unlock_irqrestore (&vbl_ctrl->lock, flags); + vbl_work->crtc_id = crtc_id; + vbl_work->enable = enable; + vbl_work->priv = priv;Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.c +1
-1-schedule_work (&vbl_ctrl->work); +schedule_work (&vbl_work->work);return 0 ; }@@ -269,14 +252,13 @@ static int msm_drm_uninit(structdevice
Quoted 27 lines
Quoted 27 lines
*dev)
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct · line 257
Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2struct msm_drm_private *priv = ddev->dev_private;struct msm_kms *kms = priv->kms;struct msm_mdss *mdss = priv->mdss; -struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev, *tmp;int i;/* We must cancel and cleanup any pending vblank enable/disable
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct · line 258
Quoted 1 line
drivers/gpu/drm/msm/msm_drv.c +0
-0* work before drm_irq_uninstall() to avoid work re-enabling an
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c +1
-0* irq after uninstall has disabled it. */ +msm_gem_shrinker_cleanup (ddev);drm_kms_helper_poll_fini (ddev);@@ -292,12 +274,6 @@ static int msm_drm_uninit(struct device*dev)
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device · line 296
Quoted 5 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2#endif drm_mode_config_cleanup (ddev); -flush_work (&vbl_ctrl->work); - list_for_each_entry_safe(vbl_ev, tmp,
&vbl_ctrl->event_list, node)Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
{
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_init(struct device
Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c +0
-4-list_del (&vbl_ev->node); -kfree (vbl_ev); - } -/* clean up event worker threads */ for (i =0 ; i < priv->num_crtcs; i++) {if (priv->event_thread[i].thread) {@@ -469,9 +445,6 @@ static int msm_drm_init(struct device*dev,
Quoted 56 lines
Quoted 33 lines
Quoted 33 lines
Quoted 33 lines
struct
Quoted 32 lines
Quoted 32 lines
drm_driver*drv)Review
Quoted 10 lines
drivers/gpu/drm/msm/msm_drv.c +0
-3priv->wq =alloc_ordered_workqueue ("msm" ,0 );INIT_LIST_HEAD (&priv->inactive_list); -INIT_LIST_HEAD (&priv->vblank_ctrl.event_list); -INIT_WORK (&priv->vblank_ctrl.work, vblank_ctrl_worker); -spin_lock_init (&priv->vblank_ctrl.lock);drm_mode_config_init (ddev);(quoted diff) +0
-0
b/drivers/gpu/drm/msm/msm_drv.hQuoted 20 lines
index 05d33a7..d4cbde2 100644
drivers/gpu/drm/msm/msm_drv.h +0
-6@@ -77,12 +77,6 @@ enum msm_mdp_plane_property { PLANE_PROP_MAX_NUM }; -struct msm_vblank_ctrl { -struct work_struct work; -struct list_head event_list; -spinlock_t lock; -}; -#define MSM_GPU_MAX_RINGS 4#define MAX_H_TILES_PER_DISPLAY 2@@ -225,7 +219,6 @@ struct msm_drm_private {struct notifier_block vmap_notifier;struct shrinker shrinker;Quoted 20 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.h +0
-2-struct msm_vblank_ctrl vblank_ctrl;struct drm_atomic_state *pm_state; }; --The Qualcomm Innovation Center, Inc. is a member of the Code
Aurora
Quoted 13 lines
Quoted 10 lines
Quoted 10 lines
Quoted 7 lines
Forum,
Quoted 6 lines
a Linux Foundation Collaborative Project
_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedreno--
Jeykumar S--
Jeykumar S--
Jeykumar SSignature · 1 line
Jeykumar S
Signature · 5 lines
Sean Paul, Software Engineer, Google / Chromium OS _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH v2 5/5] drm/msm: subclass work object for vblank events
Quoted 183 lines · Sean Paul
Quoted 6 lines · Jeykumar Sankaran
Quoted 5 lines · Sean Paul
Quoted 4 lines · Jeykumar Sankaran
Quoted 3 lines · Sean Paul
Quoted 2 lines · Jeykumar Sankaran
Quoted 1 line · Sean Paul
On Tue, Nov 20, 2018 at 02:04:14PM -0800, Jeykumar Sankaran
Quoted 2 lines · wrote:
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines · Sean Paul
Quoted 1 line
On Tue, Nov 06, 2018 at 02:36:30PM -0800, Jeykumar
Sankaran
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines · wrote:
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 8 lines
msm maintains a separate structure to define vblank work definitions and a list to track events submitted to the workqueue. We can avoid this redundant list and its protection mechanism, if we subclass the work object to encapsulate vblank event parameters.
changes in v2:
- subclass optimization on system wq (Sean Paul)I wouldn't do it like this, tbh. One problem is that
you've
Quoted 4 lines
Quoted 4 lines
lost
Quoted 3 lines
Quoted 3 lines
your
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
flush()on
unbind, so there's no way to know if you have workers inthe
Quoted 9 lines
Quoted 9 lines
wild
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
waiting
to
enable/disable vblank.Another issues is that AFAICT, we don't need a queue of enables/disables,
but
rather just the last requested state (ie: should we be onor
Quoted 11 lines
Quoted 11 lines
off).
Quoted 3 lines
Quoted 3 lines
So
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
things
don't need to be this complicated (and we're possiblythrashing
Quoted 5 lines
Quoted 5 lines
vblank
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
on/off
for no reason).I'm still of the mind that you should just make this
synchronous
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
and
Quoted 3 lines
Quoted 3 lines
be
Quoted 2 lines
Quoted 2 lines
done
with the threads (especially since we're stilluncovering/introducing
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 2 lines
races!).
While scoping out the effort to make vblank events
synchronous,
Quoted 3 lines
Quoted 3 lines
I
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
found
that the spinlock locking order of vblank request sequenceand
Quoted 10 lines
Quoted 10 lines
Quoted 7 lines
Quoted 7 lines
vblank
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
callback
sequences are the opposite.In DPU,
drm_vblank_enableacquiresvblank_time_lockbeforeregistering
Quoted 1 line
Quoted 1 line
Quoted 1 line
the crtc to encoder which happens after acquiring
encoder_spinlock.Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
But
thevblank_callbackacquiresencoder_spinlockbeforeaccessing
Quoted 3 lines
Quoted 3 lines
the
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
registered
crtc and calling intodrm_vblank_handlerwhich tries toacquire
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
vblank_time_lock.
Acquiring bothvblank_time_lockandencoder_spinlockin thesame
Quoted 7 lines
Quoted 7 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 2 lines
thread
is leading to deadlock.Hmm, I'm not sure I follow. Are you seeing issues where irq
overlaps
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
with
enable/disable? I hacked in sync vblank enable/disable quicklyto
Quoted 8 lines
Quoted 8 lines
see
Quoted 7 lines
Quoted 7 lines
if
Quoted 6 lines
Quoted 4 lines
I
could
reproduce what you're seeing, but things seemed well behaved.The race is between drm_vblank_get/put and
vblank_handlercontexts.
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
When made synchronous:
while calling
drm_vblank_get, the callstack looks like below:drm_vblank_get->drm_vblank_enable(acquiresvblank_time_lock)->
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 10 lines
__enable_vblank->dpu_crtc_vblank->dpu_encoder_toggle_vblank_for_crtc
(tries to acquireenc_spinlock)In vblank handler, the call stack will be:
dpu_encoder_phys_vid_vblank_irq->dpu_encoder_vblank_callback(acquiresenc_spinlock) ->dpu_crtc_vblank_callback->drm_handle_vblank(tries to
acquirevblank_time_lock)Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Hmm, I'm not sure how this can happen. We acquire and release the
enc_spinlock
before enabling the irq, yes we will hold on to thevbl_time_lock,but
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 4 lines
we
shouldn't be trying to reacquire an encoder's spinlock after we've enabled
it.In the synchronous approach
dpu_encoder_toggle_vblank_for_crtc(which acquires the enc_spinlock)will be called while we
are holding thevbl_time_lock.Quoted 1 line
I don't know how that can deadlock, since we should never be
running
Quoted 18 lines
Quoted 10 lines
Quoted 7 lines
Quoted 3 lines
enable and
the handler concurrently.I agree that
vblank_irqhandler should not be running before the enable
sequence. But
don't you expect the handler to be running while calling the
vblank_disableQuoted 1 line
sequence?
Quoted 8 lines
This is an entirely different problem though. It's also one that is easier
to
fix. I think we could probably grab theenc_spinlockin disable and clear
the
crtc pointer.Quoted 5 lines
we do hold
enc_spinlockindpu_encoder_assign_crtc(drm/msm: dpu: Removevblank_callbackfrom encoder)
where we clear the crtc pointer.Quoted 1 line
What I'm getting at is that there's no fundamental reason why we need
to
Quoted 6 lines
Quoted 5 lines
have
async vblank enable/disable.Sean
There is really no *need* to have them async. But I believe the reason
why
Quoted 2 lines
they
are implemented this way is to avoid deadlock between the below twopaths.
Quoted 3 lines
Restating the above findings:
vblank_handlersand vblank enable/disable can run concurrently.I think this is where we disagree. The handler will only be called when
enc->crtcis set.In the case of disable, we clear the pointer _after_ vblank is disabled, so
enc_spinlockshould be uncontested.On enable, the pointer is set _before_ vblank is enabled, so again the
enc_spinlockis uncontested.I tracked down the deadlock you found and submitted a patch for it in [1]. The
issue is that vblank was being incorrectly enabled whenever the encoder is on.
With the fix, you can add however long delay you want tovblank_enableand you
won't produce a deadlock.
Quoted 6 lines
Sean
[1]-
https://lists.freedesktop.org/archives/dri-devel/2018-December/199670.html
I am aware of the bug in DPU which enables vsync irq before vblank_enable is called. That is why I tried to explain the concurrency during vblank_disable. Your patch certainly fixes the enable sequence. But the deadlock is still reproducible during vblank_disable. Below are the logs I could collect during deadlock:
[ 13.448254] [drm:dpu_encoder_vblank_callback:1242] vbl: dpu_encoder_vblank_callback - acquire enc_spinlock [ 13.458187] vbl: drm_handle_vblank - acquire vblank_time_lock [ 13.472993] [drm:dpu_encoder_phys_vid_vblank_irq:319] vbl: dpu_encoder_phys_vid_vblank_irq [ 13.481586] [drm:dpu_encoder_vblank_callback:1242] vbl: dpu_encoder_vblank_callback - acquire enc_spinlock [ 13.491518] vbl: drm_handle_vblank - acquire vblank_time_lock
--> vblank_disable 5 sec timer expires
[Task 1] [ 13.506175] vbl: vblank_disable_fn - timer expires
--> vblank irq is fired.
[Task 2] [ 13.506326]
[drm:dpu_encoder_phys_vid_vblank_irq:319] vbl: dpu_encoder_phys_vid_vblank_irq
--> Task 1 acquires vblank_time_lock
[Task 1] [ 13.511157] vbl: drm_vblank_disable_and_save -
acquire vblank_time_lock
--> Task 2 acquires enc spin lock
[Task 2] [ 13.519748] [drm:dpu_encoder_vblank_callback:1242]
vbl: dpu_encoder_vblank_callback - acquire enc_spinlock
--> Task 2 waiting for vblank_time_lock
[Task 2] [ 13.519757] vbl: drm_handle_vblank - acquire
vblank_time_lock
[Task 1] [ 13.526682] vbl: __disable_vblank
[Task 1] [ 13.546082] vbl: vblank_ctrl_queue_work
--> Task 1 waiting for enc_spinlock causing deadlock
[Task 1] [ 13.550068] [drm:dpu_encoder_toggle_vblank_for_crtc:1287] vbl: dpu_encoder_toggle_vblank_for_crtc - acquire enc_spinlock
Task 1 represents vblank_disable thread
Task 2 represents vblank irq handler thread
The patch I used for the additional logging as below:
drivers/gpu/drm/drm_vblank.c +9 -1
@@ -330,6 +330,8 @@ static void __disable_vblank(struct drm_device *dev, unsigned int pipe) } } +pr_err ("vbl: %s \n" , __func__); + dev->driver->disable_vblank (dev, pipe); }
@@ -350,6 +352,7 @@ void drm_vblank_disable_and_save(struct drm_device *dev, unsigned int pipe)* so no updates of timestamps or count can happen after we've * disabled. Needed to prevent races in case of delayed irq's. */ +pr_err ("vbl: %s - acquire vblank_time_lock \n" , __func__);spin_lock_irqsave (&dev->vblank_time_lock, irqflags);/*
@@ -382,6 +385,8 @@ static void vblank_disable_fn(struct timer_list *t)unsigned int pipe = vblank->pipe;unsigned long irqflags; +pr_err ("vbl: %s - timer expires\n" , __func__); +spin_lock_irqsave (&dev->vbl_lock, irqflags);if (atomic_read (&vblank->refcount) ==0 && vblank->enabled) {DRM_DEBUG ("disabling vblank on crtc %u\n" , pipe);
@@ -1685,7 +1690,7 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe)struct drm_vblank_crtc *vblank = &dev->vblank[pipe];unsigned long irqflags;bool disable_irq; - +if (WARN_ON_ONCE (!dev->num_crtcs))return false;
@@ -1694,6 +1699,9 @@ bool drm_handle_vblank(struct drm_device *dev, unsigned int pipe)spin_lock_irqsave (&dev->event_lock, irqflags); +pr_err ("vbl: %s - acquire vblank_time_lock\n" , __func__); + +/* Need timestamp lock to prevent concurrent execution with * vblank enable/disable, as this would cause inconsistent * or corrupted timestamps and vblank counts.
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +4 -0
@@ -1239,7 +1239,9 @@ static void dpu_encoder_vblank_callback(struct drm_encoder *drm_enc,DPU_ATRACE_BEGIN ("encoder_vblank_callback" ); dpu_enc =to_dpu_encoder_virt (drm_enc); +pr_err ("vbl: %s - acquire enc_spinlock\n" , __func__);spin_lock_irqsave (&dpu_enc->enc_spinlock, lock_flags); +if (dpu_enc->crtc)dpu_crtc_vblank_callback (dpu_enc->crtc);spin_unlock_irqrestore (&dpu_enc->enc_spinlock, lock_flags);
@@ -1282,6 +1284,8 @@ void dpu_encoder_toggle_vblank_for_crtc(struct drm_encoder *drm_enc,trace_dpu_enc_vblank_cb (DRMID (drm_enc), enable); +pr_err ("vbl: %s - acquire enc_spinlock\n" , __func__); +spin_lock_irqsave (&dpu_enc->enc_spinlock, lock_flags);if (dpu_enc->crtc != crtc) { spin_unlock_irqrestore(&dpu_enc->enc_spinlock,
lock_flags);
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c +2 -0
@@ -316,6 +316,8 @@ static void dpu_encoder_phys_vid_vblank_irq(void *arg, int irq_idx)DPU_ATRACE_BEGIN ("vblank_irq" ); +pr_err ("vbl: %s \n" , __func__); +if (phys_enc->parent_ops->handle_vblank_virt)
phys_enc->parent_ops->handle_vblank_virt(phys_enc->parent,
phys_enc);
drivers/gpu/drm/msm/msm_drv.c +1 -0
@@ -249,7 +249,7 @@ static int vblank_ctrl_queue_work(struct msm_drm_private *priv,unsigned long flags;struct msm_kms *kms = priv->kms; +pr_err ("vbl: %s\n" , __func__);spin_lock_irqsave (&vbl_ctrl->lock, flags);if (enable)
Thanks,
Jeykumar S.
Quoted 341 lines
Quoted 6 lines
The first
trying to acquirevbl_time_lockholdingenc_spinlock. Other trying to acquireenc_spinlockholdingvbl_time_lock.Quoted 19 lines
Thanks,
Jeykumar S.Quoted 15 lines
Quoted 2 lines
vbl disable will try to acquire the locks in the opposite order to that
of
Quoted 12 lines
irq_handlerand the
same issue is bound to happen.With your patch, you should be able to simulate this deadlock if you can
inject a delay
by adding apr_errlog invblank_ctrl_queue_workThanks,
Jeykumar S.Quoted 1 line
The only thing I can think of is that the vblank interrupts are
firing
Quoted 6 lines
Quoted 6 lines
Quoted 2 lines
Quoted 2 lines
after
vblank has been disabled? In that case, it seems like we shouldproperly
Quoted 3 lines
Quoted 3 lines
flush
them.Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Sean
Quoted 3 lines
Quoted 3 lines
I do see that there is a chance to call
drm_handle_vblank()while
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
holding
enc_spinlock, but couldn't find any obvious lock recursionthere.
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 15 lines
Quoted 6 lines
Quoted 6 lines
Maybe a callstack or lockdep splat would help?
Sean
Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c static void vblank_ctrl_worker(struct
Quoted 8 lines
Here's my hack to bypass the display thread:
drivers/gpu/drm/msm/msm_drv.c +0
-0@@ -242,24 +242,19 @@ static void vblank_ctrl_worker(struct
kthread_workQuoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c static void vblank_ctrl_worker(struct · line 243
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +0
-0*work) static int vblank_ctrl_queue_work(struct msm_drm_private*priv,
Quoted 33 lines
Quoted 33 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
drivers/gpu/drm/msm/msm_drv.c +2
-4int crtc_id, bool enable) { +struct msm_kms *kms = priv->kms;struct msm_vblank_ctrl *vbl_ctrl = &priv->vblank_ctrl; -struct vblank_event *vbl_ev;unsigned long flags; - vbl_ev =kzalloc (sizeof (*vbl_ev),GFP_ATOMIC ); -if (!vbl_ev) -return -ENOMEM ; +spin_lock_irqsave (&vbl_ctrl->lock, flags);Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/gpu/drm/msm/msm_drv.c static void vblank_ctrl_worker(struct · line 253
Quoted 4 lines
drivers/gpu/drm/msm/msm_drv.c +2
-2- vbl_ev->crtc_id = crtc_id; - vbl_ev->enable = enable; +if (enable) + kms->funcs->enable_vblank(kms,priv->crtcs[crtc_id]);
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c static void vblank_ctrl_worker(struct · line 255
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +2
-0+else + kms->funcs->disable_vblank(kms,priv->crtcs[crtc_id]);
Quoted 13 lines
Quoted 13 lines
Quoted 13 lines
drivers/gpu/drm/msm/msm_drv.c static void vblank_ctrl_worker(struct · line 260
Quoted 11 lines
drivers/gpu/drm/msm/msm_drv.c +0
-5-spin_lock_irqsave (&vbl_ctrl->lock, flags); -list_add_tail (&vbl_ev->node, &vbl_ctrl->event_list);spin_unlock_irqrestore (&vbl_ctrl->lock, flags); -kthread_queue_work (&priv->disp_thread[crtc_id].worker, - &vbl_ctrl->work); -return 0 ; }Even with your patch above, I see frame is getting stuck but it recovers
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
in
Quoted 2 lines
a while.
The patch I tried was assigningcrtc->funcs->enable_vblank/disable_vblank so
Quoted 2 lines
that
__enable_vblankcan call crtc directly. But the above callstackis
Quoted 49 lines
Quoted 49 lines
Quoted 30 lines
Quoted 30 lines
Quoted 9 lines
still
valid for your patch.Thanks,
Jeykumar S.Quoted 4 lines
Quoted 4 lines
In MDP5, I see the same pattern between
vblank_time_lockand
list_lockQuoted 20 lines
Quoted 20 lines
which
Quoted 19 lines
is used to track the irq handlers.
I believe that explains why
msm_drvis queuing the vblank enable/disable
works to WQ after acquiringvblank_time_lock.Thanks,
Jeykumar S.Quoted 10 lines
Sean
Quoted 4 lines
Signed-off-by Jeykumar Sankaran <[email protected]>--- drivers/gpu/drm/msm/msm_drv.c | 67+++++++++++++------------------------------
Quoted 3 lines
drivers/gpu/drm/msm/msm_drv.h | 7 ----- 2 files changed, 20 insertions(+), 54 deletions(-)Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Review
Quoted 1 line
(quoted diff) +0
-0
b/drivers/gpu/drm/msm/msm_drv.cdrivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem
Quoted 4 lines
index 6d6c73b..8da5be2 100644
drivers/gpu/drm/msm/msm_drv.c +0
-0@@ -203,61 +203,44 @@ u32 msm_readl(const void __iomem*addr)
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
drivers/gpu/drm/msm/msm_drv.c +3
-2return val; } -struct vblank_event { -struct list_head node; +struct msm_vblank_work { +struct work_struct work;int crtc_id;bool enable; +struct msm_drm_private *priv; };Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem · line 213
Quoted 1 line
drivers/gpu/drm/msm/msm_drv.c +0
-0static void vblank_ctrl_worker(struct work_struct*work)
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem · line 214
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +0
-1{ - struct msm_vblank_ctrl *vbl_ctrl =container_of(work,
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem · line 215
Quoted 1 line
drivers/gpu/drm/msm/msm_drv.c +0
-1- struct
msm_vblank_ctrl,Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
work);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem · line 216
Quoted 1 line
drivers/gpu/drm/msm/msm_drv.c +0
-1- struct msm_drm_private *priv =container_of(vbl_ctrl,
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem · line 217
Quoted 1 line
drivers/gpu/drm/msm/msm_drv.c +0
-1- struct
msm_drm_private,Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
vblank_ctrl);drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem · line 215
Quoted 1 line
drivers/gpu/drm/msm/msm_drv.c +1
-0+ struct msm_vblank_work *vbl_work =container_of(work,
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem · line 216
Quoted 1 line
drivers/gpu/drm/msm/msm_drv.c +1
-0+ struct
msm_vblank_work,Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
work);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem · line 223
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c +1
-5+struct msm_drm_private *priv = vbl_work->priv;struct msm_kms *kms = priv->kms; -struct vblank_event *vbl_ev, *tmp; -unsigned long flags; - -spin_lock_irqsave (&vbl_ctrl->lock, flags); - list_for_each_entry_safe(vbl_ev, tmp,
&vbl_ctrl->event_list, node)Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
{
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem · line 225
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2-list_del (&vbl_ev->node); - spin_unlock_irqrestore(&vbl_ctrl->lock,flags);
Quoted 21 lines
Quoted 21 lines
Quoted 21 lines
Quoted 21 lines
Quoted 21 lines
Quoted 21 lines
Quoted 21 lines
Quoted 21 lines
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem · line 229
Quoted 4 lines
drivers/gpu/drm/msm/msm_drv.c +0
-4- -if (vbl_ev->enable) - kms->funcs->enable_vblank(kms, -priv->crtcs[vbl_ev->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem · line 232
Quoted 3 lines
drivers/gpu/drm/msm/msm_drv.c +0
-3-else - kms->funcs->disable_vblank(kms, -priv->crtcs[vbl_ev->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem · line 220
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c +2
-4-kfree (vbl_ev); - -spin_lock_irqsave (&vbl_ctrl->lock, flags); - } +if (vbl_work->enable) + kms->funcs->enable_vblank(kms,priv->crtcs[vbl_work->crtc_id]);
drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem · line 222
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c +2
-0+else + kms->funcs->disable_vblank(kms,priv->crtcs[vbl_work->crtc_id]);
Quoted 1 line
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/gpu/drm/msm/msm_drv.c
Quoted 5 lines
drivers/gpu/drm/msm/msm_drv.c +1
-1-spin_unlock_irqrestore (&vbl_ctrl->lock, flags); +kfree (vbl_work); } static int vblank_ctrl_queue_work(struct
msm_drm_privateQuoted 2 lines
Quoted 2 lines
*priv,
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
int
crtc_id, boolenable)
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
drivers/gpu/drm/msm/msm_drv.c
Quoted 2 lines
{
drivers/gpu/drm/msm/msm_drv.c +0
-1- struct msm_vblank_ctrl *vbl_ctrl =
&priv->vblank_ctrl;Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
Quoted 10 lines
drivers/gpu/drm/msm/msm_drv.c +3
-4-struct vblank_event *vbl_ev; -unsigned long flags; +struct msm_vblank_work *vbl_work; - vbl_ev =kzalloc (sizeof (*vbl_ev),GFP_ATOMIC ); -if (!vbl_ev) + vbl_work =kzalloc (sizeof (*vbl_work),GFP_ATOMIC ); +if (!vbl_work)return -ENOMEM ;Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.c
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.c +1
-4- vbl_ev->crtc_id = crtc_id; - vbl_ev->enable = enable; +INIT_WORK (&vbl_work->work, vblank_ctrl_worker); -spin_lock_irqsave (&vbl_ctrl->lock, flags); - list_add_tail(&vbl_ev->node,
&vbl_ctrl->event_list);Quoted 12 lines
Quoted 12 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
Quoted 11 lines
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct
Quoted 11 lines
drivers/gpu/drm/msm/msm_drv.c +4
-2-spin_unlock_irqrestore (&vbl_ctrl->lock, flags); + vbl_work->crtc_id = crtc_id; + vbl_work->enable = enable; + vbl_work->priv = priv; -schedule_work (&vbl_ctrl->work); +schedule_work (&vbl_work->work);return 0 ; }@@ -269,14 +252,13 @@ static int msm_drm_uninit(structdevice
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
*dev)
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct · line 272
Quoted 4 lines
drivers/gpu/drm/msm/msm_drv.c +0
-1struct msm_drm_private *priv = ddev->dev_private;struct msm_kms *kms = priv->kms;struct msm_mdss *mdss = priv->mdss; - struct msm_vblank_ctrl *vbl_ctrl =
&priv->vblank_ctrl;Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct · line 257
Quoted 4 lines
drivers/gpu/drm/msm/msm_drv.c +0
-1-struct vblank_event *vbl_ev, *tmp;int i;/* We must cancel and cleanup any pending vblank enable/disable
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
drivers/gpu/drm/msm/msm_drv.c +0
-0* work before drm_irq_uninstall() to avoid work Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
re-enabling an
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct
Quoted 7 lines
drivers/gpu/drm/msm/msm_drv.c +1
-0* irq after uninstall has disabled it. */ +msm_gem_shrinker_cleanup (ddev);drm_kms_helper_poll_fini (ddev);@@ -292,12 +274,6 @@ static int msm_drm_uninit(structdevice
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
*dev)
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct · line 296
Quoted 5 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2#endif drm_mode_config_cleanup (ddev); -flush_work (&vbl_ctrl->work); - list_for_each_entry_safe(vbl_ev, tmp,
&vbl_ctrl->event_list, node)Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
{
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c static int msm_drm_init(struct
Quoted 8 lines
drivers/gpu/drm/msm/msm_drv.c +0
-4-list_del (&vbl_ev->node); -kfree (vbl_ev); - } -/* clean up event worker threads */ for (i =0 ; i < priv->num_crtcs; i++) {if (priv->event_thread[i].thread) {@@ -469,9 +445,6 @@ static int msm_drm_init(structdevice
Quoted 8 lines
Quoted 8 lines
*dev,
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
struct
Quoted 6 lines
Quoted 6 lines
drm_driver*drv)drivers/gpu/drm/msm/msm_drv.c static int msm_drm_init(struct · line 473
Quoted 5 lines
drivers/gpu/drm/msm/msm_drv.c +0
-2priv->wq =alloc_ordered_workqueue ("msm" ,0 );INIT_LIST_HEAD (&priv->inactive_list); -INIT_LIST_HEAD (&priv->vblank_ctrl.event_list); - INIT_WORK(&priv->vblank_ctrl.work,
vblank_ctrl_worker);Quoted 26 lines
Quoted 26 lines
Quoted 26 lines
Quoted 26 lines
Quoted 26 lines
Quoted 26 lines
Quoted 26 lines
Quoted 26 lines
Review
Quoted 5 lines
drivers/gpu/drm/msm/msm_drv.c +0
-1-spin_lock_init (&priv->vblank_ctrl.lock);drm_mode_config_init (ddev);(quoted diff) +0
-0
b/drivers/gpu/drm/msm/msm_drv.hQuoted 20 lines
index 05d33a7..d4cbde2 100644
drivers/gpu/drm/msm/msm_drv.h +0
-6@@ -77,12 +77,6 @@ enum msm_mdp_plane_property { PLANE_PROP_MAX_NUM }; -struct msm_vblank_ctrl { -struct work_struct work; -struct list_head event_list; -spinlock_t lock; -}; -#define MSM_GPU_MAX_RINGS 4#define MAX_H_TILES_PER_DISPLAY 2@@ -225,7 +219,6 @@ struct msm_drm_private {struct notifier_block vmap_notifier;struct shrinker shrinker;Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
drivers/gpu/drm/msm/msm_drv.h +0
-2-struct msm_vblank_ctrl vblank_ctrl;struct drm_atomic_state *pm_state; }; --The Qualcomm Innovation Center, Inc. is a member of the
Code
Quoted 20 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Aurora
Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
Quoted 7 lines
Forum,
Quoted 6 lines
a Linux Foundation Collaborative Project
_______________________________________________
Freedreno mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/freedrenoQuoted 8 lines
Quoted 8 lines
Quoted 5 lines
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines
--
Jeykumar S--
Jeykumar S--
Jeykumar S--
Jeykumar S
Signature · 5 lines
Jeykumar S _______________________________________________ Freedreno mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/freedreno