On Wed, Dec 19, 2018 at 10:12 AM Brian Paul <bri...@vmware.com> wrote: > > On 12/19/2018 06:47 AM, Ilia Mirkin wrote: > > On Wed, Dec 19, 2018 at 8:38 AM Brian Paul <bri...@vmware.com> wrote: > >> > >> On 12/18/2018 08:50 PM, Ilia Mirkin wrote: > >>> Not sure if this ever worked, but the current logic for setting the > >>> min/max index is definitely wrong for indexed draws. While we're at it, > >>> bring in all the usual logic from the non-indirect drawing path. > >>> > >>> Bugzilla: > >>> https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs.freedesktop.org%2Fshow_bug.cgi%3Fid%3D109086&data=02%7C01%7Cbrianp%40vmware.com%7C9b8ab75c977c458b597708d665b899b4%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C636808240937735266&sdata=pHzFcdG8H%2F89D3rdD2gs7CZ%2BE12BRgGT0uzsuxWDC68%3D&reserved=0 > >>> Signed-off-by: Ilia Mirkin <imir...@alum.mit.edu> > >>> --- > >>> > >>> This makes it more or less mirror st_draw_vbo. As the comment in the > >>> code says, would be nice to refactor, but ... meh. > >>> > >>> Note that I haven't tested any of the interactions with additional > >>> features, like primitive restart or instancing or any of that. However I > >>> don't think that this would make things *worse*. > >>> > >>> src/mesa/state_tracker/st_draw_feedback.c | 61 +++++++++++++++-------- > >>> 1 file changed, 39 insertions(+), 22 deletions(-) > >>> > >>> diff --git a/src/mesa/state_tracker/st_draw_feedback.c > >>> b/src/mesa/state_tracker/st_draw_feedback.c > >>> index 6ec6d5c16f4..49fdecf7e38 100644 > >>> --- a/src/mesa/state_tracker/st_draw_feedback.c > >>> +++ b/src/mesa/state_tracker/st_draw_feedback.c > >>> @@ -84,27 +84,6 @@ set_feedback_vertex_format(struct gl_context *ctx) > >>> } > >>> > >>> > >>> -/** > >>> - * Helper for drawing current vertex arrays. > >>> - */ > >>> -static void > >>> -draw_arrays(struct draw_context *draw, unsigned mode, > >>> - unsigned start, unsigned count) > >>> -{ > >>> - struct pipe_draw_info info; > >>> - > >>> - util_draw_init_info(&info); > >>> - > >>> - info.mode = mode; > >>> - info.start = start; > >>> - info.count = count; > >>> - info.min_index = start; > >>> - info.max_index = start + count - 1; > >>> - > >>> - draw_vbo(draw, &info); > >>> -} > >>> - > >>> - > >>> /** > >>> * Called by VBO to draw arrays when in selection or feedback mode and > >>> * to implement glRasterPos. > >>> @@ -136,10 +115,18 @@ st_feedback_draw_vbo(struct gl_context *ctx, > >>> struct pipe_transfer *ib_transfer = NULL; > >>> GLuint i; > >>> const void *mapped_indices = NULL; > >>> + struct pipe_draw_info info; > >>> > >>> if (!draw) > >>> return; > >>> > >>> + /* Initialize pipe_draw_info. */ > >>> + info.primitive_restart = false; > >>> + info.vertices_per_patch = ctx->TessCtrlProgram.patch_vertices; > >>> + info.indirect = NULL; > >>> + info.count_from_stream_output = NULL; > >>> + info.restart_index = 0; > >> > >> Shouldn't we use util_draw_init_info() to make this future-proof? > >> > >> Looks good otherwise. Thanks for fixing this! > >> > >> Reviewed-by: Brian Paul <bri...@vmware.com> > > > > I'm just copying what st_draw_vbo does. I think keeping them looking > > similar is important so that any updates to one are easily ported. Why > > doesn't that use the util_draw_init_info? Not sure. > > I think Marek did that to squeeze out a few drops of performance. I > don't think we have to do that in the feedback code. But your call.
While the performance here is certainly not necessary, I opted to keep it as-is for the benefit of those functions not having unnecessary differences. valgrind didn't show any issues in a handful of tests (hardly exhaustive). Thanks for reviewing! Pushed now. -ilia _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
