| Message ID | 20231124012548.772095-7-mcanal@igalia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/v3d: Introduce CPU jobs | expand |
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@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 v3d_job_allocate to always allocate memory, is there any scenario in which we would expect to call this with an already allocated container? 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;
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@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; >
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; + + *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;
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@igalia.com> --- drivers/gpu/drm/v3d/v3d_submit.c | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-)