[Mesa-dev] [PATCH 3/3] softpipe: don't use samplers with prebaked sampler and sampler_view state
Jose Fonseca
jfonseca at vmware.com
Fri Mar 8 13:45:38 PST 2013
----- Original Message -----
> Am 08.03.2013 21:03, schrieb Jose Fonseca:
> > ----- Original Message -----
> >> From: Roland Scheidegger <sroland at vmware.com>
> >>
> >> This is needed for handling the dx10-style sample opcodes.
> >> This also simplifies the logic by getting rid of sampler variants
> >> completely (sampler_views though OTOH have sort of variants because
> >> some of their state is different depending on the shader stage they
> >> are bound to).
> >> Two piglit regressions I haven't figured out, streaming-texture-leak
> >> (pass -> fail though it's actually crash due to oom kill, looks like
> >> leaking textures somewhere), and fbo-srgb-blit (looks like the srgb->rgb
> >> or vice versa state somewhere gets lost).
> >> No significant performance difference (openarena run:
> >> 840 frames in 459.8 seconds vs. 840 frames in 463.9 seconds).
> >> ---
> >> src/gallium/drivers/softpipe/sp_context.h | 8 +-
> >> src/gallium/drivers/softpipe/sp_state.h | 8 +
> >> src/gallium/drivers/softpipe/sp_state_derived.c | 33 +-
> >> src/gallium/drivers/softpipe/sp_state_sampler.c | 184 +---
> >> src/gallium/drivers/softpipe/sp_tex_sample.c | 1274
> >> ++++++++++++-----------
> >> src/gallium/drivers/softpipe/sp_tex_sample.h | 138 +--
> >> 6 files changed, 779 insertions(+), 866 deletions(-)
> >>
> >> diff --git a/src/gallium/drivers/softpipe/sp_context.h
> >> b/src/gallium/drivers/softpipe/sp_context.h
> >> index dcd29be..ed38bac 100644
> >> --- a/src/gallium/drivers/softpipe/sp_context.h
> >> +++ b/src/gallium/drivers/softpipe/sp_context.h
> >> @@ -80,7 +80,7 @@ struct softpipe_context {
> >> struct pipe_framebuffer_state framebuffer;
> >> struct pipe_poly_stipple poly_stipple;
> >> struct pipe_scissor_state scissor;
> >> - struct pipe_sampler_view
> >> *sampler_views[PIPE_SHADER_TYPES][PIPE_MAX_SAMPLERS];
> >> + struct pipe_sampler_view
> >> *sampler_views[PIPE_SHADER_TYPES][PIPE_MAX_SHADER_SAMPLER_VIEWS];
> >>
> >> struct pipe_viewport_state viewport;
> >> struct pipe_vertex_buffer vertex_buffer[PIPE_MAX_ATTRIBS];
> >> @@ -184,8 +184,10 @@ struct softpipe_context {
> >> * Texture caches for vertex, fragment, geometry stages.
> >> * Don't use PIPE_SHADER_TYPES here to avoid allocating unused memory
> >> * for compute shaders.
> >> + * XXX wouldn't it make more sense for the tile cache to just be part
> >> + * of sp_sampler_view?
> >> */
> >> - struct softpipe_tex_tile_cache
> >> *tex_cache[PIPE_SHADER_GEOMETRY+1][PIPE_MAX_SAMPLERS];
> >> + struct softpipe_tex_tile_cache
> >> *tex_cache[PIPE_SHADER_GEOMETRY+1][PIPE_MAX_SHADER_SAMPLER_VIEWS];
> >>
> >> unsigned dump_fs : 1;
> >> unsigned dump_gs : 1;
> >> @@ -199,8 +201,6 @@ softpipe_context( struct pipe_context *pipe )
> >> return (struct softpipe_context *)pipe;
> >> }
> >>
> >> -void
> >> -softpipe_reset_sampler_variants(struct softpipe_context *softpipe);
> >>
> >> struct pipe_context *
> >> softpipe_create_context( struct pipe_screen *, void *priv );
> >> diff --git a/src/gallium/drivers/softpipe/sp_state.h
> >> b/src/gallium/drivers/softpipe/sp_state.h
> >> index e2c49d2..bf8370e 100644
> >> --- a/src/gallium/drivers/softpipe/sp_state.h
> >> +++ b/src/gallium/drivers/softpipe/sp_state.h
> >> @@ -156,6 +156,14 @@ void
> >> softpipe_update_derived(struct softpipe_context *softpipe, unsigned
> >> prim);
> >>
> >> void
> >> +softpipe_set_sampler_views(struct pipe_context *pipe,
> >> + unsigned shader,
> >> + unsigned start,
> >> + unsigned num,
> >> + struct pipe_sampler_view **views);
> >> +
> >> +
> >> +void
> >> softpipe_draw_vbo(struct pipe_context *pipe,
> >> const struct pipe_draw_info *info);
> >>
> >> diff --git a/src/gallium/drivers/softpipe/sp_state_derived.c
> >> b/src/gallium/drivers/softpipe/sp_state_derived.c
> >> index 8fbe27b..85fd47d 100644
> >> --- a/src/gallium/drivers/softpipe/sp_state_derived.c
> >> +++ b/src/gallium/drivers/softpipe/sp_state_derived.c
> >> @@ -36,6 +36,7 @@
> >> #include "sp_screen.h"
> >> #include "sp_state.h"
> >> #include "sp_texture.h"
> >> +#include "sp_tex_sample.h"
> >> #include "sp_tex_tile_cache.h"
> >>
> >>
> >> @@ -205,12 +206,32 @@ compute_cliprect(struct softpipe_context *sp)
> >>
> >>
> >> static void
> >> +set_shader_sampler(struct softpipe_context *softpipe,
> >> + unsigned shader,
> >> + int max_sampler)
> >> +{
> >> + int i;
> >> + for (i = 0; i <= max_sampler; i++) {
> >> + softpipe->tgsi.sampler[shader]->sp_sampler[i] =
> >> + (struct sp_sampler *)(softpipe->samplers[shader][i]);
> >> + }
> >> +}
> >> +
> >> +static void
> >> update_tgsi_samplers( struct softpipe_context *softpipe )
> >> {
> >> unsigned i, sh;
> >>
> >> - softpipe_reset_sampler_variants( softpipe );
> >> + set_shader_sampler(softpipe, PIPE_SHADER_VERTEX,
> >> + softpipe->vs->max_sampler);
> >> + set_shader_sampler(softpipe, PIPE_SHADER_FRAGMENT,
> >> +
> >> softpipe->fs_variant->info.file_max[TGSI_FILE_SAMPLER]);
> >> + if (softpipe->gs) {
> >> + set_shader_sampler(softpipe, PIPE_SHADER_GEOMETRY,
> >> + softpipe->gs->max_sampler);
> >> + }
> >>
> >> + /* XXX is this really necessary here??? */
> >> for (sh = 0; sh < Elements(softpipe->tex_cache); sh++) {
> >> for (i = 0; i < PIPE_MAX_SAMPLERS; i++) {
> >> struct softpipe_tex_tile_cache *tc = softpipe->tex_cache[sh][i];
> >> @@ -314,12 +335,9 @@ update_polygon_stipple_enable(struct softpipe_context
> >> *softpipe, unsigned prim)
> >> /* sampler state */
> >> softpipe->samplers[PIPE_SHADER_FRAGMENT][unit] =
> >> softpipe->pstipple.sampler;
> >>
> >> - /* sampler view */
> >> -
> >> pipe_sampler_view_reference(&softpipe->sampler_views[PIPE_SHADER_FRAGMENT][unit],
> >> - softpipe->pstipple.sampler_view);
> >
> > Why is this pipe_sampler_view_reference() call being removed?
> Because the new softpipe_set_sampler_views call below will do this now
> (I didn't change this here first actually, but this resulted in the
> sampler view changes not being picked up and hence two more piglit
> regressions where tests using polygon stipple crashed).
I hoped this was not interference because of the leak below.
> >
> >> -
> >> -
> >> sp_tex_tile_cache_set_sampler_view(softpipe->tex_cache[PIPE_SHADER_FRAGMENT][unit],
> >> -
> >> softpipe->pstipple.sampler_view);
> >> + /* sampler view state */
> >> + softpipe_set_sampler_views(&softpipe->pipe, PIPE_SHADER_FRAGMENT,
> >> + unit, 1,
> >> &softpipe->pstipple.sampler_view);
> >>
> >> softpipe->dirty |= SP_NEW_SAMPLER;
> >> }
> >> @@ -358,6 +376,7 @@ softpipe_update_derived(struct softpipe_context
> >> *softpipe, unsigned prim)
> >> update_polygon_stipple_enable(softpipe, prim);
> >> #endif
> >>
> >> + /* TODO: this looks suboptimal */
> >> if (softpipe->dirty & (SP_NEW_SAMPLER |
> >> SP_NEW_TEXTURE |
> >> SP_NEW_FS |
> >> diff --git a/src/gallium/drivers/softpipe/sp_state_sampler.c
> >> b/src/gallium/drivers/softpipe/sp_state_sampler.c
> >> index eb02ecb..f031ad2 100644
> >> --- a/src/gallium/drivers/softpipe/sp_state_sampler.c
> >> +++ b/src/gallium/drivers/softpipe/sp_state_sampler.c
> >> @@ -41,31 +41,6 @@
> >> #include "sp_tex_tile_cache.h"
> >>
> >>
> >> -struct sp_sampler {
> >> - struct pipe_sampler_state base;
> >> - struct sp_sampler_variant *variants;
> >> - struct sp_sampler_variant *current;
> >> -};
> >> -
> >> -static struct sp_sampler *sp_sampler( struct pipe_sampler_state *sampler
> >> )
> >> -{
> >> - return (struct sp_sampler *)sampler;
> >> -}
> >> -
> >> -
> >> -static void *
> >> -softpipe_create_sampler_state(struct pipe_context *pipe,
> >> - const struct pipe_sampler_state *sampler)
> >> -{
> >> - struct sp_sampler *sp_sampler = CALLOC_STRUCT(sp_sampler);
> >> -
> >> - sp_sampler->base = *sampler;
> >> - sp_sampler->variants = NULL;
> >> -
> >> - return (void *)sp_sampler;
> >> -}
> >> -
> >> -
> >> /**
> >> * Bind a range [start, start+num-1] of samplers for a shader stage.
> >> */
> >> @@ -142,24 +117,6 @@ softpipe_bind_geometry_sampler_states(struct
> >> pipe_context *pipe,
> >> }
> >>
> >>
> >> -static struct pipe_sampler_view *
> >> -softpipe_create_sampler_view(struct pipe_context *pipe,
> >> - struct pipe_resource *resource,
> >> - const struct pipe_sampler_view *templ)
> >> -{
> >> - struct pipe_sampler_view *view = CALLOC_STRUCT(pipe_sampler_view);
> >> -
> >> - if (view) {
> >> - *view = *templ;
> >> - view->reference.count = 1;
> >> - view->texture = NULL;
> >> - pipe_resource_reference(&view->texture, resource);
> >> - view->context = pipe;
> >> - }
> >> -
> >> - return view;
> >> -}
> >> -
> >>
> >> static void
> >> softpipe_sampler_view_destroy(struct pipe_context *pipe,
> >> @@ -170,7 +127,7 @@ softpipe_sampler_view_destroy(struct pipe_context
> >> *pipe,
> >> }
> >>
> >>
> >> -static void
> >> +void
> >> softpipe_set_sampler_views(struct pipe_context *pipe,
> >> unsigned shader,
> >> unsigned start,
> >> @@ -194,12 +151,29 @@ softpipe_set_sampler_views(struct pipe_context
> >> *pipe,
> >>
> >> /* set the new sampler views */
> >> for (i = 0; i < num; i++) {
> >> - pipe_sampler_view_reference(&softpipe->sampler_views[shader][start
> >> +
> >> i],
> >> - views[i]);
> >> + struct pipe_sampler_view *view =
> >> softpipe->sampler_views[shader][start
> >> + i];
> >> + struct sp_sampler_view *sp_sviewsrc;
> >> + struct sp_sampler_view *sp_sviewdst =
> >> + &softpipe->tgsi.sampler[shader]->sp_sview[start + i];
> >> + pipe_sampler_view_reference(&view, views[i]);
> >
> > Shouldn't it be:
> >
> > struct pipe_sampler_view **pview =
> > &softpipe->sampler_views[shader][start + i];
> > pipe_sampler_view_reference(pview, views[i]);
> Oh I think you're right. That could explain the textures leaking...
> Seems to fix it indeed. And as a bonus, also fix the other bug (the
> fbo-srgb-blit one). I'll rerun all tests just to make sure. Thanks!
Great. It looks good to me otherwise.
Jose
More information about the mesa-dev
mailing list