[PATCH 2/6] exa: clean up some more code

[Michel Dänzer]

On Son, 2009-03-01 at 01:36 +0100, Maarten Maathuis wrote:
> 
> @@ -73,8 +62,9 @@ unsigned long
>  exaGetPixmapOffset(PixmapPtr pPix)
>  {
>      ExaScreenPriv (pPix->drawable.pScreen);
> +    ExaPixmapPriv (pPix);
>  
> -    return (CARD8 *)ExaGetPixmapAddress(pPix) - pExaScr->info->memoryBase;
> +    return (CARD8 *)pExaPixmap->fb_ptr - pExaScr->info->memoryBase;
>  }
>  
>  void *

This looks good, might want to put it in the same commit which sets
pExaPixmap->fb_ptr for the screen pixmap though.


> @@ -449,15 +439,20 @@ exaModifyPixmapHeader(PixmapPtr pPixmap, int width, int height, int depth,
>  	}
>      }
>  
> -
>      if (pExaScr->info->ModifyPixmapHeader) {
>  	ret = pExaScr->info->ModifyPixmapHeader(pPixmap, width, height, depth,
>  						bitsPerPixel, devKind, pPixData);
>  	if (ret == TRUE)
> -	    return ret;
> +	    goto out;
>      }
> -    return pExaScr->SavedModifyPixmapHeader(pPixmap, width, height, depth,
> +    ret = pExaScr->SavedModifyPixmapHeader(pPixmap, width, height, depth,
>  					    bitsPerPixel, devKind, pPixData);
> +
> +out:
> +    /* Always NULL this, we don't want lingering pointers. */
> +    pPixmap->devPrivate.ptr = NULL;
> +
> +    return ret;
>  }

This looks like part of my patch from
http://bugs.freedesktop.org/show_bug.cgi?id=20212 . I'd prefer keeping
that as a separate change.


> @@ -520,14 +515,26 @@ exaGetOffscreenPixmap (DrawablePtr pDrawable, int *xp, int *yp)
>  Bool
>  ExaDoPrepareAccess(DrawablePtr pDrawable, int index)
>  {
> -    ScreenPtr	    pScreen = pDrawable->pScreen;
> -    ExaScreenPriv  (pScreen);
> -    PixmapPtr	    pPixmap = exaGetDrawablePixmap (pDrawable);
> -    Bool	    offscreen = exaPixmapIsOffscreen(pPixmap);
> +    ScreenPtr pScreen = pDrawable->pScreen;
> +    ExaScreenPriv (pScreen);
> +    PixmapPtr pPixmap = exaGetDrawablePixmap (pDrawable);
> +    ExaPixmapPriv(pPixmap);
> +    Bool offscreen;
> +
> +    if (!pExaPixmap)
> +	FatalError("Calling PrepareAccess on a pixmap not known to exa.\n");
>  
> -    /* Unhide pixmap pointer */
> -    if (pPixmap->devPrivate.ptr == NULL && !(pExaScr->info->flags & EXA_HANDLES_PIXMAPS)) {
> -	pPixmap->devPrivate.ptr = ExaGetPixmapAddress(pPixmap);
> +    if (pPixmap->devPrivate.ptr != NULL)
> +	FatalError("Unbalanced Prepare/FinishAccess or data corruption."
> +	    "pPixmap->devPrivate.ptr is %p.\n", pPixmap->devPrivate.ptr);

I'm not sure I like the proliferation of FatalErrors; like assert()s
they should only be used to catch conditions which 'can never happen'.
Is that true for all the cases where you want to add FatalError calls?


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