drm/panthor: Fix sync-only jobs

A sync-only job is meant to provide a synchronization point on a queue, so we can't return a NULL fence there, we have to add a signal operation to the command stream which executes after all other previously submitted jobs are done. v2: - Fixed a UAF bug - Added R-bs

Boris Brezillon committed 2 years ago

Commit 7b6f9ec1 parent 1a9a7142 files · +38 −11Browse files

Unified · Split

2 files

drivers/gpu/drm/panthor/panthor_sched.c +33 −11
include/uapi/drm/panthor_drm.h +5 −0

drivers/gpu/drm/panthor/panthor_sched.c+33−11

 		/** @seqno: Sequence number of the last initialized fence. */
 		atomic64_t seqno;
+		/**
+		 * @last_fence: Fence of the last submitted job.
+		 *
+		 * We return this fence when we get an empty command stream.
+		 * This way, we are guaranteed that all earlier jobs have completed
+		 * when drm_sched_job::s_fence::finished without having to feed
+		 * the CS ring buffer with a dummy job that only signals the fence.
+		 */
+		struct dma_fence *last_fence;
 		/**
 		 * @in_flight_jobs: List containing all in-flight jobs.
 		 *
 	panthor_kernel_bo_destroy(queue->ringbuf);
 	panthor_kernel_bo_destroy(queue->iface.mem);
+	/* Release the last_fence we were holding, if any. */
+	dma_fence_put(queue->fence_ctx.last_fence);
 	kfree(queue);
 }
 		spin_lock(&queue->fence_ctx.lock);
 		list_for_each_entry_safe(job, job_tmp, &queue->fence_ctx.in_flight_jobs, node) {
-			if (!job->call_info.size)
-				continue;
 			if (syncobj->seqno < job->done_fence->seqno)
 				break;
 	static_assert(sizeof(call_instrs) % 64 == 0,
 		      "call_instrs is not aligned on a cacheline");
-	/* Stream size is zero, nothing to do => return a NULL fence and let
+	/* Stream size is zero, nothing to do except making sure all previously
-	 * drm_sched signal the parent.
+	 * submitted jobs are done before we signal the
+	 * drm_sched_job::s_fence::finished fence.
 	 */
-	if (!job->call_info.size)
+	if (!job->call_info.size) {
-		return NULL;
+		job->done_fence = dma_fence_get(queue->fence_ctx.last_fence);
+		return dma_fence_get(job->done_fence);
+	}
 	ret = pm_runtime_resume_and_get(ptdev->base.dev);
 	if (drm_WARN_ON(&ptdev->base, ret))
 		}
 	}
+	/* Update the last fence. */
+	dma_fence_put(queue->fence_ctx.last_fence);
+	queue->fence_ctx.last_fence = dma_fence_get(job->done_fence);
 	done_fence = dma_fence_get(job->done_fence);
 out_unlock:
 		goto err_put_job;
 	}
-	job->done_fence = kzalloc(sizeof(*job->done_fence), GFP_KERNEL);
+	/* Empty command streams don't need a fence, they'll pick the one from
-	if (!job->done_fence) {
+	 * the previously submitted job.
-		ret = -ENOMEM;
+	 */
-		goto err_put_job;
+	if (job->call_info.size) {
+		job->done_fence = kzalloc(sizeof(*job->done_fence), GFP_KERNEL);
+		if (!job->done_fence) {
+			ret = -ENOMEM;
+			goto err_put_job;
+		}
 	}
 	ret = drm_sched_job_init(&job->base,

include/uapi/drm/panthor_drm.h+5−0

 	 * Must be 64-bit/8-byte aligned (the size of a CS instruction)
 	 *
 	 * Can be zero if stream_addr is zero too.
+	 *
+	 * When the stream size is zero, the queue submit serves as a
+	 * synchronization point.
 	 */
 	__u32 stream_size;
 	 * ensure the GPU doesn't get garbage when reading the indirect command
 	 * stream buffers. If you want the cache flush to happen
 	 * unconditionally, pass a zero here.
+	 *
+	 * Ignored when stream_size is zero.
 	 */
 	__u32 latest_flush;