[Xorg-driver-geode] [PATCH 1/7] Prevent the pixmap migration if the geode GP can not do the acceleration.

[Mart Raudsepp]

On P, 2010-06-13 at 18:47 +0800, Huang, FrankR wrote:
> From: Frank Huang <frankr.huang at amd.com>
> 
> Bring all the "return FALSE" condition forward from lx_prepare_composite
> to lx_check_composite. The Xserver will handle this condition. See more
> on Freedesktop Bugzilla #27243
> 
> Signed-off-by: Frank Huang <frankr.huang at amd.com>
> ---
>  src/lx_exa.c |   52 +++++++++++++++++++++++++++++-----------------------
>  1 files changed, 29 insertions(+), 23 deletions(-)
> 
> diff --git a/src/lx_exa.c b/src/lx_exa.c
> index b267cc0..ab33124 100644
> --- a/src/lx_exa.c
> +++ b/src/lx_exa.c
> @@ -536,12 +536,15 @@ static Bool
>  lx_check_composite(int op, PicturePtr pSrc, PicturePtr pMsk, PicturePtr pDst)
>  {
>      GeodeRec *pGeode = GEODEPTR_FROM_PICTURE(pDst);
> +    const struct exa_format_t *srcFmt, *dstFmt;
>  
>      /* Check that the operation is supported */
>  
>      if (op > PictOpAdd)
>  	return FALSE;
>  
> +    /* We need the off-screen buffer to do the multipass work */
> + 

Extra whitespace here. Extra line too that I don't agree, but
consistency is good enough argument for now, so we can kill the empty
line later on along with all the others. There's just a lone space in
here as well besides the linebreak.

> @@ -583,21 +586,23 @@ lx_check_composite(int op, PicturePtr pSrc, PicturePtr pMsk, PicturePtr pDst)
>  	return FALSE;
>  
>      if (pMsk && op != PictOpClear) {
> +	struct blend_ops_t *opPtr = &lx_alpha_ops[op * 2];
> +	int direction = (opPtr->channel == CIMGP_CHANNEL_A_SOURCE) ? 0 : 1;
> +
> +	/* Direction 0 indicates src->dst, 1 indiates dst->src */
> +	if (((direction == 0) && (pSrc->pDrawable->bitsPerPixel < 16)) ||
> +	    ((direction == 1) && (pDst->pDrawable->bitsPerPixel < 16))) {
> +	    ErrorF("Can't do mask blending with less then 16bpp\n");
> +	    return FALSE;
> +	}
<snip> 
> -	/* Direction 0 indicates src->dst, 1 indiates dst->src */
> -
> -	if (((direction == 0) && (pxSrc->drawable.bitsPerPixel < 16)) ||
> -	    ((direction == 1) && (pxDst->drawable.bitsPerPixel < 16))) {
> -	    ErrorF("Can't do mask blending with less then 16bpp\n");
> -	    return FALSE;
> -	}
> -


Are you sure this can be moved so easily? Is there any guarantee that if
the src/dst Picture drawable is 16bpp, that then the src/dst pixmap
drawable will be too? I'm not sure we know that absolutely surely, do
we?

So for now just pointing out that the move isn't trivial here, as the
checks are done on different data structures with this patch. Can you
explain how this is fine, or if not, perhaps it's better to just leave
this part in lx_prepare_composite for now and get the rest moved in a
new smaller patch?

I'm CCing xorg-devel for some EXA expert say on this. Full patch visible
at http://lists.x.org/archives/xorg-driver-geode/2010-June/000678.html


Cheers,
Mart Raudsepp