[Mesa-dev] [PATCH] llvmpipe: Remove x/y from cmd_bin
Jose Fonseca
jfonseca at vmware.com
Fri May 17 07:19:23 PDT 2013
----- Original Message -----
> Am 16.05.2013 21:44, schrieb Adam Jackson:
> > These were mostly just a waste of memory and cache pressure, and were
> > really only used for debugging.
> >
> > This change reduces instruction count (as measured by callgrind's Ir
> > event) of gnome-shell-perf-tool on Ivybridge by 3.5% ± 0.015% (n=20).
> >
> > Signed-off-by: Adam Jackson <ajax at redhat.com>
> > ---
> > src/gallium/drivers/llvmpipe/lp_rast.c | 37
> > +++++++++++-----------------
> > src/gallium/drivers/llvmpipe/lp_rast_debug.c | 19 +++++++-------
> > src/gallium/drivers/llvmpipe/lp_rast_priv.h | 2 +-
> > src/gallium/drivers/llvmpipe/lp_scene.c | 4 ++-
> > src/gallium/drivers/llvmpipe/lp_scene.h | 4 +--
> > src/gallium/drivers/llvmpipe/lp_setup.c | 11 +--------
> > 6 files changed, 30 insertions(+), 47 deletions(-)
> >
> > diff --git a/src/gallium/drivers/llvmpipe/lp_rast.c
> > b/src/gallium/drivers/llvmpipe/lp_rast.c
> > index a557db4..3dc00ef 100644
> > --- a/src/gallium/drivers/llvmpipe/lp_rast.c
> > +++ b/src/gallium/drivers/llvmpipe/lp_rast.c
> > @@ -87,13 +87,14 @@ lp_rast_end( struct lp_rasterizer *rast )
> > */
> > static void
> > lp_rast_tile_begin(struct lp_rasterizer_task *task,
> > - const struct cmd_bin *bin)
> > + const struct cmd_bin *bin,
> > + int x, int y)
> > {
> > - LP_DBG(DEBUG_RAST, "%s %d,%d\n", __FUNCTION__, bin->x, bin->y);
> > + LP_DBG(DEBUG_RAST, "%s %d,%d\n", __FUNCTION__, x, y);
> >
> > task->bin = bin;
> > - task->x = bin->x * TILE_SIZE;
> > - task->y = bin->y * TILE_SIZE;
> > + task->x = x * TILE_SIZE;
> > + task->y = y * TILE_SIZE;
> >
> > /* reset pointers to color and depth tile(s) */
> > memset(task->color_tiles, 0, sizeof(task->color_tiles));
> > @@ -551,13 +552,14 @@ static lp_rast_cmd_func dispatch[LP_RAST_OP_MAX] =
> >
> > static void
> > do_rasterize_bin(struct lp_rasterizer_task *task,
> > - const struct cmd_bin *bin)
> > + const struct cmd_bin *bin,
> > + int x, int y)
> > {
> > const struct cmd_block *block;
> > unsigned k;
> >
> > if (0)
> > - lp_debug_bin(bin);
> > + lp_debug_bin(bin, x, y);
> >
> > for (block = bin->head; block; block = block->next) {
> > for (k = 0; k < block->count; k++) {
> > @@ -576,11 +578,11 @@ do_rasterize_bin(struct lp_rasterizer_task *task,
> > */
> > static void
> > rasterize_bin(struct lp_rasterizer_task *task,
> > - const struct cmd_bin *bin )
> > + const struct cmd_bin *bin, int x, int y )
> > {
> > - lp_rast_tile_begin( task, bin );
> > + lp_rast_tile_begin( task, bin, x, y );
> >
> > - do_rasterize_bin(task, bin);
> > + do_rasterize_bin(task, bin, x, y);
> >
> > lp_rast_tile_end(task);
> >
> > @@ -622,27 +624,16 @@ rasterize_scene(struct lp_rasterizer_task *task,
> >
> > if (!task->rast->no_rast && !scene->discard) {
> > /* loop over scene bins, rasterize each */
> > -#if 0
> > - {
> > - unsigned i, j;
> > - for (i = 0; i < scene->tiles_x; i++) {
> > - for (j = 0; j < scene->tiles_y; j++) {
> > - struct cmd_bin *bin = lp_scene_get_bin(scene, i, j);
> > - rasterize_bin(task, bin, i, j);
> > - }
> > - }
> > - }
> > -#else
> > {
> > struct cmd_bin *bin;
> > + int i, j;
> >
> > assert(scene);
> > - while ((bin = lp_scene_bin_iter_next(scene))) {
> > + while ((bin = lp_scene_bin_iter_next(scene, &i, &j))) {
> > if (!is_empty_bin( bin ))
> > - rasterize_bin(task, bin);
> > + rasterize_bin(task, bin, i, j);
> > }
> > }
> > -#endif
> > }
> >
> >
> > diff --git a/src/gallium/drivers/llvmpipe/lp_rast_debug.c
> > b/src/gallium/drivers/llvmpipe/lp_rast_debug.c
> > index 4008251..3bc75aa 100644
> > --- a/src/gallium/drivers/llvmpipe/lp_rast_debug.c
> > +++ b/src/gallium/drivers/llvmpipe/lp_rast_debug.c
> > @@ -90,13 +90,13 @@ is_blend( const struct lp_rast_state *state,
> >
> >
> > static void
> > -debug_bin( const struct cmd_bin *bin )
> > +debug_bin( const struct cmd_bin *bin, int x, int y )
> > {
> > const struct lp_rast_state *state = NULL;
> > const struct cmd_block *head = bin->head;
> > int i, j = 0;
> >
> > - debug_printf("bin %d,%d:\n", bin->x, bin->y);
> > + debug_printf("bin %d,%d:\n", x, y);
> >
> > while (head) {
> > for (i = 0; i < head->count; i++, j++) {
> > @@ -231,13 +231,14 @@ debug_triangle(int tilex, int tiley,
> > static void
> > do_debug_bin( struct tile *tile,
> > const struct cmd_bin *bin,
> > + int x, int y,
> > boolean print_cmds)
> > {
> > unsigned k, j = 0;
> > const struct cmd_block *block;
> >
> > - int tx = bin->x * TILE_SIZE;
> > - int ty = bin->y * TILE_SIZE;
> > + int tx = x * TILE_SIZE;
> > + int ty = y * TILE_SIZE;
> >
> > memset(tile->data, ' ', sizeof tile->data);
> > tile->coverage = 0;
> > @@ -286,13 +287,13 @@ do_debug_bin( struct tile *tile,
> > }
> >
> > void
> > -lp_debug_bin( const struct cmd_bin *bin)
> > +lp_debug_bin( const struct cmd_bin *bin, int i, int j)
> > {
> > struct tile tile;
> > int x,y;
> >
> > if (bin->head) {
> > - do_debug_bin(&tile, bin, TRUE);
> > + do_debug_bin(&tile, bin, i, j, TRUE);
> >
> > debug_printf("------------------------------------------------------------------\n");
> > for (y = 0; y < TILE_SIZE; y++) {
> > @@ -349,9 +350,9 @@ lp_debug_draw_bins_by_coverage( struct lp_scene *scene
> > )
> > struct tile tile;
> >
> > if (bin->head) {
> > - //lp_debug_bin(bin);
> > + //lp_debug_bin(bin, x, y);
> >
> > - do_debug_bin(&tile, bin, FALSE);
> > + do_debug_bin(&tile, bin, x, y, FALSE);
> >
> > total += tile.coverage;
> > possible += 64*64;
> > @@ -419,7 +420,7 @@ lp_debug_bins( struct lp_scene *scene )
> > for (x = 0; x < scene->tiles_x; x++) {
> > struct cmd_bin *bin = lp_scene_get_bin(scene, x, y);
> > if (bin->head) {
> > - debug_bin(bin);
> > + debug_bin(bin, x, y);
> > }
> > }
> > }
> > diff --git a/src/gallium/drivers/llvmpipe/lp_rast_priv.h
> > b/src/gallium/drivers/llvmpipe/lp_rast_priv.h
> > index 7d01da1..854454a 100644
> > --- a/src/gallium/drivers/llvmpipe/lp_rast_priv.h
> > +++ b/src/gallium/drivers/llvmpipe/lp_rast_priv.h
> > @@ -339,6 +339,6 @@ lp_rast_set_state(struct lp_rasterizer_task *task,
> > const union lp_rast_cmd_arg arg);
> >
> > void
> > -lp_debug_bin( const struct cmd_bin *bin );
> > +lp_debug_bin( const struct cmd_bin *bin, int x, int y );
> >
> > #endif
> > diff --git a/src/gallium/drivers/llvmpipe/lp_scene.c
> > b/src/gallium/drivers/llvmpipe/lp_scene.c
> > index e05ea75..08dbe9b 100644
> > --- a/src/gallium/drivers/llvmpipe/lp_scene.c
> > +++ b/src/gallium/drivers/llvmpipe/lp_scene.c
> > @@ -470,7 +470,7 @@ lp_scene_bin_iter_begin( struct lp_scene *scene )
> > * of work (a bin) to work on.
> > */
> > struct cmd_bin *
> > -lp_scene_bin_iter_next( struct lp_scene *scene )
> > +lp_scene_bin_iter_next( struct lp_scene *scene , int *x, int *y)
> > {
> > struct cmd_bin *bin = NULL;
> >
> > @@ -487,6 +487,8 @@ lp_scene_bin_iter_next( struct lp_scene *scene )
> > }
> >
> > bin = lp_scene_get_bin(scene, scene->curr_x, scene->curr_y);
> > + *x = scene->curr_x;
> > + *y = scene->curr_y;
> >
> > end:
> > /*printf("return bin %p at %d, %d\n", (void *) bin, *bin_x, *bin_y);*/
> > diff --git a/src/gallium/drivers/llvmpipe/lp_scene.h
> > b/src/gallium/drivers/llvmpipe/lp_scene.h
> > index 1d0cd0e..fa5bbca 100644
> > --- a/src/gallium/drivers/llvmpipe/lp_scene.h
> > +++ b/src/gallium/drivers/llvmpipe/lp_scene.h
> > @@ -94,8 +94,6 @@ struct data_block {
> > * For each screen tile we have one of these bins.
> > */
> > struct cmd_bin {
> > - ushort x;
> > - ushort y;
> > const struct lp_rast_state *last_state; /* most recent state set
> > in bin */
> > struct cmd_block *head;
> > struct cmd_block *tail;
> > @@ -375,7 +373,7 @@ void
> > lp_scene_bin_iter_begin( struct lp_scene *scene );
> >
> > struct cmd_bin *
> > -lp_scene_bin_iter_next( struct lp_scene *scene );
> > +lp_scene_bin_iter_next( struct lp_scene *scene, int *x, int *y );
> >
> >
> >
> > diff --git a/src/gallium/drivers/llvmpipe/lp_setup.c
> > b/src/gallium/drivers/llvmpipe/lp_setup.c
> > index b874e6d..30e47f3 100644
> > --- a/src/gallium/drivers/llvmpipe/lp_setup.c
> > +++ b/src/gallium/drivers/llvmpipe/lp_setup.c
> > @@ -181,7 +181,7 @@ begin_binning( struct lp_setup_context *setup )
> > struct lp_scene *scene = setup->scene;
> > boolean need_zsload = FALSE;
> > boolean ok;
> > - unsigned i, j;
> > + unsigned i;
> >
> > assert(scene);
> > assert(scene->fence == NULL);
> > @@ -192,15 +192,6 @@ begin_binning( struct lp_setup_context *setup )
> > if (!scene->fence)
> > return FALSE;
> >
> > - /* Initialize the bin flags and x/y coords:
> > - */
> > - for (i = 0; i < scene->tiles_x; i++) {
> > - for (j = 0; j < scene->tiles_y; j++) {
> > - scene->tile[i][j].x = i;
> > - scene->tile[i][j].y = j;
> > - }
> > - }
> > -
> > ok = try_update_scene_state(setup);
> > if (!ok)
> > return FALSE;
> >
>
> I'm surprised this makes such a noticeable difference in instruction count.
> I guess this is ok x/y aren't really needed in there, I haven't dealt
> with binning too much though. And I'm going to introduce width/height
> parameters in there instead very soon to counter this optimization :-).
> (Actually because things like clearing bins right now always have to
> clear the whole bin, which imposes a 64x64 alignment on all resources
> which is rather hilarious for instance for 1d array textures plus I'm
> not convinced we deal with this correctly if it's a display target and
> it simply won't work for rendering to buffers.)
> width/height though can at least be ubyte at least with current bin size
> :-)
Instead of passing x, y, w, h by value on all these functions, could we pass them in a struct?
Jose
More information about the mesa-dev
mailing list