Bug 28077

(In reply to comment #1)
> Maybe all it needs is a similar fix to the one applied to exaHWCopyNtoN() for
> the non-accelerated equivalent fbCopyNtoN()? 

ExaCheckCopyNtoN() should take care of it (that's how the exaHWCopyNtoN() fix works). Basically we need to find out why the PrepareAccess calls in there don't result in ->devPrivate.ptr being non-NULL for all pixmaps involved.

BTW, does the psb driver set the EXA_MIXED_PIXMAPS flag?

(In reply to comment #1)
> ExaCheckCopyNtoN() should take care of it (that's how the exaHWCopyNtoN() fix
> works). Basically we need to find out why the PrepareAccess calls in there
> don't result in ->devPrivate.ptr being non-NULL for all pixmaps involved.
>

Any suggestions on how to debug this further?

> BTW, does the psb driver set the EXA_MIXED_PIXMAPS flag?

No, it doesn't set the EXA_MIXED_PIXMAPS flag.

(In reply to comment #2)
> (In reply to comment #1)
> > ExaCheckCopyNtoN() should take care of it (that's how the exaHWCopyNtoN() fix
> > works). Basically we need to find out why the PrepareAccess calls in there
> > don't result in ->devPrivate.ptr being non-NULL for all pixmaps involved.
> 
> Any suggestions on how to debug this further?

I'd trace the pExaScr->prepare_access_reg / exaPrepareAccess calls in ExaCheckCopyNtoN to try and find out the above.


> > BTW, does the psb driver set the EXA_MIXED_PIXMAPS flag?
> 
> No, it doesn't set the EXA_MIXED_PIXMAPS flag.

Actually I guess it still uses the 'classic' EXA scheme, where this flag isn't relevant. Maybe you can attach a log file to clarify this and maybe other things.

Created attachment 35772 [details]
Xorg log of crash with psb driver

Created attachment 35773 [details]
full backtrace of core after crash

I took the liberty of adding my log and gdb backtrace, as I have the same problem on my pc.

Any progress on this? It seems the main blocking point on having 3D support on poulsbo drivers under xorg 1.7.
Thanks.

(In reply to comment #3)
> (In reply to comment #2)
> > (In reply to comment #1)
> > > ExaCheckCopyNtoN() should take care of it (that's how the exaHWCopyNtoN() fix
> > > works). Basically we need to find out why the PrepareAccess calls in there
> > > don't result in ->devPrivate.ptr being non-NULL for all pixmaps involved.
> > 
> > Any suggestions on how to debug this further?
> 
> I'd trace the pExaScr->prepare_access_reg / exaPrepareAccess calls in
> ExaCheckCopyNtoN to try and find out the above.
> 
> 
> > > BTW, does the psb driver set the EXA_MIXED_PIXMAPS flag?
> > 
> > No, it doesn't set the EXA_MIXED_PIXMAPS flag.
> 
> Actually I guess it still uses the 'classic' EXA scheme, where this flag isn't
> relevant. Maybe you can attach a log file to clarify this and maybe other
> things.

Is it possible that our PrepareAccess handler is never called? I see not a single invocation in our xorg.0.log  ... (I added some ErrorF messages to it, as i did on other handlers)

Some other strange thing is, ->devPrivate.ptr is sometimes NULL on xorg side, but for the same object (same address) on our driver side, it is filled in allright...

-Yves

I found a bug in EXA, notably exa_classic.

In case you want to copy a region with source = dest, you have the same pixmap as source and dest.

At the end of exaPixmapIsOffscreen_classic() the devPrivate.ptr is reset to NULL (look at the sources).

Now this is what happens in ExaCheckCopyNtoN:

exaPrepareAccess( pDst );
   Calls IsOffscreen()
      sets devPrivate.ptr to NULL
   sets up devPrivate.ptr to real pointer
   Everything OK
exaPrepareAccess( pSrc );
   Calls IsOffscreen()
      sets devPrivate.ptr to NULL
   BAILS OUT CAUSE OF NESTED OPERATION SINCE DST EQUALS SRC

We end up with devPrivate.ptr as NULL, and that is clearly wrong.
   
Simple patch will follow shortly

Yves De Muyter

Created attachment 36116 [details] [review]
Patch to fix the issue

This fixed a bug in EXA. exaPixmapIsOffscreen_classic() should not set devPrivate.ptr to but set it to the previous value

Sounds good, but please attach a patch generated by Git or diff -u, or just send it to the xorg-devel mailing list for review.

Created attachment 36156 [details] [review]
unified, not inverted version of yves' patch

I believe as well as being old-style and without context, Yves' patch is inverted. I think this is what he meant (this is against 1.8.0). Untested, I'm just looking at Yves' patch to figure out how it's meant to be.

Created attachment 36157 [details] [review]
udiff tested version of Yves' patch

Here is the version we are using on the gma500 ppa. It applies successfully, and seems to fix the bug, at least now the driver won't crash.

Review of attachment 36157 [details] [review]:

Looks good, though I'd move the declaration of old_ptr into the block where it's used.

(In reply to comment #14)
> Review of attachment 36157 [details] [review]:
> 
> Looks good, though I'd move the declaration of old_ptr into the block where
> it's used.

That's exactly what mine did, and I think what Yves' did, if you count the line numbers and check the indent :)

Well, I just applied Yves patch then generated a udiff... :) But I can move the declaration inside the block and regenerate and test the patch, if you guys want.

Note that the patch will need to be posted to the xorg-devel mailing list to be considered for inclusion.

Sent a clean version of the patch on the mailing list:
http://lists.x.org/archives/xorg-devel/2010-June/009970.html

Fix has been applied to the master branch, thanks.