[Mesa-dev] [PATCH] st/mesa: allow glDrawElements to work with GL_SELECT feedback

[Ilia Mirkin]

On Wed, Dec 19, 2018 at 10:12 AM Brian Paul <brianp at vmware.com> wrote:
>
> On 12/19/2018 06:47 AM, Ilia Mirkin wrote:
> > On Wed, Dec 19, 2018 at 8:38 AM Brian Paul <brianp at 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 <imirkin at 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 <brianp at 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