| Message ID | 1342188495-3705-3-git-send-email-deathsimple@vodafone.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, Jul 13, 2012 at 04:08:15PM +0200, Christian König wrote: > Const IBs are executed on the CE not the CP, so we can't > fence them in the normal way. > > So submit them directly before the IB instead, just as > the documentation says. > > Signed-off-by: Christian König <deathsimple@vodafone.de> > --- > drivers/gpu/drm/radeon/r100.c | 2 +- > drivers/gpu/drm/radeon/r600.c | 2 +- > drivers/gpu/drm/radeon/radeon.h | 3 ++- > drivers/gpu/drm/radeon/radeon_cs.c | 25 +++++++++++-------------- > drivers/gpu/drm/radeon/radeon_ring.c | 10 +++++++++- > 5 files changed, 24 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c > index e0f5ae8..4ee5a74 100644 > --- a/drivers/gpu/drm/radeon/r100.c > +++ b/drivers/gpu/drm/radeon/r100.c > @@ -3693,7 +3693,7 @@ int r100_ib_test(struct radeon_device *rdev, struct radeon_ring *ring) > ib.ptr[6] = PACKET2(0); > ib.ptr[7] = PACKET2(0); > ib.length_dw = 8; > - r = radeon_ib_schedule(rdev, &ib); > + r = radeon_ib_schedule(rdev, &ib, NULL); > if (r) { > radeon_scratch_free(rdev, scratch); > radeon_ib_free(rdev, &ib); > diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c > index 3156d25..c2e5069 100644 > --- a/drivers/gpu/drm/radeon/r600.c > +++ b/drivers/gpu/drm/radeon/r600.c > @@ -2619,7 +2619,7 @@ int r600_ib_test(struct radeon_device *rdev, struct radeon_ring *ring) > ib.ptr[1] = ((scratch - PACKET3_SET_CONFIG_REG_OFFSET) >> 2); > ib.ptr[2] = 0xDEADBEEF; > ib.length_dw = 3; > - r = radeon_ib_schedule(rdev, &ib); > + r = radeon_ib_schedule(rdev, &ib, NULL); > if (r) { > radeon_scratch_free(rdev, scratch); > radeon_ib_free(rdev, &ib); > diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h > index 2cb355b..2d7f06c 100644 > --- a/drivers/gpu/drm/radeon/radeon.h > +++ b/drivers/gpu/drm/radeon/radeon.h > @@ -751,7 +751,8 @@ struct si_rlc { > int radeon_ib_get(struct radeon_device *rdev, int ring, > struct radeon_ib *ib, unsigned size); > void radeon_ib_free(struct radeon_device *rdev, struct radeon_ib *ib); > -int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib); > +int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib, > + struct radeon_ib *const_ib); > int radeon_ib_pool_init(struct radeon_device *rdev); > void radeon_ib_pool_fini(struct radeon_device *rdev); > int radeon_ib_ring_tests(struct radeon_device *rdev); > diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c > index 553da67..d0be5d5 100644 > --- a/drivers/gpu/drm/radeon/radeon_cs.c > +++ b/drivers/gpu/drm/radeon/radeon_cs.c > @@ -354,7 +354,7 @@ static int radeon_cs_ib_chunk(struct radeon_device *rdev, > } > radeon_cs_sync_rings(parser); > parser->ib.vm_id = 0; > - r = radeon_ib_schedule(rdev, &parser->ib); > + r = radeon_ib_schedule(rdev, &parser->ib, NULL); > if (r) { > DRM_ERROR("Failed to schedule IB !\n"); > } > @@ -452,25 +452,22 @@ static int radeon_cs_ib_vm_chunk(struct radeon_device *rdev, > } > radeon_cs_sync_rings(parser); > > + parser->ib.vm_id = vm->id; > + /* ib pool is bind at 0 in virtual address space, > + * so gpu_addr is the offset inside the pool bo > + */ > + parser->ib.gpu_addr = parser->ib.sa_bo->soffset; > + > if ((rdev->family >= CHIP_TAHITI) && > (parser->chunk_const_ib_idx != -1)) { > parser->const_ib.vm_id = vm->id; > - /* ib pool is bind at 0 in virtual address space to gpu_addr is the > - * offset inside the pool bo > - */ > + /* same reason as above */ > parser->const_ib.gpu_addr = parser->const_ib.sa_bo->soffset; > - r = radeon_ib_schedule(rdev, &parser->const_ib); > - if (r) > - goto out; > + r = radeon_ib_schedule(rdev, &parser->ib, &parser->const_ib); > + } else { > + r = radeon_ib_schedule(rdev, &parser->ib, NULL); > } > > - parser->ib.vm_id = vm->id; > - /* ib pool is bind at 0 in virtual address space to gpu_addr is the > - * offset inside the pool bo > - */ > - parser->ib.gpu_addr = parser->ib.sa_bo->soffset; > - parser->ib.is_const_ib = false; > - r = radeon_ib_schedule(rdev, &parser->ib); > out: > if (!r) { > if (vm->fence) { > diff --git a/drivers/gpu/drm/radeon/radeon_ring.c b/drivers/gpu/drm/radeon/radeon_ring.c > index 75cbe46..c48c354 100644 > --- a/drivers/gpu/drm/radeon/radeon_ring.c > +++ b/drivers/gpu/drm/radeon/radeon_ring.c > @@ -74,7 +74,8 @@ void radeon_ib_free(struct radeon_device *rdev, struct radeon_ib *ib) > radeon_fence_unref(&ib->fence); > } > > -int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib) > +int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib, > + struct radeon_ib *const_ib) Since you are modifying an uncommented function, comments should be added, per the new documentation rules. I don't mean to be picky, but there is no point having documentation rules if they aren't enforced. > { > struct radeon_ring *ring = &rdev->ring[ib->ring]; > bool need_sync = false; > @@ -105,6 +106,10 @@ int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib) > if (!need_sync) { > radeon_semaphore_free(rdev, &ib->semaphore, NULL); > } > + if (const_ib) { > + radeon_ring_ib_execute(rdev, const_ib->ring, const_ib); > + radeon_semaphore_free(rdev, &const_ib->semaphore, NULL); > + } > radeon_ring_ib_execute(rdev, ib->ring, ib); > r = radeon_fence_emit(rdev, &ib->fence, ib->ring); > if (r) { > @@ -112,6 +117,9 @@ int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib) > radeon_ring_unlock_undo(rdev, ring); > return r; > } > + if (const_ib) { > + const_ib->fence = radeon_fence_ref(ib->fence); > + } > radeon_ring_unlock_commit(rdev, ring); > return 0; > } > -- > 1.7.9.5 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On Fri, Jul 13, 2012 at 10:08 AM, Christian König <deathsimple@vodafone.de> wrote: > Const IBs are executed on the CE not the CP, so we can't > fence them in the normal way. > > So submit them directly before the IB instead, just as > the documentation says. > > Signed-off-by: Christian König <deathsimple@vodafone.de> > --- > drivers/gpu/drm/radeon/r100.c | 2 +- > drivers/gpu/drm/radeon/r600.c | 2 +- > drivers/gpu/drm/radeon/radeon.h | 3 ++- > drivers/gpu/drm/radeon/radeon_cs.c | 25 +++++++++++-------------- > drivers/gpu/drm/radeon/radeon_ring.c | 10 +++++++++- > 5 files changed, 24 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c > index e0f5ae8..4ee5a74 100644 > --- a/drivers/gpu/drm/radeon/r100.c > +++ b/drivers/gpu/drm/radeon/r100.c > @@ -3693,7 +3693,7 @@ int r100_ib_test(struct radeon_device *rdev, struct radeon_ring *ring) > ib.ptr[6] = PACKET2(0); > ib.ptr[7] = PACKET2(0); > ib.length_dw = 8; > - r = radeon_ib_schedule(rdev, &ib); > + r = radeon_ib_schedule(rdev, &ib, NULL); > if (r) { > radeon_scratch_free(rdev, scratch); > radeon_ib_free(rdev, &ib); > diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c > index 3156d25..c2e5069 100644 > --- a/drivers/gpu/drm/radeon/r600.c > +++ b/drivers/gpu/drm/radeon/r600.c > @@ -2619,7 +2619,7 @@ int r600_ib_test(struct radeon_device *rdev, struct radeon_ring *ring) > ib.ptr[1] = ((scratch - PACKET3_SET_CONFIG_REG_OFFSET) >> 2); > ib.ptr[2] = 0xDEADBEEF; > ib.length_dw = 3; > - r = radeon_ib_schedule(rdev, &ib); > + r = radeon_ib_schedule(rdev, &ib, NULL); > if (r) { > radeon_scratch_free(rdev, scratch); > radeon_ib_free(rdev, &ib); > diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h > index 2cb355b..2d7f06c 100644 > --- a/drivers/gpu/drm/radeon/radeon.h > +++ b/drivers/gpu/drm/radeon/radeon.h > @@ -751,7 +751,8 @@ struct si_rlc { > int radeon_ib_get(struct radeon_device *rdev, int ring, > struct radeon_ib *ib, unsigned size); > void radeon_ib_free(struct radeon_device *rdev, struct radeon_ib *ib); > -int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib); > +int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib, > + struct radeon_ib *const_ib); > int radeon_ib_pool_init(struct radeon_device *rdev); > void radeon_ib_pool_fini(struct radeon_device *rdev); > int radeon_ib_ring_tests(struct radeon_device *rdev); > diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c > index 553da67..d0be5d5 100644 > --- a/drivers/gpu/drm/radeon/radeon_cs.c > +++ b/drivers/gpu/drm/radeon/radeon_cs.c > @@ -354,7 +354,7 @@ static int radeon_cs_ib_chunk(struct radeon_device *rdev, > } > radeon_cs_sync_rings(parser); > parser->ib.vm_id = 0; > - r = radeon_ib_schedule(rdev, &parser->ib); > + r = radeon_ib_schedule(rdev, &parser->ib, NULL); > if (r) { > DRM_ERROR("Failed to schedule IB !\n"); > } > @@ -452,25 +452,22 @@ static int radeon_cs_ib_vm_chunk(struct radeon_device *rdev, > } > radeon_cs_sync_rings(parser); > > + parser->ib.vm_id = vm->id; > + /* ib pool is bind at 0 in virtual address space, > + * so gpu_addr is the offset inside the pool bo > + */ > + parser->ib.gpu_addr = parser->ib.sa_bo->soffset; > + > if ((rdev->family >= CHIP_TAHITI) && > (parser->chunk_const_ib_idx != -1)) { > parser->const_ib.vm_id = vm->id; > - /* ib pool is bind at 0 in virtual address space to gpu_addr is the > - * offset inside the pool bo > - */ > + /* same reason as above */ Don't remove comment, code might move and the above comment might not be the same better to duplicate comment then trying to cross reference comment across file. > parser->const_ib.gpu_addr = parser->const_ib.sa_bo->soffset; > - r = radeon_ib_schedule(rdev, &parser->const_ib); > - if (r) > - goto out; > + r = radeon_ib_schedule(rdev, &parser->ib, &parser->const_ib); > + } else { > + r = radeon_ib_schedule(rdev, &parser->ib, NULL); > } > > - parser->ib.vm_id = vm->id; > - /* ib pool is bind at 0 in virtual address space to gpu_addr is the > - * offset inside the pool bo > - */ > - parser->ib.gpu_addr = parser->ib.sa_bo->soffset; > - parser->ib.is_const_ib = false; > - r = radeon_ib_schedule(rdev, &parser->ib); > out: > if (!r) { > if (vm->fence) { > diff --git a/drivers/gpu/drm/radeon/radeon_ring.c b/drivers/gpu/drm/radeon/radeon_ring.c > index 75cbe46..c48c354 100644 > --- a/drivers/gpu/drm/radeon/radeon_ring.c > +++ b/drivers/gpu/drm/radeon/radeon_ring.c > @@ -74,7 +74,8 @@ void radeon_ib_free(struct radeon_device *rdev, struct radeon_ib *ib) > radeon_fence_unref(&ib->fence); > } > > -int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib) > +int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib, > + struct radeon_ib *const_ib) > { > struct radeon_ring *ring = &rdev->ring[ib->ring]; > bool need_sync = false; > @@ -105,6 +106,10 @@ int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib) > if (!need_sync) { > radeon_semaphore_free(rdev, &ib->semaphore, NULL); > } > + if (const_ib) { > + radeon_ring_ib_execute(rdev, const_ib->ring, const_ib); > + radeon_semaphore_free(rdev, &const_ib->semaphore, NULL); > + } > radeon_ring_ib_execute(rdev, ib->ring, ib); > r = radeon_fence_emit(rdev, &ib->fence, ib->ring); > if (r) { > @@ -112,6 +117,9 @@ int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib) > radeon_ring_unlock_undo(rdev, ring); > return r; > } > + if (const_ib) { > + const_ib->fence = radeon_fence_ref(ib->fence); > + } > radeon_ring_unlock_commit(rdev, ring); > return 0; > } > -- > 1.7.9.5 > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel
On 13.07.2012 16:17, Tom Stellard wrote: > On Fri, Jul 13, 2012 at 04:08:15PM +0200, Christian König wrote: >> Const IBs are executed on the CE not the CP, so we can't >> fence them in the normal way. >> >> So submit them directly before the IB instead, just as >> the documentation says. >> >> Signed-off-by: Christian König <deathsimple@vodafone.de> >> --- >> drivers/gpu/drm/radeon/r100.c | 2 +- >> drivers/gpu/drm/radeon/r600.c | 2 +- >> drivers/gpu/drm/radeon/radeon.h | 3 ++- >> drivers/gpu/drm/radeon/radeon_cs.c | 25 +++++++++++-------------- >> drivers/gpu/drm/radeon/radeon_ring.c | 10 +++++++++- >> 5 files changed, 24 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c >> index e0f5ae8..4ee5a74 100644 >> --- a/drivers/gpu/drm/radeon/r100.c >> +++ b/drivers/gpu/drm/radeon/r100.c >> @@ -3693,7 +3693,7 @@ int r100_ib_test(struct radeon_device *rdev, struct radeon_ring *ring) >> ib.ptr[6] = PACKET2(0); >> ib.ptr[7] = PACKET2(0); >> ib.length_dw = 8; >> - r = radeon_ib_schedule(rdev, &ib); >> + r = radeon_ib_schedule(rdev, &ib, NULL); >> if (r) { >> radeon_scratch_free(rdev, scratch); >> radeon_ib_free(rdev, &ib); >> diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c >> index 3156d25..c2e5069 100644 >> --- a/drivers/gpu/drm/radeon/r600.c >> +++ b/drivers/gpu/drm/radeon/r600.c >> @@ -2619,7 +2619,7 @@ int r600_ib_test(struct radeon_device *rdev, struct radeon_ring *ring) >> ib.ptr[1] = ((scratch - PACKET3_SET_CONFIG_REG_OFFSET) >> 2); >> ib.ptr[2] = 0xDEADBEEF; >> ib.length_dw = 3; >> - r = radeon_ib_schedule(rdev, &ib); >> + r = radeon_ib_schedule(rdev, &ib, NULL); >> if (r) { >> radeon_scratch_free(rdev, scratch); >> radeon_ib_free(rdev, &ib); >> diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h >> index 2cb355b..2d7f06c 100644 >> --- a/drivers/gpu/drm/radeon/radeon.h >> +++ b/drivers/gpu/drm/radeon/radeon.h >> @@ -751,7 +751,8 @@ struct si_rlc { >> int radeon_ib_get(struct radeon_device *rdev, int ring, >> struct radeon_ib *ib, unsigned size); >> void radeon_ib_free(struct radeon_device *rdev, struct radeon_ib *ib); >> -int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib); >> +int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib, >> + struct radeon_ib *const_ib); >> int radeon_ib_pool_init(struct radeon_device *rdev); >> void radeon_ib_pool_fini(struct radeon_device *rdev); >> int radeon_ib_ring_tests(struct radeon_device *rdev); >> diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c >> index 553da67..d0be5d5 100644 >> --- a/drivers/gpu/drm/radeon/radeon_cs.c >> +++ b/drivers/gpu/drm/radeon/radeon_cs.c >> @@ -354,7 +354,7 @@ static int radeon_cs_ib_chunk(struct radeon_device *rdev, >> } >> radeon_cs_sync_rings(parser); >> parser->ib.vm_id = 0; >> - r = radeon_ib_schedule(rdev, &parser->ib); >> + r = radeon_ib_schedule(rdev, &parser->ib, NULL); >> if (r) { >> DRM_ERROR("Failed to schedule IB !\n"); >> } >> @@ -452,25 +452,22 @@ static int radeon_cs_ib_vm_chunk(struct radeon_device *rdev, >> } >> radeon_cs_sync_rings(parser); >> >> + parser->ib.vm_id = vm->id; >> + /* ib pool is bind at 0 in virtual address space, >> + * so gpu_addr is the offset inside the pool bo >> + */ >> + parser->ib.gpu_addr = parser->ib.sa_bo->soffset; >> + >> if ((rdev->family >= CHIP_TAHITI) && >> (parser->chunk_const_ib_idx != -1)) { >> parser->const_ib.vm_id = vm->id; >> - /* ib pool is bind at 0 in virtual address space to gpu_addr is the >> - * offset inside the pool bo >> - */ >> + /* same reason as above */ >> parser->const_ib.gpu_addr = parser->const_ib.sa_bo->soffset; >> - r = radeon_ib_schedule(rdev, &parser->const_ib); >> - if (r) >> - goto out; >> + r = radeon_ib_schedule(rdev, &parser->ib, &parser->const_ib); >> + } else { >> + r = radeon_ib_schedule(rdev, &parser->ib, NULL); >> } >> >> - parser->ib.vm_id = vm->id; >> - /* ib pool is bind at 0 in virtual address space to gpu_addr is the >> - * offset inside the pool bo >> - */ >> - parser->ib.gpu_addr = parser->ib.sa_bo->soffset; >> - parser->ib.is_const_ib = false; >> - r = radeon_ib_schedule(rdev, &parser->ib); >> out: >> if (!r) { >> if (vm->fence) { >> diff --git a/drivers/gpu/drm/radeon/radeon_ring.c b/drivers/gpu/drm/radeon/radeon_ring.c >> index 75cbe46..c48c354 100644 >> --- a/drivers/gpu/drm/radeon/radeon_ring.c >> +++ b/drivers/gpu/drm/radeon/radeon_ring.c >> @@ -74,7 +74,8 @@ void radeon_ib_free(struct radeon_device *rdev, struct radeon_ib *ib) >> radeon_fence_unref(&ib->fence); >> } >> >> -int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib) >> +int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib, >> + struct radeon_ib *const_ib) > Since you are modifying an uncommented function, comments should be > added, per the new documentation rules. > > I don't mean to be picky, but there is no point having documentation > rules if they aren't enforced. Oh no, please be picky about it. Otherwise I wouldn't learn it. For this particular change Alex already had appropriate documentation in the pipeline and I wanted to rather change them instead of adding documentation in this patch. Christian. > >> { >> struct radeon_ring *ring = &rdev->ring[ib->ring]; >> bool need_sync = false; >> @@ -105,6 +106,10 @@ int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib) >> if (!need_sync) { >> radeon_semaphore_free(rdev, &ib->semaphore, NULL); >> } >> + if (const_ib) { >> + radeon_ring_ib_execute(rdev, const_ib->ring, const_ib); >> + radeon_semaphore_free(rdev, &const_ib->semaphore, NULL); >> + } >> radeon_ring_ib_execute(rdev, ib->ring, ib); >> r = radeon_fence_emit(rdev, &ib->fence, ib->ring); >> if (r) { >> @@ -112,6 +117,9 @@ int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib) >> radeon_ring_unlock_undo(rdev, ring); >> return r; >> } >> + if (const_ib) { >> + const_ib->fence = radeon_fence_ref(ib->fence); >> + } >> radeon_ring_unlock_commit(rdev, ring); >> return 0; >> } >> -- >> 1.7.9.5 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel >
On Tue, Jul 17, 2012 at 5:50 AM, Christian König <deathsimple@vodafone.de> wrote: > On 13.07.2012 16:17, Tom Stellard wrote: >> >> On Fri, Jul 13, 2012 at 04:08:15PM +0200, Christian König wrote: >>> >>> Const IBs are executed on the CE not the CP, so we can't >>> fence them in the normal way. >>> >>> So submit them directly before the IB instead, just as >>> the documentation says. >>> >>> Signed-off-by: Christian König <deathsimple@vodafone.de> >>> --- >>> drivers/gpu/drm/radeon/r100.c | 2 +- >>> drivers/gpu/drm/radeon/r600.c | 2 +- >>> drivers/gpu/drm/radeon/radeon.h | 3 ++- >>> drivers/gpu/drm/radeon/radeon_cs.c | 25 +++++++++++-------------- >>> drivers/gpu/drm/radeon/radeon_ring.c | 10 +++++++++- >>> 5 files changed, 24 insertions(+), 18 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/radeon/r100.c >>> b/drivers/gpu/drm/radeon/r100.c >>> index e0f5ae8..4ee5a74 100644 >>> --- a/drivers/gpu/drm/radeon/r100.c >>> +++ b/drivers/gpu/drm/radeon/r100.c >>> @@ -3693,7 +3693,7 @@ int r100_ib_test(struct radeon_device *rdev, struct >>> radeon_ring *ring) >>> ib.ptr[6] = PACKET2(0); >>> ib.ptr[7] = PACKET2(0); >>> ib.length_dw = 8; >>> - r = radeon_ib_schedule(rdev, &ib); >>> + r = radeon_ib_schedule(rdev, &ib, NULL); >>> if (r) { >>> radeon_scratch_free(rdev, scratch); >>> radeon_ib_free(rdev, &ib); >>> diff --git a/drivers/gpu/drm/radeon/r600.c >>> b/drivers/gpu/drm/radeon/r600.c >>> index 3156d25..c2e5069 100644 >>> --- a/drivers/gpu/drm/radeon/r600.c >>> +++ b/drivers/gpu/drm/radeon/r600.c >>> @@ -2619,7 +2619,7 @@ int r600_ib_test(struct radeon_device *rdev, struct >>> radeon_ring *ring) >>> ib.ptr[1] = ((scratch - PACKET3_SET_CONFIG_REG_OFFSET) >> 2); >>> ib.ptr[2] = 0xDEADBEEF; >>> ib.length_dw = 3; >>> - r = radeon_ib_schedule(rdev, &ib); >>> + r = radeon_ib_schedule(rdev, &ib, NULL); >>> if (r) { >>> radeon_scratch_free(rdev, scratch); >>> radeon_ib_free(rdev, &ib); >>> diff --git a/drivers/gpu/drm/radeon/radeon.h >>> b/drivers/gpu/drm/radeon/radeon.h >>> index 2cb355b..2d7f06c 100644 >>> --- a/drivers/gpu/drm/radeon/radeon.h >>> +++ b/drivers/gpu/drm/radeon/radeon.h >>> @@ -751,7 +751,8 @@ struct si_rlc { >>> int radeon_ib_get(struct radeon_device *rdev, int ring, >>> struct radeon_ib *ib, unsigned size); >>> void radeon_ib_free(struct radeon_device *rdev, struct radeon_ib *ib); >>> -int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib >>> *ib); >>> +int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib, >>> + struct radeon_ib *const_ib); >>> int radeon_ib_pool_init(struct radeon_device *rdev); >>> void radeon_ib_pool_fini(struct radeon_device *rdev); >>> int radeon_ib_ring_tests(struct radeon_device *rdev); >>> diff --git a/drivers/gpu/drm/radeon/radeon_cs.c >>> b/drivers/gpu/drm/radeon/radeon_cs.c >>> index 553da67..d0be5d5 100644 >>> --- a/drivers/gpu/drm/radeon/radeon_cs.c >>> +++ b/drivers/gpu/drm/radeon/radeon_cs.c >>> @@ -354,7 +354,7 @@ static int radeon_cs_ib_chunk(struct radeon_device >>> *rdev, >>> } >>> radeon_cs_sync_rings(parser); >>> parser->ib.vm_id = 0; >>> - r = radeon_ib_schedule(rdev, &parser->ib); >>> + r = radeon_ib_schedule(rdev, &parser->ib, NULL); >>> if (r) { >>> DRM_ERROR("Failed to schedule IB !\n"); >>> } >>> @@ -452,25 +452,22 @@ static int radeon_cs_ib_vm_chunk(struct >>> radeon_device *rdev, >>> } >>> radeon_cs_sync_rings(parser); >>> + parser->ib.vm_id = vm->id; >>> + /* ib pool is bind at 0 in virtual address space, >>> + * so gpu_addr is the offset inside the pool bo >>> + */ >>> + parser->ib.gpu_addr = parser->ib.sa_bo->soffset; >>> + >>> if ((rdev->family >= CHIP_TAHITI) && >>> (parser->chunk_const_ib_idx != -1)) { >>> parser->const_ib.vm_id = vm->id; >>> - /* ib pool is bind at 0 in virtual address space to >>> gpu_addr is the >>> - * offset inside the pool bo >>> - */ >>> + /* same reason as above */ >>> parser->const_ib.gpu_addr = >>> parser->const_ib.sa_bo->soffset; >>> - r = radeon_ib_schedule(rdev, &parser->const_ib); >>> - if (r) >>> - goto out; >>> + r = radeon_ib_schedule(rdev, &parser->ib, >>> &parser->const_ib); >>> + } else { >>> + r = radeon_ib_schedule(rdev, &parser->ib, NULL); >>> } >>> - parser->ib.vm_id = vm->id; >>> - /* ib pool is bind at 0 in virtual address space to gpu_addr is >>> the >>> - * offset inside the pool bo >>> - */ >>> - parser->ib.gpu_addr = parser->ib.sa_bo->soffset; >>> - parser->ib.is_const_ib = false; >>> - r = radeon_ib_schedule(rdev, &parser->ib); >>> out: >>> if (!r) { >>> if (vm->fence) { >>> diff --git a/drivers/gpu/drm/radeon/radeon_ring.c >>> b/drivers/gpu/drm/radeon/radeon_ring.c >>> index 75cbe46..c48c354 100644 >>> --- a/drivers/gpu/drm/radeon/radeon_ring.c >>> +++ b/drivers/gpu/drm/radeon/radeon_ring.c >>> @@ -74,7 +74,8 @@ void radeon_ib_free(struct radeon_device *rdev, struct >>> radeon_ib *ib) >>> radeon_fence_unref(&ib->fence); >>> } >>> >>> -int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib) >>> +int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib, >>> + struct radeon_ib *const_ib) >> >> Since you are modifying an uncommented function, comments should be >> added, per the new documentation rules. >> >> I don't mean to be picky, but there is no point having documentation >> rules if they aren't enforced. > > Oh no, please be picky about it. Otherwise I wouldn't learn it. > > For this particular change Alex already had appropriate documentation in the > pipeline and I wanted to rather change them instead of adding documentation > in this patch. > > Christian. Still my earlier remark matter, i would rather not see cross comment reference it's confusing especially if code move. I rather see the same comment 2 times. Cheers, Jerome
On 17.07.2012 16:25, Jerome Glisse wrote: > On Tue, Jul 17, 2012 at 5:50 AM, Christian König > <deathsimple@vodafone.de> wrote: >> On 13.07.2012 16:17, Tom Stellard wrote: >>> On Fri, Jul 13, 2012 at 04:08:15PM +0200, Christian König wrote: >>>> Const IBs are executed on the CE not the CP, so we can't >>>> fence them in the normal way. >>>> >>>> So submit them directly before the IB instead, just as >>>> the documentation says. >>>> >>>> Signed-off-by: Christian König <deathsimple@vodafone.de> >>>> --- >>>> drivers/gpu/drm/radeon/r100.c | 2 +- >>>> drivers/gpu/drm/radeon/r600.c | 2 +- >>>> drivers/gpu/drm/radeon/radeon.h | 3 ++- >>>> drivers/gpu/drm/radeon/radeon_cs.c | 25 +++++++++++-------------- >>>> drivers/gpu/drm/radeon/radeon_ring.c | 10 +++++++++- >>>> 5 files changed, 24 insertions(+), 18 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/radeon/r100.c >>>> b/drivers/gpu/drm/radeon/r100.c >>>> index e0f5ae8..4ee5a74 100644 >>>> --- a/drivers/gpu/drm/radeon/r100.c >>>> +++ b/drivers/gpu/drm/radeon/r100.c >>>> @@ -3693,7 +3693,7 @@ int r100_ib_test(struct radeon_device *rdev, struct >>>> radeon_ring *ring) >>>> ib.ptr[6] = PACKET2(0); >>>> ib.ptr[7] = PACKET2(0); >>>> ib.length_dw = 8; >>>> - r = radeon_ib_schedule(rdev, &ib); >>>> + r = radeon_ib_schedule(rdev, &ib, NULL); >>>> if (r) { >>>> radeon_scratch_free(rdev, scratch); >>>> radeon_ib_free(rdev, &ib); >>>> diff --git a/drivers/gpu/drm/radeon/r600.c >>>> b/drivers/gpu/drm/radeon/r600.c >>>> index 3156d25..c2e5069 100644 >>>> --- a/drivers/gpu/drm/radeon/r600.c >>>> +++ b/drivers/gpu/drm/radeon/r600.c >>>> @@ -2619,7 +2619,7 @@ int r600_ib_test(struct radeon_device *rdev, struct >>>> radeon_ring *ring) >>>> ib.ptr[1] = ((scratch - PACKET3_SET_CONFIG_REG_OFFSET) >> 2); >>>> ib.ptr[2] = 0xDEADBEEF; >>>> ib.length_dw = 3; >>>> - r = radeon_ib_schedule(rdev, &ib); >>>> + r = radeon_ib_schedule(rdev, &ib, NULL); >>>> if (r) { >>>> radeon_scratch_free(rdev, scratch); >>>> radeon_ib_free(rdev, &ib); >>>> diff --git a/drivers/gpu/drm/radeon/radeon.h >>>> b/drivers/gpu/drm/radeon/radeon.h >>>> index 2cb355b..2d7f06c 100644 >>>> --- a/drivers/gpu/drm/radeon/radeon.h >>>> +++ b/drivers/gpu/drm/radeon/radeon.h >>>> @@ -751,7 +751,8 @@ struct si_rlc { >>>> int radeon_ib_get(struct radeon_device *rdev, int ring, >>>> struct radeon_ib *ib, unsigned size); >>>> void radeon_ib_free(struct radeon_device *rdev, struct radeon_ib *ib); >>>> -int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib >>>> *ib); >>>> +int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib, >>>> + struct radeon_ib *const_ib); >>>> int radeon_ib_pool_init(struct radeon_device *rdev); >>>> void radeon_ib_pool_fini(struct radeon_device *rdev); >>>> int radeon_ib_ring_tests(struct radeon_device *rdev); >>>> diff --git a/drivers/gpu/drm/radeon/radeon_cs.c >>>> b/drivers/gpu/drm/radeon/radeon_cs.c >>>> index 553da67..d0be5d5 100644 >>>> --- a/drivers/gpu/drm/radeon/radeon_cs.c >>>> +++ b/drivers/gpu/drm/radeon/radeon_cs.c >>>> @@ -354,7 +354,7 @@ static int radeon_cs_ib_chunk(struct radeon_device >>>> *rdev, >>>> } >>>> radeon_cs_sync_rings(parser); >>>> parser->ib.vm_id = 0; >>>> - r = radeon_ib_schedule(rdev, &parser->ib); >>>> + r = radeon_ib_schedule(rdev, &parser->ib, NULL); >>>> if (r) { >>>> DRM_ERROR("Failed to schedule IB !\n"); >>>> } >>>> @@ -452,25 +452,22 @@ static int radeon_cs_ib_vm_chunk(struct >>>> radeon_device *rdev, >>>> } >>>> radeon_cs_sync_rings(parser); >>>> + parser->ib.vm_id = vm->id; >>>> + /* ib pool is bind at 0 in virtual address space, >>>> + * so gpu_addr is the offset inside the pool bo >>>> + */ >>>> + parser->ib.gpu_addr = parser->ib.sa_bo->soffset; >>>> + >>>> if ((rdev->family >= CHIP_TAHITI) && >>>> (parser->chunk_const_ib_idx != -1)) { >>>> parser->const_ib.vm_id = vm->id; >>>> - /* ib pool is bind at 0 in virtual address space to >>>> gpu_addr is the >>>> - * offset inside the pool bo >>>> - */ >>>> + /* same reason as above */ >>>> parser->const_ib.gpu_addr = >>>> parser->const_ib.sa_bo->soffset; >>>> - r = radeon_ib_schedule(rdev, &parser->const_ib); >>>> - if (r) >>>> - goto out; >>>> + r = radeon_ib_schedule(rdev, &parser->ib, >>>> &parser->const_ib); >>>> + } else { >>>> + r = radeon_ib_schedule(rdev, &parser->ib, NULL); >>>> } >>>> - parser->ib.vm_id = vm->id; >>>> - /* ib pool is bind at 0 in virtual address space to gpu_addr is >>>> the >>>> - * offset inside the pool bo >>>> - */ >>>> - parser->ib.gpu_addr = parser->ib.sa_bo->soffset; >>>> - parser->ib.is_const_ib = false; >>>> - r = radeon_ib_schedule(rdev, &parser->ib); >>>> out: >>>> if (!r) { >>>> if (vm->fence) { >>>> diff --git a/drivers/gpu/drm/radeon/radeon_ring.c >>>> b/drivers/gpu/drm/radeon/radeon_ring.c >>>> index 75cbe46..c48c354 100644 >>>> --- a/drivers/gpu/drm/radeon/radeon_ring.c >>>> +++ b/drivers/gpu/drm/radeon/radeon_ring.c >>>> @@ -74,7 +74,8 @@ void radeon_ib_free(struct radeon_device *rdev, struct >>>> radeon_ib *ib) >>>> radeon_fence_unref(&ib->fence); >>>> } >>>> >>>> -int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib) >>>> +int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib, >>>> + struct radeon_ib *const_ib) >>> Since you are modifying an uncommented function, comments should be >>> added, per the new documentation rules. >>> >>> I don't mean to be picky, but there is no point having documentation >>> rules if they aren't enforced. >> Oh no, please be picky about it. Otherwise I wouldn't learn it. >> >> For this particular change Alex already had appropriate documentation in the >> pipeline and I wanted to rather change them instead of adding documentation >> in this patch. >> >> Christian. > Still my earlier remark matter, i would rather not see cross comment > reference it's confusing especially if code move. I rather see the > same comment 2 times. That was in the other patch, and yeah your right I changed that one. Christian. > > Cheers, > Jerome >
diff --git a/drivers/gpu/drm/radeon/r100.c b/drivers/gpu/drm/radeon/r100.c index e0f5ae8..4ee5a74 100644 --- a/drivers/gpu/drm/radeon/r100.c +++ b/drivers/gpu/drm/radeon/r100.c @@ -3693,7 +3693,7 @@ int r100_ib_test(struct radeon_device *rdev, struct radeon_ring *ring) ib.ptr[6] = PACKET2(0); ib.ptr[7] = PACKET2(0); ib.length_dw = 8; - r = radeon_ib_schedule(rdev, &ib); + r = radeon_ib_schedule(rdev, &ib, NULL); if (r) { radeon_scratch_free(rdev, scratch); radeon_ib_free(rdev, &ib); diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600.c index 3156d25..c2e5069 100644 --- a/drivers/gpu/drm/radeon/r600.c +++ b/drivers/gpu/drm/radeon/r600.c @@ -2619,7 +2619,7 @@ int r600_ib_test(struct radeon_device *rdev, struct radeon_ring *ring) ib.ptr[1] = ((scratch - PACKET3_SET_CONFIG_REG_OFFSET) >> 2); ib.ptr[2] = 0xDEADBEEF; ib.length_dw = 3; - r = radeon_ib_schedule(rdev, &ib); + r = radeon_ib_schedule(rdev, &ib, NULL); if (r) { radeon_scratch_free(rdev, scratch); radeon_ib_free(rdev, &ib); diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/radeon.h index 2cb355b..2d7f06c 100644 --- a/drivers/gpu/drm/radeon/radeon.h +++ b/drivers/gpu/drm/radeon/radeon.h @@ -751,7 +751,8 @@ struct si_rlc { int radeon_ib_get(struct radeon_device *rdev, int ring, struct radeon_ib *ib, unsigned size); void radeon_ib_free(struct radeon_device *rdev, struct radeon_ib *ib); -int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib); +int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib, + struct radeon_ib *const_ib); int radeon_ib_pool_init(struct radeon_device *rdev); void radeon_ib_pool_fini(struct radeon_device *rdev); int radeon_ib_ring_tests(struct radeon_device *rdev); diff --git a/drivers/gpu/drm/radeon/radeon_cs.c b/drivers/gpu/drm/radeon/radeon_cs.c index 553da67..d0be5d5 100644 --- a/drivers/gpu/drm/radeon/radeon_cs.c +++ b/drivers/gpu/drm/radeon/radeon_cs.c @@ -354,7 +354,7 @@ static int radeon_cs_ib_chunk(struct radeon_device *rdev, } radeon_cs_sync_rings(parser); parser->ib.vm_id = 0; - r = radeon_ib_schedule(rdev, &parser->ib); + r = radeon_ib_schedule(rdev, &parser->ib, NULL); if (r) { DRM_ERROR("Failed to schedule IB !\n"); } @@ -452,25 +452,22 @@ static int radeon_cs_ib_vm_chunk(struct radeon_device *rdev, } radeon_cs_sync_rings(parser); + parser->ib.vm_id = vm->id; + /* ib pool is bind at 0 in virtual address space, + * so gpu_addr is the offset inside the pool bo + */ + parser->ib.gpu_addr = parser->ib.sa_bo->soffset; + if ((rdev->family >= CHIP_TAHITI) && (parser->chunk_const_ib_idx != -1)) { parser->const_ib.vm_id = vm->id; - /* ib pool is bind at 0 in virtual address space to gpu_addr is the - * offset inside the pool bo - */ + /* same reason as above */ parser->const_ib.gpu_addr = parser->const_ib.sa_bo->soffset; - r = radeon_ib_schedule(rdev, &parser->const_ib); - if (r) - goto out; + r = radeon_ib_schedule(rdev, &parser->ib, &parser->const_ib); + } else { + r = radeon_ib_schedule(rdev, &parser->ib, NULL); } - parser->ib.vm_id = vm->id; - /* ib pool is bind at 0 in virtual address space to gpu_addr is the - * offset inside the pool bo - */ - parser->ib.gpu_addr = parser->ib.sa_bo->soffset; - parser->ib.is_const_ib = false; - r = radeon_ib_schedule(rdev, &parser->ib); out: if (!r) { if (vm->fence) { diff --git a/drivers/gpu/drm/radeon/radeon_ring.c b/drivers/gpu/drm/radeon/radeon_ring.c index 75cbe46..c48c354 100644 --- a/drivers/gpu/drm/radeon/radeon_ring.c +++ b/drivers/gpu/drm/radeon/radeon_ring.c @@ -74,7 +74,8 @@ void radeon_ib_free(struct radeon_device *rdev, struct radeon_ib *ib) radeon_fence_unref(&ib->fence); } -int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib) +int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib, + struct radeon_ib *const_ib) { struct radeon_ring *ring = &rdev->ring[ib->ring]; bool need_sync = false; @@ -105,6 +106,10 @@ int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib) if (!need_sync) { radeon_semaphore_free(rdev, &ib->semaphore, NULL); } + if (const_ib) { + radeon_ring_ib_execute(rdev, const_ib->ring, const_ib); + radeon_semaphore_free(rdev, &const_ib->semaphore, NULL); + } radeon_ring_ib_execute(rdev, ib->ring, ib); r = radeon_fence_emit(rdev, &ib->fence, ib->ring); if (r) { @@ -112,6 +117,9 @@ int radeon_ib_schedule(struct radeon_device *rdev, struct radeon_ib *ib) radeon_ring_unlock_undo(rdev, ring); return r; } + if (const_ib) { + const_ib->fence = radeon_fence_ref(ib->fence); + } radeon_ring_unlock_commit(rdev, ring); return 0; }
Const IBs are executed on the CE not the CP, so we can't fence them in the normal way. So submit them directly before the IB instead, just as the documentation says. Signed-off-by: Christian König <deathsimple@vodafone.de> --- drivers/gpu/drm/radeon/r100.c | 2 +- drivers/gpu/drm/radeon/r600.c | 2 +- drivers/gpu/drm/radeon/radeon.h | 3 ++- drivers/gpu/drm/radeon/radeon_cs.c | 25 +++++++++++-------------- drivers/gpu/drm/radeon/radeon_ring.c | 10 +++++++++- 5 files changed, 24 insertions(+), 18 deletions(-)