From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?ISO-8859-1?Q?Christian_K=F6nig?= Subject: Re: [PATCH 14/15] drm/radeon: record what is next valid wptr for each ring v2 Date: Wed, 11 Jul 2012 10:08:24 +0200 Message-ID: <4FFD3478.9010708@vodafone.de> References: <1341924684-563-1-git-send-email-deathsimple@vodafone.de> <1341924684-563-14-git-send-email-deathsimple@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 93BC0A0D9B for ; Wed, 11 Jul 2012 01:08:28 -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 10.07.2012 18:56, Jerome Glisse wrote: > On Tue, Jul 10, 2012 at 8:51 AM, Christian K=F6nig > wrote: >> Before emitting any indirect buffer, emit the offset of the next >> valid ring content if any. This allow code that want to resume >> ring to resume ring right after ib that caused GPU lockup. >> >> v2: use scratch registers instead of storing it into memory >> > Why using scratch register ? To minimize bus activities ? No, but that also seems like a valid argument. If we want to reduce bus = activity we should also disable writeback for the scratch registers . My main intention was that a register write seems to be more failsafe = than a memory write. For a memory write there are just more things that = can go terribly wrong, just think of old AGP cards. Also not all rings = can write to memory, but it seems like that everybody can do a simple = register write. Reading that register once in case of a lockup isn't such a big overhead = compared what needs to be done for a complete reset. >> Signed-off-by: Jerome Glisse >> Signed-off-by: Christian K=F6nig >> --- >> drivers/gpu/drm/radeon/evergreen.c | 8 +++++++- >> drivers/gpu/drm/radeon/ni.c | 11 ++++++++++- >> drivers/gpu/drm/radeon/r600.c | 18 ++++++++++++++++-- >> drivers/gpu/drm/radeon/radeon.h | 1 + >> drivers/gpu/drm/radeon/radeon_ring.c | 4 ++++ >> drivers/gpu/drm/radeon/rv770.c | 4 +++- >> drivers/gpu/drm/radeon/si.c | 22 +++++++++++++++++++--- >> 7 files changed, 60 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/gpu/drm/radeon/evergreen.c b/drivers/gpu/drm/radeon= /evergreen.c >> index f39b900..40de347 100644 >> --- a/drivers/gpu/drm/radeon/evergreen.c >> +++ b/drivers/gpu/drm/radeon/evergreen.c >> @@ -1368,7 +1368,13 @@ void evergreen_ring_ib_execute(struct radeon_devi= ce *rdev, struct radeon_ib *ib) >> /* set to DX10/11 mode */ >> radeon_ring_write(ring, PACKET3(PACKET3_MODE_CONTROL, 0)); >> radeon_ring_write(ring, 1); >> - /* FIXME: implement */ >> + >> + if (ring->rptr_save_reg) { >> + uint32_t next_rptr =3D ring->wptr + 2 + 4; >> + radeon_ring_write(ring, PACKET0(ring->rptr_save_reg, 0)); >> + radeon_ring_write(ring, next_rptr); >> + } >> + >> radeon_ring_write(ring, PACKET3(PACKET3_INDIRECT_BUFFER, 2)); >> radeon_ring_write(ring, >> #ifdef __BIG_ENDIAN >> diff --git a/drivers/gpu/drm/radeon/ni.c b/drivers/gpu/drm/radeon/ni.c >> index f2afefb..6e3d448 100644 >> --- a/drivers/gpu/drm/radeon/ni.c >> +++ b/drivers/gpu/drm/radeon/ni.c >> @@ -855,6 +855,13 @@ void cayman_ring_ib_execute(struct radeon_device *r= dev, struct radeon_ib *ib) >> /* set to DX10/11 mode */ >> radeon_ring_write(ring, PACKET3(PACKET3_MODE_CONTROL, 0)); >> radeon_ring_write(ring, 1); >> + >> + if (ring->rptr_save_reg) { >> + uint32_t next_rptr =3D ring->wptr + 2 + 4; > I would rather also skip the surface sync so add another + 8 Akk, going to change that. > >> + radeon_ring_write(ring, PACKET0(ring->rptr_save_reg, 0)); >> + radeon_ring_write(ring, next_rptr); >> + } >> + >> radeon_ring_write(ring, PACKET3(PACKET3_INDIRECT_BUFFER, 2)); >> radeon_ring_write(ring, >> #ifdef __BIG_ENDIAN >> @@ -981,8 +988,10 @@ static int cayman_cp_start(struct radeon_device *rd= ev) >> >> static void cayman_cp_fini(struct radeon_device *rdev) >> { >> + struct radeon_ring *ring =3D &rdev->ring[RADEON_RING_TYPE_GFX_IN= DEX]; >> cayman_cp_enable(rdev, false); >> - radeon_ring_fini(rdev, &rdev->ring[RADEON_RING_TYPE_GFX_INDEX]); >> + radeon_ring_fini(rdev, ring); >> + radeon_scratch_free(rdev, ring->rptr_save_reg); >> } >> >> int cayman_cp_resume(struct radeon_device *rdev) >> diff --git a/drivers/gpu/drm/radeon/r600.c b/drivers/gpu/drm/radeon/r600= .c >> index c808fa9..74fca15 100644 >> --- a/drivers/gpu/drm/radeon/r600.c >> +++ b/drivers/gpu/drm/radeon/r600.c >> @@ -2155,18 +2155,27 @@ int r600_cp_resume(struct radeon_device *rdev) >> void r600_ring_init(struct radeon_device *rdev, struct radeon_ring *ri= ng, unsigned ring_size) >> { >> u32 rb_bufsz; >> + int r; >> >> /* Align ring size */ >> rb_bufsz =3D drm_order(ring_size / 8); >> ring_size =3D (1 << (rb_bufsz + 1)) * 4; >> ring->ring_size =3D ring_size; >> ring->align_mask =3D 16 - 1; >> + >> + r =3D radeon_scratch_get(rdev, &ring->rptr_save_reg); >> + if (r) { >> + DRM_ERROR("failed to get scratch reg for rptr save (%d).= \n", r); >> + ring->rptr_save_reg =3D 0; >> + } >> } >> >> void r600_cp_fini(struct radeon_device *rdev) >> { >> + struct radeon_ring *ring =3D &rdev->ring[RADEON_RING_TYPE_GFX_IN= DEX]; >> r600_cp_stop(rdev); >> - radeon_ring_fini(rdev, &rdev->ring[RADEON_RING_TYPE_GFX_INDEX]); >> + radeon_ring_fini(rdev, ring); >> + radeon_scratch_free(rdev, ring->rptr_save_reg); >> } >> >> >> @@ -2568,7 +2577,12 @@ void r600_ring_ib_execute(struct radeon_device *r= dev, struct radeon_ib *ib) >> { >> struct radeon_ring *ring =3D &rdev->ring[ib->ring]; >> >> - /* FIXME: implement */ >> + if (ring->rptr_save_reg) { >> + uint32_t next_rptr =3D ring->wptr + 2 + 4; >> + radeon_ring_write(ring, PACKET0(ring->rptr_save_reg, 0)); >> + radeon_ring_write(ring, next_rptr); >> + } >> + >> radeon_ring_write(ring, PACKET3(PACKET3_INDIRECT_BUFFER, 2)); >> radeon_ring_write(ring, >> #ifdef __BIG_ENDIAN >> diff --git a/drivers/gpu/drm/radeon/radeon.h b/drivers/gpu/drm/radeon/ra= deon.h >> index 872270c..64d39ad 100644 >> --- a/drivers/gpu/drm/radeon/radeon.h >> +++ b/drivers/gpu/drm/radeon/radeon.h >> @@ -622,6 +622,7 @@ struct radeon_ring { >> unsigned rptr; >> unsigned rptr_offs; >> unsigned rptr_reg; >> + unsigned rptr_save_reg; >> unsigned wptr; >> unsigned wptr_old; >> unsigned wptr_reg; >> diff --git a/drivers/gpu/drm/radeon/radeon_ring.c b/drivers/gpu/drm/rade= on/radeon_ring.c >> index 0873834..ce8eb9d 100644 >> --- a/drivers/gpu/drm/radeon/radeon_ring.c >> +++ b/drivers/gpu/drm/radeon/radeon_ring.c >> @@ -451,6 +451,10 @@ static int radeon_debugfs_ring_info(struct seq_file= *m, void *data) >> count =3D (ring->ring_size / 4) - ring->ring_free_dw; >> seq_printf(m, "wptr(0x%04x): 0x%08x\n", ring->wptr_reg, RREG32(= ring->wptr_reg)); >> seq_printf(m, "rptr(0x%04x): 0x%08x\n", ring->rptr_reg, RREG32(= ring->rptr_reg)); >> + if (ring->rptr_save_reg) { >> + seq_printf(m, "rptr next(0x%04x): 0x%08x\n", ring->rptr_= save_reg, >> + RREG32(ring->rptr_save_reg)); >> + } >> seq_printf(m, "driver's copy of the wptr: 0x%08x\n", ring->wptr= ); >> seq_printf(m, "driver's copy of the rptr: 0x%08x\n", ring->rptr= ); >> seq_printf(m, "%u free dwords in ring\n", ring->ring_free_dw); >> diff --git a/drivers/gpu/drm/radeon/rv770.c b/drivers/gpu/drm/radeon/rv7= 70.c >> index b4b1256..eb4704e 100644 >> --- a/drivers/gpu/drm/radeon/rv770.c >> +++ b/drivers/gpu/drm/radeon/rv770.c >> @@ -358,8 +358,10 @@ static int rv770_cp_load_microcode(struct radeon_de= vice *rdev) >> >> void r700_cp_fini(struct radeon_device *rdev) >> { >> + struct radeon_ring *ring =3D &rdev->ring[RADEON_RING_TYPE_GFX_IN= DEX]; >> r700_cp_stop(rdev); >> - radeon_ring_fini(rdev, &rdev->ring[RADEON_RING_TYPE_GFX_INDEX]); >> + radeon_ring_fini(rdev, ring); >> + radeon_scratch_free(rdev, ring->rptr_save_reg); >> } >> >> /* >> diff --git a/drivers/gpu/drm/radeon/si.c b/drivers/gpu/drm/radeon/si.c >> index f61b550..c7ace21 100644 >> --- a/drivers/gpu/drm/radeon/si.c >> +++ b/drivers/gpu/drm/radeon/si.c >> @@ -1765,6 +1765,12 @@ void si_ring_ib_execute(struct radeon_device *rde= v, struct radeon_ib *ib) >> struct radeon_ring *ring =3D &rdev->ring[ib->ring]; >> u32 header; >> >> + if (ring->rptr_save_reg) { >> + uint32_t next_rptr =3D ring->wptr + 2 + 4; > Same as ni I would rather also skip the surface sync so add another + 8 Dito. > >> + radeon_ring_write(ring, PACKET0(ring->rptr_save_reg, 0)); >> + radeon_ring_write(ring, next_rptr); >> + } >> + >> if (ib->is_const_ib) >> header =3D PACKET3(PACKET3_INDIRECT_BUFFER_CONST, 2); >> else >> @@ -1917,10 +1923,20 @@ static int si_cp_start(struct radeon_device *rde= v) >> >> static void si_cp_fini(struct radeon_device *rdev) >> { >> + struct radeon_ring *ring; >> si_cp_enable(rdev, false); >> - radeon_ring_fini(rdev, &rdev->ring[RADEON_RING_TYPE_GFX_INDEX]); >> - radeon_ring_fini(rdev, &rdev->ring[CAYMAN_RING_TYPE_CP1_INDEX]); >> - radeon_ring_fini(rdev, &rdev->ring[CAYMAN_RING_TYPE_CP2_INDEX]); >> + >> + ring =3D &rdev->ring[RADEON_RING_TYPE_GFX_INDEX]; >> + radeon_ring_fini(rdev, ring); >> + radeon_scratch_free(rdev, ring->rptr_save_reg); >> + >> + ring =3D &rdev->ring[CAYMAN_RING_TYPE_CP1_INDEX]; >> + radeon_ring_fini(rdev, ring); >> + radeon_scratch_free(rdev, ring->rptr_save_reg); >> + >> + ring =3D &rdev->ring[CAYMAN_RING_TYPE_CP2_INDEX]; >> + radeon_ring_fini(rdev, ring); >> + radeon_scratch_free(rdev, ring->rptr_save_reg); >> } >> >> static int si_cp_resume(struct radeon_device *rdev) >> -- >> 1.7.9.5 >> >> _______________________________________________ >> dri-devel mailing list >> dri-devel@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/dri-devel Christian.