I don't see another way to figure out if a ring is initialized if the hardware block might not be initialized.

Entities have been fixed up to handle num_rqs = 0.

Signed-off-by: Bas Nieuwenhuizen bas@basnieuwenhuizen.nl --- drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c index d85184b5b35c..30407e55593b 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c @@ -124,6 +124,7 @@ static int amdgpu_ctx_init(struct amdgpu_device *adev, struct amdgpu_ring *rings[AMDGPU_MAX_RINGS]; struct drm_sched_rq *rqs[AMDGPU_MAX_RINGS]; unsigned num_rings; + unsigned num_rqs = 0;

switch (i) { case AMDGPU_HW_IP_GFX: @@ -166,12 +167,16 @@ static int amdgpu_ctx_init(struct amdgpu_device *adev, break; }

- for (j = 0; j < num_rings; ++j) - rqs[j] = &rings[j]->sched.sched_rq[priority]; + for (j = 0; j < num_rings; ++j) { + if (rings[j]->adev) { + rqs[num_rqs++] = + &rings[j]->sched.sched_rq[priority]; + } + }

for (j = 0; j < amdgpu_ctx_num_entities[i]; ++j) r = drm_sched_entity_init(&ctx->entities[i][j].entity, - rqs, num_rings, &ctx->guilty); + rqs, num_rqs, &ctx->guilty); if (r) goto error_cleanup_entities; }

Otherwise we interpret the file private data as drm & amdgpu data while it might not be, possibly allowing one to get memory corruption.

Signed-off-by: Bas Nieuwenhuizen bas@basnieuwenhuizen.nl --- drivers/gpu/drm/amd/amdgpu/amdgpu.h | 2 ++ drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 16 ++++++++++++++++ drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c | 10 +++++++--- 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h index d67f8b1dfe80..17290cdb8ed8 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h @@ -411,6 +411,8 @@ struct amdgpu_fpriv { struct amdgpu_ctx_mgr ctx_mgr; };

+int amdgpu_file_to_fpriv(struct file *filp, struct amdgpu_fpriv **fpriv); + int amdgpu_ib_get(struct amdgpu_device *adev, struct amdgpu_vm *vm, unsigned size, struct amdgpu_ib *ib); void amdgpu_ib_free(struct amdgpu_device *adev, struct amdgpu_ib *ib, diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index c806f984bcc5..90a520034c89 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -1176,6 +1176,22 @@ static const struct file_operations amdgpu_driver_kms_fops = { #endif };

+int amdgpu_file_to_fpriv(struct file *filp, struct amdgpu_fpriv **fpriv) +{ + struct drm_file *file; + + if (!filp) + return -EINVAL; + + if (filp->f_op != &amdgpu_driver_kms_fops) { + return -EINVAL; + } + + file = filp->private_data; + *fpriv = file->driver_priv; + return 0; +} + static bool amdgpu_get_crtc_scanout_position(struct drm_device *dev, unsigned int pipe, bool in_vblank_irq, int *vpos, int *hpos, diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c index 1cafe8d83a4d..0b70410488b6 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sched.c @@ -54,16 +54,20 @@ static int amdgpu_sched_process_priority_override(struct amdgpu_device *adev, enum drm_sched_priority priority) { struct file *filp = fget(fd); - struct drm_file *file; struct amdgpu_fpriv *fpriv; struct amdgpu_ctx *ctx; uint32_t id; + int r;

if (!filp) return -EINVAL;

- file = filp->private_data; - fpriv = file->driver_priv; + r = amdgpu_file_to_fpriv(filp, &fpriv); + if (r) { + fput(filp); + return r; + } + idr_for_each_entry(&fpriv->ctx_mgr.ctx_handles, ctx, id) amdgpu_ctx_priority_override(ctx, priority);

Am 30.01.19 um 02:53 schrieb Bas Nieuwenhuizen:

Some blocks in amdgpu can have 0 rqs.

Job creation already fails with -ENOENT when entity->rq is NULL, so jobs cannot be pushed. Without a rq there is no scheduler to pop jobs, and rq selection already does the right thing with a list of length 0.

So the operations we need to fix are:

• Creation, do not set rq to rq_list[0] if the list can have length 0.
• Do not flush any jobs when there is no rq.
• On entity destruction handle the rq = NULL case.
• on set_priority, do not try to change the rq if it is NULL.

Signed-off-by: Bas Nieuwenhuizen bas@basnieuwenhuizen.nl

One minor comment on patch #2, apart from that the series is Reviewed-by: Christian König christian.koenig@amd.com.

I'm going to make the change on #2 and pick them up for inclusion in amd-staging-drm-next.

Thanks for the help, Christian.

---

drivers/gpu/drm/scheduler/sched_entity.c | 39 ++++++++++++++++-------- 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/scheduler/sched_entity.c b/drivers/gpu/drm/scheduler/sched_entity.c index 4463d3826ecb..8e31b6628d09 100644 --- a/drivers/gpu/drm/scheduler/sched_entity.c +++ b/drivers/gpu/drm/scheduler/sched_entity.c @@ -52,12 +52,12 @@ int drm_sched_entity_init(struct drm_sched_entity *entity, { int i;

• if (!(entity && rq_list && num_rq_list > 0 && rq_list[0]))

• if (!(entity && rq_list && (num_rq_list == 0 || rq_list[0]))) return -EINVAL;

  memset(entity, 0, sizeof(struct drm_sched_entity)); INIT_LIST_HEAD(&entity->list);

• entity->rq = rq_list[0];

• entity->rq = NULL; entity->guilty = guilty; entity->num_rq_list = num_rq_list; entity->rq_list = kcalloc(num_rq_list, sizeof(struct drm_sched_rq *),

@@ -67,6 +67,10 @@ int drm_sched_entity_init(struct drm_sched_entity *entity,

for (i = 0; i < num_rq_list; ++i) entity->rq_list[i] = rq_list[i];

• if (num_rq_list)

• entity->rq = rq_list[0];
  

• entity->last_scheduled = NULL;

  spin_lock_init(&entity->rq_lock);

@@ -165,6 +169,9 @@ long drm_sched_entity_flush(struct drm_sched_entity *entity, long timeout) struct task_struct *last_user; long ret = timeout;

• if (!entity->rq)

• return 0;
  

• sched = entity->rq->sched; /**
  • The client will not queue more IBs during this fini, consume existing

@@ -264,20 +271,24 @@ static void drm_sched_entity_kill_jobs(struct drm_sched_entity *entity) */ void drm_sched_entity_fini(struct drm_sched_entity *entity) {

• struct drm_gpu_scheduler *sched;

• struct drm_gpu_scheduler *sched = NULL;

• sched = entity->rq->sched;
• drm_sched_rq_remove_entity(entity->rq, entity);

• if (entity->rq) {

• sched = entity->rq->sched;
  

• drm_sched_rq_remove_entity(entity->rq, entity);
  

• }

  /* Consumption of existing IBs wasn't completed. Forcefully

  • remove them here.

  */ if (spsc_queue_peek(&entity->job_queue)) {

• /* Park the kernel for a moment to make sure it isn't processing
  

•  * our enity.
  

•  */
  

• kthread_park(sched->thread);
  

• kthread_unpark(sched->thread);
  

• if (sched) {
  

• 	/* Park the kernel for a moment to make sure it isn't processing
  

• 	 * our enity.
  

• 	 */
  

• 	kthread_park(sched->thread);
  

• 	kthread_unpark(sched->thread);
  

• }
  

  if (entity->dependency) { dma_fence_remove_callback(entity->dependency, &entity->cb);

@@ -362,9 +373,11 @@ void drm_sched_entity_set_priority(struct drm_sched_entity *entity, for (i = 0; i < entity->num_rq_list; ++i) drm_sched_entity_set_rq_priority(&entity->rq_list[i], priority);

• drm_sched_rq_remove_entity(entity->rq, entity);
• drm_sched_entity_set_rq_priority(&entity->rq, priority);
• drm_sched_rq_add_entity(entity->rq, entity);

• if (entity->rq) {

• drm_sched_rq_remove_entity(entity->rq, entity);
  

• drm_sched_entity_set_rq_priority(&entity->rq, priority);
  

• drm_sched_rq_add_entity(entity->rq, entity);
  

• }

  spin_unlock(&entity->rq_lock); }