Re: [Mesa-dev] [PATCH 06/24] st/mesa: don't unbind sampler states if none are used
On 15/06/17 03:17, Marek Olšák wrote: On Tue, Jun 13, 2017 at 7:46 AM, Timothy Arceri wrote: On 13/06/17 15:32, Timothy Arceri wrote: On 13/06/17 04:23, Ilia Mirkin wrote: On Mon, Jun 12, 2017 at 2:18 PM, Marek Olšák wrote: From: Marek Olšák --- src/mesa/state_tracker/st_atom_sampler.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mesa/state_tracker/st_atom_sampler.c b/src/mesa/state_tracker/st_atom_sampler.c index f33e334..11db6e1 100644 --- a/src/mesa/state_tracker/st_atom_sampler.c +++ b/src/mesa/state_tracker/st_atom_sampler.c @@ -263,21 +263,21 @@ update_shader_samplers(struct st_context *st, struct pipe_sampler_state *samplers, unsigned *num_samplers) { GLbitfield samplers_used = prog->SamplersUsed; GLbitfield free_slots = ~prog->SamplersUsed; GLbitfield external_samplers_used = prog->ExternalSamplersUsed; GLuint unit; const GLuint old_max = *num_samplers; const struct pipe_sampler_state *states[PIPE_MAX_SAMPLERS]; - if (*num_samplers == 0 && samplers_used == 0x0) + if (samplers_used == 0x0) return; *num_samplers = 0; Does this still need to get executed even if samplers_used == 0? It seems correct to skip this, otherwise old_max won't be set correctly in the above code the next time we get here. Although it seems we ignore old_max in the following patches anyway because cso_set_samplers() will set things to NULL for us. So maybe it would make sense to set it to 0? Yes, it should be set to 0, but it's not that important (only DrawPixels would be affected), though I think we can just drop tracking num_samplers in st_context and simply rely on num_sampler_views. I'll fix that in a follow-up patch, which I'm gonna send shortly ([25/24]]. In the meantime, I'd like an Rb on this one if there are no other comments. You still have my r-b here. 25 is also: Reviewed-by: Timothy Arceri Thanks, Marek Reviewed-by: Timothy Arceri /* loop over sampler units (aka tex image units) */ for (unit = 0; unit < max_units; unit++, samplers_used >>= 1) { struct pipe_sampler_state *sampler = samplers + unit; if (samplers_used & 1) { const GLuint texUnit = prog->SamplerUnits[unit]; -- 2.7.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 06/24] st/mesa: don't unbind sampler states if none are used
On Tue, Jun 13, 2017 at 7:46 AM, Timothy Arceri wrote: > On 13/06/17 15:32, Timothy Arceri wrote: >> >> >> >> On 13/06/17 04:23, Ilia Mirkin wrote: >>> >>> On Mon, Jun 12, 2017 at 2:18 PM, Marek Olšák wrote: From: Marek Olšák --- src/mesa/state_tracker/st_atom_sampler.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mesa/state_tracker/st_atom_sampler.c b/src/mesa/state_tracker/st_atom_sampler.c index f33e334..11db6e1 100644 --- a/src/mesa/state_tracker/st_atom_sampler.c +++ b/src/mesa/state_tracker/st_atom_sampler.c @@ -263,21 +263,21 @@ update_shader_samplers(struct st_context *st, struct pipe_sampler_state *samplers, unsigned *num_samplers) { GLbitfield samplers_used = prog->SamplersUsed; GLbitfield free_slots = ~prog->SamplersUsed; GLbitfield external_samplers_used = prog->ExternalSamplersUsed; GLuint unit; const GLuint old_max = *num_samplers; const struct pipe_sampler_state *states[PIPE_MAX_SAMPLERS]; - if (*num_samplers == 0 && samplers_used == 0x0) + if (samplers_used == 0x0) return; *num_samplers = 0; >>> >>> >>> Does this still need to get executed even if samplers_used == 0? >> >> >> It seems correct to skip this, otherwise old_max won't be set correctly in >> the above code the next time we get here. > > > > Although it seems we ignore old_max in the following patches anyway because > cso_set_samplers() will set things to NULL for us. > > So maybe it would make sense to set it to 0? Yes, it should be set to 0, but it's not that important (only DrawPixels would be affected), though I think we can just drop tracking num_samplers in st_context and simply rely on num_sampler_views. I'll fix that in a follow-up patch, which I'm gonna send shortly ([25/24]]. In the meantime, I'd like an Rb on this one if there are no other comments. Thanks, Marek > > > > >> >> Reviewed-by: Timothy Arceri >> >>> /* loop over sampler units (aka tex image units) */ for (unit = 0; unit < max_units; unit++, samplers_used >>= 1) { struct pipe_sampler_state *sampler = samplers + unit; if (samplers_used & 1) { const GLuint texUnit = prog->SamplerUnits[unit]; -- 2.7.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev >>> >>> ___ >>> mesa-dev mailing list >>> mesa-dev@lists.freedesktop.org >>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev >>> >> ___ >> mesa-dev mailing list >> mesa-dev@lists.freedesktop.org >> https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 06/24] st/mesa: don't unbind sampler states if none are used
On 13/06/17 15:32, Timothy Arceri wrote: On 13/06/17 04:23, Ilia Mirkin wrote: On Mon, Jun 12, 2017 at 2:18 PM, Marek Olšák wrote: From: Marek Olšák --- src/mesa/state_tracker/st_atom_sampler.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mesa/state_tracker/st_atom_sampler.c b/src/mesa/state_tracker/st_atom_sampler.c index f33e334..11db6e1 100644 --- a/src/mesa/state_tracker/st_atom_sampler.c +++ b/src/mesa/state_tracker/st_atom_sampler.c @@ -263,21 +263,21 @@ update_shader_samplers(struct st_context *st, struct pipe_sampler_state *samplers, unsigned *num_samplers) { GLbitfield samplers_used = prog->SamplersUsed; GLbitfield free_slots = ~prog->SamplersUsed; GLbitfield external_samplers_used = prog->ExternalSamplersUsed; GLuint unit; const GLuint old_max = *num_samplers; const struct pipe_sampler_state *states[PIPE_MAX_SAMPLERS]; - if (*num_samplers == 0 && samplers_used == 0x0) + if (samplers_used == 0x0) return; *num_samplers = 0; Does this still need to get executed even if samplers_used == 0? It seems correct to skip this, otherwise old_max won't be set correctly in the above code the next time we get here. Although it seems we ignore old_max in the following patches anyway because cso_set_samplers() will set things to NULL for us. So maybe it would make sense to set it to 0? Reviewed-by: Timothy Arceri /* loop over sampler units (aka tex image units) */ for (unit = 0; unit < max_units; unit++, samplers_used >>= 1) { struct pipe_sampler_state *sampler = samplers + unit; if (samplers_used & 1) { const GLuint texUnit = prog->SamplerUnits[unit]; -- 2.7.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 06/24] st/mesa: don't unbind sampler states if none are used
On 13/06/17 04:23, Ilia Mirkin wrote: On Mon, Jun 12, 2017 at 2:18 PM, Marek Olšák wrote: From: Marek Olšák --- src/mesa/state_tracker/st_atom_sampler.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mesa/state_tracker/st_atom_sampler.c b/src/mesa/state_tracker/st_atom_sampler.c index f33e334..11db6e1 100644 --- a/src/mesa/state_tracker/st_atom_sampler.c +++ b/src/mesa/state_tracker/st_atom_sampler.c @@ -263,21 +263,21 @@ update_shader_samplers(struct st_context *st, struct pipe_sampler_state *samplers, unsigned *num_samplers) { GLbitfield samplers_used = prog->SamplersUsed; GLbitfield free_slots = ~prog->SamplersUsed; GLbitfield external_samplers_used = prog->ExternalSamplersUsed; GLuint unit; const GLuint old_max = *num_samplers; const struct pipe_sampler_state *states[PIPE_MAX_SAMPLERS]; - if (*num_samplers == 0 && samplers_used == 0x0) + if (samplers_used == 0x0) return; *num_samplers = 0; Does this still need to get executed even if samplers_used == 0? It seems correct to skip this, otherwise old_max won't be set correctly in the above code the next time we get here. Reviewed-by: Timothy Arceri /* loop over sampler units (aka tex image units) */ for (unit = 0; unit < max_units; unit++, samplers_used >>= 1) { struct pipe_sampler_state *sampler = samplers + unit; if (samplers_used & 1) { const GLuint texUnit = prog->SamplerUnits[unit]; -- 2.7.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
Re: [Mesa-dev] [PATCH 06/24] st/mesa: don't unbind sampler states if none are used
On Mon, Jun 12, 2017 at 2:18 PM, Marek Olšák wrote: > From: Marek Olšák > > --- > src/mesa/state_tracker/st_atom_sampler.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/src/mesa/state_tracker/st_atom_sampler.c > b/src/mesa/state_tracker/st_atom_sampler.c > index f33e334..11db6e1 100644 > --- a/src/mesa/state_tracker/st_atom_sampler.c > +++ b/src/mesa/state_tracker/st_atom_sampler.c > @@ -263,21 +263,21 @@ update_shader_samplers(struct st_context *st, > struct pipe_sampler_state *samplers, > unsigned *num_samplers) > { > GLbitfield samplers_used = prog->SamplersUsed; > GLbitfield free_slots = ~prog->SamplersUsed; > GLbitfield external_samplers_used = prog->ExternalSamplersUsed; > GLuint unit; > const GLuint old_max = *num_samplers; > const struct pipe_sampler_state *states[PIPE_MAX_SAMPLERS]; > > - if (*num_samplers == 0 && samplers_used == 0x0) > + if (samplers_used == 0x0) >return; > > *num_samplers = 0; Does this still need to get executed even if samplers_used == 0? > > /* loop over sampler units (aka tex image units) */ > for (unit = 0; unit < max_units; unit++, samplers_used >>= 1) { >struct pipe_sampler_state *sampler = samplers + unit; > >if (samplers_used & 1) { > const GLuint texUnit = prog->SamplerUnits[unit]; > -- > 2.7.4 > > ___ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/mesa-dev ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
[Mesa-dev] [PATCH 06/24] st/mesa: don't unbind sampler states if none are used
From: Marek Olšák --- src/mesa/state_tracker/st_atom_sampler.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mesa/state_tracker/st_atom_sampler.c b/src/mesa/state_tracker/st_atom_sampler.c index f33e334..11db6e1 100644 --- a/src/mesa/state_tracker/st_atom_sampler.c +++ b/src/mesa/state_tracker/st_atom_sampler.c @@ -263,21 +263,21 @@ update_shader_samplers(struct st_context *st, struct pipe_sampler_state *samplers, unsigned *num_samplers) { GLbitfield samplers_used = prog->SamplersUsed; GLbitfield free_slots = ~prog->SamplersUsed; GLbitfield external_samplers_used = prog->ExternalSamplersUsed; GLuint unit; const GLuint old_max = *num_samplers; const struct pipe_sampler_state *states[PIPE_MAX_SAMPLERS]; - if (*num_samplers == 0 && samplers_used == 0x0) + if (samplers_used == 0x0) return; *num_samplers = 0; /* loop over sampler units (aka tex image units) */ for (unit = 0; unit < max_units; unit++, samplers_used >>= 1) { struct pipe_sampler_state *sampler = samplers + unit; if (samplers_used & 1) { const GLuint texUnit = prog->SamplerUnits[unit]; -- 2.7.4 ___ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev