Hey,
Looks good, ACK.
I would have split it one patch introducing dfps_update_{box,region}
with no functional change, and one patch adding the
DFPS_MAX_UPDATE_REGION limit.
Christophe
On Wed, Sep 10, 2014 at 12:26:26PM -0500, Jeremy White wrote:
> Attempting to use x11perf to measure performance revealed a fairly
> serious weakness in the dfps code in that use case. In between
> fps ticks, the updated_region would grow to have thousands of
> rectangles, which made processing extraordinarily slow.
>
> This patch provides a cap on the number of rectangles inside
> a region; once it reaches the cap, we collapse the update_region
> to just transmit the bounding rectangle instead.
>
> Signed-off-by: Jeremy White <jwhite@xxxxxxxxxxxxxxx>
> ---
> src/dfps.c | 75 +++++++++++++++++++++++++++++++++++-------------------------
> 1 file changed, 44 insertions(+), 31 deletions(-)
>
> diff --git a/src/dfps.c b/src/dfps.c
> index 4ab20a8..5db34dc 100644
> --- a/src/dfps.c
> +++ b/src/dfps.c
> @@ -128,6 +128,45 @@ static Bool unaccel (void)
> return FALSE;
> }
>
> +
> +/* Establish a maximum number of disparate regions we'll track before we just
> + treat the entire bounding rectangle as having changed.
> +
> + The number 20 seemed intuitive, and also produced the best results in
> + benchmarking x11perf -circle10 -repeat 1
> +*/
> +#define DFPS_MAX_UPDATE_REGIONS 20
> +static void dfps_update_box(RegionPtr dest, int x_1, int x_2, int y_1, int y_2);
> +
> +static void dfps_update_region(RegionPtr dest, RegionPtr src)
> +{
> + Bool throwaway_bool;
> +
> + RegionAppend(dest, src);
> + RegionValidate(dest, &throwaway_bool);
> + if (RegionNumRects(dest) > DFPS_MAX_UPDATE_REGIONS)
> + {
> + struct pixman_box16 box = * (RegionExtents(dest));
> + RegionUninit(dest);
> + RegionInit(dest, NULL, 0);
> + dfps_update_box(dest, box.x1, box.x2, box.y1, box.y2);
> + }
> +}
> +
> +static void dfps_update_box(RegionPtr dest, int x_1, int x_2, int y_1, int y_2)
> +{
> + struct pixman_box16 box;
> + RegionPtr region;
> +
> + box.x1 = x_1; box.x2 = x_2; box.y1 = y_1; box.y2 = y_2;
> + region = RegionCreate(&box, 1);
> +
> + dfps_update_region(dest, region);
> +
> + RegionUninit(region);
> + RegionDestroy(region);
> +}
> +
> static Bool dfps_prepare_solid (PixmapPtr pixmap, int alu, Pixel planemask, Pixel fg)
> {
> dfps_info_t *info;
> @@ -152,9 +191,6 @@ static Bool dfps_prepare_solid (PixmapPtr pixmap, int alu, Pixel planemask, Pixe
>
> static void dfps_solid (PixmapPtr pixmap, int x_1, int y_1, int x_2, int y_2)
> {
> - struct pixman_box16 box;
> - RegionPtr region;
> - Bool throwaway_bool;
> dfps_info_t *info;
>
> if (!(info = dfps_get_info (pixmap)))
> @@ -164,12 +200,7 @@ static void dfps_solid (PixmapPtr pixmap, int x_1, int y_1, int x_2, int y_2)
> fbFill(&pixmap->drawable, info->pgc, x_1, y_1, x_2 - x_1, y_2 - y_1);
>
> /* Track the updated region */
> - box.x1 = x_1; box.x2 = x_2; box.y1 = y_1; box.y2 = y_2;
> - region = RegionCreate(&box, 1);
> - RegionAppend(&info->updated_region, region);
> - RegionValidate(&info->updated_region, &throwaway_bool);
> - RegionUninit(region);
> - RegionDestroy(region);
> + dfps_update_box(&info->updated_region, x_1, x_2, y_1, y_2);
> return;
> }
>
> @@ -212,10 +243,6 @@ static void dfps_copy (PixmapPtr dest,
> int dest_x1, int dest_y1,
> int width, int height)
> {
> - struct pixman_box16 box;
> - RegionPtr region;
> - Bool throwaway_bool;
> -
> dfps_info_t *info;
>
> if (!(info = dfps_get_info (dest)))
> @@ -225,12 +252,7 @@ static void dfps_copy (PixmapPtr dest,
> fbCopyArea(&info->copy_src->drawable, &dest->drawable, info->pgc, src_x1, src_y1, width, height, dest_x1, dest_y1);
>
> /* Update the tracking region */
> - box.x1 = dest_x1; box.x2 = dest_x1 + width; box.y1 = dest_y1; box.y2 = dest_y1 + height;
> - region = RegionCreate(&box, 1);
> - RegionAppend(&info->updated_region, region);
> - RegionValidate(&info->updated_region, &throwaway_bool);
> - RegionUninit(region);
> - RegionDestroy(region);
> + dfps_update_box(&info->updated_region, dest_x1, dest_x1 + width, dest_y1, dest_y1 + height);
> }
>
> static void dfps_done_copy (PixmapPtr dest)
> @@ -247,20 +269,12 @@ static void dfps_done_copy (PixmapPtr dest)
> static Bool dfps_put_image (PixmapPtr dest, int x, int y, int w, int h,
> char *src, int src_pitch)
> {
> - struct pixman_box16 box;
> - RegionPtr region;
> - Bool throwaway_bool;
> dfps_info_t *info;
>
> if (!(info = dfps_get_info (dest)))
> return FALSE;
>
> - box.x1 = x; box.x2 = x + w; box.y1 = y; box.y2 = y + h;
> - region = RegionCreate(&box, 1);
> - RegionAppend(&info->updated_region, region);
> - RegionValidate(&info->updated_region, &throwaway_bool);
> - RegionUninit(region);
> - RegionDestroy(region);
> + dfps_update_box(&info->updated_region, x, x + w, y, y + h);
>
> /* We can avoid doing the put image ourselves, as the uxa driver
> will fall back and do it for us if we return false */
> @@ -274,12 +288,11 @@ static Bool dfps_prepare_access (PixmapPtr pixmap, RegionPtr region, uxa_access_
> if (requested_access == UXA_ACCESS_RW)
> {
> dfps_info_t *info;
> - Bool throwaway_bool;
>
> if (!(info = dfps_get_info (pixmap)))
> return FALSE;
> - RegionAppend(&info->updated_region, region);
> - RegionValidate(&info->updated_region, &throwaway_bool);
> +
> + dfps_update_region(&info->updated_region, region);
> }
> return TRUE;
> }
> --
> 1.7.10.4
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/spice-devel
Attachment:
pgpO_Oqyf7Mfo.pgp
Description: PGP signature
_______________________________________________ Spice-devel mailing list Spice-devel@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/spice-devel