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

["Maíra Canal" <mcanal@igalia.com>]

Hi Tvrtko,

On 04/06/26 05:58, Tvrtko Ursulin wrote:
> 
> On 03/06/2026 23:25, Maíra Canal wrote:
>> +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?

There is, but it'll be fixed in "[PATCH v3 14/14] drm/v3d: Ensure atomic
submissions in v3d_submit_jobs()".

> 
>> +            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.

Ack.

>> +
>> +    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.

This will all disappear in the next few patches.

Considering this, would you consider R-b-ing this patch and I'll address
the initializations in the next version?

Best regards,
- Maíra