[PATCH v2 1/5] drm/msm: destroy msm threads after config cleanup

Discussion:

Jeykumar Sankaran

2018-11-06 22:36:26 UTC

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)

Signed-off-by: Jeykumar Sankaran <***@codeaurora.org>
---
drivers/gpu/drm/msm/msm_drv.c | 36 +++++++++++++++++-------------------
1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 9c9f7ff..e913059 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -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);

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Jeykumar Sankaran

2018-11-06 22:36:28 UTC

Permalink

msm 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

Signed-off-by: Jeykumar Sankaran <***@codeaurora.org>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c | 26 +++++++-------------------
1 file changed, 7 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
index 82c55ef..9b3d1f2 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
@@ -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,

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Jeykumar Sankaran

2018-11-06 22:36:27 UTC

Permalink

DPU 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)

Signed-off-by: Jeykumar Sankaran <***@codeaurora.org>
---
drivers/gpu/drm/msm/msm_drv.c | 9 ++++-----
drivers/gpu/drm/msm/msm_drv.h | 2 +-
2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index e913059..7d3ca99 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -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);
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 9d11f32..126345c4 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -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;
};

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Jeykumar Sankaran

2018-11-06 22:36:29 UTC

Permalink

Since 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)

Signed-off-by: Jeykumar Sankaran <***@codeaurora.org>
---
drivers/gpu/drm/msm/msm_drv.c | 35 +----------------------------------
drivers/gpu/drm/msm/msm_drv.h | 1 -
2 files changed, 1 insertion(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 7d3ca99..6d6c73b 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -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, &param);
- 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, &param);
if (ret)
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 126345c4..05d33a7 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -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;

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Jeykumar Sankaran

2018-11-06 22:36:30 UTC

Permalink

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 <***@codeaurora.org>
---
drivers/gpu/drm/msm/msm_drv.c | 67 +++++++++++++------------------------------
drivers/gpu/drm/msm/msm_drv.h | 7 -----
2 files changed, 20 insertions(+), 54 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 6d6c73b..8da5be2 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -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);

diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 05d33a7..d4cbde2 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -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;
};

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

Jordan Crouse

2018-11-06 23:15:49 UTC

Permalink

Post by 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.
- 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(-)
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 6d6c73b..8da5be2 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -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.
*/
+

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.

Post by Jeykumar Sankaran
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);
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 05d33a7..d4cbde2 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -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;
};

--
The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

Sean Paul

2018-11-07 15:55:51 UTC

Permalink

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

Post by Jeykumar Sankaran
---
drivers/gpu/drm/msm/msm_drv.c | 67 +++++++++++++------------------------------
drivers/gpu/drm/msm/msm_drv.h | 7 -----
2 files changed, 20 insertions(+), 54 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_drv.c b/drivers/gpu/drm/msm/msm_drv.c
index 6d6c73b..8da5be2 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -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);
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 05d33a7..d4cbde2 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -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;
};
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
_______________________________________________
Freedreno mailing list
https://lists.freedesktop.org/mailman/listinfo/freedreno

--
Sean Paul, Software Engineer, Google / Chromium OS

Jeykumar Sankaran

2018-11-20 22:04:14 UTC

Permalink

Post by 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!).

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.

Post by Sean Paul
Sean

Post by Jeykumar Sankaran
---
drivers/gpu/drm/msm/msm_drv.c | 67

+++++++++++++------------------------------

Post by Jeykumar Sankaran
drivers/gpu/drm/msm/msm_drv.h | 7 -----
2 files changed, 20 insertions(+), 54 deletions(-)
diff --git a/drivers/gpu/drm/msm/msm_drv.c

b/drivers/gpu/drm/msm/msm_drv.c

Post by Jeykumar Sankaran
index 6d6c73b..8da5be2 100644
--- a/drivers/gpu/drm/msm/msm_drv.c
+++ b/drivers/gpu/drm/msm/msm_drv.c
@@ -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);

Post by Jeykumar Sankaran
- struct msm_drm_private *priv = container_of(vbl_ctrl,
- struct msm_drm_private,

vblank_ctrl);

Post by Jeykumar Sankaran
+ struct msm_vblank_work *vbl_work = container_of(work,
+ struct msm_vblank_work,

work);

Post by Jeykumar Sankaran
+ 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)

{

Post by Jeykumar Sankaran
- 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]);

Post by Jeykumar Sankaran
- else
- kms->funcs->disable_vblank(kms,
-

priv->crtcs[vbl_ev->crtc_id]);

Post by Jeykumar Sankaran
- 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]);

Post by Jeykumar Sankaran
+ else
+ kms->funcs->disable_vblank(kms,

priv->crtcs[vbl_work->crtc_id]);

Post by Jeykumar Sankaran
- 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)

{

Post by Jeykumar Sankaran
- 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)

Post by Jeykumar Sankaran
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);
diff --git a/drivers/gpu/drm/msm/msm_drv.h

b/drivers/gpu/drm/msm/msm_drv.h

Post by Jeykumar Sankaran
index 05d33a7..d4cbde2 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -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;
};
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora

Forum,

Post by Jeykumar Sankaran
a Linux Foundation Collaborative Project
_______________________________________________
Freedreno mailing list
https://lists.freedesktop.org/mailman/listinfo/freedreno

--
Jeykumar S

Sean Paul

2018-11-29 22:15:11 UTC