drm/msm: use common display thread for dispatching vblank events

msm maintains a separate structure to define vblank work definitions and a list to track events submitted to the display worker thread. We can avoid these redundant list and its protection mechanism, if we subclass the work object to encapsulate vblank event parameters.

Signed-off-by Jeykumar Sankaran <[email protected]>

---
 drivers/gpu/drm/msm/msm_drv.c | 70 ++++++++++++-------------------------------
 drivers/gpu/drm/msm/msm_drv.h |  7 -----
 2 files changed, 19 insertions(+), 58 deletions(-)

drivers/gpu/drm/msm/msm_drv.c +15 -32

index 1f384b3..67a96ee 100644
@@ -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 kthread_work work;
 	int crtc_id;
 	bool enable;
+	struct msm_drm_private *priv;
 };
 
 static void vblank_ctrl_worker(struct kthread_work *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;
+	kthread_init_work(&vbl_work->work, vblank_ctrl_worker);

drivers/gpu/drm/msm/msm_drv.c +0 -15

 	return 0;
 }

@@ -269,20 +252,8 @@ 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.
-	 */
-	kthread_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);
-	}
-
 	kthread_flush_worker(&priv->disp_thread.worker);
 	kthread_stop(priv->disp_thread.thread);
 	priv->disp_thread.thread = NULL;

@@ -474,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);
-	kthread_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

index e81b1fa..b91e306 100644
@@ -77,12 +77,6 @@ enum msm_mdp_plane_property {
 	PLANE_PROP_MAX_NUM
 };
 
-struct msm_vblank_ctrl {
-	struct kthread_work work;
-	struct list_head event_list;
-	spinlock_t lock;
-};
-
 #define MSM_GPU_MAX_RINGS 4
 #define MAX_H_TILES_PER_DISPLAY 2
 

@@ -226,7 +220,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

Quoted 10 lines · Jeykumar Sankaran

msm maintains a separate structure to define vblank work definitions and a list to track events submitted to the display worker thread. We can avoid these redundant list and its protection mechanism, if we subclass the work object to encapsulate vblank event parameters.

Signed-off-by Jeykumar Sankaran <[email protected]>

---
 drivers/gpu/drm/msm/msm_drv.c | 70

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

Review

Quoted 4 lines

 drivers/gpu/drm/msm/msm_drv.h |  7 -----
 2 files changed, 19 insertions(+), 58 deletions(-)

(quoted diff) +0 -0

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

Quoted 10 lines

index 1f384b3..67a96ee 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 kthread_work work;
 	int crtc_id;
 	bool enable;
+	struct msm_drm_private *priv;
 };
 
 static void vblank_ctrl_worker(struct kthread_work *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 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;
+	kthread_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;
 

drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 243

Quoted 2 lines

drivers/gpu/drm/msm/msm_drv.c +1 -1

-	kthread_queue_work(&priv->disp_thread.worker, &vbl_ctrl->work);
+	kthread_queue_work(&priv->disp_thread.worker, &vbl_work->work);

So I think this can get even more simplified. In the short term, you can just
use the systemwq to do the enable and disable.

In the long term, the enable_vblank/disable_vblank functions should be optimized so they don't sleep. I took a quick look at them perhaps this is all because of the crtc_lock mutex? That lock seems a bit suspicious to me,
especially being dropped around the pm_runtime calls in _dpu_crtc_vblank_enable_no_lock(). I think we could probably rely on the modeset
locks for some of these functions, and perhaps convert it to a spinlock if we
can't get rid of it entirely.

Sean

Quoted 7 lines

drivers/gpu/drm/msm/msm_drv.c +0 -0

 	return 0;
 }

@@ -269,20 +252,8 @@ 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;

drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device *dev) · line 281

Quoted 10 lines

drivers/gpu/drm/msm/msm_drv.c +0 -8

-	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.
-	 */
-	kthread_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);
-	}
-
 	kthread_flush_worker(&priv->disp_thread.worker);
 	kthread_stop(priv->disp_thread.thread);
 	priv->disp_thread.thread = NULL;

@@ -474,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 -3

 	priv->wq = alloc_ordered_workqueue("msm", 0);
 
 	INIT_LIST_HEAD(&priv->inactive_list);
-	INIT_LIST_HEAD(&priv->vblank_ctrl.event_list);
-	kthread_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.h

Quoted 25 lines

index e81b1fa..b91e306 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 kthread_work work;
-	struct list_head event_list;
-	spinlock_t lock;
-};
-
 #define MSM_GPU_MAX_RINGS 4
 #define MAX_H_TILES_PER_DISPLAY 2
 

@@ -226,7 +220,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

Quoted 12 lines · Sean Paul

Quoted 10 lines · Jeykumar Sankaran

msm maintains a separate structure to define vblank work definitions and a list to track events submitted to the display worker thread. We can avoid these redundant list and its protection mechanism, if we subclass the work object to encapsulate vblank event parameters.

Signed-off-by Jeykumar Sankaran <[email protected]>

---
 drivers/gpu/drm/msm/msm_drv.c | 70

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

Quoted 8 lines

Review

Quoted 4 lines

 drivers/gpu/drm/msm/msm_drv.h |  7 -----
 2 files changed, 19 insertions(+), 58 deletions(-)

(quoted diff) +0 -0

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

drivers/gpu/drm/msm/msm_drv.c

Quoted 3 lines

index 1f384b3..67a96ee 100644

drivers/gpu/drm/msm/msm_drv.c +0 -0

Quoted 25 lines

Quoted 25 lines

static int vblank_ctrl_queue_work(struct msm_drm_private *priv,

drivers/gpu/drm/msm/msm_drv.c +7 -10

 					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;
+	kthread_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

drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 243

Quoted 2 lines

drivers/gpu/drm/msm/msm_drv.c +1 -1

-	kthread_queue_work(&priv->disp_thread.worker, &vbl_ctrl->work);
+	kthread_queue_work(&priv->disp_thread.worker, &vbl_work->work);

So I think this can get even more simplified. In the short term, you can just
use the systemwq to do the enable and disable.

you mean priv->wq?

Quoted 13 lines

In the long term, the enable_vblank/disable_vblank functions should be optimized so they don't sleep. I took a quick look at them perhaps this is
all because of the crtc_lock mutex? That lock seems a bit suspicious to me,
especially being dropped around the pm_runtime calls in _dpu_crtc_vblank_enable_no_lock(). I think we could probably rely on the modeset
locks for some of these functions, and perhaps convert it to a spinlock if
we
can't get rid of it entirely.

crtc_lock has a history of usage in the downstream driver. It was introduced to protect
vblank variables when vblank requests were handled in the user thread (not the display thread). When event threads were introduced to receive encoder events, the lock was further expanded to protect few more vars. It was
also needed to synchronize CRTC accesses between debugfs dump calls and display thread.

Would like to deal with this cleanup bit later once we lose these extra threads.

Thanks and Regards,
Jeykumar S.

Quoted 74 lines

Sean

Quoted 7 lines

drivers/gpu/drm/msm/msm_drv.c +0 -0

 	return 0;
 }

@@ -269,20 +252,8 @@ 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;

drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device *dev) · line 281

Quoted 10 lines

drivers/gpu/drm/msm/msm_drv.c +0 -8

-	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.
-	 */
-	kthread_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);
-	}
-
 	kthread_flush_worker(&priv->disp_thread.worker);
 	kthread_stop(priv->disp_thread.thread);
 	priv->disp_thread.thread = NULL;

@@ -474,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 -3

 	priv->wq = alloc_ordered_workqueue("msm", 0);
 
 	INIT_LIST_HEAD(&priv->inactive_list);
-	INIT_LIST_HEAD(&priv->vblank_ctrl.event_list);
-	kthread_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.h

Quoted 25 lines

index e81b1fa..b91e306 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 kthread_work work;
-	struct list_head event_list;
-	spinlock_t lock;
-};
-
 #define MSM_GPU_MAX_RINGS 4
 #define MAX_H_TILES_PER_DISPLAY 2
 

@@ -226,7 +220,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 · 1 line

Jeykumar S

Quoted 13 lines · Jeykumar Sankaran

Quoted 12 lines · Sean Paul

Quoted 10 lines · Jeykumar Sankaran

msm maintains a separate structure to define vblank work definitions and a list to track events submitted to the display worker thread. We can avoid these redundant list and its protection mechanism, if we subclass the work object to encapsulate vblank event parameters.

Signed-off-by Jeykumar Sankaran <[email protected]>

---
 drivers/gpu/drm/msm/msm_drv.c | 70

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

Quoted 8 lines

Quoted 8 lines

Review

Quoted 4 lines

 drivers/gpu/drm/msm/msm_drv.h |  7 -----
 2 files changed, 19 insertions(+), 58 deletions(-)

(quoted diff) +0 -0

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

drivers/gpu/drm/msm/msm_drv.c

Quoted 3 lines

index 1f384b3..67a96ee 100644

drivers/gpu/drm/msm/msm_drv.c +0 -0

/snip

Quoted 25 lines

Quoted 25 lines

Quoted 25 lines

static int vblank_ctrl_queue_work(struct msm_drm_private *priv,

drivers/gpu/drm/msm/msm_drv.c +7 -10

 					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;
+	kthread_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 4 lines

Quoted 4 lines

drivers/gpu/drm/msm/msm_drv.c u32 msm_readl(const void __iomem *addr) · line 243

Quoted 2 lines

drivers/gpu/drm/msm/msm_drv.c +1 -1

-	kthread_queue_work(&priv->disp_thread.worker, &vbl_ctrl->work);
+	kthread_queue_work(&priv->disp_thread.worker, &vbl_work->work);

So I think this can get even more simplified. In the short term, you

can

Quoted 5 lines

Quoted 2 lines

just
use the systemwq to do the enable and disable.

you mean priv->wq?

I meant the system workqueue, we probably don't need our own for this.

Quoted 3 lines

Quoted 3 lines

In the long term, the enable_vblank/disable_vblank functions should be optimized so they don't sleep. I took a quick look at them perhaps

this

Quoted 2 lines

Quoted 2 lines

is
all because of the crtc_lock mutex? That lock seems a bit suspicious

to

Quoted 3 lines

Quoted 3 lines

me,
especially being dropped around the pm_runtime calls in _dpu_crtc_vblank_enable_no_lock(). I think we could probably rely on

the

Quoted 2 lines

Quoted 2 lines

modeset
locks for some of these functions, and perhaps convert it to a

spinlock

Quoted 5 lines

Quoted 3 lines

if
we
can't get rid of it entirely.

crtc_lock has a history of usage in the downstream driver. It was

introduced

Quoted 4 lines

to protect
vblank variables when vblank requests were handled in the user thread (not the display thread). When event threads were introduced to receive encoder events, the lock was further expanded to protect few more vars.

It

Quoted 3 lines

was
also needed to synchronize CRTC accesses between debugfs dump calls and display thread.

The debugfs case can be solved pretty easily by using the modeset locks. I haven't looked closely at the event threads, could we convert crtc_lock to a
spinlock and then make vblank enable/disable synchronous?

Sean

Quoted 28 lines

Would like to deal with this cleanup bit later once we lose these extra threads.

Thanks and Regards,
Jeykumar S.

Quoted 21 lines

Sean

Quoted 7 lines

drivers/gpu/drm/msm/msm_drv.c +0 -0

 	return 0;
 }

@@ -269,20 +252,8 @@ 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;

drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device *dev) · line 281

Quoted 10 lines

drivers/gpu/drm/msm/msm_drv.c +0 -8

-	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.
-	 */
-	kthread_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);
-	}
-
 	kthread_flush_worker(&priv->disp_thread.worker);
 	kthread_stop(priv->disp_thread.thread);
 	priv->disp_thread.thread = NULL;

@@ -474,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 -3

 	priv->wq = alloc_ordered_workqueue("msm", 0);
 
 	INIT_LIST_HEAD(&priv->inactive_list);
-	INIT_LIST_HEAD(&priv->vblank_ctrl.event_list);
-	kthread_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.h

Quoted 20 lines

index e81b1fa..b91e306 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 kthread_work work;
-	struct list_head event_list;
-	spinlock_t lock;
-};
-
 #define MSM_GPU_MAX_RINGS 4
 #define MAX_H_TILES_PER_DISPLAY 2
 

@@ -226,7 +220,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

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. Single thread is sufficient to do the job without any performance hits.

Signed-off-by Jeykumar Sankaran <[email protected]>

---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  6 +---
 drivers/gpu/drm/msm/msm_drv.c               | 50 ++++++++++++-----------------
 drivers/gpu/drm/msm/msm_drv.h               |  2 +-
 3 files changed, 23 insertions(+), 35 deletions(-)

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +1 -5

index 82c55ef..aff20f5 100644
@@ -753,11 +753,7 @@ 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];
+	disp_thread = &priv->disp_thread;
 
 	/*
 	 * when idle_pc is not supported, process only KICKOFF, STOP and MODESET

drivers/gpu/drm/msm/msm_drv.c +1 -2

index 9c9f7ff..1f384b3 100644
@@ -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);
+	kthread_queue_work(&priv->disp_thread.worker, &vbl_ctrl->work);
 
 	return 0;
 }

drivers/gpu/drm/msm/msm_drv.c +16 -27

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

@@ -537,6 +534,22 @@ static int msm_drm_init(struct device *dev, struct drm_driver *drv)
 	ddev->mode_config.funcs = &mode_config_funcs;
 	ddev->mode_config.helper_private = &mode_config_helper_funcs;
 
+	/* initialize display thread */
+	kthread_init_worker(&priv->disp_thread.worker);
+	priv->disp_thread.dev = ddev;
+	priv->disp_thread.thread = kthread_run(kthread_worker_fn,
+					       &priv->disp_thread.worker,
+					       "disp_thread");
+	if (IS_ERR(priv->disp_thread.thread)) {
+		DRM_DEV_ERROR(dev, "failed to create crtc_commit kthread\n");
+		priv->disp_thread.thread = NULL;
+		goto err_msm_uninit;
+	}
+
+	ret = sched_setscheduler(priv->disp_thread.thread, SCHED_FIFO, &param);
+	if (ret)
+		pr_warn("display thread priority update failed: %d\n", ret);
+
 	/**
 	 * this priority was found during empiric testing to have appropriate
 	 * realtime scheduling to process display updates and interact with

@@ -544,27 +557,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);

drivers/gpu/drm/msm/msm_drv.h +1 -1

index 9d11f32..e81b1fa 100644
@@ -197,7 +197,7 @@ 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 disp_thread;
 	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

Quoted 10 lines · Jeykumar Sankaran

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. Single thread is sufficient to do the job without any performance hits.

Signed-off-by Jeykumar Sankaran <[email protected]>

---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  6 +---
 drivers/gpu/drm/msm/msm_drv.c               | 50

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

Review

Quoted 4 lines

 drivers/gpu/drm/msm/msm_drv.h               |  2 +-
 3 files changed, 23 insertions(+), 35 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c static int dpu_encoder_resource_control(struct

Quoted 4 lines

index 82c55ef..aff20f5 100644

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +0 -0

@@ -753,11 +753,7 @@ static int dpu_encoder_resource_control(struct

drm_encoder *drm_enc,

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,

Quoted 12 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +1 -5

 	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];
+	disp_thread = &priv->disp_thread;
 
 	/*
 	 * when idle_pc is not supported, process only KICKOFF, STOP and

MODESET

Review

Quoted 1 line

(quoted diff) +0 -0

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

drivers/gpu/drm/msm/msm_drv.c static int vblank_ctrl_queue_work(struct

Quoted 4 lines

index 9c9f7ff..1f384b3 100644

drivers/gpu/drm/msm/msm_drv.c +0 -0

@@ -257,8 +257,7 @@ static int vblank_ctrl_queue_work(struct

msm_drm_private *priv,

Quoted 13 lines

drivers/gpu/drm/msm/msm_drv.c +1 -2

 	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);
+	kthread_queue_work(&priv->disp_thread.worker, &vbl_ctrl->work);
 
 	return 0;
 }

@@ -284,14 +283,12 @@ static int msm_drm_uninit(struct device *dev)
 		kfree(vbl_ev);
 	}
 

drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device *dev) · line 287

Quoted 2 lines

drivers/gpu/drm/msm/msm_drv.c +2 -0

+	kthread_flush_worker(&priv->disp_thread.worker);
+	kthread_stop(priv->disp_thread.thread);

I realize this is moving existing code, but is there a race here? You can't have
work enqueued in between the flush and stop?

You might also want to use kthread_destroy_worker to do this work (in a follow-up patch including the event threads too).

drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device *dev) · line 290

Quoted 3 lines

drivers/gpu/drm/msm/msm_drv.c +2 -0

+	priv->disp_thread.thread = NULL;
+
 	/* clean up display commit/event worker threads */

This comment needs updating now

drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device *dev) · line 294

Quoted 3 lines

drivers/gpu/drm/msm/msm_drv.c +0 -2

 	for (i = 0; i < priv->num_crtcs; i++) {
-		if (priv->disp_thread[i].thread) {
-

kthread_flush_worker(&priv->disp_thread[i].worker);

drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device *dev)

Quoted 6 lines

drivers/gpu/drm/msm/msm_drv.c +0 -4

-			kthread_stop(priv->disp_thread[i].thread);
-			priv->disp_thread[i].thread = NULL;
-		}
-
 		if (priv->event_thread[i].thread) {
 

kthread_flush_worker(&priv->event_thread[i].worker);

Quoted 3 lines

drivers/gpu/drm/msm/msm_drv.c +0 -0

 			kthread_stop(priv->event_thread[i].thread);

@@ -537,6 +534,22 @@ static int msm_drm_init(struct device *dev, 

struct

drm_driver *drv)

drivers/gpu/drm/msm/msm_drv.c static int msm_drm_init(struct device *dev, struct drm_driver *drv)

Quoted 11 lines

drivers/gpu/drm/msm/msm_drv.c +8 -0

 	ddev->mode_config.funcs = &mode_config_funcs;
 	ddev->mode_config.helper_private = &mode_config_helper_funcs;
 
+	/* initialize display thread */
+	kthread_init_worker(&priv->disp_thread.worker);
+	priv->disp_thread.dev = ddev;
+	priv->disp_thread.thread = kthread_run(kthread_worker_fn,
+					       &priv->disp_thread.worker,
+					       "disp_thread");
+	if (IS_ERR(priv->disp_thread.thread)) {
+		DRM_DEV_ERROR(dev, "failed to create crtc_commit

kthread\n");

drivers/gpu/drm/msm/msm_drv.c static int msm_drm_init(struct device *dev, struct drm_driver *drv)

Quoted 5 lines

drivers/gpu/drm/msm/msm_drv.c +5 -0

+		priv->disp_thread.thread = NULL;
+		goto err_msm_uninit;
+	}
+
+	ret = sched_setscheduler(priv->disp_thread.thread, SCHED_FIFO,

&param);

drivers/gpu/drm/msm/msm_drv.c

Quoted 2 lines

drivers/gpu/drm/msm/msm_drv.c +2 -0

+	if (ret)
+		pr_warn("display thread priority update failed: %d\n",

ret);

drivers/gpu/drm/msm/msm_drv.c static int msm_drm_init(struct device *dev, struct drm_driver *drv)

Quoted 3 lines

drivers/gpu/drm/msm/msm_drv.c +1 -0

+
 	/**
 	 * this priority was found during empiric testing to have

appropriate

Quoted 1 line

* realtime scheduling to process display updates and interact

with

Quoted 2 lines

drivers/gpu/drm/msm/msm_drv.c +0 -0

@@ -544,27 +557,6 @@ static int msm_drm_init(struct device *dev, 

struct

drm_driver *drv)

drivers/gpu/drm/msm/msm_drv.c static int msm_drm_init(struct device *dev, struct drm_driver *drv)

Quoted 12 lines

drivers/gpu/drm/msm/msm_drv.c +0 -9

 	 */
 	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);

drivers/gpu/drm/msm/msm_drv.c

Quoted 2 lines

drivers/gpu/drm/msm/msm_drv.c +0 -2

-		if (IS_ERR(priv->disp_thread[i].thread)) {
-			DRM_DEV_ERROR(dev, "failed to create crtc_commit

kthread\n");

drivers/gpu/drm/msm/msm_drv.c static int msm_drm_init(struct device *dev, struct drm_driver *drv)

Quoted 8 lines

drivers/gpu/drm/msm/msm_drv.c +0 -8

-			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",

Review

Quoted 6 lines

drivers/gpu/drm/msm/msm_drv.c +0 -2

-				 ret);
-
 		/* initialize event thread */
 		priv->event_thread[i].crtc_id = priv->crtcs[i]->base.id;
 		kthread_init_worker(&priv->event_thread[i].worker);

(quoted diff) +0 -0

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

Quoted 13 lines

index 9d11f32..e81b1fa 100644

drivers/gpu/drm/msm/msm_drv.h +1 -2

@@ -197,7 +197,7 @@ 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 disp_thread;
 	struct msm_drm_thread event_thread[MAX_CRTCS];
 
 	unsigned int num_encoders;
--

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

Quoted 10 lines · Jeykumar Sankaran

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. Single thread is sufficient to do the job without any performance hits.

Signed-off-by Jeykumar Sankaran <[email protected]>

---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  6 +---
 drivers/gpu/drm/msm/msm_drv.c               | 50

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

Review

Quoted 4 lines

 drivers/gpu/drm/msm/msm_drv.h               |  2 +-
 3 files changed, 23 insertions(+), 35 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c static int dpu_encoder_resource_control(struct

Quoted 4 lines

index 82c55ef..aff20f5 100644

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +0 -0

@@ -753,11 +753,7 @@ static int dpu_encoder_resource_control(struct

drm_encoder *drm_enc,

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,

Quoted 12 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +1 -5

 	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];
+	disp_thread = &priv->disp_thread;
 
 	/*
 	 * when idle_pc is not supported, process only KICKOFF, STOP and

MODESET

Review

Quoted 1 line

(quoted diff) +0 -0

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

drivers/gpu/drm/msm/msm_drv.c static int vblank_ctrl_queue_work(struct

Quoted 4 lines

index 9c9f7ff..1f384b3 100644

drivers/gpu/drm/msm/msm_drv.c +0 -0

@@ -257,8 +257,7 @@ static int vblank_ctrl_queue_work(struct

msm_drm_private *priv,

Quoted 13 lines

drivers/gpu/drm/msm/msm_drv.c +1 -2

 	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);
+	kthread_queue_work(&priv->disp_thread.worker, &vbl_ctrl->work);
 
 	return 0;
 }

@@ -284,14 +283,12 @@ static int msm_drm_uninit(struct device *dev)
 		kfree(vbl_ev);
 	}
 

drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device *dev) · line 287

Quoted 2 lines

drivers/gpu/drm/msm/msm_drv.c +2 -0

+	kthread_flush_worker(&priv->disp_thread.worker);
+	kthread_stop(priv->disp_thread.thread);

I realize this is moving existing code, but is there a race here? You can't have
work enqueued in between the flush and stop?

You might also want to use kthread_destroy_worker to do this work (in a follow-up patch including the event threads too).

drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device *dev) · line 290

Quoted 3 lines

drivers/gpu/drm/msm/msm_drv.c +2 -0

+	priv->disp_thread.thread = NULL;
+
 	/* clean up display commit/event worker threads */

This comment needs updating now

drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device *dev) · line 294

Quoted 3 lines

drivers/gpu/drm/msm/msm_drv.c +0 -2

 	for (i = 0; i < priv->num_crtcs; i++) {
-		if (priv->disp_thread[i].thread) {
-

kthread_flush_worker(&priv->disp_thread[i].worker);

drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device *dev)

Quoted 6 lines

drivers/gpu/drm/msm/msm_drv.c +0 -4

-			kthread_stop(priv->disp_thread[i].thread);
-			priv->disp_thread[i].thread = NULL;
-		}
-
 		if (priv->event_thread[i].thread) {
 

kthread_flush_worker(&priv->event_thread[i].worker);

Quoted 3 lines

drivers/gpu/drm/msm/msm_drv.c +0 -0

 			kthread_stop(priv->event_thread[i].thread);

@@ -537,6 +534,22 @@ static int msm_drm_init(struct device *dev, 

struct

drm_driver *drv)

drivers/gpu/drm/msm/msm_drv.c static int msm_drm_init(struct device *dev, struct drm_driver *drv)

Quoted 11 lines

drivers/gpu/drm/msm/msm_drv.c +8 -0

 	ddev->mode_config.funcs = &mode_config_funcs;
 	ddev->mode_config.helper_private = &mode_config_helper_funcs;
 
+	/* initialize display thread */
+	kthread_init_worker(&priv->disp_thread.worker);
+	priv->disp_thread.dev = ddev;
+	priv->disp_thread.thread = kthread_run(kthread_worker_fn,
+					       &priv->disp_thread.worker,
+					       "disp_thread");
+	if (IS_ERR(priv->disp_thread.thread)) {
+		DRM_DEV_ERROR(dev, "failed to create crtc_commit

kthread\n");

drivers/gpu/drm/msm/msm_drv.c static int msm_drm_init(struct device *dev, struct drm_driver *drv)

Quoted 5 lines

drivers/gpu/drm/msm/msm_drv.c +5 -0

+		priv->disp_thread.thread = NULL;
+		goto err_msm_uninit;
+	}
+
+	ret = sched_setscheduler(priv->disp_thread.thread, SCHED_FIFO,

&param);

drivers/gpu/drm/msm/msm_drv.c

Quoted 2 lines

drivers/gpu/drm/msm/msm_drv.c +2 -0

+	if (ret)
+		pr_warn("display thread priority update failed: %d\n",

ret);

drivers/gpu/drm/msm/msm_drv.c static int msm_drm_init(struct device *dev, struct drm_driver *drv)

Quoted 3 lines

drivers/gpu/drm/msm/msm_drv.c +1 -0

+
 	/**
 	 * this priority was found during empiric testing to have

appropriate

Quoted 1 line

* realtime scheduling to process display updates and interact

with

Quoted 2 lines

drivers/gpu/drm/msm/msm_drv.c +0 -0

@@ -544,27 +557,6 @@ static int msm_drm_init(struct device *dev, 

struct

drm_driver *drv)

drivers/gpu/drm/msm/msm_drv.c static int msm_drm_init(struct device *dev, struct drm_driver *drv)

Quoted 12 lines

drivers/gpu/drm/msm/msm_drv.c +0 -9

 	 */
 	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);

drivers/gpu/drm/msm/msm_drv.c

Quoted 2 lines

drivers/gpu/drm/msm/msm_drv.c +0 -2

-		if (IS_ERR(priv->disp_thread[i].thread)) {
-			DRM_DEV_ERROR(dev, "failed to create crtc_commit

kthread\n");

drivers/gpu/drm/msm/msm_drv.c static int msm_drm_init(struct device *dev, struct drm_driver *drv)

Quoted 8 lines

drivers/gpu/drm/msm/msm_drv.c +0 -8

-			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",

Review

Quoted 6 lines

drivers/gpu/drm/msm/msm_drv.c +0 -2

-				 ret);
-
 		/* initialize event thread */
 		priv->event_thread[i].crtc_id = priv->crtcs[i]->base.id;
 		kthread_init_worker(&priv->event_thread[i].worker);

(quoted diff) +0 -0

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

Quoted 13 lines

index 9d11f32..e81b1fa 100644

drivers/gpu/drm/msm/msm_drv.h +1 -2

@@ -197,7 +197,7 @@ 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 disp_thread;
 	struct msm_drm_thread event_thread[MAX_CRTCS];
 
 	unsigned int num_encoders;
--

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

Quoted 55 lines · Sean Paul

Quoted 10 lines · Jeykumar Sankaran

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. Single thread is sufficient to do the job without any performance hits.

Signed-off-by Jeykumar Sankaran <[email protected]>

---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  6 +---
 drivers/gpu/drm/msm/msm_drv.c               | 50

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

Review

Quoted 4 lines

 drivers/gpu/drm/msm/msm_drv.h               |  2 +-
 3 files changed, 23 insertions(+), 35 deletions(-)

(quoted diff) +0 -0

b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c static int dpu_encoder_resource_control(struct

Quoted 4 lines

index 82c55ef..aff20f5 100644

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +0 -0

@@ -753,11 +753,7 @@ static int dpu_encoder_resource_control(struct

drm_encoder *drm_enc,

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c static int dpu_encoder_resource_control(struct drm_encoder *drm_enc,

Quoted 12 lines

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +1 -5

 	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];
+	disp_thread = &priv->disp_thread;
 
 	/*
 	 * when idle_pc is not supported, process only KICKOFF, STOP and

MODESET

Review

Quoted 1 line

(quoted diff) +0 -0

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

drivers/gpu/drm/msm/msm_drv.c static int vblank_ctrl_queue_work(struct

Quoted 4 lines

index 9c9f7ff..1f384b3 100644

drivers/gpu/drm/msm/msm_drv.c +0 -0

@@ -257,8 +257,7 @@ static int vblank_ctrl_queue_work(struct

msm_drm_private *priv,

Quoted 13 lines

drivers/gpu/drm/msm/msm_drv.c +1 -2

 	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);
+	kthread_queue_work(&priv->disp_thread.worker, &vbl_ctrl->work);
 
 	return 0;
 }

@@ -284,14 +283,12 @@ static int msm_drm_uninit(struct device *dev)
 		kfree(vbl_ev);
 	}
 

Quoted 6 lines

drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device *dev) · line 287

Quoted 2 lines

drivers/gpu/drm/msm/msm_drv.c +2 -0

+	kthread_flush_worker(&priv->disp_thread.worker);
+	kthread_stop(priv->disp_thread.thread);

I realize this is moving existing code, but is there a race here? You can't have
work enqueued in between the flush and stop?

I looked further into this comment. Ideally, we call into msm_unbind only when
the device is released and we release the device only on the last close of the drm device. So the userspace doesn't have any device handle to make ioctl calls, which could queue jobs to this queue. Since we are making sure to flush out the last job already on the queue, we can safely call the kthread_stop here.

Thanks and Regards,
Jeykumar S.

Quoted 108 lines

You might also want to use kthread_destroy_worker to do this work (in a follow-up patch including the event threads too).

drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device *dev) · line 290

Quoted 3 lines

drivers/gpu/drm/msm/msm_drv.c +2 -0

+	priv->disp_thread.thread = NULL;
+
 	/* clean up display commit/event worker threads */

This comment needs updating now

drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device *dev) · line 294

Quoted 3 lines

drivers/gpu/drm/msm/msm_drv.c +0 -2

 	for (i = 0; i < priv->num_crtcs; i++) {
-		if (priv->disp_thread[i].thread) {
-

kthread_flush_worker(&priv->disp_thread[i].worker);

drivers/gpu/drm/msm/msm_drv.c static int msm_drm_uninit(struct device *dev)

Quoted 6 lines

drivers/gpu/drm/msm/msm_drv.c +0 -4

-			kthread_stop(priv->disp_thread[i].thread);
-			priv->disp_thread[i].thread = NULL;
-		}
-
 		if (priv->event_thread[i].thread) {
 

kthread_flush_worker(&priv->event_thread[i].worker);

Quoted 3 lines

drivers/gpu/drm/msm/msm_drv.c +0 -0

 			kthread_stop(priv->event_thread[i].thread);

@@ -537,6 +534,22 @@ static int msm_drm_init(struct device *dev,

struct

drm_driver *drv)

drivers/gpu/drm/msm/msm_drv.c static int msm_drm_init(struct device *dev, struct drm_driver *drv)

Quoted 11 lines

drivers/gpu/drm/msm/msm_drv.c +8 -0

 	ddev->mode_config.funcs = &mode_config_funcs;
 	ddev->mode_config.helper_private = &mode_config_helper_funcs;
 
+	/* initialize display thread */
+	kthread_init_worker(&priv->disp_thread.worker);
+	priv->disp_thread.dev = ddev;
+	priv->disp_thread.thread = kthread_run(kthread_worker_fn,
+					       &priv->disp_thread.worker,
+					       "disp_thread");
+	if (IS_ERR(priv->disp_thread.thread)) {
+		DRM_DEV_ERROR(dev, "failed to create crtc_commit

kthread\n");

drivers/gpu/drm/msm/msm_drv.c static int msm_drm_init(struct device *dev, struct drm_driver *drv)

Quoted 5 lines

drivers/gpu/drm/msm/msm_drv.c +5 -0

+		priv->disp_thread.thread = NULL;
+		goto err_msm_uninit;
+	}
+
+	ret = sched_setscheduler(priv->disp_thread.thread, SCHED_FIFO,

&param);

drivers/gpu/drm/msm/msm_drv.c

Quoted 2 lines

drivers/gpu/drm/msm/msm_drv.c +2 -0

+	if (ret)
+		pr_warn("display thread priority update failed: %d\n",

ret);

drivers/gpu/drm/msm/msm_drv.c static int msm_drm_init(struct device *dev, struct drm_driver *drv)

Quoted 3 lines

drivers/gpu/drm/msm/msm_drv.c +1 -0

+
 	/**
 	 * this priority was found during empiric testing to have

appropriate

Quoted 1 line

* realtime scheduling to process display updates and interact

with

Quoted 2 lines

drivers/gpu/drm/msm/msm_drv.c +0 -0

@@ -544,27 +557,6 @@ static int msm_drm_init(struct device *dev,

struct

drm_driver *drv)

drivers/gpu/drm/msm/msm_drv.c static int msm_drm_init(struct device *dev, struct drm_driver *drv)

Quoted 12 lines

drivers/gpu/drm/msm/msm_drv.c +0 -9

 	 */
 	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);

drivers/gpu/drm/msm/msm_drv.c

Quoted 2 lines

drivers/gpu/drm/msm/msm_drv.c +0 -2

-		if (IS_ERR(priv->disp_thread[i].thread)) {
-			DRM_DEV_ERROR(dev, "failed to create crtc_commit

kthread\n");

drivers/gpu/drm/msm/msm_drv.c static int msm_drm_init(struct device *dev, struct drm_driver *drv)

Quoted 8 lines

drivers/gpu/drm/msm/msm_drv.c +0 -8

-			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",

Review

Quoted 6 lines

drivers/gpu/drm/msm/msm_drv.c +0 -2

-				 ret);
-
 		/* initialize event thread */
 		priv->event_thread[i].crtc_id = priv->crtcs[i]->base.id;
 		kthread_init_worker(&priv->event_thread[i].worker);

(quoted diff) +0 -0

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

Quoted 13 lines

index 9d11f32..e81b1fa 100644

drivers/gpu/drm/msm/msm_drv.h +1 -2

@@ -197,7 +197,7 @@ 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 disp_thread;
 	struct msm_drm_thread event_thread[MAX_CRTCS];
 
 	unsigned int num_encoders;
--

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 · 1 line

Jeykumar S

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. Single thread is sufficient to do the job without any performance hits.

Signed-off-by Jeykumar Sankaran <[email protected]>

---
 drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c |  6 +---
 drivers/gpu/drm/msm/msm_drv.c               | 50 ++++++++++++-----------------
 drivers/gpu/drm/msm/msm_drv.h               |  2 +-
 3 files changed, 23 insertions(+), 35 deletions(-)

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c +1 -5

index 82c55ef..aff20f5 100644
@@ -753,11 +753,7 @@ 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];
+	disp_thread = &priv->disp_thread;
 
 	/*
 	 * when idle_pc is not supported, process only KICKOFF, STOP and MODESET

drivers/gpu/drm/msm/msm_drv.c +1 -2

index 9c9f7ff..1f384b3 100644
@@ -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);
+	kthread_queue_work(&priv->disp_thread.worker, &vbl_ctrl->work);
 
 	return 0;
 }

drivers/gpu/drm/msm/msm_drv.c +0 -6

 	/* 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);
-			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);