[PATCH] EXA: ModifyPixmapHeader_mixed fixes.

[Maarten Maathuis]

2009/12/5 Michel Dänzer <michel at daenzer.net>:
> 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.

Not every driver has an MPH hook and they certainly don't return TRUE
all the time. Why not remove the MPH here and just stick to deleting
the sysram copy? (don't forget deferred pixmaps)

>
>
>> > +           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
>
>