Yes, the mutex is mandatory. As I explained before it doesn't matter what userspace is doing, the kernel IOCTL must always be thread safe. Otherwise userspace could force the kernel to run into a BUG_ON() or worse. Additional to that we already use an CS interface upstream which doesn't have a pthread_mutex any more. Regards, Christian. Am 11.10.2017 um 05:28 schrieb Liu, Monk: > Hi Andrey & Christian > > Do we really need the mutext lock here? > Libdrm_amdgpu already has the pthread_mutext to protect multi-thread racing issues, kernel side should be safe with that > > BR Monk > > -----Original Message----- > From: Andrey Grodzovsky [mailto:andrey.grodzovsky at amd.com] > Sent: Wednesday, October 11, 2017 4:50 AM > To: Koenig, Christian <Christian.Koenig at amd.com>; Liu, Monk <Monk.Liu at amd.com>; amd-gfx at lists.freedesktop.org > Cc: Grodzovsky, Andrey <Andrey.Grodzovsky at amd.com> > Subject: [PATCH v2 2/2] drm/amdgpu: Move old fence waiting before reservation lock is aquired. > > Helps avoiding deadlock during GPU reset. > Added mutex to amdgpu_ctx to preserve order of fences on a ring. > > v2: > Put waiting logic in a function in a seperate function in amdgpu_ctx.c > > Signed-off-by: Andrey Grodzovsky <andrey.grodzovsky at amd.com> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu.h | 4 ++++ > drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 8 ++++++-- drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c | 30 ++++++++++++++++++++++++------ > 3 files changed, 34 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu.h b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > index da48f97..235eca5 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu.h > @@ -741,6 +741,7 @@ struct amdgpu_ctx { > bool preamble_presented; > enum amd_sched_priority init_priority; > enum amd_sched_priority override_priority; > + struct mutex lock; > }; > > struct amdgpu_ctx_mgr { > @@ -763,9 +764,12 @@ void amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx, int amdgpu_ctx_ioctl(struct drm_device *dev, void *data, > struct drm_file *filp); > > +int amdgpu_ctx_wait_prev_fence(struct amdgpu_ctx *ctx, unsigned > +ring_id); > + > void amdgpu_ctx_mgr_init(struct amdgpu_ctx_mgr *mgr); void amdgpu_ctx_mgr_fini(struct amdgpu_ctx_mgr *mgr); > > + > /* > * file private structure > */ > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > index 1a54e53..c36297c 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c > @@ -90,6 +90,8 @@ static int amdgpu_cs_parser_init(struct amdgpu_cs_parser *p, void *data) > goto free_chunk; > } > > + mutex_lock(&p->ctx->lock); > + > /* get chunks */ > chunk_array_user = u64_to_user_ptr(cs->in.chunks); > if (copy_from_user(chunk_array, chunk_array_user, @@ -737,8 +739,10 @@ static void amdgpu_cs_parser_fini(struct amdgpu_cs_parser *parser, int error, > > dma_fence_put(parser->fence); > > - if (parser->ctx) > + if (parser->ctx) { > + mutex_unlock(&parser->ctx->lock); > amdgpu_ctx_put(parser->ctx); > + } > > if (parser->bo_list) > amdgpu_bo_list_put(parser->bo_list); > @@ -992,7 +996,7 @@ static int amdgpu_cs_ib_fill(struct amdgpu_device *adev, > parser->job->ring->funcs->type == AMDGPU_RING_TYPE_VCE)) > return -EINVAL; > > - return 0; > + return amdgpu_ctx_wait_prev_fence(parser->ctx, > +parser->job->ring->idx); > } > > static int amdgpu_cs_process_fence_dep(struct amdgpu_cs_parser *p, diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c > index a78b03f6..4309820 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ctx.c > @@ -67,6 +67,8 @@ static int amdgpu_ctx_init(struct amdgpu_device *adev, > if (!ctx->fences) > return -ENOMEM; > > + mutex_init(&ctx->lock); > + > for (i = 0; i < AMDGPU_MAX_RINGS; ++i) { > ctx->rings[i].sequence = 1; > ctx->rings[i].fences = &ctx->fences[amdgpu_sched_jobs * i]; @@ -126,6 +128,8 @@ static void amdgpu_ctx_fini(struct amdgpu_ctx *ctx) > &ctx->rings[i].entity); > > amdgpu_queue_mgr_fini(adev, &ctx->queue_mgr); > + > + mutex_destroy(&ctx->lock); > } > > static int amdgpu_ctx_alloc(struct amdgpu_device *adev, @@ -296,12 +300,8 @@ int amdgpu_ctx_add_fence(struct amdgpu_ctx *ctx, struct amdgpu_ring *ring, > > idx = seq & (amdgpu_sched_jobs - 1); > other = cring->fences[idx]; > - if (other) { > - signed long r; > - r = dma_fence_wait_timeout(other, true, MAX_SCHEDULE_TIMEOUT); > - if (r < 0) > - return r; > - } > + if (other) > + BUG_ON(!dma_fence_is_signaled(other)); > > dma_fence_get(fence); > > @@ -372,6 +372,24 @@ void amdgpu_ctx_priority_override(struct amdgpu_ctx *ctx, > } > } > > +int amdgpu_ctx_wait_prev_fence(struct amdgpu_ctx *ctx, unsigned > +ring_id) { > + struct amdgpu_ctx_ring *cring = &ctx->rings[ring_id]; > + unsigned idx = cring->sequence & (amdgpu_sched_jobs - 1); > + struct dma_fence *other = cring->fences[idx]; > + > + if (other) { > + signed long r; > + r = dma_fence_wait_timeout(other, false, MAX_SCHEDULE_TIMEOUT); > + if (r < 0) { > + DRM_ERROR("Error (%ld) waiting for fence!\n", r); > + return r; > + } > + } > + > + return 0; > +} > + > void amdgpu_ctx_mgr_init(struct amdgpu_ctx_mgr *mgr) { > mutex_init(&mgr->lock); > -- > 2.7.4 >
