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

[Huang, FrankR]

-----Original Message-----
From: Mart Raudsepp [mailto:leio at gentoo.org] 
Sent: 2010?6?14? 6:50
To: Huang, FrankR
Cc: xorg-driver-geode at lists.x.org; xorg-devel at lists.x.org
Subject: Re: [Xorg-driver-geode] [PATCH 1/7] Prevent the pixmap migrationif the geode GP can not do the acceleration.

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.
[Frank] whitespace here, yes. delete it and resend?

> @@ -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?
[Frank]Good point. In my opinion, they are the same value.
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
[Frank]I agree with you by putting this question cc to xorg-devel to let EXA experts make sure of this. I do think they have the same value. The reason why I put this judge advanced is that the pixmap migration will be happened again if keep putting the "return FALSE" in lx_prepare_composite. So I try to put all the judge advanced. We can see how the EXA experts give the answer.

Cheers,
Mart Raudsepp