Hi Tvrtko,
Thanks for review!
On 25/05/26 09:29, Tvrtko Ursulin wrote:
Would it work to pass struct v3d_job **job instead of the void
**container? And add like:
BUILD_BUG_ON(offsetof(struct v3d_bin_job, base));
... for all job types ...
Callers can then pass in like:
..., &bin->base, sizeof(*bin), ...
Slightly nicer and safer I think but up to you.
I'll address it in the next version. As I'll change this function, I
won't add your R-b.
+ *container = kzalloc(size, GFP_KERNEL);
+ if (!*container)
+ return -ENOMEM;
+
+ job = *container;
+ job->v3d = v3d;
+ job->free = free;
+ job->file_priv = v3d_priv;
+
[...]
+
+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;
This check ends up quite deep in the submission chain so I wonder if it
makes sense to pull it out and have it earlier? It appears racy in any
case since AFAIU userspace can mess with it from a separate thread. Or
perhaps can keep in simple just as well.
But it possibly warrants a READ_ONCE too to document it is elsewhere
managed by (cmp)xchg.
I'll "ignore" this comment as it's addressed in a much complete way in
[1].
[1]
https://lore.kernel.org/dri-devel/[email protected]/T/
[...]
@@ -933,131 +1044,82 @@ 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));
+ ret = v3d_submit_add_job(&submit, (void **)&bin, sizeof(*bin),
+ v3d_job_free, V3D_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);
- goto fail;
- }
-
bin->start = args->bcl_start;
bin->end = args->bcl_end;
bin->qma = args->qma;
bin->qms = args->qms;
bin->qts = args->qts;
- bin->render = render;
- }
Is there a particular reason why you changed the order of render and bin
job creation? I don't think it makes a difference to be clear, but just
checking in case there is some subtlety I missed.
As we now have v3d_submit_jobs(), we need to add the jobs to the array
in order of dependency. The BIN jobs depends on the RENDER job and
that's why we need to add it first.
Best regards,
- Maíra
