[Mesa-dev] [PATCH 4/4] virgl: work around bad assumptions in virglrenderer

[Erik Faye-Lund]

On Wed, 2018-12-12 at 06:18 +0100, Mathias Fröhlich wrote:
> Erik,
> 
> On Tuesday, 11 December 2018 15:29:49 CET Erik Faye-Lund wrote:
> > On Tue, 2018-12-11 at 15:26 +0100, Erik Faye-Lund wrote:
> > > Virglrenderer does the wrong thing when given an instance
> > > divisor;
> > > it tries to use the element-index rather than the binding-index
> > > as
> > > the argument to glVertexBindingDivisor(). This worked fine as
> > > long
> > > as there was a 1:1 relationship between elements and bindings,
> > > which was the case util 19a91841c34 "st/mesa: Use Array._DrawVAO
> > > in
> > > st_atom_array.c.".
> > > 
> > > So let's detect instance divisors, and restore a 1:1 relationship
> > > in
> > > that case. This will make old versions of virglrenderer behave
> > > correctly. For newer versions, we can consider making a better
> > > interface, where the instance divisor isn't specified per
> > > element,
> > > but rather per binding. But let's save that for another day.
> > > 
> > > Signed-off-by: Erik Faye-Lund <erik.faye-lund at collabora.com>
> 
> I don't exactly know what kind of coding standards and that you use
> in virgl.
> But the patch series looks good and should help for the issues we
> observed.
> 
> One thing, may be. Do you want to add some documentation beside the
> git log message why we do something surprising like replicating out
> the
> buffers and assigning new buffer indices? Just something that allows
> a reader to get an idea why non straight forward things happen here.
> The git annotate references to the commit messages tend to vanish
> over time behind further changes in the code.
> 

Yeah, that's a good point. This deserves a comment to clear things up a
bit.

How about something like this squashed in?

---8<---
diff --git a/src/gallium/drivers/virgl/virgl_context.c
b/src/gallium/drivers/virgl/virgl_context.c
index ce72f73a0f6..2e3202b04e9 100644
--- a/src/gallium/drivers/virgl/virgl_context.c
+++ b/src/gallium/drivers/virgl/virgl_context.c
@@ -399,6 +399,10 @@ static void
*virgl_create_vertex_elements_state(struct pipe_context *ctx,
 
    for (int i = 0; i < num_elements; ++i) {
       if (elements[i].instance_divisor) {
+	 /* Virglrenderer doesn't deal with instance_divisor correctly
if
+	  * there isn't a 1:1 relationship between elements and
bindings.
+	  * So let's make sure there is, by duplicating bindings.
+	  */
 	 for (int j = 0; j < num_elements; ++j) {
             new_elements[j] = elements[j];
             new_elements[j].vertex_buffer_index = j;
---8<---
> > I suppose this should have had:
> > 
> > Fixes: 19a91841c34 "st/mesa: Use Array._DrawVAO in
> > st_atom_array.c."
> Probably at least for patch #3 and #4.
> 
> With or without such comment and for the series:
> Reviewed-by: Mathias Fröhlich <Mathias.Froehlich at web.de>
> 
> best
> 
> Mathias
> 
> 
> _______________________________________________
> mesa-dev mailing list
> mesa-dev at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/mesa-dev