Re: [PATCH v3 08/14] drm/v3d: Introduce struct v3d_submit and convert CL/TFU/CSD ioctls

[Tvrtko Ursulin <tvrtko.ursulin@igalia.com>]

On 03/06/2026 23:25, Maíra Canal wrote:
> As the V3D driver grew with time, different types of submission were added
> and the submission code grew more complex, but the driver stuck with the
> same abstractions.
> 
> Nowadays, the submission ioctls don't submit a single job, but a
> chain of jobs:
> 
> 1. v3d_submit_cl_ioctl() submits a BIN job (optional), RENDER job
>     (mandatory), and a CLEAN_CACHE job (optional).
> 2. v3d_submit_csd_ioctl() submits a CSD, and a CLEAN_CACHE job.
> 3. v3d_submit_tfu_ioctl() submits a TFU job.
> 
> Therefore, each ioctl submits a chain of jobs in which each job depends on
> the previous one. However, this concept is not well represented in software
> at the moment.
> 
> To address this, introduce a new concept: the struct v3d_submit, which
> groups the submission state and represents the submission chain formed by
> an ordered array of jobs.
> 
> Add new helpers to allocate, add jobs to the chain and submit jobs to
> the scheduler, all based on the new struct. Convert v3d_submit_cl_ioctl(),
> v3d_submit_tfu_ioctl() and v3d_submit_csd_ioctl() to the new pattern. Each
> ioctl now follows the same flow: add jobs -> attach perfmon -> lookup BOs
> -> lock reservations -> submit chain -> attach fences -> put jobs.
> 
> The CPU ioctl is left on the old helpers for now; its indirect CSD path
> requires some restructuring that will be addressed in the next few
> commits.
> 
> Signed-off-by: Maíra Canal <mcanal@igalia.com>
> ---
>   drivers/gpu/drm/v3d/v3d_drv.h    |  21 +++
>   drivers/gpu/drm/v3d/v3d_submit.c | 386 ++++++++++++++++++++++-----------------
>   2 files changed, 241 insertions(+), 166 deletions(-)
> 
> diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h
> index 07bbdf52c33c..fae0c53e7aa3 100644
> --- a/drivers/gpu/drm/v3d/v3d_drv.h
> +++ b/drivers/gpu/drm/v3d/v3d_drv.h
> @@ -288,6 +288,27 @@ to_v3d_fence(struct dma_fence *fence)
>   #define V3D_CORE_READ(core, offset) readl(v3d->core_regs[core] + offset)
>   #define V3D_CORE_WRITE(core, offset, val) writel(val, v3d->core_regs[core] + offset)
>   
> +#define V3D_MAX_JOBS_PER_SUBMISSION 3
> +
> +/* Per-ioctl submission context */
> +struct v3d_submit {
> +	struct v3d_dev *v3d;
> +
> +	struct drm_file *file_priv;
> +
> +	/* DRM exec context for this submission. */
> +	struct drm_exec exec;
> +
> +	/* Ordered array of jobs forming the submission chain. Jobs are
> +	 * appended via v3d_submit_add_job(), then chained and pushed to
> +	 * the scheduler by v3d_submit_jobs().
> +	 */
> +	struct v3d_job *jobs[V3D_MAX_JOBS_PER_SUBMISSION];
> +
> +	/* Number of jobs currently in @jobs. */
> +	u32 job_count;
> +};
> +
>   struct v3d_job {
>   	struct drm_sched_job base;
>   
> diff --git a/drivers/gpu/drm/v3d/v3d_submit.c b/drivers/gpu/drm/v3d/v3d_submit.c
> index d80afbf2c03b..fcaf3a6cfddc 100644
> --- a/drivers/gpu/drm/v3d/v3d_submit.c
> +++ b/drivers/gpu/drm/v3d/v3d_submit.c
> @@ -261,17 +261,105 @@ v3d_job_init(struct v3d_dev *v3d, struct drm_file *file_priv,
>   	return ret;
>   }
>   
> -static void
> -v3d_push_job(struct v3d_job *job)
> +static const struct {
> +	size_t size;
> +	void (*free)(struct kref *ref);
> +} v3d_job_types[] = {
> +	[V3D_BIN]		= { sizeof(struct v3d_bin_job), v3d_job_free },
> +	[V3D_RENDER]		= { sizeof(struct v3d_render_job), v3d_render_job_free },
> +	[V3D_TFU]		= { sizeof(struct v3d_tfu_job), v3d_job_free },
> +	[V3D_CSD]		= { sizeof(struct v3d_csd_job), v3d_job_free },
> +	[V3D_CACHE_CLEAN]	= { sizeof(struct v3d_job), v3d_job_free },
> +	[V3D_CPU]		= { sizeof(struct v3d_cpu_job), v3d_cpu_job_free },
> +};
> +
> +static struct v3d_job *
> +v3d_submit_add_job(struct v3d_submit *submit, enum v3d_queue queue)
>   {
> -	drm_sched_job_arm(&job->base);
> +	struct v3d_file_priv *v3d_priv = submit->file_priv->driver_priv;
> +	struct v3d_dev *v3d = submit->v3d;
> +	struct v3d_job *job;
> +	int ret;
>   
> -	job->done_fence = dma_fence_get(&job->base.s_fence->finished);
> +	if (queue >= V3D_MAX_QUEUES)
> +		return ERR_PTR(-EINVAL);
>   
> -	/* put by scheduler job completion */
> -	kref_get(&job->refcount);
> +	job = kzalloc(v3d_job_types[queue].size, GFP_KERNEL);
> +	if (!job)
> +		return ERR_PTR(-ENOMEM);
>   
> -	drm_sched_entity_push_job(&job->base);
> +	job->v3d = v3d;
> +	job->queue = queue;
> +	job->file_priv = v3d_priv;
> +	job->free = v3d_job_types[queue].free;
> +
> +	ret = drm_sched_job_init(&job->base, &v3d_priv->sched_entity[queue],
> +				 1, v3d_priv, submit->file_priv->client_id);
> +	if (ret)
> +		goto fail_free;
> +
> +	/* CPU jobs don't require hardware resources */
> +	if (queue != V3D_CPU) {
> +		ret = v3d_pm_runtime_get(v3d);
> +		if (ret)
> +			goto fail_sched_job;
> +		job->has_pm_ref = true;
> +	}
> +
> +	kref_init(&job->refcount);
> +
> +	job->client_stats = v3d_stats_get(v3d_priv->stats[queue]);
> +	job->global_stats = v3d_stats_get(v3d->queue[queue].stats);
> +
> +	submit->jobs[submit->job_count++] = job;
> +
> +	return job;
> +
> +fail_sched_job:
> +	drm_sched_job_cleanup(&job->base);
> +fail_free:
> +	kfree(job);
> +	return ERR_PTR(ret);
> +}
> +
> +static void
> +v3d_submit_put_jobs(struct v3d_submit *submit)
> +{
> +	for (int i = 0; i < submit->job_count; i++)
> +		v3d_job_put(submit->jobs[i]);
> +}
> +
> +static void
> +v3d_submit_cleanup_jobs(struct v3d_submit *submit)
> +{
> +	for (int i = 0; i < submit->job_count; i++)
> +		v3d_job_cleanup(submit->jobs[i]);
> +}
> +
> +static int
> +v3d_attach_perfmon_to_jobs(struct v3d_submit *submit, u32 perfmon_id)
> +{
> +	struct v3d_file_priv *v3d_priv = submit->file_priv->driver_priv;
> +	struct v3d_dev *v3d = submit->v3d;
> +	struct v3d_perfmon *perfmon;
> +
> +	if (!perfmon_id)
> +		return 0;
> +
> +	if (v3d->global_perfmon)
> +		return -EAGAIN;
> +
> +	perfmon = v3d_perfmon_find(v3d_priv, perfmon_id);
> +	if (!perfmon)
> +		return -ENOENT;
> +
> +	for (int i = 0; i < submit->job_count; i++) {
> +		submit->jobs[i]->perfmon = perfmon;
> +		if (i != 0)
> +			v3d_perfmon_get(perfmon);
> +	}
> +
> +	return 0;
>   }
>   
>   static void
> @@ -316,6 +404,45 @@ v3d_attach_fences_and_unlock_reservation(struct drm_file *file_priv,
>   	}
>   }
>   
> +static void
> +v3d_push_job(struct v3d_job *job)
> +{
> +	drm_sched_job_arm(&job->base);
> +
> +	job->done_fence = dma_fence_get(&job->base.s_fence->finished);
> +
> +	/* put by scheduler job completion */
> +	kref_get(&job->refcount);
> +
> +	drm_sched_entity_push_job(&job->base);
> +}
> +
> +static int
> +v3d_submit_jobs(struct v3d_submit *submit)
> +{
> +	struct v3d_dev *v3d = submit->v3d;
> +	int ret = 0;
> +
> +	mutex_lock(&v3d->sched_lock);
> +
> +	for (int i = 0; i < submit->job_count; i++) {
> +		struct v3d_job *job = submit->jobs[i];
> +
> +		v3d_push_job(job);
> +
> +		if (i + 1 < submit->job_count) {
> +			ret = drm_sched_job_add_dependency(&submit->jobs[i + 1]->base,
> +							   dma_fence_get(job->done_fence));

Is there a theoretical race where job->done_fence could be completed and 
last reference dropped between push and here?

> +			if (ret)
> +				goto err;
> +		}
> +	}
> +
> +err:
> +	mutex_unlock(&v3d->sched_lock);
> +	return ret;
> +}
> +
>   static int
>   v3d_setup_csd_jobs_and_bos(struct drm_file *file_priv,
>   			   struct v3d_dev *v3d,
> @@ -921,18 +1048,15 @@ int
>   v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
>   		    struct drm_file *file_priv)
>   {
> -	struct v3d_dev *v3d = to_v3d_dev(dev);
> -	struct v3d_file_priv *v3d_priv = file_priv->driver_priv;
> +	struct v3d_submit submit = { .v3d = to_v3d_dev(dev), .file_priv = file_priv };
>   	struct drm_v3d_submit_cl *args = data;
>   	struct v3d_submit_ext se = {0};
>   	struct v3d_bin_job *bin = NULL;
>   	struct v3d_render_job *render = NULL;
>   	struct v3d_job *clean_job = NULL;

render and clean_job do not need to be initialised.

> -	struct v3d_job *last_job;
> -	struct drm_exec exec;
> -	int ret = 0;
> +	int ret;
>   
> -	trace_v3d_submit_cl_ioctl(&v3d->drm, args->rcl_start, args->rcl_end);
> +	trace_v3d_submit_cl_ioctl(dev, args->rcl_start, args->rcl_end);
>   
>   	if (args->pad)
>   		return -EINVAL;
> @@ -952,30 +1076,10 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
>   		}
>   	}
>   
> -	ret = v3d_job_allocate(v3d, (void *)&render, sizeof(*render));
> -	if (ret)
> -		return ret;
> -
> -	ret = v3d_job_init(v3d, file_priv, &render->base,
> -			   v3d_render_job_free, args->in_sync_rcl, &se, V3D_RENDER);
> -	if (ret) {
> -		v3d_job_deallocate((void *)&render);
> -		goto fail;
> -	}
> -
> -	render->start = args->rcl_start;
> -	render->end = args->rcl_end;
> -	INIT_LIST_HEAD(&render->unref_list);
> -
>   	if (args->bcl_start != args->bcl_end) {
> -		ret = v3d_job_allocate(v3d, (void *)&bin, sizeof(*bin));
> -		if (ret)
> -			goto fail;
> -
> -		ret = v3d_job_init(v3d, file_priv, &bin->base,
> -				   v3d_job_free, args->in_sync_bcl, &se, V3D_BIN);
> -		if (ret) {
> -			v3d_job_deallocate((void *)&bin);
> +		bin = (struct v3d_bin_job *) v3d_submit_add_job(&submit, V3D_BIN);
> +		if (IS_ERR(bin)) {
> +			ret = PTR_ERR(bin);
>   			goto fail;
>   		}
>   
> @@ -984,99 +1088,71 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
>   		bin->qma = args->qma;
>   		bin->qms = args->qms;
>   		bin->qts = args->qts;
> -		bin->render = render;
> -	}
>   
> -	if (args->flags & DRM_V3D_SUBMIT_CL_FLUSH_CACHE) {
> -		ret = v3d_job_allocate(v3d, (void *)&clean_job, sizeof(*clean_job));
> +		ret = v3d_job_add_syncobjs(&bin->base, file_priv, args->in_sync_bcl,
> +					   &se);
>   		if (ret)
>   			goto fail;
> -
> -		ret = v3d_job_init(v3d, file_priv, clean_job,
> -				   v3d_job_free, 0, NULL, V3D_CACHE_CLEAN);
> -		if (ret) {
> -			v3d_job_deallocate((void *)&clean_job);
> -			goto fail;
> -		}
> -
> -		last_job = clean_job;
> -	} else {
> -		last_job = &render->base;
>   	}
>   
> -	ret = v3d_lookup_bos(dev, file_priv, last_job,
> +	render = (struct v3d_render_job *) v3d_submit_add_job(&submit, V3D_RENDER);
> +	if (IS_ERR(render)) {
> +		ret = PTR_ERR(render);
> +		goto fail;
> +	}
> +
> +	INIT_LIST_HEAD(&render->unref_list);
> +	render->start = args->rcl_start;
> +	render->end = args->rcl_end;
> +
> +	if (bin)
> +		bin->render = render;
> +
> +	ret = v3d_job_add_syncobjs(&render->base, file_priv, args->in_sync_rcl, &se);
> +	if (ret)
> +		goto fail;
> +
> +	if (args->flags & DRM_V3D_SUBMIT_CL_FLUSH_CACHE) {
> +		clean_job = v3d_submit_add_job(&submit, V3D_CACHE_CLEAN);
> +		if (IS_ERR(clean_job)) {
> +			ret = PTR_ERR(clean_job);
> +			goto fail;
> +		}
> +	}
> +
> +	ret = v3d_attach_perfmon_to_jobs(&submit, args->perfmon_id);
> +	if (ret)
> +		goto fail;
> +
> +	ret = v3d_lookup_bos(dev, file_priv,
> +			     submit.jobs[submit.job_count - 1],

submit.jobs[submit.job_count - 1] is repeated four times in the function 
so you could assign it to a last_job local for more readability. Up to you.

>   			     args->bo_handles, args->bo_handle_count);
>   	if (ret)
>   		goto fail;
>   
> -	ret = v3d_lock_bo_reservations(last_job, &exec);
> +	ret = v3d_lock_bo_reservations(submit.jobs[submit.job_count - 1],
> +				       &submit.exec);
>   	if (ret)
>   		goto fail;
>   
> -	if (args->perfmon_id) {
> -		if (v3d->global_perfmon) {
> -			ret = -EAGAIN;
> -			goto fail_perfmon;
> -		}
> -
> -		render->base.perfmon = v3d_perfmon_find(v3d_priv,
> -							args->perfmon_id);
> -
> -		if (!render->base.perfmon) {
> -			ret = -ENOENT;
> -			goto fail_perfmon;
> -		}
> -	}
> -
> -	mutex_lock(&v3d->sched_lock);
> -	if (bin) {
> -		bin->base.perfmon = render->base.perfmon;
> -		v3d_perfmon_get(bin->base.perfmon);
> -		v3d_push_job(&bin->base);
> -
> -		ret = drm_sched_job_add_dependency(&render->base.base,
> -						   dma_fence_get(bin->base.done_fence));
> -		if (ret)
> -			goto fail_unreserve;
> -	}
> -
> -	v3d_push_job(&render->base);
> -
> -	if (clean_job) {
> -		struct dma_fence *render_fence =
> -			dma_fence_get(render->base.done_fence);
> -		ret = drm_sched_job_add_dependency(&clean_job->base,
> -						   render_fence);
> -		if (ret)
> -			goto fail_unreserve;
> -		clean_job->perfmon = render->base.perfmon;
> -		v3d_perfmon_get(clean_job->perfmon);
> -		v3d_push_job(clean_job);
> -	}
> -
> -	mutex_unlock(&v3d->sched_lock);
> +	ret = v3d_submit_jobs(&submit);
> +	if (ret)
> +		goto fail_unreserve;
>   
>   	v3d_attach_fences_and_unlock_reservation(file_priv,
> -						 last_job,
> -						 &exec,
> -						 args->out_sync,
> -						 &se,
> -						 last_job->done_fence);
> +						 submit.jobs[submit.job_count - 1],
> +						 &submit.exec,
> +						 args->out_sync, &se,
> +						 submit.jobs[submit.job_count - 1]->done_fence);
>   
> -	v3d_job_put(&bin->base);
> -	v3d_job_put(&render->base);
> -	v3d_job_put(clean_job);
> +	v3d_submit_put_jobs(&submit);
>   
>   	return 0;
>   
>   fail_unreserve:
> -	mutex_unlock(&v3d->sched_lock);
> -fail_perfmon:
> -	drm_exec_fini(&exec);
> +	drm_exec_fini(&submit.exec);
>   fail:
> -	v3d_job_cleanup((void *)bin);
> -	v3d_job_cleanup((void *)render);
> -	v3d_job_cleanup(clean_job);
> +	v3d_submit_cleanup_jobs(&submit);
>   	v3d_put_multisync_post_deps(&se);
>   
>   	return ret;
> @@ -1095,14 +1171,13 @@ int
>   v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
>   		     struct drm_file *file_priv)
>   {
> -	struct v3d_dev *v3d = to_v3d_dev(dev);
> +	struct v3d_submit submit = { .v3d = to_v3d_dev(dev), .file_priv = file_priv };
>   	struct drm_v3d_submit_tfu *args = data;
>   	struct v3d_submit_ext se = {0};
>   	struct v3d_tfu_job *job = NULL;

No need to init.

Regards,

Tvrtko

> -	struct drm_exec exec;
>   	int ret = 0;
>   
> -	trace_v3d_submit_tfu_ioctl(&v3d->drm, args->iia);
> +	trace_v3d_submit_tfu_ioctl(dev, args->iia);
>   
>   	if (args->flags && !(args->flags & DRM_V3D_SUBMIT_EXTENSION)) {
>   		drm_dbg(dev, "invalid flags: %d\n", args->flags);
> @@ -1117,17 +1192,16 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
>   		}
>   	}
>   
> -	ret = v3d_job_allocate(v3d, (void *)&job, sizeof(*job));
> -	if (ret)
> -		return ret;
> -
> -	ret = v3d_job_init(v3d, file_priv, &job->base,
> -			   v3d_job_free, args->in_sync, &se, V3D_TFU);
> -	if (ret) {
> -		v3d_job_deallocate((void *)&job);
> +	job = (struct v3d_tfu_job *) v3d_submit_add_job(&submit, V3D_TFU);
> +	if (IS_ERR(job)) {
> +		ret = PTR_ERR(job);
>   		goto fail;
>   	}
>   
> +	ret = v3d_job_add_syncobjs(&job->base, file_priv, args->in_sync, &se);
> +	if (ret)
> +		goto fail;
> +
>   	job->base.bo = kzalloc_objs(*job->base.bo, ARRAY_SIZE(args->bo_handles));
>   	if (!job->base.bo) {
>   		ret = -ENOMEM;
> @@ -1155,26 +1229,27 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
>   		job->base.bo[job->base.bo_count] = bo;
>   	}
>   
> -	ret = v3d_lock_bo_reservations(&job->base, &exec);
> +	ret = v3d_lock_bo_reservations(&job->base, &submit.exec);
>   	if (ret)
>   		goto fail;
>   
> -	mutex_lock(&v3d->sched_lock);
> -	v3d_push_job(&job->base);
> -	mutex_unlock(&v3d->sched_lock);
> +	ret = v3d_submit_jobs(&submit);
> +	if (ret)
> +		goto fail_unreserve;
>   
>   	v3d_attach_fences_and_unlock_reservation(file_priv,
> -						 &job->base, &exec,
> -						 args->out_sync,
> -						 &se,
> +						 &job->base, &submit.exec,
> +						 args->out_sync, &se,
>   						 job->base.done_fence);
>   
> -	v3d_job_put(&job->base);
> +	v3d_submit_put_jobs(&submit);
>   
>   	return 0;
>   
> +fail_unreserve:
> +	drm_exec_fini(&submit.exec);
>   fail:
> -	v3d_job_cleanup((void *)job);
> +	v3d_submit_cleanup_jobs(&submit);
>   	v3d_put_multisync_post_deps(&se);
>   
>   	return ret;
> @@ -1193,21 +1268,19 @@ int
>   v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
>   		     struct drm_file *file_priv)
>   {
> -	struct v3d_dev *v3d = to_v3d_dev(dev);
> -	struct v3d_file_priv *v3d_priv = file_priv->driver_priv;
> +	struct v3d_submit submit = { .v3d = to_v3d_dev(dev), .file_priv = file_priv };
>   	struct drm_v3d_submit_csd *args = data;
>   	struct v3d_submit_ext se = {0};
>   	struct v3d_csd_job *job = NULL;
>   	struct v3d_job *clean_job = NULL;
> -	struct drm_exec exec;
>   	int ret;
>   
> -	trace_v3d_submit_csd_ioctl(&v3d->drm, args->cfg[5], args->cfg[6]);
> +	trace_v3d_submit_csd_ioctl(dev, args->cfg[5], args->cfg[6]);
>   
>   	if (args->pad)
>   		return -EINVAL;
>   
> -	if (!v3d_has_csd(v3d)) {
> +	if (!v3d_has_csd(submit.v3d)) {
>   		drm_warn(dev, "Attempting CSD submit on non-CSD hardware\n");
>   		return -EINVAL;
>   	}
> @@ -1225,55 +1298,36 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
>   		}
>   	}
>   
> -	ret = v3d_setup_csd_jobs_and_bos(file_priv, v3d, args,
> -					 &job, &clean_job, &se, &exec);
> +	ret = v3d_setup_csd_jobs_and_bos(file_priv, submit.v3d, args,
> +					 &job, &clean_job, &se, &submit.exec);
>   	if (ret)
>   		goto fail;
>   
> -	if (args->perfmon_id) {
> -		if (v3d->global_perfmon) {
> -			ret = -EAGAIN;
> -			goto fail_perfmon;
> -		}
> +	submit.jobs[submit.job_count++] = &job->base;
> +	submit.jobs[submit.job_count++] = clean_job;
>   
> -		job->base.perfmon = v3d_perfmon_find(v3d_priv,
> -						     args->perfmon_id);
> -		if (!job->base.perfmon) {
> -			ret = -ENOENT;
> -			goto fail_perfmon;
> -		}
> -	}
> -
> -	mutex_lock(&v3d->sched_lock);
> -	v3d_push_job(&job->base);
> -
> -	ret = drm_sched_job_add_dependency(&clean_job->base,
> -					   dma_fence_get(job->base.done_fence));
> +	ret = v3d_attach_perfmon_to_jobs(&submit, args->perfmon_id);
>   	if (ret)
>   		goto fail_unreserve;
>   
> -	v3d_push_job(clean_job);
> -	mutex_unlock(&v3d->sched_lock);
> +	ret = v3d_submit_jobs(&submit);
> +	if (ret)
> +		goto fail_unreserve;
>   
>   	v3d_attach_fences_and_unlock_reservation(file_priv,
>   						 clean_job,
> -						 &exec,
> -						 args->out_sync,
> -						 &se,
> +						 &submit.exec,
> +						 args->out_sync, &se,
>   						 clean_job->done_fence);
>   
> -	v3d_job_put(&job->base);
> -	v3d_job_put(clean_job);
> +	v3d_submit_put_jobs(&submit);
>   
>   	return 0;
>   
>   fail_unreserve:
> -	mutex_unlock(&v3d->sched_lock);
> -fail_perfmon:
> -	drm_exec_fini(&exec);
> +	drm_exec_fini(&submit.exec);
>   fail:
> -	v3d_job_cleanup((void *)job);
> -	v3d_job_cleanup(clean_job);
> +	v3d_submit_cleanup_jobs(&submit);
>   	v3d_put_multisync_post_deps(&se);
>   
>   	return ret;
>