Bug 24837 - Broken ring info in debugfs
Summary: Broken ring info in debugfs
Status: RESOLVED FIXED
Alias: None
Product: DRI
Classification: Unclassified
Component: DRM/Radeon (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Default DRI bug account
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-11-01 09:11 UTC by Rafał Miłecki
Modified: 2009-12-07 15:01 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
fix ring info in debugfs on r600+ (1.72 KB, patch)
2009-11-01 11:33 UTC, Rafał Miłecki
no flags Details | Splinter Review
fix ring info in debugfs on r600+ V2 (1.79 KB, patch)
2009-11-01 15:14 UTC, Rafał Miłecki
no flags Details | Splinter Review
fix ring info in debugfs on r600+ V3 (1.88 KB, patch)
2009-11-05 14:11 UTC, Rafał Miłecki
no flags Details | Splinter Review

Description Rafał Miłecki 2009-11-01 09:11:25 UTC
First I reported bug #24781 but that was invalid report, there was not bug in code where I thought.

However there are for sure some issues in current ring info debugfs.

**********

1) We count buffer's elements in following way:
r600.c::r600_debugfs_cp_ring_info
count = (rdp + rdev->cp.ring_size - wdp) & rdev->cp.ptr_mask;

a) We don't divide cp.ring_size by 4 but fortunately this doesn't cause problems as we use bit multiplication by mask. However maybe we should do so for code readability?

b) What we count is actually amount of *free* dwords in buffer. This is the same what gets done in radeon_ring_free_size. You can see results of this mistake in bug #24535 comment #6 - amount of free and used dwords is the same which is incorrect. The only exception when this work is empty ring situation. In radeon_ring.c::radeon_ring_free_size we treat 0 as (rdev->cp.ring_size / 4) so we get nice (correct):
> 262128 free dwords in ring
> 0 dwords in ring
then.

Ideally this probably should be:
count = (wdp + rdev->cp.ring_size - rdp) & rdev->cp.ptr_mask;

**********

2) We have inconsistent in reading CP_RB_WPTR and CP_RB_RPTR. This leads to situations like:
> 262127 free dwords in ring
> 0 dwords in ring
where 262127 + 0 != 262144 (and 262144 is buffer size ofc).

That happens because we call radeon_ring_free_size where we calculate amount of free dwords using rdev->cp.rptr and rdev->cp.wptr. Then we read CP_RB_RPTR and CP_RB_WPTR and calculate used dwords. As ring changes often this causes mentioned inconsistency.

I belive we should use rdev->cp.rptr and rdev->cp.wptr in r600_debugfs_cp_ring_info and eventually consider updating rdev->cp.wptr in radeon_ring_free_size.

**********

3) Finally we don't detect ring end in printing ring's content in r600_debugfs_cp_ring_info.
Comment 1 Rafał Miłecki 2009-11-01 11:33:09 UTC
Created attachment 30893 [details] [review]
fix ring info in debugfs on r600+

With this patch applied I get:

# cat r600_ring_info
CP_STAT 0x800000c1
CP_RB_WPTR 0x0000ade0
CP_RB_RPTR 0x0000add0
262128 free dwords in ring
16 dwords in ring
r[44497]=0x10181000
r[44498]=0x00000000
r[44499]=0x000002a0
r[44500]=0xc0016800
r[44501]=0x00000140
r[44502]=0x00000489
r[44503]=0x80000000
r[44504]=0x80000000
r[44505]=0x80000000
r[44506]=0x80000000
r[44507]=0x80000000
r[44508]=0x80000000
r[44509]=0x80000000
r[44510]=0x80000000
r[44511]=0x80000000
r[44512]=0x0901000d
r[44513]=0x000dc1ea

you can compare this to /broken/ data in bug #24535 comment #6
Comment 2 Rafał Miłecki 2009-11-01 15:14:05 UTC
Created attachment 30901 [details] [review]
fix ring info in debugfs on r600+ V2

First version was dumping ring from CP_RB_RPTR + 1 to CP_RB_WPTR + 1
Comment 3 Rafał Miłecki 2009-11-05 14:11:18 UTC
Created attachment 30995 [details] [review]
fix ring info in debugfs on r600+ V3

Both: V1 and V2 missed printing actual value of CP_RB_WPTR and CP_RB_RPTR. Now we print that registers and local value of them.
Comment 4 Rafał Miłecki 2009-11-06 01:11:35 UTC
Example of debug info with patch V3 applied:

CP_STAT 0x800000c1
CP_RB_WPTR 0x000126a0
CP_RB_RPTR 0x000126a0
driver's copy of the CP_RB_WPTR 0x000126a0
driver's copy of the CP_RB_RPTR 0x00012690
262128 free dwords in ring
16 dwords in ring
r[75408]=0xc0023200
r[75409]=0x10101000
r[75410]=0x00000000
r[75411]=0x0000028d
r[75412]=0xc0016800
r[75413]=0x00000140
r[75414]=0x00000e21
r[75415]=0x80000000
r[75416]=0x80000000
r[75417]=0x80000000
r[75418]=0x80000000
r[75419]=0x80000000
r[75420]=0x80000000
r[75421]=0x80000000
r[75422]=0x80000000
r[75423]=0x80000000
r[75424]=0x0030189a
Comment 5 Jerome Glisse 2009-11-23 02:49:58 UTC
I believe the patch is upstream now
Comment 6 Rafał Miłecki 2009-11-26 15:07:46 UTC
(In reply to comment #5)
> I believe the patch is upstream now

It didn't hit drm-next or drm-radeon-testing, not to mention mainline. But OK, let's keep it resolved, I'll post commit when Dave take this patch to some branch.


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.