Currently, v3d only supports single in/out syncobj per submission (in v3d_submit_cl, there are two in_sync, one for bin and another for render job); however, Vulkan queue submit operations expect multiples wait and signal semaphores. This series extends v3d interface and job dependency operations to handle more than one in/out syncobj.
The first patch just decouples the steps to lookup and add job dependency from the job init code, since the operation repeats for every syncobj that a job should wait before starting. So, the fourth patch of this series will reuse it to handle multiples wait for semaphores.
The second patch moves job memory allocation to v3d_job_init() for any type of v3d_job-based job. The main goal is to prevent errors when handling job initialization failures (doing a proper cleanup).
The third patch extends our interface by using a generic extension. This approach was inspired by i915_user_extension[1] and amd_cs_chunks[2] to give a little more flexibility in adding other submission features in the future. Therefore, the list of extensions would work as a hub of features that use an id to determine the corresponding feature data type.
With this base, the fourth patch adds multiple wait/signal semaphores support. For this, we add to the list of the generic extensions a new data type (drm_v3d_multi_sync) that points to two arrays of syncobjs (in/out) and also determines (wait_stage) the job to add wait dependencies (v3d_queue). An auxiliary struct (v3d_submit_ext) is used when parsing submission extensions. Finally, we reserve some space in the semaphore struct (drm_v3d_sem) to accommodate timeline semaphores that we aim to add support soon (same reason for already defining v3d_submit_outsync).
[1] https://cgit.freedesktop.org/drm/drm-misc/commit/drivers/gpu/drm/i915/i915_u... [2] https://cgit.freedesktop.org/drm/drm-misc/tree/include/uapi/drm/amdgpu_drm.h...
In the mesa side, the work related to this series is in progress at https://gitlab.freedesktop.org/mwen/mesa/-/commit/6c340bb35203a0418af87a7921...
v2: - move job mem alloc to v3d_job_init (Iago) - simplify and subclass the generic extension struct (Daniel) - make job dependency conditions more understandable (Iago)
v3 (Iago): - job should be initialized NULL - fix error handling for pm ops in v3d_job_init - remove irrelevant comment - fix multisync conditions to multiple vs single sempahores
Melissa Wen (4): drm/v3d: decouple adding job dependencies steps from job init drm/v3d: alloc and init job in one shot drm/v3d: add generic ioctl extension drm/v3d: add multiple syncobjs support
drivers/gpu/drm/v3d/v3d_drv.c | 10 +- drivers/gpu/drm/v3d/v3d_drv.h | 24 ++- drivers/gpu/drm/v3d/v3d_gem.c | 390 +++++++++++++++++++++++++--------- include/uapi/drm/v3d_drm.h | 78 +++++++ 4 files changed, 389 insertions(+), 113 deletions(-)
Prep work to enable a job to wait for more than one syncobj before start. Also get rid of old checkpatch warnings in the v3d_gem file. No functional changes.
Signed-off-by: Melissa Wen mwen@igalia.com --- drivers/gpu/drm/v3d/v3d_gem.c | 28 ++++++++++++++++++---------- 1 file changed, 18 insertions(+), 10 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c index ead0be8d48a7..e60fbc28ef29 100644 --- a/drivers/gpu/drm/v3d/v3d_gem.c +++ b/drivers/gpu/drm/v3d/v3d_gem.c @@ -416,7 +416,7 @@ v3d_wait_bo_ioctl(struct drm_device *dev, void *data, return -EINVAL;
ret = drm_gem_dma_resv_wait(file_priv, args->handle, - true, timeout_jiffies); + true, timeout_jiffies);
/* Decrement the user's timeout, in case we got interrupted * such that the ioctl will be restarted. @@ -434,12 +434,25 @@ v3d_wait_bo_ioctl(struct drm_device *dev, void *data, return ret; }
+static int +v3d_job_add_deps(struct drm_file *file_priv, struct v3d_job *job, + u32 in_sync, u32 point) +{ + struct dma_fence *in_fence = NULL; + int ret; + + ret = drm_syncobj_find_fence(file_priv, in_sync, point, 0, &in_fence); + if (ret == -EINVAL) + return ret; + + return drm_sched_job_add_dependency(&job->base, in_fence); +} + static int v3d_job_init(struct v3d_dev *v3d, struct drm_file *file_priv, struct v3d_job *job, void (*free)(struct kref *ref), u32 in_sync, enum v3d_queue queue) { - struct dma_fence *in_fence = NULL; struct v3d_file_priv *v3d_priv = file_priv->driver_priv; int ret;
@@ -455,11 +468,7 @@ v3d_job_init(struct v3d_dev *v3d, struct drm_file *file_priv, if (ret) goto fail;
- ret = drm_syncobj_find_fence(file_priv, in_sync, 0, 0, &in_fence); - if (ret == -EINVAL) - goto fail_job; - - ret = drm_sched_job_add_dependency(&job->base, in_fence); + ret = v3d_job_add_deps(file_priv, job, in_sync, 0); if (ret) goto fail_job;
@@ -499,7 +508,7 @@ v3d_attach_fences_and_unlock_reservation(struct drm_file *file_priv, for (i = 0; i < job->bo_count; i++) { /* XXX: Use shared fences for read-only objects. */ dma_resv_add_excl_fence(job->bo[i]->resv, - job->done_fence); + job->done_fence); }
drm_gem_unlock_reservations(job->bo, job->bo_count, acquire_ctx); @@ -903,8 +912,7 @@ v3d_gem_init(struct drm_device *dev) if (!v3d->pt) { drm_mm_takedown(&v3d->mm); dev_err(v3d->drm.dev, - "Failed to allocate page tables. " - "Please ensure you have CMA enabled.\n"); + "Failed to allocate page tables. Please ensure you have CMA enabled.\n"); return -ENOMEM; }
Move job memory allocation to v3d_job_init function. This aim to facilitate error handling in job initialization, since cleanup steps are similar for all (struct v3d_job)-based types of job involved in a command submission. To generalize v3d_job_init(), this change takes into account that all job structs have the first element a struct v3d_job (bin, render, tfu, csd) or it is a v3d_job itself (clean_job) for pointer casting.
v3: - explicitly init job as NULL (Iago) - fix pm failure handling on v3_job_init (Iago)
Suggested-by: Iago Toral itoral@igalia.com Signed-off-by: Melissa Wen mwen@igalia.com --- drivers/gpu/drm/v3d/v3d_gem.c | 135 +++++++++++++--------------------- 1 file changed, 53 insertions(+), 82 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c index e60fbc28ef29..f932e73fb5c6 100644 --- a/drivers/gpu/drm/v3d/v3d_gem.c +++ b/drivers/gpu/drm/v3d/v3d_gem.c @@ -392,6 +392,9 @@ v3d_render_job_free(struct kref *ref)
void v3d_job_cleanup(struct v3d_job *job) { + if (!job) + return; + drm_sched_job_cleanup(&job->base); v3d_job_put(job); } @@ -450,35 +453,48 @@ v3d_job_add_deps(struct drm_file *file_priv, struct v3d_job *job,
static int v3d_job_init(struct v3d_dev *v3d, struct drm_file *file_priv, - struct v3d_job *job, void (*free)(struct kref *ref), + void **container, size_t size, void (*free)(struct kref *ref), u32 in_sync, enum v3d_queue queue) { struct v3d_file_priv *v3d_priv = file_priv->driver_priv; + struct v3d_job *job; int ret;
+ *container = kcalloc(1, size, GFP_KERNEL); + if (!*container) { + DRM_ERROR("Cannot allocate memory for v3d job."); + return -ENOMEM; + } + + job = *container; job->v3d = v3d; job->free = free;
ret = pm_runtime_get_sync(v3d->drm.dev); if (ret < 0) - return ret; + goto fail;
ret = drm_sched_job_init(&job->base, &v3d_priv->sched_entity[queue], v3d_priv); if (ret) - goto fail; + goto fail_job;
ret = v3d_job_add_deps(file_priv, job, in_sync, 0); if (ret) - goto fail_job; + goto fail_deps;
kref_init(&job->refcount);
return 0; -fail_job: + +fail_deps: drm_sched_job_cleanup(&job->base); -fail: +fail_job: pm_runtime_put_autosuspend(v3d->drm.dev); +fail: + kfree(*container); + *container = NULL; + return ret; }
@@ -541,7 +557,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data, struct v3d_file_priv *v3d_priv = file_priv->driver_priv; struct drm_v3d_submit_cl *args = data; struct v3d_bin_job *bin = NULL; - struct v3d_render_job *render; + struct v3d_render_job *render = NULL; struct v3d_job *clean_job = NULL; struct v3d_job *last_job; struct ww_acquire_ctx acquire_ctx; @@ -558,35 +574,20 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data, return -EINVAL; }
- render = kcalloc(1, sizeof(*render), GFP_KERNEL); - if (!render) - return -ENOMEM; + ret = v3d_job_init(v3d, file_priv, (void *)&render, sizeof(*render), + v3d_render_job_free, args->in_sync_rcl, V3D_RENDER); + if (ret) + goto fail;
render->start = args->rcl_start; render->end = args->rcl_end; INIT_LIST_HEAD(&render->unref_list);
- ret = v3d_job_init(v3d, file_priv, &render->base, - v3d_render_job_free, args->in_sync_rcl, V3D_RENDER); - if (ret) { - kfree(render); - return ret; - } - if (args->bcl_start != args->bcl_end) { - bin = kcalloc(1, sizeof(*bin), GFP_KERNEL); - if (!bin) { - v3d_job_cleanup(&render->base); - return -ENOMEM; - } - - ret = v3d_job_init(v3d, file_priv, &bin->base, + ret = v3d_job_init(v3d, file_priv, (void *)&bin, sizeof(*bin), v3d_job_free, args->in_sync_bcl, V3D_BIN); - if (ret) { - v3d_job_cleanup(&render->base); - kfree(bin); - return ret; - } + if (ret) + goto fail;
bin->start = args->bcl_start; bin->end = args->bcl_end; @@ -597,18 +598,10 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data, }
if (args->flags & DRM_V3D_SUBMIT_CL_FLUSH_CACHE) { - clean_job = kcalloc(1, sizeof(*clean_job), GFP_KERNEL); - if (!clean_job) { - ret = -ENOMEM; - goto fail; - } - - ret = v3d_job_init(v3d, file_priv, clean_job, v3d_job_free, 0, V3D_CACHE_CLEAN); - if (ret) { - kfree(clean_job); - clean_job = NULL; + ret = v3d_job_init(v3d, file_priv, (void *)&clean_job, sizeof(*clean_job), + v3d_job_free, 0, V3D_CACHE_CLEAN); + if (ret) goto fail; - }
last_job = clean_job; } else { @@ -681,11 +674,9 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data, drm_gem_unlock_reservations(last_job->bo, last_job->bo_count, &acquire_ctx); fail: - if (bin) - v3d_job_cleanup(&bin->base); - v3d_job_cleanup(&render->base); - if (clean_job) - v3d_job_cleanup(clean_job); + v3d_job_cleanup((void *)bin); + v3d_job_cleanup((void *)render); + v3d_job_cleanup(clean_job);
return ret; } @@ -705,28 +696,22 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data, { struct v3d_dev *v3d = to_v3d_dev(dev); struct drm_v3d_submit_tfu *args = data; - struct v3d_tfu_job *job; + struct v3d_tfu_job *job = NULL; struct ww_acquire_ctx acquire_ctx; int ret = 0;
trace_v3d_submit_tfu_ioctl(&v3d->drm, args->iia);
- job = kcalloc(1, sizeof(*job), GFP_KERNEL); - if (!job) - return -ENOMEM; - - ret = v3d_job_init(v3d, file_priv, &job->base, + ret = v3d_job_init(v3d, file_priv, (void *)&job, sizeof(*job), v3d_job_free, args->in_sync, V3D_TFU); - if (ret) { - kfree(job); - return ret; - } + if (ret) + goto fail;
job->base.bo = kcalloc(ARRAY_SIZE(args->bo_handles), sizeof(*job->base.bo), GFP_KERNEL); if (!job->base.bo) { - v3d_job_cleanup(&job->base); - return -ENOMEM; + ret = -ENOMEM; + goto fail; }
job->args = *args; @@ -773,7 +758,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data, return 0;
fail: - v3d_job_cleanup(&job->base); + v3d_job_cleanup((void *)job);
return ret; } @@ -794,8 +779,8 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data, struct v3d_dev *v3d = to_v3d_dev(dev); struct v3d_file_priv *v3d_priv = file_priv->driver_priv; struct drm_v3d_submit_csd *args = data; - struct v3d_csd_job *job; - struct v3d_job *clean_job; + struct v3d_csd_job *job = NULL; + struct v3d_job *clean_job = NULL; struct ww_acquire_ctx acquire_ctx; int ret;
@@ -806,29 +791,15 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data, return -EINVAL; }
- job = kcalloc(1, sizeof(*job), GFP_KERNEL); - if (!job) - return -ENOMEM; - - ret = v3d_job_init(v3d, file_priv, &job->base, + ret = v3d_job_init(v3d, file_priv, (void *)&job, sizeof(*job), v3d_job_free, args->in_sync, V3D_CSD); - if (ret) { - kfree(job); - return ret; - } - - clean_job = kcalloc(1, sizeof(*clean_job), GFP_KERNEL); - if (!clean_job) { - v3d_job_cleanup(&job->base); - return -ENOMEM; - } + if (ret) + goto fail;
- ret = v3d_job_init(v3d, file_priv, clean_job, v3d_job_free, 0, V3D_CACHE_CLEAN); - if (ret) { - v3d_job_cleanup(&job->base); - kfree(clean_job); - return ret; - } + ret = v3d_job_init(v3d, file_priv, (void *)&clean_job, sizeof(*clean_job), + v3d_job_free, 0, V3D_CACHE_CLEAN); + if (ret) + goto fail;
job->args = *args;
@@ -877,7 +848,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data, drm_gem_unlock_reservations(clean_job->bo, clean_job->bo_count, &acquire_ctx); fail: - v3d_job_cleanup(&job->base); + v3d_job_cleanup((void *)job); v3d_job_cleanup(clean_job);
return ret;
Add support to attach generic extensions on job submission. This patch is third prep work to enable multiple syncobjs on job submission. With this work, when the job submission interface needs to be extended to accomodate a new feature, we will use a generic extension struct where an id determines the data type to be pointed. The first application is to enable multiples in/out syncobj (next patch), but the base is already done for future features. Therefore, to attach a new feature, a specific extension struct should subclass drm_v3d_extension and update the list of extensions in a job submission.
v2: - remove redundant elements to subclass struct (Daniel)
v3: - add comment for v3d_get_extensions
Signed-off-by: Melissa Wen mwen@igalia.com --- drivers/gpu/drm/v3d/v3d_drv.c | 4 +- drivers/gpu/drm/v3d/v3d_gem.c | 74 +++++++++++++++++++++++++++++++++-- include/uapi/drm/v3d_drm.h | 31 +++++++++++++++ 3 files changed, 103 insertions(+), 6 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c index c1deab2cf38d..3d6b9bcce2f7 100644 --- a/drivers/gpu/drm/v3d/v3d_drv.c +++ b/drivers/gpu/drm/v3d/v3d_drv.c @@ -83,7 +83,6 @@ static int v3d_get_param_ioctl(struct drm_device *dev, void *data, return 0; }
- switch (args->param) { case DRM_V3D_PARAM_SUPPORTS_TFU: args->value = 1; @@ -147,7 +146,7 @@ v3d_postclose(struct drm_device *dev, struct drm_file *file) DEFINE_DRM_GEM_FOPS(v3d_drm_fops);
/* DRM_AUTH is required on SUBMIT_CL for now, while we don't have GMP - * protection between clients. Note that render nodes would be be + * protection between clients. Note that render nodes would be * able to submit CLs that could access BOs from clients authenticated * with the master node. The TFU doesn't use the GMP, so it would * need to stay DRM_AUTH until we do buffer size/offset validation. @@ -219,7 +218,6 @@ static int v3d_platform_drm_probe(struct platform_device *pdev) u32 mmu_debug; u32 ident1;
- v3d = devm_drm_dev_alloc(dev, &v3d_drm_driver, struct v3d_dev, drm); if (IS_ERR(v3d)) return PTR_ERR(v3d); diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c index f932e73fb5c6..93f130fb3a13 100644 --- a/drivers/gpu/drm/v3d/v3d_gem.c +++ b/drivers/gpu/drm/v3d/v3d_gem.c @@ -537,6 +537,36 @@ v3d_attach_fences_and_unlock_reservation(struct drm_file *file_priv, } }
+/* Whenever userspace sets ioctl extensions, v3d_get_extensions parses data + * according to the extension id (name). + */ +static int +v3d_get_extensions(struct drm_file *file_priv, u64 ext_handles) +{ + struct drm_v3d_extension __user *user_ext; + + user_ext = u64_to_user_ptr(ext_handles); + while (user_ext) { + struct drm_v3d_extension ext; + + if (copy_from_user(&ext, user_ext, sizeof(ext))) { + DRM_DEBUG("Failed to copy submit extension\n"); + return -EFAULT; + } + + switch (ext.id) { + case 0: + default: + DRM_DEBUG_DRIVER("Unknown extension id: %d\n", ext.id); + return -EINVAL; + } + + user_ext = u64_to_user_ptr(ext.next); + } + + return 0; +} + /** * v3d_submit_cl_ioctl() - Submits a job (frame) to the V3D. * @dev: DRM device @@ -565,15 +595,24 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
trace_v3d_submit_cl_ioctl(&v3d->drm, args->rcl_start, args->rcl_end);
- if (args->pad != 0) + if (args->pad) return -EINVAL;
- if (args->flags != 0 && - args->flags != DRM_V3D_SUBMIT_CL_FLUSH_CACHE) { + if (args->flags && + args->flags & ~(DRM_V3D_SUBMIT_CL_FLUSH_CACHE | + DRM_V3D_SUBMIT_EXTENSION)) { DRM_INFO("invalid flags: %d\n", args->flags); return -EINVAL; }
+ if (args->flags & DRM_V3D_SUBMIT_EXTENSION) { + ret = v3d_get_extensions(file_priv, args->extensions); + if (ret) { + DRM_DEBUG("Failed to get extensions.\n"); + return ret; + } + } + ret = v3d_job_init(v3d, file_priv, (void *)&render, sizeof(*render), v3d_render_job_free, args->in_sync_rcl, V3D_RENDER); if (ret) @@ -702,6 +741,19 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
trace_v3d_submit_tfu_ioctl(&v3d->drm, args->iia);
+ if (args->flags && !(args->flags & DRM_V3D_SUBMIT_EXTENSION)) { + DRM_DEBUG("invalid flags: %d\n", args->flags); + return -EINVAL; + } + + if (args->flags & DRM_V3D_SUBMIT_EXTENSION) { + ret = v3d_get_extensions(file_priv, args->extensions); + if (ret) { + DRM_DEBUG("Failed to get extensions.\n"); + return ret; + } + } + ret = v3d_job_init(v3d, file_priv, (void *)&job, sizeof(*job), v3d_job_free, args->in_sync, V3D_TFU); if (ret) @@ -786,11 +838,27 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data,
trace_v3d_submit_csd_ioctl(&v3d->drm, args->cfg[5], args->cfg[6]);
+ if (args->pad) + return -EINVAL; + if (!v3d_has_csd(v3d)) { DRM_DEBUG("Attempting CSD submit on non-CSD hardware\n"); return -EINVAL; }
+ if (args->flags && !(args->flags & DRM_V3D_SUBMIT_EXTENSION)) { + DRM_INFO("invalid flags: %d\n", args->flags); + return -EINVAL; + } + + if (args->flags & DRM_V3D_SUBMIT_EXTENSION) { + ret = v3d_get_extensions(file_priv, args->extensions); + if (ret) { + DRM_DEBUG("Failed to get extensions.\n"); + return ret; + } + } + ret = v3d_job_init(v3d, file_priv, (void *)&job, sizeof(*job), v3d_job_free, args->in_sync, V3D_CSD); if (ret) diff --git a/include/uapi/drm/v3d_drm.h b/include/uapi/drm/v3d_drm.h index 4104f22fb3d3..55b443ca6c0b 100644 --- a/include/uapi/drm/v3d_drm.h +++ b/include/uapi/drm/v3d_drm.h @@ -58,6 +58,20 @@ extern "C" { struct drm_v3d_perfmon_get_values)
#define DRM_V3D_SUBMIT_CL_FLUSH_CACHE 0x01 +#define DRM_V3D_SUBMIT_EXTENSION 0x02 + +/* struct drm_v3d_extension - ioctl extensions + * + * Linked-list of generic extensions where the id identify which struct is + * pointed by ext_data. Therefore, DRM_V3D_EXT_ID_* is used on id to identify + * the extension type. + */ +struct drm_v3d_extension { + __u64 next; + __u32 id; +#define DRM_V3D_EXT_ID_MULTI_SYNC 0x01 + __u32 flags; /* mbz */ +};
/** * struct drm_v3d_submit_cl - ioctl argument for submitting commands to the 3D @@ -135,12 +149,16 @@ struct drm_v3d_submit_cl { /* Number of BO handles passed in (size is that times 4). */ __u32 bo_handle_count;
+ /* DRM_V3D_SUBMIT_* properties */ __u32 flags;
/* ID of the perfmon to attach to this job. 0 means no perfmon. */ __u32 perfmon_id;
__u32 pad; + + /* Pointer to an array of ioctl extensions*/ + __u64 extensions; };
/** @@ -248,6 +266,12 @@ struct drm_v3d_submit_tfu { __u32 in_sync; /* Sync object to signal when the TFU job is done. */ __u32 out_sync; + + __u32 flags; + + /* Pointer to an array of ioctl extensions*/ + __u64 extensions; + };
/* Submits a compute shader for dispatch. This job will block on any @@ -276,6 +300,13 @@ struct drm_v3d_submit_csd {
/* ID of the perfmon to attach to this job. 0 means no perfmon. */ __u32 perfmon_id; + + /* Pointer to an array of ioctl extensions*/ + __u64 extensions; + + __u32 flags; + + __u32 pad; };
enum {
Using the generic extension from the previous patch, a specific multisync extension enables more than one in/out binary syncobj per job submission. Arrays of syncobjs are set in struct drm_v3d_multisync, that also cares of determining the stage for sync (wait deps) according to the job queue.
v2: - subclass the generic extension struct (Daniel) - simplify adding dependency conditions to make understandable (Iago)
v3: - fix conditions to consider single or multiples in/out_syncs (Iago) - remove irrelevant comment (Iago)
Signed-off-by: Melissa Wen mwen@igalia.com --- drivers/gpu/drm/v3d/v3d_drv.c | 6 +- drivers/gpu/drm/v3d/v3d_drv.h | 24 +++-- drivers/gpu/drm/v3d/v3d_gem.c | 185 ++++++++++++++++++++++++++++++---- include/uapi/drm/v3d_drm.h | 49 ++++++++- 4 files changed, 232 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c index 3d6b9bcce2f7..bd46396a1ae0 100644 --- a/drivers/gpu/drm/v3d/v3d_drv.c +++ b/drivers/gpu/drm/v3d/v3d_drv.c @@ -96,6 +96,9 @@ static int v3d_get_param_ioctl(struct drm_device *dev, void *data, case DRM_V3D_PARAM_SUPPORTS_PERFMON: args->value = (v3d->ver >= 40); return 0; + case DRM_V3D_PARAM_SUPPORTS_MULTISYNC_EXT: + args->value = 1; + return 0; default: DRM_DEBUG("Unknown parameter %d\n", args->param); return -EINVAL; @@ -135,9 +138,8 @@ v3d_postclose(struct drm_device *dev, struct drm_file *file) struct v3d_file_priv *v3d_priv = file->driver_priv; enum v3d_queue q;
- for (q = 0; q < V3D_MAX_QUEUES; q++) { + for (q = 0; q < V3D_MAX_QUEUES; q++) drm_sched_entity_destroy(&v3d_priv->sched_entity[q]); - }
v3d_perfmon_close_file(v3d_priv); kfree(v3d_priv); diff --git a/drivers/gpu/drm/v3d/v3d_drv.h b/drivers/gpu/drm/v3d/v3d_drv.h index b900a050d5e2..b74b1351bfc8 100644 --- a/drivers/gpu/drm/v3d/v3d_drv.h +++ b/drivers/gpu/drm/v3d/v3d_drv.h @@ -19,15 +19,6 @@ struct reset_control;
#define GMP_GRANULARITY (128 * 1024)
-/* Enum for each of the V3D queues. */ -enum v3d_queue { - V3D_BIN, - V3D_RENDER, - V3D_TFU, - V3D_CSD, - V3D_CACHE_CLEAN, -}; - #define V3D_MAX_QUEUES (V3D_CACHE_CLEAN + 1)
struct v3d_queue_state { @@ -294,6 +285,21 @@ struct v3d_csd_job { struct drm_v3d_submit_csd args; };
+struct v3d_submit_outsync { + struct drm_syncobj *syncobj; +}; + +struct v3d_submit_ext { + u32 flags; + u32 wait_stage; + + u32 in_sync_count; + u64 in_syncs; + + u32 out_sync_count; + struct v3d_submit_outsync *out_syncs; +}; + /** * __wait_for - magic wait macro * diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c index 93f130fb3a13..d344eae36b57 100644 --- a/drivers/gpu/drm/v3d/v3d_gem.c +++ b/drivers/gpu/drm/v3d/v3d_gem.c @@ -454,11 +454,12 @@ v3d_job_add_deps(struct drm_file *file_priv, struct v3d_job *job, static int v3d_job_init(struct v3d_dev *v3d, struct drm_file *file_priv, void **container, size_t size, void (*free)(struct kref *ref), - u32 in_sync, enum v3d_queue queue) + u32 in_sync, struct v3d_submit_ext *se, enum v3d_queue queue) { struct v3d_file_priv *v3d_priv = file_priv->driver_priv; struct v3d_job *job; - int ret; + bool has_multisync = se && (se->flags & DRM_V3D_EXT_ID_MULTI_SYNC); + int ret, i;
*container = kcalloc(1, size, GFP_KERNEL); if (!*container) { @@ -479,9 +480,28 @@ v3d_job_init(struct v3d_dev *v3d, struct drm_file *file_priv, if (ret) goto fail_job;
- ret = v3d_job_add_deps(file_priv, job, in_sync, 0); - if (ret) - goto fail_deps; + if (has_multisync) { + if (se->in_sync_count && se->wait_stage == queue) { + struct drm_v3d_sem __user *handle = u64_to_user_ptr(se->in_syncs); + + for (i = 0; i < se->in_sync_count; i++) { + struct drm_v3d_sem in; + + ret = copy_from_user(&in, handle++, sizeof(in)); + if (ret) { + DRM_DEBUG("Failed to copy wait dep handle.\n"); + goto fail_deps; + } + ret = v3d_job_add_deps(file_priv, job, in.handle, 0); + if (ret) + goto fail_deps; + } + } + } else { + ret = v3d_job_add_deps(file_priv, job, in_sync, 0); + if (ret) + goto fail_deps; + }
kref_init(&job->refcount);
@@ -516,9 +536,11 @@ v3d_attach_fences_and_unlock_reservation(struct drm_file *file_priv, struct v3d_job *job, struct ww_acquire_ctx *acquire_ctx, u32 out_sync, + struct v3d_submit_ext *se, struct dma_fence *done_fence) { struct drm_syncobj *sync_out; + bool has_multisync = se && (se->flags & DRM_V3D_EXT_ID_MULTI_SYNC); int i;
for (i = 0; i < job->bo_count; i++) { @@ -530,20 +552,130 @@ v3d_attach_fences_and_unlock_reservation(struct drm_file *file_priv, drm_gem_unlock_reservations(job->bo, job->bo_count, acquire_ctx);
/* Update the return sync object for the job */ - sync_out = drm_syncobj_find(file_priv, out_sync); - if (sync_out) { - drm_syncobj_replace_fence(sync_out, done_fence); - drm_syncobj_put(sync_out); + /* If it only supports a single signal semaphore*/ + if (!has_multisync) { + sync_out = drm_syncobj_find(file_priv, out_sync); + if (sync_out) { + drm_syncobj_replace_fence(sync_out, done_fence); + drm_syncobj_put(sync_out); + } + return; + } + + /* If multiple semaphores extension is supported */ + if (se->out_sync_count) { + for (i = 0; i < se->out_sync_count; i++) { + drm_syncobj_replace_fence(se->out_syncs[i].syncobj, + done_fence); + drm_syncobj_put(se->out_syncs[i].syncobj); + } + kvfree(se->out_syncs); + } +} + +static void +v3d_put_multisync_post_deps(struct v3d_submit_ext *se) +{ + unsigned int i; + + if (!se->out_sync_count) + return; + + for (i = 0; i < se->out_sync_count; i++) + drm_syncobj_put(se->out_syncs[i].syncobj); + kvfree(se->out_syncs); +} + +static int +v3d_get_multisync_post_deps(struct drm_file *file_priv, + struct v3d_submit_ext *se, + u32 count, u64 handles) +{ + struct drm_v3d_sem __user *post_deps; + int i, ret; + + if (!count) + return 0; + + se->out_syncs = (struct v3d_submit_outsync *) + kvmalloc_array(count, + sizeof(struct v3d_submit_outsync), + GFP_KERNEL); + if (!se->out_syncs) + return -ENOMEM; + + post_deps = u64_to_user_ptr(handles); + + for (i = 0; i < count; i++) { + struct drm_v3d_sem out; + + ret = copy_from_user(&out, post_deps++, sizeof(out)); + if (ret) { + DRM_DEBUG("Failed to copy post dep handles\n"); + goto fail; + } + + se->out_syncs[i].syncobj = drm_syncobj_find(file_priv, + out.handle); + if (!se->out_syncs[i].syncobj) { + ret = -EINVAL; + goto fail; + } } + se->out_sync_count = count; + + return 0; + +fail: + for (i--; i >= 0; i--) + drm_syncobj_put(se->out_syncs[i].syncobj); + kvfree(se->out_syncs); + + return ret; +} + +/* Get data for multiple binary semaphores synchronization. Parse syncobj + * to be signaled when job completes (out_sync). + */ +static int +v3d_get_multisync_submit_deps(struct drm_file *file_priv, + struct drm_v3d_extension __user *ext, + void *data) +{ + struct drm_v3d_multi_sync multisync; + struct v3d_submit_ext *se = data; + int ret; + + ret = copy_from_user(&multisync, ext, sizeof(multisync)); + if (ret) + return ret; + + if (multisync.pad) + return -EINVAL; + + ret = v3d_get_multisync_post_deps(file_priv, data, multisync.out_sync_count, + multisync.out_syncs); + if (ret) + return ret; + + se->in_sync_count = multisync.in_sync_count; + se->in_syncs = multisync.in_syncs; + se->flags |= DRM_V3D_EXT_ID_MULTI_SYNC; + se->wait_stage = multisync.wait_stage; + + return 0; }
/* Whenever userspace sets ioctl extensions, v3d_get_extensions parses data * according to the extension id (name). */ static int -v3d_get_extensions(struct drm_file *file_priv, u64 ext_handles) +v3d_get_extensions(struct drm_file *file_priv, + u64 ext_handles, + void *data) { struct drm_v3d_extension __user *user_ext; + int ret;
user_ext = u64_to_user_ptr(ext_handles); while (user_ext) { @@ -555,7 +687,11 @@ v3d_get_extensions(struct drm_file *file_priv, u64 ext_handles) }
switch (ext.id) { - case 0: + case DRM_V3D_EXT_ID_MULTI_SYNC: + ret = v3d_get_multisync_submit_deps(file_priv, user_ext, data); + if (ret) + return ret; + break; default: DRM_DEBUG_DRIVER("Unknown extension id: %d\n", ext.id); return -EINVAL; @@ -586,6 +722,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data, struct v3d_dev *v3d = to_v3d_dev(dev); struct v3d_file_priv *v3d_priv = file_priv->driver_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; @@ -606,7 +743,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data, }
if (args->flags & DRM_V3D_SUBMIT_EXTENSION) { - ret = v3d_get_extensions(file_priv, args->extensions); + ret = v3d_get_extensions(file_priv, args->extensions, &se); if (ret) { DRM_DEBUG("Failed to get extensions.\n"); return ret; @@ -614,7 +751,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data, }
ret = v3d_job_init(v3d, file_priv, (void *)&render, sizeof(*render), - v3d_render_job_free, args->in_sync_rcl, V3D_RENDER); + v3d_render_job_free, args->in_sync_rcl, &se, V3D_RENDER); if (ret) goto fail;
@@ -624,7 +761,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
if (args->bcl_start != args->bcl_end) { ret = v3d_job_init(v3d, file_priv, (void *)&bin, sizeof(*bin), - v3d_job_free, args->in_sync_bcl, V3D_BIN); + v3d_job_free, args->in_sync_bcl, &se, V3D_BIN); if (ret) goto fail;
@@ -638,7 +775,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
if (args->flags & DRM_V3D_SUBMIT_CL_FLUSH_CACHE) { ret = v3d_job_init(v3d, file_priv, (void *)&clean_job, sizeof(*clean_job), - v3d_job_free, 0, V3D_CACHE_CLEAN); + v3d_job_free, 0, 0, V3D_CACHE_CLEAN); if (ret) goto fail;
@@ -698,6 +835,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data, last_job, &acquire_ctx, args->out_sync, + &se, last_job->done_fence);
if (bin) @@ -716,6 +854,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data, v3d_job_cleanup((void *)bin); v3d_job_cleanup((void *)render); v3d_job_cleanup(clean_job); + v3d_put_multisync_post_deps(&se);
return ret; } @@ -735,6 +874,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data, { struct v3d_dev *v3d = to_v3d_dev(dev); struct drm_v3d_submit_tfu *args = data; + struct v3d_submit_ext se = {0}; struct v3d_tfu_job *job = NULL; struct ww_acquire_ctx acquire_ctx; int ret = 0; @@ -747,7 +887,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data, }
if (args->flags & DRM_V3D_SUBMIT_EXTENSION) { - ret = v3d_get_extensions(file_priv, args->extensions); + ret = v3d_get_extensions(file_priv, args->extensions, &se); if (ret) { DRM_DEBUG("Failed to get extensions.\n"); return ret; @@ -755,7 +895,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data, }
ret = v3d_job_init(v3d, file_priv, (void *)&job, sizeof(*job), - v3d_job_free, args->in_sync, V3D_TFU); + v3d_job_free, args->in_sync, &se, V3D_TFU); if (ret) goto fail;
@@ -803,6 +943,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data, v3d_attach_fences_and_unlock_reservation(file_priv, &job->base, &acquire_ctx, args->out_sync, + &se, job->base.done_fence);
v3d_job_put(&job->base); @@ -811,6 +952,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
fail: v3d_job_cleanup((void *)job); + v3d_put_multisync_post_deps(&se);
return ret; } @@ -831,6 +973,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data, struct v3d_dev *v3d = to_v3d_dev(dev); struct v3d_file_priv *v3d_priv = file_priv->driver_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 ww_acquire_ctx acquire_ctx; @@ -852,7 +995,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data, }
if (args->flags & DRM_V3D_SUBMIT_EXTENSION) { - ret = v3d_get_extensions(file_priv, args->extensions); + ret = v3d_get_extensions(file_priv, args->extensions, &se); if (ret) { DRM_DEBUG("Failed to get extensions.\n"); return ret; @@ -860,12 +1003,12 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data, }
ret = v3d_job_init(v3d, file_priv, (void *)&job, sizeof(*job), - v3d_job_free, args->in_sync, V3D_CSD); + v3d_job_free, args->in_sync, &se, V3D_CSD); if (ret) goto fail;
ret = v3d_job_init(v3d, file_priv, (void *)&clean_job, sizeof(*clean_job), - v3d_job_free, 0, V3D_CACHE_CLEAN); + v3d_job_free, 0, 0, V3D_CACHE_CLEAN); if (ret) goto fail;
@@ -904,6 +1047,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data, clean_job, &acquire_ctx, args->out_sync, + &se, clean_job->done_fence);
v3d_job_put(&job->base); @@ -918,6 +1062,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data, fail: v3d_job_cleanup((void *)job); v3d_job_cleanup(clean_job); + v3d_put_multisync_post_deps(&se);
return ret; } diff --git a/include/uapi/drm/v3d_drm.h b/include/uapi/drm/v3d_drm.h index 55b443ca6c0b..3dfc0af8756a 100644 --- a/include/uapi/drm/v3d_drm.h +++ b/include/uapi/drm/v3d_drm.h @@ -73,6 +73,53 @@ struct drm_v3d_extension { __u32 flags; /* mbz */ };
+/* struct drm_v3d_sem - wait/signal semaphore + * + * If binary semaphore, it only takes syncobj handle and ignores flags and + * point fields. Point is defined for timeline syncobj feature. + */ +struct drm_v3d_sem { + __u32 handle; /* syncobj */ + /* rsv below, for future uses */ + __u32 flags; + __u64 point; /* for timeline sem support */ + __u64 mbz[2]; /* must be zero, rsv */ +}; + +/* Enum for each of the V3D queues. */ +enum v3d_queue { + V3D_BIN, + V3D_RENDER, + V3D_TFU, + V3D_CSD, + V3D_CACHE_CLEAN, +}; + +/** + * struct drm_v3d_multi_sync - ioctl extension to add support multiples + * syncobjs for commands submission. + * + * When an extension of DRM_V3D_EXT_ID_MULTI_SYNC id is defined, it points to + * this extension to define wait and signal dependencies, instead of single + * in/out sync entries on submitting commands. The field flags is used to + * determine the stage to set wait dependencies. + */ +struct drm_v3d_multi_sync { + struct drm_v3d_extension base; + /* Array of wait and signal semaphores */ + __u64 in_syncs; + __u64 out_syncs; + + /* Number of entries */ + __u32 in_sync_count; + __u32 out_sync_count; + + /* set the stage (v3d_queue) to sync */ + __u32 wait_stage; + + __u32 pad; /* mbz */ +}; + /** * struct drm_v3d_submit_cl - ioctl argument for submitting commands to the 3D * engine. @@ -228,6 +275,7 @@ enum drm_v3d_param { DRM_V3D_PARAM_SUPPORTS_CSD, DRM_V3D_PARAM_SUPPORTS_CACHE_FLUSH, DRM_V3D_PARAM_SUPPORTS_PERFMON, + DRM_V3D_PARAM_SUPPORTS_MULTISYNC_EXT, };
struct drm_v3d_get_param { @@ -271,7 +319,6 @@ struct drm_v3d_submit_tfu {
/* Pointer to an array of ioctl extensions*/ __u64 extensions; - };
/* Submits a compute shader for dispatch. This job will block on any
On Thu, 2021-09-30 at 17:19 +0100, Melissa Wen wrote:
Using the generic extension from the previous patch, a specific multisync extension enables more than one in/out binary syncobj per job submission. Arrays of syncobjs are set in struct drm_v3d_multisync, that also cares of determining the stage for sync (wait deps) according to the job queue.
v2:
- subclass the generic extension struct (Daniel)
- simplify adding dependency conditions to make understandable (Iago)
v3:
- fix conditions to consider single or multiples in/out_syncs (Iago)
- remove irrelevant comment (Iago)
Signed-off-by: Melissa Wen mwen@igalia.com
drivers/gpu/drm/v3d/v3d_drv.c | 6 +- drivers/gpu/drm/v3d/v3d_drv.h | 24 +++-- drivers/gpu/drm/v3d/v3d_gem.c | 185 ++++++++++++++++++++++++++++++
include/uapi/drm/v3d_drm.h | 49 ++++++++- 4 files changed, 232 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c index 3d6b9bcce2f7..bd46396a1ae0 100644 --- a/drivers/gpu/drm/v3d/v3d_drv.c +++ b/drivers/gpu/drm/v3d/v3d_drv.c @@ -96,6 +96,9 @@ static int v3d_get_param_ioctl(struct drm_device
(...)
@@ -516,9 +536,11 @@ v3d_attach_fences_and_unlock_reservation(struct drm_file *file_priv, struct v3d_job *job, struct ww_acquire_ctx *acquire_ctx, u32 out_sync,
struct v3d_submit_ext *se, struct dma_fence *done_fence){ struct drm_syncobj *sync_out;
- bool has_multisync = se && (se->flags &
We always pass the 'se' pointer from a local variable allocated in the stack of the caller so it is never going to be NULL.
I am happy to keep the NULL check if you want to protect against future changes just in case, but if we do that then...
DRM_V3D_EXT_ID_MULTI_SYNC); int i;
for (i = 0; i < job->bo_count; i++) {
(...)
+static void +v3d_put_multisync_post_deps(struct v3d_submit_ext *se) +{
- unsigned int i;
- if (!se->out_sync_count)
...we should also check for NULL here for consistency.
Also, I think there is another problem in the code: we always call v3d_job_cleanup for failed jobs, but only call v3d_job_put for successful jobs. However, reading the docs for drm_sched_job_init:
"Drivers must make sure drm_sched_job_cleanup() if this function returns successfully, even when @job is aborted before drm_sched_job_arm() is called."
So my understanding is that we should call v3d_job_cleanup instead of v3d_job_put for successful jobs or we would be leaking sched resources on every successful job, no?
Iago
return;- for (i = 0; i < se->out_sync_count; i++)
drm_syncobj_put(se->out_syncs[i].syncobj);- kvfree(se->out_syncs);
+}
+static int +v3d_get_multisync_post_deps(struct drm_file *file_priv,
struct v3d_submit_ext *se,u32 count, u64 handles)+{
- struct drm_v3d_sem __user *post_deps;
- int i, ret;
- if (!count)
return 0;- se->out_syncs = (struct v3d_submit_outsync *)
kvmalloc_array(count,sizeof(structv3d_submit_outsync),
GFP_KERNEL);- if (!se->out_syncs)
return -ENOMEM;- post_deps = u64_to_user_ptr(handles);
- for (i = 0; i < count; i++) {
struct drm_v3d_sem out;ret = copy_from_user(&out, post_deps++, sizeof(out));if (ret) {DRM_DEBUG("Failed to copy post dep handles\n");goto fail;}se->out_syncs[i].syncobj = drm_syncobj_find(file_priv,out.handle);
if (!se->out_syncs[i].syncobj) {ret = -EINVAL;goto fail;}- se->out_sync_count = count;
- return 0;
+fail:
- for (i--; i >= 0; i--)
drm_syncobj_put(se->out_syncs[i].syncobj);- kvfree(se->out_syncs);
- return ret;
+}
+/* Get data for multiple binary semaphores synchronization. Parse syncobj
- to be signaled when job completes (out_sync).
- */
+static int +v3d_get_multisync_submit_deps(struct drm_file *file_priv,
struct drm_v3d_extension __user *ext,void *data)+{
- struct drm_v3d_multi_sync multisync;
- struct v3d_submit_ext *se = data;
- int ret;
- ret = copy_from_user(&multisync, ext, sizeof(multisync));
- if (ret)
return ret;- if (multisync.pad)
return -EINVAL;- ret = v3d_get_multisync_post_deps(file_priv, data,
multisync.out_sync_count,
multisync.out_syncs);- if (ret)
return ret;- se->in_sync_count = multisync.in_sync_count;
- se->in_syncs = multisync.in_syncs;
- se->flags |= DRM_V3D_EXT_ID_MULTI_SYNC;
- se->wait_stage = multisync.wait_stage;
- return 0;
}
/* Whenever userspace sets ioctl extensions, v3d_get_extensions parses data
- according to the extension id (name).
*/ static int -v3d_get_extensions(struct drm_file *file_priv, u64 ext_handles) +v3d_get_extensions(struct drm_file *file_priv,
u64 ext_handles,void *data){ struct drm_v3d_extension __user *user_ext;
int ret;
user_ext = u64_to_user_ptr(ext_handles); while (user_ext) {
@@ -555,7 +687,11 @@ v3d_get_extensions(struct drm_file *file_priv, u64 ext_handles) }
switch (ext.id) {
case 0:
case DRM_V3D_EXT_ID_MULTI_SYNC:ret = v3d_get_multisync_submit_deps(file_priv,user_ext, data);
if (ret)return ret;break;ext.id); return -EINVAL; @@ -586,6 +722,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data, struct v3d_dev *v3d = to_v3d_dev(dev); struct v3d_file_priv *v3d_priv = file_priv->driver_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;
@@ -606,7 +743,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data, }
if (args->flags & DRM_V3D_SUBMIT_EXTENSION) {
ret = v3d_get_extensions(file_priv, args->extensions);
ret = v3d_get_extensions(file_priv, args->extensions,&se); if (ret) { DRM_DEBUG("Failed to get extensions.\n"); return ret; @@ -614,7 +751,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data, }
ret = v3d_job_init(v3d, file_priv, (void *)&render, sizeof(*render),
v3d_render_job_free, args->in_sync_rcl,V3D_RENDER);
v3d_render_job_free, args->in_sync_rcl, &se,V3D_RENDER); if (ret) goto fail;
@@ -624,7 +761,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
if (args->bcl_start != args->bcl_end) { ret = v3d_job_init(v3d, file_priv, (void *)&bin, sizeof(*bin),
v3d_job_free, args->in_sync_bcl,V3D_BIN);
v3d_job_free, args->in_sync_bcl,&se, V3D_BIN); if (ret) goto fail;
@@ -638,7 +775,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
if (args->flags & DRM_V3D_SUBMIT_CL_FLUSH_CACHE) { ret = v3d_job_init(v3d, file_priv, (void *)&clean_job, sizeof(*clean_job),
v3d_job_free, 0, V3D_CACHE_CLEAN);
v3d_job_free, 0, 0,V3D_CACHE_CLEAN); if (ret) goto fail;
@@ -698,6 +835,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data, last_job, &acquire_ctx, args->out_sync,
&se, last_job->done_fence);if (bin)
@@ -716,6 +854,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data, v3d_job_cleanup((void *)bin); v3d_job_cleanup((void *)render); v3d_job_cleanup(clean_job);
v3d_put_multisync_post_deps(&se);
return ret;
} @@ -735,6 +874,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data, { struct v3d_dev *v3d = to_v3d_dev(dev); struct drm_v3d_submit_tfu *args = data;
- struct v3d_submit_ext se = {0}; struct v3d_tfu_job *job = NULL; struct ww_acquire_ctx acquire_ctx; int ret = 0;
@@ -747,7 +887,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data, }
if (args->flags & DRM_V3D_SUBMIT_EXTENSION) {
ret = v3d_get_extensions(file_priv, args->extensions);
ret = v3d_get_extensions(file_priv, args->extensions,&se); if (ret) { DRM_DEBUG("Failed to get extensions.\n"); return ret; @@ -755,7 +895,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data, }
ret = v3d_job_init(v3d, file_priv, (void *)&job, sizeof(*job),
v3d_job_free, args->in_sync, V3D_TFU);
v3d_job_free, args->in_sync, &se, V3D_TFU);@@ -803,6 +943,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data, v3d_attach_fences_and_unlock_reservation(file_priv, &job->base, &acquire_ctx, args->out_sync,
&se, job->base.done_fence);v3d_job_put(&job->base);
@@ -811,6 +952,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
fail: v3d_job_cleanup((void *)job);
v3d_put_multisync_post_deps(&se);
return ret;
} @@ -831,6 +973,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data, struct v3d_dev *v3d = to_v3d_dev(dev); struct v3d_file_priv *v3d_priv = file_priv->driver_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 ww_acquire_ctx acquire_ctx;
@@ -852,7 +995,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data, }
if (args->flags & DRM_V3D_SUBMIT_EXTENSION) {
ret = v3d_get_extensions(file_priv, args->extensions);
ret = v3d_get_extensions(file_priv, args->extensions,&se); if (ret) { DRM_DEBUG("Failed to get extensions.\n"); return ret; @@ -860,12 +1003,12 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data, }
ret = v3d_job_init(v3d, file_priv, (void *)&job, sizeof(*job),
v3d_job_free, args->in_sync, V3D_CSD);
v3d_job_free, args->in_sync, &se, V3D_CSD);if (ret) goto fail;
ret = v3d_job_init(v3d, file_priv, (void *)&clean_job,
sizeof(*clean_job),
v3d_job_free, 0, V3D_CACHE_CLEAN);
v3d_job_free, 0, 0, V3D_CACHE_CLEAN);@@ -904,6 +1047,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data, clean_job, &acquire_ctx, args->out_sync,
&se, clean_job-done_fence);
v3d_job_put(&job->base); @@ -918,6 +1062,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data, fail: v3d_job_cleanup((void *)job); v3d_job_cleanup(clean_job);
v3d_put_multisync_post_deps(&se);
return ret;
} diff --git a/include/uapi/drm/v3d_drm.h b/include/uapi/drm/v3d_drm.h index 55b443ca6c0b..3dfc0af8756a 100644 --- a/include/uapi/drm/v3d_drm.h +++ b/include/uapi/drm/v3d_drm.h @@ -73,6 +73,53 @@ struct drm_v3d_extension { __u32 flags; /* mbz */ };
+/* struct drm_v3d_sem - wait/signal semaphore
- If binary semaphore, it only takes syncobj handle and ignores
flags and
- point fields. Point is defined for timeline syncobj feature.
- */
+struct drm_v3d_sem {
- __u32 handle; /* syncobj */
- /* rsv below, for future uses */
- __u32 flags;
- __u64 point; /* for timeline sem support */
- __u64 mbz[2]; /* must be zero, rsv */
+};
+/* Enum for each of the V3D queues. */ +enum v3d_queue {
- V3D_BIN,
- V3D_RENDER,
- V3D_TFU,
- V3D_CSD,
- V3D_CACHE_CLEAN,
+};
+/**
- struct drm_v3d_multi_sync - ioctl extension to add support
multiples
- syncobjs for commands submission.
- When an extension of DRM_V3D_EXT_ID_MULTI_SYNC id is defined, it
points to
- this extension to define wait and signal dependencies, instead of
single
- in/out sync entries on submitting commands. The field flags is
used to
- determine the stage to set wait dependencies.
- */
+struct drm_v3d_multi_sync {
- struct drm_v3d_extension base;
- /* Array of wait and signal semaphores */
- __u64 in_syncs;
- __u64 out_syncs;
- /* Number of entries */
- __u32 in_sync_count;
- __u32 out_sync_count;
- /* set the stage (v3d_queue) to sync */
- __u32 wait_stage;
- __u32 pad; /* mbz */
+};
/**
- struct drm_v3d_submit_cl - ioctl argument for submitting commands
to the 3D
- engine.
@@ -228,6 +275,7 @@ enum drm_v3d_param { DRM_V3D_PARAM_SUPPORTS_CSD, DRM_V3D_PARAM_SUPPORTS_CACHE_FLUSH, DRM_V3D_PARAM_SUPPORTS_PERFMON,
- DRM_V3D_PARAM_SUPPORTS_MULTISYNC_EXT,
};
struct drm_v3d_get_param { @@ -271,7 +319,6 @@ struct drm_v3d_submit_tfu {
/* Pointer to an array of ioctl extensions*/ __u64 extensions;
};
/* Submits a compute shader for dispatch. This job will block on any
On 10/01, Iago Toral wrote:
On Thu, 2021-09-30 at 17:19 +0100, Melissa Wen wrote:
Using the generic extension from the previous patch, a specific multisync extension enables more than one in/out binary syncobj per job submission. Arrays of syncobjs are set in struct drm_v3d_multisync, that also cares of determining the stage for sync (wait deps) according to the job queue.
v2:
- subclass the generic extension struct (Daniel)
- simplify adding dependency conditions to make understandable (Iago)
v3:
- fix conditions to consider single or multiples in/out_syncs (Iago)
- remove irrelevant comment (Iago)
Signed-off-by: Melissa Wen mwen@igalia.com
drivers/gpu/drm/v3d/v3d_drv.c | 6 +- drivers/gpu/drm/v3d/v3d_drv.h | 24 +++-- drivers/gpu/drm/v3d/v3d_gem.c | 185 ++++++++++++++++++++++++++++++
include/uapi/drm/v3d_drm.h | 49 ++++++++- 4 files changed, 232 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c index 3d6b9bcce2f7..bd46396a1ae0 100644 --- a/drivers/gpu/drm/v3d/v3d_drv.c +++ b/drivers/gpu/drm/v3d/v3d_drv.c @@ -96,6 +96,9 @@ static int v3d_get_param_ioctl(struct drm_device
(...)
@@ -516,9 +536,11 @@ v3d_attach_fences_and_unlock_reservation(struct drm_file *file_priv, struct v3d_job *job, struct ww_acquire_ctx *acquire_ctx, u32 out_sync,
struct v3d_submit_ext *se, struct dma_fence *done_fence){ struct drm_syncobj *sync_out;
- bool has_multisync = se && (se->flags &
We always pass the 'se' pointer from a local variable allocated in the stack of the caller so it is never going to be NULL.
I am happy to keep the NULL check if you want to protect against future changes just in case, but if we do that then...
DRM_V3D_EXT_ID_MULTI_SYNC); int i;
for (i = 0; i < job->bo_count; i++) {
(...)
+static void +v3d_put_multisync_post_deps(struct v3d_submit_ext *se) +{
- unsigned int i;
- if (!se->out_sync_count)
...we should also check for NULL here for consistency.
yes, consistency makes sense here.
Also, I think there is another problem in the code: we always call v3d_job_cleanup for failed jobs, but only call v3d_job_put for successful jobs. However, reading the docs for drm_sched_job_init:
"Drivers must make sure drm_sched_job_cleanup() if this function returns successfully, even when @job is aborted before drm_sched_job_arm() is called."
So my understanding is that we should call v3d_job_cleanup instead of v3d_job_put for successful jobs or we would be leaking sched resources on every successful job, no?
When job_init is successful, v3d_job_cleanup is called by scheduler when job is completed. drm_sched_job_cleanup describes how it works after drm_sched_job_arm:
" After that point of no return @job is committed to be executed by the scheduler, and this function should be called from the &drm_sched_backend_ops.free_job callback."
On v3d_sched.c, .free_job points to v3d_sched_job_free(), which in turn calls v3d_job_cleanup() (and then drm_sched_job_cleanup). So, it looks ok.
Also, we can say that the very last v3d_job_put() is in charge to decrement refcount initialized (set 1) in v3d_job_init(); while the v3d_job_cleanup() from v3d_sched_job_free() callback decrements refcount that was incremented when v3d_job_push() pushed the job to the scheduler.
So, refcount pairs seem consistent, I mean, get and put. And about drm_sched_job_cleanup, it is explicitly called when job_init fails or after that by the scheduler.
Thanks,
Melissa
Iago
return;- for (i = 0; i < se->out_sync_count; i++)
drm_syncobj_put(se->out_syncs[i].syncobj);- kvfree(se->out_syncs);
+}
+static int +v3d_get_multisync_post_deps(struct drm_file *file_priv,
struct v3d_submit_ext *se,u32 count, u64 handles)+{
- struct drm_v3d_sem __user *post_deps;
- int i, ret;
- if (!count)
return 0;- se->out_syncs = (struct v3d_submit_outsync *)
kvmalloc_array(count,sizeof(structv3d_submit_outsync),
GFP_KERNEL);- if (!se->out_syncs)
return -ENOMEM;- post_deps = u64_to_user_ptr(handles);
- for (i = 0; i < count; i++) {
struct drm_v3d_sem out;ret = copy_from_user(&out, post_deps++, sizeof(out));if (ret) {DRM_DEBUG("Failed to copy post dep handles\n");goto fail;}se->out_syncs[i].syncobj = drm_syncobj_find(file_priv,out.handle);
if (!se->out_syncs[i].syncobj) {ret = -EINVAL;goto fail;}- se->out_sync_count = count;
- return 0;
+fail:
- for (i--; i >= 0; i--)
drm_syncobj_put(se->out_syncs[i].syncobj);- kvfree(se->out_syncs);
- return ret;
+}
+/* Get data for multiple binary semaphores synchronization. Parse syncobj
- to be signaled when job completes (out_sync).
- */
+static int +v3d_get_multisync_submit_deps(struct drm_file *file_priv,
struct drm_v3d_extension __user *ext,void *data)+{
- struct drm_v3d_multi_sync multisync;
- struct v3d_submit_ext *se = data;
- int ret;
- ret = copy_from_user(&multisync, ext, sizeof(multisync));
- if (ret)
return ret;- if (multisync.pad)
return -EINVAL;- ret = v3d_get_multisync_post_deps(file_priv, data,
multisync.out_sync_count,
multisync.out_syncs);- if (ret)
return ret;- se->in_sync_count = multisync.in_sync_count;
- se->in_syncs = multisync.in_syncs;
- se->flags |= DRM_V3D_EXT_ID_MULTI_SYNC;
- se->wait_stage = multisync.wait_stage;
- return 0;
}
/* Whenever userspace sets ioctl extensions, v3d_get_extensions parses data
- according to the extension id (name).
*/ static int -v3d_get_extensions(struct drm_file *file_priv, u64 ext_handles) +v3d_get_extensions(struct drm_file *file_priv,
u64 ext_handles,void *data){ struct drm_v3d_extension __user *user_ext;
int ret;
user_ext = u64_to_user_ptr(ext_handles); while (user_ext) {
@@ -555,7 +687,11 @@ v3d_get_extensions(struct drm_file *file_priv, u64 ext_handles) }
switch (ext.id) {
case 0:
case DRM_V3D_EXT_ID_MULTI_SYNC:ret = v3d_get_multisync_submit_deps(file_priv,user_ext, data);
if (ret)return ret;break;ext.id); return -EINVAL; @@ -586,6 +722,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data, struct v3d_dev *v3d = to_v3d_dev(dev); struct v3d_file_priv *v3d_priv = file_priv->driver_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;
@@ -606,7 +743,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data, }
if (args->flags & DRM_V3D_SUBMIT_EXTENSION) {
ret = v3d_get_extensions(file_priv, args->extensions);
ret = v3d_get_extensions(file_priv, args->extensions,&se); if (ret) { DRM_DEBUG("Failed to get extensions.\n"); return ret; @@ -614,7 +751,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data, }
ret = v3d_job_init(v3d, file_priv, (void *)&render, sizeof(*render),
v3d_render_job_free, args->in_sync_rcl,V3D_RENDER);
v3d_render_job_free, args->in_sync_rcl, &se,V3D_RENDER); if (ret) goto fail;
@@ -624,7 +761,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
if (args->bcl_start != args->bcl_end) { ret = v3d_job_init(v3d, file_priv, (void *)&bin, sizeof(*bin),
v3d_job_free, args->in_sync_bcl,V3D_BIN);
v3d_job_free, args->in_sync_bcl,&se, V3D_BIN); if (ret) goto fail;
@@ -638,7 +775,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,
if (args->flags & DRM_V3D_SUBMIT_CL_FLUSH_CACHE) { ret = v3d_job_init(v3d, file_priv, (void *)&clean_job, sizeof(*clean_job),
v3d_job_free, 0, V3D_CACHE_CLEAN);
v3d_job_free, 0, 0,V3D_CACHE_CLEAN); if (ret) goto fail;
@@ -698,6 +835,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data, last_job, &acquire_ctx, args->out_sync,
&se, last_job->done_fence);if (bin)
@@ -716,6 +854,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data, v3d_job_cleanup((void *)bin); v3d_job_cleanup((void *)render); v3d_job_cleanup(clean_job);
v3d_put_multisync_post_deps(&se);
return ret;
} @@ -735,6 +874,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data, { struct v3d_dev *v3d = to_v3d_dev(dev); struct drm_v3d_submit_tfu *args = data;
- struct v3d_submit_ext se = {0}; struct v3d_tfu_job *job = NULL; struct ww_acquire_ctx acquire_ctx; int ret = 0;
@@ -747,7 +887,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data, }
if (args->flags & DRM_V3D_SUBMIT_EXTENSION) {
ret = v3d_get_extensions(file_priv, args->extensions);
ret = v3d_get_extensions(file_priv, args->extensions,&se); if (ret) { DRM_DEBUG("Failed to get extensions.\n"); return ret; @@ -755,7 +895,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data, }
ret = v3d_job_init(v3d, file_priv, (void *)&job, sizeof(*job),
v3d_job_free, args->in_sync, V3D_TFU);
v3d_job_free, args->in_sync, &se, V3D_TFU);@@ -803,6 +943,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data, v3d_attach_fences_and_unlock_reservation(file_priv, &job->base, &acquire_ctx, args->out_sync,
&se, job->base.done_fence);v3d_job_put(&job->base);
@@ -811,6 +952,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,
fail: v3d_job_cleanup((void *)job);
v3d_put_multisync_post_deps(&se);
return ret;
} @@ -831,6 +973,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data, struct v3d_dev *v3d = to_v3d_dev(dev); struct v3d_file_priv *v3d_priv = file_priv->driver_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 ww_acquire_ctx acquire_ctx;
@@ -852,7 +995,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data, }
if (args->flags & DRM_V3D_SUBMIT_EXTENSION) {
ret = v3d_get_extensions(file_priv, args->extensions);
ret = v3d_get_extensions(file_priv, args->extensions,&se); if (ret) { DRM_DEBUG("Failed to get extensions.\n"); return ret; @@ -860,12 +1003,12 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data, }
ret = v3d_job_init(v3d, file_priv, (void *)&job, sizeof(*job),
v3d_job_free, args->in_sync, V3D_CSD);
v3d_job_free, args->in_sync, &se, V3D_CSD);if (ret) goto fail;
ret = v3d_job_init(v3d, file_priv, (void *)&clean_job,
sizeof(*clean_job),
v3d_job_free, 0, V3D_CACHE_CLEAN);
v3d_job_free, 0, 0, V3D_CACHE_CLEAN);@@ -904,6 +1047,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data, clean_job, &acquire_ctx, args->out_sync,
&se, clean_job-done_fence);
v3d_job_put(&job->base); @@ -918,6 +1062,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data, fail: v3d_job_cleanup((void *)job); v3d_job_cleanup(clean_job);
v3d_put_multisync_post_deps(&se);
return ret;
} diff --git a/include/uapi/drm/v3d_drm.h b/include/uapi/drm/v3d_drm.h index 55b443ca6c0b..3dfc0af8756a 100644 --- a/include/uapi/drm/v3d_drm.h +++ b/include/uapi/drm/v3d_drm.h @@ -73,6 +73,53 @@ struct drm_v3d_extension { __u32 flags; /* mbz */ };
+/* struct drm_v3d_sem - wait/signal semaphore
- If binary semaphore, it only takes syncobj handle and ignores
flags and
- point fields. Point is defined for timeline syncobj feature.
- */
+struct drm_v3d_sem {
- __u32 handle; /* syncobj */
- /* rsv below, for future uses */
- __u32 flags;
- __u64 point; /* for timeline sem support */
- __u64 mbz[2]; /* must be zero, rsv */
+};
+/* Enum for each of the V3D queues. */ +enum v3d_queue {
- V3D_BIN,
- V3D_RENDER,
- V3D_TFU,
- V3D_CSD,
- V3D_CACHE_CLEAN,
+};
+/**
- struct drm_v3d_multi_sync - ioctl extension to add support
multiples
- syncobjs for commands submission.
- When an extension of DRM_V3D_EXT_ID_MULTI_SYNC id is defined, it
points to
- this extension to define wait and signal dependencies, instead of
single
- in/out sync entries on submitting commands. The field flags is
used to
- determine the stage to set wait dependencies.
- */
+struct drm_v3d_multi_sync {
- struct drm_v3d_extension base;
- /* Array of wait and signal semaphores */
- __u64 in_syncs;
- __u64 out_syncs;
- /* Number of entries */
- __u32 in_sync_count;
- __u32 out_sync_count;
- /* set the stage (v3d_queue) to sync */
- __u32 wait_stage;
- __u32 pad; /* mbz */
+};
/**
- struct drm_v3d_submit_cl - ioctl argument for submitting commands
to the 3D
- engine.
@@ -228,6 +275,7 @@ enum drm_v3d_param { DRM_V3D_PARAM_SUPPORTS_CSD, DRM_V3D_PARAM_SUPPORTS_CACHE_FLUSH, DRM_V3D_PARAM_SUPPORTS_PERFMON,
- DRM_V3D_PARAM_SUPPORTS_MULTISYNC_EXT,
};
struct drm_v3d_get_param { @@ -271,7 +319,6 @@ struct drm_v3d_submit_tfu {
/* Pointer to an array of ioctl extensions*/ __u64 extensions;
};
/* Submits a compute shader for dispatch. This job will block on any
On Fri, 2021-10-01 at 09:37 +0100, Melissa Wen wrote:
On 10/01, Iago Toral wrote:
On Thu, 2021-09-30 at 17:19 +0100, Melissa Wen wrote:
Using the generic extension from the previous patch, a specific multisync extension enables more than one in/out binary syncobj per job submission. Arrays of syncobjs are set in struct drm_v3d_multisync, that also cares of determining the stage for sync (wait deps) according to the job queue.
v2:
- subclass the generic extension struct (Daniel)
- simplify adding dependency conditions to make understandable
(Iago)
v3:
- fix conditions to consider single or multiples in/out_syncs
(Iago)
- remove irrelevant comment (Iago)
Signed-off-by: Melissa Wen mwen@igalia.com
drivers/gpu/drm/v3d/v3d_drv.c | 6 +- drivers/gpu/drm/v3d/v3d_drv.h | 24 +++-- drivers/gpu/drm/v3d/v3d_gem.c | 185
++++++++++++++++++++++++++++++
include/uapi/drm/v3d_drm.h | 49 ++++++++- 4 files changed, 232 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c index 3d6b9bcce2f7..bd46396a1ae0 100644 --- a/drivers/gpu/drm/v3d/v3d_drv.c +++ b/drivers/gpu/drm/v3d/v3d_drv.c @@ -96,6 +96,9 @@ static int v3d_get_param_ioctl(struct drm_device
(...)
@@ -516,9 +536,11 @@ v3d_attach_fences_and_unlock_reservation(struct drm_file *file_priv, struct v3d_job *job, struct ww_acquire_ctx *acquire_ctx, u32 out_sync,
struct v3d_submit_ext *se, struct dma_fence *done_fence){ struct drm_syncobj *sync_out;
- bool has_multisync = se && (se->flags &
We always pass the 'se' pointer from a local variable allocated in the stack of the caller so it is never going to be NULL.
I am happy to keep the NULL check if you want to protect against future changes just in case, but if we do that then...
DRM_V3D_EXT_ID_MULTI_SYNC); int i;
for (i = 0; i < job->bo_count; i++) {
(...)
+static void +v3d_put_multisync_post_deps(struct v3d_submit_ext *se) +{
- unsigned int i;
- if (!se->out_sync_count)
...we should also check for NULL here for consistency.
yes, consistency makes sense here.
Also, I think there is another problem in the code: we always call v3d_job_cleanup for failed jobs, but only call v3d_job_put for successful jobs. However, reading the docs for drm_sched_job_init:
"Drivers must make sure drm_sched_job_cleanup() if this function returns successfully, even when @job is aborted before drm_sched_job_arm() is called."
So my understanding is that we should call v3d_job_cleanup instead of v3d_job_put for successful jobs or we would be leaking sched resources on every successful job, no?
When job_init is successful, v3d_job_cleanup is called by scheduler when job is completed. drm_sched_job_cleanup describes how it works after drm_sched_job_arm:
" After that point of no return @job is committed to be executed by the scheduler, and this function should be called from the &drm_sched_backend_ops.free_job callback."
On v3d_sched.c, .free_job points to v3d_sched_job_free(), which in turn calls v3d_job_cleanup() (and then drm_sched_job_cleanup). So, it looks ok.
Also, we can say that the very last v3d_job_put() is in charge to decrement refcount initialized (set 1) in v3d_job_init(); while the v3d_job_cleanup() from v3d_sched_job_free() callback decrements refcount that was incremented when v3d_job_push() pushed the job to the scheduler.
So, refcount pairs seem consistent, I mean, get and put. And about drm_sched_job_cleanup, it is explicitly called when job_init fails or after that by the scheduler.
A. Ah ok, thanks for explaining this!
With the consistency issue discussed above fixed, for the series:
Reviewed-by: Iago Toral Quiroga itoral@igalia.com
Iago
On 10/01, Iago Toral wrote:
On Fri, 2021-10-01 at 09:37 +0100, Melissa Wen wrote:
On 10/01, Iago Toral wrote:
On Thu, 2021-09-30 at 17:19 +0100, Melissa Wen wrote:
Using the generic extension from the previous patch, a specific multisync extension enables more than one in/out binary syncobj per job submission. Arrays of syncobjs are set in struct drm_v3d_multisync, that also cares of determining the stage for sync (wait deps) according to the job queue.
v2:
- subclass the generic extension struct (Daniel)
- simplify adding dependency conditions to make understandable
(Iago)
v3:
- fix conditions to consider single or multiples in/out_syncs
(Iago)
- remove irrelevant comment (Iago)
Signed-off-by: Melissa Wen mwen@igalia.com
drivers/gpu/drm/v3d/v3d_drv.c | 6 +- drivers/gpu/drm/v3d/v3d_drv.h | 24 +++-- drivers/gpu/drm/v3d/v3d_gem.c | 185
++++++++++++++++++++++++++++++
include/uapi/drm/v3d_drm.h | 49 ++++++++- 4 files changed, 232 insertions(+), 32 deletions(-)
diff --git a/drivers/gpu/drm/v3d/v3d_drv.c b/drivers/gpu/drm/v3d/v3d_drv.c index 3d6b9bcce2f7..bd46396a1ae0 100644 --- a/drivers/gpu/drm/v3d/v3d_drv.c +++ b/drivers/gpu/drm/v3d/v3d_drv.c @@ -96,6 +96,9 @@ static int v3d_get_param_ioctl(struct drm_device
(...)
@@ -516,9 +536,11 @@ v3d_attach_fences_and_unlock_reservation(struct drm_file *file_priv, struct v3d_job *job, struct ww_acquire_ctx *acquire_ctx, u32 out_sync,
struct v3d_submit_ext *se, struct dma_fence *done_fence){ struct drm_syncobj *sync_out;
- bool has_multisync = se && (se->flags &
We always pass the 'se' pointer from a local variable allocated in the stack of the caller so it is never going to be NULL.
I am happy to keep the NULL check if you want to protect against future changes just in case, but if we do that then...
DRM_V3D_EXT_ID_MULTI_SYNC); int i;
for (i = 0; i < job->bo_count; i++) {
(...)
+static void +v3d_put_multisync_post_deps(struct v3d_submit_ext *se) +{
- unsigned int i;
- if (!se->out_sync_count)
...we should also check for NULL here for consistency.
yes, consistency makes sense here.
Also, I think there is another problem in the code: we always call v3d_job_cleanup for failed jobs, but only call v3d_job_put for successful jobs. However, reading the docs for drm_sched_job_init:
"Drivers must make sure drm_sched_job_cleanup() if this function returns successfully, even when @job is aborted before drm_sched_job_arm() is called."
So my understanding is that we should call v3d_job_cleanup instead of v3d_job_put for successful jobs or we would be leaking sched resources on every successful job, no?
When job_init is successful, v3d_job_cleanup is called by scheduler when job is completed. drm_sched_job_cleanup describes how it works after drm_sched_job_arm:
" After that point of no return @job is committed to be executed by the scheduler, and this function should be called from the &drm_sched_backend_ops.free_job callback."
On v3d_sched.c, .free_job points to v3d_sched_job_free(), which in turn calls v3d_job_cleanup() (and then drm_sched_job_cleanup). So, it looks ok.
Also, we can say that the very last v3d_job_put() is in charge to decrement refcount initialized (set 1) in v3d_job_init(); while the v3d_job_cleanup() from v3d_sched_job_free() callback decrements refcount that was incremented when v3d_job_push() pushed the job to the scheduler.
So, refcount pairs seem consistent, I mean, get and put. And about drm_sched_job_cleanup, it is explicitly called when job_init fails or after that by the scheduler.
A. Ah ok, thanks for explaining this!
nice!
With the consistency issue discussed above fixed, for the series:
Reviewed-by: Iago Toral Quiroga itoral@igalia.com
ok, thanks for reviewing and all improvement suggestions,
Melissa
Iago
dri-devel@lists.freedesktop.org
-
Iago Toral -
Melissa Wen