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 16:32:05 +0200 Message-ID: <50057765.60604@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> <50053579.2030408@vodafone.de> 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 EEC4AA0966 for ; Tue, 17 Jul 2012 07:32:12 -0700 (PDT) In-Reply-To: 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: Jerome Glisse Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org On 17.07.2012 16:25, Jerome Glisse wrote: > On Tue, Jul 17, 2012 at 5:50 AM, Christian K=F6nig > wrote: >> 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, str= uct >>>> 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, str= uct >>>> 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/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 *i= b); >>>> -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_device *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/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, stru= ct >>>> 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 documentat= ion >> 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 >