Am 04.11.2016 um 11:49 schrieb Christian König:
> Am 04.11.2016 um 10:22 schrieb Flora Cui:
>> On Fri, Nov 04, 2016 at 09:52:16AM +0100, Christian König wrote:
>>> Well clearly a NAK on this, why do you think this is just dummy code?
>>>
>>> Am 04.11.2016 um 08:32 schrieb Flora Cui:
>>>> Change-Id: I3238a2520c9161b1c2f9cf176158bdeb9cb21cbb
>>>> Signed-off-by: Flora Cui <Flora.Cui at amd.com>
>>>> ---
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c | 15 +--------------
>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 4 ----
>>>> 2 files changed, 1 insertion(+), 18 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> index 4068504..be2ad79 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
>>>> @@ -917,13 +917,6 @@ static int amdgpu_cs_ib_fill(struct
>>>> amdgpu_device *adev,
>>>> memcpy(ib->ptr, kptr, chunk_ib->ib_bytes);
>>>> amdgpu_bo_kunmap(aobj);
>>>> - } else {
>>>> - r = amdgpu_ib_get(adev, vm, 0, ib);
>>>> - if (r) {
>>>> - DRM_ERROR("Failed to get ib !\n");
>>>> - return r;
>>>> - }
>>>> -
>>> This is the IB allocation and initialization for the VM case and
>>> vital to
>>> command submission.
>> flora: amdgpu_ib_get do nothing with param size=0
>
> That's clearly a bug. The function should at least clear the structure
> members.
On the other hand we zero clear the IB array now anyway.
So what you could do is remove the size check from amdgpu_ib_get() and
then drop this code as well.
Regards,
Christian.
>
>
> Christian.
>
>>>> }
>>>> ib->gpu_addr = chunk_ib->va_start;
>>>> @@ -1008,21 +1001,15 @@ static int amdgpu_cs_submit(struct
>>>> amdgpu_cs_parser *p,
>>>> job = p->job;
>>>> p->job = NULL;
>>>> - r = amd_sched_job_init(&job->base, &ring->sched, entity,
>>>> p->filp);
>>>> + r = amdgpu_job_submit(job, ring, entity, p->filp, &p->fence);
>>>> if (r) {
>>>> amdgpu_job_free(job);
>>>> return r;
>>>> }
>>>> - job->owner = p->filp;
>>>> - job->fence_ctx = entity->fence_context;
>>>> - p->fence = fence_get(&job->base.s_fence->finished);
>>>> cs->out.handle = amdgpu_ctx_add_fence(p->ctx, ring, p->fence);
>>>> job->uf_sequence = cs->out.handle;
>>>> - amdgpu_job_free_resources(job);
>>>> -
>>>> trace_amdgpu_cs_ioctl(job);
>>>> - amd_sched_entity_push_job(&job->base);
>>> This results in a race condition because the job might already be freed
>>> after amdgpu_job_submit() succeeded.
>> flora: OK. I'll drop this.
>>
>>>> return 0;
>>>> }
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> index d6c2839..c6448e1 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>> @@ -960,10 +960,6 @@ static int amdgpu_vm_bo_update_mapping(struct
>>>> amdgpu_device *adev,
>>>> ring = container_of(vm->entity.sched, struct amdgpu_ring,
>>>> sched);
>>>> - memset(¶ms, 0, sizeof(params));
>>>> - params.adev = adev;
>>>> - params.src = src;
>>>> -
>>> This indeed looks like a duplicate, probably a leftover merge issue.
>>>
>>> Regards,
>>> Christian.
>>>
>>>> /* sync to everything on unmapping */
>>>> if (!(flags & AMDGPU_PTE_VALID))
>>>> owner = AMDGPU_FENCE_OWNER_UNDEFINED;
>>>
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
