[PATCH] drm/amdgpu: remove dummy codes

[PATCH] drm/amdgpu: remove dummy codes

[Flora Cui]

Change-Id: I3238a2520c9161b1c2f9cf176158bdeb9cb21cbb
Signed-off-by: Flora Cui <Flora.Cui@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;
-			}
-
 		}
 
 		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);
 
 	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(&params, 0, sizeof(params));
-	params.adev = adev;
-	params.src = src;
-
 	/* sync to everything on unmapping */
 	if (!(flags & AMDGPU_PTE_VALID))
 		owner = AMDGPU_FENCE_OWNER_UNDEFINED;
-- 
2.7.4

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: remove dummy codes

[Christian König]

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@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.

>   		}
>   
>   		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.

>   
>   	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(&params, 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: remove dummy codes

[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@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
> 
> >  		}
> >  		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(&params, 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: remove dummy codes

[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@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.

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(&params, 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@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH] drm/amdgpu: remove dummy codes

[Christian König]

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@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(&params, 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@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx