On Thu, Aug 5, 2021 at 3:44 PM Christian König christian.koenig@amd.com wrote:

Am 05.08.21 um 12:46 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.

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)

v3: Emma noticed that I leak the memory allocated in drm_sched_job_init if we bail out before the point of no return in subsequent driver patches. To be able to fix this change drm_sched_job_cleanup() so it can handle being called both before and after drm_sched_job_arm().

Also improve the kerneldoc for this.

v4:

• Fix the drm_sched_job_cleanup logic, I inverted the booleans, as usual (Melissa)

• Christian pointed out that drm_sched_entity_select_rq() also needs to be moved into drm_sched_job_arm, which made me realize that the job->id definitely needs to be moved too.

  Shuffle things to fit between job_init and job_arm.

v5: Reshuffle the split between init/arm once more, amdgpu abuses drm_sched.ready to signal gpu reset failures. Also document this somewhat. (Christian)

v6: Rebase on top of the msm drm/sched support. Note that the drm_sched_job_init() call is completely misplaced, and hence also the split-out drm_sched_entity_push_job(). I've put in a FIXME which the next patch will address.

Acked-by: Melissa Wen mwen@igalia.com Cc: Melissa Wen melissa.srw@gmail.com Acked-by: Emma Anholt emma@anholt.net Acked-by: Steven Price steven.price@arm.com (v2) Reviewed-by: Boris Brezillon boris.brezillon@collabora.com (v5) Signed-off-by: Daniel Vetter daniel.vetter@intel.com

At least the amdgpu parts look ok of hand, but I can't judge the rest I think.

The thing that really scares me here and that I got wrong a few times is the cleanup for drm_sched_job at the various points. Can you give those parts in drm/scheduler/ a full review pls, just to make sure? I can note that in the tag ofc, just like a bit more confidence here that it's not busted :-)

So only Acked-by: Christian König christian.koenig@amd.com

Thanks, Daniel

...

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: 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 Cc: Rob Clark robdclark@gmail.com Cc: Sean Paul sean@poorly.run Cc: linux-arm-msm@vger.kernel.org Cc: freedreno@lists.freedesktop.org

---

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/msm/msm_gem_submit.c | 3 ++ drivers/gpu/drm/panfrost/panfrost_job.c | 2 + drivers/gpu/drm/scheduler/sched_entity.c | 6 +-- drivers/gpu/drm/scheduler/sched_fence.c | 19 ++++--- drivers/gpu/drm/scheduler/sched_main.c | 69 ++++++++++++++++++++---- drivers/gpu/drm/v3d/v3d_gem.c | 2 + include/drm/gpu_scheduler.h | 7 ++- 11 files changed, 94 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index 139cd3bf1ad6..32e80bc6af22 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 feb6da1b6ceb..05f412204118 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 dba8329937a3..38f755580507 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/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c index fdc5367aecaa..6d6c44f0e1f3 100644 --- a/drivers/gpu/drm/msm/msm_gem_submit.c +++ b/drivers/gpu/drm/msm/msm_gem_submit.c @@ -52,6 +52,9 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev, return ERR_PTR(ret); }

• /* FIXME: this is way too early */
  

• drm_sched_job_arm(&job->base);
  

• xa_init_flags(&submit->deps, XA_FLAGS_ALLOC);
  
  kref_init(&submit->ref);
  

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c index 71a72fb50e6b..2992dc85325f 100644 --- a/drivers/gpu/drm/panfrost/panfrost_job.c +++ b/drivers/gpu/drm/panfrost/panfrost_job.c @@ -288,6 +288,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..bcea035cf4c6 100644 --- a/drivers/gpu/drm/scheduler/sched_fence.c +++ b/drivers/gpu/drm/scheduler/sched_fence.c @@ -90,7 +90,7 @@ static const char *drm_sched_fence_get_timeline_name(struct dma_fence *f)

• Free up the fence memory after the RCU grace period.

*/ -static void drm_sched_fence_free(struct rcu_head *rcu) +void drm_sched_fence_free(struct rcu_head *rcu) { struct dma_fence *f = container_of(rcu, struct dma_fence, rcu); struct drm_sched_fence *fence = to_drm_sched_fence(f); @@ -152,27 +152,32 @@ 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)
          return NULL;
  
  fence->owner = owner;
  

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

• fence->sched = entity->rq->sched;
  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 33c414d55fab..454cb6164bdc 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>

@@ -569,7 +571,6 @@ EXPORT_SYMBOL(drm_sched_resubmit_jobs_ext);

/**

• drm_sched_job_init - init a scheduler job

• • @job: scheduler job to init
  • @entity: scheduler entity to use
  • @owner: job owner for debugging

@@ -577,27 +578,28 @@ EXPORT_SYMBOL(drm_sched_resubmit_jobs_ext);

• Refer to drm_sched_entity_push_job() documentation
• for locking considerations.

• • Drivers must make sure drm_sched_job_cleanup() if this function returns
• • successfully, even when @job is aborted before drm_sched_job_arm() is called.
• • WARNING: amdgpu abuses &drm_sched.ready to signal when the hardware
• • has died, which can mean that there's no valid runqueue for a @entity.
• • This function returns -ENOENT in this case (which probably should be -EIO as
• • a more meanigful return value).
• • Returns 0 for success, negative error code otherwise.
  */ int drm_sched_job_init(struct drm_sched_job *job, struct drm_sched_entity *entity, void *owner) {

• struct drm_gpu_scheduler *sched;
  

• drm_sched_entity_select_rq(entity);
  if (!entity->rq)
          return -ENOENT;
  
  

• sched = entity->rq->sched;
  

• 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);
  
  INIT_LIST_HEAD(&job->list);
  
  

@@ -606,13 +608,58 @@ int drm_sched_job_init(struct drm_sched_job *job, EXPORT_SYMBOL(drm_sched_job_init);

/**

• • drm_sched_job_cleanup - clean up scheduler job resources

• • 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) +{

• struct drm_gpu_scheduler *sched;
  

• struct drm_sched_entity *entity = job->entity;
  

• BUG_ON(!entity);
  

• sched = entity->rq->sched;
  

• job->sched = sched;
  

• job->s_priority = entity->rq - sched->sched_rq;
  

• job->id = atomic64_inc_return(&sched->job_id_count);
  

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

+} +EXPORT_SYMBOL(drm_sched_job_arm);

+/**

• • drm_sched_job_cleanup - clean up scheduler job resources
  • @job: scheduler job to clean up
• • Cleans up the resources allocated with drm_sched_job_init().
• • Drivers should call this from their error unwind code if @job is aborted
• • before drm_sched_job_arm() is called.
• • After that point of no return @job is committed to be executed by the
• • scheduler, and this function should be called from the
• • &drm_sched_backend_ops.free_job callback.
  */ void drm_sched_job_cleanup(struct drm_sched_job *job) {

• dma_fence_put(&job->s_fence->finished);
  

• if (kref_read(&job->s_fence->finished.refcount)) {
  

•         /* drm_sched_job_arm() has been called */
  

•         dma_fence_put(&job->s_fence->finished);
  

• } else {
  

•         /* aborted job before committing to run it */
  

•         drm_sched_fence_free(&job->s_fence->finished.rcu);
  

• }
  

• job->s_fence = NULL;
  

  } EXPORT_SYMBOL(drm_sched_job_cleanup);

diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c index 5689da118197..2e808097b4d1 100644 --- a/drivers/gpu/drm/v3d/v3d_gem.c +++ b/drivers/gpu/drm/v3d/v3d_gem.c @@ -480,6 +480,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 88ae7f331bb1..83afc3aa8e2f 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -348,6 +348,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); @@ -387,8 +388,12 @@ 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_free(struct rcu_head *rcu);

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

On Thu, Aug 5, 2021 at 4:47 PM Christian König christian.koenig@amd.com wrote:

Am 05.08.21 um 16:07 schrieb Daniel Vetter:

...

On Thu, Aug 5, 2021 at 3:44 PM Christian König christian.koenig@amd.com wrote:

...

Am 05.08.21 um 12:46 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.

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)

v3: Emma noticed that I leak the memory allocated in drm_sched_job_init if we bail out before the point of no return in subsequent driver patches. To be able to fix this change drm_sched_job_cleanup() so it can handle being called both before and after drm_sched_job_arm().

Also improve the kerneldoc for this.

v4:

• Fix the drm_sched_job_cleanup logic, I inverted the booleans, as usual (Melissa)

• Christian pointed out that drm_sched_entity_select_rq() also needs to be moved into drm_sched_job_arm, which made me realize that the job->id definitely needs to be moved too.

  Shuffle things to fit between job_init and job_arm.

v5: Reshuffle the split between init/arm once more, amdgpu abuses drm_sched.ready to signal gpu reset failures. Also document this somewhat. (Christian)

v6: Rebase on top of the msm drm/sched support. Note that the drm_sched_job_init() call is completely misplaced, and hence also the split-out drm_sched_entity_push_job(). I've put in a FIXME which the next patch will address.

Acked-by: Melissa Wen mwen@igalia.com Cc: Melissa Wen melissa.srw@gmail.com Acked-by: Emma Anholt emma@anholt.net Acked-by: Steven Price steven.price@arm.com (v2) Reviewed-by: Boris Brezillon boris.brezillon@collabora.com (v5) Signed-off-by: Daniel Vetter daniel.vetter@intel.com

At least the amdgpu parts look ok of hand, but I can't judge the rest I think.

The thing that really scares me here and that I got wrong a few times is the cleanup for drm_sched_job at the various points. Can you give those parts in drm/scheduler/ a full review pls, just to make sure? I can note that in the tag ofc, just like a bit more confidence here that it's not busted :-)

I can take another look, but I won't have time for that in the next two weeks - vacation and kid starting school.

Hm ok I'll ask others, since this is kinda needed for the msm fix. At least the msm design relies on this split being present, so fixing it without this split here would be a pile of rather pointless work. -Daniel

Christian.

...

...

So only Acked-by: Christian König christian.koenig@amd.com

Thanks, Daniel

...

...

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: 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 Cc: Rob Clark robdclark@gmail.com Cc: Sean Paul sean@poorly.run Cc: linux-arm-msm@vger.kernel.org Cc: freedreno@lists.freedesktop.org

---

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/msm/msm_gem_submit.c | 3 ++ drivers/gpu/drm/panfrost/panfrost_job.c | 2 + drivers/gpu/drm/scheduler/sched_entity.c | 6 +-- drivers/gpu/drm/scheduler/sched_fence.c | 19 ++++--- drivers/gpu/drm/scheduler/sched_main.c | 69 ++++++++++++++++++++---- drivers/gpu/drm/v3d/v3d_gem.c | 2 + include/drm/gpu_scheduler.h | 7 ++- 11 files changed, 94 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index 139cd3bf1ad6..32e80bc6af22 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 feb6da1b6ceb..05f412204118 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 dba8329937a3..38f755580507 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/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c index fdc5367aecaa..6d6c44f0e1f3 100644 --- a/drivers/gpu/drm/msm/msm_gem_submit.c +++ b/drivers/gpu/drm/msm/msm_gem_submit.c @@ -52,6 +52,9 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev, return ERR_PTR(ret); }

• /* FIXME: this is way too early */
  

• drm_sched_job_arm(&job->base);
  

•  xa_init_flags(&submit->deps, XA_FLAGS_ALLOC);
  
   kref_init(&submit->ref);
  

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c index 71a72fb50e6b..2992dc85325f 100644 --- a/drivers/gpu/drm/panfrost/panfrost_job.c +++ b/drivers/gpu/drm/panfrost/panfrost_job.c @@ -288,6 +288,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..bcea035cf4c6 100644 --- a/drivers/gpu/drm/scheduler/sched_fence.c +++ b/drivers/gpu/drm/scheduler/sched_fence.c @@ -90,7 +90,7 @@ static const char *drm_sched_fence_get_timeline_name(struct dma_fence *f) * * Free up the fence memory after the RCU grace period. */ -static void drm_sched_fence_free(struct rcu_head *rcu) +void drm_sched_fence_free(struct rcu_head *rcu) { struct dma_fence *f = container_of(rcu, struct dma_fence, rcu); struct drm_sched_fence *fence = to_drm_sched_fence(f); @@ -152,27 +152,32 @@ 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)
           return NULL;
  
   fence->owner = owner;
  

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

• fence->sched = entity->rq->sched;
   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 33c414d55fab..454cb6164bdc 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>

@@ -569,7 +571,6 @@ EXPORT_SYMBOL(drm_sched_resubmit_jobs_ext);

/** * drm_sched_job_init - init a scheduler job

• • • @job: scheduler job to init
    • @entity: scheduler entity to use
    • @owner: job owner for debugging

@@ -577,27 +578,28 @@ EXPORT_SYMBOL(drm_sched_resubmit_jobs_ext); * Refer to drm_sched_entity_push_job() documentation * for locking considerations. *

• • Drivers must make sure drm_sched_job_cleanup() if this function returns
• • successfully, even when @job is aborted before drm_sched_job_arm() is called.
• • WARNING: amdgpu abuses &drm_sched.ready to signal when the hardware
• • has died, which can mean that there's no valid runqueue for a @entity.
• • This function returns -ENOENT in this case (which probably should be -EIO as
• • a more meanigful return value).
• • • Returns 0 for success, negative error code otherwise.
    */
  int drm_sched_job_init(struct drm_sched_job *job, struct drm_sched_entity *entity, void *owner) {

• struct drm_gpu_scheduler *sched;
  

•  drm_sched_entity_select_rq(entity);
   if (!entity->rq)
           return -ENOENT;
  
  

• sched = entity->rq->sched;
  

• 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);
  
   INIT_LIST_HEAD(&job->list);
  
  

@@ -606,13 +608,58 @@ int drm_sched_job_init(struct drm_sched_job *job, EXPORT_SYMBOL(drm_sched_job_init);

/**

• • drm_sched_job_cleanup - clean up scheduler job resources

• • 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) +{

• struct drm_gpu_scheduler *sched;
  

• struct drm_sched_entity *entity = job->entity;
  

• BUG_ON(!entity);
  

• sched = entity->rq->sched;
  

• job->sched = sched;
  

• job->s_priority = entity->rq - sched->sched_rq;
  

• job->id = atomic64_inc_return(&sched->job_id_count);
  

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

+} +EXPORT_SYMBOL(drm_sched_job_arm);

+/**

• • drm_sched_job_cleanup - clean up scheduler job resources
    • @job: scheduler job to clean up
• • Cleans up the resources allocated with drm_sched_job_init().
• • Drivers should call this from their error unwind code if @job is aborted
• • before drm_sched_job_arm() is called.
• • After that point of no return @job is committed to be executed by the
• • scheduler, and this function should be called from the
• • &drm_sched_backend_ops.free_job callback. */
  void drm_sched_job_cleanup(struct drm_sched_job *job) {

• dma_fence_put(&job->s_fence->finished);
  

• if (kref_read(&job->s_fence->finished.refcount)) {
  

•         /* drm_sched_job_arm() has been called */
  

•         dma_fence_put(&job->s_fence->finished);
  

• } else {
  

•         /* aborted job before committing to run it */
  

•         drm_sched_fence_free(&job->s_fence->finished.rcu);
  

• }
  

•  job->s_fence = NULL;
  

  } EXPORT_SYMBOL(drm_sched_job_cleanup);

diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c index 5689da118197..2e808097b4d1 100644 --- a/drivers/gpu/drm/v3d/v3d_gem.c +++ b/drivers/gpu/drm/v3d/v3d_gem.c @@ -480,6 +480,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 88ae7f331bb1..83afc3aa8e2f 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -348,6 +348,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); @@ -387,8 +388,12 @@ 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_free(struct rcu_head *rcu);

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

On Fri, Aug 6, 2021 at 12:58 AM Rob Clark robdclark@gmail.com wrote:

On Thu, Aug 5, 2021 at 3:47 AM Daniel Vetter daniel.vetter@ffwll.ch wrote:

...

Originally drm_sched_job_init was the point of no return, after which drivers must submit a job. I've split that up, which allows us to fix this issue pretty easily.

Only thing we have to take care of is to not skip to error paths after that. Other drivers do this the same for out-fence and similar things.

Fixes: 1d8a5ca436ee ("drm/msm: Conversion to drm scheduler") Cc: Rob Clark robdclark@chromium.org Cc: Rob Clark robdclark@gmail.com Cc: Sean Paul sean@poorly.run Cc: Sumit Semwal sumit.semwal@linaro.org Cc: "Christian König" christian.koenig@amd.com Cc: linux-arm-msm@vger.kernel.org Cc: dri-devel@lists.freedesktop.org Cc: freedreno@lists.freedesktop.org Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org Signed-off-by: Daniel Vetter daniel.vetter@intel.com

---

drivers/gpu/drm/msm/msm_gem_submit.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c index 6d6c44f0e1f3..d0ed4ddc509e 100644 --- a/drivers/gpu/drm/msm/msm_gem_submit.c +++ b/drivers/gpu/drm/msm/msm_gem_submit.c @@ -52,9 +52,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev, return ERR_PTR(ret); }

•   /* FIXME: this is way too early */
  

•   drm_sched_job_arm(&job->base);
  

•   xa_init_flags(&submit->deps, XA_FLAGS_ALLOC);
  
    kref_init(&submit->ref);
  

@@ -883,6 +880,9 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,

    submit->user_fence = dma_fence_get(&submit->base.s_fence->finished);

•   /* point of no return, we _have_ to submit no matter what */
  

•   drm_sched_job_arm(&submit->base);
  

•   /*
     * Allocate an id which can be used by WAIT_FENCE ioctl to map back
     * to the underlying fence.
  

@@ -892,17 +892,16 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, if (submit->fence_id < 0) { ret = submit->fence_id = 0; submit->fence_id = 0;

•           goto out;
    }
  
  

•   if (args->flags & MSM_SUBMIT_FENCE_FD_OUT) {
  

•   if (ret == 0 && args->flags & MSM_SUBMIT_FENCE_FD_OUT) {
            struct sync_file *sync_file = sync_file_create(submit->user_fence);
            if (!sync_file) {
                    ret = -ENOMEM;
  

•                   goto out;
  

•           } else {
  

•                   fd_install(out_fence_fd, sync_file->file);
  

•                   args->fence_fd = out_fence_fd;
            }
  

•           fd_install(out_fence_fd, sync_file->file);
  

•           args->fence_fd = out_fence_fd;
  

I wonder if instead we should (approximately) undo "drm/msm/submit: Simplify out-fence-fd handling" so that the point that it could fail is moved up ahead of the drm_sched_job_arm()?

Hm yeah. Up to you how you want to paint this shed, I think either is fine.

Also, does the dma_fence_get() work before drm_sched_job_arm()? From a quick look, it looks like it won't, but I'm still playing catchup and haven't had a chance to look at your entire series. If it doesn't work before drm_sched_job_arm(), then there is really no way to prevent a error path between the fence-init and job-submit.

Yes. I thought I've checked that I put the _arm() in the right spot, but I guess I screwed up and you need the fence before the point where I've put the job_arm()? And yes the error path cannot be avoided for out-fences, that's what I tried to explain in the commit message.

But, prior to your series, wouldn't a failure after drm_sched_job_init() but before the job is submitted just burn a fence-id, and otherwise carry on it's merry way?

Maybe? I'm not sure whether the scheduler gets confused about the gap and freak out abou that. I'm fairly new to that code and learning (which is part why I'm working on it). Since you look up in fences/syncobj after job_init() it should be pretty easy to whip up a testcase and see what happens. Also as long as nothing fails you won't see an issue, that's for sure. -Daniel

BR, -R

...

    }

    submit_attach_object_fences(submit);

-- 2.32.0

On Fri, Aug 6, 2021 at 7:15 PM Rob Clark robdclark@gmail.com wrote:

On Fri, Aug 6, 2021 at 9:42 AM Daniel Vetter daniel.vetter@ffwll.ch wrote:

...

On Fri, Aug 6, 2021 at 12:58 AM Rob Clark robdclark@gmail.com wrote:

...

On Thu, Aug 5, 2021 at 3:47 AM Daniel Vetter daniel.vetter@ffwll.ch wrote:

...

Originally drm_sched_job_init was the point of no return, after which drivers must submit a job. I've split that up, which allows us to fix this issue pretty easily.

Only thing we have to take care of is to not skip to error paths after that. Other drivers do this the same for out-fence and similar things.

Fixes: 1d8a5ca436ee ("drm/msm: Conversion to drm scheduler") Cc: Rob Clark robdclark@chromium.org Cc: Rob Clark robdclark@gmail.com Cc: Sean Paul sean@poorly.run Cc: Sumit Semwal sumit.semwal@linaro.org Cc: "Christian König" christian.koenig@amd.com Cc: linux-arm-msm@vger.kernel.org Cc: dri-devel@lists.freedesktop.org Cc: freedreno@lists.freedesktop.org Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org Signed-off-by: Daniel Vetter daniel.vetter@intel.com

---

drivers/gpu/drm/msm/msm_gem_submit.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c index 6d6c44f0e1f3..d0ed4ddc509e 100644 --- a/drivers/gpu/drm/msm/msm_gem_submit.c +++ b/drivers/gpu/drm/msm/msm_gem_submit.c @@ -52,9 +52,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev, return ERR_PTR(ret); }

•   /* FIXME: this is way too early */
  

•   drm_sched_job_arm(&job->base);
  

•   xa_init_flags(&submit->deps, XA_FLAGS_ALLOC);
  
    kref_init(&submit->ref);
  

@@ -883,6 +880,9 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,

    submit->user_fence = dma_fence_get(&submit->base.s_fence->finished);

•   /* point of no return, we _have_ to submit no matter what */
  

•   drm_sched_job_arm(&submit->base);
  

•   /*
     * Allocate an id which can be used by WAIT_FENCE ioctl to map back
     * to the underlying fence.
  

@@ -892,17 +892,16 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, if (submit->fence_id < 0) { ret = submit->fence_id = 0; submit->fence_id = 0;

•           goto out;
    }
  
  

•   if (args->flags & MSM_SUBMIT_FENCE_FD_OUT) {
  

•   if (ret == 0 && args->flags & MSM_SUBMIT_FENCE_FD_OUT) {
            struct sync_file *sync_file = sync_file_create(submit->user_fence);
            if (!sync_file) {
                    ret = -ENOMEM;
  

•                   goto out;
  

•           } else {
  

•                   fd_install(out_fence_fd, sync_file->file);
  

•                   args->fence_fd = out_fence_fd;
            }
  

•           fd_install(out_fence_fd, sync_file->file);
  

•           args->fence_fd = out_fence_fd;
  

I wonder if instead we should (approximately) undo "drm/msm/submit: Simplify out-fence-fd handling" so that the point that it could fail is moved up ahead of the drm_sched_job_arm()?

Hm yeah. Up to you how you want to paint this shed, I think either is fine.

...

Also, does the dma_fence_get() work before drm_sched_job_arm()? From a quick look, it looks like it won't, but I'm still playing catchup and haven't had a chance to look at your entire series. If it doesn't work before drm_sched_job_arm(), then there is really no way to prevent a error path between the fence-init and job-submit.

Yes. I thought I've checked that I put the _arm() in the right spot, but I guess I screwed up and you need the fence before the point where I've put the job_arm()? And yes the error path cannot be avoided for out-fences, that's what I tried to explain in the commit message.

...

But, prior to your series, wouldn't a failure after drm_sched_job_init() but before the job is submitted just burn a fence-id, and otherwise carry on it's merry way?

Maybe? I'm not sure whether the scheduler gets confused about the gap and freak out abou that. I'm fairly new to that code and learning (which is part why I'm working on it). Since you look up in fences/syncobj after job_init() it should be pretty easy to whip up a testcase and see what happens. Also as long as nothing fails you won't see an issue, that's for sure.

fair.. I'll try to come up with a test case.. pre-scheduler-conversion it wasn't a problem to fail after the fence seqno was allocated (well, I guess you might have problems if you had 2^31 failures in a row)

Yeah one thing drm/sched forces you to do is have a very clear notion about the point of no return in your submit ioctl. Which I think is a Very Good Thing, at least looking at i915 execbuf where the point of no return is a multi-stage thing with such interesting intermediate points like "we submit the ruquest but without actually running the batchbuffer". The downside is that the submit ioctl isn't perfectly transaction anymore, but I don't think that matters for tha tail stuff, which is generally just some out-fence installing. That generally never fails. -Daniel

BR, -R

...

-Daniel

...

BR, -R

...

    }

    submit_attach_object_fences(submit);

-- 2.32.0

-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

On Fri, Aug 6, 2021 at 8:57 PM Rob Clark robdclark@gmail.com wrote:

On Fri, Aug 6, 2021 at 11:41 AM Daniel Vetter daniel.vetter@ffwll.ch wrote:

...

On Fri, Aug 6, 2021 at 7:15 PM Rob Clark robdclark@gmail.com wrote:

...

On Fri, Aug 6, 2021 at 9:42 AM Daniel Vetter daniel.vetter@ffwll.ch wrote:

...

On Fri, Aug 6, 2021 at 12:58 AM Rob Clark robdclark@gmail.com wrote:

...

On Thu, Aug 5, 2021 at 3:47 AM Daniel Vetter daniel.vetter@ffwll.ch wrote:

...

Originally drm_sched_job_init was the point of no return, after which drivers must submit a job. I've split that up, which allows us to fix this issue pretty easily.

Only thing we have to take care of is to not skip to error paths after that. Other drivers do this the same for out-fence and similar things.

Fixes: 1d8a5ca436ee ("drm/msm: Conversion to drm scheduler") Cc: Rob Clark robdclark@chromium.org Cc: Rob Clark robdclark@gmail.com Cc: Sean Paul sean@poorly.run Cc: Sumit Semwal sumit.semwal@linaro.org Cc: "Christian König" christian.koenig@amd.com Cc: linux-arm-msm@vger.kernel.org Cc: dri-devel@lists.freedesktop.org Cc: freedreno@lists.freedesktop.org Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org Signed-off-by: Daniel Vetter daniel.vetter@intel.com

---

drivers/gpu/drm/msm/msm_gem_submit.c | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c index 6d6c44f0e1f3..d0ed4ddc509e 100644 --- a/drivers/gpu/drm/msm/msm_gem_submit.c +++ b/drivers/gpu/drm/msm/msm_gem_submit.c @@ -52,9 +52,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev, return ERR_PTR(ret); }

•   /* FIXME: this is way too early */
  

•   drm_sched_job_arm(&job->base);
  

•   xa_init_flags(&submit->deps, XA_FLAGS_ALLOC);
  
    kref_init(&submit->ref);
  

@@ -883,6 +880,9 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,

    submit->user_fence = dma_fence_get(&submit->base.s_fence->finished);

•   /* point of no return, we _have_ to submit no matter what */
  

•   drm_sched_job_arm(&submit->base);
  

•   /*
     * Allocate an id which can be used by WAIT_FENCE ioctl to map back
     * to the underlying fence.
  

@@ -892,17 +892,16 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, if (submit->fence_id < 0) { ret = submit->fence_id = 0; submit->fence_id = 0;

•           goto out;
    }
  
  

•   if (args->flags & MSM_SUBMIT_FENCE_FD_OUT) {
  

•   if (ret == 0 && args->flags & MSM_SUBMIT_FENCE_FD_OUT) {
            struct sync_file *sync_file = sync_file_create(submit->user_fence);
            if (!sync_file) {
                    ret = -ENOMEM;
  

•                   goto out;
  

•           } else {
  

•                   fd_install(out_fence_fd, sync_file->file);
  

•                   args->fence_fd = out_fence_fd;
            }
  

•           fd_install(out_fence_fd, sync_file->file);
  

•           args->fence_fd = out_fence_fd;
  

I wonder if instead we should (approximately) undo "drm/msm/submit: Simplify out-fence-fd handling" so that the point that it could fail is moved up ahead of the drm_sched_job_arm()?

Hm yeah. Up to you how you want to paint this shed, I think either is fine.

...

Also, does the dma_fence_get() work before drm_sched_job_arm()? From a quick look, it looks like it won't, but I'm still playing catchup and haven't had a chance to look at your entire series. If it doesn't work before drm_sched_job_arm(), then there is really no way to prevent a error path between the fence-init and job-submit.

Yes. I thought I've checked that I put the _arm() in the right spot, but I guess I screwed up and you need the fence before the point where I've put the job_arm()? And yes the error path cannot be avoided for out-fences, that's what I tried to explain in the commit message.

...

But, prior to your series, wouldn't a failure after drm_sched_job_init() but before the job is submitted just burn a fence-id, and otherwise carry on it's merry way?

Maybe? I'm not sure whether the scheduler gets confused about the gap and freak out abou that. I'm fairly new to that code and learning (which is part why I'm working on it). Since you look up in fences/syncobj after job_init() it should be pretty easy to whip up a testcase and see what happens. Also as long as nothing fails you won't see an issue, that's for sure.

fair.. I'll try to come up with a test case.. pre-scheduler-conversion it wasn't a problem to fail after the fence seqno was allocated (well, I guess you might have problems if you had 2^31 failures in a row)

Yeah one thing drm/sched forces you to do is have a very clear notion about the point of no return in your submit ioctl. Which I think is a Very Good Thing, at least looking at i915 execbuf where the point of no return is a multi-stage thing with such interesting intermediate points like "we submit the ruquest but without actually running the batchbuffer". The downside is that the submit ioctl isn't perfectly transaction anymore, but I don't think that matters for tha tail stuff, which is generally just some out-fence installing. That generally never fails.

So I hacked up:

---

diff --git a/drivers/gpu/drm/scheduler/sched_fence.c b/drivers/gpu/drm/scheduler/sched_fence.c index 3aa6351d2101..88e66dbc9515 100644 --- a/drivers/gpu/drm/scheduler/sched_fence.c +++ b/drivers/gpu/drm/scheduler/sched_fence.c @@ -176,6 +176,7 @@ struct drm_sched_fence *drm_sched_fence_create(struct drm_sched_entity *entity, fence->sched = entity->rq->sched; spin_lock_init(&fence->lock);

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

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index fcc601962e92..583e85adbbe0 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -593,6 +593,7 @@ int drm_sched_job_init(struct drm_sched_job *job, if (!job->s_fence) return -ENOMEM; job->id = atomic64_inc_return(&sched->job_id_count);

•   job->id = atomic64_inc_return(&sched->job_id_count);
  
    INIT_LIST_HEAD(&job->list);
  
  

---

(I guess the job->id part shouldn't really be needed, that looks like it is only used by amdgpu)

This didn't cause any problems that I could see. So I don't *think* a failure after drm_sched_job_init() is really problematic, as long as things are serialized between drm_sched_job_init() and drm_sched_entity_push_job().

I also noticed that in the atomic commit path, the out-fences are initialized before atomic-check.. so there should be plenty of precedent for skipping fence seqno's.

Oh I think I remember now. The reason why the split into init/arm is so that you can keep your critical section only around job_arm() and push_job(). My very first version just pulled the jobs_init() of that for most drivers to where I needed it, and that would result in a bit chaos because the fences would signal out of order potentially. But yeah I guess bailing out is fine with the scheduler.

Do you want me to tune down the commit message a bit, it's not a must to submit the job, but just makes a bit more sense than bailing out with a fence seqno reserved? -Daniel

Originally drm_sched_job_init was the point of no return, after which drivers really should submit a job. I've split that up, which allows us to fix this issue pretty easily.

Only thing we have to take care of is to not skip to error paths after that. Other drivers do this the same for out-fence and similar things.

v2: It's not really a bugfix, just an improvement, since all drm_sched_job_arm does is reserve the fence number. And gaps should be fine, as long as the drm_sched_job doesn't escape anywhere at all.

For robustness it's still better to align with other drivers here and not bail out after job_arm().

Cc: Rob Clark robdclark@chromium.org Cc: Rob Clark robdclark@gmail.com Cc: Sean Paul sean@poorly.run Cc: Sumit Semwal sumit.semwal@linaro.org Cc: "Christian König" christian.koenig@amd.com Cc: linux-arm-msm@vger.kernel.org Cc: dri-devel@lists.freedesktop.org Cc: freedreno@lists.freedesktop.org Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org Signed-off-by: Daniel Vetter daniel.vetter@intel.com --- drivers/gpu/drm/msm/msm_gem_submit.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c index 4d1c4d5f6a2a..371ed9154e58 100644 --- a/drivers/gpu/drm/msm/msm_gem_submit.c +++ b/drivers/gpu/drm/msm/msm_gem_submit.c @@ -52,8 +52,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev, return ERR_PTR(ret); }

- drm_sched_job_arm(&job->base); - xa_init_flags(&submit->deps, XA_FLAGS_ALLOC);

kref_init(&submit->ref); @@ -882,6 +880,8 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,

submit->user_fence = dma_fence_get(&submit->base.s_fence->finished);

+ drm_sched_job_arm(&submit->base); + /* * Allocate an id which can be used by WAIT_FENCE ioctl to map back * to the underlying fence. @@ -891,17 +891,16 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, if (submit->fence_id < 0) { ret = submit->fence_id = 0; submit->fence_id = 0; - goto out; }

- if (args->flags & MSM_SUBMIT_FENCE_FD_OUT) { + if (ret == 0 && args->flags & MSM_SUBMIT_FENCE_FD_OUT) { struct sync_file *sync_file = sync_file_create(submit->user_fence); if (!sync_file) { ret = -ENOMEM; - goto out; + } else { + fd_install(out_fence_fd, sync_file->file); + args->fence_fd = out_fence_fd; } - fd_install(out_fence_fd, sync_file->file); - args->fence_fd = out_fence_fd; }

submit_attach_object_fences(submit);

On Thu, Aug 26, 2021 at 2:33 AM Daniel Vetter daniel.vetter@ffwll.ch wrote:

Originally drm_sched_job_init was the point of no return, after which drivers really should submit a job. I've split that up, which allows us to fix this issue pretty easily.

Only thing we have to take care of is to not skip to error paths after that. Other drivers do this the same for out-fence and similar things.

v2: It's not really a bugfix, just an improvement, since all drm_sched_job_arm does is reserve the fence number. And gaps should be fine, as long as the drm_sched_job doesn't escape anywhere at all.

For robustness it's still better to align with other drivers here and not bail out after job_arm().

v3: I misplaced drm_sched_job_arm by _one_ line! Thanks to Rob for testing and debug help.

Cc: Rob Clark robdclark@chromium.org Cc: Rob Clark robdclark@gmail.com Cc: Sean Paul sean@poorly.run Cc: Sumit Semwal sumit.semwal@linaro.org Cc: "Christian König" christian.koenig@amd.com Cc: linux-arm-msm@vger.kernel.org Cc: dri-devel@lists.freedesktop.org Cc: freedreno@lists.freedesktop.org Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org Signed-off-by: Daniel Vetter daniel.vetter@intel.com

t-b && r-b

BR, -R

---

drivers/gpu/drm/msm/msm_gem_submit.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c index 4d1c4d5f6a2a..71b8c8f752a3 100644 --- a/drivers/gpu/drm/msm/msm_gem_submit.c +++ b/drivers/gpu/drm/msm/msm_gem_submit.c @@ -52,8 +52,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev, return ERR_PTR(ret); }

•   drm_sched_job_arm(&job->base);
  

•   xa_init_flags(&submit->deps, XA_FLAGS_ALLOC);
  
    kref_init(&submit->ref);
  

@@ -880,6 +878,8 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data,

    submit->nr_cmds = i;

•   drm_sched_job_arm(&submit->base);
  

•   submit->user_fence = dma_fence_get(&submit->base.s_fence->finished);
  
    /*
  

@@ -891,17 +891,16 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, if (submit->fence_id < 0) { ret = submit->fence_id = 0; submit->fence_id = 0;

•           goto out;
    }
  
  

•   if (args->flags & MSM_SUBMIT_FENCE_FD_OUT) {
  

•   if (ret == 0 && args->flags & MSM_SUBMIT_FENCE_FD_OUT) {
            struct sync_file *sync_file = sync_file_create(submit->user_fence);
            if (!sync_file) {
                    ret = -ENOMEM;
  

•                   goto out;
  

•           } else {
  

•                   fd_install(out_fence_fd, sync_file->file);
  

•                   args->fence_fd = out_fence_fd;
            }
  

•           fd_install(out_fence_fd, sync_file->file);
  

•           args->fence_fd = out_fence_fd;
    }
  
    submit_attach_object_fences(submit);
  

-- 2.32.0

Am 05.08.21 um 12:46 schrieb Daniel Vetter:

It might be good enough on x86 with just READ_ONCE, but the write side should then at least be WRITE_ONCE because x86 has total store order.

It's definitely not enough on arm.

Fix this proplery, which means

• explain the need for the barrier in both places
• point at the other side in each comment

Also pull out the !sched_list case as the first check, so that the code flow is clearer.

While at it sprinkle some comments around because it was very non-obvious to me what's actually going on here and why.

Note that we really need full barriers here, at first I thought store-release and load-acquire on ->last_scheduled would be enough, but we actually requiring ordering between that and the queue state.

v2: Put smp_rmp() in the right place and fix up comment (Andrey)

Acked-by: Melissa Wen mwen@igalia.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: "Christian König" christian.koenig@amd.com Cc: Steven Price steven.price@arm.com Cc: Daniel Vetter daniel.vetter@ffwll.ch Cc: Andrey Grodzovsky andrey.grodzovsky@amd.com Cc: Lee Jones lee.jones@linaro.org Cc: Boris Brezillon boris.brezillon@collabora.com

Reviewed-by: Christian König christian.koenig@amd.com

---

drivers/gpu/drm/scheduler/sched_entity.c | 27 ++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index f7347c284886..89e3f6eaf519 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -439,8 +439,16 @@ struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity) dma_fence_set_error(&sched_job->s_fence->finished, -ECANCELED);

dma_fence_put(entity->last_scheduled);

• entity->last_scheduled = dma_fence_get(&sched_job->s_fence->finished);

• /*

• * If the queue is empty we allow drm_sched_entity_select_rq() to
  

• * locklessly access ->last_scheduled. This only works if we set the
  

• * pointer before we dequeue and if we a write barrier here.
  

• */
  

• smp_wmb();

• spsc_queue_pop(&entity->job_queue); return sched_job; }

@@ -459,10 +467,25 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity) struct drm_gpu_scheduler *sched; struct drm_sched_rq *rq;

• if (spsc_queue_count(&entity->job_queue) || !entity->sched_list)

• /* single possible engine and already selected */
• if (!entity->sched_list)

• return;
  

• /* queue non-empty, stay on the same engine */
• if (spsc_queue_count(&entity->job_queue)) return;

• fence = READ_ONCE(entity->last_scheduled);

• /*

• * Only when the queue is empty are we guaranteed that the scheduler
  

• * thread cannot change ->last_scheduled. To enforce ordering we need
  

• * a read barrier here. See drm_sched_entity_pop_job() for the other
  

• * side.
  

• */
  

• smp_rmb();
• fence = entity->last_scheduled;
• /* stay on the same engine if the previous job hasn't finished */ if (fence && !dma_fence_is_signaled(fence)) return;

Am 05.08.21 um 12:46 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.

v2/3: Rebased.

v4: Repaint this shed. The functions are now called _add_dependency() and _add_implicit_dependency()

Reviewed-by: Boris Brezillon boris.brezillon@collabora.com (v3) Reviewed-by: Steven Price steven.price@arm.com (v1) Acked-by: Melissa Wen mwen@igalia.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com

Reviewed-by: Christian König christian.koenig@amd.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: 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 | 104 +++++++++++++++++++++++ include/drm/gpu_scheduler.h | 33 ++++++- 3 files changed, 149 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index 89e3f6eaf519..381fbf462ea7 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 454cb6164bdc..f77456929139 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);

@@ -637,6 +639,99 @@ void drm_sched_job_arm(struct drm_sched_job *job) } EXPORT_SYMBOL(drm_sched_job_arm);

+/**

• • drm_sched_job_add_dependency - 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_add_dependency(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_add_dependency);

+/**

• • drm_sched_job_add_implicit_dependencies - 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_add_implicit_dependencies(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_add_dependency(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_add_dependency(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_add_implicit_dependencies);

• /**
  • drm_sched_job_cleanup - clean up scheduler job resources
  • @job: scheduler job to clean up

@@ -652,6 +747,9 @@ EXPORT_SYMBOL(drm_sched_job_arm); */ void drm_sched_job_cleanup(struct drm_sched_job *job) {

• struct dma_fence *fence;
• unsigned long index;
• if (kref_read(&job->s_fence->finished.refcount)) { /* drm_sched_job_arm() has been called */ dma_fence_put(&job->s_fence->finished);

@@ -661,6 +759,12 @@ void drm_sched_job_cleanup(struct drm_sched_job *job) }

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 83afc3aa8e2f..a47946f904b6 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,17 @@ 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_add_dependency() and
  

• * drm_sched_job_add_implicit_dependencies().
  

• */
  

• 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 +234,15 @@ 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_add_dependency() and
  

• * drm_sched_job_add_implicit_dependencies() this can be ommitted and
  

• * left as NULL.
  

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

@@ -349,6 +369,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_add_dependency(struct drm_sched_job *job,

• 		 struct dma_fence *fence);
  

+int drm_sched_job_add_implicit_dependencies(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);

Am 05.08.21 um 12:46 schrieb Daniel Vetter:

Originally a job was only bound to the queue when we pushed this, but now that's done in drm_sched_job_init, making that parameter entirely redundant.

Remove it.

The same applies to the context parameter in lima_sched_context_queue_task, simplify that too.

v2: Rebase on top of msm adopting drm/sched

Acked-by: Emma Anholt emma@anholt.net Acked-by: Melissa Wen mwen@igalia.com Reviewed-by: Steven Price steven.price@arm.com (v1) Reviewed-by: Boris Brezillon boris.brezillon@collabora.com (v1) Signed-off-by: Daniel Vetter daniel.vetter@intel.com

Reviewed-by: Christian König christian.koenig@amd.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: Emma Anholt emma@anholt.net 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: Alex Deucher alexander.deucher@amd.com Cc: Nirmoy Das nirmoy.das@amd.com Cc: Dave Airlie airlied@redhat.com Cc: Chen Li chenli@uniontech.com Cc: Lee Jones lee.jones@linaro.org Cc: Deepak R Varma mh12gx2825@gmail.com Cc: Kevin Wang kevin1.wang@amd.com Cc: Luben Tuikov luben.tuikov@amd.com Cc: "Marek Olšák" marek.olsak@amd.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Andrey Grodzovsky andrey.grodzovsky@amd.com Cc: Dennis Li Dennis.Li@amd.com Cc: Boris Brezillon boris.brezillon@collabora.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: Rob Clark robdclark@gmail.com Cc: Sean Paul sean@poorly.run Cc: Melissa Wen mwen@igalia.com Cc: linux-arm-msm@vger.kernel.org Cc: freedreno@lists.freedesktop.org

---

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_gem.c | 3 +-- drivers/gpu/drm/lima/lima_sched.c | 5 ++--- drivers/gpu/drm/lima/lima_sched.h | 3 +-- drivers/gpu/drm/msm/msm_gem_submit.c | 2 +- drivers/gpu/drm/panfrost/panfrost_job.c | 2 +- drivers/gpu/drm/scheduler/sched_entity.c | 6 ++---- drivers/gpu/drm/v3d/v3d_gem.c | 2 +- include/drm/gpu_scheduler.h | 3 +-- 11 files changed, 13 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c index 32e80bc6af22..1d8a914108af 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c @@ -1267,7 +1267,7 @@ static int amdgpu_cs_submit(struct amdgpu_cs_parser *p,

trace_amdgpu_cs_ioctl(job); amdgpu_vm_bo_trace_cs(&fpriv->vm, &p->ticket);

• drm_sched_entity_push_job(&job->base, entity);

• drm_sched_entity_push_job(&job->base);

  amdgpu_vm_move_to_lru_tail(p->adev, &fpriv->vm);

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c index 5ddb955d2315..b8609cccc9c1 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_job.c @@ -174,7 +174,7 @@ int amdgpu_job_submit(struct amdgpu_job *job, struct drm_sched_entity *entity,

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

• drm_sched_entity_push_job(&job->base, entity);

• drm_sched_entity_push_job(&job->base);

  return 0; }

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c b/drivers/gpu/drm/etnaviv/etnaviv_sched.c index 05f412204118..180bb633d5c5 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c @@ -178,7 +178,7 @@ int etnaviv_sched_push_job(struct drm_sched_entity *sched_entity, /* the scheduler holds on to the job now */ kref_get(&submit->refcount);

• drm_sched_entity_push_job(&submit->sched_job, sched_entity);

• drm_sched_entity_push_job(&submit->sched_job);

  out_unlock: mutex_unlock(&submit->gpu->fence_lock);

diff --git a/drivers/gpu/drm/lima/lima_gem.c b/drivers/gpu/drm/lima/lima_gem.c index de62966243cd..c528f40981bb 100644 --- a/drivers/gpu/drm/lima/lima_gem.c +++ b/drivers/gpu/drm/lima/lima_gem.c @@ -359,8 +359,7 @@ int lima_gem_submit(struct drm_file *file, struct lima_submit *submit) goto err_out2; }

• fence = lima_sched_context_queue_task(

• submit->ctx->context + submit->pipe, submit->task);
  

• fence = lima_sched_context_queue_task(submit->task);

  for (i = 0; i < submit->nr_bos; i++) { if (submit->bos[i].flags & LIMA_SUBMIT_BO_WRITE)

diff --git a/drivers/gpu/drm/lima/lima_sched.c b/drivers/gpu/drm/lima/lima_sched.c index 38f755580507..e968b5a8f0b0 100644 --- a/drivers/gpu/drm/lima/lima_sched.c +++ b/drivers/gpu/drm/lima/lima_sched.c @@ -177,13 +177,12 @@ void lima_sched_context_fini(struct lima_sched_pipe *pipe, drm_sched_entity_fini(&context->base); }

-struct dma_fence *lima_sched_context_queue_task(struct lima_sched_context *context,

• 				struct lima_sched_task *task)
  

+struct dma_fence *lima_sched_context_queue_task(struct lima_sched_task *task) { struct dma_fence *fence = dma_fence_get(&task->base.s_fence->finished);

trace_lima_task_submit(task);

• drm_sched_entity_push_job(&task->base, &context->base);

• drm_sched_entity_push_job(&task->base); return fence; }

diff --git a/drivers/gpu/drm/lima/lima_sched.h b/drivers/gpu/drm/lima/lima_sched.h index 90f03c48ef4a..ac70006b0e26 100644 --- a/drivers/gpu/drm/lima/lima_sched.h +++ b/drivers/gpu/drm/lima/lima_sched.h @@ -98,8 +98,7 @@ int lima_sched_context_init(struct lima_sched_pipe *pipe, atomic_t *guilty); void lima_sched_context_fini(struct lima_sched_pipe *pipe, struct lima_sched_context *context); -struct dma_fence *lima_sched_context_queue_task(struct lima_sched_context *context,

• 				struct lima_sched_task *task);
  

+struct dma_fence *lima_sched_context_queue_task(struct lima_sched_task *task);

int lima_sched_pipe_init(struct lima_sched_pipe *pipe, const char *name); void lima_sched_pipe_fini(struct lima_sched_pipe *pipe); diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c index d0ed4ddc509e..96cea0ba4cfd 100644 --- a/drivers/gpu/drm/msm/msm_gem_submit.c +++ b/drivers/gpu/drm/msm/msm_gem_submit.c @@ -909,7 +909,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, /* The scheduler owns a ref now: */ msm_gem_submit_get(submit);

• drm_sched_entity_push_job(&submit->base, &queue->entity);

• drm_sched_entity_push_job(&submit->base);

  args->fence = submit->fence_id;

diff --git a/drivers/gpu/drm/panfrost/panfrost_job.c b/drivers/gpu/drm/panfrost/panfrost_job.c index 2992dc85325f..4bc962763e1f 100644 --- a/drivers/gpu/drm/panfrost/panfrost_job.c +++ b/drivers/gpu/drm/panfrost/panfrost_job.c @@ -301,7 +301,7 @@ int panfrost_job_push(struct panfrost_job *job)

kref_get(&job->refcount); /* put by scheduler job completion */

• drm_sched_entity_push_job(&job->base, entity);

• drm_sched_entity_push_job(&job->base);

  mutex_unlock(&pfdev->sched_lock);

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index 381fbf462ea7..e4d33db1eb45 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -516,9 +516,7 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)

/**

• drm_sched_entity_push_job - Submit a job to the entity's job queue

• • @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_arm()

@@ -526,9 +524,9 @@ void drm_sched_entity_select_rq(struct drm_sched_entity *entity)

• Returns 0 for success, negative error code otherwise.

*/ -void drm_sched_entity_push_job(struct drm_sched_job *sched_job,

• 	       struct drm_sched_entity *entity)
  

+void drm_sched_entity_push_job(struct drm_sched_job *sched_job) {

• struct drm_sched_entity *entity = sched_job->entity; bool first;

  trace_drm_sched_job(sched_job, entity);

diff --git a/drivers/gpu/drm/v3d/v3d_gem.c b/drivers/gpu/drm/v3d/v3d_gem.c index 2e808097b4d1..957228bef29c 100644 --- a/drivers/gpu/drm/v3d/v3d_gem.c +++ b/drivers/gpu/drm/v3d/v3d_gem.c @@ -487,7 +487,7 @@ v3d_push_job(struct v3d_file_priv *v3d_priv, /* put by scheduler job completion */ kref_get(&job->refcount);

• drm_sched_entity_push_job(&job->base, &v3d_priv->sched_entity[queue]);

• drm_sched_entity_push_job(&job->base);

  return 0; }

diff --git a/include/drm/gpu_scheduler.h b/include/drm/gpu_scheduler.h index a47946f904b6..b72f73b375a2 100644 --- a/include/drm/gpu_scheduler.h +++ b/include/drm/gpu_scheduler.h @@ -409,8 +409,7 @@ void drm_sched_entity_fini(struct drm_sched_entity *entity); void drm_sched_entity_destroy(struct drm_sched_entity *entity); void drm_sched_entity_select_rq(struct drm_sched_entity *entity); struct drm_sched_job *drm_sched_entity_pop_job(struct drm_sched_entity *entity); -void drm_sched_entity_push_job(struct drm_sched_job *sched_job,

• 	       struct drm_sched_entity *entity);
  

+void drm_sched_entity_push_job(struct drm_sched_job *sched_job); 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);

Thanks for the remind, indeed miss a lock. Patch is: Reviewed-by: Qiang Yu yuq825@gmail.com

Regards, Qiang

On Fri, Aug 13, 2021 at 3:28 AM Daniel Vetter daniel@ffwll.ch wrote:

On Thu, Aug 05, 2021 at 12:46:53PM +0200, Daniel Vetter wrote:

...

Nothing special going on here.

Aside reviewing the code, it seems like drm_sched_job_arm() should be moved into lima_sched_context_queue_task and put under some mutex together with drm_sched_push_job(). See the kerneldoc for drm_sched_push_job().

v2: Rebase over renamed functions to add dependencies.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Qiang Yu yuq825@gmail.com Cc: Sumit Semwal sumit.semwal@linaro.org Cc: "Christian König" christian.koenig@amd.com Cc: lima@lists.freedesktop.org Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org

Ping for an ack here please. Testing would be even better ofc. -Daniel

...

---

drivers/gpu/drm/lima/lima_gem.c | 6 ++++-- drivers/gpu/drm/lima/lima_sched.c | 21 --------------------- drivers/gpu/drm/lima/lima_sched.h | 3 --- 3 files changed, 4 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/lima/lima_gem.c b/drivers/gpu/drm/lima/lima_gem.c index c528f40981bb..640acc060467 100644 --- a/drivers/gpu/drm/lima/lima_gem.c +++ b/drivers/gpu/drm/lima/lima_gem.c @@ -267,7 +267,9 @@ static int lima_gem_sync_bo(struct lima_sched_task *task, struct lima_bo *bo, if (explicit) return 0;

• return drm_gem_fence_array_add_implicit(&task->deps, &bo->base.base, write);
  

• return drm_sched_job_add_implicit_dependencies(&task->base,
  

•                                                &bo->base.base,
  

•                                                write);
  

}

static int lima_gem_add_deps(struct drm_file *file, struct lima_submit *submit) @@ -285,7 +287,7 @@ static int lima_gem_add_deps(struct drm_file *file, struct lima_submit *submit) if (err) return err;

•         err = drm_gem_fence_array_add(&submit->task->deps, fence);
  

•         err = drm_sched_job_add_dependency(&submit->task->base, fence);
          if (err) {
                  dma_fence_put(fence);
                  return err;
  

diff --git a/drivers/gpu/drm/lima/lima_sched.c b/drivers/gpu/drm/lima/lima_sched.c index e968b5a8f0b0..99d5f6f1a882 100644 --- a/drivers/gpu/drm/lima/lima_sched.c +++ b/drivers/gpu/drm/lima/lima_sched.c @@ -134,24 +134,15 @@ int lima_sched_task_init(struct lima_sched_task *task, task->num_bos = num_bos; task->vm = lima_vm_get(vm);

• xa_init_flags(&task->deps, XA_FLAGS_ALLOC);
  

• return 0;
  

}

void lima_sched_task_fini(struct lima_sched_task *task) {

• struct dma_fence *fence;
  

• unsigned long index;
  int i;
  
  drm_sched_job_cleanup(&task->base);
  
  

• xa_for_each(&task->deps, index, fence) {
  

•         dma_fence_put(fence);
  

• }
  

• xa_destroy(&task->deps);
  

• if (task->bos) {
          for (i = 0; i < task->num_bos; i++)
                  drm_gem_object_put(&task->bos[i]->base.base);
  

@@ -186,17 +177,6 @@ struct dma_fence *lima_sched_context_queue_task(struct lima_sched_task *task) return fence; }

-static struct dma_fence *lima_sched_dependency(struct drm_sched_job *job,

•                                        struct drm_sched_entity *entity)
  

-{

• struct lima_sched_task *task = to_lima_task(job);
  

• if (!xa_empty(&task->deps))
  

•         return xa_erase(&task->deps, task->last_dep++);
  

• return NULL;
  

-}

static int lima_pm_busy(struct lima_device *ldev) { int ret; @@ -472,7 +452,6 @@ static void lima_sched_free_job(struct drm_sched_job *job) }

static const struct drm_sched_backend_ops lima_sched_ops = {

• .dependency = lima_sched_dependency,
  .run_job = lima_sched_run_job,
  .timedout_job = lima_sched_timedout_job,
  .free_job = lima_sched_free_job,
  

diff --git a/drivers/gpu/drm/lima/lima_sched.h b/drivers/gpu/drm/lima/lima_sched.h index ac70006b0e26..6a11764d87b3 100644 --- a/drivers/gpu/drm/lima/lima_sched.h +++ b/drivers/gpu/drm/lima/lima_sched.h @@ -23,9 +23,6 @@ struct lima_sched_task { struct lima_vm *vm; void *frame;

• struct xarray deps;
  

• unsigned long last_dep;
  

• struct lima_bo **bos;
  int num_bos;
  
  

-- 2.32.0

-- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch

On Thu, Aug 05, 2021 at 12:46:56PM +0200, Daniel Vetter wrote:

We need to pull the drm_sched_job_init much earlier, but that's very minor surgery.

v2: Actually fix up cleanup paths by calling drm_sched_job_init, which I wanted to to in the previous round (and did, for all other drivers). Spotted by Lucas.

v3: Rebase over renamed functions to add dependencies.

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: Sumit Semwal sumit.semwal@linaro.org Cc: "Christian König" christian.koenig@amd.com Cc: etnaviv@lists.freedesktop.org Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org

Ping for an ack&testing please. -Daniel

---

drivers/gpu/drm/etnaviv/etnaviv_gem.h | 5 +- drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c | 60 ++++++++++--------- drivers/gpu/drm/etnaviv/etnaviv_sched.c | 63 +------------------- drivers/gpu/drm/etnaviv/etnaviv_sched.h | 3 +- 4 files changed, 37 insertions(+), 94 deletions(-)

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem.h b/drivers/gpu/drm/etnaviv/etnaviv_gem.h index 98e60df882b6..63688e6e4580 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem.h +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem.h @@ -80,9 +80,6 @@ struct etnaviv_gem_submit_bo { u64 va; struct etnaviv_gem_object *obj; struct etnaviv_vram_mapping *mapping;

• struct dma_fence *excl;
• unsigned int nr_shared;
• struct dma_fence **shared;

};

/* Created per submit-ioctl, to track bo's and cmdstream bufs, etc, @@ -95,7 +92,7 @@ struct etnaviv_gem_submit { struct etnaviv_file_private *ctx; struct etnaviv_gpu *gpu; struct etnaviv_iommu_context *mmu_context, *prev_mmu_context;

• struct dma_fence *out_fence, *in_fence;

• struct dma_fence *out_fence; int out_fence_id; struct list_head node; /* GPU active submit list */ struct etnaviv_cmdbuf cmdbuf;

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c index 4dd7d9d541c0..e3d43678eb09 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_gem_submit.c @@ -188,16 +188,11 @@ static int submit_fence_sync(struct etnaviv_gem_submit *submit) if (submit->flags & ETNA_SUBMIT_NO_IMPLICIT) continue;

• if (bo->flags & ETNA_SUBMIT_BO_WRITE) {
  

• 	ret = dma_resv_get_fences(robj, &bo->excl,
  

• 				  &bo->nr_shared,
  

• 				  &bo->shared);
  

• 	if (ret)
  

• 		return ret;
  

• } else {
  

• 	bo->excl = dma_resv_get_excl_unlocked(robj);
  

• }
  

• ret = drm_sched_job_add_implicit_dependencies(&submit->sched_job,
  

• 					      &bo->obj->base,
  

• 					      bo->flags & ETNA_SUBMIT_BO_WRITE);
  

• if (ret)
  

• 	return ret;
  

  }

  return ret;

@@ -403,8 +398,6 @@ static void submit_cleanup(struct kref *kref)

wake_up_all(&submit->gpu->fence_event);

• if (submit->in_fence)

• dma_fence_put(submit->in_fence);
  

  if (submit->out_fence) { /* first remove from IDR, so fence can not be found anymore */ mutex_lock(&submit->gpu->fence_lock);

@@ -529,7 +522,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data, ret = etnaviv_cmdbuf_init(priv->cmdbuf_suballoc, &submit->cmdbuf, ALIGN(args->stream_size, 8) + 8); if (ret)

• goto err_submit_objects;
  

• goto err_submit_put;
  
  

  submit->ctx = file->driver_priv; etnaviv_iommu_context_get(submit->ctx->mmu);

@@ -537,51 +530,62 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data, submit->exec_state = args->exec_state; submit->flags = args->flags;

• ret = drm_sched_job_init(&submit->sched_job,

• 		 &ctx->sched_entity[args->pipe],
  

• 		 submit->ctx);
  

• if (ret)

• goto err_submit_put;
  

• ret = submit_lookup_objects(submit, file, bos, args->nr_bos); if (ret)

• goto err_submit_objects;
  

• goto err_submit_job;
  
  

  if ((priv->mmu_global->version != ETNAVIV_IOMMU_V2) && !etnaviv_cmd_validate_one(gpu, stream, args->stream_size / 4, relocs, args->nr_relocs)) { ret = -EINVAL;

• goto err_submit_objects;
  

• goto err_submit_job;
  

  }

  if (args->flags & ETNA_SUBMIT_FENCE_FD_IN) {

• submit->in_fence = sync_file_get_fence(args->fence_fd);
  

• if (!submit->in_fence) {
  

• struct dma_fence *in_fence = sync_file_get_fence(args->fence_fd);
  

• if (!in_fence) {
  ret = -EINVAL;
  

• 	goto err_submit_objects;
  

• 	goto err_submit_job;
  

  }

• ret = drm_sched_job_add_dependency(&submit->sched_job,
  

• 				   in_fence);
  

• if (ret)
  

• 	goto err_submit_job;
  

  }

  ret = submit_pin_objects(submit); if (ret)

• goto err_submit_objects;
  

• goto err_submit_job;
  
  

  ret = submit_reloc(submit, stream, args->stream_size / 4, relocs, args->nr_relocs); if (ret)

• goto err_submit_objects;
  

• goto err_submit_job;
  
  

  ret = submit_perfmon_validate(submit, args->exec_state, pmrs); if (ret)

• goto err_submit_objects;
  

• goto err_submit_job;
  
  

  memcpy(submit->cmdbuf.vaddr, stream, args->stream_size);

  ret = submit_lock_objects(submit, &ticket); if (ret)

• goto err_submit_objects;
  

• goto err_submit_job;
  
  

  ret = submit_fence_sync(submit); if (ret)

• goto err_submit_objects;
  

• goto err_submit_job;
  
  

• ret = etnaviv_sched_push_job(&ctx->sched_entity[args->pipe], submit);

• ret = etnaviv_sched_push_job(submit); if (ret)

• goto err_submit_objects;
  

• goto err_submit_job;
  
  

  submit_attach_object_fences(submit);

@@ -595,7 +599,7 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data, sync_file = sync_file_create(submit->out_fence); if (!sync_file) { ret = -ENOMEM;

• 	goto err_submit_objects;
  

• 	goto err_submit_job;
  

  } fd_install(out_fence_fd, sync_file->file); }

@@ -603,7 +607,9 @@ int etnaviv_ioctl_gem_submit(struct drm_device *dev, void *data, args->fence_fd = out_fence_fd; args->fence = submit->out_fence_id;

-err_submit_objects: +err_submit_job:

• drm_sched_job_cleanup(&submit->sched_job);

+err_submit_put: etnaviv_submit_put(submit);

err_submit_ww_acquire: diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.c b/drivers/gpu/drm/etnaviv/etnaviv_sched.c index 180bb633d5c5..2bbbd6ccc95e 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_sched.c +++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.c @@ -17,58 +17,6 @@ module_param_named(job_hang_limit, etnaviv_job_hang_limit, int , 0444); static int etnaviv_hw_jobs_limit = 4; module_param_named(hw_job_limit, etnaviv_hw_jobs_limit, int , 0444);

-static struct dma_fence * -etnaviv_sched_dependency(struct drm_sched_job *sched_job,

• 	 struct drm_sched_entity *entity)
  

-{

• struct etnaviv_gem_submit *submit = to_etnaviv_submit(sched_job);
• struct dma_fence *fence;
• int i;
• if (unlikely(submit->in_fence)) {

• fence = submit->in_fence;
  

• submit->in_fence = NULL;
  

• if (!dma_fence_is_signaled(fence))
  

• 	return fence;
  

• dma_fence_put(fence);
  

• }
• for (i = 0; i < submit->nr_bos; i++) {

• struct etnaviv_gem_submit_bo *bo = &submit->bos[i];
  

• int j;
  

• if (bo->excl) {
  

• 	fence = bo->excl;
  

• 	bo->excl = NULL;
  

• 	if (!dma_fence_is_signaled(fence))
  

• 		return fence;
  

• 	dma_fence_put(fence);
  

• }
  

• for (j = 0; j < bo->nr_shared; j++) {
  

• 	if (!bo->shared[j])
  

• 		continue;
  

• 	fence = bo->shared[j];
  

• 	bo->shared[j] = NULL;
  

• 	if (!dma_fence_is_signaled(fence))
  

• 		return fence;
  

• 	dma_fence_put(fence);
  

• }
  

• kfree(bo->shared);
  

• bo->nr_shared = 0;
  

• bo->shared = NULL;
  

• }
• return NULL;

-}

static struct dma_fence *etnaviv_sched_run_job(struct drm_sched_job *sched_job) { struct etnaviv_gem_submit *submit = to_etnaviv_submit(sched_job); @@ -140,29 +88,22 @@ static void etnaviv_sched_free_job(struct drm_sched_job *sched_job) }

static const struct drm_sched_backend_ops etnaviv_sched_ops = {

• .dependency = etnaviv_sched_dependency, .run_job = etnaviv_sched_run_job, .timedout_job = etnaviv_sched_timedout_job, .free_job = etnaviv_sched_free_job,

};

-int etnaviv_sched_push_job(struct drm_sched_entity *sched_entity,

• 	   struct etnaviv_gem_submit *submit)
  

+int etnaviv_sched_push_job(struct etnaviv_gem_submit *submit) { int ret = 0;

/* * Hold the fence lock across the whole operation to avoid jobs being * pushed out of order with regard to their sched fence seqnos as

• * allocated in drm_sched_job_init.
  

• * allocated in drm_sched_job_arm.
  

  */ mutex_lock(&submit->gpu->fence_lock);

• ret = drm_sched_job_init(&submit->sched_job, sched_entity,

• 		 submit->ctx);
  

• if (ret)

• goto out_unlock;
  

• drm_sched_job_arm(&submit->sched_job);

  submit->out_fence = dma_fence_get(&submit->sched_job.s_fence->finished);

diff --git a/drivers/gpu/drm/etnaviv/etnaviv_sched.h b/drivers/gpu/drm/etnaviv/etnaviv_sched.h index c0a6796e22c9..baebfa069afc 100644 --- a/drivers/gpu/drm/etnaviv/etnaviv_sched.h +++ b/drivers/gpu/drm/etnaviv/etnaviv_sched.h @@ -18,7 +18,6 @@ struct etnaviv_gem_submit *to_etnaviv_submit(struct drm_sched_job *sched_job)

int etnaviv_sched_init(struct etnaviv_gpu *gpu); void etnaviv_sched_fini(struct etnaviv_gpu *gpu); -int etnaviv_sched_push_job(struct drm_sched_entity *sched_entity,

• 	   struct etnaviv_gem_submit *submit);
  

+int etnaviv_sched_push_job(struct etnaviv_gem_submit *submit);

#endif /* __ETNAVIV_SCHED_H__ */

2.32.0

On Thu, Aug 05, 2021 at 12:46:57PM +0200, Daniel Vetter wrote:

drm_sched_job_init is already at the right place, so this boils down to deleting code.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Rob Clark robdclark@gmail.com Cc: Sean Paul sean@poorly.run Cc: Sumit Semwal sumit.semwal@linaro.org Cc: "Christian König" christian.koenig@amd.com Cc: linux-arm-msm@vger.kernel.org Cc: freedreno@lists.freedesktop.org Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org

Ping for ack&testing please. -Daniel

---

drivers/gpu/drm/msm/msm_gem.h | 5 ----- drivers/gpu/drm/msm/msm_gem_submit.c | 19 +++++-------------- drivers/gpu/drm/msm/msm_ringbuffer.c | 12 ------------ 3 files changed, 5 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h index f9e3ffb2309a..8bf0ac707fd7 100644 --- a/drivers/gpu/drm/msm/msm_gem.h +++ b/drivers/gpu/drm/msm/msm_gem.h @@ -312,11 +312,6 @@ struct msm_gem_submit { struct ww_acquire_ctx ticket; uint32_t seqno; /* Sequence number of the submit on the ring */

• /* Array of struct dma_fence * to block on before submitting this job.

• */
  

• struct xarray deps;
• unsigned long last_dep;
• /* Hw fence, which is created when the scheduler executes the job, and
  • is signaled when the hw finishes (via seqno write from cmdstream)
  */

diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c index 96cea0ba4cfd..fb5a2eab27a2 100644 --- a/drivers/gpu/drm/msm/msm_gem_submit.c +++ b/drivers/gpu/drm/msm/msm_gem_submit.c @@ -52,8 +52,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev, return ERR_PTR(ret); }

• xa_init_flags(&submit->deps, XA_FLAGS_ALLOC);
• kref_init(&submit->ref); submit->dev = dev; submit->aspace = queue->ctx->aspace;

@@ -72,8 +70,6 @@ void __msm_gem_submit_destroy(struct kref *kref) { struct msm_gem_submit *submit = container_of(kref, struct msm_gem_submit, ref);

• unsigned long index;

• struct dma_fence *fence; unsigned i;

  if (submit->fence_id) {

@@ -82,12 +78,6 @@ void __msm_gem_submit_destroy(struct kref *kref) mutex_unlock(&submit->queue->lock); }

• xa_for_each (&submit->deps, index, fence) {

• dma_fence_put(fence);
  

• }
• xa_destroy(&submit->deps);
• dma_fence_put(submit->user_fence); dma_fence_put(submit->hw_fence);

@@ -343,8 +333,9 @@ static int submit_fence_sync(struct msm_gem_submit *submit, bool no_implicit) if (no_implicit) continue;

• ret = drm_gem_fence_array_add_implicit(&submit->deps, obj,
  

• 	write);
  

• ret = drm_sched_job_add_implicit_dependencies(&submit->base,
  

• 					      obj,
  

• 					      write);
  

  if (ret) break; }

@@ -588,7 +579,7 @@ static struct drm_syncobj **msm_parse_deps(struct msm_gem_submit *submit, if (ret) break;

• ret = drm_gem_fence_array_add(&submit->deps, fence);
  

• ret = drm_sched_job_add_dependency(&submit->base, fence);
  

  if (ret) break;

@@ -798,7 +789,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, goto out_unlock; }

• ret = drm_gem_fence_array_add(&submit->deps, in_fence);
  

• ret = drm_sched_job_add_dependency(&submit->base, in_fence);
  

  if (ret) goto out_unlock; }

diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.c b/drivers/gpu/drm/msm/msm_ringbuffer.c index bd54c1412649..652b1dedd7c1 100644 --- a/drivers/gpu/drm/msm/msm_ringbuffer.c +++ b/drivers/gpu/drm/msm/msm_ringbuffer.c @@ -11,17 +11,6 @@ static uint num_hw_submissions = 8; MODULE_PARM_DESC(num_hw_submissions, "The max # of jobs to write into ringbuffer (default 8)"); module_param(num_hw_submissions, uint, 0600);

-static struct dma_fence *msm_job_dependency(struct drm_sched_job *job,

• struct drm_sched_entity *s_entity)
  

-{

• struct msm_gem_submit *submit = to_msm_submit(job);
• if (!xa_empty(&submit->deps))

• return xa_erase(&submit->deps, submit->last_dep++);
  

• return NULL;

-}

static struct dma_fence *msm_job_run(struct drm_sched_job *job) { struct msm_gem_submit *submit = to_msm_submit(job); @@ -52,7 +41,6 @@ static void msm_job_free(struct drm_sched_job *job) }

const struct drm_sched_backend_ops msm_sched_ops = {

• .dependency = msm_job_dependency, .run_job = msm_job_run, .free_job = msm_job_free

};

2.32.0

On Thu, Aug 05, 2021 at 12:46:57PM +0200, Daniel Vetter wrote:

drm_sched_job_init is already at the right place, so this boils down to deleting code.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Rob Clark robdclark@gmail.com Cc: Sean Paul sean@poorly.run Cc: Sumit Semwal sumit.semwal@linaro.org Cc: "Christian König" christian.koenig@amd.com Cc: linux-arm-msm@vger.kernel.org Cc: freedreno@lists.freedesktop.org Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org

Merged up to this patch, except for etnaviv. -Daniel

---

drivers/gpu/drm/msm/msm_gem.h | 5 ----- drivers/gpu/drm/msm/msm_gem_submit.c | 19 +++++-------------- drivers/gpu/drm/msm/msm_ringbuffer.c | 12 ------------ 3 files changed, 5 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/msm/msm_gem.h b/drivers/gpu/drm/msm/msm_gem.h index f9e3ffb2309a..8bf0ac707fd7 100644 --- a/drivers/gpu/drm/msm/msm_gem.h +++ b/drivers/gpu/drm/msm/msm_gem.h @@ -312,11 +312,6 @@ struct msm_gem_submit { struct ww_acquire_ctx ticket; uint32_t seqno; /* Sequence number of the submit on the ring */

• /* Array of struct dma_fence * to block on before submitting this job.

• */
  

• struct xarray deps;
• unsigned long last_dep;
• /* Hw fence, which is created when the scheduler executes the job, and
  • is signaled when the hw finishes (via seqno write from cmdstream)
  */

diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c index 96cea0ba4cfd..fb5a2eab27a2 100644 --- a/drivers/gpu/drm/msm/msm_gem_submit.c +++ b/drivers/gpu/drm/msm/msm_gem_submit.c @@ -52,8 +52,6 @@ static struct msm_gem_submit *submit_create(struct drm_device *dev, return ERR_PTR(ret); }

• xa_init_flags(&submit->deps, XA_FLAGS_ALLOC);
• kref_init(&submit->ref); submit->dev = dev; submit->aspace = queue->ctx->aspace;

@@ -72,8 +70,6 @@ void __msm_gem_submit_destroy(struct kref *kref) { struct msm_gem_submit *submit = container_of(kref, struct msm_gem_submit, ref);

• unsigned long index;

• struct dma_fence *fence; unsigned i;

  if (submit->fence_id) {

@@ -82,12 +78,6 @@ void __msm_gem_submit_destroy(struct kref *kref) mutex_unlock(&submit->queue->lock); }

• xa_for_each (&submit->deps, index, fence) {

• dma_fence_put(fence);
  

• }
• xa_destroy(&submit->deps);
• dma_fence_put(submit->user_fence); dma_fence_put(submit->hw_fence);

@@ -343,8 +333,9 @@ static int submit_fence_sync(struct msm_gem_submit *submit, bool no_implicit) if (no_implicit) continue;

• ret = drm_gem_fence_array_add_implicit(&submit->deps, obj,
  

• 	write);
  

• ret = drm_sched_job_add_implicit_dependencies(&submit->base,
  

• 					      obj,
  

• 					      write);
  

  if (ret) break; }

@@ -588,7 +579,7 @@ static struct drm_syncobj **msm_parse_deps(struct msm_gem_submit *submit, if (ret) break;

• ret = drm_gem_fence_array_add(&submit->deps, fence);
  

• ret = drm_sched_job_add_dependency(&submit->base, fence);
  

  if (ret) break;

@@ -798,7 +789,7 @@ int msm_ioctl_gem_submit(struct drm_device *dev, void *data, goto out_unlock; }

• ret = drm_gem_fence_array_add(&submit->deps, in_fence);
  

• ret = drm_sched_job_add_dependency(&submit->base, in_fence);
  

  if (ret) goto out_unlock; }

diff --git a/drivers/gpu/drm/msm/msm_ringbuffer.c b/drivers/gpu/drm/msm/msm_ringbuffer.c index bd54c1412649..652b1dedd7c1 100644 --- a/drivers/gpu/drm/msm/msm_ringbuffer.c +++ b/drivers/gpu/drm/msm/msm_ringbuffer.c @@ -11,17 +11,6 @@ static uint num_hw_submissions = 8; MODULE_PARM_DESC(num_hw_submissions, "The max # of jobs to write into ringbuffer (default 8)"); module_param(num_hw_submissions, uint, 0600);

-static struct dma_fence *msm_job_dependency(struct drm_sched_job *job,

• struct drm_sched_entity *s_entity)
  

-{

• struct msm_gem_submit *submit = to_msm_submit(job);
• if (!xa_empty(&submit->deps))

• return xa_erase(&submit->deps, submit->last_dep++);
  

• return NULL;

-}

static struct dma_fence *msm_job_run(struct drm_sched_job *job) { struct msm_gem_submit *submit = to_msm_submit(job); @@ -52,7 +41,6 @@ static void msm_job_free(struct drm_sched_job *job) }

const struct drm_sched_backend_ops msm_sched_ops = {

• .dependency = msm_job_dependency, .run_job = msm_job_run, .free_job = msm_job_free

};

2.32.0

On Thu, Aug 05, 2021 at 12:46:58PM +0200, Daniel Vetter wrote:

Integrated into the scheduler now and all users converted over.

Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Maarten Lankhorst maarten.lankhorst@linux.intel.com Cc: Maxime Ripard mripard@kernel.org Cc: Thomas Zimmermann tzimmermann@suse.de 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: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org

Some acks would be great here. -Daniel

---

drivers/gpu/drm/drm_gem.c | 96 --------------------------------------- include/drm/drm_gem.h | 5 -- 2 files changed, 101 deletions(-)

diff --git a/drivers/gpu/drm/drm_gem.c b/drivers/gpu/drm/drm_gem.c index 09c820045859..37e2e2820f08 100644 --- a/drivers/gpu/drm/drm_gem.c +++ b/drivers/gpu/drm/drm_gem.c @@ -1272,99 +1272,3 @@ drm_gem_unlock_reservations(struct drm_gem_object **objs, int count, ww_acquire_fini(acquire_ctx); } EXPORT_SYMBOL(drm_gem_unlock_reservations);

-/**

• • drm_gem_fence_array_add - Adds the fence to an array of fences to be
• • waited on, deduplicating fences from the same context.
• • @fence_array: array of dma_fence * for the job to block on.
• • @fence: the dma_fence to add to the list of dependencies.
• • This functions consumes the reference for @fence both on success and error
• • cases.
• • Returns:
• • 0 on success, or an error on failing to expand the array.
• */

-int drm_gem_fence_array_add(struct xarray *fence_array,

• 	    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(fence_array, index, entry) {

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

• 	continue;
  

• if (dma_fence_is_later(fence, entry)) {
  

• 	dma_fence_put(entry);
  

• 	xa_store(fence_array, index, fence, GFP_KERNEL);
  

• } else {
  

• 	dma_fence_put(fence);
  

• }
  

• return 0;
  

• }
• ret = xa_alloc(fence_array, &id, fence, xa_limit_32b, GFP_KERNEL);
• if (ret != 0)

• dma_fence_put(fence);
  

• return ret;

-} -EXPORT_SYMBOL(drm_gem_fence_array_add);

-/**

• • drm_gem_fence_array_add_implicit - Adds the implicit dependencies tracked
• • in the GEM object's reservation object to an array of dma_fences for use in
• • scheduling a rendering job.
• • 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.
• • @fence_array: array of dma_fence * for the job to block on.
• • @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).
• */

-int drm_gem_fence_array_add_implicit(struct xarray *fence_array,

• 		     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_gem_fence_array_add(fence_array, 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_gem_fence_array_add(fence_array, fences[i]);
  

• if (ret)
  

• 	break;
  

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

• dma_fence_put(fences[i]);
  

• kfree(fences);
• return ret;

-} -EXPORT_SYMBOL(drm_gem_fence_array_add_implicit); diff --git a/include/drm/drm_gem.h b/include/drm/drm_gem.h index 35e7f44c2a75..e55a767188af 100644 --- a/include/drm/drm_gem.h +++ b/include/drm/drm_gem.h @@ -407,11 +407,6 @@ int drm_gem_lock_reservations(struct drm_gem_object **objs, int count, struct ww_acquire_ctx *acquire_ctx); void drm_gem_unlock_reservations(struct drm_gem_object **objs, int count, struct ww_acquire_ctx *acquire_ctx); -int drm_gem_fence_array_add(struct xarray *fence_array,

• 	    struct dma_fence *fence);
  

-int drm_gem_fence_array_add_implicit(struct xarray *fence_array,

• 		     struct drm_gem_object *obj,
  

• 		     bool write);
  

int drm_gem_dumb_map_offset(struct drm_file *file, struct drm_device *dev, u32 handle, u64 *offset);

-- 2.32.0

Am 05.08.21 um 12:46 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.

This would mean that as-is amdgpu can't use the dependency helpers, at least not with the current approach amdgpu has for deciding whether a vm_flush is needed. Since amdgpu also has very special rules around implicit fencing it can't use those helpers either, and adding a drm_sched_job_await_fence_always or similar for amdgpu wouldn't be too onerous. That way the special case handling for amdgpu sticks even more out and we have higher chances that reviewers that go across all drivers wont miss it.

Well you should probably drop the sentence about the vm_flush, this is completely unrelated.

Additional to that I still don't think that this is a good idea. Dependency handling is something completely driver specific.

E.g. even when you have submitted jobs back to back they still might need a cache flush in between and that is not only for amdgpu like this.

What you can do is to optimize for while looking at the fences later on and then note that you have done so and what the last hw fence is you used instead.

Regards, Christian.

Reviewed-by: Lucas Stach l.stach@pengutronix.de Acked-by: Melissa Wen mwen@igalia.com 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

---

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 f77456929139..49e507f91ec0 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -660,6 +660,13 @@ int drm_sched_job_add_dependency(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.

Am 05.08.21 um 15:25 schrieb Daniel Vetter:

On Thu, Aug 5, 2021 at 3:18 PM Christian König christian.koenig@amd.com wrote:

...

Am 05.08.21 um 12:46 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.

This would mean that as-is amdgpu can't use the dependency helpers, at least not with the current approach amdgpu has for deciding whether a vm_flush is needed. Since amdgpu also has very special rules around implicit fencing it can't use those helpers either, and adding a drm_sched_job_await_fence_always or similar for amdgpu wouldn't be too onerous. That way the special case handling for amdgpu sticks even more out and we have higher chances that reviewers that go across all drivers wont miss it.

Well you should probably drop the sentence about the vm_flush, this is completely unrelated.

Additional to that I still don't think that this is a good idea. Dependency handling is something completely driver specific.

E.g. even when you have submitted jobs back to back they still might need a cache flush in between and that is not only for amdgpu like this.

What you can do is to optimize for while looking at the fences later on and then note that you have done so and what the last hw fence is you used instead.

Out of 6 drivers using drm/sched 5 can use this. When we get i915 over, that one will be added to the list. amdgpu can't use any of this anyway due to the vm_id allocation requirements, which is why I mention that. Also note that all the callbacks are still there, so you can just ignore this all and still build your own. Like amdgpu does.

The VMID allocation stuff is rather easy to handle, that's why I noted we should remove that sentence.

The problematic stuff is handling the cache flush and pipeline sync which you make impossible with this here.

So I'm not sure what exactly your object is, aside from "this doesn't fit for amdgpu", which a) I know b) the commit message explains c) doesn't actually hurt amdgpu in the slightest. And we still get the benefit that for most drivers it's a nice optimization.

Well exactly that's what I wanted to avoid. We still can use this in amdgpu even with the VMID allocation stuff and I still hope to do so.

Can't we add this as a wrapper or similar?

Christian.

-Daniel

...

Regards, Christian.

...

Reviewed-by: Lucas Stach l.stach@pengutronix.de Acked-by: Melissa Wen mwen@igalia.com 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

---

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 f77456929139..49e507f91ec0 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -660,6 +660,13 @@ int drm_sched_job_add_dependency(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, Aug 5, 2021 at 3:19 PM Christian König christian.koenig@amd.com wrote:

Am 05.08.21 um 12:47 schrieb Daniel Vetter:

...

You really need to hold the reservation here or all kinds of funny things can happen between grabbing the dependencies and inserting the new fences.

Acked-by: Melissa Wen mwen@igalia.com 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

The function name in the subject line should be updated, apart from that feel free to add my rb to this patch.

Fixed locally and r-b added, I think the later parts of this series will need to be resent anyway. Thanks for your review. -Daniel

Christian.

...

---

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

diff --git a/drivers/gpu/drm/scheduler/sched_main.c b/drivers/gpu/drm/scheduler/sched_main.c index 49e507f91ec0..1abb40b07324 100644 --- a/drivers/gpu/drm/scheduler/sched_main.c +++ b/drivers/gpu/drm/scheduler/sched_main.c @@ -715,6 +715,8 @@ int drm_sched_job_add_implicit_dependencies(struct drm_sched_job *job, struct dma_fence **fences; unsigned int i, fence_count;

• dma_resv_assert_held(obj->resv);
  

• if (!write) {
          struct dma_fence *fence = dma_resv_get_excl_unlocked(obj->resv);
  
  

On Thu, Aug 5, 2021 at 3:47 AM Daniel Vetter daniel.vetter@ffwll.ch wrote:

There's only one exclusive slot, and we must not break the ordering.

Adding a new exclusive fence drops all previous fences from the dma_resv. To avoid violating the signalling order we err on the side of over-synchronizing by waiting for the existing fences, even if userspace asked us to ignore them.

A better fix would be to us a dma_fence_chain or _array like e.g. amdgpu now uses, but

• msm has a synchronous dma_fence_wait for anything from another context, so doesn't seem to care much,
• and it probably makes sense to lift this into dma-resv.c code as a proper concept, so that drivers don't have to hack up their own solution each on their own.

v2: Improve commit message per Lucas' suggestion.

Cc: Lucas Stach l.stach@pengutronix.de Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Rob Clark robdclark@gmail.com Cc: Sean Paul sean@poorly.run Cc: linux-arm-msm@vger.kernel.org Cc: freedreno@lists.freedesktop.org

a-b

---

drivers/gpu/drm/msm/msm_gem_submit.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/msm_gem_submit.c b/drivers/gpu/drm/msm/msm_gem_submit.c index fb5a2eab27a2..66633dfd58a2 100644 --- a/drivers/gpu/drm/msm/msm_gem_submit.c +++ b/drivers/gpu/drm/msm/msm_gem_submit.c @@ -330,7 +330,8 @@ static int submit_fence_sync(struct msm_gem_submit *submit, bool no_implicit) return ret; }

•           if (no_implicit)
  

•           /* exclusive fences must be ordered */
  

•           if (no_implicit && !write)
                    continue;
  
            ret = drm_sched_job_add_implicit_dependencies(&submit->base,
  

-- 2.32.0

On Thu, Aug 05, 2021 at 12:47:05PM +0200, Daniel Vetter wrote:

Specifically document the new/clarified rules around how the shared fences do not have any ordering requirements against the exclusive fence.

But also document all the things a bit better, given how central struct dma_resv to dynamic buffer management the docs have been very inadequat.

• Lots more links to other pieces of the puzzle. Unfortunately ttm_buffer_object has no docs, so no links :-(

• Explain/complain a bit about dma_resv_locking_ctx(). I still don't like that one, but fixing the ttm call chains is going to be horrible. Plus we want to plug in real slowpath locking when we do that anyway.

• Main part of the patch is some actual docs for struct dma_resv.

Overall I think we still have a lot of bad naming in this area (e.g. dma_resv.fence is singular, but contains the multiple shared fences), but I think that's more indicative of how the semantics and rules are just not great.

Another thing that's real awkard is how chaining exclusive fences right now means direct dma_resv.exclusive_fence pointer access with an rcu_assign_pointer. Not so great either.

v2:

• Fix a pile of typos (Matt, Jason)
• Hammer it in that breaking the rules leads to use-after-free issues around dma-buf sharing (Christian)

Reviewed-by: Christian König christian.koenig@amd.com Cc: Jason Ekstrand jason@jlekstrand.net Cc: Matthew Auld matthew.auld@intel.com Reviewed-by: Matthew Auld matthew.auld@intel.com Signed-off-by: Daniel Vetter daniel.vetter@intel.com Cc: Sumit Semwal sumit.semwal@linaro.org Cc: "Christian König" christian.koenig@amd.com Cc: linux-media@vger.kernel.org Cc: linaro-mm-sig@lists.linaro.org

Also pushed to drm-misc-next. -Daniel

---

drivers/dma-buf/dma-resv.c | 24 ++++++--- include/linux/dma-buf.h | 7 +++ include/linux/dma-resv.h | 104 +++++++++++++++++++++++++++++++++++-- 3 files changed, 124 insertions(+), 11 deletions(-)

diff --git a/drivers/dma-buf/dma-resv.c b/drivers/dma-buf/dma-resv.c index e744fd87c63c..84fbe60629e3 100644 --- a/drivers/dma-buf/dma-resv.c +++ b/drivers/dma-buf/dma-resv.c @@ -48,6 +48,8 @@

• write operations) or N shared fences (read operations). The RCU
• mechanism is used to protect read access to fences from locked
• write-side updates.

• • See struct dma_resv for more details.
  */

DEFINE_WD_CLASS(reservation_ww_class); @@ -137,7 +139,11 @@ EXPORT_SYMBOL(dma_resv_fini);

• @num_fences: number of fences we want to add
• Should be called before dma_resv_add_shared_fence(). Must

• • be called with obj->lock held.

• • be called with @obj locked through dma_resv_lock().
• • Note that the preallocated slots need to be re-reserved if @obj is unlocked
• • at any time before calling dma_resv_add_shared_fence(). This is validated
• • when CONFIG_DEBUG_MUTEXES is enabled.
  • RETURNS
  • Zero for success, or -errno

@@ -234,8 +240,10 @@ EXPORT_SYMBOL(dma_resv_reset_shared_max);

• @obj: the reservation object
• @fence: the shared fence to add

• • Add a fence to a shared slot, obj->lock must be held, and

• • Add a fence to a shared slot, @obj must be locked with dma_resv_lock(), and
  • dma_resv_reserve_shared() has been called.
• • See also &dma_resv.fence for a discussion of the semantics.
  */

void dma_resv_add_shared_fence(struct dma_resv *obj, struct dma_fence *fence) { @@ -278,9 +286,11 @@ EXPORT_SYMBOL(dma_resv_add_shared_fence); /**

• dma_resv_add_excl_fence - Add an exclusive fence.
• @obj: the reservation object

• • @fence: the shared fence to add

• • @fence: the exclusive fence to add

• • Add a fence to the exclusive slot. The obj->lock must be held.

• • Add a fence to the exclusive slot. @obj must be locked with dma_resv_lock().
• • Note that this function replaces all fences attached to @obj, see also
• • &dma_resv.fence_excl for a discussion of the semantics.
  */

void dma_resv_add_excl_fence(struct dma_resv *obj, struct dma_fence *fence) { @@ -609,9 +619,11 @@ static inline int dma_resv_test_signaled_single(struct dma_fence *passed_fence)

• fence
• Callers are not required to hold specific locks, but maybe hold

• • dma_resv_lock() already

• • dma_resv_lock() already.
• • RETURNS

• • true if all fences signaled, else false

• • True if all fences signaled, else false.
  */

bool dma_resv_test_signaled(struct dma_resv *obj, bool test_all) { diff --git a/include/linux/dma-buf.h b/include/linux/dma-buf.h index 678b2006be78..fc62b5f9980c 100644 --- a/include/linux/dma-buf.h +++ b/include/linux/dma-buf.h @@ -420,6 +420,13 @@ struct dma_buf { * - Dynamic importers should set fences for any access that they can't * disable immediately from their &dma_buf_attach_ops.move_notify * callback.

• *
  

• * IMPORTANT:
  

• *
  

• * All drivers must obey the struct dma_resv rules, specifically the
  

• * rules for updating fences, see &dma_resv.fence_excl and
  

• * &dma_resv.fence. If these dependency rules are broken access tracking
  

• * can be lost resulting in use after free issues.
  

  */ struct dma_resv *resv;

diff --git a/include/linux/dma-resv.h b/include/linux/dma-resv.h index e1ca2080a1ff..9100dd3dc21f 100644 --- a/include/linux/dma-resv.h +++ b/include/linux/dma-resv.h @@ -62,16 +62,90 @@ struct dma_resv_list {

/**

• struct dma_resv - a reservation object manages fences for a buffer

• • @lock: update side lock
• • @seq: sequence count for managing RCU read-side synchronization
• • @fence_excl: the exclusive fence, if there is one currently
• • @fence: list of current shared fences

• • There are multiple uses for this, with sometimes slightly different rules in
• • how the fence slots are used.
• • One use is to synchronize cross-driver access to a struct dma_buf, either for
• • dynamic buffer management or just to handle implicit synchronization between
• • different users of the buffer in userspace. See &dma_buf.resv for a more
• • in-depth discussion.
• • The other major use is to manage access and locking within a driver in a
• • buffer based memory manager. struct ttm_buffer_object is the canonical
• • example here, since this is where reservation objects originated from. But
• • use in drivers is spreading and some drivers also manage struct
• • drm_gem_object with the same scheme.
  */

struct dma_resv {

• /**

• * @lock:
  

• *
  

• * Update side lock. Don't use directly, instead use the wrapper
  

• * functions like dma_resv_lock() and dma_resv_unlock().
  

• *
  

• * Drivers which use the reservation object to manage memory dynamically
  

• * also use this lock to protect buffer object state like placement,
  

• * allocation policies or throughout command submission.
  

• */
  

  struct ww_mutex lock;

• /**

• * @seq:
  

• *
  

• * Sequence count for managing RCU read-side synchronization, allows
  

• * read-only access to @fence_excl and @fence while ensuring we take a
  

• * consistent snapshot.
  

• */
  

  seqcount_ww_mutex_t seq;

• /**

• * @fence_excl:
  

• *
  

• * The exclusive fence, if there is one currently.
  

• *
  

• * There are two ways to update this fence:
  

• *
  

• * - First by calling dma_resv_add_excl_fence(), which replaces all
  

• *   fences attached to the reservation object. To guarantee that no
  

• *   fences are lost, this new fence must signal only after all previous
  

• *   fences, both shared and exclusive, have signalled. In some cases it
  

• *   is convenient to achieve that by attaching a struct dma_fence_array
  

• *   with all the new and old fences.
  

• *
  

• * - Alternatively the fence can be set directly, which leaves the
  

• *   shared fences unchanged. To guarantee that no fences are lost, this
  

• *   new fence must signal only after the previous exclusive fence has
  

• *   signalled. Since the shared fences are staying intact, it is not
  

• *   necessary to maintain any ordering against those. If semantically
  

• *   only a new access is added without actually treating the previous
  

• *   one as a dependency the exclusive fences can be strung together
  

• *   using struct dma_fence_chain.
  

• *
  

• * Note that actual semantics of what an exclusive or shared fence mean
  

• * is defined by the user, for reservation objects shared across drivers
  

• * see &dma_buf.resv.
  

• */
  

  struct dma_fence __rcu *fence_excl;

• /**

• * @fence:
  

• *
  

• * List of current shared fences.
  

• *
  

• * There are no ordering constraints of shared fences against the
  

• * exclusive fence slot. If a waiter needs to wait for all access, it
  

• * has to wait for both sets of fences to signal.
  

• *
  

• * A new fence is added by calling dma_resv_add_shared_fence(). Since
  

• * this often needs to be done past the point of no return in command
  

• * submission it cannot fail, and therefore sufficient slots need to be
  

• * reserved by calling dma_resv_reserve_shared().
  

• *
  

• * Note that actual semantics of what an exclusive or shared fence mean
  

• * is defined by the user, for reservation objects shared across drivers
  

• * see &dma_buf.resv.
  

• */
  

  struct dma_resv_list __rcu *fence;

};

@@ -98,6 +172,13 @@ static inline void dma_resv_reset_shared_max(struct dma_resv *obj) {}

• undefined order, a #ww_acquire_ctx is passed to unwind if a cycle
• is detected. See ww_mutex_lock() and ww_acquire_init(). A reservation
• object may be locked by itself by passing NULL as @ctx.

• • When a die situation is indicated by returning -EDEADLK all locks held by
• • @ctx must be unlocked and then dma_resv_lock_slow() called on @obj.
• • Unlocked by calling dma_resv_unlock().
• • See also dma_resv_lock_interruptible() for the interruptible variant.
  */

static inline int dma_resv_lock(struct dma_resv *obj, struct ww_acquire_ctx *ctx) @@ -119,6 +200,12 @@ static inline int dma_resv_lock(struct dma_resv *obj,

• undefined order, a #ww_acquire_ctx is passed to unwind if a cycle
• is detected. See ww_mutex_lock() and ww_acquire_init(). A reservation
• object may be locked by itself by passing NULL as @ctx.

• • When a die situation is indicated by returning -EDEADLK all locks held by
• • @ctx must be unlocked and then dma_resv_lock_slow_interruptible() called on
• • @obj.
• • Unlocked by calling dma_resv_unlock().
  */

static inline int dma_resv_lock_interruptible(struct dma_resv *obj, struct ww_acquire_ctx *ctx) @@ -134,6 +221,8 @@ static inline int dma_resv_lock_interruptible(struct dma_resv *obj,

• Acquires the reservation object after a die case. This function
• will sleep until the lock becomes available. See dma_resv_lock() as
• well.

• • See also dma_resv_lock_slow_interruptible() for the interruptible variant.
  */

static inline void dma_resv_lock_slow(struct dma_resv *obj, struct ww_acquire_ctx *ctx) @@ -167,7 +256,7 @@ static inline int dma_resv_lock_slow_interruptible(struct dma_resv *obj,

• if they overlap with a writer.
• Also note that since no context is provided, no deadlock protection is

• • possible.

• • possible, which is also not needed for a trylock.
  • Returns true if the lock was acquired, false otherwise.
  */

@@ -193,6 +282,11 @@ static inline bool dma_resv_is_locked(struct dma_resv *obj)

• Returns the context used to lock a reservation object or NULL if no context
• was used or the object is not locked at all.

• • WARNING: This interface is pretty horrible, but TTM needs it because it
• • doesn't pass the struct ww_acquire_ctx around in some very long callchains.
• • Everyone else just uses it to check whether they're holding a reservation or
• • not.
  */

static inline struct ww_acquire_ctx *dma_resv_locking_ctx(struct dma_resv *obj) { -- 2.32.0