Am 24.06.21 um 16:00 schrieb Daniel Vetter:

This is a very confusingly named function, because not just does it init an object, it arms it and provides a point of no return for pushing a job into the scheduler. It would be nice if that's a bit clearer in the interface.

We originally had that in the push_job interface, but moved that to init for some reason I don't remember.

But the real reason is that I want to push the dependency tracking helpers into the scheduler code, and that means drm_sched_job_init must be called a lot earlier, without arming the job.

I'm really questioning myself if I like that naming.

What about using drm_sched_job_add_dependency instead?

Christian.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Lucas Stach l.stach@pengutronix.de Cc: Russell King linux+etnaviv@armlinux.org.uk Cc: Christian Gmeiner christian.gmeiner@gmail.com Cc: Qiang Yu yuq825@gmail.com Cc: Rob Herring robh@kernel.org Cc: Tomeu Vizoso tomeu.vizoso@collabora.com Cc: Steven Price steven.price@arm.com Cc: Alyssa Rosenzweig alyssa.rosenzweig@collabora.com Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: Sumit Semwal sumit.semwal@linaro.org Cc: "Christian König" christian.koenig@amd.com Cc: Masahiro Yamada masahiroy@kernel.org Cc: Kees Cook keescook@chromium.org Cc: Adam Borowski kilobyte@angband.pl Cc: Nick Terrell terrelln@fb.com Cc: Mauro Carvalho Chehab mchehab+huawei@kernel.org Cc: Paul Menzel pmenzel@molgen.mpg.de Cc: Sami Tolvanen samitolvanen@google.com Cc: Viresh Kumar viresh.kumar@linaro.org Cc: Alex Deucher alexander.deucher@amd.com Cc: Dave Airlie airlied@redhat.com Cc: Nirmoy Das nirmoy.das@amd.com Cc: Deepak R Varma mh12gx2825@gmail.com Cc: Lee Jones lee.jones@linaro.org Cc: Kevin Wang kevin1.wang@amd.com Cc: Chen Li chenli@uniontech.com Cc: Luben Tuikov luben.tuikov@amd.com Cc: "Marek Olšák" marek.olsak@amd.com Cc: Dennis Li Dennis.Li@amd.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Andrey Grodzovsky andrey.grodzovsky@amd.com Cc: Sonny Jiang sonny.jiang@amd.com Cc: Boris Brezillon boris.brezillon@collabora.com Cc: Tian Tao tiantao6@hisilicon.com Cc: Jack Zhang Jack.Zhang1@amd.com Cc: etnaviv@lists.freedesktop.org Cc: lima@lists.freedesktop.org Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org

---

.gitignore | 1 + drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 2 ++ drivers/gpu/drm/etnaviv/etnaviv_sched.c | 2 ++ drivers/gpu/drm/lima/lima_sched.c | 2 ++ drivers/gpu/drm/panfrost/panfrost_job.c | 2 ++ drivers/gpu/drm/scheduler/sched_entity.c | 6 +++--- drivers/gpu/drm/scheduler/sched_fence.c | 15 ++++++++++----- drivers/gpu/drm/scheduler/sched_main.c | 23 ++++++++++++++++++++++- include/drm/gpu_scheduler.h | 6 +++++- 10 files changed, 51 insertions(+), 10 deletions(-)

diff --git a/.gitignore b/.gitignore index 7afd412dadd2..52433a930299 100644 --- a/.gitignore +++ b/.gitignore @@ -66,6 +66,7 @@ modules.order /modules.builtin /modules.builtin.modinfo /modules.nsdeps +*.builtin

# # RPM spec file (make rpm-pkg) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index c5386d13eb4a..a4ec092af9a7 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c @@ -1226,6 +1226,8 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p, if (r) goto error_unlock;

• drm_sched_job_arm(&job->base);
• /* No memory allocation is allowed while holding the notifier lock.
  • The lock is held until amdgpu_cs_submit is finished and fence is
  • added to BOs.

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c index d33e6d97cc89..5ddb955d2315 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c @@ -170,6 +170,8 @@ int amdgpu_job_submit(struct amdgpu_job *job, struct drm_sched_entity *entity, if (r) return r;

• drm_sched_job_arm(&job->base);
• *f = dma_fence_get(&job->base.s_fence->finished); amdgpu_job_free_resources(job); drm_sched_entity_push_job(&job->base, entity);

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c b/drivers/gpu/drm/etnaviv/etnaviv_sched.c index 19826e504efc..af1671f01c7f 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c @@ -163,6 +163,8 @@ int etnaviv_sched_push_job(struct drm_sched_entity *sched_entity, if (ret) goto out_unlock;

• drm_sched_job_arm(&submit->sched_job);
• submit->out_fence = dma_fence_get(&submit->sched_job.s_fence->finished); submit->out_fence_id = idr_alloc_cyclic(&submit->gpu->fence_idr, submit->out_fence, 0,

diff --git a/drivers/gpu/drm/lima/lima_sched.c b/drivers/gpu/drm/lima/lima_sched.c index ecf3267334ff..bd1af1fd8c0f 100644 --- a/drivers/gpu/drm/lima/lima_sched.c +++ b/drivers/gpu/drm/lima/lima_sched.c @@ -129,6 +129,8 @@ int lima_sched_task_init(struct lima_sched_task *task, return err; }

• drm_sched_job_arm(&task->base);
• task->num_bos = num_bos; task->vm = lima_vm_get(vm);

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c index beb62c8fc851..1e950534b9b0 100644 --- a/drivers/gpu/drm/panfrost/panfrost_job.c +++ b/drivers/gpu/drm/panfrost/panfrost_job.c @@ -244,6 +244,8 @@ int panfrost_job_push(struct panfrost_job *job) goto unlock; }

• drm_sched_job_arm(&job->base);

• job->render_done_fence = dma_fence_get(&job->base.s_fence->finished);

  ret = panfrost_acquire_object_fences(job->bos, job->bo_count,

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index 79554aa4dbb1..f7347c284886 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -485,9 +485,9 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)

• @sched_job: job to submit
• @entity: scheduler entity

• • Note: To guarantee that the order of insertion to queue matches
• • the job's fence sequence number this function should be
• • called with drm_sched_job_init under common lock.

• • Note: To guarantee that the order of insertion to queue matches the job's
• • fence sequence number this function should be called with drm_sched_job_arm()
• • under common lock.
  • Returns 0 for success, negative error code otherwise.
  */

diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c index 69de2c76731f..0ba810c198bd 100644 --- a/drivers/gpu/drm/scheduler/sched_fence.c +++ b/drivers/gpu/drm/scheduler/sched_fence.c @@ -152,11 +152,10 @@ struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f) } EXPORT_SYMBOL(to_drm_sched_fence);

-struct drm_sched_fence *drm_sched_fence_create(struct drm_sched_entity *entity,

• 			       void *owner)
  

+struct drm_sched_fence *drm_sched_fence_alloc(struct drm_sched_entity *entity,

• 			      void *owner)
  

  { struct drm_sched_fence *fence = NULL;

• unsigned seq;

  fence = kmem_cache_zalloc(sched_fence_slab, GFP_KERNEL); if (fence == NULL)

@@ -166,13 +165,19 @@ struct drm_sched_fence *drm_sched_fence_create(struct drm_sched_entity *entity, fence->sched = entity->rq->sched; spin_lock_init(&fence->lock);

• return fence;

+}

+void drm_sched_fence_init(struct drm_sched_fence *fence,

• 	  struct drm_sched_entity *entity)
  

+{

• unsigned seq;
• seq = atomic_inc_return(&entity->fence_seq); dma_fence_init(&fence->scheduled, &drm_sched_fence_ops_scheduled, &fence->lock, entity->fence_context, seq); dma_fence_init(&fence->finished, &drm_sched_fence_ops_finished, &fence->lock, entity->fence_context + 1, seq);

• return fence; }

  module_init(drm_sched_fence_slab_init);

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index 61420a9c1021..70eefed17e06 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -48,9 +48,11 @@ #include <linux/wait.h> #include <linux/sched.h> #include <linux/completion.h> +#include <linux/dma-resv.h> #include <uapi/linux/sched/types.h>

#include <drm/drm_print.h> +#include <drm/drm_gem.h> #include <drm/gpu_scheduler.h> #include <drm/spsc_queue.h>

@@ -594,7 +596,7 @@ int drm_sched_job_init(struct drm_sched_job *job, job->sched = sched; job->entity = entity; job->s_priority = entity->rq - sched->sched_rq;

• job->s_fence = drm_sched_fence_create(entity, owner);

• job->s_fence = drm_sched_fence_alloc(entity, owner); if (!job->s_fence) return -ENOMEM; job->id = atomic64_inc_return(&sched->job_id_count);

@@ -605,6 +607,25 @@ int drm_sched_job_init(struct drm_sched_job *job, } EXPORT_SYMBOL(drm_sched_job_init);

+/**

• • drm_sched_job_arm - arm a scheduler job for execution
• • @job: scheduler job to arm
• • This arms a scheduler job for execution. Specifically it initializes the
• • &drm_sched_job.s_fence of @job, so that it can be attached to struct dma_resv
• • or other places that need to track the completion of this job.
• • Refer to drm_sched_entity_push_job() documentation for locking
• • considerations.
• • This can only be called if drm_sched_job_init() succeeded.
• */

+void drm_sched_job_arm(struct drm_sched_job *job) +{

• drm_sched_fence_init(job->s_fence, job->entity);

+} +EXPORT_SYMBOL(drm_sched_job_arm);

• /**
  • drm_sched_job_cleanup - clean up scheduler job resources

diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index d18af49fd009..80438d126c9d 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -313,6 +313,7 @@ void drm_sched_fini(struct drm_gpu_scheduler *sched); int drm_sched_job_init(struct drm_sched_job *job, struct drm_sched_entity *entity, void *owner); +void drm_sched_job_arm(struct drm_sched_job *job); void drm_sched_entity_modify_sched(struct drm_sched_entity *entity, struct drm_gpu_scheduler **sched_list, unsigned int num_sched_list); @@ -352,8 +353,11 @@ void drm_sched_entity_set_priority(struct drm_sched_entity *entity, enum drm_sched_priority priority); bool drm_sched_entity_is_ready(struct drm_sched_entity *entity);

-struct drm_sched_fence *drm_sched_fence_create( +struct drm_sched_fence *drm_sched_fence_alloc( struct drm_sched_entity *s_entity, void *owner); +void drm_sched_fence_init(struct drm_sched_fence *fence,

• 	  struct drm_sched_entity *entity);
  

• void drm_sched_fence_scheduled(struct drm_sched_fence *fence); void drm_sched_fence_finished(struct drm_sched_fence *fence);

Am 24.06.21 um 19:37 schrieb Daniel Vetter:

On Thu, Jun 24, 2021 at 7:30 PM Christian König christian.koenig@amd.com wrote:

...

Am 24.06.21 um 16:00 schrieb Daniel Vetter:

...

This is a very confusingly named function, because not just does it init an object, it arms it and provides a point of no return for pushing a job into the scheduler. It would be nice if that's a bit clearer in the interface.

We originally had that in the push_job interface, but moved that to init for some reason I don't remember.

...

But the real reason is that I want to push the dependency tracking helpers into the scheduler code, and that means drm_sched_job_init must be called a lot earlier, without arming the job.

I'm really questioning myself if I like that naming.

What about using drm_sched_job_add_dependency instead?

You're suggesting a s/drm_sched_job_init/drm_sched_job_add_dependency/, or just replied to the wrong patch?

Replied to the wrong patch accidentally. I was talking about the "await" terminology.

Christian.

-Daniel

...

Christian.

...

Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Lucas Stach l.stach@pengutronix.de Cc: Russell King linux+etnaviv@armlinux.org.uk Cc: Christian Gmeiner christian.gmeiner@gmail.com Cc: Qiang Yu yuq825@gmail.com Cc: Rob Herring robh@kernel.org Cc: Tomeu Vizoso tomeu.vizoso@collabora.com Cc: Steven Price steven.price@arm.com Cc: Alyssa Rosenzweig alyssa.rosenzweig@collabora.com Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: Sumit Semwal sumit.semwal@linaro.org Cc: "Christian König" christian.koenig@amd.com Cc: Masahiro Yamada masahiroy@kernel.org Cc: Kees Cook keescook@chromium.org Cc: Adam Borowski kilobyte@angband.pl Cc: Nick Terrell terrelln@fb.com Cc: Mauro Carvalho Chehab mchehab+huawei@kernel.org Cc: Paul Menzel pmenzel@molgen.mpg.de Cc: Sami Tolvanen samitolvanen@google.com Cc: Viresh Kumar viresh.kumar@linaro.org Cc: Alex Deucher alexander.deucher@amd.com Cc: Dave Airlie airlied@redhat.com Cc: Nirmoy Das nirmoy.das@amd.com Cc: Deepak R Varma mh12gx2825@gmail.com Cc: Lee Jones lee.jones@linaro.org Cc: Kevin Wang kevin1.wang@amd.com Cc: Chen Li chenli@uniontech.com Cc: Luben Tuikov luben.tuikov@amd.com Cc: "Marek Olšák" marek.olsak@amd.com Cc: Dennis Li Dennis.Li@amd.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Andrey Grodzovsky andrey.grodzovsky@amd.com Cc: Sonny Jiang sonny.jiang@amd.com Cc: Boris Brezillon boris.brezillon@collabora.com Cc: Tian Tao tiantao6@hisilicon.com Cc: Jack Zhang Jack.Zhang1@amd.com Cc: etnaviv@lists.freedesktop.org Cc: lima@lists.freedesktop.org Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org

---

.gitignore | 1 + drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 2 ++ drivers/gpu/drm/etnaviv/etnaviv_sched.c | 2 ++ drivers/gpu/drm/lima/lima_sched.c | 2 ++ drivers/gpu/drm/panfrost/panfrost_job.c | 2 ++ drivers/gpu/drm/scheduler/sched_entity.c | 6 +++--- drivers/gpu/drm/scheduler/sched_fence.c | 15 ++++++++++----- drivers/gpu/drm/scheduler/sched_main.c | 23 ++++++++++++++++++++++- include/drm/gpu_scheduler.h | 6 +++++- 10 files changed, 51 insertions(+), 10 deletions(-)

diff --git a/.gitignore b/.gitignore index 7afd412dadd2..52433a930299 100644 --- a/.gitignore +++ b/.gitignore @@ -66,6 +66,7 @@ modules.order /modules.builtin /modules.builtin.modinfo /modules.nsdeps +*.builtin

# # RPM spec file (make rpm-pkg) diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index c5386d13eb4a..a4ec092af9a7 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c @@ -1226,6 +1226,8 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p, if (r) goto error_unlock;

• drm_sched_job_arm(&job->base);
  

•  /* No memory allocation is allowed while holding the notifier lock.
    * The lock is held until amdgpu_cs_submit is finished and fence is
    * added to BOs.
  

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c index d33e6d97cc89..5ddb955d2315 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c @@ -170,6 +170,8 @@ int amdgpu_job_submit(struct amdgpu_job *job, struct drm_sched_entity *entity, if (r) return r;

• drm_sched_job_arm(&job->base);
  

•  *f = dma_fence_get(&job->base.s_fence->finished);
   amdgpu_job_free_resources(job);
   drm_sched_entity_push_job(&job->base, entity);
  

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c b/drivers/gpu/drm/etnaviv/etnaviv_sched.c index 19826e504efc..af1671f01c7f 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c @@ -163,6 +163,8 @@ int etnaviv_sched_push_job(struct drm_sched_entity *sched_entity, if (ret) goto out_unlock;

• drm_sched_job_arm(&submit->sched_job);
  

•  submit->out_fence = dma_fence_get(&submit->sched_job.s_fence->finished);
   submit->out_fence_id = idr_alloc_cyclic(&submit->gpu->fence_idr,
                                           submit->out_fence, 0,
  

diff --git a/drivers/gpu/drm/lima/lima_sched.c b/drivers/gpu/drm/lima/lima_sched.c index ecf3267334ff..bd1af1fd8c0f 100644 --- a/drivers/gpu/drm/lima/lima_sched.c +++ b/drivers/gpu/drm/lima/lima_sched.c @@ -129,6 +129,8 @@ int lima_sched_task_init(struct lima_sched_task *task, return err; }

• drm_sched_job_arm(&task->base);
  

•  task->num_bos = num_bos;
   task->vm = lima_vm_get(vm);
  
  

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c index beb62c8fc851..1e950534b9b0 100644 --- a/drivers/gpu/drm/panfrost/panfrost_job.c +++ b/drivers/gpu/drm/panfrost/panfrost_job.c @@ -244,6 +244,8 @@ int panfrost_job_push(struct panfrost_job *job) goto unlock; }

• drm_sched_job_arm(&job->base);
  

•  job->render_done_fence = dma_fence_get(&job->base.s_fence->finished);
  
   ret = panfrost_acquire_object_fences(job->bos, job->bo_count,
  

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index 79554aa4dbb1..f7347c284886 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -485,9 +485,9 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity) * @sched_job: job to submit * @entity: scheduler entity *

• • Note: To guarantee that the order of insertion to queue matches
• • the job's fence sequence number this function should be
• • called with drm_sched_job_init under common lock.

• • Note: To guarantee that the order of insertion to queue matches the job's
• • fence sequence number this function should be called with drm_sched_job_arm()
• • under common lock.
    • Returns 0 for success, negative error code otherwise.
    */

diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c index 69de2c76731f..0ba810c198bd 100644 --- a/drivers/gpu/drm/scheduler/sched_fence.c +++ b/drivers/gpu/drm/scheduler/sched_fence.c @@ -152,11 +152,10 @@ struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f) } EXPORT_SYMBOL(to_drm_sched_fence);

-struct drm_sched_fence *drm_sched_fence_create(struct drm_sched_entity *entity,

•                                        void *owner)
  

+struct drm_sched_fence *drm_sched_fence_alloc(struct drm_sched_entity *entity,

•                                       void *owner)
  

  { struct drm_sched_fence *fence = NULL;

• unsigned seq;
  
   fence = kmem_cache_zalloc(sched_fence_slab, GFP_KERNEL);
   if (fence == NULL)
  

@@ -166,13 +165,19 @@ struct drm_sched_fence *drm_sched_fence_create(struct drm_sched_entity *entity, fence->sched = entity->rq->sched; spin_lock_init(&fence->lock);

• return fence;
  

+}

+void drm_sched_fence_init(struct drm_sched_fence *fence,

•                   struct drm_sched_entity *entity)
  

+{

• unsigned seq;
  

•  seq = atomic_inc_return(&entity->fence_seq);
   dma_fence_init(&fence->scheduled, &drm_sched_fence_ops_scheduled,
                  &fence->lock, entity->fence_context, seq);
   dma_fence_init(&fence->finished, &drm_sched_fence_ops_finished,
                  &fence->lock, entity->fence_context + 1, seq);
  

• return fence;
  

  }

  module_init(drm_sched_fence_slab_init);

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index 61420a9c1021..70eefed17e06 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -48,9 +48,11 @@ #include <linux/wait.h> #include <linux/sched.h> #include <linux/completion.h> +#include <linux/dma-resv.h> #include <uapi/linux/sched/types.h>

#include <drm/drm_print.h> +#include <drm/drm_gem.h> #include <drm/gpu_scheduler.h> #include <drm/spsc_queue.h>

@@ -594,7 +596,7 @@ int drm_sched_job_init(struct drm_sched_job *job, job->sched = sched; job->entity = entity; job->s_priority = entity->rq - sched->sched_rq;

• job->s_fence = drm_sched_fence_create(entity, owner);
  

• job->s_fence = drm_sched_fence_alloc(entity, owner);
   if (!job->s_fence)
           return -ENOMEM;
   job->id = atomic64_inc_return(&sched->job_id_count);
  

@@ -605,6 +607,25 @@ int drm_sched_job_init(struct drm_sched_job *job, } EXPORT_SYMBOL(drm_sched_job_init);

+/**

• • drm_sched_job_arm - arm a scheduler job for execution
• • @job: scheduler job to arm
• • This arms a scheduler job for execution. Specifically it initializes the
• • &drm_sched_job.s_fence of @job, so that it can be attached to struct dma_resv
• • or other places that need to track the completion of this job.
• • Refer to drm_sched_entity_push_job() documentation for locking
• • considerations.
• • This can only be called if drm_sched_job_init() succeeded.
• */

+void drm_sched_job_arm(struct drm_sched_job *job) +{

• drm_sched_fence_init(job->s_fence, job->entity);
  

+} +EXPORT_SYMBOL(drm_sched_job_arm);

• /**
  • drm_sched_job_cleanup - clean up scheduler job resources

diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index d18af49fd009..80438d126c9d 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -313,6 +313,7 @@ void drm_sched_fini(struct drm_gpu_scheduler *sched); int drm_sched_job_init(struct drm_sched_job *job, struct drm_sched_entity *entity, void *owner); +void drm_sched_job_arm(struct drm_sched_job *job); void drm_sched_entity_modify_sched(struct drm_sched_entity *entity, struct drm_gpu_scheduler **sched_list, unsigned int num_sched_list); @@ -352,8 +353,11 @@ void drm_sched_entity_set_priority(struct drm_sched_entity *entity, enum drm_sched_priority priority); bool drm_sched_entity_is_ready(struct drm_sched_entity *entity);

-struct drm_sched_fence *drm_sched_fence_create( +struct drm_sched_fence *drm_sched_fence_alloc( struct drm_sched_entity *s_entity, void *owner); +void drm_sched_fence_init(struct drm_sched_fence *fence,

•                   struct drm_sched_entity *entity);
  

• void drm_sched_fence_scheduled(struct drm_sched_fence *fence); void drm_sched_fence_finished(struct drm_sched_fence *fence);

This is a very confusingly named function, because not just does it init an object, it arms it and provides a point of no return for pushing a job into the scheduler. It would be nice if that's a bit clearer in the interface.

But the real reason is that I want to push the dependency tracking helpers into the scheduler code, and that means drm_sched_job_init must be called a lot earlier, without arming the job.

v2: - don't change .gitignore (Steven) - don't forget v3d (Emma)

Acked-by: Steven Price steven.price@arm.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Lucas Stach l.stach@pengutronix.de Cc: Russell King linux+etnaviv@armlinux.org.uk Cc: Christian Gmeiner christian.gmeiner@gmail.com Cc: Qiang Yu yuq825@gmail.com Cc: Rob Herring robh@kernel.org Cc: Tomeu Vizoso tomeu.vizoso@collabora.com Cc: Steven Price steven.price@arm.com Cc: Alyssa Rosenzweig alyssa.rosenzweig@collabora.com Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: Sumit Semwal sumit.semwal@linaro.org Cc: "Christian König" christian.koenig@amd.com Cc: Masahiro Yamada masahiroy@kernel.org Cc: Kees Cook keescook@chromium.org Cc: Adam Borowski kilobyte@angband.pl Cc: Nick Terrell terrelln@fb.com Cc: Mauro Carvalho Chehab mchehab+huawei@kernel.org Cc: Paul Menzel pmenzel@molgen.mpg.de Cc: Sami Tolvanen samitolvanen@google.com Cc: Viresh Kumar viresh.kumar@linaro.org Cc: Alex Deucher alexander.deucher@amd.com Cc: Dave Airlie airlied@redhat.com Cc: Nirmoy Das nirmoy.das@amd.com Cc: Deepak R Varma mh12gx2825@gmail.com Cc: Lee Jones lee.jones@linaro.org Cc: Kevin Wang kevin1.wang@amd.com Cc: Chen Li chenli@uniontech.com Cc: Luben Tuikov luben.tuikov@amd.com Cc: "Marek Olšák" marek.olsak@amd.com Cc: Dennis Li Dennis.Li@amd.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Andrey Grodzovsky andrey.grodzovsky@amd.com Cc: Sonny Jiang sonny.jiang@amd.com Cc: Boris Brezillon boris.brezillon@collabora.com Cc: Tian Tao tiantao6@hisilicon.com Cc: Jack Zhang Jack.Zhang1@amd.com Cc: etnaviv@lists.freedesktop.org Cc: lima@lists.freedesktop.org Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org Cc: Emma Anholt emma@anholt.net --- drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 2 ++ drivers/gpu/drm/etnaviv/etnaviv_sched.c | 2 ++ drivers/gpu/drm/lima/lima_sched.c | 2 ++ drivers/gpu/drm/panfrost/panfrost_job.c | 2 ++ drivers/gpu/drm/scheduler/sched_entity.c | 6 +++--- drivers/gpu/drm/scheduler/sched_fence.c | 15 ++++++++++----- drivers/gpu/drm/scheduler/sched_main.c | 23 ++++++++++++++++++++++- drivers/gpu/drm/v3d/v3d_gem.c | 2 ++ include/drm/gpu_scheduler.h | 6 +++++- 10 files changed, 52 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index c5386d13eb4a..a4ec092af9a7 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c @@ -1226,6 +1226,8 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p, if (r) goto error_unlock;

+ drm_sched_job_arm(&job->base); + /* No memory allocation is allowed while holding the notifier lock. * The lock is held until amdgpu_cs_submit is finished and fence is * added to BOs. diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c index d33e6d97cc89..5ddb955d2315 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c @@ -170,6 +170,8 @@ int amdgpu_job_submit(struct amdgpu_job *job, struct drm_sched_entity *entity, if (r) return r;

+ drm_sched_job_arm(&job->base); + *f = dma_fence_get(&job->base.s_fence->finished); amdgpu_job_free_resources(job); drm_sched_entity_push_job(&job->base, entity); diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c b/drivers/gpu/drm/etnaviv/etnaviv_sched.c index 19826e504efc..af1671f01c7f 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c @@ -163,6 +163,8 @@ int etnaviv_sched_push_job(struct drm_sched_entity *sched_entity, if (ret) goto out_unlock;

+ drm_sched_job_arm(&submit->sched_job); + submit->out_fence = dma_fence_get(&submit->sched_job.s_fence->finished); submit->out_fence_id = idr_alloc_cyclic(&submit->gpu->fence_idr, submit->out_fence, 0, diff --git a/drivers/gpu/drm/lima/lima_sched.c b/drivers/gpu/drm/lima/lima_sched.c index ecf3267334ff..bd1af1fd8c0f 100644 --- a/drivers/gpu/drm/lima/lima_sched.c +++ b/drivers/gpu/drm/lima/lima_sched.c @@ -129,6 +129,8 @@ int lima_sched_task_init(struct lima_sched_task *task, return err; }

+ drm_sched_job_arm(&task->base); + task->num_bos = num_bos; task->vm = lima_vm_get(vm);

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c index beb62c8fc851..1e950534b9b0 100644 --- a/drivers/gpu/drm/panfrost/panfrost_job.c +++ b/drivers/gpu/drm/panfrost/panfrost_job.c @@ -244,6 +244,8 @@ int panfrost_job_push(struct panfrost_job *job) goto unlock; }

+ drm_sched_job_arm(&job->base); + job->render_done_fence = dma_fence_get(&job->base.s_fence->finished);

ret = panfrost_acquire_object_fences(job->bos, job->bo_count, diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index 79554aa4dbb1..f7347c284886 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -485,9 +485,9 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity) * @sched_job: job to submit * @entity: scheduler entity * - * Note: To guarantee that the order of insertion to queue matches - * the job's fence sequence number this function should be - * called with drm_sched_job_init under common lock. + * Note: To guarantee that the order of insertion to queue matches the job's + * fence sequence number this function should be called with drm_sched_job_arm() + * under common lock. * * Returns 0 for success, negative error code otherwise. */ diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c index 69de2c76731f..0ba810c198bd 100644 --- a/drivers/gpu/drm/scheduler/sched_fence.c +++ b/drivers/gpu/drm/scheduler/sched_fence.c @@ -152,11 +152,10 @@ struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f) } EXPORT_SYMBOL(to_drm_sched_fence);

-struct drm_sched_fence *drm_sched_fence_create(struct drm_sched_entity *entity, - void *owner) +struct drm_sched_fence *drm_sched_fence_alloc(struct drm_sched_entity *entity, + void *owner) { struct drm_sched_fence *fence = NULL; - unsigned seq;

fence = kmem_cache_zalloc(sched_fence_slab, GFP_KERNEL); if (fence == NULL) @@ -166,13 +165,19 @@ struct drm_sched_fence *drm_sched_fence_create(struct drm_sched_entity *entity, fence->sched = entity->rq->sched; spin_lock_init(&fence->lock);

+ return fence; +} + +void drm_sched_fence_init(struct drm_sched_fence *fence, + struct drm_sched_entity *entity) +{ + unsigned seq; + seq = atomic_inc_return(&entity->fence_seq); dma_fence_init(&fence->scheduled, &drm_sched_fence_ops_scheduled, &fence->lock, entity->fence_context, seq); dma_fence_init(&fence->finished, &drm_sched_fence_ops_finished, &fence->lock, entity->fence_context + 1, seq); - - return fence; }

module_init(drm_sched_fence_slab_init); diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index 61420a9c1021..70eefed17e06 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -48,9 +48,11 @@ #include <linux/wait.h> #include <linux/sched.h> #include <linux/completion.h> +#include <linux/dma-resv.h> #include <uapi/linux/sched/types.h>

#include <drm/drm_print.h> +#include <drm/drm_gem.h> #include <drm/gpu_scheduler.h> #include <drm/spsc_queue.h>

@@ -594,7 +596,7 @@ int drm_sched_job_init(struct drm_sched_job *job, job->sched = sched; job->entity = entity; job->s_priority = entity->rq - sched->sched_rq; - job->s_fence = drm_sched_fence_create(entity, owner); + job->s_fence = drm_sched_fence_alloc(entity, owner); if (!job->s_fence) return -ENOMEM; job->id = atomic64_inc_return(&sched->job_id_count); @@ -605,6 +607,25 @@ int drm_sched_job_init(struct drm_sched_job *job, } EXPORT_SYMBOL(drm_sched_job_init);

+/** + * drm_sched_job_arm - arm a scheduler job for execution + * @job: scheduler job to arm + * + * This arms a scheduler job for execution. Specifically it initializes the + * &drm_sched_job.s_fence of @job, so that it can be attached to struct dma_resv + * or other places that need to track the completion of this job. + * + * Refer to drm_sched_entity_push_job() documentation for locking + * considerations. + * + * This can only be called if drm_sched_job_init() succeeded. + */ +void drm_sched_job_arm(struct drm_sched_job *job) +{ + drm_sched_fence_init(job->s_fence, job->entity); +} +EXPORT_SYMBOL(drm_sched_job_arm); + /** * drm_sched_job_cleanup - clean up scheduler job resources * diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c index 4eb354226972..5c3a99027ecd 100644 --- a/drivers/gpu/drm/v3d/v3d_gem.c +++ b/drivers/gpu/drm/v3d/v3d_gem.c @@ -475,6 +475,8 @@ v3d_push_job(struct v3d_file_priv *v3d_priv, if (ret) return ret;

+ drm_sched_job_arm(&job->base); + job->done_fence = dma_fence_get(&job->base.s_fence->finished);

/* put by scheduler job completion */ diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index d18af49fd009..80438d126c9d 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -313,6 +313,7 @@ void drm_sched_fini(struct drm_gpu_scheduler *sched); int drm_sched_job_init(struct drm_sched_job *job, struct drm_sched_entity *entity, void *owner); +void drm_sched_job_arm(struct drm_sched_job *job); void drm_sched_entity_modify_sched(struct drm_sched_entity *entity, struct drm_gpu_scheduler **sched_list, unsigned int num_sched_list); @@ -352,8 +353,11 @@ void drm_sched_entity_set_priority(struct drm_sched_entity *entity, enum drm_sched_priority priority); bool drm_sched_entity_is_ready(struct drm_sched_entity *entity);

-struct drm_sched_fence *drm_sched_fence_create( +struct drm_sched_fence *drm_sched_fence_alloc( struct drm_sched_entity *s_entity, void *owner); +void drm_sched_fence_init(struct drm_sched_fence *fence, + struct drm_sched_entity *entity); + void drm_sched_fence_scheduled(struct drm_sched_fence *fence); void drm_sched_fence_finished(struct drm_sched_fence *fence);

On Thu, Jun 24, 2021 at 11:00 PM Emma Anholt emma@anholt.net wrote:

On Thu, Jun 24, 2021 at 1:45 PM Daniel Vetter daniel.vetter@ffwll.ch wrote:

...

This is a very confusingly named function, because not just does it init an object, it arms it and provides a point of no return for pushing a job into the scheduler. It would be nice if that's a bit clearer in the interface.

But the real reason is that I want to push the dependency tracking helpers into the scheduler code, and that means drm_sched_job_init must be called a lot earlier, without arming the job.

v2:

• don't change .gitignore (Steven)
• don't forget v3d (Emma)

Acked-by: Steven Price steven.price@arm.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Lucas Stach l.stach@pengutronix.de Cc: Russell King linux+etnaviv@armlinux.org.uk Cc: Christian Gmeiner christian.gmeiner@gmail.com Cc: Qiang Yu yuq825@gmail.com Cc: Rob Herring robh@kernel.org Cc: Tomeu Vizoso tomeu.vizoso@collabora.com Cc: Steven Price steven.price@arm.com Cc: Alyssa Rosenzweig alyssa.rosenzweig@collabora.com Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: Sumit Semwal sumit.semwal@linaro.org Cc: "Christian König" christian.koenig@amd.com Cc: Masahiro Yamada masahiroy@kernel.org Cc: Kees Cook keescook@chromium.org Cc: Adam Borowski kilobyte@angband.pl Cc: Nick Terrell terrelln@fb.com Cc: Mauro Carvalho Chehab mchehab+huawei@kernel.org Cc: Paul Menzel pmenzel@molgen.mpg.de Cc: Sami Tolvanen samitolvanen@google.com Cc: Viresh Kumar viresh.kumar@linaro.org Cc: Alex Deucher alexander.deucher@amd.com Cc: Dave Airlie airlied@redhat.com Cc: Nirmoy Das nirmoy.das@amd.com Cc: Deepak R Varma mh12gx2825@gmail.com Cc: Lee Jones lee.jones@linaro.org Cc: Kevin Wang kevin1.wang@amd.com Cc: Chen Li chenli@uniontech.com Cc: Luben Tuikov luben.tuikov@amd.com Cc: "Marek Olšák" marek.olsak@amd.com Cc: Dennis Li Dennis.Li@amd.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Andrey Grodzovsky andrey.grodzovsky@amd.com Cc: Sonny Jiang sonny.jiang@amd.com Cc: Boris Brezillon boris.brezillon@collabora.com Cc: Tian Tao tiantao6@hisilicon.com Cc: Jack Zhang Jack.Zhang1@amd.com Cc: etnaviv@lists.freedesktop.org Cc: lima@lists.freedesktop.org Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org Cc: Emma Anholt emma@anholt.net

---

drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 2 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_job.c | 2 ++ drivers/gpu/drm/etnaviv/etnaviv_sched.c | 2 ++ drivers/gpu/drm/lima/lima_sched.c | 2 ++ drivers/gpu/drm/panfrost/panfrost_job.c | 2 ++ drivers/gpu/drm/scheduler/sched_entity.c | 6 +++--- drivers/gpu/drm/scheduler/sched_fence.c | 15 ++++++++++----- drivers/gpu/drm/scheduler/sched_main.c | 23 ++++++++++++++++++++++- drivers/gpu/drm/v3d/v3d_gem.c | 2 ++ include/drm/gpu_scheduler.h | 6 +++++- 10 files changed, 52 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index c5386d13eb4a..a4ec092af9a7 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c @@ -1226,6 +1226,8 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p, if (r) goto error_unlock;

•   drm_sched_job_arm(&job->base);
  

•   /* No memory allocation is allowed while holding the notifier lock.
     * The lock is held until amdgpu_cs_submit is finished and fence is
     * added to BOs.
  

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c index d33e6d97cc89..5ddb955d2315 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c @@ -170,6 +170,8 @@ int amdgpu_job_submit(struct amdgpu_job *job, struct drm_sched_entity *entity, if (r) return r;

•   drm_sched_job_arm(&job->base);
  

•   *f = dma_fence_get(&job->base.s_fence->finished);
    amdgpu_job_free_resources(job);
    drm_sched_entity_push_job(&job->base, entity);
  

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c b/drivers/gpu/drm/etnaviv/etnaviv_sched.c index 19826e504efc..af1671f01c7f 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c @@ -163,6 +163,8 @@ int etnaviv_sched_push_job(struct drm_sched_entity *sched_entity, if (ret) goto out_unlock;

•   drm_sched_job_arm(&submit->sched_job);
  

•   submit->out_fence = dma_fence_get(&submit->sched_job.s_fence->finished);
    submit->out_fence_id = idr_alloc_cyclic(&submit->gpu->fence_idr,
                                            submit->out_fence, 0,
  

diff --git a/drivers/gpu/drm/lima/lima_sched.c b/drivers/gpu/drm/lima/lima_sched.c index ecf3267334ff..bd1af1fd8c0f 100644 --- a/drivers/gpu/drm/lima/lima_sched.c +++ b/drivers/gpu/drm/lima/lima_sched.c @@ -129,6 +129,8 @@ int lima_sched_task_init(struct lima_sched_task *task, return err; }

•   drm_sched_job_arm(&task->base);
  

•   task->num_bos = num_bos;
    task->vm = lima_vm_get(vm);
  
  

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c index beb62c8fc851..1e950534b9b0 100644 --- a/drivers/gpu/drm/panfrost/panfrost_job.c +++ b/drivers/gpu/drm/panfrost/panfrost_job.c @@ -244,6 +244,8 @@ int panfrost_job_push(struct panfrost_job *job) goto unlock; }

•   drm_sched_job_arm(&job->base);
  

•   job->render_done_fence = dma_fence_get(&job->base.s_fence->finished);
  
    ret = panfrost_acquire_object_fences(job->bos, job->bo_count,
  

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index 79554aa4dbb1..f7347c284886 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -485,9 +485,9 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)

• @sched_job: job to submit
• @entity: scheduler entity

• • Note: To guarantee that the order of insertion to queue matches
• • the job's fence sequence number this function should be
• • called with drm_sched_job_init under common lock.

• • Note: To guarantee that the order of insertion to queue matches the job's
• • fence sequence number this function should be called with drm_sched_job_arm()
• • under common lock.
  • Returns 0 for success, negative error code otherwise.
  */

diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c index 69de2c76731f..0ba810c198bd 100644 --- a/drivers/gpu/drm/scheduler/sched_fence.c +++ b/drivers/gpu/drm/scheduler/sched_fence.c @@ -152,11 +152,10 @@ struct drm_sched_fence *to_drm_sched_fence(struct dma_fence *f) } EXPORT_SYMBOL(to_drm_sched_fence);

-struct drm_sched_fence *drm_sched_fence_create(struct drm_sched_entity *entity,

•                                          void *owner)
  

+struct drm_sched_fence *drm_sched_fence_alloc(struct drm_sched_entity *entity,

•                                         void *owner)
  

{ struct drm_sched_fence *fence = NULL;

•   unsigned seq;
  
    fence = kmem_cache_zalloc(sched_fence_slab, GFP_KERNEL);
    if (fence == NULL)
  

@@ -166,13 +165,19 @@ struct drm_sched_fence *drm_sched_fence_create(struct drm_sched_entity *entity, fence->sched = entity->rq->sched; spin_lock_init(&fence->lock);

•   return fence;
  

+}

If there's an error path between fence_alloc (job_init()) and fence_create() (job_arm()) time, how does the s_fence get freed? Before, I was committed to calling drm_sched_entity_push_job() which lead to the job being processed and freed, but now I think we need some other non-pushed-job free path.

Yeah I need to fix that in each driver when I move the drm_sched_job_init around. From a quick look I just need to move the drm_sched_job_cleanup() call around slightly in each case, but it's a bit too late to do that without screwing it up for sure. I'll look into that tomorrow. -Daniel

...

+EXPORT_SYMBOL(drm_sched_job_arm);

/**

• drm_sched_job_cleanup - clean up scheduler job resources

diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c index 4eb354226972..5c3a99027ecd 100644 --- a/drivers/gpu/drm/v3d/v3d_gem.c +++ b/drivers/gpu/drm/v3d/v3d_gem.c @@ -475,6 +475,8 @@ v3d_push_job(struct v3d_file_priv *v3d_priv, if (ret) return ret;

•   drm_sched_job_arm(&job->base);
  

•   job->done_fence = dma_fence_get(&job->base.s_fence->finished);
  
    /* put by scheduler job completion */
  

diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index d18af49fd009..80438d126c9d 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -313,6 +313,7 @@ void drm_sched_fini(struct drm_gpu_scheduler *sched); int drm_sched_job_init(struct drm_sched_job *job, struct drm_sched_entity *entity, void *owner); +void drm_sched_job_arm(struct drm_sched_job *job); void drm_sched_entity_modify_sched(struct drm_sched_entity *entity, struct drm_gpu_scheduler **sched_list, unsigned int num_sched_list); @@ -352,8 +353,11 @@ void drm_sched_entity_set_priority(struct drm_sched_entity *entity, enum drm_sched_priority priority); bool drm_sched_entity_is_ready(struct drm_sched_entity *entity);

-struct drm_sched_fence *drm_sched_fence_create( +struct drm_sched_fence *drm_sched_fence_alloc( struct drm_sched_entity *s_entity, void *owner); +void drm_sched_fence_init(struct drm_sched_fence *fence,

•                     struct drm_sched_entity *entity);
  

void drm_sched_fence_scheduled(struct drm_sched_fence *fence); void drm_sched_fence_finished(struct drm_sched_fence *fence);

-- 2.32.0.rc2

On 24/06/2021 15:00, Daniel Vetter wrote:

Instead of just a callback we can just glue in the gem helpers that panfrost, v3d and lima currently use. There's really not that many ways to skin this cat.

On the naming bikeshed: The idea for using _await_ to denote adding dependencies to a job comes from i915, where that's used quite extensively all over the place, in lots of datastructures.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: Sumit Semwal sumit.semwal@linaro.org Cc: "Christian König" christian.koenig@amd.com Cc: Andrey Grodzovsky andrey.grodzovsky@amd.com Cc: Lee Jones lee.jones@linaro.org Cc: Nirmoy Das nirmoy.aiemd@gmail.com Cc: Boris Brezillon boris.brezillon@collabora.com Cc: Luben Tuikov luben.tuikov@amd.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Jack Zhang Jack.Zhang1@amd.com Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org

LGTM:

Reviewed-by: Steven Price steven.price@arm.com

---

drivers/gpu/drm/scheduler/sched_entity.c | 18 +++- drivers/gpu/drm/scheduler/sched_main.c | 103 +++++++++++++++++++++++ include/drm/gpu_scheduler.h | 31 ++++++- 3 files changed, 146 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index f7347c284886..b6f72fafd504 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -211,6 +211,19 @@ static void drm_sched_entity_kill_jobs_cb(struct dma_fence *f, job->sched->ops->free_job(job); }

+static struct dma_fence * +drm_sched_job_dependency(struct drm_sched_job *job,

• 	 struct drm_sched_entity *entity)
  

+{

• if (!xa_empty(&job->dependencies))

• return xa_erase(&job->dependencies, job->last_dependency++);
  

• if (job->sched->ops->dependency)

• return job->sched->ops->dependency(job, entity);
  

• return NULL;

+}

/**

• drm_sched_entity_kill_jobs - Make sure all remaining jobs are killed

@@ -229,7 +242,7 @@ static void drm_sched_entity_kill_jobs(struct drm_sched_entity *entity) struct drm_sched_fence *s_fence = job->s_fence;

/* Wait for all dependencies to avoid data corruptions */

• while ((f = job->sched->ops->dependency(job, entity)))
  

• while ((f = drm_sched_job_dependency(job, entity)))
  dma_fence_wait(f, false);
  
  

  drm_sched_fence_scheduled(s_fence);

@@ -419,7 +432,6 @@ static bool drm_sched_entity_add_dependency_cb(struct drm_sched_entity *entity) */ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity) {

• struct drm_gpu_scheduler *sched = entity->rq->sched; struct drm_sched_job *sched_job;

  sched_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));

@@ -427,7 +439,7 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity) return NULL;

while ((entity->dependency =

• 	sched->ops->dependency(sched_job, entity))) {
  

• 	drm_sched_job_dependency(sched_job, entity))) {
  

  trace_drm_sched_job_wait_dep(sched_job, entity->dependency);

  if (drm_sched_entity_add_dependency_cb(entity))

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index 70eefed17e06..370c336d383f 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -603,6 +603,8 @@ int drm_sched_job_init(struct drm_sched_job *job,

INIT_LIST_HEAD(&job->list);

• xa_init_flags(&job->dependencies, XA_FLAGS_ALLOC);
• return 0;

} EXPORT_SYMBOL(drm_sched_job_init); @@ -626,6 +628,98 @@ void drm_sched_job_arm(struct drm_sched_job *job) } EXPORT_SYMBOL(drm_sched_job_arm);

+/**

• • drm_sched_job_await_fence - adds the fence as a job dependency
• • @job: scheduler job to add the dependencies to
• • @fence: the dma_fence to add to the list of dependencies.
• • Note that @fence is consumed in both the success and error cases.
• • Returns:
• • 0 on success, or an error on failing to expand the array.
• */

+int drm_sched_job_await_fence(struct drm_sched_job *job,

• 	      struct dma_fence *fence)
  

+{

• struct dma_fence *entry;
• unsigned long index;
• u32 id = 0;
• int ret;
• if (!fence)

• return 0;
  

• /* Deduplicate if we already depend on a fence from the same context.

• * This lets the size of the array of deps scale with the number of
  

• * engines involved, rather than the number of BOs.
  

• */
  

• xa_for_each(&job->dependencies, index, entry) {

• if (entry->context != fence->context)
  

• 	continue;
  

• if (dma_fence_is_later(fence, entry)) {
  

• 	dma_fence_put(entry);
  

• 	xa_store(&job->dependencies, index, fence, GFP_KERNEL);
  

• } else {
  

• 	dma_fence_put(fence);
  

• }
  

• return 0;
  

• }
• ret = xa_alloc(&job->dependencies, &id, fence, xa_limit_32b, GFP_KERNEL);
• if (ret != 0)

• dma_fence_put(fence);
  

• return ret;

+} +EXPORT_SYMBOL(drm_sched_job_await_fence);

+/**

• • drm_sched_job_await_implicit - adds implicit dependencies as job dependencies
• • @job: scheduler job to add the dependencies to
• • @obj: the gem object to add new dependencies from.
• • @write: whether the job might write the object (so we need to depend on
• • shared fences in the reservation object).
• • This should be called after drm_gem_lock_reservations() on your array of
• • GEM objects used in the job but before updating the reservations with your
• • own fences.
• • Returns:
• • 0 on success, or an error on failing to expand the array.
• */

+int drm_sched_job_await_implicit(struct drm_sched_job *job,

• 		 struct drm_gem_object *obj,
  

• 		 bool write)
  

+{

• int ret;
• struct dma_fence **fences;
• unsigned int i, fence_count;
• if (!write) {

• struct dma_fence *fence = dma_resv_get_excl_unlocked(obj->resv);
  

• return drm_sched_job_await_fence(job, fence);
  

• }
• ret = dma_resv_get_fences(obj->resv, NULL, &fence_count, &fences);
• if (ret || !fence_count)

• return ret;
  

• for (i = 0; i < fence_count; i++) {

• ret = drm_sched_job_await_fence(job, fences[i]);
  

• if (ret)
  

• 	break;
  

• }
• for (; i < fence_count; i++)

• dma_fence_put(fences[i]);
  

• kfree(fences);
• return ret;

+} +EXPORT_SYMBOL(drm_sched_job_await_implicit);

/**

• drm_sched_job_cleanup - clean up scheduler job resources

@@ -633,8 +727,17 @@ EXPORT_SYMBOL(drm_sched_job_arm); */ void drm_sched_job_cleanup(struct drm_sched_job *job) {

• struct dma_fence *fence;
• unsigned long index;
• dma_fence_put(&job->s_fence->finished); job->s_fence = NULL;
• xa_for_each(&job->dependencies, index, fence) {

• dma_fence_put(fence);
  

• }
• xa_destroy(&job->dependencies);

} EXPORT_SYMBOL(drm_sched_job_cleanup);

diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index 80438d126c9d..e4d7e1496296 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -27,9 +27,12 @@ #include <drm/spsc_queue.h> #include <linux/dma-fence.h> #include <linux/completion.h> +#include <linux/xarray.h>

#define MAX_WAIT_SCHED_ENTITY_Q_EMPTY msecs_to_jiffies(1000)

+struct drm_gem_object;

struct drm_gpu_scheduler; struct drm_sched_rq;

@@ -198,6 +201,16 @@ struct drm_sched_job { enum drm_sched_priority s_priority; struct drm_sched_entity *entity; struct dma_fence_cb cb;

• /**

• * @dependencies:
  

• *
  

• * Contains the dependencies as struct dma_fence for this job, see
  

• * drm_sched_job_await_fence() and drm_sched_job_await_implicit().
  

• */
  

• struct xarray dependencies;
• /** @last_dependency: tracks @dependencies as they signal */
• unsigned long last_dependency;

};

static inline bool drm_sched_invalidate_job(struct drm_sched_job *s_job, @@ -220,9 +233,14 @@ enum drm_gpu_sched_stat { */ struct drm_sched_backend_ops { /**

•     * @dependency: Called when the scheduler is considering scheduling
  

•     * this job next, to get another struct dma_fence for this job to
  

• * block on.  Once it returns NULL, run_job() may be called.
  

• * @dependency:
  

• *
  

• * Called when the scheduler is considering scheduling this job next, to
  

• * get another struct dma_fence for this job to block on.  Once it
  

• * returns NULL, run_job() may be called.
  

• *
  

• * If a driver exclusively uses drm_sched_job_await_fence() and
  

• * drm_sched_job_await_implicit() this can be ommitted and left as NULL.
  

  */ struct dma_fence *(*dependency)(struct drm_sched_job *sched_job, struct drm_sched_entity *s_entity);

@@ -314,6 +332,13 @@ int drm_sched_job_init(struct drm_sched_job *job, struct drm_sched_entity *entity, void *owner); void drm_sched_job_arm(struct drm_sched_job *job); +int drm_sched_job_await_fence(struct drm_sched_job *job,

• 	      struct dma_fence *fence);
  

+int drm_sched_job_await_implicit(struct drm_sched_job *job,

• 		 struct drm_gem_object *obj,
  

• 		 bool write);
  

void drm_sched_entity_modify_sched(struct drm_sched_entity *entity, struct drm_gpu_scheduler **sched_list, unsigned int num_sched_list);

On Thu, Jun 24, 2021 at 04:39:09PM +0200, Lucas Stach wrote:

Am Donnerstag, dem 24.06.2021 um 16:00 +0200 schrieb Daniel Vetter:

...

Instead of just a callback we can just glue in the gem helpers that panfrost, v3d and lima currently use. There's really not that many ways to skin this cat.

On the naming bikeshed: The idea for using _await_ to denote adding dependencies to a job comes from i915, where that's used quite extensively all over the place, in lots of datastructures.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: David Airlie airlied@linux.ie Cc: Daniel Vetter daniel@ffwll.ch Cc: Sumit Semwal sumit.semwal@linaro.org Cc: "Christian König" christian.koenig@amd.com Cc: Andrey Grodzovsky andrey.grodzovsky@amd.com Cc: Lee Jones lee.jones@linaro.org Cc: Nirmoy Das nirmoy.aiemd@gmail.com Cc: Boris Brezillon boris.brezillon@collabora.com Cc: Luben Tuikov luben.tuikov@amd.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Jack Zhang Jack.Zhang1@amd.com Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org

---

drivers/gpu/drm/scheduler/sched_entity.c | 18 +++- drivers/gpu/drm/scheduler/sched_main.c | 103 +++++++++++++++++++++++ include/drm/gpu_scheduler.h | 31 ++++++- 3 files changed, 146 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index f7347c284886..b6f72fafd504 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -211,6 +211,19 @@ static void drm_sched_entity_kill_jobs_cb(struct dma_fence *f, job->sched->ops->free_job(job); }

+static struct dma_fence * +drm_sched_job_dependency(struct drm_sched_job *job,

• 	 struct drm_sched_entity *entity)
  

+{

• if (!xa_empty(&job->dependencies))

• return xa_erase(&job->dependencies, job->last_dependency++);
  

Not sure how much it buys us now that you dedup fences before adding them to the xa, but we could avoid potentially avoid some ping-pong looping in the scheduler by checking if the fence we are about to return here is already signaled and skipping to the next one if so.

I think there's quit a bit of tricks you can play if the scheduler has more direct visibility into all (or at least most) of the fences. I'm really just trying to establish the drm_sched_job_await api so there's a notch more structure to the dependency handling.

Iow bikesheds on the interface and testing on the patches is what I'm looking for :-) -Daniel

Regards, Lucas

...

• if (job->sched->ops->dependency)

• return job->sched->ops->dependency(job, entity);
  

• return NULL;

+}

/**

• drm_sched_entity_kill_jobs - Make sure all remaining jobs are killed

@@ -229,7 +242,7 @@ static void drm_sched_entity_kill_jobs(struct drm_sched_entity *entity) struct drm_sched_fence *s_fence = job->s_fence;

/* Wait for all dependencies to avoid data corruptions */

• while ((f = job->sched->ops->dependency(job, entity)))
  

• while ((f = drm_sched_job_dependency(job, entity)))
  dma_fence_wait(f, false);
  
  

  drm_sched_fence_scheduled(s_fence);

@@ -419,7 +432,6 @@ static bool drm_sched_entity_add_dependency_cb(struct drm_sched_entity *entity) */ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity) {

• struct drm_gpu_scheduler *sched = entity->rq->sched; struct drm_sched_job *sched_job;

  sched_job = to_drm_sched_job(spsc_queue_peek(&entity->job_queue));

@@ -427,7 +439,7 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity) return NULL;

while ((entity->dependency =

• 	sched->ops->dependency(sched_job, entity))) {
  

• 	drm_sched_job_dependency(sched_job, entity))) {
  

  trace_drm_sched_job_wait_dep(sched_job, entity->dependency);

  if (drm_sched_entity_add_dependency_cb(entity))

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index 70eefed17e06..370c336d383f 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -603,6 +603,8 @@ int drm_sched_job_init(struct drm_sched_job *job,

INIT_LIST_HEAD(&job->list);

• xa_init_flags(&job->dependencies, XA_FLAGS_ALLOC);
• return 0;

} EXPORT_SYMBOL(drm_sched_job_init); @@ -626,6 +628,98 @@ void drm_sched_job_arm(struct drm_sched_job *job) } EXPORT_SYMBOL(drm_sched_job_arm);

+/**

• • drm_sched_job_await_fence - adds the fence as a job dependency
• • @job: scheduler job to add the dependencies to
• • @fence: the dma_fence to add to the list of dependencies.
• • Note that @fence is consumed in both the success and error cases.
• • Returns:
• • 0 on success, or an error on failing to expand the array.
• */

+int drm_sched_job_await_fence(struct drm_sched_job *job,

• 	      struct dma_fence *fence)
  

+{

• struct dma_fence *entry;
• unsigned long index;
• u32 id = 0;
• int ret;
• if (!fence)

• return 0;
  

• /* Deduplicate if we already depend on a fence from the same context.

• * This lets the size of the array of deps scale with the number of
  

• * engines involved, rather than the number of BOs.
  

• */
  

• xa_for_each(&job->dependencies, index, entry) {

• if (entry->context != fence->context)
  

• 	continue;
  

• if (dma_fence_is_later(fence, entry)) {
  

• 	dma_fence_put(entry);
  

• 	xa_store(&job->dependencies, index, fence, GFP_KERNEL);
  

• } else {
  

• 	dma_fence_put(fence);
  

• }
  

• return 0;
  

• }
• ret = xa_alloc(&job->dependencies, &id, fence, xa_limit_32b, GFP_KERNEL);
• if (ret != 0)

• dma_fence_put(fence);
  

• return ret;

+} +EXPORT_SYMBOL(drm_sched_job_await_fence);

+/**

• • drm_sched_job_await_implicit - adds implicit dependencies as job dependencies
• • @job: scheduler job to add the dependencies to
• • @obj: the gem object to add new dependencies from.
• • @write: whether the job might write the object (so we need to depend on
• • shared fences in the reservation object).
• • This should be called after drm_gem_lock_reservations() on your array of
• • GEM objects used in the job but before updating the reservations with your
• • own fences.
• • Returns:
• • 0 on success, or an error on failing to expand the array.
• */

+int drm_sched_job_await_implicit(struct drm_sched_job *job,

• 		 struct drm_gem_object *obj,
  

• 		 bool write)
  

+{

• int ret;
• struct dma_fence **fences;
• unsigned int i, fence_count;
• if (!write) {

• struct dma_fence *fence = dma_resv_get_excl_unlocked(obj->resv);
  

• return drm_sched_job_await_fence(job, fence);
  

• }
• ret = dma_resv_get_fences(obj->resv, NULL, &fence_count, &fences);
• if (ret || !fence_count)

• return ret;
  

• for (i = 0; i < fence_count; i++) {

• ret = drm_sched_job_await_fence(job, fences[i]);
  

• if (ret)
  

• 	break;
  

• }
• for (; i < fence_count; i++)

• dma_fence_put(fences[i]);
  

• kfree(fences);
• return ret;

+} +EXPORT_SYMBOL(drm_sched_job_await_implicit);

/**

• drm_sched_job_cleanup - clean up scheduler job resources

@@ -633,8 +727,17 @@ EXPORT_SYMBOL(drm_sched_job_arm); */ void drm_sched_job_cleanup(struct drm_sched_job *job) {

• struct dma_fence *fence;
• unsigned long index;
• dma_fence_put(&job->s_fence->finished); job->s_fence = NULL;
• xa_for_each(&job->dependencies, index, fence) {

• dma_fence_put(fence);
  

• }
• xa_destroy(&job->dependencies);

} EXPORT_SYMBOL(drm_sched_job_cleanup);

diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index 80438d126c9d..e4d7e1496296 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -27,9 +27,12 @@ #include <drm/spsc_queue.h> #include <linux/dma-fence.h> #include <linux/completion.h> +#include <linux/xarray.h>

#define MAX_WAIT_SCHED_ENTITY_Q_EMPTY msecs_to_jiffies(1000)

+struct drm_gem_object;

struct drm_gpu_scheduler; struct drm_sched_rq;

@@ -198,6 +201,16 @@ struct drm_sched_job { enum drm_sched_priority s_priority; struct drm_sched_entity *entity; struct dma_fence_cb cb;

• /**

• * @dependencies:
  

• *
  

• * Contains the dependencies as struct dma_fence for this job, see
  

• * drm_sched_job_await_fence() and drm_sched_job_await_implicit().
  

• */
  

• struct xarray dependencies;
• /** @last_dependency: tracks @dependencies as they signal */
• unsigned long last_dependency;

};

static inline bool drm_sched_invalidate_job(struct drm_sched_job *s_job, @@ -220,9 +233,14 @@ enum drm_gpu_sched_stat { */ struct drm_sched_backend_ops { /**

•     * @dependency: Called when the scheduler is considering scheduling
  

•     * this job next, to get another struct dma_fence for this job to
  

• * block on.  Once it returns NULL, run_job() may be called.
  

• * @dependency:
  

• *
  

• * Called when the scheduler is considering scheduling this job next, to
  

• * get another struct dma_fence for this job to block on.  Once it
  

• * returns NULL, run_job() may be called.
  

• *
  

• * If a driver exclusively uses drm_sched_job_await_fence() and
  

• * drm_sched_job_await_implicit() this can be ommitted and left as NULL.
  

  */ struct dma_fence *(*dependency)(struct drm_sched_job *sched_job, struct drm_sched_entity *s_entity);

@@ -314,6 +332,13 @@ int drm_sched_job_init(struct drm_sched_job *job, struct drm_sched_entity *entity, void *owner); void drm_sched_job_arm(struct drm_sched_job *job); +int drm_sched_job_await_fence(struct drm_sched_job *job,

• 	      struct dma_fence *fence);
  

+int drm_sched_job_await_implicit(struct drm_sched_job *job,

• 		 struct drm_gem_object *obj,
  

• 		 bool write);
  

void drm_sched_entity_modify_sched(struct drm_sched_entity *entity, struct drm_gpu_scheduler **sched_list, unsigned int num_sched_list);

Prep work for using the scheduler dependency handling. We need to call drm_sched_job_init earlier so we can use the new drm_sched_job_await* functions for dependency handling here.

v2: Slightly better commit message and rebase to include the drm_sched_job_arm() call (Emma).

Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Emma Anholt emma@anholt.net --- drivers/gpu/drm/v3d/v3d_gem.c | 64 +++++++++++------------------------ 1 file changed, 20 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c index 69ac20e11b09..d1028ccf6dd5 100644 --- a/drivers/gpu/drm/v3d/v3d_gem.c +++ b/drivers/gpu/drm/v3d/v3d_gem.c @@ -433,9 +433,10 @@ v3d_wait_bo_ioctl(struct drm_device *dev, void *data, static int v3d_job_init(struct v3d_dev *v3d, struct drm_file *file_priv, struct v3d_job *job, void (*free)(struct kref *ref), - u32 in_sync) + u32 in_sync, enum v3d_queue queue) { struct dma_fence *in_fence = NULL; + struct v3d_file_priv *v3d_priv = file_priv->driver_priv; int ret;

job->v3d = v3d; @@ -446,6 +447,10 @@ v3d_job_init(struct v3d_dev *v3d, struct drm_file *file_priv, return ret;

xa_init_flags(&job->deps, XA_FLAGS_ALLOC); + ret = drm_sched_job_init(&job->base, &v3d_priv->sched_entity[queue], + v3d_priv); + if (ret) + goto fail;

ret = drm_syncobj_find_fence(file_priv, in_sync, 0, 0, &in_fence); if (ret == -EINVAL) @@ -464,17 +469,9 @@ v3d_job_init(struct v3d_dev *v3d, struct drm_file *file_priv, return ret; }

-static int -v3d_push_job(struct v3d_file_priv *v3d_priv, - struct v3d_job *job, enum v3d_queue queue) +static void +v3d_push_job(struct v3d_job *job) { - int ret; - - ret = drm_sched_job_init(&job->base, &v3d_priv->sched_entity[queue], - v3d_priv); - if (ret) - return ret; - drm_sched_job_arm(&job->base);

job->done_fence = dma_fence_get(&job->base.s_fence->finished); @@ -483,8 +480,6 @@ v3d_push_job(struct v3d_file_priv *v3d_priv, kref_get(&job->refcount);

drm_sched_entity_push_job(&job->base); - - return 0; }

static void @@ -530,7 +525,6 @@ 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 drm_v3d_submit_cl *args = data; struct v3d_bin_job *bin = NULL; struct v3d_render_job *render; @@ -556,7 +550,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data, INIT_LIST_HEAD(&render->unref_list);

ret = v3d_job_init(v3d, file_priv, &render->base, - v3d_render_job_free, args->in_sync_rcl); + v3d_render_job_free, args->in_sync_rcl, V3D_RENDER); if (ret) { kfree(render); return ret; @@ -570,7 +564,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data, }

ret = v3d_job_init(v3d, file_priv, &bin->base, - v3d_job_free, args->in_sync_bcl); + v3d_job_free, args->in_sync_bcl, V3D_BIN); if (ret) { v3d_job_put(&render->base); kfree(bin); @@ -592,7 +586,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data, goto fail; }

- ret = v3d_job_init(v3d, file_priv, clean_job, v3d_job_free, 0); + ret = v3d_job_init(v3d, file_priv, clean_job, v3d_job_free, 0, V3D_CACHE_CLEAN); if (ret) { kfree(clean_job); clean_job = NULL; @@ -615,9 +609,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data,

mutex_lock(&v3d->sched_lock); if (bin) { - ret = v3d_push_job(v3d_priv, &bin->base, V3D_BIN); - if (ret) - goto fail_unreserve; + v3d_push_job(&bin->base);

ret = drm_gem_fence_array_add(&render->base.deps, dma_fence_get(bin->base.done_fence)); @@ -625,9 +617,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data, goto fail_unreserve; }

- ret = v3d_push_job(v3d_priv, &render->base, V3D_RENDER); - if (ret) - goto fail_unreserve; + v3d_push_job(&render->base);

if (clean_job) { struct dma_fence *render_fence = @@ -635,9 +625,7 @@ v3d_submit_cl_ioctl(struct drm_device *dev, void *data, ret = drm_gem_fence_array_add(&clean_job->deps, render_fence); if (ret) goto fail_unreserve; - ret = v3d_push_job(v3d_priv, clean_job, V3D_CACHE_CLEAN); - if (ret) - goto fail_unreserve; + v3d_push_job(clean_job); }

mutex_unlock(&v3d->sched_lock); @@ -684,7 +672,6 @@ v3d_submit_tfu_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 drm_v3d_submit_tfu *args = data; struct v3d_tfu_job *job; struct ww_acquire_ctx acquire_ctx; @@ -697,7 +684,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data, return -ENOMEM;

ret = v3d_job_init(v3d, file_priv, &job->base, - v3d_job_free, args->in_sync); + v3d_job_free, args->in_sync, V3D_TFU); if (ret) { kfree(job); return ret; @@ -741,9 +728,7 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data, goto fail;

mutex_lock(&v3d->sched_lock); - ret = v3d_push_job(v3d_priv, &job->base, V3D_TFU); - if (ret) - goto fail_unreserve; + v3d_push_job(&job->base); mutex_unlock(&v3d->sched_lock);

v3d_attach_fences_and_unlock_reservation(file_priv, @@ -755,10 +740,6 @@ v3d_submit_tfu_ioctl(struct drm_device *dev, void *data,

return 0;

-fail_unreserve: - mutex_unlock(&v3d->sched_lock); - drm_gem_unlock_reservations(job->base.bo, job->base.bo_count, - &acquire_ctx); fail: v3d_job_put(&job->base);

@@ -779,7 +760,6 @@ v3d_submit_csd_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 drm_v3d_submit_csd *args = data; struct v3d_csd_job *job; struct v3d_job *clean_job; @@ -798,7 +778,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data, return -ENOMEM;

ret = v3d_job_init(v3d, file_priv, &job->base, - v3d_job_free, args->in_sync); + v3d_job_free, args->in_sync, V3D_CSD); if (ret) { kfree(job); return ret; @@ -811,7 +791,7 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data, return -ENOMEM; }

- ret = v3d_job_init(v3d, file_priv, clean_job, v3d_job_free, 0); + ret = v3d_job_init(v3d, file_priv, clean_job, v3d_job_free, 0, V3D_CACHE_CLEAN); if (ret) { v3d_job_put(&job->base); kfree(clean_job); @@ -830,18 +810,14 @@ v3d_submit_csd_ioctl(struct drm_device *dev, void *data, goto fail;

mutex_lock(&v3d->sched_lock); - ret = v3d_push_job(v3d_priv, &job->base, V3D_CSD); - if (ret) - goto fail_unreserve; + v3d_push_job(&job->base);

ret = drm_gem_fence_array_add(&clean_job->deps, dma_fence_get(job->base.done_fence)); if (ret) goto fail_unreserve;

- ret = v3d_push_job(v3d_priv, clean_job, V3D_CACHE_CLEAN); - if (ret) - goto fail_unreserve; + v3d_push_job(clean_job); mutex_unlock(&v3d->sched_lock);

v3d_attach_fences_and_unlock_reservation(file_priv,

Am Donnerstag, dem 24.06.2021 um 16:00 +0200 schrieb Daniel Vetter:

This is essentially part of drm_sched_dependency_optimized(), which only amdgpu seems to make use of. Use it a bit more.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com

Reviewed-by: Lucas Stach l.stach@pengutronix.de

Cc: "Christian König" christian.koenig@amd.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Luben Tuikov luben.tuikov@amd.com Cc: Andrey Grodzovsky andrey.grodzovsky@amd.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Jack Zhang Jack.Zhang1@amd.com

---

drivers/gpu/drm/scheduler/sched_main.c | 7 +++++++ 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index 370c336d383f..c31d7cf7df74 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -649,6 +649,13 @@ int drm_sched_job_await_fence(struct drm_sched_job *job, if (!fence) return 0;

• /* if it's a fence from us it's guaranteed to be earlier */
• if (fence->context == job->entity->fence_context ||

•    fence->context == job->entity->fence_context + 1) {
  

• dma_fence_put(fence);
  

• return 0;
  

• }
• /* Deduplicate if we already depend on a fence from the same context.
  • This lets the size of the array of deps scale with the number of
  • engines involved, rather than the number of BOs.

On Thu, Jun 24, 2021 at 07:03:10PM +0200, Christian König wrote:

Am 24.06.21 um 16:00 schrieb Daniel Vetter:

...

This is essentially part of drm_sched_dependency_optimized(), which only amdgpu seems to make use of. Use it a bit more.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: "Christian König" christian.koenig@amd.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Luben Tuikov luben.tuikov@amd.com Cc: Andrey Grodzovsky andrey.grodzovsky@amd.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Jack Zhang Jack.Zhang1@amd.com

---

drivers/gpu/drm/scheduler/sched_main.c | 7 +++++++ 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index 370c336d383f..c31d7cf7df74 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -649,6 +649,13 @@ int drm_sched_job_await_fence(struct drm_sched_job *job, if (!fence) return 0;

• /* if it's a fence from us it's guaranteed to be earlier */
• if (fence->context == job->entity->fence_context ||

•    fence->context == job->entity->fence_context + 1) {
  

• dma_fence_put(fence);
  

• return 0;
  

• }

Well NAK. That would break Vulkan.

The problem is that Vulkan can insert dependencies between jobs which run on the same queue.

So we need to track those as well and if the previous job for the same queue/scheduler is not yet finished a pipeline synchronization needs to be inserted.

That's one of the reasons we wasn't able to unify the dependency handling yet.

That sounds like an extremely amdgpu specific constraint? You're also the only one who keeps track of whether the previous job we've scheduled has finished already (I guess they can get pipelined and you don't flush by default), so you insert fences.

I guess we can add a await_fence_no_dedup or so for amdgpu, but I'm not sure why we have to inflict this design constraint on all other drivers? At least I'm not seeing anything in lima, panfrost, v3d or entaviv that would break with this, and i915 will also be perfectly fine.

Also note: I'm not using this for amdgpu, exactly because there's a few funny things going on.

Finally: You _really_ need explicit dependency handling for vulkan in your uapi, instead of the kernel second-guessing what userspace might be doing. That's really not how vulkan is designed to work :-)

Cheers, Daniel

Christian.

...

/* Deduplicate if we already depend on a fence from the same context. * This lets the size of the array of deps scale with the number of * engines involved, rather than the number of BOs.

On Thu, Jun 24, 2021 at 7:38 PM Christian König christian.koenig@amd.com wrote:

Am 24.06.21 um 19:29 schrieb Daniel Vetter:

...

On Thu, Jun 24, 2021 at 07:03:10PM +0200, Christian König wrote:

...

Am 24.06.21 um 16:00 schrieb Daniel Vetter:

...

This is essentially part of drm_sched_dependency_optimized(), which only amdgpu seems to make use of. Use it a bit more.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: "Christian König" christian.koenig@amd.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Luben Tuikov luben.tuikov@amd.com Cc: Andrey Grodzovsky andrey.grodzovsky@amd.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Jack Zhang Jack.Zhang1@amd.com

---

drivers/gpu/drm/scheduler/sched_main.c | 7 +++++++ 1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index 370c336d383f..c31d7cf7df74 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -649,6 +649,13 @@ int drm_sched_job_await_fence(struct drm_sched_job *job, if (!fence) return 0;

• /* if it's a fence from us it's guaranteed to be earlier */
• if (fence->context == job->entity->fence_context ||

•   fence->context == job->entity->fence_context + 1) {
  

•       dma_fence_put(fence);
  

•       return 0;
  

• }

Well NAK. That would break Vulkan.

I'm assuming your reply means the NAK is retracted and was just the usual "this doesn't perfectly fit for amdgpu" reflex?

...

...

The problem is that Vulkan can insert dependencies between jobs which run on the same queue.

So we need to track those as well and if the previous job for the same queue/scheduler is not yet finished a pipeline synchronization needs to be inserted.

That's one of the reasons we wasn't able to unify the dependency handling yet.

That sounds like an extremely amdgpu specific constraint?

Yeah, that's totally hardware specific.

It's just that I don't know how else we could track that without having the same separation as in amdgpu between implicit and explicit fences. And as far as I understand it that's exactly what you want to avoid.

As I said this turned out to be really awkward.

...

You're also the only one who keeps track of whether the previous job we've scheduled has finished already (I guess they can get pipelined and you don't flush by default), so you insert fences.

Yes, exactly that.

...

I guess we can add a await_fence_no_dedup or so for amdgpu, but I'm not sure why we have to inflict this design constraint on all other drivers? At least I'm not seeing anything in lima, panfrost, v3d or entaviv that would break with this, and i915 will also be perfectly fine.

Also note: I'm not using this for amdgpu, exactly because there's a few funny things going on.

Yeah, exactly the reason why we never unified this.

Yeah there's clear limits to this, because you also can't use the await_implicit helper, because you have to keep filtering for owner or the current amdgpu uapi goes horribly slow. I think the benefit would be just that we could share the datastructure and the book-keeping, but aside from that you'd need your own integration in amdgpu.

One idea I just had was whether we could use the tag bits xarray has for the amdgpu purposed. Like we could do a drm_sched_job_await_fence_tagged, where you supply additional information (like the "this might be relevant for the vm_flush" and things like that). Afaiui xarray tags are very fast to enumerate on if you're looking for specific tags, but I might be wrong. Ideally this would avoid the need for the duplicated amdgpu_job->sched.

Cheers, Daniel

Regards, Christian.

...

Finally: You _really_ need explicit dependency handling for vulkan in your uapi, instead of the kernel second-guessing what userspace might be doing. That's really not how vulkan is designed to work :-)

...

Cheers, Daniel

...

Christian.

...

/* Deduplicate if we already depend on a fence from the same context.
 * This lets the size of the array of deps scale with the number of
 * engines involved, rather than the number of BOs.

On Thu, Jun 24, 2021 at 7:56 PM Christian König christian.koenig@amd.com wrote:

Am 24.06.21 um 19:43 schrieb Daniel Vetter:

...

On Thu, Jun 24, 2021 at 7:38 PM Christian König christian.koenig@amd.com wrote:

...

Am 24.06.21 um 19:29 schrieb Daniel Vetter:

...

On Thu, Jun 24, 2021 at 07:03:10PM +0200, Christian König wrote:

...

Am 24.06.21 um 16:00 schrieb Daniel Vetter:

...

This is essentially part of drm_sched_dependency_optimized(), which only amdgpu seems to make use of. Use it a bit more.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: "Christian König" christian.koenig@amd.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Luben Tuikov luben.tuikov@amd.com Cc: Andrey Grodzovsky andrey.grodzovsky@amd.com Cc: Alex Deucher alexander.deucher@amd.com Cc: Jack Zhang Jack.Zhang1@amd.com

---

drivers/gpu/drm/scheduler/sched_main.c | 7 +++++++
1 file changed, 7 insertions(+)

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index 370c336d383f..c31d7cf7df74 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -649,6 +649,13 @@ int drm_sched_job_await_fence(struct drm_sched_job *job, if (!fence) return 0;

• /* if it's a fence from us it's guaranteed to be earlier */
• if (fence->context == job->entity->fence_context ||

•   fence->context == job->entity->fence_context + 1) {
  

•       dma_fence_put(fence);
  

•       return 0;
  

• }

Well NAK. That would break Vulkan.

I'm assuming your reply means the NAK is retracted and was just the usual "this doesn't perfectly fit for amdgpu" reflex?

Well rather "NAK, you haven't considered that special handling in amdgpu and if you really want to unify this you need that as well."

...

...

...

...

The problem is that Vulkan can insert dependencies between jobs which run on the same queue.

So we need to track those as well and if the previous job for the same queue/scheduler is not yet finished a pipeline synchronization needs to be inserted.

That's one of the reasons we wasn't able to unify the dependency handling yet.

That sounds like an extremely amdgpu specific constraint?

Yeah, that's totally hardware specific.

It's just that I don't know how else we could track that without having the same separation as in amdgpu between implicit and explicit fences. And as far as I understand it that's exactly what you want to avoid.

As I said this turned out to be really awkward.

...

You're also the only one who keeps track of whether the previous job we've scheduled has finished already (I guess they can get pipelined and you don't flush by default), so you insert fences.

Yes, exactly that.

...

I guess we can add a await_fence_no_dedup or so for amdgpu, but I'm not sure why we have to inflict this design constraint on all other drivers? At least I'm not seeing anything in lima, panfrost, v3d or entaviv that would break with this, and i915 will also be perfectly fine.

Also note: I'm not using this for amdgpu, exactly because there's a few funny things going on.

Yeah, exactly the reason why we never unified this.

Yeah there's clear limits to this, because you also can't use the await_implicit helper, because you have to keep filtering for owner or the current amdgpu uapi goes horribly slow. I think the benefit would be just that we could share the datastructure and the book-keeping, but aside from that you'd need your own integration in amdgpu.

Yeah, but that is trivial. The _add_dependency() function (or however we want to call it) needs to be exported anyway for adding fences from syncfile and syncobj.

Or do you also want to unify the handling for those?

I guess we could add some convenience wrapper that pulls in a sync_file or sync_objc automatically. But there's not that much code involved there, and it's also not tricky. Also drivers might need to add dependencies for whatever anyway. The await_implicit is a bit different, because that defines how implicit sync is supposed to work.

I guess the bikeshed then boils down to which one is the simple await_fence() function. The one that filters for same timeline, or the one that doesnt. I'd make the non-filtering one the special case so that amdgpu sticks out a bit more - out of 6 drivers with schedulers (i915 included) it seems to be the special one.

...

One idea I just had was whether we could use the tag bits xarray has for the amdgpu purposed. Like we could do a drm_sched_job_await_fence_tagged, where you supply additional information (like the "this might be relevant for the vm_flush" and things like that). Afaiui xarray tags are very fast to enumerate on if you're looking for specific tags, but I might be wrong. Ideally this would avoid the need for the duplicated amdgpu_job->sched.

That could work.

Essentially we just need the information from the scheduler which is the last fence which was dependency optimized.

In other words when you push jobs like those to the same scheduler

J1 J2 -> depends on J1. J3 -> depends on whatever, but not j2

The hardware needs to insert a flush between J2 and J1, but not between j3 and j2.

This makes roughly 19% performance difference for some OpenGL games and incorrect rendering for Vulkan if you mess it up either way or the other.

Yeah that's massive. On i915 "too many pipeline stalls" even within batches is a lot less, so we never bothered with this at all. -Daniel

Regards, Christian.

...

Cheers, Daniel

...

Regards, Christian.

...

Finally: You _really_ need explicit dependency handling for vulkan in your uapi, instead of the kernel second-guessing what userspace might be doing. That's really not how vulkan is designed to work :-) Cheers, Daniel

...

Christian.

...

 /* Deduplicate if we already depend on a fence from the same context.
  * This lets the size of the array of deps scale with the number of
  * engines involved, rather than the number of BOs.