[PATCH v2 06/17] drm/v3d: Decouple job allocation from job initiation

[Maira Canal]

Hi Iago,

On 11/27/23 05:12, Iago Toral wrote:
> El jue, 23-11-2023 a las 21:47 -0300, Maíra Canal escribió:
>> We want to allow the IOCTLs to allocate the job without initiating
>> it.
>> This will be useful for the CPU job submission IOCTL, as the CPU job
>> has
>> the need to use information from the user extensions. Currently, the
>> user extensions are parsed before the job allocation, making it
>> impossible to fill the CPU job when parsing the user extensions.
>> Therefore, decouple the job allocation from the job initiation.
>>
>> Signed-off-by: Maíra Canal <mcanal at igalia.com>
>> ---
>>   drivers/gpu/drm/v3d/v3d_submit.c | 23 ++++++++++++++++++-----
>>   1 file changed, 18 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/v3d/v3d_submit.c
>> b/drivers/gpu/drm/v3d/v3d_submit.c
>> index fe46dd316ca0..ed1a310bbd2f 100644
>> --- a/drivers/gpu/drm/v3d/v3d_submit.c
>> +++ b/drivers/gpu/drm/v3d/v3d_submit.c
>> @@ -135,6 +135,21 @@ void v3d_job_put(struct v3d_job *job)
>>          kref_put(&job->refcount, job->free);
>>   }
>>   
>> +static int
>> +v3d_job_allocate(void **container, size_t size)
>> +{
>> +       if (*container)
>> +               return 0;
> 
> Mmm... is this really what we want? At least right now we expect

Yeah, it is.

> v3d_job_allocate to always allocate memory, is there any scenario in
> which we would expect to call this with an already allocated container?

Take a look here:

   19 int
   18 v3d_submit_cpu_ioctl(struct drm_device *dev, void *data,
   17                      struct drm_file *file_priv)
   16 {
   15         struct v3d_dev *v3d = to_v3d_dev(dev);
   14         struct drm_v3d_submit_cpu *args = data;
   13         struct v3d_submit_ext se = {0};
   12         struct v3d_submit_ext *out_se = NULL;
   11         struct v3d_cpu_job *cpu_job = NULL;
   10         struct v3d_csd_job *csd_job = NULL;
    9         struct v3d_job *clean_job = NULL;
    8         struct ww_acquire_ctx acquire_ctx;
    7         int ret;
    6
    5         if (args->flags && !(args->flags & 
DRM_V3D_SUBMIT_EXTENSION)) {
    4                 DRM_INFO("invalid flags: %d\n", args->flags);
    3                 return -EINVAL;
    2         }
    1
1187         ret = v3d_job_allocate((void *)&cpu_job, sizeof(*cpu_job));
    1         if (ret)
    2                 return ret;

Here we allocate the CPU job, as we need it allocated to fill its fields
with the information in the extensions.

    3
    4         if (args->flags & DRM_V3D_SUBMIT_EXTENSION) {
    5                 ret = v3d_get_extensions(file_priv, 
args->extensions, &se, cpu_job);

So, here we filling the CPU job fields with relevant information.

    6                 if (ret) {
    7                         DRM_DEBUG("Failed to get extensions.\n");
    8                         goto fail;
    9                 }
   10         }
   11
   12         /* Every CPU job must have a CPU job user extension */
   13         if (!cpu_job->job_type) {
   14                 DRM_DEBUG("CPU job must have a CPU job user 
extension.\n");
   15                 goto fail;
   16         }
   17
   18         trace_v3d_submit_cpu_ioctl(&v3d->drm, cpu_job->job_type);
   19
   20         ret = v3d_job_init(v3d, file_priv, (void *)&cpu_job, 
sizeof(*cpu_job),
   21                            v3d_job_free, 0, &se, V3D_CPU);

Here we are initiating the job (drm_sched_job_init and syncobjs stuff).
If I don't have this condition in v3d_job_allocate, I would allocate
the container twice.

   22         if (ret)
   23                 goto fail;

Best Regards,
- Maíra

> 
> Iago
> 
>> +
>> +       *container = kcalloc(1, size, GFP_KERNEL);
>> +       if (!*container) {
>> +               DRM_ERROR("Cannot allocate memory for V3D job.\n");
>> +               return -ENOMEM;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>>   static int
>>   v3d_job_init(struct v3d_dev *v3d, struct drm_file *file_priv,
>>               void **container, size_t size, void (*free)(struct kref
>> *ref),
>> @@ -145,11 +160,9 @@ v3d_job_init(struct v3d_dev *v3d, struct
>> drm_file *file_priv,
>>          bool has_multisync = se && (se->flags &
>> DRM_V3D_EXT_ID_MULTI_SYNC);
>>          int ret, i;
>>   
>> -       *container = kcalloc(1, size, GFP_KERNEL);
>> -       if (!*container) {
>> -               DRM_ERROR("Cannot allocate memory for v3d job.");
>> -               return -ENOMEM;
>> -       }
>> +       ret = v3d_job_allocate(container, size);
>> +       if (ret)
>> +               return ret;
>>   
>>          job = *container;
>>          job->v3d = v3d;
>