[PATCH v2 2/2] drm/amdgpu: Move old fence waiting before reservation lock is aquired.

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

 



After ctx mutex is added, pthread_mutext in libdrm can be removed now.

David Zhou


On 2017å¹´10æ??11æ?¥ 15:25, Christian König wrote:
> 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
>>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx



[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux