Re: [PATCH xserver v2] glamor: Handle bitplane in glamor_copy_fbo_cpu

[Michel Dänzer]

On 15/08/16 11:18 PM, walter harms wrote:
> Am 15.08.2016 11:43, schrieb Michel Dänzer:
>>
>> diff --git a/glamor/glamor_copy.c b/glamor/glamor_copy.c
>> index 3501a0d..82e040a 100644
>> --- a/glamor/glamor_copy.c
>> +++ b/glamor/glamor_copy.c
>> @@ -222,9 +222,40 @@ glamor_copy_cpu_fbo(DrawablePtr src,
>>  
>>  fbGetDrawable(src, src_bits, src_stride, src_bpp, src_xoff, src_yoff);
>>  
>> -glamor_upload_boxes(dst_pixmap, box, nbox, src_xoff + dx, src_yoff + dy,
>> -dst_xoff, dst_yoff,
>> -(uint8_t *) src_bits, src_stride * sizeof (FbBits));
> 
> 
> my i suggest to invert the question to improve readability ?
> 
>   if (!bitplane) {
>   glamor_upload_boxes(dst_pixmap, box, nbox, src_xoff + dx, 
> src_yoff + dy,
> dst_xoff, dst_yoff,
> (uint8_t *) src_bits, src_stride * sizeof 
> (FbBits));
> glamor_finish_access(src);
>   return TRUE;
> }
> 
> btw: i hope return TRUE is intended here

Yes, it is.


> and then the rest ...

Thanks for your suggestion. I don't agree that it would improve
readability, but it did make me realize other potential to slightly
simplify the patch, see v3.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver v2] glamor: Handle bitplane in glamor_copy_fbo_cpu

[walter harms]

Am 15.08.2016 11:43, schrieb Michel Dänzer:
> From: Michel Dänzer 
> 
> This can significantly speed up at least some CopyPlane cases, e.g.
> indirectly for stippled fills.
> 
> v2:
> * Make temporary pixmap the same size as the destination pixmap
>   (instead of the destination drawable size), and fix coordinate
>   parameters passed to fbCopyXtoX and glamor_upload_boxes. Fixes
>   incorrect rendering rendering with x11perf -copyplane* and crashes
>   with the xscreensaver phosphor hack.
> 
> Reported-by: Keith Raghubar 
> Signed-off-by: Michel Dänzer 
> ---
>  glamor/glamor_copy.c | 43 +--
>  1 file changed, 37 insertions(+), 6 deletions(-)
> 
> diff --git a/glamor/glamor_copy.c b/glamor/glamor_copy.c
> index 3501a0d..82e040a 100644
> --- a/glamor/glamor_copy.c
> +++ b/glamor/glamor_copy.c
> @@ -222,9 +222,40 @@ glamor_copy_cpu_fbo(DrawablePtr src,
>  
>  fbGetDrawable(src, src_bits, src_stride, src_bpp, src_xoff, src_yoff);
>  
> -glamor_upload_boxes(dst_pixmap, box, nbox, src_xoff + dx, src_yoff + dy,
> -dst_xoff, dst_yoff,
> -(uint8_t *) src_bits, src_stride * sizeof (FbBits));


my i suggest to invert the question to improve readability ?

if (!bitplane) {
glamor_upload_boxes(dst_pixmap, box, nbox, src_xoff + dx, 
src_yoff + dy,
dst_xoff, dst_yoff,
(uint8_t *) src_bits, src_stride * sizeof (FbBits));
glamor_finish_access(src);
return TRUE;
  }

btw: i hope return TRUE is intended here

and then the rest ...

just my 2 cents

re,
 wh

> +if (bitplane) {
> +FbBits *tmp_bits;
> +FbStride tmp_stride;
> +int tmp_bpp;
> +_X_UNUSED int tmp_xoff, tmp_yoff;
> +PixmapPtr tmp_pix = fbCreatePixmap(screen, 
> dst_pixmap->drawable.width,
> +   dst_pixmap->drawable.height,
> +   dst->depth, 0);
> +
> +if (!tmp_pix) {
> +glamor_finish_access(src);
> +goto bail;
> +}
> +
> +fbGetDrawable(_pix->drawable, tmp_bits, tmp_stride, tmp_bpp, 
> tmp_xoff,
> +  tmp_yoff);
> +
> +if (src->bitsPerPixel > 1)
> +fbCopyNto1(src, _pix->drawable, gc, box, nbox,
> +   dst_xoff + dx, dst_yoff + dy, reverse, upsidedown,
> +   bitplane, closure);
> +else
> +fbCopy1toN(src, _pix->drawable, gc, box, nbox,
> +   dst_xoff + dx, dst_yoff + dy, reverse, upsidedown,
> +   bitplane, closure);
> +
> +glamor_upload_boxes(dst_pixmap, box, nbox, 0, 0, 0, 0,
> +(uint8_t *) tmp_bits, tmp_stride * 
> sizeof(FbBits));
> +
> +fbDestroyPixmap(tmp_pix);
> +} else
> +glamor_upload_boxes(dst_pixmap, box, nbox, src_xoff + dx, src_yoff + 
> dy,
> +dst_xoff, dst_yoff,
> +(uint8_t *) src_bits, src_stride * sizeof 
> (FbBits));
>  glamor_finish_access(src);
>  
>  return TRUE;
> @@ -616,9 +647,9 @@ glamor_copy_gl(DrawablePtr src,
>  return glamor_copy_fbo_fbo_draw(src, dst, gc, box, nbox, dx, 
> dy,
>  reverse, upsidedown, 
> bitplane, closure);
>  }
> -if (bitplane == 0)
> -return glamor_copy_cpu_fbo(src, dst, gc, box, nbox, dx, dy,
> -   reverse, upsidedown, bitplane, 
> closure);
> +
> +return glamor_copy_cpu_fbo(src, dst, gc, box, nbox, dx, dy,
> +   reverse, upsidedown, bitplane, closure);
>  } else if (GLAMOR_PIXMAP_PRIV_HAS_FBO(src_priv) &&
> dst_priv->type != GLAMOR_DRM_ONLY &&
> bitplane == 0) {
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel