[PATCH] refactored RandR cursor code plus fix for bug #13405

newer
[Bug 21567] New: Problem with DRI...

Yang Zhao

30 Apr 2009 30 Apr '09

02:50

Attached patch is a refactoring of the RandR-based cursor code to be cleaner and faster. Fix for the cursor curruption issue is also included. (http://bugs.freedesktop.org/show_bug.cgi?id=13405) The non-RandR code path has not been touched, and will still exhibit the bug. The plan is to then rewrite that code path to use the new, cleaner code, which should make everything easier to understand and reduce the number of unnecessary register writes and conditional branches. I don't think a lot of people are using a non-RandR setup, so this should satisfy almost everybody that's currently experiencing cursor issues. -- Yang Zhao http://yangman.ca

Attachments:

0001-Cursor-refactor-RandR-cursor-code-also-fixing-bug.patch (text/x-patch — 4.8 KB)

Show replies by date

Rafał Miłecki

30 Apr 30 Apr

06:11

New subject: [PATCH] refactored RandR cursor code plus fix for bug #13405

2009/4/30 Yang Zhao :

...

Attached patch is a refactoring of the RandR-based cursor code to be cleaner and faster. Fix for the cursor curruption issue is also included. (http://bugs.freedesktop.org/show_bug.cgi?id=13405)

The non-RandR code path has not been touched, and will still exhibit the bug. The plan is to then rewrite that code path to use the new, cleaner code, which should make everything easier to understand and reduce the number of unnecessary register writes and conditional branches. I don't think a lot of people are using a non-RandR setup, so this should satisfy almost everybody that's currently experiencing cursor issues.

If this could break something in my single mode on M82 (RV620)... it didn't :) I just think /we/ like using int hotx = 0, hoty = 0; over + int hotx, hoty; + + hotx = 0; + hoty = 0; -- Rafał Miłecki -- To unsubscribe, e-mail: radeonhd+unsubscribe@opensuse.org For additional commands, e-mail: radeonhd+help@opensuse.org

Matthias Hopf

4 May 4 May

15:15

New subject: [PATCH] refactored RandR cursor code plus fix for bug #13405

On Apr 30, 09 08:11:25 +0200, Rafał Miłecki wrote:

...

I just think /we/ like using int hotx = 0, hoty = 0; over

+ int hotx, hoty; + + hotx = 0; + hoty = 0;

This is a very minor issue. Actually, I think both are reasonable. Definitely nothing I would debate on whether to include a patch or not. Thanks Matthias -- Matthias Hopf __ __ __ Maxfeldstr. 5 / 90409 Nuernberg (_ | | (_ |__ mat@mshopf.de Phone +49-911-74053-715 __) |_| __) |__ R & D www.mshopf.de -- To unsubscribe, e-mail: radeonhd+unsubscribe@opensuse.org For additional commands, e-mail: radeonhd+help@opensuse.org

Matthias Hopf

15:13

New subject: [PATCH] refactored RandR cursor code plus fix for bug #13405

On Apr 29, 09 19:50:52 -0700, Yang Zhao wrote:

...

Attached patch is a refactoring of the RandR-based cursor code to be cleaner and faster. Fix for the cursor curruption issue is also included. (http://bugs.freedesktop.org/show_bug.cgi?id=13405)

Thanks for working on this, Yang!

...

The non-RandR code path has not been touched, and will still exhibit the bug. The plan is to then rewrite that code path to use the new, cleaner code, which should make everything easier to understand and reduce the number of unnecessary register writes and conditional branches. I don't think a lot of people are using a non-RandR setup, so this should satisfy almost everybody that's currently experiencing cursor issues.

Fair enough :)

...

- Code was added to correctly set cursor dimensions. This is disabled as there are apparently no benefits on doing so.

I would actually remove that (unused) code. Yes, I stumbled over the Xserver loading MAX by MAX only as well.

...

@@ -636,11 +644,26 @@ rhdCrtcShowCursor(struct rhdCrtc *Crtc) [...] + /* Move cursor off-screen so it won't be visible*/ + RHDRegWrite(Cursor, Cursor->RegOffset + D1CUR_POSITION, + (Crtc->X + Crtc->Width) << 16 | (Crtc->Y + Crtc->Height)); + RHDRegWrite(Cursor, Cursor->RegOffset + D1CUR_HOT_SPOT, 0); +

Is this consistent with the solution Alex found for how to fix part of the corruptions (never move outside Crtc boundaries + don't let end at multiples of 128)? One way to circumvent this would be to use (0, Crtc->Y+Crtc->Height) as coordinates. What do you think?

...

rhdCrtcSetCursorPosition(struct rhdCrtc *Crtc, int x, int y) [...] - displayCursor(Crtc, TRUE); + RHDRegWrite(Cursor, Cursor->RegOffset + D1CUR_POSITION, x << 16 | y); + RHDRegWrite(Cursor, Cursor->RegOffset + D1CUR_HOT_SPOT, hotx << 16 | hoty);

This doesn't change cursor width any more if necessary (see above). That has been the reason for the change to use displayCursor everywhere. It does fix the cursor corruption issues (while it introduced other issues ;) on a machine here, so I'm positive that this is actually necessary. Matthias -- Matthias Hopf __ __ __ Maxfeldstr. 5 / 90409 Nuernberg (_ | | (_ |__ mat@mshopf.de Phone +49-911-74053-715 __) |_| __) |__ R & D www.mshopf.de -- To unsubscribe, e-mail: radeonhd+unsubscribe@opensuse.org For additional commands, e-mail: radeonhd+help@opensuse.org

Yang Zhao

16:24

New subject: [PATCH] refactored RandR cursor code plus fix for bug #13405

2009/5/4 Matthias Hopf :

...

...
- Code was added to correctly set cursor dimensions. This is disabled as there are apparently no benefits on doing so.

I would actually remove that (unused) code. Yes, I stumbled over the Xserver loading MAX by MAX only as well.

Yeah, lets get rid of that. It was just left over testing code when I squashed a bunch of commits together on rebase.

...

...
@@ -636,11 +644,26 @@ rhdCrtcShowCursor(struct rhdCrtc *Crtc) [...] + /* Move cursor off-screen so it won't be visible*/ + RHDRegWrite(Cursor, Cursor->RegOffset + D1CUR_POSITION, + (Crtc->X + Crtc->Width) << 16 | (Crtc->Y + Crtc->Height)); + RHDRegWrite(Cursor, Cursor->RegOffset + D1CUR_HOT_SPOT, 0); +

Is this consistent with the solution Alex found for how to fix part of the corruptions (never move outside Crtc boundaries + don't let end at multiples of 128)? One way to circumvent this would be to use (0, Crtc->Y+Crtc->Height) as coordinates. What do you think?

This is to work around a corner case where the cursors would get enabled while the cursor coordinates for the "inactive" CRTC is still in the visible area, leaving a ghost pointer. Last I checked, it's worked around in radeon by setting the cursor dimension to 1x1. In any case, the multiples-of-128 and don't-be-outside logic wasn't included. Lets use 0 for X if it may be an issue.

...

...
rhdCrtcSetCursorPosition(struct rhdCrtc *Crtc, int x, int y) [...] - displayCursor(Crtc, TRUE); + RHDRegWrite(Cursor, Cursor->RegOffset + D1CUR_POSITION, x << 16 | y); + RHDRegWrite(Cursor, Cursor->RegOffset + D1CUR_HOT_SPOT, hotx << 16 | hoty);

This doesn't change cursor width any more if necessary (see above). That has been the reason for the change to use displayCursor everywhere. It does fix the cursor corruption issues (while it introduced other issues ;) on a machine here, so I'm positive that this is actually necessary.

What issues are caused by cursor going beyond the CRTC area? I wasn't able to reproduce anything obvious on my M56 for the non-rotated non-panning cases. -- Yang Zhao http://yangman.ca -- To unsubscribe, e-mail: radeonhd+unsubscribe@opensuse.org For additional commands, e-mail: radeonhd+help@opensuse.org

Matthias Hopf

16:34

New subject: [PATCH] refactored RandR cursor code plus fix for bug #13405

On May 04, 09 09:24:05 -0700, Yang Zhao wrote:

...

...
...
@@ -636,11 +644,26 @@ rhdCrtcShowCursor(struct rhdCrtc *Crtc) [...] + /* Move cursor off-screen so it won't be visible*/ + RHDRegWrite(Cursor, Cursor->RegOffset + D1CUR_POSITION, + (Crtc->X + Crtc->Width) << 16 | (Crtc->Y + Crtc->Height)); + RHDRegWrite(Cursor, Cursor->RegOffset + D1CUR_HOT_SPOT, 0); +

Is this consistent with the solution Alex found for how to fix part of the corruptions (never move outside Crtc boundaries + don't let end at multiples of 128)? One way to circumvent this would be to use (0, Crtc->Y+Crtc->Height) as coordinates. What do you think?

This is to work around a corner case where the cursors would get enabled while the cursor coordinates for the "inactive" CRTC is still in the visible area, leaving a ghost pointer. Last I checked, it's

Yes, I have seen that. Side effect by my last "fix".

...

worked around in radeon by setting the cursor dimension to 1x1.

Ah. I see. This is still one pixel wrong ;) I like moving the cursor outside the visible screen better.

...

In any case, the multiples-of-128 and don't-be-outside logic wasn't included. Lets use 0 for X if it may be an issue.

0 should be safe. I don't think coordinates outside the screen with a higher y are of any issue, the logic in the Alex' patch for the issue he found in-house only addressed X as well.

...

...
...
rhdCrtcSetCursorPosition(struct rhdCrtc *Crtc, int x, int y) [...] - displayCursor(Crtc, TRUE); + RHDRegWrite(Cursor, Cursor->RegOffset + D1CUR_POSITION, x << 16 | y); + RHDRegWrite(Cursor, Cursor->RegOffset + D1CUR_HOT_SPOT, hotx << 16 | hoty);

This doesn't change cursor width any more if necessary (see above). That has been the reason for the change to use displayCursor everywhere. It does fix the cursor corruption issues (while it introduced other issues ;) on a machine here, so I'm positive that this is actually necessary.

What issues are caused by cursor going beyond the CRTC area? I wasn't

I don't know, but this was included and commented on in the same commit that fixed the end-on-multiple-of-128 patch. On some cards the cursor is garbled more or less if you hit a multiple of 128 with the right edge of the cursor - and keeps this for the neighboring pixels, even if the cursor is moved again. We have a system here that exhibits this bug. It only happens after quite some running time, so it's probably a speed path in the cache design. Matthias -- Matthias Hopf __ __ __ Maxfeldstr. 5 / 90409 Nuernberg (_ | | (_ |__ mat@mshopf.de Phone +49-911-74053-715 __) |_| __) |__ R & D www.mshopf.de -- To unsubscribe, e-mail: radeonhd+unsubscribe@opensuse.org For additional commands, e-mail: radeonhd+help@opensuse.org

Yang Zhao

16:55

New subject: [PATCH] refactored RandR cursor code plus fix for bug #13405

2009/5/4 Matthias Hopf :

...

On May 04, 09 09:24:05 -0700, Yang Zhao wrote:

...
...
...
@@ -636,11 +644,26 @@ rhdCrtcShowCursor(struct rhdCrtc *Crtc) [...] + /* Move cursor off-screen so it won't be visible*/ + RHDRegWrite(Cursor, Cursor->RegOffset + D1CUR_POSITION, + (Crtc->X + Crtc->Width) << 16 | (Crtc->Y + Crtc->Height)); + RHDRegWrite(Cursor, Cursor->RegOffset + D1CUR_HOT_SPOT, 0); +

Is this consistent with the solution Alex found for how to fix part of the corruptions (never move outside Crtc boundaries + don't let end at multiples of 128)? One way to circumvent this would be to use (0, Crtc->Y+Crtc->Height) as coordinates. What do you think?

This is to work around a corner case where the cursors would get enabled while the cursor coordinates for the "inactive" CRTC is still in the visible area, leaving a ghost pointer. Last I checked, it's

Yes, I have seen that. Side effect by my last "fix".

AFAICS it's just a logical result of RandR's cursor behaviour and us requiring both cursors to always be enabled. Moving from one CRTC to the other while passing through an invalid virtual area (due to mismatched display dimensions) results in the calls: setPosition(crtc1, <somewhere on screen>) => disable(crtc1) => setPosition(crtc2, <new position>) => enable(crtc2)

...

...
What issues are caused by cursor going beyond the CRTC area? I wasn't

I don't know, but this was included and commented on in the same commit that fixed the end-on-multiple-of-128 patch. On some cards the cursor is garbled more or less if you hit a multiple of 128 with the right edge of the cursor - and keeps this for the neighboring pixels, even if the cursor is moved again. We have a system here that exhibits this bug. It only happens after quite some running time, so it's probably a speed path in the cache design.

In that case, I can work this back into rhdCrtcSetPosition(). displayCursor() is way too overloaded right now, and I would like to get rid of it entirely. It would be nice to get a confirmation if this is still required with the reworked code. -- Yang Zhao http://yangman.ca -- To unsubscribe, e-mail: radeonhd+unsubscribe@opensuse.org For additional commands, e-mail: radeonhd+help@opensuse.org

Matthias Hopf

17:23

New subject: [PATCH] refactored RandR cursor code plus fix for bug #13405

On May 04, 09 09:55:16 -0700, Yang Zhao wrote:

...

...
We have a system here that exhibits this bug. It only happens after quite some running time, so it's probably a speed path in the cache design.

In that case, I can work this back into rhdCrtcSetPosition(). displayCursor() is way too overloaded right now, and I would like to get rid of it entirely.

Ok.

...

It would be nice to get a confirmation if this is still required with the reworked code.

Well, given that the bug doesn't show up any more with the size switching code, and it did before, I don't see how this would be gone with your changes alone :) CU Matthias -- Matthias Hopf __ __ __ Maxfeldstr. 5 / 90409 Nuernberg (_ | | (_ |__ mat@mshopf.de Phone +49-911-74053-715 __) |_| __) |__ R & D www.mshopf.de -- To unsubscribe, e-mail: radeonhd+unsubscribe@opensuse.org For additional commands, e-mail: radeonhd+help@opensuse.org

Yang Zhao

22:27

Round 2. Alex managed to find the root of the easy cursor corruption issue: the cursor mode should be the same for both CRTCs even when disabled. This is 0001. 0002 is the refactoring. It's almost trivial now that we don't need the logic to keep the cursor enabled on both CRTCs. 0003 contains the multiple-of-128px and outside-viewport code with some tweaks, and is thus different from radeon. My system that's set up with dual-screen doesn't seem to be panning capable, so the viewport part is untested; please scrutinize 0003 with extra attention. -- Yang Zhao http://yangman.ca

Alex Deucher

23:02

New subject: [PATCH] refactored RandR cursor code plus fix for bug #13405

On Mon, May 4, 2009 at 6:27 PM, Yang Zhao wrote:

...

Round 2.

Alex managed to find the root of the easy cursor corruption issue: the cursor mode should be the same for both CRTCs even when disabled. This is 0001.

0002 is the refactoring. It's almost trivial now that we don't need the logic to keep the cursor enabled on both CRTCs.

0003 contains the multiple-of-128px and outside-viewport code with some tweaks, and is thus different from radeon. My system that's set up with dual-screen doesn't seem to be panning capable, so the viewport part is untested; please scrutinize 0003 with extra attention.

These look good. Note, that the issue addressed by patch 0003 is only applicable when both crtcs are enabled. Alex -- To unsubscribe, e-mail: radeonhd+unsubscribe@opensuse.org For additional commands, e-mail: radeonhd+help@opensuse.org

Yang Zhao

5 May 5 May

01:04

New subject: [PATCH] refactored RandR cursor code plus fix for bug #13405

2009/5/4 Alex Deucher :

...

On Mon, May 4, 2009 at 6:27 PM, Yang Zhao wrote:

...
Round 2. ... 0003 contains the multiple-of-128px and outside-viewport code with some tweaks, and is thus different from radeon. My system that's set up with dual-screen doesn't seem to be panning capable, so the viewport part is untested; please scrutinize 0003 with extra attention.

These look good. Note, that the issue addressed by patch 0003 is only applicable when both crtcs are enabled.

Right; thanks for the reminder. New 0003 attached. -- Yang Zhao http://yangman.ca

Matthias Hopf

11:01

New subject: [PATCH] refactored RandR cursor code plus fix for bug #13405

On May 04, 09 15:27:00 -0700, Yang Zhao wrote:

...

Alex managed to find the root of the easy cursor corruption issue: the cursor mode should be the same for both CRTCs even when disabled. This is 0001.

Superb! Applying now.

...

0002 is the refactoring. It's almost trivial now that we don't need the logic to keep the cursor enabled on both CRTCs.

...

diff --git a/src/rhd_cursor.c b/src/rhd_cursor.c index 3a0c7a9..a2b52b1 100644 --- a/src/rhd_cursor.c +++ b/src/rhd_cursor.c @@ -625,10 +625,9 @@ void rhdCrtcShowCursor(struct rhdCrtc *Crtc) { struct rhdCursor *Cursor = Crtc->Cursor; - - lockCursor (Cursor, TRUE); - displayCursor(Crtc, TRUE); - lockCursor (Cursor, FALSE); + lockCursor(Cursor, TRUE); + enableCursor(Cursor, TRUE); + lockCursor(Cursor, FALSE);

I don't like the whitespace changes - IMHO it's much harder to read.

...

@@ -651,11 +649,27 @@ void rhdCrtcSetCursorPosition(struct rhdCrtc *Crtc, int x, int y) { struct rhdCursor *Cursor = Crtc->Cursor; + int hotx, hoty;

What I don't really like here is the duplication of hotspot calculation. That has been the reason behind displayCursor() in the first place. Not really sure what to do about that, it's probably best to first apply this and then refactor the logic. Also please separate logic and register access, so setCursorPos() should be used instead of direct RHDRegWrite(). It's a small static function, so it will be inlined anyway.

...

0003 contains the multiple-of-128px and outside-viewport code with some tweaks, and is thus different from radeon. My system that's set up with dual-screen doesn't seem to be panning capable, so the viewport part is untested; please scrutinize 0003 with extra attention.

Why doesn't it seem to be panning capable? Anyway, this patch looks good. Thanks Matthias -- Matthias Hopf __ __ __ Maxfeldstr. 5 / 90409 Nuernberg (_ | | (_ |__ mat@mshopf.de Phone +49-911-74053-715 __) |_| __) |__ R & D www.mshopf.de -- To unsubscribe, e-mail: radeonhd+unsubscribe@opensuse.org For additional commands, e-mail: radeonhd+help@opensuse.org

Yang Zhao

17:12

New subject: [PATCH] refactored RandR cursor code plus fix for bug #13405

2009/5/5 Matthias Hopf :

...

On May 04, 09 15:27:00 -0700, Yang Zhao wrote:

...
0002 is the refactoring. It's almost trivial now that we don't need the logic to keep the cursor enabled on both CRTCs.

...
diff --git a/src/rhd_cursor.c b/src/rhd_cursor.c index 3a0c7a9..a2b52b1 100644 --- a/src/rhd_cursor.c +++ b/src/rhd_cursor.c @@ -625,10 +625,9 @@ void rhdCrtcShowCursor(struct rhdCrtc *Crtc) { struct rhdCursor *Cursor = Crtc->Cursor; - - lockCursor (Cursor, TRUE); - displayCursor(Crtc, TRUE); - lockCursor (Cursor, FALSE); + lockCursor(Cursor, TRUE); + enableCursor(Cursor, TRUE); + lockCursor(Cursor, FALSE);

I don't like the whitespace changes - IMHO it's much harder to read.

Fixed.

...

...
@@ -651,11 +649,27 @@ void rhdCrtcSetCursorPosition(struct rhdCrtc *Crtc, int x, int y) { struct rhdCursor *Cursor = Crtc->Cursor; + int hotx, hoty;

What I don't really like here is the duplication of hotspot calculation. That has been the reason behind displayCursor() in the first place. Not really sure what to do about that, it's probably best to first apply this and then refactor the logic.

displayCursor() currently contains pretty much everything: position setting, size adjustment, and visibility toggle, along with logic to determine if the cursor is within each CRTC's visible area. For RandR cursor, this duplicates a lot of the work the server is already doing. Once the new RandR cursor code is in place, the idea is to refactor the non-RandR cursor code such that we check which CRTC is affected, then simply call the appropriate rhdCrtc*() function.

...

Also please separate logic and register access, so setCursorPos() should be used instead of direct RHDRegWrite(). It's a small static function, so it will be inlined anyway.

Done. I seem to have forgotten setCursorPos() existed. Revised 0002 and 0003 attached. -- Yang Zhao http://yangman.ca

Matthias Hopf

6 May 6 May

10:08

New subject: [PATCH] refactored RandR cursor code plus fix for bug #13405

All looking good now, applied. Thanks again Matthias -- Matthias Hopf __ __ __ Maxfeldstr. 5 / 90409 Nuernberg (_ | | (_ |__ mat@mshopf.de Phone +49-911-74053-715 __) |_| __) |__ R & D www.mshopf.de -- To unsubscribe, e-mail: radeonhd+unsubscribe@opensuse.org For additional commands, e-mail: radeonhd+help@opensuse.org

5498

Age (days ago)

5504

Last active (days ago)

List overview

Download

13 comments

4 participants

participants (4)

Alex Deucher
Matthias Hopf
Rafał Miłecki
Yang Zhao