[PATCH] EXA: ModifyPixmapHeader_mixed fixes.

[Michel Dänzer]

On Fri, 2009-12-04 at 20:28 +0100, Maarten Maathuis wrote: 
> 2009/12/4 Michel Dänzer <michel at daenzer.net>:
> >
> > +       /* Need to re-create system copy if there's also a GPU copy */
> 
> These criteria can also be true for a pixmap without gpu copy.

Hmm, true. Something like

has_gpu_copy = exaPixmapHasGpuCopy(pPixmap); 

	if (has_gpu_copy && pExaPixmap->sys_ptr) {
	    ...

then?

> > +       if (pExaPixmap->pDamage && pExaPixmap->sys_ptr) {
> > +           free(pExaPixmap->sys_ptr);
> > +           pExaPixmap->sys_ptr = NULL;
> > +           DamageUnregister(&pPixmap->drawable, pExaPixmap->pDamage);
> > +           DamageDestroy(pExaPixmap->pDamage);
> > +           pExaPixmap->pDamage = NULL;
> > +           REGION_EMPTY(pScreen, &pExaPixmap->validSys);
> > +
> > +           swap(pExaScr, pScreen, ModifyPixmapHeader);
> > +           pScreen->ModifyPixmapHeader(pPixmap, width, height, depth,
> > +                                       bitsPerPixel, devKind, pPixData);
> > +           swap(pExaScr, pScreen, ModifyPixmapHeader);
> > +
> 
> How is this helping, considering it's done twice (the original MPH
> still exists)?

That won't be called if the driver MPH hook returns TRUE.


> > +           pExaPixmap->sys_ptr = pPixmap->devPrivate.ptr;
> > +           pExaPixmap->sys_pitch = pPixmap->devKind;
> > +       }
> >     }
> >
> >     has_gpu_copy = exaPixmapHasGpuCopy(pPixmap);
> > @@ -158,8 +195,10 @@ exaModifyPixmapHeader_mixed(PixmapPtr pPixmap, int width, int height, int depth,
> >     if (pExaScr->info->ModifyPixmapHeader && pExaPixmap->driverPriv) {
> >        ret = pExaScr->info->ModifyPixmapHeader(pPixmap, width, height, depth,
> >                                                bitsPerPixel, devKind, pPixData);
> > -       if (ret == TRUE)
> > +       if (ret == TRUE) {
> > +           REGION_EMPTY(pScreen, &pExaPixmap->validFB);
> >            goto out;
> > +       }
> >     }
> 
> Why not for ret == FALSE?

No particular reason... e.g. the Gallium Xorg state tracker actually
tries to preserve the GPU copy contents as much as possible, but I guess
it would be safer to always clear the validFB region.


-- 
Earthling Michel Dänzer           |                http://www.vmware.com
Libre software enthusiast         |          Debian, X and DRI developer