[PATCH 0/3] drm/msm: kthread_worker conversion

[PATCH 0/3] drm/msm: kthread_worker conversion

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

In particular, converting the async atomic commit (for cursor updates,
etc) to SCHED_FIFO kthread_worker helps with some cases where we
wouldn't manage to flush the updates within the 1ms-before-vblank
deadline resulting in fps drops when there is cursor movement.

Rob Clark (3):
  drm/msm/gpu: Convert retire/recover work to kthread_worker
  drm/msm/kms: Update msm_kms_init/destroy
  drm/msm/atomic: Convert to per-CRTC kthread_work

 drivers/gpu/drm/msm/adreno/a5xx_gpu.c     |  3 +--
 drivers/gpu/drm/msm/adreno/a5xx_preempt.c |  6 ++---
 drivers/gpu/drm/msm/adreno/a6xx_gmu.c     |  4 +--
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c     |  4 +--
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   |  8 +++++-
 drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c  |  8 +++++-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c  | 11 ++++++---
 drivers/gpu/drm/msm/disp/mdp_kms.h        |  9 +++++--
 drivers/gpu/drm/msm/msm_atomic.c          | 25 +++++++++++++++----
 drivers/gpu/drm/msm/msm_drv.h             |  3 ++-
 drivers/gpu/drm/msm/msm_gpu.c             | 30 +++++++++++++++--------
 drivers/gpu/drm/msm/msm_gpu.h             | 13 +++++++---
 drivers/gpu/drm/msm/msm_kms.h             | 23 ++++++++++++++---
 13 files changed, 104 insertions(+), 43 deletions(-)

-- 
2.26.2

[PATCH 1/3] drm/msm/gpu: Convert retire/recover work to kthread_worker

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/adreno/a5xx_gpu.c     |  3 +--
 drivers/gpu/drm/msm/adreno/a5xx_preempt.c |  6 ++---
 drivers/gpu/drm/msm/adreno/a6xx_gmu.c     |  4 +--
 drivers/gpu/drm/msm/adreno/a6xx_gpu.c     |  4 +--
 drivers/gpu/drm/msm/msm_gpu.c             | 30 +++++++++++++++--------
 drivers/gpu/drm/msm/msm_gpu.h             | 13 +++++++---
 6 files changed, 34 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
index 2befaf304f04..b0005ccd81c6 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_gpu.c
@@ -1056,7 +1056,6 @@ static void a5xx_gpmu_err_irq(struct msm_gpu *gpu)
 static void a5xx_fault_detect_irq(struct msm_gpu *gpu)
 {
 	struct drm_device *dev = gpu->dev;
-	struct msm_drm_private *priv = dev->dev_private;
 	struct msm_ringbuffer *ring = gpu->funcs->active_ring(gpu);
 
 	DRM_DEV_ERROR(dev->dev, "gpu fault ring %d fence %x status %8.8X rb %4.4x/%4.4x ib1 %16.16llX/%4.4x ib2 %16.16llX/%4.4x\n",
@@ -1072,7 +1071,7 @@ static void a5xx_fault_detect_irq(struct msm_gpu *gpu)
 	/* Turn off the hangcheck timer to keep it from bothering us */
 	del_timer(&gpu->hangcheck_timer);
 
-	queue_work(priv->wq, &gpu->recover_work);
+	kthread_queue_work(gpu->worker, &gpu->recover_work);
 }
 
 #define RBBM_ERROR_MASK \
diff --git a/drivers/gpu/drm/msm/adreno/a5xx_preempt.c b/drivers/gpu/drm/msm/adreno/a5xx_preempt.c
index 183de1139eeb..42eaef7ad7c7 100644
--- a/drivers/gpu/drm/msm/adreno/a5xx_preempt.c
+++ b/drivers/gpu/drm/msm/adreno/a5xx_preempt.c
@@ -78,13 +78,12 @@ static void a5xx_preempt_timer(struct timer_list *t)
 	struct a5xx_gpu *a5xx_gpu = from_timer(a5xx_gpu, t, preempt_timer);
 	struct msm_gpu *gpu = &a5xx_gpu->base.base;
 	struct drm_device *dev = gpu->dev;
-	struct msm_drm_private *priv = dev->dev_private;
 
 	if (!try_preempt_state(a5xx_gpu, PREEMPT_TRIGGERED, PREEMPT_FAULTED))
 		return;
 
 	DRM_DEV_ERROR(dev->dev, "%s: preemption timed out\n", gpu->name);
-	queue_work(priv->wq, &gpu->recover_work);
+	kthread_queue_work(gpu->worker, &gpu->recover_work);
 }
 
 /* Try to trigger a preemption switch */
@@ -162,7 +161,6 @@ void a5xx_preempt_irq(struct msm_gpu *gpu)
 	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
 	struct a5xx_gpu *a5xx_gpu = to_a5xx_gpu(adreno_gpu);
 	struct drm_device *dev = gpu->dev;
-	struct msm_drm_private *priv = dev->dev_private;
 
 	if (!try_preempt_state(a5xx_gpu, PREEMPT_TRIGGERED, PREEMPT_PENDING))
 		return;
@@ -181,7 +179,7 @@ void a5xx_preempt_irq(struct msm_gpu *gpu)
 		set_preempt_state(a5xx_gpu, PREEMPT_FAULTED);
 		DRM_DEV_ERROR(dev->dev, "%s: Preemption failed to complete\n",
 			gpu->name);
-		queue_work(priv->wq, &gpu->recover_work);
+		kthread_queue_work(gpu->worker, &gpu->recover_work);
 		return;
 	}
 
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
index 491fee410daf..e6703ae98760 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gmu.c
@@ -19,8 +19,6 @@ static void a6xx_gmu_fault(struct a6xx_gmu *gmu)
 	struct a6xx_gpu *a6xx_gpu = container_of(gmu, struct a6xx_gpu, gmu);
 	struct adreno_gpu *adreno_gpu = &a6xx_gpu->base;
 	struct msm_gpu *gpu = &adreno_gpu->base;
-	struct drm_device *dev = gpu->dev;
-	struct msm_drm_private *priv = dev->dev_private;
 
 	/* FIXME: add a banner here */
 	gmu->hung = true;
@@ -29,7 +27,7 @@ static void a6xx_gmu_fault(struct a6xx_gmu *gmu)
 	del_timer(&gpu->hangcheck_timer);
 
 	/* Queue the GPU handler because we need to treat this as a recovery */
-	queue_work(priv->wq, &gpu->recover_work);
+	kthread_queue_work(gpu->worker, &gpu->recover_work);
 }
 
 static irqreturn_t a6xx_gmu_irq(int irq, void *data)
diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 5dddb9163bd3..2f236aadfa9c 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -965,8 +965,6 @@ static void a6xx_fault_detect_irq(struct msm_gpu *gpu)
 {
 	struct adreno_gpu *adreno_gpu = to_adreno_gpu(gpu);
 	struct a6xx_gpu *a6xx_gpu = to_a6xx_gpu(adreno_gpu);
-	struct drm_device *dev = gpu->dev;
-	struct msm_drm_private *priv = dev->dev_private;
 	struct msm_ringbuffer *ring = gpu->funcs->active_ring(gpu);
 
 	/*
@@ -989,7 +987,7 @@ static void a6xx_fault_detect_irq(struct msm_gpu *gpu)
 	/* Turn off the hangcheck timer to keep it from bothering us */
 	del_timer(&gpu->hangcheck_timer);
 
-	queue_work(priv->wq, &gpu->recover_work);
+	kthread_queue_work(gpu->worker, &gpu->recover_work);
 }
 
 static irqreturn_t a6xx_irq(struct msm_gpu *gpu)
diff --git a/drivers/gpu/drm/msm/msm_gpu.c b/drivers/gpu/drm/msm/msm_gpu.c
index 30ba3beaad0a..7f4a9466e424 100644
--- a/drivers/gpu/drm/msm/msm_gpu.c
+++ b/drivers/gpu/drm/msm/msm_gpu.c
@@ -448,7 +448,7 @@ find_submit(struct msm_ringbuffer *ring, uint32_t fence)
 
 static void retire_submits(struct msm_gpu *gpu);
 
-static void recover_worker(struct work_struct *work)
+static void recover_worker(struct kthread_work *work)
 {
 	struct msm_gpu *gpu = container_of(work, struct msm_gpu, recover_work);
 	struct drm_device *dev = gpu->dev;
@@ -560,7 +560,6 @@ static void hangcheck_handler(struct timer_list *t)
 {
 	struct msm_gpu *gpu = from_timer(gpu, t, hangcheck_timer);
 	struct drm_device *dev = gpu->dev;
-	struct msm_drm_private *priv = dev->dev_private;
 	struct msm_ringbuffer *ring = gpu->funcs->active_ring(gpu);
 	uint32_t fence = ring->memptrs->fence;
 
@@ -577,7 +576,7 @@ static void hangcheck_handler(struct timer_list *t)
 		DRM_DEV_ERROR(dev->dev, "%s:     submitted fence: %u\n",
 				gpu->name, ring->seqno);
 
-		queue_work(priv->wq, &gpu->recover_work);
+		kthread_queue_work(gpu->worker, &gpu->recover_work);
 	}
 
 	/* if still more pending work, reset the hangcheck timer: */
@@ -585,7 +584,7 @@ static void hangcheck_handler(struct timer_list *t)
 		hangcheck_timer_reset(gpu);
 
 	/* workaround for missing irq: */
-	queue_work(priv->wq, &gpu->retire_work);
+	kthread_queue_work(gpu->worker, &gpu->retire_work);
 }
 
 /*
@@ -760,7 +759,7 @@ static void retire_submits(struct msm_gpu *gpu)
 	}
 }
 
-static void retire_worker(struct work_struct *work)
+static void retire_worker(struct kthread_work *work)
 {
 	struct msm_gpu *gpu = container_of(work, struct msm_gpu, retire_work);
 	int i;
@@ -774,8 +773,7 @@ static void retire_worker(struct work_struct *work)
 /* call from irq handler to schedule work to retire bo's */
 void msm_gpu_retire(struct msm_gpu *gpu)
 {
-	struct msm_drm_private *priv = gpu->dev->dev_private;
-	queue_work(priv->wq, &gpu->retire_work);
+	kthread_queue_work(gpu->worker, &gpu->retire_work);
 	update_sw_cntrs(gpu);
 }
 
@@ -901,10 +899,18 @@ int msm_gpu_init(struct drm_device *drm, struct platform_device *pdev,
 	gpu->funcs = funcs;
 	gpu->name = name;
 
-	INIT_LIST_HEAD(&gpu->active_list);
-	INIT_WORK(&gpu->retire_work, retire_worker);
-	INIT_WORK(&gpu->recover_work, recover_worker);
+	gpu->worker = kthread_create_worker(0, "%s-worker", gpu->name);
+	if (IS_ERR(gpu->worker)) {
+		ret = PTR_ERR(gpu->worker);
+		gpu->worker = NULL;
+		goto fail;
+	}
 
+	sched_set_fifo_low(gpu->worker->task);
+
+	INIT_LIST_HEAD(&gpu->active_list);
+	kthread_init_work(&gpu->retire_work, retire_worker);
+	kthread_init_work(&gpu->recover_work, recover_worker);
 
 	timer_setup(&gpu->hangcheck_timer, hangcheck_handler, 0);
 
@@ -1037,4 +1043,8 @@ void msm_gpu_cleanup(struct msm_gpu *gpu)
 		gpu->aspace->mmu->funcs->detach(gpu->aspace->mmu);
 		msm_gem_address_space_put(gpu->aspace);
 	}
+
+	if (gpu->worker) {
+		kthread_destroy_worker(gpu->worker);
+	}
 }
diff --git a/drivers/gpu/drm/msm/msm_gpu.h b/drivers/gpu/drm/msm/msm_gpu.h
index 1806e87600c0..09938ae114ec 100644
--- a/drivers/gpu/drm/msm/msm_gpu.h
+++ b/drivers/gpu/drm/msm/msm_gpu.h
@@ -106,9 +106,6 @@ struct msm_gpu {
 	/* number of GPU hangs (for all contexts) */
 	int global_faults;
 
-	/* worker for handling active-list retiring: */
-	struct work_struct retire_work;
-
 	void __iomem *mmio;
 	int irq;
 
@@ -137,7 +134,15 @@ struct msm_gpu {
 #define DRM_MSM_HANGCHECK_PERIOD 500 /* in ms */
 #define DRM_MSM_HANGCHECK_JIFFIES msecs_to_jiffies(DRM_MSM_HANGCHECK_PERIOD)
 	struct timer_list hangcheck_timer;
-	struct work_struct recover_work;
+
+	/* work for handling GPU recovery: */
+	struct kthread_work recover_work;
+
+	/* work for handling active-list retiring: */
+	struct kthread_work retire_work;
+
+	/* worker for retire/recover: */
+	struct kthread_worker *worker;
 
 	struct drm_gem_object *memptrs_bo;
 
-- 
2.26.2

[PATCH 2/3] drm/msm/kms: Update msm_kms_init/destroy

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Add msm_kms_destroy() and add err return from msm_kms_init().  Prep work
for next patch.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c  |  8 +++++++-
 drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c |  8 +++++++-
 drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c | 11 ++++++++---
 drivers/gpu/drm/msm/disp/mdp_kms.h       |  9 +++++++--
 drivers/gpu/drm/msm/msm_kms.h            |  8 +++++++-
 5 files changed, 36 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index d93c44f6996d..b77d1a9ace2b 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -718,6 +718,8 @@ static void dpu_kms_destroy(struct msm_kms *kms)
 	dpu_kms = to_dpu_kms(kms);
 
 	_dpu_kms_hw_destroy(dpu_kms);
+
+	msm_kms_destroy(&dpu_kms->base);
 }
 
 static void _dpu_kms_set_encoder_mode(struct msm_kms *kms,
@@ -1108,7 +1110,11 @@ static int dpu_bind(struct device *dev, struct device *master, void *data)
 
 	platform_set_drvdata(pdev, dpu_kms);
 
-	msm_kms_init(&dpu_kms->base, &kms_funcs);
+	ret = msm_kms_init(&dpu_kms->base, &kms_funcs);
+	if (ret) {
+		DPU_ERROR("failed to init kms, ret=%d\n", ret);
+		goto err;
+	}
 	dpu_kms->dev = ddev;
 	dpu_kms->pdev = pdev;
 
diff --git a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
index dbf8d429223e..3d729270bde1 100644
--- a/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c
@@ -175,6 +175,8 @@ static void mdp4_destroy(struct msm_kms *kms)
 	if (mdp4_kms->rpm_enabled)
 		pm_runtime_disable(dev);
 
+	mdp_kms_destroy(&mdp4_kms->base);
+
 	kfree(mdp4_kms);
 }
 
@@ -427,7 +429,11 @@ struct msm_kms *mdp4_kms_init(struct drm_device *dev)
 		goto fail;
 	}
 
-	mdp_kms_init(&mdp4_kms->base, &kms_funcs);
+	ret = mdp_kms_init(&mdp4_kms->base, &kms_funcs);
+	if (ret) {
+		DRM_DEV_ERROR(dev->dev, "failed to init kms\n");
+		goto fail;
+	}
 
 	kms = &mdp4_kms->base.base;
 
diff --git a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
index e193865ce9a2..b3eecf869477 100644
--- a/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
+++ b/drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c
@@ -232,6 +232,8 @@ static void mdp5_kms_destroy(struct msm_kms *kms)
 		aspace->mmu->funcs->detach(aspace->mmu);
 		msm_gem_address_space_put(aspace);
 	}
+
+	mdp_kms_destroy(&mdp5_kms->base);
 }
 
 #ifdef CONFIG_DEBUG_FS
@@ -592,11 +594,14 @@ struct msm_kms *mdp5_kms_init(struct drm_device *dev)
 		return NULL;
 
 	mdp5_kms = to_mdp5_kms(to_mdp_kms(kms));
-
-	mdp_kms_init(&mdp5_kms->base, &kms_funcs);
-
 	pdev = mdp5_kms->pdev;
 
+	ret = mdp_kms_init(&mdp5_kms->base, &kms_funcs);
+	if (ret) {
+		DRM_DEV_ERROR(&pdev->dev, "failed to init kms\n");
+		goto fail;
+	}
+
 	irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
 	if (irq < 0) {
 		ret = irq;
diff --git a/drivers/gpu/drm/msm/disp/mdp_kms.h b/drivers/gpu/drm/msm/disp/mdp_kms.h
index 1535c5618491..b0286d5d5130 100644
--- a/drivers/gpu/drm/msm/disp/mdp_kms.h
+++ b/drivers/gpu/drm/msm/disp/mdp_kms.h
@@ -36,12 +36,17 @@ struct mdp_kms {
 };
 #define to_mdp_kms(x) container_of(x, struct mdp_kms, base)
 
-static inline void mdp_kms_init(struct mdp_kms *mdp_kms,
+static inline int mdp_kms_init(struct mdp_kms *mdp_kms,
 		const struct mdp_kms_funcs *funcs)
 {
 	mdp_kms->funcs = funcs;
 	INIT_LIST_HEAD(&mdp_kms->irq_list);
-	msm_kms_init(&mdp_kms->base, &funcs->base);
+	return msm_kms_init(&mdp_kms->base, &funcs->base);
+}
+
+static inline void mdp_kms_destroy(struct mdp_kms *mdp_kms)
+{
+	msm_kms_destroy(&mdp_kms->base);
 }
 
 /*
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index 1cbef6b200b7..0be9e6487556 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -160,7 +160,7 @@ struct msm_kms {
 	struct msm_pending_timer pending_timers[MAX_CRTCS];
 };
 
-static inline void msm_kms_init(struct msm_kms *kms,
+static inline int msm_kms_init(struct msm_kms *kms,
 		const struct msm_kms_funcs *funcs)
 {
 	unsigned i;
@@ -170,6 +170,12 @@ static inline void msm_kms_init(struct msm_kms *kms,
 
 	for (i = 0; i < ARRAY_SIZE(kms->pending_timers); i++)
 		msm_atomic_init_pending_timer(&kms->pending_timers[i], kms, i);
+
+	return 0;
+}
+
+static inline void msm_kms_destroy(struct msm_kms *kms)
+{
 }
 
 struct msm_kms *mdp4_kms_init(struct drm_device *dev);
-- 
2.26.2

[PATCH 3/3] drm/msm/atomic: Convert to per-CRTC kthread_work

[Rob Clark]

From: Rob Clark <robdclark@chromium.org>

Use a SCHED_FIFO kthread_worker for async atomic commits.  We have a
hard deadline if we don't want to miss a frame.

Signed-off-by: Rob Clark <robdclark@chromium.org>
---
 drivers/gpu/drm/msm/msm_atomic.c | 25 ++++++++++++++++++++-----
 drivers/gpu/drm/msm/msm_drv.h    |  3 ++-
 drivers/gpu/drm/msm/msm_kms.h    | 17 +++++++++++++----
 3 files changed, 35 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_atomic.c b/drivers/gpu/drm/msm/msm_atomic.c
index 561bfa48841c..484438f1e028 100644
--- a/drivers/gpu/drm/msm/msm_atomic.c
+++ b/drivers/gpu/drm/msm/msm_atomic.c
@@ -103,14 +103,13 @@ static enum hrtimer_restart msm_atomic_pending_timer(struct hrtimer *t)
 {
 	struct msm_pending_timer *timer = container_of(t,
 			struct msm_pending_timer, timer);
-	struct msm_drm_private *priv = timer->kms->dev->dev_private;
 
-	queue_work(priv->wq, &timer->work);
+	kthread_queue_work(timer->worker, &timer->work);
 
 	return HRTIMER_NORESTART;
 }
 
-static void msm_atomic_pending_work(struct work_struct *work)
+static void msm_atomic_pending_work(struct kthread_work *work)
 {
 	struct msm_pending_timer *timer = container_of(work,
 			struct msm_pending_timer, work);
@@ -118,14 +117,30 @@ static void msm_atomic_pending_work(struct work_struct *work)
 	msm_atomic_async_commit(timer->kms, timer->crtc_idx);
 }
 
-void msm_atomic_init_pending_timer(struct msm_pending_timer *timer,
+int msm_atomic_init_pending_timer(struct msm_pending_timer *timer,
 		struct msm_kms *kms, int crtc_idx)
 {
 	timer->kms = kms;
 	timer->crtc_idx = crtc_idx;
 	hrtimer_init(&timer->timer, CLOCK_MONOTONIC, HRTIMER_MODE_ABS);
 	timer->timer.function = msm_atomic_pending_timer;
-	INIT_WORK(&timer->work, msm_atomic_pending_work);
+
+	timer->worker = kthread_create_worker(0, "atomic-worker-%d", crtc_idx);
+	if (IS_ERR(timer->worker)) {
+		int ret = PTR_ERR(timer->worker);
+		timer->worker = NULL;
+		return ret;
+	}
+	sched_set_fifo(timer->worker->task);
+	kthread_init_work(&timer->work, msm_atomic_pending_work);
+
+	return 0;
+}
+
+void msm_atomic_destroy_pending_timer(struct msm_pending_timer *timer)
+{
+	if (timer->worker)
+		kthread_destroy_worker(timer->worker);
 }
 
 static bool can_do_async(struct drm_atomic_state *state,
diff --git a/drivers/gpu/drm/msm/msm_drv.h b/drivers/gpu/drm/msm/msm_drv.h
index 5308e636a90c..f869ed67b5da 100644
--- a/drivers/gpu/drm/msm/msm_drv.h
+++ b/drivers/gpu/drm/msm/msm_drv.h
@@ -235,8 +235,9 @@ struct msm_pending_timer;
 
 int msm_atomic_prepare_fb(struct drm_plane *plane,
 			  struct drm_plane_state *new_state);
-void msm_atomic_init_pending_timer(struct msm_pending_timer *timer,
+int msm_atomic_init_pending_timer(struct msm_pending_timer *timer,
 		struct msm_kms *kms, int crtc_idx);
+void msm_atomic_destroy_pending_timer(struct msm_pending_timer *timer);
 void msm_atomic_commit_tail(struct drm_atomic_state *state);
 struct drm_atomic_state *msm_atomic_state_alloc(struct drm_device *dev);
 void msm_atomic_state_clear(struct drm_atomic_state *state);
diff --git a/drivers/gpu/drm/msm/msm_kms.h b/drivers/gpu/drm/msm/msm_kms.h
index 0be9e6487556..26321c13f950 100644
--- a/drivers/gpu/drm/msm/msm_kms.h
+++ b/drivers/gpu/drm/msm/msm_kms.h
@@ -136,7 +136,8 @@ struct msm_kms;
  */
 struct msm_pending_timer {
 	struct hrtimer timer;
-	struct work_struct work;
+	struct kthread_work work;
+	struct kthread_worker *worker;
 	struct msm_kms *kms;
 	unsigned crtc_idx;
 };
@@ -163,19 +164,27 @@ struct msm_kms {
 static inline int msm_kms_init(struct msm_kms *kms,
 		const struct msm_kms_funcs *funcs)
 {
-	unsigned i;
+	unsigned i, ret;
 
 	mutex_init(&kms->commit_lock);
 	kms->funcs = funcs;
 
-	for (i = 0; i < ARRAY_SIZE(kms->pending_timers); i++)
-		msm_atomic_init_pending_timer(&kms->pending_timers[i], kms, i);
+	for (i = 0; i < ARRAY_SIZE(kms->pending_timers); i++) {
+		ret = msm_atomic_init_pending_timer(&kms->pending_timers[i], kms, i);
+		if (ret) {
+			return ret;
+		}
+	}
 
 	return 0;
 }
 
 static inline void msm_kms_destroy(struct msm_kms *kms)
 {
+	unsigned i;
+
+	for (i = 0; i < ARRAY_SIZE(kms->pending_timers); i++)
+		msm_atomic_destroy_pending_timer(&kms->pending_timers[i]);
 }
 
 struct msm_kms *mdp4_kms_init(struct drm_device *dev);
-- 
2.26.2

Re: [PATCH 0/3] drm/msm: kthread_worker conversion

[Daniel Vetter]

On Mon, Oct 19, 2020 at 02:10:50PM -0700, Rob Clark wrote:
> From: Rob Clark <robdclark@chromium.org>
> 
> In particular, converting the async atomic commit (for cursor updates,
> etc) to SCHED_FIFO kthread_worker helps with some cases where we
> wouldn't manage to flush the updates within the 1ms-before-vblank
> deadline resulting in fps drops when there is cursor movement.
> 
> Rob Clark (3):
>   drm/msm/gpu: Convert retire/recover work to kthread_worker
>   drm/msm/kms: Update msm_kms_init/destroy
>   drm/msm/atomic: Convert to per-CRTC kthread_work

So i915 has it's own commit worker already for $reasons, but I don't think
that's a good path to go down with more drivers. And the problem seems
entirely generic in nature ...
-Daniel

> 
>  drivers/gpu/drm/msm/adreno/a5xx_gpu.c     |  3 +--
>  drivers/gpu/drm/msm/adreno/a5xx_preempt.c |  6 ++---
>  drivers/gpu/drm/msm/adreno/a6xx_gmu.c     |  4 +--
>  drivers/gpu/drm/msm/adreno/a6xx_gpu.c     |  4 +--
>  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   |  8 +++++-
>  drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c  |  8 +++++-
>  drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c  | 11 ++++++---
>  drivers/gpu/drm/msm/disp/mdp_kms.h        |  9 +++++--
>  drivers/gpu/drm/msm/msm_atomic.c          | 25 +++++++++++++++----
>  drivers/gpu/drm/msm/msm_drv.h             |  3 ++-
>  drivers/gpu/drm/msm/msm_gpu.c             | 30 +++++++++++++++--------
>  drivers/gpu/drm/msm/msm_gpu.h             | 13 +++++++---
>  drivers/gpu/drm/msm/msm_kms.h             | 23 ++++++++++++++---
>  13 files changed, 104 insertions(+), 43 deletions(-)
> 
> -- 
> 2.26.2
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 0/3] drm/msm: kthread_worker conversion

[Rob Clark]

On Tue, Oct 20, 2020 at 1:24 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Mon, Oct 19, 2020 at 02:10:50PM -0700, Rob Clark wrote:
> > From: Rob Clark <robdclark@chromium.org>
> >
> > In particular, converting the async atomic commit (for cursor updates,
> > etc) to SCHED_FIFO kthread_worker helps with some cases where we
> > wouldn't manage to flush the updates within the 1ms-before-vblank
> > deadline resulting in fps drops when there is cursor movement.
> >
> > Rob Clark (3):
> >   drm/msm/gpu: Convert retire/recover work to kthread_worker
> >   drm/msm/kms: Update msm_kms_init/destroy
> >   drm/msm/atomic: Convert to per-CRTC kthread_work
>
> So i915 has it's own commit worker already for $reasons, but I don't think
> that's a good path to go down with more drivers. And the problem seems
> entirely generic in nature ...

I'm not *entirely* sure what your point is here?  This is just
migrating away from a shared ordered wq to per-crtc kthread so that we
don't miss vblank deadlines for silly reasons (and then stall on the
next frame's pageflip because we are still waiting for the cursor
update to latch).  Kind of like vblank-work but scheduled prior to,
rather than after, vblank.

And you're right that the problem is partially generic.. hw that (a)
doesn't have true async (cursor and/or otherwise) updates, and (b) has
various flush bits that latch register updates on vblank, is not that
uncommon.  But the current atomic helper API would have to be a bit
redesigned to look more like the interface between msm_atomic and the
display backend.  That is a fair bit of churn for re-using a small bit
of code.

BR,
-R

> -Daniel
>
> >
> >  drivers/gpu/drm/msm/adreno/a5xx_gpu.c     |  3 +--
> >  drivers/gpu/drm/msm/adreno/a5xx_preempt.c |  6 ++---
> >  drivers/gpu/drm/msm/adreno/a6xx_gmu.c     |  4 +--
> >  drivers/gpu/drm/msm/adreno/a6xx_gpu.c     |  4 +--
> >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   |  8 +++++-
> >  drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c  |  8 +++++-
> >  drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c  | 11 ++++++---
> >  drivers/gpu/drm/msm/disp/mdp_kms.h        |  9 +++++--
> >  drivers/gpu/drm/msm/msm_atomic.c          | 25 +++++++++++++++----
> >  drivers/gpu/drm/msm/msm_drv.h             |  3 ++-
> >  drivers/gpu/drm/msm/msm_gpu.c             | 30 +++++++++++++++--------
> >  drivers/gpu/drm/msm/msm_gpu.h             | 13 +++++++---
> >  drivers/gpu/drm/msm/msm_kms.h             | 23 ++++++++++++++---
> >  13 files changed, 104 insertions(+), 43 deletions(-)
> >
> > --
> > 2.26.2
> >
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [PATCH 0/3] drm/msm: kthread_worker conversion

[Daniel Vetter]

On Tue, Oct 20, 2020 at 4:01 PM Rob Clark <robdclark@gmail.com> wrote:
>
> On Tue, Oct 20, 2020 at 1:24 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Mon, Oct 19, 2020 at 02:10:50PM -0700, Rob Clark wrote:
> > > From: Rob Clark <robdclark@chromium.org>
> > >
> > > In particular, converting the async atomic commit (for cursor updates,
> > > etc) to SCHED_FIFO kthread_worker helps with some cases where we
> > > wouldn't manage to flush the updates within the 1ms-before-vblank
> > > deadline resulting in fps drops when there is cursor movement.
> > >
> > > Rob Clark (3):
> > >   drm/msm/gpu: Convert retire/recover work to kthread_worker
> > >   drm/msm/kms: Update msm_kms_init/destroy
> > >   drm/msm/atomic: Convert to per-CRTC kthread_work
> >
> > So i915 has it's own commit worker already for $reasons, but I don't think
> > that's a good path to go down with more drivers. And the problem seems
> > entirely generic in nature ...
>
> I'm not *entirely* sure what your point is here?  This is just
> migrating away from a shared ordered wq to per-crtc kthread so that we
> don't miss vblank deadlines for silly reasons (and then stall on the
> next frame's pageflip because we are still waiting for the cursor
> update to latch).  Kind of like vblank-work but scheduled prior to,
> rather than after, vblank.
>
> And you're right that the problem is partially generic.. hw that (a)
> doesn't have true async (cursor and/or otherwise) updates, and (b) has
> various flush bits that latch register updates on vblank, is not that
> uncommon.  But the current atomic helper API would have to be a bit
> redesigned to look more like the interface between msm_atomic and the
> display backend.  That is a fair bit of churn for re-using a small bit
> of code.

I was making some assumptions about what you're doing, and I was
wrong. So I went and tried to understand what's actually going on
here.

I'm trying to understand what exactly you've added with that async msm
support 2d99ced787e3d. I think this breaks the state structure update
model, you can't access any ->state pointers from the commit functions
after you've called drm_atomic_helper_commit_hw_done, or you might
have a use after free. And that seems to be happening from this commit
work thing you added to your existing commit work that the atomic
helpers provide already.

The various commit functions seem to grab various state objects by
just chasing pointers from the objects (instead of the
drm_atomic_state stuff), so this all feels like it's yolo
free-wheeling.

You also seem to be using the async_commit stuff from the atomic
helpers (which is actually synchronous (i.e. blocking) from the pov of
how the code runs, but seems to be for mdp5 only and not others. Also
your can_do_async still checks for legacy_cursor_update (maybe a
leftover, or needed on !mdp5 platforms) and ->async_update.

I'm thoroughly confused how this all works.

I do agree though that you probably want this to be a real time fifo
kthread worker, like for the vblank worker. Except now that I looked,
I'm not sure it's actually working intended and correct.
-Daniel

> BR,
> -R
>
> > -Daniel
> >
> > >
> > >  drivers/gpu/drm/msm/adreno/a5xx_gpu.c     |  3 +--
> > >  drivers/gpu/drm/msm/adreno/a5xx_preempt.c |  6 ++---
> > >  drivers/gpu/drm/msm/adreno/a6xx_gmu.c     |  4 +--
> > >  drivers/gpu/drm/msm/adreno/a6xx_gpu.c     |  4 +--
> > >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   |  8 +++++-
> > >  drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c  |  8 +++++-
> > >  drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c  | 11 ++++++---
> > >  drivers/gpu/drm/msm/disp/mdp_kms.h        |  9 +++++--
> > >  drivers/gpu/drm/msm/msm_atomic.c          | 25 +++++++++++++++----
> > >  drivers/gpu/drm/msm/msm_drv.h             |  3 ++-
> > >  drivers/gpu/drm/msm/msm_gpu.c             | 30 +++++++++++++++--------
> > >  drivers/gpu/drm/msm/msm_gpu.h             | 13 +++++++---
> > >  drivers/gpu/drm/msm/msm_kms.h             | 23 ++++++++++++++---
> > >  13 files changed, 104 insertions(+), 43 deletions(-)
> > >
> > > --
> > > 2.26.2
> > >
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 0/3] drm/msm: kthread_worker conversion

[Rob Clark]

On Tue, Oct 20, 2020 at 7:29 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Tue, Oct 20, 2020 at 4:01 PM Rob Clark <robdclark@gmail.com> wrote:
> >
> > On Tue, Oct 20, 2020 at 1:24 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Mon, Oct 19, 2020 at 02:10:50PM -0700, Rob Clark wrote:
> > > > From: Rob Clark <robdclark@chromium.org>
> > > >
> > > > In particular, converting the async atomic commit (for cursor updates,
> > > > etc) to SCHED_FIFO kthread_worker helps with some cases where we
> > > > wouldn't manage to flush the updates within the 1ms-before-vblank
> > > > deadline resulting in fps drops when there is cursor movement.
> > > >
> > > > Rob Clark (3):
> > > >   drm/msm/gpu: Convert retire/recover work to kthread_worker
> > > >   drm/msm/kms: Update msm_kms_init/destroy
> > > >   drm/msm/atomic: Convert to per-CRTC kthread_work
> > >
> > > So i915 has it's own commit worker already for $reasons, but I don't think
> > > that's a good path to go down with more drivers. And the problem seems
> > > entirely generic in nature ...
> >
> > I'm not *entirely* sure what your point is here?  This is just
> > migrating away from a shared ordered wq to per-crtc kthread so that we
> > don't miss vblank deadlines for silly reasons (and then stall on the
> > next frame's pageflip because we are still waiting for the cursor
> > update to latch).  Kind of like vblank-work but scheduled prior to,
> > rather than after, vblank.
> >
> > And you're right that the problem is partially generic.. hw that (a)
> > doesn't have true async (cursor and/or otherwise) updates, and (b) has
> > various flush bits that latch register updates on vblank, is not that
> > uncommon.  But the current atomic helper API would have to be a bit
> > redesigned to look more like the interface between msm_atomic and the
> > display backend.  That is a fair bit of churn for re-using a small bit
> > of code.
>
> I was making some assumptions about what you're doing, and I was
> wrong. So I went and tried to understand what's actually going on
> here.
>
> I'm trying to understand what exactly you've added with that async msm
> support 2d99ced787e3d. I think this breaks the state structure update
> model, you can't access any ->state pointers from the commit functions
> after you've called drm_atomic_helper_commit_hw_done, or you might
> have a use after free. And that seems to be happening from this commit
> work thing you added to your existing commit work that the atomic
> helpers provide already.
>
> The various commit functions seem to grab various state objects by
> just chasing pointers from the objects (instead of the
> drm_atomic_state stuff), so this all feels like it's yolo
> free-wheeling.
>
> You also seem to be using the async_commit stuff from the atomic
> helpers (which is actually synchronous (i.e. blocking) from the pov of
> how the code runs, but seems to be for mdp5 only and not others. Also
> your can_do_async still checks for legacy_cursor_update (maybe a
> leftover, or needed on !mdp5 platforms) and ->async_update.
>
> I'm thoroughly confused how this all works.

The legacy_cursor_update is really the thing that motivated the async
commit support in the first place.  Sadly we still have userspace that
expects to be able to use legacy cursor API, and that it will be
nonblocking (and not cause fps drop).  (I'm not a fan of the legacy
cursor UAPI.. don't hate the player..)

The premise is to do everything in terms of crtc_mask, although yeah,
it looks like there are a few points that need to look at things like
crtc->state->active.  The only point in msm-atomic itself that does
this is vblank_get/put(), possibly we can fix drm_vblank instead and
drop that workaround (see 43906812eaab06423f56af5cca9a9fcdbb4ac454)

The rest of the async part is really just supposed to be writing the
appropriate flush reg(s) and waiting until flush completes, although
dpu's excess layering makes this harder than it needs to be.

In practice, the kms->wait_flush() at the top of
msm_atomic_commit_tail() will block until a pending async commit
completes (this is where we hit the fps drop if we miss vblank
deadline), so I don't *think* you can trigger a use-after-free.  But
the dpu code could be better cleaned up to have less obj->state
dereference in the kms->flush_commit(crtc_mask)/etc path.

BR,
-R

> I do agree though that you probably want this to be a real time fifo
> kthread worker, like for the vblank worker. Except now that I looked,
> I'm not sure it's actually working intended and correct.
> -Daniel
>
> > BR,
> > -R
> >
> > > -Daniel
> > >
> > > >
> > > >  drivers/gpu/drm/msm/adreno/a5xx_gpu.c     |  3 +--
> > > >  drivers/gpu/drm/msm/adreno/a5xx_preempt.c |  6 ++---
> > > >  drivers/gpu/drm/msm/adreno/a6xx_gmu.c     |  4 +--
> > > >  drivers/gpu/drm/msm/adreno/a6xx_gpu.c     |  4 +--
> > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   |  8 +++++-
> > > >  drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c  |  8 +++++-
> > > >  drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c  | 11 ++++++---
> > > >  drivers/gpu/drm/msm/disp/mdp_kms.h        |  9 +++++--
> > > >  drivers/gpu/drm/msm/msm_atomic.c          | 25 +++++++++++++++----
> > > >  drivers/gpu/drm/msm/msm_drv.h             |  3 ++-
> > > >  drivers/gpu/drm/msm/msm_gpu.c             | 30 +++++++++++++++--------
> > > >  drivers/gpu/drm/msm/msm_gpu.h             | 13 +++++++---
> > > >  drivers/gpu/drm/msm/msm_kms.h             | 23 ++++++++++++++---
> > > >  13 files changed, 104 insertions(+), 43 deletions(-)
> > > >
> > > > --
> > > > 2.26.2
> > > >
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [PATCH 0/3] drm/msm: kthread_worker conversion

[Daniel Vetter]

On Tue, Oct 20, 2020 at 5:08 PM Rob Clark <robdclark@gmail.com> wrote:
>
> On Tue, Oct 20, 2020 at 7:29 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Tue, Oct 20, 2020 at 4:01 PM Rob Clark <robdclark@gmail.com> wrote:
> > >
> > > On Tue, Oct 20, 2020 at 1:24 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > >
> > > > On Mon, Oct 19, 2020 at 02:10:50PM -0700, Rob Clark wrote:
> > > > > From: Rob Clark <robdclark@chromium.org>
> > > > >
> > > > > In particular, converting the async atomic commit (for cursor updates,
> > > > > etc) to SCHED_FIFO kthread_worker helps with some cases where we
> > > > > wouldn't manage to flush the updates within the 1ms-before-vblank
> > > > > deadline resulting in fps drops when there is cursor movement.
> > > > >
> > > > > Rob Clark (3):
> > > > >   drm/msm/gpu: Convert retire/recover work to kthread_worker
> > > > >   drm/msm/kms: Update msm_kms_init/destroy
> > > > >   drm/msm/atomic: Convert to per-CRTC kthread_work
> > > >
> > > > So i915 has it's own commit worker already for $reasons, but I don't think
> > > > that's a good path to go down with more drivers. And the problem seems
> > > > entirely generic in nature ...
> > >
> > > I'm not *entirely* sure what your point is here?  This is just
> > > migrating away from a shared ordered wq to per-crtc kthread so that we
> > > don't miss vblank deadlines for silly reasons (and then stall on the
> > > next frame's pageflip because we are still waiting for the cursor
> > > update to latch).  Kind of like vblank-work but scheduled prior to,
> > > rather than after, vblank.
> > >
> > > And you're right that the problem is partially generic.. hw that (a)
> > > doesn't have true async (cursor and/or otherwise) updates, and (b) has
> > > various flush bits that latch register updates on vblank, is not that
> > > uncommon.  But the current atomic helper API would have to be a bit
> > > redesigned to look more like the interface between msm_atomic and the
> > > display backend.  That is a fair bit of churn for re-using a small bit
> > > of code.
> >
> > I was making some assumptions about what you're doing, and I was
> > wrong. So I went and tried to understand what's actually going on
> > here.
> >
> > I'm trying to understand what exactly you've added with that async msm
> > support 2d99ced787e3d. I think this breaks the state structure update
> > model, you can't access any ->state pointers from the commit functions
> > after you've called drm_atomic_helper_commit_hw_done, or you might
> > have a use after free. And that seems to be happening from this commit
> > work thing you added to your existing commit work that the atomic
> > helpers provide already.
> >
> > The various commit functions seem to grab various state objects by
> > just chasing pointers from the objects (instead of the
> > drm_atomic_state stuff), so this all feels like it's yolo
> > free-wheeling.
> >
> > You also seem to be using the async_commit stuff from the atomic
> > helpers (which is actually synchronous (i.e. blocking) from the pov of
> > how the code runs, but seems to be for mdp5 only and not others. Also
> > your can_do_async still checks for legacy_cursor_update (maybe a
> > leftover, or needed on !mdp5 platforms) and ->async_update.
> >
> > I'm thoroughly confused how this all works.
>
> The legacy_cursor_update is really the thing that motivated the async
> commit support in the first place.  Sadly we still have userspace that
> expects to be able to use legacy cursor API, and that it will be
> nonblocking (and not cause fps drop).  (I'm not a fan of the legacy
> cursor UAPI.. don't hate the player..)

Yeah this is why we have these atomic_async_check/commit functions,
and msm is even using them for mdp5. Not hating the player here at
all.

> The premise is to do everything in terms of crtc_mask, although yeah,
> it looks like there are a few points that need to look at things like
> crtc->state->active.  The only point in msm-atomic itself that does
> this is vblank_get/put(), possibly we can fix drm_vblank instead and
> drop that workaround (see 43906812eaab06423f56af5cca9a9fcdbb4ac454)
>
> The rest of the async part is really just supposed to be writing the
> appropriate flush reg(s) and waiting until flush completes, although
> dpu's excess layering makes this harder than it needs to be.
>
> In practice, the kms->wait_flush() at the top of
> msm_atomic_commit_tail() will block until a pending async commit
> completes (this is where we hit the fps drop if we miss vblank
> deadline), so I don't *think* you can trigger a use-after-free.  But
> the dpu code could be better cleaned up to have less obj->state
> dereference in the kms->flush_commit(crtc_mask)/etc path.

Hm this is more or less what the atomic_async_commit/check stuff was
meant to help facilitate too, and now msm is using that for mdp5, but
not for other pieces. That seems very confusing.

Also I'm not sure how this works if you still end up flushing anyway,
since then you'd be back to doing everything in-order. Or will an
normal atomic flip push all the cursor updates to the next frame (in
which case you really should be able to do this all with async helpers
we have instead of hand-rolling a bunch of it in strange places).

You probably still need the worker to push out the update at the right
time, and I'm not sure what some good locking for that is. At least
I'm not really seeing how you sync that worker against a racing update
for the next cursor move.
-Daniel


> BR,
> -R
>
> > I do agree though that you probably want this to be a real time fifo
> > kthread worker, like for the vblank worker. Except now that I looked,
> > I'm not sure it's actually working intended and correct.
> > -Daniel
> >
> > > BR,
> > > -R
> > >
> > > > -Daniel
> > > >
> > > > >
> > > > >  drivers/gpu/drm/msm/adreno/a5xx_gpu.c     |  3 +--
> > > > >  drivers/gpu/drm/msm/adreno/a5xx_preempt.c |  6 ++---
> > > > >  drivers/gpu/drm/msm/adreno/a6xx_gmu.c     |  4 +--
> > > > >  drivers/gpu/drm/msm/adreno/a6xx_gpu.c     |  4 +--
> > > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   |  8 +++++-
> > > > >  drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c  |  8 +++++-
> > > > >  drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c  | 11 ++++++---
> > > > >  drivers/gpu/drm/msm/disp/mdp_kms.h        |  9 +++++--
> > > > >  drivers/gpu/drm/msm/msm_atomic.c          | 25 +++++++++++++++----
> > > > >  drivers/gpu/drm/msm/msm_drv.h             |  3 ++-
> > > > >  drivers/gpu/drm/msm/msm_gpu.c             | 30 +++++++++++++++--------
> > > > >  drivers/gpu/drm/msm/msm_gpu.h             | 13 +++++++---
> > > > >  drivers/gpu/drm/msm/msm_kms.h             | 23 ++++++++++++++---
> > > > >  13 files changed, 104 insertions(+), 43 deletions(-)
> > > > >
> > > > > --
> > > > > 2.26.2
> > > > >
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > >
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> > > _______________________________________________
> > > dri-devel mailing list
> > > dri-devel@lists.freedesktop.org
> > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 0/3] drm/msm: kthread_worker conversion

[Rob Clark]

On Tue, Oct 20, 2020 at 10:02 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Tue, Oct 20, 2020 at 5:08 PM Rob Clark <robdclark@gmail.com> wrote:
> >
> > On Tue, Oct 20, 2020 at 7:29 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Tue, Oct 20, 2020 at 4:01 PM Rob Clark <robdclark@gmail.com> wrote:
> > > >
> > > > On Tue, Oct 20, 2020 at 1:24 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > >
> > > > > On Mon, Oct 19, 2020 at 02:10:50PM -0700, Rob Clark wrote:
> > > > > > From: Rob Clark <robdclark@chromium.org>
> > > > > >
> > > > > > In particular, converting the async atomic commit (for cursor updates,
> > > > > > etc) to SCHED_FIFO kthread_worker helps with some cases where we
> > > > > > wouldn't manage to flush the updates within the 1ms-before-vblank
> > > > > > deadline resulting in fps drops when there is cursor movement.
> > > > > >
> > > > > > Rob Clark (3):
> > > > > >   drm/msm/gpu: Convert retire/recover work to kthread_worker
> > > > > >   drm/msm/kms: Update msm_kms_init/destroy
> > > > > >   drm/msm/atomic: Convert to per-CRTC kthread_work
> > > > >
> > > > > So i915 has it's own commit worker already for $reasons, but I don't think
> > > > > that's a good path to go down with more drivers. And the problem seems
> > > > > entirely generic in nature ...
> > > >
> > > > I'm not *entirely* sure what your point is here?  This is just
> > > > migrating away from a shared ordered wq to per-crtc kthread so that we
> > > > don't miss vblank deadlines for silly reasons (and then stall on the
> > > > next frame's pageflip because we are still waiting for the cursor
> > > > update to latch).  Kind of like vblank-work but scheduled prior to,
> > > > rather than after, vblank.
> > > >
> > > > And you're right that the problem is partially generic.. hw that (a)
> > > > doesn't have true async (cursor and/or otherwise) updates, and (b) has
> > > > various flush bits that latch register updates on vblank, is not that
> > > > uncommon.  But the current atomic helper API would have to be a bit
> > > > redesigned to look more like the interface between msm_atomic and the
> > > > display backend.  That is a fair bit of churn for re-using a small bit
> > > > of code.
> > >
> > > I was making some assumptions about what you're doing, and I was
> > > wrong. So I went and tried to understand what's actually going on
> > > here.
> > >
> > > I'm trying to understand what exactly you've added with that async msm
> > > support 2d99ced787e3d. I think this breaks the state structure update
> > > model, you can't access any ->state pointers from the commit functions
> > > after you've called drm_atomic_helper_commit_hw_done, or you might
> > > have a use after free. And that seems to be happening from this commit
> > > work thing you added to your existing commit work that the atomic
> > > helpers provide already.
> > >
> > > The various commit functions seem to grab various state objects by
> > > just chasing pointers from the objects (instead of the
> > > drm_atomic_state stuff), so this all feels like it's yolo
> > > free-wheeling.
> > >
> > > You also seem to be using the async_commit stuff from the atomic
> > > helpers (which is actually synchronous (i.e. blocking) from the pov of
> > > how the code runs, but seems to be for mdp5 only and not others. Also
> > > your can_do_async still checks for legacy_cursor_update (maybe a
> > > leftover, or needed on !mdp5 platforms) and ->async_update.
> > >
> > > I'm thoroughly confused how this all works.
> >
> > The legacy_cursor_update is really the thing that motivated the async
> > commit support in the first place.  Sadly we still have userspace that
> > expects to be able to use legacy cursor API, and that it will be
> > nonblocking (and not cause fps drop).  (I'm not a fan of the legacy
> > cursor UAPI.. don't hate the player..)
>
> Yeah this is why we have these atomic_async_check/commit functions,
> and msm is even using them for mdp5. Not hating the player here at
> all.
>
> > The premise is to do everything in terms of crtc_mask, although yeah,
> > it looks like there are a few points that need to look at things like
> > crtc->state->active.  The only point in msm-atomic itself that does
> > this is vblank_get/put(), possibly we can fix drm_vblank instead and
> > drop that workaround (see 43906812eaab06423f56af5cca9a9fcdbb4ac454)
> >
> > The rest of the async part is really just supposed to be writing the
> > appropriate flush reg(s) and waiting until flush completes, although
> > dpu's excess layering makes this harder than it needs to be.
> >
> > In practice, the kms->wait_flush() at the top of
> > msm_atomic_commit_tail() will block until a pending async commit
> > completes (this is where we hit the fps drop if we miss vblank
> > deadline), so I don't *think* you can trigger a use-after-free.  But
> > the dpu code could be better cleaned up to have less obj->state
> > dereference in the kms->flush_commit(crtc_mask)/etc path.
>
> Hm this is more or less what the atomic_async_commit/check stuff was
> meant to help facilitate too, and now msm is using that for mdp5, but
> not for other pieces. That seems very confusing.
>
> Also I'm not sure how this works if you still end up flushing anyway,
> since then you'd be back to doing everything in-order. Or will an
> normal atomic flip push all the cursor updates to the next frame (in
> which case you really should be able to do this all with async helpers
> we have instead of hand-rolling a bunch of it in strange places).

So, "flush" from the core-atomic part is writing all the various
registers (overlay scanout bo/format/position/etc).. this is all done
at the normal time (ie. whenever we get the cursor update).  The only
thing we defer until close-to-vblank is writing the hw flush registers
(ie. registers with bitmasks of the various hw blocks to latch on
vblank).

So a cursor update applies the state normally, from the PoV of
sequence of atomic updates.  But tries to defer writing the flush regs
so we can merge in future cursor updates and/or pageflip into the same
frame.

Modulo the stuff that derefs kmsobj->state but shouldn't, I think (at
least for hw that works this way with flush registers) this is a
better approach to handling cursor updates.  The mdp5 async cursor
stuff predates dpu, and I've just not had a chance to update mdp5 to
use the new async flush path yet.

BR,
-R

> You probably still need the worker to push out the update at the right
> time, and I'm not sure what some good locking for that is. At least
> I'm not really seeing how you sync that worker against a racing update
> for the next cursor move.
> -Daniel
>
>
> > BR,
> > -R
> >
> > > I do agree though that you probably want this to be a real time fifo
> > > kthread worker, like for the vblank worker. Except now that I looked,
> > > I'm not sure it's actually working intended and correct.
> > > -Daniel
> > >
> > > > BR,
> > > > -R
> > > >
> > > > > -Daniel
> > > > >
> > > > > >
> > > > > >  drivers/gpu/drm/msm/adreno/a5xx_gpu.c     |  3 +--
> > > > > >  drivers/gpu/drm/msm/adreno/a5xx_preempt.c |  6 ++---
> > > > > >  drivers/gpu/drm/msm/adreno/a6xx_gmu.c     |  4 +--
> > > > > >  drivers/gpu/drm/msm/adreno/a6xx_gpu.c     |  4 +--
> > > > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   |  8 +++++-
> > > > > >  drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c  |  8 +++++-
> > > > > >  drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c  | 11 ++++++---
> > > > > >  drivers/gpu/drm/msm/disp/mdp_kms.h        |  9 +++++--
> > > > > >  drivers/gpu/drm/msm/msm_atomic.c          | 25 +++++++++++++++----
> > > > > >  drivers/gpu/drm/msm/msm_drv.h             |  3 ++-
> > > > > >  drivers/gpu/drm/msm/msm_gpu.c             | 30 +++++++++++++++--------
> > > > > >  drivers/gpu/drm/msm/msm_gpu.h             | 13 +++++++---
> > > > > >  drivers/gpu/drm/msm/msm_kms.h             | 23 ++++++++++++++---
> > > > > >  13 files changed, 104 insertions(+), 43 deletions(-)
> > > > > >
> > > > > > --
> > > > > > 2.26.2
> > > > > >
> > > > > > _______________________________________________
> > > > > > dri-devel mailing list
> > > > > > dri-devel@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > >
> > > > > --
> > > > > Daniel Vetter
> > > > > Software Engineer, Intel Corporation
> > > > > http://blog.ffwll.ch
> > > > _______________________________________________
> > > > dri-devel mailing list
> > > > dri-devel@lists.freedesktop.org
> > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > >
> > >
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [PATCH 0/3] drm/msm: kthread_worker conversion

[Daniel Vetter]

On Tue, Oct 20, 2020 at 7:23 PM Rob Clark <robdclark@gmail.com> wrote:
>
> On Tue, Oct 20, 2020 at 10:02 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Tue, Oct 20, 2020 at 5:08 PM Rob Clark <robdclark@gmail.com> wrote:
> > >
> > > On Tue, Oct 20, 2020 at 7:29 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > >
> > > > On Tue, Oct 20, 2020 at 4:01 PM Rob Clark <robdclark@gmail.com> wrote:
> > > > >
> > > > > On Tue, Oct 20, 2020 at 1:24 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > > >
> > > > > > On Mon, Oct 19, 2020 at 02:10:50PM -0700, Rob Clark wrote:
> > > > > > > From: Rob Clark <robdclark@chromium.org>
> > > > > > >
> > > > > > > In particular, converting the async atomic commit (for cursor updates,
> > > > > > > etc) to SCHED_FIFO kthread_worker helps with some cases where we
> > > > > > > wouldn't manage to flush the updates within the 1ms-before-vblank
> > > > > > > deadline resulting in fps drops when there is cursor movement.
> > > > > > >
> > > > > > > Rob Clark (3):
> > > > > > >   drm/msm/gpu: Convert retire/recover work to kthread_worker
> > > > > > >   drm/msm/kms: Update msm_kms_init/destroy
> > > > > > >   drm/msm/atomic: Convert to per-CRTC kthread_work
> > > > > >
> > > > > > So i915 has it's own commit worker already for $reasons, but I don't think
> > > > > > that's a good path to go down with more drivers. And the problem seems
> > > > > > entirely generic in nature ...
> > > > >
> > > > > I'm not *entirely* sure what your point is here?  This is just
> > > > > migrating away from a shared ordered wq to per-crtc kthread so that we
> > > > > don't miss vblank deadlines for silly reasons (and then stall on the
> > > > > next frame's pageflip because we are still waiting for the cursor
> > > > > update to latch).  Kind of like vblank-work but scheduled prior to,
> > > > > rather than after, vblank.
> > > > >
> > > > > And you're right that the problem is partially generic.. hw that (a)
> > > > > doesn't have true async (cursor and/or otherwise) updates, and (b) has
> > > > > various flush bits that latch register updates on vblank, is not that
> > > > > uncommon.  But the current atomic helper API would have to be a bit
> > > > > redesigned to look more like the interface between msm_atomic and the
> > > > > display backend.  That is a fair bit of churn for re-using a small bit
> > > > > of code.
> > > >
> > > > I was making some assumptions about what you're doing, and I was
> > > > wrong. So I went and tried to understand what's actually going on
> > > > here.
> > > >
> > > > I'm trying to understand what exactly you've added with that async msm
> > > > support 2d99ced787e3d. I think this breaks the state structure update
> > > > model, you can't access any ->state pointers from the commit functions
> > > > after you've called drm_atomic_helper_commit_hw_done, or you might
> > > > have a use after free. And that seems to be happening from this commit
> > > > work thing you added to your existing commit work that the atomic
> > > > helpers provide already.
> > > >
> > > > The various commit functions seem to grab various state objects by
> > > > just chasing pointers from the objects (instead of the
> > > > drm_atomic_state stuff), so this all feels like it's yolo
> > > > free-wheeling.
> > > >
> > > > You also seem to be using the async_commit stuff from the atomic
> > > > helpers (which is actually synchronous (i.e. blocking) from the pov of
> > > > how the code runs, but seems to be for mdp5 only and not others. Also
> > > > your can_do_async still checks for legacy_cursor_update (maybe a
> > > > leftover, or needed on !mdp5 platforms) and ->async_update.
> > > >
> > > > I'm thoroughly confused how this all works.
> > >
> > > The legacy_cursor_update is really the thing that motivated the async
> > > commit support in the first place.  Sadly we still have userspace that
> > > expects to be able to use legacy cursor API, and that it will be
> > > nonblocking (and not cause fps drop).  (I'm not a fan of the legacy
> > > cursor UAPI.. don't hate the player..)
> >
> > Yeah this is why we have these atomic_async_check/commit functions,
> > and msm is even using them for mdp5. Not hating the player here at
> > all.
> >
> > > The premise is to do everything in terms of crtc_mask, although yeah,
> > > it looks like there are a few points that need to look at things like
> > > crtc->state->active.  The only point in msm-atomic itself that does
> > > this is vblank_get/put(), possibly we can fix drm_vblank instead and
> > > drop that workaround (see 43906812eaab06423f56af5cca9a9fcdbb4ac454)
> > >
> > > The rest of the async part is really just supposed to be writing the
> > > appropriate flush reg(s) and waiting until flush completes, although
> > > dpu's excess layering makes this harder than it needs to be.
> > >
> > > In practice, the kms->wait_flush() at the top of
> > > msm_atomic_commit_tail() will block until a pending async commit
> > > completes (this is where we hit the fps drop if we miss vblank
> > > deadline), so I don't *think* you can trigger a use-after-free.  But
> > > the dpu code could be better cleaned up to have less obj->state
> > > dereference in the kms->flush_commit(crtc_mask)/etc path.
> >
> > Hm this is more or less what the atomic_async_commit/check stuff was
> > meant to help facilitate too, and now msm is using that for mdp5, but
> > not for other pieces. That seems very confusing.
> >
> > Also I'm not sure how this works if you still end up flushing anyway,
> > since then you'd be back to doing everything in-order. Or will an
> > normal atomic flip push all the cursor updates to the next frame (in
> > which case you really should be able to do this all with async helpers
> > we have instead of hand-rolling a bunch of it in strange places).
>
> So, "flush" from the core-atomic part is writing all the various
> registers (overlay scanout bo/format/position/etc).. this is all done
> at the normal time (ie. whenever we get the cursor update).  The only
> thing we defer until close-to-vblank is writing the hw flush registers
> (ie. registers with bitmasks of the various hw blocks to latch on
> vblank).
>
> So a cursor update applies the state normally, from the PoV of
> sequence of atomic updates.  But tries to defer writing the flush regs
> so we can merge in future cursor updates and/or pageflip into the same
> frame.
>
> Modulo the stuff that derefs kmsobj->state but shouldn't, I think (at
> least for hw that works this way with flush registers) this is a
> better approach to handling cursor updates.  The mdp5 async cursor
> stuff predates dpu, and I've just not had a chance to update mdp5 to
> use the new async flush path yet.

The trouble is that this is moving back to legacy_cursor_update hack
instead of retiring it for good, so I'm not super thrilled about this.

Can't we do the register update from atomic_async_commit, and then
latch the timed worker, so that it all fits into the bigger thing?
Maybe also subsume the mdp5 stuff like that.

And that commit worker then probably needs the minimal amount of state
protected by a spinlock or similar, so they're not trampling over each
other. At least I'm still not seeing how you both make stuff async and
prevent havoc when an update races with the commit worker. Or can that
only happen for cursor commits, where we don't care when the cursor is
very rarely misplaced because the hw takes an inconsistent update.
-Daniel


> BR,
> -R
>
> > You probably still need the worker to push out the update at the right
> > time, and I'm not sure what some good locking for that is. At least
> > I'm not really seeing how you sync that worker against a racing update
> > for the next cursor move.
> > -Daniel
> >
> >
> > > BR,
> > > -R
> > >
> > > > I do agree though that you probably want this to be a real time fifo
> > > > kthread worker, like for the vblank worker. Except now that I looked,
> > > > I'm not sure it's actually working intended and correct.
> > > > -Daniel
> > > >
> > > > > BR,
> > > > > -R
> > > > >
> > > > > > -Daniel
> > > > > >
> > > > > > >
> > > > > > >  drivers/gpu/drm/msm/adreno/a5xx_gpu.c     |  3 +--
> > > > > > >  drivers/gpu/drm/msm/adreno/a5xx_preempt.c |  6 ++---
> > > > > > >  drivers/gpu/drm/msm/adreno/a6xx_gmu.c     |  4 +--
> > > > > > >  drivers/gpu/drm/msm/adreno/a6xx_gpu.c     |  4 +--
> > > > > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   |  8 +++++-
> > > > > > >  drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c  |  8 +++++-
> > > > > > >  drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c  | 11 ++++++---
> > > > > > >  drivers/gpu/drm/msm/disp/mdp_kms.h        |  9 +++++--
> > > > > > >  drivers/gpu/drm/msm/msm_atomic.c          | 25 +++++++++++++++----
> > > > > > >  drivers/gpu/drm/msm/msm_drv.h             |  3 ++-
> > > > > > >  drivers/gpu/drm/msm/msm_gpu.c             | 30 +++++++++++++++--------
> > > > > > >  drivers/gpu/drm/msm/msm_gpu.h             | 13 +++++++---
> > > > > > >  drivers/gpu/drm/msm/msm_kms.h             | 23 ++++++++++++++---
> > > > > > >  13 files changed, 104 insertions(+), 43 deletions(-)
> > > > > > >
> > > > > > > --
> > > > > > > 2.26.2
> > > > > > >
> > > > > > > _______________________________________________
> > > > > > > dri-devel mailing list
> > > > > > > dri-devel@lists.freedesktop.org
> > > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > > >
> > > > > > --
> > > > > > Daniel Vetter
> > > > > > Software Engineer, Intel Corporation
> > > > > > http://blog.ffwll.ch
> > > > > _______________________________________________
> > > > > dri-devel mailing list
> > > > > dri-devel@lists.freedesktop.org
> > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > >
> > > >
> > > >
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch



-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH 0/3] drm/msm: kthread_worker conversion

[Rob Clark]

On Tue, Oct 20, 2020 at 11:14 AM Daniel Vetter <daniel@ffwll.ch> wrote:
>
> On Tue, Oct 20, 2020 at 7:23 PM Rob Clark <robdclark@gmail.com> wrote:
> >
> > On Tue, Oct 20, 2020 at 10:02 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > >
> > > On Tue, Oct 20, 2020 at 5:08 PM Rob Clark <robdclark@gmail.com> wrote:
> > > >
> > > > On Tue, Oct 20, 2020 at 7:29 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > >
> > > > > On Tue, Oct 20, 2020 at 4:01 PM Rob Clark <robdclark@gmail.com> wrote:
> > > > > >
> > > > > > On Tue, Oct 20, 2020 at 1:24 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > > > >
> > > > > > > On Mon, Oct 19, 2020 at 02:10:50PM -0700, Rob Clark wrote:
> > > > > > > > From: Rob Clark <robdclark@chromium.org>
> > > > > > > >
> > > > > > > > In particular, converting the async atomic commit (for cursor updates,
> > > > > > > > etc) to SCHED_FIFO kthread_worker helps with some cases where we
> > > > > > > > wouldn't manage to flush the updates within the 1ms-before-vblank
> > > > > > > > deadline resulting in fps drops when there is cursor movement.
> > > > > > > >
> > > > > > > > Rob Clark (3):
> > > > > > > >   drm/msm/gpu: Convert retire/recover work to kthread_worker
> > > > > > > >   drm/msm/kms: Update msm_kms_init/destroy
> > > > > > > >   drm/msm/atomic: Convert to per-CRTC kthread_work
> > > > > > >
> > > > > > > So i915 has it's own commit worker already for $reasons, but I don't think
> > > > > > > that's a good path to go down with more drivers. And the problem seems
> > > > > > > entirely generic in nature ...
> > > > > >
> > > > > > I'm not *entirely* sure what your point is here?  This is just
> > > > > > migrating away from a shared ordered wq to per-crtc kthread so that we
> > > > > > don't miss vblank deadlines for silly reasons (and then stall on the
> > > > > > next frame's pageflip because we are still waiting for the cursor
> > > > > > update to latch).  Kind of like vblank-work but scheduled prior to,
> > > > > > rather than after, vblank.
> > > > > >
> > > > > > And you're right that the problem is partially generic.. hw that (a)
> > > > > > doesn't have true async (cursor and/or otherwise) updates, and (b) has
> > > > > > various flush bits that latch register updates on vblank, is not that
> > > > > > uncommon.  But the current atomic helper API would have to be a bit
> > > > > > redesigned to look more like the interface between msm_atomic and the
> > > > > > display backend.  That is a fair bit of churn for re-using a small bit
> > > > > > of code.
> > > > >
> > > > > I was making some assumptions about what you're doing, and I was
> > > > > wrong. So I went and tried to understand what's actually going on
> > > > > here.
> > > > >
> > > > > I'm trying to understand what exactly you've added with that async msm
> > > > > support 2d99ced787e3d. I think this breaks the state structure update
> > > > > model, you can't access any ->state pointers from the commit functions
> > > > > after you've called drm_atomic_helper_commit_hw_done, or you might
> > > > > have a use after free. And that seems to be happening from this commit
> > > > > work thing you added to your existing commit work that the atomic
> > > > > helpers provide already.
> > > > >
> > > > > The various commit functions seem to grab various state objects by
> > > > > just chasing pointers from the objects (instead of the
> > > > > drm_atomic_state stuff), so this all feels like it's yolo
> > > > > free-wheeling.
> > > > >
> > > > > You also seem to be using the async_commit stuff from the atomic
> > > > > helpers (which is actually synchronous (i.e. blocking) from the pov of
> > > > > how the code runs, but seems to be for mdp5 only and not others. Also
> > > > > your can_do_async still checks for legacy_cursor_update (maybe a
> > > > > leftover, or needed on !mdp5 platforms) and ->async_update.
> > > > >
> > > > > I'm thoroughly confused how this all works.
> > > >
> > > > The legacy_cursor_update is really the thing that motivated the async
> > > > commit support in the first place.  Sadly we still have userspace that
> > > > expects to be able to use legacy cursor API, and that it will be
> > > > nonblocking (and not cause fps drop).  (I'm not a fan of the legacy
> > > > cursor UAPI.. don't hate the player..)
> > >
> > > Yeah this is why we have these atomic_async_check/commit functions,
> > > and msm is even using them for mdp5. Not hating the player here at
> > > all.
> > >
> > > > The premise is to do everything in terms of crtc_mask, although yeah,
> > > > it looks like there are a few points that need to look at things like
> > > > crtc->state->active.  The only point in msm-atomic itself that does
> > > > this is vblank_get/put(), possibly we can fix drm_vblank instead and
> > > > drop that workaround (see 43906812eaab06423f56af5cca9a9fcdbb4ac454)
> > > >
> > > > The rest of the async part is really just supposed to be writing the
> > > > appropriate flush reg(s) and waiting until flush completes, although
> > > > dpu's excess layering makes this harder than it needs to be.
> > > >
> > > > In practice, the kms->wait_flush() at the top of
> > > > msm_atomic_commit_tail() will block until a pending async commit
> > > > completes (this is where we hit the fps drop if we miss vblank
> > > > deadline), so I don't *think* you can trigger a use-after-free.  But
> > > > the dpu code could be better cleaned up to have less obj->state
> > > > dereference in the kms->flush_commit(crtc_mask)/etc path.
> > >
> > > Hm this is more or less what the atomic_async_commit/check stuff was
> > > meant to help facilitate too, and now msm is using that for mdp5, but
> > > not for other pieces. That seems very confusing.
> > >
> > > Also I'm not sure how this works if you still end up flushing anyway,
> > > since then you'd be back to doing everything in-order. Or will an
> > > normal atomic flip push all the cursor updates to the next frame (in
> > > which case you really should be able to do this all with async helpers
> > > we have instead of hand-rolling a bunch of it in strange places).
> >
> > So, "flush" from the core-atomic part is writing all the various
> > registers (overlay scanout bo/format/position/etc).. this is all done
> > at the normal time (ie. whenever we get the cursor update).  The only
> > thing we defer until close-to-vblank is writing the hw flush registers
> > (ie. registers with bitmasks of the various hw blocks to latch on
> > vblank).
> >
> > So a cursor update applies the state normally, from the PoV of
> > sequence of atomic updates.  But tries to defer writing the flush regs
> > so we can merge in future cursor updates and/or pageflip into the same
> > frame.
> >
> > Modulo the stuff that derefs kmsobj->state but shouldn't, I think (at
> > least for hw that works this way with flush registers) this is a
> > better approach to handling cursor updates.  The mdp5 async cursor
> > stuff predates dpu, and I've just not had a chance to update mdp5 to
> > use the new async flush path yet.
>
> The trouble is that this is moving back to legacy_cursor_update hack
> instead of retiring it for good, so I'm not super thrilled about this.

state->async==true for cursor updates would work for me.. at the end
of the day, it doesn't really matter that it is a cursor plane, or
what the UAPI was, just that it is async.

> Can't we do the register update from atomic_async_commit, and then
> latch the timed worker, so that it all fits into the bigger thing?
> Maybe also subsume the mdp5 stuff like that.

The current async update path replaced a previous async commit
implementation, which might have been using atomic_async_commit?  I'd
have to dig back thru git history.  The big problem with it was that
an async commit could race with a sync/nonblock commit, and one of the
paths could write flush regs while other is still updating regs.

The important thing about the current async approach is the separation
of commit and flush, and the kms->wait_flush() at the top of
commit_tail() which serializes hw updates and flush, so we don't have
problems with racing commits.  I'm not sure how that would fit in with
atomic_async_commit().

> And that commit worker then probably needs the minimal amount of state
> protected by a spinlock or similar, so they're not trampling over each
> other. At least I'm still not seeing how you both make stuff async and
> prevent havoc when an update races with the commit worker. Or can that
> only happen for cursor commits, where we don't care when the cursor is
> very rarely misplaced because the hw takes an inconsistent update.

preventing the race is a combination of the locking (which recently
slightly changed and switched to per-crtc locks) and the
kms->wait_flush() which ensures previous updates have flushed.

BR,
-R

> -Daniel
>
>
> > BR,
> > -R
> >
> > > You probably still need the worker to push out the update at the right
> > > time, and I'm not sure what some good locking for that is. At least
> > > I'm not really seeing how you sync that worker against a racing update
> > > for the next cursor move.
> > > -Daniel
> > >
> > >
> > > > BR,
> > > > -R
> > > >
> > > > > I do agree though that you probably want this to be a real time fifo
> > > > > kthread worker, like for the vblank worker. Except now that I looked,
> > > > > I'm not sure it's actually working intended and correct.
> > > > > -Daniel
> > > > >
> > > > > > BR,
> > > > > > -R
> > > > > >
> > > > > > > -Daniel
> > > > > > >
> > > > > > > >
> > > > > > > >  drivers/gpu/drm/msm/adreno/a5xx_gpu.c     |  3 +--
> > > > > > > >  drivers/gpu/drm/msm/adreno/a5xx_preempt.c |  6 ++---
> > > > > > > >  drivers/gpu/drm/msm/adreno/a6xx_gmu.c     |  4 +--
> > > > > > > >  drivers/gpu/drm/msm/adreno/a6xx_gpu.c     |  4 +--
> > > > > > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   |  8 +++++-
> > > > > > > >  drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c  |  8 +++++-
> > > > > > > >  drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c  | 11 ++++++---
> > > > > > > >  drivers/gpu/drm/msm/disp/mdp_kms.h        |  9 +++++--
> > > > > > > >  drivers/gpu/drm/msm/msm_atomic.c          | 25 +++++++++++++++----
> > > > > > > >  drivers/gpu/drm/msm/msm_drv.h             |  3 ++-
> > > > > > > >  drivers/gpu/drm/msm/msm_gpu.c             | 30 +++++++++++++++--------
> > > > > > > >  drivers/gpu/drm/msm/msm_gpu.h             | 13 +++++++---
> > > > > > > >  drivers/gpu/drm/msm/msm_kms.h             | 23 ++++++++++++++---
> > > > > > > >  13 files changed, 104 insertions(+), 43 deletions(-)
> > > > > > > >
> > > > > > > > --
> > > > > > > > 2.26.2
> > > > > > > >
> > > > > > > > _______________________________________________
> > > > > > > > dri-devel mailing list
> > > > > > > > dri-devel@lists.freedesktop.org
> > > > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > > > >
> > > > > > > --
> > > > > > > Daniel Vetter
> > > > > > > Software Engineer, Intel Corporation
> > > > > > > http://blog.ffwll.ch
> > > > > > _______________________________________________
> > > > > > dri-devel mailing list
> > > > > > dri-devel@lists.freedesktop.org
> > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > >
> > > > >
> > > > >
> > > > > --
> > > > > Daniel Vetter
> > > > > Software Engineer, Intel Corporation
> > > > > http://blog.ffwll.ch
> > >
> > >
> > >
> > > --
> > > Daniel Vetter
> > > Software Engineer, Intel Corporation
> > > http://blog.ffwll.ch
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> http://blog.ffwll.ch

Re: [PATCH 0/3] drm/msm: kthread_worker conversion

[Daniel Vetter]

On Tue, Oct 20, 2020 at 01:26:29PM -0700, Rob Clark wrote:
> On Tue, Oct 20, 2020 at 11:14 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> >
> > On Tue, Oct 20, 2020 at 7:23 PM Rob Clark <robdclark@gmail.com> wrote:
> > >
> > > On Tue, Oct 20, 2020 at 10:02 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > >
> > > > On Tue, Oct 20, 2020 at 5:08 PM Rob Clark <robdclark@gmail.com> wrote:
> > > > >
> > > > > On Tue, Oct 20, 2020 at 7:29 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > > >
> > > > > > On Tue, Oct 20, 2020 at 4:01 PM Rob Clark <robdclark@gmail.com> wrote:
> > > > > > >
> > > > > > > On Tue, Oct 20, 2020 at 1:24 AM Daniel Vetter <daniel@ffwll.ch> wrote:
> > > > > > > >
> > > > > > > > On Mon, Oct 19, 2020 at 02:10:50PM -0700, Rob Clark wrote:
> > > > > > > > > From: Rob Clark <robdclark@chromium.org>
> > > > > > > > >
> > > > > > > > > In particular, converting the async atomic commit (for cursor updates,
> > > > > > > > > etc) to SCHED_FIFO kthread_worker helps with some cases where we
> > > > > > > > > wouldn't manage to flush the updates within the 1ms-before-vblank
> > > > > > > > > deadline resulting in fps drops when there is cursor movement.
> > > > > > > > >
> > > > > > > > > Rob Clark (3):
> > > > > > > > >   drm/msm/gpu: Convert retire/recover work to kthread_worker
> > > > > > > > >   drm/msm/kms: Update msm_kms_init/destroy
> > > > > > > > >   drm/msm/atomic: Convert to per-CRTC kthread_work
> > > > > > > >
> > > > > > > > So i915 has it's own commit worker already for $reasons, but I don't think
> > > > > > > > that's a good path to go down with more drivers. And the problem seems
> > > > > > > > entirely generic in nature ...
> > > > > > >
> > > > > > > I'm not *entirely* sure what your point is here?  This is just
> > > > > > > migrating away from a shared ordered wq to per-crtc kthread so that we
> > > > > > > don't miss vblank deadlines for silly reasons (and then stall on the
> > > > > > > next frame's pageflip because we are still waiting for the cursor
> > > > > > > update to latch).  Kind of like vblank-work but scheduled prior to,
> > > > > > > rather than after, vblank.
> > > > > > >
> > > > > > > And you're right that the problem is partially generic.. hw that (a)
> > > > > > > doesn't have true async (cursor and/or otherwise) updates, and (b) has
> > > > > > > various flush bits that latch register updates on vblank, is not that
> > > > > > > uncommon.  But the current atomic helper API would have to be a bit
> > > > > > > redesigned to look more like the interface between msm_atomic and the
> > > > > > > display backend.  That is a fair bit of churn for re-using a small bit
> > > > > > > of code.
> > > > > >
> > > > > > I was making some assumptions about what you're doing, and I was
> > > > > > wrong. So I went and tried to understand what's actually going on
> > > > > > here.
> > > > > >
> > > > > > I'm trying to understand what exactly you've added with that async msm
> > > > > > support 2d99ced787e3d. I think this breaks the state structure update
> > > > > > model, you can't access any ->state pointers from the commit functions
> > > > > > after you've called drm_atomic_helper_commit_hw_done, or you might
> > > > > > have a use after free. And that seems to be happening from this commit
> > > > > > work thing you added to your existing commit work that the atomic
> > > > > > helpers provide already.
> > > > > >
> > > > > > The various commit functions seem to grab various state objects by
> > > > > > just chasing pointers from the objects (instead of the
> > > > > > drm_atomic_state stuff), so this all feels like it's yolo
> > > > > > free-wheeling.
> > > > > >
> > > > > > You also seem to be using the async_commit stuff from the atomic
> > > > > > helpers (which is actually synchronous (i.e. blocking) from the pov of
> > > > > > how the code runs, but seems to be for mdp5 only and not others. Also
> > > > > > your can_do_async still checks for legacy_cursor_update (maybe a
> > > > > > leftover, or needed on !mdp5 platforms) and ->async_update.
> > > > > >
> > > > > > I'm thoroughly confused how this all works.
> > > > >
> > > > > The legacy_cursor_update is really the thing that motivated the async
> > > > > commit support in the first place.  Sadly we still have userspace that
> > > > > expects to be able to use legacy cursor API, and that it will be
> > > > > nonblocking (and not cause fps drop).  (I'm not a fan of the legacy
> > > > > cursor UAPI.. don't hate the player..)
> > > >
> > > > Yeah this is why we have these atomic_async_check/commit functions,
> > > > and msm is even using them for mdp5. Not hating the player here at
> > > > all.
> > > >
> > > > > The premise is to do everything in terms of crtc_mask, although yeah,
> > > > > it looks like there are a few points that need to look at things like
> > > > > crtc->state->active.  The only point in msm-atomic itself that does
> > > > > this is vblank_get/put(), possibly we can fix drm_vblank instead and
> > > > > drop that workaround (see 43906812eaab06423f56af5cca9a9fcdbb4ac454)
> > > > >
> > > > > The rest of the async part is really just supposed to be writing the
> > > > > appropriate flush reg(s) and waiting until flush completes, although
> > > > > dpu's excess layering makes this harder than it needs to be.
> > > > >
> > > > > In practice, the kms->wait_flush() at the top of
> > > > > msm_atomic_commit_tail() will block until a pending async commit
> > > > > completes (this is where we hit the fps drop if we miss vblank
> > > > > deadline), so I don't *think* you can trigger a use-after-free.  But
> > > > > the dpu code could be better cleaned up to have less obj->state
> > > > > dereference in the kms->flush_commit(crtc_mask)/etc path.
> > > >
> > > > Hm this is more or less what the atomic_async_commit/check stuff was
> > > > meant to help facilitate too, and now msm is using that for mdp5, but
> > > > not for other pieces. That seems very confusing.
> > > >
> > > > Also I'm not sure how this works if you still end up flushing anyway,
> > > > since then you'd be back to doing everything in-order. Or will an
> > > > normal atomic flip push all the cursor updates to the next frame (in
> > > > which case you really should be able to do this all with async helpers
> > > > we have instead of hand-rolling a bunch of it in strange places).
> > >
> > > So, "flush" from the core-atomic part is writing all the various
> > > registers (overlay scanout bo/format/position/etc).. this is all done
> > > at the normal time (ie. whenever we get the cursor update).  The only
> > > thing we defer until close-to-vblank is writing the hw flush registers
> > > (ie. registers with bitmasks of the various hw blocks to latch on
> > > vblank).
> > >
> > > So a cursor update applies the state normally, from the PoV of
> > > sequence of atomic updates.  But tries to defer writing the flush regs
> > > so we can merge in future cursor updates and/or pageflip into the same
> > > frame.
> > >
> > > Modulo the stuff that derefs kmsobj->state but shouldn't, I think (at
> > > least for hw that works this way with flush registers) this is a
> > > better approach to handling cursor updates.  The mdp5 async cursor
> > > stuff predates dpu, and I've just not had a chance to update mdp5 to
> > > use the new async flush path yet.
> >
> > The trouble is that this is moving back to legacy_cursor_update hack
> > instead of retiring it for good, so I'm not super thrilled about this.
> 
> state->async==true for cursor updates would work for me.. at the end
> of the day, it doesn't really matter that it is a cursor plane, or
> what the UAPI was, just that it is async.

Yeah I think that might be an option, if you cut your msm commit over to
that, same way async_commit does it.

> > Can't we do the register update from atomic_async_commit, and then
> > latch the timed worker, so that it all fits into the bigger thing?
> > Maybe also subsume the mdp5 stuff like that.
> 
> The current async update path replaced a previous async commit
> implementation, which might have been using atomic_async_commit?  I'd
> have to dig back thru git history.  The big problem with it was that
> an async commit could race with a sync/nonblock commit, and one of the
> paths could write flush regs while other is still updating regs.
> 
> The important thing about the current async approach is the separation
> of commit and flush, and the kms->wait_flush() at the top of
> commit_tail() which serializes hw updates and flush, so we don't have
> problems with racing commits.  I'm not sure how that would fit in with
> atomic_async_commit().

It's all new code.

Async commit should have some amount of sync, i.e. if there's a pending
noblocking commit and stuff like that. It might not be the right amount of
sync for msm, but it would be good to know what's missing or what's wrong
with the helpers.

Also you'd still need your worker to latch the registers in the last
moment ofc, since your hw doesn't do that automatically.

> > And that commit worker then probably needs the minimal amount of state
> > protected by a spinlock or similar, so they're not trampling over each
> > other. At least I'm still not seeing how you both make stuff async and
> > prevent havoc when an update races with the commit worker. Or can that
> > only happen for cursor commits, where we don't care when the cursor is
> > very rarely misplaced because the hw takes an inconsistent update.
> 
> preventing the race is a combination of the locking (which recently
> slightly changed and switched to per-crtc locks) and the
> kms->wait_flush() which ensures previous updates have flushed.

Hm I think that's largely what the async helpers do too. Well the locking
you'd need to keep in msm, since you also need that to sync with the
worker. But the flush and all that should be there I thought ...
-Daniel

> 
> BR,
> -R
> 
> > -Daniel
> >
> >
> > > BR,
> > > -R
> > >
> > > > You probably still need the worker to push out the update at the right
> > > > time, and I'm not sure what some good locking for that is. At least
> > > > I'm not really seeing how you sync that worker against a racing update
> > > > for the next cursor move.
> > > > -Daniel
> > > >
> > > >
> > > > > BR,
> > > > > -R
> > > > >
> > > > > > I do agree though that you probably want this to be a real time fifo
> > > > > > kthread worker, like for the vblank worker. Except now that I looked,
> > > > > > I'm not sure it's actually working intended and correct.
> > > > > > -Daniel
> > > > > >
> > > > > > > BR,
> > > > > > > -R
> > > > > > >
> > > > > > > > -Daniel
> > > > > > > >
> > > > > > > > >
> > > > > > > > >  drivers/gpu/drm/msm/adreno/a5xx_gpu.c     |  3 +--
> > > > > > > > >  drivers/gpu/drm/msm/adreno/a5xx_preempt.c |  6 ++---
> > > > > > > > >  drivers/gpu/drm/msm/adreno/a6xx_gmu.c     |  4 +--
> > > > > > > > >  drivers/gpu/drm/msm/adreno/a6xx_gpu.c     |  4 +--
> > > > > > > > >  drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c   |  8 +++++-
> > > > > > > > >  drivers/gpu/drm/msm/disp/mdp4/mdp4_kms.c  |  8 +++++-
> > > > > > > > >  drivers/gpu/drm/msm/disp/mdp5/mdp5_kms.c  | 11 ++++++---
> > > > > > > > >  drivers/gpu/drm/msm/disp/mdp_kms.h        |  9 +++++--
> > > > > > > > >  drivers/gpu/drm/msm/msm_atomic.c          | 25 +++++++++++++++----
> > > > > > > > >  drivers/gpu/drm/msm/msm_drv.h             |  3 ++-
> > > > > > > > >  drivers/gpu/drm/msm/msm_gpu.c             | 30 +++++++++++++++--------
> > > > > > > > >  drivers/gpu/drm/msm/msm_gpu.h             | 13 +++++++---
> > > > > > > > >  drivers/gpu/drm/msm/msm_kms.h             | 23 ++++++++++++++---
> > > > > > > > >  13 files changed, 104 insertions(+), 43 deletions(-)
> > > > > > > > >
> > > > > > > > > --
> > > > > > > > > 2.26.2
> > > > > > > > >
> > > > > > > > > _______________________________________________
> > > > > > > > > dri-devel mailing list
> > > > > > > > > dri-devel@lists.freedesktop.org
> > > > > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > > > > >
> > > > > > > > --
> > > > > > > > Daniel Vetter
> > > > > > > > Software Engineer, Intel Corporation
> > > > > > > > http://blog.ffwll.ch
> > > > > > > _______________________________________________
> > > > > > > dri-devel mailing list
> > > > > > > dri-devel@lists.freedesktop.org
> > > > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel
> > > > > >
> > > > > >
> > > > > >
> > > > > > --
> > > > > > Daniel Vetter
> > > > > > Software Engineer, Intel Corporation
> > > > > > http://blog.ffwll.ch
> > > >
> > > >
> > > >
> > > > --
> > > > Daniel Vetter
> > > > Software Engineer, Intel Corporation
> > > > http://blog.ffwll.ch
> >
> >
> >
> > --
> > Daniel Vetter
> > Software Engineer, Intel Corporation
> > http://blog.ffwll.ch

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch