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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



El mar, 28-11-2023 a las 07:47 -0300, Maira Canal escribió:
> 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@xxxxxxxxxx>
> > > 
> > > 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@xxxxxxxxxx>
> > > Co-developed-by: Maíra Canal <mcanal@xxxxxxxxxx>
> > > Signed-off-by: Maíra Canal <mcanal@xxxxxxxxxx>
> > > ---
> > >   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.


Ah, yes, ignore my previous comment then.

Iago

> 
> 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;
> > >   
> > 
> 





[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux