Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] Missing implicit synchronization for V3D_BIN jobs leads to data corruption when reading shared BOs -- --- Patch [1]: [PATCH v4] drm/v3d: Associate BOs with every job that accesses them --- commit 72f7a40fe9b93eb2404edd2978dd5c1851c59920 Author: Maíra Canal <[email protected]> drm/v3d: Associate BOs with every job that accesses them This patch associates BOs with the specific jobs that consume them, rather than just the last job in a submission chain, to ensure correct implicit dependencies are acquired. It introduces a has_implicit_dep flag to avoid adding these dependencies to jobs that don't need them, like CACHE_CLEAN. > diff --git a/drivers/gpu/drm/v3d/v3d_submit.c > b/drivers/gpu/drm/v3d/v3d_submit.c > index 119a81c9e1c46..834d52030979d 100644 > --- a/drivers/gpu/drm/v3d/v3d_submit.c > +++ b/drivers/gpu/drm/v3d/v3d_submit.c [ ... ] > @@ -223,13 +246,14 @@ v3d_job_add_syncobjs(struct v3d_job *job, struct > drm_file *file_priv, > static const struct { > size_t size; > void (*free)(struct kref *ref); > + bool has_implicit_dep; > } 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 }, > + [V3D_BIN] = { sizeof(struct v3d_bin_job), v3d_job_free, > false }, [Severity: High] This is a pre-existing issue, but does setting has_implicit_dep to false for V3D_BIN jobs risk data corruption when a user space application shares a BO between contexts via dma-buf? If a shared BO is written by a first context and subsequently used as a Vertex Buffer in a rendering submission in a second context, the BIN job will run Vertex Shaders to compute positions. If it explicitly lacks implicit dependencies, could the DRM scheduler dispatch it immediately, causing it to read the Vertex Buffer while the first context is still writing to it? [ ... ] > @@ -1165,22 +1195,33 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void > *data, > if (ret) > goto fail; > > + /* > + * We don't associate the BOs with the BIN job. Fences are only > + * attached to the last job in the submission chain, and BIN jobs > + * don't need implicit dependencies since depending on results from > + * another context is not a realistic scenario for binning. > + */ > + ret = v3d_lookup_bos(submit.file_priv, &render->base, > + args->bo_handles, args->bo_handle_count); [Severity: High] This is a pre-existing issue, but does intentionally omitting BO attachment for the BIN job prevent it from tracking the buffers it reads? The comment mentions that depending on results from another context is not a realistic scenario for binning. However, if a buffer is exported from another subsystem (like a video decoder or compute) and used as a vertex buffer, wouldn't the binning job need to wait for that buffer's dma_resv fences to ensure the data is fully written before reading it? > + if (ret) > + goto fail; > + -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
