[PATCH] mi: removed the invisible cursor sprite; use NullCursor instead.

[Oliver McFadden]

On Tue, 2010-05-18 at 15:16 +0200, Vignatti Tiago (Nokia-D/Helsinki)
wrote:
> On Tue, May 18, 2010 at 08:34:39AM +0200, ext Keith Packard wrote:
> > On Tue, 18 May 2010 08:44:27 +0300, Oliver McFadden <oliver.mcfadden at nokia.com> wrote:
> > 
> > > I suppose for HW cursor it doesn't really matter since we wouldn't take
> > > damage on that, right? We just draw everything normally, then tell the
> > > HW to draw a sprite at X,Y? (Again, haven't looked at HW cursor code so
> > > perhaps I'm totally off the mark.)
> > 
> > Right, there won't be damage, the question is whether I can remove the
> > empty invisible cursor safely now, or whether drivers are all going to
> > explode when presented with a NULL cursor.
> 
> The patch is inserting a conditional to not let the server goes further from
> miPointerRealizeCursor when pCursor is null, i.e., when cursor is hidden. The
> stack bellow (without the patch applied) shows that there's a plenty of code
> to run when hardware cursor is being used after this function:
> 
> 
> #0  miDCRealizeCursor (pScreen=0x997b6d8, pCursor=0x99a4940) at
> midispcur.c:178
> #1  0x081fb557 in miSpriteRealizeCursor (pDev=0x99b9e80, pScreen=0x997b6d8, 
>     pCursor=0x99a4940) at misprite.c:775
> #2  0x081d4856 in xf86CursorRealizeCursor (pDev=0x99b9e80, pScreen=0x997b6d8, 
>     pCurs=0x99a4940) at xf86Cursor.c:278
> #3  0x080c85d3 in VGAarbiterSpriteRealizeCursor (pDev=0x99b9e80, 
>     pScreen=0x997b6d8, pCur=0x99a4940) at xf86VGAarbiter.c:981
> #4  0x080ab75e in miPointerRealizeCursor (pDev=0x99b9e80, pScreen=0x997b6d8, 
>     pCursor=0x99a4940) at mipointer.c:168
> #5  0x080c6a18 in VGAarbiterRealizeCursor (pDev=0x99b9e80, pScreen=0x997b6d8, 
>     pCursor=0x99a4940) at xf86VGAarbiter.c:436
> #6  0x081a78bb in AnimCurRealizeCursor (pDev=0x99b9e80, pScreen=0x997b6d8, 
>     pCursor=0x99a4940) at animcur.c:283
> #7  0x080fc827 in CursorDisplayCursor (pDev=0x99b9e80, pScreen=0x997b6d8, 
>     pCursor=0x99c1be0) at cursor.c:154
> #8  0x081a774d in AnimCurDisplayCursor (pDev=0x99b9e80, pScreen=0x997b6d8, 
>     pCursor=0x99c1be0) at animcur.c:247
> #9  0x08077856 in InitializeSprite (pDev=0x99b9e80, pWin=0x99a77e8)
>     at events.c:3017
> #10 0x0806d61f in EnableDevice (dev=0x99b9e80, sendevent=1 '\001')
>     at devices.c:292
> #11 0x0806e114 in InitCoreDevices () at devices.c:606
> #12 0x08064bcd in main (argc=3, argv=0xbf9d90f4, envp=0xbf9d9104) at
> main.c:254
> 
> 
> The code that runs after miPointerRealizeCursor looks worth, because it
> initializes a bunch of sprite and pointer structures. So I'd say it's not
> safety to insert the conditional there for hardware cursors. Another approach
> should be used or certify that such code ran after miPointerRealizeCursor is
> worthless.
> 
> On the other hand, I tested in my machine the patch and it works okay for hw
> cursor case! Xserver regen also works okay. To have a complete testing
> coverage we should test for multiple cursors though (two cases here: sw
> cursors only and mixed of sw cursors + the hw one).

This is good news. I think we are most likely okay then; as I said,
there are NULL checks in the HW cursor code -- I'm just not completely
sure that *every* case is NULL checked.

Running it successfully would seem to prove they are all checked. (I'm
still battling with Coverity and libtool so haven't got analysis results
yet.)

Oh, and from your backtrace above if we're using NullCursor then
miPointerRealizeCursor should return before jumping into the VGA
arbitrator or HW cursor code.

-- Oliver.