* [PATCH] drm/amdgpu: remove dummy codes
@ 2016-11-04 7:32 Flora Cui
[not found] ` <1478244748-15866-1-git-send-email-Flora.Cui-5C7GfCeVMHo@public.gmane.org>
0 siblings, 1 reply; 5+ messages in thread
From: Flora Cui @ 2016-11-04 7:32 UTC (permalink / raw)
To: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW; +Cc: 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(¶ms, 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
^ permalink raw reply related [flat|nested] 5+ messages in thread[parent not found: <1478244748-15866-1-git-send-email-Flora.Cui-5C7GfCeVMHo@public.gmane.org>]
* Re: [PATCH] drm/amdgpu: remove dummy codes [not found] ` <1478244748-15866-1-git-send-email-Flora.Cui-5C7GfCeVMHo@public.gmane.org> @ 2016-11-04 8:52 ` Christian König [not found] ` <d7965a02-31ba-cc14-0a37-a0debb216d49-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org> 0 siblings, 1 reply; 5+ messages in thread From: Christian König @ 2016-11-04 8:52 UTC (permalink / raw) To: Flora Cui, amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW 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(¶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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <d7965a02-31ba-cc14-0a37-a0debb216d49-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>]
* Re: [PATCH] drm/amdgpu: remove dummy codes [not found] ` <d7965a02-31ba-cc14-0a37-a0debb216d49-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org> @ 2016-11-04 9:22 ` Flora Cui 2016-11-04 10:49 ` Christian König 0 siblings, 1 reply; 5+ messages in thread From: Flora Cui @ 2016-11-04 9:22 UTC (permalink / raw) To: Christian König; +Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW 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(¶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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] drm/amdgpu: remove dummy codes 2016-11-04 9:22 ` Flora Cui @ 2016-11-04 10:49 ` Christian König [not found] ` <7418b59b-4582-10e6-4fca-dae089b2ecc4-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org> 0 siblings, 1 reply; 5+ messages in thread From: Christian König @ 2016-11-04 10:49 UTC (permalink / raw) To: Flora Cui; +Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW 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(¶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@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ^ permalink raw reply [flat|nested] 5+ messages in thread
[parent not found: <7418b59b-4582-10e6-4fca-dae089b2ecc4-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>]
* Re: [PATCH] drm/amdgpu: remove dummy codes [not found] ` <7418b59b-4582-10e6-4fca-dae089b2ecc4-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org> @ 2016-11-04 10:52 ` Christian König 0 siblings, 0 replies; 5+ messages in thread From: Christian König @ 2016-11-04 10:52 UTC (permalink / raw) To: Flora Cui; +Cc: amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW 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(¶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@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 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2016-11-04 10:52 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-11-04 7:32 [PATCH] drm/amdgpu: remove dummy codes Flora Cui
[not found] ` <1478244748-15866-1-git-send-email-Flora.Cui-5C7GfCeVMHo@public.gmane.org>
2016-11-04 8:52 ` Christian König
[not found] ` <d7965a02-31ba-cc14-0a37-a0debb216d49-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
2016-11-04 9:22 ` Flora Cui
2016-11-04 10:49 ` Christian König
[not found] ` <7418b59b-4582-10e6-4fca-dae089b2ecc4-ANTagKRnAhcb1SvskN2V4Q@public.gmane.org>
2016-11-04 10:52 ` Christian König
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.