Bad caching of the scratch pixmap

[Daniel Drake]

Hi Chris,

Thanks a lot for the quick response.

On Sat, Apr 13, 2013 at 11:14 AM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> Given the same fix works, it does sound like a very similar issue. The
> difference is that the DDX is under control of the creating the scratch
> pixmap in your case and should be well aware of the lifetime constraints
> and so remember to decouple the GPU object when it destroys the screen
> pixmap. So the bug is really in the driver and this just happens to
> workaround the bug by forcing the release

Good that you have a feeling for where the real problem actually is.
I'm happy to fix the driver, but I am facing a bit of a learning
curve. If I get a good grasp on things, I will try to produce some
documentation for the next person...

So, the DDX is in control of pixmap allocations. It does that by
providing these hooks:

    if (pExa->flags & EXA_HANDLES_PIXMAPS)
    {
        pExa->CreatePixmap       = NULL;
        pExa->CreatePixmap2      = mrvlExaCreatePixmap2;
        pExa->DestroyPixmap      = mrvlDestroyPixmap;
        pExa->ModifyPixmapHeader = mrvlModifyPixmapHeader;
        pExa->PixmapIsOffscreen  = mrvlPixmapIsOffscreen;
    }

You say that the driver should "be well aware of the lifetime
constraints and so remember to decouple the GPU object when it
destroys the screen pixmap".

I assume that the scratch pixbuf is created via a call into
->CreatePixmap2, and gets destroyed when ->DestroyPixmap is called.
Those sound like lifetime constraints. Are you suggesting that
DestroyPixmap is being called in the right places but you suspect that
the dove DDX is not correctly tearing down the GPU object in response?

Or are there additional "lifetime constraints" (not involving
->DestroyPixmap calls) that should be somehow considered here? (Is
there any documentation on these topics?)

To aid my understanding I have performed an audit of the
CreatePixmap2, ModifyPixmapHeader and DestroyPixmap calls that happen
without your patch applied. If you have a minute to do a quick sanity
check that what happens looks reasonable it would be much appreciated:
http://dev.laptop.org/ticket/12542#comment:10

It suggests that the badness originates from the reuse of the scratch
buffer, which (in the corruption case) was created while the screen
was rotated, as rotation does use the scratch pixmap:
http://dev.laptop.org/git/projects/xf86-video-dove/tree/src/mrvl_crtc.c#n80

When no rotation is involved, the scratch pixmap is not created until
the point when image drawing happens.

I found that the same issue affected the geode driver in the past:
https://bugs.launchpad.net/ubuntu/+source/xserver-xorg-video-geode/+bug/377929/comments/24
and the fix chosen there (follow the link to the patch) is to make
rotation not use the scratch pixmap.

Does that seem like a correct fix to you? I could do the same in dove.

> when as noted in the bug
> report, the driver is likely to need to take even more precautions with
> an old framebuffer.

I don't understand this part of your mail - which bug report? Old framebuffer?

Thanks
Daniel