drm/v3d: Refactor job management.

The CL submission had two jobs embedded in an exec struct.  When I
added TFU support, I had to replicate some of the exec stuff and some
of the job stuff.  As I went to add CSD, it became clear that actually
what was in exec should just be in the two CL jobs, and it would let
us share a lot more code between the 4 queues.

v2: Fix missing error path in TFU ioctl's bo[] allocation.

Signed-off-by: Eric Anholt <eric@anholt.net>
Link: https://patchwork.freedesktop.org/patch/msgid/20190416225856.20264-3-eric@anholt.net
Acked-by: Rob Clark <robdclark@gmail.com>

drivers/gpu/drm/v3d/v3d_drv.h

@@ -67,8 +67,8 @@ struct v3d_dev {
 
 	struct work_struct overflow_mem_work;
 
-	struct v3d_exec_info *bin_job;
+	struct v3d_bin_job *bin_job;
-	struct v3d_exec_info *render_job;
+	struct v3d_render_job *render_job;
 	struct v3d_tfu_job *tfu_job;
 
 	struct v3d_queue_state queue[V3D_MAX_QUEUES];
@@ -117,7 +117,7 @@ struct v3d_bo {
 	struct drm_mm_node node;
 
 	/* List entry for the BO's position in
-	 * v3d_exec_info->unref_list
+	 * v3d_render_job->unref_list
 	 */
 	struct list_head unref_head;
 };
@@ -157,7 +157,15 @@ to_v3d_fence(struct dma_fence *fence)
 struct v3d_job {
 	struct drm_sched_job base;
 
-	struct v3d_exec_info *exec;
+	struct kref refcount;
+
+	struct v3d_dev *v3d;
+
+	/* This is the array of BOs that were looked up at the start
+	 * of submission.
+	 */
+	struct drm_gem_object **bo;
+	u32 bo_count;
 
 	/* An optional fence userspace can pass in for the job to depend on. */
 	struct dma_fence *in_fence;
@@ -165,59 +173,53 @@ struct v3d_job {
 	/* v3d fence to be signaled by IRQ handler when the job is complete. */
 	struct dma_fence *irq_fence;
 
+	/* scheduler fence for when the job is considered complete and
+	 * the BO reservations can be released.
+	 */
+	struct dma_fence *done_fence;
+
+	/* Callback for the freeing of the job on refcount going to 0. */
+	void (*free)(struct kref *ref);
+};
+
+struct v3d_bin_job {
+	struct v3d_job base;
+
 	/* GPU virtual addresses of the start/end of the CL job. */
 	u32 start, end;
 
 	u32 timedout_ctca, timedout_ctra;
-};
 
-struct v3d_exec_info {
+	/* Corresponding render job, for attaching our overflow memory. */
-	struct v3d_dev *v3d;
+	struct v3d_render_job *render;
-
-	struct v3d_job bin, render;
-
-	/* Fence for when the scheduler considers the binner to be
-	 * done, for render to depend on.
-	 */
-	struct dma_fence *bin_done_fence;
-
-	/* Fence for when the scheduler considers the render to be
-	 * done, for when the BOs reservations should be complete.
-	 */
-	struct dma_fence *render_done_fence;
-
-	struct kref refcount;
-
-	/* This is the array of BOs that were looked up at the start of exec. */
-	struct drm_gem_object **bo;
-	u32 bo_count;
-
-	/* List of overflow BOs used in the job that need to be
-	 * released once the job is complete.
-	 */
-	struct list_head unref_list;
 
 	/* Submitted tile memory allocation start/size, tile state. */
 	u32 qma, qms, qts;
 };
 
+struct v3d_render_job {
+	struct v3d_job base;
+
+	/* Optional fence for the binner, to depend on before starting
+	 * our job.
+	 */
+	struct dma_fence *bin_done_fence;
+
+	/* GPU virtual addresses of the start/end of the CL job. */
+	u32 start, end;
+
+	u32 timedout_ctca, timedout_ctra;
+
+	/* List of overflow BOs used in the job that need to be
+	 * released once the job is complete.
+	 */
+	struct list_head unref_list;
+};
+
 struct v3d_tfu_job {
-	struct drm_sched_job base;
+	struct v3d_job base;
 
 	struct drm_v3d_submit_tfu args;
-
-	/* An optional fence userspace can pass in for the job to depend on. */
-	struct dma_fence *in_fence;
-
-	/* v3d fence to be signaled by IRQ handler when the job is complete. */
-	struct dma_fence *irq_fence;
-
-	struct v3d_dev *v3d;
-
-	struct kref refcount;
-
-	/* This is the array of BOs that were looked up at the start of exec. */
-	struct drm_gem_object *bo[4];
 };
 
 /**
@@ -283,8 +285,7 @@ int v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
 			 struct drm_file *file_priv);
 int v3d_wait_bo_ioctl(struct drm_device *dev, void *data,
 		      struct drm_file *file_priv);
-void v3d_exec_put(struct v3d_exec_info *exec);
+void v3d_job_put(struct v3d_job *job);
-void v3d_tfu_job_put(struct v3d_tfu_job *exec);
 void v3d_reset(struct v3d_dev *v3d);
 void v3d_invalidate_caches(struct v3d_dev *v3d);
 

drivers/gpu/drm/v3d/v3d_irq.c

@@ -62,7 +62,7 @@ v3d_overflow_mem_work(struct work_struct *work)
 	}
 
 	drm_gem_object_get(obj);
-	list_add_tail(&bo->unref_head, &v3d->bin_job->unref_list);
+	list_add_tail(&bo->unref_head, &v3d->bin_job->render->unref_list);
 	spin_unlock_irqrestore(&v3d->job_lock, irqflags);
 
 	V3D_CORE_WRITE(0, V3D_PTB_BPOA, bo->node.start << PAGE_SHIFT);
@@ -96,7 +96,7 @@ v3d_irq(int irq, void *arg)
 
 	if (intsts & V3D_INT_FLDONE) {
 		struct v3d_fence *fence =
-			to_v3d_fence(v3d->bin_job->bin.irq_fence);
+			to_v3d_fence(v3d->bin_job->base.irq_fence);
 
 		trace_v3d_bcl_irq(&v3d->drm, fence->seqno);
 		dma_fence_signal(&fence->base);
@@ -105,7 +105,7 @@ v3d_irq(int irq, void *arg)
 
 	if (intsts & V3D_INT_FRDONE) {
 		struct v3d_fence *fence =
-			to_v3d_fence(v3d->render_job->render.irq_fence);
+			to_v3d_fence(v3d->render_job->base.irq_fence);
 
 		trace_v3d_rcl_irq(&v3d->drm, fence->seqno);
 		dma_fence_signal(&fence->base);
@@ -141,7 +141,7 @@ v3d_hub_irq(int irq, void *arg)
 
 	if (intsts & V3D_HUB_INT_TFUC) {
 		struct v3d_fence *fence =
-			to_v3d_fence(v3d->tfu_job->irq_fence);
+			to_v3d_fence(v3d->tfu_job->base.irq_fence);
 
 		trace_v3d_tfu_irq(&v3d->drm, fence->seqno);
 		dma_fence_signal(&fence->base);

drivers/gpu/drm/v3d/v3d_sched.c

@@ -30,10 +30,22 @@ to_v3d_job(struct drm_sched_job *sched_job)
 	return container_of(sched_job, struct v3d_job, base);
 }
 
+static struct v3d_bin_job *
+to_bin_job(struct drm_sched_job *sched_job)
+{
+	return container_of(sched_job, struct v3d_bin_job, base.base);
+}
+
+static struct v3d_render_job *
+to_render_job(struct drm_sched_job *sched_job)
+{
+	return container_of(sched_job, struct v3d_render_job, base.base);
+}
+
 static struct v3d_tfu_job *
 to_tfu_job(struct drm_sched_job *sched_job)
 {
-	return container_of(sched_job, struct v3d_tfu_job, base);
+	return container_of(sched_job, struct v3d_tfu_job, base.base);
 }
 
 static void
@@ -42,31 +54,20 @@ v3d_job_free(struct drm_sched_job *sched_job)
 	struct v3d_job *job = to_v3d_job(sched_job);
 
 	drm_sched_job_cleanup(sched_job);
-
+	v3d_job_put(job);
-	v3d_exec_put(job->exec);
-}
-
-static void
-v3d_tfu_job_free(struct drm_sched_job *sched_job)
-{
-	struct v3d_tfu_job *job = to_tfu_job(sched_job);
-
-	drm_sched_job_cleanup(sched_job);
-
-	v3d_tfu_job_put(job);
 }
 
 /**
- * Returns the fences that the bin or render job depends on, one by one.
+ * Returns the fences that the job depends on, one by one.
- * v3d_job_run() won't be called until all of them have been signaled.
+ *
+ * If placed in the scheduler's .dependency method, the corresponding
+ * .run_job won't be called until all of them have been signaled.
  */
 static struct dma_fence *
 v3d_job_dependency(struct drm_sched_job *sched_job,
 		   struct drm_sched_entity *s_entity)
 {
 	struct v3d_job *job = to_v3d_job(sched_job);
-	struct v3d_exec_info *exec = job->exec;
-	enum v3d_queue q = job == &exec->bin ? V3D_BIN : V3D_RENDER;
 	struct dma_fence *fence;
 
 	fence = job->in_fence;
@@ -75,113 +76,132 @@ v3d_job_dependency(struct drm_sched_job *sched_job,
 		return fence;
 	}
 
-	if (q == V3D_RENDER) {
-		/* If we had a bin job, the render job definitely depends on
-		 * it. We first have to wait for bin to be scheduled, so that
-		 * its done_fence is created.
-		 */
-		fence = exec->bin_done_fence;
-		if (fence) {
-			exec->bin_done_fence = NULL;
-			return fence;
-		}
-	}
-
 	/* XXX: Wait on a fence for switching the GMP if necessary,
 	 * and then do so.
 	 */
 
-	return fence;
-}
-
-/**
- * Returns the fences that the TFU job depends on, one by one.
- * v3d_tfu_job_run() won't be called until all of them have been
- * signaled.
- */
-static struct dma_fence *
-v3d_tfu_job_dependency(struct drm_sched_job *sched_job,
-		       struct drm_sched_entity *s_entity)
-{
-	struct v3d_tfu_job *job = to_tfu_job(sched_job);
-	struct dma_fence *fence;
-
-	fence = job->in_fence;
-	if (fence) {
-		job->in_fence = NULL;
-		return fence;
-	}
-
 	return NULL;
 }
 
-static struct dma_fence *v3d_job_run(struct drm_sched_job *sched_job)
+/**
+ * Returns the fences that the render job depends on, one by one.
+ * v3d_job_run() won't be called until all of them have been signaled.
+ */
+static struct dma_fence *
+v3d_render_job_dependency(struct drm_sched_job *sched_job,
+			  struct drm_sched_entity *s_entity)
 {
-	struct v3d_job *job = to_v3d_job(sched_job);
+	struct v3d_render_job *job = to_render_job(sched_job);
-	struct v3d_exec_info *exec = job->exec;
+	struct dma_fence *fence;
-	enum v3d_queue q = job == &exec->bin ? V3D_BIN : V3D_RENDER;
+
-	struct v3d_dev *v3d = exec->v3d;
+	fence = v3d_job_dependency(sched_job, s_entity);
+	if (fence)
+		return fence;
+
+	/* If we had a bin job, the render job definitely depends on
+	 * it. We first have to wait for bin to be scheduled, so that
+	 * its done_fence is created.
+	 */
+	fence = job->bin_done_fence;
+	if (fence) {
+		job->bin_done_fence = NULL;
+		return fence;
+	}
+
+	return fence;
+}
+
+static struct dma_fence *v3d_bin_job_run(struct drm_sched_job *sched_job)
+{
+	struct v3d_bin_job *job = to_bin_job(sched_job);
+	struct v3d_dev *v3d = job->base.v3d;
 	struct drm_device *dev = &v3d->drm;
 	struct dma_fence *fence;
 	unsigned long irqflags;
 
-	if (unlikely(job->base.s_fence->finished.error))
+	if (unlikely(job->base.base.s_fence->finished.error))
 		return NULL;
 
 	/* Lock required around bin_job update vs
 	 * v3d_overflow_mem_work().
 	 */
 	spin_lock_irqsave(&v3d->job_lock, irqflags);
-	if (q == V3D_BIN) {
+	v3d->bin_job = job;
-		v3d->bin_job = job->exec;
+	/* Clear out the overflow allocation, so we don't
-
+	 * reuse the overflow attached to a previous job.
-		/* Clear out the overflow allocation, so we don't
+	 */
-		 * reuse the overflow attached to a previous job.
+	V3D_CORE_WRITE(0, V3D_PTB_BPOS, 0);
-		 */
-		V3D_CORE_WRITE(0, V3D_PTB_BPOS, 0);
-	} else {
-		v3d->render_job = job->exec;
-	}
 	spin_unlock_irqrestore(&v3d->job_lock, irqflags);
 
-	/* Can we avoid this flush when q==RENDER?  We need to be
-	 * careful of scheduling, though -- imagine job0 rendering to
-	 * texture and job1 reading, and them being executed as bin0,
-	 * bin1, render0, render1, so that render1's flush at bin time
-	 * wasn't enough.
-	 */
 	v3d_invalidate_caches(v3d);
 
-	fence = v3d_fence_create(v3d, q);
+	fence = v3d_fence_create(v3d, V3D_BIN);
 	if (IS_ERR(fence))
 		return NULL;
 
-	if (job->irq_fence)
+	if (job->base.irq_fence)
-		dma_fence_put(job->irq_fence);
+		dma_fence_put(job->base.irq_fence);
-	job->irq_fence = dma_fence_get(fence);
+	job->base.irq_fence = dma_fence_get(fence);
 
-	trace_v3d_submit_cl(dev, q == V3D_RENDER, to_v3d_fence(fence)->seqno,
+	trace_v3d_submit_cl(dev, false, to_v3d_fence(fence)->seqno,
 			    job->start, job->end);
 
-	if (q == V3D_BIN) {
-		if (exec->qma) {
-			V3D_CORE_WRITE(0, V3D_CLE_CT0QMA, exec->qma);
-			V3D_CORE_WRITE(0, V3D_CLE_CT0QMS, exec->qms);
-		}
-		if (exec->qts) {
-			V3D_CORE_WRITE(0, V3D_CLE_CT0QTS,
-				       V3D_CLE_CT0QTS_ENABLE |
-				       exec->qts);
-		}
-	} else {
-		/* XXX: Set the QCFG */
-	}
-
 	/* Set the current and end address of the control list.
 	 * Writing the end register is what starts the job.
 	 */
-	V3D_CORE_WRITE(0, V3D_CLE_CTNQBA(q), job->start);
+	if (job->qma) {
-	V3D_CORE_WRITE(0, V3D_CLE_CTNQEA(q), job->end);
+		V3D_CORE_WRITE(0, V3D_CLE_CT0QMA, job->qma);
+		V3D_CORE_WRITE(0, V3D_CLE_CT0QMS, job->qms);
+	}
+	if (job->qts) {
+		V3D_CORE_WRITE(0, V3D_CLE_CT0QTS,
+			       V3D_CLE_CT0QTS_ENABLE |
+			       job->qts);
+	}
+	V3D_CORE_WRITE(0, V3D_CLE_CT0QBA, job->start);
+	V3D_CORE_WRITE(0, V3D_CLE_CT0QEA, job->end);
+
+	return fence;
+}
+
+static struct dma_fence *v3d_render_job_run(struct drm_sched_job *sched_job)
+{
+	struct v3d_render_job *job = to_render_job(sched_job);
+	struct v3d_dev *v3d = job->base.v3d;
+	struct drm_device *dev = &v3d->drm;
+	struct dma_fence *fence;
+
+	if (unlikely(job->base.base.s_fence->finished.error))
+		return NULL;
+
+	v3d->render_job = job;
+
+	/* Can we avoid this flush?  We need to be careful of
+	 * scheduling, though -- imagine job0 rendering to texture and
+	 * job1 reading, and them being executed as bin0, bin1,
+	 * render0, render1, so that render1's flush at bin time
+	 * wasn't enough.
+	 */
+	v3d_invalidate_caches(v3d);
+
+	fence = v3d_fence_create(v3d, V3D_RENDER);
+	if (IS_ERR(fence))
+		return NULL;
+
+	if (job->base.irq_fence)
+		dma_fence_put(job->base.irq_fence);
+	job->base.irq_fence = dma_fence_get(fence);
+
+	trace_v3d_submit_cl(dev, true, to_v3d_fence(fence)->seqno,
+			    job->start, job->end);
+
+	/* XXX: Set the QCFG */
+
+	/* Set the current and end address of the control list.
+	 * Writing the end register is what starts the job.
+	 */
+	V3D_CORE_WRITE(0, V3D_CLE_CT1QBA, job->start);
+	V3D_CORE_WRITE(0, V3D_CLE_CT1QEA, job->end);
 
 	return fence;
 }
@@ -190,7 +210,7 @@ static struct dma_fence *
 v3d_tfu_job_run(struct drm_sched_job *sched_job)
 {
 	struct v3d_tfu_job *job = to_tfu_job(sched_job);
-	struct v3d_dev *v3d = job->v3d;
+	struct v3d_dev *v3d = job->base.v3d;
 	struct drm_device *dev = &v3d->drm;
 	struct dma_fence *fence;
 
@@ -199,9 +219,9 @@ v3d_tfu_job_run(struct drm_sched_job *sched_job)
 		return NULL;
 
 	v3d->tfu_job = job;
-	if (job->irq_fence)
+	if (job->base.irq_fence)
-		dma_fence_put(job->irq_fence);
+		dma_fence_put(job->base.irq_fence);
-	job->irq_fence = dma_fence_get(fence);
+	job->base.irq_fence = dma_fence_get(fence);
 
 	trace_v3d_submit_tfu(dev, to_v3d_fence(fence)->seqno);
 
@@ -251,51 +271,74 @@ v3d_gpu_reset_for_timeout(struct v3d_dev *v3d, struct drm_sched_job *sched_job)
 	mutex_unlock(&v3d->reset_lock);
 }
 
+/* If the current address or return address have changed, then the GPU
+ * has probably made progress and we should delay the reset.  This
+ * could fail if the GPU got in an infinite loop in the CL, but that
+ * is pretty unlikely outside of an i-g-t testcase.
+ */
 static void
-v3d_job_timedout(struct drm_sched_job *sched_job)
+v3d_cl_job_timedout(struct drm_sched_job *sched_job, enum v3d_queue q,
+		    u32 *timedout_ctca, u32 *timedout_ctra)
 {
 	struct v3d_job *job = to_v3d_job(sched_job);
-	struct v3d_exec_info *exec = job->exec;
+	struct v3d_dev *v3d = job->v3d;
-	struct v3d_dev *v3d = exec->v3d;
+	u32 ctca = V3D_CORE_READ(0, V3D_CLE_CTNCA(q));
-	enum v3d_queue job_q = job == &exec->bin ? V3D_BIN : V3D_RENDER;
+	u32 ctra = V3D_CORE_READ(0, V3D_CLE_CTNRA(q));
-	u32 ctca = V3D_CORE_READ(0, V3D_CLE_CTNCA(job_q));
-	u32 ctra = V3D_CORE_READ(0, V3D_CLE_CTNRA(job_q));
 
-	/* If the current address or return address have changed, then
+	if (*timedout_ctca != ctca || *timedout_ctra != ctra) {
-	 * the GPU has probably made progress and we should delay the
+		*timedout_ctca = ctca;
-	 * reset.  This could fail if the GPU got in an infinite loop
+		*timedout_ctra = ctra;
-	 * in the CL, but that is pretty unlikely outside of an i-g-t
-	 * testcase.
-	 */
-	if (job->timedout_ctca != ctca || job->timedout_ctra != ctra) {
-		job->timedout_ctca = ctca;
-		job->timedout_ctra = ctra;
 		return;
 	}
 
 	v3d_gpu_reset_for_timeout(v3d, sched_job);
 }
 
+static void
+v3d_bin_job_timedout(struct drm_sched_job *sched_job)
+{
+	struct v3d_bin_job *job = to_bin_job(sched_job);
+
+	v3d_cl_job_timedout(sched_job, V3D_BIN,
+			    &job->timedout_ctca, &job->timedout_ctra);
+}
+
+static void
+v3d_render_job_timedout(struct drm_sched_job *sched_job)
+{
+	struct v3d_render_job *job = to_render_job(sched_job);
+
+	v3d_cl_job_timedout(sched_job, V3D_RENDER,
+			    &job->timedout_ctca, &job->timedout_ctra);
+}
+
 static void
 v3d_tfu_job_timedout(struct drm_sched_job *sched_job)
 {
-	struct v3d_tfu_job *job = to_tfu_job(sched_job);
+	struct v3d_job *job = to_v3d_job(sched_job);
 
 	v3d_gpu_reset_for_timeout(job->v3d, sched_job);
 }
 
-static const struct drm_sched_backend_ops v3d_sched_ops = {
+static const struct drm_sched_backend_ops v3d_bin_sched_ops = {
 	.dependency = v3d_job_dependency,
-	.run_job = v3d_job_run,
+	.run_job = v3d_bin_job_run,
-	.timedout_job = v3d_job_timedout,
+	.timedout_job = v3d_bin_job_timedout,
-	.free_job = v3d_job_free
+	.free_job = v3d_job_free,
+};
+
+static const struct drm_sched_backend_ops v3d_render_sched_ops = {
+	.dependency = v3d_render_job_dependency,
+	.run_job = v3d_render_job_run,
+	.timedout_job = v3d_render_job_timedout,
+	.free_job = v3d_job_free,
 };
 
 static const struct drm_sched_backend_ops v3d_tfu_sched_ops = {
-	.dependency = v3d_tfu_job_dependency,
+	.dependency = v3d_job_dependency,
 	.run_job = v3d_tfu_job_run,
 	.timedout_job = v3d_tfu_job_timedout,
-	.free_job = v3d_tfu_job_free
+	.free_job = v3d_job_free,
 };
 
 int
@@ -307,7 +350,7 @@ v3d_sched_init(struct v3d_dev *v3d)
 	int ret;
 
 	ret = drm_sched_init(&v3d->queue[V3D_BIN].sched,
-			     &v3d_sched_ops,
+			     &v3d_bin_sched_ops,
 			     hw_jobs_limit, job_hang_limit,
 			     msecs_to_jiffies(hang_limit_ms),
 			     "v3d_bin");
@@ -317,7 +360,7 @@ v3d_sched_init(struct v3d_dev *v3d)
 	}
 
 	ret = drm_sched_init(&v3d->queue[V3D_RENDER].sched,
-			     &v3d_sched_ops,
+			     &v3d_render_sched_ops,
 			     hw_jobs_limit, job_hang_limit,
 			     msecs_to_jiffies(hang_limit_ms),
 			     "v3d_render");