Re: [PATCH 1/2] glamor: Add GLAMOR_ACCESS_WO

2016-09-25 Thread Eric Anholt 
Michel Dänzer  writes:

> Adding Marek and Nicolai, maybe they have some feedback from a GL
> (driver) perspective.
>
>
> On 23/08/16 10:41 AM, Dave Airlie wrote:
>> From: Michel Dänzer 
>> 
>> [airlied: rebased onto master -
>> I left WO alone as it's more like the GL interface
>> review suggested changing it to bits.]
>
> After realizing that patch 2 can only affect the !ZPixmap case, I tested
> this series with
>
>  x11perf -putimagexy{10,100,500} -shmputxy{10,100,500}
>
> and to my surprise, all of the numbers went down by around an order of
> magnitude (using radeonsi on Kaveri). I investigated a little bit what's
> going on:

I should have replied before.  Based on the performance numbers, I've
considered these patches rejected.


signature.asc
Description: PGP signature
___
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 1/2] glamor: Add GLAMOR_ACCESS_WO

2016-08-24 Thread Marek Olšák 
On Wed, Aug 24, 2016 at 5:27 AM, Michel Dänzer  wrote:
>
> Adding Marek and Nicolai, maybe they have some feedback from a GL
> (driver) perspective.
>
>
> On 23/08/16 10:41 AM, Dave Airlie wrote:
>> From: Michel Dänzer 
>>
>> [airlied: rebased onto master -
>> I left WO alone as it's more like the GL interface
>> review suggested changing it to bits.]
>
> After realizing that patch 2 can only affect the !ZPixmap case, I tested
> this series with
>
>  x11perf -putimagexy{10,100,500} -shmputxy{10,100,500}
>
> and to my surprise, all of the numbers went down by around an order of
> magnitude (using radeonsi on Kaveri). I investigated a little bit what's
> going on:
>
>
>> @@ -86,7 +86,10 @@ glamor_prep_pixmap_box(PixmapPtr pixmap, glamor_access_t 
>> access, BoxPtr box)
>>  if (priv->pbo == 0)
>>  glGenBuffers(1, >pbo);
>>
>> -gl_usage = GL_STREAM_READ;
>> +if (access == GLAMOR_ACCESS_WO)
>> +gl_usage = GL_STREAM_DRAW;
>> +else
>> +gl_usage = GL_STREAM_READ;
>>
>>  glBindBuffer(GL_PIXEL_PACK_BUFFER, priv->pbo);
>>  glBufferData(GL_PIXEL_PACK_BUFFER,
>
> This change results in write-combining for the PBO CPU mapping.
> Apparently, fbPutXYImage ends up either reading from the PBO, or at
> least writing to it in a WC-unfriendly manner, causing a big slowdown.

GL_STREAM_READ means cached GART. GL_STREAM_DRAW means uncached WC GART.

Marek
___
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 1/2] glamor: Add GLAMOR_ACCESS_WO

2016-08-24 Thread Adam Jackson 
On Wed, 2016-08-24 at 12:27 +0900, Michel Dänzer wrote:

> This change results in write-combining for the PBO CPU mapping.
> Apparently, fbPutXYImage ends up either reading from the PBO, or at
> least writing to it in a WC-unfriendly manner, causing a big slowdown.

It does read from the destination, and the implementation is pretty
much the least cache-friendly thing one could imagine. fbPutXYImage
works by walking plane-by-plane over the source image and combining
each plane with the destination according to the rop mode, so even if
the equivalent ZPixmap operation wouldn't read the destination (say, pm
of ~0 and rop of GXcopy), the XYPixmap version will.

- ajax
___
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 1/2] glamor: Add GLAMOR_ACCESS_WO

2016-08-23 Thread Michel Dänzer 

Adding Marek and Nicolai, maybe they have some feedback from a GL
(driver) perspective.


On 23/08/16 10:41 AM, Dave Airlie wrote:
> From: Michel Dänzer 
> 
> [airlied: rebased onto master -
> I left WO alone as it's more like the GL interface
> review suggested changing it to bits.]

After realizing that patch 2 can only affect the !ZPixmap case, I tested
this series with

 x11perf -putimagexy{10,100,500} -shmputxy{10,100,500}

and to my surprise, all of the numbers went down by around an order of
magnitude (using radeonsi on Kaveri). I investigated a little bit what's
going on:


> @@ -86,7 +86,10 @@ glamor_prep_pixmap_box(PixmapPtr pixmap, glamor_access_t 
> access, BoxPtr box)
>  if (priv->pbo == 0)
>  glGenBuffers(1, >pbo);
>  
> -gl_usage = GL_STREAM_READ;
> +if (access == GLAMOR_ACCESS_WO)
> +gl_usage = GL_STREAM_DRAW;
> +else
> +gl_usage = GL_STREAM_READ;
>  
>  glBindBuffer(GL_PIXEL_PACK_BUFFER, priv->pbo);
>  glBufferData(GL_PIXEL_PACK_BUFFER,

This change results in write-combining for the PBO CPU mapping.
Apparently, fbPutXYImage ends up either reading from the PBO, or at
least writing to it in a WC-unfriendly manner, causing a big slowdown.

Reverting this hunk makes performance match or exceed the level before
this series, except for -putimagexy10 remaining at ~50%. Another issue
there is the PBO allocation overhead[0]. Changing glamor_prep_pixmap_box
to use the non-PBO path for GLAMOR_ACCESS_WO[1] makes all numbers match
or exceed (by several times for the 10x10 tests) those before this series.

[0] The PBO must be large enough to hold the destination pixmap, which
is the screen pixmap in the non-composited case, so it's several MBs.
This probably blows up the userspace BO cache, and TTM clears all pages
of the BOs allocated from the kernel, even though most of them end up
completely unused.

[1] A better heuristic for not using a PBO might be based on the
relative size (and maybe also the absolute sizes) of the box and the pixmap.


-- 
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

[PATCH 1/2] glamor: Add GLAMOR_ACCESS_WO

2016-08-22 Thread Dave Airlie 
From: Michel Dänzer 

[airlied: rebased onto master -
I left WO alone as it's more like the GL interface
review suggested changing it to bits.]

Reviewed-by: Dave Airlie 
Signed-off-by: Michel Dänzer 
Signed-off-by: Dave Airlie 
---
 glamor/glamor_prepare.c | 16 +++-
 glamor/glamor_priv.h|  1 +
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/glamor/glamor_prepare.c b/glamor/glamor_prepare.c
index 5a73e6c..da31677 100644
--- a/glamor/glamor_prepare.c
+++ b/glamor/glamor_prepare.c
@@ -71,7 +71,7 @@ glamor_prep_pixmap_box(PixmapPtr pixmap, glamor_access_t 
access, BoxPtr box)
 if (!RegionNotEmpty())
 return TRUE;
 
-if (access == GLAMOR_ACCESS_RW)
+if (access != GLAMOR_ACCESS_RO)
 FatalError("attempt to remap buffer as writable");
 
 if (priv->pbo) {
@@ -86,7 +86,10 @@ glamor_prep_pixmap_box(PixmapPtr pixmap, glamor_access_t 
access, BoxPtr box)
 if (priv->pbo == 0)
 glGenBuffers(1, >pbo);
 
-gl_usage = GL_STREAM_READ;
+if (access == GLAMOR_ACCESS_WO)
+gl_usage = GL_STREAM_DRAW;
+else
+gl_usage = GL_STREAM_READ;
 
 glBindBuffer(GL_PIXEL_PACK_BUFFER, priv->pbo);
 glBufferData(GL_PIXEL_PACK_BUFFER,
@@ -101,14 +104,17 @@ glamor_prep_pixmap_box(PixmapPtr pixmap, glamor_access_t 
access, BoxPtr box)
 priv->map_access = access;
 }
 
-glamor_download_boxes(pixmap, RegionRects(), 
RegionNumRects(),
-  0, 0, 0, 0, pixmap->devPrivate.ptr, pixmap->devKind);
+if (access != GLAMOR_ACCESS_WO)
+glamor_download_boxes(pixmap, RegionRects(), 
RegionNumRects(),
+  0, 0, 0, 0, pixmap->devPrivate.ptr, 
pixmap->devKind);
 
 RegionUninit();
 
 if (glamor_priv->has_rw_pbo) {
 if (priv->map_access == GLAMOR_ACCESS_RW)
 gl_access = GL_READ_WRITE;
+else if (priv->map_access == GLAMOR_ACCESS_WO)
+gl_access = GL_WRITE_ONLY;
 else
 gl_access = GL_READ_ONLY;
 
@@ -144,7 +150,7 @@ glamor_fini_pixmap(PixmapPtr pixmap)
 pixmap->devPrivate.ptr = NULL;
 }
 
-if (priv->map_access == GLAMOR_ACCESS_RW) {
+if (priv->map_access != GLAMOR_ACCESS_RO) {
 glamor_upload_boxes(pixmap,
 RegionRects(>prepare_region),
 RegionNumRects(>prepare_region),
diff --git a/glamor/glamor_priv.h b/glamor/glamor_priv.h
index 2e491a6..2b7009e 100644
--- a/glamor/glamor_priv.h
+++ b/glamor/glamor_priv.h
@@ -300,6 +300,7 @@ typedef struct glamor_screen_private {
 typedef enum glamor_access {
 GLAMOR_ACCESS_RO,
 GLAMOR_ACCESS_RW,
+GLAMOR_ACCESS_WO,
 } glamor_access_t;
 
 enum glamor_fbo_state {
-- 
2.5.5

___
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

[PATCH 1/2] glamor: Add GLAMOR_ACCESS_WO

2016-03-10 Thread Dave Airlie 
From: Michel Dänzer 

[airlied: rebased onto master -
I left WO alone as it's more like the GL interface
review suggested changing it to bits.]

Reviewed-by: Dave Airlie 
Signed-off-by: Michel Dänzer 
Signed-off-by: Dave Airlie 
---
 glamor/glamor_prepare.c | 16 +++-
 glamor/glamor_priv.h|  1 +
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/glamor/glamor_prepare.c b/glamor/glamor_prepare.c
index 5a73e6c..da31677 100644
--- a/glamor/glamor_prepare.c
+++ b/glamor/glamor_prepare.c
@@ -71,7 +71,7 @@ glamor_prep_pixmap_box(PixmapPtr pixmap, glamor_access_t 
access, BoxPtr box)
 if (!RegionNotEmpty())
 return TRUE;
 
-if (access == GLAMOR_ACCESS_RW)
+if (access != GLAMOR_ACCESS_RO)
 FatalError("attempt to remap buffer as writable");
 
 if (priv->pbo) {
@@ -86,7 +86,10 @@ glamor_prep_pixmap_box(PixmapPtr pixmap, glamor_access_t 
access, BoxPtr box)
 if (priv->pbo == 0)
 glGenBuffers(1, >pbo);
 
-gl_usage = GL_STREAM_READ;
+if (access == GLAMOR_ACCESS_WO)
+gl_usage = GL_STREAM_DRAW;
+else
+gl_usage = GL_STREAM_READ;
 
 glBindBuffer(GL_PIXEL_PACK_BUFFER, priv->pbo);
 glBufferData(GL_PIXEL_PACK_BUFFER,
@@ -101,14 +104,17 @@ glamor_prep_pixmap_box(PixmapPtr pixmap, glamor_access_t 
access, BoxPtr box)
 priv->map_access = access;
 }
 
-glamor_download_boxes(pixmap, RegionRects(), 
RegionNumRects(),
-  0, 0, 0, 0, pixmap->devPrivate.ptr, pixmap->devKind);
+if (access != GLAMOR_ACCESS_WO)
+glamor_download_boxes(pixmap, RegionRects(), 
RegionNumRects(),
+  0, 0, 0, 0, pixmap->devPrivate.ptr, 
pixmap->devKind);
 
 RegionUninit();
 
 if (glamor_priv->has_rw_pbo) {
 if (priv->map_access == GLAMOR_ACCESS_RW)
 gl_access = GL_READ_WRITE;
+else if (priv->map_access == GLAMOR_ACCESS_WO)
+gl_access = GL_WRITE_ONLY;
 else
 gl_access = GL_READ_ONLY;
 
@@ -144,7 +150,7 @@ glamor_fini_pixmap(PixmapPtr pixmap)
 pixmap->devPrivate.ptr = NULL;
 }
 
-if (priv->map_access == GLAMOR_ACCESS_RW) {
+if (priv->map_access != GLAMOR_ACCESS_RO) {
 glamor_upload_boxes(pixmap,
 RegionRects(>prepare_region),
 RegionNumRects(>prepare_region),
diff --git a/glamor/glamor_priv.h b/glamor/glamor_priv.h
index 8f994ea..355408c 100644
--- a/glamor/glamor_priv.h
+++ b/glamor/glamor_priv.h
@@ -313,6 +313,7 @@ typedef struct glamor_screen_private {
 typedef enum glamor_access {
 GLAMOR_ACCESS_RO,
 GLAMOR_ACCESS_RW,
+GLAMOR_ACCESS_WO,
 } glamor_access_t;
 
 enum glamor_fbo_state {
-- 
2.5.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

[PATCH 1/2] glamor: Add GLAMOR_ACCESS_WO

2014-08-05 Thread Michel Dänzer 
From: Michel Dänzer michel.daen...@amd.com

Signed-off-by: Michel Dänzer michel.daen...@amd.com
---
 glamor/glamor_prepare.c | 13 +
 glamor/glamor_priv.h|  1 +
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/glamor/glamor_prepare.c b/glamor/glamor_prepare.c
index 561c55d..3e370b7 100644
--- a/glamor/glamor_prepare.c
+++ b/glamor/glamor_prepare.c
@@ -69,7 +69,7 @@ glamor_prep_pixmap_box(PixmapPtr pixmap, glamor_access_t 
access, BoxPtr box)
 if (!RegionNotEmpty(region))
 return TRUE;
 
-if (access == GLAMOR_ACCESS_RW)
+if (access != GLAMOR_ACCESS_RO)
 FatalError(attempt to remap buffer as writable);
 
 if (priv-base.pbo) {
@@ -86,6 +86,8 @@ glamor_prep_pixmap_box(PixmapPtr pixmap, glamor_access_t 
access, BoxPtr box)
 
 if (access == GLAMOR_ACCESS_RW)
 gl_usage = GL_DYNAMIC_DRAW;
+else if (access == GLAMOR_ACCESS_WO)
+gl_usage = GL_STREAM_DRAW;
 else
 gl_usage = GL_STREAM_READ;
 
@@ -102,14 +104,17 @@ glamor_prep_pixmap_box(PixmapPtr pixmap, glamor_access_t 
access, BoxPtr box)
 priv-base.map_access = access;
 }
 
-glamor_download_boxes(pixmap, RegionRects(region), 
RegionNumRects(region),
-  0, 0, 0, 0, pixmap-devPrivate.ptr, pixmap-devKind);
+if (access != GLAMOR_ACCESS_WO)
+glamor_download_boxes(pixmap, RegionRects(region), 
RegionNumRects(region),
+  0, 0, 0, 0, pixmap-devPrivate.ptr, 
pixmap-devKind);
 
 RegionUninit(region);
 
 if (glamor_priv-has_rw_pbo) {
 if (priv-base.map_access == GLAMOR_ACCESS_RW)
 gl_access = GL_READ_WRITE;
+else if (priv-base.map_access == GLAMOR_ACCESS_WO)
+gl_access = GL_WRITE_ONLY;
 else
 gl_access = GL_READ_ONLY;
 
@@ -145,7 +150,7 @@ glamor_fini_pixmap(PixmapPtr pixmap)
 pixmap-devPrivate.ptr = NULL;
 }
 
-if (priv-base.map_access == GLAMOR_ACCESS_RW) {
+if (priv-base.map_access != GLAMOR_ACCESS_RO) {
 glamor_upload_boxes(pixmap,
 RegionRects(priv-base.prepare_region),
 RegionNumRects(priv-base.prepare_region),
diff --git a/glamor/glamor_priv.h b/glamor/glamor_priv.h
index 385c027..91af6fc 100644
--- a/glamor/glamor_priv.h
+++ b/glamor/glamor_priv.h
@@ -302,6 +302,7 @@ typedef struct glamor_screen_private {
 typedef enum glamor_access {
 GLAMOR_ACCESS_RO,
 GLAMOR_ACCESS_RW,
+GLAMOR_ACCESS_WO,
 } glamor_access_t;
 
 enum glamor_fbo_state {
-- 
2.0.1

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

Re: [PATCH 1/2] glamor: Add GLAMOR_ACCESS_WO

2014-08-05 Thread walter harms 


Am 05.08.2014 10:08, schrieb Michel Dänzer:
 From: Michel Dänzer michel.daen...@amd.com
 
 Signed-off-by: Michel Dänzer michel.daen...@amd.com
 ---
  glamor/glamor_prepare.c | 13 +
  glamor/glamor_priv.h|  1 +
  2 files changed, 10 insertions(+), 4 deletions(-)
 
 diff --git a/glamor/glamor_prepare.c b/glamor/glamor_prepare.c
 index 561c55d..3e370b7 100644
 --- a/glamor/glamor_prepare.c
 +++ b/glamor/glamor_prepare.c
 @@ -69,7 +69,7 @@ glamor_prep_pixmap_box(PixmapPtr pixmap, glamor_access_t 
 access, BoxPtr box)
  if (!RegionNotEmpty(region))
  return TRUE;
  
 -if (access == GLAMOR_ACCESS_RW)
 +if (access != GLAMOR_ACCESS_RO)
  FatalError(attempt to remap buffer as writable);
  
  if (priv-base.pbo) {
 @@ -86,6 +86,8 @@ glamor_prep_pixmap_box(PixmapPtr pixmap, glamor_access_t 
 access, BoxPtr box)
  
  if (access == GLAMOR_ACCESS_RW)
  gl_usage = GL_DYNAMIC_DRAW;
 +else if (access == GLAMOR_ACCESS_WO)
 +gl_usage = GL_STREAM_DRAW;
  else
  gl_usage = GL_STREAM_READ;
  
 @@ -102,14 +104,17 @@ glamor_prep_pixmap_box(PixmapPtr pixmap, 
 glamor_access_t access, BoxPtr box)
  priv-base.map_access = access;
  }
  
 -glamor_download_boxes(pixmap, RegionRects(region), 
 RegionNumRects(region),
 -  0, 0, 0, 0, pixmap-devPrivate.ptr, 
 pixmap-devKind);
 +if (access != GLAMOR_ACCESS_WO)
 +glamor_download_boxes(pixmap, RegionRects(region), 
 RegionNumRects(region),
 +  0, 0, 0, 0, pixmap-devPrivate.ptr, 
 pixmap-devKind);
  
  RegionUninit(region);
  
  if (glamor_priv-has_rw_pbo) {
  if (priv-base.map_access == GLAMOR_ACCESS_RW)
  gl_access = GL_READ_WRITE;
 +else if (priv-base.map_access == GLAMOR_ACCESS_WO)
 +gl_access = GL_WRITE_ONLY;
  else
  gl_access = GL_READ_ONLY;
  
 @@ -145,7 +150,7 @@ glamor_fini_pixmap(PixmapPtr pixmap)
  pixmap-devPrivate.ptr = NULL;
  }
  
 -if (priv-base.map_access == GLAMOR_ACCESS_RW) {
 +if (priv-base.map_access != GLAMOR_ACCESS_RO) {
  glamor_upload_boxes(pixmap,
  RegionRects(priv-base.prepare_region),
  RegionNumRects(priv-base.prepare_region),
 diff --git a/glamor/glamor_priv.h b/glamor/glamor_priv.h
 index 385c027..91af6fc 100644
 --- a/glamor/glamor_priv.h
 +++ b/glamor/glamor_priv.h
 @@ -302,6 +302,7 @@ typedef struct glamor_screen_private {
  typedef enum glamor_access {
  GLAMOR_ACCESS_RO,
  GLAMOR_ACCESS_RW,
 +GLAMOR_ACCESS_WO,
  } glamor_access_t;
  
  enum glamor_fbo_state {


I would go bitwise:
GLAMOR_ACCESS_RO=1
GLAMOR_ACCESS_WO=2
GLAMOR_ACCESS_RW=GLAMOR_ACCESS_RO|GLAMOR_ACCESS_WO

its a more natural feeling ..

just my 2 cents,

re,
 wh

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