From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Christian_K=F6nig?= Subject: Re: [PATCH 3/3] drm/radeon: fix const IB handling Date: Tue, 17 Jul 2012 11:50:49 +0200 Message-ID: <50053579.2030408@vodafone.de> References: <1342188495-3705-1-git-send-email-deathsimple@vodafone.de> <1342188495-3705-3-git-send-email-deathsimple@vodafone.de> <20120713141721.GB1789@L7-CNU1252LKR-172027226155.amd.com> Mime-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1"; Format="flowed" Content-Transfer-Encoding: quoted-printable Return-path: Received: from outgoing.email.vodafone.de (outgoing.email.vodafone.de [139.7.28.128]) by gabe.freedesktop.org (Postfix) with ESMTP id 6B7DAA0A7D for ; Tue, 17 Jul 2012 02:50:54 -0700 (PDT) In-Reply-To: <20120713141721.GB1789@L7-CNU1252LKR-172027226155.amd.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org Errors-To: dri-devel-bounces+sf-dri-devel=m.gmane.org@lists.freedesktop.org To: Tom Stellard Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On 13.07.2012 16:17, Tom Stellard wrote: > On Fri, Jul 13, 2012 at 04:08:15PM +0200, Christian K=F6nig 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=F6nig >> --- >> 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, struc= t radeon_ring *ring) >> ib.ptr[6] =3D PACKET2(0); >> ib.ptr[7] =3D PACKET2(0); >> ib.length_dw =3D 8; >> - r =3D radeon_ib_schedule(rdev, &ib); >> + r =3D 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, struc= t radeon_ring *ring) >> ib.ptr[1] =3D ((scratch - PACKET3_SET_CONFIG_REG_OFFSET) >> 2); >> ib.ptr[2] =3D 0xDEADBEEF; >> ib.length_dw =3D 3; >> - r =3D radeon_ib_schedule(rdev, &ib); >> + r =3D 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/ra= deon.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 =3D 0; >> - r =3D radeon_ib_schedule(rdev, &parser->ib); >> + r =3D 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_dev= ice *rdev, >> } >> radeon_cs_sync_rings(parser); >> = >> + parser->ib.vm_id =3D 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 =3D parser->ib.sa_bo->soffset; >> + >> if ((rdev->family >=3D CHIP_TAHITI) && >> (parser->chunk_const_ib_idx !=3D -1)) { >> parser->const_ib.vm_id =3D 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 =3D parser->const_ib.sa_bo->soffset; >> - r =3D radeon_ib_schedule(rdev, &parser->const_ib); >> - if (r) >> - goto out; >> + r =3D radeon_ib_schedule(rdev, &parser->ib, &parser->const_ib); >> + } else { >> + r =3D radeon_ib_schedule(rdev, &parser->ib, NULL); >> } >> = >> - parser->ib.vm_id =3D 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 =3D parser->ib.sa_bo->soffset; >> - parser->ib.is_const_ib =3D false; >> - r =3D 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/rade= on/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 =3D &rdev->ring[ib->ring]; >> bool need_sync =3D 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 =3D radeon_fence_emit(rdev, &ib->fence, ib->ring); >> if (r) { >> @@ -112,6 +117,9 @@ int radeon_ib_schedule(struct radeon_device *rdev, s= truct radeon_ib *ib) >> radeon_ring_unlock_undo(rdev, ring); >> return r; >> } >> + if (const_ib) { >> + const_ib->fence =3D 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 >