Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] Reference count double-decrement and underflow in job submission error path - [High] ABI Breakage: `seqno` is not returned to userspace upon successful job submission - [High] Uninitialized `exec->bo` and `exec->rcl_write_bo_count` fields break job validation and dependency tracking --
--- Patch [4]: [PATCH v3 4/8] drm/vc4: Add new job submission implementation --- commit 394b85fa6f0403b5520bcd58bae0ec277c6cabc7 Author: Maíra Canal <[email protected]> drm/vc4: Add new job submission implementation Introduce vc4_submit.c with the job submission path rewritten to integrate with the DRM GPU scheduler. Most of this code is adapted from vc4_gem.c, with key changes concentrated in the job creation and lifecycle management. > --- /dev/null > +++ b/drivers/gpu/drm/vc4/vc4_submit.c [ ... ] > +static int > +vc4_add_implicit_dependencies(struct vc4_exec_info *exec) > +{ > + struct vc4_render_job *render = exec->render; [ ... ] > + /* > + * The renderer writes the RCL targets (color/Z/MSAA buffers), so it > + * additionally has to wait for prior readers of those. > + */ > + for (int i = 0; i < render->rcl_write_bo_count; i++) { > + ret = > drm_sched_job_add_implicit_dependencies(&render->base.base, > + > &render->rcl_write_bo[i]->base, > + true); > + if (ret) > + return ret; > + } > + > + return 0; > +} [Severity: High] Does this loop skip adding implicit dependencies? The validation code in vc4_get_rcl() appears to populate exec->rcl_write_bo_count, but this loop iterates over render->rcl_write_bo_count, which remains 0. [ ... ] > +static int > +vc4_lookup_bos(struct drm_device *dev, struct drm_file *file_priv, > + struct vc4_render_job *job, u64 bo_handles, u32 bo_count) > +{ [ ... ] > + for (i = 0; i < job->bo_count; i++) { > + ret = vc4_bo_inc_usecnt(to_vc4_bo(job->bo[i])); > + if (ret) > + goto fail_dec_usecnt; > + } > + > + return 0; > + > +fail_dec_usecnt: > + /* Decrease usecnt on acquired objects */ > + for (i--; i >= 0; i--) > + vc4_bo_dec_usecnt(to_vc4_bo(job->bo[i])); > + return ret; > +} [ ... ] > +static void > +vc4_render_job_free(struct kref *ref) > +{ > + struct vc4_render_job *job = container_of(ref, struct vc4_render_job, > + base.refcount); > + struct vc4_dev *vc4 = job->base.vc4; > + struct vc4_bo *bo, *tmp; > + unsigned long irqflags; > + > + if (job->bo) { > + for (int i = 0; i < job->bo_count; i++) { > + struct vc4_bo *bo = to_vc4_bo(job->bo[i]); > + > + vc4_bo_dec_usecnt(bo); [Severity: High] If vc4_bo_inc_usecnt() fails in vc4_lookup_bos(), the error path rolls back the increments but leaves job->bo fully populated. When the error propagates up and triggers vc4_render_job_free(), the cleanup code iterates over job->bo and unconditionally decrements the usecnt again. Could this cause a double-decrement and underflow for the BOs? [ ... ] > +int > +vc4_submit_cl_ioctl(struct drm_device *dev, void *data, > + struct drm_file *file_priv) > +{ [ ... ] > + exec.render = render; > + INIT_LIST_HEAD(&render->unref_list); > + > + ret = vc4_lookup_bos(dev, file_priv, render, args->bo_handles, > + args->bo_handle_count); > + if (ret) > + goto fail; [Severity: High] This call populates render->bo, but does it leave exec.bo uninitialized and exec.bo_count as 0? If validation functions like vc4_get_bcl() and vc4_get_rcl() still rely on the legacy exec->bo array via vc4_use_bo(), this could cause shader validation to fail with -EINVAL. [ ... ] > + scoped_guard(mutex, &vc4->sched_lock) { > + ret = vc4_push_jobs(&exec); > + } > + > + if (!ret) { > + vc4_attach_fences(render, sync_out, render->base.done_fence); > + } else if (sync_out) { > + /* The jobs were never submitted, so release the unpublished > syncobj */ > + drm_syncobj_put(sync_out); > + } [Severity: High] Are we missing the sequence number assignment to userspace here? The drm_vc4_submit_cl structure expects the kernel to return the sequence number of the submitted job in the seqno field. Since args->seqno is not populated before returning success, could this prevent userspace from properly waiting on the submitted job? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=4
