[PATCH 1/6] DownloadFromScreenCS: download via a scratch BO if pixmap domain is unknown

[Michel Dänzer]

On Mon, 2010-08-23 at 18:27 +1200, Karl Tomlinson wrote: 
> 
> radeon_bo_is_busy() may return without setting the domain out-parameter.
> If this happens, then download via a scratch GTT BO to avoid CPU VRAM read.

I'm on the fence about this one. I can see how it could help *if*
radeon_bo_is_busy() doesn't set the domain[0] and the BO happens to be
in VRAM, but OTOH it might also hurt if the BO is not in VRAM? Can you
elaborate a bit on how often you're encountering this condition and what
if any measurements you've made about the impact on performance of this
patch and possibly how often the BO is actually in VRAM or not in those
cases.

[0] AFAICT that can only happen if messages like

        radeon 0000:00:10.0: ee7e7400 reserve failed for wait

appear in dmesg, which only happens very rarely here and might be
fixable in the kernel anyway.


> ---
>  src/r600_exa.c         |    2 +-
>  src/radeon_exa_funcs.c |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/src/r600_exa.c b/src/r600_exa.c
> index d6e98ff..9b7a0c9 100644
> --- a/src/r600_exa.c
> +++ b/src/r600_exa.c
> @@ -1885,7 +1885,7 @@ R600DownloadFromScreenCS(PixmapPtr pSrc, int x, int y, int w,
>      if (!src_domain)
>  	radeon_bo_is_busy(driver_priv->bo, &src_domain);
>  
> -    if (src_domain != RADEON_GEM_DOMAIN_VRAM)
> +    if (src_domain & ~(uint32_t)RADEON_GEM_DOMAIN_VRAM)
>  	return FALSE;
>  
>      size = scratch_pitch * h;
> diff --git a/src/radeon_exa_funcs.c b/src/radeon_exa_funcs.c
> index 2df6ccb..a82e416 100644
> --- a/src/radeon_exa_funcs.c
> +++ b/src/radeon_exa_funcs.c
> @@ -598,7 +598,7 @@ RADEONDownloadFromScreenCS(PixmapPtr pSrc, int x, int y, int w,
>      if (!src_domain)
>  	radeon_bo_is_busy(driver_priv->bo, &src_domain);
>  
> -    if (src_domain != RADEON_GEM_DOMAIN_VRAM) {
> +    if (src_domain & ~(uint32_t)RADEON_GEM_DOMAIN_VRAM) {
>  #if X_BYTE_ORDER == X_BIG_ENDIAN
>  	/* Can't return FALSE here if we need to swap bytes */
>  	if (swap != RADEON_HOST_DATA_SWAP_NONE) {


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