[PATCH v3 10/17] drm/v3d: Detach the CSD job BO setup

[Maira Canal]

Hi Iago,

On 11/28/23 05:42, Iago Toral wrote:
> El lun, 27-11-2023 a las 15:48 -0300, Maíra Canal escribió:
>> From: Melissa Wen <mwen at igalia.com>
>>
>> Detach CSD job setup from CSD submission ioctl to reuse it in CPU
>> submission ioctl for indirect CSD job.
>>
>> Signed-off-by: Melissa Wen <mwen at igalia.com>
>> Co-developed-by: Maíra Canal <mcanal at igalia.com>
>> Signed-off-by: Maíra Canal <mcanal at igalia.com>
>> ---
>>   drivers/gpu/drm/v3d/v3d_submit.c | 68 ++++++++++++++++++++----------
>> --
>>   1 file changed, 42 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/v3d/v3d_submit.c
>> b/drivers/gpu/drm/v3d/v3d_submit.c
>> index c134b113b181..eb26fe1e27e3 100644
>> --- a/drivers/gpu/drm/v3d/v3d_submit.c
>> +++ b/drivers/gpu/drm/v3d/v3d_submit.c
>> @@ -256,6 +256,45 @@ v3d_attach_fences_and_unlock_reservation(struct
>> drm_file *file_priv,
>>          }
>>   }
>>   
>> +static int
>> +v3d_setup_csd_jobs_and_bos(struct drm_file *file_priv,
>> +                          struct v3d_dev *v3d,
>> +                          struct drm_v3d_submit_csd *args,
>> +                          struct v3d_csd_job **job,
>> +                          struct v3d_job **clean_job,
>> +                          struct v3d_submit_ext *se,
>> +                          struct ww_acquire_ctx *acquire_ctx)
>> +{
>> +       int ret;
>> +
>> +       ret = v3d_job_allocate((void *)job, sizeof(**job));
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = v3d_job_init(v3d, file_priv, &(*job)->base,
>> +                          v3d_job_free, args->in_sync, se, V3D_CSD);
>> +       if (ret)
> 
> 
> We should free the job here.
> 
>> +               return ret;
>> +
>> +       ret = v3d_job_allocate((void *)clean_job,
>> sizeof(**clean_job));
>> +       if (ret)
>> +               return ret;
>> +
>> +       ret = v3d_job_init(v3d, file_priv, *clean_job,
>> +                          v3d_job_free, 0, NULL, V3D_CACHE_CLEAN);
>> +       if (ret)
> 
> We should free job and clean_job here.
> 
>> +               return ret;
>> +
>> +       (*job)->args = *args;
>> +
>> +       ret = v3d_lookup_bos(&v3d->drm, file_priv, *clean_job,
>> +                            args->bo_handles, args-
>>> bo_handle_count);
>> +       if (ret)
> 
> Same here.
> 
> I think we probably want to have a fail label where we do this and just
> jump there from all the paths I mentioned above.

Actually, we are freeing the job in `v3d_submit_csd_ioctl`. Take a look
here:

   48         ret = v3d_setup_csd_jobs_and_bos(file_priv, v3d, args,
   47                                          &job, &clean_job, &se,
   46                                          &acquire_ctx);
   45         if (ret)
   44                 goto fail;

If `v3d_setup_csd_jobs_and_bos` fails, we go to fail.

   43
   42         if (args->perfmon_id) {
   41                 job->base.perfmon = v3d_perfmon_find(v3d_priv,
   40 
args->perfmon_id);
   39                 if (!job->base.perfmon) {
   38                         ret = -ENOENT;
   37                         goto fail_perfmon;
   36                 }
   35         }
   34
   33         mutex_lock(&v3d->sched_lock);
   32         v3d_push_job(&job->base);
   31
   30         ret = drm_sched_job_add_dependency(&clean_job->base,
   29 
dma_fence_get(job->base.done_fence));
   28         if (ret)
   27                 goto fail_unreserve;
   26
   25         v3d_push_job(clean_job);
   24         mutex_unlock(&v3d->sched_lock);
   23
   22         v3d_attach_fences_and_unlock_reservation(file_priv,
   21                                                  clean_job,
   20                                                  &acquire_ctx,
   19                                                  args->out_sync,
   18                                                  &se,
   17 
clean_job->done_fence);
   16
   15         v3d_job_put(&job->base);
   14         v3d_job_put(clean_job);
   13
   12         return 0;
   11
   10 fail_unreserve:
    9         mutex_unlock(&v3d->sched_lock);
    8 fail_perfmon:
    7         drm_gem_unlock_reservations(clean_job->bo, 
clean_job->bo_count,
    6                                     &acquire_ctx);
    5 fail:
    4         v3d_job_cleanup((void *)job);
    3         v3d_job_cleanup(clean_job);

Here we cleanup `job` and `clean_job`. This will call `v3d_job_free` and
free the jobs.

Best Regards,
- Maíra

    2         v3d_put_multisync_post_deps(&se);
    1
1167         return ret;

> 
>> +               return ret;
>> +
>> +       return v3d_lock_bo_reservations(*clean_job, acquire_ctx);
>> +}
>> +
>>   static void
>>   v3d_put_multisync_post_deps(struct v3d_submit_ext *se)
>>   {
>> @@ -700,32 +739,9 @@ v3d_submit_csd_ioctl(struct drm_device *dev,
>> void *data,
>>                  }
>>          }
>>   
>> -       ret = v3d_job_allocate((void *)&job, sizeof(*job));
>> -       if (ret)
>> -               return ret;
>> -
>> -       ret = v3d_job_init(v3d, file_priv, &job->base,
>> -                          v3d_job_free, args->in_sync, &se,
>> V3D_CSD);
>> -       if (ret)
>> -               goto fail;
>> -
>> -       ret = v3d_job_allocate((void *)&clean_job,
>> sizeof(*clean_job));
>> -       if (ret)
>> -               goto fail;
>> -
>> -       ret = v3d_job_init(v3d, file_priv, clean_job,
>> -                          v3d_job_free, 0, NULL, V3D_CACHE_CLEAN);
>> -       if (ret)
>> -               goto fail;
>> -
>> -       job->args = *args;
>> -
>> -       ret = v3d_lookup_bos(dev, file_priv, clean_job,
>> -                            args->bo_handles, args-
>>> bo_handle_count);
>> -       if (ret)
>> -               goto fail;
>> -
>> -       ret = v3d_lock_bo_reservations(clean_job, &acquire_ctx);
>> +       ret = v3d_setup_csd_jobs_and_bos(file_priv, v3d, args,
>> +                                        &job, &clean_job, &se,
>> +                                        &acquire_ctx);
>>          if (ret)
>>                  goto fail;
>>   
>